Add support for basic cursors and limits to LookupSubjects #1379

josephschorr · 2023-06-01T21:33:15Z

This change supports a limit (called the "concrete limit") on LookupSubjects and will filter concrete subjects based on the returned cursor.

This change does not filter intermediate lookups, which will be done in a followup PR.

vroldanbet

I didn't have time to finish the review, sorry! This is my first attempt. So far it's looking good, although I have a bunch of questions.

Something I noticed is that when one resumes with a cursor, we are evaluating the whole graph again and sending queries to the database, even though they will return empty. It works, but is a lot of work that is wasted after resuming, and will be directly proportional to the complexity of the schema. Ideally the evaluation of the schema can also resume from where the cursor left off, e.g. if permission view = a + b + c you'd restart evaluation at c if that's where you left when the limit was hit. It would spare a bunch of dispatching (network calls across SpiceDBs!) and DB roundtrips.

internal/graph/lookupsubjects.go

vroldanbet · 2023-07-28T14:05:35Z

internal/services/v1/permissions.go

+				excludedSubjectIDs := make([]string, 0, len(foundSubject.ExcludedSubjects))
+				for _, excludedSubject := range foundSubject.ExcludedSubjects {
+					excludedSubjectIDs = append(excludedSubjectIDs, excludedSubject.SubjectId)
+				}


can we merge this into the loop below?

I did so, but I'm thinking maybe we just remove it entirely now? The field has been marked deprecated for a number of versions

sounds good to me!

internal/services/v1/permissions.go

vroldanbet · 2023-07-28T14:36:24Z

internal/services/v1/permissions.go

