Skip to content

fix(mapper): honor first-position @Or/@And in compound composition - #210

Open
shihyuho wants to merge 1 commit into
jakartafrom
fix/wp13-compound-composition
Open

fix(mapper): honor first-position @Or/@And in compound composition#210
shihyuho wants to merge 1 commit into
jakartafrom
fix/wp13-compound-composition

Conversation

@shihyuho

Copy link
Copy Markdown
Member

Closes #193

Problem

CompoundSpecification.toPredicate folded its specs with a seedless specs.stream().reduce(this::combine). combine(result, element) only ever inspects the element's wrapper, so the first-declared spec — which becomes the accumulator — never had its @Or/@And wrapper examined.

The same annotation therefore produced opposite SQL depending purely on field declaration position:

criteria fields before after
[@Or nickname, name] nickname AND name (the @Or silently dropped) name OR nickname
[name, @Or nickname] name OR nickname name OR nickname

Mirrored for a field-level @And under a class-level @Or (Disjunction).

Changes

  • CompoundSpecification (COR-15/COR-16) — before reducing, the fold now stably sorts elements that override the compound's default operator to the end. The accumulator is then always a default-operator element, so no wrapper is ever ignored. Composition becomes permutation invariant: any declaration order of the same annotated fields yields the same predicate.
  • CompoundSpecification (MAINT-08) — specs field and the Conjunction/Disjunction constructors narrowed from Collection to List, making the ordered fold an explicit contract rather than a dependency on an unspecified iteration order. All existing call sites (SpecMapper, JoinSpecificationResolver, JoinFetchSpecificationResolver) already pass a List, so no caller changed.
  • Conjunction / Disjunction — new overridesOperator(element) hook expressing which wrapper flips the default operator; combine is now defined in terms of it, so the two stay in sync.
  • SimpleSpecificationResolver (MAINT-09) — a field carrying both @And and @Or now fails fast with a descriptive IllegalArgumentException naming the offending class and field, instead of silently resolving to And.

Verification

  • make test green — 119 mapper + 15 starter tests, 0 failures (JDK 17, the project's declared target).
  • New tests (TEST-09), all of which fail without the fold fix — verified by temporarily removing the sort, which produced 9 failures:
    • ConjunctionTest / DisjunctionTest (8 tests each) — direct unit tests for combine and overridesOperator; first-position wrapper honored; two-element and exhaustive three-element permutation-invariance assertions over real query result sets; all-wrapped and empty-spec edge cases.
    • SimpleSpecificationResolverTestforceOr3 / forceAnd2 pin a first-position @Or under class-level AND and a first-position @And under class-level OR; forceOrIsPermutationInvariant / forceAndIsPermutationInvariant assert every declaration order of the same criteria returns the same rows; andOrAreMutuallyExclusive pins the new fail-fast.

⚠️ Behavior change / release note

Previously-ignored first-position @Or/@And wrappers now take effect, so affected consumers' result sets change. This is a bug fix restoring the annotations' documented semantics — a first-position @Or that used to be silently dropped now actually ORs.

The existing forceOr2 test is the concrete illustration: [name, @Or birthday, gender] previously folded to (name OR birthday) AND gender and returned 2 rows; it now folds to (name AND gender) OR birthday and returns 3 rows — the same result as its permutation forceOr ([name, gender, @Or birthday]), which was already returning 3. Its assertion was updated accordingly, and that update is the regression proof for COR-15.

Two notes for reviewers:

  1. The CollectionList narrowing of the public Conjunction/Disjunction constructors is source-incompatible for anyone constructing them directly with a non-List Collection. Per the plan this is framed as a fix rather than a breaking change, so no BREAKING CHANGE: footer was added — flagging it in case you'd rather it drive a major bump.
  2. NestedSpecificationResolver has the same silent @And-wins behavior as MAINT-09, but it sits outside this work package's scope and was deliberately left untouched.

🤖 Generated with Claude Code

CompoundSpecification folded its specs with a seedless reduce, so the
first-declared spec became the accumulator and its @Or/@and wrapper was
never inspected. The same annotated criteria class therefore produced
opposite SQL depending on field declaration order.

The fold now stably sorts elements that override the compound's default
operator to the end before reducing, so composition is permutation
invariant — any declaration order of the same annotated fields yields
the same predicate. Previously-ignored first-position wrappers now take
effect, so affected consumers' result sets change.

Also narrows the specs field/constructor from Collection to List so the
ordered fold is an explicit contract, and rejects a field carrying both
@and and @or with a descriptive IllegalArgumentException instead of
silently resolving to And.

Co-authored-by: Claude Opus 4.8 (1M context) <[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.

[WP13] Compound composition: first-position @Or/@And silently ignored (order-dependent boolean semantics)

1 participant