Update BaseSubjectSet to support caveat expressions #932

josephschorr · 2022-10-24T17:39:56Z

This is the first (massive) step in supporting caveats in LookupSubjects, as this implements all the bookkeeping and tracking associated with each subject added to the subject set

First part of #931

ecordell

did an initial pass with questions, definitely haven't read the implementation and tests in detail yet

ecordell · 2022-10-24T18:32:46Z

proto/internal/dispatch/v1/dispatch.proto

@@ -161,7 +161,8 @@ message DispatchLookupSubjectsRequest {

 message FoundSubject {
  string subject_id = 1;
-  repeated string excluded_subject_ids = 2;
+  CaveatExpression conditional_expression = 2;


I don't know off the top of my head what the best way to break a grpc api is - it might be better to skip 2 here entirely?

It only really matters if we intend back compat and to do so we'd have to fill in the excluded IDs as well. We could do that, but it would require keeping around the extra logic

ecordell · 2022-10-24T18:35:22Z

internal/membership/foundsubject.go

+		for _, excludedSubject := range fs.excludedSubjects {
+			// TODO(jschorr): Fix once we add caveats support to debug tooling
+			if excludedSubject.conditionalExpression != nil {
+				panic("not yet supported")


won't panic-ing here prevent us from cutting an release without the debug tooling implemented?

Yep! That's intended; I want to do something here, even if it is just returning "(unsupported)", but I don't know exactly what yet, so I added in panics

internal/util/basesubjectset.go

ecordell · 2022-10-24T21:37:26Z

internal/util/basesubjectset.go

 // Add adds the found subject to the set. This is equivalent to a Union operation between the
-// existing set of subjects and a set containing the single subject.
+// existing set of subjects and a set containing the single subject, but modifies the set
+// *in place*.
 func (bss BaseSubjectSet[T]) Add(foundSubject T) bool {


I haven't gotten to the callsites yet, but it's not obvious to me why Add should return a bool or why it's helpful to know if the set had a wildcard before the add.

standard set semantics: did this subject already exist when I added it

ecordell · 2022-10-24T21:39:54Z

internal/util/basesubjectset.go

 }

 // SubtractAll subtracts the other set of subjects from this set of subtracts, modifying this
-// set in place.
+// set *in place*.
 func (bss BaseSubjectSet[T]) SubtractAll(other BaseSubjectSet[T]) {


Why not just have Subtract(sets ...BaseSubjectSet[T]) to cover Subtract and SubtractAll?

Subtract takes in a single element, not a set and to have a combined method, we'd have to convert from one or the other

internal/util/basesubjectset.go

ecordell · 2022-10-24T21:44:19Z

internal/util/basesubjectset.go

 	}
+
+	bss.wildcard.subtractConcrete(foundSubject)
+	bss.concrete.subtractConcrete(foundSubject)


I think this double-bookkeeping is weird but I don't have an alternate suggestion at the moment.

Every operation actually modifies these two underlying sets, and I'll have to figure out how they get combined elsewhere.

The combination is just taking the values in each; they each track their own state independently.

internal/util/basesubjectset.go

vroldanbet

Not done with the review, but have some comments

internal/util/basesubjectset.go

internal/util/subjectset_test.go

josephschorr · 2022-10-27T16:22:55Z

Updated

vroldanbet

Had final look, this is some complex stuff and I really appreciate how thorough you were with tests and comments. Could only find one test-test that seems to be missing, touches a portion of code that was untested, and is not giving the result (I, at least) expected

Once that's addressed this LGTM

EDIT: there is also a linter failure

internal/util/subjectset_test.go

internal/util/basesubjectset.go

josephschorr · 2022-10-27T19:05:43Z

Updated

ecordell

LGTM

I'm convinced this works 👍

Added some extra tests to convince myself in this branch, for future reference: josephschorr/spicedb@josephschorr:dc79c72...ecordell:3b6fbc6

josephschorr · 2022-11-03T00:56:09Z

Updated and rewritten as discussed, and added additional tests

internal/util/basesubjectset.go

internal/util/subjectset_test.go

internal/util/basesubjectset.go

josephschorr · 2022-11-03T19:38:41Z

Updated

josephschorr · 2022-11-03T20:54:29Z

Updated

This is the first (massive) step in supporting caveats in LookupSubjects, as this implements all the bookkeeping and tracking associated with each subject added to the subject set First part of authzed#931

ecordell

test changes LGTM

josephschorr requested review from ecordell, vroldanbet and a team October 24, 2022 17:39

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

josephschorr force-pushed the ls-subjectset branch from 3bec295 to 21c5fca Compare October 24, 2022 17:47

ecordell reviewed Oct 24, 2022

View reviewed changes

josephschorr force-pushed the ls-subjectset branch from 21c5fca to ccc2211 Compare October 24, 2022 22:10

vroldanbet reviewed Oct 27, 2022

View reviewed changes

internal/util/basesubjectset.go Outdated Show resolved Hide resolved

internal/util/subjectset_test.go Show resolved Hide resolved

internal/util/subjectset_test.go Show resolved Hide resolved

josephschorr force-pushed the ls-subjectset branch 2 times, most recently from 2e2a07d to 8a0920c Compare October 27, 2022 16:22

vroldanbet reviewed Oct 27, 2022

View reviewed changes

internal/util/subjectset_test.go Outdated Show resolved Hide resolved

internal/util/basesubjectset.go Outdated Show resolved Hide resolved

josephschorr force-pushed the ls-subjectset branch from 8a0920c to f84ed73 Compare October 27, 2022 19:05

josephschorr force-pushed the ls-subjectset branch 2 times, most recently from f4aaae0 to 442752d Compare October 27, 2022 20:21

vroldanbet previously approved these changes Oct 27, 2022

View reviewed changes

josephschorr dismissed vroldanbet’s stale review via 40d2565 October 31, 2022 16:56

josephschorr force-pushed the ls-subjectset branch 5 times, most recently from 0d2d1f5 to dc79c72 Compare November 1, 2022 16:05

ecordell previously approved these changes Nov 1, 2022

View reviewed changes

josephschorr dismissed ecordell’s stale review via 3c1b892 November 1, 2022 21:00

josephschorr force-pushed the ls-subjectset branch from 3c1b892 to 521d4c7 Compare November 1, 2022 21:01

ecordell previously approved these changes Nov 1, 2022

View reviewed changes

josephschorr dismissed ecordell’s stale review via 4bb3acb November 3, 2022 00:54

josephschorr force-pushed the ls-subjectset branch 2 times, most recently from 4bb3acb to 23fe801 Compare November 3, 2022 00:55

vroldanbet force-pushed the ls-subjectset branch from 23fe801 to cc6800b Compare November 3, 2022 17:07

vroldanbet previously approved these changes Nov 3, 2022

View reviewed changes

josephschorr dismissed vroldanbet’s stale review via 4fd8043 November 3, 2022 19:38

josephschorr force-pushed the ls-subjectset branch from cc6800b to 4fd8043 Compare November 3, 2022 19:38

vroldanbet previously approved these changes Nov 3, 2022

View reviewed changes

josephschorr dismissed vroldanbet’s stale review via 33a00fd November 3, 2022 20:54

josephschorr force-pushed the ls-subjectset branch from 4fd8043 to 33a00fd Compare November 3, 2022 20:54

josephschorr force-pushed the ls-subjectset branch from 33a00fd to b496292 Compare November 3, 2022 20:55

vroldanbet previously approved these changes Nov 3, 2022

View reviewed changes

Update BaseSubjectSet to support caveat expressions

0cb2cfc

This is the first (massive) step in supporting caveats in LookupSubjects, as this implements all the bookkeeping and tracking associated with each subject added to the subject set First part of authzed#931

josephschorr dismissed vroldanbet’s stale review via 0cb2cfc November 3, 2022 21:45

josephschorr force-pushed the ls-subjectset branch from b496292 to 0cb2cfc Compare November 3, 2022 21:45

vroldanbet approved these changes Nov 3, 2022

View reviewed changes

ecordell approved these changes Nov 7, 2022

View reviewed changes

josephschorr merged commit ce5587c into authzed:main Nov 7, 2022

github-actions bot locked and limited conversation to collaborators Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update BaseSubjectSet to support caveat expressions #932

Update BaseSubjectSet to support caveat expressions #932

josephschorr commented Oct 24, 2022

ecordell left a comment

ecordell Oct 24, 2022

josephschorr Oct 24, 2022

ecordell Oct 24, 2022

josephschorr Oct 24, 2022

ecordell Oct 24, 2022

josephschorr Oct 24, 2022

ecordell Oct 24, 2022

josephschorr Oct 24, 2022

ecordell Oct 24, 2022

josephschorr Oct 24, 2022

vroldanbet left a comment

josephschorr commented Oct 27, 2022

vroldanbet left a comment •

edited

Loading

josephschorr commented Oct 27, 2022

ecordell left a comment

josephschorr commented Nov 3, 2022

josephschorr commented Nov 3, 2022

josephschorr commented Nov 3, 2022

ecordell left a comment

Update BaseSubjectSet to support caveat expressions #932

Update BaseSubjectSet to support caveat expressions #932

Conversation

josephschorr commented Oct 24, 2022

ecordell 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

Choose a reason for hiding this 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

josephschorr commented Oct 27, 2022

vroldanbet left a comment • edited Loading

Choose a reason for hiding this comment

josephschorr commented Oct 27, 2022

ecordell left a comment

Choose a reason for hiding this comment

josephschorr commented Nov 3, 2022

josephschorr commented Nov 3, 2022

josephschorr commented Nov 3, 2022

ecordell left a comment

Choose a reason for hiding this comment

vroldanbet left a comment •

edited

Loading