diff --git a/.changeset/tenant-index-declared-in-indexes.md b/.changeset/tenant-index-declared-in-indexes.md new file mode 100644 index 0000000000..50f0dd515a --- /dev/null +++ b/.changeset/tenant-index-declared-in-indexes.md @@ -0,0 +1,62 @@ +--- +"@objectstack/objectql": patch +"@objectstack/driver-mongodb": patch +--- + +fix(objectql,driver-mongodb): declare the tenant index in `indexes[]`, so a registry-backed object stops reporting itself invalid (#6810) + +`applySystemFields` provisioned the injected `organization_id` column with +`indexed: opts.multiTenant`. `indexed` is **not a `FieldSchema` key** — #2377 / +ADR-0049 removed it because a field-level index flag built no index — and +`FieldSchema` is a `strictObject`, so a field carrying it is rejected **by +name**, with a purpose-written message. + +`registerObject` runs `applySystemFields` *before* storing and +`getItem('object', …)` serves that post-injection document, so the key travelled +all the way out to `/meta`, where `decorateMetadataItem` re-parsed the served +body and stamped the verdict on it. Measured on every registry-backed object, in +**both** tenancy modes, at **both** read exits: + +``` +_diagnostics: { valid: false, + errors: [{ path: 'fields.organization_id', code: 'unrecognized_keys' }] } +``` + +`_diagnostics` is what Studio renders invalid-metadata banners from and what an +AI author reads to judge a document it produced. So the platform was reporting a +defect on its own column — one the author never wrote and could not fix — and +making the verdict useless as a signal on those objects, because a real +authoring error was indistinguishable from this one. + +**Two directions, both of them user-visible:** + +- **The false `valid: false` verdict is gone.** A tenancy-enabled object + registered through the real `SchemaRegistry` now reads back + `_diagnostics: { valid: true }` at both `/meta` exits, in both tenancy modes. + Nothing else about the served field changed — `type`, `reference`, and the + governance keys that decide who may write it are byte-identical. +- **The tenant index moved from a field-level flag to `indexes[]`**, the one + surface an index is declared on in this system. On a multi-tenant stack the + object now declares `{ fields: ['organization_id'] }`; on a single-tenant + stack it declares **nothing** — the absence *is* what `indexed: false` used to + say, since nothing filters by organization on an unwalled stack. + +This is also the first time the intent is actually **enforced**. The sole reader +of the old flag was one line in `driver-mongodb`; `driver-sql` — which every +walled deployment runs — only ever materialized `indexes[]`, so the wall's +hottest predicate ran unindexed no matter what the flag said. Expect the tenant +index to now appear as ordinary index drift on existing SQL tables +(`idx__organization_id`), created by `os migrate apply` or by the +`autoMigrate: 'safe'` path in dev, like any other declared index. + +`driver-mongodb` reads declared `indexes[]` in place of the retired flag. The +generated index name matches the field-level convention already in that file +(`idx_` / `idx__unique`), so a re-synced collection finds its +existing `idx_organization_id` rather than building a second index under a new +name. Declarations are materialized over their columns **verbatim** at every +`unique` scope, `'organization'` included — the same call the driver's +field-level `unique` documents, because it implements no row-level tenancy and +refuses to boot into a multi-tenant deployment (#3724). + +No `FieldSchema` change: re-declaring `indexed` would restore exactly the +declared-but-unenforced key #2377 removed. diff --git a/packages/drivers/driver-mongodb/README.md b/packages/drivers/driver-mongodb/README.md index 102197a8b0..0ace28d9be 100644 --- a/packages/drivers/driver-mongodb/README.md +++ b/packages/drivers/driver-mongodb/README.md @@ -158,14 +158,20 @@ try { Schema sync creates collections and indexes: +Field-level `unique` and lookup fields index themselves; everything else is +declared in the object's `indexes[]` — the one surface an index is declared on +(a field-level `indexed` flag is not a `FieldSchema` key and never built an +index, #2377 / #6810). + ```typescript await driver.syncSchema('account', { name: 'account', fields: { name: { type: 'string', unique: true }, - email: { type: 'email', indexed: true }, + email: { type: 'email' }, company_id: { type: 'lookup', reference_to: 'company' }, }, + indexes: [{ fields: ['email'] }], }); // Creates: idx_id_unique, idx_name_unique, idx_email, idx_company_id_lookup ``` diff --git a/packages/drivers/driver-mongodb/src/mongodb-driver.test.ts b/packages/drivers/driver-mongodb/src/mongodb-driver.test.ts index efd0f76b86..7e26f41115 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-driver.test.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-driver.test.ts @@ -343,9 +343,15 @@ describe.skipIf(!sharedMongod)('MongoDBDriver', () => { name: 'account', fields: { name: { type: 'string', unique: true }, - email: { type: 'email', indexed: true }, + email: { type: 'email' }, company_id: { type: 'lookup', reference_to: 'company' }, }, + // [#6810] `email` used to carry a field-level `indexed: true` here. That + // was never a `FieldSchema` key (#2377 / ADR-0049); the index is + // declared in `indexes[]`, where every other index in this system is + // declared. Same resulting index name — the assertions are unchanged, + // which is the point. + indexes: [{ fields: ['email'] }], }); const db = driver.getDb(); diff --git a/packages/drivers/driver-mongodb/src/mongodb-schema-declared-indexes.test.ts b/packages/drivers/driver-mongodb/src/mongodb-schema-declared-indexes.test.ts new file mode 100644 index 0000000000..f9fb1a3ca8 --- /dev/null +++ b/packages/drivers/driver-mongodb/src/mongodb-schema-declared-indexes.test.ts @@ -0,0 +1,159 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #6810 — `syncCollectionSchema` materializes the object's DECLARED `indexes[]`. +// +// This driver used to read a field-level `indexed` flag instead. That flag was +// never a `FieldSchema` key (#2377 / ADR-0049 removed it, and `FieldSchema` is a +// `strictObject` that rejects it by name), so its one remaining producer — the +// kernel's `organization_id` injection — was stamping every registry-backed +// object with a document the platform's own schema refused. The declaration +// moved to `indexes[]`; this pins that the DDL outcome came with it, rather than +// the fix quietly deleting an index the flag used to build. +// +// Driven against a fake `Db` on purpose: the `mongodb-memory-server` suite in +// this package is OPT-IN (it downloads a real server binary, #5517), so a DDL +// assertion parked there would not run on any ordinary CI lane — which is +// exactly the lane that has to notice if this regresses. + +import { describe, it, expect } from 'vitest'; +import type { Db } from 'mongodb'; +import { syncCollectionSchema } from './mongodb-schema.js'; + +interface CreatedIndex { + spec: Record; + options: Record; +} + +/** + * The narrow slice of `Db` `syncCollectionSchema` touches, recording every + * `createIndex` call in order. Nothing is stubbed beyond that slice — the + * function under test runs verbatim. + */ +function fakeDb(existingCollections: string[] = []) { + const created: CreatedIndex[] = []; + const collectionsCreated: string[] = []; + const db = { + listCollections: ({ name }: { name: string }) => ({ + toArray: async () => (existingCollections.includes(name) ? [{ name }] : []), + }), + createCollection: async (name: string) => { + collectionsCreated.push(name); + }, + collection: () => ({ + createIndex: async (spec: Record, options: Record) => { + created.push({ spec, options }); + }, + }), + } as unknown as Db; + return { db, created, collectionsCreated }; +} + +/** Every index name the sync asked MongoDB to create, core indexes included. */ +const names = (created: CreatedIndex[]) => created.map((c) => c.options.name); + +/** The one recorded creation for `name`, or `undefined`. */ +const byName = (created: CreatedIndex[], name: string) => + created.find((c) => c.options.name === name); + +describe('#6810 — syncCollectionSchema materializes declared indexes[]', () => { + it('creates the kernel-declared tenant index, byte-identical to what the retired flag built', async () => { + // THE regression pin. Before #6810 this DDL came from + // `organization_id: { …, indexed: true }`; it now comes from the object's + // `indexes[]`. Same collection, same key spec, same index NAME — a + // re-synced deployment finds its index already present rather than + // building a second one under a new name. + const { db, created } = fakeDb(); + await syncCollectionSchema(db, 'lead', { + name: 'lead', + fields: { + first_name: { type: 'text' }, + organization_id: { type: 'lookup' }, + }, + indexes: [{ fields: ['organization_id'] }], + }); + + const tenant = byName(created, 'idx_organization_id'); + expect(tenant).toBeDefined(); + expect(tenant!.spec).toEqual({ organization_id: 1 }); + // A plain lookup index, never a constraint. + expect(tenant!.options.unique).toBeUndefined(); + }); + + it('declares no tenant index when the object declares none', async () => { + // The single-tenant half: `multiTenant: false` now declares NOTHING rather + // than a `false` flag, so absence has to stay absence here. + const { db, created } = fakeDb(); + await syncCollectionSchema(db, 'lead', { + name: 'lead', + fields: { first_name: { type: 'text' }, organization_id: { type: 'lookup' } }, + }); + + expect(names(created)).not.toContain('idx_organization_id'); + // The core set is untouched by any of this. + expect(names(created)).toEqual(['idx_id_unique', 'idx_created_at', 'idx_updated_at']); + }); + + it('honours an explicit index name and a multi-column declaration', async () => { + const { db, created } = fakeDb(); + await syncCollectionSchema(db, 'lead', { + name: 'lead', + fields: { organization_id: { type: 'lookup' }, code: { type: 'text' } }, + indexes: [{ name: 'lead_scope_idx', fields: ['organization_id', 'code'] }], + }); + + const idx = byName(created, 'lead_scope_idx'); + expect(idx).toBeDefined(); + // Key order follows the declaration — a compound index is order-sensitive. + expect(Object.keys(idx!.spec)).toEqual(['organization_id', 'code']); + }); + + it('materializes a declared unique index, at every scope, over the columns VERBATIM', async () => { + // `'organization'` is NOT scoped up with a tenant key part here, and that is + // deliberate — the same call `FieldDef.unique` documents in the source. + // This driver implements no row-level tenancy and refuses to boot into a + // multi-tenant deployment (#3724), so a `(tenant, field)` index would + // advertise an isolation it does not deliver. + for (const scope of [true, 'global', 'organization'] as const) { + const { db, created } = fakeDb(); + await syncCollectionSchema(db, 'lead', { + name: 'lead', + fields: { organization_id: { type: 'lookup' }, code: { type: 'text' } }, + indexes: [{ fields: ['code'], unique: scope }], + }); + + const idx = byName(created, 'idx_code_unique'); + expect(idx, String(scope)).toBeDefined(); + expect(idx!.spec).toEqual({ code: 1 }); + expect(idx!.options.unique).toBe(true); + expect(names(created)).not.toContain('idx_organization_id_code_unique'); + } + }); + + it('a declared index and a field-level `unique` on the same column converge on ONE index', async () => { + // Why the generated name mirrors the field-level convention: two routes + // asking for the same constraint must land on the same name, or MongoDB + // sees two indexes over one key spec. + const { db, created } = fakeDb(); + await syncCollectionSchema(db, 'lead', { + name: 'lead', + fields: { email: { type: 'email', unique: true } }, + indexes: [{ fields: ['email'], unique: true }], + }); + + expect(names(created).filter((n) => n === 'idx_email_unique')).toHaveLength(2); + const [a, b] = created.filter((c) => c.options.name === 'idx_email_unique'); + expect(a.spec).toEqual(b.spec); + expect(a.options).toEqual(b.options); + }); + + it('skips a declaration with no usable fields rather than emitting empty DDL', async () => { + const { db, created } = fakeDb(); + await syncCollectionSchema(db, 'lead', { + name: 'lead', + fields: { code: { type: 'text' } }, + indexes: [{ fields: [] }, { name: 'ghost' } as { name: string }], + }); + + expect(names(created)).toEqual(['idx_id_unique', 'idx_created_at', 'idx_updated_at']); + }); +}); diff --git a/packages/drivers/driver-mongodb/src/mongodb-schema.ts b/packages/drivers/driver-mongodb/src/mongodb-schema.ts index 0f1a7a2e97..7373499ec6 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-schema.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-schema.ts @@ -33,18 +33,37 @@ interface FieldDef { * `SqlDriver.uniqueIndexesFromFields`. */ unique?: boolean | 'global'; - indexed?: boolean; required?: boolean; reference_to?: string; multiple?: boolean; } +/** + * A declared object-level index — `IndexSchema` in @objectstack/spec, narrowed + * to what this driver materializes. + * + * [#6810] This is the ONE surface an index is declared on. The field-level + * `indexed` flag this driver used to read alongside it was never a + * `FieldSchema` key: #2377 / ADR-0049 removed it because a field-level index + * flag built no index, and `FieldSchema` — a `strictObject` — rejects it by + * name. The single producer still emitting it was the kernel's + * `organization_id` injection, which therefore stamped every registry-backed + * object with a document its own schema refused. That declaration moved to + * `indexes[]`; nothing else in the repo ever read the flag, so it is gone. + */ +interface IndexDef { + name?: string; + fields?: string[]; + unique?: boolean | 'global' | 'organization'; +} + /** * ObjectStack object definition (subset needed for schema sync). */ interface ObjectDef { name: string; fields?: Record; + indexes?: IndexDef[]; } /** @@ -53,8 +72,9 @@ interface ObjectDef { * - Creates the collection if it doesn't exist * - Creates a unique index on `id` * - Creates indexes on `created_at` and `updated_at` - * - Creates indexes for fields marked `unique` or `indexed` + * - Creates indexes for fields marked `unique` * - Creates indexes on lookup (reference) fields + * - Creates the object's DECLARED `indexes[]` (#6810) */ export async function syncCollectionSchema( db: Db, @@ -84,11 +104,6 @@ export async function syncCollectionSchema( spec: { [fieldName]: 1 }, options: { unique: true, sparse: true, name: `idx_${fieldName}_unique` }, }); - } else if (field.indexed) { - indexOps.push({ - spec: { [fieldName]: 1 }, - options: { name: `idx_${fieldName}` }, - }); } // Lookup + user (a lookup specialized to sys_user) fields get an index for @@ -106,6 +121,37 @@ export async function syncCollectionSchema( } } + // Declared object-level indexes (#6810) — the surface `indexes[]`, which is + // where every other index in this system is declared and where the kernel now + // declares the tenant index on `organization_id`. + // + // The generated name is `idx_` / `idx__unique`, matching the + // field-level convention above so the two routes converge on ONE index rather + // than racing to create two with different options on the same column set + // (Mongo index names are per-collection, so no table qualifier is needed — + // unlike `SqlDriver`'s `buildIndexName`, which is why neither driver takes a + // name from the declaration when the author left it out). + // + // Every `unique` scope materializes the columns VERBATIM, `'organization'` + // included. That is the same call `FieldDef.unique` documents above and for + // the same reason: this driver implements no row-level tenancy at all and + // refuses to boot into a multi-tenant deployment (#3724), so prepending a + // tenant key part would advertise an isolation it does not deliver. + for (const idx of schema.indexes ?? []) { + const fields = (idx.fields ?? []).filter((f) => typeof f === 'string' && f.length > 0); + if (fields.length === 0) continue; + const unique = Boolean(idx.unique); + const spec: Record = {}; + for (const f of fields) spec[f] = 1; + indexOps.push({ + spec: spec as IndexSpecification, + options: { + ...(unique ? { unique: true, sparse: true } : {}), + name: idx.name ?? `idx_${fields.join('_')}${unique ? '_unique' : ''}`, + }, + }); + } + // Create indexes (idempotent — MongoDB ignores duplicates) for (const { spec, options } of indexOps) { try { diff --git a/packages/metadata-core/src/injected-system-columns.ts b/packages/metadata-core/src/injected-system-columns.ts index ab4d869a8a..4a43073e85 100644 --- a/packages/metadata-core/src/injected-system-columns.ts +++ b/packages/metadata-core/src/injected-system-columns.ts @@ -44,13 +44,13 @@ * (2026-08-08) is Option B: the read serves the EFFECTIVE runtime schema, and * the overlay-backed minority path converges on the registry-backed majority. * - * ## The one key this table deliberately does NOT carry: `indexed` + * ## The key that used to sit beside this table: `indexed` (#6810, closed) * - * `applySystemFields` stamps `indexed: ` onto its `organization_id` - * definition, for the MongoDB driver's schema builder (the only consumer; - * `driver-mongodb/src/mongodb-schema.ts`). `indexed` is **not a `FieldSchema` - * key** — it was removed in the 16.x line (#2377, ADR-0049) and `FieldSchema` is - * `strictObject`, so an object document carrying it is rejected BY NAME: + * `applySystemFields` used to stamp `indexed: ` on top of + * {@link TENANT_SCOPE_FIELD_DEF}, for the MongoDB driver's schema builder — the + * only consumer. `indexed` is **not a `FieldSchema` key**: it was removed in the + * 16.x line (#2377, ADR-0049) and `FieldSchema` is `strictObject`, so an object + * document carrying it is rejected BY NAME: * * ``` * Unrecognized key(s) on this field: `indexed`. @@ -58,13 +58,18 @@ * ``` * * Measured on `origin/main` (2026-08-08): a registry-backed `/meta` object read - * therefore already answers `_diagnostics: { valid: false }` on exactly that - * key, in BOTH multiTenant modes — filed as #6810, and deliberately not - * inherited here. Converging the overlay-backed exit onto a key the object - * schema refuses would spread that defect rather than close #6562's; the field - * SET and every spec-authorable key converge, and the DDL hint stays where the - * DDL is. `multiTenant` is also the *only* thing that key depends on, which is - * why nothing in this module takes a `multiTenant` input: per + * therefore answered `_diagnostics: { valid: false }` on exactly that key, in + * BOTH multiTenant modes — filed as #6810, and deliberately not inherited here, + * since converging the overlay-backed exit onto a key the object schema refuses + * would have spread that defect rather than closed #6562's. + * + * #6810 closed it at the injection site rather than here: the tenant index is + * declared in the object's `indexes[]` — the one surface an index is declared on + * — and no served field carries `indexed` on either exit any more. What this + * table carries is unchanged; there is simply nothing spread on top of it now. + * + * `multiTenant` was the *only* thing that key depended on, which is still why + * nothing in this module takes a `multiTenant` input: per * `resolveInjectedSystemColumns`' own measurement, the flag changes whether * `organization_id` is INDEXED, never whether it EXISTS. */ @@ -131,9 +136,9 @@ export const AUDIT_FIELD_DEFS = { /** * `organization_id` — THE tenant scope anchor, in its **authorable** shape. * - * ⚠️ `applySystemFields` spreads `indexed: opts.multiTenant` on top of this when - * it provisions the physical column; see the module header for why that key - * lives at the injection site and never in a served document. + * Spread verbatim by `applySystemFields` — nothing is layered on top of it. + * (#6810 removed the `indexed: opts.multiTenant` that used to be; the tenant + * index is declared in the object's `indexes[]` instead. See the module header.) */ export const TENANT_SCOPE_FIELD_DEF: Readonly> = { type: 'lookup', diff --git a/packages/objectql/src/injected-system-columns-parity.test.ts b/packages/objectql/src/injected-system-columns-parity.test.ts index b6959e10a6..35c90276ac 100644 --- a/packages/objectql/src/injected-system-columns-parity.test.ts +++ b/packages/objectql/src/injected-system-columns-parity.test.ts @@ -96,7 +96,11 @@ describe('[#5378] resolveInjectedSystemColumns ↔ applySystemFields parity', () } const mt = applySystemFields({ name: 'crm_contact', fields: fields() } as any, { multiTenant: true }); const st = applySystemFields({ name: 'crm_contact', fields: fields() } as any, { multiTenant: false }); - expect((mt.fields as any).organization_id.indexed).toBe(true); - expect((st.fields as any).organization_id.indexed).toBe(false); + // [#6810] The index moved from a field-level `indexed` boolean — never a + // `FieldSchema` key — to the object's `indexes[]`. The property this test + // guards is unchanged: the flag moves the INDEX, never the column set. + expect((mt as any).indexes).toEqual([{ fields: ['organization_id'] }]); + expect((st as any).indexes).toBeUndefined(); + expect((mt.fields as any).organization_id).toEqual((st.fields as any).organization_id); }); }); diff --git a/packages/objectql/src/protocol-meta-effective-schema.test.ts b/packages/objectql/src/protocol-meta-effective-schema.test.ts index 19f04e8a3c..6f6977cb68 100644 --- a/packages/objectql/src/protocol-meta-effective-schema.test.ts +++ b/packages/objectql/src/protocol-meta-effective-schema.test.ts @@ -226,17 +226,23 @@ describe.each([true, false])('[#6562] /meta object read — effective schema (mu it('the injected columns carry the SAME metadata the registry answer carries', async () => { const { registryBacked, overlayBacked } = await bothAnswers(); - // The whole disagreement, computed. `organization_id.indexed` is the ONE - // residual entry and it is not this issue's: `indexed` is not a - // `FieldSchema` key at all — removed in the 16.x line (#2377, ADR-0049) - // and rejected BY NAME by the strict schema — so `applySystemFields` - // stamping it is why a registry-backed read already answers - // `_diagnostics: { valid: false }` (pinned below, filed as #6810). Its - // only consumer is `driver-mongodb`'s schema builder, which reads the - // REGISTERED schema and never a served document. Converging the served - // answer onto a key the object schema refuses would have spread that - // defect instead of closing this one. - expect(divergences(registryBacked, overlayBacked)).toEqual(['organization_id.indexed']); + // The whole disagreement, computed — and it is now EMPTY. + // + // [#6810 — FLIPPED, deliberately] This read `['organization_id.indexed']` + // when #6562 landed. That entry was never this issue's: `indexed` is not + // a `FieldSchema` key at all — removed in the 16.x line (#2377, + // ADR-0049) and rejected BY NAME by the strict schema — so + // `applySystemFields` stamping it was why a registry-backed read + // answered `_diagnostics: { valid: false }` (pinned below). #6562 + // deliberately did NOT converge onto it, because converging onto a key + // the object schema refuses spreads that defect rather than closing this + // one, and pinned the residual in both directions so the fix at the + // injection site had to come back and flip it rather than leave a stale + // expectation behind. #6810 did exactly that: the tenant index is + // declared in the object's `indexes[]` now, the field definition carries + // only authorable keys, and the two producers agree on every key of + // every field. + expect(divergences(registryBacked, overlayBacked)).toEqual([]); // …and the markers the `engine-audit-anchor-write` pin is about, spelled // out so a failure names the contract rather than a key list. @@ -261,10 +267,35 @@ describe.each([true, false])('[#6562] /meta object read — effective schema (mu // `injected-system-columns-parity.test.ts` pins it for the injection // pass; this is the same fact one layer up, on the SERVED document — // which is why nothing in the read path takes a `multiTenant` input. + // + // [#6810 — FLIPPED] The index used to be read off the field + // (`registryBacked.fields.organization_id.indexed === multiTenant`). It + // is declared in `indexes[]` now, so the same fact is read there, and + // `multiTenant: false` is the ABSENCE of a declaration rather than one + // whose value is false. NO field on either answer carries `indexed` any + // more — asserted, because that key's presence is the whole of #6810. const { registryBacked, overlayBacked } = await bothAnswers(); expect(namesOf(registryBacked)).toContain('organization_id'); expect(overlayBacked.fields.organization_id.indexed).toBeUndefined(); - expect(registryBacked.fields.organization_id.indexed).toBe(multiTenant); + expect(registryBacked.fields.organization_id.indexed).toBeUndefined(); + + const tenantIndexes = (doc: any) => + (doc.indexes ?? []).filter( + (i: any) => Array.isArray(i?.fields) && i.fields.length === 1 + && i.fields[0] === 'organization_id', + ); + expect(tenantIndexes(registryBacked)).toEqual(multiTenant ? [{ fields: ['organization_id'] }] : []); + + // The one residual this fix leaves, recorded rather than left to be + // rediscovered: the DECLARATION does not converge the way the field set + // does. `divergences()` above compares fields, and the overlay-backed + // answer is rebuilt from the stored body, which declares no indexes. It + // is inert on this surface — a driver materializes from the REGISTERED + // schema, never from a served document (the same reasoning #6562 used to + // leave the flag at the injection site), and both answers parse green + // either way. If a served-document consumer of `indexes[]` ever appears, + // this is the line that says so. + expect(tenantIndexes(overlayBacked)).toEqual([]); }); it('the served correction never becomes a phantom customization', async () => { @@ -283,19 +314,25 @@ describe.each([true, false])('[#6562] /meta object read — effective schema (mu .toContain('organization_id'); }); - it('[residual, filed as #6810] only the registry-backed answer fails its own schema', async () => { - // Not a defect this PR introduces and not one it papers over: the - // registry stamps `indexed`, `FieldSchema` rejects it by name, so the - // registry-backed exit has been answering `valid: false` on every - // multi-tenant-capable object since #4001 closed the schema. Pinned in - // both directions so #6810's fix at the injection site has to come back - // and flip these lines, rather than leave a stale expectation behind. + it('[#6810, closed] BOTH answers now pass their own schema', async () => { + // [#6810 — FLIPPED, deliberately. Do not read the old text as history to + // restore.] This block used to assert the inverse: + // + // expect(overlayBacked._diagnostics).toEqual({ valid: true }); + // expect(registryBacked._diagnostics.valid).toBe(false); + // expect(registryBacked._diagnostics.errors[0]).toMatchObject({ + // path: 'fields.organization_id', code: 'unrecognized_keys' }); + // + // The registry stamped `indexed`, `FieldSchema` rejected it by name, and + // the registry-backed exit had been answering `valid: false` on every + // multi-tenant-capable object since #4001 closed the schema. #6562 + // pinned that in BOTH directions precisely so the fix at the injection + // site could not quietly forget it; #6810 moved the declaration to + // `indexes[]` and the verdict inverted. Asserted as the whole object, not + // `.valid` alone: a fix that left an `errors` array behind a `true` + // verdict would be a different defect. const { registryBacked, overlayBacked } = await bothAnswers(); expect(overlayBacked._diagnostics).toEqual({ valid: true }); - expect(registryBacked._diagnostics.valid).toBe(false); - expect(registryBacked._diagnostics.errors[0]).toMatchObject({ - path: 'fields.organization_id', - code: 'unrecognized_keys', - }); + expect(registryBacked._diagnostics).toEqual({ valid: true }); }); }); diff --git a/packages/objectql/src/registry-tenancy-posture.test.ts b/packages/objectql/src/registry-tenancy-posture.test.ts index 517b78fea2..436d67f8e5 100644 --- a/packages/objectql/src/registry-tenancy-posture.test.ts +++ b/packages/objectql/src/registry-tenancy-posture.test.ts @@ -18,6 +18,16 @@ // deployment. That is the fact these tests pin, deliberately and narrowly, // rather than the broader "columns go missing" reading. // +// [#6810] WHERE that index is declared moved, and these assertions moved with +// it: from a field-level `organization_id.indexed` boolean to an entry in the +// object's `indexes[]`. The boolean was never a `FieldSchema` key (#2377 / +// ADR-0049) and only `driver-mongodb` ever read it, so what #5262 pinned as +// "the index" was in truth a flag that built nothing on the SQL drivers every +// walled deployment runs. Reading `indexes[]` is the first spelling of this +// fact a driver actually materializes — and `multiTenant: false` now says +// "no index declared" rather than "an index declared false". The posture logic +// under test is untouched; only the surface the answer is read off. +// // Driven through the REAL `SchemaRegistry` constructor + `registerObject` // pipeline, reading the stored definition back out — the same path the kernel // takes at boot. Nothing about the resolver is stubbed: the tests set the real @@ -49,10 +59,20 @@ const registerUnder = (env: { posture?: string; legacy?: string }) => { 'crm', 'own', ); - const stored = (registry as any).objectContributors.get('lead')[0].definition; - return stored.fields.organization_id; + return (registry as any).objectContributors.get('lead')[0].definition; }; +/** + * [#6810] Is the tenant index declared on the object this posture produced? + * + * The `indexes[]` entry the drivers materialize — the successor to the + * field-level `indexed` boolean these tests used to read. + */ +const indexesTenantColumn = (stored: any): boolean => + (stored.indexes ?? []).some( + (i: any) => Array.isArray(i?.fields) && i.fields.length === 1 && i.fields[0] === 'organization_id', + ); + beforeEach(() => { delete process.env.OS_TENANCY_POSTURE; delete process.env.OS_MULTI_ORG_ENABLED; @@ -69,11 +89,11 @@ describe('#5262 — SchemaRegistry keys its multi-tenant default off OS_TENANCY_ // THE regression. Configured exactly as the v17 docs say: the authoritative // knob and nothing else. Before the fix `resolveMultiOrgEnabled()` returned // false here and the column landed UNINDEXED on a fully walled deployment. - const field = registerUnder({ posture: 'isolated' }); + const stored = registerUnder({ posture: 'isolated' }); - expect(field).toBeDefined(); - expect(field.reference).toBe('sys_organization'); - expect(field.indexed).toBe(true); + expect(stored.fields.organization_id).toBeDefined(); + expect(stored.fields.organization_id.reference).toBe('sys_organization'); + expect(indexesTenantColumn(stored)).toBe(true); }); it('`group` is a walled posture too — not just `isolated`', () => { @@ -82,29 +102,29 @@ describe('#5262 — SchemaRegistry keys its multi-tenant default off OS_TENANCY_ // just as much (ADR-0105 D1); it only widens READ scope to the membership // set, and `organization_id IN (...)` needs the index every bit as much as // `organization_id = ?` does. - expect(registerUnder({ posture: 'group' }).indexed).toBe(true); + expect(indexesTenantColumn(registerUnder({ posture: 'group' }))).toBe(true); }); it('legacy-boolean-only deployment keeps working — back-compat via the posture resolver', () => { // Nothing already deployed changes behaviour: `resolveTenancyPosture()` // falls back to `OS_MULTI_ORG_ENABLED` when the posture knob is unset, so // the pre-ADR-0105 configuration still resolves to `isolated`. - expect(registerUnder({ legacy: 'true' }).indexed).toBe(true); + expect(indexesTenantColumn(registerUnder({ legacy: 'true' }))).toBe(true); }); it('single-org deployments still leave the column unindexed', () => { // Intent unchanged — only the knob is corrected. Nothing filters by // organization on an unwalled stack, so the index would be dead weight. - expect(registerUnder({ posture: 'single' }).indexed).toBe(false); - expect(registerUnder({ legacy: 'false' }).indexed).toBe(false); - expect(registerUnder({}).indexed).toBe(false); + expect(indexesTenantColumn(registerUnder({ posture: 'single' }))).toBe(false); + expect(indexesTenantColumn(registerUnder({ legacy: 'false' }))).toBe(false); + expect(indexesTenantColumn(registerUnder({}))).toBe(false); }); it('an explicit legacy `false` does not veto the authoritative posture', () => { // The precise inversion the demotion created: the canonical knob asks for a // wall, the superseded one says "no multi-org". The canonical knob wins — // otherwise the legacy flag would still be authoritative in disguise. - expect(registerUnder({ posture: 'isolated', legacy: 'false' }).indexed).toBe(true); + expect(indexesTenantColumn(registerUnder({ posture: 'isolated', legacy: 'false' }))).toBe(true); }); it('an explicit `multiTenant` option still overrides the env entirely', () => { @@ -114,14 +134,15 @@ describe('#5262 — SchemaRegistry keys its multi-tenant default off OS_TENANCY_ const registry = new SchemaRegistry({ multiTenant: false }); registry.registerObject({ name: 'lead', fields: {} }, 'crm', 'crm', 'own'); const stored = (registry as any).objectContributors.get('lead')[0].definition; - expect(stored.fields.organization_id.indexed).toBe(false); + expect(indexesTenantColumn(stored)).toBe(false); + expect(stored.fields.organization_id).toBeDefined(); }); it('the column itself is provisioned either way — only the index moves', () => { // Guards the claim this file is scoped on. If a future change makes the // COLUMN conditional again, the blast radius of a posture misread grows // from "slow" to "the wall has nothing to filter on", and this fails. - expect(registerUnder({ posture: 'isolated' })).toBeDefined(); - expect(registerUnder({ posture: 'single' })).toBeDefined(); + expect(registerUnder({ posture: 'isolated' }).fields.organization_id).toBeDefined(); + expect(registerUnder({ posture: 'single' }).fields.organization_id).toBeDefined(); }); }); diff --git a/packages/objectql/src/registry-tenant-index-declaration.test.ts b/packages/objectql/src/registry-tenant-index-declaration.test.ts new file mode 100644 index 0000000000..dcc0de9481 --- /dev/null +++ b/packages/objectql/src/registry-tenant-index-declaration.test.ts @@ -0,0 +1,200 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #6810 — the injected `organization_id` no longer carries a key `FieldSchema` +// rejects, so a registry-backed object reads back `_diagnostics: { valid: true }`. +// +// The defect: `applySystemFields` provisioned the tenant column with +// `indexed: opts.multiTenant`. `indexed` is not a `FieldSchema` key — #2377 / +// ADR-0049 removed it because a field-level index flag built no index — and +// `FieldSchema` is a `strictObject`, so it was rejected BY NAME with a +// purpose-written message. `registerObject` runs `applySystemFields` BEFORE +// storing and `getItem('object', …)` serves that post-injection document, so the +// key travelled out to `/meta`, where `decorateMetadataItem` re-parsed the served +// body and stamped `valid: false` on it. Measured on `origin/main` @ `4fedb11`: +// BOTH tenancy modes, BOTH read exits, on every registry-backed object — a +// defect report the platform wrote about its OWN column, on a document the +// author never wrote and could not fix, in the exact channel Studio renders +// invalid-metadata banners from and an AI author reads to judge its own work. +// +// The fix declares the tenant index in the object's `indexes[]` instead, which +// is where every other index in this system is declared. So these tests pin two +// halves that must travel together: the false verdict is gone, AND the index is +// still declared — a fix that merely deleted the key would pass the first half +// while silently dropping the intent. +// +// Driven through the REAL `SchemaRegistry` + the REAL +// `ObjectStackProtocolImplementation`, i.e. the same path a `/meta` request +// takes at runtime. Nothing about the parse or the decoration is stubbed. + +import { describe, it, expect } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +// [#5619] The producer's OWN write-verb dispatch decisions, so the fake engine +// below cannot accept a call ObjectQL refuses. +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { SchemaRegistry, applySystemFields } from './registry.js'; + +/** A plain business object — one authored field, nothing else. */ +const LEAD = { name: 'lead', label: 'Lead', fields: { first_name: { type: 'text' } } } as any; + +/** + * The registry-backed `/meta` surface for a tenancy-enabled object, with no DB + * behind it: every overlay lookup answers empty, so both exits are served from + * the SchemaRegistry — the path the defect lived on. + */ +function metaSurface(multiTenant: boolean) { + const registry = new SchemaRegistry({ multiTenant }); + registry.registerObject(LEAD, 'crm', 'crm', 'own'); + const engine = { + registry, + find: async () => [], + findOne: async () => null, + insert: async () => ({ id: 'x' }), + update: async (_t: string, data: Record, opts?: Record) => { + assertEngineUpdateDispatch(data, opts); + return { id: 'x' }; + }, + delete: async (_t: string, opts?: Record) => { + assertEngineDeleteDispatch(opts); + return { deleted: 0 }; + }, + count: async () => 0, + aggregate: async () => [], + } as any; + return { registry, protocol: new ObjectStackProtocolImplementation(engine) }; +} + +/** The stored (post-injection) definition, as `registerObject` left it. */ +const storedDefinition = (registry: SchemaRegistry) => + (registry as any).objectContributors.get('lead')[0].definition; + +/** Declared indexes whose column list is exactly `['organization_id']`. */ +const tenantIndexes = (def: any) => + (def.indexes ?? []).filter( + (i: any) => Array.isArray(i?.fields) && i.fields.length === 1 && i.fields[0] === 'organization_id', + ); + +describe('#6810 — a registry-backed object reads back valid at both /meta exits', () => { + for (const multiTenant of [true, false]) { + describe(`multiTenant=${multiTenant}`, () => { + it('getMetaItem answers _diagnostics: { valid: true }', async () => { + const { protocol } = metaSurface(multiTenant); + const res: any = await protocol.getMetaItem({ type: 'object', name: 'lead' }); + + // Asserted as the whole object, not `.valid` alone: the pre-fix body + // carried an `errors` array alongside the verdict, and a fix that left + // errors behind a `true` verdict would be a different defect. + expect(res.item._diagnostics).toEqual({ valid: true }); + }); + + it('getMetaItems (the list exit) answers _diagnostics: { valid: true }', async () => { + const { protocol } = metaSurface(multiTenant); + const res: any = await protocol.getMetaItems({ type: 'object' }); + const lead = res.items.find((i: any) => i.name === 'lead'); + + expect(lead).toBeDefined(); + expect(lead._diagnostics).toEqual({ valid: true }); + }); + + it('no served field carries the retired `indexed` key', async () => { + // The verdict is the symptom; this is the cause. Asserted across EVERY + // injected column so a future injection cannot reintroduce the shape + // one field over (`owner_id`, `owning_business_unit_id`, the audit + // family) and be caught only by the aggregate verdict above. + const { protocol } = metaSurface(multiTenant); + const res: any = await protocol.getMetaItem({ type: 'object', name: 'lead' }); + + const carrying = Object.entries(res.item.fields as Record) + .filter(([, f]) => f && Object.hasOwn(f, 'indexed')) + .map(([name]) => name); + expect(carrying).toEqual([]); + }); + }); + } + + it('multiTenant=true DECLARES the tenant index in indexes[]', async () => { + // The other half of the fix. Deleting the key would satisfy every + // assertion above while silently dropping the index the flag was asking + // for — so the declaration is pinned in its own right, at the surface the + // drivers actually materialize from. + const { registry } = metaSurface(true); + const def = storedDefinition(registry); + + expect(tenantIndexes(def)).toEqual([{ fields: ['organization_id'] }]); + // No `name`: each driver derives its own. SQL's is table-qualified + // (`idx_lead_organization_id`) because index names are schema-global there; + // Mongo's is not, because they are per-collection. A name pinned here could + // not be right for both. + expect(tenantIndexes(def)[0].name).toBeUndefined(); + // A lookup index, never a constraint. + expect(tenantIndexes(def)[0].unique).toBeUndefined(); + }); + + it('multiTenant=false declares NO tenant index — absence, not a false flag', async () => { + // What `indexed: false` used to say, said the way `indexes[]` says it. + // Nothing filters by organization on an unwalled stack, so the index is + // dead weight. + const { registry } = metaSurface(false); + const def = storedDefinition(registry); + + expect(tenantIndexes(def)).toEqual([]); + // And the COLUMN is still provisioned either way — that decoupling is + // older than this fix and must survive it (sudo writers stamp the column + // on single-tenant stacks; it just stays NULL). + expect(def.fields.organization_id).toBeDefined(); + }); + + it('the served document is otherwise byte-identical to the pre-fix one, minus the key', async () => { + // Scopes the blast radius: this changed exactly two things about the served + // field — nothing about type, reference, or the governance keys that decide + // who may write it. + const out: any = applySystemFields(LEAD, { multiTenant: true }); + expect(out.fields.organization_id).toEqual({ + type: 'lookup', + reference: 'sys_organization', + label: 'Organization', + required: false, + hidden: true, + readonly: true, + system: true, + description: + 'Tenant scope (auto-populated by org-scoping on insert; NULL on single-tenant stacks).', + }); + }); + + it("appends to an author's indexes[] rather than replacing them", async () => { + const authored: any = { + name: 'lead', + fields: { code: { type: 'text' } }, + indexes: [{ name: 'lead_code_idx', fields: ['code'], unique: true }], + }; + const out: any = applySystemFields(authored, { multiTenant: true }); + + expect(out.indexes).toEqual([ + { name: 'lead_code_idx', fields: ['code'], unique: true }, + { fields: ['organization_id'] }, + ]); + }); + + it("does not duplicate an author's own single-column organization_id index", async () => { + // An array append is the one part of this injection that is not naturally + // idempotent, unlike the field merge beside it. + const authored: any = { + name: 'lead', + fields: { code: { type: 'text' } }, + indexes: [{ name: 'my_tenant_idx', fields: ['organization_id'] }], + }; + const out: any = applySystemFields(authored, { multiTenant: true }); + + expect(out.indexes).toEqual([{ name: 'my_tenant_idx', fields: ['organization_id'] }]); + }); + + it('leaves objects that opt out of the tenant column entirely alone', async () => { + // `systemFields: false` is the hard opt-out — no column, and therefore no + // index declaration either. + const optedOut: any = { name: 'seed_table', fields: { code: { type: 'text' } }, systemFields: false }; + const out: any = applySystemFields(optedOut, { multiTenant: true }); + + expect(out.fields.organization_id).toBeUndefined(); + expect(out.indexes).toBeUndefined(); + }); +}); diff --git a/packages/objectql/src/registry.test.ts b/packages/objectql/src/registry.test.ts index b6d5a95a63..7a76cfd005 100644 --- a/packages/objectql/src/registry.test.ts +++ b/packages/objectql/src/registry.test.ts @@ -572,8 +572,12 @@ describe('applySystemFields', () => { expect(out.fields.organization_id).toBeDefined(); expect(out.fields.organization_id.type).toBe('lookup'); expect(out.fields.organization_id.reference).toBe('sys_organization'); - // Multi-tenant stacks index the column (per-tenant filtering). - expect(out.fields.organization_id.indexed).toBe(true); + // [#6810] Multi-tenant stacks index the column (per-tenant filtering) — + // declared in `indexes[]`, which is what a driver materializes from. The + // field-level `indexed: true` this used to read was never a + // `FieldSchema` key (#2377 / ADR-0049) and only ever reached one driver. + expect((out as any).indexes).toEqual([{ fields: ['organization_id'] }]); + expect(out.fields.organization_id.indexed).toBeUndefined(); // author-declared field still present expect(out.fields.first_name).toBeDefined(); }); @@ -585,7 +589,10 @@ describe('applySystemFields', () => { const out = applySystemFields(baseLead, { multiTenant: false }); expect(out.fields.organization_id).toBeDefined(); expect(out.fields.organization_id.type).toBe('lookup'); - expect(out.fields.organization_id.indexed).toBe(false); + // [#6810] "Unindexed" is now the ABSENCE of a declaration, not a + // declaration whose value is false. + expect((out as any).indexes).toBeUndefined(); + expect(out.fields.organization_id.indexed).toBeUndefined(); // audit fields are tenant-independent — still injected expect(out.fields.created_at).toBeDefined(); expect(out.fields.updated_at).toBeDefined(); diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 7c40263a95..bd3d6edbb6 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -404,17 +404,54 @@ export function applySystemFields( // Platform-owned field settings that must WIN over a declared field, rather // than lose to it like `additions` does (#4447). const overrides: Record = {}; + // Platform-owned index declarations, appended to the object's `indexes[]` — + // the ONE surface an index is declared on in this system (#6810, below). + const indexAdditions: Array<{ fields: string[] }> = []; if (wantTenant && !schema.fields?.organization_id) { - // [#6562] The authorable shape is the shared table's; `indexed` is spread on - // top HERE and only here. It is the one key of this definition that is not a - // `FieldSchema` key at all — removed in the 16.x line (#2377, ADR-0049), and - // `FieldSchema` is `strictObject`, so a document carrying it is rejected by - // name ("never a FieldSchema key; a field-level index flag built no index"). - // Its only consumer is `driver-mongodb`'s schema builder, which reads the - // REGISTERED schema and never a served `/meta` document — so it stays at the - // injection site and the served answer converges on everything else. - additions.organization_id = { ...TENANT_SCOPE_FIELD_DEF, indexed: opts.multiTenant }; + // [#6562] The authorable shape is the shared table's, spread verbatim. + // + // [#6810] Nothing is spread ON TOP of it any more. This line used to read + // `{ ...TENANT_SCOPE_FIELD_DEF, indexed: opts.multiTenant }`, and `indexed` + // is the one key that was never a `FieldSchema` key at all — removed in the + // 16.x line (#2377, ADR-0049) because a field-level index flag built no + // index, and `FieldSchema` is a `strictObject`, so a document carrying it is + // rejected BY NAME ("never a FieldSchema key; a field-level index flag built + // no index"). #6562's reasoning for leaving it here — that its only consumer + // reads the REGISTERED schema, never a served document — held for the + // consumer and not for the document: `registerObject` runs this function + // BEFORE storing and `getItem('object', …)` serves that post-injection + // document, so the key reached `/meta`, where `decorateMetadataItem` + // re-parsed the served body and stamped `_diagnostics: { valid: false, + // errors: [{ path: 'fields.organization_id', code: 'unrecognized_keys' }] }` + // on EVERY registry-backed object — both tenancy modes, both read exits. A + // defect report the platform wrote about its own column, on a document the + // author never wrote and could not fix, in the channel Studio renders + // invalid-metadata banners from. + additions.organization_id = { ...TENANT_SCOPE_FIELD_DEF }; + + // [#6810] So the tenant index is declared where every other index in this + // system is declared: the object's `indexes[]`. + // + // This is also the first time the intent is actually ENFORCED. The sole + // reader of the old flag was one line in `driver-mongodb` + // (`mongodb-schema.ts`), while `driver-sql` — which every walled deployment + // runs — only ever materialized `indexes[]`, so the wall's hottest predicate + // ran unindexed no matter what the flag said. + // + // No `name`: each driver derives its own (SQL's `buildIndexName` is + // table-qualified, which a hardcoded name could not be without colliding + // across tables on Postgres; Mongo's index names are per-collection). + // `unique` is left at its default `false` — a plain lookup index, never a + // constraint. + // + // `multiTenant: false` declares NO index rather than a false one: on an + // unwalled stack nothing filters by organization, so the index is dead + // weight — the same intent the old flag's value carried, expressed as + // presence instead of a boolean. + if (opts.multiTenant && !declaresTenantIndex(schema)) { + indexAdditions.push({ fields: ['organization_id'] }); + } } if (wantAudit) { @@ -496,16 +533,51 @@ export function applySystemFields( additions[OWNING_BUSINESS_UNIT_FIELD] = { ...OWNING_BUSINESS_UNIT_FIELD_DEF }; } - if (Object.keys(additions).length === 0 && Object.keys(overrides).length === 0) return schema; + if ( + Object.keys(additions).length === 0 && + Object.keys(overrides).length === 0 && + indexAdditions.length === 0 + ) { + return schema; + } return { ...schema, // `additions` LOSE to an author's field (a declared `owner_id` is theirs); // `overrides` WIN over it (the audit family's governance is not authorable). fields: { ...additions, ...(schema.fields ?? {}), ...overrides }, + // [#6810] Author-declared indexes keep their position; the platform's + // tenant index is APPENDED, never merged into or reordering theirs. + ...(indexAdditions.length > 0 + ? { indexes: [...((schema as any).indexes ?? []), ...indexAdditions] } + : {}), }; } +/** + * [#6810] Is the tenant index already declared on this object? + * + * The append is the one part of this injection that is not naturally + * idempotent — the field injection re-runs harmlessly because + * `!schema.fields?.organization_id` stops it, an array push does not — so an + * author who hand-wrote `indexes: [{ fields: ['organization_id'] }]` must not + * end up with the platform's duplicate beside it. + * + * Matched on the single-column shape this function emits, deliberately: an + * author's composite (`['organization_id', 'code']`) is a leading-column match + * on some dialects and not on others, so it is not treated as a substitute. + */ +function declaresTenantIndex(schema: ServiceObject): boolean { + const declared = (schema as any).indexes; + if (!Array.isArray(declared)) return false; + return declared.some( + (idx: any) => + Array.isArray(idx?.fields) && + idx.fields.length === 1 && + idx.fields[0] === 'organization_id', + ); +} + /** * Generic-write `apiMethods` verbs mapped to the {@link resolveCrudAffordances} * flag each one needs. Read verbs (`get`/`list`/`search`/`history`/…) are