Skip to content

Commit 2626bdd

Browse files
fix(metadata-protocol): a plain clone does not report the source's empty readonly columns as dropped fields (#29290)
<!-- claude-projects-attribution: {"github_login":"os-steve"} --> _Requested by **Steve** · [project thread](https://claude.ai/code/project/chan_01FdL1XeUyvVNxLreh3F4Qi9?thread=cmsg_01FdL1XeUyvVNxLreh3F4Qi935ZoSza1EBztq9TAUZYgTk)_ Fixes #29200 Clause-②: no Metadata-protocol: none ## 维护者速读 - **改了什么**:克隆记录时,源记录里为空的只读字段不再被复制进新记录的插入数据,所以一次什么都没改的克隆不会再在响应里报告"字段被丢弃",`X-ObjectStack-Dropped-Fields` 响应头也不再出现。 - **为什么改**:控制台每次克隆一条只读字段为空的记录(如 showcase 联系人的 `lead_score`),都会提示"某些字段被丢弃";集成方读 `droppedFields` 或响应头,会把每次克隆都当成丢了数据。用户其实什么都没发。 - **风险与回滚**:存下来的新记录与修复前逐字节相同(空值本来就会被剥掉、插入时空值与缺省同样取默认值)。只读字段有值时照旧复制、剥离并报告(#15703 裁决),`overrides` 里带只读字段也照旧报告。回滚即 revert 本 PR。 - **先看**:`packages/metadata-protocol/src/protocol.ts` 的 `cloneData`(新增一行与注释);`packages/rest/src/rest-dropped-fields-every-write-door.test.ts` 的 clone 用例(新增 3 个)。 - **两分钟验证**:`pnpm --filter @objectstack/rest exec vitest run src/rest-dropped-fields-every-write-door.test.ts`,预期 18 个用例全过(需先构建 `@objectstack/metadata-protocol`)。 - **需要你做的**:无,席位落地。 ## What changes `cloneData` (`packages/metadata-protocol/src/protocol.ts`) copied every column of the source row into the insert, read-only ones included, even where the source held `null`. `engine.insert`'s static-readonly strip judges every present key as caller-supplied, so it took that empty key and reported it. A plain clone (no `overrides`) of a row whose read-only column was empty therefore answered `droppedFields` naming that column, and since #29034 the `X-ObjectStack-Dropped-Fields` header repeated it. The fix is on the producer side, as the contract review on #29034 ruled: in the loop that already removes engine-owned columns from the copy, a field declared `readonly` whose copied value is `null`/`undefined` is now left out of the insert payload. Nothing changes in `packages/rest/src/error-response.ts`; the header still mirrors the body. | clone | body `droppedFields` before → after | header before → after | |:--|:--|:--| | no overrides, source's read-only column empty | names the column → absent | names the column → absent | | no overrides, source's read-only column holds a value | names the column → names the column | `ro;reason=readonly` → same | | `overrides` sets the read-only field | names the column → names the column | `ro;reason=readonly` → same | **Why the stored row is unchanged.** For a non-system caller the strip removed the empty key anyway. For every caller, the insert's `applyFieldDefaults` treats an explicit `null` exactly as an omitted key (`out[f.name] != null` is its only skip), so the copy takes the same default either way. Injected platform columns (`organization_id`, owner, audit) are `system: true` and were already removed from the copy by the existing branch. **Acceptance 3 (a read-only column that holds a value) — measured, kept.** On `main` at `45e9b2f1`, a plain clone of a row whose `ro` holds `'seeded'` answers 201 with `droppedFields: [{ object, fields: ['ro'], reason: 'readonly' }]` and header `ro;reason=readonly`; after the fix it answers the same (the new control case passes on both trees). This is not the same defect: the value was really copied from the source and really stripped, so the caller reads the default back, and the maintainer ruling on #15703 (option 1, record 5578819560) is exactly that the clone reports this strip so the caller is told why. The card's route (a) is therefore applied to EMPTY read-only values only; dropping valued read-only columns from the copy would silence a report that ruling requires. Route (b) (the engine strip not reporting a present `null` key) was not taken: it changes the #3431 "fields stripped from THIS write" report on every insert path, and a caller that really sends `ro: null` on a create did send that key. ## Why The console announces every non-empty `droppedFields`, so users saw a "fields dropped" notice on a clone where they changed nothing, and integrations reading the body or the header would treat every such clone as data loss. ## Tests - `packages/rest/src/rest-dropped-fields-every-write-door.test.ts` (real `ObjectQL` + protocol + `RestServer` over better-sqlite3), three new clone cases on a new seed row `src_empty` (read-only `ro` left empty): - a plain clone of `src_empty`: 201, no `droppedFields`, no header, `record.ro` null (red on `main`); - `overrides: { ro: 'forged' }` on `src_empty`: still reported, body and header (control, acceptance 2); - a plain clone of `src1` (`ro: 'seeded'`): still reported, body and header (control, acceptance 3, #15703). - `packages/qa/dogfood/test/showcase-static-readonly.dogfood.test.ts`: new case, a plain clone of a stock showcase contact whose `lead_score` is empty answers no `droppedFields` and no header. **Forced test edits** (assertions that accommodated the defect): - dogfood `clone with overrides.lead_score`: the header assertion moves from `toContain('lead_score;reason=readonly')` to `toBe('lead_score;reason=readonly')`, and its comment ("a plain clone copies the source's other readonly columns too, so the header may name more than lead_score") is rewritten, since the copy no longer carries empty read-only columns. - rest test file: the comment above `CLONE_OBJECT` ("a clone of a PROJ row also reports the source's `ro2`") is rewritten to say only valued read-only columns are copied; no assertion moved. The seed gains one row (`src_empty`); no case counts that object's rows. ## Acceptance notes - Serial note: open PR #28731 edits `protocol.ts` at `probeRecord`; this diff touches only `cloneData`. - The showcase cannot exercise acceptance 3 (its only read-only column, `lead_score`, cannot be given a value by a non-system caller); the real-stack rest test carries it. ## Verification Head: `85ebba60` (merge of `origin/main` at `f6a04e3a` over fix `f825d6bd`; red-first base `45e9b2f1`). <details><summary>Red first, fix, ablation</summary> - Red on `main` (`45e9b2f1`, tests only, `@objectstack/rest^...` built): `rest-dropped-fields-every-write-door.test.ts` Tests 1 failed | 17 passed (18); the failure is the new plain-clone case, `expected { object: 'hdr_drops_clone', ... } to not have property "droppedFields"`. The valued-column and overrides controls passed on `main`. - Fix (`f825d6bd`, `metadata-protocol` rebuilt): Tests 18 passed (18). - Ablation through `scripts/ablation-replace.mjs` (anchor hit 1 -> 0, blob changed), the new line made dead by a `name === '__ABLATION_29200__'` guard; `ablation-dist-preflight.mjs` found the marker in `dist/index.js` and `dist/index.cjs`. Rest pin: Tests 1 failed | 17 passed (the plain-clone case). Dogfood file: Tests 1 failed | 8 passed (the new plain-clone case, body named `lead_score`). Restore leg each time: blob == HEAD, `git diff HEAD` empty, rebuilt, preflight `--absent` exit 0, Tests 18/18 and 9/9. </details> <details><summary>Package suites and gates</summary> - `pnpm --filter @objectstack/metadata-protocol test && typecheck`: Test Files 300 passed, 4 skipped (304); Tests 22100 passed, 26 skipped; VERDICT command-exit 0 (at `f825d6bd`). - `pnpm --filter @objectstack/rest test && typecheck`: Test Files 367 passed (367); Tests 6966 passed, 332 skipped; VERDICT command-exit 0 (at `f825d6bd`). - After merging `origin/main` (`85ebba60`): `pnpm install --frozen-lockfile`, build of the `@objectstack/rest^...` and `@objectstack/dogfood^...` closures, `pnpm --filter @objectstack/spec check:generated`: VERDICT command-exit 0. Rest pin Tests 18/18, dogfood `showcase-static-readonly` Tests 9/9. - `node scripts/pm/dispatch-gates.mjs --commands`: 65 derived; all 65 recorded `exit 0` at `85ebba60` (`check:dual-build-cjs-loads` first exit 3, PREREQUISITE NOT MET, re-run exit 0 after building `./packages/**`). `--ran`: 65 run, 0 NOT-MEASURED, 0 UNRUN. - eslint (`--no-inline-config`) on the three touched source and test files: exit 0. - Full dogfood battery: not owed (no `!`, no narrowing arm). </details> ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes. - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed. (not touched) - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. - [x] The *Before Submitting* list in CONTRIBUTING.md is done. --- _Generated by [Claude Code](https://claude.ai/code/session_01ADHkmQZyRQCzui7zekMYY7)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5780fa4 commit 2626bdd

4 files changed

Lines changed: 78 additions & 6 deletions

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
fix(metadata-protocol): a plain clone no longer reports the source's empty read-only columns as dropped fields
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
`cloneData` copied every column of the source into the insert, read-only ones included, even where the source held no value. The engine's read-only strip judged each such column a caller-supplied key and reported it, so a clone with no `overrides` of a row whose read-only column was empty answered `droppedFields` naming that column, and the REST `X-ObjectStack-Dropped-Fields` header repeated it. Nothing had been sent and nothing was lost, but the console announced "fields dropped" on every such clone.
11+
12+
The clone now leaves a read-only column whose source value is empty (`null`) out of the copy. The stored row is the same as before: the strip removed that key from a non-system caller anyway, and the insert's defaults treat an explicit `null` exactly as an omitted key. What still reports:
13+
14+
- a read-only column the source holds a VALUE in is still copied, stripped and reported, body and header, so the caller reading the default back is told why;
15+
- a read-only field set through `overrides` is still stripped and reported, whatever the source held.

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14670,7 +14670,19 @@ export class ObjectStackProtocolImplementation implements
1467014670
|| (def as any).type === 'formula'
1467114671
|| (def as any).type === 'summary') {
1467214672
delete data[name];
14673+
continue;
1467314674
}
14675+
// [#29200] A read-only column the source left EMPTY is not carried
14676+
// into the insert. Carried, it reached `engine.insert`'s strip as a
14677+
// present key and was reported in `droppedFields` (and the
14678+
// `X-ObjectStack-Dropped-Fields` header) although neither the caller
14679+
// nor the source supplied a value. Leaving it out stores the same
14680+
// row: the strip would have taken it from a non-system caller, and
14681+
// the insert's defaults treat an explicit `null` exactly as an
14682+
// omitted key. A read-only column that HOLDS a value is still
14683+
// carried, stripped and reported (#15703), and so is one the caller
14684+
// sets through `overrides`, which are applied below.
14685+
if ((def as any).readonly && data[name] == null) delete data[name];
1467414686
}
1467514687
// Caller overrides win (new name, cleared unique field, reset status…).
1467614688
if (request.overrides && typeof request.overrides === 'object') {

‎packages/qa/dogfood/test/showcase-static-readonly.dogfood.test.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -148,9 +148,19 @@ describe('showcase: clone and createMany advertise the readonly strip in the hea
148148
expect(res.status).toBe(201);
149149
const body = await res.json();
150150
expect(namesReadonlyLeadScore(body), JSON.stringify(body?.droppedFields)).toBe(true);
151-
// A plain clone copies the source's other readonly columns too, so the
152-
// header may name more than lead_score; it names what the body names.
153-
expect(res.headers.get('X-ObjectStack-Dropped-Fields') ?? '').toContain('lead_score;reason=readonly');
151+
// The source's readonly columns are all empty, so the clone carries none of
152+
// them into the insert (#29200): the header names lead_score and nothing else.
153+
expect(res.headers.get('X-ObjectStack-Dropped-Fields')).toBe('lead_score;reason=readonly');
154+
});
155+
156+
// #29200: the source's lead_score is empty and the caller sent none, so a
157+
// plain clone strips nothing and must not announce a dropped field.
158+
it('a plain clone of a source whose lead_score is empty: no droppedFields, no header', async () => {
159+
const res = await stack.apiAs(admin, 'POST', `${OBJ}/${sourceId}/clone`, {});
160+
const body = await res.json();
161+
expect(res.status, JSON.stringify(body)).toBe(201);
162+
expect(body?.droppedFields, JSON.stringify(body?.droppedFields)).toBeUndefined();
163+
expect(res.headers.get('X-ObjectStack-Dropped-Fields')).toBeNull();
154164
});
155165

