Skip to content

fix(mapper): validate join machinery instead of silently mis-resolving - #211

Open
shihyuho wants to merge 1 commit into
jakartafrom
fix/wp3-join-machinery
Open

fix(mapper): validate join machinery instead of silently mis-resolving#211
shihyuho wants to merge 1 commit into
jakartafrom
fix/wp3-join-machinery

Conversation

@shihyuho

Copy link
Copy Markdown
Member

Closes #198

WP3 — Join machinery. The cluster's root cause was shared: the join resolvers assumed well-formed, well-ordered input and silently mis-handled anything else. One pass, validate-or-fail-loud.

Findings

Finding What changed How it was verified
COR-05query.distinct() last-writer-wins Join.toPredicate now accumulates: query.distinct(query.isDistinct() || distinct), so a join declared distinct=false can no longer undo an earlier distinct=true. distinctShouldBeAccumulatedAcrossJoins — two class-level joins with opposing flags; asserts query.isDistinct(). Fails on jakarta (flag ends up false).
COR-06 — conflicting definitions sharing an alias silently dropped When the alias is already registered, Join compares the stored join's parent, attribute name and joinType against the new definition and throws IllegalArgumentException naming the alias, the existing joinType + attribute, and the conflicting joinType + path. Identical redefinitions still reuse the existing join (existing dedup behaviour preserved). conflictingPathOnSameAliasShouldThrow (orders vs badges on alias shared) and conflictingJoinTypeOnSameAliasShouldThrow (INNER vs LEFT on alias order). Both fail on jakarta (no exception).
COR-07 — dotted path drops segments after the second Join validates byDot.length == 2 and throws, citing the actual segment count and the two-segment '<parent-alias>.<association>' limit, with the advice to define an intermediate join. joinPathWithMoreThanTwoSegmentsShouldThrow (o.tags.name). Fails on jakarta — it silently joined o.tags.
COR-10 — unregistered alias silently falls back to root.get(alias) SimpleSpecification.getExpr routes the fallback through getAttribute(..), which wraps any provider failure in an IllegalArgumentException naming the segment, the full path, the entity, and the declaration-order requirement (including that a join on a null-valued field is never applied). unregisteredJoinAliasShouldThrow — reproduces the finding exactly: the @Join sits on a null-valued field so alias o is never registered, while a second field references o.itemName. Fails on jakarta (opaque Hibernate PathElementException).
COR-11 — count and content resolve a @JoinFetch alias with different join semantics getFetchPath resolves through the fetch node itself (a Path in Hibernate), which carries the declared joinType, instead of re-navigating root.get(..) — which emitted an implicit inner join. Content and count now agree. Falls back to the old path-walk if a provider's Fetch is not a Path. nonInnerJoinFetchShouldKeepCountAndContentInSyncLEFT @JoinFetch + IsNull on the joined column, with a customer that has no orders. On jakarta the content query returned [] while count returned 1; now both find him, and the page assertion uses PageRequest.of(0, 1) so Spring Data cannot skip the count query.
MAINT-04 — bare NoSuchElementException on direct construction SimpleSpecification resolves CTX_JOIN through joinContext(), which translates the missing key into an IllegalStateException explaining that multi-segment paths need the registry SpecMapper populates, with the original exception as cause. multiSegmentPathWithoutJoinContextShouldExplainTheMapperPipeline — directly-constructed spec with a dotted path over an empty SpecContext.
PERF-02joined/fetched grow per execution of a reused Specification SpecJoinContext now keys both maps weakly by Root, so bookkeeping dies with the criteria tree that owns it. The criteria nodes stored as values reference their Root, so they are held weakly too — a strong value would keep its own key reachable and defeat the weak key entirely. That is safe because Hibernate's AbstractSqmFrom owns every join and fetch built from it (addSqmJoin), keeping the referents alive for as long as the execution can reach them. No size cap. shouldNotRetainBookkeepingOfCollectedRoot — registers a join and a fetch, drops the criteria tree, and asserts under await() that both maps empty out once the Root is collected. Fails on jakarta (times out; entries never clear). Ran 3× for flakiness. Plus reusedSpecificationShouldStayConsistentAcrossExecutions as a functional regression guard.
TEST-04 — no coverage of join edge/error paths 9 new tests across JoinSpecificationResolverTest, JoinFetchSpecificationResolverTest, SpecJoinContextTest and ConstructSimpleSpecificationTest. 8 of the 9 were confirmed discriminating: with mapper/src/main reverted to jakarta and only the new tests applied, all 8 fail. The 9th is a deliberate regression guard that passes on both.

