Skip to content

Commit 17c26b3

Browse files
dmealingclaude
dmealing
and
claude
committed
fix(#209): derive INNER vs LEFT OUTER view join from FK optionality (TS)
renderJoin hardcoded LEFT OUTER JOIN for every join in a synthesized projection / entity-read-view. A join over a REQUIRED (NOT NULL) belongs-to reference is semantically INNER: lowering it to LEFT OUTER silently returns extra rows (a base row with a dangling FK appears with NULL joined columns) and produces a different result set than the INNER-join view it stands in for — invisible to verify --db, surfacing only as wrong data downstream. The join type is now DERIVED from the FK's optionality, using the SAME NOT-NULL predicate (isRequired) that drives a column's .notNull(): - a required belongs-to FK (referenceHolder=source) → INNER JOIN - a nullable belongs-to FK → LEFT OUTER JOIN (unchanged) - ANY has-many hop (referenceHolder=target — a base row may have zero children, e.g. an aggregate that must COALESCE to 0) → LEFT OUTER JOIN (unchanged) A required belongs-to FK can neither drop nor NULL-fill a base row, so INNER and LEFT OUTER return the same set — but INNER is the honest semantic and matches a hand-written INNER-join view (keeping verify --db fingerprints aligned). JoinNode gains a `joinType`; extract-view-spec computes it per hop; view-ddl-emit renders it. Scoped out (follow-up): the optional `@join: inner|left` override attr for the rare mis-derivation — it needs a registered provider + registry-conformance fixture across all 5 ports, so it's a separate coordinated change; the derivation alone is the correctness fix and is safe. TS-migrate-only. Verified: new join-type-derivation test (required→INNER, nullable→LEFT OUTER, has-many→LEFT OUTER); codegen-ts 955 pass; typecheck exit 0; migrate-ts 340 pass; canonical schema.postgres.sql drift gate byte-identical (its only view join is has-many → correctly unchanged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGQ7oSuNcjhsMHWwZzhBwr
1 parent ce98902 commit 17c26b3

5 files changed

Lines changed: 129 additions & 4 deletions

File tree

‎server/typescript/packages/codegen-ts/src/projection/extract-view-spec.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ import {
4949
columnNameFromField,
5050
viewNameFromProjection,
5151
} from "../naming.js";
52+
// #209 — the SAME NOT-NULL predicate that drives a column's `.notNull()` decides
53+
// whether a belongs-to join is INNER (required FK) vs LEFT OUTER (nullable FK).
54+
import { isRequired } from "../column-mapper.js";
5255
import type { ColumnNamingStrategy } from "../metaobjects-config.js";
5356
import type {
5457
JoinNode, JoinTree, SelectColumn, SelectSpec, ViewSpec, ViewFilterClause,
@@ -428,6 +431,8 @@ interface PathStep {
428431
fkColumn: string;
429432
pkColumn: string;
430433
referenceHolder: "source" | "target";
434+
/** #209 — derived join type: `inner` for a required belongs-to FK, else `left`. */
435+
joinType: "inner" | "left";
431436
targetEntity: string;
432437
}
433438

@@ -513,13 +518,26 @@ function buildJoinTree(
513518
const fkHolder = referenceHolder === "source" ? currentObj : target;
514519
const pkHolder = referenceHolder === "source" ? target : currentObj;
515520

521+
// #209 — a belongs-to hop (FK on the parent) whose FK is NOT NULL is
522+
// semantically INNER: every base row has a match, so INNER and LEFT OUTER
523+
// return the same set, and INNER matches the hand-written view it stands in
524+
// for (and keeps `verify --db` fingerprints aligned). A nullable belongs-to
525+
// FK, or ANY has-many hop (FK on the child — a base row may have zero
526+
// children), stays LEFT OUTER so no base row is dropped.
527+
const fkFieldObj = (fkHolder as MetaObject).findField(fkField);
528+
const joinType: "inner" | "left" =
529+
referenceHolder === "source" && fkFieldObj !== undefined && isRequired(fkFieldObj)
530+
? "inner"
531+
: "left";
532+
516533
path.push({
517534
entity: currentObj,
518535
relationship: relName,
519536
cardinality,
520537
fkColumn: joinColumnFor(fkHolder, fkField, ctx),
521538
pkColumn: joinColumnFor(pkHolder, resolvedPkField, ctx),
522539
referenceHolder,
540+
joinType,
523541
targetEntity: stripPackage(targetName),
524542
});
525543
currentObj = target;
@@ -552,6 +570,7 @@ function buildJoinTree(
552570
fkColumn: step.fkColumn,
553571
pkColumn: step.pkColumn,
554572
referenceHolder: step.referenceHolder,
573+
joinType: step.joinType,
555574
children: Array.from(node.children.values()).map(toJoinNode),
556575
};
557576
}

‎server/typescript/packages/codegen-ts/src/projection/view-ddl-emit.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,10 @@ function renderJoin(
227227
const onClause = node.referenceHolder === "source"
228228
? `${childAlias}.${pkCol} = ${parentAlias}.${fkCol}`
229229
: `${childAlias}.${fkCol} = ${parentAlias}.${pkCol}`;
230-
let sql = ` LEFT OUTER JOIN ${quoteIfNeeded(table)} ${childAlias} ON ${onClause}`;
230+
// #209 — join type derived from FK optionality (extract-view-spec): a required
231+
// belongs-to FK → INNER (matches the hand-written INNER-join view); else LEFT OUTER.
232+
const joinKw = node.joinType === "inner" ? "INNER JOIN" : "LEFT OUTER JOIN";
233+
let sql = ` ${joinKw} ${quoteIfNeeded(table)} ${childAlias} ON ${onClause}`;
231234
for (const childJoin of node.children) {
232235
sql += "\n" + renderJoin(childJoin, childAlias, options);
233236
}

‎server/typescript/packages/codegen-ts/src/projection/view-spec.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ export interface JoinNode {
1616
readonly pkColumn: string;
1717
/** Which side of this hop physically holds the FK: the parent (source) or the child (target). */
1818
readonly referenceHolder: "source" | "target";
19+
/** #209 — `inner` when this is a belongs-to hop whose FK is NOT NULL (required):
20+
* the join can neither drop nor NULL-fill a base row, so it is semantically INNER
21+
* and matches the hand-written INNER-join view it stands in for. `left` otherwise —
22+
* a nullable belongs-to FK, or ANY has-many (inverse-FK) hop, where a base row with
23+
* no match must survive (aggregates COALESCE to 0, not drop the row). */
24+
readonly joinType: "inner" | "left";
1925
/** Child joins. */
2026
readonly children: readonly JoinNode[];
2127
}

‎server/typescript/packages/codegen-ts/test/projection/join-type-derivation.test.ts‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
// Issue #209 — a synthesized view's joins were ALL hardcoded to LEFT OUTER JOIN.
2+
// A join over a REQUIRED (NOT NULL) belongs-to reference is semantically INNER;
3+
// lowering it to LEFT OUTER silently returns extra rows (a base row with a dangling
4+
// FK appears with NULL joined columns) and produces a different result set than the
5+
// INNER-join view it stands in for. The join type is now DERIVED from the FK's
6+
// optionality: a required belongs-to FK → INNER; a nullable one → LEFT OUTER; a
7+
// has-many (inverse-FK) join stays LEFT OUTER (a base row with zero children must
8+
// survive).
9+
10+
import { describe, test, expect } from "bun:test";
11+
import { MetaDataLoader, InMemoryStringSource } from "@metaobjectsdev/metadata";
12+
import { buildProjectionViews } from "../../src/projection/build-projection-views.js";
13+
14+
async function load(children: unknown[]) {
15+
const json = JSON.stringify({ "metadata.root": { package: "acme", children } });
16+
const { root, errors } = await new MetaDataLoader().load([new InMemoryStringSource(json)]);
17+
expect(errors).toEqual([]);
18+
return root;
19+
}
20+
21+
// Order --belongs-to--> Customer (FK `customerId` on Order). A projection passes
22+
// through Customer.name via the single-hop `customer` relationship, forcing a join
23+
// from orders to customers.
24+
const belongsToModel = (customerIdRequired: boolean) => [
25+
{ "object.entity": { name: "Customer", children: [
26+
{ "source.rdb": { "@table": "customers" } },
27+
{ "field.long": { name: "id" } },
28+
{ "field.string": { name: "name" } },
29+
{ "identity.primary": { name: "pk", "@fields": "id" } },
30+
] } },
31+
{ "object.entity": { name: "Order", children: [
32+
{ "source.rdb": { "@table": "orders" } },
33+
{ "field.long": { name: "id" } },
34+
{ "field.long": { name: "customerId", ...(customerIdRequired ? { "@required": true } : {}) } },
35+
{ "identity.primary": { name: "pk", "@fields": "id" } },
36+
{ "identity.reference": { name: "ref_customer", "@fields": "customerId", "@references": "Customer" } },
37+
{ "relationship.association": { name: "customer", "@objectRef": "Customer", "@cardinality": "one" } },
38+
] } },
39+
{ "object.projection": { name: "OrderView", children: [
40+
{ "source.rdb": { "@kind": "view", "@table": "v_order" } },
41+
{ "field.long": { name: "id", extends: "Order.id" } },
42+
{ "field.string": { name: "customerName", children: [{ "origin.passthrough": { "@from": "Customer.name", "@via": "Order.customer" } }] } },
43+
{ "identity.primary": { name: "pk", extends: "Order.pk" } },
44+
] } },
45+
];
46+
47+
describe("#209 — belongs-to join type derived from FK optionality", () => {
48+
test("required FK → INNER JOIN (not LEFT OUTER)", async () => {
49+
const root = await load(belongsToModel(true));
50+
const [v] = buildProjectionViews(root, { dialect: "postgres", columnNamingStrategy: "snake_case" });
51+
expect(v!.sql).toContain("INNER JOIN customers");
52+
expect(v!.sql).not.toContain("LEFT OUTER JOIN customers");
53+
});
54+
55+
test("nullable FK → LEFT OUTER JOIN (preserved)", async () => {
56+
const root = await load(belongsToModel(false));
57+
const [v] = buildProjectionViews(root, { dialect: "postgres", columnNamingStrategy: "snake_case" });
58+
expect(v!.sql).toContain("LEFT OUTER JOIN customers");
59+
expect(v!.sql).not.toContain("INNER JOIN customers");
60+
});
61+
});
62+
63+
describe("#209 — has-many (inverse-FK) join stays LEFT OUTER", () => {
64+
// Program 1→many Week (FK on Week). Even though Week.programId is required, the
65+
// Program→weeks join must stay LEFT OUTER: a program with zero weeks must survive
66+
// (the aggregate COALESCEs to 0, not drops the row).
67+
test("required inverse FK still emits LEFT OUTER (base row with no children survives)", async () => {
68+
const root = await load([
69+
{ "object.entity": { name: "Program", children: [
70+
{ "source.rdb": { "@table": "programs" } },
71+
{ "field.int": { name: "id" } },
72+
{ "identity.primary": { name: "pk", "@fields": "id" } },
73+
{ "relationship.association": { name: "weeks", "@objectRef": "Week", "@cardinality": "many" } },
74+
] } },
75+
{ "object.entity": { name: "Week", children: [
76+
{ "source.rdb": { "@table": "weeks" } },
77+
{ "field.int": { name: "id" } },
78+
{ "field.int": { name: "programId", "@required": true } },
79+
{ "identity.primary": { name: "pk", "@fields": "id" } },
80+
{ "identity.reference": { name: "ref_program", "@fields": "programId", "@references": "Program" } },
81+
] } },
82+
{ "object.projection": { name: "ProgramSummary", children: [
83+
{ "source.rdb": { "@kind": "view", "@table": "v_program_summary" } },
84+
{ "field.int": { name: "id", extends: "Program.id" } },
85+
{ "field.int": { name: "weekCount", children: [{ "origin.aggregate": { "@agg": "count", "@of": "Week.id", "@via": "Program.weeks" } }] } },
86+
{ "identity.primary": { name: "pk", extends: "Program.pk" } },
87+
] } },
88+
]);
89+
const [v] = buildProjectionViews(root, { dialect: "postgres", columnNamingStrategy: "snake_case" });
90+
expect(v!.sql).toContain("LEFT OUTER JOIN weeks");
91+
expect(v!.sql).not.toContain("INNER JOIN weeks");
92+
});
93+
});

‎server/typescript/packages/codegen-ts/test/projection/view-ddl-emit.test.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ const programSummarySpec: ViewSpec = {
1616
fkColumn: "program_id",
1717
pkColumn: "id",
1818
referenceHolder: "target",
19+
joinType: "left",
1920
children: [
2021
{
2122
relationship: "workouts",
@@ -25,6 +26,7 @@ const programSummarySpec: ViewSpec = {
2526
fkColumn: "week_id",
2627
pkColumn: "id",
2728
referenceHolder: "target",
29+
joinType: "left",
2830
children: [],
2931
},
3032
],
@@ -106,6 +108,7 @@ describe("emitViewDdl — non-id parent join (pkColumn)", () => {
106108
fkColumn: "customer_email",
107109
pkColumn: "email", // non-id join
108110
referenceHolder: "target",
111+
joinType: "left",
109112
children: [],
110113
},
111114
],
@@ -145,7 +148,7 @@ describe("emitViewDdl — #195 predicate quantifier (any/all)", () => {
145148
baseAlias: "s",
146149
joins: [
147150
{ relationship: "turns", targetEntity: "Turn", alias: "t", cardinality: "many",
148-
fkColumn: "session_id", pkColumn: "id", referenceHolder: "target", children: [] },
151+
fkColumn: "session_id", pkColumn: "id", referenceHolder: "target", joinType: "left", children: [] },
149152
],
150153
},
151154
selectSpec: {
@@ -192,7 +195,7 @@ describe("emitViewDdl — #195 array rollup (collect)", () => {
192195
baseAlias: "o",
193196
joins: [
194197
{ relationship: "items", targetEntity: "Item", alias: "i", cardinality: "many",
195-
fkColumn: "order_id", pkColumn: "id", referenceHolder: "target", children: [] },
198+
fkColumn: "order_id", pkColumn: "id", referenceHolder: "target", joinType: "left", children: [] },
196199
],
197200
},
198201
selectSpec: {
@@ -319,7 +322,7 @@ describe("emitViewDdl — #195 correlated first (argmax-then-project)", () => {
319322
joinTree: {
320323
baseEntity: "Parent", baseAlias: "p",
321324
joins: [ { relationship: "childAs", targetEntity: "ChildA", alias: "c1", cardinality: "many",
322-
fkColumn: "parent_id", pkColumn: "id", referenceHolder: "target", children: [] } ],
325+
fkColumn: "parent_id", pkColumn: "id", referenceHolder: "target", joinType: "left", children: [] } ],
323326
},
324327
selectSpec: {
325328
columns: [
@@ -359,6 +362,7 @@ describe("emitViewDdl — belongs-to (reference on source)", () => {
359362
fkColumn: "program_id",
360363
pkColumn: "id",
361364
referenceHolder: "source",
365+
joinType: "left",
362366
children: [],
363367
},
364368
],

0 commit comments

Comments
 (0)