From 6390b785228c2d334fc1fdd7271e1fd24d6874e3 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 23 Feb 2024 12:43:41 -0800 Subject: [PATCH] Remove stale TODOs Most of the removed TODOs are changes that we won't end up making. In the case of the MySQL datastore, the Postgres datastore implementation from which it was copied has diverged significantly, so there isn't really a reason to try to refactor into a combined datastore anymore (since the PG one uses PG-specific code) --- internal/datasets/subjectsetbytype.go | 3 --- internal/datastore/common/sql.go | 2 -- internal/datastore/memdb/memdb.go | 1 - internal/datastore/mysql/datastore.go | 5 ----- internal/datastore/mysql/gc.go | 4 ---- internal/datastore/mysql/reader.go | 8 -------- internal/datastore/mysql/readwrite.go | 16 ++-------------- internal/datastore/mysql/revisions.go | 1 - internal/datastore/mysql/watch.go | 1 - internal/datastore/postgres/revisions.go | 3 +-- .../zz_migration.0001_initial_schema.go | 1 - internal/datastore/spanner/options.go | 1 - internal/dispatch/keys/keys.go | 2 -- internal/graph/context.go | 3 --- pkg/datastore/test/tuples.go | 4 ++-- 15 files changed, 5 insertions(+), 50 deletions(-) diff --git a/internal/datasets/subjectsetbytype.go b/internal/datasets/subjectsetbytype.go index 46c22a43b1..388eb08799 100644 --- a/internal/datasets/subjectsetbytype.go +++ b/internal/datasets/subjectsetbytype.go @@ -6,9 +6,6 @@ import ( "github.com/authzed/spicedb/pkg/tuple" ) -// TODO(jschorr): See if there is a nice way we can combine this withn ONRByTypeSet and the multimap -// used in Check to allow for a simple implementation. - // SubjectByTypeSet is a set of SubjectSet's, grouped by their subject types. type SubjectByTypeSet struct { byType map[string]SubjectSet diff --git a/internal/datastore/common/sql.go b/internal/datastore/common/sql.go index 8ff329dce0..2b85e36ba3 100644 --- a/internal/datastore/common/sql.go +++ b/internal/datastore/common/sql.go @@ -278,7 +278,6 @@ func (sqf SchemaQueryFilterer) MustFilterToResourceIDs(resourceIds []string) Sch // FilterToResourceIDs returns a new SchemaQueryFilterer that is limited to resources with any of the // specified IDs. func (sqf SchemaQueryFilterer) FilterToResourceIDs(resourceIds []string) (SchemaQueryFilterer, error) { - // TODO(jschorr): Change this panic into an automatic query split, if we find it necessary. if len(resourceIds) > int(datastore.FilterMaximumIDCount) { return sqf, spiceerrors.MustBugf("cannot have more than %d resources IDs in a single filter", datastore.FilterMaximumIDCount) } @@ -380,7 +379,6 @@ func (sqf SchemaQueryFilterer) FilterWithSubjectsSelectors(selectors ...datastor } if len(selector.OptionalSubjectIds) > 0 { - // TODO(jschorr): Change this panic into an automatic query split, if we find it necessary. if len(selector.OptionalSubjectIds) > int(datastore.FilterMaximumIDCount) { return sqf, spiceerrors.MustBugf("cannot have more than %d subject IDs in a single filter", datastore.FilterMaximumIDCount) } diff --git a/internal/datastore/memdb/memdb.go b/internal/datastore/memdb/memdb.go index 330b86a0eb..1e48ab3aa2 100644 --- a/internal/datastore/memdb/memdb.go +++ b/internal/datastore/memdb/memdb.go @@ -304,7 +304,6 @@ func (mdb *memdbDatastore) Close() error { mdb.Lock() defer mdb.Unlock() - // TODO Make this nil once we have removed all access to closed datastores if db := mdb.db; db != nil { mdb.revisions = []snapshot{ { diff --git a/internal/datastore/mysql/datastore.go b/internal/datastore/mysql/datastore.go index 714a61c5a1..0bb9b88b18 100644 --- a/internal/datastore/mysql/datastore.go +++ b/internal/datastore/mysql/datastore.go @@ -252,7 +252,6 @@ func newMySQLDatastore(ctx context.Context, uri string, options ...Option) (*Dat return store, nil } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor func (mds *Datastore) SnapshotReader(rev datastore.Revision) datastore.Reader { createTxFunc := func(ctx context.Context) (*sql.Tx, txCleanupFunc, error) { tx, err := mds.db.BeginTx(ctx, mds.readTxOptions) @@ -277,7 +276,6 @@ func (mds *Datastore) SnapshotReader(rev datastore.Revision) datastore.Reader { func noCleanup() error { return nil } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor // ReadWriteTx starts a read/write transaction, which will be committed if no error is // returned and rolled back if an error is returned. func (mds *Datastore) ReadWriteTx( @@ -457,7 +455,6 @@ type Datastore struct { // Close closes the data store. func (mds *Datastore) Close() error { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor mds.cancelGc() if mds.gcGroup != nil { if err := mds.gcGroup.Wait(); err != nil && !errors.Is(err, context.Canceled) { @@ -597,7 +594,6 @@ func (mds *Datastore) seedDatabase(ctx context.Context) error { }) } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor func buildLivingObjectFilterForRevision(revision datastore.Revision) queryFilterer { return func(original sq.SelectBuilder) sq.SelectBuilder { return original.Where(sq.LtOrEq{colCreatedTxn: revision.(revisions.TransactionIDRevision).TransactionID()}). @@ -608,7 +604,6 @@ func buildLivingObjectFilterForRevision(revision datastore.Revision) queryFilter } } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor func currentlyLivingObjects(original sq.SelectBuilder) sq.SelectBuilder { return original.Where(sq.Eq{colDeletedTxn: liveDeletedTxnID}) } diff --git a/internal/datastore/mysql/gc.go b/internal/datastore/mysql/gc.go index 2b9a35a03b..255a791a36 100644 --- a/internal/datastore/mysql/gc.go +++ b/internal/datastore/mysql/gc.go @@ -27,7 +27,6 @@ func (mds *Datastore) ResetGCCompleted() { mds.gcHasRun.Store(false) } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor func (mds *Datastore) Now(ctx context.Context) (time.Time, error) { // Retrieve the `now` time from the database. nowSQL, nowArgs, err := getNow.ToSql() @@ -46,7 +45,6 @@ func (mds *Datastore) Now(ctx context.Context) (time.Time, error) { return now.UTC(), nil } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor // - main difference is how the PSQL driver handles null values func (mds *Datastore) TxIDBefore(ctx context.Context, before time.Time) (datastore.Revision, error) { // Find the highest transaction ID before the GC window. @@ -69,7 +67,6 @@ func (mds *Datastore) TxIDBefore(ctx context.Context, before time.Time) (datasto return revisions.NewForTransactionID(uint64(value.Int64)), nil } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor // - implementation misses metrics func (mds *Datastore) DeleteBeforeTx( ctx context.Context, @@ -95,7 +92,6 @@ func (mds *Datastore) DeleteBeforeTx( return } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor // - query was reworked to make it compatible with Vitess // - API differences with PSQL driver func (mds *Datastore) batchDelete(ctx context.Context, tableName string, filter sqlFilter) (int64, error) { diff --git a/internal/datastore/mysql/reader.go b/internal/datastore/mysql/reader.go index d624f99d6d..24d0e2c049 100644 --- a/internal/datastore/mysql/reader.go +++ b/internal/datastore/mysql/reader.go @@ -35,7 +35,6 @@ const ( errUnableToQueryTuples = "unable to query tuples: %w" ) -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor var schema = common.NewSchemaInformation( colNamespace, colObjectID, @@ -52,7 +51,6 @@ func (mr *mysqlReader) QueryRelationships( filter datastore.RelationshipsFilter, opts ...options.QueryOptionsOption, ) (iter datastore.RelationshipIterator, err error) { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor qBuilder, err := common.NewSchemaQueryFilterer(schema, mr.filterer(mr.QueryTuplesQuery)).FilterWithRelationshipsFilter(filter) if err != nil { return nil, err @@ -66,7 +64,6 @@ func (mr *mysqlReader) ReverseQueryRelationships( subjectsFilter datastore.SubjectsFilter, opts ...options.ReverseQueryOptionsOption, ) (iter datastore.RelationshipIterator, err error) { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor qBuilder, err := common.NewSchemaQueryFilterer(schema, mr.filterer(mr.QueryTuplesQuery)). FilterWithSubjectsSelectors(subjectsFilter.AsSelector()) if err != nil { @@ -91,7 +88,6 @@ func (mr *mysqlReader) ReverseQueryRelationships( } func (mr *mysqlReader) ReadNamespaceByName(ctx context.Context, nsName string) (*core.NamespaceDefinition, datastore.Revision, error) { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor tx, txCleanup, err := mr.txSource(ctx) if err != nil { return nil, datastore.NoRevision, fmt.Errorf(errUnableToReadConfig, err) @@ -110,7 +106,6 @@ func (mr *mysqlReader) ReadNamespaceByName(ctx context.Context, nsName string) ( } func loadNamespace(ctx context.Context, namespace string, tx *sql.Tx, baseQuery sq.SelectBuilder) (*core.NamespaceDefinition, datastore.Revision, error) { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor ctx, span := tracer.Start(ctx, "loadNamespace") defer span.End() @@ -138,7 +133,6 @@ func loadNamespace(ctx context.Context, namespace string, tx *sql.Tx, baseQuery } func (mr *mysqlReader) ListAllNamespaces(ctx context.Context) ([]datastore.RevisionedNamespace, error) { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor tx, txCleanup, err := mr.txSource(ctx) if err != nil { return nil, err @@ -160,7 +154,6 @@ func (mr *mysqlReader) LookupNamespacesWithNames(ctx context.Context, nsNames [] return nil, nil } - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor tx, txCleanup, err := mr.txSource(ctx) if err != nil { return nil, err @@ -183,7 +176,6 @@ func (mr *mysqlReader) LookupNamespacesWithNames(ctx context.Context, nsNames [] } func loadAllNamespaces(ctx context.Context, tx *sql.Tx, queryBuilder sq.SelectBuilder) ([]datastore.RevisionedNamespace, error) { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor query, args, err := queryBuilder.ToSql() if err != nil { return nil, err diff --git a/internal/datastore/mysql/readwrite.go b/internal/datastore/mysql/readwrite.go index 500ef25d1f..411e150e5d 100644 --- a/internal/datastore/mysql/readwrite.go +++ b/internal/datastore/mysql/readwrite.go @@ -68,10 +68,6 @@ func (cc *caveatContextWrapper) Value() (driver.Value, error) { func (rwt *mysqlReadWriteTXN) WriteRelationships(ctx context.Context, mutations []*core.RelationTupleUpdate) error { // TODO(jschorr): Determine if we can do this in a more efficient manner using ON CONFLICT UPDATE // rather than SELECT FOR UPDATE as we've been doing. - // - // NOTE: There are some fundamental changes introduced to prevent a deadlock in MySQL vs the initial - // Postgres implementation from which this was copied. - bulkWrite := rwt.WriteTupleQuery bulkWriteHasValues := false @@ -218,7 +214,6 @@ func (rwt *mysqlReadWriteTXN) WriteRelationships(ctx context.Context, mutations } func (rwt *mysqlReadWriteTXN) DeleteRelationships(ctx context.Context, filter *v1.RelationshipFilter) error { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor // Add clauses for the ResourceFilter query := rwt.DeleteTupleQuery.Where(sq.Eq{colNamespace: filter.ResourceType}) if filter.OptionalResourceId != "" { @@ -254,8 +249,6 @@ func (rwt *mysqlReadWriteTXN) DeleteRelationships(ctx context.Context, filter *v } func (rwt *mysqlReadWriteTXN) WriteNamespaces(ctx context.Context, newNamespaces ...*core.NamespaceDefinition) error { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor - deletedNamespaceClause := sq.Or{} writeQuery := rwt.WriteNamespaceQuery @@ -296,8 +289,6 @@ func (rwt *mysqlReadWriteTXN) WriteNamespaces(ctx context.Context, newNamespaces } func (rwt *mysqlReadWriteTXN) DeleteNamespaces(ctx context.Context, nsNames ...string) error { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor - // For each namespace, check they exist and collect predicates for the // "WHERE" clause to delete the namespaces and associated tuples. nsClauses := make([]sq.Sqlizer, 0, len(nsNames)) @@ -311,13 +302,11 @@ func (rwt *mysqlReadWriteTXN) DeleteNamespaces(ctx context.Context, nsNames ...s // TODO(jzelinskie): return the name of the missing namespace return err case err == nil: - break + nsClauses = append(nsClauses, sq.Eq{colNamespace: nsName, colCreatedTxn: createdAt}) + tplClauses = append(tplClauses, sq.Eq{colNamespace: nsName}) default: return fmt.Errorf(errUnableToDeleteConfig, err) } - - nsClauses = append(nsClauses, sq.Eq{colNamespace: nsName, colCreatedTxn: createdAt}) - tplClauses = append(tplClauses, sq.Eq{colNamespace: nsName}) } delSQL, delArgs, err := rwt.DeleteNamespaceQuery. @@ -459,7 +448,6 @@ func convertToWriteConstraintError(err error) error { return nil } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor func exactRelationshipClause(r *core.RelationTuple) sq.Eq { return sq.Eq{ colNamespace: r.ResourceAndRelation.Namespace, diff --git a/internal/datastore/mysql/revisions.go b/internal/datastore/mysql/revisions.go index 8b86a66a80..5570e670a9 100644 --- a/internal/datastore/mysql/revisions.go +++ b/internal/datastore/mysql/revisions.go @@ -114,7 +114,6 @@ func (mds *Datastore) CheckRevision(ctx context.Context, revision datastore.Revi } func (mds *Datastore) loadRevision(ctx context.Context) (uint64, error) { - // TODO (@vroldanbet) dupe from postgres datastore - need to refactor // slightly changed to support no revisions at all, needed for runtime seeding of first transaction ctx, span := tracer.Start(ctx, "loadRevision") defer span.End() diff --git a/internal/datastore/mysql/watch.go b/internal/datastore/mysql/watch.go index e33d2a6791..170394a2a7 100644 --- a/internal/datastore/mysql/watch.go +++ b/internal/datastore/mysql/watch.go @@ -111,7 +111,6 @@ func (mds *Datastore) Watch(ctx context.Context, afterRevisionRaw datastore.Revi return updates, errs } -// TODO (@vroldanbet) dupe from postgres datastore - need to refactor func (mds *Datastore) loadChanges( ctx context.Context, afterRevision uint64, diff --git a/internal/datastore/postgres/revisions.go b/internal/datastore/postgres/revisions.go index 72506ecd68..01dfa21411 100644 --- a/internal/datastore/postgres/revisions.go +++ b/internal/datastore/postgres/revisions.go @@ -220,8 +220,7 @@ func parseRevisionDecimal(revisionStr string) (datastore.Revision, error) { return nil, fmt.Errorf("received revision delta in excess of that expected; are you sure you're not passing a ZedToken from an incompatible datastore?") } - // TODO(jschorr): Remove this deprecated code path at some point and maybe look into - // a more memory-efficient encoding of the XIP list if necessary. + // TODO(jschorr): Remove this deprecated code path once we have per-datastore-marked ZedTokens. xipList = make([]uint64, 0, xmax-xmin) for i := xmin; i < xid; i++ { xipList = append(xipList, i) diff --git a/internal/datastore/spanner/migrations/zz_migration.0001_initial_schema.go b/internal/datastore/spanner/migrations/zz_migration.0001_initial_schema.go index 3648f1678b..0d0043a57f 100644 --- a/internal/datastore/spanner/migrations/zz_migration.0001_initial_schema.go +++ b/internal/datastore/spanner/migrations/zz_migration.0001_initial_schema.go @@ -28,7 +28,6 @@ const ( version_num STRING(1024) NOT NULL ) PRIMARY KEY (version_num)` - // TODO see if we can make the operation smaller createChangelog = `CREATE TABLE changelog ( timestamp TIMESTAMP NOT NULL OPTIONS (allow_commit_timestamp=true), uuid STRING(36) NOT NULL, diff --git a/internal/datastore/spanner/options.go b/internal/datastore/spanner/options.go index 4e645448ae..06001501fa 100644 --- a/internal/datastore/spanner/options.go +++ b/internal/datastore/spanner/options.go @@ -72,7 +72,6 @@ func generateConfig(options []Option) (spannerOptions, error) { } // Run any checks on the config that need to be done - // TODO set a limit to revision quantization? if computed.revisionQuantization >= maxRevisionQuantization { return computed, fmt.Errorf( errQuantizationTooLarge, diff --git a/internal/dispatch/keys/keys.go b/internal/dispatch/keys/keys.go index 605975cacb..fa384eb626 100644 --- a/internal/dispatch/keys/keys.go +++ b/internal/dispatch/keys/keys.go @@ -118,8 +118,6 @@ func (c *CanonicalKeyHandler) CheckCacheKey(ctx context.Context, req *v1.Dispatc return emptyDispatchCacheKey, err } - // TODO(jschorr): Remove this conditional once we have a verified migration ordering system that ensures a backfill migration has - // run after the namespace annotation code has been fully deployed by users. if relation.CanonicalCacheKey != "" { return checkRequestToKeyWithCanonical(req, relation.CanonicalCacheKey) } diff --git a/internal/graph/context.go b/internal/graph/context.go index 1d4689c4a5..7dcec087ab 100644 --- a/internal/graph/context.go +++ b/internal/graph/context.go @@ -12,9 +12,6 @@ import ( // branchContext returns a context disconnected from the parent context, but populated with the datastore. // Also returns a function for canceling the newly created context, without canceling the parent context. func branchContext(ctx context.Context) (context.Context, func(cancelErr error)) { - // TODO(jschorr): Replace with https://pkg.go.dev/context@master#WithoutCancel once - // Go 1.21 lands. - // Add tracing to the context. span := trace.SpanFromContext(ctx) detachedContext := trace.ContextWithSpan(context.Background(), span) diff --git a/pkg/datastore/test/tuples.go b/pkg/datastore/test/tuples.go index a59364a245..9787f09e94 100644 --- a/pkg/datastore/test/tuples.go +++ b/pkg/datastore/test/tuples.go @@ -384,8 +384,8 @@ func DeleteRelationshipsTest(t *testing.T, tester DatastoreTester) { tRequire := testfixtures.TupleChecker{Require: require, DS: ds} - // TODO temporarily store tuples in multiple calls to ReadWriteTransaction since no Datastore - // handles correctly duplicate tuples + // NOTE: we write tuples in multiple calls to ReadWriteTransaction because it is not allowed to change + // the same tuple in the same transaction. _, err = ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error { for _, tpl := range tt.inputTuples { update := tuple.Touch(tpl)