Prepare for single stored schema #2805

josephschorr · 2026-01-06T21:31:32Z

Refactors the code in preparation for storing schema as a singleton and accessing it in that manner

Testing

Unit tests

…aces

…gacy Will allow for easy replacement in the followup commit

…n the legacy methods

codecov · 2026-01-06T21:40:14Z

Codecov Report

❌ Patch coverage is 60.71019% with 343 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.57%. Comparing base (8a6d4a9) to head (12ff6ac).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/datastore/mocks/mock_datastore.go	0.00%	154 Missing ⚠️
internal/datastore/schema/schema.go	62.58%	48 Missing and 13 partials ⚠️
internal/datastore/proxy/proxy_test/mock.go	25.00%	24 Missing ⚠️
internal/datastore/proxy/strictreplicated.go	10.00%	18 Missing ⚠️
internal/services/shared/schema.go	63.34%	7 Missing and 4 partials ⚠️
internal/datastore/proxy/checkingreplicated.go	37.50%	10 Missing ⚠️
pkg/schema/resolver.go	60.00%	4 Missing and 4 partials ⚠️
internal/namespace/util.go	72.73%	3 Missing and 3 partials ⚠️
pkg/datastore/errors.go	33.34%	6 Missing ⚠️
internal/testfixtures/validating.go	76.20%	5 Missing ⚠️
... and 18 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2805      +/-   ##
==========================================
- Coverage   77.71%   77.57%   -0.14%     
==========================================
  Files         472      473       +1     
  Lines       49709    50047     +338     
==========================================
+ Hits        38627    38819     +192     
- Misses       8231     8363     +132     
- Partials     2851     2865      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tstirrat15

LGTM, see comments

tstirrat15 · 2026-01-06T22:41:04Z

pkg/datastore/schema.go

+	// AddDefinitionsForTesting adds or overwrites the given schema definitions. This method is
+	// only for use in testing and requires a testing.TB instance to enforce this constraint.
+	AddDefinitionsForTesting(ctx context.Context, tb testing.TB, definitions ...SchemaDefinition) error


This seems like a sane way to do this that i wouldn't have thought of. Is there a reason that it's a part of the interface rather than something that each concrete type defines its own instance of?

So that tests can operate on it without having to know to which impl they are talking, e.g. for the generic datastore tests

tstirrat15 · 2026-01-06T22:42:31Z

pkg/datastore/legacyschema.go

+// RevisionedCaveat is a revisioned version of a caveat definition.
+type RevisionedCaveat = RevisionedDefinition[*core.CaveatDefinition]


Why is this one in legacy but the other one is in schema? Will caveats be handled differently under the new system?

History - I plan to move over the legacy one after this PR when the new impl goes in

tstirrat15 · 2026-01-06T22:42:58Z

pkg/datastore/errors.go

+func NewSchemaNotDefinedErr() error {
+	return SchemaNotDefinedError{
+		error: errors.New("no schema has been defined; please call WriteSchema to start"),
+	}
+}


tstirrat15 · 2026-01-06T22:50:41Z

internal/services/v1/schema.go

 		return nil, ss.rewriteError(ctx, err)
 	}

-	caveatDefs, err := reader.ListAllCaveats(ctx)


I like the way this simplifies (knowing that there's more of this to come)

tstirrat15 · 2026-01-06T22:59:08Z

internal/services/v1/permissions.go

+				for _, def := range foundDefs {
+					if nsDef, ok := def.(*core.NamespaceDefinition); ok {
+						newDef, err := schema.NewDefinition(ts, nsDef)
+						if err != nil {
+							return err
+						}
+						loadedNamespaces[nsDef.Name] = newDef
+					} else if caveatDef, ok := def.(*core.CaveatDefinition); ok {
+						loadedCaveats[caveatDef.Name] = caveatDef
+					}


This probably isn't a "now" problem, but if we're going to upstream things like partials into the main schema DSL, will this logic need to change? Or are we only storing materialized definitions in the database and we'll say that users can't write a schema with partials?

We're not going to allow partials in "final" schemas - it will have to be complied before hand

tstirrat15 · 2026-01-06T22:59:58Z

internal/services/shared/schema.go

-		// as that is handled by the ensureNoRelationshipsExistWithResourceType call above.
-		if removedObjectDefNames.Len() > 0 {
-			if err := rwt.DeleteNamespaces(ctx, removedObjectDefNames.AsSlice(), datastore.DeleteNamespacesOnly); err != nil {
+		// Write the new/changes caveats.


Suggested change

// Write the new/changes caveats.

// Write the new/changed caveats.

tstirrat15 · 2026-01-06T23:00:47Z

internal/services/shared/schema.go

-	}
+	if validated.additiveOnly {
+		// DEPRECATED: Use of legacy methods for additive-only schema changes is deprecated.
+		// This path is maintained for backwards compatibility but will be removed in a future version.


What's the backwards compatibility in question here?

tstirrat15 · 2026-01-06T23:04:56Z

internal/datastore/schema/schema.go

+	// Check if there is any schema defined
+	if len(namespaces) == 0 {
+		return "", datastore.NewSchemaNotDefinedErr()
+	}


Nit, but this can go above the above block

tstirrat15 · 2026-01-06T23:06:47Z

internal/datastore/schema/schema.go

+// future use but is currently ignored by implementations. The method validates that no
+// definition names overlap, loads existing definitions, replaces changed ones, and deletes
+// definitions no longer present.


Is it implicit that all of this is happening within the same transaction?

The writer is defined on the transaction, so yes

tstirrat15

LGTM

josephschorr added 3 commits December 30, 2025 16:29

refactor: move all current schema functions into legacy schema interf…

48d6aa2

…aces

chore: rename all the legacy schema accessor methods to start with Le…

30ba2af

…gacy Will allow for easy replacement in the followup commit

refactor: Change schema reads to use the new schema reader rather tha…

cead21d

…n the legacy methods

josephschorr requested a review from a team as a code owner January 6, 2026 21:31

josephschorr requested review from barakmich and removed request for a team January 6, 2026 21:31

github-actions bot added area/cli Affects the command line area/api v1 Affects the v1 API area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) area/dispatch Affects dispatching of requests labels Jan 6, 2026

tstirrat15 previously approved these changes Jan 6, 2026

View reviewed changes

refactor: change schema writers to use a new schema writer

12ff6ac

josephschorr dismissed tstirrat15’s stale review via 12ff6ac January 7, 2026 17:00

josephschorr force-pushed the schema-refactor branch from fd8f3a7 to 12ff6ac Compare January 7, 2026 17:00

tstirrat15 approved these changes Jan 7, 2026

View reviewed changes

		// RevisionedCaveat is a revisioned version of a caveat definition.
		type RevisionedCaveat = RevisionedDefinition[*core.CaveatDefinition]

	// Write the new/changes caveats.
	// Write the new/changed caveats.

Prepare for single stored schema #2805

Are you sure you want to change the base?

Prepare for single stored schema #2805

Conversation

josephschorr commented Jan 6, 2026

Testing

Uh oh!

codecov bot commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tstirrat15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstirrat15 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jan 6, 2026 •

edited

Loading