Skip to content

Commit 3fe0ff1

Browse files
claudeos-zhuang
authored andcommitted
fix(driver-sql): the plan must not promise columns the apply can never create (#3978)
The mirror image of #3954. That issue closed "the plan understates what apply does"; this closes the other direction — the plan promising work apply CANNOT do. `previewDeferredSchemaWork()` listed every declared field name when computing pending create_table / add_columns work, but `createColumn` returns early for a virtual `formula` field: no column is ever created. So a formula field appeared as pending `add_columns`, `apply` reported it as performed without doing anything, and the very next `plan` reported it again. A freshly-applied database looked permanently un-migrated with no invocation able to clear the finding. On examples/app-crm that was four columns reported forever: crm_contact.full_name, crm_lead.is_closed, crm_opportunity.expected_revenue and crm_opportunity.days_to_close. The preview now filters through `fieldHasColumn` — the same helper `createColumn` and the column differ already answer "does this field materialize a column?" with, and the one place that had not adopted it — so the plan and the flush cannot disagree. `multiple` fields are unaffected: they materialize as a JSON column and are still reported. `PendingSchemaWork.columns` now states the constraint in the type. Regression coverage in sql-driver-deferred-ddl.test.ts: the formula field is omitted from a create_table preview while the JSON column is kept; the same for add_columns; a flush is followed by an empty preview (the convergence property the issue asks for); and what flush REPORTS having done matches the columns physically created. All four fail without the filter. Closes #3978 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FcphmGzBuWwF8SaYPSCtCw
1 parent 5c8afed commit 3fe0ff1

4 files changed

Lines changed: 114 additions & 1 deletion

File tree

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
fix(driver-sql): `os migrate plan` no longer promises columns the apply can never create (#3978)
6+
7+
`previewDeferredSchemaWork()` listed every declared field name when computing
8+
pending `create_table` / `add_columns` work, but `createColumn` returns early
9+
for a virtual `formula` field — no column is ever created for it.
10+
11+
So a formula field showed up as pending `add_columns` that `apply` reported as
12+
performed without doing anything, and the very next `plan` reported it again.
13+
A freshly-applied database looked permanently un-migrated, with no invocation
14+
able to clear the finding. On `examples/app-crm` that was 4 columns
15+
(`crm_contact.full_name`, `crm_lead.is_closed`, `crm_opportunity.expected_revenue`,
16+
`crm_opportunity.days_to_close`) reported forever.
17+
18+
The preview now filters through `fieldHasColumn` — the same helper `createColumn`
19+
and the column differ already answer "does this field materialize a column?"
20+
with — so the plan and the flush cannot disagree. `multiple` fields are
21+
unaffected: they materialize as a JSON column and are still reported.

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,11 @@ export interface PendingSchemaWork {
110110
/**
111111
* Declared columns for a create; the missing ones for an add; the columns
112112
* being converged for the two datetime steps.
113+
*
114+
* The additive kinds name only fields that MATERIALIZE a column
115+
* ({@link fieldHasColumn}) — a virtual `formula` field never appears. The
116+
* promise above cuts both ways: a plan may not promise work `apply` cannot
117+
* deliver either, or the finding can never be cleared (#3978).
113118
*/
114119
columns: string[];
115120
/**

packages/plugins/driver-sql/src/sql-driver-deferred-ddl.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,4 +136,79 @@ describe('SqlDriver deferred schema DDL (#3917)', () => {
136136
expect(driver.deferredSchemaObjectCount).toBe(0);
137137
expect(await driver.previewDeferredSchemaWork()).toEqual([]);
138138
});
139+
140+
// ── #3978: the plan may only promise work the flush can deliver ───────────
141+
//
142+
// The mirror image of #3954. That one closed "the plan understates what apply
143+
// does"; this closes "the plan promises what apply cannot do". `createColumn`
144+
// returns early for a virtual `formula` field — no column is ever created —
145+
// but the preview listed every declared field name regardless, so a formula
146+
// field showed up as pending `add_columns` that `apply` reported as performed
147+
// without doing anything, and the next `plan` reported it again, forever.
148+
// Both sides must answer "does this field become a column?" the same way.
149+
describe('virtual fields are never promised as pending work (#3978)', () => {
150+
const CONTACT = {
151+
name: 'contacts',
152+
fields: {
153+
first_name: { type: 'text' },
154+
last_name: { type: 'text' },
155+
// Virtual — computed on read, no physical column (see createColumn).
156+
full_name: { type: 'formula', expression: 'record.first_name' },
157+
// `multiple` is NOT virtual: it materializes as a JSON column.
158+
tags: { type: 'text', multiple: true },
159+
},
160+
};
161+
162+
it('omits the formula field from a create_table preview, keeps the JSON column', async () => {
163+
driver = makeDriver();
164+
driver.setDeferredDdl(true);
165+
await driver.initObjects([CONTACT]);
166+
167+
expect(await driver.previewDeferredSchemaWork()).toEqual([
168+
{ table: 'contacts', kind: 'create_table', columns: ['first_name', 'last_name', 'tags'] },
169+
]);
170+
});
171+
172+
it('omits it from an add_columns preview too', async () => {
173+
driver = makeDriver();
174+
await driver.initObjects([{ name: 'contacts', fields: { first_name: { type: 'text' } } }]);
175+
176+
driver.setDeferredDdl(true);
177+
await driver.initObjects([CONTACT]);
178+
179+
expect(await driver.previewDeferredSchemaWork()).toEqual([
180+
{ table: 'contacts', kind: 'add_columns', columns: ['last_name', 'tags'] },
181+
]);
182+
});
183+
184+
it('converges: after a flush the next preview is empty', async () => {
185+
// The defect proper. Pre-fix the second preview still reported
186+
// `add_columns: ['full_name']` — a freshly-applied database looking
187+
// permanently un-migrated, with no apply able to clear it.
188+
driver = makeDriver();
189+
driver.setDeferredDdl(true);
190+
await driver.initObjects([CONTACT]);
191+
await driver.flushDeferredSchemaDdl();
192+
193+
driver.setDeferredDdl(true);
194+
await driver.initObjects([CONTACT]);
195+
expect(await driver.previewDeferredSchemaWork()).toEqual([]);
196+
});
197+
198+
it('what flush REPORTS having done matches the columns it actually created', async () => {
199+
driver = makeDriver();
200+
driver.setDeferredDdl(true);
201+
await driver.initObjects([CONTACT]);
202+
203+
const performed = await driver.flushDeferredSchemaDdl();
204+
const create = performed.find((p) => p.kind === 'create_table')!;
205+
const physical = new Set(Object.keys(await (driver as any).knex('contacts').columnInfo()));
206+
207+
// Every promised column exists...
208+
for (const c of create.columns) expect(physical.has(c)).toBe(true);
209+
// ...and the virtual one was neither promised nor created.
210+
expect(create.columns).not.toContain('full_name');
211+
expect(physical.has('full_name')).toBe(false);
212+
});
213+
});
139214
});

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
diffManagedTable,
2121
driftKey,
2222
expectedIndexes,
23+
fieldHasColumn,
2324
isIndexDriftOp,
2425
legacyUniqueReplacements,
2526
uniqueIndexesFromFields,
@@ -2864,11 +2865,22 @@ export class SqlDriver implements IDataDriver {
28642865
* a database that has not been migrated yet. That is the right trade for a
28652866
* command the operator ran to be told the size of the job — and it is paid
28662867
* once, by `plan`/`apply`, never on a normal boot.
2868+
*
2869+
* The obligation runs both ways (#3978). #3954 closed "the plan understates
2870+
* what apply does"; this closes its mirror image — the plan must not promise
2871+
* work apply CANNOT do. Only fields that materialize a column are listed,
2872+
* decided by {@link fieldHasColumn}, the same helper `createColumn` and the
2873+
* column differ use. A virtual `formula` field has no column, so listing it
2874+
* produced an `add_columns` entry `apply` reported as performed without doing
2875+
* anything and the next `plan` reported again: a finding no invocation could
2876+
* ever clear, making a freshly-applied database look un-migrated.
28672877
*/
28682878
async previewDeferredSchemaWork(): Promise<PendingSchemaWork[]> {
28692879
const out: PendingSchemaWork[] = [];
28702880
for (const [tableName, obj] of this.deferredSchemaObjects) {
2871-
const declared = Object.keys(obj.fields ?? {});
2881+
const declared = Object.entries<any>(obj.fields ?? {})
2882+
.filter(([, field]) => fieldHasColumn(field ?? {}))
2883+
.map(([name]) => name);
28722884
if (!(await this.knex.schema.hasTable(tableName))) {
28732885
// A table that does not exist yet is created empty, so nothing to converge.
28742886
out.push({ table: tableName, kind: 'create_table', columns: declared });

0 commit comments

Comments
 (0)