feat: spec for new BaseFilter#115
Open
scardanzan wants to merge 2 commits into
Open
Conversation
WalkthroughThis PR introduces a comprehensive annotation-driven JPA criteria filtering abstraction named BaseFilter to replace the imperative QuerySpec/Constraint model. It adds a new BaseFilter class with ordering and pagination, field-level metadata annotations ( ChangesBaseFilter JPA Criteria Filtering Implementation
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
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 |
Basic default implementation for the BaseFilters, extendable to enable creation of custom constraints. Deprecating QuerySpec and all related methods
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend-core-model/src/test/java/com/flowingcode/backendcore/model/filter/BaseFilterTest.java (1)
139-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand
toBuilderregression coverage to includeordersisolationThe current test proves scalar independence only. Add an assertion path that mutates
ordersin the cloned builder so aliasing bugs on mutable fields are caught.Suggested test update
`@Test` void toBuilder_producesIndependentCopy() { - SampleFilter original = SampleFilter.builder().name("Ada").maxResult(50).build(); - SampleFilter tweaked = original.toBuilder().maxResult(10).build(); + SampleFilter original = SampleFilter.builder() + .name("Ada") + .addOrder("name") + .maxResult(50) + .build(); + SampleFilter tweaked = original.toBuilder() + .addOrder("birthDate", BaseFilter.Order.ASC) + .maxResult(10) + .build(); assertEquals(50, original.getMaxResult()); assertEquals(10, tweaked.getMaxResult()); assertEquals("Ada", tweaked.getName()); + assertIterableEquals(Arrays.asList("name"), original.getOrders().keySet()); + assertIterableEquals(Arrays.asList("name", "birthDate"), tweaked.getOrders().keySet()); assertNotSame(original, tweaked); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend-core-model/src/test/java/com/flowingcode/backendcore/model/filter/BaseFilterTest.java` around lines 139 - 147, The test method `toBuilder_producesIndependentCopy` currently only verifies independence of scalar fields like name and maxResult. Add test assertions to verify that mutable collection fields like orders are also independently copied and not aliased. Modify the test to create a SampleFilter with initial orders, clone it using toBuilder(), mutate the orders collection in the cloned builder, then assert that the original filter's orders remain unchanged while the cloned filter's orders reflect the mutation. This ensures proper deep copying of mutable fields and catches potential aliasing bugs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/AttributePathResolver.java`:
- Around line 75-83: The resolve method accepts malformed attribute paths that
fail later with provider-specific exceptions instead of failing fast. Add
explicit validation for the attributePath parameter after the null check to
ensure the path is not blank, does not have leading or trailing dots, and does
not contain consecutive dots (like a..b). Throw an IllegalArgumentException with
a descriptive message if any of these conditions are detected, before proceeding
to split and process the path. This will catch invalid inputs deterministically
at the entry point of the resolve method.
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/BaseFilterJpaProcessor.java`:
- Around line 107-113: The filterWithSingleResult method calls filter(filter)
which applies paging settings from the filter parameter, potentially truncating
results before the greater-than-one check occurs. This masks cases where
multiple matches actually exist. Refactor the filterWithSingleResult method to
create a dedicated query that bypasses the filter's firstResult and maxResult
paging settings and instead limits results to 2 rows for cardinality detection.
This ensures the method reliably detects when more than one match exists
regardless of any paging configuration in the original filter.
In
`@backend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/BaseFilterDaoHookTest.java`:
- Around line 41-42: The EntityManagerFactory created in the setUp() method is
never closed, causing resource leaks. Add an `@AfterEach` import from
org.junit.jupiter.api and create a corresponding tearDown() or cleanup() method
annotated with `@AfterEach` that properly closes the EntityManagerFactory instance
after each test completes. This ensures resources are released and prevents
destabilization of longer test runs.
In
`@backend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/BaseFilter.java`:
- Around line 148-153: The addOrder method in BaseFilter mutates the orders map
in place, which causes issues when the builder originates from toBuilder()
because the builder and the original filter instance share the same map
reference. To fix this, ensure that when the builder is initialized (in the
toBuilder() method or builder constructor), the orders map is defensively copied
into a new LinkedHashMap rather than sharing the reference. This way, when
addOrder is called on the builder, it modifies only the builder's copy and not
the original filter's orders.
---
Nitpick comments:
In
`@backend-core-model/src/test/java/com/flowingcode/backendcore/model/filter/BaseFilterTest.java`:
- Around line 139-147: The test method `toBuilder_producesIndependentCopy`
currently only verifies independence of scalar fields like name and maxResult.
Add test assertions to verify that mutable collection fields like orders are
also independently copied and not aliased. Modify the test to create a
SampleFilter with initial orders, clone it using toBuilder(), mutate the orders
collection in the cloned builder, then assert that the original filter's orders
remain unchanged while the cloned filter's orders reflect the mutation. This
ensures proper deep copying of mutable fields and catches potential aliasing
bugs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6799d05f-eeb9-49a1-9795-669ed50aab2f
📒 Files selected for processing (29)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/AttributePathResolver.javabackend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/BaseFilterJpaProcessor.javabackend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.javabackend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConversionJpaDaoSupport.javabackend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/BaseFilterDaoHookTest.javabackend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.javabackend-core-data/src/main/java/com/flowingcode/backendcore/dao/QueryDao.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintBuilder.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformerException.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/QuerySpec.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeBetweenConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeILikeConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeInConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeLikeConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeNullConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeRelationalConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/NegatedConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/RelationalConstraint.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/Attribute.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/BaseFilter.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/From.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/To.javabackend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/WhenNull.javabackend-core-model/src/test/java/com/flowingcode/backendcore/model/filter/BaseFilterTest.javaspecs/base-filter.md
✅ Files skipped from review due to trivial changes (9)
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeInConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeConstraint.java
- backend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/NegatedConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeRelationalConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeBetweenConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintBuilder.java
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/base-filter.md
Comment on lines
+75
to
+83
| public <V> Expression<V> resolve(String attributePath, Class<V> expectedType) { | ||
| Objects.requireNonNull(attributePath, "attributePath"); | ||
| String[] path = attributePath.split("\\."); | ||
| String attributeName = path[path.length - 1]; | ||
| String[] joinPath = Arrays.copyOf(path, path.length - 1); | ||
| Expression<?> expression = traverse(root, joinPath).get(attributeName); | ||
| boxed(expression.getJavaType()).asSubclass(expectedType); | ||
| return (Expression<V>) expression; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail fast on malformed attribute paths.
resolve accepts malformed values (e.g. blank, leading/trailing dot, a..b) and then fails later with provider-specific exceptions. Add explicit validation here to return a deterministic IllegalArgumentException.
Proposed fix
public <V> Expression<V> resolve(String attributePath, Class<V> expectedType) {
Objects.requireNonNull(attributePath, "attributePath");
+ if (attributePath.isBlank() || attributePath.startsWith(".")
+ || attributePath.endsWith(".") || attributePath.contains("..")) {
+ throw new IllegalArgumentException("Invalid attributePath: " + attributePath);
+ }
String[] path = attributePath.split("\\.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/AttributePathResolver.java`
around lines 75 - 83, The resolve method accepts malformed attribute paths that
fail later with provider-specific exceptions instead of failing fast. Add
explicit validation for the attributePath parameter after the null check to
ensure the path is not blank, does not have leading or trailing dots, and does
not contain consecutive dots (like a..b). Throw an IllegalArgumentException with
a descriptive message if any of these conditions are detected, before proceeding
to split and process the path. This will catch invalid inputs deterministically
at the entry point of the resolve method.
Comment on lines
+107
to
+113
| Optional<T> filterWithSingleResult(BaseFilter filter) { | ||
| List<T> result = filter(filter); | ||
| if (result.size() > 1) { | ||
| throw new IllegalStateException("Current filter returned more than one result"); | ||
| } | ||
| return result.stream().findFirst(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
filterWithSingleResult can return incorrect results when paging is set.
This method calls filter(filter), so firstResult/maxResult may truncate matches before the > 1 check. That violates the method contract and can hide multi-match cases. Build a dedicated query and cap it to 2 rows for cardinality detection, without applying filter paging.
Proposed fix
Optional<T> filterWithSingleResult(BaseFilter filter) {
- List<T> result = filter(filter);
+ CriteriaBuilder cb = em.getCriteriaBuilder();
+ CriteriaQuery<T> cq = cb.createQuery(persistentClass);
+ Root<T> root = cq.from(persistentClass);
+ cq.select(root);
+ applyPredicates(filter, cb, cq, root);
+ applyOrders(filter, cb, cq, root);
+ hooks.customizeCriteria(filter, cb, cq, root);
+ List<T> result = em.createQuery(cq).setMaxResults(2).getResultList();
if (result.size() > 1) {
throw new IllegalStateException("Current filter returned more than one result");
}
return result.stream().findFirst();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/BaseFilterJpaProcessor.java`
around lines 107 - 113, The filterWithSingleResult method calls filter(filter)
which applies paging settings from the filter parameter, potentially truncating
results before the greater-than-one check occurs. This masks cases where
multiple matches actually exist. Refactor the filterWithSingleResult method to
create a dedicated query that bypasses the filter's firstResult and maxResult
paging settings and instead limits results to 2 rows for cardinality detection.
This ensures the method reliably detects when more than one match exists
regardless of any paging configuration in the original filter.
Comment on lines
+41
to
+42
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close EntityManagerFactory after each test.
A new EntityManagerFactory is created in setUp() for every test but never closed. This can leak resources and destabilize longer CI runs.
Proposed fix
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@@
private HookDao dao;
+ private EntityManagerFactory emf;
@@
`@BeforeEach`
void setUp() {
- EntityManagerFactory emf = Persistence.createEntityManagerFactory("person");
+ emf = Persistence.createEntityManagerFactory("person");
EntityManager em = emf.createEntityManager();
@@
dao = new HookDao(emf);
}
+
+ `@AfterEach`
+ void tearDown() {
+ if (emf != null && emf.isOpen()) {
+ emf.close();
+ }
+ }Also applies to: 103-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/BaseFilterDaoHookTest.java`
around lines 41 - 42, The EntityManagerFactory created in the setUp() method is
never closed, causing resource leaks. Add an `@AfterEach` import from
org.junit.jupiter.api and create a corresponding tearDown() or cleanup() method
annotated with `@AfterEach` that properly closes the EntityManagerFactory instance
after each test completes. This ensures resources are released and prevents
destabilization of longer test runs.
Comment on lines
+148
to
+153
| public B addOrder(String attribute, Order direction) { | ||
| if (this.orders == null) { | ||
| this.orders = new LinkedHashMap<>(); | ||
| } | ||
| this.orders.put(attribute, direction); | ||
| return self(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
toBuilder().addOrder(...) can mutate the original filter instance
On Line 149-Line 153, the builder mutates this.orders in place. When the builder comes from toBuilder(), this can share the same map reference as the original object, so adding an order in the builder can modify the original filter before build(). That breaks copy-independence expectations.
Proposed fix
public B addOrder(String attribute, Order direction) {
- if (this.orders == null) {
- this.orders = new LinkedHashMap<>();
- }
+ this.orders = (this.orders == null)
+ ? new LinkedHashMap<>()
+ : new LinkedHashMap<>(this.orders);
this.orders.put(attribute, direction);
return self();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/BaseFilter.java`
around lines 148 - 153, The addOrder method in BaseFilter mutates the orders map
in place, which causes issues when the builder originates from toBuilder()
because the builder and the original filter instance share the same map
reference. To fix this, ensure that when the builder is initialized (in the
toBuilder() method or builder constructor), the orders map is defensively copied
into a new LinkedHashMap rather than sharing the reference. This way, when
addOrder is called on the builder, it modifies only the builder's copy and not
the original filter's orders.
javier-godoy
requested changes
Jun 25, 2026
javier-godoy
left a comment
javier-godoy
left a comment
Member
There was a problem hiding this comment.
This PR introduce breaking changes, but it targets a minor version (and the commit is not marked as breaking):
- com.flowingcode.backendcore.dao.QueryDao.count(com.flowingcode.backendcore.model.filter.BaseFilter):METHOD_ADDED_TO_INTERFACE
- com.flowingcode.backendcore.dao.QueryDao.filter(com.flowingcode.backendcore.model.filter.BaseFilter):METHOD_ADDED_TO_INTERFACE
- com.flowingcode.backendcore.dao.QueryDao.filterWithSingleResult(com.flowingcode.backendcore.model.filter.BaseFilter):METHOD_ADDED_TO_INTERFACE
(you can run mvn verify for a report on source and binary incompatible changes)
Action required: please refactor as a non-breaking feature; or confirm that a breaking change is intended, mark the commit as a breaking change and increment major version.
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
2 participants
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.
Spec and implementation for BaseFilter
Summary by CodeRabbit
BaseFilterabstraction with fluent sorting and pagination.BaseFilter.QuerySpec/constraint-based filtering API as deprecated in favor ofBaseFilter.