jb/shared schemas - #6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces shared schemas: a schema can be stored once and assigned to multiple tenants, reducing duplication and enabling shared-versioning across tenants.
Changes:
- Adds a new
SharedSchemagRPC/HTTP service and corresponding proto messages for write/read/assign/list operations. - Extends schema resolution to support shared schemas by adding
shared_schema_idto relevant request metadata and updating storage interfaces/implementations (Postgres + Memory) and schema-reader proxies. - Updates API docs/OpenAPI artifacts, generated protobuf outputs, and release/version strings.
Reviewed changes
Copilot reviewed 59 out of 60 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| proto/base/v1/service.proto | Adds shared_schema_id to several request metadata messages and defines the new SharedSchema service/messages. |
| proto/base/v1/openapi.proto | Bumps documented API version string. |
| proto/base/v1/base.proto | Adds shared_schema_id to Tenant. |
| pkg/pb/base/v1/service.pb.gw.go | Regenerates grpc-gateway handlers for SharedSchema endpoints. |
| pkg/pb/base/v1/service_grpc.pb.go | Regenerates gRPC service/client/server stubs for SharedSchema. |
| pkg/pb/base/v1/openapi.pb.go | Regenerates OpenAPI proto descriptor output (version string change). |
| pkg/pb/base/v1/base.pb.validate.go | Regenerates validation code for Tenant (includes new field). |
| pkg/pb/base/v1/base.pb.go | Regenerates protobuf Go types for Tenant.shared_schema_id. |
| pkg/pb/base/v1/base_vtproto.pb.go | Regenerates vtproto code to include Tenant.shared_schema_id. |
| pkg/development/development.go | Adapts dev container and schema reads to new schema-reader signatures. |
| pkg/cmd/validate.go | Updates schema-reader calls to new signature. |
| pkg/cmd/serve.go | Wires shared-schema reader/writer factories into the server container. |
| internal/storage/storage.go | Extends SchemaReader interface for shared schema ID + adds shared-schema reader/writer interfaces and noops. |
| internal/storage/proxies/singleflight/schema_reader.go | Updates singleflight schema reader wrapper for new interface + shared schema head-version result. |
| internal/storage/proxies/singleflight/schema_reader_test.go | Updates tests to new schema-reader signatures and head-version return values. |
| internal/storage/proxies/circuitbreaker/schema_reader.go | Updates circuit-breaker schema reader wrapper for new interface. |
| internal/storage/proxies/cache/schema_reader.go | Updates cache proxy schema reader wrapper; adds cache keying that can share cache entries across tenants via shared schema ID. |
| internal/storage/postgres/tenant_reader.go | Includes shared_schema_id in tenant listing reads. |
| internal/storage/postgres/shared_schema_writer.go | Adds Postgres shared-schema write + tenant assignment implementation. |
| internal/storage/postgres/shared_schema_reader.go | Adds Postgres shared-schema read/list/head-version/tenant-assignment lookups. |
| internal/storage/postgres/schema_writer.go | Clears tenants.shared_schema_id when writing a per-tenant schema. |
| internal/storage/postgres/schema_writer_test.go | Updates tests to new ReadSchema signature. |
| internal/storage/postgres/schema_reader.go | Extends schema reads to support shared schema ID and resolves shared vs per-tenant head version. |
| internal/storage/postgres/schema_reader_test.go | Updates tests for new signatures and head-version return values. |
| internal/storage/postgres/migrations/20260707000000_shared_schemas.sql | Adds shared_schema_definitions table and tenants.shared_schema_id column. |
| internal/storage/postgres/consts.go | Adds table name constant for shared_schema_definitions. |
| internal/storage/model.go | Adds shared schema definition model + tenant model now carries shared schema ID. |
| internal/storage/memory/shared_schema_writer.go | Adds memory shared-schema write + tenant assignment implementation. |
| internal/storage/memory/shared_schema_writer_test.go | Adds tests for memory shared-schema writer behavior. |
| internal/storage/memory/shared_schema_reader.go | Adds memory shared-schema read/list/head-version/tenant-assignment lookups. |
| internal/storage/memory/shared_schema_reader_test.go | Adds tests for memory shared-schema reader behavior (including tenant assignment + head-version behavior). |
| internal/storage/memory/schema_writer.go | Clears tenant shared schema assignment when writing per-tenant schema (memory). |
| internal/storage/memory/schema_writer_test.go | Updates tests to new ReadSchema signature. |
| internal/storage/memory/schema_reader.go | Extends schema reads to support shared schema ID and resolves shared vs per-tenant head version. |
| internal/storage/memory/schema_reader_test.go | Updates tests for new signatures and head-version return values. |
| internal/storage/memory/migrations/schema.go | Adds memory DB table/indexes for shared schema definitions. |
| internal/storage/memory/constants/constants.go | Adds memory table constant for shared schema definitions. |
| internal/servers/shared_schema_server.go | Implements new SharedSchema gRPC server (write/read/assign/list). |
| internal/servers/server.go | Wires shared-schema server into gRPC + HTTP gateway registration and adds SSR/SSW to the container. |
| internal/servers/schema_server.go | Updates schema server reads/lists/partial writes to use new schema-reader signatures. |
| internal/servers/data_server.go | Updates data write paths to pass shared schema ID into schema reads. |
| internal/invoke/invoke.go | Populates request metadata with shared schema ID when defaulting schema version via HeadVersion. |
| internal/invoke/batch.go | Preserves shared_schema_id when cloning batch check requests with different depth. |
| internal/info.go | Bumps internal version string. |
| internal/factories/storage.go | Adds factories for shared-schema reader/writer by engine type. |
| internal/engines/subject_permission.go | Passes shared_schema_id through to schema reads and nested check metadata. |
| internal/engines/subject_filter.go | Passes shared_schema_id through to schema reads. |
| internal/engines/lookup.go | Updates schema caching + reads to include shared schema ID and passes it through to nested requests. |
| internal/engines/lookup_test.go | Updates mock schema reader interface to match new signatures. |
| internal/engines/expand.go | Passes shared_schema_id through to schema reads. |
| internal/engines/entity_filter.go | Preserves shared_schema_id in emitted metadata. |
| internal/engines/check.go | Passes shared_schema_id through to schema reads. |
| internal/engines/cache/check.go | Passes shared_schema_id through to schema reads. |
| internal/engines/balancer/balancer.go | Passes shared_schema_id through to schema reads. |
| docs/api-reference/openapiv2/apidocs.swagger.json | Updates swagger docs for shared schema endpoints and new fields; bumps version string. |
| docs/api-reference/apidocs.swagger.json | Updates swagger docs for shared schema endpoints and new fields; bumps version string. |
| .github/workflows/release.yml | Updates workflow input description example for jb tag format. |
Suppressed comments (2)
internal/servers/data_server.go:272
- Same issue as
Write: ifschema_versionis provided butshared_schema_idis empty, shared-schema tenants will try to validate tuples/attributes against a per-tenant schema and may fail incorrectly. Consider resolvingsharedSchemaIDfromSchemaReader.HeadVersionwhenever eitherschema_versionorshared_schema_idis missing, while only defaultingschema_versionwhen it’s empty.
version := request.GetMetadata().GetSchemaVersion()
sharedSchemaID := request.GetMetadata().GetSharedSchemaId()
if version == "" {
var err error
sharedSchemaID, version, err = r.sr.HeadVersion(ctx, request.GetTenantId())
if err != nil {
span.RecordError(err)
span.SetStatus(otelCodes.Error, err.Error())
internal/invoke/invoke.go:156
- If a caller supplies
schema_versionexplicitly but leavesshared_schema_idempty, downstream engines will read per-tenant schema definitions and can fail for shared-schema tenants. Consider also populatingSharedSchemaIdwhen it’s missing (and only defaultSchemaVersionwhen it’s missing).
// Set the SchemaVersion if it's not provided in the request.
if request.Metadata.GetSchemaVersion() == "" {
request.Metadata.SharedSchemaId, request.Metadata.SchemaVersion, err = invoker.schemaReader.HeadVersion(ctx, request.TenantID)
if err != nil {
span.RecordError(err)
span.SetStatus(otelCodes.Error, err.Error())
span.SetAttributes(attribute.KeyValue{Key: "can", Value: attribute.StringValue(base.CheckResult_CHECK_RESULT_DENIED.String())})
return denied, err
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 61 changed files in this pull request and generated 2 comments.
Suppressed comments (10)
internal/servers/data_server.go:172
- shared_schema_id is taken directly from the request metadata, but the storage layer treats a non-empty sharedSchemaID as authoritative (it switches to shared_schema_definitions). This means clients can (a) break tenants that use shared schemas by omitting shared_schema_id when providing schema_version, and (b) potentially force schema evaluation against a schema the tenant is not assigned to. Resolve sharedSchemaID from tenant assignment (HeadVersion) and only use request schema_version to select the version.
version := request.GetMetadata().GetSchemaVersion()
sharedSchemaID := request.GetMetadata().GetSharedSchemaId()
if version == "" {
var err error
sharedSchemaID, version, err = r.sr.HeadVersion(ctx, request.GetTenantId())
internal/servers/data_server.go:269
- Same issue as DataServer.Write: shared_schema_id is trusted from request metadata. If a tenant is assigned a shared schema and the client supplies schema_version but omits shared_schema_id, schema lookups will incorrectly fall back to per-tenant schema tables. Resolve sharedSchemaID from tenant assignment and only default the version from HeadVersion when not provided.
version := request.GetMetadata().GetSchemaVersion()
sharedSchemaID := request.GetMetadata().GetSharedSchemaId()
if version == "" {
var err error
sharedSchemaID, version, err = r.sr.HeadVersion(ctx, request.GetTenantId())
internal/invoke/invoke.go:202
- SharedSchemaId is only populated when SchemaVersion is empty. If callers set schema_version for a tenant that uses shared schemas but omit shared_schema_id, schema lookups will incorrectly use per-tenant schemas. Populate SharedSchemaId whenever missing, and only default SchemaVersion when missing.
if request.GetMetadata().GetSchemaVersion() == "" {
request.Metadata.SharedSchemaId, request.Metadata.SchemaVersion, err = invoker.schemaReader.HeadVersion(ctx, request.GetTenantId())
if err != nil {
span.RecordError(err)
span.SetStatus(otelCodes.Error, err.Error())
internal/invoke/invoke.go:239
- LookupEntity only resolves shared schema routing when schema_version is empty. For tenants assigned a shared schema, requests with an explicit schema_version will mis-route to per-tenant schemas unless clients also set shared_schema_id. Populate SharedSchemaId when missing and only default SchemaVersion when missing.
if request.GetMetadata().GetSchemaVersion() == "" { // Check if the request has a SchemaVersion.
request.Metadata.SharedSchemaId, request.Metadata.SchemaVersion, err = invoker.schemaReader.HeadVersion(ctx, request.GetTenantId()) // Retrieve the head schema version from the schema reader.
if err != nil {
span.RecordError(err)
span.SetStatus(otelCodes.Error, err.Error())
internal/invoke/invoke.go:279
- LookupEntityStream resolves shared schema routing only when schema_version is empty. For tenants assigned a shared schema, callers providing schema_version but omitting shared_schema_id will cause schema reads to fall back to per-tenant tables. Populate SharedSchemaId whenever missing and only default SchemaVersion when missing.
// Set SchemaVersion if not provided
if request.GetMetadata().GetSchemaVersion() == "" { // Check if the request has a SchemaVersion.
request.Metadata.SharedSchemaId, request.Metadata.SchemaVersion, err = invoker.schemaReader.HeadVersion(ctx, request.GetTenantId()) // Retrieve the head schema version from the schema reader.
if err != nil {
span.RecordError(err)
internal/invoke/invoke.go:323
- LookupSubject only sets SharedSchemaId when schema_version is empty. For tenants assigned shared schemas, requests with explicit schema_version will mis-route schema reads unless shared_schema_id is also set. Populate SharedSchemaId whenever missing and only default SchemaVersion when missing.
// Similar to SnapToken, check if the request has a SchemaVersion. If not, a SchemaVersion is set.
if request.GetMetadata().GetSchemaVersion() == "" {
// Retrieve the head schema version from the schema reader
request.Metadata.SharedSchemaId, request.Metadata.SchemaVersion, err = invoker.schemaReader.HeadVersion(ctx, request.GetTenantId())
// If there's an error retrieving the schema version, return the response and the error
internal/invoke/invoke.go:370
- SubjectPermission only sets SharedSchemaId when schema_version is empty. For tenants assigned shared schemas, providing an explicit schema_version without shared_schema_id can cause schema reads to hit per-tenant tables. Populate SharedSchemaId whenever missing and only default SchemaVersion when missing.
// Similar to SnapToken, check if the request has a SchemaVersion. If not, a SchemaVersion is set.
if request.GetMetadata().GetSchemaVersion() == "" {
// Retrieve the head schema version from the schema reader
request.Metadata.SharedSchemaId, request.Metadata.SchemaVersion, err = invoker.schemaReader.HeadVersion(ctx, request.GetTenantId())
// If there's an error retrieving the schema version, return the response and the error
internal/servers/schema_server.go:214
- SchemaServer.Read only resolves sharedSchemaID when schema_version is empty. If a tenant is assigned a shared schema and the caller provides an explicit schema_version, sharedSchemaID stays empty and ReadSchema will query per-tenant schema_definitions instead of shared_schema_definitions (likely returning empty/incorrect results). Consider resolving the tenant’s sharedSchemaID independent of whether schema_version was provided (e.g., via a dedicated storage method or by wiring SharedSchemaReader into SchemaServer).
version := request.GetMetadata().GetSchemaVersion()
var sharedSchemaID string
if version == "" {
var err error
sharedSchemaID, version, err = r.sr.HeadVersion(ctx, request.GetTenantId())
internal/storage/memory/shared_schema_reader.go:201
- ListSharedSchemas silently skips entries if the stored head version is not a valid xid. This can create pagination gaps/duplicates and hides corrupted state. It should return an error (similar to memory.SchemaReader.ListSchemas) so callers can detect and remediate the issue.
id, parseErr := xid.FromString(e.version)
if parseErr != nil {
continue
}
internal/storage/postgres/shared_schema_reader.go:41
- Postgres implementations for shared schema read/list/head-version and tenant assignment are newly introduced but there are no corresponding postgres storage tests (unlike schema_reader/schema_writer). Adding integration tests would help catch query/pagination/version-order edge cases and prevent regressions.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 61 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
internal/storage/memory/shared_schema_writer.go:32
- If
definitionsis empty, this function inserts nothing but still updatessharedHeadVersion[""], which corrupts the in-memory head-version index. Return an InvalidArgument error when there is nothing to write.
func (w *SharedSchemaWriter) WriteSharedSchema(_ context.Context, definitions []storage.SharedSchemaDefinition) error {
txn := w.database.DB.Txn(true)
defer txn.Abort()
var sharedSchemaID string
var version string
internal/storage/postgres/shared_schema_writer.go:38
- When
definitionsis empty,squirrel.InsertBuilder.ToSql()returns an error (no VALUES). This currently bubbles up as a SQL builder/internal error even though it's a client-side invalid request. Consider returning an InvalidArgument error code explicitly for empty writes.
pkg/pb/base/v1/service_grpc.pb.go:1570 - Typo in generated comment: "pancis" → "panics".
No description provided.