diff --git a/.changeset/lint-unique-double-declaration.md b/.changeset/lint-unique-double-declaration.md new file mode 100644 index 0000000000..62c37346f9 --- /dev/null +++ b/.changeset/lint-unique-double-declaration.md @@ -0,0 +1,37 @@ +--- +"@objectstack/cli": patch +--- + +feat(cli): lint the contradictory uniqueness double-declaration (#3991) + +New advisory rule `unique/double-declaration`, reported by `os lint` and +`os build`. It fires when one column carries BOTH a field-level `unique: true` +and an object-level single-column unique index: + +```ts +email: Field.email({ unique: true }), // per-tenant since #3696 +indexes: [{ fields: ['email'], unique: true }], // platform-wide, verbatim +``` + +The two spellings deliberately mean different things (see `IndexSchema`), and +each is legitimate alone. Together on one column they never are: + +- On a **tenant-scoped** object they contradict. The stricter one wins + physically, so the global index enforces uniqueness and the per-tenant + composite becomes a constraint nothing can trip — one of the two authored + intents is silently discarded. Worse, it hides the #3696 semantic change: + the switch from global to per-tenant has *no observable effect* while the + declared index still enforces the old behaviour, so the author never learns + their tenancy model and their real constraint disagree — until a second + tenant reuses the value and is rejected. +- On a **tenancy-less** object they are the same index declared twice. + +Tenancy is deliberately not inferred at authoring time (`organization_id` is +injected by the kernel at registration, not authored), so the message names +both readings and the fix spells out the choice: `unique: 'global'` plus +dropping the index for platform-wide, or dropping the index for per-tenant +(or writing it out as `fields: ['organization_id', 'email']`). + +A field already declared `unique: 'global'` is exempt — the index restates +that intent rather than losing it. Advisory only: the artifact is well-defined, +so this never fails a build. diff --git a/content/docs/data-modeling/indexing.mdx b/content/docs/data-modeling/indexing.mdx index e66b9a67c7..4aa559f4e7 100644 --- a/content/docs/data-modeling/indexing.mdx +++ b/content/docs/data-modeling/indexing.mdx @@ -24,6 +24,51 @@ indexes: [ ] ``` +## Two ways to say "unique" — and they mean different things + +Uniqueness can be declared in two places, and the choice is not cosmetic: + +| Declaration | Materializes as | Scope | +|:---|:---|:---| +| Field-level `unique: true` | `(organization_id, field)` | Unique **within** an organization | +| Field-level `unique: 'global'` | `(field)` | Platform-wide | +| Declared index `{ fields: ['email'], unique: true }` | `(email)` — exactly the listed columns | Platform-wide | + +A field-level `unique: true` is **tenant-scoped**. It has no syntax for a +composite, so the platform supplies the tenant column for you — which is what +a multi-tenant application almost always wants: two organizations may each +have a contact `john@acme.com`. + +A **declared index is taken verbatim**. No tenant column is injected, because +many declared indexes are legitimately platform-wide (a DNS hostname, a +reserved slug, an external provider id). To scope one per tenant, list the +column yourself: `{ fields: ['organization_id', 'email'], unique: true }`. + + +**Do not declare both on the same column.** The stricter one wins physically, +so the platform-wide index enforces uniqueness and the per-tenant constraint +can never be reached — one of the two intents you wrote is silently discarded: + +```typescript +// ⚠️ contradictory — the global index wins, the per-tenant scope is dead +email: Field.email({ unique: true }), +indexes: [{ fields: ['email'], unique: true }], +``` + +`os lint` / `os build` report this as `unique/double-declaration`. Pick one: +set `unique: 'global'` on the field and drop the index for platform-wide +uniqueness, or drop the index for per-tenant uniqueness (the field-level +declaration already builds the composite). + + + +**Never put a platform-wide unique index on an `autonumber` field.** The +autonumber sequence is per tenant — every organization counts from `1` — so a +global unique index rejects the second organization's `CASE-00001` on insert. +Use `{ fields: ['organization_id', 'case_number'], unique: true }` so the +constraint matches the sequence that feeds it. + + ### When to Add Indexes ✅ **Add indexes for:** diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index 09bec64fce..7bd47d3730 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -19,6 +19,7 @@ import { validateSecurityPosture, validateOrgAxisRedLines, buildAccessMatrix, di import { validateReadonlyFlowWrites } from '@objectstack/lint'; import { lintFlowPatterns } from '../utils/lint-flow-patterns.js'; import { lintAutonumberFormats } from '../utils/lint-autonumber-formats.js'; +import { lintUniqueDeclarations } from '../lint/data-model-rules.js'; import { lintLivenessProperties } from '../utils/lint-liveness-properties.js'; import { lintViewRefs } from '../utils/lint-view-refs.js'; import { preflightRequiredCapabilities, renderCapabilityMessage } from '../utils/capability-preflight.js'; @@ -499,6 +500,28 @@ export default class Compile extends Command { } } + // 3d-quinquies. Contradictory uniqueness declarations (#3991). A column + // carrying BOTH a field-level `unique: true` and a single-column + // declared unique index has two intents, of which exactly one takes + // effect: since #3696 the field-level form is per-tenant while a + // declared index is platform-wide, so the global index wins and the + // tenant composite becomes unreachable. Advisory — the artifact is + // well-defined; the cost is a declaration that does nothing. Shares + // `lintUniqueDeclarations` with `os lint` so both agree. + const uniqueLint = lintUniqueDeclarations( + Array.isArray((result.data as Record).objects) + ? ((result.data as Record).objects as any[]) + : [], + ); + if (uniqueLint.length > 0 && !flags.json) { + console.log(''); + for (const f of uniqueLint) { + printWarning(`${f.path}: ${f.message}`); + if (f.fix) console.log(chalk.dim(` ${f.fix}`)); + console.log(chalk.dim(` rule: ${f.rule}`)); + } + } + // 3d-quater. View-reference lint (#2554) — resolves form action targets // and view-key collisions at build time. A `type:'form'` target that // names a missing view or a LIST view opens a broken/blank form at diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index ea879437fc..d3b8a7c14a 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -24,6 +24,7 @@ import { validateFlowTriggerReadiness } from '@objectstack/lint'; import { validateReadonlyFlowWrites } from '@objectstack/lint'; import { lintFlowPatterns } from '../utils/lint-flow-patterns.js'; import { lintAutonumberFormats } from '../utils/lint-autonumber-formats.js'; +import { lintUniqueDeclarations } from '../lint/data-model-rules.js'; import { lintLivenessProperties } from '../utils/lint-liveness-properties.js'; import { lintViewRefs } from '../utils/lint-view-refs.js'; import { preflightRequiredCapabilities, renderCapabilityMessage } from '../utils/capability-preflight.js'; @@ -592,12 +593,24 @@ export default class Validate extends Command { const viewRefErrors = viewRefLint.filter((f) => f.severity === 'error'); const viewRefWarnings = viewRefLint.filter((f) => f.severity !== 'error'); + // Contradictory uniqueness declarations (#3991) — a column carrying both a + // field-level `unique: true` and a single-column declared unique index has + // two intents, of which exactly one takes effect. Advisory. Mapped into the + // `{ where, hint }` shape the shared renderer below expects; the rule lives + // in `lint/data-model-rules.ts` so `os lint` reports the same finding. + const uniqueLintWarnings = lintUniqueDeclarations( + Array.isArray((result.data as Record).objects) + ? ((result.data as Record).objects as any[]) + : [], + ).map((f) => ({ where: f.path, message: f.message, hint: f.fix ?? '', rule: f.rule, severity: 'warning' as const })); + const authoringLintErrors = [...flowLintErrors, ...autonumberErrors, ...viewRefErrors]; const authoringLintWarnings = [ ...flowLintWarnings, ...livenessLint, ...autonumberWarnings, ...viewRefWarnings, + ...uniqueLintWarnings, ]; if (authoringLintErrors.length > 0) { if (flags.json) { diff --git a/packages/cli/src/lint/data-model-rules.ts b/packages/cli/src/lint/data-model-rules.ts index a1c42a8728..9221b55581 100644 --- a/packages/cli/src/lint/data-model-rules.ts +++ b/packages/cli/src/lint/data-model-rules.ts @@ -69,6 +69,92 @@ function refOf(def: any): string | undefined { return def?.reference || def?.reference_to; } +// ─── Uniqueness declarations ──────────────────────────────────────── + +export const UNIQUE_DOUBLE_DECLARATION = 'unique/double-declaration'; + +/** Is `unique` declared at all? Mirrors `isUniqueDeclared` in @objectstack/spec/data. */ +function uniqueDeclared(u: unknown): boolean { + return u === true || u === 'global'; +} + +/** + * R10 — the same column carries BOTH a field-level `unique: true` and an + * object-level single-column unique index (#3991). + * + * The two spellings are deliberately different (see `IndexSchema`): field-level + * `unique: true` is tenant-scoped since #3696 — it materializes as + * `(organization_id, col)`, unique *within* the tenant — while a declared index + * is materialized over exactly the columns listed, i.e. platform-wide. Both are + * legitimate on their own; together on one column they are never right: + * + * - On a tenant-scoped object they CONTRADICT. The stricter one wins + * physically, so the global index enforces uniqueness and the tenant + * composite becomes a constraint nothing can ever trip. One of the two + * intents the author wrote is silently discarded. + * - On a tenancy-less object they are exactly REDUNDANT — both describe the + * same single-column unique index, under the same generated name. + * + * Tenancy is deliberately NOT inferred here: `organization_id` is injected by + * the kernel at registration rather than authored, so an authoring-time guess + * would be wrong half the time. The combination is worth flagging either way, + * and the message names both readings so the author picks the one they meant. + * + * A field declared `unique: 'global'` is exempt: it already says + * platform-wide, so the declared index restates the same intent rather than + * contradicting it (still redundant, but not a silent loss of meaning). + * + * 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[] = []; + if (!Array.isArray(objects) || objects.length === 0) return issues; + + for (let i = 0; i < objects.length; i++) { + const obj = objects[i]; + if (!obj?.name) continue; + const declaredIndexes = Array.isArray(obj.indexes) ? obj.indexes : []; + if (declaredIndexes.length === 0) continue; + + // Columns covered by a declared SINGLE-column unique index. A composite + // (`['organization_id', 'email']`) is the explicit tenant-scoped spelling — + // it agrees with the field-level default rather than fighting it. + const singleColumnUniqueIndexes = new Map(); + for (const idx of declaredIndexes) { + if (!uniqueDeclared(idx?.unique)) continue; + const cols = Array.isArray(idx?.fields) ? idx.fields.filter((f: unknown) => typeof f === 'string') : []; + if (cols.length !== 1) continue; + if (!singleColumnUniqueIndexes.has(cols[0])) singleColumnUniqueIndexes.set(cols[0], idx); + } + if (singleColumnUniqueIndexes.size === 0) continue; + + for (const { name, def } of fieldEntries(obj.fields)) { + if (!uniqueDeclared(def?.unique)) continue; + if (def.unique === 'global') continue; // already says platform-wide — no lost intent + const idx = singleColumnUniqueIndexes.get(name); + if (!idx) continue; + const indexLabel = typeof idx?.name === 'string' && idx.name.trim() ? ` '${idx.name.trim()}'` : ''; + issues.push({ + severity: 'warning', + rule: UNIQUE_DOUBLE_DECLARATION, + message: + `"${obj.name}.${name}" declares field-level \`unique: true\` AND a single-column unique index${indexLabel} on the same column. ` + + `Since #3696 the field-level form is scoped per tenant — \`(tenant, ${name})\` — while a declared index is materialized ` + + `over exactly its \`fields\`, i.e. platform-wide. On a tenant-scoped object the global index wins and the per-tenant ` + + `constraint can never be reached; on a tenancy-less object the two are the same index declared twice. Either way one of ` + + `the two declarations has no effect.`, + path: `objects[${i}]`, + fix: + `Pick the intent: for platform-wide uniqueness set \`unique: 'global'\` on '${name}' and drop the duplicate index; ` + + `for per-tenant uniqueness drop the index (the field-level declaration already builds the tenant composite), ` + + `or spell the index out as \`fields: ['organization_id', '${name}']\` if you want it explicit.`, + }); + } + } + return issues; +} + // ─── Rule engine ──────────────────────────────────────────────────── /** @@ -77,7 +163,9 @@ function refOf(def: any): string | undefined { * metadata-generation scorer. */ export function lintDataModel(objects: any[]): LintIssue[] { - const issues: LintIssue[] = []; + // R10 lives in its own exported function so `os build` can run that ONE rule + // without pulling in the whole best-practice sweep (#3991). + const issues: LintIssue[] = lintUniqueDeclarations(objects); if (!Array.isArray(objects) || objects.length === 0) return issues; // Index: parent object name → child relationships pointing at it. diff --git a/packages/cli/test/data-model-rules.test.ts b/packages/cli/test/data-model-rules.test.ts index 9e7ed6f8ac..ccf7007972 100644 --- a/packages/cli/test/data-model-rules.test.ts +++ b/packages/cli/test/data-model-rules.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { lintDataModel } from '../src/lint/data-model-rules'; +import { lintDataModel, lintUniqueDeclarations } from '../src/lint/data-model-rules'; import { lintConfig } from '../src/commands/lint'; const rulesOf = (issues: { rule: string }[]) => issues.map((i) => i.rule); @@ -173,3 +173,119 @@ describe('lintConfig integration', () => { expect(dataModel.filter((i) => i.severity !== 'suggestion')).toEqual([]); }); }); + +// #3991 — the same column declared unique twice, in two spellings that mean +// different things. One of the two intents is always discarded. +describe('lintUniqueDeclarations — contradictory uniqueness (#3991)', () => { + const RULE = 'unique/double-declaration'; + + const withBoth = [ + { + name: 'crm_contact', + fields: { email: { type: 'email', unique: true } }, + indexes: [{ fields: ['email'], unique: true }], + }, + ]; + + it('returns [] for empty input', () => { + expect(lintUniqueDeclarations([])).toEqual([]); + expect(lintUniqueDeclarations(undefined as any)).toEqual([]); + }); + + it('flags field-level unique + a single-column unique index on the same column', () => { + const issues = lintUniqueDeclarations(withBoth); + expect(issues).toHaveLength(1); + expect(issues[0].rule).toBe(RULE); + expect(issues[0].severity).toBe('warning'); // advisory — never fails a build + expect(issues[0].message).toContain('crm_contact.email'); + // The message must name BOTH readings, since tenancy is not inferred here. + expect(issues[0].message).toMatch(/per tenant|tenant/i); + expect(issues[0].message).toMatch(/platform-wide/i); + // And the fix must spell out both ways to resolve it. + expect(issues[0].fix).toContain("unique: 'global'"); + expect(issues[0].fix).toContain('organization_id'); + }); + + it('surfaces through lintDataModel too, so `os lint` reports it', () => { + expect(has(lintDataModel(withBoth), RULE)).toBe(true); + }); + + // ── Shapes that must stay quiet ────────────────────────────────────── + + it("exempts unique: 'global' — the index restates the intent, it does not lose it", () => { + const issues = lintUniqueDeclarations([ + { + name: 'runtime', + fields: { hostname: { type: 'text', unique: 'global' } }, + indexes: [{ fields: ['hostname'], unique: true }], + }, + ]); + expect(issues).toEqual([]); + }); + + it('exempts an explicit tenant COMPOSITE index — that agrees with the field-level default', () => { + const issues = lintUniqueDeclarations([ + { + name: 'crm_contact', + fields: { email: { type: 'email', unique: true } }, + indexes: [{ fields: ['organization_id', 'email'], unique: true }], + }, + ]); + expect(issues).toEqual([]); + }); + + it('ignores a NON-unique index on the same column', () => { + const issues = lintUniqueDeclarations([ + { + name: 'crm_contact', + fields: { email: { type: 'email', unique: true } }, + indexes: [{ fields: ['email'] }], + }, + ]); + expect(issues).toEqual([]); + }); + + it('ignores a unique index on a DIFFERENT column', () => { + const issues = lintUniqueDeclarations([ + { + name: 'crm_contact', + fields: { email: { type: 'email', unique: true }, code: { type: 'text' } }, + indexes: [{ fields: ['code'], unique: true }], + }, + ]); + expect(issues).toEqual([]); + }); + + it('is quiet when only one of the two spellings is used', () => { + expect(lintUniqueDeclarations([ + { name: 'a', fields: { email: { type: 'email', unique: true } } }, + ])).toEqual([]); + expect(lintUniqueDeclarations([ + { name: 'b', fields: { email: { type: 'email' } }, indexes: [{ fields: ['email'], unique: true }] }, + ])).toEqual([]); + }); + + it('names the declared index when it carries an explicit name', () => { + const issues = lintUniqueDeclarations([ + { + name: 'crm_product', + fields: { sku: { type: 'text', unique: true } }, + indexes: [{ name: 'uniq_product_sku', fields: ['sku'], unique: true }], + }, + ]); + expect(issues[0].message).toContain("'uniq_product_sku'"); + }); + + it('reports each offending column once, across several objects', () => { + const issues = lintUniqueDeclarations([ + ...withBoth, + { + name: 'crm_lead', + fields: { email: { type: 'email', unique: true }, sku: { type: 'text', unique: true } }, + indexes: [{ fields: ['email'], unique: true }, { fields: ['sku'], unique: true }], + }, + ]); + expect(issues.map((i) => i.message.match(/"([^"]+)"/)?.[1]).sort()) + .toEqual(['crm_contact.email', 'crm_lead.email', 'crm_lead.sku']); + }); +});