Skip to content

Commit 39eb01b

Browse files
authored
fix(runtime,cli,types,driver-sql): one schema view for serve/dev and migrate — __search companions + declared unique indexes (#3955) (#3981)
Closes #3955. Two defects, both reproduced against HotCRM's real metadata shape. 1. `os migrate` flagged live `__search` companion columns as destructive orphans. `createStandaloneStack` never derived the ADR-0098 locale-gated pinyin decision from the compiled artifact's i18n, so the migrate boot's SchemaRegistry provisioned no companion columns and drift reported every live one as `drop_column` — with `--allow-destructive` as the printed remediation. Fixed by a single shared resolve-and-stamp helper (`stampSearchPinyinEnabled`) used by BOTH config-seeing boot paths. 2. A currently-declared unique index was mistaken for pre-#3696 legacy debt, producing a plan that never converged: apply dropped the declared index, the next plan recreated it, the one after called it legacy again — an unbounded drop/create cycle on a live unique index, every round labelled "safe". `legacyUniqueReplacements` now filters declared index names out of the legacy candidate set via the same `normalizeDeclaredIndex` the create path uses. Regression coverage at four layers; all new tests fail without their fix. Follow-ups filed: #3991 (authoring-time lint for the contradictory field-level + declared unique combination).
1 parent 9774b78 commit 39eb01b

12 files changed

Lines changed: 498 additions & 32 deletions
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
fix(driver-sql): a currently-declared unique index is never legacy debt — index drift no longer ping-pongs (#3955)
6+
7+
An object may declare both a tenant-scoped field-level `unique: true` and an
8+
object-level single-column unique index on the same column:
9+
10+
```ts
11+
email: Field.email({ unique: true }),
12+
indexes: [{ fields: ['email'], unique: true }],
13+
```
14+
15+
The declared index materializes under `buildIndexName` as
16+
`uniq_<table>_<column>` — which is also one of the two spellings
17+
`legacyUniqueIndexNames` looks for when hunting pre-#3696 platform-wide
18+
uniques. The detector therefore read an index the current metadata declares
19+
as legacy debt and proposed replacing it with the tenant composite (which
20+
the same sync had already created).
21+
22+
The resulting plan never converged: `apply` dropped the declared index, the
23+
next `plan` reported it missing and recreated it, and the one after that
24+
called it legacy again — an unbounded drop/create cycle on a live unique
25+
index, every round rendered as a "safe" change.
26+
27+
`legacyUniqueReplacements` now takes the object's `declaredIndexes` and
28+
filters their normalized names out of the legacy candidate set, so an index
29+
metadata declares today is never mistaken for debt. Genuinely legacy indexes
30+
are still retired, including the knex-spelled `<table>_<column>_unique` when
31+
only the `uniq_…` spelling is declared.
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
"@objectstack/types": patch
3+
"@objectstack/runtime": patch
4+
"@objectstack/cli": patch
5+
---
6+
7+
fix(runtime,cli,types): `os migrate` and the dev runtime now share one `__search` companion schema view (#3955)
8+
9+
On a zh-locale deployment the dev runtime provisions the hidden `__search`
10+
pinyin companion column (ADR-0098) on every eligible object, but the
11+
`os migrate plan`/`apply` boot went through `createStandaloneStack`, which
12+
never derived the locale-gated pinyin decision from the compiled artifact.
13+
Its metadata therefore lacked every companion column, and `migrate plan`
14+
reported each live `__search` column of a dev-created database as a
15+
destructive orphan — with `--allow-destructive` as the printed remediation,
16+
which would have dropped live feature columns.
17+
18+
- `@objectstack/types`: new `collectConfiguredLocales(i18n)` and
19+
`stampSearchPinyinEnabled(i18n)` — the single resolve-and-stamp helper for
20+
`OS_SEARCH_PINYIN_ENABLED`. An explicit env value still wins; only a
21+
positive locale-derived decision is stamped.
22+
- `@objectstack/runtime`: `createStandaloneStack` stamps the decision from
23+
the artifact's `i18n` before any plugin constructs a `SchemaRegistry`, and
24+
surfaces `i18n` on its result like `requires`/`objects`/`manifest`.
25+
- `@objectstack/cli`: the `serve`/`dev` boot now stamps through the same
26+
shared helper (behaviour unchanged), so create/serve and plan/apply cannot
27+
compute different schema views of the same source tree.
28+
29+
A fresh CLI-created database is now also born with the same `__search`
30+
columns the dev runtime would provision, instead of acquiring them on the
31+
next dev boot.

docs/adr/0098-pinyin-search-companion-column.md

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,19 @@ time**.
3131

3232
1. **Locale-gated platform switch, no field metadata.**
3333
`OS_SEARCH_PINYIN_ENABLED` (resolved by `resolveSearchPinyinEnabled()` in
34-
`@objectstack/types`) gates the feature end-to-end. When unset, the CLI
35-
boot path derives the default from the stack's configured locales (any
36-
`zh-*` → on) and stamps the decision back into the env var so every
37-
consumer — the per-engine `SchemaRegistry` and the plugin gate — reads the
38-
same single decision. No field-level `pinyin` marker exists, so there is
39-
no declared-but-unenforced dead metadata (ADR-0049) and no half-state
40-
where a field "pretends" to support pinyin.
34+
`@objectstack/types`) gates the feature end-to-end. When unset, every boot
35+
path that sees the stack config derives the default from its configured
36+
locales (any `zh-*` → on) and stamps the decision back into the env var
37+
(shared `stampSearchPinyinEnabled()` helper) so every consumer — the
38+
per-engine `SchemaRegistry` and the plugin gate — reads the same single
39+
decision. There are exactly two such paths: the CLI `serve`/`dev` boot
40+
(from `objectstack.config.ts`) and `createStandaloneStack` (from the
41+
compiled artifact's `i18n``os migrate plan`/`apply`, embedders). A path
42+
that resolved without stamping would compute a schema view without the
43+
companion columns; that is how `os migrate` once flagged live `__search`
44+
columns as destructive orphans (#3955). No field-level `pinyin` marker
45+
exists, so there is no declared-but-unenforced dead metadata (ADR-0049)
46+
and no half-state where a field "pretends" to support pinyin.
4147

4248
2. **Materialization set ≠ search set: one column per object.** Only the
4349
ADR-0079 display/name field (`resolveDisplayField`) feeds the hidden

packages/cli/src/commands/serve.ts

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { bundleRequire } from 'bundle-require';
99
import { loadConfig, BUNDLE_REQUIRE_EXTERNALS } from '../utils/config.js';
1010
import { isHostConfig, shouldBootWithLibrary } from '../utils/plugin-detection.js';
1111
import { resolveDriverType, resolveStorageDefinition, UnsupportedDriverError } from '../utils/storage-driver.js';
12-
import { readEnvWithDeprecation, resolveMultiOrgEnabled, resolveTenancyPosture, resolveAllowDegradedTenancy, isMcpServerEnabled, resolveSearchPinyinEnabled, isModuleNotFoundError } from '@objectstack/types';
12+
import { readEnvWithDeprecation, resolveMultiOrgEnabled, resolveTenancyPosture, resolveAllowDegradedTenancy, isMcpServerEnabled, stampSearchPinyinEnabled, isModuleNotFoundError } from '@objectstack/types';
1313
import { PLATFORM_CAPABILITY_TOKENS } from '@objectstack/spec/kernel';
1414
import { missingProviderMessage } from '../utils/capability-preflight.js';
1515
import { resolveObjectStackHome } from '@objectstack/runtime';
@@ -726,22 +726,16 @@ export default class Serve extends Command {
726726
}
727727
// Pinyin search recall (#2486): locale-gated platform capability. When
728728
// `OS_SEARCH_PINYIN_ENABLED` is unset, the default derives from the
729-
// stack's configured locales (any `zh-*` → on). This is the ONE place
730-
// that sees the stack config, so the resolved decision is stamped back
731-
// into the env var — every later consumer (each engine's SchemaRegistry
732-
// provisioning the `__search` companion column, the plugin's own gate)
733-
// reads the same answer via the no-arg `resolveSearchPinyinEnabled()`.
734-
{
735-
const i18nCfg = (config as any).i18n ?? {};
736-
const configuredLocales = [
737-
i18nCfg.defaultLocale,
738-
i18nCfg.fallbackLocale,
739-
...(Array.isArray(i18nCfg.supportedLocales) ? i18nCfg.supportedLocales : []),
740-
].filter((l: unknown): l is string => typeof l === 'string');
741-
if (resolveSearchPinyinEnabled({ locales: configuredLocales })) {
742-
process.env.OS_SEARCH_PINYIN_ENABLED = 'true';
743-
if (!requires.includes('pinyin-search')) requires.push('pinyin-search');
744-
}
729+
// stack's configured locales (any `zh-*` → on), and the resolved
730+
// decision is stamped back into the env var — every later consumer
731+
// (each engine's SchemaRegistry provisioning the `__search` companion
732+
// column, the plugin's own gate) reads the same answer via the no-arg
733+
// `resolveSearchPinyinEnabled()`. The shared `stampSearchPinyinEnabled`
734+
// helper is also what `createStandaloneStack` stamps from the compiled
735+
// artifact, so serve/dev and `os migrate plan`/`apply` cannot compute
736+
// different schema views of the same source tree (#3955).
737+
if (stampSearchPinyinEnabled((config as any).i18n)) {
738+
if (!requires.includes('pinyin-search')) requires.push('pinyin-search');
745739
}
746740
// Default capability slate — every preset except `minimal` gets the
747741
// foundational services (queue + job + cache + settings + email +

packages/cli/src/utils/schema-migrate.integration.test.ts

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,3 +117,103 @@ describe('bootSchemaStack + migrate engine (integration)', () => {
117117
}
118118
}, 30_000);
119119
});
120+
121+
/**
122+
* #3955 — `os migrate` and the dev runtime must compute ONE schema view.
123+
*
124+
* A zh-locale deployment's dev runtime provisions the hidden `__search`
125+
* pinyin companion column (ADR-0098) on every eligible object. The migrate
126+
* boot goes through `createStandaloneStack`, which — before the fix — never
127+
* derived the locale-gated pinyin decision from the artifact, so its metadata
128+
* lacked every companion column and `os migrate plan` reported each live
129+
* `__search` column as a destructive orphan, with `--allow-destructive` as
130+
* the printed remediation. Following that advice would have dropped live
131+
* feature columns.
132+
*/
133+
describe('bootSchemaStack — dev-provisioned __search companions are not orphans (#3955)', () => {
134+
let dir: string;
135+
let dbFile: string;
136+
const savedEnv: Record<string, string | undefined> = {};
137+
138+
beforeAll(async () => {
139+
dir = mkdtempSync(join(tmpdir(), 'os-mig-pinyin-'));
140+
mkdirSync(join(dir, 'dist'), { recursive: true });
141+
mkdirSync(join(dir, 'data'), { recursive: true });
142+
dbFile = join(dir, 'data', 'app.db');
143+
144+
// Compiled-artifact stand-in for a Chinese deployment: the i18n block is
145+
// exactly what `os build` compiles out of `objectstack.config.ts`.
146+
writeFileSync(
147+
join(dir, 'dist', 'objectstack.json'),
148+
JSON.stringify({
149+
id: 'mig_pinyin_smoke',
150+
name: 'Migrate Pinyin Smoke',
151+
i18n: { defaultLocale: 'en', supportedLocales: ['en', 'zh-CN'], fallbackLocale: 'en' },
152+
objects: [
153+
{
154+
name: 'mig_person',
155+
fields: {
156+
name: { type: 'text', required: true },
157+
email: { type: 'text' },
158+
},
159+
},
160+
],
161+
}),
162+
);
163+
164+
// Seed the database the way a dev-runtime first boot leaves it: the
165+
// object's columns PLUS the provisioned `__search` companion.
166+
const seed = new SqlDriver({ client: 'better-sqlite3', connection: { filename: dbFile }, useNullAsDefault: true });
167+
const k = (seed as any).knex;
168+
await k.schema.createTable('mig_person', (t: any) => {
169+
t.string('id').primary();
170+
t.timestamp('created_at');
171+
t.timestamp('updated_at');
172+
t.string('name').notNullable();
173+
t.string('email');
174+
t.string('__search');
175+
});
176+
await k('mig_person').insert({ id: '1', name: '张伟', email: 'zw@example.com', __search: 'zhangwei zw' });
177+
await k.destroy();
178+
179+
savedEnv.OS_ARTIFACT_PATH = process.env.OS_ARTIFACT_PATH;
180+
savedEnv.NODE_ENV = process.env.NODE_ENV;
181+
savedEnv.OS_SEARCH_PINYIN_ENABLED = process.env.OS_SEARCH_PINYIN_ENABLED;
182+
process.env.OS_ARTIFACT_PATH = join(dir, 'dist', 'objectstack.json');
183+
process.env.NODE_ENV = 'production';
184+
// The defect precondition: nothing external decided pinyin — the boot must
185+
// derive it from the artifact's locales, exactly like serve/dev does.
186+
delete process.env.OS_SEARCH_PINYIN_ENABLED;
187+
});
188+
189+
afterAll(() => {
190+
process.env.OS_ARTIFACT_PATH = savedEnv.OS_ARTIFACT_PATH;
191+
process.env.NODE_ENV = savedEnv.NODE_ENV;
192+
if (savedEnv.OS_SEARCH_PINYIN_ENABLED === undefined) delete process.env.OS_SEARCH_PINYIN_ENABLED;
193+
else process.env.OS_SEARCH_PINYIN_ENABLED = savedEnv.OS_SEARCH_PINYIN_ENABLED;
194+
try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ }
195+
});
196+
197+
it('the migrate boot provisions the companion in metadata, so plan reports no __search drift', async () => {
198+
const stack = await bootSchemaStack({ databaseUrl: `file:${dbFile}` });
199+
try {
200+
expect(stack.driver).toBeTruthy();
201+
202+
// The fix, observed at the seam: the artifact's zh-CN locale was stamped
203+
// into the pinyin decision, so the migrate boot's OWN metadata carries
204+
// the companion column — the same schema view the dev runtime serves.
205+
const managed = (stack.driver as any).managedObjectFields.get('mig_person');
206+
expect(managed, 'mig_person must be metadata-managed').toBeDefined();
207+
expect(Object.keys(managed)).toContain('__search');
208+
209+
// And the defect, gone: no drift on __search — before the fix this was a
210+
// destructive drop_column "orphaned" finding pointing at --allow-destructive.
211+
const drift = await stack.driver!.detectManagedDrift();
212+
const searchDrift = drift.filter((d) => d.column === '__search');
213+
expect(searchDrift).toEqual([]);
214+
expect(drift.filter((d) => d.category === 'destructive')).toEqual([]);
215+
} finally {
216+
await stack.shutdown();
217+
}
218+
}, 30_000);
219+
});

