Skip to content

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

Closed as not planned

Description

@shihyuho

此內容由 AI 產生(specification-mapper 全庫審查 2026-07-16)。

Work package WP3 · findings: COR-07, COR-05, COR-06, COR-08, COR-11, COR-10, PERF-02, TEST-04 · worst severity: medium

From the 2026-07-16 whole-codebase review (branch jakarta, modules mapper/ + starter/). Each finding below is CONFIRMED by independent adversarial verification. File paths and line numbers reflect the review snapshot and may drift — treat them as starting points, not exact coordinates.


COR-07 — Dotted join path silently drops every segment after the second

  • Category / severity: correctness / medium · effort S · confidence high
  • Location: mapper/src/main/java/tw/com/softleader/data/jpa/spec/domain/Join.java:112
  • Evidence: Join.join() splits pathToJoinOn on '.' and uses only byDot[0] (alias lookup) and byDot[1] (join attribute); byDot[2..] are never read and no length check exists. The default alias, however, is derived from the FULL path (pathToJoinOn.replace('.', '_') at line 75), so a 3-segment path registers a 2-level join under a 3-level-looking alias.
  • Failure scenario: @Join(path = "o.tags.category") (with alias 'o' registered earlier) creates only the join o.tags but registers it as alias 'o_tags_category'. An @SPEC(path = "o_tags_category.name") then filters tags.name instead of category.name — either a Hibernate PathElementException if 'name' does not exist on Tag, or silently wrong results if an attribute with the same name exists on both entities.
  • Suggested fix: Validate byDot.length == 2 and throw an IllegalArgumentException naming the unsupported path (mirroring the existing missing-alias message), or implement N-level resolution by looping over the remaining segments.
  • Verification: [CONFIRMED] Traced Join.java directly. Line 75 derives the default alias from the FULL path (pathToJoinOn.replace(".", "_")). Line 102 splits pathToJoinOn on ".", but only byDot[0] (line 104, alias lookup) and byDot[1] (line 112, join attribute) are ever read; byDot[2..] are ignored and there is no byDot.length check. JoinSpecificationResolver.java:113-115 forwards def.path() unvalidated into the constructor, so no upstream guard exists. Thus a 3-segment path like "o.tags.category" (alias "o" registered earlier) creates only the join o.tags yet registers it under alias "o_tags_category", and a later @SPEC(path="o_tags_category.name") resolves against the tags join — either a Hibernate PathEl…

COR-05 — query.distinct() is overwritten by the last-executed join spec — distinct flag is last-writer-wins

  • Category / severity: correctness / medium · effort S · confidence high
  • Location: mapper/src/main/java/tw/com/softleader/data/jpa/spec/domain/Join.java:84
  • Evidence: Join.toPredicate() unconditionally calls query.distinct(distinct) (JoinFetch.toPredicate does the same at JoinFetch.java:83). When a POJO carries multiple @Join/@JoinFetch annotations with different distinct values, each spec's toPredicate overwrites the query-global flag, so the value of the join that happens to execute last (field declaration order) wins.
  • Failure scenario: class Criteria { @Join(path="orders", alias="o", distinct=true) ... fieldA; @Join(path="tags", alias="t", distinct=false) ... fieldB; } — fieldB's spec executes last and sets distinct(false), so the to-many join on orders returns duplicate root entities even though the first annotation (and the annotation default) requested distinct. Reordering the fields silently flips the result set.
  • Suggested fix: Make distinct accumulate (e.g., only ever set distinct(true) when requested, never reset to false; or resolve a single effective flag per query at mapping time) and document that distinct is query-global, not per-join.
  • Verification: [CONFIRMED] Traced Join.java:84 — query.distinct(distinct) is called unconditionally whenever query != null. JoinFetch.java:83 does the same, and its number-result delegate path (lines 94-98) forwards distinct into a new Join, which again sets it unconditionally. JPA's query.distinct() sets a single query-global boolean, so when a POJO carries multiple @Join/@JoinFetch with differing distinct values, each spec's toPredicate overwrites the flag and the last-executed (field declaration order) wins. The annotation default is true (annotation/Join.java:72) to prevent duplicate roots, so an explicit distinct=false on a later join silently disables deduplication for the whole query, and reordering…

