fix(query): decide the alias self edge by comparison - #3386
Open
jzelinskie wants to merge 2 commits into
Open
jzelinskie wants to merge 2 commits into
jzelinskie wants to merge 2 commits into
Conversation
github-actions
Bot
added
area/tooling
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The reflexive identity subject (group:a#member is one of the subjects of
group:a#member) was gated on an existence probe: a LIMIT 1 query per object
per alias level, filtered on subject type, ID and relation with no resource
constraint at all. On CheckWideGroups' LookupSubjects that was 163 of 175
queries, all distinct so no cache would help, all returning true, and every
resulting self edge then discarded by the target type filter.
The probe was also wrong. Whether identity holds depends on what the object
is, not on whether some unrelated relationship happens to point at it:
group:eng#member@user:alice only
planner LS(group:eng#member, target group#member) = []
classic LS(group:eng#member, target group#member) = [group:eng#member]
adding group:parent#member@group:eng#member, which says nothing about
eng's own members, flips the planner to [group:eng#member].
The classic dispatcher decides this with a comparison and never queries for
it (internal/graph/lookupsubjects.go, req.SubjectRelation against
req.ResourceRelation). Do the same: Context.TargetSubjectType records what
the request asked for, kept separate from the per-call filterSubjectType,
which arrows and recursion legitimately leave empty because they must walk
intermediate-typed results to keep traversing. The decision is per node, not
per traversal — in `active = member - banned` with a target of group#member
it holds for `member` and not for `banned`, and that asymmetry is what makes
the exclusion come out right.
The target has to cross the dispatch boundary too: the receiver builds a
fresh Context, so its aliases saw no target and silently omitted identities
the sender included. PlanContext.top_level_operation already exists for this
exact reason, so target_subject_relation joins it as field 8.
Expected values in the new tests were captured from the classic dispatcher.
CheckWideGroups LookupSubjects drops from 175 datastore queries to 12 with
no change to any result.
This alone leaves queryconsistency failing, because it lets LookupSubjects
return an identity subject that the planner's Check does not agree with; the
Check side is fixed in the commit that follows.
Check decides the reflexive identity subject locally in resolveCheckPath, by
comparing the resource to the subject, and that works when the subject's
object is the resource being checked. It is never reached when the subject is
reached *through* the graph: a recursive permission resolves via
recursiveCheckIterSubjects, which answers by running the IterSubjects
machinery, and every alias in that traversal took shouldIncludeSelfEdge's
early return because the operation that started it was a Check.
The result was a wrong answer, verified against the classic dispatcher:
folder { relation parent: folder; relation viewer: user | folder#view
permission view = viewer + owner + parent->view }
folder:strategy#parent@folder:company
classic planner
Check(strategy#view <- company#view) MEMBER no path
Check(strategy#view <- company#viewer) MEMBER no path
Check(strategy#view <- strategy#view) MEMBER MEMBER
Check(strategy#view <- strategy#viewer) MEMBER MEMBER
Note the second row: identity holds at `viewer`, not only at the recursion's
own relation, so anything keyed to the recursion relation alone would have
been incomplete.
Give the traversal the subject it is checking. recursiveCheckIterSubjects
sets Context.TargetSubjectType for the duration of its BFS and restores it
afterwards, and the alias gate accepts a Check that has a target. Nothing
else changes: the result filter already matches on object equality ignoring
the relation, so both self edges above satisfy it without touching
filterSubjectType — which must stay relation-free, since it is also the
filter arrows and recursion leave empty in order to keep traversing.
This supersedes the TODO added in the previous commit, and unblocks the
LookupSubjects identity fix: queryconsistency now passes, having failed on
the pair where LookupSubjects and Check disagreed.
Two things found while measuring, both pre-existing and left alone:
recursiveCheckIterResources answers all four cases correctly and would have
been a one-line "fix", but it changes the shape of every recursive Check
(ShareWith goes from 12 datastore queries to 23) and leaves the IterSubjects
direction wrong, so it belongs to the advisor as a cost decision rather than
here. recursiveCheckDeepening does not terminate usefully on this schema —
buildTreeAtDepth climbs to maxDepth building exponentially larger trees — so
selecting that strategy is a hazard.
jzelinskie
force-pushed
the
qp-self-edge
branch
from
September 29, 2026 21:09
2a48164 to
69ec712
Compare
This branch has not been deployed
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
1 participant
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.
Description
The query planner decided the reflexive identity subject (
group:a#memberbeing one of the subjects ofgroup:a#member) with a datastore query. That query was expensive, and it gave wrong answers. This PR swaps it for the comparison the classic dispatcher makes, and fixes the Check path that the swap exposed.perf(query): decide the alias self edge by comparison, not a queryThe existing check (
SubjectExistsAsRelationship) ran aLIMIT 1query per object per alias level:CheckWideGroupsLookupSubjects, all distinct, and every resulting self edge was then thrown away.Like classic (
internal/graph/lookupsubjects.go), the alias now compares the requested subject relation against its own. The requested target lives inContext.TargetSubjectType, kept separate fromfilterSubjectType, which arrows and recursion leave empty. It crosses dispatch hops asPlanContext.target_subject_relation(field 8). This commit also adds aCountingReaderfor measuring datastore round-trips.CheckWideGroupsLookupSubjects goes from 175 queries to 12, and no results change.fix(query): keep the self edge when Check resolves through recursionA recursive Check resolves by running the IterSubjects machinery, and every alias in it skipped the self edge because the operation was a Check. For example,
Check(folder:strategy#view <- folder:company#view)returned MEMBER in classic but no path in the planner.recursiveCheckIterSubjectsnow sets the target to the subject being checked for the duration of its traversal, and restores the previous value afterwards.Notes:
Contextis only sound because a single goroutine drives eachContext. ATODOincontext.godescribes passing it as a parameter instead.IterSubjectsImplissues a Check. Planner LookupSubjects caching, if added, must include the target.Testing
TestAliasSelfEdge,TestCheckSelfEdgeThroughRecursion,TestCountingReader.queryconsistencypasses. It covers classic vs. planner Check and LookupResources.