feat: add OR operator to QuerySpec constraints#108
Draft
javier-godoy wants to merge 2 commits intomasterfrom
Draft
feat: add OR operator to QuerySpec constraints#108javier-godoy wants to merge 2 commits intomasterfrom
javier-godoy wants to merge 2 commits intomasterfrom
Conversation
WalkthroughAdds OR (disjunction) support: new DisjunctionConstraint model and factory, convenience Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Close #61
Summary by CodeRabbit
New Features
Tests
Chores