Skip to content

feat: spec for new BaseFilter - #115

Open
scardanzan wants to merge 4 commits into
masterfrom
base-filter
Open

feat: spec for new BaseFilter#115
scardanzan wants to merge 4 commits into
masterfrom
base-filter

Conversation

@scardanzan

@scardanzan scardanzan commented May 14, 2026

Copy link
Copy Markdown
Member

Spec and implementation for BaseFilter

Summary by CodeRabbit

  • New Features
    • Added annotation-driven BaseFilter with fluent ordering and pagination.
    • Introduced field annotations for attribute mapping, inclusive/exclusive ranges, and null handling (including manual fields).
    • Added QueryDao filter(...), filterWithSingleResult(...), and count(...) overloads using a new JPA processor with attribute-path resolution and paging/order support.
    • Enabled customization hooks to extend criteria and predicates.
  • Deprecation / Migration
    • Marked the legacy QuerySpec/constraint-based filtering API as deprecated.
  • Tests
    • Added coverage for hooks, manual-field behavior, invalid configurations, and criteria mutations.

@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

This PR adds an annotation-driven BaseFilter model and JPA criteria processing pipeline, wires it into DAO query APIs and hooks, and deprecates the legacy QuerySpec/Constraint types. It also adds tests and a specification draft covering the new filtering behavior.

Changes

BaseFilter filtering rollout

Layer / File(s) Summary
BaseFilter model and annotations
backend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/BaseFilter.java, backend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/{Attribute,From,To,WhenNull}.java, backend-core-model/src/test/java/com/flowingcode/backendcore/model/filter/BaseFilterTest.java
BaseFilter adds ordered sorting, pagination, fluent setters, and builder support with validation. New annotations map fields to entity attributes, range bounds, and null policies. Tests cover state, ordering, pagination, and builder isolation.
DAO API and path resolution
backend-core-data/src/main/java/com/flowingcode/backendcore/dao/QueryDao.java, backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/AttributePathResolver.java
QueryDao gains BaseFilter overloads for filtering, single-result lookup, and count. AttributePathResolver resolves dotted attribute paths through joins and validates the leaf type.
Processor, DAO hooks, and tests
backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/{BaseFilterJpaProcessor,ConversionJpaDaoSupport}.java, backend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/{BaseFilterDaoHookTest,JpaDaoSupportTest}.java
The processor builds criteria queries from annotated filter fields, applies sorting and paging, and supports hook customization. DAO support routes the new API through the processor, exposes field-value helpers, and the tests exercise hooks, manual fields, invalid combinations, and criteria mutation.
Legacy QuerySpec deprecations
backend-core-model/src/main/java/com/flowingcode/backendcore/model/, backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java
QuerySpec, Constraint, ConstraintBuilder, ConstraintTransformer, ConstraintTransformerException, ConstraintTransformerJpaImpl, and the constraint implementations are marked deprecated with legacy-API Javadocs.
BaseFilter specification draft
specs/base-filter.md
The specification describes the new filter model, annotations, DAO surface, processor flow, hooks, usage example, deprecation plan, placement guidance, and open questions.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

  • FlowingCode/backend-core#108: Updates ConstraintTransformerJpaImpl join handling in the same codepath that this PR now deprecates and isolates behind shared path resolution.

Suggested reviewers: mlopezfc

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: introducing the new BaseFilter spec and implementation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch base-filter

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 PMD (7.26.0)
backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/AttributeILikeConstraint.java

openjdk version "17.0.19" 2026-04-21 LTS
OpenJDK Runtime Environment Corretto-17.0.19.10.1 (build 17.0.19+10-LTS)
OpenJDK 64-Bit Server VM Corretto-17.0.19.10.1 (build 17.0.19+10-LTS, mixed mode, sharing)
/usr/local/bin/pmd: line 89: 22 Aborted java -version > /dev/null 2>&1
No java executable found in PATH


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Basic default implementation for the BaseFilters, extendable to enable creation of custom constraints.
Deprecating QuerySpec and all related methods
@scardanzan
scardanzan marked this pull request as ready for review June 23, 2026 19:02

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Expand toBuilder regression coverage to include orders isolation

