Skip to content

Commit 7b3c033

Browse files
hotlongclaude
andauthored
fix(metadata-protocol): resolve the promoted draft under its ADR-0048 package dimension (#8933)
* fix(metadata-protocol): resolve the promoted draft under its ADR-0048 package dimension publishPackageDrafts lists a package's drafts with listDrafts({ packageId }) and then promoted each one through a lookup that dropped the package dimension. ADR-0048 keys overlay rows by (org, type, name, package_id), so with two packages holding drafts for the same (type, name) the two spellings disagree: publishing package A promoted package B's unreviewed draft, drained B's row, recorded it under A's ADR-0067 commit and ADR-0010 audit row, and left A's own edit pending while answering success: true. Which row won was driver-order dependent. Thread the listed row's package_id from listDrafts through promoteDraftForPublish into promoteDraft, mirroring the existing draftOrgId threading for the #3115 org-partition analogue. whereFor already accepted the dimension, so this states a value rather than adding a query shape. The drain is fixed with it: promoteDraft now drops the row it actually promoted, using that row's own binding. An unscoped drain re-runs the same ambiguous lookup, so it could delete another package's draft, and when the bodies differ it instead trips the optimistic-lock mismatch that draftDrainVerdict classifies as benign and silences, leaving the just-published draft pending forever. Callers that name no package (publishMetaItem) are unchanged: an omitted packageId keeps the historical "match any package" resolution. The same question is open for the delete and restoreVersion resolving reads (#6215); they are deliberately untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 * chore(changeset): patch note for the package-scoped draft promotion fix Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 510a4bd commit 7b3c033

4 files changed

Lines changed: 443 additions & 9 deletions

File tree

‎.changeset/eighty-jars-shave.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
---
4+
5+
Publishing a package no longer promotes another package's draft row.
6+
7+
`publishPackageDrafts` lists a package's pending drafts with
8+
`listDrafts({ packageId })`, but the promotion then re-resolved each row without
9+
the ADR-0048 `package_id` dimension. Overlay rows are keyed by
10+
`(org, type, name, package_id)` precisely so two installed packages shipping the
11+
same name keep separate rows, so that lookup could not tell them apart: with two
12+
packages holding drafts for the same `(type, name)`, publishing package A
13+
promoted package B's unreviewed draft to active, drained B's draft row, recorded
14+
it under A's ADR-0067 commit and ADR-0010 audit row, and left A's own edit still
15+
pending — while answering `success: true`. Which of the two rows won was
16+
driver-order dependent, so on a real driver this was a coin toss per publish.
17+
18+
The listed row's `package_id` is now threaded through to the promotion, which
19+
resolves and drains the draft under the same key it was listed by. Publishes
20+
that name no package (`publishMetaItem`) are unchanged.
Lines changed: 305 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,305 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, expect, it } from 'vitest';
4+
// [#5619] The producer's OWN write-verb dispatch decisions (#4550 delete /
5+
// #5480 update), so the fake engine below cannot accept a call ObjectQL
6+
// refuses. Imported from `@objectstack/metadata-core` and not from
7+
// `@objectstack/objectql`: objectql DEPENDS ON this package, so that import
8+
// would close a dependency cycle turbo rejects outright.
9+
import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core';
10+
import { ObjectStackProtocolImplementation } from './protocol.js';
11+
12+
/**
13+
* Regression for #8907 — `publishPackageDrafts` promotes (and drains) ANOTHER
14+
* package's draft row.
15+
*
16+
* Root cause: `publishPackageDrafts` lists the package's drafts with
17+
* `repo.listDrafts({ packageId })` — a `package_id = :packageId` filter — then
18+
* promotes each listed row through `promoteDraftForPublish` →
19+
* `repo.promoteDraft`. `promoteDraft` re-resolved the row it was about to
20+
* promote with `whereFor(ref, 'draft')`, which OMITS the `package_id`
21+
* dimension. ADR-0048 keys overlay rows by `(org, type, name, package_id)`
22+
* precisely so two installed packages shipping the same name each keep their
23+
* OWN row, so that lookup cannot distinguish them: publishing `app.demo`
24+
* promoted whichever `(type, name)` draft the driver returned first.
25+
*
26+
* Measured consequence (the card's reproduction): publishing `app.demo`
27+
* promoted **app.other's** pending draft to active, drained THAT package's
28+
* draft row, recorded it under app.demo's ADR-0067 commit — and left app.demo's
29+
* own edit pending, while answering `success: true`.
30+
*
31+
* ## Why this fixture pins the defect rather than passing by luck
32+
*
33+
* Which of the two rows wins is DRIVER-ORDER DEPENDENT — `findOne` has no
34+
* defined order without an `ORDER BY` — so a fixture that happened to resolve
35+
* the right row would prove nothing. The ordering is therefore made
36+
* deterministic HERE, and the fixture is built so the WRONG row is the one the
37+
* pre-fix code selects:
38+
*
39+
* - the stub's `findRow` falls back to an insertion-ordered scan exactly when
40+
* `package_id` is absent from the `where` (which is the pre-fix promote's
41+
* lookup), and returns the FIRST match;
42+
* - `app.other`'s draft is therefore saved FIRST, so the package-agnostic
43+
* lookup lands on it and not on the package being published.
44+
*
45+
* The first case asserts that precondition explicitly, so a future change to
46+
* insertion order fails loudly instead of silently draining the fixture's
47+
* discriminating power.
48+
*/
49+
50+
interface Row {
51+
id: string;
52+
type: string;
53+
name: string;
54+
organization_id: string | null;
55+
package_id: string | null;
56+
state: string;
57+
metadata: string;
58+
checksum?: string;
59+
version?: number;
60+
updated_at?: string;
61+
created_at?: string;
62+
}
63+
64+
interface HistoryRow {
65+
id: string;
66+
event_seq: number;
67+
name: string;
68+
type: string;
69+
version: number;
70+
operation_type: string;
71+
metadata: string | null;
72+
checksum: string | null;
73+
previous_checksum: string | null;
74+
change_note?: string | null;
75+
source?: string | null;
76+
organization_id: string | null;
77+
recorded_by?: string | null;
78+
recorded_at: string;
79+
}
80+
81+
// Overlay rows are keyed by (type, name, org, state, package_id) — the ADR-0048
82+
// key — so two packages' drafts for the SAME (type, name) coexist as distinct
83+
// rows. That coexistence is the whole precondition of this defect.
84+
function keyOf(w: Record<string, unknown>) {
85+
return `${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}|${w.package_id ?? '__nopkg__'}`;
86+
}
87+
88+
/** Does row `r` satisfy `where` (top-level eq + `$or` + `organization_id IS NULL`)? */
89+
function matchesMetadataWhere(r: Row, where: Record<string, unknown>): boolean {
90+
for (const [k, v] of Object.entries(where)) {
91+
if (k === '$or') {
92+
const clauses = v as Array<Record<string, unknown>>;
93+
if (!clauses.some((c) => matchesMetadataWhere(r, c))) return false;
94+
continue;
95+
}
96+
// `undefined` = "dimension not constrained"; `null` = "must be NULL".
97+
if (v === undefined) continue;
98+
if ((r as any)[k] !== v) return false;
99+
}
100+
return true;
101+
}
102+
103+
function makeStubEngine() {
104+
const rows = new Map<string, Row>();
105+
const historyRows: HistoryRow[] = [];
106+
let nextId = 0;
107+
108+
const findRow = (w: Record<string, unknown>): { key: string; row: Row } | null => {
109+
if (w.id !== undefined) {
110+
for (const [k, r] of rows) if (r.id === w.id) return { key: k, row: r };
111+
return null;
112+
}
113+
// Exact-key lookup when the caller SCOPED the read by package. When
114+
// `package_id` is absent (the pre-fix promote's "match any package"
115+
// lookup) fall back to an insertion-ordered scan returning the FIRST
116+
// match — the deterministic stand-in for a real driver's undefined
117+
// `findOne` order, and what makes the wrong row win below.
118+
if (w.package_id !== undefined) {
119+
const k = keyOf(w);
120+
const r = rows.get(k);
121+
return r ? { key: k, row: r } : null;
122+
}
123+
for (const [k, r] of rows) if (matchesMetadataWhere(r, w)) return { key: k, row: r };
124+
return null;
125+
};
126+
127+
const matchesHistory = (h: HistoryRow, w: Record<string, unknown>): boolean => {
128+
if (w.organization_id !== undefined && h.organization_id !== w.organization_id) return false;
129+
if (w.type !== undefined && h.type !== w.type) return false;
130+
if (w.name !== undefined && h.name !== w.name) return false;
131+
if (w.version !== undefined && h.version !== w.version) return false;
132+
if (w.operation_type !== undefined && h.operation_type !== w.operation_type) return false;
133+
return true;
134+
};
135+
136+
const engine: any = {
137+
async findOne(table: string, opts: { where: Record<string, unknown> }) {
138+
if (table === 'sys_metadata_history') {
139+
return historyRows.find((h) => matchesHistory(h, opts.where)) ?? null;
140+
}
141+
return findRow(opts.where)?.row ?? null;
142+
},
143+
async find(table: string, opts: { where: Record<string, unknown> }) {
144+
if (table === 'sys_metadata_history') {
145+
return historyRows.filter((h) => matchesHistory(h, opts.where));
146+
}
147+
return Array.from(rows.values()).filter((r) => matchesMetadataWhere(r, opts.where));
148+
},
149+
async insert(table: string, data: Record<string, unknown>) {
150+
if (table === 'sys_metadata_audit') return { id: 'audit_skip' };
151+
if (table === 'sys_metadata_history') {
152+
nextId += 1;
153+
const h: HistoryRow = { id: `h_${nextId}`, ...(data as any) };
154+
historyRows.push(h);
155+
return { id: h.id };
156+
}
157+
nextId += 1;
158+
const row = { id: `r_${nextId}`, ...(data as any) } as Row;
159+
rows.set(keyOf(data), row);
160+
return { id: row.id };
161+
},
162+
async update(_t: string, data: Record<string, unknown>, opts: { where: Record<string, unknown> }) {
163+
assertEngineUpdateDispatch(data, opts);
164+
const found = findRow(opts.where);
165+
if (!found) return { id: null };
166+
const merged = { ...found.row, ...(data as any) };
167+
rows.delete(found.key);
168+
rows.set(keyOf(merged), merged);
169+
return { id: found.row.id };
170+
},
171+
async delete(_t: string, opts: { where: Record<string, unknown> }) {
172+
assertEngineDeleteDispatch(opts);
173+
const found = findRow(opts.where);
174+
if (!found) return { deleted: 0 };
175+
rows.delete(found.key);
176+
return { deleted: 1 };
177+
},
178+
async transaction<T>(cb: (ctx: any, info: { owned: boolean }) => Promise<T>): Promise<T> {
179+
return cb(undefined, { owned: true });
180+
},
181+
registry: {
182+
registerItem: () => {},
183+
registerObject: () => {},
184+
// No declared package namespace → publishPackageDrafts skips the
185+
// ADR-0028 prefix check (legacy-grandfathered path).
186+
getPackage: () => undefined,
187+
},
188+
};
189+
return { engine, rows, historyRows };
190+
}
191+
192+
/**
193+
* Two packages ship the SAME object name. `label` is the marker that says which
194+
* package's body a row is carrying — the fact the assertions turn on.
195+
*
196+
* [#8308] Authored OWD: the publish gate refuses an OWD-less custom object
197+
* (`security-owd-unset`).
198+
*/
199+
const objectBody = (name: string, label: string) => ({
200+
name,
201+
label,
202+
sharingModel: 'private',
203+
fields: {
204+
title: { type: 'text', label: 'Title' },
205+
},
206+
});
207+
208+
const draftRowsOf = (rows: Map<string, Row>) =>
209+
Array.from(rows.values()).filter((r) => r.state === 'draft');
210+
const activeRowsOf = (rows: Map<string, Row>) =>
211+
Array.from(rows.values()).filter((r) => r.state === 'active');
212+
const labelOf = (r: Row) => (JSON.parse(r.metadata) as { label?: string }).label;
213+
214+
/** Save the two colliding drafts, `app.other` FIRST — see the file header. */
215+
async function seedTwoPackageDrafts(protocol: ObjectStackProtocolImplementation) {
216+
await protocol.saveMetaItem({
217+
type: 'object',
218+
name: 'shared_ticket',
219+
item: objectBody('shared_ticket', 'FROM_OTHER'),
220+
packageId: 'app.other',
221+
mode: 'draft',
222+
});
223+
await protocol.saveMetaItem({
224+
type: 'object',
225+
name: 'shared_ticket',
226+
item: objectBody('shared_ticket', 'FROM_DEMO'),
227+
packageId: 'app.demo',
228+
mode: 'draft',
229+
});
230+
}
231+
232+
describe('publishPackageDrafts — two packages holding drafts for one (type, name) (#8907)', () => {
233+
it('parks both drafts as distinct ADR-0048 rows, with the OTHER package first in scan order', async () => {
234+
const { engine, rows } = makeStubEngine();
235+
const protocol = new ObjectStackProtocolImplementation(engine);
236+
237+
await seedTwoPackageDrafts(protocol);
238+
239+
const drafts = draftRowsOf(rows);
240+
expect(drafts).toHaveLength(2);
241+
// The precondition the two cases below depend on: a package-agnostic
242+
// scan hits `app.other` FIRST, so resolving without the package
243+
// dimension selects the row that is NOT being published.
244+
expect(drafts.map((r) => r.package_id)).toEqual(['app.other', 'app.demo']);
245+
expect(drafts.map(labelOf)).toEqual(['FROM_OTHER', 'FROM_DEMO']);
246+
});
247+
248+
it('promotes the PUBLISHING package own draft, not the first row that shares the name', async () => {
249+
const { engine, rows } = makeStubEngine();
250+
const protocol = new ObjectStackProtocolImplementation(engine);
251+
252+
await seedTwoPackageDrafts(protocol);
253+
254+
const res = await protocol.publishPackageDrafts({ packageId: 'app.demo' });
255+
256+
expect(res.failed).toEqual([]);
257+
expect(res).toMatchObject({ success: true, publishedCount: 1, failedCount: 0 });
258+
259+
// Before the fix the active row carried `package_id: 'app.other'` and
260+
// the body labelled FROM_OTHER — app.other's unreviewed pending change,
261+
// published under app.demo's commit.
262+
const active = activeRowsOf(rows);
263+
expect(active).toHaveLength(1);
264+
expect(active[0].package_id).toBe('app.demo');
265+
expect(labelOf(active[0])).toBe('FROM_DEMO');
266+
});
267+
268+
it('drains the published package own draft and leaves the other package draft pending', async () => {
269+
const { engine, rows } = makeStubEngine();
270+
const protocol = new ObjectStackProtocolImplementation(engine);
271+
272+
await seedTwoPackageDrafts(protocol);
273+
await protocol.publishPackageDrafts({ packageId: 'app.demo' });
274+
275+
// Exactly one draft survives, and it is the one nobody published.
276+
// Before the fix this was inverted: app.other's row was drained and
277+
// app.demo's edit stayed pending while the response said "published".
278+
const drafts = draftRowsOf(rows);
279+
expect(drafts).toHaveLength(1);
280+
expect(drafts[0].package_id).toBe('app.other');
281+
expect(labelOf(drafts[0])).toBe('FROM_OTHER');
282+
});
283+
284+
it('still publishes a lone package-bound draft (no regression)', async () => {
285+
const { engine, rows } = makeStubEngine();
286+
const protocol = new ObjectStackProtocolImplementation(engine);
287+
288+
await protocol.saveMetaItem({
289+
type: 'object',
290+
name: 'solo_ticket',
291+
item: objectBody('solo_ticket', 'SOLO'),
292+
packageId: 'app.demo',
293+
mode: 'draft',
294+
});
295+
296+
const res = await protocol.publishPackageDrafts({ packageId: 'app.demo' });
297+
298+
expect(res).toMatchObject({ success: true, publishedCount: 1, failedCount: 0 });
299+
expect(draftRowsOf(rows)).toHaveLength(0);
300+
const active = activeRowsOf(rows);
301+
expect(active).toHaveLength(1);
302+
expect(active[0].package_id).toBe('app.demo');
303+
expect(labelOf(active[0])).toBe('SOLO');
304+
});
305+
});

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13246,6 +13246,19 @@ export class ObjectStackProtocolImplementation implements
1324613246
*/
1324713247
private async promoteDraftForPublish(request: {
1324813248
type: string; name: string; organizationId?: string; actor?: string; message?: string;
13249+
/**
13250+
* [#8907] ADR-0048 — the package binding of the draft being promoted,
13251+
* when the caller listed it under one. Threaded straight into
13252+
* `repo.promoteDraft` so the promotion resolves the draft under the
13253+
* SAME key it was listed by, exactly as `organizationId` above threads
13254+
* the draft's own org scope for the #3115 partition analogue.
13255+
*
13256+
* `undefined` (the `publishMetaItem` path, which names no package)
13257+
* keeps the historical "match any package" resolution. `null` pins the
13258+
* lookup to the unbound row — so the field is passed through only when
13259+
* the caller actually has a binding to state.
13260+
*/
13261+
packageId?: string | null;
1324913262
}): Promise<{
1325013263
singularType: string;
1325113264
orgId: string | null;
@@ -13334,6 +13347,11 @@ export class ObjectStackProtocolImplementation implements
1333413347
source: 'protocol.publishMetaItem',
1333513348
...(request.message ? { message: request.message } : {}),
1333613349
intent,
13350+
// [#8907] Spread, not `packageId: request.packageId`: `null` is
13351+
// a meaningful scope (the unbound row) and `undefined` means
13352+
// "no package in hand", so the key must be ABSENT rather than
13353+
// present-and-undefined for the historical resolution to hold.
13354+
...('packageId' in request ? { packageId: request.packageId ?? null } : {}),
1333713355
});
1333813356
return { singularType, orgId, result };
1333913357
} catch (err: any) {
@@ -13963,6 +13981,18 @@ export class ObjectStackProtocolImplementation implements
1396313981
type: d.type,
1396413982
name: d.name,
1396513983
...(draftOrgId ? { organizationId: draftOrgId } : {}),
13984+
// [#8907] Promote each draft under the PACKAGE
13985+
// `listDrafts` surfaced it from, for the same reason
13986+
// `draftOrgId` above threads its org: ADR-0048 keys
13987+
// overlay rows by `(org, type, name, package_id)`,
13988+
// so with two packages holding drafts for one
13989+
// `(type, name)` a promote that omits the package
13990+
// dimension cannot tell them apart — publishing A
13991+
// promoted and drained B's row, left A's own edit
13992+
// pending, and still answered `success: true`.
13993+
// `d.packageId` is the row's own binding, so this
13994+
// is the listed key restated, never a new one.
13995+
packageId: d.packageId,
1396613996
...(request.actor ? { actor: request.actor } : {}),
1396713997
message: `publish app package '${request.packageId}'`,
1396813998
});

0 commit comments

Comments
 (0)