refactor(document): extract a SearchFilters record for the document search signatures (#683) #702
Reference in New Issue
Block a user
Delete Branch "feat/issue-683-search-filters-record"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #683.
What
Threads a filter-only
SearchFiltersrecord through the entire document search call chain so no positional 10-field filter list survives anywhere in the path. Pure refactor — no behaviour change.searchDocuments(SearchFilters, DocumentSort, String dir, Pageable)— sort/dir/pageable stay separate (not filter predicates).findIdsForFilter(SearchFilters)— clean single-arg.buildSearchSpec,runSearch,countUndatedForFilter,isPureTextRelevance) no longer carry the positional 10-field list.tagOp/undatedcoercions.Why
searchDocumentshad 13 positional params wheresender/receiver(bothUUID) andfrom/to(bothLocalDate) sat adjacent — a transposition compiles cleanly and silently returns the wrong rows. A named record kills the entire class. Mirrors the existingDensityFiltersprecedent; the two records are kept separate as decided in the issue.Key invariants preserved
searchDocumentsbuilds the base filter once, passesfilters.withUndated(true)to the count path whilerunSearchhonours the user's toggle.searchDocuments_undatedTrue_usesSpecificationPath_notPureTextRelevanceShortcutstays green, unmodified.@RequirePermission(WRITE_ALL)ongetDocumentIds— untouched (controller method annotation, independent of the service signature).generate:apizero diff — the controller diff touches no@RequestParam, mapping,@RequestBody, or response type, so the OpenAPI contract is byte-for-byte identical.Scope note
buildSearchSpecis shared with the density path (loadFilteredDates). Changing its signature forced that call site too — theDensityFiltersis adapted into aSearchFilters(null dates,undated=false) right at the call, keeping the two records separate.Tests
No new tests (pure refactor). Controller binding tests swap positional
any()/eq()matchers forArgumentCaptor<SearchFilters>, assertingcaptured.undated()/.status()/.sender()— strictly stronger than the previousany()-soup.All green:
DocumentServiceTest168 ·DocumentControllerTest104 ·DocumentServiceSortTest7 ·DocumentSearchPagedIntegrationTest8 ·UndatedDocumentOrderingIntegrationTest6 ·DocumentListItemIntegrationTest4 ·DocumentLazyLoadingTest5 ·DocumentDensityIntegrationTest7../mvnw clean package -DskipTests✅🤖 Generated with Claude Code
🏛️ Markus Keller — Senior Application Architect
✅ Approved
A textbook parameter-object refactor. Ten positional arguments — including two
UUIDs (sender/receiver) and twoLocalDates (from/to) that compile cleanly when transposed — are now bundled into a named, immutable record threaded through the whole search chain (searchDocuments/findIdsForFilter/buildSearchSpec/runSearch/countUndatedForFilter/isPureTextRelevance+ controller). Pure behavior-preserving refactor: no SQL, no specification logic, no endpoint signature changes. The accidental-transposition risk this removes is real, and the call sites are now legible.Architecture lens
Layer boundaries — clean.
SearchFilterslives indocument/alongsideDensityFilters,DocumentSort, and the rest of the domain. It is an intra-domain value object, never crosses a module boundary, imports onlytag.TagOperator(already a legitimate cross-domain dependency the search path used before). No new coupling introduced.Value-object purity — correct.
SearchFilters.java:28-43is a plain record: no validation, no coercion, no business logic. The single helperwithUndated(boolean)is a pure copy-with-override returning a new instance — the canonical immutable-record idiom, not behavior. The boolean-default /Boolean.TRUE.equals(undated)coercion stays at the HTTP boundary in the controller where it belongs (DocumentController.java:319,392), not inside the record. Exactly right.The DensityFilters → SearchFilters adapter (
DocumentService.java:170-173) — this is the right minimal bridge, not unwanted coupling. The adaptation is local to the density path's privateloadFilteredDates; it widens the seven density fields into the ten-field search bundle by supplyingfrom=null, to=null, undated=falseat the one sharedbuildSearchSpeccall.DensityFiltersitself remains unaware ofSearchFilters— the dependency points one way, density → search, and only inside a single private method. Field mapping verified correct (text/sender/receiver/tags/tagQ/status/tagOperator line up one-to-one).Keep-records-separate decision — endorsed, and the Javadoc on both records documents the why (
SearchFilters.java:21-23,DensityFilters.java:9-10): density deliberately excludes date/undated semantics, so merging the records would force the density path to reason about — and pass dummy values for — fields it has no business knowing. Three null/false literals at one call site is a far cheaper price than a shared type that leaks date semantics into the density domain. This is the correct DRY-vs-coupling trade: a tiny duplication beats an over-general shared type. I'd resist any later "consolidate these two records" suggestion for exactly this reason.Documentation currency — no updates triggered, correctly. No new package, route, controller, service, migration,
ErrorCode, orPermission.SearchFiltersis a new value object inside the existingdocumentpackage, not a new domain module — the C4l3-backenddiagrams and theCLAUDE.mdpackage table operate at package/service granularity, not per-record, so nothing is stale. No ADR required: this is a clean-code refactor, not a structural decision with lasting consequences (and it conforms to the existingDensityFiltersprecedent rather than setting a new one).Blockers
None.
Suggestions
None blocking. One optional note: the test fixtures now construct
new SearchFilters(null, null, …, false)with ten positional args in many places — the very positional-fragility this record removes from production code is now concentrated in tests. Not worth churning this PR, but if aSearchFilters.builder()or a test helper (emptyFilters()) emerges later, the unfiltered-search test setups would read better. Production call sites are already clear; this is test ergonomics only.Solid, well-scoped, well-documented refactor. Ship it.
🎨 Leonie Voss — UX Designer & Accessibility
✅ Approved — nothing in my domain to evaluate. This is a pure backend Java refactor with zero client-visible surface.
What I checked
git diff 2cc43c3c HEAD --name-onlyfiltered for.svelte/.ts/.js/.css/.json→ none. The diff touches only 9 Java files (DocumentController,DocumentService, the newSearchFiltersrecord, and their tests).findIdsForFilter,searchDocuments) keep their exact@RequestParambindings at the HTTP boundary. The change only collapses the parameter list into an internalSearchFiltersrecord passed between controller and service. Query-param names, types, and the JSON response shape are unchanged —generate:apiis expected to produce a zero diff. No old client breaks.No UX, accessibility, brand, or responsive concerns. Clean refactor from my seat.
👨💻 Felix Brandt — Senior Fullstack Developer
✅ Approved
This is exactly what a behaviour-preserving refactor should look like. The 13-positional-param signature was a swap-bug magnet (two UUIDs
sender/receiver, twoLocalDatesfrom/to), andSearchFilterscollapses it into one named, immutable bundle. No behaviour change, no lost coverage.Behaviour preservation — verified
I traced the two paths that could plausibly drift:
buildSearchSpecnever readsfilters.text()(DocumentService.java:520-531). The text predicate is driven byhasText+ftsIds, exactly as before. So the newSearchFilterscarriestextpurely as a passenger for the helpers that do need it (searchDocuments,findIdsForFilter). Puttingfilters.text()into the record inloadFilteredDatesis inert for the spec — identical to the old call that started atfrom.withUndated(true)is a faithful 1:1 replacement for the old forcedbuildSearchSpec(..., true)flag (DocumentService.java:685), whilerunSearchstill receives the originalfilterscarrying the user's real toggle. The#668undated-count semantics are untouched, and the comment block at659-673still guards the pure-text RELEVANCE fast path against the undated flag correctly.Test rewrites — faithful, and in two spots stronger
The mechanical
any(), any(), …collapses are correct. Two call-sites improved rather than just survived:search_withStatusParam_passesItToService(DocumentControllerTest.java:142): oldeq(DocumentStatus.REVIEWED)matcher-in-stub → explicitassertThat(filtersCaptor.getValue().status()).isEqualTo(REVIEWED). Clearer failure message, same guarantee.getDocumentIds_passesSenderIdParamToService(:404-406): oldeq(senderId)→filtersCaptor.sender()assertion. Same.No assertions were dropped; the integration tests (e.g. the date-range case at
DocumentSearchPagedIntegrationTest.java:178) preserve positional order into the record faithfully —(null, from, to, null…)maps totext, from, to, sender…correctly.Clean code
SearchFilters.java— record + singlewithUndatedcopy method, no logic. The Javadoc earns its place: it documents why sort/dir/pagination are excluded and whyDensityFiltersstays a separate sibling rather than being folded in. That's a "why" comment, not a "what" — correct per our style.isPureTextRelevancereads cleanly off the record; guard-clause shape preserved.searchDocuments(/findIdsForFilter(acrosssrc/mainandsrc/test; every call goes throughSearchFilters.Is the DensityFilters→SearchFilters bridge a smell?
loadFilteredDates(DocumentService.java:168-175) adaptingDensityFilters→SearchFilterswith literalnull, nullfor from/to andfalsefor undated is acceptable, not a smell. The two records are deliberately kept distinct (density excludes date/undated by design — documented in both Javadocs), so the bridge is the honest seam between "I have no date concept" and "the shared spec builder wants the full filter shape". The literals are explicit and local. The only marginally nicer alternative would be aSearchFilters.fromDensity(DensityFilters)factory to name the bridge — but that's a taste call, not a defect, and KISS argues against adding a factory used exactly once.Suggestions (non-blocking, optional)
SearchFilters.fromDensity(...)factory — would name the bridge and keep thenull, null, …, falseliterals out ofloadFilteredDates. Worth it only if a second density-style adapter ever appears; today YAGNI.Nothing to change. Ship it.
🔒 Nora Steiner ("NullX") — Application Security Engineer
Verdict: Approve — no security delta. This is a clean signature-only refactor that bundles ten already-typed search predicates into a
SearchFiltersrecord. Every security boundary I checked is preserved bit-for-bit. No blockers, one minor observation.I reviewed the full diff (
2cc43c3c..HEAD) and readDocumentController.javain full.Verified controls (all intact)
1.
@RequirePermission(Permission.WRITE_ALL)on/ids— present and unmoved.DocumentController.java:304-305— the annotation sits directly abovegetDocumentIds, exactly where it was. The bulk-edit ID enumeration path stays write-gated, so aREAD_ALL-only user still gets a 403 (enforced byPermissionAspect). TheBULK_EDIT_FILTER_MAX_IDScap (line 321) that prevents materialising the whole table is also untouched.2. Null-Boolean and raw-String coercion stay at the HTTP boundary — fail-safe to
false/AND.DocumentController.java:319and:392—Boolean.TRUE.equals(undated)collapses a null boxedBooleanto primitivefalsebefore the record is constructed.Boolean.TRUE.equals(null)returnsfalseby contract — no NPE, no auto-unboxing risk. Correct: an omittedundatedparam resolves to "not undated-only", not an exception.DocumentController.java:318and:391—tagOp(rawString) is mapped to theTagOperatorenum at the boundary; any non-ORvalue defaults toAND. The threat-model comment at:389-390is retained.3. No new injection surface.
Queries remain JPA
Specificationpredicates assembled inbuildSearchSpec(DocumentService.java:81-99) —isBetween,hasSender,hasReceiver,hasTags,hasTagPartial,hasStatus,undatedOnly. All parameterized via the Criteria API; zero string concatenation introduced. FTS still routes throughfindAllMatchingIdsByFts(filters.text())(:59,:126), i.e. the existing named-parameter repository method. No raw SQL/JPQL added. (CWE-89: not present.)4. The record carries only already-typed values.
SearchFilters(SearchFilters.java:228-238) declaresboolean undated(primitive) andTagOperator tagOperator— no rawStringoperator and no boxedBooleanleaks past the controller into the service. The boundary→typed conversion happens once, at the controller, which is exactly where I want it.withUndated(true)(:241-243) is an immutable copy used only for the undated-count path — no mutation of the original filter, no shared-state aliasing.5. No auth check dropped or weakened on
/searchvs/ids./search(:367) was and remains intentionally unannotated — it is aREAD_ALLread path, reachable by any authenticated user (the twosearch_undated*tests at the controller level still assert this, guarding against an accidental write-guard per #668)./idsstaysWRITE_ALL. The asymmetry is deliberate and unchanged.Test surface — strengthened, not weakened
The captor-based assertions actually improve the security posture of the suite:
getDocumentIds_passesSenderIdParamToServicenow assertsfiltersCaptor.getValue().sender()(DocumentControllerTest.java), and the undated forwarding tests assertfiltersCaptor.getValue().undated()istrue/false— semantic field checks that survive future signature churn better than positionalanyBoolean()matchers did.Minor (non-blocking) — detection note for the future
The fail-safe at
:319/:392is correct but implicit. A one-line guard against a future "simplification" that swapsBoolean.TRUE.equals(undated)forundated(which would NPE-on-unbox for the common omitted-param case):A Semgrep rule
pattern: new SearchFilters(..., $B)where$Bis a boxedBooleanwould catch the regression class at scale, but that is overkill for a 10-field record — the unit test is sufficient.Nothing here changes who can reach what or how input is parameterized. Ship it.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved — no concerns in my domain.
I reviewed this PR strictly for infrastructure/platform impact. It is a pure backend Java refactor introducing a
SearchFiltersrecord; nothing touches my domain.What I checked (all clear):
Dockerfile,docker-compose*.yml, or overlay changes.gitea/or.github/; no runner, cache, or action-version changes.sqlfiles (the refactor doesn't alter the schema)application*.propertiesor build-config changesThe full diff is confined to
backend/src/main/java/.../document/(DocumentController,DocumentService, newSearchFilters.java) and the correspondingsrc/test/javafiles. The only CI-visible effect is the existing test suite, which the author reports green — exactly what I'd expect from a behavior-preserving refactor.Nothing for me to flag. Ship it.
🧪 Sara Holt — Senior QA Engineer
Verdict: Approve. The matcher→
ArgumentCaptor<SearchFilters>conversion is faithful — every value assertion from the old positionaleq()/captor tests survives as a field assertion on the captured record. No test was silently downgraded to all-any(). This is a clean, behaviour-preserving refactor.Critical check: were any value assertions lost? — No.
I traced each load-bearing assertion across the conversion:
search_undatedTrue_isForwardedToServiceAsTrueundatedCaptor→isTrue()filtersCaptor.getValue().undated()→isTrue()search_withoutUndatedParam_forwardsFalseToServiceundatedCaptor→isFalse().undated()→isFalse()search_withStatusParam_passesItToServiceeq(DocumentStatus.REVIEWED).status()→isEqualTo(REVIEWED)getDocumentIds_passesSenderIdParamToServiceeq(senderId).sender()→isEqualTo(senderId)search_passesPageRequestToServiceThe
when(...).thenReturn(...)stub lines that collapsed to all-any()are harmless — the assertion lives in the pairedverify(...captor.capture()...)+ field assert, not the stub. No test left asserting nothing.Transposition check (the real risk in this refactor) — clean.
The record order
(text, from, to, sender, receiver, tags, tagQ, status, tagOperator, undated)matches the oldfindIdsForFilterpositional order exactly, so those conversions are a straight 1:1. ThesearchDocumentsconversions are the tricky ones because the old signature interleavedsort, dirbetweenstatusandtagOperator; I spot-checked each reshuffle:DocumentServiceTeststatus test (searchDocuments_withStatusFilter..., was line ~1600):DocumentStatus.REVIEWEDlands in record slot 8 (status),sort/dir/pageablestay out. ✅undated=truecorrectly in slot 10,RELEVANCEstays as thesortarg — still proves the FTS-id path runs (findAllMatchingIdsByFts("brief")verified) and the raw-page SQL shortcut is skipped becauseundatedshort-circuits the fast path. ✅DocumentServiceSortTestrelevance + active filter (...uses_inMemory_path):fromcorrectly placed in slot 2, keeping the date filter active soisPureTextRelevancereturns false and the in-memory path is taken —never() findFtsPageRawstill asserted. ✅findIdsForFiltertag-OR test:tags=List.of("Brief")in slot 6,TagOperator.ORin slot 9. ✅#668 undated-count tests — intact.
In
DocumentSearchPagedIntegrationTest, both #668 guards keep their assertions through the call-shape change only:undated=trueboth still assertundatedCount == undatedTotal(count ignores the toggle);undatedCount().isZero()(collision rule).The production side backs this:
searchDocumentspassesfilters.withUndated(true)intocountUndatedForFilter, andwithUndated()copies all ten fields — verified field-for-field, no field dropped.Relevance fast-path bypass — preserved.
isPureTextRelevance(hasText, sort, filters)mapsfrom/to/sender/receiver/tags/tagQ/statusone-to-one from the old param list, andsearchDocumentsstill gates it behind!filters.undated()(was!undated). No behavioural drift.Blockers
None.
Suggestions (non-blocking, optional)
new SearchFilters(null, null, null, null, null, null, null, null, null, false)literal now appears ~20 times across the integration/unit tests. A small private factory —private static SearchFilters noFilters()/filters().undated(true).build()— would kill the positional-null wall and make the intent (which field is non-null in a given test) jump out. This is exactly the transposition footgun the production record was introduced to remove; mirroring it in the test fixtures would finish the job. Pure readability, no behaviour change.isEqualTovs inlineeq()matcher) — a nice side benefit worth keeping in mind as the pattern for future captor conversions.Confidence is high that "no behaviour change" holds at the test level. Ship it.
📋 Elicit — Requirements Engineer
Verdict: Approve. Every acceptance criterion and every DoD checkbox in #683 traces to evidence in the diff. The one scope addition (density call site) is correctly handled and stays inside the "no behaviour change" contract. No blockers.
I reviewed this purely against the requirements I helped lock in the issue's persona round — does the PR deliver exactly what was specified, no more, no less.
AC-by-AC traceability
SearchFilters; four private helpers lose the positional 10-field listsearchDocuments(SearchFilters, DocumentSort, String dir, Pageable)andfindIdsForFilter(SearchFilters)are the new public signatures. All four helpers (buildSearchSpec,runSearch,countUndatedForFilter,isPureTextRelevance) now takeSearchFilters— no positional 10-field list survives anywhere in the chain. Verified line-by-line inDocumentService.java.searchDocumentsbuilds the base filter once, passesfilters.withUndated(true)tocountUndatedForFilterwhilerunSearchhonours the user toggle — exactly the edge case I flagged in my issue-round comment. The!undated &&relevance guard stays at the caller;isPureTextRelevancereads 9 fields and does not consumeundated. Felix reports the full named suite green. Caveat: I cannot independently re-run the suite from a diff; this rests on the reported run + CI.ArgumentCaptorpreserving value assertionsany()/eq()matchers swapped toArgumentCaptor<SearchFilters>withcaptured.undated(),.status(),.sender()assertions. The prioreq(senderId)/eq(REVIEWED)/undatedCaptorvalue checks are preserved as field assertions on the captured record — strictly stronger than the oldany()-soup, and the behaviour-preservation guard is intact.generate:apizero diff@RequestParam,@GetMapping/@PostMapping,@RequestBody,@Parameter, or response type — I confirmed all bindings present and unchanged inDocumentController.java.SearchFiltersis server-internal, built after param binding. The HTTP contract is byte-identical, so the OpenAPI spec cannot change. Judgement: structural verification is acceptable evidence here.generate:apireflects the binding surface, not internal call shapes; with zero binding-surface delta there is no mechanism by which the spec could drift. Spinning up the stack would add cost without changing the falsifiable conclusion.WRITE_ALLpreserved ongetDocumentIds@RequirePermission(Permission.WRITE_ALL)sits atDocumentController.java:305on the controller method, untouched by the diff. The aspect binds to the controller, not the service signature.ErrorCode/Permissionupdates triggereddocument), no route, no migration, noErrorCode, noPermission, no new glossary term (it renames nothing — it bundles an existing parameter list). Nothing to update.In-PR scope addition — density
buildSearchSpeccall siteThis is the one thing not in the issue's enumerated six-site list. Because
buildSearchSpecis shared with the density path (loadFilteredDates→DensityFilters), threadingSearchFiltersthrough it forced that call site too. Felix adaptedDensityFiltersinto aSearchFiltersright at the call (from=null, to=null, undated=false, mappingfilters.text()).Assessment: correctly handled, and within "no behaviour change." I traced the old vs. new args: the prior call passed
buildSearchSpec(hasFts, ftsIds, null, null, sender, receiver, tags, tagQ, status, tagOperator, false); the newSearchFilterscarries identical values in identical positions. The two records stay separate as decided (no consolidation, no coupling). This is a necessary consequence of the chosen Option-A threading — not scope creep — and the issue's "all sites in the path" framing arguably already covers it. Worth a one-line PR-description note (Felix added one), which keeps traceability honest.Requirements-quality observations (non-blocking)
SearchFiltersJavadoc carries the same swap-bug rationale and theDensityFilterssibling note I asked for in the issue round — the why is documented at the artifact, not just in the issue. Good.withUndated(boolean)copy method is the exact mechanism I called out as necessary to preserve the dual-undatedinvariant without mutating a shared record. It reads as intent. Resolved correctly.Suggestions (optional, non-blocking)
DensityFilters → SearchFiltersbridge inloadFilteredDatesis a small piece of glue that wasn't in the issue's enumerated scope. A one-line code comment at that call site (// density path has no date/undated predicates → null/null/false) would spare a future reader the trip back to this PR thread. Cosmetic.No open questions. This is a clean, well-bounded P3 refactor that delivers its spec exactly.
Follow-up — addressed the review's optional suggestions
All three "nice to have" items from the review are now in (3 commits pushed):
4c2f036— test ergonomics (Sara/Markus/Felix): collapsed the ~29 repeated all-nullnew SearchFilters(null, …)literals into a sharedSearchFiltersFixtures.noFilters()factory (+noFilters().withUndated(true)for the undated-only case). Tests that pin a specific field keep their explicit constructor so intent stays visible.2e44cab— density bridge comment (Elicit): a 3-line "why" comment atloadFilteredDatesexplaining theDensityFilters → SearchFiltersadaptation and the keep-separate rationale.3b594c0— coercion pin (Nora):getDocumentIds_withoutUndatedParam_coercesNullToFalse— the symmetric/idspin for theBoolean undated → falseboundary coercion (the/searchside was already pinned).Still no behaviour change. Suites green:
DocumentControllerTest105 (+1),DocumentServiceTest168,DocumentServiceSortTest7,DocumentSearchPagedIntegrationTest8,DocumentListItemIntegrationTest4,DocumentLazyLoadingTest5.