The current test proves scalar independence only. Add an assertion path that mutates orders in 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4bcc761 and 7aa87b8.

📒 Files selected for processing (29)
  • backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/AttributePathResolver.java
  • backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/BaseFilterJpaProcessor.java
  • backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConstraintTransformerJpaImpl.java
  • backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConversionJpaDaoSupport.java
  • backend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/BaseFilterDaoHookTest.java
  • backend-core-data-impl/src/test/java/com/flowingcode/backendcore/dao/jpa/JpaDaoSupportTest.java
  • backend-core-data/src/main/java/com/flowingcode/backendcore/dao/QueryDao.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/Constraint.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintBuilder.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformer.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/ConstraintTransformerException.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/QuerySpec.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/AttributeConstraint.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/AttributeInConstraint.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/AttributeNullConstraint.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/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/RelationalConstraint.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/Attribute.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/BaseFilter.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/From.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/To.java
  • backend-core-model/src/main/java/com/flowingcode/backendcore/model/filter/WhenNull.java
  • backend-core-model/src/test/java/com/flowingcode/backendcore/model/filter/BaseFilterTest.java
  • specs/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 javier-godoy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@scardanzan

Copy link
Copy Markdown
Member Author

Issues reported by CodeRabbit fixed

@sonarqubecloud

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lift

Restrict 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 where T is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 40f07ab and 9babdd8.

📒 Files selected for processing (13)
  • backend-core-data-impl/src/main/java/com/flowingcode/backendcore/dao/jpa/ConversionJpaDaoSupport.java
  • backend-core-data/src/main/java/com/flowingcode/backendcore/dao/QueryDao.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/AttributeConstraint.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/AttributeInConstraint.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/AttributeNullConstraint.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/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/RelationalConstraint.java
  • backend-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

@scardanzan

Copy link
Copy Markdown
Member Author

@javier-godoy @mlopezFC default methods implemented to not break compatibility

@mlopezFC mlopezFC left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.filter(j -> j.getAttribute().getName().equals(attributeName))
.filter(j -> j.getJoinType() == currentJoinType)
.findFirst();
return existing.orElseGet(() -> source.join(attributeName, currentJoinType));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WhenNull(IS_NULL) on a nested path can never match a null association.

Joins are always created with JoinType.INNER, so the association is joined away before the IS NULL predicate is evaluated. Confirmed against the branch with @Attribute("city.name") @WhenNull(WhenNull.Policy.IS_NULL) over three Person rows — one in a named city, one in a city whose name is null, one with no city at all:

select ... from Person p1_0 join City c1_0 on c1_0.id=p1_0.city_id where c1_0.name is null

Matched 1 row, not 2 — the person with no city is dropped by the join. Any IS_NULL policy on a dotted path is silently unsatisfiable for exactly the rows it's meant to find, which is the common intent ("city unknown").

Plain @Attribute equality on a nested path has the same shape: rows with a null association are silently excluded. That one is standard JPA behavior rather than a bug, but it isn't mentioned in the @Attribute javadoc, and the auto-join is implicit enough that it will surprise people.

A LEFT join for the traversal fixes the IS_NULL case; see the separate note on setCurrentJoinType for how that might be exposed.

}

/** Adds an order on {@code attribute} with the given {@code direction}. */
public BaseFilter addOrder(String attribute, Order direction) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The orders aliasing fix is only partial — the previous round patched BaseFilterBuilder.addOrder, but the map still escapes two other ways here. Both confirmed failing against the branch:

  1. original.toBuilder().build() copies the map reference, and this instance-level addOrder mutates in place, so it writes through to the source filter:
ProbeFilter original = ProbeFilter.builder().name("Ada").addOrder("name").build();
ProbeFilter copy = original.toBuilder().build();
copy.addOrder("birthDate", Order.DESC);
// original.getOrders() == {name=ASC, birthDate=DESC}  <-- leaked
  1. Lombok's generated builder orders(Map) setter stores the caller's map by reference, so later mutations by the caller land in the built filter:
