fix: M:N derivation classified inherited relationships against the visiting entity, not the declaring one - #373
Merged
Conversation
…he visiting one 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
… the visiting one 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
…ntity as the M:N subject 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
… entity as the M:N subject 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
…ect, not the visiting entity 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
… the declaring entity 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
…rges
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
…e Java port
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
…align IsSelfJoin 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
…le doc claims
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 <summary> 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC
…-declaring-entity # Conflicts: # CHANGELOG.md # server/python/src/metaobjects/meta/core/relationship/derive_m2m_fields.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found while reviewing #368. Stacked on
fix/368-assoc-ref-disambiguation(#371).M:N field derivation decided whether a relationship is a self-join by comparing
@objectRefagainst a passed-in source entity — and every caller populated that from the entity it was iterating, not the entity that declares the relationship.relationships()is resolving, so underextendsthose differ.An inherited M:N relationship was therefore misclassified and derivation threw. In C# the exception is caught and swallowed, so the generated navigation was silently dropped; on Java/Kotlin/Python the generation run fails instead. The same misclassification reaches the runtime M:N resolvers, not just codegen.
12 call sites across all five ports (Kotlin has no derivation of its own — it calls the Java SSOT). Fixed in the derivation itself, one change per port, so a future caller cannot reintroduce it.
The subject rule
The accepted subject is either the declaring entity or the navigating entity, and only those two. Both are legitimate: a junction FK may reference the declaring base, or the concrete child — the latter being the common case, since an abstract base has no table. That contract is now documented in
docs/features/relationships.md; it previously existed only in source comments.This is not a pure widening. One shape that derived before now refuses: base declares
@objectRef: "Base"+@throughwith neither@symmetricnor@sourceRefField, and the junction references both the child and the base. Pre-fix that derived(childFk, baseFk)from the child while throwing from the base — inconsistent per visitor, with an arbitrary FK direction. Refusing is correct, but it is a behaviour change, and on the ports whose callers do not catch, a generation run that previously produced output now fails.Comparison is by resolved entity identity
Both junction-reference matches — subject side and target side — resolve the name to an entity and compare identity, mirroring
M2MFields.java, which never had the collision. Two review rounds were needed to get here:b::NodeBasecollide with a subject set holdinga::NodeBase, turning a cross-package hetero M:N into a spurious self-join. That was a regression this branch introduced, caught by measuring pre-branch behaviour.(ownerId, ownerId)) on exactly the model Java'sderiveCrossPackageHeteroBindsCorrectPackagegates. Converting both restores the invariant structurally — in the hetero branch the target is by construction not a subject, so the two matches cannot collide.C#'s
M2MNavigation.IsSelfJoinis identity-based for the same reason, andM2MNavigationBuilder.Buildresolves through the derivation's resolver so the descriptor and the derivation cannot disagree — otherwise a silent drop becomes silently wrong EFUsingEntityconfiguration one layer up.Two narrow resolution consequences, both matching Java and both recorded in the changelog: a package-qualified
@referencesmust now resolve exactly (no bare-tail fallback), and a bare colliding one binds first-declared (#174).Also here
Python's
_ref_target_qualifiednow delegates to the canonicalreference_target_entityhead-parse instead of hand-rolling a third copy in the same file. That incidentally fixes Python deriving nothing for a junction authored with dotted@references— measured:@references: "Post.id"/"Tag.id"previously threw, now derives(postId, tagId)— and removes a semantic half-merge with #372, which makes the sibling function split the same way. The two branches still touch adjacent hunks; the conflict is now trivial.Verification
25 new tests across five ports, each confirmed failing against the pre-fix code — including reverting the C# derivation and the
IsSelfJoinhalf separately, and checking out pre-branch source rather than hand-editing. Counter-cases pin that both legitimate junction shapes still derive, andtest_n2m_resolver_inherited.pywas run explicitly.Not order-gated, unlike #368's loader passes: there is no
checkeddedupe here, so the bug fired for every inheriting entity in any order. The fixtures still pin visit order, but the tests do not depend on it.TS 2716 · 1859 · 454 · 46 · Java 1622 · 252 · 378 · 58 · C# 456 · 1088 · 78 · 291 · Python 2044 — all 0 fail.
scripts/ci-local.sh --quickpasses. Corpus stays 329,metamodelVersion1.0,expected-registry.jsonuntouched.No conformance fixture.
fixtures/conformance/is load→serialize round-trip and cannot surface a derivation result. The behaviour corpora (api-contract-conformance,persistence-conformance) could, but adding a scenario there executes immediately across 5 ports × 2 lanes against hand-rolled reference servers with hard-coded tables, seeds and routes — roughly ten server files in four languages, with no half-added green state. Ruled out with that reason stated rather than waved past;persistence-conformanceis the cheaper follow-up.Known follow-ups, not fixed here
@objectRefnaming an abstract entity makes TS codegen import a table const that is never generated (verified to occur for a plain 1:1 too — general, merely exposed via M:N). C#'sroot.FindObjecthas no FQN fallback, andM2MFields.cs's@throughresolution is unstripped. The derivation's bare resolution is first-match-wins, which ADR-0042 retired in favour of package-local resolution — so the four derivations agree with each other and with Java, but not with the loader (#174 / ADR-0042 territory).Note on history:
a25dbef6pins the bare-name behaviour as a known divergence; rounds 2-4 then establish it was a regression and fix it. The commit is left in place rather than rewritten, since the correction is part of the record.🤖 Generated with Claude Code
https://claude.ai/code/session_01Lfd8nat1WcSpXffd8YetiC