feat : introduce criteriagroup for searching #1605
Conversation
WalkthroughThis pull request introduces a comprehensive enhancement to the keyset pagination and query specification system. The changes focus on creating a more flexible and robust search criteria mechanism by introducing an Changes
Sequence DiagramsequenceDiagram
participant Client
participant SearchRequest
participant EntitySpecification
participant CriteriaGroup
participant SearchCriteria
Client->>SearchRequest: Create with search criteria
SearchRequest->>EntitySpecification: Build specification
EntitySpecification->>CriteriaGroup: Process criteria group
CriteriaGroup->>SearchCriteria: Convert individual criteria
SearchCriteria-->>EntitySpecification: Generate predicates
EntitySpecification-->>Client: Return filtered results
Possibly related PRs
Suggested labels
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
type: dependency-upgrade
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/services/EntitySpecification.java (2)
22-28: Consider returning a typed specification.
ThespecificationBuildermethod currently returns a rawSpecification, potentially sacrificing type safety. UsingSpecification<T>instead of an unparameterized type strengthens compile-time checks and improves readability.- public Specification specificationBuilder(List<ISearchCriteria> searchCriteriaList, Class<T> entityClass) { + public Specification<T> specificationBuilder(List<ISearchCriteria> searchCriteriaList, Class<T> entityClass) {
50-53: Simple and effective validation.
Throwing anIllegalArgumentExceptionfor invalid fields is straightforward. Optionally, you may wish to include the entity class name in the error message for more context.- throw new IllegalArgumentException("Invalid field: " + fieldName); + throw new IllegalArgumentException("Invalid field '" + fieldName + "' in entity " + entityClass.getName());jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/model/query/CriteriaGroupTest.java (1)
1-33: Solid JSON serialization test.
This test verifies basic serialization correctly. Consider adding further test cases for multiple nested criteria or different operators to ensure comprehensive coverage.jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/CriteriaGroup.java (2)
13-16: Avoid silent failures in the constructor.
If either theoperatororcriteriaListis null, the subsequent usage (e.g., intoSpecification()) could fail. Consider throwing an IllegalArgumentException or similar if invalid arguments are provided.
22-25: Consider stricter immutability.
The setter pattern is acceptable, but for a domain object representing criteria, immutability can reduce bug surfaces. An alternative would be to create new instances whenever the operator or criteria list changes.jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/repository/EntitySpecificationTest.java (1)
169-176: Comprehensive AND-criteria test.
These assertions confirm that the query logic for twoSearchCriteriaobjects joined by AND returns exactly one matching animal. This validates proper integration withCriteriaGroup. To further improve test coverage, consider adding edge cases where the criteria list is empty or partially null.jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerIT.java (1)
477-477: Optional: unify JSON request structure.
Including"type": "criteria"might be helpful for clarity, but ensure consistent usage throughout the test suite so future maintainers easily recognize the format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
jpa/keyset-pagination/boot-data-window-pagination/pom.xml(1 hunks)jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/CriteriaGroup.java(1 hunks)jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/ISearchCriteria.java(1 hunks)jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SearchCriteria.java(2 hunks)jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SearchRequest.java(1 hunks)jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/services/EntitySpecification.java(2 hunks)jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/model/query/CriteriaGroupTest.java(1 hunks)jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/repository/EntitySpecificationTest.java(2 hunks)jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerIT.java(2 hunks)
🧰 Additional context used
📓 Learnings (1)
jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/services/EntitySpecification.java (1)
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1544
File: jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/services/EntitySpecification.java:29-38
Timestamp: 2024-12-03T09:06:24.894Z
Learning: In `EntitySpecification.specificationBuilder()` method within `EntitySpecification.java`, the `searchCriteriaList` is validated through `SearchCriteria`, ensuring it cannot be null or empty. Therefore, additional null or empty checks in `specificationBuilder()` are unnecessary.
🔇 Additional comments (13)
jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/services/EntitySpecification.java (3)
3-4: Imports look fine.
No issues spotted in these import statements.
31-36: Metadata validation logic appears consistent with retrieved learnings.
As per previous learnings, the searchCriteriaList validation is already handled, so additional null/empty checks are unnecessary. No issues found here.
40-45: Be mindful of nested CriteriaGroups.
This recursive approach processes nested groups effectively. However, if CriteriaGroup objects end up referencing each other in a loop, there’s a potential for infinite recursion. Consider implementing cycle checks if cyclical references are possible.
jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SearchCriteria.java (3)
3-18: Interface implementation and imports look good.
Implementing ISearchCriteria<T> and these imports are aligned with the new design.
63-136: Extensive operator handling in toSpecification().
The switch statement covers a wide range of operators thoroughly. The fallback to default throwing an IllegalArgumentException is good defensive coding.
138-167: Robust type conversion logic.
The convertToType method supports many common types. The exception thrown on invalid/unrecognized types is appropriate.
jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/ISearchCriteria.java (1)
1-20: New interface for polymorphic search criteria.
This JSON-annotated interface nicely supports both CriteriaGroup and SearchCriteria.
jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SearchRequest.java (1)
8-8: Flexibility improvement with List<ISearchCriteria>.
Switching to the interface-based list broadens applicability for future custom criteria. The constructor’s null checks are thoughtful.
Also applies to: 16-16, 21-21, 25-25
jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/CriteriaGroup.java (2)
8-9: Clarify defaults or constraints for operator and criteriaList.
Currently, if a new CriteriaGroup is created without initializing the operator or criteriaList, it may produce unexpected behavior in toSpecification(). Consider adding validations or defaults (e.g., a default operator or an empty list) to avoid potential null pointer issues.
31-34: Validate non-empty criteriaList.
When empty, toSpecification() could lead to an IndexOutOfBoundsException. Consider returning a Specification that always evaluates to true/false for an empty list, or handle it gracefully.
jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/repository/EntitySpecificationTest.java (1)
9-9: Good usage of CriteriaGroup import.
Importing CriteriaGroup helps unify the testing of combined search criteria.
jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerIT.java (1)
585-634: Excellent complex query testing.
This test correctly verifies nested grouping (AND containing an OR group). It ensures that combining CriteriaGroup objects works end-to-end through the controller layer. If possible, add a negative test scenario (e.g., a habitat that doesn't exist) to confirm no records are returned.
jpa/keyset-pagination/boot-data-window-pagination/pom.xml (1)
10-10: Verify Maven dependency compatibility after version downgrade.
Downgrading from 3.4.1 to 3.4.0 may affect plugin or transitive dependencies. Ensure that no feature from 3.4.1 is required elsewhere.
✅ Verification successful
Let me analyze the version usage in the codebase to verify the impact of downgrading from 3.4.1 to 3.4.0.
Let me check the dependencies and configuration in the specific project to understand the impact.
Spring Boot 3.4.0 version is compatible with project dependencies
The downgrade from 3.4.1 to 3.4.0 is safe for the following reasons:
- The project uses standard Spring Boot starters (web, data-jpa, actuator, etc.) which are compatible across minor versions
- Third-party dependency versions used (springdoc-openapi 2.7.0, datasource-micrometer-boot 1.0.6) work with both Spring Boot 3.4.0 and 3.4.1
- No usage of features specific to 3.4.1 was found in the codebase
- The project includes Spring Milestones repository, indicating it's prepared for milestone/RC versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the codebase for explicit references to Spring Boot version 3.4.1 or features introduced after 3.4.0
rg "3.4.1"
Length of output: 66506
Script:
#!/bin/bash
# Check for any Spring Boot version references in pom.xml files
rg -t xml "<version>3\.4\." -g "pom.xml"
# Check for any Spring Boot parent version references
rg -t xml "<spring.boot.version>3\.4\." -g "pom.xml"
rg -t xml "<spring-boot.version>3\.4\." -g "pom.xml"
# Check the parent pom if it exists
fd "^parent.pom.xml$" --exec cat {}
Length of output: 2736
Script:
#!/bin/bash
# Check the full pom.xml content of the project
cat jpa/keyset-pagination/boot-data-window-pagination/pom.xml
# Check for any Spring Boot specific imports or configurations
rg -t java "org.springframework.boot" jpa/keyset-pagination/boot-data-window-pagination/
Length of output: 19229
| private Predicate combinePredicates( | ||
| List<Object> values, | ||
| Function<Object, Predicate> predicateFunction, | ||
| BiFunction<Predicate, Predicate, Predicate> combiner) { | ||
| if (values.size() == 1) { | ||
| return predicateFunction.apply(values.getFirst()); | ||
| } | ||
| return values.stream() | ||
| .map(predicateFunction) | ||
| .reduce(combiner::apply) | ||
| .orElseThrow(() -> new IllegalArgumentException( | ||
| String.format("No predicates could be generated from values: %s", values))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Likely bug in single-value predicate creation.
Line 174 uses values.getFirst(), but List in Java has no getFirst() method. This should be values.get(0) unless a custom List implementation provides it. This can cause a runtime error.
- return predicateFunction.apply(values.getFirst());
+ return predicateFunction.apply(values.get(0));📝 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.
| private Predicate combinePredicates( | |
| List<Object> values, | |
| Function<Object, Predicate> predicateFunction, | |
| BiFunction<Predicate, Predicate, Predicate> combiner) { | |
| if (values.size() == 1) { | |
| return predicateFunction.apply(values.getFirst()); | |
| } | |
| return values.stream() | |
| .map(predicateFunction) | |
| .reduce(combiner::apply) | |
| .orElseThrow(() -> new IllegalArgumentException( | |
| String.format("No predicates could be generated from values: %s", values))); | |
| } | |
| private Predicate combinePredicates( | |
| List<Object> values, | |
| Function<Object, Predicate> predicateFunction, | |
| BiFunction<Predicate, Predicate, Predicate> combiner) { | |
| if (values.size() == 1) { | |
| return predicateFunction.apply(values.get(0)); | |
| } | |
| return values.stream() | |
| .map(predicateFunction) | |
| .reduce(combiner::apply) | |
| .orElseThrow(() -> new IllegalArgumentException( | |
| String.format("No predicates could be generated from values: %s", values))); | |
| } |
| @Override | ||
| public Specification<?> toSpecification() { | ||
| List<Specification> specs = | ||
| criteriaList.stream().map(ISearchCriteria::toSpecification).toList(); | ||
|
|
||
| Specification result = specs.getFirst(); | ||
| for (int i = 1; i < specs.size(); i++) { | ||
| if (getOperator() == QueryOperator.AND) { | ||
| result = result.and(specs.get(i)); | ||
| } else if (getOperator() == QueryOperator.OR) { | ||
| result = result.or(specs.get(i)); | ||
| } else { | ||
| throw new UnsupportedOperationException("Operator not supported in group: " + operator); | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Prevent possible IndexOutOfBoundsException in specs.getFirst().
- If
criteriaListis empty, thenspecsis empty too, andspecs.getFirst()is not a standard JavaListmethod unless you rely on a specific library extension. Double-check the utility used here. - If
specsis from a standardList, you probably needspecs.get(0)instead. - Consider returning a no-op specification when empty instead of throwing a runtime error.
- Specification result = specs.getFirst();
+ if (specs.isEmpty()) {
+ return (root, query, cb) -> cb.conjunction();
+ }
+ Specification result = specs.get(0);📝 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.
| @Override | |
| public Specification<?> toSpecification() { | |
| List<Specification> specs = | |
| criteriaList.stream().map(ISearchCriteria::toSpecification).toList(); | |
| Specification result = specs.getFirst(); | |
| for (int i = 1; i < specs.size(); i++) { | |
| if (getOperator() == QueryOperator.AND) { | |
| result = result.and(specs.get(i)); | |
| } else if (getOperator() == QueryOperator.OR) { | |
| result = result.or(specs.get(i)); | |
| } else { | |
| throw new UnsupportedOperationException("Operator not supported in group: " + operator); | |
| } | |
| } | |
| return result; | |
| } | |
| @Override | |
| public Specification<?> toSpecification() { | |
| List<Specification> specs = | |
| criteriaList.stream().map(ISearchCriteria::toSpecification).toList(); | |
| if (specs.isEmpty()) { | |
| return (root, query, cb) -> cb.conjunction(); | |
| } | |
| Specification result = specs.get(0); | |
| for (int i = 1; i < specs.size(); i++) { | |
| if (getOperator() == QueryOperator.AND) { | |
| result = result.and(specs.get(i)); | |
| } else if (getOperator() == QueryOperator.OR) { | |
| result = result.or(specs.get(i)); | |
| } else { | |
| throw new UnsupportedOperationException("Operator not supported in group: " + operator); | |
| } | |
| } | |
| return result; | |
| } |
Qodana Community for JVM102 new problems were found
☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
Summary by CodeRabbit
New Features
Improvements
Tests
Chores