Repository navigation
[WP13] Compound composition: first-position @Or/@And silently ignored (order-dependent boolean semantics) #175
Copy link
Copy link
Closed as not planned
Closed as not planned
Copy link
Labels
status: pending-design-work動工前需要更多的設計或研究動工前需要更多的設計或研究type: bugSomething isn't workingSomething isn't working
Description
此內容由 AI 產生(specification-mapper 全庫審查 2026-07-16)。
Work package WP13 · findings: COR-15, COR-16, TEST-09, MAINT-08, MAINT-09 · 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-15 — Field-level @or on the first-declared field is silently ignored because combine() inspects only the reduce element, never the seed
- Category / severity: correctness / medium · effort M · confidence high
- Location:
mapper/src/main/java/tw/com/softleader/data/jpa/spec/domain/Conjunction.java:38 - Evidence: Conjunction.combine checks
if (element instanceof Or) return result.or(element)— only the second argument is inspected. CompoundSpecification.toPredicate (CompoundSpecification.java:45) folds via seedlessspecs.stream().reduce(this::combine), so the first spec becomes the accumulator and its Or wrapper (applied by SimpleSpecificationResolver.java:91 / NestedSpecificationResolver for field-level @or) is never examined. - Failure scenario: Criteria class
class C { @Or @Spec String nickname; @Spec String name; }with both fields set produces specs [Or(nickname), name]; combine(Or(nickname), name) takes the AND branch, yieldingnickname AND name. Declaring the fields in the opposite order yieldsname OR nickname. The same annotation produces opposite SQL depending on declaration position, and in first position it is a silent no-op — the query returns fewer rows than the user's declared OR semantics with no warning. The site docs (mapper/combine) document order-dependence for later positions but never that a first-position @or has no effect. - Suggested fix: Make the fold seed-aware: e.g. reduce over an explicit accumulator that checks the wrapper of the FIRST element too (a lone leading Or can be defined as identity — document that), or fail fast / log a warning when the first resolved spec carries an Or wrapper in a Conjunction. At minimum, document the first-position no-op in the combine docs.
- Verification: [CONFIRMED] Traced all four cited locations and confirmed the mechanism exactly. Conjunction.combine (Conjunction.java:38) checks only
element instanceof Or, never theresultaccumulator. CompoundSpecification.toPredicate (line 45) uses seedlessreduce(this::combine), making the first spec the initial accumulator whose wrapper type is never inspected. SimpleSpecificationResolver.buildSpecification (line 91-92) wraps a field-level @or in domain.Or, and SpecMapper.toSpec (line 107-118) collects specs in declaration order into a Conjunction. Therefore criteria{@Or @Spec String nickname; @Spec String name;}with both set yields specs [Or(nickname), name] and combine(Or(nickname), name) …
COR-16 — Mirror defect: field-level @and on the first-declared field of an @or class is silently ignored
- Category / severity: correctness / medium · effort M · confidence high
- Location:
mapper/src/main/java/tw/com/softleader/data/jpa/spec/domain/Disjunction.java:38 - Evidence: Disjunction.combine checks
if (element instanceof And) return result.and(element)— only the element, never the seed of the seedless reduce in CompoundSpecification.toPredicate. An And wrapper (SimpleSpecificationResolver.java:88) on the first resolved field is never inspected. - Failure scenario:
@Or class C { @And @Spec LocalDate birthday; @Spec String name; @Spec Gender gender; }produces [And(birthday), name, gender] → combine(And(birthday), name) takes the OR branch →birthday OR name OR gender; the user's @and is dropped, returning MORE rows than intended (the tested ForceAnd fixture only pins @and in third position, where it works). Moving the @and field later flips the semantics. - Suggested fix: Fix together with COMP-01 in the shared fold (seed-aware combine in CompoundSpecification), since both subclasses share the identical root cause.
- Verification: [CONFIRMED] Traced through the actual code. CompoundSpecification.toPredicate (lines 44-45) does specs.stream().reduce(this::combine) — a seedless reduce whose initial accumulator is the first stream element. That first element is only ever the
resultargument to combine, never theelementargument. Disjunction.combine (line 38) branches solely onelement instanceof And, so an And wrapper occupying the first position is never inspected; it falls through to result.or(element), and because And @Delegates Specification, its .or() delegation drops the AND intent. SimpleSpecificationResolver.java:88-90 confirms the per-field And wrapper is produced whenever a field carries @and. For an @or …
TEST-09 — No test pins a first-position @Or/@and; Conjunction/Disjunction combine has no direct unit tests
- Category / severity: testing / low · effort S · confidence high
- Location:
mapper/src/test/java/tw/com/softleader/data/jpa/spec/SimpleSpecificationResolverTest.java:438 - Evidence: The earliest combinator position exercised anywhere is field 2: fixtures ForceOr (line 425, @or on 3rd field), ForceOr2 (line 438, @or on 2nd field), ForceAnd (line 452, @and on 3rd field), and NestedSpecificationResolverTest's ForceAnd/ForceOr fixtures also put the wrapped member last. mapper/src/test/java/tw/com/softleader/data/jpa/spec/domain/ contains only DomainRuntimeHintsTest — no unit test constructs a Conjunction/Disjunction with a wrapped FIRST element to assert the fold's behavior.
- Failure scenario: The seed-ignoring behavior of COMP-01/COMP-02 is unpinned: a refactor of CompoundSpecification.toPredicate (e.g. switching reduce to an identity-seeded fold or reversing iteration) would silently change query semantics for every existing user relying on current first-position behavior, and the whole green test suite would not notice.
- Suggested fix: Add fixtures with @or as the first-declared field (and @and first under a class-level @or) asserting the produced SQL/result set, plus direct unit tests for Conjunction/Disjunction combine covering [Or(x), y], [y, Or(x)], [Or(x), Or(y)], and single-element lists — whichever semantics is chosen for COMP-01, pin it.
- Verification: [CONFIRMED] Core claim verified against source: CompoundSpecification.toPredicate uses specs.stream().reduce(this::combine) (no identity), so the first element is the seed and a first-position @Or/@and never reaches combine (seed-ignoring). No fixture puts @Or/@and on the first-declared field — earliest is ForceOr2 at position 2 (line 438); ForceOr/ForceAnd wrap position 3; NestedSpecificationResolverTest fixtures also place the wrapped member last. There is no direct ConjunctionTest/DisjunctionTest. That coverage gap is real and as described.\n\nTwo weaknesses lower severity below the medium claim: (1) The evidence's statement that domain/ 'contains only DomainRuntimeHintsTest' is factually…
MAINT-08 — Order-dependent fold semantics over an unordered Collection type
- Category / severity: maintainability / low · effort S · confidence high
- Location:
mapper/src/main/java/tw/com/softleader/data/jpa/spec/domain/CompoundSpecification.java:39 - Evidence:
@NonNull protected final transient Collection<Specification<T>> specs— the public Conjunction/Disjunction constructors accept any Collection, yet combine() semantics (AND/OR selection and left-associative grouping) depend entirely on iteration order. SpecMapper happens to pass a List (toSpec collects with toList()), but the API contract does not require ordering. - Failure scenario: A library user composing specs manually calls
new Conjunction<>(Set.of(nameSpec, new Or<>(nickSpec))); whether the result isname OR nickornick AND namedepends on HashSet iteration order of the JVM run — nondeterministic query semantics that pass tests on one machine and differ on another. - Suggested fix: Narrow the constructor/field type to List<Specification> (or document the ordering requirement on the public constructors) to make the order dependency explicit in the API.
- Verification: [CONFIRMED] Traced and verified.
CompoundSpecification(line 39) storesCollection<Specification<T>>and folds it withspecs.stream().reduce(this::combine), which is inherently iteration-order dependent. BothConjunctionandDisjunctionare public classes whose public constructors accept anyCollection<Specification<T>>. Thecombineimplementations only inspect the second argument (element) forinstanceof Or/And; the first stream element is the untyped seed accumulator. Thus both operator selection and left-associative grouping depend on position. The reproduction is correct:new Conjunction<>(Set.of(nameSpec, new Or<>(nickSpec)))yieldsname OR nickfor one iteration…
MAINT-09 — A field annotated with both @and and @or silently resolves to And with no validation
- Category / severity: maintainability / low · effort S · confidence high
- Location:
mapper/src/main/java/tw/com/softleader/data/jpa/spec/SimpleSpecificationResolver.java:88 - Evidence: buildSpecification checks
isAnnotationPresent(And.class)first (line 88) and returns the And wrapper before ever checking @or (line 91); NestedSpecificationResolver.buildSpecification has the same first-wins ordering. Neither resolver rejects or logs the conflicting combination. - Failure scenario: During a refactor a developer leaves both
@Andand@Oron a criteria field; the @or is silently discarded, the field is AND-combined, and the query narrows results with no error, warning, or documentation explaining the precedence. - Suggested fix: Throw an IllegalStateException (or at least log a warning) when a field carries both @and and @or, in both SimpleSpecificationResolver and NestedSpecificationResolver.
- Verification: [CONFIRMED] Traced both resolvers. In SimpleSpecificationResolver.buildSpecification (lines 88-93), the code checks isAnnotationPresent(And.class) first and returns the And wrapper before ever reaching the Or check at line 91 — exactly as described. NestedSpecificationResolver has the identical first-wins ordering (And at line 63, Or at line 66). Neither resolver validates, rejects, or logs the conflicting @and + @or combination. The failure scenario is real: a field carrying both annotations silently AND-combines and discards @or with no error or warning. This is accurate maintainability debt. Severity low is correct and stays low: it is not reachable by an end user (attacker), only by the …
Full report: see the codebase-review-2026-07-16 branch (linked on the parent tracking issue).
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
status: pending-design-work動工前需要更多的設計或研究動工前需要更多的設計或研究type: bugSomething isn't workingSomething isn't working
Activity
shihyuho commentedon Jul 16, 2026
Triage Notes - needs human
Category: bug (worst severity: correctness / medium; the WP also carries maintainability and test-coverage sub-findings)
Summary: A field-level
@Or(or@And) on the first-declared criteria field is silently ignored, so the same annotation yields opposite boolean SQL depending on declaration position — but resolving this requires a maintainer to fix the intended semantics before an agent can implement and pin it.Current behavior
CompoundSpecification.toPredicatedoesspecs.stream().reduce(this::combine). The first spec becomes the initial accumulator (theresultargument) and is never passed as theelementargument.Conjunction.combine(result, element)selects OR only whenelement instanceof Or; otherwise AND.Disjunction.combine(result, element)selects AND only whenelement instanceof And; otherwise OR. Neither inspects the wrapper type ofresult(the seed).SimpleSpecificationResolver.buildSpecificationandNestedSpecificationResolver.buildSpecificationwrap a field carrying@Orindomain.Orand a field carrying@Andindomain.And;SpecMapper.toSpeccollects them in declaration order into aConjunction(orDisjunctionwhen the criteria class itself is@Or).@Or/@Andwrapper occupying the first position is never examined, so it is a silent no-op. Example — criteria{ @Or @Spec String nickname; @Spec String name; }with both set produces specs[Or(nickname), name], andcombine(Or(nickname), name)takes the AND branch →nickname AND name, whereas declaring the fields in the opposite order yieldsname OR nickname. Mirror case:@Or-class{ @And @Spec LocalDate birthday; @Spec String name; ... }drops the@Andin first position.Collection<Specification<T>>on the publicConjunction/Disjunctionconstructors, yet the fold's semantics depend entirely on iteration order — a caller passing aSetgets nondeterministic operator/grouping selection; (b) a field carrying both@Andand@Orsilently resolves toAnd(And is checked first in both resolvers) with no validation or warning; (c) no test pins a first-position@Or/@And, and there is no direct unit test forConjunction/Disjunctioncombine(the earliest wrapped position exercised by fixturesForceOr/ForceOr2/ForceAndand the nested equivalents is position 2).Desired behavior
To be fixed by a maintainer decision (see Needs human because). Once the target semantics is chosen, the fold and both
combineoverrides must apply the chosen rule to the first element as well, both resolvers must handle the conflicting-annotation case per the chosen policy, and the public constructor contract must make the ordering requirement explicit. Tests must then pin the chosen semantics.Key interfaces
CompoundSpecification<T>.toPredicate(Root, CriteriaQuery, CriteriaBuilder)and itscombine(Specification<T> result, Specification<T> element)contract; theCollection<Specification<T>> specsfield.Conjunction<T>.combine(...),Disjunction<T>.combine(...), and their publicCollection-taking constructors.domain.And<T>/domain.Or<T>marker wrappers (both@Delegatethe wrappedSpecification).SimpleSpecificationResolver.buildSpecification(...)andNestedSpecificationResolver.buildSpecification(...)(annotation-precedence and wrapping site).SpecMapper.toSpec(Context, Object)(collects child specs in declaration order into aConjunction/Disjunction).Acceptance criteria
(To be finalized once the semantics decision is made. Provisional shape:)
@Orin a (default)@Conjunctioncriteria class produces the decided, documented SQL — no longer a silent no-op — and the result is independent of whether the same annotated field is declared first or later.@Andunder a class-level@Or) behaves per the same decided rule.@And+@Oron one field behaves per the decided policy (reject, warn, or documented precedence) consistently in bothSimpleSpecificationResolverandNestedSpecificationResolver.Conjunction/Disjunctionconstructors is made explicit (narrowed type and/or documented contract) per the decision.@Or/@Andon the first-declared field, plus directConjunction/Disjunctioncombineunit tests covering[Or(x), y],[y, Or(x)],[Or(x), Or(y)], and single-element lists.mvn testpasses.Out of scope
@Or/@Andcurrently combine (that behavior is documented and relied upon).Needs human because
The correctness fix hinges on a maintainer policy/API decision that an autonomous agent must not make on its own:
@Or/@Andshould mean. OR/AND is a binary combiner with no left operand at position 0, so the intended semantics is genuinely undefined. The issue itself lists mutually exclusive options — make it effective (bind forward to the next element), treat a lone leading wrapper as identity and document the no-op, or fail-fast / log a warning. Each is a different observable behavior.@Or/@Andtake effect silently changes the SQL (and result sets) of every existing user who happens to declare such a field first — a behavior break that needs sign-off.IllegalStateExceptionon@And+@Oris a breaking change for anyone currently relying on the silent And-wins precedence; warn-vs-throw-vs-document is a policy call.Conjunction/Disjunctionconstructor parameter fromCollectiontoListis a source/binary-breaking public-API change.Every other sub-finding (the tests, the docs) is downstream of decision (1): the tests must pin "whichever semantics is chosen," so they cannot be written until the target behavior is fixed. Once a maintainer records the intended semantics and accepts the associated breaking-change scope, the implementation + tests become self-contained and agent-safe.
shihyuho commentedon Jul 20, 2026
Superseded by #193 in the execution map #204, which carries the same findings (COR-15, COR-16, TEST-09, MAINT-08, MAINT-09). This 2026-07-16 static plan is being retired in favour of that map.