diff --git a/docs/spec.md b/docs/spec.md index 0446f9b..7fb4e4c 100644 --- a/docs/spec.md +++ b/docs/spec.md @@ -250,10 +250,18 @@ type StackType = { **Type identity:** Two Types are the same if their `id` matches (including version). Two stacks running the same app will have the same Type IDs and can rely on that for interop. -**Schema drift detection:** If two Records share a `typeId` but their Type definitions have different `schemaHash` values, that is unambiguously a bug — intentional changes always produce a new version number. +**Schema drift detection:** `defineType()` on an `id` that already has a stored Type is checked against it, rather than silently replacing it — same `schemaHash` as stored is unambiguously the same schema (a different `schemaHash` for the same `typeId` with no version bump is the exact corruption `schemaHash` exists to catch): + +- **Identical schema** (`schemaHash` matches) — a no-op; the stored Type is returned unchanged, `createdAt` untouched. Calling `defineType()` for every Type at every app startup is therefore cheap, not a rewrite each time. +- **Identical schema, different `name`** — always persists (display metadata, not schema), `createdAt` still preserved from the stored Type. +- **Different schema** — legal only if the change is a pure [additive-in-place evolution](#additive-evolution-within-a-version): new _optional_ fields only, recursively into `object` properties and `array` items; nothing removed, no field's `kind` changed, no field's `required` flipped in either direction. An illegal change throws `StackSchemaDriftError` (wire: **409**, code `schema_drift`) naming each violation — the remedy is always a new version (`defineType('...@n+1', ...)` + `registerMigration()`), never redefining the same `id` in place. + +`POST /types` (see [Types](#types-1) under the wire format) applies the same check server-side, so the wire path can't silently replace a Type either. **Type compatibility:** Structural/duck-typed — a Type is **read-compatible** with a required schema if, for every required field, the candidate declares that same field as required, at a read-compatible kind. Array and object fields recurse: their `items`/`properties` must themselves be read-compatible. This licenses _consuming_ Records, not writing them — a consumer writing through a "compatible" view still has to validate against the candidate's full schema (its other required fields, which compatibility checking never inspects). +**Two distinct relations, easy to conflate:** schema drift detection (above) answers _"may this schema replace that one under the same `id`?"_ — evolution legality. Type compatibility (below) answers _"may a consumer expecting this shape read Records of that Type?"_ — read compatibility. They deliberately disagree on `text`/`string`: read-compatible (both are strings at the value level) but **not** evolution-legal (changing a field's declared `kind` is drift, even to a read-compatible one) — a stored `kind: 'string'` field silently becoming `kind: 'text'` is exactly the kind of change a version bump should surface, even though every existing reader could still consume the value. + A field's kind is read-compatible with a required kind per this table (row = required kind, columns = candidate kinds accepted): | required → | `string` | `text` | `number` | `boolean` | `date` | `record-ref` | `file-ref` | @@ -321,7 +329,9 @@ This is what makes duck-typed cross-app consumption (`isCompatible()`, see [Type A schema accumulating many optional fields is a named smell that a consolidating bump is due — but the bump itself stays rare and semantic ("`@2` means `dueDate` is now guaranteed"), not a changelog entry for every field ever added. Bumping per addition costs a full-table rewrite per field, version-number noise, and — since records only reach a new version when the owning app next runs `migrateAll()` — doesn't even deliver per-record schema exactness in the interim; consumers face mixed-version data either way and need `baseId` queries plus duck typing regardless. -> **Not yet implemented:** a drift guard that mechanically enforces this boundary (accepting additive-in-place diffs, rejecting anything else with "bump the version"), and validation of migration function output against the target schema at _registration_ time (write-time validation, in `migrateAll()`, is the enforced backstop today). +The boundary between "accept in place" and "bump the version" above is exactly what `defineType()`'s schema drift detection ([Types](#types)) mechanically enforces — a diff, not the hash alone, since the hash necessarily changes on any legal additive diff too. + +> **Not yet implemented:** validation of migration function output against the target schema at _registration_ time (write-time validation, in `migrateAll()`, is the enforced backstop today). --- @@ -848,7 +858,7 @@ Standard HTTP status codes are used throughout: | **401** | Unauthorized | Missing or invalid bearer token | | **403** | Forbidden | `StackPermissionError` — record exists but the requester lacks access | | **404** | Not found | `StackNotFoundError` — record or version does not exist | -| **409** | Conflict | `StackConflictError` — operation blocked by a constraint violation (e.g. deleting an attachment still referenced by a record, a client-supplied `id` that already exists, deleting `_config` or changing its `entityId` — see [Stack initialization](#stack-initialization)) | +| **409** | Conflict | `StackConflictError` — operation blocked by a constraint violation (e.g. deleting an attachment still referenced by a record, a client-supplied `id` that already exists, deleting `_config` or changing its `entityId` — see [Stack initialization](#stack-initialization)); or `StackSchemaDriftError` (code `schema_drift`) — `POST /types` redefining an existing `id` with a non-additive schema change (see [Types](#types)) | | **412** | Precondition failed | `StackVersionConflictError` (code `version_conflict`) — an `If-Match` precondition doesn't match the record's current version (see [Versions](#versions)). A distinct error type and status from `StackConflictError`/409, not a subtype of it — the two have different recovery stories | | **413** | Request entity too large | Attachment upload exceeds the server's size limit | | **422** | Unprocessable entity | `StackValidationError` — request is syntactically valid but content fails schema validation (e.g. a required field has the wrong type) | @@ -863,17 +873,18 @@ Every non-2xx response whose failure maps to the core error taxonomy carries a J ```json { "error": { - "code": "permission" | "not_found" | "conflict" | "version_conflict" | "validation" | "migration" | "bad_request", + "code": "permission" | "not_found" | "conflict" | "version_conflict" | "validation" | "migration" | "bad_request" | "schema_drift", "message": "human-readable description", "details": [ { "path": "title", "message": "expected string, got number" } ], - "versionConflict": { "recordId": "rec-abc123", "expectedVersion": 5, "actualVersion": 7 } + "versionConflict": { "recordId": "rec-abc123", "expectedVersion": 5, "actualVersion": 7 }, + "schemaDrift": { "typeId": "com.example.myapp/note@1", "violations": [ { "path": "title", "message": "field removed" } ] } } } ``` -Each error code that carries extra structured data gets its own uniquely-named, uniquely-typed field, present only for that code — `details` for `code: "validation"` (`StackValidationError.errors`), `versionConflict` for `code: "version_conflict"` (`StackVersionConflictError`'s `recordId`/`expectedVersion`/`actualVersion` — the data an `ifVersion` retry loop needs: which record, what it expected, what actually won the race). This keeps each field's shape fixed rather than making any one field polymorphic across codes. +Each error code that carries extra structured data gets its own uniquely-named, uniquely-typed field, present only for that code — `details` for `code: "validation"` (`StackValidationError.errors`), `versionConflict` for `code: "version_conflict"` (`StackVersionConflictError`'s `recordId`/`expectedVersion`/`actualVersion` — the data an `ifVersion` retry loop needs: which record, what it expected, what actually won the race), `schemaDrift` for `code: "schema_drift"` (`StackSchemaDriftError`'s `typeId`/`violations` — which Type, and which specific fields made the change non-additive). This keeps each field's shape fixed rather than making any one field polymorphic across codes. -`code` is the authoritative discriminator — HTTP status is a transport hint (proxies and intermediaries rewrite statuses more often than bodies). Each core error class exposes the mapping as a static `code` (e.g. `StackPermissionError.code === 'permission'`), so a server serializes a caught error mechanically rather than via a hand-maintained switch, and `APIAdapter` reconstructs the same class from the response. Every wire code maps to exactly one status — `version_conflict` gets its own **412**, deliberately not sharing **409** with `conflict` — so when a response has no parseable wire error body (a foreign or legacy server, or a proxy that strips bodies but preserves status), `APIAdapter` still recovers the precise error from status alone for the unambiguous statuses above (400/403/404/409/412/422) — **not** for 500, since that status is a generic "unhandled server exception" signal and would misclassify ordinary server bugs as `StackMigrationError`. When neither the body nor the status yields a typed error, `APIAdapter` throws its own generic `APIAdapterError`. +`code` is the authoritative discriminator — HTTP status is a transport hint (proxies and intermediaries rewrite statuses more often than bodies). Each core error class exposes the mapping as a static `code` (e.g. `StackPermissionError.code === 'permission'`), so a server serializes a caught error mechanically rather than via a hand-maintained switch, and `APIAdapter` reconstructs the same class from the response. Most wire codes map to exactly one status — `version_conflict` gets its own **412**, deliberately not sharing **409** with `conflict` — so when a response has no parseable wire error body (a foreign or legacy server, or a proxy that strips bodies but preserves status), `APIAdapter` still recovers the precise error from status alone for the unambiguous statuses (400/403/404/412/422) — **not** for 500, since that status is a generic "unhandled server exception" signal and would misclassify ordinary server bugs as `StackMigrationError`. `schema_drift` is the one deliberate exception: it shares **409** with `conflict` (both are "operation conflicts with a constraint" in HTTP terms, and unlike `version_conflict` there's no competing convention pulling it to its own status), so status-only reconstruction of a bodyless 409 degrades to the generic `StackConflictError` rather than recovering `StackSchemaDriftError` specifically — the precise class is only recoverable with a parseable body. When neither the body nor the status yields a typed error, `APIAdapter` throws its own generic `APIAdapterError`. This mapping is pinned by the shared conformance fixtures (`@haverstack/conformance-fixtures`) so `APIAdapter` and any server implementation can't drift on it independently. @@ -995,9 +1006,11 @@ Response shape is consistent regardless of kind: ``` GET /types — list all types known to this stack GET /types/:id — get one type definition (id is URL-encoded) -POST /types — register or replace a type +POST /types — register a type, or evolve an existing one in place ``` +`POST /types` on an `id` that already has a stored Type runs the same schema drift check as `Stack.defineType()` (see [Types](#types) under the data model, and [Error responses](#error-responses) for the `schema_drift` wire code) — the server-side storage layer never blindly overwrites a Type definition; legality is decided once, in the same invariant layer both the local and wire paths share. + ### Attachments ``` diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 3fb240a..cd22c4c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -19,6 +19,7 @@ export { StackConflictError, StackVersionConflictError, StackQueryError, + StackSchemaDriftError, } from './stack.js'; export type { StackClient, @@ -92,7 +93,15 @@ export { isValidIdFormat, idTimestamp, } from './id.js'; -export { hashSchema, isCompatible, parseTypeId, buildTypeId, baseIdOf } from './schema.js'; +export { + hashSchema, + isCompatible, + diffSchemas, + parseTypeId, + buildTypeId, + baseIdOf, +} from './schema.js'; +export type { SchemaDriftViolation } from './schema.js'; export { validateContent, isValid } from './validate.js'; export type { ValidationError } from './validate.js'; export { applyMergePatch } from './merge.js'; diff --git a/packages/core/src/schema.ts b/packages/core/src/schema.ts index f03fdfa..000003c 100644 --- a/packages/core/src/schema.ts +++ b/packages/core/src/schema.ts @@ -150,6 +150,126 @@ const isCompatibleAtDepth = ( export const isCompatible = (candidateSchema: TypeSchema, requiredSchema: TypeSchema): boolean => isCompatibleAtDepth(candidateSchema, requiredSchema, 0); +// ------------------------------------------------------- +// Schema evolution legality (drift detection) +// ------------------------------------------------------- +// +// Deliberately distinct from isCompatible() above, despite both walking a +// TypeSchema pair recursively: +// +// - isCompatible() — read compatibility: may a consumer of this shape +// read records of that type? (`text` and `string` +// interchange; a candidate may have *extra* required +// fields the consumer doesn't ask about.) +// - diffSchemas() — evolution legality: may this schema replace that one +// in place, under the same typeId? (`text` and +// `string` are NOT interchangeable — changing a +// field's declared kind is drift regardless of value- +// level overlap; nothing about the *other* schema's +// extra fields is relevant, every field on both sides +// matters.) +// +// Conflating them is the obvious future bug: a read-compatible schema +// (e.g. one that merely has extra optional fields) is not necessarily a +// legal in-place evolution, and vice versa. + +export type SchemaDriftViolation = { + /** Field path where the drift was detected, e.g. "title" or "author.name". Empty string means array-item context. */ + path: string; + message: string; +}; + +// Same rationale and bound as MAX_COMPATIBILITY_DEPTH: a pathological or +// circular schema shouldn't be walked forever. Past the limit we can't +// verify the change is additive, so we fail closed — report it as drift +// rather than silently accept. +const MAX_DIFF_DEPTH = 32; + +const diffField = ( + path: string, + stored: FieldDef, + candidate: FieldDef, + depth: number, + violations: SchemaDriftViolation[], +): void => { + if (depth > MAX_DIFF_DEPTH) { + violations.push({ path, message: 'exceeds max nesting depth; cannot verify additive change' }); + return; + } + if (stored.kind !== candidate.kind) { + violations.push({ + path, + message: `kind changed from "${stored.kind}" to "${candidate.kind}"`, + }); + return; // kinds differ — nested comparison (properties/items) is meaningless + } + if (!!stored.required !== !!candidate.required) { + violations.push({ + path, + message: `required changed from ${!!stored.required} to ${!!candidate.required}`, + }); + } + if (stored.kind === 'array' && candidate.kind === 'array') { + diffField(`${path}[]`, stored.items, candidate.items, depth + 1, violations); + } + if (stored.kind === 'object' && candidate.kind === 'object') { + diffFields(path, stored.properties, candidate.properties, depth + 1, violations); + } +}; + +const diffFields = ( + prefix: string, + stored: TypeSchema, + candidate: TypeSchema, + depth: number, + violations: SchemaDriftViolation[], +): void => { + if (depth > MAX_DIFF_DEPTH) { + violations.push({ + path: prefix, + message: 'exceeds max nesting depth; cannot verify additive change', + }); + return; + } + for (const [key, storedDef] of Object.entries(stored)) { + const path = prefix ? `${prefix}.${key}` : key; + const candidateDef = candidate[key]; + if (!candidateDef) { + violations.push({ path, message: 'field removed' }); + continue; + } + diffField(path, storedDef, candidateDef, depth, violations); + } + for (const [key, candidateDef] of Object.entries(candidate)) { + if (key in stored) continue; // already compared above + if (candidateDef.required) { + const path = prefix ? `${prefix}.${key}` : key; + violations.push({ path, message: 'new field is required; new fields must be optional' }); + } + // A new optional field is exactly what additive evolution allows — no violation. + } +}; + +/** + * Check whether `candidate` is a legal in-place evolution of `stored` — the + * same typeId, a new schemaHash. Legal iff every existing field is + * unchanged (same kind, same required-ness, recursively into object + * properties and array items) and every field `candidate` adds beyond + * `stored` is optional. Returns the list of violations; empty means legal. + * + * This is the callable-facing sibling of isCompatible() but answers a + * different question — see the note above. Removing a field, changing a + * field's kind, or flipping required either direction (optional→required or + * required→optional) is drift: a version bump communicates that a consumer + * pinned to the old shape needs to notice, in a way an in-place change + * cannot. + */ +export const diffSchemas = (stored: TypeSchema, candidate: TypeSchema): SchemaDriftViolation[] => { + const violations: SchemaDriftViolation[] = []; + diffFields('', stored, candidate, 0, violations); + return violations; +}; + // ------------------------------------------------------- // Type ID parsing // ------------------------------------------------------- diff --git a/packages/core/src/stack.ts b/packages/core/src/stack.ts index 79f58bb..0ac4796 100644 --- a/packages/core/src/stack.ts +++ b/packages/core/src/stack.ts @@ -15,7 +15,8 @@ */ import { generateId, isValidIdFormat, idTimestamp } from './id.js'; -import { hashSchema, isCompatible, parseTypeId, baseIdOf } from './schema.js'; +import { hashSchema, isCompatible, parseTypeId, baseIdOf, diffSchemas } from './schema.js'; +import type { SchemaDriftViolation } from './schema.js'; import { validateContent } from './validate.js'; import { applyMergePatch } from './merge.js'; import { checkAccess, groupRoleFromAssociations } from './access.js'; @@ -205,6 +206,29 @@ export class StackQueryError extends Error { } } +/** + * Thrown by defineType() when redefining an existing typeId with a schema + * change that isn't a legal in-place evolution (see diffSchemas() in + * schema.ts) — same typeId, a shape change beyond "new optional fields + * added." The remedy is always the same: register a new version instead of + * redefining this one in place. + */ +export class StackSchemaDriftError extends Error { + static readonly code = 'schema_drift' as const; + constructor( + public readonly typeId: TypeId, + public readonly violations: SchemaDriftViolation[], + ) { + super( + `Schema drift detected for type "${typeId}": the stored schema and the new definition ` + + `differ beyond additive evolution (new optional fields only). Bump the version instead ` + + `of redefining "${typeId}" in place — e.g. defineType(\`${baseIdOf(typeId)}@${(parseTypeId(typeId)?.version ?? 0) + 1}\`, ...) plus registerMigration().\n` + + violations.map((v) => ` ${v.path || '(root)'}: ${v.message}`).join('\n'), + ); + this.name = 'StackSchemaDriftError'; + } +} + // ------------------------------------------------------- // Record ID validation // ------------------------------------------------------- @@ -413,8 +437,27 @@ export class Stack implements StackClient { // ------------------------------------------------------- /** - * Define and persist a new Type. Computes the schemaHash automatically. + * Define and persist a Type. Computes the schemaHash automatically. * Should be called at app startup before creating any records of this type. + * + * Redefining an existing typeId is checked against the stored schema + * (#68), instead of silently replacing it (the exact corruption + * schemaHash exists to catch, previously undetected since nothing ever + * compared it): + * + * - Identical schema and name — fully idempotent no-op, `createdAt` + * untouched. This is what makes calling defineType() for every system + * type on every `Stack.create()` (seedSystemTypes()) cheap instead of + * six unconditional rewrites per open. + * - Identical schema, different name — name is display metadata, not + * schema, so this always persists; `createdAt` is preserved from the + * stored type either way. + * - Different schema — legal only if the change is a pure additive + * evolution (diffSchemas(): new *optional* fields only, nothing + * removed/retyped/re-required). Otherwise throws StackSchemaDriftError + * naming each violation; the remedy is a new version + * (`defineType('...@n+1', ...)` + `registerMigration()`), never an + * in-place rewrite. */ async defineType( id: TypeId, @@ -429,7 +472,28 @@ export class Stack implements StackClient { ); } + // This instance now knows this version exists, independent of whether + // the adapter write below turns out to be a no-op — presentAtLatest()'s + // stale-writer detection depends on every defineType() call registering + // here, including the idempotent-no-op path. + const priorMax = this.maxDefinedVersion.get(parsed.baseId) ?? 0; + if (parsed.version > priorMax) this.maxDefinedVersion.set(parsed.baseId, parsed.version); + const schemaHash = await hashSchema(schema); + const existing = await this.adapter.getType(id); + + if (existing) { + if (existing.schemaHash === schemaHash) { + if (existing.name === name) return existing; + // else: name-only change — falls through to the write below, + // schema/hash/createdAt all carried over unchanged. + } else { + const violations = diffSchemas(existing.schema, schema); + if (violations.length > 0) { + throw new StackSchemaDriftError(id, violations); + } + } + } const type: StackType = { id, @@ -438,13 +502,10 @@ export class Stack implements StackClient { name, schema, schemaHash, - createdAt: new Date(), + createdAt: existing?.createdAt ?? new Date(), ...(opts.migratesFrom && { migratesFrom: opts.migratesFrom }), }; - const priorMax = this.maxDefinedVersion.get(parsed.baseId) ?? 0; - if (parsed.version > priorMax) this.maxDefinedVersion.set(parsed.baseId, parsed.version); - await this.adapter.saveType(type); return type; } diff --git a/packages/core/tests/schema.test.ts b/packages/core/tests/schema.test.ts index 02b1829..524dd97 100644 --- a/packages/core/tests/schema.test.ts +++ b/packages/core/tests/schema.test.ts @@ -1,5 +1,5 @@ import { describe, test, expect } from 'vitest'; -import { hashSchema, isCompatible, parseTypeId, buildTypeId } from '../src/schema.js'; +import { hashSchema, isCompatible, diffSchemas, parseTypeId, buildTypeId } from '../src/schema.js'; import type { TypeSchema } from '../src/types.js'; // ------------------------------------------------------- @@ -306,6 +306,174 @@ describe('isCompatible', () => { }); }); +// ------------------------------------------------------- +// diffSchemas (#68) +// ------------------------------------------------------- + +describe('diffSchemas', () => { + test('identical schemas produce no violations', () => { + const schema: TypeSchema = { text: { kind: 'text', required: true } }; + expect(diffSchemas(schema, schema)).toEqual([]); + }); + + test('a new optional field is additive-legal', () => { + const stored: TypeSchema = { text: { kind: 'text', required: true } }; + const candidate: TypeSchema = { + text: { kind: 'text', required: true }, + title: { kind: 'string' }, + }; + expect(diffSchemas(stored, candidate)).toEqual([]); + }); + + test('a new required field is drift', () => { + const stored: TypeSchema = { text: { kind: 'text', required: true } }; + const candidate: TypeSchema = { + text: { kind: 'text', required: true }, + title: { kind: 'string', required: true }, + }; + const violations = diffSchemas(stored, candidate); + expect(violations).toEqual([ + { path: 'title', message: 'new field is required; new fields must be optional' }, + ]); + }); + + test('removing a field is drift', () => { + const stored: TypeSchema = { + text: { kind: 'text', required: true }, + title: { kind: 'string' }, + }; + const candidate: TypeSchema = { text: { kind: 'text', required: true } }; + const violations = diffSchemas(stored, candidate); + expect(violations).toEqual([{ path: 'title', message: 'field removed' }]); + }); + + test('changing a field’s kind is drift, even to a read-compatible kind (text/string)', () => { + const stored: TypeSchema = { body: { kind: 'text', required: true } }; + const candidate: TypeSchema = { body: { kind: 'string', required: true } }; + const violations = diffSchemas(stored, candidate); + expect(violations).toEqual([{ path: 'body', message: 'kind changed from "text" to "string"' }]); + }); + + test('flipping an existing field from optional to required is drift', () => { + const stored: TypeSchema = { title: { kind: 'string' } }; + const candidate: TypeSchema = { title: { kind: 'string', required: true } }; + const violations = diffSchemas(stored, candidate); + expect(violations).toEqual([{ path: 'title', message: 'required changed from false to true' }]); + }); + + test('flipping an existing field from required to optional is drift', () => { + const stored: TypeSchema = { title: { kind: 'string', required: true } }; + const candidate: TypeSchema = { title: { kind: 'string' } }; + const violations = diffSchemas(stored, candidate); + expect(violations).toEqual([{ path: 'title', message: 'required changed from true to false' }]); + }); + + test('a new optional field nested inside an existing object is additive-legal', () => { + const stored: TypeSchema = { + author: { kind: 'object', required: true, properties: { name: { kind: 'string' } } }, + }; + const candidate: TypeSchema = { + author: { + kind: 'object', + required: true, + properties: { name: { kind: 'string' }, email: { kind: 'string' } }, + }, + }; + expect(diffSchemas(stored, candidate)).toEqual([]); + }); + + test('removing a nested object field is drift, with a dotted path', () => { + const stored: TypeSchema = { + author: { + kind: 'object', + required: true, + properties: { name: { kind: 'string', required: true } }, + }, + }; + const candidate: TypeSchema = { + author: { kind: 'object', required: true, properties: {} }, + }; + const violations = diffSchemas(stored, candidate); + expect(violations).toEqual([{ path: 'author.name', message: 'field removed' }]); + }); + + test('changing an array item field is drift, with a bracketed path', () => { + const stored: TypeSchema = { + tags: { kind: 'array', required: true, items: { kind: 'string' } }, + }; + const candidate: TypeSchema = { + tags: { kind: 'array', required: true, items: { kind: 'number' } }, + }; + const violations = diffSchemas(stored, candidate); + expect(violations).toEqual([ + { path: 'tags[]', message: 'kind changed from "string" to "number"' }, + ]); + }); + + test('a field changing kind from object to array (or vice versa) is drift, not a recursion', () => { + const stored: TypeSchema = { + data: { kind: 'object', required: true, properties: { x: { kind: 'string' } } }, + }; + const candidate: TypeSchema = { + data: { kind: 'array', required: true, items: { kind: 'string' } }, + }; + const violations = diffSchemas(stored, candidate); + expect(violations).toEqual([ + { path: 'data', message: 'kind changed from "object" to "array"' }, + ]); + }); + + test('multiple independent violations are all reported', () => { + const stored: TypeSchema = { + text: { kind: 'text', required: true }, + title: { kind: 'string' }, + }; + const candidate: TypeSchema = { + text: { kind: 'string', required: true }, // kind change + newRequired: { kind: 'string', required: true }, // new required field + // title removed entirely + }; + const violations = diffSchemas(stored, candidate); + expect(violations).toContainEqual({ + path: 'text', + message: 'kind changed from "text" to "string"', + }); + expect(violations).toContainEqual({ path: 'title', message: 'field removed' }); + expect(violations).toContainEqual({ + path: 'newRequired', + message: 'new field is required; new fields must be optional', + }); + expect(violations).toHaveLength(3); + }); + + test('exceeding the max diff depth fails closed (reported as drift, not silently accepted)', () => { + const buildNested = (depth: number, leafKind: 'string' | 'number'): TypeSchema => { + let schema: TypeSchema = { leaf: { kind: leafKind, required: true } }; + for (let i = 0; i < depth; i++) { + schema = { nested: { kind: 'object', required: true, properties: schema } }; + } + return schema; + }; + const stored = buildNested(1000, 'string'); + const candidate = buildNested(1000, 'string'); + expect(() => diffSchemas(stored, candidate)).not.toThrow(); + expect(diffSchemas(stored, candidate).length).toBeGreaterThan(0); + }); + + test('matching schemas within the depth limit produce no violations', () => { + const buildNested = (depth: number): TypeSchema => { + let schema: TypeSchema = { leaf: { kind: 'string', required: true } }; + for (let i = 0; i < depth; i++) { + schema = { nested: { kind: 'object', required: true, properties: schema } }; + } + return schema; + }; + const stored = buildNested(10); + const candidate = buildNested(10); + expect(diffSchemas(stored, candidate)).toEqual([]); + }); +}); + // ------------------------------------------------------- // parseTypeId // ------------------------------------------------------- diff --git a/packages/core/tests/stack.test.ts b/packages/core/tests/stack.test.ts index b9a21c3..1e72447 100644 --- a/packages/core/tests/stack.test.ts +++ b/packages/core/tests/stack.test.ts @@ -6,6 +6,7 @@ import { StackNotFoundError, StackConflictError, StackVersionConflictError, + StackSchemaDriftError, } from '../src/stack.js'; import { generateId, crockford32Encode } from '../src/id.js'; import { MemoryAdapter } from '../src/testing.js'; @@ -96,6 +97,129 @@ describe('defineType', () => { const type = await stack.getType(NOTE_V2); expect(type?.migratesFrom).toBe(NOTE_V1); }); + + // ------------------------------------------------------- + // Schema drift detection (#68) + // ------------------------------------------------------- + + test('redefining with an identical schema is a no-op — createdAt does not churn', async () => { + const before = await stack.getType(NOTE_V1); + await new Promise((resolve) => setTimeout(resolve, 5)); + await stack.defineType(NOTE_V1, 'Note', { text: { kind: 'text', required: true } }); + const after = await stack.getType(NOTE_V1); + expect(after?.createdAt.getTime()).toBe(before?.createdAt.getTime()); + }); + + test('a name-only change persists, preserving createdAt', async () => { + const before = await stack.getType(NOTE_V1); + await stack.defineType(NOTE_V1, 'Renamed Note', { + text: { kind: 'text', required: true }, + }); + const after = await stack.getType(NOTE_V1); + expect(after?.name).toBe('Renamed Note'); + expect(after?.createdAt.getTime()).toBe(before?.createdAt.getTime()); + }); + + test('adding a new optional field in place is accepted, preserving createdAt', async () => { + const before = await stack.getType(NOTE_V1); + await stack.defineType(NOTE_V1, 'Note', { + text: { kind: 'text', required: true }, + title: { kind: 'string' }, + }); + const after = await stack.getType(NOTE_V1); + expect(after?.schema.title).toEqual({ kind: 'string' }); + expect(after?.createdAt.getTime()).toBe(before?.createdAt.getTime()); + expect(after?.schemaHash).not.toBe(before?.schemaHash); + }); + + test('adding a new optional field nested inside an existing object is accepted', async () => { + const nested = 'com.example.test/nested@1'; + await stack.defineType(nested, 'Nested', { + author: { kind: 'object', required: true, properties: { name: { kind: 'string' } } }, + }); + await stack.defineType(nested, 'Nested', { + author: { + kind: 'object', + required: true, + properties: { name: { kind: 'string' }, email: { kind: 'string' } }, + }, + }); + const type = await stack.getType(nested); + expect((type?.schema.author as { properties: unknown }).properties).toHaveProperty('email'); + }); + + test('adding a new required field is rejected with StackSchemaDriftError', async () => { + await expect( + stack.defineType(NOTE_V1, 'Note', { + text: { kind: 'text', required: true }, + title: { kind: 'string', required: true }, + }), + ).rejects.toThrow(StackSchemaDriftError); + }); + + test('removing a field is rejected with StackSchemaDriftError', async () => { + await stack.defineType(NOTE_V2, 'Note', { + text: { kind: 'text', required: true }, + title: { kind: 'string' }, + }); + await expect( + stack.defineType(NOTE_V2, 'Note', { text: { kind: 'text', required: true } }), + ).rejects.toThrow(StackSchemaDriftError); + }); + + test('changing a field kind is rejected with StackSchemaDriftError, even text/string', async () => { + await expect( + stack.defineType(NOTE_V1, 'Note', { text: { kind: 'string', required: true } }), + ).rejects.toThrow(StackSchemaDriftError); + }); + + test('flipping an existing field required is rejected with StackSchemaDriftError', async () => { + await expect(stack.defineType(NOTE_V1, 'Note', { text: { kind: 'text' } })).rejects.toThrow( + StackSchemaDriftError, + ); + }); + + test('StackSchemaDriftError names the specific violation', async () => { + try { + await stack.defineType(NOTE_V1, 'Note', { + text: { kind: 'text', required: true }, + title: { kind: 'string', required: true }, + }); + expect.unreachable(); + } catch (e) { + expect(e).toBeInstanceOf(StackSchemaDriftError); + const err = e as StackSchemaDriftError; + expect(err.typeId).toBe(NOTE_V1); + expect(err.violations).toEqual([ + { path: 'title', message: 'new field is required; new fields must be optional' }, + ]); + expect(err.message).toContain('title'); + expect(err.message).toContain('Bump the version'); + } + }); + + test('an illegal redefinition does not overwrite the stored type', async () => { + const before = await stack.getType(NOTE_V1); + await expect( + stack.defineType(NOTE_V1, 'Note', { + text: { kind: 'text', required: true }, + title: { kind: 'string', required: true }, + }), + ).rejects.toThrow(StackSchemaDriftError); + const after = await stack.getType(NOTE_V1); + expect(after).toEqual(before); + }); + + test('repeated seedSystemTypes()-style redefinition across Stack.create() calls stays idempotent', async () => { + // Simulates the every-open churn this issue closes: a second Stack + // instance (e.g. a fresh process reopening the same adapter) redefines + // the same types on the same underlying storage. + const before = await stack.getType(NOTE_V1); + const stackB = await Stack.create(adapter); + await stackB.defineType(NOTE_V1, 'Note', { text: { kind: 'text', required: true } }); + const after = await stackB.getType(NOTE_V1); + expect(after?.createdAt.getTime()).toBe(before?.createdAt.getTime()); + }); }); // ------------------------------------------------------- diff --git a/packages/wire-types/src/index.ts b/packages/wire-types/src/index.ts index c5483fa..febb751 100644 --- a/packages/wire-types/src/index.ts +++ b/packages/wire-types/src/index.ts @@ -5,6 +5,7 @@ import type { Association, Permission, ValidationError, + SchemaDriftViolation, } from '@haverstack/core'; import { StackValidationError, @@ -14,6 +15,7 @@ import { StackVersionConflictError, StackMigrationError, StackQueryError, + StackSchemaDriftError, } from '@haverstack/core'; export type WireRecord = { @@ -125,7 +127,8 @@ export type WireErrorCode = | 'conflict' | 'version_conflict' | 'validation' - | 'migration'; + | 'migration' + | 'schema_drift'; export type WireError = { error: { @@ -139,6 +142,11 @@ export type WireError = { expectedVersion: number; actualVersion: number; }; + /** The rejected defineType() call's target and violations. Only present for code: 'schema_drift'. */ + schemaDrift?: { + typeId: string; + violations: SchemaDriftViolation[]; + }; }; }; @@ -162,6 +170,14 @@ export const WIRE_ERROR_STATUS: Record = { * future server-side migration-graph check has a defined status to use. */ migration: 500, + // Shares 409 with 'conflict' — both are "operation conflicts with a + // constraint" in HTTP terms, and unlike version_conflict there's no + // competing convention pulling schema_drift to its own status. The + // shared status means STATUS_TO_CODE below can only pick one canonical + // code for status-only reconstruction (see its doc comment) — that's + // 'conflict'; a schema-drift response without a parseable body degrades + // to a generic StackConflictError rather than being lost entirely. + schema_drift: 409, }; /** @@ -257,12 +273,24 @@ export function serializeError(err: unknown): { status: number; body: WireError body: { error: { code: 'migration', message: err.message } }, }; } + if (err instanceof StackSchemaDriftError) { + return { + status: WIRE_ERROR_STATUS.schema_drift, + body: { + error: { + code: 'schema_drift', + message: err.message, + schemaDrift: { typeId: err.typeId, violations: err.violations }, + }, + }, + }; + } return null; } /** Reconstruct the core error a WireError body describes. */ export function deserializeError(body: WireError): Error { - const { code, message, details, versionConflict } = body.error; + const { code, message, details, versionConflict, schemaDrift } = body.error; switch (code) { case 'validation': return new StackValidationError(details ?? []); @@ -283,6 +311,8 @@ export function deserializeError(body: WireError): Error { return new StackQueryError(message); case 'migration': return new StackMigrationError(message); + case 'schema_drift': + return new StackSchemaDriftError(schemaDrift?.typeId ?? '', schemaDrift?.violations ?? []); } }