COR-06 — Conflicting join definitions sharing an alias are silently dropped — first registration wins without any compatibility check

  • Category / severity: correctness / medium · effort S · confidence high
  • Location: mapper/src/main/java/tw/com/softleader/data/jpa/spec/domain/Join.java:94
  • Evidence: Join.join() returns early when jc.getJoin(root, alias) != null, without verifying that the existing join was created from the same path/joinType. The dedup test (JoinSpecificationResolverTest.duplicateAliasJoinOnField) only covers identical definitions; JoinFetch.fetch() has the same unchecked skip at JoinFetch.java:104.
  • Failure scenario: Copy-paste error: @Join(path="orders", alias="x") on one field and @Join(path="schools", alias="x") on another. The second join is silently skipped; @SPEC(path="x.name") intended for schools filters orders.name instead — wrong rows returned with no error. A joinType conflict (INNER vs LEFT) under one alias is likewise silently resolved to whichever executed first.
  • Suggested fix: When the alias already exists, compare the stored join's path and joinType with the requested ones and throw on mismatch instead of silently skipping.
  • Verification: [CONFIRMED] Traced Join.java:90-114 and JoinFetch.java:100-125. Both guard clauses (Join.java:94 jc.getJoin(root, alias) != null, JoinFetch.java:104 jc.getFetch(root, alias) != null) return early on alias collision with zero comparison of the existing join's path or joinType — confirmed verbatim, matching the comment "skip creating a new join". The alias is the map key; when two fields explicitly share an alias but declare different paths/joinTypes, the first-composed spec registers the join and the second is silently dropped, so a later @SPEC(path="alias.name") resolves against the wrong association — wrong rows, no error. The failure scenario is reachable and reproducible. It requires …

COR-08 — Number-result-type delegation heuristic does not cover delete(Specification), contradicting its own comment

  • Category / severity: correctness / medium · effort M · confidence medium
  • Location: mapper/src/main/java/tw/com/softleader/data/jpa/spec/domain/JoinFetch.java:91
  • Evidence: The comment at line 89 says 'delegate to join for number result type queries, e.g. count or delete', but Spring Data 3.5.x SimpleJpaRepository.delete(Specification) invokes spec.toPredicate(delete.from(domainClass), builder.createQuery(getDomainClass()), builder) — the throwaway CriteriaQuery's result type is the ENTITY class, not a Number (verified in spring-data-jpa-3.5.12-sources, SimpleJpaRepository lines 493-509). shouldDelegateToJoin() therefore returns false and fetch(root) executes root.fetch(...) on the CriteriaDelete's root.
  • Failure scenario: repository.delete(mapper.toSpec(pojo, Customer.class)) where the POJO carries @JoinFetch: a fetch join is attached to the bulk-delete root; Hibernate rejects joins/fetches in mutation queries at translation time, so the delete throws instead of executing (and even plain @Join predicates hit the same joins-in-bulk-delete restriction).
  • Suggested fix: Detect mutation usage more robustly — e.g., delegate (or no-op the fetch) whenever query.getResultType() is not the root's java type, and document that @Join/@JoinFetch criteria are not usable with delete(Specification).
  • Verification: [CONFIRMED] Traced JoinFetch.java:90-92: shouldDelegateToJoin delegates only when query.getResultType() is a Number. Verified SimpleJpaRepository.delete(Specification) in the local spring-data-jpa clone (tag 3.5.10, matching spring-boot 3.5.15 in pom.xml; identical to cited 3.5.12): it calls spec.toPredicate(delete.from(domainClass), builder.createQuery(getDomainClass()), builder). The throwaway query's result type is the ENTITY class, so Number.isAssignableFrom(...) is false, shouldDelegateToJoin returns false, and fetch(root) runs root.fetch(...) on the CriteriaDelete root — exactly as claimed, and directly contradicting the line-89 comment ('e.g. count or delete'). Count genuinely delegat…

