|
| 1 | +/** |
| 2 | + * ObjectUI |
| 3 | + * Copyright (c) 2024-present ObjectStack Inc. |
| 4 | + * |
| 5 | + * This source code is licensed under the MIT license found in the |
| 6 | + * LICENSE file in the root directory of this source tree. |
| 7 | + */ |
| 8 | + |
| 9 | +/** |
| 10 | + * objectui#6480 — `saveFields` does not PUT the framework's own read |
| 11 | + * decorations back. |
| 12 | + * |
| 13 | + * `saveFields` spreads the served object verbatim (`...existingObject`) to |
| 14 | + * preserve every key this service does not model. That spread is load-bearing |
| 15 | + * and pinned in `MetadataService.objectPayloadFieldsMap.test.ts` — but it does |
| 16 | + * not distinguish keys the AUTHOR owns from keys the FRAMEWORK adds on the way |
| 17 | + * out. `_diagnostics` and `_draft` are the second kind, and `ObjectSchema` |
| 18 | + * refuses both BY NAME, so a served document carrying either one produced a |
| 19 | + * body the schema rejects. |
| 20 | + * |
| 21 | + * ## The instrument, and why it is the spec's list rather than a local one |
| 22 | + * |
| 23 | + * Everything below reads `METADATA_READ_DECORATIONS` from |
| 24 | + * `@objectstack/spec/kernel` instead of hard-coding `['_diagnostics','_draft']`. |
| 25 | + * That is the point of the fix as much as of the test: a local copy of the list |
| 26 | + * silently goes stale the next time the framework adds a decoration, and a |
| 27 | + * decoration the writer does not know to remove is exactly the defect this card |
| 28 | + * describes. If the spec grows a third member, `the instrument` below starts |
| 29 | + * describing it and the round-trip pin covers it without an edit here. |
| 30 | + * |
| 31 | + * ## Why this is NOT "strip whatever the schema refuses" (AGENTS.md #0.1) |
| 32 | + * |
| 33 | + * The lenient-consumer shape would swallow the next genuinely-unrecognized key |
| 34 | + * along with these two and hide a real producer bug. The strip is bounded to |
| 35 | + * the keys the framework itself ADDS AT READ TIME and never stores, which is |
| 36 | + * why the last describe below asserts that an off-spec key the author owns |
| 37 | + * still goes out and still fails the schema — loudly, where someone can see it. |
| 38 | + * |
| 39 | + * ## Why dropping them is not silent loss on an upsert |
| 40 | + * |
| 41 | + * A PUT here is an upsert: an absent key is absent from the stored document, |
| 42 | + * so removing a key is never neutral by default. These two are the exception, |
| 43 | + * and by the spec's own declaration rather than by assumption — `_diagnostics` |
| 44 | + * is the read-path validation verdict (`decorateMetadataItem` spreads it onto |
| 45 | + * every read and recomputes it every time) and `_draft` reflects the row's |
| 46 | + * `state` column and the `mode` parameter, never the body. Neither is author |
| 47 | + * state, so neither can be lost by not echoing it. The keys that ARE write-path |
| 48 | + * state — the ADR-0010 protection envelope — are deliberately not members of |
| 49 | + * `METADATA_READ_DECORATIONS`, and the last test of `the instrument` pins that |
| 50 | + * this strip leaves them alone. |
| 51 | + */ |
| 52 | + |
| 53 | +import { describe, expect, it, vi } from 'vitest'; |
| 54 | +import { ObjectSchema } from '@objectstack/spec/data'; |
| 55 | +import { METADATA_READ_DECORATIONS, stripReadDecorations } from '@objectstack/spec/kernel'; |
| 56 | +import { ObjectStackAdapter } from '@object-ui/data-objectstack'; |
| 57 | +import type { DesignerFieldDefinition } from '@object-ui/types'; |
| 58 | +import { MetadataService } from './MetadataService'; |
| 59 | + |
| 60 | +/** |
| 61 | + * Captures the bodies of every PUT the SDK issued, exactly as they went over |
| 62 | + * the wire, and serves a caller-supplied document to the GET `saveFields` does. |
| 63 | + * |
| 64 | + * Deliberately the same harness as `MetadataService.objectPayloadFieldsMap.test.ts`: |
| 65 | + * assertions read `JSON.parse` of a captured request body, so what is measured |
| 66 | + * is the bytes rather than an in-memory object that never had to serialise. |
| 67 | + */ |
| 68 | +function makeCapturingAdapter(served?: Record<string, unknown>) { |
| 69 | + const puts: Array<Record<string, unknown>> = []; |
| 70 | + const adapter = new ObjectStackAdapter({ |
| 71 | + baseUrl: 'http://test.local', |
| 72 | + fetch: vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { |
| 73 | + const method = (init?.method ?? 'GET').toUpperCase(); |
| 74 | + if (method === 'PUT') { |
| 75 | + puts.push(JSON.parse(String(init?.body ?? '{}')) as Record<string, unknown>); |
| 76 | + } |
| 77 | + if (method === 'GET' && served) { |
| 78 | + return new Response(JSON.stringify({ item: served }), { |
| 79 | + status: 200, |
| 80 | + headers: { 'content-type': 'application/json' }, |
| 81 | + }); |
| 82 | + } |
| 83 | + return new Response(JSON.stringify({ success: true }), { |
| 84 | + status: 200, |
| 85 | + headers: { 'content-type': 'application/json' }, |
| 86 | + }); |
| 87 | + }) as unknown as typeof fetch, |
| 88 | + }); |
| 89 | + return { adapter, puts }; |
| 90 | +} |
| 91 | + |
| 92 | +const issuesOf = (result: ReturnType<typeof ObjectSchema.safeParse>): string[] => |
| 93 | + result.success ? [] : result.error.issues.map((i) => `${i.code} @ ${i.path.join('.')}`); |
| 94 | + |
| 95 | +/** Every `unrecognized_keys` key the schema named, flattened. */ |
| 96 | +const refusedKeys = (doc: unknown): string[] => { |
| 97 | + const r = ObjectSchema.safeParse(doc); |
| 98 | + if (r.success) return []; |
| 99 | + return r.error.issues.flatMap((i) => |
| 100 | + i.code === 'unrecognized_keys' ? ((i as unknown as { keys: string[] }).keys ?? []) : [], |
| 101 | + ); |
| 102 | +}; |
| 103 | + |
| 104 | +const designerField = (name: string, over: Partial<DesignerFieldDefinition> = {}): DesignerFieldDefinition => ({ |
| 105 | + id: name, |
| 106 | + name, |
| 107 | + label: name, |
| 108 | + type: 'text', |
| 109 | + ...over, |
| 110 | +}); |
| 111 | + |
| 112 | +/** A served document decorated exactly as the framework's read path decorates it. */ |
| 113 | +const DECORATED_SERVED = { |
| 114 | + name: 'account', |
| 115 | + label: 'Account', |
| 116 | + pluralLabel: 'Accounts', |
| 117 | + icon: 'Building', |
| 118 | + fields: { legacy: { type: 'text', label: 'Legacy' } }, |
| 119 | + _diagnostics: { valid: false, errors: [{ path: 'fields.legacy', message: 'stale' }] }, |
| 120 | + _draft: true, |
| 121 | +} satisfies Record<string, unknown>; |
| 122 | + |
| 123 | +// --------------------------------------------------------------------------- |
| 124 | + |
| 125 | +describe('the instrument', () => { |
| 126 | + it('is the spec that names the decorations, and it names exactly these two', () => { |
| 127 | + // Stated before any claim about the fix. If this list grows, the pins below |
| 128 | + // follow it without an edit — that is why they read it rather than spell it. |
| 129 | + expect([...METADATA_READ_DECORATIONS]).toEqual(['_diagnostics', '_draft']); |
| 130 | + }); |
| 131 | + |
| 132 | + it('refuses each decoration BY NAME — the defect, on the schema', () => { |
| 133 | + const base = { name: 'account', label: 'Account', fields: { n: { type: 'text', label: 'N' } } }; |
| 134 | + // Control first, so the refusals below are a result about these keys and |
| 135 | + // not a schema that refuses everything. |
| 136 | + expect(ObjectSchema.safeParse(base).success).toBe(true); |
| 137 | + for (const key of METADATA_READ_DECORATIONS) { |
| 138 | + expect(refusedKeys({ ...base, [key]: {} })).toEqual([key]); |
| 139 | + } |
| 140 | + expect(refusedKeys({ ...base, _diagnostics: {}, _draft: true }).sort()).toEqual(['_diagnostics', '_draft']); |
| 141 | + }); |
| 142 | + |
| 143 | + it('leaves the ADR-0010 protection envelope alone — the keys that ARE write-path state', () => { |
| 144 | + // The silent-loss question, answered on the helper rather than assumed. |
| 145 | + // These share the underscore spelling and are deliberately NOT decorations: |
| 146 | + // the server merges them back, so stripping them WOULD lose state. |
| 147 | + const envelope = { |
| 148 | + name: 'account', |
| 149 | + label: 'Account', |
| 150 | + fields: { n: { type: 'text', label: 'N' } }, |
| 151 | + _lock: 'full', |
| 152 | + _lockReason: 'shipped by a package', |
| 153 | + _lockSource: 'package', |
| 154 | + _provenance: 'package', |
| 155 | + _packageId: 'crm', |
| 156 | + _packageVersion: '1.2.3', |
| 157 | + _lockDocsUrl: 'https://docs.objectstack.ai/locks', |
| 158 | + }; |
| 159 | + expect(ObjectSchema.safeParse(envelope).success).toBe(true); |
| 160 | + |
| 161 | + const stripped = stripReadDecorations({ ...envelope, _diagnostics: {}, _draft: true }) as Record<string, unknown>; |
| 162 | + expect(stripped).toEqual(envelope); |
| 163 | + expect(ObjectSchema.safeParse(stripped).success).toBe(true); |
| 164 | + }); |
| 165 | +}); |
| 166 | + |
| 167 | +describe('objectui#6480 · saveFields strips the read decorations before it PUTs', () => { |
| 168 | + it('sends neither decoration — asserted on the request bytes', async () => { |
| 169 | + const { adapter, puts } = makeCapturingAdapter(DECORATED_SERVED); |
| 170 | + |
| 171 | + await new MetadataService(adapter).saveFields('account', [ |
| 172 | + designerField('first_name', { label: 'First name' }), |
| 173 | + ]); |
| 174 | + |
| 175 | + // Falsification: the save really happened and really described this object, |
| 176 | + // so the absences below are absences from a body that exists. |
| 177 | + expect(puts).toHaveLength(1); |
| 178 | + expect(puts[0].name).toBe('account'); |
| 179 | + |
| 180 | + for (const key of METADATA_READ_DECORATIONS) { |
| 181 | + expect(key in puts[0]).toBe(false); |
| 182 | + } |
| 183 | + }); |
| 184 | + |
| 185 | + it('and the whole body parses green — the red-to-green witness of this card', async () => { |
| 186 | + const { adapter, puts } = makeCapturingAdapter(DECORATED_SERVED); |
| 187 | + await new MetadataService(adapter).saveFields('account', [designerField('first_name', { label: 'First name' })]); |
| 188 | + |
| 189 | + // Before this change: ['unrecognized_keys @ '] naming both decorations. |
| 190 | + expect(issuesOf(ObjectSchema.safeParse(puts[0]))).toEqual([]); |
| 191 | + expect(refusedKeys(puts[0])).toEqual([]); |
| 192 | + }); |
| 193 | + |
| 194 | + it('STILL preserves the author keys the spread exists to carry', async () => { |
| 195 | + // The property this fix must not cost. `_diagnostics` and `pluralLabel` |
| 196 | + // arrive on the same document by the same spread; only the first may go. |
| 197 | + const { adapter, puts } = makeCapturingAdapter({ |
| 198 | + ...DECORATED_SERVED, |
| 199 | + fieldGroups: { contact: { label: 'Contact' } }, |
| 200 | + }); |
| 201 | + |
| 202 | + await new MetadataService(adapter).saveFields('account', [designerField('first_name', { label: 'First name' })]); |
| 203 | + |
| 204 | + expect(puts[0]).toMatchObject({ |
| 205 | + name: 'account', |
| 206 | + label: 'Account', |
| 207 | + pluralLabel: 'Accounts', |
| 208 | + icon: 'Building', |
| 209 | + fieldGroups: { contact: { label: 'Contact' } }, |
| 210 | + }); |
| 211 | + // Positive control in the same output: `fields` really WAS replaced, so the |
| 212 | + // line above is about preservation and not about a body echoed back whole. |
| 213 | + expect(Object.keys(puts[0].fields as Record<string, unknown>)).toEqual(['first_name']); |
| 214 | + }); |
| 215 | + |
| 216 | + it('strips each decoration on its own, not only when both are present', async () => { |
| 217 | + // A strip keyed on one of them, or on the pair, would pass the test above |
| 218 | + // and still leave the single-decoration document unsaveable. |
| 219 | + for (const key of METADATA_READ_DECORATIONS) { |
| 220 | + const { adapter, puts } = makeCapturingAdapter({ |
| 221 | + name: 'account', |
| 222 | + label: 'Account', |
| 223 | + fields: { legacy: { type: 'text', label: 'Legacy' } }, |
| 224 | + [key]: key === '_draft' ? true : { valid: true, errors: [] }, |
| 225 | + }); |
| 226 | + await new MetadataService(adapter).saveFields('account', [designerField('first_name', { label: 'First name' })]); |
| 227 | + |
| 228 | + expect(puts).toHaveLength(1); |
| 229 | + expect(key in puts[0]).toBe(false); |
| 230 | + expect(ObjectSchema.safeParse(puts[0]).success).toBe(true); |
| 231 | + } |
| 232 | + }); |
| 233 | + |
| 234 | + it('leaves an undecorated document byte-identical apart from `fields`', async () => { |
| 235 | + // The no-op control: the strip must not be observable on the common path. |
| 236 | + const { adapter, puts } = makeCapturingAdapter({ |
| 237 | + name: 'account', |
| 238 | + label: 'Account', |
| 239 | + pluralLabel: 'Accounts', |
| 240 | + fields: { legacy: { type: 'text', label: 'Legacy' } }, |
| 241 | + }); |
| 242 | + await new MetadataService(adapter).saveFields('account', [designerField('first_name', { label: 'First name' })]); |
| 243 | + |
| 244 | + expect(puts[0]).toEqual({ |
| 245 | + name: 'account', |
| 246 | + label: 'Account', |
| 247 | + pluralLabel: 'Accounts', |
| 248 | + fields: { first_name: { name: 'first_name', type: 'text', label: 'First name' } }, |
| 249 | + }); |
| 250 | + }); |
| 251 | +}); |
| 252 | + |
| 253 | +describe('objectui#6480 · the strip is bounded — it is not a lenient "drop what the schema refuses" pass', () => { |
| 254 | + it('still sends an off-spec key the AUTHOR owns, and the schema still refuses it', async () => { |
| 255 | + // AGENTS.md #0.1. A general strip would swallow this key and hide the |
| 256 | + // producer's bug; only the framework's own read decorations may be removed. |
| 257 | + const { adapter, puts } = makeCapturingAdapter({ |
| 258 | + name: 'account', |
| 259 | + label: 'Account', |
| 260 | + fields: { legacy: { type: 'text', label: 'Legacy' } }, |
| 261 | + _diagnostics: { valid: true, errors: [] }, |
| 262 | + notASpecKey: 'authored, wrong, and it must stay visible', |
| 263 | + }); |
| 264 | + |
| 265 | + await new MetadataService(adapter).saveFields('account', [designerField('first_name', { label: 'First name' })]); |
| 266 | + |
| 267 | + expect(puts[0].notASpecKey).toBe('authored, wrong, and it must stay visible'); |
| 268 | + // The decoration went; the off-spec author key stayed and is still refused. |
| 269 | + expect('_diagnostics' in puts[0]).toBe(false); |
| 270 | + expect(refusedKeys(puts[0])).toEqual(['notASpecKey']); |
| 271 | + }); |
| 272 | +}); |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?v=4&size=40){kind=avatar}
0 commit comments