Skip to content

Commit 7c53e96

Browse files
committed
fix: address PR authzed#2775 review feedback
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
1 parent 126c8ff commit 7c53e96

File tree

14 files changed

+92
-34
lines changed

14 files changed

+92
-34
lines changed

internal/datastore/common/errors.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,8 @@ type SchemaNotInitializedError struct {
184184
}
185185

186186
func (err SchemaNotInitializedError) GRPCStatus() *status.Status {
187+
// TODO: Create ERROR_REASON_DATASTORE_NOT_MIGRATED in authzed/api and use it here
188+
// See: https://github.com/authzed/spicedb/pull/2775
187189
return spiceerrors.WithCodeAndDetails(
188190
err,
189191
codes.FailedPrecondition,
@@ -202,6 +204,6 @@ func (err SchemaNotInitializedError) Unwrap() error {
202204
// instructing the user to run migrations.
203205
func NewSchemaNotInitializedError(underlying error) error {
204206
return SchemaNotInitializedError{
205-
fmt.Errorf("datastore error: the database schema has not been initialized; please run \"spicedb datastore migrate\": %w", underlying),
207+
fmt.Errorf("%w. please run \"spicedb datastore migrate\"", underlying),
206208
}
207209
}

internal/datastore/common/errors_test.go

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ func TestSchemaNotInitializedError(t *testing.T) {
1414

1515
t.Run("error message contains migration instructions", func(t *testing.T) {
1616
require.Contains(t, err.Error(), "spicedb datastore migrate")
17-
require.Contains(t, err.Error(), "database schema has not been initialized")
17+
// The error message now includes the underlying error first, followed by the instruction
18+
require.Contains(t, err.Error(), "relation \"caveat\" does not exist")
1819
})
1920

2021
t.Run("unwrap returns underlying error", func(t *testing.T) {
@@ -40,4 +41,15 @@ func TestSchemaNotInitializedError(t *testing.T) {
4041
var schemaErr SchemaNotInitializedError
4142
require.ErrorAs(t, wrappedErr, &schemaErr)
4243
})
44+
45+
t.Run("grpc status extractable from wrapped error", func(t *testing.T) {
46+
// This tests the scenario where SchemaNotInitializedError is wrapped
47+
// by another fmt.Errorf (e.g., in crdb/caveat.go). The gRPC library
48+
// uses errors.As to extract GRPCStatus from wrapped errors.
49+
wrappedErr := fmt.Errorf("outer context: %w", err)
50+
var schemaErr SchemaNotInitializedError
51+
require.ErrorAs(t, wrappedErr, &schemaErr)
52+
status := schemaErr.GRPCStatus()
53+
require.Equal(t, codes.FailedPrecondition, status.Code())
54+
})
4355
}

internal/datastore/crdb/caveat.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
sq "github.com/Masterminds/squirrel"
1010
"github.com/jackc/pgx/v5"
1111

12+
dscommon "github.com/authzed/spicedb/internal/datastore/common"
1213
"github.com/authzed/spicedb/internal/datastore/crdb/schema"
1314
pgxcommon "github.com/authzed/spicedb/internal/datastore/postgres/common"
1415
"github.com/authzed/spicedb/internal/datastore/revisions"
@@ -54,8 +55,8 @@ func (cr *crdbReader) ReadCaveatByName(ctx context.Context, name string) (*core.
5455
if errors.Is(err, pgx.ErrNoRows) {
5556
err = datastore.NewCaveatNameNotFoundErr(name)
5657
}
57-
if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil {
58-
return nil, datastore.NoRevision, wrappedErr
58+
if pgxcommon.IsMissingTableError(err) {
59+
err = dscommon.NewSchemaNotInitializedError(err)
5960
}
6061
return nil, datastore.NoRevision, fmt.Errorf(errReadCaveat, name, err)
6162
}
@@ -113,8 +114,8 @@ func (cr *crdbReader) lookupCaveats(ctx context.Context, caveatNames []string) (
113114
return nil
114115
}, sql, args...)
115116
if err != nil {
116-
if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil {
117-
return nil, wrappedErr
117+
if pgxcommon.IsMissingTableError(err) {
118+
err = dscommon.NewSchemaNotInitializedError(err)
118119
}
119120
return nil, fmt.Errorf(errListCaveats, err)
120121
}

internal/datastore/crdb/crdb.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,11 @@ func (cds *crdbDatastore) features(ctx context.Context) (*datastore.Features, er
573573
}
574574

575575
features.Watch.Status = datastore.FeatureUnsupported
576-
features.Watch.Reason = "Range feeds must be enabled in CockroachDB and the user must have permission to create them in order to enable the Watch API: " + err.Error()
576+
if pgxcommon.IsMissingTableError(err) {
577+
features.Watch.Reason = "Database schema has not been initialized. Please run \"spicedb datastore migrate\": " + err.Error()
578+
} else {
579+
features.Watch.Reason = "Range feeds must be enabled in CockroachDB and the user must have permission to create them in order to enable the Watch API: " + err.Error()
580+
}
577581
return nil
578582
}, fmt.Sprintf(cds.beginChangefeedQuery, cds.schema.RelationshipTableName, head, "-1s"))
579583
} else {

internal/datastore/crdb/migrations/driver.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ import (
1515
const (
1616
errUnableToInstantiate = "unable to instantiate CRDBDriver: %w"
1717

18-
postgresMissingTableErrorCode = "42P01"
19-
2018
queryLoadVersion = "SELECT version_num from schema_version"
2119
queryWriteVersion = "UPDATE schema_version SET version_num=$1 WHERE version_num=$2"
2220
)
@@ -52,7 +50,7 @@ func (apd *CRDBDriver) Version(ctx context.Context) (string, error) {
5250

5351
if err := apd.db.QueryRow(ctx, queryLoadVersion).Scan(&loaded); err != nil {
5452
var pgErr *pgconn.PgError
55-
if errors.As(err, &pgErr) && pgErr.Code == postgresMissingTableErrorCode {
53+
if errors.As(err, &pgErr) && pgErr.Code == pgxcommon.PgMissingTable {
5654
return "", nil
5755
}
5856
return "", fmt.Errorf("unable to load alembic revision: %w", err)

internal/datastore/postgres/common/bulk.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"github.com/ccoveille/go-safecast/v2"
77
"github.com/jackc/pgx/v5"
88

9+
dscommon "github.com/authzed/spicedb/internal/datastore/common"
910
"github.com/authzed/spicedb/pkg/datastore"
1011
"github.com/authzed/spicedb/pkg/spiceerrors"
1112
"github.com/authzed/spicedb/pkg/tuple"
@@ -77,9 +78,15 @@ func BulkLoad(
7778
colNames: colNames,
7879
}
7980
copied, err := tx.CopyFrom(ctx, pgx.Identifier{tupleTableName}, colNames, adapter)
81+
if err != nil {
82+
if IsMissingTableError(err) {
83+
return 0, dscommon.NewSchemaNotInitializedError(err)
84+
}
85+
return 0, err
86+
}
8087
uintCopied, castErr := safecast.Convert[uint64](copied)
8188
if castErr != nil {
8289
return 0, spiceerrors.MustBugf("number copied was negative: %v", castErr)
8390
}
84-
return uintCopied, err
91+
return uintCopied, nil
8592
}

internal/datastore/postgres/common/errors.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ const (
1919
pgReadOnlyTransaction = "25006"
2020
pgQueryCanceled = "57014"
2121
pgInvalidArgument = "22023"
22-
pgMissingTable = "42P01"
22+
23+
// PgMissingTable is the Postgres error code for "relation does not exist".
24+
// This is used to detect when migrations have not been run.
25+
PgMissingTable = "42P01"
2326
)
2427

2528
var (
@@ -112,7 +115,7 @@ func ConvertToWriteConstraintError(livingTupleConstraints []string, err error) e
112115
// This typically happens when migrations have not been run.
113116
func IsMissingTableError(err error) bool {
114117
var pgerr *pgconn.PgError
115-
return errors.As(err, &pgerr) && pgerr.Code == pgMissingTable
118+
return errors.As(err, &pgerr) && pgerr.Code == PgMissingTable
116119
}
117120

118121
// WrapMissingTableError checks if the error is a missing table error and wraps it with

internal/datastore/postgres/common/errors_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import (
1313
func TestIsMissingTableError(t *testing.T) {
1414
t.Run("returns true for missing table error", func(t *testing.T) {
1515
pgErr := &pgconn.PgError{
16-
Code: pgMissingTable,
16+
Code: PgMissingTable,
1717
Message: "relation \"caveat\" does not exist",
1818
}
1919
require.True(t, IsMissingTableError(pgErr))
@@ -38,7 +38,7 @@ func TestIsMissingTableError(t *testing.T) {
3838

3939
t.Run("returns true for wrapped missing table error", func(t *testing.T) {
4040
pgErr := &pgconn.PgError{
41-
Code: pgMissingTable,
41+
Code: PgMissingTable,
4242
Message: "relation \"caveat\" does not exist",
4343
}
4444
wrappedErr := fmt.Errorf("query failed: %w", pgErr)
@@ -49,7 +49,7 @@ func TestIsMissingTableError(t *testing.T) {
4949
func TestWrapMissingTableError(t *testing.T) {
5050
t.Run("wraps missing table error", func(t *testing.T) {
5151
pgErr := &pgconn.PgError{
52-
Code: pgMissingTable,
52+
Code: PgMissingTable,
5353
Message: "relation \"caveat\" does not exist",
5454
}
5555
wrapped := WrapMissingTableError(pgErr)
@@ -79,7 +79,7 @@ func TestWrapMissingTableError(t *testing.T) {
7979

8080
t.Run("preserves original error in chain", func(t *testing.T) {
8181
pgErr := &pgconn.PgError{
82-
Code: pgMissingTable,
82+
Code: PgMissingTable,
8383
Message: "relation \"caveat\" does not exist",
8484
}
8585
wrapped := WrapMissingTableError(pgErr)
@@ -88,12 +88,12 @@ func TestWrapMissingTableError(t *testing.T) {
8888
// The original postgres error should be accessible via unwrapping
8989
var foundPgErr *pgconn.PgError
9090
require.ErrorAs(t, wrapped, &foundPgErr)
91-
require.Equal(t, pgMissingTable, foundPgErr.Code)
91+
require.Equal(t, PgMissingTable, foundPgErr.Code)
9292
})
9393

9494
t.Run("does not double-wrap already wrapped errors", func(t *testing.T) {
9595
pgErr := &pgconn.PgError{
96-
Code: pgMissingTable,
96+
Code: PgMissingTable,
9797
Message: "relation \"caveat\" does not exist",
9898
}
9999
// First wrap

internal/datastore/postgres/migrations/driver.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ import (
1515
"github.com/authzed/spicedb/pkg/migrate"
1616
)
1717

18-
const postgresMissingTableErrorCode = "42P01"
19-
2018
var tracer = otel.Tracer("spicedb/internal/datastore/common")
2119

2220
// AlembicPostgresDriver implements a schema migration facility for use in
@@ -74,7 +72,7 @@ func (apd *AlembicPostgresDriver) Version(ctx context.Context) (string, error) {
7472

7573
if err := apd.db.QueryRow(ctx, "SELECT version_num from alembic_version").Scan(&loaded); err != nil {
7674
var pgErr *pgconn.PgError
77-
if errors.As(err, &pgErr) && pgErr.Code == postgresMissingTableErrorCode {
75+
if errors.As(err, &pgErr) && pgErr.Code == pgxcommon.PgMissingTable {
7876
return "", nil
7977
}
8078
return "", fmt.Errorf("unable to load alembic revision: %w", err)

internal/datastore/postgres/reader.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,8 @@ func (r *pgReader) CountRelationships(ctx context.Context, name string) (int, er
8484
return rows.Err()
8585
}, sql, args...)
8686
if err != nil {
87-
if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil {
88-
return 0, wrappedErr
87+
if pgxcommon.IsMissingTableError(err) {
88+
err = common.NewSchemaNotInitializedError(err)
8989
}
9090
return 0, err
9191
}
@@ -142,8 +142,8 @@ func (r *pgReader) lookupCounters(ctx context.Context, optionalName string) ([]d
142142
return rows.Err()
143143
}, sql, args...)
144144
if err != nil {
145-
if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil {
146-
return nil, wrappedErr
145+
if pgxcommon.IsMissingTableError(err) {
146+
err = common.NewSchemaNotInitializedError(err)
147147
}
148148
return nil, fmt.Errorf("unable to query counters: %w", err)
149149
}
@@ -213,8 +213,8 @@ func (r *pgReader) ReadNamespaceByName(ctx context.Context, nsName string) (*cor
213213
case err == nil:
214214
return loaded, version, nil
215215
default:
216-
if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil {
217-
return nil, datastore.NoRevision, wrappedErr
216+
if pgxcommon.IsMissingTableError(err) {
217+
err = common.NewSchemaNotInitializedError(err)
218218
}
219219
return nil, datastore.NoRevision, fmt.Errorf(errUnableToReadConfig, err)
220220
}
@@ -241,8 +241,8 @@ func (r *pgReader) loadNamespace(ctx context.Context, namespace string, tx pgxco
241241
func (r *pgReader) ListAllNamespaces(ctx context.Context) ([]datastore.RevisionedNamespace, error) {
242242
nsDefsWithRevisions, err := loadAllNamespaces(ctx, r.query, r.aliveFilter)
243243
if err != nil {
244-
if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil {
245-
return nil, wrappedErr
244+
if pgxcommon.IsMissingTableError(err) {
245+
err = common.NewSchemaNotInitializedError(err)
246246
}
247247
return nil, fmt.Errorf(errUnableToListNamespaces, err)
248248
}
@@ -264,8 +264,8 @@ func (r *pgReader) LookupNamespacesWithNames(ctx context.Context, nsNames []stri
264264
return r.aliveFilter(original).Where(clause)
265265
})
266266
if err != nil {
267-
if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil {
268-
return nil, wrappedErr
267+
if pgxcommon.IsMissingTableError(err) {
268+
err = common.NewSchemaNotInitializedError(err)
269269
}
270270
return nil, fmt.Errorf(errUnableToListNamespaces, err)
271271
}

0 commit comments

Comments
 (0)