Fix experimental LookupResources2 to shear the tree earlier on indirect permissions #2005
Conversation
github-actions
bot
added
area/tooling
josephschorr
force-pushed
the
lrv2-fix
branch
5 times, most recently
from
6f4ad7a
to
61fb9d1
Compare
| clonedMap := rsm.resourcesAndSubjects.Clone() | ||
| return resourcesSubjectMap2{ | ||
| resourceType: rsm.resourceType, | ||
| resourcesAndSubjects: clonedMap, | ||
| } |
There was a problem hiding this comment.
| clonedMap := rsm.resourcesAndSubjects.Clone() | |
| return resourcesSubjectMap2{ | |
| resourceType: rsm.resourceType, | |
| resourcesAndSubjects: clonedMap, | |
| } | |
| return resourcesSubjectMap2{ | |
| resourceType: rsm.resourceType, | |
| resourcesAndSubjects: rsm.resourcesAndSubjects.Clone(), | |
| } |
| permission indirect = viewer & editor | ||
| permission view = indirect - banned | ||
| }`, | ||
| joinTuples( |
There was a problem hiding this comment.
make joinTuples variadic so you don't have to nest like this?
| resourceRelation: RR("document", "view_and_admin"), | ||
| subject: ONR("user", "tom", "..."), | ||
| disallowedQueries: []*core.RelationReference{ | ||
| RR("document", "viewer"), |
There was a problem hiding this comment.
these are disallowed because we should dispatch over viewer_of_some_kind?
There was a problem hiding this comment.
Yeah and the check is the second half of the permission
internal/graph/lr2streams.go
Outdated
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
|
|
||
| default: | ||
| } |
There was a problem hiding this comment.
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| } | |
| if ctx.Err() != nil { | |
| return ctx.Err() | |
| } |
| } | ||
|
|
||
| func (rdc *dispatchAndCheckRunner) runChecker(ctx context.Context, collected []*v1.DispatchLookupResources2Response) error { | ||
| func (rdc *checkAndDispatchRunner) runChecker(ctx context.Context, startingIndex int) error { | ||
| rdc.lock.Lock() | ||
| if rdc.ci.limits.hasExhaustedLimit() { |
There was a problem hiding this comment.
I think the limit should just be an atomic, and then you can get rid of the locking.
There was a problem hiding this comment.
Its not; we'd have to change the implementation and its use everywhere
There was a problem hiding this comment.
My suggestion was to change it to an atomic and update the callsites. That can be a followup though.
| @@ -261,7 +287,7 @@ func publishResultToParentStream( | |||
|
|
|||
| // The cursor for the response is that of the parent response + the cursor from the result itself. | |||
| afterResponseCursor, err := combineCursors( | |||
| ci.responsePartialCursor(), | |||
| responseCursor, | |||
| result.AfterResponseCursor, | |||
| ) | |||
| if err != nil { | |||
There was a problem hiding this comment.
I can't comment on it, but:
mappedResource, err := foundResources.mapPossibleResource(result.Resource)
...
missingContextParameters := mapz.NewSet(mappedResource.MissingContextParams...)
missingContextParameters.Extend(result.Resource.MissingContextParams)
missingContextParameters.Extend(additionalMissingContext)
mappedResource.MissingContextParams = missingContextParameters.AsSlice()Why not just have mappedResource.MissingContextParams be a set so that you can just do:
mappedResource, err := foundResources.mapPossibleResource(result.Resource)
mappedResource.MissingContextParams.Extend(result.Resource.MissingContextParams)
mappedResource.MissingContextParams.Extend(additionalMissingContext)and then convert to a slice when you need to?
There was a problem hiding this comment.
Its a proto so it can't be a set
| @@ -261,7 +287,7 @@ func publishResultToParentStream( | |||
|
|
|||
| // The cursor for the response is that of the parent response + the cursor from the result itself. | |||
| afterResponseCursor, err := combineCursors( | |||
| ci.responsePartialCursor(), | |||
| responseCursor, | |||
| result.AfterResponseCursor, | |||
| ) | |||
| if err != nil { | |||
There was a problem hiding this comment.
Can't comment on this either but:
metadata := result.Metadata
if isFirstPublishCall {
metadata = addCallToResponseMetadata(metadata)
metadata = combineResponseMetadata(metadata, additionalMetadata)
} else {
metadata = addAdditionalDepthRequired(metadata)
}addCallToResponseMetadata increments Dispatch and DepthRequired
addAdditionalDepthRequired increments just DepthRequired
Shouldn't depth and dispatch go hand in hand? Why is dispatch only incremented on the first publish for a stream - or alternatively, why is depth required incremented for messages on an existing stream?
There was a problem hiding this comment.
No; we only increase the dispatch count on the first result lest we double count it (this is what caused the dispatch count bug in LR many moons ago)
|
Updated |
No description provided.