|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | + |
| 3 | +/** |
| 4 | + * [#5225 / #5393] Every predicate write this app ships declares its bulk intent. |
| 5 | + * |
| 6 | + * ## The defect this pins |
| 7 | + * |
| 8 | + * `showcase_inquiry_purge`'s `delete_record` node deleted by the predicate |
| 9 | + * `{ status: 'closed' }` and declared no bulk intent. The data engine accepts a |
| 10 | + * write without `options.multi` only when `filter` names ONE row by a scalar |
| 11 | + * `id`, so every run of the flow — both the declarative endpoint |
| 12 | + * (`POST /api/v1/apps/showcase/inquiries/purge`) and the built-in trigger route |
| 13 | + * — failed on that node with |
| 14 | + * |
| 15 | + * Node 'purge' failed: delete_record(showcase_inquiry) failed: |
| 16 | + * Delete requires an ID or options.multi=true |
| 17 | + * |
| 18 | + * and reported `acted: 0`. The showcase's own coverage manifest claims |
| 19 | + * `delete_record` is demonstrated by this flow, so the delete half of the CRUD |
| 20 | + * quartet was `declared ≠ enforced` (PD #10) from the day it was written until |
| 21 | + * #5112's boot probes hit it. |
| 22 | + * |
| 23 | + * The fix is a DECLARATION, not a rewrite: until #5393 (PR #5485) no spelling of |
| 24 | + * bulk intent existed on the node config at all, which is why the third triage |
| 25 | + * round correctly refused to route around the engine with a |
| 26 | + * get→loop→delete-by-id rewrite (PD #5 workaround). With `multi` declared, the |
| 27 | + * one-line fix is the long-term-correct shape. |
| 28 | + * |
| 29 | + * ## Why this file sweeps instead of asserting one node |
| 30 | + * |
| 31 | + * A test naming only the purge node would go green for the wrong reason the day |
| 32 | + * someone adds a second predicate write. So the invariant below is stated over |
| 33 | + * EVERY `delete_record` / `update_record` node in `allFlows`, and it is |
| 34 | + * two-sided — which matters, because `multi` cuts both ways: |
| 35 | + * |
| 36 | + * - a predicate write WITHOUT `multi: true` is refused by the engine at run |
| 37 | + * time (the #5225 failure, silent in every unit test that fakes the engine); |
| 38 | + * - `multi: true` with an absent or empty `filter` is a declared WHOLE-OBJECT |
| 39 | + * write — every row, by declaration. Authoring-time linting for that shape |
| 40 | + * is queued as #5482, and this app is meant to be its "must be zero |
| 41 | + * warnings" sample, so the second side is asserted here too. |
| 42 | + * |
| 43 | + * Node configs are additionally driven through the REAL spec schemas rather than |
| 44 | + * inspected as plain objects: the claim is about a VALUE verdict (`multi` is |
| 45 | + * `true`, `filter` is a non-empty predicate), not merely about a key being an |
| 46 | + * authorable surface, so full `safeParse` green is the right bar. |
| 47 | + */ |
| 48 | + |
| 49 | +import { describe, it, expect } from 'vitest'; |
| 50 | +import { DeleteRecordConfigSchema, UpdateRecordConfigSchema } from '@objectstack/spec/automation'; |
| 51 | + |
| 52 | +import { allFlows } from '../src/automation/flows/index.js'; |
| 53 | + |
| 54 | +type NodeLike = { id?: string; type?: string; config?: Record<string, unknown> }; |
| 55 | +type FlowLike = { name?: string; nodes?: NodeLike[] }; |
| 56 | + |
| 57 | +const WRITE_SCHEMAS = { |
| 58 | + delete_record: DeleteRecordConfigSchema, |
| 59 | + update_record: UpdateRecordConfigSchema, |
| 60 | +} as const; |
| 61 | + |
| 62 | +type WriteNodeType = keyof typeof WRITE_SCHEMAS; |
| 63 | + |
| 64 | +interface WriteNode { |
| 65 | + flow: string; |
| 66 | + node: string; |
| 67 | + type: WriteNodeType; |
| 68 | + config: Record<string, unknown>; |
| 69 | +} |
| 70 | + |
| 71 | +/** |
| 72 | + * Collect write nodes by walking the flow DEEPLY, not just its top-level |
| 73 | + * `nodes` array. |
| 74 | + * |
| 75 | + * This app nests real write nodes inside ADR-0031 structured containers — the |
| 76 | + * `catch` region of `showcase_task_crm_sync`'s try/catch holds an |
| 77 | + * `update_record`, and branch/loop bodies elsewhere hold others. A flat scan of |
| 78 | + * `flow.nodes` silently skips every one of them, which would leave the guard |
| 79 | + * below passing while the exact class of defect it exists to catch hid one |
| 80 | + * level down. So the walk is generic over the object graph rather than a list |
| 81 | + * of container key names (`try`/`catch`/`body`/`branches`/…) that a new |
| 82 | + * container shape could quietly fall outside of. |
| 83 | + */ |
| 84 | +function collectWriteNodes(flowName: string, value: unknown, out: WriteNode[]): void { |
| 85 | + if (Array.isArray(value)) { |
| 86 | + for (const entry of value) collectWriteNodes(flowName, entry, out); |
| 87 | + return; |
| 88 | + } |
| 89 | + if (!value || typeof value !== 'object') return; |
| 90 | + |
| 91 | + const node = value as NodeLike; |
| 92 | + const type = node.type as WriteNodeType | undefined; |
| 93 | + if (typeof type === 'string' && type in WRITE_SCHEMAS && node.id !== undefined) { |
| 94 | + out.push({ |
| 95 | + flow: flowName, |
| 96 | + node: String(node.id), |
| 97 | + type, |
| 98 | + config: (node.config ?? {}) as Record<string, unknown>, |
| 99 | + }); |
| 100 | + } |
| 101 | + |
| 102 | + for (const child of Object.values(value as Record<string, unknown>)) { |
| 103 | + collectWriteNodes(flowName, child, out); |
| 104 | + } |
| 105 | +} |
| 106 | + |
| 107 | +const writeNodes: WriteNode[] = []; |
| 108 | +for (const flow of allFlows as unknown as FlowLike[]) { |
| 109 | + collectWriteNodes(String(flow.name), flow.nodes, writeNodes); |
| 110 | +} |
| 111 | + |
| 112 | +/** |
| 113 | + * Does this filter name exactly one row the way the engine's non-`multi` path |
| 114 | + * requires — a SCALAR `id`? `{ id: { $in: [...] } }` does not qualify (the |
| 115 | + * engine refuses it), and neither does any other predicate. |
| 116 | + * |
| 117 | + * `{recordId}` / `{record.id}` templates count: they interpolate to one scalar |
| 118 | + * id, and #3810 already refuses the node outright when such a template erases |
| 119 | + * to nothing, so a "scalar" that vanished never reaches the write. |
| 120 | + */ |
| 121 | +function namesOneRowById(filter: unknown): boolean { |
| 122 | + if (!filter || typeof filter !== 'object') return false; |
| 123 | + const keys = Object.keys(filter as Record<string, unknown>); |
| 124 | + if (keys.length !== 1 || keys[0] !== 'id') return false; |
| 125 | + const id = (filter as { id: unknown }).id; |
| 126 | + return typeof id === 'string' || typeof id === 'number'; |
| 127 | +} |
| 128 | + |
| 129 | +function isNonEmptyPredicate(filter: unknown): boolean { |
| 130 | + return ( |
| 131 | + !!filter |
| 132 | + && typeof filter === 'object' |
| 133 | + && !Array.isArray(filter) |
| 134 | + && Object.keys(filter as Record<string, unknown>).length > 0 |
| 135 | + ); |
| 136 | +} |
| 137 | + |
| 138 | +describe('[#5225] showcase predicate writes declare bulk intent', () => { |
| 139 | + it('the app really does ship write nodes — this suite is not vacuous', () => { |
| 140 | + // If a refactor drops every CRUD write node, the per-node cases below would |
| 141 | + // pass by iterating nothing, which is exactly how #5225 hid for so long. |
| 142 | + expect(writeNodes.length).toBeGreaterThan(0); |
| 143 | + expect(writeNodes.some((n) => n.type === 'delete_record')).toBe(true); |
| 144 | + expect(writeNodes.some((n) => n.type === 'update_record')).toBe(true); |
| 145 | + }); |
| 146 | + |
| 147 | + it('reaches write nodes nested inside structured containers', () => { |
| 148 | + // `record_failure` lives in the `catch` region of `showcase_task_crm_sync`, |
| 149 | + // not in its top-level `nodes`. A flat walk finds everything else and misses |
| 150 | + // exactly this one, so naming it is what keeps the collector honest — a |
| 151 | + // regression to `flow.nodes` alone fails here rather than silently shrinking |
| 152 | + // the sweep's coverage. |
| 153 | + expect(writeNodes.map((n) => n.node)).toContain('record_failure'); |
| 154 | + }); |
| 155 | + |
| 156 | + describe.each(writeNodes)('$flow / $node ($type)', ({ type, config }) => { |
| 157 | + it('parses green against the real spec schema', () => { |
| 158 | + const result = WRITE_SCHEMAS[type].safeParse(config); |
| 159 | + expect(result.success ? null : JSON.stringify(result.error?.issues)).toBeNull(); |
| 160 | + }); |
| 161 | + |
| 162 | + it('either names one row by scalar id, or declares `multi: true`', () => { |
| 163 | + // The engine's rule, restated as the authoring rule. A node that satisfies |
| 164 | + // neither branch is the #5225 shape: it parses, it publishes, and it fails |
| 165 | + // on every single execution with `requires an ID or options.multi=true`. |
| 166 | + const single = namesOneRowById(config.filter); |
| 167 | + expect(single || config.multi === true).toBe(true); |
| 168 | + }); |
| 169 | + |
| 170 | + it('never declares `multi: true` without a bounding filter', () => { |
| 171 | + // `multi: true` + absent/empty filter = a declared whole-object write. It |
| 172 | + // is a legal thing to author deliberately, and it is NOT something this |
| 173 | + // reference app should ever demonstrate by accident — #5482's lint rule |
| 174 | + // uses this app as its zero-warning sample. |
| 175 | + if (config.multi === true) { |
| 176 | + expect(isNonEmptyPredicate(config.filter)).toBe(true); |
| 177 | + } |
| 178 | + }); |
| 179 | + }); |
| 180 | +}); |
| 181 | + |
| 182 | +describe('[#5225] the purge flow specifically — the node that never deleted anything', () => { |
| 183 | + const purge = writeNodes.find((n) => n.flow === 'showcase_inquiry_purge' && n.node === 'purge'); |
| 184 | + |
| 185 | + it('is still the delete half of the CRUD quartet src/coverage.ts claims', () => { |
| 186 | + // coverage.ts names `get+delete: InquiryPurgeFlow` under flowNodeTypes. If |
| 187 | + // this node is ever renamed or retyped, that claim needs re-checking rather |
| 188 | + // than this file silently finding nothing. |
| 189 | + expect(purge).toBeDefined(); |
| 190 | + expect(purge!.type).toBe('delete_record'); |
| 191 | + }); |
| 192 | + |
| 193 | + it('deletes closed inquiries by predicate, with bulk intent declared', () => { |
| 194 | + expect(purge!.config).toMatchObject({ |
| 195 | + objectName: 'showcase_inquiry', |
| 196 | + filter: { status: 'closed' }, |
| 197 | + multi: true, |
| 198 | + }); |
| 199 | + // Not `{ id: … }` — the point of the node is the predicate path, so the |
| 200 | + // scalar-id escape must NOT be what makes the sweep above pass for it. |
| 201 | + expect(namesOneRowById(purge!.config.filter)).toBe(false); |
| 202 | + }); |
| 203 | + |
| 204 | + it('is refused by the engine contract the moment `multi` is dropped', () => { |
| 205 | + // Reverse verification, direction decided up front: removing the |
| 206 | + // declaration must land the node back in the branch that produced |
| 207 | + // `Delete requires an ID or options.multi=true` / `acted: 0`. The schema |
| 208 | + // still accepts the stripped config — `multi` is optional by design, since |
| 209 | + // omitting it is a valid deliberate choice — so the regression this pins is |
| 210 | + // an EXECUTION one, and the sweep rule above is what catches it statically. |
| 211 | + const { multi: _multi, ...withoutIntent } = purge!.config; |
| 212 | + expect(DeleteRecordConfigSchema.safeParse(withoutIntent).success).toBe(true); |
| 213 | + expect(namesOneRowById(withoutIntent.filter) || withoutIntent.multi === true).toBe(false); |
| 214 | + }); |
| 215 | +}); |
0 commit comments