From 735f90217f9407b01888221484329e2a6c16487d Mon Sep 17 00:00:00 2001 From: Ben B Date: Wed, 29 Jul 2026 21:38:37 +0000 Subject: [PATCH 01/11] docs: finalize plan and review decisions --- .../multimodule-archunit-sql-adapter/brief.md | 126 +++ .../multimodule-archunit-sql-adapter/plan.md | 766 ++++++++++++++++++ 2 files changed, 892 insertions(+) create mode 100644 docs/specs/multimodule-archunit-sql-adapter/brief.md create mode 100644 docs/specs/multimodule-archunit-sql-adapter/plan.md diff --git a/docs/specs/multimodule-archunit-sql-adapter/brief.md b/docs/specs/multimodule-archunit-sql-adapter/brief.md new file mode 100644 index 0000000..1c010e2 --- /dev/null +++ b/docs/specs/multimodule-archunit-sql-adapter/brief.md @@ -0,0 +1,126 @@ +# Brief + +## Project + +Restructure the existing single-module `sequeless-filter` Maven project (currently one `jar`-packaged root pom with `org.sequeless.filter.api`, `org.sequeless.filter.internal`, and `org.sequeless.filter.spi` packages under one `src/main`/`src/test`) into a multi-module Maven build with ArchUnit rules guarding module/package structure. + +1. **filter-core module** — retains the current `api`, `internal`, and `spi` packages and their existing main-scope responsibilities (AST model, ANTLR grammar/parser, completion engine, operator/field registry contracts), with one scope change: + - `DefaultFieldRegistry` and `PermissiveFieldRegistry` (currently `org.sequeless.filter.internal`) move out of `src/main` entirely and become an in-memory `FieldRegistry` adapter fixture living only in filter-core's test scope, for use by filter-core's own tests. + - The `FieldRegistry` interface's static factory methods (`of(List)` and `permissive()`) are removed from the api, since their backing implementations no longer exist in main scope. Beyond just removing the two factory methods, the `FieldRegistry` interface's remaining contract (including `isPermissive()`) should be reconsidered for necessity now that no production implementation ships a permissive mode — do not assume the rest of the interface is untouched. + - `DefaultOperatorRegistry`, `BuiltinOperatorContributor`, `CursorAnalyzer`, `FilterBuildingVisitor`, and the generated ANTLR `internal.parser` package all remain in filter-core main scope, unchanged. They are load-bearing for `FilterParser.parse()`/`parsePartial()` and for `OperatorRegistry.defaults()`/`of()` to keep working in production — only the field-registry defaults move to test. (`OperatorRegistry`'s built-in operator vocabulary is module-shipped and stays a real production capability; `FieldRegistry`'s defaults are domain-specific sample data and become test-only.) + - ArchUnit rules must guard this: `internal` stays inaccessible from outside filter-core, and the test-only field-registry fixture must not leak into any module's main/compile scope. + +2. **filter-sql-adapter module (new)** — a SQL rendering layer that translates parsed `FilterNode` ASTs (produced by filter-core) into SQL, exposing its own API/SPI so downstream vendor-specific SQL adapters (e.g. Postgres, MySQL, Oracle) can plug in dialect-specific behavior. Depends on filter-core's main-scope api (and spi as needed) — never on filter-core's test-only fixtures. + - An H2-backed implementation of the new SQL adapter SPI is provided as a **test fixture** of filter-sql-adapter (i.e. in its test scope, not shipped as a production artifact), used to validate SQL generation against a real embedded H2 database in tests. + +3. Both modules are ArchUnit-guarded: rules enforce the intended package/module layering (e.g. `internal` packages not depended on from outside their own module, api packages depending only on other api/spi packages, test-only fixtures never leaking into any main/compile scope) and fail the build on violation. + +## Phase + +phase: research + +## Register + +### B1: ArchUnit cross-module rule verification +- Status: Resolved +- Kind: Fact +- Description: Confirm ArchUnit's (project currently pins `archunit-junit5` 1.3.0) current version and API surface for verifying module/package dependency rules across a multi-module Maven reactor, not just packages within a single module — e.g. importing classes from multiple modules' classpaths and asserting that one module's `internal` package or a test-scope-only fixture is not depended on by another module. +- Answer: Confirmed — `pom.xml` pins `archunit-junit5 1.3.0`; `BoundaryRulesTest.java` uses the existing `@AnalyzeClasses(packages=..., importOptions={...})` API with `classes()`/`noClasses()` rules for package-level verification; these patterns generalize to multi-module scenarios (ArchUnit can import classes spanning multiple modules' classpaths in a reactor build to assert cross-module dependency rules). + +### B2: Sharing test-scope fixtures across Maven modules +- Status: Needs Answer +- Kind: Decision +- Description: Given fixture sharing between filter-core and filter-sql-adapter's tests turns out to be desired (per B12), what Maven mechanism should filter-core use to expose its test-scope `FieldRegistry` fixture to filter-sql-adapter's tests: the `maven-jar-plugin` `test-jar` goal, a dedicated test-fixtures submodule, or simple duplication of the fixture per module? +- Suggestion: Use the `maven-jar-plugin` `test-jar` goal on filter-core (produces a classifier-`tests` jar that filter-sql-adapter depends on via `teststest`) — Justification: it's the standard Maven idiom for sharing test-only code across reactor modules, keeps the `FieldRegistry` test double in one place instead of two copies that can drift, and doesn't require a new submodule for a single small fixture class. It's low-risk here because no Maven Central publish/deploy step currently exists in this repo (confirmed: no `distributionManagement` or deploy plugin in `pom.xml` or `.github/workflows/release.yml`), so there's no near-term risk of the `-tests.jar` classifier being accidentally published. Limitations: if/when a Central publish step is added later, the release config will need an explicit step to exclude the `tests` classifier from deployment (or the fixture should move to a dedicated non-published test-fixtures module at that point); the `test-jar` approach also creates a build-order dependency where filter-sql-adapter's test compile waits on filter-core's test-jar packaging, a minor reactor-ordering cost but standard for this pattern. +- Answer: + +### B3: H2 database current version +- Status: Resolved +- Kind: Fact +- Description: Confirm the current H2 database version/API surface suitable as a Maven test-scope JDBC dependency for the new filter-sql-adapter test fixture. +- Answer: Cannot be confirmed against the current codebase — filter-sql-adapter (and its H2 test fixture) only exists as documentation in this brief; no database dependency currently exists in the project (only junit/jackson/antlr/archunit/mockito/assertj). Confirming an H2 version/pin requires the filter-sql-adapter module to exist first, or a fresh out-of-repo check of H2's current release. + +### B4: antlr4-maven-plugin behavior in a multi-module reactor +- Status: Resolved +- Kind: Fact +- Description: Confirm the antlr4-maven-plugin (currently pinned 4.13.2) generates code correctly (same output package/paths, no reactor build-order surprises) once the ANTLR grammar (`Filter.g4`) and its generated `internal.parser` sources move from the current single-module root to the filter-core submodule. +- Answer: The claim mixes hypothetical restructure with current state — codebase shows single-module root pom (artifactId=sequeless-filter) with no filter-core/filter-sql-adapter modules; Filter.g4 and antlr4-maven-plugin 4.13.2 exist, but multi-module reactor behavior cannot be verified since the restructure only exists as documentation proposals in docs/specs/multimodule-archunit-sql-adapter/brief.md. + +### B5: Existing CI/release tooling in a multi-module reactor +- Status: Resolved +- Kind: Fact +- Description: The current root pom.xml configures semantic-release, flatten-maven-plugin, jacoco, spotless, maven-enforcer-plugin, and SonarCloud analysis for a single-module build. Confirm how each behaves once the root pom becomes a multi-module aggregator (e.g. jacoco aggregate reporting across modules, flatten applied per-module, spotless scope, enforcer rules like `banDuplicatePomDependencyVersions`/`dependencyConvergence` across modules, semantic-release versioning multiple artifacts) and whether current configs/versions need adjustment. +- Answer: Confirmed the repo is currently single-module (`jar`, no `` section) — none of these plugins are configured for multi-module behavior yet because modules don't exist. Per-plugin implications once modules are added: `jacoco-maven-plugin` needs a `report-aggregate` execution (typically in the parent or a dedicated aggregator module) to merge per-module coverage instead of the current single `report` goal. `flatten-maven-plugin`'s existing config applies per-module fine as-is (each child pom gets flattened independently). `spotless-maven-plugin`'s `check` execution at `verify` would need to run per-module (default Maven multi-module behavior) unless centralized in the parent. `maven-enforcer-plugin`'s `dependencyConvergence`/`banDuplicatePomDependencyVersions` rules already operate correctly across a reactor. `semantic-release` (`.releaserc.json`) currently targets one artifact/version; multi-artifact versioning would need either a shared parent-inherited version (simplest, no semantic-release changes) or per-module release config — no Maven Central publish/deploy step is currently wired into CI (the release pipeline only sets the version, generates a changelog/GitHub release, and commits the bump back via `@semantic-release/git`; there is no `mvn deploy`, `distributionManagement`, or Central-publishing plugin anywhere in `pom.xml` or `.github/workflows/release.yml`), so that question is moot until publishing is added. SonarCloud does **not** need `sonar-project.properties` source-path edits once children exist: `sonar-project.properties` already states "Source/test layout is discovered from the Maven reactor" (comment directly above `sonar.exclusions`), and CI invokes the Sonar Maven plugin directly (`./mvnw -B verify sonar:sonar`, in `.github/workflows/ci.yml`) rather than a standalone `sonar-scanner` — the Maven-integrated scanner auto-discovers reactor modules and their source/test directories from the POM structure, so no `sonar.modules`/`sonar.sources` changes are needed. The one thing that may need revisiting is `sonar.coverage.jacoco.xmlReportPaths`, which currently points at a single `target/site/jacoco/jacoco.xml` and would need to point at the aggregated report (per the `jacoco-maven-plugin` `report-aggregate` change above) or list each module's report path. + +### B6: Multi-module reactor / parent pom shape +- Status: Needs Answer +- Kind: Decision +- Description: What should the root pom become — a `pom`-packaged parent/aggregator at the repo root with `filter-core` and `filter-sql-adapter` as `` children (each with its own artifactId), replacing the current single `jar`-packaged root pom? Confirm groupId/artifactId conventions for the new child modules and whether shared build config (dependency versions, plugin config) lives in the parent's ``/`` or is duplicated per module. +- Suggestion: Convert root pom to parent with `filter-core` as child module for general DSL, `filter-sql-adapter` created separately — Current single-jar root pom contains all API/internal/SPI types; no existing multi-module structure — refactor to split core DSL from backend adapter and use parent's dependencyManagement/pluginManagement for shared versions. Limitations: Requires a refactoring move of code from monolith to two modules, which entails migration risk and updating build/tool scripts (Makefile/mvnw) plus potentially adjusting tests. +- Suggestion: Convert root pom to parent with filter-core and filter-sql-adapter as child modules, sharing build config in parent's dependencyManagement/pluginManagement — Root becomes `pom` with ``; `filter-core` retains `org.sequeless.filter:filter-core`, `filter-sql-adapter` gets `org.sequeless.filter:filter-sql-adapter`; parent's dependencyManagement/pluginManagement centralizes common versions (JDK 21, lombok 1.18.36, jackson-databind 2.18.3) and plugin configs (compiler 10+, spotless check), children declare only their explicit dependencies. Limitations: Requires existing single-module code to be reorganized into filter-core module boundary; filter-sql-adapter must be created from scratch as per B14; no current Maven Central publish step minimizes classifier-risk but future OSSRH config would need explicit `excludeClassifiers` for any shared `-tests.jar`; SonarCloud's sonar.coverage.jacoco.xmlReportPaths must aggregate paths across modules; version 1.0.0 will apply uniformly to both artifacts until B7 decides on independent versioning +- Suggestion: Keep single jar root for now; split only later if architecture evolves to add backends/adapters — All functionality is one cohesive filter/query language package per README, and splitting would create unnecessary complexity without current need (no existing backends or adapters that require independence). Limitations: If team adds new concerns (e.g., SQL adapter, multiple DSL variants), they'll face a refactoring decision later; also prevents parallel development on different sub-concerns in this monolithic phase +- Answer: + +### B7: Module versioning strategy +- Status: Needs Answer +- Kind: Decision +- Description: Given semantic-release currently manages a single artifact's version, should filter-core and filter-sql-adapter share one version (released together, e.g. inherited from the parent pom) or be versioned/released independently? +- Suggestion: Share one parent-inherited version and release together until filter-sql-adapter is independently published to Maven Central (or another CI publishes separately) — Per docs/specs/multimodule-archunit-sql-adapter/brief.md B7 context—semantic-release configures single artifact only—.releaserc.json uses @semantic-release/exec + @semantic-release/git touching root pom.xml; current release.yml runs npx semantic-release for one artifact. The root will be a parent pom with filter-core/filter-sql-adapter as children (per B6), and without Maven Central deployment wired in, shared version minimizes complexity while allowing future independent releases if publishing is added. Limitations: This choice assumes no immediate need to publish independently; if filter-sql-adapter must ship at different cadence than filter-core before publishing, this option requires refactoring .releaserc.json or adding multiple release workers (higher operational cost). If filter-sql-adapter introduces breaking API changes that filter-core users don't depend on yet, they'll be delayed until compatible backport; this option also inherits any single-repo GitHub Actions rate limit and branch policy constraints to both artifacts together +- Suggestion: Share one parent-inherited version for both modules until independently, filter-sql-adapter should publish its own release artifacts to Maven Central or have separate CI publishing configured — Current `semantic-release` setup targets one artifact via single-root `pom.xml`; once multi-module exists (per B6), keep them in sync by inheriting the parent Pom's version unless/until both modules can independently build and deploy with their own `.releaserc.json` files, then use a coordinated bump approach. Limitations: Requires filter-sql-adapter to eventually support independent CI deployment; if semantic-release is removed or replaced for multi-module publishing, new config needed per child module; until filter-sql-adapter publishes standalone, version lock remains in parent; no current deploy step means this limitation only applies at future publish time +- Suggestion: Defer versioning strategy until filter-sql-adapter module actually exists — maintain single artifact at this time as described in README's "cohesive filter/query language package" stance; when splitting occurs, use shared parent version then evolve based on actual adoption patterns rather than abstract speculation about Maven Central publishes that don't exist yet. — Single-module project (no `` section in root pom), semantic-release manages one version via `.releaserc.json` and release.yml (no Maven Central publishing step present), multi-module structure doesn't exist per B14's answer confirming filter-sql-adapter module is not implemented; all two accepted suggestions assume the modules will eventually be published to Maven Central, but since no CI publish/deploy is configured yet, deferring avoids premature architectural decisions around versioning mechanics the project doesn't currently need.. Limitations: Assumes project can operate as monolith longer than expected (no immediate SQL adapter integration); if multi-module split must happen urgently for other reasons unrelated to version concerns, this deferral may cause additional rework; team policy on when/how modules actually separate is not codified in docs; de facto "keep single jar" effectively conflicts with any strategy suggesting independent versions or immediate splitting. +- Answer: + +### B8: filter-sql-adapter package naming convention +- Status: Needs Answer +- Kind: Decision +- Description: What package names does filter-sql-adapter use for its own api/spi (e.g. `org.sequeless.filter.sql.api`, `org.sequeless.filter.sql.spi`), consistent with filter-core's existing `org.sequeless.filter.{api,internal,spi}` convention? +- Suggestion: Use `org.sequeless.filter.sql.api` / `org.sequeless.filter.sql.spi` (matching filter-core's `{api,internal,spi}` subdomain layering) — aligns with Maven multi-module adapter patterns where SQL-specific concerns extend the same namespace by adding a subdomain qualifier. — filter-core uses explicit separation of public contracts via `.api` and service provider interfaces via `.spi`; filter-sql-adapter introducing new SPI types (SQLRenderingHook, ConditionDescriptor, etc.) should mirror this layered structure under its own module boundary, analogous to how Spring Boot's JDBC has `com.zaxxer.hikari.config` and H2 uses dedicated `javax.sql.rowset.*` extensions; also B6/B7 in docs/specs/multimodule-archunit-sql-adapter/brief.md propose filter-core (api/internal/spi) and filter-sql-adapter as distinct modules, implying SQL gets its own namespace prefix (`sql`) under shared `org.sequeless.filter` base.. Limitations: The project currently has no multi-module structure or published modules (no Maven Central publishing; .github/workflows/release.yml only sets local version/artifact via semantic-release and commits CHANGES), so this recommendation is aspirational for future implementation — actual package choices can't be validated against real filter-sql-adapter code that doesn't yet exist beyond documentation; if the team chooses B6's deferral strategy (keep single jar until adapting backend demands arise), SQL adapter won't exist yet to demonstrate/package conventions +- Answer: + +### B9: filter-sql-adapter SPI output shape +- Status: Needs Answer +- Kind: Decision +- Description: Should the new SQL adapter SPI produce raw SQL string fragments + a bind-parameter list (JDBC-style `PreparedStatement` usage), a JPA Criteria/`Predicate`-style object graph, or does it need to support both? This shapes the entire SPI contract vendor adapters (and the H2 test fixture) implement against. +- Suggestion: Implement raw SQL fragments + bind-parameter list (JDBC PreparedStatement), matching how filters already carry fully-resolved values rather than parameter placeholders — The FilterNode AST uses JsonNode value nodes (not placeholder markers), the project is explicitly JPA-agnostic per README/pom, so binding-style output fits best without introducing Framework dependencies; field-path-to-column mapping would naturally occur in an adapter subclass that knows domain entities/columns. Limitations: Assumes no immediate need for Criteria-builder style queries which could be added later as a separate SPI variant if/when JPA integration is pursued; JDBC PreparedStatement usage requires callers to manually execute statements with prepared/prepared parameters, which may require documentation/example code for teams new to this pattern +- Suggestion: Return SQL string fragments + JDBC `PreparedStatement` value placeholders matching how `FilterNode` AST carries fully-resolved `JsonNode` values directly — The `FilterNode` AST (FieldFilter/AnyFilter) carries explicit `JsonNode value` fields per code read, making PreparedStatement the natural fit; no JPA dependencies exist per pom.xml showing only junit/jackson/antlr/archunit/mockito/assertj, and a non-JPA project fundamentally cannot support Criteria/Predicate-style graphs which are JPA-specific.. Limitations: This assumes target databases all support named/bind parameters (nearly all JDBC drivers do) but omits scenarios where raw dynamic SQL interpolation is required for derived expressions; vendor adapters must implement their own column/path-to-SQL resolution since the SPI receives AST nodes with field paths but no mapping contract per B10 question. +- Suggestion: Return fully-resolved Condition descriptors (path string, canonical op name, type-safe boxed value) as an immutable AST-like interface optimized for SQL emission to a `visit*` pattern — The FilterNode tree carries path strings from FieldDefinition's schema metadata plus OperatorDefinition.canonicalName and fully resolved JsonNode values - this allows vendors to walk it with their own visitor that produces either raw fragments, placeholder lists, or typed object graphs without repeating SPI contract negotiation. Limitations: Vendors must implement a custom FilterVisitor implementation for SQL translation which adds implementation complexity; the API only exposes string paths and boxed Java types so adapters cannot infer specific DB column type precision beyond what is defined in FieldDefinition.jsonSchemaType +- Answer: + +### B10: filter-sql-adapter field-path-to-column mapping +- Status: Needs Answer +- Kind: Decision +- Description: Does the SQL adapter SPI itself define a contract for mapping a `FilterNode`'s field paths to actual SQL column names/expressions (e.g. a `ColumnMapping` SPI type), or is that mapping assumed to happen entirely outside the adapter, with the adapter only ever receiving already-resolved column references? +- Suggestion: The filter-sql-adapter SPI leaves field-path-to-column mapping entirely outside its contract—assumed to happen externally—so the adapter receives FilterNode ASTs already resolved by whatever caller knows entity/table schemas — FieldFilter/AnyFilter stores `path` as plain String in their api records without binding metadata; B10's own context from B9 suggests vendor adapters implement their own column/path resolution since SPI only exposes path strings and no ColumnMapping contract exists or is being defined. Limitations: Assumes callers can resolve paths-to-columns (e.g., JPA metamodels, table schemas) if/when SQL backend integration is added; if JPA/Criteria-style query APIs are later introduced, mapping semantics would need revisiting around that new variant +- Suggestion: SQL adapter module defines a `ColumnMapping` SPI contract where adapters declare path-to-column mappings during construction, and adapters receive FilterNodes with fully-resolved column references — The current single-module project has no multi-module structure (confirmed by pom.xml packaging=jar with no modules), FieldFilter carries string paths not SQL column refs (code read confirms path is String in record), mapping must happen at adapter initialization time when domain entities are available, allowing adapters to register path->column mappings from their persistence models. Limitations: Requires filter-sql-adapter module to exist for SPI contract acceptance; vendors would need to implement custom visitor pattern for SQL generation as the core only exposes string paths and boxed values without schema inference beyond FieldDefinition.jsonSchemaType +- Answer: + +### B11: filter-sql-adapter dependency on filter-core's spi package +- Status: Needs Answer +- Kind: Decision +- Description: Does filter-sql-adapter need to depend on filter-core's `spi` package (e.g. `FieldContributor`/`OperatorContributor`) in addition to `api`, or does it only ever consume already-built `FilterNode`/`OperatorDefinition` instances from `api`? +- Suggestion: filter-sql-adapter depends *only* on filter-core's `api` package, never on spi — FilterNode/OperatorDefinition instances in api are concrete value objects populated directly by parser/builders and stored as Definition references (not contributor instances) when exposed; FieldRegistry only exposes already-built FieldDefinition lists via its static factory methods; default registries (DefaultOperatorRegistry, DefaultFieldRegistry) assemble definitions from contributors' resolved output before making available via their public APIs. Limitations: This answer assumes filter-sql-adapter follows the design outlined in docs/specs/multimodule-archunit-sql-adapter/brief.md (FilterNode AST translation only) and does not require contributing new operators/fields at runtime; verification requires implementing a minimal sql adapter stub and confirming no imports of org.sequeless.filter.spi.* leak into filter-sql-adapter's main scope +- Suggestion: filter-sql-adapter depends only on filter-core's api package, never on spi — The api package declares only api and internal (not spi) imports; OperatorDefinition/FieldDefinition are value types consumed to build ASTs — spi is solely for registry building via extension points at framework composition time. Limitations: Risk if future API changes introduce new SPI-only features; needs version pinning during filter-core updates; assumes contributor-based configuration pattern remains stable +- Suggestion: filter-sql-adapter depends only on filter-core's api package, never on spi — The brief explicitly states filter-sql-adapter "defines its own API and SPI for translating parsed filters into SQL" that takes a FilterNode from parser/filter-core and produces SQL — the api package already exports built FilterNode/OperatorDefinition instances via OperatorRegistry/FieldRegistry; spi interfaces (OperatorContributor/FieldContributor) are registry extension points, not runtime data objects consumed by adapters.. Limitations: This reasoning assumes the brief's multi-module refactoring plan is implemented as described (filter-core as parent with api/internal/spi packages). The answer also holds even before restructure because FilterNode and OperatorDefinition classes have never imported from org.sequeless.filter.spi in their Javadoc or code comments; spi interfaces are design/registration hooks, not runtime dependencies for end-to-end adapter translation pipelines. +- Answer: + +### B12: Reuse vs duplicate in-memory field-registry fixtures for filter-sql-adapter tests +- Status: Needs Answer +- Kind: Decision +- Description: Should filter-sql-adapter's own tests (including the H2 fixture's tests) reuse filter-core's test-only `FieldRegistry` fixture (per B2's findings on how that could be shared), or should filter-sql-adapter define its own separate in-memory field registry for test purposes? +- Suggestion: filter-sql-adapter should define its own in-memory FieldRegistry fixture (no reusing from filter-core suggested) — sequeless-filter tests locally instantiate FieldRegistry instances per test scenario; no filter-sql-adapter module exists in this workspace. Limitations: this workspace lacks a 'filter-sql-adapter' module - only pure java Filter library + Spring starter which depends ON it not vice versa +- Answer: + +### B13: FieldRegistry interface contract reconsideration +- Status: Needs Answer +- Kind: Decision +- Description: With `permissive()`/`PermissiveFieldRegistry` moving to test-only and no production `FieldRegistry` implementation shipping a permissive mode, should `FieldRegistry.isPermissive()` be removed from the interface, kept as-is for implementors outside this codebase (e.g. a vendor-supplied registry) to still express, or changed in some other way? Identify any other `FieldRegistry` contract members that only make sense in light of the removed default implementations. +- Suggestion: Change `isPermissive()` to a package-private default in the public interface or convert all internal implementations and remove it from API entirely — The method only serves as an implementation hint for tests and external vendors don't need this contract since DefaultFieldRegistry fully implements find()/all()/compatibleWith() per their actual behavior; moving `isPermissive` out of its default position would expose API consumers to breaking changes if they previously inherited from internal implementations, but keeping it reveals no real purpose for public consumers who create registry via static factories. Limitations: External vendors implementing FieldRegistry won't break by omitting isPermissive() since the interface uses a default method; however, any existing production code relying on default-interface semantics to differentiate registry types will lose that ability without explicit overrides +- Suggestion: Make `isPermissive()` package-private (remove from public API) while keeping its implementation behavior in internal classes, then remove all callers or replace with path-based validation check — The current codebase uses `isPermissive()` defensively in two internal places (`FilterBuildingVisitor.resolvePath`, `FilterValidator.validateField`) to skip field-path validation when the registry is permissive; both callers are internal and can be simplified to always-validate since only `DefaultFieldRegistry` ships for production. With PermissiveFieldRegistry/test-only removed from the shipped product and no vendor-supplied FieldRegistry implementations documented or expected, exposing a boolean marker that only ever differs in test code serves no purpose—converting it to package-private (`private` in internal packages) or removing it entirely prevents future implementors outside this module from being tripped up by an unrelated flag. Callers should validate against `find(path).isEmpty()` unconditionally or use a path wildcard mechanism if desired, rather than relying on the method's existence. Limitations: This suggestion changes interface stability for external implementors who may depend on isPermissive() as part of a contract; removing it (or making it non-public) breaks binary compatibility but improves conceptual clarity. Replacing callers with unconditional validation increases risk of spurious "Unknown field" errors if vendors later add permissive behavior, unless they adapt by using the wildcard mechanism or wrapping validation logic themselves +- Suggestion: Remove `isPermissive()` as a public interface method and rely solely on path-based validation logic — Only two internal implementations exist (both in org.sequeless.filter.internal package); the permissive() static factory returns PermissiveFieldRegistry which always allows any path, so validation should be gate-checked against using either FieldRegistry.permissive() or the of() factory rather than calling isPermissive(). Limitations: Requires updating FilterBuildingVisitor and FilterValidator to explicitly pass a boolean parameter (permissive flag) instead of querying the registry; external implementations would need to accept this permissive flag via constructor/injectable property; slightly more verbose API for callers who previously relied on default method +- Answer: + +### B14: Implement filter-sql-adapter module +- Status: Resolved +- Kind: Fact +- Description: Implement filter-sql-adapter module (raised while researching B3 — H2 test-fixture dependency confirmation requires the filter-sql-adapter module to actually exist first). +- Answer: The filter-sql-adapter module does not exist yet — no `filter-sql-adapter` directory, package, or module anywhere in the repo, and the root `pom.xml` has `jar` with no `` section — so B3's H2 test-fixture work remains blocked pending this module's creation. (This brief's Register uses only "Resolved" and "Needs Answer" statuses; there is no "Needs Research" status value in this document to confirm against.) diff --git a/docs/specs/multimodule-archunit-sql-adapter/plan.md b/docs/specs/multimodule-archunit-sql-adapter/plan.md new file mode 100644 index 0000000..b780cd6 --- /dev/null +++ b/docs/specs/multimodule-archunit-sql-adapter/plan.md @@ -0,0 +1,766 @@ +# Plan — Multi-module restructure, ArchUnit guards, filter-sql-adapter + +## Goal + +Turn the single-module `org.sequeless:sequeless-filter` jar into a Maven reactor with two +modules — `filter-core` (the existing DSL: AST, ANTLR grammar/parser, serializer, registries, +completion) and a new `filter-sql-adapter` (FilterNode AST → SQL), with ArchUnit guarding the +package/module boundaries that remain enforceable. (Neither module is actually published today — +no `` or deploy step exists anywhere in this repo's tooling; that's a +pre-existing gap this plan doesn't attempt to close.) **Note on scope of "ArchUnit-guarded":** two +of the rules originally envisioned (an api→internal facade rule in filter-core, and a +"testfixtures never leak to main" rule) were dropped during review because Maven's own compilation +model already makes both invariants un-violatable — see D8/D15. What ArchUnit actually enforces +after this plan: filter-core's existing `BoundaryRulesTest` rules (`filter_is_free_of_spring`, +`api_and_spi_types_are_public`) survive untouched; filter-sql-adapter's new ArchUnit test adds a +module-local public-types rule, a cross-module rule that `filter-sql-adapter`'s main scope may not +reach filter-core's `internal`/`spi`, and — kept for documentation purposes despite being +unfalsifiable given Maven already rejects the cycle — a rule that filter-core may not depend on +`sql..` (D26). That is a smaller guarantee than "layering is fully policed," and is accurate to +state as such. + +Along the way, filter-core's field-registry *defaults* stop being production code: they become +test-scope fixtures, and `FieldRegistry`'s contract is trimmed to what production actually needs — +except `of(...)`, which Stage 3 decided to keep (see D5). Separately, `OperatorDefinition` gains a +value-shape signal and `FilterValidator` gains a value-shape check (D17) — a small, independently +motivated improvement to filter-core surfaced by reviewing the SQL adapter's requirements, not +something filter-sql-adapter depends on filter-core changing for it (the adapter still degrades +gracefully via its own `SqlRenderException` if a shape mismatch somehow reaches it). + +## Settled decisions + +These were `Needs Answer` in the brief's Register; each was verified against the codebase and is +now folded into the plan body below. Deviations from the brief's first-ranked suggestion are +marked and justified. **Several of these were revised during Stage 3 (user interview) and Stage 4 +(two rounds of automated review) — see `## Decisions` below for the current, authoritative +version; rows here are kept as the historical Stage 2 record.** + +| Ref | Decision | +| --- | --- | +| B6 | Root `pom.xml` becomes `pom` aggregator/parent (`org.sequeless:sequeless-filter`), with `filter-core` and `filter-sql-adapter` as `` children. groupId stays `org.sequeless`; artifactIds are `filter-core` / `filter-sql-adapter` matching their directory names. Shared versions/plugin config live in the parent's `` / `` / ``, never duplicated per module — **refined by D9**: most plugins stay in the parent's inherited ``, only `flatten` moves to ``. **Deviation:** the suggestion proposed groupId `org.sequeless.filter`; keeping `org.sequeless` avoids inventing a second group for a project that already ships under `org.sequeless`. | +| B7 | One shared, parent-inherited version. Children declare no `` of their own and reference each other with `${project.version}`. Verified: `.releaserc.json` drives a single `versions:set` and no `mvn deploy`/`distributionManagement` exists anywhere, so per-module release cadence buys nothing today. | +| B8 | filter-sql-adapter uses `org.sequeless.filter.sql.api`, `org.sequeless.filter.sql.spi`, `org.sequeless.filter.sql.internal` — the same `{api,internal,spi}` layering filter-core already uses, one subdomain down. **Refined by D12**: the shipped default `QueryBuilder` implementation lives in `sql.spi` (public, extensible), not `sql.internal`; `sql.internal` currently has no classes reserved for it. | +| B9 | The SPI produces **raw SQL fragments plus an ordered bind-parameter list** (JDBC `PreparedStatement` style). Verified: `FieldFilter`/`AnyFilter` carry fully-resolved `JsonNode` values (not placeholders), and the enforcer `ban-spring` rule plus the "no storage/query engine dependency" stance in README rule out a JPA Criteria object graph. Reaffirmed directly by **D2**. **Superseded mechanism** — see D3/D6: the original plan had a single `SqlRenderer` walk the tree with a `SqlDialect` override hook; this is now a hexagonal `QueryBuilder` port (`binary`/`unary`/`and`/`or`, each receiving `OperatorDefinition`) that owns full construction. | +| B10 | The adapter **does** own a path-to-column contract: a `ColumnMapping` functional interface in `org.sequeless.filter.sql.api`, supplied by the caller at render time, with a `ColumnMapping.identity()` for callers whose paths already equal column names. **Refined by D13**: `identity()` is documented as trusted-input-only (no runtime validation added) — callers exposing untrusted paths must supply an explicit `of(Map)` allowlist instead. **Scope limit surfaced by D20**: only flat, single-table column mappings are supported — there is no way to contribute a join, correlated subquery, or JSON path expression. | +| B11 | filter-sql-adapter main scope depends on filter-core's `api` package only — never `spi`, never `internal`. Unaffected by Stage 3/4; still holds under D4 (AnyFilter expansion uses `AnyFilterExpander`, itself in `api`). | +| B12 | **Superseded by D5.** The original finding (filter-sql-adapter defines its own test-only `FieldRegistry` fixture rather than sharing filter-core's) is moot: D5 reinstates `FieldRegistry.of(...)` as production API, so there is no test-only fixture to share or duplicate in the first place. | +| B2 | **Superseded by D5**, for the same reason as B12 — no test-scope fixture-sharing question remains once `FieldRegistry.of(...)` is production API again. | +| B13 | **Partially reversed by D5.** The behavior-preservation finding for removing `isPermissive()` still holds (its only two callers collapse to `fields.find(path).isEmpty()` unconditionally), and `permissive()`/`PermissiveFieldRegistry` still move to test-only. What changed: `of(List)` does **not** go away — it's kept, retyped to take `FieldRegistrySpec` (D5). | + +## Decisions + +Answers reached by interviewing the user in Stage 3 and via two rounds of Stage 4 automated +review, in the order resolved. These are the authoritative, current design — the +`## Settled decisions` table above is historical. + +### Stage 3 (user interview) + +- **D1 — Commit message convention: no breaking-change markers.** Removing + `FieldRegistry.isPermissive()` is technically a breaking API change, but **no commit produced + while implementing this plan should be marked as breaking** (no `!` after the conventional-commit + type, no `BREAKING CHANGE:` footer) — use a plain `fix:` (or otherwise appropriate type) instead, + for this and every other commit in the plan. Reconsidered and reaffirmed at D10 after the review + flagged the semver implications. +- **D2 — SQL SPI output stays raw SQL, confirmed.** Re-affirms B9's conclusion (`SqlFragment`, not + JPA Criteria) after being asked directly; only the internal mechanism changed (D3/D6). +- **D3 — Hexagonal query-builder architecture replaces the renderer+dialect-override design.** + `sql.spi.QueryBuilder` is now the **port**: vendor adapters implement it and own construction of + each `SqlFragment` — identifier quoting, operator-to-SQL translation, value expansion, AND/OR + combination — rather than a shared renderer that only exposes override hooks. `sql.api`'s + translator is the vendor-agnostic core: it walks the `FilterNode` tree and delegates every node to + the injected `QueryBuilder`. The port stays **fixed to producing `SqlFragment`** (not a generic + `QueryBuilder`) — considered and rejected, since full genericity would reopen the + already-settled JPA-Criteria question (B9/D2) and would mean filter-sql-adapter's own H2 fixture + and ArchUnit guards could no longer assume one concrete output shape. This supersedes the Stage 2 + plan's `SqlRenderer`/`SqlDialect` design entirely; the exact port signature is D6. +- **D4 — The SQL adapter expands `AnyFilter` itself.** Reverses the Stage 2 plan's stance (which + rejected unexpanded `AnyFilter` and required callers to pre-expand). The translator now depends on + filter-core's `AnyFilterExpander` (api) and expands any `AnyFilter` node before delegating to the + `QueryBuilder`, using `AnyFilterExpander`'s **default** expansion strategy. This means the + translator's entry point needs a `FieldRegistry` and `OperatorRegistry` in addition to + `ColumnMapping` and `QueryBuilder` — which motivates D5. **Caveat:** callers who need a *custom* + `AnyExpansionStrategy` (a real, documented feature of `AnyFilterExpander`) have no way to supply + one through the adapter's entry point — they must still pre-expand with their own strategy before + calling `translate(...)` if they need anything other than the default. **Refined by D18**: an + operand of the expansion that has no `ColumnMapping` entry is dropped rather than failing the + whole filter — see D18. +- **D5 — `FieldRegistry.of(...)` reinstated in filter-core main scope, parameterized by a new + builder-constructed spec type, not a raw `List`.** Partially reverses B13/T1 (which removed + `FieldRegistry.of(List)` entirely and moved `DefaultFieldRegistry` to test-only). + `DefaultFieldRegistry` stays in filter-core's main scope (not moved to test), backing a reinstated + `FieldRegistry.of(FieldRegistrySpec)` static factory. The parameter type changes from a raw + `List` to a new `FieldRegistrySpec` value type, built via Lombok `@Builder` + (holding `List fields` today), so the registry can be configured by more than a + flat field list later without another signature break. `DefaultFieldRegistry` itself does **not** + need its own builder — only its constructor's parameter type changes, from `List` + to `FieldRegistrySpec`. `PermissiveFieldRegistry`/`permissive()`/`isPermissive()` still move to + test-only / get removed exactly as B13 already settled ("keep the permissive one out unless we + discover later that we'll need it," per the user — main scope, that is). This eliminates the + Stage 2 plan's `testfixtures.InMemoryFieldRegistry` fixture entirely: tests construct a + `FieldRegistry` the same way any production caller now does, via + `FieldRegistry.of(FieldRegistrySpec.builder().fields(...).build())`. Only + `testfixtures.PermissiveFieldRegistry` remains as a genuine test-only fixture. **Supersedes B2 and + B12**: with the default registry back in production, there is no test-only fixture left to share + or duplicate across modules. + +### Stage 4, first review pass + +- **D6 — `QueryBuilder`'s methods take `OperatorDefinition`, not a bare canonical-name string.** + `SqlFragment binary(String column, OperatorDefinition operator, Object value)`, + `SqlFragment unary(String column, OperatorDefinition operator)`, + `SqlFragment and(List operands)`, `SqlFragment or(List operands)`. Lets + a vendor branch on `operator.getSyntax()`/`isUnary()`/`getParameters()` without re-deriving them + from a string, and — as a side effect, not a scope expansion — structurally accommodates a future + `Syntax.FUNCTION` operator (same `(column, operator, value)` shape; `value` would be the + argument list) without a fifth port method. `BuiltinOperatorContributor` ships no `FUNCTION` + operator today, so the translator/builder tasks implement only the 13 shipped `INFIX` operators; + this is forward compatibility, not a commitment to build FUNCTION support now. `value` here is the + **already-converted** Java value — see D7. **Refined by D16**: `binary()`/`unary()` are `final` + dispatchers on the shipped `AnsiQueryBuilder`, which needs its own fallback path for operators + outside the 13 built-ins — see D16. +- **D7 — The translator (not the vendor `QueryBuilder`) converts `JsonNode` values to plain Java + objects before calling the port.** One documented mapping, applied uniformly so every vendor gets + it for free: `TextNode`→`String`; numeric (`IntNode`/`LongNode`/`DoubleNode`, matching + `FilterBuildingVisitor.parseNumber`'s int/long/double selection)→the corresponding boxed + `Number`; `BooleanNode`→`Boolean`; `NullNode`→Java `null`; `ArrayNode`→`List`, each + element converted recursively (the grammar allows nested arrays). **Explicit non-goal:** no + temporal coercion — a `jsonSchemaFormat: "date-time"` field's value arrives as a plain `String` + (the grammar has no date literal); binding it against a `TIMESTAMP` column is left to the vendor's + JDBC driver and is a known limitation (carried into Caveats). **Extended by D19**: this table + doesn't cover every `JsonNode` subtype a deserialized (not just parsed) `FilterNode` can carry — + D19 adds an explicit catch-all. +- **D8 — No api→internal ArchUnit rule in filter-core.** Dropped entirely rather than fixed. The + incomplete version in the Stage 2 plan named only two exemptions + (`DefaultOperatorRegistry`/`DefaultFieldRegistry`) but missed `BuiltinOperatorContributor` (via + `OperatorRegistry.defaults()`) and `FilterParser`'s reach into `CursorAnalyzer`, + `FilterBuildingVisitor`, and the generated `internal.parser` package. filter-core's `api` is + deliberately a facade over `internal`; policing that edge would mean editing an exemption list on + every future facade method. The invariant that actually matters — nothing *outside* filter-core + depending on its `internal` — is covered by the cross-module ArchUnit rule instead. +- **D9 — Only `flatten-maven-plugin` moves to ``; everything else stays in the + parent's inherited ``.** `compiler`, `antlr4`, `spotless`, `jacoco`, `surefire`, + `failsafe`, `enforcer` stay exactly where they are today (Maven inherits their executions to + children automatically); only `flatten` — the one plugin the B5 caveat already identified as + needing per-module control — moves to ``. **Corrected by D24**: no `` flag + belongs in the managed entry itself — see D24 for the actual mechanics. +- **D10 — Reaffirms D1: proceed with `fix:`-only commits, no breaking markers, despite shipping 4 + breaking API changes as a nominal patch release.** Confirmed acceptable specifically because no + `` or deploy step exists anywhere in this repo's tooling — there is no + real published artifact and thus no actual external consumer to break today. +- **D11 — The translator explicitly rejects two cases `AnyFilterExpander` doesn't itself guard + against.** `AnyFilterExpander.expand` silently returns an empty `OrFilter`/`AndFilter` when zero + fields are compatible (not an exception, contrary to what an earlier draft of this plan claimed); + the translator now detects a zero-operand expansion result and raises `SqlRenderException`. + Separately, `is in []` (an empty array) is legal DSL but `IN ()` is a syntax error in every SQL + dialect; the translator raises `SqlRenderException` for an empty converted value list on `is in` + as well. **Generalized by D22**: the empty-operand rejection applies to *any* zero-operand + `AndFilter`/`OrFilter` the translator encounters, not only ones produced by `any`-expansion. +- **D12 — The default `QueryBuilder` implementation is public, extensible, and structured as a + Template Method — but `QueryBuilder` itself has no static factory pointing at it.** Refined twice + after the initial finding, in order: + 1. *Public + extensible, in `sql.spi` not `sql.internal`.* Target: `sql.spi.AnsiQueryBuilder`, a + non-`final` public class implementing `QueryBuilder`. Reasoning: D3 explicitly rejected "a + shared renderer that only exposes override hooks," but the initial idea had the H2 fixture + override one method of an *internal* class — only possible because the fixture shares a module + with `sql.internal`. A real third-party vendor can't reach `sql.internal` at all and would have + had to reimplement the entire port to change one method. + 2. *Template Method, not a single override-the-whole-method class.* Because all 13 built-in + operators funnel through one `binary()`/`unary()` call, "extend and override one method" was + still not cheap partial customization — overriding `binary()` alone means reimplementing the + dispatch for every other operator too. `AnsiQueryBuilder`'s `binary()`/`unary()` are `final` + dispatchers that delegate to `protected` per-concern hooks (e.g. `quoteIdentifier(String)`, + `renderIs(...)`, `renderLike(...)`, `renderIn(...)`) — a vendor overriding, say, only `LIKE` + escaping touches one small hook, not a 13-way switch. + 3. *No `QueryBuilder.ansi()` static factory.* Considered keeping it (mirroring + `OperatorRegistry.defaults()`), but concluded the factory and vendor extensibility are + orthogonal: a vendor writes `class PostgresQueryBuilder extends AnsiQueryBuilder` and never + calls `ansi()` at all — extensibility comes entirely from (1)+(2). The factory only helped a + *non-customizing* caller reach the plain default one line shorter (`QueryBuilder.ansi()` vs. + `new AnsiQueryBuilder()`), which isn't worth the port interface carrying a compile-time + reference to one specific concrete adapter. `QueryBuilder` stays a pure port; callers and + vendors instantiate `AnsiQueryBuilder` (or a subclass) directly. + + **Extended by D16 and D23** (second review pass): D16 adds a fallback hook for operators outside + the 13 built-ins; D23 mandates a specific escaping contract for the default `quoteIdentifier` + hook. +- **D13 — `ColumnMapping.identity()` is documented as trusted-input-only; no runtime validation is + added.** The translator accepts arbitrary `FilterNode`s (including ones built from untrusted or + deserialized input), and `identity()` places the resolved path directly in SQL identifier position + with no check independent of whatever quoting a vendor's `QueryBuilder` applies. Rather than adding + a validation pass to the shared core, this is called out explicitly in `identity()`'s Javadoc: + intended for paths the caller already controls, not for exposing arbitrary input as column names. + Callers with untrusted paths must use `ColumnMapping.of(Map)` (an explicit allowlist) instead. This + same "deserialized/untrusted `FilterNode` is in-scope input" framing is what motivates D19's + catch-all requirement for value conversion, and D23's requirement that the *shipped* default + quoting still be technically sound even though `identity()` itself isn't validated. +- **D14 — `LIKE` escaping for `contains`/`starts with`/`is like`/`is not like` is left entirely to + each `QueryBuilder` implementation's discretion; the plan does not mandate specific escaping + semantics.** Consistent with D3's "vendor owns 100% of construction" — accepted with the explicit + understanding that escaping behavior (and thus exact matching semantics for values containing `%` + or `_`) may differ across vendor adapters, including between the shipped `AnsiQueryBuilder` and any + third-party one, and no cross-vendor test can assert one specific outcome. This is intentional, not + an oversight. (This is distinct from D23's `quoteIdentifier` mandate: `LIKE` escaping is a + matching-semantics question left open by design; identifier quoting is an injection boundary in + the one implementation this repo actually ships, so it isn't.) +- **D15 — The `testfixtures` "fixture-leak" ArchUnit rule is dropped entirely, not just its + "confirm it fails" verification step.** It can never trip: Maven's compilation model already makes + a main-scope class depending on a test-scope class impossible, and filter-sql-adapter has no + test-jar dependency on filter-core's tests (D5 eliminated that mechanism), so the rule would guard + against something that literally cannot happen through any build-time path. + +### Stage 4, second review pass + +- **D16 — `AnsiQueryBuilder` gets a fallback hook for operators outside the 13 built-ins, rather + than leaving `binary()`/`unary()` as closed `final` dispatchers.** `OperatorRegistry.of(List)` + is public API and custom operators are a real, already-tested extension point + (`FunctionArgValidationTest` exercises one) — without a fallback, a vendor registering a custom + operator hits a `final` method it cannot extend and must reimplement `QueryBuilder` from scratch, + recreating the exact gap D12 was raised to close, one level down. `AnsiQueryBuilder` adds + `protected SqlFragment renderUnknown(String column, OperatorDefinition operator, Object value)` + and a unary twin, both throwing `SqlRenderException` by default and overridable by a vendor that + wants to support the custom operator. The full `protected` hook set (`quoteIdentifier`, `renderIs`, + `renderLike`, `renderIn`, `renderUnknown`, plus whatever else the 13-operator implementation needs) + must be enumerated explicitly when the contracts are defined, not left as "etc." — subclass-visible + members are as much a compatibility commitment as public ones. +- **D17 — Value-shape validation moves into filter-core's `FilterValidator`, not the SQL adapter.** + Nothing today checks that a value's shape matches what its operator expects: the grammar accepts + `status is in 'open'` (scalar into a list-shaped operator) or `status is ['a','b']` (list into a + scalar operator) equally, and either would reach a vendor `QueryBuilder` as the wrong Java shape. + Rather than fixing this only for the SQL adapter, the check is added to the existing, shared + `FilterValidator` so every consumer benefits. This requires giving `OperatorDefinition` a + value-shape signal it doesn't have today (e.g. distinguishing `is in`, list-shaped, from the other + 12 built-ins, which are scalar) — a small, independently-motivated filter-core change, not + something filter-sql-adapter depends on to function (a shape mismatch that somehow still reaches + the adapter is still caught there as an ordinary `SqlRenderException`, just later than ideal). This + is a new task in Phase 1 alongside T1, not a change to filter-sql-adapter's tasks. +- **D18 — Partial `ColumnMapping` coverage during `any` expansion drops the unmapped operands + instead of failing the whole filter.** `AnyFilterExpander` expands `any` to one `FieldFilter` per + *registry*-compatible field, but `FieldRegistry` (domain vocabulary) and `ColumnMapping` + (persistence vocabulary — especially once a caller follows D13's advice to use an explicit + allowlist) are independently populated and can drift out of sync. Under the original "empty result + ⇒ reject" rule (D11), a single unmapped registry field breaks `any` entirely — likely on the first + realistic use. The translator now silently drops any expanded operand whose path has no + `ColumnMapping` entry, and only raises `SqlRenderException` if *none* of the expanded operands + survive (folding into D11's existing empty-expansion check). An explicitly-named path (not reached + through `any`) is unaffected — it still hard-fails on an unmapped column, exactly as before. +- **D19 — Value conversion (D7) gets an explicit catch-all for `JsonNode` subtypes outside the + original five-case table.** `FilterNode` is fully Jackson-deserializable + (`@JsonTypeInfo`/`@JsonSubTypes`), and D13 already treats deserialized/untrusted ASTs as in-scope + input to `translate(FilterNode …)` — but a JSON integer past `Long.MAX_VALUE` deserializes to + `BigIntegerNode` and an object-valued field to `ObjectNode`, neither handled by D7's table as + originally written. The translator now additionally: converts any other numeric node via + `isNumber()`/`numberValue()`; raises `SqlRenderException` for anything else unrecognized + (`ObjectNode`, `BinaryNode`, `POJONode`, `MissingNode`, etc.) rather than guessing. +- **D20 — The single-table, flat-column limitation is stated as an explicit caveat, not silently + discovered by the first real user.** `ColumnMapping` returns a bare column name and `SqlFragment` + is a single `WHERE`-fragment plus binds — there's no way to express a join, a correlated `EXISTS`, + or a JSON path expression, yet the DSL's own headline example is a nested path + (`lineItems.qty > 5`, per `FieldFilter`'s Javadoc and both `README.md` and `docs/filter-dsl.md`) + that on a relational schema typically means a join, not a column. No design change — this is + purely calling out a real limitation explicitly (see Caveats and `docs/sql-adapter.md`, added to + the docs task) rather than widening scope to cover it in this plan. +- **D21 — `SqlFragment`'s bind-parameter list is defensively copied in a way that tolerates `null` + elements.** Every existing collection-carrying type in this codebase (`AndFilter`, `OrFilter`, + `OperatorDefinition`'s builder) defensively copies with `List.copyOf`, which throws + `NullPointerException` on a `null` element — but D7 legitimately produces `null` (from + `NullNode`), and `is in ['a', null]` is grammatically valid, landing a `null` in the parameter + list. `SqlFragment`'s compact constructor uses + `Collections.unmodifiableList(new ArrayList<>(parameters))` instead of `List.copyOf`, deliberately + deviating from the house idiom for this one type since the idiom would silently turn a legal + filter into a construction-time `NullPointerException`. +- **D22 — Generalizes D11: the translator rejects *any* zero-operand `AndFilter`/`OrFilter` it + encounters, not only ones produced by `any`-expansion.** `AndFilter`/`OrFilter` null-check but + don't size-check their operand lists, so both `Filters.and()` and a deserialized + `{"type":"and","operands":[]}` construct without error — and since `AnyFilterExpander.expand` + recurses, a zero-compatible-field `any` nested inside a larger `and`/`or` produces an empty + operand *inside* the tree, not necessarily at the root. Any such node reaching the port would + render as `()`, a syntax error in every dialect; the translator now checks every `AndFilter`/ + `OrFilter` node it visits, at any depth, not just the top-level result of an expansion. +- **D23 — The shipped `AnsiQueryBuilder.quoteIdentifier` hook has a mandated escaping contract: emit + `"…"` with any embedded `"` doubled.** D13 accepted `ColumnMapping.identity()` shipping with no + runtime validation specifically on the reasoning that quoting is the vendor `QueryBuilder`'s + responsibility — but that only holds if the one `QueryBuilder` this repo actually ships has a + correct quoting implementation to point to. Parser-produced paths are already safe + (`WORD : [a-zA-Z_][a-zA-Z_0-9]*`); the exposure is entirely from programmatic/deserialized ASTs, + which D13 already treats as in-scope. `quoteIdentifier` is `protected` and overridable (D12.2), so + its Javadoc must also say overriding it is security-relevant, not just a formatting choice. +- **D24 — Corrects D9's mechanics: no `` flag belongs on `flatten` in the parent's + ``.** `` configuration is inherited by every child that + *declares* the plugin — putting `true` inside the managed entry would make every + child that re-declares `flatten` inherit the skip too, silently disabling flattening everywhere + (nothing would surface the mistake immediately, since nothing consumes `.flattened-pom.xml` + today). The actual mechanism: `flatten` goes into `` with **no** `` + element at all; the aggregator simply never declares/binds it in its own `` + (an unbound plugin doesn't run — no skip flag needed); each child declares it normally in its own + `` and gets the managed config. +- **D25 — Before touching Sonar configuration, verify whether `sonar-project.properties` is read at + all by the scanner CI actually invokes.** CI runs `./mvnw -B verify sonar:sonar` — the + Maven-integrated Sonar scanner, which is documented to source its configuration from the POM and + `-D` flags, not from a standalone `sonar-project.properties` file (that file is a SonarScanner CLI + artifact). If true, the B5 coverage-path caveat's whole premise is moot, and coverage may already + be mis-reported today for reasons unrelated to this restructure. Sequencing: check empirically + first (a CI run / Sonar dashboard check for non-zero current coverage) before making changes; only + if confirmed, move `sonar.projectKey`/`sonar.organization`/exclusions/`sonar.coverage.jacoco.xmlReportPaths` + into the parent POM's `` as part of the reactor-tooling task, and retire + `sonar-project.properties`. Do not fix speculatively. +- **D26 — Keeps the "no filter-core class may depend on `sql..`" cross-module ArchUnit rule despite + it being technically unfalsifiable by the same argument that removed D15's rule.** A + filter-core→filter-sql-adapter dependency is a reactor build-order cycle Maven rejects outright, + so — like the deleted fixture-leak rule — this one "can never trip through any build-time path" + either. Kept anyway, and the inconsistency with D15 is deliberate, not an oversight: it costs two + lines, documents the intended dependency direction at the module boundary for a human reader, and + would start actually mattering the moment a third module or an unexpected dependency edge appears. + D15's rule had no equivalent documentation value once removed (it was guarding an accident, not a + direction). + +### Stage 4, third review pass + +- **D27 — `FilterQueryTranslator.translate(...)` validates its input itself, calling + `FilterValidator` before translating.** Neither `FilterParser.parse` nor anything else in this + plan actually invokes `FilterValidator` — validation is entirely opt-in today — so D17's new + value-shape check never protected the SQL path as D17/the Goal originally claimed: a shape + mismatch would have reached a vendor `QueryBuilder` as a raw `ClassCastException` or, past that, + a driver-level `SQLException`, not the clean `SqlRenderException` the plan asserted. `translate(...)` + already receives both `FieldRegistry` and `OperatorRegistry` — the two things `FilterValidator` + needs — so calling it costs no new parameters. Any violation `FilterValidator` reports is + converted to `SqlRenderException`. Accepted cost: one extra validation pass per `translate(...)` + call, and the adapter is now coupled to validator semantics it doesn't own — judged worth it so + the adapter is genuinely safe to call standalone on an unvalidated or deserialized `FilterNode`. +- **D28 — The value-shape signal on `OperatorDefinition` (D17) is a derived three-state signal, not + a boolean defaulting to scalar.** A plain boolean breaks on the operator model's other two shapes: + unary operators (`exists`/`does not exist`) carry no value at all, and `Syntax.FUNCTION` operators + (a real, already-tested extension point — `FunctionArgValidationTest`'s custom `withinLast`) always + carry an `ArrayNode` of arguments regardless of arity. As specified in D17/T2 ("mark `is in` as + list-shaped … the other 12 default to scalar"), a `FilterValidator` shape check would have wrongly + rejected every `FUNCTION`-syntax filter, including the one this plan's own D16 cites as proof that + custom operators matter. The signal is instead **derived**, not independently set per operator: + `NONE` when `operator.isUnary()`, `LIST` when `operator.getSyntax() == Syntax.FUNCTION`, `SCALAR` + otherwise (which is also the correct default for `is in` once explicitly marked — see D17/T2's + "mark `is in` as list-shaped" for the one exception to the `SCALAR` default among `INFIX` + operators). +- **D29 — `FilterQueryTranslator` expands `AnyFilter` per-node inside its own tree walk (in + `visitAny`), not via one whole-tree `AnyFilterExpander.expand(node, …)` call.** D18 requires + distinguishing an `any`-expansion-produced `OrFilter` (whose unmapped operands get silently + dropped) from an author-written or deserialized `OrFilter` at the same tree position (which still + hard-fails on an unmapped column) — but `AnyFilterExpander.expand` recurses over the *entire* tree + and returns plain `OrFilter`s indistinguishable from any other, so a single whole-tree call would + either drop unmapped columns everywhere (silently widening every explicit path's failure mode, + which D18 explicitly says should stay a hard fail) or drop nothing (reintroducing the exact + problem D18 exists to fix). The translator instead expands each `AnyFilter` node exactly where it + visits one, so the resulting operands are known, at that point in the code, to be + expansion-derived, and D18's drop-unmapped behavior is applied only there. +- **D30 — The empty-list rejection (D11/D22) keys on the value-shape signal (D28), not on the + literal operator name `is in`.** D28 gives `OperatorDefinition` a general list-shape signal + precisely so the core doesn't need to special-case operator names — hardcoding the check to `is + in` would mean a vendor-registered custom list-shaped operator (e.g. an `is not in`) produces an + `IN ()`/similar syntax error at execution instead of the clean `SqlRenderException` this plan + promises, the same class of gap D16 closed for the unknown-operator dispatch case one level down. + The check becomes: "any operator whose declared shape is `LIST` and whose converted value is an + empty list is rejected." +- **D31 — `filter-sql-adapter` declares `jackson-databind` as a direct compile dependency in its own + `pom.xml`, not just relies on filter-core's transitive.** The adapter's entire D7/D19 value + conversion step operates directly on `com.fasterxml.jackson.databind.JsonNode` — a first-class + compile dependency of this module, not an incidental one. T3 groups `jackson-databind` with + `antlr4-runtime` as parent-``-only specifically so `filter-sql-adapter` + doesn't silently inherit ANTLR (which it never uses); the same reasoning means it must explicitly + redeclare `jackson-databind` rather than relying on a transitive that would silently break if + filter-core's own Jackson scope ever changed. +- **D32 — `ColumnMapping.identity()` returns `Optional.empty()` for any path containing `.`.** D20 + already documents flat/single-table mappings as the adapter's scope limit, but nothing previously + made that limit fail fast: `identity()` plus D23's mandated quoting turns the DSL's own headline + example, `lineItems.qty` (the first line of both `README.md` and `docs/filter-dsl.md`, and the + Javadoc example on `FieldDefinition`/`FieldFilter`), into a single well-formed, injection-safe, + but nonexistent quoted identifier — `"lineItems.qty"` — which fails only as a vendor-specific + database error at execution time, not as `SqlRenderException`. Scoped to `identity()` only, not to + the translator generally: a caller using `ColumnMapping.of(Map)` who has deliberately mapped a + dotted path to a real (e.g. joined-view) column is unaffected. +- **D33 — The translator dispatches `binary()` vs. `unary()` by `operator.isUnary()`, never by + `value == null`; the port's Javadoc states that a `null` passed to `binary(...)` means SQL + `NULL`, not "no value."** `FieldFilter.value` is `null` for a genuinely unary operator, but + `status is null` parses to a *binary* `FieldFilter("status", "is", NullNode)` — and D7 converts + `NullNode` to Java `null` — so after conversion the two cases are indistinguishable by value alone. + A `value == null` dispatch would silently render `status is null` as `exists`, contradicting this + plan's own Operator-coverage requirement that `is`/`is not` produce `IS NULL`/`IS NOT NULL` for + null values. Dispatch is by the operator's own `isUnary()` flag, which is unambiguous regardless + of what the (possibly absent) value converts to. + +## Target layout + +``` +pom.xml # org.sequeless:sequeless-filter, packaging=pom, aggregator+parent +filter-core/ + pom.xml # org.sequeless:filter-core + src/main/antlr4/org/sequeless/filter/internal/parser/Filter.g4 + src/main/java/org/sequeless/filter/ + api/FieldRegistry.java # of(FieldRegistrySpec) restored; permissive()/isPermissive() gone + api/FieldRegistrySpec.java # NEW — Lombok @Builder, holds List fields + api/OperatorDefinition.java # gains a derived 3-state value-shape signal (D17/D28): + # NONE if isUnary(), LIST if syntax==FUNCTION, else SCALAR + # ("is in" explicitly marked LIST among INFIX operators) + api/FilterValidator.java # gains a value-shape check alongside its existing + # applicableTypes/permittedOperators checks (D17) + internal/DefaultFieldRegistry.java # stays in main; ctor now takes FieldRegistrySpec + internal/BuiltinOperatorContributor.java # marks "is in" as list-shaped (D17) + internal/… # DefaultOperatorRegistry, CursorAnalyzer, + # FilterBuildingVisitor otherwise unchanged + spi/… # unchanged + src/test/java/org/sequeless/filter/… # existing tests + src/test/java/org/sequeless/filter/testfixtures/ # NEW, test scope only + PermissiveFieldRegistry.java # was internal/PermissiveFieldRegistry — the only fixture left +filter-sql-adapter/ + pom.xml # org.sequeless:filter-sql-adapter + src/main/java/org/sequeless/filter/sql/ + api/SqlFragment.java # null-tolerant defensive copy of parameters (D21) + api/ColumnMapping.java # identity() rejects dotted paths, returns empty (D32) + api/FilterQueryTranslator.java # core: validates via FilterValidator first (D27); walks + # FilterNode, converts values (with catch-all, D19), + # expands AnyFilter per-node in visitAny (D29), dropping + # unmapped operands only there (D18), rejects any empty + # and/or at any depth keyed on the shape signal (D22/D30), + # dispatches binary()/unary() by operator.isUnary() (D33), + # delegates to QueryBuilder + api/SqlRenderException.java + spi/QueryBuilder.java # vendor port (binary/unary/and/or); no static factory (D12.3) + spi/AnsiQueryBuilder.java # public, Template Method default impl — 8 protected hooks: + # quoteIdentifier (mandated escaping, D23), renderIs, + # renderComparison, renderLike, renderIn, renderExists, + # renderUnknown, unaryUnknown (fallback pair, D16) + internal/ # currently unused — reserved, no classes today + src/test/java/org/sequeless/filter/sql/… # H2 QueryBuilder fixture + tests — + # must include at least one deliberate hook override to + # exercise the Template Method extension path, even if H2 + # otherwise needs no ANSI deviations +``` + +The test-fixture package name `org.sequeless.filter.testfixtures` exists purely as a naming +convention now (D15 dropped the ArchUnit rule that would have keyed on it) — kept for readability, +not enforcement. + +## Design notes + +**filter-core contract change.** `FieldRegistry` keeps `find`/`all`/`compatibleWith`/`of` and loses +`permissive`/`isPermissive`. `of`'s parameter changes from `List` to the new +`FieldRegistrySpec` (Lombok `@Builder`). `DefaultFieldRegistry` stays a production class in +`internal`, backing `of`; only `PermissiveFieldRegistry` moves to test scope. There is no +api→internal ArchUnit rule (D8) — the facade relationship is intentional and unpoliced within +filter-core; cross-module leakage is what the ArchUnit task actually guards. + +`isPermissive()` is still removed (verified behavior-preserving: its two call sites collapse +unconditionally to `fields.find(path).isEmpty()`), but per **D1/D10**, no commit implementing this +should be flagged as breaking even though it technically is one. + +`FieldContributor`'s Javadoc referencing `FieldRegistry#of(List)` needs updating for the new +signature (`FieldRegistrySpec`), not deletion — `of` still exists, just with a different parameter. +`FieldContributor` itself remains unwired (no production implementor or consumer in this repo, same +as before this plan) — `FieldRegistrySpec` is not being extended to accept `List` +in this pass; that stays a known, pre-existing gap, not something this plan closes. + +**Value-shape validation (D17/D28).** `OperatorDefinition` gains a *derived* three-state shape +signal — `NONE` for unary operators, `LIST` for `Syntax.FUNCTION` operators (which always carry an +argument-list `ArrayNode` regardless of arity) and for `is in` (the one `INFIX` operator explicitly +marked list-shaped), `SCALAR` for every other `INFIX` operator. A plain boolean defaulting to scalar +was considered and rejected (D28): it would wrongly reject every `FUNCTION`-syntax filter, including +`FunctionArgValidationTest`'s already-shipped custom `withinLast` operator. `FilterValidator` checks +a `FieldFilter`/`AnyFilter`'s value shape (JSON array vs. non-array, or absent for `NONE`) against +its resolved operator's declared shape, alongside its existing `applicableTypes`/`permittedOperators` +checks, and rejects a mismatch the same way it rejects an unknown field today. This benefits every +filter-core consumer, not just filter-sql-adapter — a parsed-then-hand-edited or deserialized +`FilterNode` with `status is in 'open'` (scalar into a list operator) or `status is ['a','b']` (list +into a scalar operator) is now rejected at validation time instead of reaching a downstream consumer +as a malformed value. **Per D27**, `FilterQueryTranslator.translate(...)` calls `FilterValidator` +itself before translating (converting any violation to `SqlRenderException`) — neither +`FilterParser.parse` nor anything else validates automatically, so without this the SQL adapter path +would never actually benefit from this check. + +**filter-sql-adapter contracts** (`org.sequeless.filter.sql.api`): + +- `SqlFragment(String sql, List parameters)` — immutable record; `sql` contains `?` + placeholders positionally matching `parameters`. Per **D21**, the compact constructor defensively + copies `parameters` with `Collections.unmodifiableList(new ArrayList<>(parameters))`, not + `List.copyOf` — the list may legitimately contain `null` elements (a `null`-valued `is in` member). +- `ColumnMapping` — `Optional columnFor(String path)`; an empty result on an + explicitly-named path makes the translator reject the filter with `SqlRenderException`. Ships + `ColumnMapping.identity()` (trusted-input-only, per D13 — documented, not validated; **per D32**, + returns `Optional.empty()` for any path containing `.`, so the flat/single-table scope limit + fails fast as `SqlRenderException` instead of producing a well-formed but nonexistent quoted + identifier like `"lineItems.qty"`) and `ColumnMapping.of(Map)` (an explicit + allowlist, safe for untrusted paths, and unaffected by D32 — a caller who has deliberately mapped + a dotted path to a real column is not blocked). Scoped to flat, single-table mappings only — see + D20/Caveats. +- `FilterQueryTranslator` — the vendor-agnostic core. Entry point: + `SqlFragment translate(FilterNode node, ColumnMapping columns, FieldRegistry fields, OperatorRegistry ops, QueryBuilder builder)`. + **Per D27**, first calls `FilterValidator` (using the same `fields`/`ops`) and converts any + violation to `SqlRenderException` — the translator does not assume its input was validated + upstream. Then walks the tree via `FilterVisitor`; **per D29**, `AnyFilter` expansion + happens per-node inside the visitor's `visitAny` (calling `AnyFilterExpander`'s default strategy + there, not once over the whole tree up front), so the resulting operands are known at that point + to be expansion-derived — this is what lets **D18**'s behavior (drop any operand with no + `ColumnMapping` entry, rather than hard-failing) apply only to `any`-expansion results, while an + author-written or deserialized `OrFilter` at the same tree position still hard-fails on an + unmapped column exactly as before. Resolves each field path through `columns` and converts each + `JsonNode` value per D7/D19's mapping (including the catch-all for deserialization-only node + types) before handing it to the builder; dispatches to `builder.binary(...)` vs. `builder.unary(...)` + by `operator.isUnary()` — **never** by `value == null` (D33), since a null-valued *binary* + condition (`status is null`, i.e. `NullNode` converted per D7) and a genuinely *unary* condition + are otherwise indistinguishable after conversion, and dispatching on value would silently turn + `status is null` into `exists`. Per **D11/D22/D30**, raises `SqlRenderException` for any + zero-operand `AndFilter`/`OrFilter` encountered at any depth (including an `any`-expansion result + with nothing left after D18's drop-unmapped step) and for any `LIST`-shaped operator (per D28's + signal — not hardcoded to the literal name `is in`) whose converted value is an empty list. +- `SqlRenderException extends RuntimeException` — a `FilterValidator` violation caught at the + translator's entry (D27), unknown column, unmappable operator, empty and/or or empty list-shaped + value (D11/D22/D30), or an unrecognized `JsonNode` subtype during conversion (D19). + +`org.sequeless.filter.sql.spi.QueryBuilder` is the vendor **port** (D6): `binary(String column, +OperatorDefinition operator, Object value)`, `unary(String column, OperatorDefinition operator)`, +`and(List operands)`, `or(List operands)`. A vendor implementation owns +full construction of each `SqlFragment` from these calls: identifier quoting, operator-to-SQL +translation (including `is in`'s list expansion — the converted `value` arrives as `List` +for that operator), and `LIKE`/`is like`/`is not like`/`contains`/`starts with` escaping semantics, +which are deliberately unspecified by the plan (D14) — except identifier quoting on the *shipped +default*, which D23 does mandate. `sql.spi.AnsiQueryBuilder` (D12) is the shipped default — public, +non-`final`, structured as a Template Method: `final binary()`/`unary()` dispatchers (dispatch +between them by `operator.isUnary()`, per D33) delegating to exactly **8 `protected` hooks**, +enumerated here rather than left as "etc." (per D16's own requirement that the hook set be a +deliberate, complete commitment, not something that accretes): +`quoteIdentifier(String)` (mandated escaping, D23); `renderIs(String column, boolean negated, Object value)` +(`is`/`is not`, including `IS NULL`/`IS NOT NULL` for a `null` value); `renderComparison(String column, OperatorDefinition operator, Object value)` +(the 4 numeric comparisons); `renderLike(String column, OperatorDefinition operator, Object value)` +(`contains`/`starts with`/`is like`/`is not like` — escaping left to the implementation, D14); +`renderIn(String column, List values)` (`is in`); `renderExists(String column, boolean negated)` +(the unary `exists`/`does not exist` pair); and the D16 fallback pair, +`renderUnknown(String column, OperatorDefinition operator, Object value)` / +`unaryUnknown(String column, OperatorDefinition operator)`, both throwing `SqlRenderException` by +default. Any vendor — including this repo's own H2 test fixture — can override just one hook instead +of reimplementing the whole dispatch. Unlike `OperatorRegistry.defaults()`/`FieldRegistry.of()`, +`QueryBuilder` itself has **no** static factory referencing `AnsiQueryBuilder` (D12.3) — the port +stays free of any reference to a concrete adapter; callers and vendors instantiate +`new AnsiQueryBuilder()` (or a subclass) directly. + +**Operator coverage.** `BuiltinOperatorContributor` ships 13 operators: `is`, `is not`, +`is greater than`, `is greater than or equal to`, `is less than`, `is less than or equal to`, +`contains`, `starts with`, `is like`, `is not like`, `is in`, and the unary `exists` / +`does not exist`. All 13 must translate; `is`/`is not` need `IS NULL`/`IS NOT NULL` handling for null +values, `is in` expands to an `IN (?, ?, …)` list from its converted `List` value (rejecting +an empty list, checked by the `LIST`-shape signal per D28/D30 rather than the literal name, per +D11/D22), and the unary pair renders to `IS NOT NULL` / `IS NULL` with no bind parameter. Any +operator outside these 13 reaches `AnsiQueryBuilder`'s `renderUnknown`/`unaryUnknown` fallback +(D16), which throws `SqlRenderException` by default. + +**Release plumbing.** `.releaserc.json`'s `prepareCmd` becomes +`./mvnw -B versions:set -DnewVersion=${nextRelease.version} -DgenerateBackupPoms=false -DprocessAllModules=true` +and its `@semantic-release/git` assets grow to `["CHANGELOG.md", "pom.xml", "*/pom.xml"]` so the +children's `` bumps are committed. A `${revision}`/flatten CI-friendly-version +setup was considered and rejected: it interacts awkwardly with the existing `versions:set` step for +no benefit while the version is shared. + +## Caveats (carried from the brief's Register, plus Stage 4 review findings) + +- **H2 version (B3)** could not be pinned from the repo — no database dependency exists today. Pin + the current H2 2.3.x line (2.3.232 at last check) in the parent's `` with + `test`; `renovate.json` will keep it current. Confirm the exact latest patch at + implementation time rather than trusting this number. +- **antlr4-maven-plugin in a reactor (B4)** could not be verified pre-restructure. The generated + package is derived from the grammar's directory path under `src/main/antlr4`, which is preserved + verbatim by the move, so no output change is expected — but the reactor-conversion task must prove + it by confirming + `filter-core/target/generated-sources/antlr4/org/sequeless/filter/internal/parser/` is populated. +- **flatten-maven-plugin on the aggregator (B5/D9/D24).** `flattenMode=ossrh` on a `pom`-packaged + parent can rewrite the parent POM that children resolve against. `flatten` moves to the parent's + `` with **no `` element** (D24 — a skip flag in the managed entry would be + inherited by every child that declares the plugin, silently disabling it everywhere); the + aggregator simply never declares/binds `flatten` in its own ``; each child declares + it normally and gets the managed config. Every other plugin stays in the parent's inherited + `` unchanged. +- **JaCoCo/Sonar coverage paths (B5/D25).** `sonar.coverage.jacoco.xmlReportPaths` currently names a + single `target/site/jacoco/jacoco.xml` in `sonar-project.properties` — but CI invokes the + Maven-integrated Sonar scanner (`./mvnw -B verify sonar:sonar`), which is documented to read + configuration from the POM, not from that file. Per D25: verify empirically first (confirm current + coverage is actually non-zero) before changing anything; if `sonar-project.properties` is indeed + unread, move the relevant Sonar properties into the parent POM's `` and retire the + file, as part of the reactor-tooling task. Do not add a coverage-aggregator module speculatively — + only if a genuine post-fix zero-coverage symptom appears. +- **Split packages.** Both jars contribute classes under a shared package *prefix* + (`org.sequeless.filter.*`), not the same package — no class is contributed by both jars, so this + is not actually a split-package situation, and neither JPMS nor OSGi would reject it as one. No + `module-info.java` exists anywhere in the repo today; the real work if JPMS/OSGi is ever adopted + would be adding module descriptors, not renaming packages. +- **Custom `AnyExpansionStrategy` is unreachable through the adapter (D4).** `FilterQueryTranslator` + always uses `AnyFilterExpander`'s default strategy internally. A caller needing a non-default + `AnyExpansionStrategy` must pre-expand the filter with their own strategy before calling + `translate(...)` — the adapter's entry point has no parameter for supplying one. +- **Date-time values pass through as plain strings (D7).** No temporal coercion is performed; + binding a `date-time`-formatted field's string value against a `TIMESTAMP`/`DATE` column is left + entirely to the vendor `QueryBuilder`/JDBC driver. +- **`LIKE`/pattern-escaping semantics are vendor-defined, not specified (D14).** Different + `QueryBuilder` implementations — including the shipped `AnsiQueryBuilder` versus any third-party + one — may treat `%`/`_` inside `contains`/`starts with`/`is like`/`is not like` values differently. + (Identifier *quoting* on the shipped default is nonetheless mandated — see D23 — this caveat is + about matching semantics, a different concern from the injection boundary D23 closes.) +- **Only flat, single-table column mappings are supported (D20/B10).** `ColumnMapping` returns a + bare column name; there is no way to express a join, correlated subquery, or JSON path expression. + The DSL's own headline nested-path example (`lineItems.qty > 5`) typically requires a join on a + relational schema and is out of scope for this adapter as specified. Per **D32**, + `ColumnMapping.identity()` fails fast on this (`Optional.empty()` for any dotted path) rather than + producing a syntactically valid but nonexistent quoted identifier; a caller using `of(Map)` can + still deliberately map a dotted path if they have a real column for it. State this plainly in + `docs/sql-adapter.md`. +- **`BigIntegerNode`'s catch-all conversion (D19) can still fail at the JDBC driver, not as + `SqlRenderException`.** D19's numeric catch-all uses `isNumber()`/`numberValue()`, which for a + `BigIntegerNode` (a JSON integer past `Long.MAX_VALUE`) yields a raw `BigInteger` — a type most + JDBC drivers' `setObject` rejects. Consider normalizing to `BigDecimal` at implementation time + (`DecimalNode`, produced when a caller enables `USE_BIG_DECIMAL_FOR_FLOATS`, is unaffected — it + already binds fine); note the distinction in `docs/sql-adapter.md` regardless of which is chosen. +- **`QueryBuilder`/`FilterQueryTranslator` implementations are expected to be stateless and + thread-safe.** Not enforced by any type in the plan, but callers will naturally hold one instance + as a long-lived singleton across request threads; state this expectation in the port's Javadoc. +- **The `JsonNode`→`Object` conversion (D7/D19) still leaves each `QueryBuilder` writing its own + type dispatch** (`instanceof`/pattern-match over `String`/`Number`/`Boolean`/`null`/`List`) to + decide scalar binding vs. list expansion vs. null handling — centralizing *conversion* doesn't + eliminate needing to branch on the *result* type. Accepted as consistent with D3's minimalism + (no sealed value type introduced for this), just noting the "every vendor gets it for free" framing + in D7 covers conversion, not dispatch. + +## Phases + +- Phase 1: Core contract cleanup — T1, T2 +- Phase 2: Multi-module reactor — T3, T4 +- Phase 3: SQL adapter contracts and translator — T5, T6 +- Phase 4: H2 verification and cross-module guards — T7, T8, T9 + +## Tasks + +- [ ] T1: (Phase 1) Add `FieldRegistrySpec` (Lombok `@Builder`, `List fields`) to + `org.sequeless.filter.api`; change `DefaultFieldRegistry`'s constructor and + `FieldRegistry.of(...)` to accept `FieldRegistrySpec` instead of `List` + (`DefaultFieldRegistry` itself stays a plain constructor, no builder needed); delete + `FieldRegistry.permissive()` and `FieldRegistry.isPermissive()`; move `PermissiveFieldRegistry` + out of `internal` into test scope as `org.sequeless.filter.testfixtures.PermissiveFieldRegistry`; + collapse `FilterBuildingVisitor.resolvePath` (line 156) and `FilterValidator.validateField` + (line 42) to an unconditional `fields.find(path).isEmpty()` check; update call sites in + `FilterParserTest`, `FilterSerializerTest`, `NormalizationTest`, `FunctionArgValidationTest`, + `FilterSchemaProviderTest`, `AnyFilterExpanderTest`, `CompletionEngineTest` — `FieldRegistry.of(List.of(...))` + calls become `FieldRegistry.of(FieldRegistrySpec.builder().fields(List.of(...)).build())`, the + handful using `FieldRegistry.permissive()` switch to the new test fixture; update + `FieldContributor`'s Javadoc for the new `of` signature. No new ArchUnit rule is added for this + change (D8 dropped the api→internal rule; D15 dropped the fixture-leak rule) — `BoundaryRulesTest` + is expected to keep passing unmodified. One atomic commit, plain `fix:` type — per D1/D10, do + not mark it a breaking change even though `isPermissive()` removal technically is one. `make + verify` stays green. +- [ ] T2: (Phase 1) Give `OperatorDefinition` a **derived**, three-state value-shape signal (per + **D17/D28**) — `NONE` when `isUnary()`, `LIST` when `getSyntax() == Syntax.FUNCTION`, `SCALAR` + otherwise — and mark `is in` as the one `LIST`-shaped exception among the `INFIX` built-ins in + `BuiltinOperatorContributor`. A plain scalar-default boolean is explicitly wrong here: it would + reject every `FUNCTION`-syntax filter, including `FunctionArgValidationTest`'s existing custom + `withinLast` operator. Extend `FilterValidator` to reject a `FieldFilter`/`AnyFilter` whose + value shape (absent for `NONE`, JSON array for `LIST`, non-array for `SCALAR`) doesn't match + its resolved operator's declared shape, alongside the existing + `applicableTypes`/`permittedOperators` checks. This is a filter-core improvement independently + motivated by reviewing the SQL adapter's needs — benefits every consumer of `FilterValidator`, + not just filter-sql-adapter (which additionally calls it directly per D27/T6, since nothing + calls `FilterValidator` automatically). Separate commit from T1 (different concern, though both + touch validator-adjacent code). +- [ ] T3: (Phase 2) Convert the root `pom.xml` to `pom` with + `filter-core`, hoisting the current plain `` + (jackson-databind, antlr4-runtime, lombok, junit-jupiter, assertj-core, mockito-core, + archunit-junit5 — there is no `` block today, so this is introducing one, + not hoisting an existing one) into the parent's ``. Resolved split: + `jackson-databind` and `antlr4-runtime` are ``-only at the parent (so + `filter-sql-adapter` doesn't silently inherit an ANTLR runtime dependency it never uses, and + per **D31** must redeclare `jackson-databind` itself in T5 rather than relying on a + transitive); `lombok`/`junit-jupiter`/`assertj-core`/`mockito-core`/`archunit-junit5` are + wanted as real dependencies in both modules and stay as plain, inherited `` on + the parent directly (simplest — neither child needs to redeclare these). `filter-core/pom.xml` + redeclares `jackson-databind` and `antlr4-runtime` directly (both are first-class compile + dependencies of the DSL/parser). Per **D9/D24**, leave `compiler`, `antlr4`, `spotless`, `jacoco`, `surefire`, + `failsafe`, `enforcer` in the parent's `` exactly as today (inherited + automatically), and move only `flatten` to `` with **no `` element** + — the aggregator simply doesn't declare/bind `flatten` in its own ``; create + `filter-core/pom.xml` (`org.sequeless:filter-core`, no ``) re-declaring `flatten` in + its own ``; `git mv` `src/` and the ANTLR grammar under `filter-core/`. Verify + `filter-core/target/generated-sources/antlr4/org/sequeless/filter/internal/parser/` is + generated and all existing tests still run. +- [ ] T4: (Phase 2) Reactor-proof the surrounding tooling: `.releaserc.json` (`-DprocessAllModules=true`, + git assets `["CHANGELOG.md", "pom.xml", "*/pom.xml"]`), `README.md` dependency coordinates + (`filter-core`, and a short module table), a `make verify` smoke check, and an ADR recording the + split via `make new-adr`. Per **D25**: before touching Sonar config, verify empirically whether + `sonar-project.properties` is actually read by CI's `sonar:sonar` Maven-plugin invocation + (check current/pre-split coverage reporting). Only if confirmed unread, move + `sonar.projectKey`/`sonar.organization`/exclusions/`sonar.coverage.jacoco.xmlReportPaths` into + the parent POM's `` and retire the file — do not change it speculatively. +- [ ] T5: (Phase 3) Add `filter-sql-adapter` to `` with `org.sequeless:filter-sql-adapter` + depending on `org.sequeless:filter-core:${project.version}`, re-declaring `flatten` in its own + `` per D9/D24/T3; per **D31**, also directly declare `jackson-databind` (its + value-conversion step is a first-class `JsonNode` consumer, not just a transitive rider on + filter-core); define the public contracts: `sql.api.SqlFragment` + (null-tolerant parameter copy per D21), `sql.api.ColumnMapping` (+`identity()` documented + trusted-input-only per D13 and rejecting dotted paths per D32, `of(Map)`; document the + flat/single-table scope limit per D20), + `sql.api.FilterQueryTranslator`, `sql.api.SqlRenderException`, and the vendor port + `sql.spi.QueryBuilder` with its four methods (`binary`/`unary`/`and`/`or`, D6). State the + expected statelessness/thread-safety of implementations in the port's Javadoc. Contracts + + Javadoc only; no translation logic yet. +- [ ] T6: (Phase 3) Implement `sql.spi.AnsiQueryBuilder` (public, non-`final`, Template Method: + `final` `binary()`/`unary()` dispatchers delegating to exactly 8 `protected` hooks — + `quoteIdentifier` (mandated `"…"`-with-doubled-quotes escaping, security-relevant per D23), + `renderIs`, `renderComparison`, `renderLike`, `renderIn`, `renderExists`, and the + `renderUnknown`/`unaryUnknown` fallback pair for non-built-in operators, throwing + `SqlRenderException` by default (D16) — this is the complete, fixed hook set, not a starting + point to extend ad hoc) and `FilterQueryTranslator`'s tree walk as a `FilterVisitor`: + **first** call `FilterValidator` (per **D27**) with the same `fields`/`ops` and convert any + violation to `SqlRenderException` — `translate(...)` does not assume its `FilterNode` was + validated upstream; convert each `JsonNode` value per D7/D19's mapping including the catch-all + for unrecognized subtypes; expand `AnyFilter` **per-node inside `visitAny`** (per **D29** — not + one whole-tree `AnyFilterExpander.expand` call up front), using the default strategy, dropping + any expanded operand with no `ColumnMapping` entry (D18) and raising `SqlRenderException` only + if none survive — an author-written or deserialized `OrFilter` at any other tree position still + hard-fails on an unmapped column, exactly as before; dispatch `binary()` vs. `unary()` by + `operator.isUnary()`, **never** by `value == null` (D33 — `status is null` is a binary + condition whose converted value happens to be Java `null`, and must render `IS NULL`, not be + mistaken for `exists`); reject any zero-operand `AndFilter`/`OrFilter` encountered at any depth, + and any operator whose declared shape (per D28's signal, not the literal name `is in`) is + `LIST` and whose converted value is an empty list (D11/D22/D30); all 13 built-in operators + including `IS NULL`/`IS NOT NULL` for null-valued `is`/`is not`, `IN (?, …)` expansion for + `is in`, the unary `exists`/`does not exist` pair, AND/OR grouping with parentheses; + `ColumnMapping` failures on an explicitly-named path raising `SqlRenderException`; unit tests + asserting emitted SQL text and bind-parameter order, with ASTs built directly via `Filters` and + fields via `FieldRegistry.of(FieldRegistrySpec...)`, including at least one custom operator + exercising the `renderUnknown` fallback path and at least one `status is null` case proving the + dispatch-by-`isUnary()` rule (D33) renders `IS NULL`, not `exists`. +- [ ] T7: (Phase 4) Add H2 (test scope) plus a test-scope `QueryBuilder` fixture for H2 — extend + `sql.spi.AnsiQueryBuilder`, and deliberately override **at least one** `protected` hook (even + if H2 needs no real ANSI deviation, override something — e.g. a distinct quoting or escaping + choice — solely to exercise the Template Method extension path with real test coverage, per a + review finding that otherwise this mechanism could ship completely untested); build a + `FieldRegistry` the same way any production caller would + (`FieldRegistry.of(FieldRegistrySpec...)`, no separate test-only registry fixture needed); + create an H2 schema + seed data, then execute translated `SqlFragment`s as real + `PreparedStatement`s and assert the returned rows — covering each operator family end to end, + including at least one filter using `any` (with a deliberately partial `ColumnMapping`, to + exercise D18's drop-unmapped-operands behavior) and at least one `is in` containing a `null` + element (to exercise D21's null-tolerant parameter list). Name these tests `*Test` (surefire), + not `*IT` — the reactor's JaCoCo setup has no `report-integration` execution, so `*IT`-named + tests would run but their coverage wouldn't reach the Sonar report. +- [ ] T8: (Phase 4) Add `filter-sql-adapter`'s ArchUnit test carrying the module-local rule + (`sql.api`/`sql.spi` types are public) and the cross-module rules that only this module's + classpath can see: no main-scope class in `org.sequeless.filter.sql..` may depend on + `org.sequeless.filter.internal..` or `org.sequeless.filter.spi..`; and no filter-core class may + depend on `org.sequeless.filter.sql..` (kept per **D26** as documentation of intent even though + it's currently unfalsifiable — Maven's reactor structure already rejects the cycle). Per + **D15**, do not add a `testfixtures`-leak rule here — it can never trip (no test-jar dependency + exists between the modules) and would be unenforceable documentation, not a real guard. Confirm + the rule that *can* meaningfully fail (`sql..` → `internal..`/`spi..`) does so on a deliberate + violation before reverting it. +- [ ] T9: (Phase 4) Document the adapter: a `docs/sql-adapter.md` covering the + parse → validate (`FilterValidator`, called internally by `translate(...)` — D27) → + `FilterQueryTranslator` (which expands `any` per-node internally, using the default expansion + strategy — D4/D29 — and drops unmapped operands rather than failing, D18) → `ColumnMapping` + (flat/single-table scope limit and dotted-path rejection in `identity()`, D20/D32) → + `QueryBuilder` → `PreparedStatement` pipeline, the `JsonNode`→Java value-conversion table + (D7/D19, including the `BigInteger`-binding caveat) and its date-time non-goal, `LIKE`-escaping + being vendor-defined while identifier quoting on the shipped default is not (D14 vs. D23), and + how to write a vendor `QueryBuilder` — implement the interface from scratch, or extend + `AnsiQueryBuilder` and override just the `protected` hooks that differ (the 8-hook set, D16 — + including `renderUnknown`/`unaryUnknown` for custom operators) — (no `QueryBuilder.ansi()` + factory exists; instantiate `AnsiQueryBuilder`/a subclass directly, D12) — linked from + `README.md` and `docs/filter-dsl.md`. From 4b0a95a1550a8177b3acade0e8016c95a36c20c9 Mon Sep 17 00:00:00 2001 From: Ben B Date: Wed, 29 Jul 2026 21:52:15 +0000 Subject: [PATCH 02/11] fix: retype FieldRegistry.of to FieldRegistrySpec and drop permissive mode (T1) FieldRegistry.of now takes a Lombok-built FieldRegistrySpec instead of a raw List, so the registry can gain configuration later without another signature change. FieldRegistry.permissive()/isPermissive() are removed and PermissiveFieldRegistry becomes a test-only fixture; the two isPermissive() call sites collapse to an unconditional fields.find(path).isEmpty() check, which is behaviour-preserving because the fixture's find() always resolves. --- .../multimodule-archunit-sql-adapter/plan.md | 2 +- .../sequeless/filter/api/FieldRegistry.java | 28 ++++------------- .../filter/api/FieldRegistrySpec.java | 30 +++++++++++++++++++ .../sequeless/filter/api/FilterValidator.java | 2 +- .../filter/internal/DefaultFieldRegistry.java | 6 +++- .../internal/FilterBuildingVisitor.java | 2 +- .../filter/spi/FieldContributor.java | 5 +++- .../filter/AnyFilterExpanderTest.java | 5 +++- .../filter/CompletionEngineTest.java | 5 +++- .../sequeless/filter/FilterParserTest.java | 6 ++-- .../filter/FilterSchemaProviderTest.java | 8 +++-- .../filter/FilterSerializerTest.java | 3 +- .../filter/FunctionArgValidationTest.java | 3 +- .../sequeless/filter/NormalizationTest.java | 3 +- .../PermissiveFieldRegistry.java | 15 +++------- 15 files changed, 76 insertions(+), 47 deletions(-) create mode 100644 src/main/java/org/sequeless/filter/api/FieldRegistrySpec.java rename src/{main/java/org/sequeless/filter/internal => test/java/org/sequeless/filter/testfixtures}/PermissiveFieldRegistry.java (66%) diff --git a/docs/specs/multimodule-archunit-sql-adapter/plan.md b/docs/specs/multimodule-archunit-sql-adapter/plan.md index b780cd6..16445cf 100644 --- a/docs/specs/multimodule-archunit-sql-adapter/plan.md +++ b/docs/specs/multimodule-archunit-sql-adapter/plan.md @@ -630,7 +630,7 @@ no benefit while the version is shared. ## Tasks -- [ ] T1: (Phase 1) Add `FieldRegistrySpec` (Lombok `@Builder`, `List fields`) to +- [x] T1: (Phase 1) Add `FieldRegistrySpec` (Lombok `@Builder`, `List fields`) to `org.sequeless.filter.api`; change `DefaultFieldRegistry`'s constructor and `FieldRegistry.of(...)` to accept `FieldRegistrySpec` instead of `List` (`DefaultFieldRegistry` itself stays a plain constructor, no builder needed); delete diff --git a/src/main/java/org/sequeless/filter/api/FieldRegistry.java b/src/main/java/org/sequeless/filter/api/FieldRegistry.java index 89ad48a..fedceb3 100644 --- a/src/main/java/org/sequeless/filter/api/FieldRegistry.java +++ b/src/main/java/org/sequeless/filter/api/FieldRegistry.java @@ -3,12 +3,12 @@ import java.util.List; import java.util.Optional; import org.sequeless.filter.internal.DefaultFieldRegistry; -import org.sequeless.filter.internal.PermissiveFieldRegistry; /** * Queryable collection of {@link FieldDefinition}s. * - *

