Skip to content

Commit bf6fdb7

Browse files
dmealingclaude
andcommitted
fix(codegen-csharp,gates): an unconfigured FK is NoAction, not EF's cascade (#294, #337)
Review findings on the two preceding commits. The serious one: establishing a relationship without an explicit DeleteBehavior does NOT reproduce "no action". EF applies its own convention, and for a REQUIRED foreign key that convention is Cascade — so an FK with no resolved action (no correlated relationship, or an explicit @onDelete: "no-action") would have made the generated context cascade-delete rows the database refuses to orphan. Before the feature EF had no relationship at all and did nothing; establishing one and leaving it unconfigured was a behaviour change in the destructive direction, which is the opposite of the point. Verified against a real EF model — it reported Cascade where the DDL says nothing — and the integration test now pins NoAction. The unit test that asserted "no clause" was pinning the defect and has been rewritten to say why the clause is required. The M:N junction sides deliberately keep EF's existing convention unless the metadata declares an action: they have been configured that way since FR-018, and pinning them to NoAction would stop EF clearing junction rows on a tracked delete — a behaviour change for every M:N adopter, unrelated to this issue. Also from review: - the junction skip is per FK COLUMN, not per entity, so a junction carrying a third reference of its own still gets it configured; - that map is keyed by node identity rather than bare name, so two same-named entities in different packages cannot cross-suppress (ADR-0041/0042); - the gate scans .txt as well as .md — docs/llms/ ships llms.txt and llms-full.txt, the files #343 landed in, which the gate built for that incident could not see. It immediately found a real one: the headline "defining metadata" example declared createdAt as a field.string carrying @autoset, which is registered on field.timestamp; - the gate checks YAML, not just JSON. ADR-0006 makes YAML the authoring front-end and the authoring skill teaches in it, so a sigil-free YAML block with a retired attribute is the #337 shape exactly. Blocks are re-emitted as YAML so the desugar still sees bare keys; a self-test case fails if the path is ever skipped again; - ERR_BAD_ATTR_FILTER was in both classification sets, making the outcome depend on check order. Simplifications: the raw @onDelete/@onUpdate accessors move onto MetaRelationship (closing a divergence from the TS SSOT, which reads them off the node), M:N navigations are derived once instead of twice per entity, and three sites use the memoized MetaObject.FindField. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DhpswkF1NvwxhFWMmdAT15
1 parent 1ebd82c commit bf6fdb7

10 files changed

Lines changed: 251 additions & 93 deletions

File tree

CHANGELOG.md

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -121,13 +121,30 @@ The precedence is a **port of `migrate-ts`'s `referential-actions.ts`**, the cro
121121
tier for tier: reference-level `@onDelete` → a correlated sibling relationship → the
122122
parent-side reverse relationship, package-aware (ADR-0042), with `@through` excluded, the
123123
more-than-one-reference-to-the-same-target ambiguity guard, and the inferred-`set-null`-on-
124-
NOT-NULL satisfiability guard. Three behaviours are decided rather than inherited: a TPH
125-
base and subtype declaring the **same** FK configure it **once**, on the base that owns the
126-
shared column (a duplicate is the ambiguity the adopter was working around); an M:N
127-
junction's FK sides ride on the `UsingEntity` call that creates them, so the per-reference
128-
pass skips junction entities rather than configuring one FK twice; and a resolved
129-
`set-null` over a `@required` FK **warns and emits no clause**, because EF fails MODEL
130-
VALIDATION there and would take down the entire `DbContext` rather than one relationship.
124+
NOT-NULL satisfiability guard. Four behaviours are decided rather than inherited:
125+
126+
- **An FK with no resolved action is `DeleteBehavior.NoAction`, stated explicitly.** This is
127+
the one that is easy to get backwards, and a pre-merge review caught it: `no-action` IS
128+
the database default and the DDL writes no `ON DELETE` clause for it, so emitting nothing
129+
looks right — but EF does not read an absent `OnDelete` as "no action". It applies its own
130+
convention, and for a **required** FK that convention is **`Cascade`**. Leaving the call
131+
off would have made the generated context delete rows the database would have refused to
132+
orphan: a destructive disagreement with the schema, introduced by the change meant to end
133+
the disagreement. Confirmed against a real EF model, which reported `Cascade` where the
134+
DDL says nothing.
135+
- **A TPH base and subtype declaring the same FK configure it once**, on the base that owns
136+
the shared column — a duplicate is the ambiguity the adopter was working around.
137+
- **An M:N junction's two join columns ride on the `UsingEntity` call** that creates them,
138+
so the per-reference pass skips *those columns* rather than the whole entity (a junction
139+
carrying a third reference of its own still gets it configured). Those two keep EF's
140+
existing convention unless the metadata declares an action: they have been configured that
141+
way since FR-018, and pinning them to `NoAction` now would stop EF clearing junction rows
142+
on a tracked delete — a behaviour change for every M:N adopter, and nothing to do with
143+
this issue. The 1:N path defaults precisely because those relationships are **new** here.
144+
- **A resolved `set-null` over a `@required` FK warns and falls back to `NoAction`**, because
145+
EF fails MODEL VALIDATION there and would take down the entire `DbContext` rather than one
146+
relationship.
147+
131148
`@onUpdate` is deliberately not surfaced — `DeleteBehavior` covers deletes only, so it
132149
remains a DDL-level fact.
133150

