diff --git a/.changeset/lint-fieldless-object-skip.md b/.changeset/lint-fieldless-object-skip.md new file mode 100644 index 0000000000..2782e69016 --- /dev/null +++ b/.changeset/lint-fieldless-object-skip.md @@ -0,0 +1,47 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): an object declaring no fields is unjudgeable, not "has no such field" (#4383) + +`hook-body-write-unknown-field` and `action-body-write-unknown-field` reported +**every** field write to an object that declares no `fields` — an external +object, or a datasource-introspected schema whose columns are resolved at +runtime. Measured before the fix: + +``` +hook : ["hook-body-write-unknown-field / warning"] ← false +action: ["action-body-write-unknown-field / warning"] ← false +flow : [] ← correct +``` + +`indexObjectFields` returns an **empty Set** for such an object rather than +`undefined`, and both rules only asked "is this object in the stack?" — +`targetSets.every((s) => s !== undefined)` and `if (!known) continue`. An empty +Set is neither undefined nor falsy, so it became the answer to `has(field)`, +and the answer is always `false`. + +That field map is not empty, it is **unknown**. The distinction already existed +in two other rules of the same family, each with its reason written down — +`validate-searchable-fields` skip #2 and `validate-flow-node-writes` (#4369, +which added the guard because it gates). Two of four had it; the drift shape +#3583 and #4330 exist to remove. + +**Fixed once, not twice.** The guard now lives in a shared +`judgeableFieldsOf(index, objectName)` that returns the declared names only when +they are a sound basis for a "resolves to nothing" judgement, and `undefined` +for both unjudgeable cases — cross-package objects and fields-less ones. All +three write-set rules route their lookups through it, so a fourth cannot repeat +the omission. It is internal to the family (not re-exported from the package +barrel), same as `indexObjectFields` and `IMPLICIT_FIELDS`. + +One semantic call worth naming: a **multi-target** hook where only *some* +targets are judgeable is now skipped entirely. The `ctx.input` finding fires +only when a field is missing from EVERY target, and an unjudgeable target is one +the field might well exist on — so judging the remainder would assert "missing +everywhere" on evidence that does not cover everywhere. Consistent with the +rule's stated asymmetry: prefer a missed finding to a false one. + +No behaviour change for objects that declare fields: an unknown field on a +normal object still warns exactly as before, pinned by a test placed next to +each new skip so the guard cannot swallow the real finding. diff --git a/packages/lint/src/validate-action-body-writes.test.ts b/packages/lint/src/validate-action-body-writes.test.ts index 15b624be81..b2305b8fb3 100644 --- a/packages/lint/src/validate-action-body-writes.test.ts +++ b/packages/lint/src/validate-action-body-writes.test.ts @@ -199,6 +199,24 @@ describe('validateActionBodyWrites — ctx.api writes', () => { expect(findings).toEqual([]); }); + // #4383 — an object declaring NO fields (external / datasource-introspected, + // columns resolved at runtime) is unjudgeable, not "has no such field". The + // empty Set used to answer `has()` false for every write to it. + it('stays silent on an object that declares no fields', () => { + const stack = { + objects: [dealObject, { name: 'legacy_deal', label: 'Legacy Deal', external: true }], + actions: [ + { + name: 'sync_legacy', + label: 'Sync Legacy', + objectName: 'crm_deal', + body: { language: 'js', source: "await ctx.api.object('legacy_deal').update({ stage: 'won' });" }, + }, + ], + }; + expect(validateActionBodyWrites(stack)).toEqual([]); + }); + it('reports each unknown field once per action, even when written repeatedly', () => { const findings = validateActionBodyWrites( stackWith( diff --git a/packages/lint/src/validate-action-body-writes.ts b/packages/lint/src/validate-action-body-writes.ts index e247954670..20fd9d2cd5 100644 --- a/packages/lint/src/validate-action-body-writes.ts +++ b/packages/lint/src/validate-action-body-writes.ts @@ -77,6 +77,7 @@ import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; import { extractHookBodyWriteSet, indexObjectFields, + judgeableFieldsOf, IMPLICIT_FIELDS, HOOK_BODY_WRITE_PATTERNS, type BodyWritePatternExclusion, @@ -319,8 +320,8 @@ export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[ const dedupeKey = `${w.object}\u0000${w.field}`; if (reported.has(dedupeKey)) continue; - const known = objectFields.get(w.object); - if (!known) continue; // object declared by another package — cannot judge + const known = judgeableFieldsOf(objectFields, w.object); + if (!known) continue; // cross-package, or no declared fields — cannot judge if (IMPLICIT_FIELDS.has(w.field) || known.has(w.field)) continue; reported.add(dedupeKey); diff --git a/packages/lint/src/validate-flow-node-writes.ts b/packages/lint/src/validate-flow-node-writes.ts index 1e634bd113..90d4d4fac1 100644 --- a/packages/lint/src/validate-flow-node-writes.ts +++ b/packages/lint/src/validate-flow-node-writes.ts @@ -84,7 +84,7 @@ import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; -import { indexObjectFields, IMPLICIT_FIELDS } from './validate-hook-body-writes.js'; +import { indexObjectFields, judgeableFieldsOf, IMPLICIT_FIELDS } from './validate-hook-body-writes.js'; export type FlowNodeWriteSeverity = 'error'; @@ -210,13 +210,12 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] { if (!objectName) return; // templated / dynamic object — resolved at run time objectFields ??= indexObjectFields(stack); - const known = objectFields.get(objectName); - if (!known) return; // object declared by another package — cannot judge its fields - // An object that declares NO fields is an external / datasource-introspected - // schema whose columns are resolved at runtime (the same skip - // validate-searchable-fields takes). Every write to it would otherwise be - // flagged, and this rule gates. - if (known.size === 0) return; + // Cross-package objects and objects declaring no fields at all (external / + // datasource-introspected schemas) are both unjudgeable, and this rule + // gates — see {@link judgeableFieldsOf}, which is where that guard now + // lives for the whole family rather than once per rule (#4383). + const known = judgeableFieldsOf(objectFields, objectName); + if (!known) return; const nodeName = typeof node.label === 'string' && node.label diff --git a/packages/lint/src/validate-hook-body-writes.test.ts b/packages/lint/src/validate-hook-body-writes.test.ts index 02b9b93b38..4021bd7746 100644 --- a/packages/lint/src/validate-hook-body-writes.test.ts +++ b/packages/lint/src/validate-hook-body-writes.test.ts @@ -183,6 +183,52 @@ describe('validateHookBodyWrites — ctx.input writes', () => { ); expect(findings).toEqual([]); }); + + // #4383 — an object that declares NO fields is an external object or a + // datasource-introspected schema whose columns are resolved at runtime. Its + // field map is not empty, it is UNKNOWN; treating the empty Set as an answer + // reported every write to such an object as a missing field. + it('a target declaring no fields at all is unjudgeable — silent, not "no such field"', () => { + const stack = { + objects: [{ name: 'legacy_deal', label: 'Legacy Deal', external: true }], + hooks: [ + { + name: 'h', + object: 'legacy_deal', + events: ['beforeInsert'], + body: { language: 'js', source: "ctx.input.stage = 'won';" }, + }, + ], + }; + expect(validateHookBodyWrites(stack)).toEqual([]); + }); + + it('one fields-less target makes a multi-target everywhere-miss unsound — silent', () => { + // The finding fires only when a field is missing from EVERY target, and the + // opaque target is one it might well exist on. Judging the rest would state + // "missing everywhere" on evidence that does not cover everywhere. + const stack = { + objects: [dealObject, { name: 'legacy_deal', external: true }], + hooks: [ + { + name: 'h', + object: ['crm_deal', 'legacy_deal'], + events: ['beforeInsert'], + body: { language: 'js', source: "ctx.input.nowhere_field = 'x';" }, + }, + ], + }; + expect(validateHookBodyWrites(stack)).toEqual([]); + }); + + it('still warns when every target is judgeable', () => { + // The guard above must not swallow the real finding: same shape, but both + // targets declare fields. + const findings = validateHookBodyWrites( + stackWith("ctx.input.nowhere_field = 'x';", { object: ['crm_deal', 'crm_contact'] }), + ); + expect(findings).toHaveLength(1); + }); }); describe('validateHookBodyWrites — ctx.api writes', () => { @@ -219,6 +265,22 @@ describe('validateHookBodyWrites — ctx.api writes', () => { ); expect(findings).toEqual([]); }); + + // #4383 — same unjudgeable class as a cross-package object, on the ctx.api side. + it('stays silent on an object that declares no fields', () => { + const stack = { + objects: [dealObject, { name: 'legacy_deal', external: true }], + hooks: [ + { + name: 'h', + object: 'crm_deal', + events: ['afterInsert'], + body: { language: 'js', source: "await ctx.api.object('legacy_deal').update({ stage: 'won' });" }, + }, + ], + }; + expect(validateHookBodyWrites(stack)).toEqual([]); + }); }); describe('validateHookBodyWrites — scope and shape tolerance', () => { diff --git a/packages/lint/src/validate-hook-body-writes.ts b/packages/lint/src/validate-hook-body-writes.ts index 783e3693a0..7b47cc205d 100644 --- a/packages/lint/src/validate-hook-body-writes.ts +++ b/packages/lint/src/validate-hook-body-writes.ts @@ -288,6 +288,39 @@ export function indexObjectFields(stack: AnyRec): Map> { return out; } +/** + * The declared field names of `objectName` — but ONLY when they are a sound + * basis for judging "this name resolves to nothing". Otherwise `undefined`. + * + * Two different unknowns collapse to one answer on purpose, because every + * caller in this family owes them the same silence: + * + * • the object is not in this stack — another package declares it, and a + * field map we cannot see cannot be judged; + * • the object is here but declares NO fields at all — an external object or + * a datasource-introspected schema whose columns are resolved at runtime. + * Its field map is not empty, it is *unknown*, and an empty Set answers + * `has(anything) === false`, which reads as "no such field" for EVERY write + * to it. That is a false-positive generator, and a false positive kills an + * advisory lint (#4383). + * + * The distinction is unused today — no rule in the family wants to act on one + * and not the other — so collapsing it here is what stops the guard from being + * hand-copied per call site and forgotten at one of them, which is exactly how + * it went missing from the hook and action rules while + * `validate-searchable-fields` (skip #2) and `validate-flow-node-writes` both + * had it. A future caller that genuinely needs to tell them apart should read + * the index directly and say why. + */ +export function judgeableFieldsOf( + index: ReadonlyMap>, + objectName: string, +): Set | undefined { + const declared = index.get(objectName); + if (!declared || declared.size === 0) return undefined; + return declared; +} + /** One statically-extracted field write found in an L2 body. */ export interface ExtractedHookBodyWrite { /** Which {@link HOOK_BODY_WRITE_PATTERNS} entry matched. */ @@ -548,12 +581,18 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { const hookName = typeof hook.name === 'string' && hook.name ? hook.name : `#${hookIndex}`; // The hook's own target set, for `ctx.input` writes. A wildcard target has - // no single object to check against; an unknown (cross-package) target - // cannot be judged — either way `ctx.input` writes are skipped, not guessed. + // no single object to check against; a target whose fields cannot be judged + // ({@link judgeableFieldsOf} — cross-package, or declaring no fields at all) + // gives nothing to resolve against — either way `ctx.input` writes are + // skipped, not guessed. const targets = (Array.isArray(hook.object) ? hook.object : [hook.object]).filter( (o): o is string => typeof o === 'string' && o.trim() !== '', ); - const targetSets = targets.map((t) => objectFields!.get(t)); + const targetSets = targets.map((t) => judgeableFieldsOf(objectFields!, t)); + // ALL targets must be judgeable, not just one: the finding below fires only + // when a field is missing from EVERY target, and an unjudgeable target is + // one the field might well exist on. One opaque target therefore makes the + // whole "missing everywhere" claim unsound, not merely narrower (#4383). const inputJudgeable = targets.length > 0 && !targets.includes('*') && targetSets.every((s) => s !== undefined); @@ -592,8 +631,8 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { }); } else { // ctx.api write → the named object. - const known = objectFields!.get(w.object); - if (!known) continue; // object declared by another package — cannot judge + const known = judgeableFieldsOf(objectFields!, w.object); + if (!known) continue; // cross-package, or no declared fields — cannot judge if (IMPLICIT_FIELDS.has(w.field) || known.has(w.field)) continue; reported.add(dedupeKey);