From 923e25206d68258aa6319bcb90ce7bf42782bd3b Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 13 Sep 2026 23:10:51 -0400 Subject: [PATCH 01/12] fix(metadata-ts): M:N derivation must use the DECLARING entity, not the visiting one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deriveM2MFields classified the self-join — and matched the hetero junction reference — against the `source` entity its caller passed. Every caller walks the RESOLVING `obj.relationships()` (codegen-ts's relation-resolver, runtime-ts's n2m-resolver, docs-site's link-graph) and passes the entity it is iterating, so for a relationship inherited via `extends` that is the INHERITING entity rather than the one that declared it. Two failures follow: * an inherited SELF-JOIN compares @objectRef (the base) against the child, reads as hetero, looks for a junction reference to the child, finds none and throws; * an inherited HETERO relationship looks for a junction reference to the child when the junction references the base, and throws too. codegen-ts and docs-site catch that throw and return null, so the navigation was silently dropped from generated output; runtime-ts rethrows it, so an inherited M:N was untraversable at run time as well. Fixed in the derivation rather than at each call site: the answer must not depend on who asked, and a future caller cannot forget it. Same shape as the #368 loader fix (`declaringEntity = rel.parent ?? obj` in validation-passes). The `source` parameter stays as the no-parent fallback, so the exported signature is unchanged. Tests: three metadata regressions (inherited symmetric, inherited directed, inherited hetero) plus an own-declaration control; two buildRelationMap regressions in codegen-ts; two resolveN2mDescriptor regressions in runtime-ts. Each fixture declares the child BEFORE the base and pins the visit order, the shape #368 established. Unlike the #368 loader passes there is no once-per-node `checked` set here, so the defect is not order-gated — it is wrong for every inheriting entity in any order; the pinned order keeps the fixtures honest if iteration ever changes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- .../relation-resolver-inherited-m2m.test.ts | 108 ++++++++++++ .../core/relationship/derive-m2m-fields.ts | 52 ++++-- .../metadata/test/relationship-m2m.test.ts | 159 ++++++++++++++++++ .../runtime-ts/test/n2m-resolver.test.ts | 76 +++++++++ 4 files changed, 382 insertions(+), 13 deletions(-) create mode 100644 server/typescript/packages/codegen-ts/test/relation-resolver-inherited-m2m.test.ts diff --git a/server/typescript/packages/codegen-ts/test/relation-resolver-inherited-m2m.test.ts b/server/typescript/packages/codegen-ts/test/relation-resolver-inherited-m2m.test.ts new file mode 100644 index 000000000..573beb448 --- /dev/null +++ b/server/typescript/packages/codegen-ts/test/relation-resolver-inherited-m2m.test.ts @@ -0,0 +1,108 @@ +// Follow-up to #368 — M:N derivation used the VISITING entity, not the entity +// that DECLARES the relationship. +// +// buildRelationMap walks `root.objects()` and, for each, the RESOLVING +// `obj.relationships()` — so a M:N relationship declared on an abstract base +// is reached again through every entity that extends it, with `obj` being the +// INHERITING entity. buildM2mEntry passed that entity to deriveM2MFields as +// the source, so: +// * an inherited SELF-JOIN compared @objectRef (the base) against the child, +// concluded "hetero", looked for a junction reference to the child, found +// none and threw; +// * an inherited HETERO relationship looked for a junction reference to the +// child when the junction references the base, and threw too. +// buildM2mEntry catches and returns null, so the navigation was silently +// dropped from the generated relations() block — no error, no output. +// +// The child is declared BEFORE the base in both fixtures, so the walk reaches +// it first (the order shape the #368 loader regressions established). + +import { describe, expect, test } from "bun:test"; +import { MetaDataLoader, InMemoryStringSource, type MetaRoot } from "@metaobjectsdev/metadata"; +import { buildRelationMap } from "../src/relation-resolver.js"; + +const SELF_JOIN = { + "metadata.root": { + package: "repro", + children: [ + { "object.entity": { name: "Node", extends: "NodeBase", children: [ + { "field.int": { name: "id" } }, + { "identity.primary": { name: "id", "@fields": ["id"], "@generation": "increment" } }, + ] } }, + { "object.entity": { name: "NodeBase", "@isAbstract": true, children: [ + { "relationship.association": { name: "peers", "@cardinality": "many", "@objectRef": "NodeBase", + "@through": "NodeLink", "@symmetric": true } }, + ] } }, + { "object.entity": { name: "NodeLink", children: [ + { "field.int": { name: "id" } }, + { "field.int": { name: "aId" } }, + { "field.int": { name: "bId" } }, + { "identity.primary": { name: "id", "@fields": ["id"], "@generation": "increment" } }, + { "identity.reference": { name: "a", "@fields": ["aId"], "@references": "NodeBase" } }, + { "identity.reference": { name: "b", "@fields": ["bId"], "@references": "NodeBase" } }, + ] } }, + ], + }, +}; + +const HETERO = { + "metadata.root": { + package: "repro", + children: [ + { "object.entity": { name: "Article", extends: "ArticleBase", children: [ + { "field.int": { name: "id" } }, + { "identity.primary": { name: "id", "@fields": ["id"], "@generation": "increment" } }, + ] } }, + { "object.entity": { name: "ArticleBase", "@isAbstract": true, children: [ + { "relationship.association": { name: "tags", "@cardinality": "many", "@objectRef": "Tag", + "@through": "ArticleTag" } }, + ] } }, + { "object.entity": { name: "Tag", children: [ + { "field.int": { name: "id" } }, + { "identity.primary": { name: "id", "@fields": ["id"], "@generation": "increment" } }, + ] } }, + { "object.entity": { name: "ArticleTag", children: [ + { "field.int": { name: "id" } }, + { "field.int": { name: "articleId" } }, + { "field.int": { name: "tagId" } }, + { "identity.primary": { name: "id", "@fields": ["id"], "@generation": "increment" } }, + { "identity.reference": { name: "articleRef", "@fields": ["articleId"], "@references": "ArticleBase" } }, + { "identity.reference": { name: "tagRef", "@fields": ["tagId"], "@references": "Tag" } }, + ] } }, + ], + }, +}; + +async function load(model: unknown): Promise { + const res = await new MetaDataLoader().load([new InMemoryStringSource(JSON.stringify(model))]); + expect(res.errors).toEqual([]); + return res.root; +} + +describe("buildRelationMap — M:N inherited via extends", () => { + test("inherited self-join emits a navigation on the child (was: silently dropped)", async () => { + const root = await load(SELF_JOIN); + // Pin the premise: the child is walked before the base it inherits from. + expect(root.objects().map((o) => o.name)).toEqual(["Node", "NodeBase", "NodeLink"]); + + const entries = buildRelationMap(root).get("Node") ?? []; + const peers = entries.find((e) => e.name === "peers"); + expect(peers).toBeDefined(); + expect(peers!.cardinality).toBe("many"); + expect(peers!.junctionEntity).toBe("NodeLink"); + expect(peers!.sourceJoinField).toBe("aId"); + expect(peers!.targetJoinField).toBe("bId"); + expect(peers!.symmetric).toBe(true); + }); + + test("inherited hetero M:N emits a navigation on the child (was: silently dropped)", async () => { + const root = await load(HETERO); + const entries = buildRelationMap(root).get("Article") ?? []; + const tags = entries.find((e) => e.name === "tags"); + expect(tags).toBeDefined(); + expect(tags!.targetEntity).toBe("Tag"); + expect(tags!.junctionEntity).toBe("ArticleTag"); + expect(tags!.sourceJoinField).toBe("articleId"); + expect(tags!.targetJoinField).toBe("tagId"); + }); +}); diff --git a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts index 695f8433f..8b0fc3871 100644 --- a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts +++ b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts @@ -17,12 +17,26 @@ // two references are taken in declaration order (sourceField = first, // targetField = second). Resolution unions both at read time. // Ambiguous (source == target, neither @sourceRefField nor @symmetric) → throw. +// +// "source" above always means the entity that DECLARES the relationship — never +// whichever entity's effective view reached it. Every caller walks the RESOLVING +// `obj.relationships()`, so for a relationship inherited via `extends` the entity +// it is iterating is the INHERITING one, and both the self-join classification and +// the hetero reference match would then be made against the wrong entity (an +// inherited self-join reads as hetero and derivation throws; an inherited hetero +// finds no junction reference to the inheriting entity and throws too). The +// declaring entity is resolved HERE, from `rel.parent`, rather than asked of each +// caller — same shape as the #368 loader fix (`declaringEntity = rel.parent ?? obj` +// in validation-passes.ts), and for the same reason: the answer must not depend on +// who asked. The `source` parameter is kept as the fallback for a synthetic +// relationship with no parent (and as a non-breaking signature). import type { MetaObject } from "../object/meta-object.js"; import type { MetaRoot } from "../../shared/meta-root.js"; import type { MetaRelationship } from "./meta-relationship.js"; import type { MetaReferenceIdentity } from "../identity/meta-identity.js"; import { stripPackage } from "../../naming.js"; +import { TYPE_OBJECT } from "../../shared/base-types.js"; /** Thrown when a M:N relationship's junction FK fields cannot be derived. */ export class M2MDerivationError extends Error { @@ -50,7 +64,8 @@ function refFkField(ref: MetaReferenceIdentity): string | undefined { * * @param rel the M:N relationship (carries @objectRef + @through + optional * @sourceRefField / @symmetric) - * @param source the entity declaring `rel` + * @param source fallback declaring entity, used only when `rel` has no parent + * (the declaring entity is normally read from `rel.parent`) * @param root the loaded model root (to find the junction entity) * @throws M2MDerivationError when the junction is missing/malformed or the * self-join is ambiguous. @@ -60,10 +75,21 @@ export function deriveM2MFields( source: MetaObject, root: MetaRoot, ): M2MFields { + // The entity that DECLARES `rel` — see the header note. `rel.parent` is the + // owning entity for both an own declaration and an inherited one (an unmodified + // inherited child is the SAME node object, reused in place by + // MetaData._effectiveChildren; an override is a genuinely different node whose + // parent is the overriding entity, which is also correct). + const relParent = rel.parent; + const declaringEntity: MetaObject = + relParent !== undefined && relParent.type === TYPE_OBJECT + ? (relParent as MetaObject) + : source; + const throughName = rel.through; if (throughName === undefined) { throw new M2MDerivationError( - `relationship "${source.name}.${rel.name}" is missing @through (required for M:N derivation)`, + `relationship "${declaringEntity.name}.${rel.name}" is missing @through (required for M:N derivation)`, ); } // @through may be package-qualified (FQN); findObject is keyed by bare name, @@ -76,37 +102,37 @@ export function deriveM2MFields( : undefined); if (junction === undefined) { throw new M2MDerivationError( - `relationship "${source.name}.${rel.name}" @through "${throughName}" does not resolve to an entity`, + `relationship "${declaringEntity.name}.${rel.name}" @through "${throughName}" does not resolve to an entity`, ); } const targetName = rel.objectRef; if (targetName === undefined) { throw new M2MDerivationError( - `relationship "${source.name}.${rel.name}" is missing @objectRef (the M:N target)`, + `relationship "${declaringEntity.name}.${rel.name}" is missing @objectRef (the M:N target)`, ); } const refs = junction.referenceIdentities(); if (refs.length !== 2) { throw new M2MDerivationError( - `junction "${throughName}" for relationship "${source.name}.${rel.name}" must declare exactly two ` + + `junction "${throughName}" for relationship "${declaringEntity.name}.${rel.name}" must declare exactly two ` + `identity.reference children (found ${refs.length})`, ); } - const isSelfJoin = stripPackage(targetName) === source.name; + const isSelfJoin = stripPackage(targetName) === declaringEntity.name; if (!isSelfJoin) { // Hetero: match each reference by the entity it resolves to. - const sourceRef = refs.find((r) => r.targetEntity !== undefined && stripPackage(r.targetEntity) === source.name); + const sourceRef = refs.find((r) => r.targetEntity !== undefined && stripPackage(r.targetEntity) === declaringEntity.name); const targetRef = refs.find((r) => r.targetEntity !== undefined && stripPackage(r.targetEntity) === stripPackage(targetName)); const sourceField = sourceRef ? refFkField(sourceRef) : undefined; const targetField = targetRef ? refFkField(targetRef) : undefined; if (sourceField === undefined || targetField === undefined) { throw new M2MDerivationError( - `junction "${throughName}" for relationship "${source.name}.${rel.name}" must declare one ` + - `identity.reference to "${source.name}" and one to "${stripPackage(targetName)}"`, + `junction "${throughName}" for relationship "${declaringEntity.name}.${rel.name}" must declare one ` + + `identity.reference to "${declaringEntity.name}" and one to "${stripPackage(targetName)}"`, ); } return { sourceField, targetField }; @@ -119,7 +145,7 @@ export function deriveM2MFields( const b = refFkField(refs[1]!); if (a === undefined || b === undefined) { throw new M2MDerivationError( - `symmetric junction "${throughName}" for "${source.name}.${rel.name}" has a reference with no @fields`, + `symmetric junction "${throughName}" for "${declaringEntity.name}.${rel.name}" has a reference with no @fields`, ); } return { sourceField: a, targetField: b }; @@ -128,7 +154,7 @@ export function deriveM2MFields( const sourceRefField = rel.sourceRefField; if (sourceRefField === undefined) { throw new M2MDerivationError( - `self-join relationship "${source.name}.${rel.name}" through "${throughName}" is ambiguous: ` + + `self-join relationship "${declaringEntity.name}.${rel.name}" through "${throughName}" is ambiguous: ` + `set @sourceRefField (directed) or @symmetric (undirected)`, ); } @@ -137,7 +163,7 @@ export function deriveM2MFields( const sourceRef = refs.find((r) => refFkField(r) === sourceRefField); if (sourceRef === undefined) { throw new M2MDerivationError( - `@sourceRefField "${sourceRefField}" on "${source.name}.${rel.name}" does not match any ` + + `@sourceRefField "${sourceRefField}" on "${declaringEntity.name}.${rel.name}" does not match any ` + `identity.reference FK field on junction "${throughName}"`, ); } @@ -145,7 +171,7 @@ export function deriveM2MFields( const targetField = targetRef ? refFkField(targetRef) : undefined; if (targetField === undefined) { throw new M2MDerivationError( - `junction "${throughName}" for "${source.name}.${rel.name}" has no distinct target-side reference`, + `junction "${throughName}" for "${declaringEntity.name}.${rel.name}" has no distinct target-side reference`, ); } return { sourceField: sourceRefField, targetField }; diff --git a/server/typescript/packages/metadata/test/relationship-m2m.test.ts b/server/typescript/packages/metadata/test/relationship-m2m.test.ts index 6d0a45765..9d867e9b2 100644 --- a/server/typescript/packages/metadata/test/relationship-m2m.test.ts +++ b/server/typescript/packages/metadata/test/relationship-m2m.test.ts @@ -695,3 +695,162 @@ describe("FR-017 #368 order-dependence regressions (declaring entity vs. visitin expect(codesOf(errors)).not.toContain("ERR_INVALID_RELATIONSHIP"); }); }); + +// --------------------------------------------------------------------------- +// deriveM2MFields — declaring entity vs. visiting entity (the #368 follow-up). +// +// Same confusion as the two regressions above, one layer down: the derivation +// classified the self-join, and matched the hetero junction reference, against +// the `source` entity its CALLER passed. Every caller walks the RESOLVING +// `obj.relationships()` (codegen-ts's relation-resolver, runtime-ts's +// n2m-resolver, docs-site's link-graph, and their Java/C#/Python twins) and +// passes the entity it is iterating — so for a relationship inherited via +// `extends` that is the INHERITING entity, not the one that declared it. +// +// Unlike the loader-pass regressions above, this defect is NOT gated on visit +// order: there is no once-per-node `checked` set here, so the derivation is +// simply wrong for every inheriting entity, whichever order they are reached +// in. The fixtures still declare the child BEFORE the base and pin the visit +// order, matching the #368 convention and covering the order-sensitive shape +// for free. +// --------------------------------------------------------------------------- + +describe("FR-017 deriveM2MFields uses the DECLARING entity, not the visiting one", () => { + function objectVisitOrder(root: { children(): readonly { type: string; resolutionKey(): string }[] }): string[] { + return root.children().filter((c) => c.type === TYPE_OBJECT).map((o) => o.resolutionKey()); + } + + test("inherited symmetric self-join derives both FK sides (was: read as hetero, threw)", async () => { + // NodeBase declares a @symmetric self-join onto itself; Node extends it and + // is declared FIRST. Reached through Node's effective view, the derivation + // used to compare @objectRef "NodeBase" against the VISITING "Node", + // conclude "not a self-join", take the hetero branch, look for a junction + // reference to "Node" (there is none — both point at NodeBase) and throw + // M2MDerivationError. Codegen swallows that throw, so the navigation just + // vanished from the generated output. + const { root, errors } = await loadDoc({ "metadata.root": { package: "acme", children: [ + { "object.entity": { name: "Node", "extends": "NodeBase", children: [ + { "field.long": { name: "id" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + { "object.entity": { name: "NodeBase", "@isAbstract": true, children: [ + { "relationship.association": { name: "peers", "@cardinality": "many", "@objectRef": "NodeBase", + "@through": "NodeLink", "@symmetric": true } } ] } }, + { "object.entity": { name: "NodeLink", children: [ + { "field.long": { name: "id" } }, + { "field.long": { name: "aId" } }, + { "field.long": { name: "bId" } }, + { "identity.primary": { "name": "id", "@fields": "id" } }, + { "identity.reference": { name: "a", "@fields": ["aId"], "@references": "NodeBase" } }, + { "identity.reference": { name: "b", "@fields": ["bId"], "@references": "NodeBase" } } ] } }, + ] } }); + expect(errors).toHaveLength(0); + expect(objectVisitOrder(root)).toEqual(["acme::Node", "acme::NodeBase", "acme::NodeLink"]); + + const node = findObj(root, "Node"); + // RESOLVING accessor — this is exactly what every caller walks, and it is + // what surfaces the inherited relationship on the child. + const rel = node.relationships().find((r) => r.name === "peers") as MetaRelationship; + expect(node.ownRelationships()).toHaveLength(0); + expect(rel.parent?.name).toBe("NodeBase"); + + const derived = deriveM2MFields(rel, node, root); + expect(derived.sourceField).toBe("aId"); + expect(derived.targetField).toBe("bId"); + // And the declaring entity itself must still agree — same node, same answer. + expect(deriveM2MFields(rel, findObj(root, "NodeBase"), root)).toEqual(derived); + }); + + test("inherited directed self-join honours @sourceRefField through the child's view", async () => { + const { root, errors } = await loadDoc({ "metadata.root": { package: "acme", children: [ + { "object.entity": { name: "Person", "extends": "PartyBase", children: [ + { "field.long": { name: "id" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + { "object.entity": { name: "PartyBase", "@isAbstract": true, children: [ + { "relationship.association": { name: "follows", "@cardinality": "many", "@objectRef": "PartyBase", + "@through": "Follow", "@sourceRefField": "followerId" } } ] } }, + { "object.entity": { name: "Follow", children: [ + { "field.long": { name: "id" } }, + { "field.long": { name: "followerId" } }, + { "field.long": { name: "followeeId" } }, + { "identity.primary": { "name": "id", "@fields": "id" } }, + { "identity.reference": { name: "followerRef", "@fields": ["followerId"], "@references": "PartyBase" } }, + { "identity.reference": { name: "followeeRef", "@fields": ["followeeId"], "@references": "PartyBase" } } ] } }, + ] } }); + expect(errors).toHaveLength(0); + expect(objectVisitOrder(root)).toEqual(["acme::Person", "acme::PartyBase", "acme::Follow"]); + + const person = findObj(root, "Person"); + const rel = person.relationships().find((r) => r.name === "follows") as MetaRelationship; + const derived = deriveM2MFields(rel, person, root); + expect(derived.sourceField).toBe("followerId"); + expect(derived.targetField).toBe("followeeId"); + }); + + test("inherited HETERO M:N matches the junction reference to the declaring base", async () => { + // The mirror image: the junction references the DECLARING base (ArticleBase), + // so a hetero match against the visiting child (Article) found nothing and + // threw the "must declare one identity.reference to ..." error. + const { root, errors } = await loadDoc({ "metadata.root": { package: "acme", children: [ + { "object.entity": { name: "Article", "extends": "ArticleBase", children: [ + { "field.long": { name: "id" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + { "object.entity": { name: "ArticleBase", "@isAbstract": true, children: [ + { "relationship.association": { name: "tags", "@cardinality": "many", "@objectRef": "Tag", + "@through": "ArticleTag" } } ] } }, + { "object.entity": { name: "Tag", children: [ + { "field.long": { name: "id" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + { "object.entity": { name: "ArticleTag", children: [ + { "field.long": { name: "id" } }, + { "field.long": { name: "articleId" } }, + { "field.long": { name: "tagId" } }, + { "identity.primary": { "name": "id", "@fields": "id" } }, + { "identity.reference": { name: "articleRef", "@fields": ["articleId"], "@references": "ArticleBase" } }, + { "identity.reference": { name: "tagRef", "@fields": ["tagId"], "@references": "Tag" } } ] } }, + ] } }); + expect(errors).toHaveLength(0); + expect(objectVisitOrder(root)).toEqual(["acme::Article", "acme::ArticleBase", "acme::Tag", "acme::ArticleTag"]); + + const article = findObj(root, "Article"); + const rel = article.relationships().find((r) => r.name === "tags") as MetaRelationship; + const derived = deriveM2MFields(rel, article, root); + expect(derived.sourceField).toBe("articleId"); + expect(derived.targetField).toBe("tagId"); + }); + + test("an OWN relationship still derives against its own entity (no regression)", async () => { + // The override case: Sub re-declares `peers` itself, so rel.parent IS Sub and + // the self-join must be classified against Sub, not the base it shadows. + const { root, errors } = await loadDoc({ "metadata.root": { package: "acme", children: [ + { "object.entity": { name: "Sub", "extends": "Base", children: [ + { "field.long": { name: "id" } }, + { "relationship.association": { name: "peers", "@cardinality": "many", "@objectRef": "Sub", + "@through": "SubLink", "@symmetric": true } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + { "object.entity": { name: "Base", "@isAbstract": true, children: [ + { "relationship.association": { name: "peers", "@cardinality": "many", "@objectRef": "Base", + "@through": "BaseLink", "@symmetric": true } } ] } }, + { "object.entity": { name: "SubLink", children: [ + { "field.long": { name: "id" } }, + { "field.long": { name: "leftId" } }, + { "field.long": { name: "rightId" } }, + { "identity.primary": { "name": "id", "@fields": "id" } }, + { "identity.reference": { name: "l", "@fields": ["leftId"], "@references": "Sub" } }, + { "identity.reference": { name: "r", "@fields": ["rightId"], "@references": "Sub" } } ] } }, + { "object.entity": { name: "BaseLink", children: [ + { "field.long": { name: "id" } }, + { "field.long": { name: "aId" } }, + { "field.long": { name: "bId" } }, + { "identity.primary": { "name": "id", "@fields": "id" } }, + { "identity.reference": { name: "a", "@fields": ["aId"], "@references": "Base" } }, + { "identity.reference": { name: "b", "@fields": ["bId"], "@references": "Base" } } ] } }, + ] } }); + expect(errors).toHaveLength(0); + const sub = findObj(root, "Sub"); + const rel = sub.relationships().find((r) => r.name === "peers") as MetaRelationship; + expect(rel.parent?.name).toBe("Sub"); + const derived = deriveM2MFields(rel, sub, root); + expect(derived.sourceField).toBe("leftId"); + expect(derived.targetField).toBe("rightId"); + }); +}); diff --git a/server/typescript/packages/runtime-ts/test/n2m-resolver.test.ts b/server/typescript/packages/runtime-ts/test/n2m-resolver.test.ts index 056c6d834..a55c4da16 100644 --- a/server/typescript/packages/runtime-ts/test/n2m-resolver.test.ts +++ b/server/typescript/packages/runtime-ts/test/n2m-resolver.test.ts @@ -243,3 +243,79 @@ describe("symmetric self-join — User.friends via Friendship (union on read)", expect((targetSpec!.where as { values: unknown[] }).values).toEqual([5]); }); }); + +// --------------------------------------------------------------------------- +// Inherited M:N — the runtime resolver reaches a relationship declared on a +// base entity (sourceEntity.children() is RESOLVING) and used to hand the +// derivation the VISITING entity. For a self-join declared on the base that +// read as hetero and threw; the runtime surfaced it as a MetadataError, so an +// inherited self-join was untraversable at run time, not just at codegen time. +// +// The child is declared BEFORE the base so the model's visit order is +// child-first — the shape the #368 loader regressions established. +// --------------------------------------------------------------------------- + +const INHERITED_SELF_JOIN = { + "metadata.root": { + package: "demo", + children: [ + { "object.entity": { name: "Node", extends: "NodeBase", children: [ + { "field.long": { name: "id" } }, + primary("id"), + ] } }, + { "object.entity": { name: "NodeBase", "@isAbstract": true, children: [ + relMany("peers", "NodeBase", "NodeLink", { "@symmetric": true }), + ] } }, + entity("NodeLink", ["aId", "bId"], [ + primary("aId", "bId"), + reference("a", "aId", "NodeBase"), + reference("b", "bId", "NodeBase"), + ]), + ], + }, +}; + +const INHERITED_HETERO = { + "metadata.root": { + package: "demo", + children: [ + { "object.entity": { name: "Article", extends: "ArticleBase", children: [ + { "field.long": { name: "id" } }, + primary("id"), + ] } }, + { "object.entity": { name: "ArticleBase", "@isAbstract": true, children: [ + relMany("tags", "Tag", "ArticleTag"), + ] } }, + entity("Tag", ["id", "name"], [primary("id")]), + entity("ArticleTag", ["articleId", "tagId"], [ + primary("articleId", "tagId"), + reference("articleRef", "articleId", "ArticleBase"), + reference("tagRef", "tagId", "Tag"), + ]), + ], + }, +}; + +describe("inherited M:N — derivation follows the DECLARING entity", () => { + test("self-join inherited via extends resolves on the child (was: MetadataError)", async () => { + const root = await load(INHERITED_SELF_JOIN); + // Pin the premise: Node is reached before NodeBase. + expect(root.ownChildren().filter((c) => c.type === "object").map((c) => c.name)) + .toEqual(["Node", "NodeBase", "NodeLink"]); + const node = root.ownChildByName("Node")!; + const desc = resolveN2mDescriptor(node, "peers", root)!; + expect(desc.sourceJoinField).toBe("aId"); + expect(desc.targetJoinField).toBe("bId"); + expect(desc.symmetric).toBe(true); + // The descriptor still names the entity actually being queried. + expect(desc.sourceEntityName).toBe("Node"); + }); + + test("hetero inherited via extends matches the base's junction reference", async () => { + const root = await load(INHERITED_HETERO); + const article = root.ownChildByName("Article")!; + const desc = resolveN2mDescriptor(article, "tags", root)!; + expect(desc.sourceJoinField).toBe("articleId"); + expect(desc.targetJoinField).toBe("tagId"); + }); +}); From 94fb8a9367b506efa6d4ccf290acb6c264aa6492 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 13 Sep 2026 23:13:23 -0400 Subject: [PATCH 02/12] fix(metadata-java): M:N derivation must use the DECLARING entity, not the visiting one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Java port of the TS fix. M2MFields.derive classified the self-join — and matched the hetero junction reference — against the `source` entity its caller passed. SpringM2mSupport, KotlinM2mSupport (which reaches the same SSOT) and omdb's M2MResolver all walk the RESOLVING getRelationships() and pass the entity they are iterating, so for a relationship inherited via `extends` that is the INHERITING entity. An inherited self-join then read as hetero and threw; an inherited hetero looked for a junction reference to the child and threw too. The declaring entity is now resolved inside derive() from rel.getParent(), mirroring the #368 ValidationPhase fix; `source` stays as the no-parent fallback so the public signature is unchanged. Kotlin needs no change of its own — codegen-kotlin calls this same helper. Both new tests were confirmed failing against the unfixed derive() with the pre-fix "must declare one identity.reference to ..." message. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- .../metaobjects/relationship/M2MFields.java | 48 +++++++--- .../relationship/M2MSlimVocabularyTest.java | 95 +++++++++++++++++++ 2 files changed, 129 insertions(+), 14 deletions(-) diff --git a/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java b/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java index 16ef5aee0..f7005613a 100644 --- a/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java +++ b/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java @@ -46,6 +46,18 @@ * * Ambiguous (source == target, neither {@code @sourceRefField} nor {@code @symmetric}) → throw. * + *

"source" above always means the entity that DECLARES the relationship — never + * whichever entity's effective view reached it. Every caller walks the RESOLVING + * {@code getRelationships()}, so for a relationship inherited via {@code extends} the + * entity it is iterating is the INHERITING one, and both the self-join classification + * and the hetero reference match would then be made against the wrong entity (an + * inherited self-join reads as hetero and derivation throws; an inherited hetero finds + * no junction reference to the inheriting entity and throws too). The declaring entity + * is resolved HERE, from {@code rel.getParent()}, rather than asked of each caller — + * same shape as the #368 loader fix in {@code ValidationPhase}, and for the same reason: + * the answer must not depend on who asked. The {@code source} parameter is kept as the + * fallback for a relationship with no entity parent (and as a non-breaking signature).

+ * *

This carries the same semantics as the loader-phase M:N validation * ({@code ValidationPhase.validateRelationshipsM2M}); the validation pass guarantees a * well-formed junction (exactly two references, matching {@code @sourceRefField}) before @@ -83,30 +95,38 @@ public M2MDerivationException(String message) { * * @param rel the M:N relationship (carries {@code @objectRef} + {@code @through} * + optional {@code @sourceRefField} / {@code @symmetric}) - * @param source the entity declaring {@code rel} + * @param source fallback declaring entity, used only when {@code rel} has no + * {@link MetaObject} parent (normally read from {@code rel.getParent()}) * @param root the loaded model root (to find the junction entity) * @return the derived source/target junction FK fields * @throws M2MDerivationException when the junction is missing/malformed or the * self-join is ambiguous. */ public static M2MFields derive(MetaRelationship rel, MetaObject source, MetaRoot root) { + // The entity that DECLARES `rel` — see the class note. getParent() is the + // owning entity for both an own declaration and an inherited one (an + // unmodified inherited child is the SAME node object; an override is a + // different node whose parent is the overriding entity, also correct). + MetaData relParent = rel.getParent(); + MetaObject declaring = (relParent instanceof MetaObject) ? (MetaObject) relParent : source; + String throughName = rel.getThrough(); if (throughName == null || throughName.isEmpty()) { throw new M2MDerivationException( - "relationship \"" + source.getShortName() + "." + rel.getShortName() + "relationship \"" + declaring.getShortName() + "." + rel.getShortName() + "\" is missing @through (required for M:N derivation)"); } MetaObject junction = findObject(root, throughName); if (junction == null) { throw new M2MDerivationException( - "relationship \"" + source.getShortName() + "." + rel.getShortName() + "relationship \"" + declaring.getShortName() + "." + rel.getShortName() + "\" @through \"" + throughName + "\" does not resolve to an entity"); } String targetName = rel.getObjectRef(); if (targetName == null || targetName.isEmpty()) { throw new M2MDerivationException( - "relationship \"" + source.getShortName() + "." + rel.getShortName() + "relationship \"" + declaring.getShortName() + "." + rel.getShortName() + "\" is missing @objectRef (the M:N target)"); } MetaObject target = findObject(root, targetName); @@ -114,7 +134,7 @@ public static M2MFields derive(MetaRelationship rel, MetaObject source, MetaRoot List refs = referenceIdentities(junction); if (refs.size() != 2) { throw new M2MDerivationException( - "junction \"" + throughName + "\" for relationship \"" + source.getShortName() + "junction \"" + throughName + "\" for relationship \"" + declaring.getShortName() + "." + rel.getShortName() + "\" must declare exactly two" + " identity.reference children (found " + refs.size() + ")"); } @@ -126,22 +146,22 @@ public static M2MFields derive(MetaRelationship rel, MetaObject source, MetaRoot // back to a bare-name compare only when @objectRef is unresolvable (defensive // — loader validation normally guarantees resolution before derive runs). boolean isSelfJoin = (target != null) - ? target.getName().equals(source.getName()) - : stripPackage(targetName).equals(source.getShortName()); + ? target.getName().equals(declaring.getName()) + : stripPackage(targetName).equals(declaring.getShortName()); if (!isSelfJoin) { // Hetero: match each reference by the ENTITY OBJECT its @references // resolves to (FQN-exact), so a same-bare-name cross-package reference // binds the correct package rather than the first bare-tail match. - MetaIdentity sourceRef = findRefToObject(root, refs, source); + MetaIdentity sourceRef = findRefToObject(root, refs, declaring); MetaIdentity targetRef = findRefToObject(root, refs, target); String sourceField = sourceRef != null ? refFkField(sourceRef) : null; String targetField = targetRef != null ? refFkField(targetRef) : null; if (sourceField == null || targetField == null) { throw new M2MDerivationException( - "junction \"" + throughName + "\" for relationship \"" + source.getShortName() + "junction \"" + throughName + "\" for relationship \"" + declaring.getShortName() + "." + rel.getShortName() + "\" must declare one identity.reference to \"" - + source.getShortName() + "\" and one to \"" + stripPackage(targetName) + "\""); + + declaring.getShortName() + "\" and one to \"" + stripPackage(targetName) + "\""); } return new M2MFields(sourceField, targetField); } @@ -153,7 +173,7 @@ public static M2MFields derive(MetaRelationship rel, MetaObject source, MetaRoot String b = refFkField(refs.get(1)); if (a == null || b == null) { throw new M2MDerivationException( - "symmetric junction \"" + throughName + "\" for \"" + source.getShortName() + "symmetric junction \"" + throughName + "\" for \"" + declaring.getShortName() + "." + rel.getShortName() + "\" has a reference with no @fields"); } return new M2MFields(a, b); @@ -162,7 +182,7 @@ public static M2MFields derive(MetaRelationship rel, MetaObject source, MetaRoot String sourceRefField = rel.getSourceRefField(); if (sourceRefField == null || sourceRefField.isEmpty()) { throw new M2MDerivationException( - "self-join relationship \"" + source.getShortName() + "." + rel.getShortName() + "self-join relationship \"" + declaring.getShortName() + "." + rel.getShortName() + "\" through \"" + throughName + "\" is ambiguous: set @sourceRefField" + " (directed) or @symmetric (undirected)"); } @@ -177,7 +197,7 @@ public static M2MFields derive(MetaRelationship rel, MetaObject source, MetaRoot } if (sourceRef == null) { throw new M2MDerivationException( - "@sourceRefField \"" + sourceRefField + "\" on \"" + source.getShortName() + "." + "@sourceRefField \"" + sourceRefField + "\" on \"" + declaring.getShortName() + "." + rel.getShortName() + "\" does not match any identity.reference FK field on" + " junction \"" + throughName + "\""); } @@ -190,7 +210,7 @@ public static M2MFields derive(MetaRelationship rel, MetaObject source, MetaRoot } if (targetField == null) { throw new M2MDerivationException( - "junction \"" + throughName + "\" for \"" + source.getShortName() + "." + "junction \"" + throughName + "\" for \"" + declaring.getShortName() + "." + rel.getShortName() + "\" has no distinct target-side reference"); } return new M2MFields(sourceRefField, targetField); diff --git a/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java b/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java index ff42f134b..fb8054b62 100644 --- a/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java +++ b/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java @@ -465,4 +465,99 @@ public void deriveCrossPackageJunctionCollisionBindsCorrectPackage() { assertEquals("postId", f.getSourceField()); assertEquals("tagId", f.getTargetField()); } + + // --- 6. Declaring entity vs. visiting entity (the #368 follow-up) ------------------ + // + // Same confusion as the #368 loader passes, one layer down: derive() classified the + // self-join, and matched the hetero junction reference, against the `source` entity + // its CALLER passed. Every caller walks the RESOLVING getRelationships() + // (SpringM2mSupport, KotlinM2mSupport, omdb's M2MResolver) and passes the entity it + // is iterating — so for a relationship inherited via `extends` that is the + // INHERITING entity, not the one that declared it. + // + // Unlike the loader passes there is no once-per-node `checked` set here, so the + // defect is NOT gated on visit order — it is wrong for every inheriting entity in + // any order. The fixtures still declare the child BEFORE the base and pin the root + // visit order, matching the #368 convention. + + /** Node extends NodeBase, which declares a @symmetric self-join onto ITSELF. Node + * is declared FIRST so the root's child order is child-before-base. */ + private static final String INHERITED_SELF_JOIN = + "{ \"metadata.root\": { \"package\": \"acme\", \"children\": [" + + " { \"object.entity\": { \"name\": \"Node\", \"extends\": \"NodeBase\", \"children\": [" + + " { \"field.long\": { \"name\": \"id\" } }," + + " { \"identity.primary\": { \"@fields\": \"id\" } } ] } }," + + " { \"object.entity\": { \"name\": \"NodeBase\", \"@isAbstract\": true, \"children\": [" + + " { \"relationship.association\": { \"name\": \"peers\", \"@cardinality\": \"many\"," + + " \"@objectRef\": \"NodeBase\", \"@through\": \"NodeLink\", \"@symmetric\": true } } ] } }," + + " { \"object.entity\": { \"name\": \"NodeLink\", \"children\": [" + + " { \"field.long\": { \"name\": \"id\" } }," + + " { \"field.long\": { \"name\": \"aId\" } }," + + " { \"field.long\": { \"name\": \"bId\" } }," + + " { \"identity.primary\": { \"@fields\": \"id\" } }," + + " { \"identity.reference\": { \"name\": \"aRef\", \"@fields\": \"aId\", \"@references\": \"NodeBase\" } }," + + " { \"identity.reference\": { \"name\": \"bRef\", \"@fields\": \"bId\", \"@references\": \"NodeBase\" } } ] } }" + + "] } }"; + + /** Article extends ArticleBase, which declares a HETERO M:N whose junction references + * the BASE — so a match against the visiting child finds nothing. */ + private static final String INHERITED_HETERO = + "{ \"metadata.root\": { \"package\": \"acme\", \"children\": [" + + " { \"object.entity\": { \"name\": \"Article\", \"extends\": \"ArticleBase\", \"children\": [" + + " { \"field.long\": { \"name\": \"id\" } }," + + " { \"identity.primary\": { \"@fields\": \"id\" } } ] } }," + + " { \"object.entity\": { \"name\": \"ArticleBase\", \"@isAbstract\": true, \"children\": [" + + " { \"relationship.association\": { \"name\": \"tags\", \"@cardinality\": \"many\"," + + " \"@objectRef\": \"Tag\", \"@through\": \"ArticleTag\" } } ] } }," + + " { \"object.entity\": { \"name\": \"Tag\", \"children\": [" + + " { \"field.long\": { \"name\": \"id\" } }," + + " { \"identity.primary\": { \"@fields\": \"id\" } } ] } }," + + " { \"object.entity\": { \"name\": \"ArticleTag\", \"children\": [" + + " { \"field.long\": { \"name\": \"id\" } }," + + " { \"field.long\": { \"name\": \"articleId\" } }," + + " { \"field.long\": { \"name\": \"tagId\" } }," + + " { \"identity.primary\": { \"@fields\": \"id\" } }," + + " { \"identity.reference\": { \"name\": \"aRef\", \"@fields\": \"articleId\", \"@references\": \"ArticleBase\" } }," + + " { \"identity.reference\": { \"name\": \"tRef\", \"@fields\": \"tagId\", \"@references\": \"Tag\" } } ] } }" + + "] } }"; + + /** Root-level object order, so a test's child-before-base premise fails loudly if + * the loader ever stops preserving declaration order. */ + private static List objectOrder(MetaDataLoader loader) { + List out = new java.util.ArrayList<>(); + for (MetaObject mo : loader.getRoot().getChildren(MetaObject.class, false)) out.add(mo.getName()); + return out; + } + + @Test + public void deriveInheritedSelfJoinUsesDeclaringEntity() { + MetaDataLoader loader = loadThrough(INHERITED_SELF_JOIN, "inherited-self-join.json"); + assertEquals(List.of("acme::Node", "acme::NodeBase", "acme::NodeLink"), objectOrder(loader)); + MetaObject node = objExact(loader, "acme::Node"); + MetaRelationship rel = relOf(node, "peers"); + // The relationship is INHERITED: it is not one of Node's own children. + assertTrue(node.getRelationships(false).isEmpty()); + assertEquals("acme::NodeBase", ((MetaObject) rel.getParent()).getName()); + // Pre-fix: @objectRef "NodeBase" vs the visiting "acme::Node" => not a self-join + // => hetero branch => no junction reference to Node => M2MDerivationException. + M2MFields f = M2MFields.derive(rel, node, loader.getRoot()); + assertEquals("aId", f.getSourceField()); + assertEquals("bId", f.getTargetField()); + // The declaring entity itself must agree — same node, same answer. + MetaObject base = objExact(loader, "acme::NodeBase"); + M2MFields viaBase = M2MFields.derive(rel, base, loader.getRoot()); + assertEquals(f.getSourceField(), viaBase.getSourceField()); + assertEquals(f.getTargetField(), viaBase.getTargetField()); + } + + @Test + public void deriveInheritedHeteroMatchesDeclaringEntityReference() { + MetaDataLoader loader = loadThrough(INHERITED_HETERO, "inherited-hetero.json"); + assertEquals(List.of("acme::Article", "acme::ArticleBase", "acme::Tag", "acme::ArticleTag"), + objectOrder(loader)); + MetaObject article = objExact(loader, "acme::Article"); + M2MFields f = M2MFields.derive(relOf(article, "tags"), article, loader.getRoot()); + assertEquals("articleId", f.getSourceField()); + assertEquals("tagId", f.getTargetField()); + } } From 10621919586b577ac915420ae3796b3935bbdb29 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 13 Sep 2026 23:17:07 -0400 Subject: [PATCH 03/12] fix(metadata-ts): accept BOTH the declaring base and the navigating entity as the M:N subject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up correction to the previous commit. Resolving the declaring entity and discarding `source` outright broke the other — and more common — authoring shape: an abstract base declares the M:N, and the junction FK references the CONCRETE entity, because the base has no table. That shape is what python/tests/unit/test_n2m_resolver_inherited.py pins, and it is the only one that worked before this branch. So the derivation now treats BOTH names of the relationship's subject as valid — the declaring entity (rel.parent) and the entity the caller is navigating from — for the self-join classification and for the hetero source-side reference match alike. That is a strict widening: every model that derived before still derives, and the two inherited shapes that used to throw now work. Not covered, and stated in the comment: a junction reference naming an entity strictly BETWEEN the base and the navigating entity in a deeper hierarchy. Adds the counter-case test (inherited hetero whose junction references the concrete child), which fails against the discard-source version. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- .../core/relationship/derive-m2m-fields.ts | 40 +++++++++++++++---- .../metadata/test/relationship-m2m.test.ts | 34 ++++++++++++++++ 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts index 8b0fc3871..b2148c656 100644 --- a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts +++ b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts @@ -28,8 +28,12 @@ // declaring entity is resolved HERE, from `rel.parent`, rather than asked of each // caller — same shape as the #368 loader fix (`declaringEntity = rel.parent ?? obj` // in validation-passes.ts), and for the same reason: the answer must not depend on -// who asked. The `source` parameter is kept as the fallback for a synthetic -// relationship with no parent (and as a non-breaking signature). +// who asked. The passed `source` is NOT discarded: under `extends` the declaring +// base and the navigating entity are two legitimate names for the relationship's +// subject (a junction FK usually references the concrete entity, which is the one +// with a table; @objectRef on an inherited self-join names the base), so both are +// accepted — see `subjectNames` below. It is also the fallback when `rel` has no +// entity parent, which keeps the exported signature unchanged. import type { MetaObject } from "../object/meta-object.js"; import type { MetaRoot } from "../../shared/meta-root.js"; @@ -64,8 +68,9 @@ function refFkField(ref: MetaReferenceIdentity): string | undefined { * * @param rel the M:N relationship (carries @objectRef + @through + optional * @sourceRefField / @symmetric) - * @param source fallback declaring entity, used only when `rel` has no parent - * (the declaring entity is normally read from `rel.parent`) + * @param source the entity the caller is navigating from. Accepted alongside + * `rel.parent` as a name for the relationship's subject, and used + * as the declaring entity when `rel` has no entity parent. * @param root the loaded model root (to find the junction entity) * @throws M2MDerivationError when the junction is missing/malformed or the * self-join is ambiguous. @@ -121,18 +126,39 @@ export function deriveM2MFields( ); } - const isSelfJoin = stripPackage(targetName) === declaringEntity.name; + // The relationship's SUBJECT — the entity the M:N hangs off. Under `extends` + // there are two legitimate names for it and BOTH occur in real models: + // * the DECLARING entity (rel.parent) — what @objectRef names for a self-join + // declared on an abstract base, and what a junction reference names when the + // author points the FK at the base type; + // * the NAVIGATING entity (`source`) — the concrete entity the caller is + // iterating, which is what a junction FK usually references, because that is + // the entity with the physical table. + // Accepting either is what makes the derivation independent of which entity's + // effective view reached the relationship. (Not covered: a junction reference + // naming an entity strictly BETWEEN the declaring base and the navigating + // entity in a deeper hierarchy — no model does that, and widening to the whole + // super chain would make the "must declare one identity.reference to ..." error + // unfalsifiable.) + const subjectNames = declaringEntity.name === source.name + ? [declaringEntity.name] + : [declaringEntity.name, source.name]; + const isSubject = (name: string | undefined): boolean => + name !== undefined && subjectNames.includes(stripPackage(name)); + const subjectLabel = subjectNames.map((n) => `"${n}"`).join(" or "); + + const isSelfJoin = isSubject(targetName); if (!isSelfJoin) { // Hetero: match each reference by the entity it resolves to. - const sourceRef = refs.find((r) => r.targetEntity !== undefined && stripPackage(r.targetEntity) === declaringEntity.name); + const sourceRef = refs.find((r) => isSubject(r.targetEntity)); const targetRef = refs.find((r) => r.targetEntity !== undefined && stripPackage(r.targetEntity) === stripPackage(targetName)); const sourceField = sourceRef ? refFkField(sourceRef) : undefined; const targetField = targetRef ? refFkField(targetRef) : undefined; if (sourceField === undefined || targetField === undefined) { throw new M2MDerivationError( `junction "${throughName}" for relationship "${declaringEntity.name}.${rel.name}" must declare one ` + - `identity.reference to "${declaringEntity.name}" and one to "${stripPackage(targetName)}"`, + `identity.reference to ${subjectLabel} and one to "${stripPackage(targetName)}"`, ); } return { sourceField, targetField }; diff --git a/server/typescript/packages/metadata/test/relationship-m2m.test.ts b/server/typescript/packages/metadata/test/relationship-m2m.test.ts index 9d867e9b2..a649d006e 100644 --- a/server/typescript/packages/metadata/test/relationship-m2m.test.ts +++ b/server/typescript/packages/metadata/test/relationship-m2m.test.ts @@ -818,6 +818,40 @@ describe("FR-017 deriveM2MFields uses the DECLARING entity, not the visiting one expect(derived.targetField).toBe("tagId"); }); + test("inherited HETERO whose junction references the CONCRETE child still derives", async () => { + // The other legitimate authoring shape, and the common one: the base is + // abstract (no table), so the junction FK references the CONCRETE entity. + // Both names — the declaring base and the navigating child — must be + // accepted as the relationship's subject, or fixing the base-referencing + // shape would break this one. Pinned by + // python/tests/unit/test_n2m_resolver_inherited.py, which is authored this way. + const { root, errors } = await loadDoc({ "metadata.root": { package: "acme", children: [ + { "object.entity": { name: "Post", "extends": "PostBase", children: [ + { "field.long": { name: "id" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + { "object.entity": { name: "PostBase", "@isAbstract": true, children: [ + { "relationship.association": { name: "tags", "@cardinality": "many", "@objectRef": "Tag", + "@through": "PostTag" } } ] } }, + { "object.entity": { name: "Tag", children: [ + { "field.long": { name: "id" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + { "object.entity": { name: "PostTag", children: [ + { "field.long": { name: "id" } }, + { "field.long": { name: "postId" } }, + { "field.long": { name: "tagId" } }, + { "identity.primary": { "name": "id", "@fields": "id" } }, + { "identity.reference": { name: "postRef", "@fields": ["postId"], "@references": "Post" } }, + { "identity.reference": { name: "tagRef", "@fields": ["tagId"], "@references": "Tag" } } ] } }, + ] } }); + expect(errors).toHaveLength(0); + const post = findObj(root, "Post"); + const rel = post.relationships().find((r) => r.name === "tags") as MetaRelationship; + expect(rel.parent?.name).toBe("PostBase"); + const derived = deriveM2MFields(rel, post, root); + expect(derived.sourceField).toBe("postId"); + expect(derived.targetField).toBe("tagId"); + }); + test("an OWN relationship still derives against its own entity (no regression)", async () => { // The override case: Sub re-declares `peers` itself, so rel.parent IS Sub and // the self-join must be classified against Sub, not the base it shadows. From 8f7f55bfb41535660465db8455cc1c7ef76db5a3 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 13 Sep 2026 23:19:00 -0400 Subject: [PATCH 04/12] fix(metadata-java): accept BOTH the declaring base and the navigating entity as the M:N subject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Java port of the TS correction. derive() now treats the declaring entity (rel.getParent()) and the navigating entity (`source`) as two equally valid names for the relationship's subject, for the self-join classification and for the hetero source-side reference match alike — because a junction FK usually references the CONCRETE entity (the base is abstract and has no table) while @objectRef on an inherited self-join names the base. Both comparisons stay ADR-0041 FQN-exact via the new isSubject / findRefToSubject helpers. Adds the counter-case test (inherited hetero whose junction references the concrete child) alongside the two regressions. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- .../metaobjects/relationship/M2MFields.java | 69 +++++++++++++++---- .../relationship/M2MSlimVocabularyTest.java | 34 +++++++++ 2 files changed, 88 insertions(+), 15 deletions(-) diff --git a/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java b/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java index f7005613a..6f25db3ed 100644 --- a/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java +++ b/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java @@ -55,8 +55,11 @@ * no junction reference to the inheriting entity and throws too). The declaring entity * is resolved HERE, from {@code rel.getParent()}, rather than asked of each caller — * same shape as the #368 loader fix in {@code ValidationPhase}, and for the same reason: - * the answer must not depend on who asked. The {@code source} parameter is kept as the - * fallback for a relationship with no entity parent (and as a non-breaking signature).

+ * the answer must not depend on who asked. The passed {@code source} is NOT discarded: + * under {@code extends} the declaring base and the navigating entity are two legitimate + * names for the relationship's subject (a junction FK usually references the concrete + * entity, which is the one with a table), so BOTH are accepted. It is also the fallback + * when {@code rel} has no entity parent, which keeps the signature unchanged.

* *

This carries the same semantics as the loader-phase M:N validation * ({@code ValidationPhase.validateRelationshipsM2M}); the validation pass guarantees a @@ -95,8 +98,9 @@ public M2MDerivationException(String message) { * * @param rel the M:N relationship (carries {@code @objectRef} + {@code @through} * + optional {@code @sourceRefField} / {@code @symmetric}) - * @param source fallback declaring entity, used only when {@code rel} has no - * {@link MetaObject} parent (normally read from {@code rel.getParent()}) + * @param source the entity the caller is navigating from. Accepted alongside + * {@code rel.getParent()} as a name for the relationship's subject, + * and used as the declaring entity when {@code rel} has no parent * @param root the loaded model root (to find the junction entity) * @return the derived source/target junction FK fields * @throws M2MDerivationException when the junction is missing/malformed or the @@ -139,29 +143,44 @@ public static M2MFields derive(MetaRelationship rel, MetaObject source, MetaRoot + " identity.reference children (found " + refs.size() + ")"); } - // ADR-0041: classify the self-join by RESOLVED object identity (FQN-exact), - // never a stripped bare tail. A cross-package hetero M:N whose target shares - // a bare name with the source (or an unrelated entity) must NOT be mis-read - // as a self-join, and an FQN @objectRef must bind the correct package. Falls - // back to a bare-name compare only when @objectRef is unresolvable (defensive - // — loader validation normally guarantees resolution before derive runs). + // The relationship's SUBJECT — the entity the M:N hangs off. Under `extends` + // there are two legitimate names for it and BOTH occur in real models: the + // DECLARING entity (what @objectRef names for a self-join declared on an + // abstract base, and what a junction reference names when the FK points at + // the base type), and the NAVIGATING entity (`source`, the concrete entity + // the caller is iterating — usually what a junction FK references, because + // that is the entity with the physical table). Accepting either is what + // makes derivation independent of which entity's effective view reached the + // relationship. Not covered: a junction reference naming an entity strictly + // BETWEEN the base and the navigating entity in a deeper hierarchy. + // + // ADR-0041: classify by RESOLVED object identity (FQN-exact), never a + // stripped bare tail. A cross-package hetero M:N whose target shares a bare + // name with the subject must NOT be mis-read as a self-join, and an FQN + // @objectRef must bind the correct package. Falls back to a bare-name + // compare only when @objectRef is unresolvable (defensive — loader + // validation normally guarantees resolution before derive runs). boolean isSelfJoin = (target != null) - ? target.getName().equals(declaring.getName()) - : stripPackage(targetName).equals(declaring.getShortName()); + ? isSubject(target, declaring, source) + : (stripPackage(targetName).equals(declaring.getShortName()) + || stripPackage(targetName).equals(source.getShortName())); if (!isSelfJoin) { // Hetero: match each reference by the ENTITY OBJECT its @references // resolves to (FQN-exact), so a same-bare-name cross-package reference // binds the correct package rather than the first bare-tail match. - MetaIdentity sourceRef = findRefToObject(root, refs, declaring); + MetaIdentity sourceRef = findRefToSubject(root, refs, declaring, source); MetaIdentity targetRef = findRefToObject(root, refs, target); String sourceField = sourceRef != null ? refFkField(sourceRef) : null; String targetField = targetRef != null ? refFkField(targetRef) : null; if (sourceField == null || targetField == null) { + String subjectLabel = declaring.getName().equals(source.getName()) + ? "\"" + declaring.getShortName() + "\"" + : "\"" + declaring.getShortName() + "\" or \"" + source.getShortName() + "\""; throw new M2MDerivationException( "junction \"" + throughName + "\" for relationship \"" + declaring.getShortName() - + "." + rel.getShortName() + "\" must declare one identity.reference to \"" - + declaring.getShortName() + "\" and one to \"" + stripPackage(targetName) + "\""); + + "." + rel.getShortName() + "\" must declare one identity.reference to " + + subjectLabel + " and one to \"" + stripPackage(targetName) + "\""); } return new M2MFields(sourceField, targetField); } @@ -269,6 +288,26 @@ private static MetaObject refTargetObject(MetaRoot root, MetaIdentity ref) { * bare tail — two junction references to same-bare-name entities in different * packages must be distinguished by their full package-qualified name. */ + /** + * True when {@code candidate} is one of the relationship's SUBJECT entities — + * the declaring entity or the entity the caller is navigating from. Compared by + * package-qualified name (ADR-0041 identity), never a bare tail. + */ + private static boolean isSubject(MetaObject candidate, MetaObject declaring, MetaObject source) { + if (candidate == null) return false; + return candidate.getName().equals(declaring.getName()) + || candidate.getName().equals(source.getName()); + } + + /** The junction reference resolving to either subject entity, or {@code null}. */ + private static MetaIdentity findRefToSubject(MetaRoot root, List refs, + MetaObject declaring, MetaObject source) { + for (MetaIdentity ref : refs) { + if (isSubject(refTargetObject(root, ref), declaring, source)) return ref; + } + return null; + } + private static MetaIdentity findRefToObject(MetaRoot root, List refs, MetaObject entity) { if (entity == null) return null; for (MetaIdentity ref : refs) { diff --git a/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java b/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java index fb8054b62..8b0db16d2 100644 --- a/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java +++ b/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java @@ -529,6 +529,40 @@ private static List objectOrder(MetaDataLoader loader) { return out; } + /** The OTHER legitimate shape, and the common one: the base is abstract (no table), + * so the junction FK references the CONCRETE child. Both names of the subject must + * be accepted, or fixing the base-referencing shape breaks this one. */ + private static final String INHERITED_HETERO_CONCRETE_REF = + "{ \"metadata.root\": { \"package\": \"acme\", \"children\": [" + + " { \"object.entity\": { \"name\": \"Post\", \"extends\": \"PostBase\", \"children\": [" + + " { \"field.long\": { \"name\": \"id\" } }," + + " { \"identity.primary\": { \"@fields\": \"id\" } } ] } }," + + " { \"object.entity\": { \"name\": \"PostBase\", \"@isAbstract\": true, \"children\": [" + + " { \"relationship.association\": { \"name\": \"tags\", \"@cardinality\": \"many\"," + + " \"@objectRef\": \"Tag\", \"@through\": \"PostTag\" } } ] } }," + + " { \"object.entity\": { \"name\": \"Tag\", \"children\": [" + + " { \"field.long\": { \"name\": \"id\" } }," + + " { \"identity.primary\": { \"@fields\": \"id\" } } ] } }," + + " { \"object.entity\": { \"name\": \"PostTag\", \"children\": [" + + " { \"field.long\": { \"name\": \"id\" } }," + + " { \"field.long\": { \"name\": \"postId\" } }," + + " { \"field.long\": { \"name\": \"tagId\" } }," + + " { \"identity.primary\": { \"@fields\": \"id\" } }," + + " { \"identity.reference\": { \"name\": \"pRef\", \"@fields\": \"postId\", \"@references\": \"Post\" } }," + + " { \"identity.reference\": { \"name\": \"tRef\", \"@fields\": \"tagId\", \"@references\": \"Tag\" } } ] } }" + + "] } }"; + + @Test + public void deriveInheritedHeteroWithConcreteJunctionReference() { + MetaDataLoader loader = loadThrough(INHERITED_HETERO_CONCRETE_REF, "inherited-hetero-concrete.json"); + MetaObject post = objExact(loader, "acme::Post"); + MetaRelationship rel = relOf(post, "tags"); + assertEquals("acme::PostBase", ((MetaObject) rel.getParent()).getName()); + M2MFields f = M2MFields.derive(rel, post, loader.getRoot()); + assertEquals("postId", f.getSourceField()); + assertEquals("tagId", f.getTargetField()); + } + @Test public void deriveInheritedSelfJoinUsesDeclaringEntity() { MetaDataLoader loader = loadThrough(INHERITED_SELF_JOIN, "inherited-self-join.json"); From 7c09e0b3607fe624f8f755168a5ee3b2745a1a60 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 13 Sep 2026 23:22:35 -0400 Subject: [PATCH 05/12] fix(metadata-python): M:N derivation must use the relationship's subject, not the visiting entity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Python port of the same fix. derive_m2m_fields classified the self-join, and matched the hetero junction reference, against the `source` entity its caller passed; resolve_n2m_descriptor (runtime) and m2m_codegen.resolve_m2m_descriptors (codegen) both walk RESOLVING children() and pass the entity they are iterating, so an inherited self-join read as hetero and raised, and an inherited hetero whose junction references the BASE raised too. The runtime path re-raises as N2mResolutionError, so the relationship was untraversable, not merely dropped. The declaring entity now comes from rel.parent, and BOTH it and the navigating entity are accepted as the relationship's subject — the junction FK usually references the concrete child, the shape test_n2m_resolver_inherited.py pins. Three of the four new tests were confirmed failing against the unfixed derivation; the fourth is the counter-case that guards the widening (it fails against a declaring-entity-only fix, which is how the widening was found). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- .../core/relationship/derive_m2m_fields.py | 77 +++++-- .../unit/test_derive_m2m_declaring_entity.py | 216 ++++++++++++++++++ 2 files changed, 278 insertions(+), 15 deletions(-) create mode 100644 server/python/tests/unit/test_derive_m2m_declaring_entity.py diff --git a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py index c389a0856..5cfc228a3 100644 --- a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py +++ b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py @@ -16,13 +16,30 @@ two references are taken in declaration order (source_field = first, target_field = second). Resolution unions both at read time. Ambiguous (source == target, neither @sourceRefField nor @symmetric) → raise. + +"source" above always means the entity that DECLARES the relationship — never +whichever entity's effective view reached it. Every caller walks the RESOLVING +``children()`` / ``m2m_relationships()``, so for a relationship inherited via +``extends`` the entity it is iterating is the INHERITING one, and both the +self-join classification and the hetero reference match would then be made +against the wrong entity (an inherited self-join reads as hetero and derivation +raises; an inherited hetero finds no junction reference to the inheriting entity +and raises too). The declaring entity is resolved HERE, from ``rel.parent``, +rather than asked of each caller — same shape as the #368 loader fix +(``declaring_entity = rel.parent if rel.parent is not None else obj`` in +``validation_passes.py``), and for the same reason: the answer must not depend on +who asked. The passed ``source`` is NOT discarded: under ``extends`` the declaring +base and the navigating entity are two legitimate names for the relationship's +subject (a junction FK usually references the concrete entity, which is the one +with a table), so BOTH are accepted. It is also the fallback when ``rel`` has no +object parent, which keeps the signature unchanged. """ from __future__ import annotations from dataclasses import dataclass from ...meta_data import MetaData -from ....shared.base_types import TYPE_IDENTITY +from ....shared.base_types import TYPE_IDENTITY, TYPE_OBJECT from ....shared.separators import PACKAGE_SEP from ..identity.identity_constants import ( IDENTITY_ATTR_FIELDS, @@ -103,43 +120,73 @@ def derive_m2m_fields( *object_index* is a bare-name → object map of the loaded model's top-level objects (the Python loader's resolution surface; mirrors the TS - ``root.findObject``). Raises :class:`M2MDerivationError` when the junction is - missing/malformed or the self-join is ambiguous. + ``root.findObject``). *source* is the entity the caller is navigating from; + it is accepted alongside ``rel.parent`` as a name for the relationship's + subject, and used as the declaring entity when *rel* has no object parent — + see the module docstring. Raises + :class:`M2MDerivationError` when the junction is missing/malformed or the + self-join is ambiguous. """ + # The entity that DECLARES ``rel`` — see the module docstring. ``parent`` is + # the owning entity for both an own declaration and an inherited one (an + # unmodified inherited child is the SAME node object; an override is a + # different node whose parent is the overriding entity, also correct). + rel_parent = rel.parent + declaring = ( + rel_parent + if rel_parent is not None and rel_parent.type == TYPE_OBJECT + else source + ) + through_name = rel.through() if through_name is None: raise M2MDerivationError( - f'relationship "{source.name}.{rel.name}" is missing @through ' + f'relationship "{declaring.name}.{rel.name}" is missing @through ' f"(required for M:N derivation)" ) junction = object_index.get(_strip_package(through_name)) if junction is None: raise M2MDerivationError( - f'relationship "{source.name}.{rel.name}" @through "{through_name}" ' + f'relationship "{declaring.name}.{rel.name}" @through "{through_name}" ' f"does not resolve to an entity" ) target_name = rel.object_ref() if target_name is None: raise M2MDerivationError( - f'relationship "{source.name}.{rel.name}" is missing @objectRef ' + f'relationship "{declaring.name}.{rel.name}" is missing @objectRef ' f"(the M:N target)" ) refs = _reference_children(junction) if len(refs) != 2: raise M2MDerivationError( - f'junction "{through_name}" for relationship "{source.name}.{rel.name}" ' + f'junction "{through_name}" for relationship "{declaring.name}.{rel.name}" ' f"must declare exactly two identity.reference children " f"(found {len(refs)})" ) - is_self_join = _strip_package(target_name) == source.name + # The relationship's SUBJECT — the entity the M:N hangs off. Under ``extends`` + # there are two legitimate names for it and BOTH occur in real models: the + # DECLARING entity (what @objectRef names for a self-join declared on an + # abstract base, and what a junction reference names when the FK points at the + # base type), and the NAVIGATING entity (*source*, the concrete entity the + # caller is iterating — usually what a junction FK references, because that is + # the entity with the physical table). Accepting either is what makes the + # derivation independent of which entity's effective view reached the + # relationship. Not covered: a junction reference naming an entity strictly + # BETWEEN the base and the navigating entity in a deeper hierarchy. + subject_names = [declaring.name] + if source.name != declaring.name: + subject_names.append(source.name) + subject_label = " or ".join(f'"{n}"' for n in subject_names) + + is_self_join = _strip_package(target_name) in subject_names if not is_self_join: # Hetero: match each reference by the entity it resolves to. source_ref = next( - (r for r in refs if _ref_target_entity(r) == source.name), None + (r for r in refs if _ref_target_entity(r) in subject_names), None ) target_ref = next( (r for r in refs if _ref_target_entity(r) == _strip_package(target_name)), @@ -150,8 +197,8 @@ def derive_m2m_fields( if source_field is None or target_field is None: raise M2MDerivationError( f'junction "{through_name}" for relationship ' - f'"{source.name}.{rel.name}" must declare one identity.reference ' - f'to "{source.name}" and one to "{_strip_package(target_name)}"' + f'"{declaring.name}.{rel.name}" must declare one identity.reference ' + f'to {subject_label} and one to "{_strip_package(target_name)}"' ) return M2MFields(source_field=source_field, target_field=target_field) @@ -163,14 +210,14 @@ def derive_m2m_fields( if a is None or b is None: raise M2MDerivationError( f'symmetric junction "{through_name}" for ' - f'"{source.name}.{rel.name}" has a reference with no @fields' + f'"{declaring.name}.{rel.name}" has a reference with no @fields' ) return M2MFields(source_field=a, target_field=b) source_ref_field = rel.source_ref_field() if source_ref_field is None: raise M2MDerivationError( - f'self-join relationship "{source.name}.{rel.name}" through ' + f'self-join relationship "{declaring.name}.{rel.name}" through ' f'"{through_name}" is ambiguous: set @sourceRefField (directed) or ' f"@symmetric (undirected)" ) @@ -182,7 +229,7 @@ def derive_m2m_fields( ) if source_ref is None: raise M2MDerivationError( - f'@sourceRefField "{source_ref_field}" on "{source.name}.{rel.name}" ' + f'@sourceRefField "{source_ref_field}" on "{declaring.name}.{rel.name}" ' f"does not match any identity.reference FK field on junction " f'"{through_name}"' ) @@ -190,7 +237,7 @@ def derive_m2m_fields( target_field = _ref_fk_field(target_ref) if target_ref is not None else None if target_field is None: raise M2MDerivationError( - f'junction "{through_name}" for "{source.name}.{rel.name}" has no ' + f'junction "{through_name}" for "{declaring.name}.{rel.name}" has no ' f"distinct target-side reference" ) return M2MFields(source_field=source_ref_field, target_field=target_field) diff --git a/server/python/tests/unit/test_derive_m2m_declaring_entity.py b/server/python/tests/unit/test_derive_m2m_declaring_entity.py new file mode 100644 index 000000000..052403f3c --- /dev/null +++ b/server/python/tests/unit/test_derive_m2m_declaring_entity.py @@ -0,0 +1,216 @@ +"""Follow-up to #368 — M:N derivation used the VISITING entity, not the DECLARING one. + +``derive_m2m_fields`` classified the self-join, and matched the hetero junction +reference, against the ``source`` entity its CALLER passed. Every caller walks a +RESOLVING accessor — ``resolve_n2m_descriptor`` iterates ``source_entity.children()``, +``m2m_codegen.m2m_relationships`` iterates ``entity.children()`` — and passes the +entity it is iterating, so for a relationship inherited via ``extends`` that is the +INHERITING entity. An inherited self-join then read as hetero and raised; an +inherited hetero whose junction references the BASE raised too. + +The fix accepts BOTH names of the relationship's subject — the declaring entity +(``rel.parent``) and the navigating entity — so the pre-existing shape (junction FK +references the CONCRETE child, pinned by ``test_n2m_resolver_inherited.py``) keeps +working. ``test_inherited_hetero_concrete_junction_reference`` below is the +counter-case that guards that. + +Unlike the #368 loader passes there is no once-per-node ``checked`` set here, so the +defect is not gated on visit order — it is wrong for every inheriting entity in any +order. The fixtures still declare the child BEFORE the base and pin the root object +order, matching the #368 convention. +""" +from __future__ import annotations + +import json + +from metaobjects import load_string +from metaobjects.meta.core.object.meta_object import MetaObject +from metaobjects.meta.core.relationship.derive_m2m_fields import derive_m2m_fields +from metaobjects.meta.core.relationship.meta_relationship import MetaRelationship +from metaobjects.runtime.n2m_resolver import resolve_n2m_descriptor + + +def _entity(name: str, children: list[dict], **extra: object) -> dict: + return {"object.entity": {"name": name, "children": children, **extra}} + + +def _pk(field: str = "id") -> dict: + return {"identity.primary": {"name": field, "@fields": field}} + + +def _ref(name: str, fk: str, target: str) -> dict: + return {"identity.reference": {"name": name, "@fields": fk, "@references": target}} + + +# Node extends NodeBase, which declares a @symmetric self-join onto ITSELF; the +# junction references the BASE. Node is declared FIRST (child before base). +SELF_JOIN = { + "metadata.root": { + "package": "acme", + "children": [ + _entity("Node", [{"field.long": {"name": "id"}}, _pk()], extends="NodeBase"), + _entity( + "NodeBase", + [ + { + "relationship.association": { + "name": "peers", + "@cardinality": "many", + "@objectRef": "NodeBase", + "@through": "NodeLink", + "@symmetric": True, + } + } + ], + **{"@isAbstract": True}, + ), + _entity( + "NodeLink", + [ + {"field.long": {"name": "id"}}, + {"field.long": {"name": "aId"}}, + {"field.long": {"name": "bId"}}, + _pk(), + _ref("aRef", "aId", "NodeBase"), + _ref("bRef", "bId", "NodeBase"), + ], + ), + ], + } +} + +# Article extends ArticleBase; the junction references the BASE. +HETERO_BASE_REF = { + "metadata.root": { + "package": "acme", + "children": [ + _entity("Article", [{"field.long": {"name": "id"}}, _pk()], extends="ArticleBase"), + _entity( + "ArticleBase", + [ + { + "relationship.association": { + "name": "tags", + "@cardinality": "many", + "@objectRef": "Tag", + "@through": "ArticleTag", + } + } + ], + **{"@isAbstract": True}, + ), + _entity("Tag", [{"field.long": {"name": "id"}}, _pk()]), + _entity( + "ArticleTag", + [ + {"field.long": {"name": "id"}}, + {"field.long": {"name": "articleId"}}, + {"field.long": {"name": "tagId"}}, + _pk(), + _ref("aRef", "articleId", "ArticleBase"), + _ref("tRef", "tagId", "Tag"), + ], + ), + ], + } +} + +# The OTHER legitimate shape: the junction references the CONCRETE child. +HETERO_CONCRETE_REF = { + "metadata.root": { + "package": "acme", + "children": [ + _entity("Post", [{"field.long": {"name": "id"}}, _pk()], extends="PostBase"), + _entity( + "PostBase", + [ + { + "relationship.association": { + "name": "tags", + "@cardinality": "many", + "@objectRef": "Tag", + "@through": "PostTag", + } + } + ], + **{"@isAbstract": True}, + ), + _entity("Tag", [{"field.long": {"name": "id"}}, _pk()]), + _entity( + "PostTag", + [ + {"field.long": {"name": "id"}}, + {"field.long": {"name": "postId"}}, + {"field.long": {"name": "tagId"}}, + _pk(), + _ref("pRef", "postId", "Post"), + _ref("tRef", "tagId", "Tag"), + ], + ), + ], + } +} + + +def _index(meta: dict) -> dict[str, MetaObject]: + root = load_string(json.dumps(meta)).root + return {c.name: c for c in root.children() if isinstance(c, MetaObject)} + + +def _rel(entity: MetaObject, name: str) -> MetaRelationship: + for c in entity.children(): + if isinstance(c, MetaRelationship) and c.name == name: + return c + raise AssertionError(f"no relationship {name} on {entity.name}") + + +def test_inherited_self_join_derives_both_fk_sides() -> None: + index = _index(SELF_JOIN) + # Pin the premise: the child is reached before the base it inherits from. + assert list(index) == ["Node", "NodeBase", "NodeLink"] + node = index["Node"] + rel = _rel(node, "peers") + # Genuinely inherited — not one of Node's own children. + assert "peers" not in {c.name for c in node.own_children()} + assert rel.parent is not None and rel.parent.name == "NodeBase" + + fields = derive_m2m_fields(rel, node, index) + assert fields.source_field == "aId" + assert fields.target_field == "bId" + # The declaring entity itself must agree — same node, same answer. + assert derive_m2m_fields(rel, index["NodeBase"], index) == fields + + +def test_inherited_hetero_matches_the_declaring_bases_reference() -> None: + index = _index(HETERO_BASE_REF) + assert list(index) == ["Article", "ArticleBase", "Tag", "ArticleTag"] + fields = derive_m2m_fields(_rel(index["Article"], "tags"), index["Article"], index) + assert fields.source_field == "articleId" + assert fields.target_field == "tagId" + + +def test_inherited_hetero_concrete_junction_reference() -> None: + """Counter-case: accepting ONLY the declaring entity would break this shape.""" + index = _index(HETERO_CONCRETE_REF) + rel = _rel(index["Post"], "tags") + assert rel.parent is not None and rel.parent.name == "PostBase" + fields = derive_m2m_fields(rel, index["Post"], index) + assert fields.source_field == "postId" + assert fields.target_field == "tagId" + + +def test_runtime_resolver_traverses_an_inherited_self_join() -> None: + """The runtime path re-raises the derivation error, so an inherited self-join + was untraversable at run time, not merely dropped from generated code.""" + index = _index(SELF_JOIN) + desc = resolve_n2m_descriptor(index["Node"], "peers", index) + assert desc is not None + assert desc.source_entity_name == "Node" + assert desc.source_field == "aId" + assert desc.target_field == "bId" + assert desc.symmetric is True + +# The Python codegen path (``m2m_codegen.resolve_m2m_descriptors``) calls the same +# SSOT and so is fixed transitively; it is not exercised here because it additionally +# requires a physical ``source.rdb`` on the junction and target, which an abstract +# declaring base does not have — a separate concern from the derivation. From 75f2415c9a5874e30fe8edbd9298774ce3bb854d Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 13 Sep 2026 23:25:31 -0400 Subject: [PATCH 06/12] =?UTF-8?q?fix(csharp):=20M:N=20derivation=20?= =?UTF-8?q?=E2=80=94=20and=20M2MNavigation.IsSelfJoin=20=E2=80=94=20must?= =?UTF-8?q?=20see=20the=20declaring=20entity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C# port of the same fix, plus the consequence that is unique to this port. DeriveM2MFields resolves the declaring entity from rel.Parent and accepts BOTH it and the navigating `source` as the relationship's subject, for the self-join classification and the hetero reference match alike (see the TS/Java commits for why both names are legitimate). M2MNavigation.IsSelfJoin had the identical confusion one layer up: it compared Target against Source only. That never mattered before, because an inherited self-join threw inside the derivation and M2MNavigationBuilder.Build swallowed the exception and dropped the navigation entirely. With derivation fixed, the navigation now reaches DbContextGenerator — which excludes self-joins from the EF UsingEntity wiring — and EntityGenerator, which marks them [NotMapped]. A Source-only comparison would therefore convert a silent DROP into silently WRONG EF configuration. IsSelfJoin now also compares the new DeclaringEntity. Verified by reverting each half separately: reverting the derivation fails 4 of the 5 new tests; reverting only the IsSelfJoin comparison fails the 5th. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- .../M2MInheritedDeclaringEntityTests.cs | 198 ++++++++++++++++++ .../Generators/M2MNavigation.cs | 28 ++- .../Core/Relationship/M2MFields.cs | 54 +++-- 3 files changed, 263 insertions(+), 17 deletions(-) create mode 100644 server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs diff --git a/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs b/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs new file mode 100644 index 000000000..ffd5e470e --- /dev/null +++ b/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs @@ -0,0 +1,198 @@ +// Follow-up to #368 — M:N derivation used the VISITING entity, not the entity that +// DECLARES the relationship. +// +// M2MNavigationBuilder.For walks the RESOLVING Relationships(), so a M:N declared on +// an abstract base is reached again through every entity that extends it, with the +// INHERITING entity as `source`. M2MDerivation then compared @objectRef against that +// entity: an inherited self-join read as hetero, looked for a junction reference to +// the child, found none and threw — and M2MNavigationBuilder.Build CATCHES +// M2MDerivationException and returns null, so the navigation vanished from the +// generated entity, DbContext and routes with no error at all. +// +// Both authoring shapes must work: the junction FK may reference the declaring BASE, +// or (more usually) the CONCRETE child, since the abstract base has no table. The +// counter-case test below guards the second. +// +// The child is declared BEFORE the base so the walk reaches it first — the order +// shape the #368 loader regressions established. (DbContextGenerator additionally +// sorts entities by name, and "Node" < "NodeBase" ordinally, so that path sees the +// child first too.) + +using MetaObjects.Codegen.Generators; +using MetaObjects.Core.Relationship; +using MetaObjects.Loader; +using MetaObjects.Meta; +using Xunit; + +namespace MetaObjects.Codegen.Tests; + +public class M2MInheritedDeclaringEntityTests +{ + // NodeBase declares a @symmetric self-join onto ITSELF; the junction references + // the BASE. Node extends it and is declared first. + private const string SelfJoinModel = """ + { "metadata.root": { "package": "acme", "children": [ + { "object.entity": { "name": "Node", "extends": "NodeBase", "children": [ + { "source.rdb": { "@table": "nodes" } }, + { "field.long": { "name": "id" } }, + { "identity.primary": { "@fields": "id" } } + ]}}, + { "object.entity": { "name": "NodeBase", "@isAbstract": true, "children": [ + { "relationship.association": { "name": "peers", "@cardinality": "many", "@objectRef": "NodeBase", "@through": "NodeLink", "@symmetric": true } } + ]}}, + { "object.entity": { "name": "NodeLink", "children": [ + { "source.rdb": { "@table": "node_links" } }, + { "field.long": { "name": "aId" } }, + { "field.long": { "name": "bId" } }, + { "identity.primary": { "@fields": ["aId", "bId"] } }, + { "identity.reference": { "name": "fkA", "@fields": "aId", "@references": "NodeBase" } }, + { "identity.reference": { "name": "fkB", "@fields": "bId", "@references": "NodeBase" } } + ]}} + ]}} + """; + + // ArticleBase declares a HETERO M:N; the junction references the BASE. + private const string HeteroBaseRefModel = """ + { "metadata.root": { "package": "acme", "children": [ + { "object.entity": { "name": "Article", "extends": "ArticleBase", "children": [ + { "source.rdb": { "@table": "articles" } }, + { "field.long": { "name": "id" } }, + { "identity.primary": { "@fields": "id" } } + ]}}, + { "object.entity": { "name": "ArticleBase", "@isAbstract": true, "children": [ + { "relationship.association": { "name": "tags", "@cardinality": "many", "@objectRef": "Tag", "@through": "ArticleTag" } } + ]}}, + { "object.entity": { "name": "Tag", "children": [ + { "source.rdb": { "@table": "tags" } }, + { "field.long": { "name": "id" } }, + { "identity.primary": { "@fields": "id" } } + ]}}, + { "object.entity": { "name": "ArticleTag", "children": [ + { "source.rdb": { "@table": "article_tags" } }, + { "field.long": { "name": "articleId" } }, + { "field.long": { "name": "tagId" } }, + { "identity.primary": { "@fields": ["articleId", "tagId"] } }, + { "identity.reference": { "name": "fkArticle", "@fields": "articleId", "@references": "ArticleBase" } }, + { "identity.reference": { "name": "fkTag", "@fields": "tagId", "@references": "Tag" } } + ]}} + ]}} + """; + + // The other legitimate shape: the junction references the CONCRETE child. + private const string HeteroConcreteRefModel = """ + { "metadata.root": { "package": "acme", "children": [ + { "object.entity": { "name": "Post", "extends": "PostBase", "children": [ + { "source.rdb": { "@table": "posts" } }, + { "field.long": { "name": "id" } }, + { "identity.primary": { "@fields": "id" } } + ]}}, + { "object.entity": { "name": "PostBase", "@isAbstract": true, "children": [ + { "relationship.association": { "name": "tags", "@cardinality": "many", "@objectRef": "Tag", "@through": "PostTag" } } + ]}}, + { "object.entity": { "name": "Tag", "children": [ + { "source.rdb": { "@table": "tags" } }, + { "field.long": { "name": "id" } }, + { "identity.primary": { "@fields": "id" } } + ]}}, + { "object.entity": { "name": "PostTag", "children": [ + { "source.rdb": { "@table": "post_tags" } }, + { "field.long": { "name": "postId" } }, + { "field.long": { "name": "tagId" } }, + { "identity.primary": { "@fields": ["postId", "tagId"] } }, + { "identity.reference": { "name": "fkPost", "@fields": "postId", "@references": "Post" } }, + { "identity.reference": { "name": "fkTag", "@fields": "tagId", "@references": "Tag" } } + ]}} + ]}} + """; + + private static MetaRoot Load(string model) + { + var r = new MetaDataLoader().Load([new InMemoryStringSource(model, id: "inherited-m2m.json")]); + Assert.Empty(r.Errors); + return r.Root; + } + + private static MetaRelationship Rel(MetaObject entity, string name) => + entity.Relationships().Single(r => r.Name == name); + + [Fact] + public void Derive_inherited_self_join_uses_the_declaring_entity() + { + var root = Load(SelfJoinModel); + // Pin the premise: the child is walked before the base it inherits from. + Assert.Equal(["Node", "NodeBase", "NodeLink"], root.Objects().Select(o => o.Name).ToArray()); + + var node = root.FindObject("Node")!; + var rel = Rel(node, "peers"); + Assert.Empty(node.OwnRelationships()); // genuinely inherited + Assert.Equal("NodeBase", (rel.Parent as MetaObject)!.Name); + + var fields = M2MDerivation.DeriveM2MFields(rel, node, root); + Assert.Equal("aId", fields.SourceField); + Assert.Equal("bId", fields.TargetField); + // The declaring entity itself must agree — same node, same answer. + Assert.Equal(fields, M2MDerivation.DeriveM2MFields(rel, root.FindObject("NodeBase")!, root)); + } + + [Fact] + public void Derive_inherited_hetero_matches_the_bases_junction_reference() + { + var root = Load(HeteroBaseRefModel); + var article = root.FindObject("Article")!; + var fields = M2MDerivation.DeriveM2MFields(Rel(article, "tags"), article, root); + Assert.Equal("articleId", fields.SourceField); + Assert.Equal("tagId", fields.TargetField); + } + + [Fact] + public void Derive_inherited_hetero_matches_a_concrete_junction_reference() + { + // Counter-case: accepting ONLY the declaring entity would break this shape, + // which is the common one (the abstract base has no table). + var root = Load(HeteroConcreteRefModel); + var post = root.FindObject("Post")!; + var rel = Rel(post, "tags"); + Assert.Equal("PostBase", (rel.Parent as MetaObject)!.Name); + var fields = M2MDerivation.DeriveM2MFields(rel, post, root); + Assert.Equal("postId", fields.SourceField); + Assert.Equal("tagId", fields.TargetField); + } + + [Fact] + public void Navigation_builder_emits_the_inherited_self_join_and_flags_it() + { + var root = Load(SelfJoinModel); + var node = root.FindObject("Node")!; + + // Was: Build caught M2MDerivationException and returned null -> empty list. + var nav = Assert.Single(M2MNavigationBuilder.For(node, root)); + Assert.Equal("peers", nav.Name); + Assert.Equal("NodeLink", nav.Junction.Name); + Assert.Equal("aId", nav.SourceField); + Assert.Equal("bId", nav.TargetField); + Assert.True(nav.Symmetric); + + // IsSelfJoin must compare against the DECLARING entity too: Source is the + // inheriting "Node" while Target is "NodeBase". This is exactly the filter + // DbContextGenerator applies — a false here would emit EF UsingEntity wiring + // for a self-join, turning the old silent drop into silently wrong output. + Assert.Equal("Node", nav.Source.Name); + Assert.Equal("NodeBase", nav.Target.Name); + Assert.Equal("NodeBase", nav.DeclaringEntity.Name); + Assert.True(nav.IsSelfJoin); + Assert.DoesNotContain(M2MNavigationBuilder.For(node, root), n => !n.IsSelfJoin); + } + + [Fact] + public void Navigation_builder_emits_the_inherited_hetero_navigation() + { + var root = Load(HeteroBaseRefModel); + var article = root.FindObject("Article")!; + var nav = Assert.Single(M2MNavigationBuilder.For(article, root)); + Assert.Equal("tags", nav.Name); + Assert.Equal("Tag", nav.Target.Name); + Assert.Equal("articleId", nav.SourceField); + Assert.Equal("tagId", nav.TargetField); + Assert.False(nav.IsSelfJoin); + } +} diff --git a/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs b/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs index fee80330e..255592a2e 100644 --- a/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs +++ b/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs @@ -38,9 +38,31 @@ public sealed record M2MNavigation( ///

The navigation member name (the relationship name, e.g. "tags"). public string Name => Relationship.Name; - /// True when the target entity is the source entity (self-join). - public bool IsSelfJoin => ReferenceEquals(Source, Target) || - string.Equals(Source.Name, Target.Name, StringComparison.Ordinal); + /// + /// The entity that DECLARES the relationship. Under extends that is not + /// : the builder walks the RESOLVING Relationships(), so + /// is whichever entity INHERITED the relationship, while + /// Relationship.Parent is the one that declared it. + /// + public MetaObject DeclaringEntity => Relationship.Parent as MetaObject ?? Source; + + /// + /// True when the target entity is the relationship's subject (self-join) — matched + /// against BOTH the navigating and the + /// , the same pair M2MDerivation accepts. + /// + /// Load-bearing: DbContextGenerator excludes self-joins from the EF + /// UsingEntity wiring and EntityGenerator marks their navigation + /// [NotMapped] (it is route-traversed). Before the derivation fix an + /// inherited self-join threw and the whole navigation was dropped, so this never + /// ran on one; comparing only would now turn that silent + /// drop into silently WRONG EF configuration. + /// + public bool IsSelfJoin => + ReferenceEquals(Source, Target) || + string.Equals(Source.Name, Target.Name, StringComparison.Ordinal) || + ReferenceEquals(DeclaringEntity, Target) || + string.Equals(DeclaringEntity.Name, Target.Name, StringComparison.Ordinal); } /// diff --git a/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs b/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs index 00e92baaa..3f786ae3d 100644 --- a/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs +++ b/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs @@ -19,6 +19,19 @@ // two references are taken in declaration order (sourceField = first, // targetField = second). Resolution unions both at read time. // Ambiguous (source == target, neither @sourceRefField nor @symmetric) -> throw. +// +// "source" above means the relationship's SUBJECT, and under `extends` there are two +// legitimate names for it. Every caller walks the RESOLVING Relationships() +// (M2MNavigationBuilder for codegen, M2MResolver at run time) and passes the entity it +// is ITERATING, which for an inherited relationship is not the one that declared it. +// So the DECLARING entity is resolved here from rel.Parent (same shape as the #368 +// loader fix, ValidationPasses' `declaringEntity = rel.Parent ?? obj`), and the passed +// `source` is kept alongside it rather than discarded: a junction FK usually references +// the CONCRETE entity, because the abstract base has no table, while @objectRef on an +// inherited self-join names the base. Both are accepted, for the self-join +// classification and the hetero reference match alike. Not covered: a junction +// reference naming an entity strictly BETWEEN the base and the navigating entity in a +// deeper hierarchy. using MetaObjects.Meta; @@ -62,49 +75,62 @@ private static string StripPackage(string name) /// Derive the source/target junction FK fields for a M:N relationship. /// /// the M:N relationship (carries @objectRef + @through + optional @sourceRefField / @symmetric). - /// the entity declaring . + /// the entity the caller is navigating from. Accepted alongside + /// rel.Parent as a name for the relationship's subject, and used as the + /// declaring entity when has no parent. /// the loaded model root (to find the junction entity). /// /// when the junction is missing/malformed or the self-join is ambiguous. /// public static M2MFields DeriveM2MFields(MetaRelationship rel, MetaObject source, MetaRoot root) { + // The entity that DECLARES `rel` — see the header note. Parent is the owning + // entity for both an own declaration and an inherited one (an unmodified + // inherited child is the SAME node object; an override is a different node + // whose parent is the overriding entity, also correct). + var declaring = rel.Parent as MetaObject ?? source; + // The relationship's subject: either name is valid (see the header note). + var subjectNames = declaring.Name == source.Name + ? new[] { declaring.Name } + : new[] { declaring.Name, source.Name }; + bool IsSubject(string? name) => name is not null && subjectNames.Contains(StripPackage(name)); + var subjectLabel = string.Join(" or ", subjectNames.Select(n => $"\"{n}\"")); + string? throughName = rel.Through; if (throughName is null) { throw new M2MDerivationException( - $"relationship \"{source.Name}.{rel.Name}\" is missing @through (required for M:N derivation)"); + $"relationship \"{declaring.Name}.{rel.Name}\" is missing @through (required for M:N derivation)"); } var junction = root.FindObject(throughName); if (junction is null) { throw new M2MDerivationException( - $"relationship \"{source.Name}.{rel.Name}\" @through \"{throughName}\" does not resolve to an entity"); + $"relationship \"{declaring.Name}.{rel.Name}\" @through \"{throughName}\" does not resolve to an entity"); } string? targetName = rel.ObjectRef; if (targetName is null) { throw new M2MDerivationException( - $"relationship \"{source.Name}.{rel.Name}\" is missing @objectRef (the M:N target)"); + $"relationship \"{declaring.Name}.{rel.Name}\" is missing @objectRef (the M:N target)"); } var refs = junction.ReferenceIdentities(); if (refs.Count != 2) { throw new M2MDerivationException( - $"junction \"{throughName}\" for relationship \"{source.Name}.{rel.Name}\" must declare exactly two " + + $"junction \"{throughName}\" for relationship \"{declaring.Name}.{rel.Name}\" must declare exactly two " + $"identity.reference children (found {refs.Count})"); } - bool isSelfJoin = StripPackage(targetName) == source.Name; + bool isSelfJoin = IsSubject(targetName); if (!isSelfJoin) { // Hetero: match each reference by the entity it resolves to. - var sourceRef = refs.FirstOrDefault( - r => r.TargetEntity is not null && StripPackage(r.TargetEntity) == source.Name); + var sourceRef = refs.FirstOrDefault(r => IsSubject(r.TargetEntity)); var targetRef = refs.FirstOrDefault( r => r.TargetEntity is not null && StripPackage(r.TargetEntity) == StripPackage(targetName)); var sourceField = sourceRef is not null ? RefFkField(sourceRef) : null; @@ -112,8 +138,8 @@ public static M2MFields DeriveM2MFields(MetaRelationship rel, MetaObject source, if (sourceField is null || targetField is null) { throw new M2MDerivationException( - $"junction \"{throughName}\" for relationship \"{source.Name}.{rel.Name}\" must declare one " + - $"identity.reference to \"{source.Name}\" and one to \"{StripPackage(targetName)}\""); + $"junction \"{throughName}\" for relationship \"{declaring.Name}.{rel.Name}\" must declare one " + + $"identity.reference to {subjectLabel} and one to \"{StripPackage(targetName)}\""); } return new M2MFields(sourceField, targetField); } @@ -127,7 +153,7 @@ public static M2MFields DeriveM2MFields(MetaRelationship rel, MetaObject source, if (a is null || b is null) { throw new M2MDerivationException( - $"symmetric junction \"{throughName}\" for \"{source.Name}.{rel.Name}\" has a reference with no @fields"); + $"symmetric junction \"{throughName}\" for \"{declaring.Name}.{rel.Name}\" has a reference with no @fields"); } return new M2MFields(a, b); } @@ -136,7 +162,7 @@ public static M2MFields DeriveM2MFields(MetaRelationship rel, MetaObject source, if (sourceRefField is null) { throw new M2MDerivationException( - $"self-join relationship \"{source.Name}.{rel.Name}\" through \"{throughName}\" is ambiguous: " + + $"self-join relationship \"{declaring.Name}.{rel.Name}\" through \"{throughName}\" is ambiguous: " + "set @sourceRefField (directed) or @symmetric (undirected)"); } @@ -145,7 +171,7 @@ public static M2MFields DeriveM2MFields(MetaRelationship rel, MetaObject source, if (directedSourceRef is null) { throw new M2MDerivationException( - $"@sourceRefField \"{sourceRefField}\" on \"{source.Name}.{rel.Name}\" does not match any " + + $"@sourceRefField \"{sourceRefField}\" on \"{declaring.Name}.{rel.Name}\" does not match any " + $"identity.reference FK field on junction \"{throughName}\""); } var directedTargetRef = refs.FirstOrDefault(r => !ReferenceEquals(r, directedSourceRef)); @@ -153,7 +179,7 @@ public static M2MFields DeriveM2MFields(MetaRelationship rel, MetaObject source, if (directedTargetField is null) { throw new M2MDerivationException( - $"junction \"{throughName}\" for \"{source.Name}.{rel.Name}\" has no distinct target-side reference"); + $"junction \"{throughName}\" for \"{declaring.Name}.{rel.Name}\" has no distinct target-side reference"); } return new M2MFields(sourceRefField, directedTargetField); } From aa31ec46a23b5ddbaab86f00aa3203e110c9d2d9 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 13 Sep 2026 23:29:08 -0400 Subject: [PATCH 07/12] test(codegen-kotlin): pin that Kotlin reaches the fixed M:N derivation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codegen-kotlin needs no fix of its own — KotlinM2mSupport calls the same cross-port SSOT (com.metaobjects.relationship.M2MFields.derive) as codegen-spring and omdb, so it is fixed transitively. This test proves that link rather than assuming it: an inherited symmetric self-join, reached through the concrete child, resolves to the junction's two FK sides. Worth pinning because resolve() does NOT catch M2MDerivationException — unlike the C# builder, Kotlin (and codegen-spring) let it escape, so before the fix an inherited self-join failed the whole generation run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- .../generator/kotlin/KotlinM2mCodegenTest.kt | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/server/java/codegen-kotlin/src/test/kotlin/com/metaobjects/generator/kotlin/KotlinM2mCodegenTest.kt b/server/java/codegen-kotlin/src/test/kotlin/com/metaobjects/generator/kotlin/KotlinM2mCodegenTest.kt index 9487bdab7..6607cc6ce 100644 --- a/server/java/codegen-kotlin/src/test/kotlin/com/metaobjects/generator/kotlin/KotlinM2mCodegenTest.kt +++ b/server/java/codegen-kotlin/src/test/kotlin/com/metaobjects/generator/kotlin/KotlinM2mCodegenTest.kt @@ -1,8 +1,10 @@ package com.metaobjects.generator.kotlin +import com.metaobjects.`object`.MetaObject import com.metaobjects.metadata.ktx.loadString import java.nio.file.Files import kotlin.test.Test +import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -174,4 +176,57 @@ class KotlinM2mCodegenTest { assertFalse("/{id}/" in src && "Query(id)" in src, "junction controller must not emit an M:N traversal endpoint; saw:\n$src") } + + // --- Inherited M:N — the declaring-entity follow-up to #368 --------------------- + // + // codegen-kotlin reaches the SAME cross-port SSOT (com.metaobjects.relationship + // .M2MFields.derive) as codegen-spring and omdb, so it carried the same defect: + // KotlinM2mSupport.resolve walks the RESOLVING `entity.relationships` and passed + // the entity it was iterating, so a M:N declared on an abstract base and reached + // through a concrete child was derived against the CHILD. An inherited self-join + // then read as hetero and threw — and resolve() does NOT catch, so the whole + // Kotlin generation run failed rather than silently dropping the navigation. + // + // This test pins that codegen-kotlin genuinely reaches the fixed helper; the + // derivation logic itself is gated by the Java M2MSlimVocabularyTest. The child + // is declared BEFORE the base, the #368 order convention. + private val inheritedSelfJoinFixture = """{ + "metadata.root": { "package": "acme::graph", "children": [ + { "object.entity": { "name": "Node", "extends": "NodeBase", "children": [ + { "source.rdb": { "@table": "nodes" } }, + { "field.long": { "name": "id" } }, + { "field.string": { "name": "label", "@required": true, "@maxLength": 80 } }, + { "identity.primary": { "@fields": "id", "@generation": "increment" } } + ] } }, + { "object.entity": { "name": "NodeBase", "@isAbstract": true, "children": [ + { "relationship.association": { "name": "peers", "@cardinality": "many", + "@objectRef": "NodeBase", "@through": "NodeLink", "@symmetric": true } } + ] } }, + { "object.entity": { "name": "NodeLink", "children": [ + { "source.rdb": { "@table": "node_links" } }, + { "field.long": { "name": "aId", "@required": true } }, + { "field.long": { "name": "bId", "@required": true } }, + { "identity.primary": { "@fields": ["aId", "bId"] } }, + { "identity.reference": { "name": "fkA", "@fields": "aId", "@references": "NodeBase" } }, + { "identity.reference": { "name": "fkB", "@fields": "bId", "@references": "NodeBase" } } + ] } } + ] } + }""".trimIndent() + + @Test fun inheritedSelfJoinResolvesThroughTheSharedDerivation() { + val loader = loadString("km2m-inherited", inheritedSelfJoinFixture) + val node = loader.root.getChildren(MetaObject::class.java, false) + .first { it.name == "acme::graph::Node" } + // Pin the premise: the child is reached before the base it inherits from. + assertEquals( + listOf("acme::graph::Node", "acme::graph::NodeBase", "acme::graph::NodeLink"), + loader.root.getChildren(MetaObject::class.java, false).map { it.name }, + ) + val navs = KotlinM2mSupport.resolve(node, loader) + assertEquals(1, navs.size, "expected the inherited peers navigation; saw $navs") + assertEquals("peers", navs[0].relationName) + assertEquals("aId", navs[0].sourceField) + assertEquals("bId", navs[0].targetField) + assertTrue(navs[0].symmetric) + } } From a25dbef6f3333960202856c21a0c5a851e20f9bf Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 13 Sep 2026 23:54:48 -0400 Subject: [PATCH 08/12] test: pin the ADR-0041 bare-name divergence the subject widening enlarges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fix round 1, items 4-6. (4) Java's derive() compares RESOLVED object identity (FQN-exact); TS, C# and Python compare stripped short names. The subject set held one short name and now holds two, so the surface on which a bare-name compare can mis-bind is twice as large. Concrete case: `a::NodeBase` declares a genuine cross-package hetero M:N onto `b::NodeBase`, and `a::Node extends a::NodeBase`. Deriving from `a::Node`, the subject short names are {"NodeBase", "Node"} and "b::NodeBase" strips to "NodeBase", so the target reads as the subject and the relationship misreads as an ambiguous self-join. Java gets it right. Measured, not assumed: this model derived CORRECTLY (srcId/dstId) before this branch, so the widening REGRESSED it on the three bare-name ports. Recorded as such rather than filed as a pre-existing divergence. Not fixed here — FQN-exactness on three ports is ADR-0041 work and a separate branch. Instead each of the three now has an "ADR-0041 GAP" test encoding what that port ACTUALLY does today, so the divergence is gated rather than latent and adopting FQN-exactness fails loudly. Java's existing deriveCrossPackageHeteroBindsCorrectPackage gains a cross-port note naming all three. The fixtures use a BARE @through: C#'s FindObject has no FQN fallback (a separate pre-existing gap that would otherwise mask the collision under a different error). (5) M2MNavigationBuilder.For's summary said "own relationships" while it walks the resolving Relationships() — the exact belief that caused the bug. Corrected. (6) The TS, Java and Python derivation headers still opened with "`source` always means the entity that DECLARES the relationship" and contradicted themselves ten lines later. Aligned to the C# header, which was written after the correction. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- .../M2MInheritedDeclaringEntityTests.cs | 61 ++++++++++++++ .../Generators/M2MNavigation.cs | 11 ++- .../metaobjects/relationship/M2MFields.java | 27 +++--- .../relationship/M2MSlimVocabularyTest.java | 8 ++ .../core/relationship/derive_m2m_fields.py | 30 ++++--- .../unit/test_derive_m2m_declaring_entity.py | 83 ++++++++++++++++++- .../core/relationship/derive-m2m-fields.ts | 29 +++---- .../metadata/test/relationship-m2m.test.ts | 55 ++++++++++++ 8 files changed, 252 insertions(+), 52 deletions(-) diff --git a/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs b/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs index ffd5e470e..515e60703 100644 --- a/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs +++ b/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs @@ -158,6 +158,67 @@ public void Derive_inherited_hetero_matches_a_concrete_junction_reference() Assert.Equal("tagId", fields.TargetField); } + // ADR-0041 DIVERGENCE — pinned, not fixed. Java's M2MFields compares RESOLVED object + // identity (FQN-exact); C#, TS and Python compare StripPackage() short names. The + // subject set used to hold one short name and now holds two, so the surface on which + // a bare-name compare can mis-bind is twice as large. Here a::NodeBase declares a + // genuine CROSS-PACKAGE hetero M:N onto b::NodeBase and a::Node extends a::NodeBase: + // deriving from a::Node the subject short names are {"NodeBase", "Node"}, and + // "b::NodeBase" strips to "NodeBase", so the target reads as the subject and the + // relationship is misclassified as an ambiguous self-join. Java's equivalent + // (M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage) gets it right. + // + // Honest about severity: this model derived CORRECTLY before this branch (the + // one-member subject set did not collide), so the widening regressed it. Accepted + // deliberately per the ADR-0041 split. This encodes what C# ACTUALLY does so the + // divergence is gated rather than latent; when C# adopts FQN-exactness this test + // fails and must be rewritten to assert srcId/dstId. + private const string XpkgAModel = """ + { "metadata.root": { "package": "a", "children": [ + { "object.entity": { "name": "Node", "extends": "a::NodeBase", "children": [ + { "field.long": { "name": "id" } }, + { "identity.primary": { "@fields": "id" } } + ]}}, + { "object.entity": { "name": "NodeBase", "@isAbstract": true, "children": [ + { "relationship.association": { "name": "links", "@cardinality": "many", "@objectRef": "b::NodeBase", "@through": "L" } } + ]}}, + { "object.entity": { "name": "L", "children": [ + { "field.long": { "name": "srcId" } }, + { "field.long": { "name": "dstId" } }, + { "identity.primary": { "@fields": ["srcId", "dstId"] } }, + { "identity.reference": { "name": "s", "@fields": "srcId", "@references": "a::Node" } }, + { "identity.reference": { "name": "d", "@fields": "dstId", "@references": "b::NodeBase" } } + ]}} + ]}} + """; + + private const string XpkgBModel = """ + { "metadata.root": { "package": "b", "children": [ + { "object.entity": { "name": "NodeBase", "children": [ + { "field.long": { "name": "id" } }, + { "identity.primary": { "@fields": "id" } } + ]}} + ]}} + """; + + [Fact] + public void Adr0041_gap_cross_package_target_sharing_a_subject_short_name() + { + var r = new MetaDataLoader().Load([ + new InMemoryStringSource(XpkgAModel, id: "a.json"), + new InMemoryStringSource(XpkgBModel, id: "b.json"), + ]); + // The model itself is perfectly legal — the loader raises nothing. + Assert.Empty(r.Errors); + var node = r.Root.Objects().First(o => o.Name == "Node"); + var rel = Rel(node, "links"); + + // CURRENT C# BEHAVIOUR (wrong; Java derives srcId/dstId here): + var ex = Assert.Throws( + () => M2MDerivation.DeriveM2MFields(rel, node, r.Root)); + Assert.Contains("is ambiguous", ex.Message, StringComparison.Ordinal); + } + [Fact] public void Navigation_builder_emits_the_inherited_self_join_and_flags_it() { diff --git a/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs b/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs index 255592a2e..c299f5be9 100644 --- a/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs +++ b/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs @@ -73,10 +73,13 @@ public sealed record M2MNavigation( public static class M2MNavigationBuilder { /// - /// All M:N navigations declared on (own relationships - /// with @cardinality: "many" + @through). Returns an empty list for - /// an entity with no M:N relationships. A relationship whose junction FK columns - /// cannot be derived is skipped (the loader validation surfaces the error). + /// All M:N navigations VISIBLE on — every relationship in + /// its EFFECTIVE view (Relationships() is resolving: own + inherited via + /// extends) carrying @cardinality: "many" + @through. NOT own-only: + /// believing otherwise is what produced the declaring-vs-visiting bug this class's + /// now guards. Returns an empty list for an + /// entity with no M:N relationships. A relationship whose junction FK columns cannot be + /// derived is skipped (the loader validation surfaces the error). /// public static IReadOnlyList For(MetaObject entity, MetaRoot root) { diff --git a/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java b/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java index 6f25db3ed..0ec2a7d7f 100644 --- a/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java +++ b/server/java/metadata/src/main/java/com/metaobjects/relationship/M2MFields.java @@ -46,20 +46,19 @@ * * Ambiguous (source == target, neither {@code @sourceRefField} nor {@code @symmetric}) → throw. * - *

"source" above always means the entity that DECLARES the relationship — never - * whichever entity's effective view reached it. Every caller walks the RESOLVING - * {@code getRelationships()}, so for a relationship inherited via {@code extends} the - * entity it is iterating is the INHERITING one, and both the self-join classification - * and the hetero reference match would then be made against the wrong entity (an - * inherited self-join reads as hetero and derivation throws; an inherited hetero finds - * no junction reference to the inheriting entity and throws too). The declaring entity - * is resolved HERE, from {@code rel.getParent()}, rather than asked of each caller — - * same shape as the #368 loader fix in {@code ValidationPhase}, and for the same reason: - * the answer must not depend on who asked. The passed {@code source} is NOT discarded: - * under {@code extends} the declaring base and the navigating entity are two legitimate - * names for the relationship's subject (a junction FK usually references the concrete - * entity, which is the one with a table), so BOTH are accepted. It is also the fallback - * when {@code rel} has no entity parent, which keeps the signature unchanged.

+ *

"source" above means the relationship's SUBJECT, and under {@code extends} there are + * two legitimate names for it. Every caller walks the RESOLVING {@code getRelationships()} + * (SpringM2mSupport and KotlinM2mSupport for codegen, omdb's M2MResolver at run time) and + * passes the entity it is ITERATING, which for an inherited relationship is not the one + * that declared it. So the DECLARING entity is resolved here from {@code rel.getParent()} + * (same shape as the #368 loader fix in {@code ValidationPhase}), and the passed + * {@code source} is kept alongside it rather than discarded: a junction FK usually + * references the CONCRETE entity, because the abstract base has no table, while + * {@code @objectRef} on an inherited self-join names the base. Both are accepted, for the + * self-join classification and the hetero reference match alike. {@code source} is also the + * fallback when {@code rel} has no entity parent, which keeps the signature unchanged. Not + * covered: a junction reference naming an entity strictly BETWEEN the base and the + * navigating entity in a deeper hierarchy.

* *

This carries the same semantics as the loader-phase M:N validation * ({@code ValidationPhase.validateRelationshipsM2M}); the validation pass guarantees a diff --git a/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java b/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java index 8b0db16d2..1e465c363 100644 --- a/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java +++ b/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java @@ -403,6 +403,14 @@ private static MetaRelationship relOf(MetaObject obj, String relName) { + " { \"identity.reference\": { \"name\": \"partnerRef\", \"@fields\": \"partnerId\", \"@references\": \"xpkg::partner::Account\" } } ] } }" + "] } }"; + // CROSS-PORT NOTE: Java is the only port whose derive() resolves @objectRef and the + // junction references to OBJECTS and compares FQN identity. TS, C# and Python compare + // stripped short names, so the shape below — a cross-package hetero target whose bare + // name collides with the subject's — misreads as an ambiguous self-join there. That + // divergence is now PINNED on all three (relationship-m2m.test.ts, + // M2MInheritedDeclaringEntityTests.cs, test_derive_m2m_declaring_entity.py, each + // named "ADR-0041 GAP"), so adopting FQN-exactness elsewhere fails those tests loudly + // instead of silently changing behaviour. This test is the correct-behaviour side. @Test public void deriveCrossPackageHeteroBindsCorrectPackage() { // ADR-0041: same-bare-name entities/junctions in different packages. Under the diff --git a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py index 5cfc228a3..8dca41987 100644 --- a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py +++ b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py @@ -17,22 +17,20 @@ target_field = second). Resolution unions both at read time. Ambiguous (source == target, neither @sourceRefField nor @symmetric) → raise. -"source" above always means the entity that DECLARES the relationship — never -whichever entity's effective view reached it. Every caller walks the RESOLVING -``children()`` / ``m2m_relationships()``, so for a relationship inherited via -``extends`` the entity it is iterating is the INHERITING one, and both the -self-join classification and the hetero reference match would then be made -against the wrong entity (an inherited self-join reads as hetero and derivation -raises; an inherited hetero finds no junction reference to the inheriting entity -and raises too). The declaring entity is resolved HERE, from ``rel.parent``, -rather than asked of each caller — same shape as the #368 loader fix -(``declaring_entity = rel.parent if rel.parent is not None else obj`` in -``validation_passes.py``), and for the same reason: the answer must not depend on -who asked. The passed ``source`` is NOT discarded: under ``extends`` the declaring -base and the navigating entity are two legitimate names for the relationship's -subject (a junction FK usually references the concrete entity, which is the one -with a table), so BOTH are accepted. It is also the fallback when ``rel`` has no -object parent, which keeps the signature unchanged. +"source" above means the relationship's SUBJECT, and under ``extends`` there are two +legitimate names for it. Every caller walks a RESOLVING accessor — +``resolve_n2m_descriptor`` over ``children()``, ``m2m_codegen.m2m_relationships`` over +``entity.children()`` — and passes the entity it is ITERATING, which for an inherited +relationship is not the one that declared it. So the DECLARING entity is resolved here +from ``rel.parent`` (same shape as the #368 loader fix, ``validation_passes``' +``declaring_entity = rel.parent if rel.parent is not None else obj``), and the passed +``source`` is kept alongside it rather than discarded: a junction FK usually references +the CONCRETE entity, because the abstract base has no table, while ``@objectRef`` on an +inherited self-join names the base. Both are accepted, for the self-join classification +and the hetero reference match alike. ``source`` is also the fallback when ``rel`` has no +object parent, which keeps the signature unchanged. Not covered: a junction reference +naming an entity strictly BETWEEN the base and the navigating entity in a deeper +hierarchy. """ from __future__ import annotations diff --git a/server/python/tests/unit/test_derive_m2m_declaring_entity.py b/server/python/tests/unit/test_derive_m2m_declaring_entity.py index 052403f3c..abea68570 100644 --- a/server/python/tests/unit/test_derive_m2m_declaring_entity.py +++ b/server/python/tests/unit/test_derive_m2m_declaring_entity.py @@ -23,9 +23,14 @@ import json -from metaobjects import load_string +import pytest + +from metaobjects import InMemoryStringSource, MetaDataLoader, load_string from metaobjects.meta.core.object.meta_object import MetaObject -from metaobjects.meta.core.relationship.derive_m2m_fields import derive_m2m_fields +from metaobjects.meta.core.relationship.derive_m2m_fields import ( + M2MDerivationError, + derive_m2m_fields, +) from metaobjects.meta.core.relationship.meta_relationship import MetaRelationship from metaobjects.runtime.n2m_resolver import resolve_n2m_descriptor @@ -214,3 +219,77 @@ def test_runtime_resolver_traverses_an_inherited_self_join() -> None: # SSOT and so is fixed transitively; it is not exercised here because it additionally # requires a physical ``source.rdb`` on the junction and target, which an abstract # declaring base does not have — a separate concern from the derivation. + + +# ADR-0041 DIVERGENCE — pinned, not fixed. Java's M2MFields compares RESOLVED object +# identity (FQN-exact); Python, TS and C# compare stripped short names. The subject set +# used to hold one short name and now holds two, so the surface on which a bare-name +# compare can mis-bind is twice as large. Here ``a::NodeBase`` declares a genuine +# CROSS-PACKAGE hetero M:N onto ``b::NodeBase`` and ``a::Node`` extends ``a::NodeBase``: +# deriving from ``a::Node`` the subject short names are {"NodeBase", "Node"}, and +# ``b::NodeBase`` strips to "NodeBase", so the target reads as the subject and the +# relationship is misclassified as an ambiguous self-join. Java's equivalent +# (M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage) gets it right. +# +# Honest about severity: this model derived CORRECTLY before this branch (the one-member +# subject set did not collide), so the widening regressed it. Accepted deliberately per +# the ADR-0041 split; the test encodes what Python ACTUALLY does so the divergence is +# gated rather than latent, and must be rewritten when Python adopts FQN-exactness. +XPKG_A = { + "metadata.root": { + "package": "a", + "children": [ + _entity("Node", [{"field.long": {"name": "id"}}, _pk()], extends="a::NodeBase"), + _entity( + "NodeBase", + [ + { + "relationship.association": { + "name": "links", + "@cardinality": "many", + "@objectRef": "b::NodeBase", + "@through": "L", + } + } + ], + **{"@isAbstract": True}, + ), + _entity( + "L", + [ + {"field.long": {"name": "srcId"}}, + {"field.long": {"name": "dstId"}}, + {"identity.primary": {"name": "id", "@fields": ["srcId", "dstId"]}}, + _ref("s", "srcId", "a::Node"), + _ref("d", "dstId", "b::NodeBase"), + ], + ), + ], + } +} + +XPKG_B = { + "metadata.root": { + "package": "b", + "children": [_entity("NodeBase", [{"field.long": {"name": "id"}}, _pk()])], + } +} + + +def test_adr0041_gap_cross_package_target_sharing_a_subject_short_name() -> None: + """CURRENT Python behaviour (wrong; Java derives srcId/dstId here).""" + result = MetaDataLoader().load([ + InMemoryStringSource(json.dumps(XPKG_A), id="a.json"), + InMemoryStringSource(json.dumps(XPKG_B), id="b.json"), + ]) + # The model itself is perfectly legal — the loader raises nothing. + assert result.errors == [] + root = result.root + objects = [c for c in root.children() if isinstance(c, MetaObject)] + # Pin the premise: the child is reached before the base it inherits from. + assert [o.name for o in objects] == ["Node", "NodeBase", "L", "NodeBase"] + index = {o.name: o for o in objects} + node = objects[0] + rel = _rel(node, "links") + with pytest.raises(M2MDerivationError, match="is ambiguous"): + derive_m2m_fields(rel, node, index) diff --git a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts index b2148c656..1075e25ec 100644 --- a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts +++ b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts @@ -18,22 +18,19 @@ // targetField = second). Resolution unions both at read time. // Ambiguous (source == target, neither @sourceRefField nor @symmetric) → throw. // -// "source" above always means the entity that DECLARES the relationship — never -// whichever entity's effective view reached it. Every caller walks the RESOLVING -// `obj.relationships()`, so for a relationship inherited via `extends` the entity -// it is iterating is the INHERITING one, and both the self-join classification and -// the hetero reference match would then be made against the wrong entity (an -// inherited self-join reads as hetero and derivation throws; an inherited hetero -// finds no junction reference to the inheriting entity and throws too). The -// declaring entity is resolved HERE, from `rel.parent`, rather than asked of each -// caller — same shape as the #368 loader fix (`declaringEntity = rel.parent ?? obj` -// in validation-passes.ts), and for the same reason: the answer must not depend on -// who asked. The passed `source` is NOT discarded: under `extends` the declaring -// base and the navigating entity are two legitimate names for the relationship's -// subject (a junction FK usually references the concrete entity, which is the one -// with a table; @objectRef on an inherited self-join names the base), so both are -// accepted — see `subjectNames` below. It is also the fallback when `rel` has no -// entity parent, which keeps the exported signature unchanged. +// "source" above means the relationship's SUBJECT, and under `extends` there are two +// legitimate names for it. Every caller walks the RESOLVING `obj.relationships()` +// (codegen-ts's relation-resolver, runtime-ts's n2m-resolver, docs-site's link-graph) +// and passes the entity it is ITERATING, which for an inherited relationship is not +// the one that declared it. So the DECLARING entity is resolved here from `rel.parent` +// (same shape as the #368 loader fix, validation-passes' `declaringEntity = rel.parent +// ?? obj`), and the passed `source` is kept alongside it rather than discarded: a +// junction FK usually references the CONCRETE entity, because the abstract base has no +// table, while @objectRef on an inherited self-join names the base. Both are accepted, +// for the self-join classification and the hetero reference match alike. `source` is +// also the fallback when `rel` has no entity parent, which keeps the exported signature +// unchanged. Not covered: a junction reference naming an entity strictly BETWEEN the +// base and the navigating entity in a deeper hierarchy. import type { MetaObject } from "../object/meta-object.js"; import type { MetaRoot } from "../../shared/meta-root.js"; diff --git a/server/typescript/packages/metadata/test/relationship-m2m.test.ts b/server/typescript/packages/metadata/test/relationship-m2m.test.ts index a649d006e..e42104ac1 100644 --- a/server/typescript/packages/metadata/test/relationship-m2m.test.ts +++ b/server/typescript/packages/metadata/test/relationship-m2m.test.ts @@ -852,6 +852,61 @@ describe("FR-017 deriveM2MFields uses the DECLARING entity, not the visiting one expect(derived.targetField).toBe("tagId"); }); + // ADR-0041 DIVERGENCE — pinned, not fixed. Java's M2MFields compares RESOLVED + // object identity (FQN-exact); TS, C# and Python compare stripPackage() short + // names. The subject set used to hold one short name and now holds two, so the + // surface on which a bare-name compare can mis-bind is twice as large. This + // fixture is the concrete case: a::NodeBase declares a genuine CROSS-PACKAGE + // hetero M:N onto b::NodeBase, and a::Node extends a::NodeBase. Deriving from + // a::Node, the subject short names are {"NodeBase", "Node"}; stripPackage + // ("b::NodeBase") is "NodeBase", which is IN that set, so the target reads as + // the subject and the relationship is misclassified as an ambiguous self-join. + // + // Java's equivalent (M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage) + // gets this right. This test encodes what TS ACTUALLY does today, not what it + // ought to do, so the divergence is gated rather than latent: the day TS adopts + // FQN-exact resolution (ADR-0041 work, a separate branch), this test fails and + // must be rewritten to assert srcId/dstId. + // + // Honest about severity: this model DERIVED CORRECTLY before this branch + // (the one-member subject set {"Node"} did not collide), so the widening + // regressed it. Accepted deliberately, per the ADR-0041 split. + test("ADR-0041 GAP: a cross-package hetero target sharing a subject short name misreads as a self-join", async () => { + const aDoc = { "metadata.root": { package: "a", children: [ + { "object.entity": { name: "Node", "extends": "a::NodeBase", children: [ + { "field.long": { name: "id" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + { "object.entity": { name: "NodeBase", "@isAbstract": true, children: [ + { "relationship.association": { name: "links", "@cardinality": "many", + "@objectRef": "b::NodeBase", "@through": "L" } } ] } }, + { "object.entity": { name: "L", children: [ + { "field.long": { name: "srcId" } }, + { "field.long": { name: "dstId" } }, + { "identity.primary": { "name": "id", "@fields": ["srcId", "dstId"] } }, + { "identity.reference": { name: "s", "@fields": ["srcId"], "@references": "a::Node" } }, + { "identity.reference": { name: "d", "@fields": ["dstId"], "@references": "b::NodeBase" } } ] } }, + ] } }; + const bDoc = { "metadata.root": { package: "b", children: [ + { "object.entity": { name: "NodeBase", children: [ + { "field.long": { name: "id" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + ] } }; + const { root, errors } = await new MetaDataLoader().load([ + new InMemoryStringSource(JSON.stringify(aDoc), { id: "a.json" }), + new InMemoryStringSource(JSON.stringify(bDoc), { id: "b.json" }), + ]); + // The model itself is perfectly legal — the loader raises nothing. + expect(errors).toHaveLength(0); + expect(objectVisitOrder(root)).toEqual(["a::Node", "a::NodeBase", "a::L", "b::NodeBase"]); + + const node = findObj(root, "Node"); + const rel = node.relationships().find((r) => r.name === "links") as MetaRelationship; + // CURRENT TS BEHAVIOUR (wrong; Java derives srcId/dstId here): + expect(() => deriveM2MFields(rel, node, root)).toThrow( + /is ambiguous: set @sourceRefField \(directed\) or @symmetric \(undirected\)/, + ); + }); + test("an OWN relationship still derives against its own entity (no regression)", async () => { // The override case: Sub re-declares `peers` itself, so rel.parent IS Sub and // the self-join must be classified against Sub, not the base it shadows. From de477a5418da6d59d2916530d739b1bd9a49a162 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 13 Sep 2026 23:57:01 -0400 Subject: [PATCH 09/12] docs: record the inherited-M:N junction-FK contract and the fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fix round 1, item 3. The branch changed behaviour across five ports and touched no documentation, while the #368 base branch updated four doc surfaces for the equivalent change. docs/features/relationships.md: * the Attributes table was missing the entire M:N vocabulary — @through, @sourceRefField's M:N reading, and @symmetric now have rows; * a new "Inheriting an M:N relationship through `extends`" section states the authoring contract this fix establishes, which until now existed only in source comments: for an inherited M:N the junction FK may reference EITHER the declaring base OR the concrete child, and ONLY those two — an entity lying strictly between them in a deeper hierarchy is not accepted; * the known ADR-0041 bare-name gap is disclosed there rather than left to be rediscovered; * "Verified by" now says why the derivation is NOT in fixtures/conformance/ (that corpus round-trips the serializer, which never surfaces which junction column was picked — two models that derive differently serialize identically) and names the per-port tests that gate it instead. CHANGELOG.md gains an [Unreleased] → Fixed entry covering all five ports, the C#-only IsSelfJoin consequence, and — stated plainly rather than glossed — the two shapes that regress: the ambiguous inherited self-join that is now correctly refused (and fails the run outright on the ports that do not catch), and the cross-package short-name collision that regresses on TS/C#/Python only. Both regressions were reproduced before being written down, not inferred: the first derives (childFk, baseFk) from the child and throws from the base pre-fix, and throws consistently post-fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- CHANGELOG.md | 52 ++++++++++++++++++++++ docs/features/relationships.md | 79 ++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b961a208c..9dd13adba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,58 @@ here.** ## [Unreleased] +### Fixed + +- **An M:N relationship inherited through `extends` derived its junction FK columns + against the wrong entity — and in codegen the failure was silent.** The derivation + classified the self-join, and matched the junction's source-side + `identity.reference`, against the entity the CALLER was iterating. Every caller walks + a resolving relationship accessor, so for a relationship declared on a base and + reached through a subclass that is the INHERITING entity, not the one that declared + it. Two failures followed: an inherited self-join compared `@objectRef` (the base) + against the child, read as hetero, looked for a junction reference to the child and + found none; and an inherited hetero whose junction references the base found nothing + either. TypeScript codegen and the docs-site link graph catch the resulting error and + return `null`, so the navigation was **dropped from the generated output with no + error at all**; C# codegen did the same; the TypeScript, Java, Kotlin and Python + runtime and codegen paths let it escape, so the traversal or the generation run + failed outright. + + The declaring entity now comes from the relationship's own parent — the same shape as + [#368](https://github.com/metaobjectsdev/metaobjects/issues/368)'s loader fix — in all + four derivations (TypeScript, Java, C#, Python; Kotlin calls the Java helper). The + entity being navigated from is kept alongside it rather than discarded: under + inheritance both are legitimate names for the relationship's subject, because a + junction FK usually references the concrete child while `@objectRef` on a hoisted + self-join names the base. The authoring contract this establishes — **the junction FK + may reference either the declaring base or the concrete child, and only those two** — + is now written down in + [`docs/features/relationships.md`](docs/features/relationships.md). + + C# additionally fixes `M2MNavigation.IsSelfJoin`, which had the same confusion one + layer up. It never ran on an inherited self-join before (the derivation threw first), + and `DbContextGenerator` uses it to decide whether to emit EF `UsingEntity` wiring — + so fixing only the derivation would have turned a silent drop into silently wrong EF + configuration. + + **Not a pure widening.** One shape that derived before now refuses: a base declaring + `@objectRef: ` + `@through` with neither `@symmetric` nor `@sourceRefField`, + reached through a subclass, used to be misread as hetero and returned an arbitrary FK + direction; it is now correctly recognised as an ambiguous self-join and refused. That + model was already broken — deriving the same relationship from the base itself threw — + so codegen emitted for the child and dropped it for the base. The refusal is the + correct behaviour, but on Java, Kotlin and Python, whose callers do not catch, it + moves from "generates wrongly" to "the generation run fails", and the fix is to add + `@symmetric` or `@sourceRefField`. A second shape regresses on TypeScript, C# and + Python only: a cross-package hetero M:N whose target's short name collides with the + subject's now misreads as a self-join, because those three compare bare names where + Java compares resolved package-qualified identity. Each port pins its current + behaviour in a test; closing it is + [ADR-0041](spec/decisions/ADR-0041-cross-package-reference-resolution.md) work. + + No vocabulary change: `metamodelVersion` stays `1.0` and the registry manifest is + untouched. + ### Changed - **Codegen is OPT-IN: no port ships a default generator suite** (ADR-0034 Amendment 2). diff --git a/docs/features/relationships.md b/docs/features/relationships.md index b4ee52e8a..0b142461c 100644 --- a/docs/features/relationships.md +++ b/docs/features/relationships.md @@ -102,6 +102,9 @@ metadata: | `@cardinality` | `relationship.composition` | `one` / `many` | Multiplicity on the target side | | `@fields` | `identity.reference` | One field name or array | The FK column(s) on this entity | | `@references` | `identity.reference` | Entity name | The target entity (PK on the other side) | +| `@through` | `relationship.*` | Junction entity name | Makes the relationship M:N. With `@cardinality: many`, names the junction entity whose two `identity.reference` children the FK columns are DERIVED from — the relationship never restates them. | +| `@sourceRefField` | `relationship.*` | FK field name | On an M:N, names the source-side FK field on the junction (a DIRECTED self-join). On a `@cardinality: one` relationship, picks which of several `identity.reference` nodes onto the same target it navigates (see below). Mutually exclusive with `@symmetric`. | +| `@symmetric` | `relationship.*` | `true` | Marks an UNDIRECTED M:N self-join (union-on-read). Valid only when `@objectRef` is the relationship's own subject. Mutually exclusive with `@sourceRefField`. | | `@onDelete` | `relationship.*` and `identity.reference` | `cascade` / `set-null` / `restrict` / `no-action` | RDB referential action. Default derives from the relationship subtype: composition -> `cascade`, aggregation -> `set-null`, association -> `restrict`. | | `@onUpdate` | `relationship.*` and `identity.reference` | same as `@onDelete` (default `cascade` when a relationship correlates) | RDB referential action | @@ -240,6 +243,71 @@ See [ADR-0029](../../spec/decisions/ADR-0029-entity-child-extends-and-via-infere Amendment 1 for the full ladder specification, including why suffix-stripping applies to candidates only. +## Inheriting an M:N relationship through `extends` + +An M:N relationship declared on an abstract base is visible on every entity that +`extends` it — relationship accessors are RESOLVING, so `Post` sees the `tags` +relationship its `PostBase` declared. The junction's two `identity.reference` +children are what give the FK direction, and under inheritance there are two +defensible entities for the source-side reference to name: + +- the **declaring base** (`PostBase`) — the entity the relationship is written on, and + what `@objectRef` names for a self-join hoisted onto a base; or +- the **concrete child** (`Post`) — usually what the FK actually references, because + an abstract base has no table for a foreign key to point at. + +**Both are accepted, and only those two.** The FK derivation treats the declaring +entity and the entity you are navigating from as the relationship's *subject*: the +source-side junction reference may name either, and `@objectRef` naming either makes +the relationship a self-join. Nothing else counts — in particular an entity lying +strictly *between* the declaring base and the navigating entity in a deeper hierarchy +is **not** accepted, and a junction reference naming one fails derivation with +`ERR_INVALID_RELATIONSHIP`. + +```yaml +# PostBase (abstract) declares the M:N; Post extends it. The junction may reference +# EITHER PostBase or Post — both derive postId/tagId for Post.tags. +- object.entity: + name: PostBase + isAbstract: true + children: + - relationship.association: + name: tags + objectRef: Tag + cardinality: many + through: PostTag +- object.entity: + name: Post + extends: PostBase +- object.entity: + name: PostTag + children: + - identity.reference: + name: fkPost + fields: postId + references: Post # or PostBase — either resolves + - identity.reference: + name: fkTag + fields: tagId + references: Tag +``` + +The same rule governs an inherited **self-join**: a base declaring +`@objectRef: ` with `@symmetric` or `@sourceRefField` derives the same two FK +sides whichever subclass you reach it through. The derivation's answer never depends +on which entity's effective view got there first — that independence is the point, and +it is what +[#368](https://github.com/metaobjectsdev/metaobjects/issues/368)'s loader fix +established for validation and this rule extends to FK derivation. + +**Known gap, documented rather than fixed:** the Java port compares the relationship's +subject by RESOLVED package-qualified identity; TypeScript, C# and Python compare bare +short names. So a genuine cross-package hetero M:N whose target's short name collides +with the subject's — `a::NodeBase` relating to `b::NodeBase` — is misread as a +self-join on those three ports and fails derivation. Each port carries a test pinning +its current behaviour; closing it is +[ADR-0041](../../spec/decisions/ADR-0041-cross-package-reference-resolution.md) work. + ## What each port generates ### TypeScript @@ -372,6 +440,17 @@ Cross-port runner coverage: TS / Java / Kotlin / C# / Python all execute these via their respective conformance runners. See [`docs/CONFORMANCE.md`](../CONFORMANCE.md) for the per-port pass/skip ledger. +**Not fixture-gated, and why.** The M:N junction-FK DERIVATION — including the +inherited-relationship rule above — cannot be expressed in `fixtures/conformance/`: +that corpus is a load→canonical-serialize round-trip, and the serializer preserves the +declared `@objectRef` / `@through` / `@sourceRefField` strings without ever surfacing +which junction column the derivation picked. Two models that derive differently +serialize identically. It is gated instead by per-port unit tests over the shared +derivation helper (`relationship-m2m.test.ts`, `M2MSlimVocabularyTest.java`, +`M2MInheritedDeclaringEntityTests.cs`, `test_derive_m2m_declaring_entity.py`, +`KotlinM2mCodegenTest.kt`), which is the same call the M:N FQN-collision cases already +made. + ## See also - [entities.md](entities.md) — host node `object.entity` From c6f0cee763fd2424b964282156c9e2413e96e1f1 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Mon, 14 Sep 2026 00:34:31 -0400 Subject: [PATCH 10/12] fix: compare the M:N subject by resolved entity identity, matching the Java port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fix round 2 — reversing round 1's item 4. Round 1 pinned this as an ADR-0041 divergence; measuring it showed it was a REGRESSION this branch introduced, so it is fixed rather than recorded. `a::NodeBase` declares a genuine cross-package hetero M:N onto `b::NodeBase`, and `a::Node extends a::NodeBase`. Deriving from `a::Node` the subject is {a::NodeBase, a::Node}; TS, C# and Python compared package-STRIPPED names, so "b::NodeBase" stripped to "NodeBase", landed in the subject set, and the relationship refused to derive as an ambiguous self-join. It derived correctly (srcId/dstId) before the subject set grew from one name to two. Java never had the collision: its derive() resolves @objectRef and each junction reference to an OBJECT and compares identity. The three ports now do the same, via a local FindEntity/findEntity/_find_entity mirroring Java's private M2MFields.findObject one-for-one — fully-qualified names resolve exactly on the package-folded key, bare names match a short name first-match-wins (the bare collision stays issue #174, as in Java), and an unresolvable @objectRef keeps the defensive bare fallback Java also has. This REDUCES cross-port divergence; it is not an ADR-0041 sweep. Scoped to the subject predicate alone — the hetero TARGET-side reference match, @through resolution and every other name comparison are untouched. No new resolution semantics were added anywhere shared: each port's helper is private to its derivation, exactly as Java's is. In particular C#'s root.FindObject was NOT extended — it still has no FQN fallback, which remains an open question for a separate decision. The three round-1 pinning tests are now regression tests asserting the correct derivation, the same thing Java's deriveCrossPackageHeteroBindsCorrectPackage asserts. Each was confirmed failing against the bare-name code and passing after. Java's test gains a note saying it is the reference the other three follow. docs/features/relationships.md and the CHANGELOG entry described the divergence as a known limitation; both now describe the fixed behaviour, and the CHANGELOG records the divergence reduction. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- CHANGELOG.md | 17 ++-- docs/features/relationships.md | 16 ++-- .../M2MInheritedDeclaringEntityTests.cs | 38 ++++----- .../Core/Relationship/M2MFields.cs | 48 ++++++++++- .../relationship/M2MSlimVocabularyTest.java | 17 ++-- .../core/relationship/derive_m2m_fields.py | 83 ++++++++++++++++++- .../unit/test_derive_m2m_declaring_entity.py | 42 +++++----- .../core/relationship/derive-m2m-fields.ts | 51 ++++++++++-- .../metadata/test/relationship-m2m.test.ts | 41 +++++---- 9 files changed, 256 insertions(+), 97 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9dd13adba..36e5696e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,12 +52,17 @@ here.** so codegen emitted for the child and dropped it for the base. The refusal is the correct behaviour, but on Java, Kotlin and Python, whose callers do not catch, it moves from "generates wrongly" to "the generation run fails", and the fix is to add - `@symmetric` or `@sourceRefField`. A second shape regresses on TypeScript, C# and - Python only: a cross-package hetero M:N whose target's short name collides with the - subject's now misreads as a self-join, because those three compare bare names where - Java compares resolved package-qualified identity. Each port pins its current - behaviour in a test; closing it is - [ADR-0041](spec/decisions/ADR-0041-cross-package-reference-resolution.md) work. + `@symmetric` or `@sourceRefField`. That is the only behaviour this change takes away. + + **Cross-port divergence goes DOWN, not up.** Deciding whether `@objectRef` names the + relationship's subject now resolves the name to an ENTITY and compares identity in all + four derivations, which is what the Java port already did. TypeScript, C# and Python + had been comparing package-stripped short names, so a genuine cross-package hetero M:N + onto a target whose short name matched the subject's (`a::NodeBase` relating to + `b::NodeBase`) was misread as a self-join on those three — a regression the two-name + subject introduced, caught in review and fixed here rather than documented. Scoped to + that one predicate: no other name comparison changed, and this is not a general + [ADR-0041](spec/decisions/ADR-0041-cross-package-reference-resolution.md) sweep. No vocabulary change: `metamodelVersion` stays `1.0` and the registry manifest is untouched. diff --git a/docs/features/relationships.md b/docs/features/relationships.md index 0b142461c..a5c87dbd9 100644 --- a/docs/features/relationships.md +++ b/docs/features/relationships.md @@ -300,13 +300,15 @@ it is what [#368](https://github.com/metaobjectsdev/metaobjects/issues/368)'s loader fix established for validation and this rule extends to FK derivation. -**Known gap, documented rather than fixed:** the Java port compares the relationship's -subject by RESOLVED package-qualified identity; TypeScript, C# and Python compare bare -short names. So a genuine cross-package hetero M:N whose target's short name collides -with the subject's — `a::NodeBase` relating to `b::NodeBase` — is misread as a -self-join on those three ports and fails derivation. Each port carries a test pinning -its current behaviour; closing it is -[ADR-0041](../../spec/decisions/ADR-0041-cross-package-reference-resolution.md) work. +**Cross-package targets are safe.** All five ports decide "is `@objectRef` the subject?" +by resolving the name to an ENTITY and comparing identity, not by comparing bare short +names — so a genuine cross-package hetero M:N whose target's short name happens to match +the subject's (`a::NodeBase` relating to `b::NodeBase`) stays hetero and derives, rather +than being misread as a self-join. A package-qualified name resolves exactly +([ADR-0041](../../spec/decisions/ADR-0041-cross-package-reference-resolution.md)); a bare +name matches a short name, where a collision across packages is the deferred follow-up +[#174](https://github.com/metaobjectsdev/metaobjects/issues/174), the same as everywhere +else a bare reference is resolved. ## What each port generates diff --git a/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs b/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs index 515e60703..b2187d963 100644 --- a/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs +++ b/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs @@ -158,21 +158,20 @@ public void Derive_inherited_hetero_matches_a_concrete_junction_reference() Assert.Equal("tagId", fields.TargetField); } - // ADR-0041 DIVERGENCE — pinned, not fixed. Java's M2MFields compares RESOLVED object - // identity (FQN-exact); C#, TS and Python compare StripPackage() short names. The - // subject set used to hold one short name and now holds two, so the surface on which - // a bare-name compare can mis-bind is twice as large. Here a::NodeBase declares a - // genuine CROSS-PACKAGE hetero M:N onto b::NodeBase and a::Node extends a::NodeBase: - // deriving from a::Node the subject short names are {"NodeBase", "Node"}, and - // "b::NodeBase" strips to "NodeBase", so the target reads as the subject and the - // relationship is misclassified as an ambiguous self-join. Java's equivalent - // (M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage) gets it right. + // REGRESSION — a cross-package hetero M:N must not be read as a self-join just + // because the target's SHORT name matches one of the subject's. // - // Honest about severity: this model derived CORRECTLY before this branch (the - // one-member subject set did not collide), so the widening regressed it. Accepted - // deliberately per the ADR-0041 split. This encodes what C# ACTUALLY does so the - // divergence is gated rather than latent; when C# adopts FQN-exactness this test - // fails and must be rewritten to assert srcId/dstId. + // a::NodeBase declares a genuine cross-package hetero M:N onto b::NodeBase, and + // a::Node extends a::NodeBase. Deriving from a::Node the subject is + // {a::NodeBase, a::Node}; under the old StripPackage compare "b::NodeBase" + // stripped to "NodeBase", landed in the subject set, and the relationship refused + // to derive as an ambiguous self-join. It had derived correctly before the subject + // set grew to two names, so that was a regression, not a pre-existing gap. + // + // Fixed by comparing RESOLVED OBJECT IDENTITY, which is what the Java port has + // always done — this is the C# half of the pair with + // M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage, and it + // REDUCES the cross-port divergence rather than pinning it. private const string XpkgAModel = """ { "metadata.root": { "package": "a", "children": [ { "object.entity": { "name": "Node", "extends": "a::NodeBase", "children": [ @@ -202,7 +201,7 @@ public void Derive_inherited_hetero_matches_a_concrete_junction_reference() """; [Fact] - public void Adr0041_gap_cross_package_target_sharing_a_subject_short_name() + public void Cross_package_target_sharing_a_subject_short_name_is_not_a_self_join() { var r = new MetaDataLoader().Load([ new InMemoryStringSource(XpkgAModel, id: "a.json"), @@ -213,10 +212,11 @@ public void Adr0041_gap_cross_package_target_sharing_a_subject_short_name() var node = r.Root.Objects().First(o => o.Name == "Node"); var rel = Rel(node, "links"); - // CURRENT C# BEHAVIOUR (wrong; Java derives srcId/dstId here): - var ex = Assert.Throws( - () => M2MDerivation.DeriveM2MFields(rel, node, r.Root)); - Assert.Contains("is ambiguous", ex.Message, StringComparison.Ordinal); + // Identity resolution binds "b::NodeBase" to the b-package entity, which is + // neither subject — so this stays hetero and derives, exactly as Java does. + var fields = M2MDerivation.DeriveM2MFields(rel, node, r.Root); + Assert.Equal("srcId", fields.SourceField); + Assert.Equal("dstId", fields.TargetField); } [Fact] diff --git a/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs b/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs index 3f786ae3d..afb571163 100644 --- a/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs +++ b/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs @@ -32,6 +32,13 @@ // classification and the hetero reference match alike. Not covered: a junction // reference naming an entity strictly BETWEEN the base and the navigating entity in a // deeper hierarchy. +// +// The subject comparison is made on RESOLVED OBJECT IDENTITY, not on stripped short +// names, matching the Java reference (M2MFields.java). Bare-name equality cannot tell +// `a::NodeBase` from `b::NodeBase`, so once the subject set held two names a genuine +// CROSS-PACKAGE hetero M:N whose target shares a short name with the subject read as a +// self-join and refused to derive. Scoped deliberately to this one predicate — every +// other name comparison in this file is untouched. using MetaObjects.Meta; @@ -71,6 +78,28 @@ private static string StripPackage(string name) return idx < 0 ? name : name[(idx + PACKAGE_SEPARATOR.Length)..]; } + ///

+ /// The root entity a reference name denotes, or null. Mirrors the Java + /// reference's M2MFields.findObject exactly: a FULLY-QUALIFIED name (one + /// containing ::) resolves EXACTLY on the object's package-folded key, never + /// a bare-tail fallback; a bare name matches a short name, first match wins (the + /// bare-collision case is the deferred follow-up Java records as issue #174). + /// + /// This exists so the SUBJECT comparison can be made on object IDENTITY the way + /// Java's already is. A bare-name compare cannot tell a::NodeBase from + /// b::NodeBase, which made a genuine cross-package hetero M:N read as a + /// self-join the moment the subject set held two names. + /// + private static MetaObject? FindEntity(MetaRoot root, string? name) + { + if (string.IsNullOrEmpty(name)) return null; + var objects = root.Objects(); + if (name.Contains(PACKAGE_SEPARATOR, StringComparison.Ordinal)) + return objects.FirstOrDefault(o => string.Equals(o.ResolutionKey(), name, StringComparison.Ordinal)); + var bare = StripPackage(name); + return objects.FirstOrDefault(o => string.Equals(o.Name, bare, StringComparison.Ordinal)); + } + /// /// Derive the source/target junction FK fields for a M:N relationship. /// @@ -93,8 +122,14 @@ public static M2MFields DeriveM2MFields(MetaRelationship rel, MetaObject source, var subjectNames = declaring.Name == source.Name ? new[] { declaring.Name } : new[] { declaring.Name, source.Name }; - bool IsSubject(string? name) => name is not null && subjectNames.Contains(StripPackage(name)); var subjectLabel = string.Join(" or ", subjectNames.Select(n => $"\"{n}\"")); + // Compared by resolved object IDENTITY, matching the Java reference. A + // StripPackage compare cannot distinguish `a::NodeBase` from `b::NodeBase`, so + // with two names in the set a genuine cross-package hetero M:N read as a + // self-join and refused to derive. + bool IsSubject(MetaObject? entity) => + entity is not null && (ReferenceEquals(entity, declaring) || ReferenceEquals(entity, source)); + bool IsSubjectName(string? name) => name is not null && subjectNames.Contains(StripPackage(name)); string? throughName = rel.Through; if (throughName is null) @@ -125,12 +160,17 @@ public static M2MFields DeriveM2MFields(MetaRelationship rel, MetaObject source, $"identity.reference children (found {refs.Count})"); } - bool isSelfJoin = IsSubject(targetName); + // Defensive bare fallback when @objectRef does not resolve — loader validation + // normally guarantees it does. Same carve-out the Java reference makes. + var targetEntityNode = FindEntity(root, targetName); + bool isSelfJoin = targetEntityNode is not null + ? IsSubject(targetEntityNode) + : IsSubjectName(targetName); if (!isSelfJoin) { - // Hetero: match each reference by the entity it resolves to. - var sourceRef = refs.FirstOrDefault(r => IsSubject(r.TargetEntity)); + // Hetero: match each reference by the ENTITY OBJECT it resolves to. + var sourceRef = refs.FirstOrDefault(r => IsSubject(FindEntity(root, r.TargetEntity))); var targetRef = refs.FirstOrDefault( r => r.TargetEntity is not null && StripPackage(r.TargetEntity) == StripPackage(targetName)); var sourceField = sourceRef is not null ? RefFkField(sourceRef) : null; diff --git a/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java b/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java index 1e465c363..229f9f6a1 100644 --- a/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java +++ b/server/java/metadata/src/test/java/com/metaobjects/relationship/M2MSlimVocabularyTest.java @@ -403,14 +403,15 @@ private static MetaRelationship relOf(MetaObject obj, String relName) { + " { \"identity.reference\": { \"name\": \"partnerRef\", \"@fields\": \"partnerId\", \"@references\": \"xpkg::partner::Account\" } } ] } }" + "] } }"; - // CROSS-PORT NOTE: Java is the only port whose derive() resolves @objectRef and the - // junction references to OBJECTS and compares FQN identity. TS, C# and Python compare - // stripped short names, so the shape below — a cross-package hetero target whose bare - // name collides with the subject's — misreads as an ambiguous self-join there. That - // divergence is now PINNED on all three (relationship-m2m.test.ts, - // M2MInheritedDeclaringEntityTests.cs, test_derive_m2m_declaring_entity.py, each - // named "ADR-0041 GAP"), so adopting FQN-exactness elsewhere fails those tests loudly - // instead of silently changing behaviour. This test is the correct-behaviour side. + // CROSS-PORT NOTE: this port's identity-based subject comparison is the reference the + // other three were brought into line with. TS, C# and Python used to compare stripped + // short names, so the shape below — a cross-package hetero target whose bare name + // collides with the subject's — misread as an ambiguous self-join there once the + // subject set held two names (declaring + navigating entity). All four now resolve + // the name to an ENTITY and compare identity, and each of the three carries the + // matching regression test (relationship-m2m.test.ts, + // M2MInheritedDeclaringEntityTests.cs, test_derive_m2m_declaring_entity.py). Keep + // this test and those four in step. @Test public void deriveCrossPackageHeteroBindsCorrectPackage() { // ADR-0041: same-bare-name entities/junctions in different packages. Under the diff --git a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py index 8dca41987..1346aaa18 100644 --- a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py +++ b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py @@ -31,6 +31,13 @@ object parent, which keeps the signature unchanged. Not covered: a junction reference naming an entity strictly BETWEEN the base and the navigating entity in a deeper hierarchy. + +The subject comparison is made on RESOLVED OBJECT IDENTITY, not on stripped short +names, matching the Java reference (``M2MFields.java``). Bare-name equality cannot +tell ``a::NodeBase`` from ``b::NodeBase``, so once the subject set held two names a +genuine CROSS-PACKAGE hetero M:N whose target shares a short name with the subject +read as a self-join and refused to derive. Scoped deliberately to this one predicate +— every other name comparison in this module is untouched. """ from __future__ import annotations @@ -109,6 +116,55 @@ def _ref_target_entity(ref: MetaData) -> str | None: return _strip_package(v) if isinstance(v, str) and v else None +def _ref_target_raw(ref: MetaData) -> str | None: + """The @references value of a reference, VERBATIM (package intact). + + Distinct from :func:`_ref_target_entity`, which strips the package for the + legacy bare-name compare. Used only by the subject resolution below, which + needs the qualified form to tell ``a::NodeBase`` from ``b::NodeBase``. The + dotted ``Entity.field`` form is deliberately NOT split here — that is the + separate documented gap on :func:`_ref_target_entity`, and splitting would + change behaviour beyond this fix. A dotted value simply resolves to nothing, + exactly as it matches nothing today. + """ + v = ref.get_meta_attr(IDENTITY_REFERENCE_ATTR_REFERENCES) # ADR-0039: resolving + return v if isinstance(v, str) and v else None + + +def _root_objects(node: MetaData) -> list[MetaData]: + """Every top-level object of the tree *node* belongs to, in declaration order. + + Walks up to the root rather than taking the caller's ``object_index``: that + index is keyed by BARE name, so two same-short-name entities in different + packages collapse to one entry and FQN-exact resolution is impossible from it. + """ + root = node + while root.parent is not None: + root = root.parent + return [c for c in root.children() if c.type == TYPE_OBJECT] + + +def _find_entity(objects: list[MetaData], name: str | None) -> MetaData | None: + """The root entity a reference name denotes, or ``None``. + + Mirrors the Java reference's ``M2MFields.findObject`` exactly: a FULLY-QUALIFIED + name (one containing ``::``) resolves EXACTLY on the object's package-folded key, + never a bare-tail fallback; a bare name matches a short name, first match wins + (the bare-collision case is the deferred follow-up Java records as issue #174). + + This exists so the SUBJECT comparison can be made on object IDENTITY the way + Java's already is. A bare-name compare cannot tell ``a::NodeBase`` from + ``b::NodeBase``, which made a genuine cross-package hetero M:N read as a + self-join the moment the subject set held two names. + """ + if not name: + return None + if PACKAGE_SEP in name: + return next((o for o in objects if o.resolution_key() == name), None) + bare = _strip_package(name) + return next((o for o in objects if o.name == bare), None) + + def derive_m2m_fields( rel: MetaRelationship, source: MetaData, @@ -179,12 +235,33 @@ def derive_m2m_fields( subject_names.append(source.name) subject_label = " or ".join(f'"{n}"' for n in subject_names) - is_self_join = _strip_package(target_name) in subject_names + # Compared by resolved object IDENTITY, matching the Java reference. A + # package-stripped compare cannot distinguish ``a::NodeBase`` from + # ``b::NodeBase``, so with two names in the set a genuine cross-package hetero + # M:N read as a self-join and refused to derive. + root_objects = _root_objects(declaring) + + def _is_subject(entity: MetaData | None) -> bool: + return entity is not None and (entity is declaring or entity is source) + + # Defensive bare fallback when @objectRef does not resolve — loader validation + # normally guarantees it does. Same carve-out the Java reference makes. + target_entity_node = _find_entity(root_objects, target_name) + is_self_join = ( + _is_subject(target_entity_node) + if target_entity_node is not None + else _strip_package(target_name) in subject_names + ) if not is_self_join: - # Hetero: match each reference by the entity it resolves to. + # Hetero: match each reference by the ENTITY OBJECT it resolves to. source_ref = next( - (r for r in refs if _ref_target_entity(r) in subject_names), None + ( + r + for r in refs + if _is_subject(_find_entity(root_objects, _ref_target_raw(r))) + ), + None, ) target_ref = next( (r for r in refs if _ref_target_entity(r) == _strip_package(target_name)), diff --git a/server/python/tests/unit/test_derive_m2m_declaring_entity.py b/server/python/tests/unit/test_derive_m2m_declaring_entity.py index abea68570..66b346acb 100644 --- a/server/python/tests/unit/test_derive_m2m_declaring_entity.py +++ b/server/python/tests/unit/test_derive_m2m_declaring_entity.py @@ -23,14 +23,9 @@ import json -import pytest - from metaobjects import InMemoryStringSource, MetaDataLoader, load_string from metaobjects.meta.core.object.meta_object import MetaObject -from metaobjects.meta.core.relationship.derive_m2m_fields import ( - M2MDerivationError, - derive_m2m_fields, -) +from metaobjects.meta.core.relationship.derive_m2m_fields import derive_m2m_fields from metaobjects.meta.core.relationship.meta_relationship import MetaRelationship from metaobjects.runtime.n2m_resolver import resolve_n2m_descriptor @@ -221,20 +216,20 @@ def test_runtime_resolver_traverses_an_inherited_self_join() -> None: # declaring base does not have — a separate concern from the derivation. -# ADR-0041 DIVERGENCE — pinned, not fixed. Java's M2MFields compares RESOLVED object -# identity (FQN-exact); Python, TS and C# compare stripped short names. The subject set -# used to hold one short name and now holds two, so the surface on which a bare-name -# compare can mis-bind is twice as large. Here ``a::NodeBase`` declares a genuine -# CROSS-PACKAGE hetero M:N onto ``b::NodeBase`` and ``a::Node`` extends ``a::NodeBase``: -# deriving from ``a::Node`` the subject short names are {"NodeBase", "Node"}, and -# ``b::NodeBase`` strips to "NodeBase", so the target reads as the subject and the -# relationship is misclassified as an ambiguous self-join. Java's equivalent -# (M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage) gets it right. +# REGRESSION — a cross-package hetero M:N must not be read as a self-join just because +# the target's SHORT name matches one of the subject's. +# +# ``a::NodeBase`` declares a genuine cross-package hetero M:N onto ``b::NodeBase``, and +# ``a::Node`` extends ``a::NodeBase``. Deriving from ``a::Node`` the subject is +# {a::NodeBase, a::Node}; under the old package-stripped compare "b::NodeBase" stripped +# to "NodeBase", landed in the subject set, and the relationship refused to derive as an +# ambiguous self-join. It had derived correctly before the subject set grew to two names, +# so that was a regression, not a pre-existing gap. # -# Honest about severity: this model derived CORRECTLY before this branch (the one-member -# subject set did not collide), so the widening regressed it. Accepted deliberately per -# the ADR-0041 split; the test encodes what Python ACTUALLY does so the divergence is -# gated rather than latent, and must be rewritten when Python adopts FQN-exactness. +# Fixed by comparing RESOLVED OBJECT IDENTITY, which is what the Java port has always +# done — this is the Python half of the pair with +# M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage, and it REDUCES the +# cross-port divergence rather than pinning it. XPKG_A = { "metadata.root": { "package": "a", @@ -276,8 +271,8 @@ def test_runtime_resolver_traverses_an_inherited_self_join() -> None: } -def test_adr0041_gap_cross_package_target_sharing_a_subject_short_name() -> None: - """CURRENT Python behaviour (wrong; Java derives srcId/dstId here).""" +def test_cross_package_target_sharing_a_subject_short_name_is_not_a_self_join() -> None: + """Identity resolution keeps this hetero, exactly as Java does.""" result = MetaDataLoader().load([ InMemoryStringSource(json.dumps(XPKG_A), id="a.json"), InMemoryStringSource(json.dumps(XPKG_B), id="b.json"), @@ -291,5 +286,6 @@ def test_adr0041_gap_cross_package_target_sharing_a_subject_short_name() -> None index = {o.name: o for o in objects} node = objects[0] rel = _rel(node, "links") - with pytest.raises(M2MDerivationError, match="is ambiguous"): - derive_m2m_fields(rel, node, index) + fields = derive_m2m_fields(rel, node, index) + assert fields.source_field == "srcId" + assert fields.target_field == "dstId" diff --git a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts index 1075e25ec..b5f4abb27 100644 --- a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts +++ b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts @@ -31,6 +31,13 @@ // also the fallback when `rel` has no entity parent, which keeps the exported signature // unchanged. Not covered: a junction reference naming an entity strictly BETWEEN the // base and the navigating entity in a deeper hierarchy. +// +// The subject comparison is made on RESOLVED OBJECT IDENTITY, not on stripped +// short names, matching the Java reference (M2MFields.java). Bare-name equality +// cannot tell `a::NodeBase` from `b::NodeBase`, so once the subject set held two +// names a genuine CROSS-PACKAGE hetero M:N whose target shares a short name with +// the subject read as a self-join and refused to derive. Scoped deliberately to +// this one predicate — every other name comparison in this file is untouched. import type { MetaObject } from "../object/meta-object.js"; import type { MetaRoot } from "../../shared/meta-root.js"; @@ -38,6 +45,7 @@ import type { MetaRelationship } from "./meta-relationship.js"; import type { MetaReferenceIdentity } from "../identity/meta-identity.js"; import { stripPackage } from "../../naming.js"; import { TYPE_OBJECT } from "../../shared/base-types.js"; +import { PACKAGE_SEPARATOR } from "../../shared/structural.js"; /** Thrown when a M:N relationship's junction FK fields cannot be derived. */ export class M2MDerivationError extends Error { @@ -60,6 +68,28 @@ function refFkField(ref: MetaReferenceIdentity): string | undefined { return ref.fields[0]; } +/** + * The root entity a reference name denotes, or undefined. Mirrors the Java + * reference's `M2MFields.findObject` exactly: a FULLY-QUALIFIED name (one + * containing "::") resolves EXACTLY on the object's package-folded key, never a + * bare-tail fallback; a bare name matches a short name, first match wins (the + * bare-collision case is the deferred follow-up Java records as issue #174). + * + * This exists so the SUBJECT comparison below can be made on object IDENTITY the + * way Java's already is. A bare-name compare cannot tell `a::NodeBase` from + * `b::NodeBase`, which made a genuine cross-package hetero M:N read as a + * self-join the moment the subject set held two names. + */ +function findEntity(root: MetaRoot, name: string | undefined): MetaObject | undefined { + if (name === undefined || name === "") return undefined; + const objects = root.objects(); + if (name.includes(PACKAGE_SEPARATOR)) { + return objects.find((o) => o.resolutionKey() === name); + } + const bare = stripPackage(name); + return objects.find((o) => o.name === bare); +} + /** * Derive the source/target junction FK fields for a M:N relationship. * @@ -140,15 +170,26 @@ export function deriveM2MFields( const subjectNames = declaringEntity.name === source.name ? [declaringEntity.name] : [declaringEntity.name, source.name]; - const isSubject = (name: string | undefined): boolean => - name !== undefined && subjectNames.includes(stripPackage(name)); const subjectLabel = subjectNames.map((n) => `"${n}"`).join(" or "); + // Compared by resolved object IDENTITY, matching the Java reference. A + // stripPackage() compare cannot distinguish `a::NodeBase` from `b::NodeBase`, + // so with two names in the set a genuine cross-package hetero M:N read as a + // self-join and refused to derive. + const isSubject = (entity: MetaObject | undefined): boolean => + entity !== undefined && (entity === declaringEntity || entity === source); + const isSubjectName = (name: string | undefined): boolean => + name !== undefined && subjectNames.includes(stripPackage(name)); - const isSelfJoin = isSubject(targetName); + // Defensive bare fallback when @objectRef does not resolve — loader validation + // normally guarantees it does. Same carve-out the Java reference makes. + const targetEntityNode = findEntity(root, targetName); + const isSelfJoin = targetEntityNode !== undefined + ? isSubject(targetEntityNode) + : isSubjectName(targetName); if (!isSelfJoin) { - // Hetero: match each reference by the entity it resolves to. - const sourceRef = refs.find((r) => isSubject(r.targetEntity)); + // Hetero: match each reference by the ENTITY OBJECT it resolves to. + const sourceRef = refs.find((r) => isSubject(findEntity(root, r.targetEntity))); const targetRef = refs.find((r) => r.targetEntity !== undefined && stripPackage(r.targetEntity) === stripPackage(targetName)); const sourceField = sourceRef ? refFkField(sourceRef) : undefined; const targetField = targetRef ? refFkField(targetRef) : undefined; diff --git a/server/typescript/packages/metadata/test/relationship-m2m.test.ts b/server/typescript/packages/metadata/test/relationship-m2m.test.ts index e42104ac1..e408f0a10 100644 --- a/server/typescript/packages/metadata/test/relationship-m2m.test.ts +++ b/server/typescript/packages/metadata/test/relationship-m2m.test.ts @@ -852,26 +852,22 @@ describe("FR-017 deriveM2MFields uses the DECLARING entity, not the visiting one expect(derived.targetField).toBe("tagId"); }); - // ADR-0041 DIVERGENCE — pinned, not fixed. Java's M2MFields compares RESOLVED - // object identity (FQN-exact); TS, C# and Python compare stripPackage() short - // names. The subject set used to hold one short name and now holds two, so the - // surface on which a bare-name compare can mis-bind is twice as large. This - // fixture is the concrete case: a::NodeBase declares a genuine CROSS-PACKAGE - // hetero M:N onto b::NodeBase, and a::Node extends a::NodeBase. Deriving from - // a::Node, the subject short names are {"NodeBase", "Node"}; stripPackage - // ("b::NodeBase") is "NodeBase", which is IN that set, so the target reads as - // the subject and the relationship is misclassified as an ambiguous self-join. + // REGRESSION — a cross-package hetero M:N must not be read as a self-join just + // because the target's SHORT name matches one of the subject's. // - // Java's equivalent (M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage) - // gets this right. This test encodes what TS ACTUALLY does today, not what it - // ought to do, so the divergence is gated rather than latent: the day TS adopts - // FQN-exact resolution (ADR-0041 work, a separate branch), this test fails and - // must be rewritten to assert srcId/dstId. + // `a::NodeBase` declares a genuine cross-package hetero M:N onto `b::NodeBase`, + // and `a::Node extends a::NodeBase`. Deriving from `a::Node` the subject is + // {a::NodeBase, a::Node}; under the old stripPackage() compare "b::NodeBase" + // stripped to "NodeBase", landed in the subject set, and the relationship + // refused to derive as an ambiguous self-join. It had derived correctly before + // the subject set grew to two names, so that was a regression, not a + // pre-existing gap. // - // Honest about severity: this model DERIVED CORRECTLY before this branch - // (the one-member subject set {"Node"} did not collide), so the widening - // regressed it. Accepted deliberately, per the ADR-0041 split. - test("ADR-0041 GAP: a cross-package hetero target sharing a subject short name misreads as a self-join", async () => { + // Fixed by comparing RESOLVED OBJECT IDENTITY, which is what the Java port has + // always done — this is the TS half of the pair with + // M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage, and it + // REDUCES the cross-port divergence rather than pinning it. + test("a cross-package hetero target sharing a subject short name is NOT a self-join", async () => { const aDoc = { "metadata.root": { package: "a", children: [ { "object.entity": { name: "Node", "extends": "a::NodeBase", children: [ { "field.long": { name: "id" } }, @@ -901,10 +897,11 @@ describe("FR-017 deriveM2MFields uses the DECLARING entity, not the visiting one const node = findObj(root, "Node"); const rel = node.relationships().find((r) => r.name === "links") as MetaRelationship; - // CURRENT TS BEHAVIOUR (wrong; Java derives srcId/dstId here): - expect(() => deriveM2MFields(rel, node, root)).toThrow( - /is ambiguous: set @sourceRefField \(directed\) or @symmetric \(undirected\)/, - ); + // Identity resolution binds "b::NodeBase" to the b-package entity, which is + // neither subject — so this stays hetero and derives, exactly as Java does. + const derived = deriveM2MFields(rel, node, root); + expect(derived.sourceField).toBe("srcId"); + expect(derived.targetField).toBe("dstId"); }); test("an OWN relationship still derives against its own entity (no regression)", async () => { From f72b4c55852e5e5893438b6b2335d5643cc94e41 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Mon, 14 Sep 2026 01:04:18 -0400 Subject: [PATCH 11/12] fix: match the hetero TARGET junction reference by identity too, and align IsSelfJoin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fix round 3 — Critical. Round 2 converted only the SOURCE side of the junction match to identity and left the target side on a bare compare. The two searches are independent and, unlike the directed self-join branch, nothing excludes sourceRef from the target search. Before round 2 that hole was structurally unreachable: both sides used the same bare compare, so a colliding short name forced isSelfJoin true and the hetero branch was never entered. Making only isSelfJoin identity-based broke that invariant. Measured on TS, no inheritance required — Java's own deriveCrossPackageHeteroBindsCorrectPackage model: a::Account -partners-> b::Account through AccountLink(ownerRef->a::Account, partnerRef->b::Account) pre-branch: threw "is ambiguous" (loud) after round 2: (ownerId, ownerId) <- the target search re-matched ownerRef now: (ownerId, partnerId) <- same as Java And on the shape the new docs bless, a junction source ref naming the declaring base: (srcId, srcId) -> (srcId, dstId). Both junction matches are now identity-based in TS, C# and Python, with the bare fallback kept only where @objectRef does not resolve — mirroring Java's findRefToSubject + findRefToObject pair. C# M2MNavigation.IsSelfJoin is identity-only (the two short-name arms are gone), so the descriptor cannot disagree with the derivation that produced it. Dropping those arms alone was NOT sufficient, which measurement showed rather than argument: M2MNavigationBuilder.Build resolved its target with StripPkg + FindObject, binding "b::Account" to a::Account, so IsSelfJoin stayed true by reference. Build now resolves target and junction through the derivation's own rule (M2MDerivation.ResolveEntity, newly public precisely so the two cannot drift), falling back to the previous resolution when a qualified name does not resolve exactly. Asserted through M2MNavigationBuilder.For — the path DbContextGenerator, EntityGenerator, RoutesGenerator and CSharpApiModelBuilder take, and the one nothing covered. Python's _ref_target_raw now splits at the first "." while keeping the package. That matches TS/C#/Java, and removes a seam with PR #372 (which makes _ref_target_entity split): without it the two sides of the same `if` would disagree about a dotted junction reference once #372 lands. Tests: the three regression fixtures now name the DECLARING BASE as the junction source ref (their short name then collides, which is what makes them bite), plus a ported Java-parity cross-package hetero case on each port. All were confirmed failing against round-2 code with exactly (srcId, srcId) / (ownerId, ownerId). docs/features/relationships.md and the CHANGELOG said "resolving the name to an ENTITY and comparing identity" and "cross-package targets are safe" — true only now. Both also record the two narrow resolution changes that come with it: a qualified reference must resolve exactly, and a bare colliding one binds first-declared (#174). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- CHANGELOG.md | 28 +++++-- docs/features/relationships.md | 18 +++-- .../M2MInheritedDeclaringEntityTests.cs | 81 ++++++++++++++++++- .../Generators/M2MNavigation.cs | 27 +++++-- .../Core/Relationship/M2MFields.cs | 21 ++++- .../core/relationship/derive_m2m_fields.py | 47 ++++++++--- .../unit/test_derive_m2m_declaring_entity.py | 69 +++++++++++++++- .../core/relationship/derive-m2m-fields.ts | 12 ++- .../metadata/test/relationship-m2m.test.ts | 46 ++++++++++- 9 files changed, 313 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36e5696e1..301f8e87c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,11 +52,29 @@ here.** so codegen emitted for the child and dropped it for the base. The refusal is the correct behaviour, but on Java, Kotlin and Python, whose callers do not catch, it moves from "generates wrongly" to "the generation run fails", and the fix is to add - `@symmetric` or `@sourceRefField`. That is the only behaviour this change takes away. - - **Cross-port divergence goes DOWN, not up.** Deciding whether `@objectRef` names the - relationship's subject now resolves the name to an ENTITY and compares identity in all - four derivations, which is what the Java port already did. TypeScript, C# and Python + `@symmetric` or `@sourceRefField`. + + Two narrower resolution changes come with moving the junction match onto identity, both + matching what the Java port already did. A junction `@references` (or an `@objectRef`) + that **is** package-qualified must now resolve **exactly**: a partially-qualified or + stale package no longer falls back to matching the bare tail, so a reference that used + to bind by luck now does not match the subject. And a **bare** reference whose short + name exists in more than one package resolves first-declared-wins, which can pick the + wrong-package entity — that is the pre-existing + [#174](https://github.com/metaobjectsdev/metaobjects/issues/174) behaviour, now reached + by M:N derivation as well. Both are narrow, and both bring the other ports onto Java's + semantics rather than away from them. + + **Cross-port divergence goes DOWN, not up.** Both junction matches — "does this + reference name the relationship's subject?" and "does this one name the target?" — now + resolve the name to an ENTITY and compare identity in all four derivations, which is + what the Java port already did on both sides. Matching only one side would be worse + than matching neither: the two searches are independent and nothing excludes the + source-side reference from the target search, so a cross-package M:N could bind the + same junction column as BOTH sides and emit `(srcFk, srcFk)` silently. C#'s + `M2MNavigation` descriptor resolves its target the same way for the same reason — its + `IsSelfJoin` feeds the EF `UsingEntity` wiring, and a descriptor that disagreed with + the derivation would mis-map the relationship. TypeScript, C# and Python had been comparing package-stripped short names, so a genuine cross-package hetero M:N onto a target whose short name matched the subject's (`a::NodeBase` relating to `b::NodeBase`) was misread as a self-join on those three — a regression the two-name diff --git a/docs/features/relationships.md b/docs/features/relationships.md index a5c87dbd9..9caf2a569 100644 --- a/docs/features/relationships.md +++ b/docs/features/relationships.md @@ -300,15 +300,21 @@ it is what [#368](https://github.com/metaobjectsdev/metaobjects/issues/368)'s loader fix established for validation and this rule extends to FK derivation. -**Cross-package targets are safe.** All five ports decide "is `@objectRef` the subject?" -by resolving the name to an ENTITY and comparing identity, not by comparing bare short -names — so a genuine cross-package hetero M:N whose target's short name happens to match -the subject's (`a::NodeBase` relating to `b::NodeBase`) stays hetero and derives, rather -than being misread as a self-join. A package-qualified name resolves exactly +**Cross-package targets are safe.** All five ports resolve `@objectRef` and each junction +`identity.reference` to an ENTITY and compare identity — on **both** sides of the +derivation, the source-side match and the target-side match alike. So a genuine +cross-package hetero M:N whose target's short name happens to match the source's +(`a::Account` relating to `b::Account`, or `a::NodeBase` to `b::NodeBase`) binds each +junction reference to its own entity instead of matching one of them twice. A +package-qualified name resolves exactly ([ADR-0041](../../spec/decisions/ADR-0041-cross-package-reference-resolution.md)); a bare name matches a short name, where a collision across packages is the deferred follow-up [#174](https://github.com/metaobjectsdev/metaobjects/issues/174), the same as everywhere -else a bare reference is resolved. +else a bare reference is resolved. Two consequences worth knowing when authoring: a +junction `@references` that *is* package-qualified must resolve **exactly** — a +partially-qualified or stale package no longer falls back to a bare-tail match — and a +**bare** `@references` whose short name exists in more than one package binds the first +declared, which is #174 and not specific to M:N. ## What each port generates diff --git a/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs b/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs index b2187d963..5fe2981f9 100644 --- a/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs +++ b/server/csharp/MetaObjects.Codegen.Tests/M2MInheritedDeclaringEntityTests.cs @@ -172,6 +172,13 @@ public void Derive_inherited_hetero_matches_a_concrete_junction_reference() // always done — this is the C# half of the pair with // M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage, and it // REDUCES the cross-port divergence rather than pinning it. + // + // The junction's SOURCE reference names the DECLARING BASE ("a::NodeBase") — the + // shape docs/features/relationships.md blesses, and the one whose short name + // collides with the target's. That matters: making isSelfJoin identity-based while + // the hetero TARGET search was still a bare compare let that search re-match this + // very reference (nothing excludes sourceRef from it, unlike the directed self-join + // branch) and return (srcId, srcId) silently. Both searches are identity-based now. private const string XpkgAModel = """ { "metadata.root": { "package": "a", "children": [ { "object.entity": { "name": "Node", "extends": "a::NodeBase", "children": [ @@ -185,7 +192,7 @@ public void Derive_inherited_hetero_matches_a_concrete_junction_reference() { "field.long": { "name": "srcId" } }, { "field.long": { "name": "dstId" } }, { "identity.primary": { "@fields": ["srcId", "dstId"] } }, - { "identity.reference": { "name": "s", "@fields": "srcId", "@references": "a::Node" } }, + { "identity.reference": { "name": "s", "@fields": "srcId", "@references": "a::NodeBase" } }, { "identity.reference": { "name": "d", "@fields": "dstId", "@references": "b::NodeBase" } } ]}} ]}} @@ -200,6 +207,78 @@ public void Derive_inherited_hetero_matches_a_concrete_junction_reference() ]}} """; + // Java's deriveCrossPackageHeteroBindsCorrectPackage model, ported. No inheritance + // is needed to reach the defect: `a::Account` relates to `b::Account` through a + // junction holding one reference to each. The two junction searches are + // INDEPENDENT and nothing excludes the source ref from the target search, so a + // bare-name target match found `ownerRef` a second time and returned + // (ownerId, ownerId) — silently. Pre-branch this threw loudly. + private const string XpkgHeteroA = """ + { "metadata.root": { "package": "a", "children": [ + { "object.entity": { "name": "Account", "children": [ + { "field.long": { "name": "id" } }, + { "relationship.association": { "name": "partners", "@cardinality": "many", "@objectRef": "b::Account", "@through": "AccountLink" } }, + { "identity.primary": { "@fields": "id" } } + ]}}, + { "object.entity": { "name": "AccountLink", "children": [ + { "field.long": { "name": "ownerId" } }, + { "field.long": { "name": "partnerId" } }, + { "identity.primary": { "@fields": ["ownerId", "partnerId"] } }, + { "identity.reference": { "name": "ownerRef", "@fields": "ownerId", "@references": "a::Account" } }, + { "identity.reference": { "name": "partnerRef", "@fields": "partnerId", "@references": "b::Account" } } + ]}} + ]}} + """; + + private const string XpkgHeteroB = """ + { "metadata.root": { "package": "b", "children": [ + { "object.entity": { "name": "Account", "children": [ + { "field.long": { "name": "id" } }, + { "identity.primary": { "@fields": "id" } } + ]}} + ]}} + """; + + [Fact] + public void Cross_package_hetero_matches_each_junction_reference_to_its_own_entity() + { + var r = new MetaDataLoader().Load([ + new InMemoryStringSource(XpkgHeteroA, id: "a.json"), + new InMemoryStringSource(XpkgHeteroB, id: "b.json"), + ]); + Assert.Empty(r.Errors); + var account = r.Root.Objects().First(o => o.ResolutionKey() == "a::Account"); + var fields = M2MDerivation.DeriveM2MFields(Rel(account, "partners"), account, r.Root); + // Was (ownerId, ownerId): the target search re-matched the SOURCE reference. + Assert.Equal("ownerId", fields.SourceField); + Assert.Equal("partnerId", fields.TargetField); + } + + [Fact] + public void Navigation_builder_agrees_with_the_derivation_on_a_cross_package_hetero() + { + // Asserted through M2MNavigationBuilder.For — the path DbContextGenerator, + // EntityGenerator, RoutesGenerator and CSharpApiModelBuilder actually take, and + // the one nothing covered. IsSelfJoin must agree with the derivation: a + // cross-package M:N whose target merely shares a short name with the source is + // HETERO, and calling it a self-join would skip its EF UsingEntity wiring and + // mark the navigation [NotMapped]. + var r = new MetaDataLoader().Load([ + new InMemoryStringSource(XpkgHeteroA, id: "a.json"), + new InMemoryStringSource(XpkgHeteroB, id: "b.json"), + ]); + Assert.Empty(r.Errors); + var account = r.Root.Objects().First(o => o.ResolutionKey() == "a::Account"); + + var nav = Assert.Single(M2MNavigationBuilder.For(account, r.Root)); + Assert.Equal("partners", nav.Name); + Assert.Equal("ownerId", nav.SourceField); + Assert.Equal("partnerId", nav.TargetField); + // The target is the OTHER package's Account, not this one. + Assert.Equal("b::Account", nav.Target.ResolutionKey()); + Assert.False(nav.IsSelfJoin); + } + [Fact] public void Cross_package_target_sharing_a_subject_short_name_is_not_a_self_join() { diff --git a/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs b/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs index c299f5be9..ed873807c 100644 --- a/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs +++ b/server/csharp/MetaObjects.Codegen/Generators/M2MNavigation.cs @@ -51,18 +51,21 @@ public sealed record M2MNavigation( /// against BOTH the navigating and the /// , the same pair M2MDerivation accepts. /// + /// Compared by OBJECT IDENTITY, never by short name — the same predicate + /// M2MDerivation uses, so the descriptor cannot disagree with the FK + /// derivation that produced it. A short-name compare said "self-join" for a + /// cross-package M:N whose target merely shares a short name with the subject, + /// while the derivation correctly called it hetero. + /// /// Load-bearing: DbContextGenerator excludes self-joins from the EF /// UsingEntity wiring and EntityGenerator marks their navigation /// [NotMapped] (it is route-traversed). Before the derivation fix an /// inherited self-join threw and the whole navigation was dropped, so this never - /// ran on one; comparing only would now turn that silent - /// drop into silently WRONG EF configuration. + /// ran on one; getting it wrong now turns that silent drop into silently WRONG + /// EF configuration. /// public bool IsSelfJoin => - ReferenceEquals(Source, Target) || - string.Equals(Source.Name, Target.Name, StringComparison.Ordinal) || - ReferenceEquals(DeclaringEntity, Target) || - string.Equals(DeclaringEntity.Name, Target.Name, StringComparison.Ordinal); + ReferenceEquals(Source, Target) || ReferenceEquals(DeclaringEntity, Target); } /// @@ -95,8 +98,16 @@ public static IReadOnlyList For(MetaObject entity, MetaRoot root) private static M2MNavigation? Build(MetaObject source, MetaRelationship rel, MetaRoot root) { if (rel.ObjectRef is not { } targetRef || rel.Through is not { } throughRef) return null; - var target = root.FindObject(CSharpNaming.StripPkg(targetRef)); - var junction = root.FindObject(CSharpNaming.StripPkg(throughRef)); + // Resolve by the SAME rule the derivation uses (FQN-exact when qualified, bare + // short name otherwise), or IsSelfJoin below can disagree with the FK derivation: + // StripPkg + FindObject binds "b::Account" to a same-short-named `a::Account`, + // which then compares identity-equal to the source and reports a cross-package + // hetero M:N as a self-join. Falls back to the previous resolution when the + // qualified name does not resolve exactly, so nothing that bound before stops. + var target = M2MDerivation.ResolveEntity(root, targetRef) + ?? root.FindObject(CSharpNaming.StripPkg(targetRef)); + var junction = M2MDerivation.ResolveEntity(root, throughRef) + ?? root.FindObject(CSharpNaming.StripPkg(throughRef)); if (target is null || junction is null) return null; M2MFields fields; diff --git a/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs b/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs index afb571163..eb4671a89 100644 --- a/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs +++ b/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs @@ -90,6 +90,16 @@ private static string StripPackage(string name) /// b::NodeBase, which made a genuine cross-package hetero M:N read as a /// self-join the moment the subject set held two names. /// + /// + /// Public so codegen descriptors can resolve an entity reference by the SAME rule + /// the derivation uses. M2MNavigation.IsSelfJoin compares the descriptor's + /// target against its source by identity; if the builder resolved the target by a + /// package-stripped name while the derivation resolved it exactly, the two could + /// disagree about whether a relationship is a self-join — and the EF wiring follows + /// the descriptor. Additive: no existing resolution changed. + /// + public static MetaObject? ResolveEntity(MetaRoot root, string? name) => FindEntity(root, name); + private static MetaObject? FindEntity(MetaRoot root, string? name) { if (string.IsNullOrEmpty(name)) return null; @@ -171,8 +181,15 @@ bool IsSubject(MetaObject? entity) => { // Hetero: match each reference by the ENTITY OBJECT it resolves to. var sourceRef = refs.FirstOrDefault(r => IsSubject(FindEntity(root, r.TargetEntity))); - var targetRef = refs.FirstOrDefault( - r => r.TargetEntity is not null && StripPackage(r.TargetEntity) == StripPackage(targetName)); + // Identity here too. The two searches are INDEPENDENT — nothing excludes + // sourceRef from this one, unlike the directed self-join branch below — so a + // bare compare could match the SOURCE-side reference again whenever the + // target's short name equals the source's, and silently return (srcFk, srcFk). + // Java matches identity on both sides (findRefToSubject + findRefToObject). + var targetRef = targetEntityNode is not null + ? refs.FirstOrDefault(r => ReferenceEquals(FindEntity(root, r.TargetEntity), targetEntityNode)) + : refs.FirstOrDefault( + r => r.TargetEntity is not null && StripPackage(r.TargetEntity) == StripPackage(targetName)); var sourceField = sourceRef is not null ? RefFkField(sourceRef) : null; var targetField = targetRef is not null ? RefFkField(targetRef) : null; if (sourceField is null || targetField is null) diff --git a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py index 1346aaa18..fe2fff0b6 100644 --- a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py +++ b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py @@ -119,16 +119,24 @@ def _ref_target_entity(ref: MetaData) -> str | None: def _ref_target_raw(ref: MetaData) -> str | None: """The @references value of a reference, VERBATIM (package intact). - Distinct from :func:`_ref_target_entity`, which strips the package for the - legacy bare-name compare. Used only by the subject resolution below, which - needs the qualified form to tell ``a::NodeBase`` from ``b::NodeBase``. The - dotted ``Entity.field`` form is deliberately NOT split here — that is the - separate documented gap on :func:`_ref_target_entity`, and splitting would - change behaviour beyond this fix. A dotted value simply resolves to nothing, - exactly as it matches nothing today. + Distinct from :func:`_ref_target_entity`, which strips the PACKAGE for the + legacy bare-name compare. This keeps the package — identity resolution needs + the qualified form to tell ``a::NodeBase`` from ``b::NodeBase`` — and takes the + entity head of the dotted ``Entity.field`` form, since packages use ``::`` and + never ``.``, so the first ``.`` splits the entity off. That matches the TS + ``MetaIdentity.targetEntity``, the C# ``TargetEntity`` and Java's + ``refTargetObject``, so all four resolve a dotted junction reference alike. + + (The dotted blind spot on :func:`_ref_target_entity` is a separate, older gap + being closed on its own branch. Splitting HERE removes the seam: without it the + two sides of the same ``if`` would disagree about dotted references once that + lands.) """ v = ref.get_meta_attr(IDENTITY_REFERENCE_ATTR_REFERENCES) # ADR-0039: resolving - return v if isinstance(v, str) and v else None + if not isinstance(v, str) or not v: + return None + dot = v.find(".") + return v if dot < 0 else v[:dot] def _root_objects(node: MetaData) -> list[MetaData]: @@ -263,9 +271,26 @@ def _is_subject(entity: MetaData | None) -> bool: ), None, ) - target_ref = next( - (r for r in refs if _ref_target_entity(r) == _strip_package(target_name)), - None, + # Identity here too. The two searches are INDEPENDENT — nothing excludes + # source_ref from this one, unlike the directed self-join branch below — so a + # bare compare could match the SOURCE-side reference again whenever the + # target's short name equals the source's, and silently return (src_fk, src_fk). + # Java matches identity on both sides (findRefToSubject + findRefToObject). + target_ref = ( + next( + ( + r + for r in refs + if _find_entity(root_objects, _ref_target_raw(r)) + is target_entity_node + ), + None, + ) + if target_entity_node is not None + else next( + (r for r in refs if _ref_target_entity(r) == _strip_package(target_name)), + None, + ) ) source_field = _ref_fk_field(source_ref) if source_ref is not None else None target_field = _ref_fk_field(target_ref) if target_ref is not None else None diff --git a/server/python/tests/unit/test_derive_m2m_declaring_entity.py b/server/python/tests/unit/test_derive_m2m_declaring_entity.py index 66b346acb..e7e63e43d 100644 --- a/server/python/tests/unit/test_derive_m2m_declaring_entity.py +++ b/server/python/tests/unit/test_derive_m2m_declaring_entity.py @@ -230,6 +230,13 @@ def test_runtime_resolver_traverses_an_inherited_self_join() -> None: # done — this is the Python half of the pair with # M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage, and it REDUCES the # cross-port divergence rather than pinning it. +# +# The junction's SOURCE reference names the DECLARING BASE ("a::NodeBase") — the shape +# docs/features/relationships.md blesses, and the one whose short name collides with the +# target's. That matters: making is_self_join identity-based while the hetero TARGET +# search was still a bare compare let that search re-match this very reference (nothing +# excludes source_ref from it, unlike the directed self-join branch) and return +# (srcId, srcId) silently. Both searches are identity-based now, as in Java. XPKG_A = { "metadata.root": { "package": "a", @@ -255,7 +262,7 @@ def test_runtime_resolver_traverses_an_inherited_self_join() -> None: {"field.long": {"name": "srcId"}}, {"field.long": {"name": "dstId"}}, {"identity.primary": {"name": "id", "@fields": ["srcId", "dstId"]}}, - _ref("s", "srcId", "a::Node"), + _ref("s", "srcId", "a::NodeBase"), _ref("d", "dstId", "b::NodeBase"), ], ), @@ -289,3 +296,63 @@ def test_cross_package_target_sharing_a_subject_short_name_is_not_a_self_join() fields = derive_m2m_fields(rel, node, index) assert fields.source_field == "srcId" assert fields.target_field == "dstId" + + +# Java's deriveCrossPackageHeteroBindsCorrectPackage model, ported. No inheritance is +# needed to reach the same defect: ``a::Account`` relates to ``b::Account`` through a +# junction holding one reference to each. The two junction searches are INDEPENDENT, so +# a bare-name target match found ``ownerRef`` a second time and returned +# (ownerId, ownerId). Measured before the fix; pre-branch it threw loudly instead. +XPKG_HETERO_A = { + "metadata.root": { + "package": "a", + "children": [ + _entity( + "Account", + [ + {"field.long": {"name": "id"}}, + { + "relationship.association": { + "name": "partners", + "@cardinality": "many", + "@objectRef": "b::Account", + "@through": "AccountLink", + } + }, + _pk(), + ], + ), + _entity( + "AccountLink", + [ + {"field.long": {"name": "ownerId"}}, + {"field.long": {"name": "partnerId"}}, + {"identity.primary": {"name": "id", "@fields": ["ownerId", "partnerId"]}}, + _ref("ownerRef", "ownerId", "a::Account"), + _ref("partnerRef", "partnerId", "b::Account"), + ], + ), + ], + } +} + +XPKG_HETERO_B = { + "metadata.root": { + "package": "b", + "children": [_entity("Account", [{"field.long": {"name": "id"}}, _pk()])], + } +} + + +def test_cross_package_hetero_matches_each_reference_to_its_own_entity() -> None: + result = MetaDataLoader().load([ + InMemoryStringSource(json.dumps(XPKG_HETERO_A), id="a.json"), + InMemoryStringSource(json.dumps(XPKG_HETERO_B), id="b.json"), + ]) + assert result.errors == [] + objects = [c for c in result.root.children() if isinstance(c, MetaObject)] + account = next(o for o in objects if o.resolution_key() == "a::Account") + index = {o.name: o for o in objects} + fields = derive_m2m_fields(_rel(account, "partners"), account, index) + assert fields.source_field == "ownerId" + assert fields.target_field == "partnerId" # was "ownerId" diff --git a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts index b5f4abb27..c99a0de97 100644 --- a/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts +++ b/server/typescript/packages/metadata/src/core/relationship/derive-m2m-fields.ts @@ -190,7 +190,17 @@ export function deriveM2MFields( if (!isSelfJoin) { // Hetero: match each reference by the ENTITY OBJECT it resolves to. const sourceRef = refs.find((r) => isSubject(findEntity(root, r.targetEntity))); - const targetRef = refs.find((r) => r.targetEntity !== undefined && stripPackage(r.targetEntity) === stripPackage(targetName)); + // Identity here too. The two searches are INDEPENDENT — nothing excludes + // sourceRef from this one, unlike the directed self-join branch below — so a + // bare compare could match the SOURCE-side reference again whenever the + // target's short name equals the source's, and silently return (srcFk, srcFk). + // Structurally unreachable while isSelfJoin was also bare (a colliding short + // name forced the self-join branch); making only isSelfJoin identity-based + // broke that invariant. Java matches identity on both sides (findRefToSubject + // + findRefToObject) and never had the hole. + const targetRef = targetEntityNode !== undefined + ? refs.find((r) => findEntity(root, r.targetEntity) === targetEntityNode) + : refs.find((r) => r.targetEntity !== undefined && stripPackage(r.targetEntity) === stripPackage(targetName)); const sourceField = sourceRef ? refFkField(sourceRef) : undefined; const targetField = targetRef ? refFkField(targetRef) : undefined; if (sourceField === undefined || targetField === undefined) { diff --git a/server/typescript/packages/metadata/test/relationship-m2m.test.ts b/server/typescript/packages/metadata/test/relationship-m2m.test.ts index e408f0a10..de719e017 100644 --- a/server/typescript/packages/metadata/test/relationship-m2m.test.ts +++ b/server/typescript/packages/metadata/test/relationship-m2m.test.ts @@ -867,6 +867,14 @@ describe("FR-017 deriveM2MFields uses the DECLARING entity, not the visiting one // always done — this is the TS half of the pair with // M2MSlimVocabularyTest.deriveCrossPackageHeteroBindsCorrectPackage, and it // REDUCES the cross-port divergence rather than pinning it. + // + // The junction's SOURCE reference names the DECLARING BASE ("a::NodeBase") — the + // shape docs/features/relationships.md blesses, and the one whose short name + // collides with the target's. That matters: making isSelfJoin identity-based + // while the hetero TARGET search was still a bare compare let that search + // re-match this very reference (nothing excludes sourceRef from it, unlike the + // directed self-join branch) and return (srcId, srcId) silently. Both searches + // are identity-based now, as in Java. test("a cross-package hetero target sharing a subject short name is NOT a self-join", async () => { const aDoc = { "metadata.root": { package: "a", children: [ { "object.entity": { name: "Node", "extends": "a::NodeBase", children: [ @@ -879,7 +887,7 @@ describe("FR-017 deriveM2MFields uses the DECLARING entity, not the visiting one { "field.long": { name: "srcId" } }, { "field.long": { name: "dstId" } }, { "identity.primary": { "name": "id", "@fields": ["srcId", "dstId"] } }, - { "identity.reference": { name: "s", "@fields": ["srcId"], "@references": "a::Node" } }, + { "identity.reference": { name: "s", "@fields": ["srcId"], "@references": "a::NodeBase" } }, { "identity.reference": { name: "d", "@fields": ["dstId"], "@references": "b::NodeBase" } } ] } }, ] } }; const bDoc = { "metadata.root": { package: "b", children: [ @@ -904,6 +912,42 @@ describe("FR-017 deriveM2MFields uses the DECLARING entity, not the visiting one expect(derived.targetField).toBe("dstId"); }); + // Java's deriveCrossPackageHeteroBindsCorrectPackage model, ported. No inheritance + // is needed to reach the same defect: `a::Account` relates to `b::Account` through a + // junction holding one reference to each. The two junction searches are INDEPENDENT, + // so a bare-name target match found `ownerRef` a second time and returned + // (ownerId, ownerId). Measured before the fix; pre-branch it threw loudly instead. + test("cross-package hetero matches each junction reference to its OWN entity", async () => { + const aDoc = { "metadata.root": { package: "a", children: [ + { "object.entity": { name: "Account", children: [ + { "field.long": { name: "id" } }, + { "relationship.association": { name: "partners", "@cardinality": "many", + "@objectRef": "b::Account", "@through": "AccountLink" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + { "object.entity": { name: "AccountLink", children: [ + { "field.long": { name: "ownerId" } }, + { "field.long": { name: "partnerId" } }, + { "identity.primary": { "name": "id", "@fields": ["ownerId", "partnerId"] } }, + { "identity.reference": { name: "ownerRef", "@fields": ["ownerId"], "@references": "a::Account" } }, + { "identity.reference": { name: "partnerRef", "@fields": ["partnerId"], "@references": "b::Account" } } ] } }, + ] } }; + const bDoc = { "metadata.root": { package: "b", children: [ + { "object.entity": { name: "Account", children: [ + { "field.long": { name: "id" } }, + { "identity.primary": { "name": "id", "@fields": "id" } } ] } }, + ] } }; + const { root, errors } = await new MetaDataLoader().load([ + new InMemoryStringSource(JSON.stringify(aDoc), { id: "a.json" }), + new InMemoryStringSource(JSON.stringify(bDoc), { id: "b.json" }), + ]); + expect(errors).toHaveLength(0); + const account = root.objects().find((o) => o.resolutionKey() === "a::Account")!; + const rel = account.relationships().find((r) => r.name === "partners") as MetaRelationship; + const derived = deriveM2MFields(rel, account, root); + expect(derived.sourceField).toBe("ownerId"); + expect(derived.targetField).toBe("partnerId"); // was "ownerId" + }); + test("an OWN relationship still derives against its own entity (no regression)", async () => { // The override case: Sub re-declares `peers` itself, so rel.parent IS Sub and // the self-join must be classified against Sub, not the base it shadows. From 06daf4ff1adf2ba60777a51617a3b621eba61eaf Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Mon, 14 Sep 2026 01:24:59 -0400 Subject: [PATCH 12/12] refactor(python): delegate the reference head-parse; correct four stale doc claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fix round 4 — three items, no behaviour change beyond item 1's parse. (1) derive_m2m_fields' local head-parse is gone: _ref_target_raw (now _ref_target_qualified, since it was no longer returning anything raw) delegates to the canonical relationship_references.reference_target_entity. That helper searches the dot only AFTER the last "::" — it declines the assumption my hand-rolled find(".") made, that a package segment can never contain a dot. Unreachable today, but this file was holding a third copy of the same parse, and #372 proves the import is cycle-free. It is also the cleanest finish to the #372 seam: one parse, one behaviour, both sides of the same `if`. (2) Stale prose my own changes left behind: * _ref_target_qualified's summary still said "VERBATIM (package intact)" — it returns the entity head, not the verbatim value; * _ref_target_entity's KNOWN GAP still said a dotted junction "derives no M:N fields on this port". It does now; that function survives only on the defensive fallback path where @objectRef does not resolve at all, and the docstring says so; * M2MFields.cs had two stacked blocks, so the text describing FindEntity had reattached to ResolveEntity and FindEntity was undocumented. Split, and ResolveEntity's "Additive: no existing resolution changed" is corrected — true of the derivation, false of M2MNavigationBuilder.Build, which is the call site the change exists for. Its doc now also points at NamingRefs.ResolveObjectRef and #174 and says plainly that this is NOT the port's general resolver and has no referrer-package awareness. (3) CHANGELOG: "Scoped to that one predicate: no other name comparison changed" survived from round 2 and contradicted its own paragraph — replaced with what actually changed (both junction matches, the C# builder's resolution, Python's head-parse) and what did not (@through, everything outside M:N derivation). The "binds by luck" note now says both sides are affected, not just the subject, and the Python dotted-@references change is recorded: measured pre-branch as `must declare one identity.reference to "Post" and one to "Tag"`, and deriving (postId, tagId) now. Python suite re-run in full: 2044 passed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC --- CHANGELOG.md | 43 ++++++++------ .../Core/Relationship/M2MFields.cs | 39 ++++++++----- .../core/relationship/derive_m2m_fields.py | 57 +++++++++---------- 3 files changed, 81 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 301f8e87c..7a76ce1fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,16 +54,20 @@ here.** moves from "generates wrongly" to "the generation run fails", and the fix is to add `@symmetric` or `@sourceRefField`. - Two narrower resolution changes come with moving the junction match onto identity, both - matching what the Java port already did. A junction `@references` (or an `@objectRef`) - that **is** package-qualified must now resolve **exactly**: a partially-qualified or - stale package no longer falls back to matching the bare tail, so a reference that used - to bind by luck now does not match the subject. And a **bare** reference whose short - name exists in more than one package resolves first-declared-wins, which can pick the - wrong-package entity — that is the pre-existing + Three narrower resolution changes come with moving the junction matches onto identity, + all three matching what the Java port already did. A junction `@references` (or an + `@objectRef`) that **is** package-qualified must now resolve **exactly**: a + partially-qualified or stale package no longer falls back to matching the bare tail, so + a reference that used to bind by luck now matches neither the subject **nor** the + target — both sides are affected, not just the subject side. A **bare** reference whose + short name exists in more than one package resolves first-declared-wins, which can pick + the wrong-package entity — the pre-existing [#174](https://github.com/metaobjectsdev/metaobjects/issues/174) behaviour, now reached - by M:N derivation as well. Both are narrow, and both bring the other ports onto Java's - semantics rather than away from them. + by M:N derivation as well. And on **Python only**, a junction whose `@references` use + the dotted `Entity.field` form now resolves: that port compared the whole attr value, so + `Team.id` never matched the entity `Team` and an M:N through such a junction failed + derivation outright. Both junction matches now take the entity head through the same + canonical parse the loader uses, so those models derive where they previously raised. **Cross-port divergence goes DOWN, not up.** Both junction matches — "does this reference name the relationship's subject?" and "does this one name the target?" — now @@ -74,13 +78,20 @@ here.** same junction column as BOTH sides and emit `(srcFk, srcFk)` silently. C#'s `M2MNavigation` descriptor resolves its target the same way for the same reason — its `IsSelfJoin` feeds the EF `UsingEntity` wiring, and a descriptor that disagreed with - the derivation would mis-map the relationship. TypeScript, C# and Python - had been comparing package-stripped short names, so a genuine cross-package hetero M:N - onto a target whose short name matched the subject's (`a::NodeBase` relating to - `b::NodeBase`) was misread as a self-join on those three — a regression the two-name - subject introduced, caught in review and fixed here rather than documented. Scoped to - that one predicate: no other name comparison changed, and this is not a general - [ADR-0041](spec/decisions/ADR-0041-cross-package-reference-resolution.md) sweep. + the derivation would mis-map the relationship. TypeScript, C# and Python had been + comparing package-stripped short names, so a genuine cross-package hetero M:N onto a + target whose short name matched the subject's (`a::NodeBase` relating to `b::NodeBase`) + was misread as a self-join on those three — a regression the two-name subject + introduced, caught in review and fixed rather than documented. + + What changed, exactly: both junction matches in the four derivations, the C# navigation + builder's own target/junction resolution (so the descriptor cannot disagree with the + derivation feeding it), and Python's reference head-parse, which now delegates to the + loader's canonical helper instead of keeping a third copy. `@through` resolution and + every comparison outside M:N derivation are untouched, and this is **not** a general + [ADR-0041](spec/decisions/ADR-0041-cross-package-reference-resolution.md) sweep — the + resolver added here is deliberately narrow and is not the port's general reference + resolver. No vocabulary change: `metamodelVersion` stays `1.0` and the registry manifest is untouched. diff --git a/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs b/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs index eb4671a89..789a207f4 100644 --- a/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs +++ b/server/csharp/MetaObjects/Core/Relationship/M2MFields.cs @@ -78,6 +78,28 @@ private static string StripPackage(string name) return idx < 0 ? name : name[(idx + PACKAGE_SEPARATOR.Length)..]; } + /// + /// , exposed so a codegen DESCRIPTOR can resolve an entity + /// reference by the SAME rule this derivation uses. + /// + /// + /// M2MNavigation.IsSelfJoin compares the descriptor's target against + /// its source by identity; while M2MNavigationBuilder.Build resolved that + /// target by a package-stripped name and the derivation resolved it exactly, the two + /// disagreed — b::Account bound to a same-short-named a::Account, which + /// then compared identity-equal to the source and reported a cross-package hetero M:N + /// as a self-join, and the EF wiring follows the descriptor. Build now calls this. + /// So this is additive to the DERIVATION — no derivation behaviour changed — but it + /// deliberately DID change resolution at that one call site, which is the point. + /// NOT the port's general reference resolver. It is deliberately narrow (see + /// ) and has no referrer-package awareness: a bare name is + /// resolved against every root object, not against the referrer's package first. + /// NamingRefs.ResolveObjectRef is the package-aware resolver the loader uses; + /// prefer it anywhere that is not matching a junction reference to an entity, and see + /// issue #174 for the bare-collision case both leave open. + /// + public static MetaObject? ResolveEntity(MetaRoot root, string? name) => FindEntity(root, name); + /// /// The root entity a reference name denotes, or null. Mirrors the Java /// reference's M2MFields.findObject exactly: a FULLY-QUALIFIED name (one @@ -85,21 +107,12 @@ private static string StripPackage(string name) /// a bare-tail fallback; a bare name matches a short name, first match wins (the /// bare-collision case is the deferred follow-up Java records as issue #174). /// - /// This exists so the SUBJECT comparison can be made on object IDENTITY the way - /// Java's already is. A bare-name compare cannot tell a::NodeBase from + /// This exists so the junction matches can be made on object IDENTITY the way Java's + /// already are. A bare-name compare cannot tell a::NodeBase from /// b::NodeBase, which made a genuine cross-package hetero M:N read as a - /// self-join the moment the subject set held two names. + /// self-join the moment the subject set held two names — and, once the subject side + /// alone was fixed, made the target search re-match the source-side reference. /// - /// - /// Public so codegen descriptors can resolve an entity reference by the SAME rule - /// the derivation uses. M2MNavigation.IsSelfJoin compares the descriptor's - /// target against its source by identity; if the builder resolved the target by a - /// package-stripped name while the derivation resolved it exactly, the two could - /// disagree about whether a relationship is a self-join — and the EF wiring follows - /// the descriptor. Additive: no existing resolution changed. - /// - public static MetaObject? ResolveEntity(MetaRoot root, string? name) => FindEntity(root, name); - private static MetaObject? FindEntity(MetaRoot root, string? name) { if (string.IsNullOrEmpty(name)) return null; diff --git a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py index fe2fff0b6..9858538be 100644 --- a/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py +++ b/server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py @@ -52,6 +52,7 @@ IDENTITY_SUBTYPE_REFERENCE, ) from .meta_relationship import MetaRelationship +from .relationship_references import reference_target_entity class M2MDerivationError(Exception): @@ -102,41 +103,39 @@ def _ref_fk_field(ref: MetaData) -> str | None: def _ref_target_entity(ref: MetaData) -> str | None: """The @references target-entity name of a reference (bare, package-stripped). - KNOWN GAP (pre-dates #368, deliberately NOT fixed here): this compares the - WHOLE @references value, so the dotted ``Entity.field`` form ("Team.id") - never matches a bare entity name — a junction whose references are authored - dotted derives no M:N fields on this port, where TS's derive-m2m-fields.ts - (which reads ``ref.targetEntity``) resolves them. The one-line repair is to - delegate to ``relationship_references.reference_target_entity``; it is left - alone because it would change M:N derivation behaviour, which is outside the - #368 fix. Tracked separately from the rule-(e) ladder, whose copy of this - blind spot IS fixed. + Compares the WHOLE @references value, so the dotted ``Entity.field`` form + ("Team.id") never matches a bare entity name. That blind spot NO LONGER + affects M:N derivation: both junction matches now run through + :func:`_ref_target_qualified` (which delegates to the canonical + ``reference_target_entity``), and this function is reached only on the + DEFENSIVE fallback path — when ``@objectRef`` does not resolve to an entity at + all, which loader validation normally prevents. It is left package-stripped + because that fallback is deliberately the pre-identity behaviour; the + remaining copy of the dotted blind spot is being closed on its own branch. """ v = ref.get_meta_attr(IDENTITY_REFERENCE_ATTR_REFERENCES) # ADR-0039: resolving (identity attr) return _strip_package(v) if isinstance(v, str) and v else None -def _ref_target_raw(ref: MetaData) -> str | None: - """The @references value of a reference, VERBATIM (package intact). +def _ref_target_qualified(ref: MetaData) -> str | None: + """The target-entity half of a reference's ``@references``, PACKAGE INTACT. - Distinct from :func:`_ref_target_entity`, which strips the PACKAGE for the - legacy bare-name compare. This keeps the package — identity resolution needs - the qualified form to tell ``a::NodeBase`` from ``b::NodeBase`` — and takes the - entity head of the dotted ``Entity.field`` form, since packages use ``::`` and - never ``.``, so the first ``.`` splits the entity off. That matches the TS - ``MetaIdentity.targetEntity``, the C# ``TargetEntity`` and Java's - ``refTargetObject``, so all four resolve a dotted junction reference alike. + Distinct from :func:`_ref_target_entity`, which strips the package for the + legacy bare-name compare. Identity resolution needs the qualified form to tell + ``a::NodeBase`` from ``b::NodeBase``, and needs the dotted ``Entity.field`` + form reduced to its entity head — which is exactly what the canonical + :func:`~metaobjects.meta.core.relationship.relationship_references.reference_target_entity` + already computes, so this delegates rather than keeping a third copy of the + parse. That helper searches the dot only AFTER the last ``::``, so a package + segment can never be mistaken for the field separator; a local ``find(".")`` + would have assumed packages never contain a dot instead of declining the + assumption. - (The dotted blind spot on :func:`_ref_target_entity` is a separate, older gap - being closed on its own branch. Splitting HERE removes the seam: without it the - two sides of the same ``if`` would disagree about dotted references once that - lands.) + Delegating also finishes the seam with the branch that makes + :func:`_ref_target_entity` split: one parse, one behaviour, both sides of the + same ``if``. """ - v = ref.get_meta_attr(IDENTITY_REFERENCE_ATTR_REFERENCES) # ADR-0039: resolving - if not isinstance(v, str) or not v: - return None - dot = v.find(".") - return v if dot < 0 else v[:dot] + return reference_target_entity(ref) def _root_objects(node: MetaData) -> list[MetaData]: @@ -267,7 +266,7 @@ def _is_subject(entity: MetaData | None) -> bool: ( r for r in refs - if _is_subject(_find_entity(root_objects, _ref_target_raw(r))) + if _is_subject(_find_entity(root_objects, _ref_target_qualified(r))) ), None, ) @@ -281,7 +280,7 @@ def _is_subject(entity: MetaData | None) -> bool: ( r for r in refs - if _find_entity(root_objects, _ref_target_raw(r)) + if _find_entity(root_objects, _ref_target_qualified(r)) is target_entity_node ), None,