Merge pull request #1764 from josephschorr/remove-stale-todos

Remove stale TODOs

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

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)
 			}

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{
 			{

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})
 }

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) {

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

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,

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()

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,

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)

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,

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,

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)
 		}

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)

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)