@@ -294,8 +311,16 @@ worked example, the exact spelling that release turned into a load error; **#343
294311
`@verifiedBy` and the pre-`0.24.0` `@status` enum a full release after both went. Each was
295312
fixed by hand, in a different file — which is why the family recurred instead of converging.
296313

297-
`scripts/check-doc-examples.ts` now loads every fenced JSON example under `docs/` and the
298-
`agent-context` skills against the **strict** registry, in the `gates` lane.
314+
`scripts/check-doc-examples.ts` now loads every fenced JSON **and YAML** example under
315+
`docs/` and the `agent-context` skills against the **strict** registry, in the `gates` lane.
316+
YAML is not optional coverage: ADR-0006 makes it the universal authoring front-end and the
317+
authoring skill teaches in it, so a sigil-free YAML block carrying a retired attribute is
318+
the #337 shape exactly. `.txt` is scanned alongside `.md` because `docs/llms/` ships
319+
`llms.txt` / `llms-full.txt` — the very files #343 landed in, which an `.md`-only sweep
320+
would have left invisible to the gate built for them. It found one on its first full run:
321+
`llms-full.txt`'s headline "defining metadata" example declared `createdAt` as a
322+
`field.string` carrying `@autoSet`, an attribute registered on `field.timestamp` — a broken
323+
model in the file whose entire audience is agents copying it.
299324

300325
**The hard part was never extraction, it was telling a real drift from an illustration**,
301326
because most doc blocks are deliberately partial. The rule is the KIND of error, not a