Map<String, Order> caller = new LinkedHashMap<>(Map.of("name", Order.ASC));
ProbeFilter f = ProbeFilter.builder().orders(caller).build();
caller.put("sneaky", Order.DESC);
// f.getOrders() == {name=ASC, sneaky=DESC}

Copying defensively in $fillValuesFrom/the constructor (or overriding the builder's orders(Map) to copy, matching what addOrder now does) closes both.

* Returns the configured sort orders, preserving insertion order. Never
* {@code null}; an empty map indicates no ordering.
*/
public Map<String, Order> getOrders() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Third aliasing path: getOrders() hands out the live internal map. Confirmed against the branch:

ProbeFilter f = ProbeFilter.builder().addOrder("name").build();
f.getOrders().put("injected", Order.DESC);
// f.getOrders() == {name=ASC, injected=DESC}

There's also an inconsistency worth fixing while you're here: when orders is null this returns Collections.emptyMap(), so the identical put throws UnsupportedOperationException on an unset filter but silently succeeds on a set one. Collections.unmodifiableMap(orders) in the non-null branch makes both cases behave the same and closes the escape.


/** Sets the join type used for newly created joins by subsequent resolutions. */
public void setCurrentJoinType(JoinType joinType) {
this.currentJoinType = Objects.requireNonNull(joinType, "joinType");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This knob is unreachable from the filter API, but the class is public, so it becomes committed API on merge.

BaseFilterJpaProcessor never calls setCurrentJoinType — it constructs a resolver and resolves paths, so every join is INNER and no filter can change that. Since AttributePathResolver is public in com.flowingcode.backendcore.dao.jpa, japicmp will lock this signature from 1.2.0 onward.

Two reasonable directions:

  • Wire it up — e.g. @Attribute(joinType = LEFT) threaded through to the resolver per path. That also fixes the @WhenNull(IS_NULL) issue flagged above, so there's a real reason to do it now rather than later.
  • Reduce visibility to package-private until the join-type story is settled, so you're free to change the shape once the annotation surface is designed.

Either is fine; the thing to avoid is shipping it public and unused.

* @throws IllegalArgumentException if the filter class has no field with the
* given name
*/
default Object getFilterFieldValue(BaseFilter filter, String fieldName) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String-keyed field lookup is refactor-unsafe, and it's on the hot path for anything the declarative model can't express.

Renaming a filter field turns into a runtime IllegalArgumentException rather than a compile error, and nothing points at the hook that referenced the old name. Because @Like/@In/@Or are v1 non-goals, manual = true + getFilterFieldValue is the escape hatch every downstream will reach for the first time they need a LIKE, so this gets a lot of traffic.

The hook already receives the filter instance, so a cast is type-safe and needs no reflection at all:

if (filter instanceof ManualPersonFilter f && f.getNameLike() != null) {
    return List.of(cb.like(root.get("name"), f.getNameLike()));
}

Worth either recommending that pattern in the javadoc and in BaseFilterDaoHookTest, or generifying Hooks on the filter type so the cast disappears too. Not blocking, but it shapes how every consumer writes hooks, so better decided now than after downstreams have built on it.

* subclass.
*/
@Deprecated(since = "1.2.0", forRemoval = false)
List<T> filter(QuerySpec filter);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 QuerySpec is 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.


@Override
public EntityManager getEntityManager() {
return emf.createEntityManager();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EntityManagers leak here — separate from the EntityManagerFactory fix in the last round.

This returns a fresh EntityManager on every call and never closes any of them, and the DAO defaults call getEntityManager() once per filter/count/filterWithSingleResult. predicateHookRestrictsResults alone makes four calls, so each test drops several open EntityManagers on the floor; emf.close() in tearDown cleans up at the factory level and hides it.

Holding a single EntityManager for the DAO's lifetime and closing it in tearDown is the smaller change, and it also makes the test closer to how the DAO is used under Spring (one EntityManager per transaction rather than one per query).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants