feat: add OR operator to QuerySpec constraints#108
feat: add OR operator to QuerySpec constraints#108javier-godoy wants to merge 2 commits intomasterfrom
Conversation
WalkthroughAdds OR/disjunction support: new DisjunctionConstraint type and factory, Constraint.or(...) convenience, transformer hook and JPA implementation combining sub-predicates with OR using LEFT joins, tests for OR behavior, and Maven version bumps across modules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java`:
- Around line 173-175: The stream currently maps child constraints with
this::transform which bypasses the public unsupported-constraint guard and can
produce null entries; instead call the transformer’s public apply(...) for each
child so any ConstraintTransformerException is thrown immediately. Replace the
mapping in ConstraintTransformerJpaImpl where predicates are built (the
c.getConstraints() stream that produces predicates) to invoke apply(constraint)
for each child constraint (e.g., map(constraint -> apply(constraint))) so
unsupported constraints are surfaced rather than allowing nulls into the
Predicate[].
- Around line 171-176: transformDisjunctionConstraint currently maps child
constraints with this::transform (which returns null for unsupported
constraints) and uses path navigation that defaults to JoinType.INNER, causing
nullable associations to be filtered out before OR evaluation; change the
implementation to use apply() instead of transform() so unsupported child
constraints throw, and ensure path navigation invoked during disjunctions uses
left-join-safe handling (use LEFT joins for attribute path resolution when
building predicates for transformDisjunctionConstraint) so expressions like
"city.name = X OR id = Y" will include rows with null city; keep combining the
resulting Predicate[] with criteriaBuilder.or(...) as before.
In
`@backend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.java`:
- Around line 150-167: The tests testFilterWithOrConstraint and
testFilterWithOrConstraintPartialMatch rely on generated city names being
distinct; make the predicates deterministic by comparing on city.id (e.g.,
ConstraintBuilder.of("city","id").equal(...)) or by using fixed known names
instead of cities.get(...).getName() so the OR branches are unambiguous—update
the ConstraintBuilder calls in both tests (the ones using "city","name") to use
"city","id" (or fixed literals) and adjust expected counts if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65232f81-59a7-4593-92e7-142f997b555d
📒 Files selected for processing (5)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.javabackend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java
...ata-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java
Outdated
Show resolved
Hide resolved
...ata-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java
Outdated
Show resolved
Hide resolved
backend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java (1)
87-90:⚠️ Potential issue | 🟠 MajorLEFT-join disjunction can still reuse an INNER join and drop nullable rows.
At Line 89, existing joins are matched only by attribute name. If an INNER join was created earlier on the same path, the disjunction at Line 174 (which switches to LEFT) can still reuse that INNER join, so rows with null associations may still be filtered before the
ORis evaluated.Suggested fix
`@SuppressWarnings`("rawtypes") private From<?,?> join(From<?,?> source, String attributeName) { - Optional<Join> existingJoin = source.getJoins().stream().filter(join->join.getAttribute().getName().equals(attributeName)).map(join->(Join)join).findFirst(); + Optional<Join> existingJoin = source.getJoins().stream() + .map(join -> (Join) join) + .filter(join -> join.getAttribute().getName().equals(attributeName)) + .filter(join -> join.getJoinType() == currentJoinType) + .findFirst(); return existingJoin.orElseGet(()->source.join(attributeName, currentJoinType)); }Also applies to: 174-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java` around lines 87 - 90, The join reuse logic in join(From<?,?> source, String attributeName) matches only on attribute name and can return an INNER join when currentJoinType is LEFT; update the matching so you only reuse an existing Join if both the attribute name and the Join.getJoinType() equal the requested/currentJoinType, otherwise create a new join with source.join(attributeName, currentJoinType); apply the same change to the other join helper used around the disjunction (the overloaded join method at the 174-181 area) so LEFT joins are never replaced by previously-created INNER joins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java`:
- Around line 87-90: The join reuse logic in join(From<?,?> source, String
attributeName) matches only on attribute name and can return an INNER join when
currentJoinType is LEFT; update the matching so you only reuse an existing Join
if both the attribute name and the Join.getJoinType() equal the
requested/currentJoinType, otherwise create a new join with
source.join(attributeName, currentJoinType); apply the same change to the other
join helper used around the disjunction (the overloaded join method at the
174-181 area) so LEFT joins are never replaced by previously-created INNER
joins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 012bb055-a583-4c99-8b5a-7dee1619ed7a
📒 Files selected for processing (2)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.javabackend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.java
javier-godoy
force-pushed
the
querySpec-or
branch
from
16a2c21 to
7a5834f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java (1)
39-44: Consider null-safety for constraint elements.The factory accepts varargs that could contain null elements, which would propagate to the list and potentially cause NPEs during transformation. The
firstparameter also lacks explicit null validation.💡 Proposed defensive implementation
public static DisjunctionConstraint of(Constraint first, Constraint... rest) { + Objects.requireNonNull(first, "first constraint must not be null"); List<Constraint> list = new java.util.ArrayList<>(); list.add(first); - list.addAll(Arrays.asList(rest)); + for (Constraint c : rest) { + list.add(Objects.requireNonNull(c, "constraint must not be null")); + } return new DisjunctionConstraint(list); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java` around lines 39 - 44, The factory DisjunctionConstraint.of(Constraint first, Constraint... rest) should defensively validate inputs: throw a clear exception (e.g., NullPointerException or IllegalArgumentException) if first is null, and filter out any null entries from the varargs before constructing the list; update the method that builds the internal List<Constraint> so it only adds non-null elements from rest (or throws if an empty/invalid set is produced) and then passes that sanitized list into the DisjunctionConstraint constructor to avoid NPEs during later transformation/processing.backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java (1)
84-89: Mutable state for join type introduced.The
currentJoinTypefield makes this class not thread-safe. This is acceptable if instances are created per-query (which appears to be the case from constructor usage), but consider adding a class-level Javadoc note about thread-safety expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java` around lines 84 - 89, The class currently holds mutable state in the field currentJoinType (used by the join(From<?,?>, String) method), making instances not thread-safe; add a class-level Javadoc on ConstraintTransformerJpaImpl that clearly states instances are not thread-safe, that currentJoinType is mutable and that callers must create a new instance per query (referencing the constructor and the join(...) method), or alternatively remove the mutable field and pass JoinType as a parameter to join(...) to make instances thread-safe—include which approach you choose in the Javadoc so callers know usage expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java`:
- Around line 84-89: The class currently holds mutable state in the field
currentJoinType (used by the join(From<?,?>, String) method), making instances
not thread-safe; add a class-level Javadoc on ConstraintTransformerJpaImpl that
clearly states instances are not thread-safe, that currentJoinType is mutable
and that callers must create a new instance per query (referencing the
constructor and the join(...) method), or alternatively remove the mutable field
and pass JoinType as a parameter to join(...) to make instances
thread-safe—include which approach you choose in the Javadoc so callers know
usage expectations.
In
`@backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java`:
- Around line 39-44: The factory DisjunctionConstraint.of(Constraint first,
Constraint... rest) should defensively validate inputs: throw a clear exception
(e.g., NullPointerException or IllegalArgumentException) if first is null, and
filter out any null entries from the varargs before constructing the list;
update the method that builds the internal List<Constraint> so it only adds
non-null elements from rest (or throws if an empty/invalid set is produced) and
then passes that sanitized list into the DisjunctionConstraint constructor to
avoid NPEs during later transformation/processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37b60e1c-ddd8-4860-ac1c-ec7009d5140b
📒 Files selected for processing (12)
backend-core-business-impl/pom.xmlbackend-core-business-spring-impl/pom.xmlbackend-core-business/pom.xmlbackend-core-data-impl/pom.xmlbackend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.javabackend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.javabackend-core-data/pom.xmlbackend-core-model/pom.xmlbackend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.javapom.xml
🚧 Files skipped from review as they are similar to previous changes (8)
- pom.xml
- backend-core-data/pom.xml
- backend-core-business/pom.xml
- backend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java
- backend-core-model/pom.xml
- backend-core-business-spring-impl/pom.xml
- backend-core-data-impl/pom.xml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java (1)
33-46: Consider making the constraint list immutable.The
@Getteronconstraintsexposes the internal mutableArrayList, allowing callers to modify it after construction. For a value object representing a logical OR, immutability is preferred.♻️ Proposed fix to return an unmodifiable list
+import java.util.Collections; ... `@Getter` `@RequiredArgsConstructor` `@FieldDefaults`(level = AccessLevel.PRIVATE, makeFinal = true) public class DisjunctionConstraint implements Constraint { - `@NonNull` List<Constraint> constraints; + `@NonNull` List<Constraint> constraints; + + public List<Constraint> getConstraints() { + return Collections.unmodifiableList(constraints); + } public static DisjunctionConstraint of(Constraint first, Constraint... rest) { List<Constraint> list = new java.util.ArrayList<>(); list.add(Objects.requireNonNull(first, "constraint must not be null")); for (Constraint c : rest) { list.add(Objects.requireNonNull(c, "constraint must not be null")); } - return new DisjunctionConstraint(list); + return new DisjunctionConstraint(Collections.unmodifiableList(list)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java` around lines 33 - 46, The constraints List exposed by DisjunctionConstraint (field constraints and getter generated by `@Getter`) is currently mutable; make it immutable by wrapping the list used in the constructor/factory (e.g., in DisjunctionConstraint.of(Constraint first, Constraint... rest)) with an unmodifiable copy (for example use Collections.unmodifiableList(new ArrayList<>(list)) or List.copyOf(list)) before constructing the DisjunctionConstraint so callers cannot modify the internal list after creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java`:
- Around line 33-46: The constraints List exposed by DisjunctionConstraint
(field constraints and getter generated by `@Getter`) is currently mutable; make
it immutable by wrapping the list used in the constructor/factory (e.g., in
DisjunctionConstraint.of(Constraint first, Constraint... rest)) with an
unmodifiable copy (for example use Collections.unmodifiableList(new
ArrayList<>(list)) or List.copyOf(list)) before constructing the
DisjunctionConstraint so callers cannot modify the internal list after creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6fee4fd-4be0-4f36-aef8-eeeb1e9ed6f2
📒 Files selected for processing (2)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java
javier-godoy
force-pushed
the
querySpec-or
branch
from
fe7413f to
b75b25d
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java (1)
32-37: Consider restricting constructor visibility (optional).The Lombok-generated public constructor allows bypassing the factory method's per-element null validation. If you want to enforce the factory pattern:
♻️ Optional: Make constructor private
`@Getter` -@RequiredArgsConstructor +@RequiredArgsConstructor(access = AccessLevel.PRIVATE) `@FieldDefaults`(level = AccessLevel.PRIVATE, makeFinal = true) public class DisjunctionConstraint implements Constraint {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java` around lines 32 - 37, The public constructor generated by Lombok (`@RequiredArgsConstructor`) on DisjunctionConstraint lets callers bypass the factory null-checks for the constraints field; change the constructor visibility to private by updating the Lombok annotation to generate a private constructor (e.g., `@RequiredArgsConstructor`(access = AccessLevel.PRIVATE)) so callers must use the factory method that enforces per-element null validation for the List<Constraint> constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java`:
- Around line 39-47: The or(Constraint first, Constraint... rest) path assumes
rest is non-null and will NPE in prepend(...) for calls like or(x,
(Constraint[]) null); modify prepend(Constraint first, Constraint[] rest) (or
add a null-guard in or()) to treat a null rest as an empty array before creating
the result array and calling System.arraycopy — e.g., normalize rest to an empty
Constraint[] when null so the array creation and copy logic work safely and
subsequent constraint-level null validations can run.
---
Nitpick comments:
In
`@backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java`:
- Around line 32-37: The public constructor generated by Lombok
(`@RequiredArgsConstructor`) on DisjunctionConstraint lets callers bypass the
factory null-checks for the constraints field; change the constructor visibility
to private by updating the Lombok annotation to generate a private constructor
(e.g., `@RequiredArgsConstructor`(access = AccessLevel.PRIVATE)) so callers must
use the factory method that enforces per-element null validation for the
List<Constraint> constraints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60535b5a-a58f-4190-8082-a5ec1579447d
📒 Files selected for processing (12)
backend-core-business-impl/pom.xmlbackend-core-business-spring-impl/pom.xmlbackend-core-business/pom.xmlbackend-core-data-impl/pom.xmlbackend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.javabackend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.javabackend-core-data/pom.xmlbackend-core-model/pom.xmlbackend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.javapom.xml
| default Constraint or(Constraint first, Constraint... rest) { | ||
| return DisjunctionConstraint.of(this, prepend(first, rest)); | ||
| } | ||
|
|
||
| private static Constraint[] prepend(Constraint first, Constraint[] rest) { | ||
| Constraint[] result = new Constraint[1 + rest.length]; | ||
| result[0] = first; | ||
| System.arraycopy(rest, 0, result, 1, rest.length); | ||
| return result; |
There was a problem hiding this comment.
Handle explicit null varargs defensively in or(...).
Line 43 assumes rest is non-null. A call like or(x, (Constraint[]) null) throws an NPE in prepend(...) before constraint-level null validation runs.
Suggested fix
import com.flowingcode.backendcore.model.constraints.DisjunctionConstraint;
import com.flowingcode.backendcore.model.constraints.NegatedConstraint;
+import java.util.Objects;
@@
default Constraint or(Constraint first, Constraint... rest) {
- return DisjunctionConstraint.of(this, prepend(first, rest));
+ return DisjunctionConstraint.of(this, prepend(first, Objects.requireNonNull(rest, "rest must not be null")));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| default Constraint or(Constraint first, Constraint... rest) { | |
| return DisjunctionConstraint.of(this, prepend(first, rest)); | |
| } | |
| private static Constraint[] prepend(Constraint first, Constraint[] rest) { | |
| Constraint[] result = new Constraint[1 + rest.length]; | |
| result[0] = first; | |
| System.arraycopy(rest, 0, result, 1, rest.length); | |
| return result; | |
| default Constraint or(Constraint first, Constraint... rest) { | |
| return DisjunctionConstraint.of(this, prepend(first, Objects.requireNonNull(rest, "rest must not be null"))); | |
| } | |
| private static Constraint[] prepend(Constraint first, Constraint[] rest) { | |
| Constraint[] result = new Constraint[1 + rest.length]; | |
| result[0] = first; | |
| System.arraycopy(rest, 0, result, 1, rest.length); | |
| return result; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java`
around lines 39 - 47, The or(Constraint first, Constraint... rest) path assumes
rest is non-null and will NPE in prepend(...) for calls like or(x,
(Constraint[]) null); modify prepend(Constraint first, Constraint[] rest) (or
add a null-guard in or()) to treat a null rest as an empty array before creating
the result array and calling System.arraycopy — e.g., normalize rest to an empty
Constraint[] when null so the array creation and copy logic work safely and
subsequent constraint-level null validations can run.
There was a problem hiding this comment.
@coderabbitai please check if the comment has been addressed.
There was a problem hiding this comment.
@javier-godoy, based on the current file content, the comment has not been addressed yet. The or(Constraint first, Constraint... rest) method still passes rest directly to prepend(...) without any null-guard:
default Constraint or(Constraint first, Constraint... rest) {
return DisjunctionConstraint.of(this, prepend(first, rest));
}A call like or(x, (Constraint[]) null) will still throw an NPE inside prepend(...) at rest.length before any constraint-level validation runs. The suggested fix (Objects.requireNonNull(rest, "rest must not be null")) has not been applied.
There was a problem hiding this comment.
@coderabbitai Note that DisjunctionConstraint.of already checks whether the varargs is null
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd "DisjunctionConstraint.java" --exec cat {}
Length of output: 1759
@javier-godoy, you're right that DisjunctionConstraint.of does include a null check (Objects.requireNonNull(rest, "constraints must not be null")). However, that check is never reached in this code path when rest is null. The call chain is:
or(first, null)→ callsprepend(first, null)- In
prepend:new Constraint[1 + rest.length]→ NPE here (no message), beforeDisjunctionConstraint.ofis ever invoked.
So the null guard in DisjunctionConstraint.of doesn't cover this case — prepend fails first. The original concern remains: a caller passing or(x, (Constraint[]) null) gets an NPE with no helpful message, whereas the guard in DisjunctionConstraint.of would give "constraints must not be null".
That said, explicitly casting null to Constraint[] is a very unusual call pattern, and both paths still fail fast with an NPE. If you consider this acceptable behavior, I can close the comment.
There was a problem hiding this comment.
@coderabbitai I consider this acceptable behavior.
There was a problem hiding this comment.
@javier-godoy, understood — acknowledged and noted. The existing fast-fail NPE behavior for or(x, (Constraint[]) null) is acceptable as-is.
javier-godoy
force-pushed
the
querySpec-or
branch
from
b75b25d to
90692ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java`:
- Around line 39-44: DisjunctionConstraint.of currently assumes the varargs
array 'rest' is non-null and will throw an implicit NPE in the for-loop; add an
explicit null check for the varargs container before iterating (e.g.,
Objects.requireNonNull(rest, "constraints must not be null")) so callers calling
of(first, (Constraint[]) null) get a predictable error, then continue to
validate each element in 'rest' with Objects.requireNonNull as already done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e688aad-15c0-42c2-b24c-b0b3f2181db7
📒 Files selected for processing (5)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.javabackend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java
🚧 Files skipped from review as they are similar to previous changes (3)
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.java
- backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java
...model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java
Show resolved
Hide resolved
javier-godoy
force-pushed
the
querySpec-or
branch
from
90692ec to
4c30971
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java (1)
39-47:⚠️ Potential issue | 🟡 MinorGuard the varargs container before
prepend(...).
or(x, (Constraint[]) null)still blows up onrest.lengthinsideprepend()beforeDisjunctionConstraint.of(...)can validate anything. Treat a null varargs container as an empty array here so this path behaves predictably.Suggested fix
private static Constraint[] prepend(Constraint first, Constraint[] rest) { + rest = rest == null ? new Constraint[0] : rest; Constraint[] result = new Constraint[1 + rest.length]; result[0] = first; System.arraycopy(rest, 0, result, 1, rest.length); return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java` around lines 39 - 47, The prepend(...) helper doesn't guard against a null varargs array, so calling or(x, (Constraint[]) null) throws on rest.length before DisjunctionConstraint.of(...) can validate; update the code to treat a null rest as an empty array (e.g., if rest == null then set rest = new Constraint[0]) either inside prepend(Constraint first, Constraint[] rest) or by handling null before calling prepend in or(...), ensuring DisjunctionConstraint.of(this, prepend(...)) always receives a non-null array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java`:
- Around line 39-47: The prepend(...) helper doesn't guard against a null
varargs array, so calling or(x, (Constraint[]) null) throws on rest.length
before DisjunctionConstraint.of(...) can validate; update the code to treat a
null rest as an empty array (e.g., if rest == null then set rest = new
Constraint[0]) either inside prepend(Constraint first, Constraint[] rest) or by
handling null before calling prepend in or(...), ensuring
DisjunctionConstraint.of(this, prepend(...)) always receives a non-null array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cff07aaf-5473-4d1c-9fe5-2e15f414dba8
📒 Files selected for processing (5)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.javabackend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java
🚧 Files skipped from review as they are similar to previous changes (1)
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.java
javier-godoy
force-pushed
the
querySpec-or
branch
from
4c30971 to
2060334
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java`:
- Around line 92-95: The join() method currently reuses existing joins based
only on attribute name, which can reuse an INNER join when currentJoinType is
LEFT and break OR semantics in transformDisjunctionConstraint; update
join(From<?,?> source, String attributeName) to check both
join.getAttribute().getName().equals(attributeName) and that the existing Join's
join type equals currentJoinType (compare via join.getJoinType() or equivalent),
and only reuse when both match—otherwise call source.join(attributeName,
currentJoinType) to create a join with the correct type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80d29861-6306-4ddc-b1a4-d993da05803d
📒 Files selected for processing (6)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.javabackend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.javabackend-core-model/src/test/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraintTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java
| private From<?,?> join(From<?,?> source, String attributeName) { | ||
| Optional<Join> existingJoin = source.getJoins().stream().filter(join->join.getAttribute().getName().equals(attributeName)).map(join->(Join)join).findFirst(); | ||
| return existingJoin.orElseGet(()->source.join(attributeName, JoinType.INNER)); | ||
| return existingJoin.orElseGet(()->source.join(attributeName, currentJoinType)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
impl_file="$(fd ConstraintTransformerJpaImpl.java | head -n1)"
test_file="$(fd JpaDaoSupportTest.java | head -n1)"
echo "== join(...) implementation around Line 92 =="
sed -n '88,102p' "$impl_file"
echo
echo "== occurrences of getJoinType() in transformer (expecting join-type-aware reuse) =="
rg -n 'getJoinType\(\)' "$impl_file" || true
echo
echo "== OR-related tests currently present =="
rg -n 'testFilterWithOrConstraint|testFilterWithOrConstraintPartialMatch|\\.or\\(' "$test_file" -A3 -B2Repository: FlowingCode/backend-core
Length of output: 1805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the implementation file
impl_file="backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java"
if [ -f "$impl_file" ]; then
echo "== join(...) implementation around Line 92 =="
sed -n '88,102p' "$impl_file"
echo
echo "== All getJoinType() calls in transformer =="
rg -n 'getJoinType\(\)' "$impl_file" -B2 -A2 || true
echo
echo "== transformDisjunctionConstraint method context (lines 179-186) =="
sed -n '175,190p' "$impl_file"
else
echo "File not found at: $impl_file"
# Try to locate it
find . -name "ConstraintTransformerJpaImpl.java" -type f 2>/dev/null || echo "No ConstraintTransformerJpaImpl.java found"
fiRepository: FlowingCode/backend-core
Length of output: 1388
Make join reuse join-type-aware to preserve OR semantics.
The join() method (lines 92-95) reuses any existing join by attribute name only. When transformDisjunctionConstraint() switches currentJoinType to LEFT (line 183), a previous INNER join with the same attribute name will be incorrectly reused, causing the OR evaluation to apply INNER join semantics (dropping nullable-association rows) before OR logic.
The suggested patch correctly adds a join-type check to ensure reused joins match both the attribute name and the current join type.
Suggested patch
`@SuppressWarnings`("rawtypes")
private From<?,?> join(From<?,?> source, String attributeName) {
- Optional<Join> existingJoin = source.getJoins().stream().filter(join->join.getAttribute().getName().equals(attributeName)).map(join->(Join)join).findFirst();
- return existingJoin.orElseGet(()->source.join(attributeName, currentJoinType));
+ Optional<Join> existingJoin = source.getJoins().stream()
+ .map(join -> (Join) join)
+ .filter(join -> join.getAttribute().getName().equals(attributeName))
+ .filter(join -> join.getJoinType() == currentJoinType)
+ .findFirst();
+ return existingJoin.orElseGet(() -> source.join(attributeName, currentJoinType));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java`
around lines 92 - 95, The join() method currently reuses existing joins based
only on attribute name, which can reuse an INNER join when currentJoinType is
LEFT and break OR semantics in transformDisjunctionConstraint; update
join(From<?,?> source, String attributeName) to check both
join.getAttribute().getName().equals(attributeName) and that the existing Join's
join type equals currentJoinType (compare via join.getJoinType() or equivalent),
and only reuse when both match—otherwise call source.join(attributeName,
currentJoinType) to create a join with the correct type.
There was a problem hiding this comment.
@coderabbitai The finding is technically correct — the filter on line 93 does not check join type — but it has no observable effect on query results. When a prior non-disjunction constraint has already created an INNER JOIN on an association, rows with a null foreign key are eliminated by that join before any WHERE predicate is evaluated, including the OR predicate from the disjunction. Conversely, when the disjunction runs first and creates a LEFT JOIN, any subsequent non-disjunction predicate on that association (e.g. city.population > 0) evaluates to NULL for no-city rows, which SQL treats as UNKNOWN and filters out — the same rows excluded by an INNER JOIN. In both orderings the final result set is identical. The fix would not change which rows are returned.
There was a problem hiding this comment.
@javier-godoy, that's a well-reasoned explanation — thank you for the detailed walkthrough. You're right on both orderings:
- INNER JOIN pre-exists: null-FK rows are already absent from the result set before the OR predicate is even applied, so reusing the INNER join inside
transformDisjunctionConstraintproduces no different outcome. - LEFT JOIN pre-exists (disjunction ran first): any subsequent equality/comparison predicate on the joined table's columns will evaluate to
UNKNOWNfor the null-padded rows (SQL three-valued logic), causing them to be silently excluded — effectively the same result as an INNER join would produce.
I'll retract the suggestion. The code is functionally correct as written; the join-type mismatch has no observable effect on query results.
ᓚᘏᗢ
|
#108 (comment): the finding is technically correct — the filter on line 93 does not check join type — but it has no observable effect on query results. Setup (from the test fixture):
Case 1 — INNER JOIN pre-exists, disjunction runs second// Constraint 1: non-disjunction → creates INNER JOIN on city
pf.addConstraint(ConstraintBuilder.of("city", "name").equal(cities.get(0).getName()));
// Constraint 2: disjunction → join() finds the existing INNER JOIN by attribute name
// and reuses it instead of creating a LEFT JOIN (the reported issue)
pf.addConstraint(
ConstraintBuilder.of("city", "id").equal(cities.get(0).getId())
.or(ConstraintBuilder.of("id").equal(persistedPerson.getId())));Current code (INNER JOIN reused inside the OR): FROM person
INNER JOIN city ON person.city_id = city.id -- eliminates persistedPerson here
WHERE city.name = 'Springfield'
AND (city.id = ? OR person.id = ?) -- persistedPerson never reaches this→ 5 rows "Fixed" code (separate LEFT JOIN created for the OR branch): FROM person
INNER JOIN city c1 ON person.city_id = c1.id -- still eliminates persistedPerson
LEFT JOIN city c2 ON person.city_id = c2.id
WHERE c1.name = 'Springfield'
AND (c2.id = ? OR person.id = ?)→ 5 rows — the INNER JOIN on Case 2 — LEFT JOIN pre-exists (disjunction ran first), non-disjunction runs second// Constraint 1: disjunction → creates LEFT JOIN on city
pf.addConstraint(
ConstraintBuilder.of("city", "id").equal(cities.get(0).getId())
.or(ConstraintBuilder.of("id").equal(persistedPerson.getId())));
// Constraint 2: non-disjunction → join() finds the existing LEFT JOIN by attribute name
// and reuses it instead of creating a new INNER JOIN (the reported issue)
pf.addConstraint(ConstraintBuilder.of("city", "name").equal(cities.get(0).getName()));Current code (LEFT JOIN reused for the equality predicate on FROM person
LEFT JOIN city ON person.city_id = city.id
WHERE (city.id = ? OR person.id = ?) -- persistedPerson matches (person.id = ?)
AND city.name = 'Springfield' -- city.name is NULL for persistedPerson
-- NULL = 'Springfield' → UNKNOWN → row excluded→ 5 rows "Fixed" code (new INNER JOIN created for FROM person
LEFT JOIN city c1 ON person.city_id = c1.id
INNER JOIN city c2 ON person.city_id = c2.id -- eliminates persistedPerson
WHERE (c1.id = ? OR person.id = ?)
AND c2.name = 'Springfield'→ 5 rows — In both orderings the result is 5. |
1 participant
Close #61
Summary by CodeRabbit
New Features
Tests
Chores