Always required by the parser. Use {@link #permissive()} to skip field-path validation. + *

Always required by the parser. Build one with {@link #of(FieldRegistrySpec)}, or supply your + * own implementation. */ public interface FieldRegistry { @@ -36,26 +36,10 @@ public interface FieldRegistry { List compatibleWith(String canonicalOperator, OperatorRegistry operators); /** - * Whether this registry skips field-path validation. - * Returns {@code false} for the default strict registry. + * Builds a strict registry from the given spec. + * The parser will reject any path not present in {@link FieldRegistrySpec#getFields()}. */ - default boolean isPermissive() { - return false; - } - - /** - * Builds a strict registry from the given field definitions. - * The parser will reject any path not present in this list. - */ - static FieldRegistry of(List fields) { - return new DefaultFieldRegistry(fields); - } - - /** - * No-op registry that accepts any valid dot-notation path without validation. - * Use when field-path validation is not desired. - */ - static FieldRegistry permissive() { - return PermissiveFieldRegistry.INSTANCE; + static FieldRegistry of(FieldRegistrySpec spec) { + return new DefaultFieldRegistry(spec); } } diff --git a/src/main/java/org/sequeless/filter/api/FieldRegistrySpec.java b/src/main/java/org/sequeless/filter/api/FieldRegistrySpec.java new file mode 100644 index 0000000..e12fa55 --- /dev/null +++ b/src/main/java/org/sequeless/filter/api/FieldRegistrySpec.java @@ -0,0 +1,30 @@ +package org.sequeless.filter.api; + +import java.util.List; +import lombok.Builder; +import lombok.Value; + +/** + * Configuration for a strict {@link FieldRegistry}, built via {@link FieldRegistry#of(FieldRegistrySpec)}. + * + *

Exists so the registry can gain further configuration knobs later without another + * signature change on {@code FieldRegistry.of(...)}. + */ +@Value +@Builder +public class FieldRegistrySpec { + + /** Registered field definitions, in registration order. Empty means an empty strict registry. */ + @Builder.Default + List fields = List.of(); + + /** Defensive-copy override for the Lombok-generated builder setter. */ + public static class FieldRegistrySpecBuilder { + + public FieldRegistrySpecBuilder fields(List fields) { + this.fields$value = List.copyOf(fields); + this.fields$set = true; + return this; + } + } +} diff --git a/src/main/java/org/sequeless/filter/api/FilterValidator.java b/src/main/java/org/sequeless/filter/api/FilterValidator.java index d0f3fc4..c8adc07 100644 --- a/src/main/java/org/sequeless/filter/api/FilterValidator.java +++ b/src/main/java/org/sequeless/filter/api/FilterValidator.java @@ -39,7 +39,7 @@ private static void validateNode( private static void validateField( FieldFilter f, OperatorRegistry ops, FieldRegistry fields, List violations) { - if (!fields.isPermissive() && fields.find(f.path()).isEmpty()) { + if (fields.find(f.path()).isEmpty()) { violations.add(new FilterViolation(f.path(), "Unknown field: '" + f.path() + "'")); return; } diff --git a/src/main/java/org/sequeless/filter/internal/DefaultFieldRegistry.java b/src/main/java/org/sequeless/filter/internal/DefaultFieldRegistry.java index 6c8b41d..0554a9d 100644 --- a/src/main/java/org/sequeless/filter/internal/DefaultFieldRegistry.java +++ b/src/main/java/org/sequeless/filter/internal/DefaultFieldRegistry.java @@ -4,9 +4,11 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import org.sequeless.filter.api.FieldDefinition; import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FieldRegistrySpec; import org.sequeless.filter.api.OperatorDefinition; import org.sequeless.filter.api.OperatorRegistry; @@ -16,7 +18,9 @@ public class DefaultFieldRegistry implements FieldRegistry { private final List ordered; private final Map index; - public DefaultFieldRegistry(List fields) { + public DefaultFieldRegistry(FieldRegistrySpec spec) { + Objects.requireNonNull(spec, "spec"); + List fields = spec.getFields(); this.ordered = new ArrayList<>(fields); this.index = new LinkedHashMap<>(); for (FieldDefinition field : fields) { diff --git a/src/main/java/org/sequeless/filter/internal/FilterBuildingVisitor.java b/src/main/java/org/sequeless/filter/internal/FilterBuildingVisitor.java index 2c8b4e9..5af2f20 100644 --- a/src/main/java/org/sequeless/filter/internal/FilterBuildingVisitor.java +++ b/src/main/java/org/sequeless/filter/internal/FilterBuildingVisitor.java @@ -153,7 +153,7 @@ private FilterNode buildCondition(FilterParser.FieldOrAnyContext fieldOrAny, Str private String resolvePath(FilterParser.PathContext ctx) { String path = ctx.WORD().stream().map(TerminalNode::getText).collect(Collectors.joining(".")); - if (!fields.isPermissive() && fields.find(path).isEmpty()) { + if (fields.find(path).isEmpty()) { throw new FilterParseException( "Unknown field path: '" + path + "'", ctx.getStart().getStartIndex()); } diff --git a/src/main/java/org/sequeless/filter/spi/FieldContributor.java b/src/main/java/org/sequeless/filter/spi/FieldContributor.java index b732ad6..1b2c98a 100644 --- a/src/main/java/org/sequeless/filter/spi/FieldContributor.java +++ b/src/main/java/org/sequeless/filter/spi/FieldContributor.java @@ -5,7 +5,10 @@ /** * Extension point for registering filterable fields. - * Implement and pass to {@link org.sequeless.filter.api.FieldRegistry#of(List)}. + * + *

Not consumed automatically: pass the contributed definitions into + * {@link org.sequeless.filter.api.FieldRegistrySpec}'s {@code fields} property and build a registry + * with {@link org.sequeless.filter.api.FieldRegistry#of(org.sequeless.filter.api.FieldRegistrySpec)}. */ @FunctionalInterface public interface FieldContributor { diff --git a/src/test/java/org/sequeless/filter/AnyFilterExpanderTest.java b/src/test/java/org/sequeless/filter/AnyFilterExpanderTest.java index c50409f..d01882d 100644 --- a/src/test/java/org/sequeless/filter/AnyFilterExpanderTest.java +++ b/src/test/java/org/sequeless/filter/AnyFilterExpanderTest.java @@ -17,6 +17,7 @@ import org.sequeless.filter.api.FieldDefinition; import org.sequeless.filter.api.FieldFilter; import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FieldRegistrySpec; import org.sequeless.filter.api.FilterNode; import org.sequeless.filter.api.Filters; import org.sequeless.filter.api.OperatorRegistry; @@ -38,7 +39,9 @@ void setUp() { FieldDefinition age = FieldDefinition.builder().path("age").jsonSchemaType("number").build(); - fields = FieldRegistry.of(List.of(name, description, age)); + fields = FieldRegistry.of(FieldRegistrySpec.builder() + .fields(List.of(name, description, age)) + .build()); ops = OperatorRegistry.defaults(); } diff --git a/src/test/java/org/sequeless/filter/CompletionEngineTest.java b/src/test/java/org/sequeless/filter/CompletionEngineTest.java index e0602fc..559324a 100644 --- a/src/test/java/org/sequeless/filter/CompletionEngineTest.java +++ b/src/test/java/org/sequeless/filter/CompletionEngineTest.java @@ -8,6 +8,7 @@ import org.sequeless.filter.api.CompletionEngine; import org.sequeless.filter.api.FieldDefinition; import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FieldRegistrySpec; import org.sequeless.filter.api.OperatorRegistry; class CompletionEngineTest { @@ -24,7 +25,9 @@ void setUp() { FieldDefinition ageField = FieldDefinition.builder().path("age").jsonSchemaType("number").build(); - FieldRegistry fields = FieldRegistry.of(List.of(statusField, ageField)); + FieldRegistry fields = FieldRegistry.of(FieldRegistrySpec.builder() + .fields(List.of(statusField, ageField)) + .build()); OperatorRegistry ops = OperatorRegistry.defaults(); engine = new CompletionEngine(ops, fields); } diff --git a/src/test/java/org/sequeless/filter/FilterParserTest.java b/src/test/java/org/sequeless/filter/FilterParserTest.java index dbef583..456f3c1 100644 --- a/src/test/java/org/sequeless/filter/FilterParserTest.java +++ b/src/test/java/org/sequeless/filter/FilterParserTest.java @@ -8,16 +8,18 @@ import org.sequeless.filter.api.AnyFilter; import org.sequeless.filter.api.FieldFilter; import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FieldRegistrySpec; import org.sequeless.filter.api.FilterNode; import org.sequeless.filter.api.FilterParseException; import org.sequeless.filter.api.FilterParser; import org.sequeless.filter.api.OperatorRegistry; import org.sequeless.filter.api.OrFilter; +import org.sequeless.filter.testfixtures.PermissiveFieldRegistry; class FilterParserTest { private static final OperatorRegistry OPS = OperatorRegistry.defaults(); - private static final FieldRegistry FIELDS = FieldRegistry.permissive(); + private static final FieldRegistry FIELDS = PermissiveFieldRegistry.INSTANCE; private FilterNode parse(String input) { return FilterParser.parse(input, OPS, FIELDS); @@ -165,7 +167,7 @@ void parsesStringWithEmbeddedSingleQuote() { @Test void throwsOnUnknownField() { - FieldRegistry strict = FieldRegistry.of(java.util.List.of()); + FieldRegistry strict = FieldRegistry.of(FieldRegistrySpec.builder().build()); assertThatThrownBy(() -> FilterParser.parse("status is 'active'", OPS, strict)) .isInstanceOf(FilterParseException.class) .hasMessageContaining("status"); diff --git a/src/test/java/org/sequeless/filter/FilterSchemaProviderTest.java b/src/test/java/org/sequeless/filter/FilterSchemaProviderTest.java index 1afd472..c7805c4 100644 --- a/src/test/java/org/sequeless/filter/FilterSchemaProviderTest.java +++ b/src/test/java/org/sequeless/filter/FilterSchemaProviderTest.java @@ -13,6 +13,7 @@ import org.sequeless.filter.api.CompletionProvider; import org.sequeless.filter.api.FieldDefinition; import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FieldRegistrySpec; import org.sequeless.filter.api.FilterSchemaProvider; import org.sequeless.filter.api.OperatorDefinition; import org.sequeless.filter.api.OperatorRegistry; @@ -35,7 +36,9 @@ void setUp() { .permittedOperators(List.of("is greater than", "is less than")) .build(); - fields = FieldRegistry.of(List.of(statusField, ageField)); + fields = FieldRegistry.of(FieldRegistrySpec.builder() + .fields(List.of(statusField, ageField)) + .build()); ops = OperatorRegistry.defaults(); provider = new FilterSchemaProvider(ops, fields); } @@ -75,7 +78,8 @@ void suggestValuesDelegatesToCompletionProvider() { .jsonSchemaType("string") .completionProvider(mockProvider) .build(); - FieldRegistry fieldsWithProvider = FieldRegistry.of(List.of(statusWithProvider)); + FieldRegistry fieldsWithProvider = FieldRegistry.of( + FieldRegistrySpec.builder().fields(List.of(statusWithProvider)).build()); FilterSchemaProvider p = new FilterSchemaProvider(ops, fieldsWithProvider); List suggestions = p.suggestValues("status", "is", "act"); diff --git a/src/test/java/org/sequeless/filter/FilterSerializerTest.java b/src/test/java/org/sequeless/filter/FilterSerializerTest.java index 36a2075..0cef546 100644 --- a/src/test/java/org/sequeless/filter/FilterSerializerTest.java +++ b/src/test/java/org/sequeless/filter/FilterSerializerTest.java @@ -12,11 +12,12 @@ import org.sequeless.filter.api.FilterSerializer; import org.sequeless.filter.api.Filters; import org.sequeless.filter.api.OperatorRegistry; +import org.sequeless.filter.testfixtures.PermissiveFieldRegistry; class FilterSerializerTest { private static final OperatorRegistry OPS = OperatorRegistry.defaults(); - private static final FieldRegistry FIELDS = FieldRegistry.permissive(); + private static final FieldRegistry FIELDS = PermissiveFieldRegistry.INSTANCE; private FilterNode roundTrip(FilterNode node) { String serialized = FilterSerializer.serialize(node, OPS); diff --git a/src/test/java/org/sequeless/filter/FunctionArgValidationTest.java b/src/test/java/org/sequeless/filter/FunctionArgValidationTest.java index 1465fad..3f2b869 100644 --- a/src/test/java/org/sequeless/filter/FunctionArgValidationTest.java +++ b/src/test/java/org/sequeless/filter/FunctionArgValidationTest.java @@ -15,6 +15,7 @@ import org.sequeless.filter.api.ParameterType; import org.sequeless.filter.api.Syntax; import org.sequeless.filter.spi.OperatorContributor; +import org.sequeless.filter.testfixtures.PermissiveFieldRegistry; class FunctionArgValidationTest { @@ -42,7 +43,7 @@ void setUp() { OperatorContributor custom = () -> List.of(withinLast); ops = OperatorRegistry.of(List.of(custom)); - fields = FieldRegistry.permissive(); + fields = PermissiveFieldRegistry.INSTANCE; } @Test diff --git a/src/test/java/org/sequeless/filter/NormalizationTest.java b/src/test/java/org/sequeless/filter/NormalizationTest.java index 6144e44..393f00f 100644 --- a/src/test/java/org/sequeless/filter/NormalizationTest.java +++ b/src/test/java/org/sequeless/filter/NormalizationTest.java @@ -7,11 +7,12 @@ import org.sequeless.filter.api.FieldRegistry; import org.sequeless.filter.api.FilterParser; import org.sequeless.filter.api.OperatorRegistry; +import org.sequeless.filter.testfixtures.PermissiveFieldRegistry; class NormalizationTest { private static final OperatorRegistry OPS = OperatorRegistry.defaults(); - private static final FieldRegistry FIELDS = FieldRegistry.permissive(); + private static final FieldRegistry FIELDS = PermissiveFieldRegistry.INSTANCE; @Test void equalsAliasResolvesToIsCanonical() { diff --git a/src/main/java/org/sequeless/filter/internal/PermissiveFieldRegistry.java b/src/test/java/org/sequeless/filter/testfixtures/PermissiveFieldRegistry.java similarity index 66% rename from src/main/java/org/sequeless/filter/internal/PermissiveFieldRegistry.java rename to src/test/java/org/sequeless/filter/testfixtures/PermissiveFieldRegistry.java index 70784d6..2d84695 100644 --- a/src/main/java/org/sequeless/filter/internal/PermissiveFieldRegistry.java +++ b/src/test/java/org/sequeless/filter/testfixtures/PermissiveFieldRegistry.java @@ -1,4 +1,4 @@ -package org.sequeless.filter.internal; +package org.sequeless.filter.testfixtures; import java.util.List; import java.util.Optional; @@ -7,16 +7,14 @@ import org.sequeless.filter.api.OperatorRegistry; /** - * No-op {@link FieldRegistry} that accepts any well-formed dot-notation path. - * Field validation is disabled; {@link #compatibleWith} always returns an empty list. + * Test-only no-op {@link FieldRegistry} that accepts any well-formed dot-notation path by + * synthesizing a string-typed {@link FieldDefinition} for it. {@link #compatibleWith} always + * returns an empty list. */ public final class PermissiveFieldRegistry implements FieldRegistry { public static final PermissiveFieldRegistry INSTANCE = new PermissiveFieldRegistry(); - private static final FieldDefinition WILDCARD = - FieldDefinition.builder().path("*").jsonSchemaType("string").build(); - private PermissiveFieldRegistry() {} @Override @@ -34,9 +32,4 @@ public List all() { public List compatibleWith(String canonicalOperator, OperatorRegistry operators) { return List.of(); } - - @Override - public boolean isPermissive() { - return true; - } } From e9b890759f3447d31660df5c096ee07ebc63cb96 Mon Sep 17 00:00:00 2001 From: Ben B Date: Wed, 29 Jul 2026 21:55:09 +0000 Subject: [PATCH 03/11] feat: add derived value-shape signal and FilterValidator shape check (T2) OperatorDefinition gains a derived, three-state ValueShape (NONE for unary, LIST for FUNCTION-syntax or explicitly list-valued operators like "is in", SCALAR otherwise) rather than a plain boolean, since a scalar-defaulting boolean would wrongly reject every FUNCTION-syntax filter. FilterValidator now checks a FieldFilter/AnyFilter's value shape against its resolved operator's declared shape, alongside the existing applicableTypes/ permittedOperators checks, benefiting every FilterValidator consumer. --- .../multimodule-archunit-sql-adapter/plan.md | 2 +- .../sequeless/filter/api/FilterValidator.java | 34 +++- .../filter/api/OperatorDefinition.java | 24 +++ .../org/sequeless/filter/api/ValueShape.java | 11 ++ .../internal/BuiltinOperatorContributor.java | 12 +- .../sequeless/filter/FilterValidatorTest.java | 186 ++++++++++++++++++ 6 files changed, 266 insertions(+), 3 deletions(-) create mode 100644 src/main/java/org/sequeless/filter/api/ValueShape.java create mode 100644 src/test/java/org/sequeless/filter/FilterValidatorTest.java diff --git a/docs/specs/multimodule-archunit-sql-adapter/plan.md b/docs/specs/multimodule-archunit-sql-adapter/plan.md index 16445cf..505986c 100644 --- a/docs/specs/multimodule-archunit-sql-adapter/plan.md +++ b/docs/specs/multimodule-archunit-sql-adapter/plan.md @@ -647,7 +647,7 @@ no benefit while the version is shared. is expected to keep passing unmodified. One atomic commit, plain `fix:` type — per D1/D10, do not mark it a breaking change even though `isPermissive()` removal technically is one. `make verify` stays green. -- [ ] T2: (Phase 1) Give `OperatorDefinition` a **derived**, three-state value-shape signal (per +- [x] T2: (Phase 1) Give `OperatorDefinition` a **derived**, three-state value-shape signal (per **D17/D28**) — `NONE` when `isUnary()`, `LIST` when `getSyntax() == Syntax.FUNCTION`, `SCALAR` otherwise — and mark `is in` as the one `LIST`-shaped exception among the `INFIX` built-ins in `BuiltinOperatorContributor`. A plain scalar-default boolean is explicitly wrong here: it would diff --git a/src/main/java/org/sequeless/filter/api/FilterValidator.java b/src/main/java/org/sequeless/filter/api/FilterValidator.java index c8adc07..ca7f8a6 100644 --- a/src/main/java/org/sequeless/filter/api/FilterValidator.java +++ b/src/main/java/org/sequeless/filter/api/FilterValidator.java @@ -1,5 +1,6 @@ package org.sequeless.filter.api; +import com.fasterxml.jackson.databind.JsonNode; import java.util.ArrayList; import java.util.List; @@ -50,6 +51,8 @@ private static void validateField( return; } + checkValueShape(f.path(), f.op(), op, f.value(), violations); + fields.find(f.path()).ifPresent(fieldDef -> { if (!op.getApplicableTypes().isEmpty() && !op.getApplicableTypes().contains(fieldDef.getJsonSchemaType())) { violations.add(new FilterViolation( @@ -65,8 +68,37 @@ private static void validateField( } private static void validateAny(AnyFilter a, OperatorRegistry ops, List violations) { - if (ops.findByCanonicalOrAlias(a.op()).isEmpty()) { + OperatorDefinition op = ops.findByCanonicalOrAlias(a.op()).orElse(null); + if (op == null) { violations.add(new FilterViolation(null, "Unknown operator in AnyFilter: '" + a.op() + "'")); + return; + } + checkValueShape(null, a.op(), op, a.value(), violations); + } + + private static void checkValueShape( + String path, String opName, OperatorDefinition op, JsonNode value, List violations) { + switch (op.getValueShape()) { + case NONE -> { + if (value != null && !value.isNull()) { + violations.add(new FilterViolation(path, "Operator '" + opName + "' is unary and takes no value")); + } + } + case LIST -> { + if (value == null) { + violations.add(new FilterViolation(path, "Operator '" + opName + "' requires a value")); + } else if (!value.isArray()) { + violations.add(new FilterViolation(path, "Operator '" + opName + "' expects a list value")); + } + } + case SCALAR -> { + if (value == null) { + violations.add(new FilterViolation(path, "Operator '" + opName + "' requires a value")); + } else if (value.isArray()) { + violations.add( + new FilterViolation(path, "Operator '" + opName + "' expects a single value, not a list")); + } + } } } } diff --git a/src/main/java/org/sequeless/filter/api/OperatorDefinition.java b/src/main/java/org/sequeless/filter/api/OperatorDefinition.java index 819de5d..15485b0 100644 --- a/src/main/java/org/sequeless/filter/api/OperatorDefinition.java +++ b/src/main/java/org/sequeless/filter/api/OperatorDefinition.java @@ -41,10 +41,34 @@ public class OperatorDefinition { @Builder.Default boolean unary = false; + /** + * {@code true} for {@link Syntax#INFIX} operators whose right-hand side is a JSON array + * (only {@code is in} among the built-ins). Ignored for unary and {@link Syntax#FUNCTION} + * operators, whose shape follows from those flags — see {@link #getValueShape()}. + */ + @Builder.Default + boolean listValued = false; + /** Non-empty only for {@link Syntax#FUNCTION} operators. */ @Builder.Default List parameters = List.of(); + /** + * Derived value shape — never set directly. {@link ValueShape#NONE} when {@link #isUnary()}; + * {@link ValueShape#LIST} when {@link #getSyntax()} is {@link Syntax#FUNCTION} (function + * operators always carry an argument array, regardless of arity) or {@link #isListValued()}; + * {@link ValueShape#SCALAR} otherwise. Precedence: unary > function/list > scalar. + */ + public ValueShape getValueShape() { + if (unary) { + return ValueShape.NONE; + } + if (syntax == Syntax.FUNCTION || listValued) { + return ValueShape.LIST; + } + return ValueShape.SCALAR; + } + /** Defensive-copy overrides for the Lombok-generated builder setters. */ public static class OperatorDefinitionBuilder { diff --git a/src/main/java/org/sequeless/filter/api/ValueShape.java b/src/main/java/org/sequeless/filter/api/ValueShape.java new file mode 100644 index 0000000..a4e684c --- /dev/null +++ b/src/main/java/org/sequeless/filter/api/ValueShape.java @@ -0,0 +1,11 @@ +package org.sequeless.filter.api; + +/** The shape of value an operator expects on the right-hand side of a condition. */ +public enum ValueShape { + /** No value at all — unary operators such as {@code exists} / {@code does not exist}. */ + NONE, + /** Exactly one JSON scalar: string, number, boolean, or null. */ + SCALAR, + /** A JSON array — {@code is in}'s member list, or a {@link Syntax#FUNCTION} argument list. */ + LIST +} diff --git a/src/main/java/org/sequeless/filter/internal/BuiltinOperatorContributor.java b/src/main/java/org/sequeless/filter/internal/BuiltinOperatorContributor.java index 2930502..e2ec52d 100644 --- a/src/main/java/org/sequeless/filter/internal/BuiltinOperatorContributor.java +++ b/src/main/java/org/sequeless/filter/internal/BuiltinOperatorContributor.java @@ -25,7 +25,7 @@ public List operators() { op("starts with", List.of("starts_with"), STRING_TYPE), op("is like", List.of("like"), STRING_TYPE), op("is not like", List.of("not like"), STRING_TYPE), - op("is in", List.of("in"), ALL_TYPES), + listOp("is in", List.of("in"), ALL_TYPES), unary("exists", List.of(), ALL_TYPES), unary("does not exist", List.of("not exists"), ALL_TYPES)); } @@ -39,6 +39,16 @@ private static OperatorDefinition op(String canonical, List aliases, Lis .build(); } + private static OperatorDefinition listOp(String canonical, List aliases, List types) { + return OperatorDefinition.builder() + .canonicalName(canonical) + .aliases(aliases) + .applicableTypes(types) + .syntax(Syntax.INFIX) + .listValued(true) + .build(); + } + private static OperatorDefinition unary(String canonical, List aliases, List types) { return OperatorDefinition.builder() .canonicalName(canonical) diff --git a/src/test/java/org/sequeless/filter/FilterValidatorTest.java b/src/test/java/org/sequeless/filter/FilterValidatorTest.java new file mode 100644 index 0000000..27176ef --- /dev/null +++ b/src/test/java/org/sequeless/filter/FilterValidatorTest.java @@ -0,0 +1,186 @@ +package org.sequeless.filter; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.NullNode; +import com.fasterxml.jackson.databind.node.TextNode; +import java.util.List; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.sequeless.filter.api.AnyFilter; +import org.sequeless.filter.api.FieldDefinition; +import org.sequeless.filter.api.FieldFilter; +import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FieldRegistrySpec; +import org.sequeless.filter.api.FilterValidator; +import org.sequeless.filter.api.FilterViolation; +import org.sequeless.filter.api.Filters; +import org.sequeless.filter.api.OperatorDefinition; +import org.sequeless.filter.api.OperatorRegistry; +import org.sequeless.filter.api.ParameterDefinition; +import org.sequeless.filter.api.ParameterType; +import org.sequeless.filter.api.Syntax; +import org.sequeless.filter.spi.OperatorContributor; + +class FilterValidatorTest { + + private static final JsonNodeFactory JSON = JsonNodeFactory.instance; + + private OperatorRegistry ops; + private FieldRegistry fields; + + @BeforeEach + void setUp() { + FieldDefinition status = FieldDefinition.builder() + .path("status") + .jsonSchemaType("string") + .build(); + FieldDefinition age = + FieldDefinition.builder().path("age").jsonSchemaType("number").build(); + FieldDefinition deletedAt = FieldDefinition.builder() + .path("deletedAt") + .jsonSchemaType("string") + .build(); + FieldDefinition restricted = FieldDefinition.builder() + .path("restricted") + .jsonSchemaType("string") + .permittedOperators(List.of("is")) + .build(); + FieldDefinition createdAt = FieldDefinition.builder() + .path("createdAt") + .jsonSchemaType("string") + .jsonSchemaFormat("date-time") + .build(); + + fields = FieldRegistry.of(FieldRegistrySpec.builder() + .fields(List.of(status, age, deletedAt, restricted, createdAt)) + .build()); + ops = OperatorRegistry.defaults(); + } + + private static ArrayNode arrayOf(String... values) { + ArrayNode array = JSON.arrayNode(); + for (String value : values) { + array.add(value); + } + return array; + } + + @Test + void isInWithArrayValueIsValid() { + FieldFilter filter = Filters.field("status", "is in", arrayOf("a", "b")); + assertThat(FilterValidator.validate(filter, ops, fields)).isEmpty(); + } + + @Test + void isInWithScalarValueIsRejected() { + FieldFilter filter = Filters.field("status", "is in", TextNode.valueOf("open")); + List violations = FilterValidator.validate(filter, ops, fields); + assertThat(violations).hasSize(1); + assertThat(violations.get(0).message()).contains("expects a list value"); + } + + @Test + void isWithArrayValueIsRejected() { + FieldFilter filter = Filters.field("status", "is", arrayOf("a", "b")); + List violations = FilterValidator.validate(filter, ops, fields); + assertThat(violations).hasSize(1); + assertThat(violations.get(0).message()).contains("expects a single value"); + } + + @Test + void isWithNullValueIsValid() { + FieldFilter filter = Filters.field("deletedAt", "is", NullNode.getInstance()); + assertThat(FilterValidator.validate(filter, ops, fields)).isEmpty(); + } + + @Test + void unaryOperatorWithNoValueIsValid() { + FieldFilter filter = Filters.field("deletedAt", "exists"); + assertThat(FilterValidator.validate(filter, ops, fields)).isEmpty(); + } + + @Test + void unaryOperatorWithValueIsRejected() { + FieldFilter filter = new FieldFilter("deletedAt", "exists", TextNode.valueOf("x")); + List violations = FilterValidator.validate(filter, ops, fields); + assertThat(violations).hasSize(1); + assertThat(violations.get(0).message()).contains("takes no value"); + } + + @Test + void functionSyntaxOperatorWithArrayArgsIsValid() { + OperatorDefinition withinLast = OperatorDefinition.builder() + .canonicalName("withinLast") + .syntax(Syntax.FUNCTION) + .applicableTypes(List.of("string")) + .applicableFormats(List.of("date-time")) + .parameters(List.of( + ParameterDefinition.builder() + .name("amount") + .type(ParameterType.INT) + .build(), + ParameterDefinition.builder() + .name("unit") + .type(ParameterType.ENUM) + .allowedValues(List.of("days", "hours", "minutes", "seconds")) + .build())) + .build(); + OperatorContributor custom = () -> List.of(withinLast); + OperatorRegistry customOps = OperatorRegistry.of(List.of(custom)); + + ArrayNode args = JSON.arrayNode(); + args.add(7); + args.add("days"); + FieldFilter filter = Filters.field("createdAt", "withinLast", args); + + assertThat(FilterValidator.validate(filter, customOps, fields)).isEmpty(); + } + + @Test + void anyWithArrayValueForIsInIsValid() { + AnyFilter filter = Filters.any("is in", arrayOf("a", "b")); + assertThat(FilterValidator.validate(filter, ops, fields)).isEmpty(); + } + + @Test + void anyWithScalarValueForIsInIsRejectedWithNullPath() { + AnyFilter filter = Filters.any("is in", TextNode.valueOf("open")); + List violations = FilterValidator.validate(filter, ops, fields); + assertThat(violations).hasSize(1); + assertThat(violations.get(0).path()).isNull(); + assertThat(violations.get(0).message()).contains("expects a list value"); + } + + @Test + void unknownFieldIsRejected() { + FieldFilter filter = Filters.field("nope", "is", TextNode.valueOf("x")); + List violations = FilterValidator.validate(filter, ops, fields); + assertThat(violations).hasSize(1); + assertThat(violations.get(0).message()).contains("Unknown field"); + } + + @Test + void unknownOperatorIsRejected() { + FieldFilter filter = Filters.field("status", "nope", TextNode.valueOf("x")); + List violations = FilterValidator.validate(filter, ops, fields); + assertThat(violations).hasSize(1); + assertThat(violations.get(0).message()).contains("Unknown operator"); + } + + @Test + void operatorNotApplicableToTypeIsRejected() { + FieldFilter filter = Filters.field("age", "contains", TextNode.valueOf("5")); + List violations = FilterValidator.validate(filter, ops, fields); + assertThat(violations).anyMatch(v -> v.message().contains("not applicable to type")); + } + + @Test + void operatorNotPermittedForFieldIsRejected() { + FieldFilter filter = Filters.field("restricted", "is not", TextNode.valueOf("x")); + List violations = FilterValidator.validate(filter, ops, fields); + assertThat(violations).anyMatch(v -> v.message().contains("not permitted for field")); + } +} From 8c92c2237a7ff066db22418a82088014d8876976 Mon Sep 17 00:00:00 2001 From: Ben B Date: Thu, 30 Jul 2026 02:49:59 +0000 Subject: [PATCH 04/11] build: convert to a Maven reactor with a filter-core module (T3) --- .../multimodule-archunit-sql-adapter/plan.md | 2 +- filter-core/pom.xml | 38 ++++++++ .../filter/internal/parser/Filter.g4 | 0 .../org/sequeless/filter/api/AndFilter.java | 0 .../filter/api/AnyExpansionStrategy.java | 0 .../org/sequeless/filter/api/AnyFilter.java | 0 .../filter/api/AnyFilterExpander.java | 0 .../filter/api/CompletionContext.java | 0 .../filter/api/CompletionEngine.java | 0 .../sequeless/filter/api/CompletionHint.java | 0 .../filter/api/CompletionProvider.java | 0 .../sequeless/filter/api/CursorPosition.java | 0 .../sequeless/filter/api/FieldDefinition.java | 0 .../org/sequeless/filter/api/FieldFilter.java | 0 .../sequeless/filter/api/FieldRegistry.java | 0 .../filter/api/FieldRegistrySpec.java | 0 .../org/sequeless/filter/api/FilterNode.java | 0 .../filter/api/FilterParseException.java | 0 .../sequeless/filter/api/FilterParser.java | 0 .../filter/api/FilterSchemaProvider.java | 0 .../filter/api/FilterSerializer.java | 0 .../sequeless/filter/api/FilterValidator.java | 0 .../sequeless/filter/api/FilterViolation.java | 0 .../sequeless/filter/api/FilterVisitor.java | 0 .../org/sequeless/filter/api/Filters.java | 0 .../filter/api/OperatorDefinition.java | 0 .../filter/api/OperatorRegistry.java | 0 .../org/sequeless/filter/api/OrFilter.java | 0 .../filter/api/ParameterDefinition.java | 0 .../sequeless/filter/api/ParameterType.java | 0 .../org/sequeless/filter/api/ParseResult.java | 0 .../java/org/sequeless/filter/api/Syntax.java | 0 .../org/sequeless/filter/api/ValueShape.java | 0 .../internal/BuiltinOperatorContributor.java | 0 .../filter/internal/CursorAnalyzer.java | 0 .../filter/internal/DefaultFieldRegistry.java | 0 .../internal/DefaultOperatorRegistry.java | 0 .../internal/FilterBuildingVisitor.java | 0 .../filter/spi/FieldContributor.java | 0 .../filter/spi/OperatorContributor.java | 0 .../filter/AnyFilterExpanderTest.java | 0 .../sequeless/filter/BoundaryRulesTest.java | 0 .../filter/CompletionEngineTest.java | 0 .../sequeless/filter/FilterParserTest.java | 0 .../filter/FilterSchemaProviderTest.java | 0 .../filter/FilterSerializerTest.java | 0 .../sequeless/filter/FilterValidatorTest.java | 0 .../filter/FunctionArgValidationTest.java | 0 .../sequeless/filter/NormalizationTest.java | 0 .../filter/RegistryConflictTest.java | 0 .../testfixtures/PermissiveFieldRegistry.java | 0 pom.xml | 88 +++++++++++-------- 52 files changed, 89 insertions(+), 39 deletions(-) create mode 100644 filter-core/pom.xml rename {src => filter-core/src}/main/antlr4/org/sequeless/filter/internal/parser/Filter.g4 (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/AndFilter.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/AnyExpansionStrategy.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/AnyFilter.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/AnyFilterExpander.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/CompletionContext.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/CompletionEngine.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/CompletionHint.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/CompletionProvider.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/CursorPosition.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FieldDefinition.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FieldFilter.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FieldRegistry.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FieldRegistrySpec.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FilterNode.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FilterParseException.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FilterParser.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FilterSchemaProvider.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FilterSerializer.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FilterValidator.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FilterViolation.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/FilterVisitor.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/Filters.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/OperatorDefinition.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/OperatorRegistry.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/OrFilter.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/ParameterDefinition.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/ParameterType.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/ParseResult.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/Syntax.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/api/ValueShape.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/internal/BuiltinOperatorContributor.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/internal/CursorAnalyzer.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/internal/DefaultFieldRegistry.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/internal/DefaultOperatorRegistry.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/internal/FilterBuildingVisitor.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/spi/FieldContributor.java (100%) rename {src => filter-core/src}/main/java/org/sequeless/filter/spi/OperatorContributor.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/AnyFilterExpanderTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/BoundaryRulesTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/CompletionEngineTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/FilterParserTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/FilterSchemaProviderTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/FilterSerializerTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/FilterValidatorTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/FunctionArgValidationTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/NormalizationTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/RegistryConflictTest.java (100%) rename {src => filter-core/src}/test/java/org/sequeless/filter/testfixtures/PermissiveFieldRegistry.java (100%) diff --git a/docs/specs/multimodule-archunit-sql-adapter/plan.md b/docs/specs/multimodule-archunit-sql-adapter/plan.md index 505986c..3985fb4 100644 --- a/docs/specs/multimodule-archunit-sql-adapter/plan.md +++ b/docs/specs/multimodule-archunit-sql-adapter/plan.md @@ -660,7 +660,7 @@ no benefit while the version is shared. not just filter-sql-adapter (which additionally calls it directly per D27/T6, since nothing calls `FilterValidator` automatically). Separate commit from T1 (different concern, though both touch validator-adjacent code). -- [ ] T3: (Phase 2) Convert the root `pom.xml` to `pom` with +- [x] T3: (Phase 2) Convert the root `pom.xml` to `pom` with `filter-core`, hoisting the current plain `` (jackson-databind, antlr4-runtime, lombok, junit-jupiter, assertj-core, mockito-core, archunit-junit5 — there is no `` block today, so this is introducing one, diff --git a/filter-core/pom.xml b/filter-core/pom.xml new file mode 100644 index 0000000..f76922d --- /dev/null +++ b/filter-core/pom.xml @@ -0,0 +1,38 @@ + + + 4.0.0 + + + org.sequeless + sequeless-filter + 1.0.0 + ../pom.xml + + + filter-core + jar + + Sequeless Filter Core + Filter DSL core: AST model, ANTLR 4 grammar and parser, serializer, operator and + field registries, and completion utilities. + + + + com.fasterxml.jackson.core + jackson-databind + + + org.antlr + antlr4-runtime + + + + + + + org.codehaus.mojo + flatten-maven-plugin + + + + diff --git a/src/main/antlr4/org/sequeless/filter/internal/parser/Filter.g4 b/filter-core/src/main/antlr4/org/sequeless/filter/internal/parser/Filter.g4 similarity index 100% rename from src/main/antlr4/org/sequeless/filter/internal/parser/Filter.g4 rename to filter-core/src/main/antlr4/org/sequeless/filter/internal/parser/Filter.g4 diff --git a/src/main/java/org/sequeless/filter/api/AndFilter.java b/filter-core/src/main/java/org/sequeless/filter/api/AndFilter.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/AndFilter.java rename to filter-core/src/main/java/org/sequeless/filter/api/AndFilter.java diff --git a/src/main/java/org/sequeless/filter/api/AnyExpansionStrategy.java b/filter-core/src/main/java/org/sequeless/filter/api/AnyExpansionStrategy.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/AnyExpansionStrategy.java rename to filter-core/src/main/java/org/sequeless/filter/api/AnyExpansionStrategy.java diff --git a/src/main/java/org/sequeless/filter/api/AnyFilter.java b/filter-core/src/main/java/org/sequeless/filter/api/AnyFilter.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/AnyFilter.java rename to filter-core/src/main/java/org/sequeless/filter/api/AnyFilter.java diff --git a/src/main/java/org/sequeless/filter/api/AnyFilterExpander.java b/filter-core/src/main/java/org/sequeless/filter/api/AnyFilterExpander.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/AnyFilterExpander.java rename to filter-core/src/main/java/org/sequeless/filter/api/AnyFilterExpander.java diff --git a/src/main/java/org/sequeless/filter/api/CompletionContext.java b/filter-core/src/main/java/org/sequeless/filter/api/CompletionContext.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/CompletionContext.java rename to filter-core/src/main/java/org/sequeless/filter/api/CompletionContext.java diff --git a/src/main/java/org/sequeless/filter/api/CompletionEngine.java b/filter-core/src/main/java/org/sequeless/filter/api/CompletionEngine.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/CompletionEngine.java rename to filter-core/src/main/java/org/sequeless/filter/api/CompletionEngine.java diff --git a/src/main/java/org/sequeless/filter/api/CompletionHint.java b/filter-core/src/main/java/org/sequeless/filter/api/CompletionHint.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/CompletionHint.java rename to filter-core/src/main/java/org/sequeless/filter/api/CompletionHint.java diff --git a/src/main/java/org/sequeless/filter/api/CompletionProvider.java b/filter-core/src/main/java/org/sequeless/filter/api/CompletionProvider.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/CompletionProvider.java rename to filter-core/src/main/java/org/sequeless/filter/api/CompletionProvider.java diff --git a/src/main/java/org/sequeless/filter/api/CursorPosition.java b/filter-core/src/main/java/org/sequeless/filter/api/CursorPosition.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/CursorPosition.java rename to filter-core/src/main/java/org/sequeless/filter/api/CursorPosition.java diff --git a/src/main/java/org/sequeless/filter/api/FieldDefinition.java b/filter-core/src/main/java/org/sequeless/filter/api/FieldDefinition.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FieldDefinition.java rename to filter-core/src/main/java/org/sequeless/filter/api/FieldDefinition.java diff --git a/src/main/java/org/sequeless/filter/api/FieldFilter.java b/filter-core/src/main/java/org/sequeless/filter/api/FieldFilter.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FieldFilter.java rename to filter-core/src/main/java/org/sequeless/filter/api/FieldFilter.java diff --git a/src/main/java/org/sequeless/filter/api/FieldRegistry.java b/filter-core/src/main/java/org/sequeless/filter/api/FieldRegistry.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FieldRegistry.java rename to filter-core/src/main/java/org/sequeless/filter/api/FieldRegistry.java diff --git a/src/main/java/org/sequeless/filter/api/FieldRegistrySpec.java b/filter-core/src/main/java/org/sequeless/filter/api/FieldRegistrySpec.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FieldRegistrySpec.java rename to filter-core/src/main/java/org/sequeless/filter/api/FieldRegistrySpec.java diff --git a/src/main/java/org/sequeless/filter/api/FilterNode.java b/filter-core/src/main/java/org/sequeless/filter/api/FilterNode.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FilterNode.java rename to filter-core/src/main/java/org/sequeless/filter/api/FilterNode.java diff --git a/src/main/java/org/sequeless/filter/api/FilterParseException.java b/filter-core/src/main/java/org/sequeless/filter/api/FilterParseException.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FilterParseException.java rename to filter-core/src/main/java/org/sequeless/filter/api/FilterParseException.java diff --git a/src/main/java/org/sequeless/filter/api/FilterParser.java b/filter-core/src/main/java/org/sequeless/filter/api/FilterParser.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FilterParser.java rename to filter-core/src/main/java/org/sequeless/filter/api/FilterParser.java diff --git a/src/main/java/org/sequeless/filter/api/FilterSchemaProvider.java b/filter-core/src/main/java/org/sequeless/filter/api/FilterSchemaProvider.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FilterSchemaProvider.java rename to filter-core/src/main/java/org/sequeless/filter/api/FilterSchemaProvider.java diff --git a/src/main/java/org/sequeless/filter/api/FilterSerializer.java b/filter-core/src/main/java/org/sequeless/filter/api/FilterSerializer.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FilterSerializer.java rename to filter-core/src/main/java/org/sequeless/filter/api/FilterSerializer.java diff --git a/src/main/java/org/sequeless/filter/api/FilterValidator.java b/filter-core/src/main/java/org/sequeless/filter/api/FilterValidator.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FilterValidator.java rename to filter-core/src/main/java/org/sequeless/filter/api/FilterValidator.java diff --git a/src/main/java/org/sequeless/filter/api/FilterViolation.java b/filter-core/src/main/java/org/sequeless/filter/api/FilterViolation.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FilterViolation.java rename to filter-core/src/main/java/org/sequeless/filter/api/FilterViolation.java diff --git a/src/main/java/org/sequeless/filter/api/FilterVisitor.java b/filter-core/src/main/java/org/sequeless/filter/api/FilterVisitor.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/FilterVisitor.java rename to filter-core/src/main/java/org/sequeless/filter/api/FilterVisitor.java diff --git a/src/main/java/org/sequeless/filter/api/Filters.java b/filter-core/src/main/java/org/sequeless/filter/api/Filters.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/Filters.java rename to filter-core/src/main/java/org/sequeless/filter/api/Filters.java diff --git a/src/main/java/org/sequeless/filter/api/OperatorDefinition.java b/filter-core/src/main/java/org/sequeless/filter/api/OperatorDefinition.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/OperatorDefinition.java rename to filter-core/src/main/java/org/sequeless/filter/api/OperatorDefinition.java diff --git a/src/main/java/org/sequeless/filter/api/OperatorRegistry.java b/filter-core/src/main/java/org/sequeless/filter/api/OperatorRegistry.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/OperatorRegistry.java rename to filter-core/src/main/java/org/sequeless/filter/api/OperatorRegistry.java diff --git a/src/main/java/org/sequeless/filter/api/OrFilter.java b/filter-core/src/main/java/org/sequeless/filter/api/OrFilter.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/OrFilter.java rename to filter-core/src/main/java/org/sequeless/filter/api/OrFilter.java diff --git a/src/main/java/org/sequeless/filter/api/ParameterDefinition.java b/filter-core/src/main/java/org/sequeless/filter/api/ParameterDefinition.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/ParameterDefinition.java rename to filter-core/src/main/java/org/sequeless/filter/api/ParameterDefinition.java diff --git a/src/main/java/org/sequeless/filter/api/ParameterType.java b/filter-core/src/main/java/org/sequeless/filter/api/ParameterType.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/ParameterType.java rename to filter-core/src/main/java/org/sequeless/filter/api/ParameterType.java diff --git a/src/main/java/org/sequeless/filter/api/ParseResult.java b/filter-core/src/main/java/org/sequeless/filter/api/ParseResult.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/ParseResult.java rename to filter-core/src/main/java/org/sequeless/filter/api/ParseResult.java diff --git a/src/main/java/org/sequeless/filter/api/Syntax.java b/filter-core/src/main/java/org/sequeless/filter/api/Syntax.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/Syntax.java rename to filter-core/src/main/java/org/sequeless/filter/api/Syntax.java diff --git a/src/main/java/org/sequeless/filter/api/ValueShape.java b/filter-core/src/main/java/org/sequeless/filter/api/ValueShape.java similarity index 100% rename from src/main/java/org/sequeless/filter/api/ValueShape.java rename to filter-core/src/main/java/org/sequeless/filter/api/ValueShape.java diff --git a/src/main/java/org/sequeless/filter/internal/BuiltinOperatorContributor.java b/filter-core/src/main/java/org/sequeless/filter/internal/BuiltinOperatorContributor.java similarity index 100% rename from src/main/java/org/sequeless/filter/internal/BuiltinOperatorContributor.java rename to filter-core/src/main/java/org/sequeless/filter/internal/BuiltinOperatorContributor.java diff --git a/src/main/java/org/sequeless/filter/internal/CursorAnalyzer.java b/filter-core/src/main/java/org/sequeless/filter/internal/CursorAnalyzer.java similarity index 100% rename from src/main/java/org/sequeless/filter/internal/CursorAnalyzer.java rename to filter-core/src/main/java/org/sequeless/filter/internal/CursorAnalyzer.java diff --git a/src/main/java/org/sequeless/filter/internal/DefaultFieldRegistry.java b/filter-core/src/main/java/org/sequeless/filter/internal/DefaultFieldRegistry.java similarity index 100% rename from src/main/java/org/sequeless/filter/internal/DefaultFieldRegistry.java rename to filter-core/src/main/java/org/sequeless/filter/internal/DefaultFieldRegistry.java diff --git a/src/main/java/org/sequeless/filter/internal/DefaultOperatorRegistry.java b/filter-core/src/main/java/org/sequeless/filter/internal/DefaultOperatorRegistry.java similarity index 100% rename from src/main/java/org/sequeless/filter/internal/DefaultOperatorRegistry.java rename to filter-core/src/main/java/org/sequeless/filter/internal/DefaultOperatorRegistry.java diff --git a/src/main/java/org/sequeless/filter/internal/FilterBuildingVisitor.java b/filter-core/src/main/java/org/sequeless/filter/internal/FilterBuildingVisitor.java similarity index 100% rename from src/main/java/org/sequeless/filter/internal/FilterBuildingVisitor.java rename to filter-core/src/main/java/org/sequeless/filter/internal/FilterBuildingVisitor.java diff --git a/src/main/java/org/sequeless/filter/spi/FieldContributor.java b/filter-core/src/main/java/org/sequeless/filter/spi/FieldContributor.java similarity index 100% rename from src/main/java/org/sequeless/filter/spi/FieldContributor.java rename to filter-core/src/main/java/org/sequeless/filter/spi/FieldContributor.java diff --git a/src/main/java/org/sequeless/filter/spi/OperatorContributor.java b/filter-core/src/main/java/org/sequeless/filter/spi/OperatorContributor.java similarity index 100% rename from src/main/java/org/sequeless/filter/spi/OperatorContributor.java rename to filter-core/src/main/java/org/sequeless/filter/spi/OperatorContributor.java diff --git a/src/test/java/org/sequeless/filter/AnyFilterExpanderTest.java b/filter-core/src/test/java/org/sequeless/filter/AnyFilterExpanderTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/AnyFilterExpanderTest.java rename to filter-core/src/test/java/org/sequeless/filter/AnyFilterExpanderTest.java diff --git a/src/test/java/org/sequeless/filter/BoundaryRulesTest.java b/filter-core/src/test/java/org/sequeless/filter/BoundaryRulesTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/BoundaryRulesTest.java rename to filter-core/src/test/java/org/sequeless/filter/BoundaryRulesTest.java diff --git a/src/test/java/org/sequeless/filter/CompletionEngineTest.java b/filter-core/src/test/java/org/sequeless/filter/CompletionEngineTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/CompletionEngineTest.java rename to filter-core/src/test/java/org/sequeless/filter/CompletionEngineTest.java diff --git a/src/test/java/org/sequeless/filter/FilterParserTest.java b/filter-core/src/test/java/org/sequeless/filter/FilterParserTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/FilterParserTest.java rename to filter-core/src/test/java/org/sequeless/filter/FilterParserTest.java diff --git a/src/test/java/org/sequeless/filter/FilterSchemaProviderTest.java b/filter-core/src/test/java/org/sequeless/filter/FilterSchemaProviderTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/FilterSchemaProviderTest.java rename to filter-core/src/test/java/org/sequeless/filter/FilterSchemaProviderTest.java diff --git a/src/test/java/org/sequeless/filter/FilterSerializerTest.java b/filter-core/src/test/java/org/sequeless/filter/FilterSerializerTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/FilterSerializerTest.java rename to filter-core/src/test/java/org/sequeless/filter/FilterSerializerTest.java diff --git a/src/test/java/org/sequeless/filter/FilterValidatorTest.java b/filter-core/src/test/java/org/sequeless/filter/FilterValidatorTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/FilterValidatorTest.java rename to filter-core/src/test/java/org/sequeless/filter/FilterValidatorTest.java diff --git a/src/test/java/org/sequeless/filter/FunctionArgValidationTest.java b/filter-core/src/test/java/org/sequeless/filter/FunctionArgValidationTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/FunctionArgValidationTest.java rename to filter-core/src/test/java/org/sequeless/filter/FunctionArgValidationTest.java diff --git a/src/test/java/org/sequeless/filter/NormalizationTest.java b/filter-core/src/test/java/org/sequeless/filter/NormalizationTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/NormalizationTest.java rename to filter-core/src/test/java/org/sequeless/filter/NormalizationTest.java diff --git a/src/test/java/org/sequeless/filter/RegistryConflictTest.java b/filter-core/src/test/java/org/sequeless/filter/RegistryConflictTest.java similarity index 100% rename from src/test/java/org/sequeless/filter/RegistryConflictTest.java rename to filter-core/src/test/java/org/sequeless/filter/RegistryConflictTest.java diff --git a/src/test/java/org/sequeless/filter/testfixtures/PermissiveFieldRegistry.java b/filter-core/src/test/java/org/sequeless/filter/testfixtures/PermissiveFieldRegistry.java similarity index 100% rename from src/test/java/org/sequeless/filter/testfixtures/PermissiveFieldRegistry.java rename to filter-core/src/test/java/org/sequeless/filter/testfixtures/PermissiveFieldRegistry.java diff --git a/pom.xml b/pom.xml index f267643..8aebc5b 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.sequeless sequeless-filter 1.0.0 - jar + pom Sequeless Filter Human-readable filter/query DSL: AST model, ANTLR 4 grammar, parser, serializer, @@ -19,6 +19,10 @@ + + filter-core + + 21 21 @@ -48,6 +52,21 @@ ${project.build.directory}/jacoco.exec + + + + com.fasterxml.jackson.core + jackson-databind + ${jackson-databind.version} + + + org.antlr + antlr4-runtime + ${antlr4.version} + + + + org.projectlombok @@ -56,17 +75,6 @@ provided - - com.fasterxml.jackson.core - jackson-databind - ${jackson-databind.version} - - - org.antlr - antlr4-runtime - ${antlr4.version} - - org.junit.jupiter junit-jupiter @@ -94,6 +102,36 @@ + + + + org.codehaus.mojo + flatten-maven-plugin + ${flatten.version} + + true + ossrh + + + + flatten + + flatten + + process-resources + + + flatten-clean + + clean + + clean + + + + + + org.apache.maven.plugins @@ -285,32 +323,6 @@ - - - org.codehaus.mojo - flatten-maven-plugin - ${flatten.version} - - true - ossrh - - - - flatten - - flatten - - process-resources - - - flatten-clean - - clean - - clean - - - From c8a0c08081c1fbc49231cf3fa11afcbbf953d971 Mon Sep 17 00:00:00 2001 From: Ben B Date: Thu, 30 Jul 2026 02:52:27 +0000 Subject: [PATCH 05/11] build: reactor-proof release tooling and docs (T4) --- .releaserc.json | 4 +- CONTRIBUTING.md | 2 +- README.md | 12 +++- ...01-split-the-build-into-a-maven-reactor.md | 60 +++++++++++++++++++ .../multimodule-archunit-sql-adapter/plan.md | 2 +- 5 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 docs/adr/0001-split-the-build-into-a-maven-reactor.md diff --git a/.releaserc.json b/.releaserc.json index 62fa782..fabc687 100644 --- a/.releaserc.json +++ b/.releaserc.json @@ -7,14 +7,14 @@ [ "@semantic-release/exec", { - "prepareCmd": "./mvnw -B versions:set -DnewVersion=${nextRelease.version} -DgenerateBackupPoms=false" + "prepareCmd": "./mvnw -B versions:set -DnewVersion=${nextRelease.version} -DgenerateBackupPoms=false -DprocessAllModules=true" } ], "@semantic-release/github", [ "@semantic-release/git", { - "assets": ["CHANGELOG.md", "pom.xml"], + "assets": ["CHANGELOG.md", "pom.xml", "*/pom.xml"], "message": "chore(release): ${nextRelease.version} [skip ci]\n\n${nextRelease.notes}" } ] diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 48137ce..53c18e2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,4 +53,4 @@ the properties in `pom.xml`: ``` Set `jacoco.line.min` from the module's actually-measured coverage (see the report under -`target/site/jacoco/`), and ratchet it up over time rather than guessing a threshold now. +`filter-core/target/site/jacoco/`), and ratchet it up over time rather than guessing a threshold now. diff --git a/README.md b/README.md index f6eaec6..a0cdcee 100644 --- a/README.md +++ b/README.md @@ -23,11 +23,21 @@ Requires JDK 21. ```xml org.sequeless - sequeless-filter + filter-core 0.0.0-SNAPSHOT ``` +## Modules + +| Module | Coordinates | Contents | +| --- | --- | --- | +| [`filter-core`](filter-core) | `org.sequeless:filter-core` | The DSL: AST model, ANTLR 4 grammar and parser, serializer, operator and field registries, completion utilities. | + +The repository root is a `pom`-packaged aggregator/parent (`org.sequeless:sequeless-filter`) that +ships no code — it holds the single shared version, dependency management, and build plugins. All +modules are released together from that one version. + ## Build ```bash diff --git a/docs/adr/0001-split-the-build-into-a-maven-reactor.md b/docs/adr/0001-split-the-build-into-a-maven-reactor.md new file mode 100644 index 0000000..698187e --- /dev/null +++ b/docs/adr/0001-split-the-build-into-a-maven-reactor.md @@ -0,0 +1,60 @@ +# 0001. Split the build into a Maven reactor + +- Status: accepted +- Date: 2026-07-30 + +## Context + +`sequeless-filter` was a single `jar` module holding the entire filter +DSL: AST model, ANTLR 4 grammar/parser, serializer, operator and field registries, and +completion utilities. A SQL adapter (`FilterNode` AST → SQL) is arriving as a second module and +must not inherit the ANTLR runtime dependency it never uses — a plain single-jar layout has no +way to express "this dependency belongs to one consumer of the DSL, not the DSL itself." +Publishing does not exist yet for this project: there is no `` and no +`mvn deploy` step anywhere in CI, so there is no real external consumer whose published +coordinates would be broken by this restructuring. + +## Decision + +We will convert the root `pom.xml` into a `pom`-packaged aggregator/parent that keeps the +existing coordinates `org.sequeless:sequeless-filter`, and move all existing source under a new +`filter-core` module (`org.sequeless:filter-core`). The two modules share one +parent-inherited version — children declare no `` of their own. `jackson-databind` and +`antlr4-runtime` move to the parent's `` only, so each module must opt in +explicitly by redeclaring the dependency it actually needs (this is what will let the upcoming +`filter-sql-adapter` module avoid pulling in ANTLR). `lombok`, `junit-jupiter`, `assertj-core`, +`mockito-core`, and `archunit-junit5` stay as plain, parent-inherited ``, since +every module wants all five and redeclaring them per module would buy nothing. All build plugins +stay in the parent's inherited `` exactly as before, with one exception: +`flatten-maven-plugin` moves to `` with no `` element, and the +aggregator simply never declares/binds it in its own `` — each child declares it +bare (groupId + artifactId only) and inherits the full managed configuration. + +## Consequences + +Published coordinates change from `sequeless-filter` to `filter-core` for the actual DSL jar. +This is accepted and intentionally left unmarked as a breaking change in commit messages, since +nothing is published today and there is no real external consumer to break. + +`.releaserc.json` must now commit `*/pom.xml` (alongside the root `pom.xml`) as a release asset, +because `versions:set -DprocessAllModules=true` rewrites both the aggregator's `` and +each child's `` — omitting the child glob would leave the child POM's parent +reference stale after a release commit. + +JaCoCo coverage reports are now produced per module (e.g. `filter-core/target/site/jacoco/`) +with no aggregate report across the reactor. This is deliberate, not an oversight — a coverage +aggregator module is not needed until there is a concrete reason to view coverage across +modules in one report. + +The Sonar configuration in `sonar-project.properties` was deliberately left untouched by this +change. CI's SonarCloud job has never actually executed an analysis: every run to date has +logged "SONAR_TOKEN not set; skipping SonarCloud scan." and exited 0, so whether +`sonar-project.properties` (a SonarScanner-CLI-style file) is even read by the +Maven-integrated `sonar:sonar` goal CI invokes remains unconfirmed either way — there is no +"ANALYSIS SUCCESSFUL ... dashboard?id=..." log line to point at a working configuration to +preserve or fix. Speculatively migrating Sonar properties into the parent POM without that +confirmation risks guessing wrong about the project key or paths it depends on. Whoever enables +`SONAR_TOKEN` later must check the resulting analysis log's `dashboard?id=` line and then either +fix `sonar.coverage.jacoco.xmlReportPaths` to point at +`filter-core/target/site/jacoco/jacoco.xml`, or migrate the relevant Sonar properties into the +parent POM's `` and retire `sonar-project.properties` at that point. diff --git a/docs/specs/multimodule-archunit-sql-adapter/plan.md b/docs/specs/multimodule-archunit-sql-adapter/plan.md index 3985fb4..840f050 100644 --- a/docs/specs/multimodule-archunit-sql-adapter/plan.md +++ b/docs/specs/multimodule-archunit-sql-adapter/plan.md @@ -680,7 +680,7 @@ no benefit while the version is shared. its own ``; `git mv` `src/` and the ANTLR grammar under `filter-core/`. Verify `filter-core/target/generated-sources/antlr4/org/sequeless/filter/internal/parser/` is generated and all existing tests still run. -- [ ] T4: (Phase 2) Reactor-proof the surrounding tooling: `.releaserc.json` (`-DprocessAllModules=true`, +- [x] T4: (Phase 2) Reactor-proof the surrounding tooling: `.releaserc.json` (`-DprocessAllModules=true`, git assets `["CHANGELOG.md", "pom.xml", "*/pom.xml"]`), `README.md` dependency coordinates (`filter-core`, and a short module table), a `make verify` smoke check, and an ADR recording the split via `make new-adr`. Per **D25**: before touching Sonar config, verify empirically whether From ea19a989a815e27983147d664ae47a4f3c5bd5b0 Mon Sep 17 00:00:00 2001 From: Ben B Date: Thu, 30 Jul 2026 21:03:44 +0000 Subject: [PATCH 06/11] feat: add filter-sql-adapter module contracts (T5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire filter-sql-adapter into the reactor and define its public API/SPI contracts (sql.api.SqlFragment/ColumnMapping/FilterQueryTranslator/ SqlRenderException, sql.spi.QueryBuilder) per plan.md's Phase 3 design. Contracts and Javadoc only — no translation logic yet (T6). --- filter-sql-adapter/pom.xml | 39 +++++++++ .../filter/sql/api/ColumnMapping.java | 70 ++++++++++++++++ .../filter/sql/api/FilterQueryTranslator.java | 84 +++++++++++++++++++ .../sequeless/filter/sql/api/SqlFragment.java | 37 ++++++++ .../filter/sql/api/SqlRenderException.java | 23 +++++ .../filter/sql/spi/QueryBuilder.java | 74 ++++++++++++++++ pom.xml | 1 + 7 files changed, 328 insertions(+) create mode 100644 filter-sql-adapter/pom.xml create mode 100644 filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/ColumnMapping.java create mode 100644 filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/FilterQueryTranslator.java create mode 100644 filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/SqlFragment.java create mode 100644 filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/SqlRenderException.java create mode 100644 filter-sql-adapter/src/main/java/org/sequeless/filter/sql/spi/QueryBuilder.java diff --git a/filter-sql-adapter/pom.xml b/filter-sql-adapter/pom.xml new file mode 100644 index 0000000..0ed3b3e --- /dev/null +++ b/filter-sql-adapter/pom.xml @@ -0,0 +1,39 @@ + + + 4.0.0 + + + org.sequeless + sequeless-filter + 1.0.0 + ../pom.xml + + + filter-sql-adapter + jar + + Sequeless Filter SQL Adapter + Translates parsed filter ASTs into vendor SQL via a hexagonal QueryBuilder port, + with a Template Method ANSI default implementation. + + + + org.sequeless + filter-core + ${project.version} + + + com.fasterxml.jackson.core + jackson-databind + + + + + + + org.codehaus.mojo + flatten-maven-plugin + + + + diff --git a/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/ColumnMapping.java b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/ColumnMapping.java new file mode 100644 index 0000000..f13e59b --- /dev/null +++ b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/ColumnMapping.java @@ -0,0 +1,70 @@ +package org.sequeless.filter.sql.api; + +import java.util.Map; +import java.util.Optional; + +/** + * Resolves a filter's dot-notation field path to a SQL column name. + * + *

Scope limit: this contract supports only flat, single-table column + * mappings — a {@code columnFor} implementation returns a bare column name, and + * {@link SqlFragment} represents a single {@code WHERE}-fragment plus its binds. There is no way + * to express a join, a correlated subquery, or a JSON path expression. This matters because the + * DSL's own headline example is a nested path ({@code lineItems.qty > 5}), which on a typical + * relational schema means a join, not a column — translating such a path requires a caller-supplied + * {@link #of(Map)} mapping onto a view or denormalized column that already exists; there is no + * built-in way to synthesize the join itself. + */ +@FunctionalInterface +public interface ColumnMapping { + + /** + * Resolves {@code path} to a column name. + * + * @param path dot-notation field path, as it appears in the filter AST + * @return the column name to use in generated SQL, or empty if {@code path} has no mapping + */ + Optional columnFor(String path); + + /** + * A mapping under which every path resolves to a column of the same name. + * + *

Trusted input only. This performs no validation beyond the dotted-path + * check below: the resolved path is placed directly in SQL identifier position, subject only + * to whatever quoting the {@link org.sequeless.filter.sql.spi.QueryBuilder} implementation + * applies. It is intended for paths the caller already controls (e.g. field names drawn from + * a fixed, developer-authored {@link org.sequeless.filter.api.FieldRegistry}) — never for + * exposing arbitrary or untrusted input as column names. Callers whose filter paths may + * originate from untrusted or deserialized input must use {@link #of(Map)} instead, which acts + * as an explicit allowlist. + * + *

Because this contract only supports flat, single-table mappings (see the type-level + * Javadoc), any path containing {@code .} resolves to {@link Optional#empty()} rather than a + * well-formed but nonexistent quoted identifier such as {@code "lineItems.qty"} — this fails + * fast as {@link SqlRenderException} at translation time instead of failing later as a + * vendor-specific "column does not exist" error at execution time. A caller with a real column + * for a dotted path (e.g. backed by a joined view) can still map it explicitly via + * {@link #of(Map)}, which is unaffected by this restriction. + * + * @return an identity column mapping + */ + static ColumnMapping identity() { + return path -> path.indexOf('.') >= 0 ? Optional.empty() : Optional.of(path); + } + + /** + * A mapping backed by an explicit path-to-column allowlist. + * + *

Safe for untrusted paths: any path not present as a key in {@code mapping} resolves to + * {@link Optional#empty()}. A dotted path is permitted here if the caller has deliberately + * mapped it — e.g. onto a real column exposed by a joined view — since the caller controls the + * mapping's contents directly, unlike {@link #identity()}. + * + * @param mapping explicit field-path-to-column-name allowlist + * @return a mapping backed by {@code mapping} + */ + static ColumnMapping of(Map mapping) { + Map copy = Map.copyOf(mapping); + return path -> Optional.ofNullable(copy.get(path)); + } +} diff --git a/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/FilterQueryTranslator.java b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/FilterQueryTranslator.java new file mode 100644 index 0000000..b7840ea --- /dev/null +++ b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/FilterQueryTranslator.java @@ -0,0 +1,84 @@ +package org.sequeless.filter.sql.api; + +import org.sequeless.filter.api.AnyFilterExpander; +import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FilterNode; +import org.sequeless.filter.api.FilterValidator; +import org.sequeless.filter.api.FilterVisitor; +import org.sequeless.filter.api.OperatorRegistry; +import org.sequeless.filter.sql.spi.QueryBuilder; + +/** + * Translates a validated {@link FilterNode} AST into a single {@link SqlFragment}, delegating + * every vendor-specific rendering decision to an injected {@link QueryBuilder}. + * + *

This is the vendor-agnostic core of the hexagonal design: it walks the tree (as a + * {@link FilterVisitor}), resolves each field path via a {@link ColumnMapping}, converts each + * {@link com.fasterxml.jackson.databind.JsonNode} value to a plain Java value, and hands the + * result to the supplied {@link QueryBuilder} port — but never renders SQL text itself. + * + *

{@link #translate} performs, in order: + * + *

    + *
  1. Validates {@code node} against {@code fields}/{@code ops} via + * {@link FilterValidator#validate}. {@code translate} does not assume its input was + * validated upstream — nothing else in this library calls {@code FilterValidator} + * automatically, so this is the only place that guarantee is actually enforced for callers + * of the SQL adapter. Any reported violation is converted to a {@link SqlRenderException}. + *
  2. Walks the tree. For a {@link org.sequeless.filter.api.FieldFilter}, resolves + * {@code path} via {@code columns}, raising {@link SqlRenderException} if unmapped, converts + * {@code value} to a plain Java object, and dispatches to {@link QueryBuilder#binary} or + * {@link QueryBuilder#unary} — by the resolved operator's + * {@link org.sequeless.filter.api.OperatorDefinition#isUnary()}, never by whether + * the converted value is {@code null} (a null-valued binary condition, e.g. + * {@code status is null}, and a genuinely unary condition are otherwise indistinguishable + * after conversion). + *
  3. For an {@link org.sequeless.filter.api.AnyFilter}, expands it in place — inside the visit + * for that node, not via one whole-tree {@link AnyFilterExpander#expand(FilterNode, + * FieldRegistry, OperatorRegistry)} call up front — using {@code AnyFilterExpander}'s default + * expansion strategy. Because the expansion happens exactly where the {@code AnyFilter} node + * is visited, the resulting operands are known at that point to be expansion-derived; any + * operand whose path has no {@code ColumnMapping} entry is silently dropped rather than + * failing the whole filter, and {@link SqlRenderException} is raised only if none of the + * expanded operands survive. An explicitly-named path at any other tree position is + * unaffected by this leniency — it still hard-fails on an unmapped column. + *
  4. For an {@link org.sequeless.filter.api.AndFilter}/{@link org.sequeless.filter.api.OrFilter} + * at any depth, rejects a zero-operand result with {@link SqlRenderException} (this also + * catches an {@code any}-expansion left empty by the previous step's drop-unmapped rule) + * before delegating to {@link QueryBuilder#and}/{@link QueryBuilder#or}. + *
  5. Rejects, with {@link SqlRenderException}, any list-shaped operator (per + * {@link org.sequeless.filter.api.OperatorDefinition#getValueShape()} — not the literal + * operator name {@code is in}) whose converted value is an empty list. + *
+ * + *

Custom {@code any} expansion is out of reach here. This translator always + * uses {@code AnyFilterExpander}'s default strategy; a caller needing a non-default + * {@link org.sequeless.filter.api.AnyExpansionStrategy} must pre-expand the filter with their own + * strategy before calling {@link #translate}. + * + *

Implementations are expected to be stateless and thread-safe, the same as + * {@link QueryBuilder} — callers will naturally hold a single instance as a long-lived singleton. + */ +public interface FilterQueryTranslator { + + /** + * Translates {@code node} into a single SQL fragment. + * + * @param node the filter AST to translate; may be untrusted or deserialized input — this + * method validates it itself and does not assume it was validated upstream + * @param columns resolves field paths to SQL column names + * @param fields field registry used both for {@link FilterValidator} and for + * {@code any}-expansion's field-compatibility lookup + * @param ops operator registry used both for {@link FilterValidator} and for resolving + * each node's operator + * @param builder vendor port that renders the resolved columns/operators/values to SQL + * @return the rendered fragment + * @throws SqlRenderException if {@code node} fails validation, references an unmapped column + * outside an {@code any}-expansion, contains a zero-operand + * {@code and}/{@code or} at any depth, supplies an empty list to a + * list-shaped operator, or contains a value of an unrecognized + * {@code JsonNode} subtype + */ + SqlFragment translate( + FilterNode node, ColumnMapping columns, FieldRegistry fields, OperatorRegistry ops, QueryBuilder builder); +} diff --git a/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/SqlFragment.java b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/SqlFragment.java new file mode 100644 index 0000000..a314dcc --- /dev/null +++ b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/SqlFragment.java @@ -0,0 +1,37 @@ +package org.sequeless.filter.sql.api; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * A rendered piece of SQL together with its positional bind parameters. + * + *

{@code sql} contains {@code ?} placeholders, in the same left-to-right order as the entries + * in {@code parameters} — the classic JDBC {@code PreparedStatement} convention. A fragment may + * represent a single condition (e.g. {@code "status" = ?}) or a combination of other fragments + * (e.g. {@code ("status" = ? AND "region" = ?)}); {@link org.sequeless.filter.sql.spi.QueryBuilder} + * is responsible for keeping the two lists aligned when it combines fragments. + * + * @param sql SQL text with positional {@code ?} placeholders + * @param parameters bind values, one per placeholder in {@code sql}, in order + */ +public record SqlFragment(String sql, List parameters) { + + /** + * Defensively copies {@code parameters}. + * + *

Deliberately uses {@link Collections#unmodifiableList(List)} over a fresh + * {@link ArrayList} rather than this codebase's usual {@code List.copyOf} idiom: a + * value-converted filter can legitimately contain a {@code null} element (e.g. {@code is in + * ['a', null]}), and {@code List.copyOf} throws {@link NullPointerException} on a {@code null} + * element. Using the house idiom here would turn an otherwise-legal filter into a + * construction-time crash. + */ + public SqlFragment { + Objects.requireNonNull(sql, "sql"); + Objects.requireNonNull(parameters, "parameters"); + parameters = Collections.unmodifiableList(new ArrayList<>(parameters)); + } +} diff --git a/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/SqlRenderException.java b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/SqlRenderException.java new file mode 100644 index 0000000..46194d2 --- /dev/null +++ b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/SqlRenderException.java @@ -0,0 +1,23 @@ +package org.sequeless.filter.sql.api; + +/** + * Thrown by {@link FilterQueryTranslator#translate} when a {@link org.sequeless.filter.api.FilterNode} + * cannot be translated to SQL. + * + *

Raised for: a {@link org.sequeless.filter.api.FilterValidator} violation detected on entry; + * an explicitly-named field path with no {@link ColumnMapping} entry; a zero-operand + * {@link org.sequeless.filter.api.AndFilter}/{@link org.sequeless.filter.api.OrFilter} encountered + * at any depth (including one left empty after dropping unmapped {@code any}-expansion operands); + * an empty converted value for a list-shaped operator (e.g. {@code is in []}); or a + * {@link com.fasterxml.jackson.databind.JsonNode} value of an unrecognized subtype. + */ +public class SqlRenderException extends RuntimeException { + + public SqlRenderException(String message) { + super(message); + } + + public SqlRenderException(String message, Throwable cause) { + super(message, cause); + } +} diff --git a/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/spi/QueryBuilder.java b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/spi/QueryBuilder.java new file mode 100644 index 0000000..8f22be6 --- /dev/null +++ b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/spi/QueryBuilder.java @@ -0,0 +1,74 @@ +package org.sequeless.filter.sql.spi; + +import java.util.List; +import org.sequeless.filter.api.OperatorDefinition; +import org.sequeless.filter.sql.api.FilterQueryTranslator; +import org.sequeless.filter.sql.api.SqlFragment; + +/** + * Vendor port for rendering filter conditions to SQL. + * + *

This is the hexagonal boundary between {@link FilterQueryTranslator} — which walks the + * {@link org.sequeless.filter.api.FilterNode} tree and resolves paths/values but knows nothing + * about any particular SQL dialect — and a concrete vendor implementation, which owns full + * construction of each returned {@link SqlFragment}: identifier quoting, operator-to-SQL + * translation (including {@code is in}'s list expansion), value binding, and AND/OR combination. + * The port is deliberately fixed to producing {@link SqlFragment} rather than a generic + * {@code QueryBuilder} — every implementation renders raw, parameterized SQL text. + * + *

Each method receives the already-resolved column name and the already-converted Java value + * (see {@link FilterQueryTranslator}'s value-conversion behavior) — never the raw + * {@link com.fasterxml.jackson.databind.JsonNode} or unresolved field path. A {@code null} passed + * as {@code value} to {@link #binary} means SQL {@code NULL} (e.g. from {@code status is null}, + * i.e. a {@code null}-converted {@link com.fasterxml.jackson.databind.node.NullNode}), not "no + * value" — dispatch between {@link #binary} and {@link #unary} is by + * {@link OperatorDefinition#isUnary()}, never by whether {@code value} is {@code null}. + * + *

Implementations are expected to be stateless and thread-safe. Nothing in + * this contract enforces that, but callers will naturally hold a single instance as a long-lived + * singleton shared across concurrent request threads. + */ +public interface QueryBuilder { + + /** + * Renders a binary condition: a column, an operator, and a value. + * + *

{@code value} is the already-converted Java value — a {@link String}, a boxed + * {@link Number}, a {@link Boolean}, {@code null} (meaning SQL {@code NULL}), or a + * {@code List} for a list-shaped operator such as {@code is in}. + * + * @param column the resolved SQL column name + * @param operator the operator being applied; never one for which {@link OperatorDefinition#isUnary()} is true + * @param value the already-converted value (never itself a raw {@code JsonNode}) + * @return the rendered fragment + */ + SqlFragment binary(String column, OperatorDefinition operator, Object value); + + /** + * Renders a unary condition: a column and an operator that takes no value (e.g. {@code exists} + * / {@code does not exist}). + * + * @param column the resolved SQL column name + * @param operator the operator being applied; always one for which {@link OperatorDefinition#isUnary()} is true + * @return the rendered fragment + */ + SqlFragment unary(String column, OperatorDefinition operator); + + /** + * Combines fragments with SQL {@code AND}. + * + * @param operands the fragments to combine; never empty (the translator rejects any + * zero-operand conjunction before it would reach this method) + * @return the combined fragment + */ + SqlFragment and(List operands); + + /** + * Combines fragments with SQL {@code OR}. + * + * @param operands the fragments to combine; never empty (the translator rejects any + * zero-operand disjunction before it would reach this method) + * @return the combined fragment + */ + SqlFragment or(List operands); +} diff --git a/pom.xml b/pom.xml index 8aebc5b..ed5e06c 100644 --- a/pom.xml +++ b/pom.xml @@ -21,6 +21,7 @@ filter-core + filter-sql-adapter From 49f6b5a832744126a6d9d0798e2d5c40e7514131 Mon Sep 17 00:00:00 2001 From: Ben B Date: Thu, 30 Jul 2026 21:09:34 +0000 Subject: [PATCH 07/11] feat: implement AnsiQueryBuilder and FilterQueryTranslator (T6) Adds sql.spi.AnsiQueryBuilder, a public Template Method QueryBuilder implementation whose final binary()/unary() dispatchers delegate to 8 protected hooks covering all 13 built-in operators plus a renderUnknown/unaryUnknown fallback pair for custom operators (D16), with a security-relevant mandated quoteIdentifier escaping contract (D23). Adds sql.api.DefaultFilterQueryTranslator, the concrete FilterQueryTranslator implementation: validates via FilterValidator on entry (D27), converts JsonNode values per D7/D19's mapping (including the catch-all for unrecognized subtypes), expands AnyFilter per-node in visitAny while dropping unmapped operands (D18/D29), rejects zero-operand And/OrFilter at any depth and empty list-shaped values (D11/D22/D30), and dispatches binary()/unary() by operator.isUnary() rather than by null value (D33). Adds unit tests covering all 13 built-in operators' SQL/bind-parameter output, a custom-operator renderUnknown fallback (with and without an override), the status-is-null vs. exists dispatch distinction, and the empty and/or / empty-list / any-expansion drop-unmapped edge cases. --- .../sql/api/DefaultFilterQueryTranslator.java | 164 ++++++++ .../filter/sql/spi/AnsiQueryBuilder.java | 246 ++++++++++++ .../sql/DefaultFilterQueryTranslatorTest.java | 373 ++++++++++++++++++ 3 files changed, 783 insertions(+) create mode 100644 filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/DefaultFilterQueryTranslator.java create mode 100644 filter-sql-adapter/src/main/java/org/sequeless/filter/sql/spi/AnsiQueryBuilder.java create mode 100644 filter-sql-adapter/src/test/java/org/sequeless/filter/sql/DefaultFilterQueryTranslatorTest.java diff --git a/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/DefaultFilterQueryTranslator.java b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/DefaultFilterQueryTranslator.java new file mode 100644 index 0000000..c0de285 --- /dev/null +++ b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/api/DefaultFilterQueryTranslator.java @@ -0,0 +1,164 @@ +package org.sequeless.filter.sql.api; + +import com.fasterxml.jackson.databind.JsonNode; +import java.util.ArrayList; +import java.util.List; +import org.sequeless.filter.api.AndFilter; +import org.sequeless.filter.api.AnyFilter; +import org.sequeless.filter.api.AnyFilterExpander; +import org.sequeless.filter.api.FieldFilter; +import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FilterNode; +import org.sequeless.filter.api.FilterValidator; +import org.sequeless.filter.api.FilterViolation; +import org.sequeless.filter.api.FilterVisitor; +import org.sequeless.filter.api.OperatorDefinition; +import org.sequeless.filter.api.OperatorRegistry; +import org.sequeless.filter.api.OrFilter; +import org.sequeless.filter.api.ValueShape; +import org.sequeless.filter.sql.spi.QueryBuilder; + +/** + * The sole shipped {@link FilterQueryTranslator} implementation. + * + *

Stateless and thread-safe: a fresh {@link Visitor} is constructed per {@link #translate} + * call, closing over that call's {@code columns}/{@code fields}/{@code ops}/{@code builder} + * arguments, so a single {@code DefaultFilterQueryTranslator} instance may safely be shared and + * reused across concurrent callers. + */ +public final class DefaultFilterQueryTranslator implements FilterQueryTranslator { + + @Override + public SqlFragment translate( + FilterNode node, ColumnMapping columns, FieldRegistry fields, OperatorRegistry ops, QueryBuilder builder) { + List violations = FilterValidator.validate(node, ops, fields); + if (!violations.isEmpty()) { + throw new SqlRenderException("Filter failed validation: " + violations); + } + return new Visitor(columns, fields, ops, builder).walk(node); + } + + /** + * Per-call tree walker. Not reused across {@link #translate} calls, so it may safely hold the + * call's arguments as fields without introducing shared mutable state. + */ + private static final class Visitor extends FilterVisitor { + + private final ColumnMapping columns; + private final FieldRegistry fields; + private final OperatorRegistry ops; + private final QueryBuilder builder; + + Visitor(ColumnMapping columns, FieldRegistry fields, OperatorRegistry ops, QueryBuilder builder) { + this.columns = columns; + this.fields = fields; + this.ops = ops; + this.builder = builder; + } + + @Override + public SqlFragment visitField(FieldFilter node) { + OperatorDefinition operator = resolveOperator(node.op()); + String column = resolveColumn(node.path()); + return renderCondition(column, operator, node.value()); + } + + @Override + public SqlFragment visitAny(AnyFilter node) { + FilterNode expanded = AnyFilterExpander.expand(node, fields, ops); + List candidateOperands = + switch (expanded) { + case OrFilter o -> o.operands(); + case AndFilter a -> a.operands(); + default -> List.of(expanded); + }; + + List survivors = new ArrayList<>(); + for (FilterNode candidate : candidateOperands) { + if (candidate instanceof FieldFilter f + && columns.columnFor(f.path()).isEmpty()) { + continue; + } + survivors.add(walk(candidate)); + } + + if (survivors.isEmpty()) { + throw new SqlRenderException( + "'any " + node.op() + "' expansion produced no operands mapped by the supplied ColumnMapping"); + } + return builder.or(survivors); + } + + @Override + public SqlFragment visitAnd(AndFilter node) { + if (node.operands().isEmpty()) { + throw new SqlRenderException("'and' has no operands"); + } + List rendered = + node.operands().stream().map(this::walk).toList(); + return builder.and(rendered); + } + + @Override + public SqlFragment visitOr(OrFilter node) { + if (node.operands().isEmpty()) { + throw new SqlRenderException("'or' has no operands"); + } + List rendered = + node.operands().stream().map(this::walk).toList(); + return builder.or(rendered); + } + + private SqlFragment renderCondition(String column, OperatorDefinition operator, JsonNode rawValue) { + if (operator.isUnary()) { + return builder.unary(column, operator); + } + Object value = convertValue(rawValue); + if (operator.getValueShape() == ValueShape.LIST && value instanceof List list && list.isEmpty()) { + throw new SqlRenderException("Operator '" + operator.getCanonicalName() + "' on column '" + column + + "' was given an empty list value"); + } + return builder.binary(column, operator, value); + } + + private OperatorDefinition resolveOperator(String opName) { + return ops.findByCanonicalOrAlias(opName) + .orElseThrow(() -> new SqlRenderException("Unknown operator: '" + opName + "'")); + } + + private String resolveColumn(String path) { + return columns.columnFor(path) + .orElseThrow(() -> new SqlRenderException("Unmapped column for path: '" + path + "'")); + } + + private static Object convertValue(JsonNode node) { + if (node == null || node.isNull()) { + return null; + } + if (node.isTextual()) { + return node.asText(); + } + if (node.isInt()) { + return node.intValue(); + } + if (node.isLong()) { + return node.longValue(); + } + if (node.isDouble()) { + return node.doubleValue(); + } + if (node.isBoolean()) { + return node.booleanValue(); + } + if (node.isArray()) { + List values = new ArrayList<>(); + node.forEach(element -> values.add(convertValue(element))); + return values; + } + if (node.isNumber()) { + return node.numberValue(); + } + throw new SqlRenderException("Unsupported JSON value type: " + node.getNodeType()); + } + } +} diff --git a/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/spi/AnsiQueryBuilder.java b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/spi/AnsiQueryBuilder.java new file mode 100644 index 0000000..b4acc19 --- /dev/null +++ b/filter-sql-adapter/src/main/java/org/sequeless/filter/sql/spi/AnsiQueryBuilder.java @@ -0,0 +1,246 @@ +package org.sequeless.filter.sql.spi; + +import java.util.ArrayList; +import java.util.List; +import org.sequeless.filter.api.OperatorDefinition; +import org.sequeless.filter.sql.api.SqlFragment; +import org.sequeless.filter.sql.api.SqlRenderException; + +/** + * ANSI SQL default {@link QueryBuilder} implementation, structured as a Template Method. + * + *

{@link #binary} and {@link #unary} are {@code final} dispatchers: they inspect the + * operator's canonical name and delegate to one of eight {@code protected} hooks, each owning one + * rendering concern. A vendor that needs to change only one concern — say, {@code LIKE} escaping, + * or identifier quoting — extends this class and overrides just that hook, rather than + * reimplementing the full 13-operator dispatch from scratch. + * + *

This is the complete, fixed hook set — not a starting point meant to accrete further + * overloads: + * + *

    + *
  • {@link #quoteIdentifier(String)} + *
  • {@link #renderIs(String, boolean, Object)} + *
  • {@link #renderComparison(String, OperatorDefinition, Object)} + *
  • {@link #renderLike(String, OperatorDefinition, Object)} + *
  • {@link #renderIn(String, List)} + *
  • {@link #renderExists(String, boolean)} + *
  • {@link #renderUnknown(String, OperatorDefinition, Object)} + *
  • {@link #unaryUnknown(String, OperatorDefinition)} + *
+ * + *

Implementations are expected to be stateless and thread-safe — this class + * holds no mutable state, and subclasses should preserve that. + */ +public class AnsiQueryBuilder implements QueryBuilder { + + @Override + public final SqlFragment binary(String column, OperatorDefinition operator, Object value) { + String quoted = quoteIdentifier(column); + return switch (operator.getCanonicalName()) { + case "is" -> renderIs(quoted, false, value); + case "is not" -> renderIs(quoted, true, value); + case "is greater than", + "is greater than or equal to", + "is less than", + "is less than or equal to" -> renderComparison(quoted, operator, value); + case "contains", "starts with", "is like", "is not like" -> renderLike(quoted, operator, value); + case "is in" -> renderIn(quoted, toValueList(column, operator, value)); + default -> renderUnknown(quoted, operator, value); + }; + } + + @Override + public final SqlFragment unary(String column, OperatorDefinition operator) { + String quoted = quoteIdentifier(column); + return switch (operator.getCanonicalName()) { + case "exists" -> renderExists(quoted, false); + case "does not exist" -> renderExists(quoted, true); + default -> unaryUnknown(quoted, operator); + }; + } + + @SuppressWarnings("unchecked") + private static List toValueList(String column, OperatorDefinition operator, Object value) { + if (value instanceof List list) { + return (List) list; + } + throw new SqlRenderException( + "Operator '" + operator.getCanonicalName() + "' on column '" + column + "' expects a list value"); + } + + @Override + public SqlFragment and(List operands) { + return combine(operands, "AND"); + } + + @Override + public SqlFragment or(List operands) { + return combine(operands, "OR"); + } + + private static SqlFragment combine(List operands, String connective) { + StringBuilder sql = new StringBuilder("("); + List parameters = new ArrayList<>(); + for (int i = 0; i < operands.size(); i++) { + if (i > 0) { + sql.append(' ').append(connective).append(' '); + } + SqlFragment operand = operands.get(i); + sql.append(operand.sql()); + parameters.addAll(operand.parameters()); + } + sql.append(')'); + return new SqlFragment(sql.toString(), parameters); + } + + /** + * Quotes {@code column} as an ANSI SQL delimited identifier. + * + *

Security-relevant. Emits {@code "…"} with any embedded {@code "} + * doubled. This is the only defense between a resolved column name and SQL injection when + * that name did not originate from the grammar's own identifier token (e.g. a + * programmatically or deserialization-supplied {@link org.sequeless.filter.sql.api.ColumnMapping}). + * A subclass overriding this hook must preserve an equivalent injection-safe guarantee for + * its target dialect's quoting syntax. + * + * @param column the resolved column name + * @return the quoted identifier + */ + protected String quoteIdentifier(String column) { + return "\"" + column.replace("\"", "\"\"") + "\""; + } + + /** + * Renders {@code is} / {@code is not}, including {@code IS NULL} / {@code IS NOT NULL} when + * {@code value} is {@code null}. + * + * @param column already-quoted column identifier + * @param negated {@code true} for {@code is not}, {@code false} for {@code is} + * @param value the already-converted value; {@code null} means SQL {@code NULL} + * @return the rendered fragment + */ + protected SqlFragment renderIs(String column, boolean negated, Object value) { + if (value == null) { + return new SqlFragment(column + (negated ? " IS NOT NULL" : " IS NULL"), List.of()); + } + return new SqlFragment(column + (negated ? " <> ?" : " = ?"), newList(value)); + } + + /** + * Renders one of the four numeric comparison operators ({@code is greater than}, {@code is + * greater than or equal to}, {@code is less than}, {@code is less than or equal to}). + * + * @param column already-quoted column identifier + * @param operator the resolved operator + * @param value the already-converted comparison value + * @return the rendered fragment + */ + protected SqlFragment renderComparison(String column, OperatorDefinition operator, Object value) { + String sqlOperator = + switch (operator.getCanonicalName()) { + case "is greater than" -> ">"; + case "is greater than or equal to" -> ">="; + case "is less than" -> "<"; + case "is less than or equal to" -> "<="; + default -> throw new SqlRenderException( + "Unsupported comparison operator: '" + operator.getCanonicalName() + "'"); + }; + return new SqlFragment(column + " " + sqlOperator + " ?", newList(value)); + } + + /** + * Renders {@code contains} / {@code starts with} / {@code is like} / {@code is not like}. + * + *

Escaping of literal {@code %} / {@code _} characters already present in {@code value} is + * not performed by this default implementation — matching semantics for such + * values are left to each vendor's discretion (see {@code QueryBuilder}'s Javadoc). This + * implementation uses a plain ANSI {@code LIKE} / {@code NOT LIKE} with the wildcard placed + * according to the operator: + * + *

    + *
  • {@code contains}: {@code %value%} + *
  • {@code starts with}: {@code value%} + *
  • {@code is like}: {@code value} used as-is (the caller supplies its own wildcards) + *
  • {@code is not like}: same as {@code is like}, negated + *
+ * + * @param column already-quoted column identifier + * @param operator the resolved operator + * @param value the already-converted string value + * @return the rendered fragment + */ + protected SqlFragment renderLike(String column, OperatorDefinition operator, Object value) { + String pattern = + switch (operator.getCanonicalName()) { + case "contains" -> "%" + value + "%"; + case "starts with" -> value + "%"; + case "is like", "is not like" -> String.valueOf(value); + default -> throw new SqlRenderException( + "Unsupported LIKE-family operator: '" + operator.getCanonicalName() + "'"); + }; + boolean negated = "is not like".equals(operator.getCanonicalName()); + return new SqlFragment(column + (negated ? " NOT LIKE ?" : " LIKE ?"), newList(pattern)); + } + + /** + * Renders {@code is in}, expanding to {@code column IN (?, ?, …)}. + * + * @param column already-quoted column identifier + * @param values the already-converted, non-empty member list (the translator rejects an empty + * list-shaped value before it would reach this hook) + * @return the rendered fragment + */ + protected SqlFragment renderIn(String column, List values) { + String placeholders = String.join(", ", values.stream().map(v -> "?").toList()); + return new SqlFragment(column + " IN (" + placeholders + ")", new ArrayList<>(values)); + } + + /** + * Renders the unary {@code exists} / {@code does not exist} pair, with no bind parameter. + * + * @param column already-quoted column identifier + * @param negated {@code true} for {@code does not exist}, {@code false} for {@code exists} + * @return the rendered fragment + */ + protected SqlFragment renderExists(String column, boolean negated) { + return new SqlFragment(column + (negated ? " IS NULL" : " IS NOT NULL"), List.of()); + } + + /** + * Fallback for any binary operator outside the 13 built-ins. + * + *

Throws {@link SqlRenderException} by default; a vendor supporting a custom operator + * overrides this hook rather than reimplementing {@link #binary}'s dispatch. + * + * @param column already-quoted column identifier + * @param operator the unrecognized operator + * @param value the already-converted value + * @return never returns normally in the default implementation + * @throws SqlRenderException always, unless overridden + */ + protected SqlFragment renderUnknown(String column, OperatorDefinition operator, Object value) { + throw new SqlRenderException( + "No ANSI rendering for operator '" + operator.getCanonicalName() + "' on column " + column); + } + + /** + * Fallback twin of {@link #renderUnknown} for unary operators outside the built-in + * {@code exists} / {@code does not exist} pair. + * + * @param column already-quoted column identifier + * @param operator the unrecognized unary operator + * @return never returns normally in the default implementation + * @throws SqlRenderException always, unless overridden + */ + protected SqlFragment unaryUnknown(String column, OperatorDefinition operator) { + throw new SqlRenderException( + "No ANSI rendering for unary operator '" + operator.getCanonicalName() + "' on column " + column); + } + + private static List newList(Object value) { + List list = new ArrayList<>(); + list.add(value); + return list; + } +} diff --git a/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/DefaultFilterQueryTranslatorTest.java b/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/DefaultFilterQueryTranslatorTest.java new file mode 100644 index 0000000..673baae --- /dev/null +++ b/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/DefaultFilterQueryTranslatorTest.java @@ -0,0 +1,373 @@ +package org.sequeless.filter.sql; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import java.util.List; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.sequeless.filter.api.AnyFilter; +import org.sequeless.filter.api.FieldDefinition; +import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FieldRegistrySpec; +import org.sequeless.filter.api.FilterNode; +import org.sequeless.filter.api.Filters; +import org.sequeless.filter.api.OperatorDefinition; +import org.sequeless.filter.api.OperatorRegistry; +import org.sequeless.filter.api.Syntax; +import org.sequeless.filter.spi.OperatorContributor; +import org.sequeless.filter.sql.api.ColumnMapping; +import org.sequeless.filter.sql.api.DefaultFilterQueryTranslator; +import org.sequeless.filter.sql.api.FilterQueryTranslator; +import org.sequeless.filter.sql.api.SqlFragment; +import org.sequeless.filter.sql.api.SqlRenderException; +import org.sequeless.filter.sql.spi.AnsiQueryBuilder; +import org.sequeless.filter.sql.spi.QueryBuilder; + +/** + * Covers all 13 built-in operators' translation through {@link DefaultFilterQueryTranslator} and + * {@link AnsiQueryBuilder}, plus the D11/D18/D22/D30/D33 edge cases: empty and/or rejection, empty + * list-shaped value rejection, {@code any}-expansion's drop-unmapped-then-succeed and + * drop-all-then-fail paths, and dispatch-by-{@code isUnary()} rather than by null value. + */ +class DefaultFilterQueryTranslatorTest { + + private FieldRegistry fields; + private OperatorRegistry ops; + private ColumnMapping columns; + private QueryBuilder builder; + private FilterQueryTranslator translator; + + @BeforeEach + void setUp() { + FieldDefinition status = FieldDefinition.builder() + .path("status") + .jsonSchemaType("string") + .build(); + FieldDefinition name = + FieldDefinition.builder().path("name").jsonSchemaType("string").build(); + FieldDefinition description = FieldDefinition.builder() + .path("description") + .jsonSchemaType("string") + .build(); + FieldDefinition age = + FieldDefinition.builder().path("age").jsonSchemaType("number").build(); + FieldDefinition archivedAt = FieldDefinition.builder() + .path("archivedAt") + .jsonSchemaType("string") + .build(); + + fields = FieldRegistry.of(FieldRegistrySpec.builder() + .fields(List.of(status, name, description, age, archivedAt)) + .build()); + ops = OperatorRegistry.defaults(); + columns = ColumnMapping.identity(); + builder = new AnsiQueryBuilder(); + translator = new DefaultFilterQueryTranslator(); + } + + private SqlFragment translate(FilterNode node) { + return translator.translate(node, columns, fields, ops, builder); + } + + // --- 13 built-in operators --- + + @Test + void translatesIsToEquals() { + SqlFragment f = translate(Filters.field("status", "is", JsonNodeFactory.instance.textNode("open"))); + assertThat(f.sql()).isEqualTo("\"status\" = ?"); + assertThat(f.parameters()).containsExactly("open"); + } + + @Test + void translatesIsNotToNotEquals() { + SqlFragment f = translate(Filters.field("status", "is not", JsonNodeFactory.instance.textNode("open"))); + assertThat(f.sql()).isEqualTo("\"status\" <> ?"); + assertThat(f.parameters()).containsExactly("open"); + } + + @Test + void statusIsNullRendersIsNullNotExists() { + // D33: dispatch by operator.isUnary(), never by value == null. 'is' is binary; a null + // value must render IS NULL, not be mistaken for the unary 'exists' operator. + SqlFragment f = translate(Filters.field("status", "is", JsonNodeFactory.instance.nullNode())); + assertThat(f.sql()).isEqualTo("\"status\" IS NULL"); + assertThat(f.parameters()).isEmpty(); + } + + @Test + void statusIsNotNullRendersIsNotNull() { + SqlFragment f = translate(Filters.field("status", "is not", JsonNodeFactory.instance.nullNode())); + assertThat(f.sql()).isEqualTo("\"status\" IS NOT NULL"); + assertThat(f.parameters()).isEmpty(); + } + + @Test + void translatesIsGreaterThan() { + SqlFragment f = translate(Filters.field("age", "is greater than", JsonNodeFactory.instance.numberNode(21))); + assertThat(f.sql()).isEqualTo("\"age\" > ?"); + assertThat(f.parameters()).containsExactly(21); + } + + @Test + void translatesIsGreaterThanOrEqualTo() { + SqlFragment f = + translate(Filters.field("age", "is greater than or equal to", JsonNodeFactory.instance.numberNode(21))); + assertThat(f.sql()).isEqualTo("\"age\" >= ?"); + assertThat(f.parameters()).containsExactly(21); + } + + @Test + void translatesIsLessThan() { + SqlFragment f = translate(Filters.field("age", "is less than", JsonNodeFactory.instance.numberNode(65))); + assertThat(f.sql()).isEqualTo("\"age\" < ?"); + assertThat(f.parameters()).containsExactly(65); + } + + @Test + void translatesIsLessThanOrEqualTo() { + SqlFragment f = + translate(Filters.field("age", "is less than or equal to", JsonNodeFactory.instance.numberNode(65))); + assertThat(f.sql()).isEqualTo("\"age\" <= ?"); + assertThat(f.parameters()).containsExactly(65); + } + + @Test + void translatesContains() { + SqlFragment f = translate(Filters.field("name", "contains", JsonNodeFactory.instance.textNode("foo"))); + assertThat(f.sql()).isEqualTo("\"name\" LIKE ?"); + assertThat(f.parameters()).containsExactly("%foo%"); + } + + @Test + void translatesStartsWith() { + SqlFragment f = translate(Filters.field("name", "starts with", JsonNodeFactory.instance.textNode("foo"))); + assertThat(f.sql()).isEqualTo("\"name\" LIKE ?"); + assertThat(f.parameters()).containsExactly("foo%"); + } + + @Test + void translatesIsLike() { + SqlFragment f = translate(Filters.field("name", "is like", JsonNodeFactory.instance.textNode("f_o%"))); + assertThat(f.sql()).isEqualTo("\"name\" LIKE ?"); + assertThat(f.parameters()).containsExactly("f_o%"); + } + + @Test + void translatesIsNotLike() { + SqlFragment f = translate(Filters.field("name", "is not like", JsonNodeFactory.instance.textNode("f_o%"))); + assertThat(f.sql()).isEqualTo("\"name\" NOT LIKE ?"); + assertThat(f.parameters()).containsExactly("f_o%"); + } + + @Test + void translatesIsInToInClause() { + var values = JsonNodeFactory.instance.arrayNode(); + values.add("open"); + values.add("closed"); + SqlFragment f = translate(Filters.field("status", "is in", values)); + assertThat(f.sql()).isEqualTo("\"status\" IN (?, ?)"); + assertThat(f.parameters()).containsExactly("open", "closed"); + } + + @Test + void isInWithNullMemberBindsNullParameter() { + // D21: SqlFragment tolerates a null bind parameter (e.g. 'is in' with a null member). + var values = JsonNodeFactory.instance.arrayNode(); + values.add("open"); + values.addNull(); + SqlFragment f = translate(Filters.field("status", "is in", values)); + assertThat(f.sql()).isEqualTo("\"status\" IN (?, ?)"); + assertThat(f.parameters()).containsExactly("open", null); + } + + @Test + void translatesExists() { + SqlFragment f = translate(Filters.field("archivedAt", "exists")); + assertThat(f.sql()).isEqualTo("\"archivedAt\" IS NOT NULL"); + assertThat(f.parameters()).isEmpty(); + } + + @Test + void translatesDoesNotExist() { + SqlFragment f = translate(Filters.field("archivedAt", "does not exist")); + assertThat(f.sql()).isEqualTo("\"archivedAt\" IS NULL"); + assertThat(f.parameters()).isEmpty(); + } + + // --- AND / OR combination and bind-parameter ordering --- + + @Test + void andCombinesFragmentsWithParenthesesAndOrderedParameters() { + SqlFragment f = translate(Filters.and( + Filters.field("status", "is", JsonNodeFactory.instance.textNode("open")), + Filters.field("age", "is greater than", JsonNodeFactory.instance.numberNode(21)))); + assertThat(f.sql()).isEqualTo("(\"status\" = ? AND \"age\" > ?)"); + assertThat(f.parameters()).containsExactly("open", 21); + } + + @Test + void orCombinesFragmentsWithParenthesesAndOrderedParameters() { + SqlFragment f = translate(Filters.or( + Filters.field("status", "is", JsonNodeFactory.instance.textNode("open")), + Filters.field("status", "is", JsonNodeFactory.instance.textNode("closed")))); + assertThat(f.sql()).isEqualTo("(\"status\" = ? OR \"status\" = ?)"); + assertThat(f.parameters()).containsExactly("open", "closed"); + } + + // --- Empty and/or rejection (D11/D22) --- + + @Test + void emptyAndFilterAtRootIsRejected() { + assertThatThrownBy(() -> translate(new org.sequeless.filter.api.AndFilter(List.of()))) + .isInstanceOf(SqlRenderException.class); + } + + @Test + void emptyOrFilterNestedInsideAndIsRejectedAtAnyDepth() { + FilterNode nestedEmptyOr = Filters.and( + Filters.field("status", "is", JsonNodeFactory.instance.textNode("open")), + new org.sequeless.filter.api.OrFilter(List.of())); + assertThatThrownBy(() -> translate(nestedEmptyOr)).isInstanceOf(SqlRenderException.class); + } + + // --- Empty list-shaped value rejection (D11/D22/D30) --- + + @Test + void isInWithEmptyListIsRejected() { + var empty = JsonNodeFactory.instance.arrayNode(); + assertThatThrownBy(() -> translate(Filters.field("status", "is in", empty))) + .isInstanceOf(SqlRenderException.class); + } + + // --- any-expansion: drop unmapped operands (D18/D29) --- + + @Test + void anyExpansionDropsUnmappedOperandsThenSucceeds() { + // 'name' and 'description' are both string fields compatible with 'contains'; map only + // 'name' to a column, leaving 'description' unmapped. The any-expansion should drop the + // unmapped operand rather than failing the whole filter. + ColumnMapping partial = ColumnMapping.of(java.util.Map.of("name", "name")); + FilterQueryTranslator t = new DefaultFilterQueryTranslator(); + SqlFragment f = t.translate( + new AnyFilter("contains", JsonNodeFactory.instance.textNode("foo")), partial, fields, ops, builder); + // Only 'name' survives the drop-unmapped step; the single survivor is still combined via + // builder.or(...), so it comes back parenthesized like any other OR result. + assertThat(f.sql()).isEqualTo("(\"name\" LIKE ?)"); + assertThat(f.parameters()).containsExactly("%foo%"); + } + + @Test + void anyExpansionWithAllOperandsUnmappedFails() { + ColumnMapping none = ColumnMapping.of(java.util.Map.of()); + FilterQueryTranslator t = new DefaultFilterQueryTranslator(); + assertThatThrownBy(() -> t.translate( + new AnyFilter("contains", JsonNodeFactory.instance.textNode("foo")), + none, + fields, + ops, + builder)) + .isInstanceOf(SqlRenderException.class); + } + + @Test + void explicitlyNamedUnmappedColumnStillHardFails() { + // An explicitly-named path (not reached through 'any') must still hard-fail on an + // unmapped column — D18's leniency is scoped to any-expansion only. + ColumnMapping none = ColumnMapping.of(java.util.Map.of()); + FilterQueryTranslator t = new DefaultFilterQueryTranslator(); + assertThatThrownBy(() -> t.translate( + Filters.field("status", "is", JsonNodeFactory.instance.textNode("open")), + none, + fields, + ops, + builder)) + .isInstanceOf(SqlRenderException.class); + } + + // --- Unknown/custom operator fallback (D16) --- + + @Test + void customBinaryOperatorFallsBackToRenderUnknown() { + OperatorDefinition soundsLike = OperatorDefinition.builder() + .canonicalName("sounds like") + .syntax(Syntax.INFIX) + .applicableTypes(List.of("string")) + .build(); + OperatorContributor custom = () -> List.of(soundsLike); + OperatorRegistry customOps = OperatorRegistry.of(List.of(custom)); + FieldRegistry soundexFields = FieldRegistry.of(FieldRegistrySpec.builder() + .fields(List.of(FieldDefinition.builder() + .path("name") + .jsonSchemaType("string") + .build())) + .build()); + + FilterQueryTranslator t = new DefaultFilterQueryTranslator(); + FilterNode node = Filters.field("name", "sounds like", JsonNodeFactory.instance.textNode("smith")); + + assertThatThrownBy(() -> t.translate(node, columns, soundexFields, customOps, builder)) + .isInstanceOf(SqlRenderException.class); + } + + @Test + void customBinaryOperatorCanBeSupportedByOverridingRenderUnknown() { + OperatorDefinition soundsLike = OperatorDefinition.builder() + .canonicalName("sounds like") + .syntax(Syntax.INFIX) + .applicableTypes(List.of("string")) + .build(); + OperatorContributor custom = () -> List.of(soundsLike); + OperatorRegistry customOps = OperatorRegistry.of(List.of(custom)); + FieldRegistry soundexFields = FieldRegistry.of(FieldRegistrySpec.builder() + .fields(List.of(FieldDefinition.builder() + .path("name") + .jsonSchemaType("string") + .build())) + .build()); + + QueryBuilder soundexBuilder = new AnsiQueryBuilder() { + @Override + protected SqlFragment renderUnknown(String column, OperatorDefinition operator, Object value) { + if ("sounds like".equals(operator.getCanonicalName())) { + return new SqlFragment("SOUNDEX(" + column + ") = SOUNDEX(?)", List.of(value)); + } + return super.renderUnknown(column, operator, value); + } + }; + + FilterQueryTranslator t = new DefaultFilterQueryTranslator(); + FilterNode node = Filters.field("name", "sounds like", JsonNodeFactory.instance.textNode("smith")); + SqlFragment f = t.translate(node, columns, soundexFields, customOps, soundexBuilder); + + assertThat(f.sql()).isEqualTo("SOUNDEX(\"name\") = SOUNDEX(?)"); + assertThat(f.parameters()).containsExactly("smith"); + } + + @Test + void customUnaryOperatorFallsBackToUnaryUnknown() { + OperatorDefinition isArchived = OperatorDefinition.builder() + .canonicalName("is archived") + .syntax(Syntax.INFIX) + .unary(true) + .applicableTypes(List.of("string")) + .build(); + OperatorContributor custom = () -> List.of(isArchived); + OperatorRegistry customOps = OperatorRegistry.of(List.of(custom)); + + FilterQueryTranslator t = new DefaultFilterQueryTranslator(); + FilterNode node = Filters.field("status", "is archived"); + + assertThatThrownBy(() -> t.translate(node, columns, fields, customOps, builder)) + .isInstanceOf(SqlRenderException.class); + } + + // --- Validation entry point (D27) --- + + @Test + void unknownFieldFailsValidationBeforeReachingQueryBuilder() { + assertThatThrownBy(() -> translate(Filters.field("nope", "is", JsonNodeFactory.instance.textNode("x")))) + .isInstanceOf(SqlRenderException.class); + } +} From d1fbdfd43e2aa6e421e8999fc6d4456aac3a2efc Mon Sep 17 00:00:00 2001 From: Ben B Date: Thu, 30 Jul 2026 21:14:33 +0000 Subject: [PATCH 08/11] feat: add H2 test fixture and end-to-end SQL adapter tests (T7) Pins H2 2.3.232 (test scope) in the parent dependencyManagement and filter-sql-adapter's own dependencies. Adds a test-scope H2QueryBuilder extending AnsiQueryBuilder, overriding quoteIdentifier solely to exercise the Template Method extension path with real coverage. Adds H2FilterQueryTranslatorTest, executing DefaultFilterQueryTranslator output as real PreparedStatements against an in-memory H2 schema/seed dataset, covering each operator family end to end plus D18's any-expansion drop-unmapped-operands path and D21's null-tolerant is-in parameter list. --- filter-sql-adapter/pom.xml | 5 + .../sql/h2/H2FilterQueryTranslatorTest.java | 243 ++++++++++++++++++ .../filter/sql/h2/H2QueryBuilder.java | 28 ++ pom.xml | 7 + 4 files changed, 283 insertions(+) create mode 100644 filter-sql-adapter/src/test/java/org/sequeless/filter/sql/h2/H2FilterQueryTranslatorTest.java create mode 100644 filter-sql-adapter/src/test/java/org/sequeless/filter/sql/h2/H2QueryBuilder.java diff --git a/filter-sql-adapter/pom.xml b/filter-sql-adapter/pom.xml index 0ed3b3e..adea803 100644 --- a/filter-sql-adapter/pom.xml +++ b/filter-sql-adapter/pom.xml @@ -26,6 +26,11 @@ com.fasterxml.jackson.core jackson-databind + + com.h2database + h2 + test + diff --git a/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/h2/H2FilterQueryTranslatorTest.java b/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/h2/H2FilterQueryTranslatorTest.java new file mode 100644 index 0000000..b0634e4 --- /dev/null +++ b/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/h2/H2FilterQueryTranslatorTest.java @@ -0,0 +1,243 @@ +package org.sequeless.filter.sql.h2; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.sequeless.filter.api.AnyFilter; +import org.sequeless.filter.api.FieldDefinition; +import org.sequeless.filter.api.FieldRegistry; +import org.sequeless.filter.api.FieldRegistrySpec; +import org.sequeless.filter.api.FilterNode; +import org.sequeless.filter.api.Filters; +import org.sequeless.filter.api.OperatorRegistry; +import org.sequeless.filter.sql.api.ColumnMapping; +import org.sequeless.filter.sql.api.DefaultFilterQueryTranslator; +import org.sequeless.filter.sql.api.FilterQueryTranslator; +import org.sequeless.filter.sql.api.SqlFragment; +import org.sequeless.filter.sql.spi.QueryBuilder; + +/** + * Executes {@link DefaultFilterQueryTranslator} output as real {@code PreparedStatement}s against + * an in-memory H2 database, covering each operator family end to end (T7) rather than only + * asserting SQL text/bind-parameter shape as {@code DefaultFilterQueryTranslatorTest} does. + * + *

Named {@code *Test}, not {@code *IT}, per the plan: the reactor's JaCoCo setup has no + * {@code report-integration} execution, so an {@code *IT} name would run under Failsafe but its + * coverage wouldn't reach the Sonar report. + */ +class H2FilterQueryTranslatorTest { + + private Connection connection; + private FieldRegistry fields; + private OperatorRegistry ops; + private QueryBuilder builder; + private FilterQueryTranslator translator; + + @BeforeEach + void setUp() throws SQLException { + // Fresh in-memory database per test method (unique URL) so tests can't see each other's + // schema/state and don't need explicit cleanup between runs. + // DATABASE_TO_LOWER folds unquoted identifiers to lower case, matching the lower-case + // column names ColumnMapping/quoteIdentifier produce below (H2 defaults to upper case). + connection = DriverManager.getConnection("jdbc:h2:mem:" + getClass().getSimpleName() + "_" + System.nanoTime() + + ";DB_CLOSE_DELAY=-1;DATABASE_TO_LOWER=TRUE"); + try (Statement stmt = connection.createStatement()) { + stmt.execute( + """ + CREATE TABLE products ( + id BIGINT PRIMARY KEY, + status VARCHAR(32), + name VARCHAR(128), + description VARCHAR(256), + age INT, + archived_at TIMESTAMP + ) + """); + stmt.execute( + """ + INSERT INTO products (id, status, name, description, age, archived_at) VALUES + (1, 'open', 'Widget', 'A useful widget', 21, NULL), + (2, 'closed', 'Gadget', 'A fancy gadget', 65, TIMESTAMP '2024-01-01 00:00:00'), + (3, 'open', 'Sprocket', 'A rugged sprocket', 42, NULL), + (4, 'pending', 'Widget Pro', NULL, 30, TIMESTAMP '2023-06-15 00:00:00') + """); + } + + FieldDefinition status = FieldDefinition.builder() + .path("status") + .jsonSchemaType("string") + .build(); + FieldDefinition name = + FieldDefinition.builder().path("name").jsonSchemaType("string").build(); + FieldDefinition description = FieldDefinition.builder() + .path("description") + .jsonSchemaType("string") + .build(); + FieldDefinition age = + FieldDefinition.builder().path("age").jsonSchemaType("number").build(); + FieldDefinition archivedAt = FieldDefinition.builder() + .path("archivedAt") + .jsonSchemaType("string") + .build(); + // Registry-compatible but deliberately left out of most tests' ColumnMapping (see + // anyExpansionSkipsUnmappedRegistryField) to exercise D18's drop-unmapped-operands path. + FieldDefinition nickname = FieldDefinition.builder() + .path("nickname") + .jsonSchemaType("string") + .build(); + + fields = FieldRegistry.of(FieldRegistrySpec.builder() + .fields(List.of(status, name, description, age, archivedAt, nickname)) + .build()); + ops = OperatorRegistry.defaults(); + builder = new H2QueryBuilder(); + translator = new DefaultFilterQueryTranslator(); + } + + @AfterEach + void tearDown() throws SQLException { + connection.close(); + } + + private List> query(FilterNode node, ColumnMapping columns) throws SQLException { + SqlFragment fragment = translator.translate(node, columns, fields, ops, builder); + String sql = "SELECT * FROM products WHERE " + fragment.sql() + " ORDER BY id"; + try (PreparedStatement stmt = connection.prepareStatement(sql)) { + List parameters = fragment.parameters(); + for (int i = 0; i < parameters.size(); i++) { + stmt.setObject(i + 1, parameters.get(i)); + } + try (ResultSet rs = stmt.executeQuery()) { + List> rows = new ArrayList<>(); + while (rs.next()) { + rows.add(Map.of( + "id", rs.getLong("id"), + "status", rs.getString("status"), + "name", rs.getString("name"))); + } + return rows; + } + } + } + + private static ColumnMapping fullMapping() { + return ColumnMapping.of(Map.of( + "status", "status", + "name", "name", + "description", "description", + "age", "age", + "archivedAt", "archived_at")); + } + + // --- equality / is-not --- + + @Test + void isEqualsFiltersToMatchingRows() throws SQLException { + List> rows = + query(Filters.field("status", "is", JsonNodeFactory.instance.textNode("open")), fullMapping()); + assertThat(rows).extracting(r -> r.get("id")).containsExactly(1L, 3L); + } + + @Test + void isNotExcludesMatchingRows() throws SQLException { + List> rows = + query(Filters.field("status", "is not", JsonNodeFactory.instance.textNode("open")), fullMapping()); + assertThat(rows).extracting(r -> r.get("id")).containsExactly(2L, 4L); + } + + // --- comparison --- + + @Test + void isGreaterThanFiltersNumerically() throws SQLException { + List> rows = + query(Filters.field("age", "is greater than", JsonNodeFactory.instance.numberNode(30)), fullMapping()); + assertThat(rows).extracting(r -> r.get("id")).containsExactly(2L, 3L); + } + + // --- like-family --- + + @Test + void containsMatchesSubstring() throws SQLException { + List> rows = + query(Filters.field("name", "contains", JsonNodeFactory.instance.textNode("Widget")), fullMapping()); + assertThat(rows).extracting(r -> r.get("id")).containsExactly(1L, 4L); + } + + // --- in, including a null element (D21) --- + + @Test + void isInWithNullElementExecutesWithoutThrowing() throws SQLException { + var values = JsonNodeFactory.instance.arrayNode(); + values.add("open"); + values.addNull(); + List> rows = query(Filters.field("status", "is in", values), fullMapping()); + // The null member contributes an "= NULL"-equivalent bind that matches no row (SQL NULL + // comparison semantics), but the query must execute cleanly and still return the 'open' rows. + assertThat(rows).extracting(r -> r.get("id")).containsExactly(1L, 3L); + } + + // --- exists-family --- + + @Test + void existsFiltersToNonNullColumn() throws SQLException { + List> rows = query(Filters.field("archivedAt", "exists"), fullMapping()); + assertThat(rows).extracting(r -> r.get("id")).containsExactly(2L, 4L); + } + + @Test + void doesNotExistFiltersToNullColumn() throws SQLException { + List> rows = query(Filters.field("archivedAt", "does not exist"), fullMapping()); + assertThat(rows).extracting(r -> r.get("id")).containsExactly(1L, 3L); + } + + // --- and/or combination --- + + @Test + void andCombinesConditions() throws SQLException { + List> rows = query( + Filters.and( + Filters.field("status", "is", JsonNodeFactory.instance.textNode("open")), + Filters.field("age", "is greater than", JsonNodeFactory.instance.numberNode(30))), + fullMapping()); + assertThat(rows).extracting(r -> r.get("id")).containsExactly(3L); + } + + @Test + void orCombinesConditions() throws SQLException { + List> rows = query( + Filters.or( + Filters.field("status", "is", JsonNodeFactory.instance.textNode("closed")), + Filters.field("status", "is", JsonNodeFactory.instance.textNode("pending"))), + fullMapping()); + assertThat(rows).extracting(r -> r.get("id")).containsExactly(2L, 4L); + } + + // --- any-expansion with a deliberately partial ColumnMapping (D18) --- + + @Test + void anyExpansionSkipsUnmappedRegistryField() throws SQLException { + // 'name', 'description', and 'nickname' are all registry-compatible with 'contains', but + // 'nickname' is deliberately left out of this ColumnMapping — there is no such column in + // the schema at all. The translator must drop that operand rather than failing the whole + // filter, and the query must still execute correctly against only the mapped columns. + ColumnMapping partial = ColumnMapping.of(Map.of( + "status", "status", + "name", "name", + "description", "description")); + List> rows = + query(new AnyFilter("contains", JsonNodeFactory.instance.textNode("Widget")), partial); + assertThat(rows).extracting(r -> r.get("id")).containsExactly(1L, 4L); + } +} diff --git a/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/h2/H2QueryBuilder.java b/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/h2/H2QueryBuilder.java new file mode 100644 index 0000000..36e5379 --- /dev/null +++ b/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/h2/H2QueryBuilder.java @@ -0,0 +1,28 @@ +package org.sequeless.filter.sql.h2; + +import org.sequeless.filter.sql.spi.AnsiQueryBuilder; + +/** + * Test-scope {@link AnsiQueryBuilder} subclass for the H2-backed tests. + * + *

H2 accepts the default ANSI {@code "…"} quoting as-is, so this override isn't required for + * correctness against H2 — it exists solely to exercise the Template Method extension path (per + * T7) with a real database round-trip, not because H2 needs different behavior. The override + * re-implements the same {@code "…"}-with-doubled-quotes contract independently rather than + * delegating to {@code super}, so the test coverage actually exercises this class's own code path. + */ +public class H2QueryBuilder extends AnsiQueryBuilder { + + @Override + protected String quoteIdentifier(String column) { + StringBuilder quoted = new StringBuilder(column.length() + 2).append('"'); + for (int i = 0; i < column.length(); i++) { + char c = column.charAt(i); + if (c == '"') { + quoted.append('"'); + } + quoted.append(c); + } + return quoted.append('"').toString(); + } +} diff --git a/pom.xml b/pom.xml index ed5e06c..c73a6c9 100644 --- a/pom.xml +++ b/pom.xml @@ -37,6 +37,7 @@ 5.11.4 3.26.3 5.14.2 + 2.3.232 2.44.0 2.50.0 @@ -65,6 +66,12 @@ antlr4-runtime ${antlr4.version} + + com.h2database + h2 + ${h2.version} + test + From 51f48414f248885d18c33d1bf8020969a080511c Mon Sep 17 00:00:00 2001 From: Ben B Date: Thu, 30 Jul 2026 21:17:59 +0000 Subject: [PATCH 09/11] feat: add filter-sql-adapter ArchUnit boundary rules (T8) Adds BoundaryRulesTest to filter-sql-adapter with a module-local public-types rule (sql.api/sql.spi types must be public, mirroring filter-core's existing rule), a cross-module rule that filter-sql-adapter's main scope may not depend on filter-core's internal/spi packages, and a rule that filter-core may not depend on filter-sql-adapter (kept per D26 as documentation of intent even though Maven's reactor build order already makes it unfalsifiable). No testfixtures-leak rule is added, per D15. Co-Authored-By: Claude Sonnet 5 --- .../filter/sql/BoundaryRulesTest.java | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 filter-sql-adapter/src/test/java/org/sequeless/filter/sql/BoundaryRulesTest.java diff --git a/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/BoundaryRulesTest.java b/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/BoundaryRulesTest.java new file mode 100644 index 0000000..c82d848 --- /dev/null +++ b/filter-sql-adapter/src/test/java/org/sequeless/filter/sql/BoundaryRulesTest.java @@ -0,0 +1,52 @@ +package org.sequeless.filter.sql; + +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses; + +import com.tngtech.archunit.core.importer.ImportOption; +import com.tngtech.archunit.junit.AnalyzeClasses; +import com.tngtech.archunit.junit.ArchTest; +import com.tngtech.archunit.lang.ArchRule; + +/** + * Mechanically enforced hexagonal boundaries for {@code filter-sql-adapter}, including its + * cross-module relationship with {@code filter-core}. + */ +@AnalyzeClasses( + packages = "org.sequeless.filter", + importOptions = {ImportOption.DoNotIncludeTests.class}) +class BoundaryRulesTest { + + @ArchTest + static final ArchRule api_and_spi_types_are_public = classes() + .that() + .resideInAnyPackage("org.sequeless.filter.sql.api..", "org.sequeless.filter.sql.spi..") + .and() + .areTopLevelClasses() + .and() + .haveSimpleNameNotEndingWith("package-info") + .should() + .bePublic() + .as("the published contract (sql.api + sql.spi) must be exported as public types"); + + @ArchTest + static final ArchRule sql_adapter_does_not_reach_filter_core_internals = noClasses() + .that() + .resideInAnyPackage("org.sequeless.filter.sql..") + .should() + .dependOnClassesThat() + .resideInAnyPackage("org.sequeless.filter.internal..", "org.sequeless.filter.spi..") + .as("filter-sql-adapter's main scope may only depend on filter-core's api package"); + + @ArchTest + static final ArchRule filter_core_does_not_depend_on_sql_adapter = noClasses() + .that() + .resideInAnyPackage("org.sequeless.filter..") + .and() + .resideOutsideOfPackage("org.sequeless.filter.sql..") + .should() + .dependOnClassesThat() + .resideInAnyPackage("org.sequeless.filter.sql..") + .as("filter-core must not depend on filter-sql-adapter (documents the intended " + + "dependency direction; Maven's reactor build order already rejects this cycle)"); +} From 05a5df700be11a9e86f5dd7044d4ae94bb76c99b Mon Sep 17 00:00:00 2001 From: Ben B Date: Thu, 30 Jul 2026 21:21:32 +0000 Subject: [PATCH 10/11] docs: add filter-sql-adapter reference (T9) Documents the SQL adapter's pipeline (validate -> translate -> ColumnMapping -> QueryBuilder -> PreparedStatement), the JsonNode-to-Java value-conversion table with its date-time and BigInteger caveats, the vendor-defined LIKE escaping vs. mandated identifier quoting distinction, how to write a vendor QueryBuilder by extending AnsiQueryBuilder's 8 protected hooks, and the adapter's known limitations (flat/single-table mappings, no custom AnyExpansionStrategy). Linked from README.md and docs/filter-dsl.md. Co-Authored-By: Claude Sonnet 5 --- README.md | 2 + docs/filter-dsl.md | 5 + docs/sql-adapter.md | 232 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 239 insertions(+) create mode 100644 docs/sql-adapter.md diff --git a/README.md b/README.md index a0cdcee..a560eb7 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,7 @@ Requires JDK 21. | Module | Coordinates | Contents | | --- | --- | --- | | [`filter-core`](filter-core) | `org.sequeless:filter-core` | The DSL: AST model, ANTLR 4 grammar and parser, serializer, operator and field registries, completion utilities. | +| [`filter-sql-adapter`](filter-sql-adapter) | `org.sequeless:filter-sql-adapter` | Translates a parsed `FilterNode` AST into raw, parameterized SQL via a hexagonal `QueryBuilder` vendor port. See [docs/sql-adapter.md](docs/sql-adapter.md). | The repository root is a `pom`-packaged aggregator/parent (`org.sequeless:sequeless-filter`) that ships no code — it holds the single shared version, dependency management, and build plugins. All @@ -52,6 +53,7 @@ install is required. ## Documentation - [Filter DSL reference](docs/filter-dsl.md) — grammar, operators, serialization, completion. +- [SQL adapter](docs/sql-adapter.md) — translating a `FilterNode` AST into parameterized SQL. - Architecture decisions live under [`docs/adr/`](docs/adr/). ## Contributing diff --git a/docs/filter-dsl.md b/docs/filter-dsl.md index acb20a0..12099d7 100644 --- a/docs/filter-dsl.md +++ b/docs/filter-dsl.md @@ -93,3 +93,8 @@ function-argument index the cursor sits in. A field's `CompletionProvider` is optional and embedder-supplied (e.g. backing a `status` field's value completion with the live set of distinct values) — fields without one simply offer no value candidates. + +## See also + +- [SQL adapter](sql-adapter.md) — translating a parsed `FilterNode` AST into parameterized SQL + against a relational database, via `filter-sql-adapter`. diff --git a/docs/sql-adapter.md b/docs/sql-adapter.md new file mode 100644 index 0000000..b47bd37 --- /dev/null +++ b/docs/sql-adapter.md @@ -0,0 +1,232 @@ +# SQL adapter + +`filter-sql-adapter` (`org.sequeless:filter-sql-adapter`) translates a parsed `FilterNode` AST into +raw, parameterized SQL — a `String` with `?` placeholders plus an ordered bind-parameter list, the +classic JDBC `PreparedStatement` shape. It has no dependency on any particular database driver and +no ORM/Criteria object graph: you get SQL text and values back, and you run them yourself. + +This page covers the translation pipeline, the `JsonNode`→Java value-conversion rules, the +distinction between vendor-defined `LIKE` escaping and mandated identifier quoting, how to write a +vendor-specific `QueryBuilder`, and this adapter's known limitations. + +See [docs/filter-dsl.md](filter-dsl.md) for the DSL itself (grammar, operators, parsing). + +## The pipeline + +``` +DSL text --FilterParser.parse--> FilterNode + | + v + FilterQueryTranslator.translate(node, columns, fields, ops, builder) + | + 1. FilterValidator.validate(node, ops, fields) -- internal, not opt-in + 2. walk the tree, expanding `any` per-node via AnyFilterExpander's default strategy + 3. resolve each path via ColumnMapping + 4. convert each JsonNode value to a plain Java value + 5. delegate rendering to QueryBuilder + | + v + SqlFragment(sql, parameters) + | + v + PreparedStatement + positional setObject(...) +``` + +`FilterQueryTranslator` (`org.sequeless.filter.sql.api`) is the single vendor-agnostic entry point; +`DefaultFilterQueryTranslator` is its (and currently only) implementation. A few behaviors worth +calling out explicitly: + +- **Validation is internal, not opt-in.** Nothing else in this library calls `FilterValidator` + automatically — `FilterParser.parse` does not validate. `translate(...)` calls + `FilterValidator.validate(node, ops, fields)` itself, first, and converts any reported violation + to `SqlRenderException`. This means `translate(...)` is safe to call directly against an + unvalidated, hand-built, or deserialized `FilterNode` — you never need a separate validation step + before calling it. +- **`any` is expanded internally, per-node, using the default expansion strategy.** As the walker + visits an `AnyFilter` node, it calls `AnyFilterExpander`'s default strategy right there (not once + over the whole tree up front) and continues walking the resulting operands. Any operand whose + path has no `ColumnMapping` entry is silently dropped rather than failing the whole filter; + `SqlRenderException` is raised only if *no* expanded operand survives. This leniency applies only + to `any`-expansion results — an explicitly-named path anywhere else in the tree still hard-fails + on an unmapped column exactly as before. There is no way to supply a *custom* + `AnyExpansionStrategy` through `translate(...)` — see [Known limitations](#known-limitations). +- **`ColumnMapping` resolves each path to a bare column name**, and (per its `identity()` factory) + rejects dotted paths — see [Known limitations](#known-limitations). +- **`QueryBuilder` owns all SQL construction** — identifier quoting, operator-to-SQL translation, + value binding, `AND`/`OR` combination. The translator never renders SQL text itself; it only + walks, validates, resolves, and converts, then delegates. + +### Usage example + +```java +import org.sequeless.filter.api.*; +import org.sequeless.filter.sql.api.*; +import org.sequeless.filter.sql.spi.AnsiQueryBuilder; +import org.sequeless.filter.sql.spi.QueryBuilder; + +// 1. Registries — built the same way any production caller would. +FieldRegistry fields = FieldRegistry.of(FieldRegistrySpec.builder() + .fields(List.of( + FieldDefinition.builder().path("status").jsonSchemaType("string").build(), + FieldDefinition.builder().path("age").jsonSchemaType("number").build())) + .build()); +OperatorRegistry ops = OperatorRegistry.defaults(); + +// 2. A caller-supplied path-to-column mapping (D20/B10: flat, single-table only). +ColumnMapping columns = ColumnMapping.of(Map.of( + "status", "status", + "age", "age")); + +// 3. The vendor QueryBuilder — the shipped ANSI default, or a subclass for your dialect. +QueryBuilder builder = new AnsiQueryBuilder(); + +// 4. Parse and translate. +FilterNode node = FilterParser.parse("status is 'open' and age is greater than 30", ops, fields); +FilterQueryTranslator translator = new DefaultFilterQueryTranslator(); +SqlFragment fragment = translator.translate(node, columns, fields, ops, builder); + +// 5. Run it as a real PreparedStatement. +String sql = "SELECT * FROM products WHERE " + fragment.sql(); +try (PreparedStatement stmt = connection.prepareStatement(sql)) { + List parameters = fragment.parameters(); + for (int i = 0; i < parameters.size(); i++) { + stmt.setObject(i + 1, parameters.get(i)); + } + try (ResultSet rs = stmt.executeQuery()) { + // ... + } +} +``` + +`fragment.sql()` is a bare `WHERE`-clause fragment (e.g. `("status" = ? AND "age" > ?)`) — it's your +responsibility to embed it in a full statement (`SELECT ... WHERE `, or combined with +other application-level conditions). + +## Value conversion (`JsonNode` → Java) + +`FieldFilter`/`AnyFilter` values arrive as Jackson `JsonNode`s (a parsed filter's literals, or +whatever a deserialized `FilterNode` happens to carry). The translator converts each value to a +plain Java object *before* calling `QueryBuilder`, so every vendor gets one consistent conversion +for free: + +| `JsonNode` type | Java value | +| --- | --- | +| `TextNode` | `String` | +| `IntNode` / `LongNode` / `DoubleNode` | the matching boxed `Number` (`Integer`/`Long`/`Double`) | +| `BooleanNode` | `Boolean` | +| `NullNode` | `null` | +| `ArrayNode` | `List`, each element converted recursively | +| any other numeric node (e.g. `BigIntegerNode`, `DecimalNode`) | via `isNumber()`/`numberValue()` | +| anything else (`ObjectNode`, `BinaryNode`, `POJONode`, `MissingNode`, ...) | `SqlRenderException` | + +Two things worth calling out explicitly: + +- **No temporal coercion.** There is no date/time literal in the grammar, so a field with + `jsonSchemaFormat: "date-time"` still arrives as a plain `TextNode` and converts to a plain + `String`. Binding that string against a `TIMESTAMP`/`DATE` column is left entirely to the vendor + `QueryBuilder`/JDBC driver — this adapter performs no parsing or coercion of date-time values. +- **`BigInteger` binding caveat.** A `BigIntegerNode` (a JSON integer past `Long.MAX_VALUE`, which + can only arise from a deserialized `FilterNode`, not the parser) converts via the numeric + catch-all to a raw `BigInteger`. Most JDBC drivers' `PreparedStatement.setObject` do not accept a + `BigInteger` directly and will throw at execution time — this is a known limitation of the + current conversion table, not something worked around elsewhere in the pipeline. If your values + may exceed `Long.MAX_VALUE`, be prepared to handle this at the JDBC layer (e.g. converting to + `BigDecimal` or a `String` column yourself). + +A `null` Java value passed to `QueryBuilder.binary(...)` means SQL `NULL` (e.g. from +`status is null`, i.e. a `NullNode` converted to `null`) — it does **not** mean "no value." Dispatch +between `binary(...)` and `unary(...)` is always by the operator's own `isUnary()` flag, never by +whether the converted value is `null`, so `status is null` reliably renders `IS NULL` rather than +being mistaken for a unary condition like `exists`. + +## `LIKE` escaping vs. identifier quoting + +These look like similar "vendor gets to decide the details" questions but are not the same kind of +decision: + +- **`LIKE`-family escaping (`contains`, `starts with`, `is like`, `is not like`) is vendor-defined, + by design.** Whether a literal `%` or `_` inside a value is escaped before being placed in a + `LIKE` pattern — and therefore whether such a value matches literally or as a wildcard — is left + entirely to each `QueryBuilder` implementation's discretion. The shipped `AnsiQueryBuilder` + performs **no** escaping: `contains` renders `%value%`, `starts with` renders `value%`, and + `is like`/`is not like` use the value as-is (the caller supplies its own wildcards). A different + `QueryBuilder` — including a different vendor adapter you write yourself — may escape `%`/`_` + differently, and no cross-vendor test in this repository asserts one specific outcome. This is a + matching-semantics choice, not a security boundary. +- **Identifier quoting on the shipped default is mandated, because it's a security boundary, not a + style choice.** `AnsiQueryBuilder.quoteIdentifier` emits `"…"` with any embedded `"` doubled, and + its Javadoc states this explicitly: this is the one thing standing between a caller-supplied + column name and SQL injection when a `ColumnMapping` (especially `identity()`, which performs no + validation of its own) hands the translator a path that ultimately came from programmatic or + deserialized input rather than the grammar's own restricted identifier token + (`[a-zA-Z_][a-zA-Z_0-9]*`). A subclass that overrides `quoteIdentifier` must preserve an + equivalent injection-safe guarantee for its target dialect's quoting syntax — this is not free to + change for cosmetic reasons the way `LIKE` escaping is. + +## Writing a vendor `QueryBuilder` + +There are two ways to supply your own SQL rendering: + +1. **Implement `org.sequeless.filter.sql.spi.QueryBuilder` from scratch.** Four methods: + `binary(String column, OperatorDefinition operator, Object value)`, + `unary(String column, OperatorDefinition operator)`, `and(List operands)`, + `or(List operands)`. You own every rendering decision — quoting, operator syntax, + value binding, combination. +2. **Extend `org.sequeless.filter.sql.spi.AnsiQueryBuilder`** and override only the `protected` hooks + that differ for your dialect. `AnsiQueryBuilder` is a public, non-`final` Template Method: its + `binary()`/`unary()` are `final` dispatchers that delegate to exactly these 8 `protected` hooks + (the complete, fixed set — not a starting point meant to accrete further overloads): + + | Hook | Covers | + | --- | --- | + | `quoteIdentifier(String column)` | Identifier quoting (mandated escaping contract, see above) | + | `renderIs(String column, boolean negated, Object value)` | `is` / `is not`, including `IS NULL`/`IS NOT NULL` | + | `renderComparison(String column, OperatorDefinition operator, Object value)` | The 4 numeric comparisons | + | `renderLike(String column, OperatorDefinition operator, Object value)` | `contains` / `starts with` / `is like` / `is not like` | + | `renderIn(String column, List values)` | `is in` | + | `renderExists(String column, boolean negated)` | The unary `exists` / `does not exist` pair | + | `renderUnknown(String column, OperatorDefinition operator, Object value)` | Fallback for any binary operator outside the 13 built-ins | + | `unaryUnknown(String column, OperatorDefinition operator)` | Fallback twin of `renderUnknown` for unary operators | + + A vendor that only needs to change one concern — say, `LIKE` escaping, or supporting a custom + operator registered via `OperatorRegistry.of(List)` — overrides just that + hook, instead of reimplementing the whole 13-operator dispatch. `renderUnknown`/`unaryUnknown` + throw `SqlRenderException` by default; override them to support operators outside the built-in + set (the H2 test fixture in this repo, `H2QueryBuilder`, demonstrates overriding a single hook — + `quoteIdentifier` — purely to exercise the extension path). + +There is **no `QueryBuilder.ansi()` static factory** — instantiate `AnsiQueryBuilder` (or your +subclass) directly with `new AnsiQueryBuilder()`. This was a deliberate choice: the port interface +stays free of any compile-time reference to a specific concrete adapter. + +## Known limitations + +- **Flat, single-table column mappings only.** `ColumnMapping` returns a bare column name, and a + `SqlFragment` is a single `WHERE`-clause fragment plus its binds — there is no way to express a + join, a correlated subquery, or a JSON path expression. This matters because the DSL's own + headline example, `lineItems.qty > 5`, typically means a join on a relational schema, not a + column — that shape is out of scope for this adapter as specified. If you have a real column + backing a dotted path (e.g. via a joined view), you can still map it explicitly with + `ColumnMapping.of(Map)`. +- **`ColumnMapping.identity()` rejects dotted paths.** Because of the single-table limitation above, + `identity()` returns `Optional.empty()` for any path containing `.`, which the translator turns + into a clean `SqlRenderException` at translation time — rather than producing a syntactically + valid but nonexistent quoted identifier like `"lineItems.qty"` that would only fail later as a + vendor-specific "column does not exist" error at execution time. This restriction is scoped to + `identity()` only; `ColumnMapping.of(Map)` is unaffected and can map a dotted path deliberately. + Also note `identity()` is documented as trusted-input-only — it performs no other validation, so + paths from untrusted or deserialized input should use an explicit `of(Map)` allowlist instead. +- **A custom `AnyExpansionStrategy` is unreachable through this adapter.** `translate(...)` always + uses `AnyFilterExpander`'s default expansion strategy internally; there is no parameter to supply + a different one. If you need non-default `any` expansion semantics, pre-expand the filter + yourself (with your own `AnyExpansionStrategy`) before calling `translate(...)`. +- **`LIKE`-family escaping is vendor-defined**, not a fixed cross-vendor contract — see + [`LIKE` escaping vs. identifier quoting](#like-escaping-vs-identifier-quoting) above. +- **Date-time values pass through as plain strings**, with no temporal coercion — see + [Value conversion](#value-conversion-jsonnode--java) above. +- **`BigInteger` values may not bind via `PreparedStatement.setObject`** on most JDBC drivers — see + [Value conversion](#value-conversion-jsonnode--java) above. +- **`QueryBuilder` and `FilterQueryTranslator` implementations are expected to be stateless and + thread-safe.** This isn't enforced by any type, but callers will naturally hold one instance of + each as a long-lived singleton shared across request threads — `DefaultFilterQueryTranslator` and + `AnsiQueryBuilder` both hold no mutable state, and a subclass should preserve that. From 61b644501f020c14a17d6f882778f20d83c99283 Mon Sep 17 00:00:00 2001 From: Ben B Date: Thu, 30 Jul 2026 23:20:09 +0000 Subject: [PATCH 11/11] docs: improve readability of ADR and SQL adapter documentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restructure dense paragraphs into scannable bullets and tables: - ADR: tighter Context/Decision/Consequences with bullet-point breakdown - SQL adapter: top-of-page nav, inline pipeline diagram, tables for LIKE/quoting and known limitations, consolidated caveats to avoid duplication No content changes — all substantive information preserved, just reorganized for better skimmability. Co-Authored-By: Claude Sonnet 5 --- ...01-split-the-build-into-a-maven-reactor.md | 95 ++++----- docs/sql-adapter.md | 185 ++++++++---------- 2 files changed, 129 insertions(+), 151 deletions(-) diff --git a/docs/adr/0001-split-the-build-into-a-maven-reactor.md b/docs/adr/0001-split-the-build-into-a-maven-reactor.md index 698187e..3af01aa 100644 --- a/docs/adr/0001-split-the-build-into-a-maven-reactor.md +++ b/docs/adr/0001-split-the-build-into-a-maven-reactor.md @@ -5,56 +5,57 @@ ## Context -`sequeless-filter` was a single `jar` module holding the entire filter -DSL: AST model, ANTLR 4 grammar/parser, serializer, operator and field registries, and -completion utilities. A SQL adapter (`FilterNode` AST → SQL) is arriving as a second module and -must not inherit the ANTLR runtime dependency it never uses — a plain single-jar layout has no -way to express "this dependency belongs to one consumer of the DSL, not the DSL itself." -Publishing does not exist yet for this project: there is no `` and no -`mvn deploy` step anywhere in CI, so there is no real external consumer whose published -coordinates would be broken by this restructuring. +`sequeless-filter` was a single `jar` module holding the entire filter DSL: +AST model, ANTLR 4 grammar/parser, serializer, operator and field registries, and completion +utilities. + +A SQL adapter (`FilterNode` AST → SQL) is arriving as a second module. It must not inherit the +ANTLR runtime dependency it never uses — a single-jar layout has no way to express "this +dependency belongs to one consumer of the DSL, not the DSL itself." + +Publishing doesn't exist for this project today: there's no `` and no +`mvn deploy` step anywhere in CI, so there's no real external consumer whose published coordinates +this restructuring could break. ## Decision -We will convert the root `pom.xml` into a `pom`-packaged aggregator/parent that keeps the -existing coordinates `org.sequeless:sequeless-filter`, and move all existing source under a new -`filter-core` module (`org.sequeless:filter-core`). The two modules share one -parent-inherited version — children declare no `` of their own. `jackson-databind` and -`antlr4-runtime` move to the parent's `` only, so each module must opt in -explicitly by redeclaring the dependency it actually needs (this is what will let the upcoming -`filter-sql-adapter` module avoid pulling in ANTLR). `lombok`, `junit-jupiter`, `assertj-core`, -`mockito-core`, and `archunit-junit5` stay as plain, parent-inherited ``, since -every module wants all five and redeclaring them per module would buy nothing. All build plugins -stay in the parent's inherited `` exactly as before, with one exception: -`flatten-maven-plugin` moves to `` with no `` element, and the -aggregator simply never declares/binds it in its own `` — each child declares it -bare (groupId + artifactId only) and inherits the full managed configuration. +Convert the root `pom.xml` into a `pom`-packaged aggregator/parent that keeps the existing +coordinates `org.sequeless:sequeless-filter`, and move all existing source under a new +`filter-core` module (`org.sequeless:filter-core`). + +- **One shared, parent-inherited version.** Children declare no `` of their own. +- **`jackson-databind` and `antlr4-runtime` move to ``-only.** Each module + must opt in by redeclaring the dependency it actually needs — this is what lets the upcoming + `filter-sql-adapter` module avoid pulling in ANTLR. +- **`lombok`, `junit-jupiter`, `assertj-core`, `mockito-core`, `archunit-junit5` stay as plain, + parent-inherited ``.** Every module wants all five, so redeclaring them per module + would buy nothing. +- **All build plugins stay in the parent's inherited ``, with one exception:** + `flatten-maven-plugin` moves to `` with no `` element, and the aggregator + never declares/binds it in its own ``. Each child declares it bare + (groupId + artifactId only) and inherits the full managed configuration. ## Consequences -Published coordinates change from `sequeless-filter` to `filter-core` for the actual DSL jar. -This is accepted and intentionally left unmarked as a breaking change in commit messages, since -nothing is published today and there is no real external consumer to break. - -`.releaserc.json` must now commit `*/pom.xml` (alongside the root `pom.xml`) as a release asset, -because `versions:set -DprocessAllModules=true` rewrites both the aggregator's `` and -each child's `` — omitting the child glob would leave the child POM's parent -reference stale after a release commit. - -JaCoCo coverage reports are now produced per module (e.g. `filter-core/target/site/jacoco/`) -with no aggregate report across the reactor. This is deliberate, not an oversight — a coverage -aggregator module is not needed until there is a concrete reason to view coverage across -modules in one report. - -The Sonar configuration in `sonar-project.properties` was deliberately left untouched by this -change. CI's SonarCloud job has never actually executed an analysis: every run to date has -logged "SONAR_TOKEN not set; skipping SonarCloud scan." and exited 0, so whether -`sonar-project.properties` (a SonarScanner-CLI-style file) is even read by the -Maven-integrated `sonar:sonar` goal CI invokes remains unconfirmed either way — there is no -"ANALYSIS SUCCESSFUL ... dashboard?id=..." log line to point at a working configuration to -preserve or fix. Speculatively migrating Sonar properties into the parent POM without that -confirmation risks guessing wrong about the project key or paths it depends on. Whoever enables -`SONAR_TOKEN` later must check the resulting analysis log's `dashboard?id=` line and then either -fix `sonar.coverage.jacoco.xmlReportPaths` to point at -`filter-core/target/site/jacoco/jacoco.xml`, or migrate the relevant Sonar properties into the -parent POM's `` and retire `sonar-project.properties` at that point. +- **Published coordinates change** from `sequeless-filter` to `filter-core` for the actual DSL jar. + This is accepted and intentionally left unmarked as a breaking change in commit messages, since + nothing is published today and there's no real external consumer to break. +- **`.releaserc.json` must commit `*/pom.xml`** (alongside the root `pom.xml`) as a release asset. + `versions:set -DprocessAllModules=true` rewrites both the aggregator's `` and each + child's `` — omitting the child glob would leave the child POM's parent + reference stale after a release commit. +- **JaCoCo coverage reports are now produced per module** (e.g. `filter-core/target/site/jacoco/`), + with no aggregate report across the reactor. Deliberate, not an oversight — a coverage aggregator + module isn't needed until there's a concrete reason to view coverage across modules in one report. +- **`sonar-project.properties` was deliberately left untouched.** CI's SonarCloud job has never + actually executed an analysis — every run to date logs "SONAR_TOKEN not set; skipping SonarCloud + scan." and exits 0. Whether `sonar-project.properties` (a SonarScanner-CLI-style file) is even + read by the Maven-integrated `sonar:sonar` goal CI invokes remains unconfirmed: there's no + "ANALYSIS SUCCESSFUL ... dashboard?id=..." log line to point at a working configuration to + preserve or fix. Speculatively migrating Sonar properties into the parent POM without that + confirmation risks guessing wrong about the project key or paths it depends on. + + **Follow-up for whoever enables `SONAR_TOKEN` later:** check the resulting analysis log's + `dashboard?id=` line, then either fix `sonar.coverage.jacoco.xmlReportPaths` to point at + `filter-core/target/site/jacoco/jacoco.xml`, or migrate the relevant Sonar properties into the + parent POM's `` and retire `sonar-project.properties`. diff --git a/docs/sql-adapter.md b/docs/sql-adapter.md index b47bd37..4fde5eb 100644 --- a/docs/sql-adapter.md +++ b/docs/sql-adapter.md @@ -2,59 +2,61 @@ `filter-sql-adapter` (`org.sequeless:filter-sql-adapter`) translates a parsed `FilterNode` AST into raw, parameterized SQL — a `String` with `?` placeholders plus an ordered bind-parameter list, the -classic JDBC `PreparedStatement` shape. It has no dependency on any particular database driver and -no ORM/Criteria object graph: you get SQL text and values back, and you run them yourself. +classic JDBC `PreparedStatement` shape. No database driver dependency, no ORM/Criteria object graph: +you get SQL text and values back, and you run them yourself. -This page covers the translation pipeline, the `JsonNode`→Java value-conversion rules, the -distinction between vendor-defined `LIKE` escaping and mandated identifier quoting, how to write a -vendor-specific `QueryBuilder`, and this adapter's known limitations. +**On this page:** the [translation pipeline](#the-pipeline), the +[value-conversion rules](#value-conversion-jsonnode--java), the +[`LIKE` escaping vs. identifier quoting](#like-escaping-vs-identifier-quoting) distinction, +[writing a vendor `QueryBuilder`](#writing-a-vendor-querybuilder), and +[known limitations](#known-limitations). See [docs/filter-dsl.md](filter-dsl.md) for the DSL itself (grammar, operators, parsing). ## The pipeline +`FilterQueryTranslator` (`org.sequeless.filter.sql.api`) is the single vendor-agnostic entry point; +`DefaultFilterQueryTranslator` is its (and currently only) implementation. + ``` -DSL text --FilterParser.parse--> FilterNode - | - v - FilterQueryTranslator.translate(node, columns, fields, ops, builder) - | - 1. FilterValidator.validate(node, ops, fields) -- internal, not opt-in - 2. walk the tree, expanding `any` per-node via AnyFilterExpander's default strategy - 3. resolve each path via ColumnMapping - 4. convert each JsonNode value to a plain Java value - 5. delegate rendering to QueryBuilder - | - v - SqlFragment(sql, parameters) - | - v - PreparedStatement + positional setObject(...) +DSL text + │ FilterParser.parse + ▼ +FilterNode ──► FilterQueryTranslator.translate(node, columns, fields, ops, builder) + 1. validate FilterValidator.validate(node, ops, fields) — internal, not opt-in + 2. walk the tree, expanding `any` per-node via AnyFilterExpander's default strategy + 3. resolve each path via ColumnMapping + 4. convert each JsonNode value to a plain Java value + 5. delegate rendering to QueryBuilder + ▼ + SqlFragment(sql, parameters) + ▼ + PreparedStatement + positional setObject(...) ``` -`FilterQueryTranslator` (`org.sequeless.filter.sql.api`) is the single vendor-agnostic entry point; -`DefaultFilterQueryTranslator` is its (and currently only) implementation. A few behaviors worth -calling out explicitly: - -- **Validation is internal, not opt-in.** Nothing else in this library calls `FilterValidator` - automatically — `FilterParser.parse` does not validate. `translate(...)` calls - `FilterValidator.validate(node, ops, fields)` itself, first, and converts any reported violation - to `SqlRenderException`. This means `translate(...)` is safe to call directly against an - unvalidated, hand-built, or deserialized `FilterNode` — you never need a separate validation step - before calling it. -- **`any` is expanded internally, per-node, using the default expansion strategy.** As the walker - visits an `AnyFilter` node, it calls `AnyFilterExpander`'s default strategy right there (not once - over the whole tree up front) and continues walking the resulting operands. Any operand whose - path has no `ColumnMapping` entry is silently dropped rather than failing the whole filter; - `SqlRenderException` is raised only if *no* expanded operand survives. This leniency applies only - to `any`-expansion results — an explicitly-named path anywhere else in the tree still hard-fails - on an unmapped column exactly as before. There is no way to supply a *custom* - `AnyExpansionStrategy` through `translate(...)` — see [Known limitations](#known-limitations). -- **`ColumnMapping` resolves each path to a bare column name**, and (per its `identity()` factory) - rejects dotted paths — see [Known limitations](#known-limitations). -- **`QueryBuilder` owns all SQL construction** — identifier quoting, operator-to-SQL translation, - value binding, `AND`/`OR` combination. The translator never renders SQL text itself; it only - walks, validates, resolves, and converts, then delegates. +Four behaviors worth calling out explicitly: + +**Validation is internal, not opt-in.** Nothing else in this library calls `FilterValidator` +automatically — `FilterParser.parse` does not validate. `translate(...)` calls +`FilterValidator.validate(node, ops, fields)` itself, first, and converts any reported violation to +`SqlRenderException`. You never need a separate validation step before calling it, even against an +unvalidated, hand-built, or deserialized `FilterNode`. + +**`any` is expanded internally, per-node, using the default expansion strategy.** As the walker +visits an `AnyFilter` node, it calls `AnyFilterExpander`'s default strategy right there — not once +over the whole tree up front — and continues walking the resulting operands. An operand whose path +has no `ColumnMapping` entry is silently dropped rather than failing the whole filter; +`SqlRenderException` is raised only if *no* expanded operand survives. This leniency is scoped to +`any`-expansion results only — an explicitly-named path anywhere else in the tree still hard-fails on +an unmapped column. There's no way to supply a *custom* `AnyExpansionStrategy` through +`translate(...)` — see [Known limitations](#known-limitations). + +**`ColumnMapping` resolves each path to a bare column name**, and (per its `identity()` factory) +rejects dotted paths — see [Known limitations](#known-limitations). + +**`QueryBuilder` owns all SQL construction** — identifier quoting, operator-to-SQL translation, value +binding, `AND`/`OR` combination. The translator never renders SQL text itself; it only walks, +validates, resolves, and converts, then delegates. ### Usage example @@ -119,49 +121,44 @@ for free: | any other numeric node (e.g. `BigIntegerNode`, `DecimalNode`) | via `isNumber()`/`numberValue()` | | anything else (`ObjectNode`, `BinaryNode`, `POJONode`, `MissingNode`, ...) | `SqlRenderException` | -Two things worth calling out explicitly: +Two caveats worth calling out explicitly: -- **No temporal coercion.** There is no date/time literal in the grammar, so a field with +- **No temporal coercion.** There's no date/time literal in the grammar, so a field with `jsonSchemaFormat: "date-time"` still arrives as a plain `TextNode` and converts to a plain `String`. Binding that string against a `TIMESTAMP`/`DATE` column is left entirely to the vendor `QueryBuilder`/JDBC driver — this adapter performs no parsing or coercion of date-time values. - **`BigInteger` binding caveat.** A `BigIntegerNode` (a JSON integer past `Long.MAX_VALUE`, which can only arise from a deserialized `FilterNode`, not the parser) converts via the numeric - catch-all to a raw `BigInteger`. Most JDBC drivers' `PreparedStatement.setObject` do not accept a - `BigInteger` directly and will throw at execution time — this is a known limitation of the - current conversion table, not something worked around elsewhere in the pipeline. If your values - may exceed `Long.MAX_VALUE`, be prepared to handle this at the JDBC layer (e.g. converting to - `BigDecimal` or a `String` column yourself). - -A `null` Java value passed to `QueryBuilder.binary(...)` means SQL `NULL` (e.g. from -`status is null`, i.e. a `NullNode` converted to `null`) — it does **not** mean "no value." Dispatch + catch-all to a raw `BigInteger`. Most JDBC drivers' `PreparedStatement.setObject` don't accept a + `BigInteger` directly and will throw at execution time. If your values may exceed + `Long.MAX_VALUE`, be prepared to handle this at the JDBC layer (e.g. converting to `BigDecimal` + or a `String` column yourself). + +A `null` Java value passed to `QueryBuilder.binary(...)` means SQL `NULL` — e.g. from +`status is null`, a `NullNode` converted to `null` — it does **not** mean "no value." Dispatch between `binary(...)` and `unary(...)` is always by the operator's own `isUnary()` flag, never by whether the converted value is `null`, so `status is null` reliably renders `IS NULL` rather than being mistaken for a unary condition like `exists`. ## `LIKE` escaping vs. identifier quoting -These look like similar "vendor gets to decide the details" questions but are not the same kind of -decision: - -- **`LIKE`-family escaping (`contains`, `starts with`, `is like`, `is not like`) is vendor-defined, - by design.** Whether a literal `%` or `_` inside a value is escaped before being placed in a - `LIKE` pattern — and therefore whether such a value matches literally or as a wildcard — is left - entirely to each `QueryBuilder` implementation's discretion. The shipped `AnsiQueryBuilder` - performs **no** escaping: `contains` renders `%value%`, `starts with` renders `value%`, and - `is like`/`is not like` use the value as-is (the caller supplies its own wildcards). A different - `QueryBuilder` — including a different vendor adapter you write yourself — may escape `%`/`_` - differently, and no cross-vendor test in this repository asserts one specific outcome. This is a - matching-semantics choice, not a security boundary. -- **Identifier quoting on the shipped default is mandated, because it's a security boundary, not a - style choice.** `AnsiQueryBuilder.quoteIdentifier` emits `"…"` with any embedded `"` doubled, and - its Javadoc states this explicitly: this is the one thing standing between a caller-supplied - column name and SQL injection when a `ColumnMapping` (especially `identity()`, which performs no - validation of its own) hands the translator a path that ultimately came from programmatic or - deserialized input rather than the grammar's own restricted identifier token - (`[a-zA-Z_][a-zA-Z_0-9]*`). A subclass that overrides `quoteIdentifier` must preserve an - equivalent injection-safe guarantee for its target dialect's quoting syntax — this is not free to - change for cosmetic reasons the way `LIKE` escaping is. +These look like similar "vendor gets to decide the details" questions, but they aren't the same +kind of decision. + +| | `LIKE`-family escaping | Identifier quoting | +| --- | --- | --- | +| Covers | `contains`, `starts with`, `is like`, `is not like` | Every column name the translator emits | +| Shipped default (`AnsiQueryBuilder`) | **No escaping.** `contains` → `%value%`, `starts with` → `value%`, `is like`/`is not like` use the value as-is (caller supplies wildcards) | `quoteIdentifier` emits `"…"` with any embedded `"` doubled | +| Vendor-overridable? | Yes, freely — a matching-semantics choice, not a security boundary | Yes, but must preserve an equivalent injection-safe guarantee | +| Why it's not fixed cross-vendor | Whether `%`/`_` inside a value is escaped changes literal-vs-wildcard matching; no cross-vendor test asserts one outcome | N/A — this one is fixed on the shipped default | + +Identifier quoting on the shipped default is mandated because it's the one thing standing between a +caller-supplied column name and SQL injection — specifically when a `ColumnMapping` (especially +`identity()`, which performs no validation of its own) hands the translator a path that came from +programmatic or deserialized input rather than the grammar's own restricted identifier token +(`[a-zA-Z_][a-zA-Z_0-9]*`). A subclass overriding `quoteIdentifier` must preserve that guarantee for +its target dialect's quoting syntax — it isn't free to change for cosmetic reasons the way `LIKE` +escaping is. ## Writing a vendor `QueryBuilder` @@ -201,32 +198,12 @@ stays free of any compile-time reference to a specific concrete adapter. ## Known limitations -- **Flat, single-table column mappings only.** `ColumnMapping` returns a bare column name, and a - `SqlFragment` is a single `WHERE`-clause fragment plus its binds — there is no way to express a - join, a correlated subquery, or a JSON path expression. This matters because the DSL's own - headline example, `lineItems.qty > 5`, typically means a join on a relational schema, not a - column — that shape is out of scope for this adapter as specified. If you have a real column - backing a dotted path (e.g. via a joined view), you can still map it explicitly with - `ColumnMapping.of(Map)`. -- **`ColumnMapping.identity()` rejects dotted paths.** Because of the single-table limitation above, - `identity()` returns `Optional.empty()` for any path containing `.`, which the translator turns - into a clean `SqlRenderException` at translation time — rather than producing a syntactically - valid but nonexistent quoted identifier like `"lineItems.qty"` that would only fail later as a - vendor-specific "column does not exist" error at execution time. This restriction is scoped to - `identity()` only; `ColumnMapping.of(Map)` is unaffected and can map a dotted path deliberately. - Also note `identity()` is documented as trusted-input-only — it performs no other validation, so - paths from untrusted or deserialized input should use an explicit `of(Map)` allowlist instead. -- **A custom `AnyExpansionStrategy` is unreachable through this adapter.** `translate(...)` always - uses `AnyFilterExpander`'s default expansion strategy internally; there is no parameter to supply - a different one. If you need non-default `any` expansion semantics, pre-expand the filter - yourself (with your own `AnyExpansionStrategy`) before calling `translate(...)`. -- **`LIKE`-family escaping is vendor-defined**, not a fixed cross-vendor contract — see - [`LIKE` escaping vs. identifier quoting](#like-escaping-vs-identifier-quoting) above. -- **Date-time values pass through as plain strings**, with no temporal coercion — see - [Value conversion](#value-conversion-jsonnode--java) above. -- **`BigInteger` values may not bind via `PreparedStatement.setObject`** on most JDBC drivers — see - [Value conversion](#value-conversion-jsonnode--java) above. -- **`QueryBuilder` and `FilterQueryTranslator` implementations are expected to be stateless and - thread-safe.** This isn't enforced by any type, but callers will naturally hold one instance of - each as a long-lived singleton shared across request threads — `DefaultFilterQueryTranslator` and - `AnsiQueryBuilder` both hold no mutable state, and a subclass should preserve that. +| Limitation | Why / workaround | +| --- | --- | +| **Flat, single-table column mappings only.** No join, correlated subquery, or JSON path expression. | `ColumnMapping` returns a bare column name and `SqlFragment` is a single `WHERE`-clause fragment plus binds. Matters because the DSL's headline example, `lineItems.qty > 5`, typically means a join on a relational schema, not a column. If you have a real column backing a dotted path (e.g. a joined view), map it explicitly with `ColumnMapping.of(Map)`. | +| **`ColumnMapping.identity()` rejects dotted paths** — returns `Optional.empty()`, which becomes `SqlRenderException`. | Fails fast at translation time instead of producing a valid-looking but nonexistent quoted identifier (`"lineItems.qty"`) that would only fail later as a vendor-specific "column does not exist" error. Scoped to `identity()` only — `ColumnMapping.of(Map)` can map a dotted path deliberately. `identity()` is also trusted-input-only (no other validation); use an explicit `of(Map)` allowlist for untrusted/deserialized paths. | +| **A custom `AnyExpansionStrategy` is unreachable through this adapter.** | `translate(...)` always uses `AnyFilterExpander`'s default strategy internally, with no parameter to override it. Pre-expand the filter yourself with your own strategy before calling `translate(...)` if you need different semantics. | +| **`LIKE`-family escaping is vendor-defined**, not a fixed cross-vendor contract. | See [`LIKE` escaping vs. identifier quoting](#like-escaping-vs-identifier-quoting). | +| **Date-time values pass through as plain strings**, no temporal coercion. | See [Value conversion](#value-conversion-jsonnode--java). | +| **`BigInteger` values may not bind via `PreparedStatement.setObject`** on most JDBC drivers. | See [Value conversion](#value-conversion-jsonnode--java). | +| **`QueryBuilder`/`FilterQueryTranslator` implementations are expected to be stateless and thread-safe.** | Not enforced by any type — callers will naturally hold one long-lived singleton instance shared across request threads. `DefaultFilterQueryTranslator` and `AnsiQueryBuilder` both hold no mutable state; a subclass should preserve that. |