Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change from using protos to structs for relationships, ONRs and RRs #2081

Merged
merged 28 commits into from
Oct 21, 2024
Merged

Change from using protos to structs for relationships, ONRs and RRs #2081

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
6522b97
Reorganize tuple package and add relationship structs
josephschorr Sep 16, 2024
3f4430a
Start on datastore conversion with MemDB
josephschorr Sep 16, 2024
bf7f9eb
Update tests and get memdb passing for struct and iterator changes
josephschorr Sep 16, 2024
bafcc3e
Small tuple package improvements
josephschorr Sep 16, 2024
5dcbb03
Tuple -> Relationship renames
josephschorr Sep 17, 2024
b705c64
Start addressing RelationTuple in all other packages
josephschorr Sep 25, 2024
b0e613d
Update postgres driver for relationship changes
josephschorr Sep 28, 2024
73ed91e
Update MySQL driver for relationship changes
josephschorr Sep 29, 2024
3cce89d
Update CRDB and Spanner and a few other locations for relationship ch…
josephschorr Sep 29, 2024
272c6ab
Address lint errors
josephschorr Sep 30, 2024
4d4d494
Move the remaining go files to 1.23.1
josephschorr Sep 30, 2024
8e247c0
Fix CRDB tests for relationship changes
josephschorr Sep 30, 2024
51f65db
Fix Postgres tests for relationship changes
josephschorr Sep 30, 2024
b1778ee
Fix MySQL tests for relationship changes
josephschorr Sep 30, 2024
7913b10
Fix Spanner tests for relationship changes
josephschorr Sep 30, 2024
6d83776
Fix benchmark test
josephschorr Sep 30, 2024
c54eaa5
Fix WASM test for relationships changes
josephschorr Sep 30, 2024
ea0a2cd
Integration test fixes for changed relationships
josephschorr Sep 30, 2024
2457c82
Integration test fixes for changed relationships
josephschorr Sep 30, 2024
0ac9ad8
Disable close-after-usage check for rel iterator, as it handles it in…
josephschorr Sep 30, 2024
24b87e8
Update read-only mode test for PG
josephschorr Sep 30, 2024
ebb4e52
Fix dev test error
josephschorr Sep 30, 2024
42b7324
Fixes for new relationship structs after rebase
josephschorr Oct 1, 2024
a23ec29
Add DebugAssertNotNil
josephschorr Oct 16, 2024
afad40e
Address review feedback
josephschorr Oct 16, 2024
7337008
Address PR feedback
josephschorr Oct 18, 2024
70d7928
Address PR feedback
josephschorr Oct 21, 2024
487e85b
Adjustments after running `fieldalignment -fix github.com/authzed/spi…
josephschorr Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/spicedb/servetesting_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,15 @@ func TestTestServer(t *testing.T) {
// Try writing a simple relationship against readonly and ensure it fails.
_, err = rov1client.WriteRelationships(context.Background(), &v1.WriteRelationshipsRequest{
Updates: []*v1.RelationshipUpdate{
tuple.UpdateToRelationshipUpdate(tuple.Create(relationship)),
tuple.MustUpdateToV1RelationshipUpdate(tuple.Create(relationship)),
},
})
require.Equal("rpc error: code = Unavailable desc = service read-only", err.Error())

// Write a simple relationship.
_, err = v1client.WriteRelationships(context.Background(), &v1.WriteRelationshipsRequest{
Updates: []*v1.RelationshipUpdate{
tuple.UpdateToRelationshipUpdate(tuple.Create(relationship)),
tuple.MustUpdateToV1RelationshipUpdate(tuple.Create(relationship)),
},
})
require.NoError(err)
Expand Down
2 changes: 1 addition & 1 deletion e2e/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/authzed/spicedb/e2e

go 1.22.7
go 1.23.1

require (
github.com/authzed/authzed-go v0.16.1-0.20241001202507-27cc182a7b92
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/authzed/spicedb

go 1.22.7
go 1.23.1

require (
buf.build/gen/go/prometheus/prometheus/protocolbuffers/go v1.34.2-20240802094132-5b212ab78fb7.2
Expand Down
10 changes: 5 additions & 5 deletions internal/datasets/subjectsetbyresourceid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ package datasets
import (
"fmt"

core "github.com/authzed/spicedb/pkg/proto/core/v1"
v1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
"github.com/authzed/spicedb/pkg/tuple"
)

// NewSubjectSetByResourceID creates and returns a map of subject sets, indexed by resource ID.
Expand Down Expand Up @@ -34,10 +34,10 @@ func (ssr SubjectSetByResourceID) add(resourceID string, subject *v1.FoundSubjec

// AddFromRelationship adds the subject found in the given relationship to this map, indexed at
// the resource ID specified in the relationship.
func (ssr SubjectSetByResourceID) AddFromRelationship(relationship *core.RelationTuple) error {
return ssr.add(relationship.ResourceAndRelation.ObjectId, &v1.FoundSubject{
SubjectId: relationship.Subject.ObjectId,
CaveatExpression: wrapCaveat(relationship.Caveat),
func (ssr SubjectSetByResourceID) AddFromRelationship(relationship tuple.Relationship) error {
return ssr.add(relationship.Resource.ObjectID, &v1.FoundSubject{
SubjectId: relationship.Subject.ObjectID,
CaveatExpression: wrapCaveat(relationship.OptionalCaveat),
})
}

Expand Down
12 changes: 6 additions & 6 deletions internal/datasets/subjectsetbytype.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,24 @@ func NewSubjectByTypeSet() *SubjectByTypeSet {
}

// AddSubjectOf adds the subject found in the given relationship, along with its caveat.
func (s *SubjectByTypeSet) AddSubjectOf(relationship *core.RelationTuple) error {
return s.AddSubject(relationship.Subject, relationship.Caveat)
func (s *SubjectByTypeSet) AddSubjectOf(relationship tuple.Relationship) error {
return s.AddSubject(relationship.Subject, relationship.OptionalCaveat)
}

// AddConcreteSubject adds a non-caveated subject to the set.
func (s *SubjectByTypeSet) AddConcreteSubject(subject *core.ObjectAndRelation) error {
func (s *SubjectByTypeSet) AddConcreteSubject(subject tuple.ObjectAndRelation) error {
return s.AddSubject(subject, nil)
}

// AddSubject adds the specified subject to the set.
func (s *SubjectByTypeSet) AddSubject(subject *core.ObjectAndRelation, caveat *core.ContextualizedCaveat) error {
key := tuple.JoinRelRef(subject.Namespace, subject.Relation)
func (s *SubjectByTypeSet) AddSubject(subject tuple.ObjectAndRelation, caveat *core.ContextualizedCaveat) error {
vroldanbet marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were the contextualized caveat and integrity bits left as protos? Will it be addressed in a follow-up?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few reasons:

  1. They aren't used nearly as often, so the space savings are minimal
  2. Context can be quite large, so copying it around on the stack seems unwise
  3. Context is often nil, so there won't be any overhead in the common case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context. It just feels odd because it won't be clear to the reader why the codebase is left in this odd mixture of structs and protos, and what pattern should be followed as the codebase evolves. From the arguments laid, context being quite large seems like something that would be equally problematic for the heap.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context being large is better to be on the heap: it ensures we're not copying it around; granted, context is not usually super large, but either way, its not used as much

key := tuple.JoinRelRef(subject.ObjectType, subject.Relation)
if _, ok := s.byType[key]; !ok {
s.byType[key] = NewSubjectSet()
}

return s.byType[key].Add(&v1.FoundSubject{
SubjectId: subject.ObjectId,
SubjectId: subject.ObjectID,
CaveatExpression: wrapCaveat(caveat),
})
}
Expand Down
10 changes: 5 additions & 5 deletions internal/datasets/subjectsetbytype_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,19 @@ func TestSubjectByTypeSet(t *testing.T) {
require.True(t, set.IsEmpty())

// Add some concrete subjects.
err := set.AddConcreteSubject(tuple.ParseONR("document:foo#viewer"))
err := set.AddConcreteSubject(tuple.MustParseONR("document:foo#viewer"))
require.NoError(t, err)

err = set.AddConcreteSubject(tuple.ParseONR("document:bar#viewer"))
err = set.AddConcreteSubject(tuple.MustParseONR("document:bar#viewer"))
require.NoError(t, err)

err = set.AddConcreteSubject(tuple.ParseONR("team:something#member"))
err = set.AddConcreteSubject(tuple.MustParseONR("team:something#member"))
require.NoError(t, err)

err = set.AddConcreteSubject(tuple.ParseONR("team:other#member"))
err = set.AddConcreteSubject(tuple.MustParseONR("team:other#member"))
require.NoError(t, err)

err = set.AddConcreteSubject(tuple.ParseONR("team:other#manager"))
err = set.AddConcreteSubject(tuple.MustParseONR("team:other#manager"))
require.NoError(t, err)
Comment on lines +44 to 57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easier to maintain over time if you write these with a loop:

for _, rel := range []string{
    "document:foo#viewer",
    "document:bar#viewer",
    "team:something#member",
    "team:other#member",
    "team:other#manager",
} {
    require.NoError(t, set.AddConcreteSubject(tuple.MustParseONR(rel)))
}


// Add a caveated subject.
Expand Down
92 changes: 49 additions & 43 deletions internal/datastore/benchmark/driver_bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
dsconfig "github.com/authzed/spicedb/pkg/cmd/datastore"
"github.com/authzed/spicedb/pkg/datastore"
"github.com/authzed/spicedb/pkg/datastore/options"
core "github.com/authzed/spicedb/pkg/proto/core/v1"
"github.com/authzed/spicedb/pkg/tuple"
)

Expand Down Expand Up @@ -80,12 +79,9 @@ func BenchmarkDatastoreDriver(b *testing.B) {
// Write a fair amount of data, much more than a functional test
for docNum := 0; docNum < numDocuments; docNum++ {
_, err := ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error {
var updates []*core.RelationTupleUpdate
var updates []tuple.RelationshipUpdate
for userNum := 0; userNum < usersPerDoc; userNum++ {
updates = append(updates, &core.RelationTupleUpdate{
Operation: core.RelationTupleUpdate_CREATE,
Tuple: docViewer(strconv.Itoa(docNum), strconv.Itoa(userNum)),
})
updates = append(updates, tuple.Create(docViewer(strconv.Itoa(docNum), strconv.Itoa(userNum))))
}

return rwt.WriteRelationships(ctx, updates)
Expand All @@ -110,14 +106,26 @@ func BenchmarkDatastoreDriver(b *testing.B) {
})
require.NoError(b, err)
var count int
for rel := iter.Next(); rel != nil; rel = iter.Next() {
for _, err := range iter {
require.NoError(b, err)
count++
}
require.NoError(b, iter.Err())
iter.Close()
require.Equal(b, usersPerDoc, count)
}
})
b.Run("SnapshotReadOnlyNamespace", func(b *testing.B) {
for n := 0; n < b.N; n++ {
iter, err := ds.SnapshotReader(headRev).QueryRelationships(ctx, datastore.RelationshipsFilter{
OptionalResourceType: testfixtures.DocumentNS.Name,
})
require.NoError(b, err)
var count int
for _, err := range iter {
require.NoError(b, err)
count++
}
}
})
b.Run("SortedSnapshotReadOnlyNamespace", func(b *testing.B) {
for orderName, order := range sortOrders {
order := order
Expand All @@ -128,11 +136,10 @@ func BenchmarkDatastoreDriver(b *testing.B) {
}, options.WithSort(order))
require.NoError(b, err)
var count int
for rel := iter.Next(); rel != nil; rel = iter.Next() {
for _, err := range iter {
require.NoError(b, err)
count++
}
require.NoError(b, iter.Err())
iter.Close()
}
})
}
Expand All @@ -148,11 +155,10 @@ func BenchmarkDatastoreDriver(b *testing.B) {
}, options.WithSort(order))
require.NoError(b, err)
var count int
for rel := iter.Next(); rel != nil; rel = iter.Next() {
for _, err := range iter {
require.NoError(b, err)
count++
}
require.NoError(b, iter.Err())
iter.Close()
}
})
}
Expand All @@ -170,11 +176,10 @@ func BenchmarkDatastoreDriver(b *testing.B) {
}, options.WithSort(order))
require.NoError(b, err)
var count int
for rel := iter.Next(); rel != nil; rel = iter.Next() {
for _, err := range iter {
require.NoError(b, err)
count++
}
require.NoError(b, iter.Err())
iter.Close()
}
})
}
Expand All @@ -186,32 +191,31 @@ func BenchmarkDatastoreDriver(b *testing.B) {
}, options.WithSortForReverse(options.ByResource))
require.NoError(b, err)
var count int
for rel := iter.Next(); rel != nil; rel = iter.Next() {
for _, err := range iter {
require.NoError(b, err)
count++
}
require.NoError(b, iter.Err())
iter.Close()
}
})
b.Run("Touch", buildTupleTest(ctx, ds, core.RelationTupleUpdate_TOUCH))
b.Run("Create", buildTupleTest(ctx, ds, core.RelationTupleUpdate_CREATE))
b.Run("Touch", buildRelTest(ctx, ds, tuple.UpdateOperationTouch))
b.Run("Create", buildRelTest(ctx, ds, tuple.UpdateOperationCreate))
b.Run("CreateAndTouch", func(b *testing.B) {
const totalRelationships = 1000
for _, portionCreate := range []float64{0, 0.10, 0.25, 0.50, 1} {
portionCreate := portionCreate
b.Run(fmt.Sprintf("%v_", portionCreate), func(b *testing.B) {
for n := 0; n < b.N; n++ {
portionCreateIndex := int(math.Floor(portionCreate * totalRelationships))
mutations := make([]*core.RelationTupleUpdate, 0, totalRelationships)
mutations := make([]tuple.RelationshipUpdate, 0, totalRelationships)
for index := 0; index < totalRelationships; index++ {
if index >= portionCreateIndex {
stableID := fmt.Sprintf("id-%d", index)
tpl := docViewer(stableID, stableID)
mutations = append(mutations, tuple.Touch(tpl))
rel := docViewer(stableID, stableID)
mutations = append(mutations, tuple.Touch(rel))
} else {
randomID := testfixtures.RandomObjectID(32)
tpl := docViewer(randomID, randomID)
mutations = append(mutations, tuple.Create(tpl))
rel := docViewer(randomID, randomID)
mutations = append(mutations, tuple.Create(rel))
}
}

Expand Down Expand Up @@ -244,15 +248,15 @@ func TestAllDriversBenchmarkedOrSkipped(t *testing.T) {
require.Empty(t, notBenchmarked)
}

func buildTupleTest(ctx context.Context, ds datastore.Datastore, op core.RelationTupleUpdate_Operation) func(b *testing.B) {
func buildRelTest(ctx context.Context, ds datastore.Datastore, op tuple.UpdateOperation) func(b *testing.B) {
return func(b *testing.B) {
for n := 0; n < b.N; n++ {
_, err := ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error {
randomID := testfixtures.RandomObjectID(32)
return rwt.WriteRelationships(ctx, []*core.RelationTupleUpdate{
return rwt.WriteRelationships(ctx, []tuple.RelationshipUpdate{
{
Operation: op,
Tuple: docViewer(randomID, randomID),
Operation: op,
Relationship: docViewer(randomID, randomID),
},
})
})
Expand All @@ -261,17 +265,19 @@ func buildTupleTest(ctx context.Context, ds datastore.Datastore, op core.Relatio
}
}

func docViewer(documentID, userID string) *core.RelationTuple {
return &core.RelationTuple{
ResourceAndRelation: &core.ObjectAndRelation{
Namespace: testfixtures.DocumentNS.Name,
ObjectId: documentID,
Relation: "viewer",
},
Subject: &core.ObjectAndRelation{
Namespace: testfixtures.UserNS.Name,
ObjectId: userID,
Relation: datastore.Ellipsis,
func docViewer(documentID, userID string) tuple.Relationship {
return tuple.Relationship{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have been great if we had a fluent API to create struct values

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add it, but it runs the risk of having "partial state" Relationships or needing a builder struct. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The risk is the same if you are creating a struct value directly. The struct builder will be in the stack and the overhead is minimal, and the impact on readability would be great - we do this a lot in the codebase. At the very least we could add the helper to use it in tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followup?

RelationshipReference: tuple.RelationshipReference{
Resource: tuple.ObjectAndRelation{
ObjectType: testfixtures.DocumentNS.Name,
ObjectID: documentID,
Relation: "viewer",
},
Subject: tuple.ObjectAndRelation{
ObjectType: testfixtures.UserNS.Name,
ObjectID: userID,
Relation: datastore.Ellipsis,
},
},
}
}
Loading
Loading