packages/plugins/driver-sql/src/schema-drift.ts

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -513,28 +513,50 @@ export interface LegacyUniqueReplacement {
513513
* names to look for and the composite that replaces them. `unique: 'global'`
514514
* fields are excluded — their single-column index is the declared intent now,
515515
* not legacy debt.
516+
*
517+
* `declaredIndexes` are excluded the same way, and for the same reason (#3955).
518+
* An object may declare a single-column unique index alongside a tenant-scoped
519+
* field-level `unique: true` — `email: { unique: true }` plus
520+
* `indexes: [{ fields: ['email'], unique: true }]`. That declared index
521+
* materializes under {@link buildIndexName}, which is *also* one of the two
522+
* spellings {@link legacyUniqueIndexNames} looks for, so without this filter the
523+
* detector reads an index metadata declares TODAY as pre-#3696 debt and proposes
524+
* dropping it. The plan then never converges: apply drops the declared index,
525+
* the next plan reports it missing and recreates it, and the one after that
526+
* calls it legacy again — an unbounded drop/create cycle on a live unique index.
527+
* An index the current metadata declares is by definition not legacy.
516528
*/
517529
export function legacyUniqueReplacements(args: {
518530
table: string;
519531
fields: Record<string, any>;
520532
tenantField: string | null;
521533
physicalColumns: Set<string>;
534+
declaredIndexes?: Array<{ name?: string; fields?: string[]; unique?: boolean | 'global' }>;
522535
}): LegacyUniqueReplacement[] {
523-
const { table, fields, tenantField, physicalColumns } = args;
536+
const { table, fields, tenantField, physicalColumns, declaredIndexes } = args;
524537
if (!tenantField) return []; // Nothing was ever mis-scoped on a tenant-less table.
525538
// Without a physical tenant column there is no composite to replace the
526539
// legacy index with, and dropping it unreplaced would remove the constraint
527540
// outright rather than relax it. Leave it alone.
528541
if (!physicalColumns.has(tenantField)) return [];
542+
// Normalized through the same helper the create path uses, so "what the
543+
// declared index is named" is answered once, not guessed at twice.
544+
const declaredNames = new Set(
545+
(Array.isArray(declaredIndexes) ? declaredIndexes : [])
546+
.map((idx) => normalizeDeclaredIndex(table, idx)?.name)
547+
.filter((n): n is string => typeof n === 'string'),
548+
);
529549
const out: LegacyUniqueReplacement[] = [];
530550
for (const [name, field] of Object.entries<any>(fields ?? {})) {
531551
if (!isUniqueDeclared(field?.unique)) continue;
532552
if (isGlobalUnique(field.unique)) continue;
533553
if (name === tenantField || !physicalColumns.has(name)) continue;
554+
const legacyNames = legacyUniqueIndexNames(table, name).filter((n) => !declaredNames.has(n));
555+
if (legacyNames.length === 0) continue;
534556
const columns = [tenantField, name];
535557
out.push({
536558
column: name,
537-
legacyNames: legacyUniqueIndexNames(table, name),
559+
legacyNames,
538560
replacement: { name: buildIndexName(table, columns, true), columns, unique: true },
539561
});
540562
}

packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,98 @@ describe('SqlDriver index drift (#3728)', () => {
153153
});
154154
});
155155

156+
// ── #3955: a DECLARED single-column unique is not legacy debt ─────────────
157+
//
158+
// An object may declare both a tenant-scoped field-level `unique: true` and
159+
// an object-level single-column unique index on the same column (HotCRM's
160+
// `crm_contact` does exactly this: `email: { unique: true }` plus
161+
// `indexes: [{ fields: ['email'], unique: true }]`). The declared index
162+
// materializes under `buildIndexName` — `uniq_<table>_<col>` — which is also
163+
// one of the two spellings the legacy detector looks for. It was therefore
164+
// reported as pre-#3696 debt to be dropped, and the plan never converged.
165+
describe('a currently-declared single-column unique index is never called legacy (#3955)', () => {
166+
const contactMeta = [
167+
{
168+
name: 'hp_contact',
169+
fields: {
170+
organization_id: { type: 'string' },
171+
email: { type: 'string', unique: true },
172+
last_name: { type: 'string' },
173+
},
174+
indexes: [
175+
{ fields: ['email'], unique: true },
176+
{ fields: ['last_name'] },
177+
],
178+
},
179+
];
180+
181+
it('builds both indexes and then reports NO drift on a fresh table', async () => {
182+
const driver = makeDriver();
183+
await driver.initObjects(contactMeta);
184+
185+
// Both are current intent: the tenant composite from the field-level
186+
// `unique: true`, and the verbatim declared global unique.
187+
const uniques = await uniqueIndexColumns('hp_contact');
188+
expect(uniques['uniq_hp_contact_organization_id_email']).toEqual(['organization_id', 'email']);
189+
expect(uniques['uniq_hp_contact_email']).toEqual(['email']);
190+
191+
// Before the fix this reported `replace_unique_index` — proposing to drop
192+
// the index the very same sync had just created from metadata.
193+
expect(await driver.detectManagedDrift()).toHaveLength(0);
194+
});
195+
196+
it('does not ping-pong: applying the plan converges instead of recreating work', async () => {
197+
const driver = makeDriver();
198+
await driver.initObjects(contactMeta);
199+
200+
// Round 1: nothing to do. Round 2 (the old loop's second half) likewise —
201+
// the declared index is still there, so nothing reports it missing.
202+
for (let round = 0; round < 3; round++) {
203+
const drift = await driver.detectManagedDrift();
204+
expect(drift, `round ${round + 1} should be clean`).toHaveLength(0);
205+
await driver.applyMigrationEntries(drift, { allowDestructive: false });
206+
}
207+
208+
const uniques = await uniqueIndexColumns('hp_contact');
209+
expect(Object.keys(uniques).sort()).toEqual([
210+
'uniq_hp_contact_email',
211+
'uniq_hp_contact_organization_id_email',
212+
]);
213+
});
214+
215+
it('still retires a genuinely legacy index when metadata declares no such index', async () => {
216+
// The #3728 behaviour must survive the filter: the same `uniq_<table>_<col>`
217+
// spelling IS legacy when nothing in metadata declares it.
218+
const driver = makeDriver();
219+
await seedLegacyGlobalUnique('uniq_product_code');
220+
await driver.initObjects(productMeta); // no declared `indexes[]`
221+
222+
const drift = await driver.detectManagedDrift();
223+
const entry = drift.find((d) => d.op.type === 'replace_unique_index');
224+
expect(entry, 'a legacy index nobody declares must still be retired').toBeDefined();
225+
expect((entry!.op as any).dropIndexNames).toEqual(['uniq_product_code']);
226+
});
227+
228+
it('retires the knex-spelled legacy index even when the buildIndexName spelling is declared', async () => {
229+
// Only the declared spelling is exempt. A pre-#3696 `<table>_<col>_unique`
230+
// left over from the old createColumn path is still debt.
231+
const driver = makeDriver();
232+
await knexInstance.schema.createTable('hp_contact', (t: any) => {
233+
t.string('id').primary();
234+
t.string('organization_id');
235+
t.string('email');
236+
t.string('last_name');
237+
});
238+
await knexInstance.raw('CREATE UNIQUE INDEX hp_contact_email_unique ON hp_contact (email)');
239+
await driver.initObjects(contactMeta);
240+
241+
const drift = await driver.detectManagedDrift();
242+
const entry = drift.find((d) => d.op.type === 'replace_unique_index');
243+
expect(entry).toBeDefined();
244+
expect((entry!.op as any).dropIndexNames).toEqual(['hp_contact_email_unique']);
245+
});
246+
});
247+
156248
// ── Applying it through `os migrate apply` ────────────────────────────────
157249

158250
describe('applyMigrationEntries', () => {

0 commit comments

Comments
 (0)