COR-11 — Count and content queries resolve a @JoinFetch alias with different join semantics — page totals can disagree with page content for non-INNER fetches

  • Category / severity: correctness / medium · effort M · confidence medium
  • Location: mapper/src/main/java/tw/com/softleader/data/jpa/spec/domain/SimpleSpecification.java:100
  • Evidence: For the content query, an @SPEC path through a fetch alias resolves via getFetchPath(), which rebuilds the path with root.get(...) chains — an implicit join, which is INNER per JPA semantics, ignoring the annotation's joinType. For the count query, JoinFetch delegates to Join (JoinFetch.java:80-82), which creates an explicit root.join(path, joinType) honoring the declared joinType (e.g. LEFT). The same alias therefore maps to a LEFT join in count queries and an INNER join in content queries.
  • Failure scenario: @JoinFetch(path="orders", alias="o", joinType=LEFT) with @SPEC(path="o.itemName", value=IsNull.class): countBySpec/existsBySpec count customers WITHOUT orders (LEFT join + IS NULL), while findBySpec's content query (implicit INNER join) returns none of them — findBySpec(pojo, pageable) reports totalElements=N with empty content.
  • Suggested fix: Resolve fetch aliases consistently: for content queries cast the stored Fetch to Join where the provider allows it (Hibernate's fetch implements Join) or create an explicit join with the declared joinType instead of implicit root.get() navigation.
  • Verification: [CONFIRMED] Traced the code end to end. SimpleSpecification.getFetchPath (lines 99-105) resolves a @SPEC path through a fetch alias by rebuilding from the root via current.get(path) — implicit INNER-join navigation that ignores the fetch's declared joinType and does not reuse ref.fetch(). JoinFetch.toPredicate (lines 78-96) delegates to Join for Number result-type queries (count/exists/delete), and Join.join (Join.java:96) creates root.join(pathToJoinOn, joinType) stored under the alias, which the @SPEC then resolves via getJoin honoring the declared joinType (e.g. LEFT). So the same alias maps to LEFT in count queries and INNER in content queries — exactly as claimed. JoinFetchSpecification…

COR-10 — A @SPEC path referencing an unregistered join alias silently falls back to root.get(alias) — declaration order and null-valued join fields break alias resolution

  • Category / severity: correctness / medium · effort M · confidence high
  • Location: mapper/src/main/java/tw/com/softleader/data/jpa/spec/domain/SimpleSpecification.java:86
  • Evidence: getExpr() falls back to root.get(field) when the alias is in neither the joined nor fetched map. Aliases are only registered once the owning Join/JoinFetch spec's toPredicate has ALREADY executed, and execution order equals field declaration order (CompoundSpecification reduces the specs list left-to-right; ReflectionDatabind.of appends fields in declaration order). Additionally, joinsDefOnField() in both resolvers (JoinSpecificationResolver.java:78) skips the join entirely when the annotated field's value is empty.
  • Failure scenario: Field A declares @SPEC(path="o.itemName") and a LATER field B declares @Join(path="orders", alias="o") — or B carries the @Join but arrives null in the web form while A is set. In both cases 'o' is unregistered when A's predicate is built, so root.get("o") is attempted: Hibernate throws PathElementException ('could not resolve attribute o') with no hint about alias ordering; if the alias happens to equal a real attribute name (e.g. default alias 'orders'), an unintended implicit INNER join is created instead, ignoring the declared joinType.
  • Suggested fix: When a multi-segment @SPEC path's first segment is not a registered alias, fail with an explicit 'alias not registered — declare the @Join before or on the same field' error; alternatively register field-level joins independently of the field value.
  • Verification: [CONFIRMED] Traced the complete path. SimpleSpecification.java:86 getExpr does fall back to root.get(field) when the first path segment is registered in neither the joined nor fetched maps (getJoin/getFetch return the JoinContext entry; only .orElseGet(() -> root.get(field)) otherwise). Confirmed the ordering claims: (1) JoinContext registration happens only inside Join.toPredicate (domain/Join.java:80-99, jc.putIfAbsent(...)); (2) CompoundSpecification.java:44-47 reduces the specs list left-to-right, and Conjunction/Disjunction combine builds result.and(element), so earlier specs' toPredicate runs first; (3) SpecMapper.toSpec:107-111 flatMaps over ReflectionDatabind.of, which …

PERF-02 — joined/fetched maps keyed by Root grow on every execution of a reused Specification and are never cleaned

  • Category / severity: performance / medium · effort S · confidence high
  • Location: mapper/src/main/java/tw/com/softleader/data/jpa/spec/SpecJoinContext.java:41
  • Evidence: SpecJoinContext.joined and fetched are plain synchronized HashMaps keyed by JoinKey(Root, alias)/FetchKey(Root, alias). Every toPredicate execution uses a fresh Root, so putIfAbsent adds new entries each time the same Specification object is executed (even a single findBySpec(pojo, pageable) adds two: count root + content root). Nothing ever removes entries, and each entry strongly references the provider's criteria Join/Fetch node, which pins the whole query AST.
  • Failure scenario: An application caches a mapped Specification (e.g. a constant 'active customers' spec, or composes toSpec output into a stored filter) and executes it per request: every request permanently adds Root-keyed entries plus their retained Hibernate SqmJoin graphs to the spec's SpecJoinContext — unbounded heap growth leading to OOM under sustained traffic.
  • Suggested fix: Key the per-execution state off the query lifecycle instead of accumulating in the spec — e.g. use identity-based weak keys for Root or scope join registrations to a per-toPredicate map.
  • Verification: [CONFIRMED] Traced the lifecycle end-to-end. SpecMapper.toSpec(rootObject) (SpecMapper.java:84) creates one SpecJoinContext per call, captured as a strong field by every built spec (SimpleSpecification.context, Join.context, JoinFetch.context). putIfAbsent(root, alias, ...) runs at toPredicate/execution time (Join.java:99/113, JoinFetch.java:109/123), keyed by JoinKey(Root, alias)/FetchKey(Root, alias) in plain synchronized HashMaps (SpecJoinContext.java:41-42). Spring supplies a fresh Root per execution, so each run adds new entries; nothing ever removes them, and the keys/values strongly retain the Hibernate Root and Join/FetchRef Sqm nodes. So a reused Specification object accumulates ent…

TEST-04 — Join machinery edge and error paths have no test coverage

  • Category / severity: testing / medium · effort M · confidence high
  • Location: mapper/src/test/java/tw/com/softleader/data/jpa/spec/JoinSpecificationResolverTest.java:48
  • Evidence: JoinSpecificationResolverTest, JoinFetchSpecificationResolverTest and JoinFetchElementCollectionTest only exercise happy paths with default INNER/distinct=true: there is no test for a 3+-segment dotted path, for the documented IllegalArgumentException when the first-segment alias is unregistered, for conflicting definitions sharing an alias, for joinType=LEFT (including count vs content agreement), for distinct=false, or for delete(Specification) with join/fetch POJOs.
  • Failure scenario: Any of the behaviors above regresses (or is 'fixed' differently in the two duplicated resolvers) and the suite stays green — e.g. a refactor of Join.join()'s dotted-path branch could change the silent-tail-drop behavior into a crash or vice versa without a failing test.
  • Suggested fix: Add tests pinning: 3-segment path behavior, the missing-alias IllegalArgumentException message, alias-conflict handling, LEFT joinType count/content parity, distinct=false with multiple joins, and delete(spec) expectations.
  • Verification: [CONFIRMED] Traced domain/Join.java and JoinSpecificationResolver.java plus the full test package. Join.join() only uses byDot[0]/byDot[1] (silent tail-drop for 3+ segments) and throws IllegalArgumentException for an unregistered first-segment alias. grep across mapper/src/test/.../spec found zero references to joinType, distinct, assertThatThrownBy/IllegalArgumentException/"not found", delete(, or any 3-segment dotted path — every join test asserts default INNER/distinct=true happy paths. The lone alias test (duplicateAliasJoinOnField) uses two identical definitions, covering only the dedup skip-path, not a conflicting-definition or LEFT/distinct=false case. All enumerated gaps are real; te…

Full report: see the codebase-review-2026-07-16 branch (linked on the parent tracking issue).

Activity

  1. added theissue type on Jul 16, 2026
  2. shihyuho commented on Jul 16, 2026

    @shihyuho
    MemberAuthor

    This was generated by AI during triage.

    Triage Notes - needs human

    Category: bug (correctness-dominant work package; also bundles one performance and one test-coverage finding)

    Summary: The @Join / @JoinFetch machinery has several silent-wrong-result edge cases (dotted-path tail-drop, query-global distinct last-writer-wins, alias-collision drop, count-vs-content join-semantics divergence, unregistered-alias fallback), an unbounded per-Root accumulation in the join context, and no edge/error test coverage — but every proposed fix turns on a public-contract or lifecycle decision that a human must make first.

    Current behavior (all confirmed in current code)

    • COR-07 — dotted join path drops segments after the second. Join.join(Root) splits pathToJoinOn on . and reads only byDot[0] (parent alias) and byDot[1] (join attribute); there is no byDot.length check. The default alias is derived from the full path (pathToJoinOn.replace(".", "_") in the constructor), so a 3-segment path registers a 2-level join under a 3-level-looking alias. JoinFetch.fetch(Root) has the identical shape. JoinSpecificationResolver forwards def.path() into the constructor unvalidated.
    • COR-05 — distinct is query-global, last-writer-wins. Join.toPredicate and JoinFetch.toPredicate both call query.distinct(distinct) unconditionally. When a POJO carries multiple joins with differing distinct values, the join executed last (field declaration order) wins; an explicit distinct=false on a later join silently disables deduplication for the whole query.
    • COR-06 — alias collision silently drops the second definition. Join.join returns early when getJoin(root, alias) != null, and JoinFetch.fetch when getFetch(root, alias) != null, with no comparison of the existing entry's path or joinType. Two fields explicitly sharing an alias but declaring different paths/joinTypes resolve to whichever composed first.
    • COR-08 — delete(Specification) is not covered by the number-result heuristic. JoinFetch.shouldDelegateToJoin delegates only when query.getResultType() is a Number. Spring Data's SimpleJpaRepository.delete(Specification) passes a throwaway CriteriaQuery whose result type is the entity class, so the heuristic returns false and a fetch/join is attached to the mutation root. The line-89 comment ("e.g. count or delete") is inaccurate.
    • COR-11 — count and content queries resolve a fetch alias with different join semantics. For content queries, SimpleSpecification.getFetchPath rebuilds the path with root.get(...) chains (implicit INNER join, ignoring the declared joinType). For count/exists queries, JoinFetch.toPredicate delegates to Join, which does root.join(path, joinType) honoring the declared joinType. The same alias is therefore LEFT in count queries and INNER in content queries — page totals can disagree with page content.
    • COR-10 — unregistered alias silently falls back to root.get(alias). SimpleSpecification.getExpr falls back to root.get(field) when the first path segment is in neither the joined nor fetched map. Aliases register only after the owning Join/JoinFetch spec's toPredicate runs, and execution order equals field declaration order. JoinSpecificationResolver.joinsDefOnField also skips the join entirely when the annotated field value is empty. So a @Spec whose alias is declared on a later field (or on a null-valued field) resolves against the root instead of the intended association.
    • PERF-02 — join context accumulates per-Root entries forever. SpecJoinContext.joined and .fetched are HashMaps keyed by JoinKey(Root, alias) / FetchKey(Root, alias). Each toPredicate execution uses a fresh Root, so putIfAbsent adds new entries on every execution of a reused Specification (two per findBySpec(pojo, pageable) — count root + content root), and the entries strongly retain the provider's criteria Join/Fetch nodes. Nothing removes them.
    • TEST-04 — no edge/error coverage. JoinSpecificationResolverTest, JoinFetchSpecificationResolverTest, and JoinFetchElementCollectionTest exercise only happy paths (default INNER, distinct=true). No test covers a 3+-segment path, the unregistered-alias IllegalArgumentException, a conflicting alias, joinType=LEFT count/content parity, distinct=false, or delete(Specification) with join/fetch POJOs. The lone duplicateAlias* tests use two identical definitions.

    Desired behavior

    Each edge case should stop producing silently-wrong results — but the correct target behavior for each depends on a decision listed below. Once decided, the corresponding acceptance criteria can be pinned by tests.

    Key interfaces

    • tw.com.softleader.data.jpa.spec.domain.Join — toPredicate(Root, CriteriaQuery, CriteriaBuilder), join(Root), constructor (Context, String pathToJoinOn, String alias, JoinType, boolean distinct)
    • tw.com.softleader.data.jpa.spec.domain.JoinFetch — toPredicate(...), fetch(Root), shouldDelegateToJoin(CriteriaQuery), delegatePredicateToJoin(...)
    • tw.com.softleader.data.jpa.spec.domain.SimpleSpecification — getPath(Root), getExpr(Root, String), getJoin/getFetch, getFetchPath(Root, FetchRef)
    • tw.com.softleader.data.jpa.spec.SpecJoinContext (implements JoinContext) — joined, fetched maps; JoinKey, FetchKey, HandleKey
    • tw.com.softleader.data.jpa.spec.JoinSpecificationResolver / JoinFetchSpecificationResolver — joinsDefOnField, newJoin/forwarding of def.path()
    • Annotations @Join, @JoinFetch (declared defaults joinType=INNER, distinct=true)

    Acceptance criteria (to be finalized after the decisions below)

    • A 3+-segment dotted path has a defined, tested outcome (either an explicit exception naming the unsupported path, or genuine N-level resolution) — no silent tail-drop.
    • distinct behavior across multiple joins on one POJO is defined and tested — deterministic regardless of field order.
    • An alias collision between incompatible definitions has a defined, tested outcome (explicit failure vs documented first-wins).
    • delete(Specification) with a @Join/@JoinFetch POJO has a defined, tested outcome.
    • For a non-INNER fetch, count/exists totals and content query use the same join semantics (tested: totalElements agrees with content size).
    • A @Spec referencing an alias declared later, or on a null-valued field, has a defined, tested outcome (explicit error vs order-independent registration).
    • Repeated execution of a reused mapped Specification does not grow SpecJoinContext without bound (tested).
    • Edge/error tests exist for each of the above in both the Join and JoinFetch resolvers.

    Out of scope

    • The @NestedSpec / @Or / @And resolvers and other SimpleSpecification subtypes not on the join path.

    Needs human because:

    Each sub-finding's fix turns on a public-contract or lifecycle decision that an autonomous agent must not make unilaterally:

    • COR-05 (distinct semantics): "make distinct accumulate (only ever set true, never reset to false)" changes the meaning of the public distinct=false annotation value — an API-behavior decision, and arguably a breaking change.
    • COR-07 (dotted path): throw-on-unsupported vs implement N-level resolution is a feature-scope decision; throwing is also a behavior change for paths that currently (mis)behave silently.
    • COR-06 (alias conflict): introducing a thrown exception where the code is currently silently first-wins is a behavior/compat decision.
    • COR-08 (delete): requires deciding the supported contract (document that join/fetch POJOs are unsupported with delete(Specification) vs detect-and-no-op) and depends on Spring Data internal behavior; medium confidence.
    • COR-11 (count/content parity): the suggested fix relies on Hibernate-specific behavior (casting Fetch to Join) or restructuring fetch-alias resolution — a design decision with provider-portability implications; medium confidence.
    • COR-10 (unregistered alias): fail-fast vs order-independent field-level join registration is a design decision that also interacts with the empty-field-value skip.
    • PERF-02 (context lifecycle): rekeying per-execution state off the query lifecycle (weak/identity keys or per-toPredicate scoping) is an architectural change to JoinContext with thread-safety and correctness implications.
    • TEST-04: cannot be pinned independently — the "desired" assertions for every case above depend on the decisions; tests written now would either lock in the current bugs or pre-empt the design.

    Because the sub-findings are interdependent (they all shape one public join contract) and several require a policy/API/architecture call, the work package as a whole needs a human design pass before it is agent-ready. Recommend splitting into per-decision issues afterward; the test additions can then follow each decision.

  3. shihyuho commented on Jul 20, 2026

    @shihyuho
    MemberAuthor

    Superseded by #198 in the execution map #204, which carries the same join-machinery findings plus the surviving finding from the old WP5. This 2026-07-16 static plan is being retired in favour of that map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions