Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions .changeset/lint-fieldless-object-skip.md
Original file line number Diff line number Diff line change
@@ -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.
18 changes: 18 additions & 0 deletions packages/lint/src/validate-action-body-writes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
5 changes: 3 additions & 2 deletions packages/lint/src/validate-action-body-writes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared';
import {
extractHookBodyWriteSet,
indexObjectFields,
judgeableFieldsOf,
IMPLICIT_FIELDS,
HOOK_BODY_WRITE_PATTERNS,
type BodyWritePatternExclusion,
Expand Down Expand Up @@ -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);
Expand Down
15 changes: 7 additions & 8 deletions packages/lint/src/validate-flow-node-writes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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
Expand Down
62 changes: 62 additions & 0 deletions packages/lint/src/validate-hook-body-writes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down
49 changes: 44 additions & 5 deletions packages/lint/src/validate-hook-body-writes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,39 @@ export function indexObjectFields(stack: AnyRec): Map<string, Set<string>> {
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<string, Set<string>>,
objectName: string,
): Set<string> | 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. */
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down
Loading