docs/llms/llms-full.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ metadata.root:
229229
children:
230230
- field.long: { name: id }
231231
- field.string: { name: email }
232-
- field.string: { name: createdAt, autoSet: onCreate }
232+
- field.timestamp: { name: createdAt, autoSet: onCreate }
233233
- identity.primary: { fields: [id] }
234234
```
235235

scripts/check-doc-examples.ts

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import { isAbsolute, join, relative } from "node:path";
4646
// node_modules and is not reachable from `scripts/`. The source import also means the
4747
// gate reads the loader as it is NOW, with no build step between edit and gate.
4848
import { MetaDataLoader } from "../server/typescript/packages/metadata/src/index.js";
49+
import type { MetaDataFormat } from "../server/typescript/packages/metadata/src/loader/meta-data-source.js";
4950

5051
const REPO_ROOT = new URL("..", import.meta.url).pathname.replace(/\/$/, "");
5152

@@ -116,7 +117,6 @@ const FAIL_CODES = new Set([
116117
"ERR_RELATIVE_REF_IN_CANONICAL",
117118
"ERR_TOO_MANY_OCCURRENCES",
118119
"ERR_UNKNOWN_EXPR_NODE",
119-
"ERR_BAD_ATTR_FILTER",
120120
"ERR_YAML_COERCION",
121121
"ERR_MISSING_SUBTYPE",
122122
]);
@@ -195,12 +195,24 @@ interface Block {
195195
readonly body: string;
196196
}
197197

198-
function markdownFiles(dir: string, out: string[] = []): string[] {
198+
/** A block rendered back into something the loader will accept, in its own format. */
199+
interface LoadableModel {
200+
readonly content: string;
201+
readonly format: MetaDataFormat;
202+
}
203+
204+
/**
205+
* Documents that carry shipped examples. `.txt` is included deliberately: `docs/llms/`
206+
* ships `llms.txt` / `llms-full.txt`, and #343 — one of the three incidents this gate
207+
* exists for — landed in exactly those files. Scanning only `.md` would have left the
208+
* gate blind to the surface that motivated it.
209+
*/
210+
function documentFiles(dir: string, out: string[] = []): string[] {
199211
for (const entry of readdirSync(dir)) {
200212
if (entry === "node_modules" || entry.startsWith(".")) continue;
201213
const full = join(dir, entry);
202-
if (statSync(full).isDirectory()) markdownFiles(full, out);
203-
else if (entry.endsWith(".md")) out.push(full);
214+
if (statSync(full).isDirectory()) documentFiles(full, out);
215+
else if (entry.endsWith(".md") || entry.endsWith(".txt")) out.push(full);
204216
}
205217
return out;
206218
}
@@ -292,20 +304,30 @@ const HOST_DEPENDENT_TYPES = new Set(["origin"]);
292304
* synthetic root so the loader sees a well-formed document. That wrapping is what makes
293305
* fragment-ness mechanical: nothing about the block has to be annotated.
294306
*/
295-
function asLoadableModel(block: Block): string | undefined {
296-
const text = block.lang === "yaml" || block.lang === "yml" ? undefined : stripJsonComments(block.body);
297-
if (text === undefined) return undefined; // YAML handled separately (see gate docs)
307+
function asLoadableModel(block: Block): LoadableModel | undefined {
308+
const isYaml = block.lang === "yaml" || block.lang === "yml";
309+
// YAML authoring is sigil-free (ADR-0006) and the loader desugars it, so a YAML block
310+
// is parsed, wrapped like any other fragment, and handed BACK to the loader as YAML —
311+
// re-emitting keeps the bare attribute keys the desugar expects, where canonical JSON
312+
// would demand `@` sigils this gate must not invent. ADR-0006 makes YAML the universal
313+
// authoring front-end, so a YAML example teaching a retired attribute is exactly the
314+
// #337 shape and must not sail through unchecked.
315+
const format: MetaDataFormat = isYaml ? "yaml" : "json";
316+
const emit = (value: unknown): LoadableModel => ({
317+
content: isYaml ? Bun.YAML.stringify(value) : JSON.stringify(value),
318+
format,
319+
});
298320

299321
let parsed: unknown;
300322
try {
301-
parsed = JSON.parse(text);
323+
parsed = isYaml ? Bun.YAML.parse(block.body) : JSON.parse(stripJsonComments(block.body));
302324
} catch {
303325
return undefined; // an elision or a prose snippet — makes no metadata claim
304326
}
305327
if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) return undefined;
306328

307329
const keys = Object.keys(parsed as Record<string, unknown>);
308-
if (keys.includes("metadata.root")) return JSON.stringify(parsed);
330+
if (keys.includes("metadata.root")) return emit(parsed);
309331
if (keys.length !== 1 || !NODE_KEY.test(keys[0]!)) return undefined;
310332

311333
const key = keys[0]!;
@@ -327,7 +349,7 @@ function asLoadableModel(block: Block): string | undefined {
327349
.map((n) => ({ "field.string": { name: n } }));
328350
node = { "object.entity": { name: "DocExample", children: [...fields, parsed] } };
329351
}
330-
return JSON.stringify({ "metadata.root": { package: "docexample", children: [node] } });
352+
return emit({ "metadata.root": { package: "docexample", children: [node] } });
331353
}
332354

333355
interface Finding {
@@ -346,7 +368,7 @@ async function main(): Promise<void> {
346368
for (const root of roots) {
347369
const dir = isAbsolute(root) ? root : join(REPO_ROOT, root);
348370
try { statSync(dir); } catch { continue; }
349-
for (const file of markdownFiles(dir)) {
371+
for (const file of documentFiles(dir)) {
350372
const path = relative(REPO_ROOT, file);
351373
if (SKIP_PREFIXES.some((p) => path.startsWith(p))) continue;
352374
blocks.push(...fencedBlocks(file));
@@ -364,15 +386,15 @@ async function main(): Promise<void> {
364386

365387
let errors: readonly Error[];
366388
try {
367-
({ errors } = await MetaDataLoader.fromString(model, "json", { strict: true }));
389+
({ errors } = await MetaDataLoader.fromString(model.content, model.format, { strict: true }));
368390
} catch (e) {
369391
errors = [e as Error];
370392
}
371393

372394
for (const error of errors) {
373395
const code = (error as { code?: string }).code ?? "ERR_UNCODED";
374-
const finding: Finding = { block, code, message: error.message };
375396
if (FRAGMENT_CODES.has(code)) continue;
397+
const finding: Finding = { block, code, message: error.message };
376398
if (FAIL_CODES.has(code)) { findings.push(finding); continue; }
377399
if (!unclassified.has(code)) unclassified.set(code, finding);
378400
}

scripts/test-doc-examples.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,22 @@ const CASES: readonly Case[] = [
6363
"```",
6464
].join("\n"),
6565
},
66+
{
67+
// ADR-0006 makes YAML the universal authoring front-end, and the authoring skill
68+
// teaches in it — so a YAML example carrying retired vocabulary is the #337 shape
69+
// exactly. This case fails if the YAML path is ever skipped again.
70+
name: "retired attribute in a sigil-free YAML block",
71+
shouldFail: true,
72+
because: "ERR_UNKNOWN_ATTR",
73+
markdown: [
74+
"```yaml",
75+
"requirement.functional:",
76+
" name: recallMeasurement",
77+
" level: 4",
78+
" verifiedBy: [MeasurementTest]",
79+
"```",
80+
].join("\n"),
81+
},
6682
{
6783
// The everyday case: a fragment that omits everything around it must stay silent,
6884
// or the gate is noise and gets disabled.

server/csharp/MetaObjects.Codegen.Tests/Issue294ReferentialActionTests.cs

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -115,12 +115,40 @@ public void Tier3_the_parent_side_composition_supplies_its_subtype_default()
115115
}
116116

