diff --git a/.changeset/20397-diff-default-range-labels.md b/.changeset/20397-diff-default-range-labels.md new file mode 100644 index 00000000000..5d1b4aba118 --- /dev/null +++ b/.changeset/20397-diff-default-range-labels.md @@ -0,0 +1,11 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +fix(metadata-protocol): `diffMetaItem`'s default range labels its to side with the active row's own version, so `GET /meta/:type/:name/diff` with no `from` / `to` names the versions it compares while a draft is pending (#20397) + +With no `toVersion`, the to side is the current active `sys_metadata` row. Its body was compared, but `toVersion` came from the newest `sys_metadata_history` row, which is a draft save whenever a draft is pending: every draft save appends a history row. The labels and the bodies then named different rows. Measured on the real REST stack, an app with one active save and two draft saves answered `fromVersion 2 → toVersion 3` over its version-1 body, and a view with one active save and one draft save answered "no changes" labelled `1 → 2` while version 2 differs. + +- **Now:** `toVersion` is the active row's own `version`, read in the same read as its body. The default `fromVersion` is still the history version immediately before that label. An item whose active row is version 2 with a draft pending answers `1 → 2`, the same answer as `?from=1&to=2`. +- **No active row** (a draft-only item, or a deleted one): the to side is absent, and both labels are `null` with empty buckets, as the response schema declares for an absent side. Before, a draft-only item was labelled with its newest draft save, and its from side could be an earlier draft save's body. A deleted item was labelled `N-1 → N` up to its tombstone. That deletion is still read by naming its versions (`?from=N-1&to=N`). +- Unchanged: the response shape, explicit `from` / `to` ranges, and the default range of an item with no draft pending. diff --git a/packages/metadata-protocol/src/protocol.diff-dead-history-read.test.ts b/packages/metadata-protocol/src/protocol.diff-dead-history-read.test.ts index 1d82ef6dec0..6e284609cbb 100644 --- a/packages/metadata-protocol/src/protocol.diff-dead-history-read.test.ts +++ b/packages/metadata-protocol/src/protocol.diff-dead-history-read.test.ts @@ -282,3 +282,157 @@ describe('#8798 — a history-table outage now answers the same way for every ty expect(err.status).toBe(503); }); }); + +/** + * [#20397] The DEFAULT range (no `toVersion`) labels its to side with the + * version whose body it compares. + * + * The to-side body is the active `sys_metadata` row; the label used to be the + * newest `sys_metadata_history` row's version, which is a draft save whenever a + * draft is pending (every draft save appends a history row). The label is now + * the active row's own `version` column, the one `SysMetadataRepository.put` + * stamps with the version of the history row it appends in the same + * transaction. Pinned here over seeded rows, beside this file's double, which + * already honours the `where` on both tables; the same readings through the + * real REST stack are `packages/rest/src/meta-diff-default-range-labels.test.ts`. + */ +describe('#20397 — the default range labels the to side with the active row\'s own version', () => { + type Seed = { version: number; op: string; body: Record | null }; + /** History rows in version order, plus the active and draft rows they left behind. */ + function seedLineage( + tables: Record>>, + type: string, + name: string, + history: Seed[], + rows: { active?: Seed; draft?: Seed }, + ) { + const base = { organization_id: null, type, name }; + history.forEach((h, i) => { + tables.sys_metadata_history!.push({ + ...base, + id: `h_${h.version}`, + version: h.version, + event_seq: i + 1, + operation_type: h.op, + metadata: h.body == null ? null : JSON.stringify(h.body), + checksum: h.body == null ? null : hashSpec(h.body), + recorded_at: new Date(i + 1).toISOString(), + }); + }); + for (const state of ['active', 'draft'] as const) { + const row = rows[state]; + if (!row) continue; + tables.sys_metadata!.push({ + ...base, + id: `m_${state}`, + state, + version: row.version, + metadata: JSON.stringify(row.body), + checksum: hashSpec(row.body!), + }); + } + } + + for (const type of [ORDINARY_TYPE, 'app']) { + it(`${type}: with a draft pending, toVersion is the active row's version and the answer is the explicit range's`, async () => { + const { engine, tables } = makeStubEngine(); + const one = { name: 'item', label: 'One' }; + const two = { name: 'item', label: 'Two' }; + const pending = { name: 'item', label: 'Pending draft' }; + seedLineage(tables, type, 'item', [ + { version: 1, op: 'create', body: one }, + { version: 2, op: 'update', body: two }, + { version: 3, op: 'create', body: pending }, + ], { active: { version: 2, op: 'update', body: two }, draft: { version: 3, op: 'create', body: pending } }); + const protocol = new ObjectStackProtocolImplementation(engine); + + const res: any = await protocol.diffMetaItem({ type, name: 'item' }); + const explicit: any = await protocol.diffMetaItem({ type, name: 'item', fromVersion: 1, toVersion: 2 }); + + expect(res).toEqual({ + type, + name: 'item', + fromVersion: 1, + toVersion: 2, + added: [], + removed: [], + changed: [{ path: 'label', from: 'One', to: 'Two' }], + }); + expect(res).toEqual(explicit); + }); + } + + it('the card\'s app reading: one active save and two draft saves label the to side 1 (was 3), with no from side before it', async () => { + const { engine, tables } = makeStubEngine(); + const v1 = { name: 'atlas', label: 'Atlas v1' }; + const d2 = { name: 'atlas', label: 'Atlas v2 draft' }; + const d3 = { name: 'atlas', label: 'Atlas v3 draft' }; + seedLineage(tables, 'app', 'atlas', [ + { version: 1, op: 'create', body: v1 }, + { version: 2, op: 'create', body: d2 }, + { version: 3, op: 'update', body: d3 }, + ], { active: { version: 1, op: 'create', body: v1 }, draft: { version: 3, op: 'update', body: d3 } }); + const protocol = new ObjectStackProtocolImplementation(engine); + + const res: any = await protocol.diffMetaItem({ type: 'app', name: 'atlas' }); + + expect(res).toEqual({ + type: 'app', + name: 'atlas', + fromVersion: null, + toVersion: 1, + added: [{ path: 'name', value: 'atlas' }, { path: 'label', value: 'Atlas v1' }], + removed: [], + changed: [], + }); + }); + + it('no active row (draft saves only): the to side is absent, so its label is null and no draft body is either side', async () => { + const { engine, tables } = makeStubEngine(); + const d1 = { name: 'item', label: 'Only draft one' }; + const d2 = { name: 'item', label: 'Only draft two' }; + seedLineage(tables, ORDINARY_TYPE, 'item', [ + { version: 1, op: 'create', body: d1 }, + { version: 2, op: 'update', body: d2 }, + ], { draft: { version: 2, op: 'update', body: d2 } }); + const protocol = new ObjectStackProtocolImplementation(engine); + + const res: any = await protocol.diffMetaItem({ type: ORDINARY_TYPE, name: 'item' }); + + expect(res).toEqual({ + type: ORDINARY_TYPE, name: 'item', fromVersion: null, toVersion: null, added: [], removed: [], changed: [], + }); + }); + + it('no active row (deleted): the default range answers null on both sides; the deletion stays readable by naming its versions', async () => { + // The same rule as the draft-only case: the to side is absent, so is + // its label. Before #20397 this one case happened to agree (the newest + // history row was the tombstone, whose body is also absent); the + // deletion itself is still one explicit range away. + const { engine, tables } = makeStubEngine(); + const one = { name: 'item', label: 'One' }; + const two = { name: 'item', label: 'Two' }; + seedLineage(tables, ORDINARY_TYPE, 'item', [ + { version: 1, op: 'create', body: one }, + { version: 2, op: 'update', body: two }, + { version: 3, op: 'delete', body: null }, + ], {}); + const protocol = new ObjectStackProtocolImplementation(engine); + + const res: any = await protocol.diffMetaItem({ type: ORDINARY_TYPE, name: 'item' }); + const deletion: any = await protocol.diffMetaItem({ type: ORDINARY_TYPE, name: 'item', fromVersion: 2, toVersion: 3 }); + + expect(res).toEqual({ + type: ORDINARY_TYPE, name: 'item', fromVersion: null, toVersion: null, added: [], removed: [], changed: [], + }); + expect(deletion).toEqual({ + type: ORDINARY_TYPE, + name: 'item', + fromVersion: 2, + toVersion: 3, + added: [], + removed: [{ path: 'name', value: 'item' }, { path: 'label', value: 'Two' }], + changed: [], + }); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 711fded5dde..3bb7041da29 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -21286,9 +21286,10 @@ export class ObjectStackProtocolImplementation implements /** * Compute a shallow structural diff between two historical * versions of a metadata item. Either side may be omitted: when - * `toVersion` is undefined the current active body is used; when - * `fromVersion` is undefined the immediately previous history row - * is used. Returns `{ added, removed, changed }` keyed by JSON + * `toVersion` is undefined the current active body is used, labelled + * with that active row's own `version` (`null` when there is no active + * row); when `fromVersion` is undefined the immediately previous history + * row is used. Returns `{ added, removed, changed }` keyed by JSON * pointer-style paths for primitive leaves; nested objects/arrays * are reported as a single change record. * @@ -21391,12 +21392,6 @@ export class ObjectStackProtocolImplementation implements // `catch` below is this function's only stated intent for that failure, // so removing the call makes every type take it. Pinned in // `protocol.diff-dead-history-read.test.ts`. - const repo = this.getOverlayRepo(orgId); - const fullRef = { - type: singularType, - name: request.name, - org: orgId ?? 'env', - } as { type: string; name: string; org: string }; const histRows: Array<{ version: number; body: Record | null }> = []; try { const engineAny = this.engine as any; @@ -21467,9 +21462,40 @@ export class ObjectStackProtocolImplementation implements toVersion = request.toVersion; toBody = byVersion.get(request.toVersion) ?? null; } else { - const current = await repo.get(fullRef as any, { state: 'active' }); - toBody = current ? (current.body as Record) : null; - toVersion = histRows.length ? histRows[histRows.length - 1]!.version : null; + // [#20397] The default `to` side is the CURRENT ACTIVE ROW, and ONE + // read of that row supplies both of its facts: the body compared and + // the `version` it is labelled with. `SysMetadataRepository.put` + // stamps that column in the same transaction that appends the history + // row carrying the same body, so it names the version this body is. + // + // The label used to come from the NEWEST `sys_metadata_history` row + // instead, which is a draft save whenever a draft is pending (every + // draft save appends a row too). Body and label then named different + // rows: on the real REST stack an app answered `2 → 3` over its + // version-1 body, and a view answered "no changes" labelled `1 → 2` + // while version 2 differs. + // + // Read here, not through `SysMetadataRepository.get`: its + // `MetadataItem` projection carries the row's content hash but not + // its lineage `version`. Same predicate as that read (active state, no + // package scope), and VERBATIM like the history bodies it is compared + // against: no ADR-0087 conversion on either side. + // + // No active row (a draft-only item, a deleted one) ⇒ that side is + // absent and so is its label: `null`, as `DiffMetaItemResponseSchema` + // declares, never the number of a row whose body is not the one + // compared. ⛔ Do not recover a number by matching bodies or hashes + // against history: a publish and a revert both write rows whose + // bodies repeat earlier ones. + const current = (await this.engine.findOne('sys_metadata', { + where: { organization_id: orgId, type: singularType, name: request.name, state: 'active' }, + })) as { metadata?: unknown; version?: unknown } | null; + toBody = current?.metadata == null + ? null + : (typeof current.metadata === 'string' + ? JSON.parse(current.metadata) + : current.metadata as Record); + toVersion = current && typeof current.version === 'number' ? current.version : null; } if (request.fromVersion !== undefined) { fromVersion = request.fromVersion; diff --git a/packages/rest/src/meta-diff-default-range-labels.test.ts b/packages/rest/src/meta-diff-default-range-labels.test.ts new file mode 100644 index 00000000000..98163d034d2 --- /dev/null +++ b/packages/rest/src/meta-diff-default-range-labels.test.ts @@ -0,0 +1,248 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20397] `GET /meta/:type/:name/diff` with no `from` / `to` compares the + * previous version with the CURRENT one, and labels each side with the version + * whose body it is. + * + * ## The defect + * + * `diffMetaItem` read the to-side BODY from the active `sys_metadata` row but + * took `toVersion` from the NEWEST `sys_metadata_history` row, which is a draft + * save whenever a draft is pending: every draft save appends a history row too. + * Measured on this stack before the change: an app with one active save and two + * draft saves answered `2 → 3` over its version-1 body, and a view with one + * active save and one draft save answered "no changes" labelled `1 → 2` while + * version 2 differs. + * + * ## What the default range answers now + * + * The to side is the active row, labelled with that row's own `version` (the + * column `SysMetadataRepository.put` stamps with the version of the history row + * it appends), and the from side is the history row immediately preceding that + * label. With no active row the to side is absent and so is its label: `null` + * on both sides, as `DiffMetaItemResponseSchema` declares. + * + * ## Why this file boots the real stack + * + * The version numbers are the repository's: the draft saves, the active save + * and the rows they append are real writes through the real routes, over a + * better-sqlite3 `:memory:` engine with the real `sys_metadata*` objects and a + * real `ObjectStackProtocolImplementation`. The stubs are the auth boundary + * (`resolveExecCtx`) and the service probe that says `tenancy` is off here. + * + * The reader is an author (`manage_metadata`): the default range is a builder's + * question, and who else may read `/diff` is not this file's subject. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { + SysMetadata, + SysMetadataHistoryObject, + SysMetadataAuditObject, +} from '@objectstack/platform-objects/metadata'; +import { RestServer } from './rest-server.js'; + +/** `registry.registerObject` requires a package id (see the sibling real-stack tests). */ +const TEST_PACKAGE_ID = 'objectstack-test'; + +const CALLERS = { + /** Writes the fixtures: no principal, the machine-write shape. */ + system: { isSystem: true }, + /** Reads the diff: holds the authoring capability. */ + author: { userId: 'u_author', systemPermissions: ['manage_metadata'] }, +} as const; +type CallerName = keyof typeof CALLERS; + +const liveEngines: ObjectQL[] = []; +afterEach(async () => { + while (liveEngines.length) { + try { await liveEngines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +function createMockServer() { + const noop = () => {}; + return { + get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, + listen: async () => {}, close: async () => {}, + }; +} + +function makeRes() { + const res: any = { statusCode: 200, body: undefined, headers: {} as Record }; + res.status = (code: number) => { res.statusCode = code; return res; }; + res.json = (body: any) => { res.body = body; return res; }; + res.send = () => res; + res.end = () => res; + res.header = (k: string, v: string) => { res.headers[k] = v; return res; }; + res.setHeader = () => {}; res.write = () => true; + return res; +} + +const META = '/api/v1/meta'; + +async function boot() { + const engine = new ObjectQL(); + liveEngines.push(engine); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), true); + await engine.init(); + engine.registry.registerObject(SysMetadata as any, TEST_PACKAGE_ID); + engine.registry.registerObject(SysMetadataHistoryObject as any, TEST_PACKAGE_ID); + engine.registry.registerObject(SysMetadataAuditObject as any, TEST_PACKAGE_ID); + await engine.syncSchemas(); + + const protocol = new ObjectStackProtocolImplementation(engine as any); + const rest: any = new RestServer(createMockServer() as any, protocol as any, { api: { requireAuth: false } } as any); + let caller: CallerName = 'system'; + rest.resolveExecCtx = async () => ({ ...CALLERS[caller] }); + // ADR-0057 D10 — `tenancy` is an optional service this deployment lacks. + rest.serviceExistsProvider = (name: string) => name !== 'tenancy'; + rest.registerRoutes(); + + const route = (method: string, path: string) => { + const found = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + if (!found) throw new Error(`${method} ${path} is not registered`); + return found; + }; + const as = async (who: CallerName, method: string, routePath: string, req: Record) => { + caller = who; + const res = makeRes(); + await route(method, routePath).handler({ method, headers: {}, query: {}, params: {}, ...req }, res); + return res; + }; + const save = async (type: string, body: { name: string }, mode: 'active' | 'draft') => { + const res = await as('system', 'PUT', `${META}/:type/:name`, { + path: `${META}/${type}/${body.name}`, + params: { type, name: body.name }, + query: mode === 'draft' ? { mode: 'draft' } : {}, + body, + }); + if (res.statusCode !== 200) throw new Error(`seeding ${type}/${body.name} failed: ${JSON.stringify(res.body)}`); + }; + const diff = (type: string, name: string, query: Record = {}) => + as('author', 'GET', `${META}/:type/:name/diff`, { path: `${META}/${type}/${name}/diff`, params: { type, name }, query }); + /** The stored rows, read past every door: the fixture proof each assertion leans on. */ + const storedRow = async (type: string, name: string, state: 'active' | 'draft') => + (await engine.find('sys_metadata', { where: { type, name, state } }))[0] as { version?: unknown } | undefined; + const historyVersions = async (type: string, name: string) => + ((await engine.find('sys_metadata_history', { where: { type, name } })) as Array<{ version: number }>) + .map((r) => r.version) + .sort((a, b) => a - b); + return { save, diff, storedRow, historyVersions }; +} + +const view = (label: string, columns: string[] = ['name']) => + ({ name: 'lead_all', object: 'lead', viewKind: 'list', label, type: 'grid', columns }); +const app = (label: string, draftOnlyEntry = false) => ({ + name: 'atlas', + label, + navigation: [ + { id: 'nav_leads', type: 'page', label: 'Leads', pageName: 'leads_home' }, + ...(draftOnlyEntry ? [{ id: 'nav_launch_plan', type: 'page', label: 'Launch plan', pageName: 'launch_plan' }] : []), + ], +}); + +const FIXTURES = { + view: { type: 'view', name: 'lead_all', make: (label: string, pending: boolean) => view(label, pending ? ['name', 'owner'] : ['name']) }, + app: { type: 'app', name: 'atlas', make: (label: string, pending: boolean) => app(label, pending) }, +} as const; + +const text = (res: any): string => JSON.stringify(res.body ?? null); + +describe('[#20397] GET /meta/:type/:name/diff — the default range labels the to side with the active row\'s own version', () => { + for (const [kind, f] of Object.entries(FIXTURES)) { + it(`${kind}: with a draft pending, toVersion is the active row's version and the to side is that row's body — the explicit range's answer, byte for byte`, async () => { + const b = await boot(); + await b.save(f.type, f.make('Version one', false), 'active'); + await b.save(f.type, f.make('Version two', false), 'active'); + await b.save(f.type, f.make('Pending draft', true), 'draft'); + + // Fixture proof: the draft save appended the newest history row, so + // the newest history version is NOT the active row's. + const active = await b.storedRow(f.type, f.name, 'active'); + expect(active?.version).toBe(2); + expect(await b.historyVersions(f.type, f.name)).toEqual([1, 2, 3]); + + const res = await b.diff(f.type, f.name); + const explicit = await b.diff(f.type, f.name, { from: '1', to: '2' }); + + expect(res.statusCode, text(res)).toBe(200); + expect(explicit.statusCode, text(explicit)).toBe(200); + expect(res.body?.toVersion).toBe(active?.version); + expect(res.body?.fromVersion).toBe(1); + expect(res.body?.changed).toEqual([{ path: 'label', from: 'Version one', to: 'Version two' }]); + expect(res.body).toEqual(explicit.body); + expect(text(res)).not.toContain('Pending draft'); + }, 60_000); + } + + it('app: the card\'s reading — one active save and two draft saves label the to side 1, never 3, over the version-1 body', async () => { + const b = await boot(); + await b.save('app', app('Atlas v1'), 'active'); + await b.save('app', app('Atlas v2 draft', true), 'draft'); + await b.save('app', app('Atlas v3 draft', true), 'draft'); + expect(await b.historyVersions('app', 'atlas')).toEqual([1, 2, 3]); + + const res = await b.diff('app', 'atlas'); + + expect(res.statusCode, text(res)).toBe(200); + expect(res.body?.toVersion).toBe(1); + // Nothing precedes version 1, so the from side is absent: the whole + // version-1 body arrives as added, and no draft save is either side. + expect(res.body?.fromVersion).toBeNull(); + expect(res.body?.removed).toEqual([]); + expect(res.body?.changed).toEqual([]); + expect(res.body?.added).toContainEqual({ path: 'label', value: 'Atlas v1' }); + for (const s of ['Atlas v2 draft', 'Atlas v3 draft', 'nav_launch_plan']) expect(text(res)).not.toContain(s); + }, 60_000); + + it('view: the card\'s reading — one active save and one draft save no longer answer "no changes" labelled 1 → 2', async () => { + const b = await boot(); + await b.save('view', view('A'), 'active'); + await b.save('view', view('B draft', ['name', 'owner']), 'draft'); + + const res = await b.diff('view', 'lead_all'); + + expect(res.statusCode, text(res)).toBe(200); + expect(res.body?.toVersion).toBe(1); + expect(res.body?.fromVersion).toBeNull(); + expect(res.body?.added).toContainEqual({ path: 'label', value: 'A' }); + expect(text(res)).not.toContain('B draft'); + }, 60_000); + + it('view: with no active row (draft saves only), the to side and its label are absent — null on both sides, and no draft content served', async () => { + const b = await boot(); + await b.save('view', view('Only draft one'), 'draft'); + await b.save('view', view('Only draft two', ['name', 'owner']), 'draft'); + expect(await b.storedRow('view', 'lead_all', 'active')).toBeUndefined(); + expect(await b.historyVersions('view', 'lead_all')).toEqual([1, 2]); + + const res = await b.diff('view', 'lead_all'); + + expect(res.statusCode, text(res)).toBe(200); + expect(res.body).toEqual({ + type: 'view', name: 'lead_all', fromVersion: null, toVersion: null, added: [], removed: [], changed: [], + }); + }, 60_000); + + it('control: with no draft pending the default range is unchanged — the last two active saves, labelled 1 → 2', async () => { + const b = await boot(); + await b.save('view', view('A'), 'active'); + await b.save('view', view('B'), 'active'); + + const res = await b.diff('view', 'lead_all'); + + expect(res.statusCode, text(res)).toBe(200); + expect(res.body?.fromVersion).toBe(1); + expect(res.body?.toVersion).toBe(2); + expect(res.body?.changed).toEqual([{ path: 'label', from: 'A', to: 'B' }]); + }, 60_000); +});