Release note

Tightened validation — several silent-wrong-result paths now throw. Configurations that previously produced a quietly incorrect query now fail fast:

  • two @Join declarations sharing an alias but differing in path or joinTypeIllegalArgumentException
  • a join path with more than two dot-separated segments → IllegalArgumentException
  • a @Spec path whose first segment is neither a registered join alias nor an entity attribute → IllegalArgumentException (previously fell back to a same-named attribute on the root)
  • a SimpleSpecification with a multi-segment path constructed outside SpecMapperIllegalStateException (previously a bare NoSuchElementException)

Also behavioural: distinct now accumulates across joins rather than being decided by the last join executed, and predicates on a non-INNER @JoinFetch alias now use the declared join type in the content query, so page totals match page content.

Verification

make test green — 107 mapper tests + 15 starter tests, 0 failures. Run on Temurin 17.0.6 (the project's declared java.version; the machine default resolves to JDK 25, switched via SDKMAN for the build only — no pom changes).

Deliberately not done

  • JoinFetch.java is outside this work package's file allowlist, so two things were left alone even though they touch the same findings:
    • JoinFetch.toPredicate still calls query.distinct(distinct) unconditionally — the COR-05 last-writer-wins hazard survives for @JoinFetch, and for criteria that mix @Join with @JoinFetch. The Join side is fixed, and count queries inherit the fix through the delegation, but the content-query path in JoinFetch needs the same one-line change. Worth a follow-up issue — same finding, sibling class.
    • JoinFetch.fetch(..) has the COR-07 twin (no byDot.length validation) and the COR-06 twin (early return without comparing the stored definition).
  • [WP2] @JoinFetch on paged queries forces Hibernate in-memory pagination (+ delete fetch heuristic) #192 (WP2) boundary: shouldDelegateToJoin / delegatePredicateToJoin were not touched. COR-11 was fixed entirely on the resolution side in SimpleSpecification, so the count query's semantics are unchanged and nothing here collides with WP2's redesign of the delegation.
  • COR-10 keeps the root.get(field) fallback rather than throwing on every unregistered alias. Throwing unconditionally would break legitimate nested attribute paths (@Spec(path = "address.city")), which are indistinguishable from aliases at this layer — the registry records aliases only at toPredicate time and carries no record of which aliases were declared. Making the fallback's failure loud and descriptive is the strongest check available without changing the JoinContext interface (also outside the allowlist). One residual case stays silent: an alias that collides with a real attribute name (e.g. the default alias of @Join(path = "orders") is orders) still resolves to that attribute when the join was not applied.

🤖 Generated with Claude Code

The join resolvers assumed well-formed, well-ordered input and silently
mis-handled anything else. Validate or fail loud in one pass:

- Join.toPredicate accumulates the distinct flag rather than overwriting
  it, so a later join can no longer undo an earlier distinct=true.
- Registering an alias that already exists now compares the stored
  parent, attribute and joinType, and throws when they conflict instead
  of keeping the first definition.
- Join paths deeper than two segments throw instead of silently joining
  only the first two.
- A dotted path whose first segment resolves to neither a registered
  join/fetch alias nor a root attribute throws a message naming the
  alias and the declaration-order requirement, rather than falling back
  to a same-named attribute on the root.
- A missing JoinContext (direct construction outside SpecMapper) throws
  a message explaining the mapper-pipeline requirement instead of a bare
  NoSuchElementException.
- Fetch aliases resolve through the fetch node itself, which carries the
  declared joinType, so a non-INNER @JoinFetch no longer restricts the
  content query differently from the count query.
- Join/fetch bookkeeping is keyed weakly by Root so a reused
  Specification no longer accumulates one entry per execution.

Several silent-wrong-result paths now throw.

Co-authored-by: Claude Opus 4.8 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[WP3] Join machinery: silent-wrong-result edge cases + missing edge/error tests

1 participant