Extend support for relationship filtering and add relationship filtering to other APIs #1739

josephschorr · 2024-02-07T00:51:49Z

Important

If a relationships filter is used without a resource type, there can be a performance impact due to the existing indexes all starting with the resource type. We may examine adding additional indexes in the future, but for now, the performance impact should be noted

Extends the relationship filter to allow for an optional resource_type, and a prefix for object IDs
Adds support for the relationship filter on BulkExport and Watch APIs

With this change, all objects can now be removed from the datastore with two calls (once with the object ID as the resource ID and once with it as the subject ID).

Fixes #315 (more or less; its two calls instead of one)

pkg/datastore/datastore.go

internal/services/v1/watch.go

vroldanbet · 2024-03-11T17:45:36Z

internal/services/v1/watch.go

+			// If there are filters, we need to check if the update matches any of them.
+			matched := false
+			for _, filter := range filters {
+				// TODO(jschorr): Maybe we should add TestRelationship to avoid the conversion?


This seems like a good compromise to avoid unnecessary allocations 🙏🏻

internal/services/v1/watch_test.go

internal/services/v1/watch.go

pkg/datastore/datastore.go

ecordell · 2024-03-11T20:57:46Z

internal/datastore/common/sql.go

+func (sqf SchemaQueryFilterer) MustFilterWithResourceIDPrefix(prefix string) SchemaQueryFilterer {
+	updated, err := sqf.FilterWithResourceIDPrefix(prefix)
+	if err != nil {
+		panic(err)


I see that the other filters use this pattern as well, but I thought we had decided to avoid panics outside of tests?

This function is only called from tests

ecordell · 2024-03-11T21:04:30Z

internal/services/v1/errors.go

+		err,
+		codes.InvalidArgument,
+		spiceerrors.ForReason(
+			v1.ErrorReason_ERROR_REASON_UNSPECIFIED,


any reason to wait on defining a real reason here?

It requires an API repo change and I didn't want to block this change, so I left it as a TODO

ecordell · 2024-03-11T21:05:53Z

internal/services/v1/experimental.go

 	}

-	return atRevision, cur, nil
+	return atRevision, decoded.GetV1().GetSections()[0], cur, nil


maybe a note about what this new return value is?

internal/datastore/common/sql.go

internal/datastore/memdb/readonly.go

vroldanbet · 2024-03-12T13:49:28Z

internal/datastore/memdb/readonly.go

-	index := indexNamespace
-	args := []any{filter.ResourceType}
-	if filter.OptionalResourceRelation != "" {
+	index := indexNamespace + "_prefix"


can you explain why _prefix was added to the default index? No index like that can be found in spicedb/internal/datastore/memdb/schema.go

Its a built-in to the backing memdb for prefix filtering; I'll add a note

internal/datastore/memdb/readwrite.go

vroldanbet · 2024-03-12T14:53:34Z

internal/datastore/postgres/readwrite.go

 		case err == nil:
-			break
+			nsClauses = append(nsClauses, sq.Eq{colNamespace: nsName})


a bit weird to see this here but seems correct

Has this changed appreciably?

it hasn't, just surprised me to see logic next to error handling

vroldanbet · 2024-03-12T18:59:41Z

internal/services/v1/experimental.go

@@ -647,29 +669,30 @@ func queryForEach(
 	return cur, nil
 }

-func decodeCursor(ds datastore.Datastore, encoded *v1.Cursor) (datastore.Revision, *core.RelationTuple, error) {
+func decodeCursor(ds datastore.Datastore, encoded *v1.Cursor) (datastore.Revision, string, *core.RelationTuple, error) {


not sure to understand if the format of the cursor has changed, and if this is a breaking change.

yes, it has: we now have a namespace contained in it

in that case, don't we have a mechanism to version cursors already in place, and that we should be using it?

e.g. a cursor V1 is not compatible with cursor V2 or viceversa

vroldanbet · 2024-03-12T19:00:36Z

internal/services/v1/experimental.go

-
-		// Datastore namespace order might not be exactly the same as go namespace order
-		// so we shouldn't assume cursors are valid across namespaces
-		cur = nil


can you clarify what happened here? I see a similar change was introduced earlier in the loop to make sure the cursor is reset for each iteration as the namespace changes

Not sure I follow

internal/services/v1/relationships.go

… relationships APIs

josephschorr · 2024-03-13T03:23:12Z

Updated. Good catch on the deletion side: it was missing handlers for the prefix filtering

josephschorr requested review from vroldanbet and a team as code owners February 7, 2024 00:51

github-actions bot added area/api v1 Affects the v1 API area/datastore Affects the storage system area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Feb 7, 2024

josephschorr mentioned this pull request Feb 7, 2024

Filter improvements in zed authzed/zed#341

Merged

josephschorr force-pushed the new-rel-filtering branch from 3179299 to ca4cc05 Compare March 6, 2024 20:06

vroldanbet enabled auto-merge March 6, 2024 20:09

vroldanbet reviewed Mar 11, 2024

View reviewed changes

josephschorr force-pushed the new-rel-filtering branch 3 times, most recently from 4b1ff59 to cfa8e45 Compare March 11, 2024 20:33

ecordell reviewed Mar 11, 2024

View reviewed changes

josephschorr force-pushed the new-rel-filtering branch from cfa8e45 to 7acac9a Compare March 11, 2024 21:20

vroldanbet reviewed Mar 12, 2024

View reviewed changes

josephschorr added 4 commits March 12, 2024 21:51

Update buf and API version for new relationships filters

78a880f

Add support for extended relationship filtering in all datastores

690f99a

Add extended relationships filtering to bulk export API

530584b

Add extended relationships filtering to read relationships and delete…

61914f2

… relationships APIs

josephschorr force-pushed the new-rel-filtering branch from 7acac9a to 3c735d8 Compare March 13, 2024 03:22

Add extended relationships filtering to watch API

d76d12c

josephschorr force-pushed the new-rel-filtering branch from 3c735d8 to d76d12c Compare March 13, 2024 03:57

vroldanbet approved these changes Mar 13, 2024

View reviewed changes

vroldanbet added this pull request to the merge queue Mar 13, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 13, 2024

vroldanbet added this pull request to the merge queue Mar 13, 2024

Merged via the queue into authzed:main with commit 031e5b4 Mar 13, 2024
20 checks passed

github-actions bot locked and limited conversation to collaborators Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend support for relationship filtering and add relationship filtering to other APIs #1739

Extend support for relationship filtering and add relationship filtering to other APIs #1739

josephschorr commented Feb 7, 2024 •

edited

Loading

vroldanbet Mar 11, 2024

ecordell Mar 11, 2024

josephschorr Mar 11, 2024

ecordell Mar 11, 2024

josephschorr Mar 11, 2024

ecordell Mar 11, 2024

vroldanbet Mar 12, 2024

josephschorr Mar 12, 2024

vroldanbet Mar 12, 2024

josephschorr Mar 13, 2024

vroldanbet Mar 13, 2024

vroldanbet Mar 12, 2024

josephschorr Mar 13, 2024

vroldanbet Mar 13, 2024

vroldanbet Mar 12, 2024

josephschorr Mar 13, 2024

josephschorr commented Mar 13, 2024

Extend support for relationship filtering and add relationship filtering to other APIs #1739

Extend support for relationship filtering and add relationship filtering to other APIs #1739

Conversation

josephschorr commented Feb 7, 2024 • edited Loading

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

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 Mar 13, 2024

josephschorr commented Feb 7, 2024 •

edited

Loading