diff --git a/.changeset/index-rule-where-slot-names-the-object.md b/.changeset/index-rule-where-slot-names-the-object.md new file mode 100644 index 0000000000..b5a3ad24d6 --- /dev/null +++ b/.changeset/index-rule-where-slot-names-the-object.md @@ -0,0 +1,47 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): the three ADR-0120 uniqueness rules name the object in the `where` slot instead of repeating the config path (#9600) + +`AuthoringFinding` declares two location slots with different jobs — `where` +("human-readable location", e.g. `object "leave_request"`) and `path` ("config +path", e.g. `objects[3].sharingModel`). Three registry adapters set the first +from the second (`where: f.path`), so every CLI command printed the same +positional string twice and the only human-readable slot said nothing the `at` +clause did not already say: + +``` +• objects[44].indexes[1]: "sys_account" declares index [provider_id, account_id] with bare `unique: true` … + rule: unique/unscoped-declared-index at objects[44].indexes[1] +``` + +That index is a position in the MERGED object array, which appears in no file +the author wrote. `unique/unscoped-declared-index`, `unique/double-declaration` +and `unique/legacy-organization-composite` now spell it the way the rest of the +table does: + +``` +• object "sys_account" · index [provider_id, account_id]: "sys_account" declares index … + rule: unique/unscoped-declared-index at objects[44].indexes[1] +``` + +An index is identified by its `name` when it has one, and otherwise by the +columns the author actually wrote (`· index [provider_id, account_id]`) — both +searchable in their source, which a bare ordinal is not. + +`where` is stated by the rule functions themselves rather than reconstructed in +the adapter, because only the rule still holds the object it walked. Their +return type is now `LocatedLintIssue` (a `LintIssue` with a REQUIRED `where`), +newly exported, so a fourth rule joining this family cannot reach the adapter +without one — a `f.where ?? f.path` fallback at the adapter would have let the +positional spelling ship again silently. + +Display text only, and the rules' population is unchanged: measured over the 45 +object declarations `@objectstack/platform-objects` and +`@objectstack/metadata-core` ship, the registry produced 1050 findings from the +same 5 rules before and after, with the count of findings whose `where` was a +bare config path going 72 to 0. `path` is deliberately untouched and stays +positional — it is the slot that is supposed to be a config path, and the +runtime gate's `fingerprint` reads `where` and `path` together, so making +`where` more specific cannot merge two findings that were distinct. diff --git a/packages/lint/src/authoring-rules.ts b/packages/lint/src/authoring-rules.ts index d0058b07e6..10ed78cfab 100644 --- a/packages/lint/src/authoring-rules.ts +++ b/packages/lint/src/authoring-rules.ts @@ -1121,7 +1121,7 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [ lintUnscopedDeclaredIndexes(Array.isArray(stack.objects) ? (stack.objects as unknown[]) : []).map((f) => ({ severity: f.severity === 'suggestion' ? ('info' as const) : f.severity, rule: f.rule, - where: f.path, + where: f.where, path: f.path, message: f.message, hint: f.fix ?? '', @@ -1146,7 +1146,7 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [ lintUniqueDeclarations(Array.isArray(stack.objects) ? (stack.objects as unknown[]) : []).map((f) => ({ severity: f.severity === 'suggestion' ? ('info' as const) : f.severity, rule: f.rule, - where: f.path, + where: f.where, path: f.path, message: f.message, hint: f.fix ?? '', @@ -1172,7 +1172,7 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [ lintLegacyOrganizationComposites(Array.isArray(stack.objects) ? (stack.objects as unknown[]) : []).map((f) => ({ severity: f.severity === 'suggestion' ? ('info' as const) : f.severity, rule: f.rule, - where: f.path, + where: f.where, path: f.path, message: f.message, hint: f.fix ?? '', diff --git a/packages/lint/src/data-model-rule-where-slot.test.ts b/packages/lint/src/data-model-rule-where-slot.test.ts new file mode 100644 index 0000000000..e58e02a012 --- /dev/null +++ b/packages/lint/src/data-model-rule-where-slot.test.ts @@ -0,0 +1,174 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +// +// `AuthoringFinding` declares two location slots with different jobs: +// +// where — "Human-readable location, e.g. `object \"leave_request\"`" +// path — "Config path, e.g. `objects[3].sharingModel`" +// +// Every CLI command renders the FIRST as the line's prefix. `os validate` +// prints `• ${where}: ${message}` and then `rule: ${rule} at ${path}`, and +// `os lint` prints `${where}: ${message}` with `path` as the machine slot — so +// the two slots print side by side and a `where` copied from `path` spends the +// human-readable one saying what the `at` clause already said. +// +// Scope, measured rather than assumed: all three of these rules are +// `surfaces: CLI_ONLY`, and `runtime-gate.ts` dispatches only rules whose +// `surfaces` include `runtime-publish`. They therefore do NOT reach +// `SaveMetaItemResponseSchema.advisories` and Studio does not render them +// today — the card that filed this predates the #4716 split, which crossed the +// five GATING object rules and deliberately left the six advisory-tier ones +// (these among them) behind the door. This is a CLI diagnostic defect now, and +// the guard below is what keeps it fixed if that door later opens. +// +// The three ADR-0120 uniqueness rules used to set `where: f.path`, so an author +// read the same positional string twice and the location slot carried nothing: +// +// • objects[44].indexes[1]: "sys_account" declares index [provider_id, … +// rule: unique/unscoped-declared-index at objects[44].indexes[1] +// +// That index is a position in the MERGED object array (44 objects from +// `@objectstack/platform-objects` plus one from `@objectstack/metadata-core` in +// the measurement that filed this), which appears in no file the author wrote. +// +// This file pins BOTH halves, because pinning only the three would leave the +// class open: the second test runs the whole registry and fails on any rule +// that puts a bare config path in `where`. It is a guard on the shape of the +// corpus, not on these three rules — measured when this landed, the registry +// held 41 rules and exactly these 3 carried the shape, with no rule building a +// positional `where` in its own module. +// +// The fix changes the `where` STRING only, never which code a rule matches: +// over the same 45 shipped object declarations the card measured, the registry +// produced 1050 findings from 5 rules both before and after, and the count of +// findings whose `where` was a bare config path went 72 → 0. +import { describe, expect, it } from 'vitest'; +import { AUTHORING_RULES, runAuthoringRules, type AuthoringFinding } from './authoring-rules.js'; +import { + lintLegacyOrganizationComposites, + lintUniqueDeclarations, + lintUnscopedDeclaredIndexes, +} from './data-model-rules.js'; + +/** + * Filler so the object under test does NOT sit at `objects[0]`. The defect is + * invisible at index 0 — `objects[0].indexes[0]` reads plausibly enough that a + * fixture rooted there would pass a human review of the old spelling too. + */ +const filler = (n: number) => + Array.from({ length: n }, (_, k) => ({ name: `filler_${k}`, fields: { id: { type: 'text' } } })); + +const SYS_ACCOUNT = { + name: 'sys_account', + fields: { + email: { type: 'text', unique: true }, + provider_id: { type: 'text' }, + account_id: { type: 'text' }, + organization_id: { type: 'text' }, + }, + indexes: [ + // named + lists the organization column → R11 and R12 + { name: 'uniq_org_email', unique: true, fields: ['organization_id', 'email'] }, + // unnamed composite → R11, and exercises the column-list `where` label + { unique: true, fields: ['provider_id', 'account_id'] }, + // single column that ALSO carries a field-level `unique` → R11 and R10 + { unique: true, fields: ['email'] }, + ], +}; + +const objects = [...filler(44), SYS_ACCOUNT]; +const stack = { objects } as Record; + +const whereOf = (rule: string, findings: readonly AuthoringFinding[]) => + findings.filter((f) => f.rule === rule).map((f) => f.where); + +describe('the three ADR-0120 uniqueness rules name the object in `where`', () => { + // Through the REGISTRY, not the rule functions: the defect lived in the + // registry adapter (`where: f.path`), so a test that called the rule directly + // would have stayed green through the whole bug. + const findings = runAuthoringRules('validate', { normalized: stack, parsed: stack }); + + it('R11 `unique/unscoped-declared-index` names the object and the index', () => { + expect(whereOf('unique/unscoped-declared-index', findings)).toEqual([ + `object "sys_account" · index 'uniq_org_email'`, + 'object "sys_account" · index [provider_id, account_id]', + 'object "sys_account" · index [email]', + ]); + }); + + it('R10 `unique/double-declaration` names the object and the column', () => { + expect(whereOf('unique/double-declaration', findings)).toEqual([ + `object "sys_account" · field 'email'`, + ]); + }); + + it('R12 `unique/legacy-organization-composite` names the object and the index', () => { + expect(whereOf('unique/legacy-organization-composite', findings)).toEqual([ + `object "sys_account" · index 'uniq_org_email'`, + ]); + }); + + // The card that filed this asked for `where` only. `path` is the slot that is + // SUPPOSED to be positional, `os validate` prints it after `at`, and the + // runtime gate's `fingerprint` reads `where` and `path` together — so a + // consumer diffing findings across two stack shapes still sees the index + // move. Pinned so a later "clean up the indexes" pass has to be deliberate. + it('leaves `path` positional', () => { + expect(findings.filter((f) => f.rule === 'unique/double-declaration').map((f) => f.path)).toEqual([ + 'objects[44]', + ]); + expect( + findings.filter((f) => f.rule === 'unique/legacy-organization-composite').map((f) => f.path), + ).toEqual(['objects[44].indexes[0]']); + }); + + // The rules' own return type carries `where`, so a fourth rule joining this + // family cannot reach the adapter without one. Reading it off the direct call + // proves the producer states it — not the adapter reconstructing it. + it('the rule functions themselves state `where`', () => { + for (const issue of [ + ...lintUnscopedDeclaredIndexes(objects), + ...lintUniqueDeclarations(objects), + ...lintLegacyOrganizationComposites(objects), + ]) { + expect(issue.where).toMatch(/^object "sys_account" /); + expect(issue.path).toMatch(/^objects\[44\]/); + } + }); +}); + +describe('no authoring rule puts a bare config path in the `where` slot', () => { + /** `objects[3]`, `flows[0].nodes[2].config` — a config path, not a location. */ + const BARE_CONFIG_PATH = /^[A-Za-z_$][\w$]*\[\d+\]/; + + it('holds across every rule the violating stack triggers', () => { + const findings = runAuthoringRules('validate', { normalized: stack, parsed: stack }); + + // Non-vacuous: the stack must actually trip the three rules this guard was + // written for, or the sweep below is asserting over an empty list. + const rules = new Set(findings.map((f) => f.rule)); + expect(rules).toContain('unique/unscoped-declared-index'); + expect(rules).toContain('unique/double-declaration'); + expect(rules).toContain('unique/legacy-organization-composite'); + + const offenders = findings + .filter((f) => BARE_CONFIG_PATH.test(f.where)) + .map((f) => `${f.rule}: where = ${f.where}`); + expect( + offenders, + '`where` is the slot every CLI command prints as the line prefix. A config path ' + + 'belongs in `path`, which the same commands print separately — putting one here ' + + 'spends the only human-readable slot on a number the `at` clause already carries.', + ).toEqual([]); + }); + + // The sweep above only sees rules this one stack happens to trip. This second + // assertion is static and covers all 41 entries: no registry adapter may map + // the `where` slot from a source finding's `path`. + it('holds for every registry adapter, including rules this stack does not trip', () => { + const source = AUTHORING_RULES.map((r) => r.run.toString()).join('\n'); + expect( + source.match(/where:\s*\w+\.path\b/g) ?? [], + 'a registry adapter is mapping `where` from a positional config path again', + ).toEqual([]); + }); +}); diff --git a/packages/lint/src/data-model-rules.ts b/packages/lint/src/data-model-rules.ts index e839499d6b..3ef9f3b9ac 100644 --- a/packages/lint/src/data-model-rules.ts +++ b/packages/lint/src/data-model-rules.ts @@ -26,6 +26,68 @@ export interface LintIssue { fix?: string; } +/** + * A {@link LintIssue} that also states the HUMAN-READABLE location, for rules + * that are adapted into `AuthoringFinding` (`packages/lint/src/authoring-rules.ts`). + * + * `AuthoringFinding` declares two location slots with different jobs — `where` + * ("human-readable location", e.g. `object "leave_request"`) and `path` ("config + * path", e.g. `objects[3].sharingModel`) — and every CLI command renders the + * first as the line's prefix: `os validate` prints + * `• ${where}: ${message}` and then `rule: ${rule} at ${path}`. + * + * `LintIssue` carries only the positional `path`, so the three ADR-0120 + * uniqueness rules' registry adapters had nothing else to map and set + * `where: f.path`. Measured on `origin/main`: an author saw + * `• objects[44].indexes[1]: "sys_account" declares index … at objects[44].indexes[1]` + * — the same positional string twice, and an index into the MERGED object array + * that appears in no file the author wrote (44 objects from + * `@objectstack/platform-objects` plus one from `@objectstack/metadata-core`). + * No information was lost — the object's name is in the message — but the + * location slot said nothing the `at` clause did not already say, and every + * other rule in the table spells it `object "sys_account"`. + * + * Stating `where` at the PRODUCER rather than reconstructing it in the adapter + * is deliberate: only the rule still holds the object it walked. Making it + * REQUIRED here (rather than adding `where?` to `LintIssue` and writing + * `f.where ?? f.path` at the adapter) is the same discipline — a consumer-side + * fallback would let the next rule ship the positional spelling again, silently. + * + * `path` is unchanged and stays positional: it is the slot that is SUPPOSED to + * be a config path, `os validate` prints it after `at`, and the runtime gate's + * `fingerprint` reads `where` and `path` together (making `where` more specific + * cannot merge two findings that were distinct). + */ +export interface LocatedLintIssue extends LintIssue { + /** Human-readable location, e.g. `object "sys_account" · index 'uniq_org_email'`. */ + where: string; +} + +/** `object "sys_account"` — the spelling the rest of the authoring table uses. */ +function objectWhere(obj: any): string { + return `object "${obj?.name}"`; +} + +/** + * `object "sys_account" · index 'uniq_org_email'`. + * + * A declared index's `name` is optional, so an unnamed one is identified by the + * columns the author actually wrote (`· index [provider_id, account_id]`) — searchable + * in their source, which a bare ordinal is not. The ordinal is the last resort + * for an index that has neither, and is scoped to the named object rather than + * to the merged stack array. + */ +function indexWhere(obj: any, idx: any, j: number, cols: readonly string[]): string { + const named = typeof idx?.name === 'string' && idx.name.trim() ? `'${idx.name.trim()}'` : ''; + const label = named || (cols.length > 0 ? `[${cols.join(', ')}]` : `#${j}`); + return `${objectWhere(obj)} · index ${label}`; +} + +/** `object "sys_account" · field 'email'` — the field-scoped form of the above. */ +function fieldWhere(obj: any, fieldName: string): string { + return `${objectWhere(obj)} · field '${fieldName}'`; +} + // ─── Heuristics ───────────────────────────────────────────────────── const RELATIONSHIP_TYPES = new Set(['lookup', 'master_detail']); @@ -162,8 +224,8 @@ function indexUniqueScope(u: unknown): 'organization' | 'global' { * Wiring: own AUTHORING_RULES entry (validate/build), and `lintDataModel` * calls it for `os lint` — each command reports each finding exactly once. */ -export function lintUnscopedDeclaredIndexes(objects: any[]): LintIssue[] { - const issues: LintIssue[] = []; +export function lintUnscopedDeclaredIndexes(objects: any[]): LocatedLintIssue[] { + const issues: LocatedLintIssue[] = []; if (!Array.isArray(objects) || objects.length === 0) return issues; for (let i = 0; i < objects.length; i++) { @@ -173,13 +235,15 @@ export function lintUnscopedDeclaredIndexes(objects: any[]): LintIssue[] { for (let j = 0; j < declaredIndexes.length; j++) { const idx = declaredIndexes[j]; if (idx?.unique !== true) continue; // fires on the bare spelling only - const cols = Array.isArray(idx?.fields) - ? idx.fields.filter((f: unknown) => typeof f === 'string').join(', ') - : ''; + const colList: string[] = Array.isArray(idx?.fields) + ? idx.fields.filter((f: unknown) => typeof f === 'string') + : []; + const cols = colList.join(', '); const indexLabel = typeof idx?.name === 'string' && idx.name.trim() ? ` '${idx.name.trim()}'` : ''; issues.push({ severity: 'warning', rule: UNIQUE_UNSCOPED_DECLARED_INDEX, + where: indexWhere(obj, idx, j, colList), message: `"${obj.name}" declares index${indexLabel} [${cols}] with bare \`unique: true\` — a unique index whose scope is ` + `unstated (ADR-0120). Today the bare spelling materializes over exactly its \`fields\`, i.e. installation-wide; ` + @@ -225,8 +289,8 @@ export function lintUnscopedDeclaredIndexes(objects: any[]): LintIssue[] { * Advisory. The resulting stack is well-defined — the cost is an intent that * never takes effect, not a broken artifact — so this never fails a build. */ -export function lintUniqueDeclarations(objects: any[]): LintIssue[] { - const issues: LintIssue[] = []; +export function lintUniqueDeclarations(objects: any[]): LocatedLintIssue[] { + const issues: LocatedLintIssue[] = []; if (!Array.isArray(objects) || objects.length === 0) return issues; for (let i = 0; i < objects.length; i++) { @@ -290,6 +354,11 @@ export function lintUniqueDeclarations(objects: any[]): LintIssue[] { issues.push({ severity: 'warning', rule: UNIQUE_DOUBLE_DECLARATION, + // More specific than `path` on purpose: this rule's finding is about ONE + // column, but its `path` has always been the whole object (`objects[i]`) + // because the defect straddles `fields..unique` and an entry of + // `indexes`. `where` can name the column without picking one of the two. + where: fieldWhere(obj, name), message, path: `objects[${i}]`, fix, @@ -325,8 +394,8 @@ export function lintUniqueDeclarations(objects: any[]): LintIssue[] { * unique declared on the organization column ALONE, which is not a composite and * has no per-organization reading to recover. */ -export function lintLegacyOrganizationComposites(objects: any[]): LintIssue[] { - const issues: LintIssue[] = []; +export function lintLegacyOrganizationComposites(objects: any[]): LocatedLintIssue[] { + const issues: LocatedLintIssue[] = []; if (!Array.isArray(objects) || objects.length === 0) return issues; for (let i = 0; i < objects.length; i++) { @@ -351,6 +420,7 @@ export function lintLegacyOrganizationComposites(objects: any[]): LintIssue[] { issues.push({ severity: 'warning', rule: UNIQUE_LEGACY_ORGANIZATION_COMPOSITE, + where: indexWhere(obj, idx, j, cols), message: `"${obj.name}" declares index${indexLabel} [${cols.join(', ')}] with ${spelling} and lists the organization ` + `column '${tenantColumn}' itself — the hand-written per-organization composite that predates the scope ` + diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index e79e0c6c13..88b459b2e2 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -628,7 +628,7 @@ export { UNIQUE_UNSCOPED_DECLARED_INDEX, UNIQUE_LEGACY_ORGANIZATION_COMPOSITE, } from './data-model-rules.js'; -export type { LintIssue, Severity } from './data-model-rules.js'; +export type { LintIssue, LocatedLintIssue, Severity } from './data-model-rules.js'; // ─── The registry itself (#4409, relocated #4463) ──────────────────── //