perf(user): let authorization accessors honour eager-loaded relations (12 → 0 queries/row) - #251
Open
dounisaur wants to merge 1 commit into
Conversation
`User::$role`, `$roles`, `$policies` and `$permissions` each execute a fresh query on every read, because they call the relation as a query builder (`$this->companyUser->roles()->first()`) rather than reading the loaded relation. Eager-loading `companyUser.roles` therefore does nothing: the data is loaded and the accessor queries anyway. `Http\Resources\User::toArray()` compounds it by evaluating `$this->role` four times per row (twice for `role`, twice for `role_name`). Measured on a real page with `DB::listen`, 5 users: today 12 queries/row resource reads `role` once 8 queries/row + eager-load `companyUser` 7 queries/row + deep eager-load (roles/policies/permissions) 7 queries/row (no change) accessors honour the loaded relation + eager-load 0 queries/row Each accessor now prefers the loaded relation and falls back to the query when it is absent, so behaviour is unchanged for callers that do not eager-load. The `instanceof Model` guard keeps duck-typed pivots working -- the suite's own UserModelAuthorizationPivotFake is one. No response shape changes, no memoisation, and nothing new appears in the model's array output. Adds a test whose pivot throws from roles()/policies()/permissions(), so the suite fails loudly if an accessor ever queries past a loaded relation again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
User::$role,$roles,$policiesand$permissionseach execute a fresh query on every read, because they call the relation as a query builder rather than reading the loaded relation:The consequence is that eager-loading does nothing.
with('companyUser.roles')loads the data and the accessor queries anyway.Http\Resources\User::toArray()compounds it by evaluating$this->rolefour times per row — twice forrole, twice forrole_name— each one paying the full accessor cost.Measurements
Taken with
DB::listenover a real page of users (5 rows), on a deployed 1.6.55 install. The four methods are byte-identical onmainatb7691c0, so these apply to HEAD.roleoncecompanyUsercompanyUser.roles/policies/permissions)The fourth row is the interesting one: deep eager-loading currently buys nothing, which is what identifies the accessors rather than the query as the cause.
For scale, on the install this was found on the IAM Users list took ~12 s for 18 rows and grew linearly with row count.
The change
Models/User.php— the four authorization accessors prefer the eager-loaded relation and fall back to the query when it is absent. Behaviour is unchanged for callers that do not eager-load.Http/Controllers/Internal/v1/UserController.php—onQueryRecord()eager-loadscompanyUser.roles,companyUser.policies,companyUser.permissions. Placed before the early return so it applies on both branches.Http/Resources/User.php— reads theroleaccessor once into a local.What it deliberately does not do
setRelationon the accessors. Caching into a relation slot would makerole/policies/permissionsappear in the model's array output, which would be a silent serialization change. This approach has no such side effect.instanceof Modelguard.companyUseris not always an Eloquent model — this repo's ownUserModelAuthorizationPivotFakeis duck-typed — so the loaded-relation path is guarded and those callers keep the query path. Without the guard,Tests\Unit\Models\UserModelTestfails.Test
Adds
it reads eager-loaded authorization relations without re-querying them. Its pivot throws fromroles()/policies()/permissions(), so the suite fails loudly if an accessor ever queries past a loaded relation again.Verified the test has teeth: reverting the accessor change makes it fail.
Deprecations and warnings are unchanged from the baseline run.
Noted, not addressed here
Traits/ProxiesAuthorizationMethods— its__callproxy forwards any role/policy/permission-named method to the pivot and queries per call, sogetRoleName()and friends still pay per-row. The new test found this; it is a separate path and out of scope for this change.Http/Resources/RoleandHttp/Resources/Policyalways serialize their fullpermissionsarray. On the same install this made the IAM Roles and Policies lists ~357 KB and ~332 KB per page. A list-contextpermissions_countwould fix it, but that is a breaking response-shape change, so I have left it out rather than bundle it here. Happy to raise it separately if you would take it.Setting::lookup('user.<uuid>.locale')and thecompanyUser()fallback whenusers.company_uuidis null are both further per-row costs, also left out to keep this reviewable.