feat: spec for new BaseFilter - #115
scardanzan wants to merge 6 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: WalkthroughThis PR adds an annotation-driven ChangesBaseFilter filtering rollout
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
javier-godoy
left a comment
javier-godoy
left a comment
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.
- run filterWithSingleResult as a dedicated non-paged query using getSingleResult - copy orders map in builder addOrder to keep toBuilder copies independent - validate attribute paths eagerly in AttributePathResolver - close EntityManagerFactory after each BaseFilterDaoHookTest
|
Issues reported by CodeRabbit fixed |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConversionJpaDaoSupport.java (1)
171-181: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRestrict criteria customization for count and single-result queries.
This hook is documented to run for count and single-result queries while allowing projections and
groupBy. A customization can therefore replace the count selection, produce multiple count rows, or return scalar/object-array results whereTis expected. Restrict this hook to entity-result queries, or provide the query mode and enforce count/single-result-safe mutations.🤖 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/ConversionJpaDaoSupport.java` around lines 171 - 181, Update customizeCriteria and its callers so customization runs only for entity-result queries, excluding count and single-result paths; preserve the existing filter behavior and ensure count queries retain their count selection and single-result queries retain their expected T-shaped result.
🤖 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.
Outside diff comments:
In
`@backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConversionJpaDaoSupport.java`:
- Around line 171-181: Update customizeCriteria and its callers so customization
runs only for entity-result queries, excluding count and single-result paths;
preserve the existing filter behavior and ensure count queries retain their
count selection and single-result queries retain their expected T-shaped result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b0e8f74-4751-4063-abe8-3c35a5c0950e
📒 Files selected for processing (13)
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConversionJpaDaoSupport.javabackend-core-data/src/main/java/com/flowingcode/backendcore/dao/QueryDao.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/test/java/com/flowingcode/backendcore/model/filter/BaseFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (12)
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeNullConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/RelationalConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeLikeConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeInConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeBetweenConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeILikeConstraint.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/AttributeRelationalConstraint.java
- backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/NegatedConstraint.java
- backend-core-data/src/main/java/com/flowingcode/backendcore/dao/QueryDao.java
- backend-core-model/src/test/java/com/flowingcode/backendcore/model/filter/BaseFilterTest.java
|
@javier-godoy @mlopezFC default methods implemented to not break compatibility |
mlopezFC
left a comment
mlopezFC
left a comment
There was a problem hiding this comment.
Reviewed on top of the earlier round, so I've skipped what's already addressed in 40f07ab/9babdd8 (path validation, filterWithSingleResult paging, EMF close, and the japicmp breakage — CI is green now).
Two correctness issues below are confirmed by tests I ran locally against the branch; the rest are API-surface questions worth settling before this becomes committed API, since japicmp will lock it.
Still open from the previous round and not re-reported here: customizeCriteria running for count and single-result queries, the toBuilder orders test nitpick, and the 42.86% docstring gate.
Remaining smaller items (no distinct on count(), HashMap predicate ordering, METADATA_CACHE retention, unvalidated @Attribute paths, JPMS setAccessible, undocumented paging in filterWithSingleResult, subclass-chaining on the base setters, and the missing end-to-end coverage for the annotation pipeline) we'll file as issues after merge.
Build note for anyone picking this up locally: Lombok 1.18.32 doesn't process under JDK 23 — every @SuperBuilder/@Getter fails to compile. JDK 17 works.
| * subclass. | ||
| */ | ||
| @Deprecated(since = "1.2.0", forRemoval = false) | ||
| List<T> filter(QuerySpec filter); |
There was a problem hiding this comment.
@scardanzan — a question rather than a finding, and it's really a call for you as the spec author.
Every QuerySpec method is deprecated as of this PR, but the spec lists @Like/@ILike/@In/@Or/@Not and returnedAttributes-equivalent projections as explicit v1 non-goals. So a downstream that filters with a LIKE or an IN, or that uses projections, now gets a deprecation warning with nothing to migrate to — the only path is hand-written criteria via manual = true plus a hook, which is more code than the QuerySpec call it replaces.
That feels like it inverts the intent of "downstream consumers can migrate at their own pace": the warnings arrive before the replacement can absorb the traffic.
What's your read on the timing? A few options:
- Hold the deprecation until BaseFilter reaches rough parity (operators + projections), shipping this release as additive only.
- Deprecate now but only the methods that have a real replacement, leaving the rest clean until parity.
- Deprecate everything now as a deliberate signal that
QuerySpecis frozen, and accept that some call sites will carry suppressions for a while.
Happy with any of them — I'd just rather the choice be explicit in the spec's deprecation section than a side effect of marking the whole surface at once. Since forRemoval = false, there's no hard deadline either way.
There was a problem hiding this comment.
The new BaseFilter is not intended to cover all the cases QuerySpec covered, the idea is to provide a baseline and having downstream developers implement complex queries by hand in a single place. The promise to allow consumers to migrate at their own pace refers to the fact that while QuerySpec was deprecated it will no be removed anytime soon. You can use it as is and migrate dao by dao.
I added some of the out of scope operations.
The orders map could still leak between instances after the previous fix: toBuilder() copied the map reference and the instance addOrder mutated it in place, the builder and POJO orders setters stored the caller's map, and getOrders() returned the live map. Both addOrder variants now copy before writing, both setters copy their argument, and getOrders() returns an unmodifiable view.
@like matches String fields with LIKE, escaping wildcards unless match = RAW, optionally ignoring case. @in matches Collection fields with IN; an empty collection is skipped unless whenEmpty = MATCH_NONE. @or places a field's predicate in the filter's single disjunction. Associations are left-joined where an inner join would drop rows the query must return: IS_NULL predicates, @or fields and sort orders. A join is reused whatever its type, so each association is joined once. AttributePathResolver becomes package-private. customizeCriteria may no longer replace the query selection or group the count query, and a distinct count query now counts distinct roots. getFilterFieldValue is removed: hooks read filter values through the filter's accessors, which filters must provide for the fields hooks consume. This also fixes annotated fields shadowed by a subclass field, which were never made accessible. Fix the hook tests, which failed to resolve the entity type of anonymous DAO subclasses and leaked an EntityManager per query, and add end-to-end coverage of the annotation pipeline. Update the spec.
|
| this.hooks = hooks; | ||
| } | ||
|
|
||
| List<T> filter(BaseFilter filter) { |
There was a problem hiding this comment.
Collection paths return duplicate entities
A predicate on a one-to-many path (e.g. @Attribute("tags.name")) adds a plain join, and nothing removes duplicate rows. filter() returns the same entity once per matching child, count() counts join rows (cb.count(root), line 144), and paging slices rows instead of entities.
Either add distinct automatically when the path goes through a collection, or state in the spec (§4.1) that collection paths aren't supported. For now, a DAO can work around it by calling cq.distinct(true) in customizeCriteria.
| List<jakarta.persistence.criteria.Order> jpaOrders = new ArrayList<>(orders.size()); | ||
| for (Entry<String, BaseFilter.Order> e : orders.entrySet()) { | ||
| // Left join: sorting must not drop rows whose association is null. | ||
| Expression<?> expr = resolver.resolve(e.getKey(), JoinType.LEFT); |
There was a problem hiding this comment.
Sorting on a collection path makes filter() and count() disagree
Orders left-join their path only in filter(). On a collection path such as addOrder("phones.number"), that join multiplies rows while count() stays the same. This breaks the rule in spec §4.1: "sorting must not change the result set, nor make filter disagree with count".
It has the same cause as the duplicate-results issue at line 109. Rejecting sort paths that go through a collection is another option.
| private From<?, ?> join(From<?, ?> source, String attributeName, JoinType joinType) { | ||
| Optional<Join> existing = source.getJoins().stream() | ||
| .map(j -> (Join) j) | ||
| .filter(j -> j.getAttribute().getName().equals(attributeName)) |
There was a problem hiding this comment.
Possible NullPointerException
j.getAttribute().getName() assumes every join has an attribute. Hibernate 6 entity joins (root.join(Other.class)) have none. Because customizePredicates runs before applyOrders, a hook that adds such a join, combined with a sort on a nested path, fails with an NPE.
Suggested fix: skip joins whose getAttribute() is null before comparing names.
| Predicate toPredicate(BaseFilter filter, CriteriaBuilder cb, AttributePathResolver resolver) | ||
| throws IllegalAccessException { | ||
| String value = (String) field.get(filter); | ||
| if (value == null) return null; |
There was a problem hiding this comment.
@Like doesn't skip empty strings
Only null is skipped. A cleared Vaadin TextField returns "", which becomes LIKE '%%'. That isn't a no-op: it excludes rows where the attribute is null, and on a nested path it inner-joins, which also drops rows whose association is null.
Suggested fix: treat an empty value (or a blank one) as "no criterion", like null.
| } | ||
|
|
||
| private static final class LikeHandler extends FieldHandler { | ||
| private static final char ESCAPE = '\\'; |
There was a problem hiding this comment.
Backslash used as the LIKE escape character
In MySQL and MariaDB's default SQL mode, \ is also an escape character inside string literals. Depending on how the dialect prints it, escape '\' can produce broken SQL or wrong matches. H2 and PostgreSQL are fine.
A character with no special meaning, such as '!', avoids the problem, and escape() already works with any ESCAPE value.
| private static final List<Class<? extends Annotation>> MODIFIERS = | ||
| List.of(From.class, To.class, Like.class, In.class, WhenNull.class, Or.class); | ||
|
|
||
| private static final ConcurrentMap<Class<? extends BaseFilter>, List<FieldHandler>> HANDLER_CACHE = |
There was a problem hiding this comment.
Static HANDLER_CACHE can keep classloaders alive (can be fixed later)
The ConcurrentHashMap holds strong references to filter classes and their Fields. When backend-core and the application are loaded by different classloaders (Spring DevTools restarts, a shared library on an app server), old classloaders stay alive after each restart. ClassValue<List<FieldHandler>> fixes this.
It's internal only and doesn't touch the API, so it can wait for a later PR.
3 participants
Spec and implementation for BaseFilter
Summary by CodeRabbit
BaseFilterwith fluent ordering and pagination.QueryDaofilter(...),filterWithSingleResult(...), andcount(...)overloads using a new JPA processor with attribute-path resolution and paging/order support.QuerySpec/constraint-based filtering API as deprecated.