Skip to content

Commit 992f01c

Browse files
dmealingclaude
dmealing
and
claude
committed
feat(migrate-ts): flag set-null referential action on a NOT NULL FK column
ON DELETE SET NULL requires the FK column(s) to be nullable. With aggregation relationships now defaulting to set-null, flag the conflict at expected-schema build time so the unsafe DDL never reaches emit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e793402 commit 992f01c

5 files changed

Lines changed: 337 additions & 3 deletions

File tree

‎server/typescript/packages/migrate-ts/src/errors.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,38 @@
11
import type { Change, ChangeKind } from "./types.js";
22

3+
// ---------------------------------------------------------------------------
4+
// SetNullNotNullableError — surfaced by buildExpectedSchema
5+
// ---------------------------------------------------------------------------
6+
7+
/**
8+
* Thrown when a foreign key's resolved ON DELETE action is "set-null" but one
9+
* or more of its FK columns map to a NOT NULL field (@required: true).
10+
*
11+
* ON DELETE SET NULL requires the FK column(s) to be nullable — Postgres and
12+
* SQLite both reject the combination at DDL execution time.
13+
*
14+
* Fix: either remove @required from the FK field(s), or override the
15+
* relationship with @onDelete: "restrict" / "no-action" to avoid set-null.
16+
*/
17+
export class SetNullNotNullableError extends Error {
18+
override readonly name = "SetNullNotNullableError";
19+
readonly entityName: string;
20+
readonly constraintName: string;
21+
readonly offendingFields: string[];
22+
23+
constructor(entityName: string, constraintName: string, offendingFields: string[]) {
24+
const fieldList = offendingFields.join(", ");
25+
super(
26+
`Entity "${entityName}": FK constraint "${constraintName}" uses ON DELETE SET NULL ` +
27+
`but field(s) [${fieldList}] are NOT NULL (@required: true). ` +
28+
`Fix: remove @required from the FK field(s), or override with @onDelete: "restrict" / "no-action" on the relationship.`,
29+
);
30+
this.entityName = entityName;
31+
this.constraintName = constraintName;
32+
this.offendingFields = offendingFields;
33+
}
34+
}
35+
336
const ENABLE_FLAG_BY_KIND: Partial<Record<ChangeKind, string>> = {
437
"drop-column": "allow.dropColumn",
538
"drop-table": "allow.dropTable",

‎server/typescript/packages/migrate-ts/src/expected-schema.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ import type { SqlType } from "./sql-type.js";
3434
import type {
3535
SchemaSnapshot, TableDescriptor, ColumnDescriptor, IndexDescriptor, FkDescriptor,
3636
} from "./types.js";
37-
import { resolveReferentialActions } from "./referential-actions.js";
37+
import { resolveReferentialActions, validateSetNullNullability } from "./referential-actions.js";
3838

3939
export interface BuildExpectedSchemaOptions {
4040
/**
@@ -244,8 +244,13 @@ function buildForeignKeys(
244244
: [toSnake(refChild.resolvedTargetPkField(root) ?? "id")];
245245

246246
const { onDelete, onUpdate } = resolveReferentialActions(entity, refChild);
247+
const constraintName = `${tableName}_${fkCols[0]}_fk`;
248+
249+
// Guard: ON DELETE SET NULL requires nullable FK columns.
250+
validateSetNullNullability(entity, refChild, onDelete, constraintName);
251+
247252
const fk: FkDescriptor = {
248-
name: `${tableName}_${fkCols[0]}_fk`,
253+
name: constraintName,
249254
columns: fkCols,
250255
refTable,
251256
refColumns,

‎server/typescript/packages/migrate-ts/src/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ export { emit } from "./emit/index.js";
1515
export { writeMigration } from "./write-migration.js";
1616

1717
// Errors
18-
export { BlockedChangesError } from "./errors.js";
18+
export { BlockedChangesError, SetNullNotNullableError } from "./errors.js";
1919

2020
// SqlType helpers (rarely needed but useful for advanced consumers)
2121
export { isWidening, sqlTypeEquals } from "./sql-type.js";

‎server/typescript/packages/migrate-ts/src/referential-actions.ts‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,16 @@
11
import {
22
ON_DELETE_DEFAULT_BY_SUBTYPE,
33
ON_UPDATE_DEFAULT,
4+
FIELD_ATTR_REQUIRED,
5+
IDENTITY_ATTR_FIELDS,
6+
TYPE_VALIDATOR,
7+
VALIDATOR_SUBTYPE_REQUIRED,
48
type MetaObject,
59
type MetaReferenceIdentity,
10+
type MetaData,
611
} from "@metaobjectsdev/metadata";
712
import type { FkAction } from "./types.js";
13+
import { SetNullNotNullableError } from "./errors.js";
814

915
/**
1016
* Resolve the referential actions for a foreign key inferred from an
@@ -49,3 +55,67 @@ function normalize(a: string | undefined): FkAction | undefined {
4955
if (a === undefined || a === "no-action") return undefined;
5056
return a as FkAction;
5157
}
58+
59+
// ---------------------------------------------------------------------------
60+
// Set-null / NOT NULL guard
61+
// ---------------------------------------------------------------------------
62+
63+
/**
64+
* Validate that a FK whose resolved ON DELETE action is "set-null" does not
65+
* contain any NOT NULL column.
66+
*
67+
* ON DELETE SET NULL requires all FK columns to be nullable. Postgres and
68+
* SQLite both reject the combination at DDL execution time.
69+
*
70+
* Call this from buildExpectedSchema AFTER resolving the referential action
71+
* (i.e. after resolveReferentialActions) so that explicit overrides such as
72+
* @onDelete: "restrict" are already applied before the check.
73+
*
74+
* @param entity The owning entity.
75+
* @param ref The identity.reference node being processed.
76+
* @param onDelete The resolved onDelete action (undefined = no-action).
77+
* @param constraintName The FK constraint name as it will appear in the DDL.
78+
*/
79+
export function validateSetNullNullability(
80+
entity: MetaObject,
81+
ref: MetaReferenceIdentity,
82+
onDelete: FkAction | undefined,
83+
constraintName: string,
84+
): void {
85+
if (onDelete !== "set-null") return;
86+
87+
const fkFieldJsNames = readRefFields(ref);
88+
const offending: string[] = [];
89+
for (const jsName of fkFieldJsNames) {
90+
const field = findField(entity, jsName);
91+
if (field !== undefined && isRequired(field)) {
92+
offending.push(jsName);
93+
}
94+
}
95+
96+
if (offending.length > 0) {
97+
throw new SetNullNotNullableError(entity.name, constraintName, offending);
98+
}
99+
}
100+
101+
function readRefFields(ref: MetaReferenceIdentity): string[] {
102+
const raw = ref.ownAttr(IDENTITY_ATTR_FIELDS);
103+
if (Array.isArray(raw)) return raw.map(String).filter((s) => s.length > 0);
104+
if (typeof raw === "string") return raw.split(",").map((s) => s.trim()).filter((s) => s.length > 0);
105+
return [];
106+
}
107+
108+
function findField(entity: MetaObject, name: string): MetaData | undefined {
109+
for (const field of entity.fields()) {
110+
if (field.name === name) return field;
111+
}
112+
return undefined;
113+
}
114+
115+
function isRequired(field: MetaData): boolean {
116+
const attr = field.ownAttr(FIELD_ATTR_REQUIRED);
117+
if (attr === true || attr === "true") return true;
118+
return field.ownChildren().some(
119+
(c) => c.type === TYPE_VALIDATOR && c.subType === VALIDATOR_SUBTYPE_REQUIRED,
120+
);
121+
}

‎server/typescript/packages/migrate-ts/test/unit/referential-actions-nullable.test.ts‎

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
/**
2+
* Guard: ON DELETE SET NULL requires nullable FK columns.
3+
*
4+
* Emitting SET NULL on a NOT NULL column is a Postgres/SQLite error at
5+
* runtime. buildExpectedSchema() must surface this conflict early — before
6+
* diff or emit — via SetNullNotNullableError.
7+
*
8+
* Three cases:
9+
* 1. aggregation (default set-null) + @required FK field → throws
10+
* 2. aggregation (default set-null) + nullable FK field → no throw
11+
* 3. aggregation + explicit @onDelete: restrict + @required FK field → no throw
12+
* (override avoids set-null entirely)
13+
*/
14+
import { describe, test, expect } from "bun:test";
15+
import { MetaDataLoader, InMemorySource } from "@metaobjectsdev/metadata";
16+
import { buildExpectedSchema } from "../../src/expected-schema.js";
17+
import { SetNullNotNullableError } from "../../src/errors.js";
18+
19+
async function loadDoc(doc: unknown) {
20+
const { root, errors } = await new MetaDataLoader().load([
21+
new InMemorySource(JSON.stringify(doc)),
22+
]);
23+
expect(errors).toHaveLength(0);
24+
return root;
25+
}
26+
27+
// ---------------------------------------------------------------------------
28+
// Helpers to build the test document
29+
// ---------------------------------------------------------------------------
30+
31+
function makeDoc(options: {
32+
required: boolean;
33+
relationship: Record<string, unknown>;
34+
}) {
35+
const fieldChildren: unknown[] = options.required
36+
? [{ "validator.required": {} }]
37+
: [];
38+
39+
return {
40+
"metadata.root": {
41+
package: "acme",
42+
children: [
43+
{
44+
"object.entity": {
45+
name: "Program",
46+
children: [
47+
{ "field.long": { name: "id" } },
48+
{ "identity.primary": { "@fields": "id" } },
49+
],
50+
},
51+
},
52+
{
53+
"object.entity": {
54+
name: "Week",
55+
children: [
56+
{ "field.long": { name: "id" } },
57+
{
58+
"field.long": {
59+
name: "programId",
60+
children: fieldChildren,
61+
},
62+
},
63+
options.relationship,
64+
{
65+
"identity.reference": {
66+
name: "ref_program",
67+
"@fields": ["programId"],
68+
"@references": "Program",
69+
},
70+
},
71+
{ "identity.primary": { "@fields": "id" } },
72+
],
73+
},
74+
},
75+
],
76+
},
77+
};
78+
}
79+
80+
// ---------------------------------------------------------------------------
81+
// Tests
82+
// ---------------------------------------------------------------------------
83+
84+
describe("buildExpectedSchema — set-null requires nullable FK columns", () => {
85+
test("aggregation default (set-null) + @required FK field → SetNullNotNullableError", async () => {
86+
const root = await loadDoc(
87+
makeDoc({
88+
required: true,
89+
relationship: {
90+
"relationship.aggregation": {
91+
name: "program",
92+
"@objectRef": "Program",
93+
"@cardinality": "one",
94+
},
95+
},
96+
}),
97+
);
98+
99+
expect(() => buildExpectedSchema(root)).toThrow(SetNullNotNullableError);
100+
});
101+
102+
test("SetNullNotNullableError message names entity, FK constraint, and offending field", async () => {
103+
const root = await loadDoc(
104+
makeDoc({
105+
required: true,
106+
relationship: {
107+
"relationship.aggregation": {
108+
name: "program",
109+
"@objectRef": "Program",
110+
"@cardinality": "one",
111+
},
112+
},
113+
}),
114+
);
115+
116+
let caught: unknown;
117+
try {
118+
buildExpectedSchema(root);
119+
} catch (e) {
120+
caught = e;
121+
}
122+
123+
expect(caught).toBeInstanceOf(SetNullNotNullableError);
124+
const err = caught as SetNullNotNullableError;
125+
// Must name the entity
126+
expect(err.message).toContain("Week");
127+
// Must name the FK constraint
128+
expect(err.message).toContain("weeks_program_id_fk");
129+
// Must name the offending field
130+
expect(err.message).toContain("programId");
131+
// Must suggest a fix
132+
expect(err.message.toLowerCase()).toMatch(/@required|@ondelete|restrict|no-action/i);
133+
});
134+
135+
test("aggregation default (set-null) + nullable FK field → no error", async () => {
136+
const root = await loadDoc(
137+
makeDoc({
138+
required: false,
139+
relationship: {
140+
"relationship.aggregation": {
141+
name: "program",
142+
"@objectRef": "Program",
143+
"@cardinality": "one",
144+
},
145+
},
146+
}),
147+
);
148+
149+
// Must not throw
150+
expect(() => buildExpectedSchema(root)).not.toThrow();
151+
});
152+
153+
test("aggregation + explicit @onDelete: restrict + @required FK field → no error (override avoids set-null)", async () => {
154+
const root = await loadDoc(
155+
makeDoc({
156+
required: true,
157+
relationship: {
158+
"relationship.aggregation": {
159+
name: "program",
160+
"@objectRef": "Program",
161+
"@cardinality": "one",
162+
"@onDelete": "restrict",
163+
},
164+
},
165+
}),
166+
);
167+
168+
// The explicit override changes onDelete to restrict → no set-null → no error
169+
expect(() => buildExpectedSchema(root)).not.toThrow();
170+
});
171+
172+
test("aggregation + explicit @onDelete: no-action + @required FK field → no error (override avoids set-null)", async () => {
173+
const root = await loadDoc(
174+
makeDoc({
175+
required: true,
176+
relationship: {
177+
"relationship.aggregation": {
178+
name: "program",
179+
"@objectRef": "Program",
180+
"@cardinality": "one",
181+
"@onDelete": "no-action",
182+
},
183+
},
184+
}),
185+
);
186+
187+
// no-action normalizes to undefined in the FK descriptor → no set-null → no error
188+
expect(() => buildExpectedSchema(root)).not.toThrow();
189+
});
190+
191+
test("composition (cascade) + @required FK field → no error (cascade is fine on NOT NULL)", async () => {
192+
const root = await loadDoc(
193+
makeDoc({
194+
required: true,
195+
relationship: {
196+
"relationship.composition": {
197+
name: "program",
198+
"@objectRef": "Program",
199+
"@cardinality": "one",
200+
},
201+
},
202+
}),
203+
);
204+
205+
expect(() => buildExpectedSchema(root)).not.toThrow();
206+
});
207+
208+
test("explicit @onDelete: set-null on composition + @required FK field → SetNullNotNullableError", async () => {
209+
// Explicit set-null on any relationship subtype should also be caught
210+
const root = await loadDoc(
211+
makeDoc({
212+
required: true,
213+
relationship: {
214+
"relationship.composition": {
215+
name: "program",
216+
"@objectRef": "Program",
217+
"@cardinality": "one",
218+
"@onDelete": "set-null",
219+
},
220+
},
221+
}),
222+
);
223+
224+
expect(() => buildExpectedSchema(root)).toThrow(SetNullNotNullableError);
225+
});
226+
});

0 commit comments

Comments
 (0)