diff --git a/.changeset/hashspec-serialized-form.md b/.changeset/hashspec-serialized-form.md new file mode 100644 index 0000000000..02a03a5ded --- /dev/null +++ b/.changeset/hashspec-serialized-form.md @@ -0,0 +1,65 @@ +--- +"@objectstack/metadata-core": minor +"@objectstack/metadata-fs": patch +--- + +fix(metadata-core,metadata-fs): hash the serialized form, so `put().version` identifies the bytes actually stored (#7856) + +`hashSpec` canonicalised a `Date` to `{}`, because `canonicalize` walked a +value's own enumerable keys and a `Date` has none. `JSON.stringify` — what every +repository actually writes — turns the same `Date` into an ISO string. So the +hash of the in-memory spec and the hash of the bytes on disk were **different +hashes for the same item**, and the version handed back to a caller did not +identify what had been stored. + +Measured on `main`, one spec carrying one `Date`: + +``` +canonicalize(in-memory) : {"createdAt":{},"label":"Home"} +JSON.stringify (bytes) : {"label":"Home","createdAt":"2024-01-01T00:00:00.000Z"} +``` + +`canonicalize` now honours `toJSON` exactly as `JSON.stringify` does — +consulted once per position, its result serialised as-is and never +re-consulted — which makes a new guarantee true by construction: + +``` +canonicalize(x) === canonicalize(JSON.parse(JSON.stringify(x))) +``` + +**Both repository implementations were wrong, in different places**, which is +why the fix is one function rather than two patches. `FileSystemRepository` +broke `put().version === get().hash`: it hashed the spec it was handed, wrote +`JSON.stringify` of it, and re-hashed the parse on the way back out. +`InMemoryRepository` broke the repository contract's invariant 4 +(`item.hash === hashSpec(item.body)`): it stores `body` already serialised +(`clonePlain`) while hashing the in-memory spec, so the item it returns +disagreed with its own hash. `SysMetadataRepository` inherits the fix through +the same function. + +Downstream, an incoherent version meant a repository could report an +`{op:'update', actor:'fs'}` for a file nothing outside the process had touched: +the head index held a hash the disk could never reproduce, so re-reading one's +own write looked like somebody else's edit. That surfaces without any watcher — +a restart rebuilds the index from disk and the version the caller was handed no +longer matches it. + +**Ordinary specs hash exactly as before, and this is not a migration.** The new +path diverges only at a position carrying a callable `toJSON`; a graph without +one is byte-identical through `canonicalize`. Verified against this repository's +entire checked-in JSON corpus — 1973 files hashed under both the old and the new +implementation, **0 hashes changed** — and the `hashSpec({})` regression guard +in `metadata-core` is unmoved. Stored versions for ordinary specs keep their +meaning. Versions for `toJSON`-carrying specs do change, and those are exactly +the versions that never identified their stored bytes in the first place. + +Also supported as a consequence: a class instance with a `toJSON` now hashes as +whatever it serialises to, rather than as its private fields. One without a +`toJSON` still hashes as its own enumerable keys — which is what +`JSON.stringify` writes for it. + +The pin is table-driven and lives in the shared repository contract suite, so +every `MetadataRepository` implementation is held to it: `Date` at a key, `Date` +under an array index, a class whose `toJSON` yields a string, an object literal +carrying its own `toJSON`, a nested case, and a plain-JSON control row that +proves the fix did not simply change every hash. diff --git a/packages/metadata-core/src/canonicalize.ts b/packages/metadata-core/src/canonicalize.ts index 55c08dac55..615107405e 100644 --- a/packages/metadata-core/src/canonicalize.ts +++ b/packages/metadata-core/src/canonicalize.ts @@ -16,12 +16,39 @@ * they cannot survive a JSON round trip. * 5. **Idempotence.** `canonicalize(canonicalize(x))` === `canonicalize(x)`. * 6. **Pure.** No side-effects, no mutation of input. + * 7. **Serialized-form identity (#7856).** The hash describes the bytes a + * value serialises to, never the in-memory object graph that produced + * them. Formally, for every `x` this module accepts: + * + * canonicalize(x) === canonicalize(JSON.parse(JSON.stringify(x))) + * + * This is the guarantee that makes a repository's + * `put(spec).version === get().hash` hold: `put` hashes the value it + * was handed, `get` hashes what it parsed back off the disk, and + * guarantee 7 says those are one hash. + * + * It is delivered by honouring `toJSON` exactly as `JSON.stringify` + * does — consulted once per position, its result serialised as-is and + * never re-consulted. Before #7856 `normalise` ignored `toJSON` and + * walked own enumerable keys instead, so a `Date` canonicalised to a + * key-less `{}` while the bytes on disk held an ISO string, and the two + * hashed differently: the version handed to a caller did not identify + * the bytes stored, and re-reading one's own write looked like an + * external edit. + * + * Values carrying no `toJSON` anywhere in the graph — ordinary specs — + * are untouched by this: their canonical form is byte-identical to what + * it was before, so no already-stored version changes meaning. * * Non-goals (deliberately not supported): * - * - Functions, symbols, class instances. These have no canonical JSON - * form. Callers must serialise out-of-band (e.g. for formula fields, - * use the CEL string, not the compiled function). + * - Functions and symbols. These have no canonical JSON form. Callers must + * serialise out-of-band (e.g. for formula fields, use the CEL string, + * not the compiled function). + * - Class instances *without* a `toJSON`. One that has a `toJSON` is + * supported by guarantee 7 and hashes as whatever it serialises to; + * one that does not still hashes as its own enumerable keys, which is + * exactly what `JSON.stringify` writes for it. * - BigInt. Rejected because there is no agreed-upon JSON representation. */ @@ -29,11 +56,36 @@ import { createHash } from 'node:crypto'; /** Stable JSON serialisation. See module-level doc for guarantees. */ export function canonicalize(value: unknown): string { - return JSON.stringify(normalise(value)); + // `''` is the key `JSON.stringify` hands a root-position `toJSON`. + return JSON.stringify(normalise(value, '')); +} + +/** + * Resolve one position's value the way `JSON.stringify` does: if it carries + * a callable `toJSON`, that is what gets serialised. + * + * Applied ONCE per position, per the `SerializeJSONProperty` algorithm — the + * returned value is serialised as-is and is never itself re-examined for a + * `toJSON`. `normalise` therefore calls this on entry and then dispatches on + * the result, recursing (and so re-applying it) only for child positions. + */ +function resolveToJson(value: unknown, key: string): unknown { + if (value === null || typeof value !== 'object') return value; + const toJson = (value as { toJSON?: unknown }).toJSON; + if (typeof toJson !== 'function') return value; + return (toJson as (this: unknown, key: string) => unknown).call(value, key); } -/** Convert a value into a canonical, JSON-serialisable form. */ -function normalise(value: unknown): unknown { +/** + * Convert a value into a canonical, JSON-serialisable form. + * + * @param value Raw value at this position. + * @param key The position's key — `''` at the root, the property name + * inside an object, the stringified index inside an array. + * Passed to `toJSON` because `JSON.stringify` passes it. + */ +function normalise(rawValue: unknown, key: string): unknown { + const value = resolveToJson(rawValue, key); if (value === null) return null; if (typeof value === 'undefined') return undefined; // caller-dropped if (typeof value === 'number') { @@ -51,7 +103,11 @@ function normalise(value: unknown): unknown { if (typeof value === 'string' || typeof value === 'boolean') return value; if (Array.isArray(value)) { - return value.map(normalise); + // NOT `value.map(normalise)` — `Array.prototype.map` passes the index as + // the second argument, which is this function's `key` parameter. The + // index is the right key, but it has to arrive as the string + // `JSON.stringify` would hand a `toJSON`, not as a number. + return value.map((element, index) => normalise(element, String(index))); } // Plain object: sort keys, drop undefineds. @@ -60,7 +116,7 @@ function normalise(value: unknown): unknown { const out: Record = {}; const keys = Object.keys(obj).sort(); for (const k of keys) { - const v = normalise(obj[k]); + const v = normalise(obj[k], k); if (typeof v === 'undefined') continue; out[k] = v; } diff --git a/packages/metadata-core/src/contract-suite.ts b/packages/metadata-core/src/contract-suite.ts index 5d30ace144..cb83eb3e69 100644 --- a/packages/metadata-core/src/contract-suite.ts +++ b/packages/metadata-core/src/contract-suite.ts @@ -38,6 +38,64 @@ const refOf = (overrides: Partial = {}): MetaRef => ({ const spec = (label: string) => ({ label, columns: ['a', 'b'] }); +/** A value whose `toJSON` collapses it to something other than its own keys. */ +class Money { + constructor( + private readonly cents: number, + private readonly currency: string, + ) {} + + toJSON(): string { + return `${(this.cents / 100).toFixed(2)} ${this.currency}`; + } +} + +/** + * Spec shapes for the #7856 identity pin, spanning the ways a value's + * serialised form can differ from its in-memory object graph. + * + * Deliberately a TABLE and not one hand-picked `Date`. The defect was never + * about `Date` — it was about `canonicalize` walking own enumerable keys + * while the disk received whatever `JSON.stringify` produced — and a + * single-case pin is exactly what lets the next value in this class through. + * `Date` is merely its most reachable instance; a `toJSON` returning a + * string, an object, or sitting under an array index are the same defect + * wearing different clothes, and each row below failed differently before + * the fix (see the PR's per-arm table). + * + * The control row matters as much as the rest: it is what proves a fix here + * did not simply change every hash. + */ +const SERIALISATION_SHAPES: ReadonlyArray<{ + label: string; + spec: () => Record; +}> = [ + { + label: 'control — plain JSON, no toJSON anywhere in the graph', + spec: () => ({ label: 'Home', columns: ['a', 'b'], nested: { n: 1, ok: true, nil: null } }), + }, + { + label: 'Date at a top-level key', + spec: () => ({ label: 'Home', createdAt: new Date('2024-01-01T00:00:00.000Z') }), + }, + { + label: 'Date under an array index', + spec: () => ({ label: 'Audit', stamps: [new Date('2020-06-01T12:00:00.000Z')] }), + }, + { + label: 'class instance whose toJSON collapses it to a string', + spec: () => ({ label: 'Price', amount: new Money(1250, 'USD') }), + }, + { + label: 'object literal carrying its own toJSON', + spec: () => ({ label: 'Range', span: { toJSON: () => ({ from: 1, to: 9 }) } }), + }, + { + label: 'toJSON nested inside an array element', + spec: () => ({ rows: [{ at: new Date('2022-02-02T02:02:02.000Z') }] }), + }, +]; + /** Drain at most `n` events from an async iterable with a timeout. */ async function take(iter: AsyncIterable, n: number, timeoutMs = 1000): Promise { const out: T[] = []; @@ -104,6 +162,64 @@ export function runRepositoryContractTests( expect(got!.hash).toBe(hashSpec(got!.body)); }); + // ── #7856 — the version identifies the STORED BYTES ─────────── + // + // Table-driven on purpose; see SERIALISATION_SHAPES. Every row + // asserts BOTH faces of the same invariant, because the two + // implementations in this repo broke DIFFERENT ones: + // + // put().version === get().hash + // the face FileSystemRepository broke — it hashed the spec it + // was handed, wrote `JSON.stringify` of it, and re-hashed the + // parse on the way back out. + // get().hash === hashSpec(get().body) + // invariant 4's face, the one InMemoryRepository broke — it + // stores `body` already serialised (`clonePlain`) while + // hashing the in-memory spec, so the item it hands back + // disagrees with its own hash. + // + // Asserting only one face would have left the other implementation's + // divergence unpinned, which is the whole reason this lives in the + // shared contract suite rather than beside either bug. + describe('serialized-form identity (#7856)', () => { + for (const shape of SERIALISATION_SHAPES) { + it(`version identifies the stored bytes — ${shape.label}`, async () => { + const repo = await factory(); + const ref = refOf(); + const put = await repo.put(ref, shape.spec(), { + parentVersion: null, + actor: 't', + }); + + const got = await repo.get(ref); + expect(got).not.toBeNull(); + expect(got!.hash).toBe(put.version); + expect(got!.hash).toBe(hashSpec(got!.body)); + }); + + it(`re-putting the same spec is a no-op — ${shape.label}`, async () => { + const repo = await factory(); + const ref = refOf(); + const first = await repo.put(ref, shape.spec(), { + parentVersion: null, + actor: 't', + }); + + // Chains on the version the caller was HANDED. When that + // version does not identify the stored bytes, the head index + // it is compared against holds the other hash and this second + // write is not recognised as the no-op it is — it either + // conflicts or republishes the item as a fresh revision. + const second = await repo.put(ref, shape.spec(), { + parentVersion: first.version, + actor: 't', + }); + expect(second.version).toBe(first.version); + expect(second.seq).toBe(first.seq); + }); + } + }); + it('returns null for missing item', async () => { const repo = await factory(); expect(await repo.get(refOf({ name: 'never_existed' }))).toBeNull(); diff --git a/packages/metadata-core/test/canonicalize.test.ts b/packages/metadata-core/test/canonicalize.test.ts index 087d8ad65f..b70716efb8 100644 --- a/packages/metadata-core/test/canonicalize.test.ts +++ b/packages/metadata-core/test/canonicalize.test.ts @@ -147,4 +147,86 @@ describe('hashSpec', () => { { numRuns: 100 }, ); }); + + /** + * Guarantee 7 (#7856): the hash describes the bytes a value serialises to. + * + * canonicalize(x) === canonicalize(JSON.parse(JSON.stringify(x))) + * + * Stated as a property and a table rather than as a list of golden hashes, + * because the defect was never about any particular value — it was about + * `normalise` walking own enumerable keys while the disk received + * `JSON.stringify`'s output. Everything with a `toJSON` diverged; `Date` is + * simply the instance everybody meets first. + */ + describe('serialized-form identity (#7856)', () => { + const roundTrip = (x: unknown): unknown => JSON.parse(JSON.stringify(x)); + + class Money { + constructor( + private readonly cents: number, + private readonly currency: string, + ) {} + + toJSON(): string { + return `${(this.cents / 100).toFixed(2)} ${this.currency}`; + } + } + + const CASES: ReadonlyArray<{ label: string; value: unknown; canonical: string }> = [ + { + label: 'Date at a key', + value: { createdAt: new Date('2024-01-01T00:00:00.000Z'), label: 'H' }, + canonical: '{"createdAt":"2024-01-01T00:00:00.000Z","label":"H"}', + }, + { + label: 'Date under an array index', + value: { stamps: [new Date('2020-06-01T12:00:00.000Z')] }, + canonical: '{"stamps":["2020-06-01T12:00:00.000Z"]}', + }, + { + label: 'class instance whose toJSON yields a string', + value: { amount: new Money(1250, 'USD') }, + canonical: '{"amount":"12.50 USD"}', + }, + { + label: 'object literal carrying its own toJSON', + value: { span: { toJSON: () => ({ to: 9, from: 1 }) } }, + canonical: '{"span":{"from":1,"to":9}}', + }, + ]; + + for (const c of CASES) { + it(`canonicalises to the serialised form — ${c.label}`, () => { + expect(canonicalize(c.value)).toBe(c.canonical); + expect(hashSpec(c.value)).toBe(hashSpec(roundTrip(c.value))); + }); + } + + it('applies toJSON once per position, never re-consulting its result', () => { + // `JSON.stringify` applies toJSON exactly once per position, so a result + // that itself carries a toJSON is serialised literally. Matching + // JSON.stringify IS the contract, so it is asserted against it directly. + const value = { wrapped: { toJSON: () => ({ inner: { toJSON: () => 'deep' } }) } }; + expect(canonicalize(value)).toBe(JSON.stringify(roundTrip(value))); + }); + + it('leaves a graph with no toJSON byte-identical', () => { + // The half that makes this a fix rather than a migration: an ordinary + // spec's canonical form must be exactly what it has always been. + expect(canonicalize({ b: 1, a: [1, 2, { c: null }], d: 'x' })).toBe( + '{"a":[1,2,{"c":null}],"b":1,"d":"x"}', + ); + }); + + it('property: hashing is invariant under a JSON round trip', () => { + fc.assert( + fc.property( + fc.dictionary(fc.string({ minLength: 1, maxLength: 6 }), fc.jsonValue()), + (obj) => hashSpec(obj) === hashSpec(roundTrip(obj)), + ), + { numRuns: 200 }, + ); + }); + }); }); diff --git a/packages/metadata-fs/test/self-write-suppression.test.ts b/packages/metadata-fs/test/self-write-suppression.test.ts index c500d9e421..6347c41727 100644 --- a/packages/metadata-fs/test/self-write-suppression.test.ts +++ b/packages/metadata-fs/test/self-write-suppression.test.ts @@ -193,6 +193,124 @@ describe('#7335 self-write suppression is keyed on observed content, not on a cl } }); +/** + * #7856 — a spec whose serialised form differs from its in-memory graph is + * not an "external edit". + * + * Same seam, adjacent defect, and the reason it belongs beside #7335: both + * decide whether a watcher event is OURS by comparing observed content to + * the head index, so both are wrong the moment the index holds a hash the + * disk can never reproduce. `put()` hashed the spec it was handed while the + * file received `JSON.stringify` of it, so for a `Date`-carrying spec the + * index held the hash of `{}` and every re-read of our OWN write looked like + * somebody else's edit. + * + * The three cases below separate consequences the filing bundled together, + * because they do not share a trigger: + * + * 1. `put().version === get().hash` — no watcher, no second write. + * 2. a restart, whose `scanHeads()` rebuilds the index FROM DISK — this is + * where the incoherent index becomes observable without any watcher at + * all, as a ConflictError against the version the caller was handed. + * 3. a watcher event at lag ≈ 0 — the spurious `{op:'update', actor:'fs'}`. + * + * Case 2 is the one worth being precise about: with the watcher disabled AND + * the process still holding its in-memory index, a second `put()` of the same + * spec is recognised as a no-op even on the pre-fix tree, because both sides + * of that comparison are the same wrong hash. It takes a restart — or the + * watcher — to surface it. Measured both ways; see the PR's per-arm table. + */ +describe('#7856 a self-written spec is never republished as an external edit', () => { + let root: string; + let repo: FileSystemRepository | null = null; + + /** Carries a `Date`: in-memory `{}`, on disk an ISO string. */ + const dateSpec = () => ({ label: 'ours', createdAt: new Date('2024-01-01T00:00:00.000Z') }); + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'os-7856-')); + }); + + afterEach(async () => { + if (repo) await repo.close(); + repo = null; + await fs.rm(root, { recursive: true, force: true }); + }); + + it('the version handed back identifies the bytes on disk', async () => { + repo = new FileSystemRepository({ root, org: 'system', disableWatch: true }); + await repo.start(); + const r = ref('dated'); + + const put = await repo.put(r, dateSpec(), { parentVersion: null, actor: 't' }); + const got = await repo.get(r); + + expect(got).not.toBeNull(); + expect(got!.hash).toBe(put.version); + // The stored body really is the serialised form — this is what makes the + // assertion above a claim about BYTES and not about object identity. + expect(got!.body).toEqual({ label: 'ours', createdAt: '2024-01-01T00:00:00.000Z' }); + }); + + it('survives a restart: the rebuilt head index still matches the handed-out version', async () => { + repo = new FileSystemRepository({ root, org: 'system', disableWatch: true }); + await repo.start(); + const r = ref('dated'); + const put = await repo.put(r, dateSpec(), { parentVersion: null, actor: 't' }); + await repo.close(); + + // A fresh repository over the same root: `scanHeads()` rebuilds the index + // by hashing what is ON DISK. No watcher is involved anywhere here. + repo = new FileSystemRepository({ root, org: 'system', disableWatch: true }); + await repo.start(); + + // Chaining on the version the first process returned must still be + // recognised as the no-op it is, rather than raising ConflictError + // against a head the caller was never told about. + const second = await repo.put(r, dateSpec(), { parentVersion: put.version, actor: 't' }); + expect(second.version).toBe(put.version); + }); + + it('a watcher event for our own write publishes nothing', async () => { + repo = new FileSystemRepository({ root, org: 'system', disableWatch: true }); + await repo.start(); + const r = ref('dated'); + const file = path.join(root, 'view', 'dated.json'); + + await repo.put(r, dateSpec(), { parentVersion: null, actor: 't' }); + const before = await readLog(root); + + // Delivered at lag ≈ 0, exactly as the #7335 cases above do it. + await deliver(repo, file, 'change'); + + // Pre-fix this appended `{op:'update', actor:'fs'}` — the repository + // reporting an external actor for a file nothing outside it touched. + expect(await readLog(root)).toEqual(before); + }); + + it('still detects a GENUINE external edit to a Date-carrying item', async () => { + repo = new FileSystemRepository({ root, org: 'system', disableWatch: true }); + await repo.start(); + const r = ref('dated'); + const file = path.join(root, 'view', 'dated.json'); + + await repo.put(r, dateSpec(), { parentVersion: null, actor: 't' }); + const externalSpec = { label: 'theirs', createdAt: '2024-01-01T00:00:00.000Z' }; + await fs.writeFile(file, JSON.stringify(externalSpec, null, 2) + '\n', 'utf8'); + const before = await readLog(root); + + await deliver(repo, file, 'change'); + + // The complementary direction: suppressing the false positive must not be + // achieved by blinding the watcher on this class of item. + const added = (await readLog(root)).slice(before.length); + expect(added).toHaveLength(1); + expect(added[0]!.op).toBe('update'); + expect(added[0]!.actor).toBe('fs'); + expect(added[0]!.hash).toBe(hashSpec(externalSpec)); + }); +}); + /** The durable change log — the record a swallow erases. */ async function readLog(root: string): Promise { const file = path.join(root, '.objectstack', '.log', 'main.jsonl');