117117
[Fact]
118-
public void No_action_emits_no_clause_because_it_is_the_database_default()
118+
public void No_action_is_stated_explicitly_rather_than_left_to_EF_s_convention()
119119
{
120+
// Tempting to emit nothing here — `no-action` IS the database default, and the
121+
// TS-owned DDL writes no ON DELETE clause for it. But EF does not treat an absent
122+
// OnDelete as "no action": it applies its own convention, which for a REQUIRED
123+
// foreign key is Cascade. Omitting the call would therefore make the generated
124+
// context delete rows the database would refuse to orphan.
120125
var (source, _) = Generate(TwoEntityModel(referenceAttrs: """, "@onDelete": "no-action" """));
121-
var line = WeekFkLine(source);
122-
Assert.Contains("HasForeignKey(nameof(Week.ProgramId));", line);
123-
Assert.DoesNotContain(".OnDelete(", line);
126+
Assert.Contains(".OnDelete(DeleteBehavior.NoAction)", WeekFkLine(source));
127+
}
128+
129+
[Fact]
130+
public void An_uncorrelated_reference_is_NoAction_too()
131+
{
132+
// No @onDelete, and no relationship on either side to correlate with: the resolved
133+
// action is "none", which must still be stated for the reason above.
134+
var (source, _) = Generate("""
135+
{ "metadata.root": { "package": "acme", "children": [
136+
{ "object.entity": { "name": "Program", "children": [
137+
{ "source.rdb": { "@table": "programs" } },
138+
{ "field.long": { "name": "id" } },
139+
{ "identity.primary": { "@fields": "id" } }
140+
]}},
141+
{ "object.entity": { "name": "Week", "children": [
142+
{ "source.rdb": { "@table": "weeks" } },
143+
{ "field.long": { "name": "id" } },
144+
{ "field.long": { "name": "programId" } },
145+
{ "identity.primary": { "@fields": "id" } },
146+
{ "identity.reference": { "name": "refProgram", "@fields": "programId",
147+
"@references": "Program" } }
148+
]}}
149+
]}}
150+
""");
151+
Assert.Contains(".OnDelete(DeleteBehavior.NoAction)", WeekFkLine(source));
124152
}
125153

126154
[Fact]
@@ -132,7 +160,10 @@ public void Set_null_over_a_required_fk_warns_instead_of_breaking_model_validati
132160
referenceAttrs: """, "@onDelete": "set-null" """,
133161
programIdAttrs: ""","@required": true"""));
134162

135-
Assert.DoesNotContain(".OnDelete(", WeekFkLine(source));
163+
// NoAction rather than nothing: an omitted call would leave EF's convention in
164+
// charge, and for this REQUIRED foreign key that convention is Cascade — turning
165+
// an unsatisfiable set-null into destructive cascade deletes.
166+
Assert.Contains(".OnDelete(DeleteBehavior.NoAction)", WeekFkLine(source));
136167
Assert.Contains(warnings, w => w.Contains("SET NULL") && w.Contains("programId"));
137168
}
138169

0 commit comments

Comments
 (0)