-			if subject == nil {
-				continue
-			}
+				encodedCursor, err := cursor.EncodeFromDispatchCursor(


do we need to allocate a new encoded cursor each time here? Could we reuse the same proto instance and just modify the corresponding field? I did something as an optimization in the ReadRelationships streaming bits and helped a bunch with allocations and GC overhead. It would also spare us calling revision.String() repeatedly.

It would be far less maintainable, but I'll see what I can do

Done, but its a bit ugly

After a second pass, I thought the new code could have a race. Perhaps it's not worth the trouble? I'll leave it up to you

internal/services/v1/permissions.go

internal/graph/lookupsubjects.go

vroldanbet · 2023-07-28T17:18:56Z

internal/graph/lookupsubjects.go

+	for _, foundSubjects := range subjects {
+		for _, foundSubject := range foundSubjects.FoundSubjects {
+			// NOTE: wildcard is always returned, because it is needed by all branches, at all times.
+			if foundSubject.SubjectId == tuple.PublicWildcard || (afterSubjectID == "" || foundSubject.SubjectId > afterSubjectID) {


in not sure to understand how we guarantee that we do not skip found subjects that are alphabetically after the current cursor but that haven't been seen by the stream before.

Because the stream always returns in sorted order from the root. That way, we are always guaranteed to have a defined ordering (alphabetical) coming out of each subproblem

Doesn't that mean that we have to start collecting subjects from all subproblems before we can start streaming? We can make sure that each subproblem retrieves results in order from DB, but you still need to execute all of them in order to determine what's the first subject to stream?

internal/graph/lookupsubjects.go

pkg/genutil/slicez/chunking_test.go

internal/services/v1/permissions_test.go

vroldanbet · 2023-08-21T19:53:06Z

internal/services/v1/permissions_test.go

+
+								dispatchCount, err := responsemeta.GetIntResponseTrailerMetadata(trailer, responsemeta.DispatchedOperationsCount)
+								req.NoError(err)
+								req.GreaterOrEqual(dispatchCount, 0)


there had been some regressions recently around dispatches and I wonder if we could use a new test-case that checks the number of dispatches over a cursored LR call

You mean over the LS call? We do now have the test for LR calls

I mean testing for LS. The logic should be different enough that it warrants another set of tests?

Gotcha. Added

internal/datasets/subjectsetbytype.go

internal/services/integrationtesting/consistency_test.go

internal/datasets/basesubjectset.go

internal/dispatch/keys/computed.go

internal/graph/lookupsubjects.go

vroldanbet · 2023-09-05T12:19:49Z

internal/graph/lookupsubjects.go

+}
+
+// filterSubjectsMap filters the subjects found in the subjects map to only those allowed, returning an updated map.
+func filterSubjectsMap(subjects map[string]*v1.FoundSubjects, allowedSubjectIds ...string) map[string]*v1.FoundSubjects {


I see no use of the variadic allowedSubjectIds so please pass as a slice. NewSet should also support a slice - a lot of calls in the codebase that end up doing ... just to pass it to it.

Changed here but not NewSet. We have some places where the variadic is helpful. If you like, I can add another NewSetFromSlice and change the call sites?

yep I think that'd be a good idea

Done and changed all the call sites

internal/graph/lookupsubjects.go

internal/graph/lookupsubjects_test.go

internal/dispatch/graph/lookupsubjects_test.go

internal/graph/lookupsubjects.go

josephschorr · 2023-09-06T17:57:58Z

Updated

josephschorr · 2024-03-11T17:41:04Z

Rebased

josephschorr · 2024-04-26T19:14:05Z

Rebased

vroldanbet

Doing a complete re-review to load all the context. First batch of feedback.

internal/services/v1/permissions.go

vroldanbet · 2024-05-16T12:06:08Z

internal/services/v1/permissions.go

-			excludedSubjectIDs := make([]string, 0, len(foundSubject.ExcludedSubjects))
-			for _, excludedSubject := range foundSubject.ExcludedSubjects {
-				excludedSubjectIDs = append(excludedSubjectIDs, excludedSubject.SubjectId)
+	for {


The handling stream should handle all the elements up to the concrete limit, wouldn't it? From looking at the code, it would seem the only sole purpose is making sure the wildcard is sent, since in the worst-case scenario we stream up to the concrete limit, and we have to handle the potential +1 of the wildcard. Is that correct?

internal/services/v1/permissions.go

pkg/proto/dispatch/v1/dispatch_vtproto.pb.go

vroldanbet

@josephschorr last batch of feedback. Let's address this quickly so we don't loose the context and we can land it soon.

internal/graph/lookupsubjects.go

vroldanbet · 2024-05-17T13:59:50Z

internal/services/v1/permissions_test.go

+
+								dispatchCount, err := responsemeta.GetIntResponseTrailerMetadata(trailer, responsemeta.DispatchedOperationsCount)
+								req.NoError(err)
+								req.GreaterOrEqual(dispatchCount, 0)


I mean testing for LS. The logic should be different enough that it warrants another set of tests?

vroldanbet · 2024-05-17T14:02:18Z

internal/services/v1/permissions.go

-			if subject == nil {
-				continue
-			}
+				encodedCursor, err := cursor.EncodeFromDispatchCursor(


After a second pass, I thought the new code could have a race. Perhaps it's not worth the trouble? I'll leave it up to you

vroldanbet · 2024-05-17T14:03:53Z

internal/services/v1/permissions.go

+				excludedSubjectIDs := make([]string, 0, len(foundSubject.ExcludedSubjects))
+				for _, excludedSubject := range foundSubject.ExcludedSubjects {
+					excludedSubjectIDs = append(excludedSubjectIDs, excludedSubject.SubjectId)
+				}


sounds good to me!

internal/graph/lookupsubjects_test.go

internal/graph/lookupsubjects.go

josephschorr · 2024-05-17T21:05:29Z

Updated

This change supports a limit (called the "concrete limit") on LookupSubjects and will filter concrete subjects based on the returned cursor. This change does *not* filter intermediate lookups, which will be done in a followup PR.

josephschorr requested a review from vroldanbet June 1, 2023 21:33

josephschorr requested a review from a team as a code owner June 1, 2023 21:33

github-actions bot added area/api v1 Affects the v1 API area/dependencies Affects dependencies area/dispatch Affects dispatching of requests area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Jun 1, 2023

josephschorr force-pushed the cursored-lookup-subjects branch 2 times, most recently from da48a6b to 19099a2 Compare June 1, 2023 21:50

josephschorr marked this pull request as draft June 20, 2023 23:38

josephschorr force-pushed the cursored-lookup-subjects branch 3 times, most recently from 21ae960 to c386a05 Compare July 12, 2023 19:36

josephschorr marked this pull request as ready for review July 12, 2023 19:58

vroldanbet reviewed Jul 28, 2023

View reviewed changes

vroldanbet reviewed Aug 21, 2023

View reviewed changes

vroldanbet reviewed Sep 5, 2023

View reviewed changes

internal/dispatch/graph/lookupsubjects_test.go Show resolved Hide resolved

internal/graph/lookupsubjects.go Show resolved Hide resolved

josephschorr force-pushed the cursored-lookup-subjects branch from c386a05 to 137e08e Compare September 6, 2023 17:57

github-actions bot removed the area/dependencies Affects dependencies label Sep 6, 2023

josephschorr force-pushed the cursored-lookup-subjects branch from 137e08e to 1dec055 Compare March 11, 2024 17:40

josephschorr force-pushed the cursored-lookup-subjects branch 2 times, most recently from 3202a4b to b00913b Compare March 11, 2024 20:36

josephschorr force-pushed the cursored-lookup-subjects branch from b00913b to 83a96b2 Compare March 25, 2024 13:11

josephschorr force-pushed the cursored-lookup-subjects branch from 83a96b2 to 39f033f Compare April 26, 2024 19:13

josephschorr force-pushed the cursored-lookup-subjects branch 2 times, most recently from 57a79b9 to 07dbacd Compare May 14, 2024 15:48

vroldanbet reviewed May 16, 2024

View reviewed changes

vroldanbet reviewed May 17, 2024

View reviewed changes

josephschorr force-pushed the cursored-lookup-subjects branch from 07dbacd to 0c9fbb7 Compare May 17, 2024 21:05

vroldanbet mentioned this pull request May 20, 2024

introduce lookup subjects page limit authzed/zed#379

Draft

josephschorr force-pushed the cursored-lookup-subjects branch from 0c9fbb7 to 1f765d0 Compare May 20, 2024 18:33

github-actions bot added the area/datastore Affects the storage system label May 20, 2024

josephschorr force-pushed the cursored-lookup-subjects branch from 1f765d0 to e74c6f3 Compare May 20, 2024 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for basic cursors and limits to LookupSubjects #1379

Add support for basic cursors and limits to LookupSubjects #1379

josephschorr commented Jun 1, 2023 •

edited

vroldanbet left a comment

vroldanbet Jul 28, 2023

josephschorr Sep 6, 2023

vroldanbet May 17, 2024

vroldanbet Jul 28, 2023

josephschorr Sep 6, 2023

josephschorr Sep 6, 2023

vroldanbet May 17, 2024

vroldanbet Jul 28, 2023 •

edited

josephschorr Sep 6, 2023

vroldanbet May 17, 2024

vroldanbet Aug 21, 2023

josephschorr Sep 6, 2023

vroldanbet May 17, 2024 •

edited

josephschorr May 20, 2024

vroldanbet Sep 5, 2023

josephschorr Sep 6, 2023

vroldanbet May 17, 2024

josephschorr May 20, 2024

josephschorr commented Sep 6, 2023

josephschorr commented Mar 11, 2024

josephschorr commented Apr 26, 2024

vroldanbet left a comment

vroldanbet May 16, 2024

vroldanbet left a comment

vroldanbet May 17, 2024 •

edited

vroldanbet May 17, 2024

vroldanbet May 17, 2024

josephschorr commented May 17, 2024

Add support for basic cursors and limits to LookupSubjects #1379

Are you sure you want to change the base?

Add support for basic cursors and limits to LookupSubjects #1379

Conversation

josephschorr commented Jun 1, 2023 • edited

vroldanbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vroldanbet Jul 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vroldanbet May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented Sep 6, 2023

josephschorr commented Mar 11, 2024

josephschorr commented Apr 26, 2024

vroldanbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vroldanbet left a comment

Choose a reason for hiding this comment

vroldanbet May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented May 17, 2024

josephschorr commented Jun 1, 2023 •

edited

vroldanbet Jul 28, 2023 •

edited

vroldanbet May 17, 2024 •

edited

vroldanbet May 17, 2024 •

edited