156166
it('a two-row createMany carrying lead_score: body and header name lead_score once', async () => {

‎packages/rest/src/rest-dropped-fields-every-write-door.test.ts‎

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,10 @@ const PROJ = {
5454
},
5555
};
5656

57-
// Clone copies every column of the source it does not reset, readonly ones
58-
// included, so a clone of a PROJ row also reports the source's `ro2`. Clone is
59-
// read on its own objects: one readonly column, and none.
57+
// Clone copies the source's readonly columns that hold a value (#15703: the
58+
// strip of a copied value is reported), so a clone of a PROJ row would also
59+
// report any `ro2` the source carried. Clone is read on its own objects: one
60+
// readonly column, and none.
6061
const CLONE_OBJECT = 'hdr_drops_clone';
6162
const PLAIN_OBJECT = 'hdr_drops_plain';
6263
const CLONE_PROJ = {
@@ -116,6 +117,8 @@ async function boot() {
116117
// `readonlyWhen` already true; seeded under `isSystem`.
117118
await engine.insert(OBJECT, { id: 'closed1', title: 'closed', status: 'closed', locked: 'L' } as any, { context: { isSystem: true } } as any);
118119
await engine.insert(CLONE_OBJECT, { id: 'src1', title: 'source', ro: 'seeded' } as any, { context: { isSystem: true } } as any);
120+
// #29200: the same object, its readonly column left empty (null).
121+
await engine.insert(CLONE_OBJECT, { id: 'src_empty', title: 'empty source' } as any);
119122
await engine.insert(PLAIN_OBJECT, { id: 'plain1', title: 'source' } as any);
120123

121124
const protocol = new ObjectStackProtocolImplementation(engine as any);
@@ -147,6 +150,38 @@ describe('clone: the header names what the body reports', () => {
147150
expect(r.header).toBe('ro;reason=readonly');
148151
});
149152

153+
// #29200: the caller sent no readonly key and the copy carried none with a
154+
// value, so the engine stripped nothing the caller or the source supplied.
155+
it('a plain clone of a row whose readonly column is empty: no droppedFields, no header', async () => {
156+
const b = await boot();
157+
const r = await b.call('/api/v1/data/:object/:id/clone', {}, { object: CLONE_OBJECT, id: 'src_empty' });
158+
expect(r.status, JSON.stringify(r.body)).toBe(201);
159+
expect(r.body).not.toHaveProperty('droppedFields');
160+
expect(r.header).toBeUndefined();
161+
expect(r.body.record.ro ?? null).toBeNull();
162+
expect(r.body.record.title).toBe('empty source');
163+
});
164+
165+
it('overrides setting the readonly field on that empty source: still reported, body and header', async () => {
166+
const b = await boot();
167+
const r = await b.call('/api/v1/data/:object/:id/clone', { overrides: { ro: 'forged' } }, { object: CLONE_OBJECT, id: 'src_empty' });
168+
expect(r.status, JSON.stringify(r.body)).toBe(201);
169+
expect(r.body.droppedFields).toEqual([{ object: CLONE_OBJECT, fields: ['ro'], reason: 'readonly' }]);
170+
expect(r.header).toBe('ro;reason=readonly');
171+
expect(r.body.record.ro ?? null).toBeNull();
172+
});
173+
174+
// #15703 ruled that a clone REPORTS the readonly value it copied and the
175+
// engine stripped: the caller reads the default back and is told why.
176+
it('a plain clone of a row whose readonly column holds a value: still reported, body and header', async () => {
177+
const b = await boot();
178+
const r = await b.call('/api/v1/data/:object/:id/clone', {}, { object: CLONE_OBJECT, id: 'src1' });
179+
expect(r.status, JSON.stringify(r.body)).toBe(201);
180+
expect(r.body.droppedFields).toEqual([{ object: CLONE_OBJECT, fields: ['ro'], reason: 'readonly' }]);
181+
expect(r.header).toBe('ro;reason=readonly');
182+
expect(r.body.record.ro ?? null).toBeNull();
183+
});
184+
150185
it('a clone that strips nothing: no droppedFields, no header (the control)', async () => {
151186
const b = await boot();
152187
const r = await b.call('/api/v1/data/:object/:id/clone', { overrides: { title: 'plain copy' } }, { object: PLAIN_OBJECT, id: 'plain1' });

0 commit comments

Comments
 (0)