feat(search): NL search backend — POST /api/search/nl with Ollama integration (#738) #756
Reference in New Issue
Block a user
Delete Branch "worktree-feat+issue-738-nl-search-backend"
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 #738
Summary
POST /api/search/nl— natural-language query → structuredNlSearchResponseRestClientOllamaClient) with grammar-constrained JSON schema (Qwen 2.5 7B); graceful degradation on timeout/unavailable → 503SMART_SEARCH_UNAVAILABLENlQueryParserService: single-match, ambiguous (returns early), no-match folded into keywords, sender+receiver role detection, 3+ names with extra fragmentsNlSearchRateLimiter) → 429SMART_SEARCH_RATE_LIMITEDsearchDocumentsByPersonIdinDocumentServiceusing JPA Specification (avoids PostgreSQL null-type inference issue with JPQLIS NULL OR)smart_search_keywords_not_appliedNlSearchRequest,NlQueryInterpretation,NlSearchResponse,PersonHint)Test plan
NlQueryParserServiceTest— 23 Mockito testsNlSearchControllerTest— 7 @WebMvcTest tests (incl. 429 rate limit, 503 unavailable)RestClientOllamaClientTest— 4 WireMock testsNlSearchRateLimiterTest— 4 testsDocumentRepositoryTest— 34 tests (incl. 5 new Specification-based sender/receiver tests)curl -X POST /api/search/nl -d '{"query":"Was hat Walter im Krieg geschrieben?"}' | jq .→ 200 with NlSearchResponse shape (requires Ollama + qwen2.5:7b running)🤖 Generated with Claude Code
🏗️ Markus Keller (@mkeller) — Senior Application Architect
Verdict: ✅ Approved
The architecture is clean and well-thought-out. The PR earns a straightforward approval, with a couple of minor observations.
What's correct
search/is a new, bounded domain package. No layer packages created.NlQueryParserServicereaches intoPersonServiceandDocumentService— not their repositories.searchDocumentsByPersonIdinDocumentServicefetchesPersonviapersonService.getById(), notpersonRepository. ✅l1-context.puml✅l2-containers.puml+DEPLOYMENT.md✅CLAUDE.mdpackage table +l3-backend-3h-search.puml✅GLOSSARY.md✅ErrorCodevalues →CLAUDE.md+errors.ts✅docs/adr/028-nl-search-ollama.md✅Minor observations (not blockers)
buildPersonSpecuses raw field name strings —root.get("sender"),root.get("receivers")— instead of a JPA static metamodel. This is consistent with how the rest ofDocumentServiceuses Criteria API in this codebase, so it's not a regression. A compile-time checked metamodel would be more robust, but not required here.Inline full-qualified class names in
buildPersonSpec—jakarta.persistence.criteria.JoinType.LEFT,new java.util.ArrayList<>,new jakarta.persistence.criteria.Predicate[0]— without corresponding imports. Works fine, but it's stylistically noisy. Consider adding the imports at the top ofDocumentService.java.OllamaHealthClient.isHealthy()is implemented but not wired to a Spring BootHealthIndicator. The interface exists and is tested, but Actuator/actuator/healthwon't show Ollama's status. This is acceptable for the initial implementation; a future improvement could expose it as an actuator health contributor.👨💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer
Verdict: ✅ Approved
Solid TDD discipline throughout. 72 tests across 5 classes, all passing. Clean code with some minor style observations.
What's correct
@RequiredArgsConstructor. All new services use Lombok constructor injection. Dependencies arefinal. ✅NlQueryParserService.search()validates query length before calling Ollama. ✅@MockitoBean(Spring Boot 4) used inNlSearchControllerTest— not the deprecated@MockBean. ✅@Schema(requiredMode = REQUIRED)on all always-populated fields inPersonHint,NlQueryInterpretation,NlSearchResponse. TypeScript codegen is correct. ✅@AuthenticationPrincipal UserDetails principalin controller for rate limit key — matches the pattern fromInviteController. ✅personNames/keywordstoList.of()before use — no NPE risk. ✅Suggestions (not blockers)
DocumentService.buildPersonSpechas inline fully-qualified names —jakarta.persistence.criteria.JoinType.LEFT,new java.util.ArrayList<>,new jakarta.persistence.criteria.Predicate[0]— without top-level imports. These should be import statements, not inline FQNs.NlQueryParserServiceTestusesMockitoAnnotations.openMocks(this)in@BeforeEachrather than@ExtendWith(MockitoExtension.class)on the class. Both work, but the annotation approach is the project convention (seePersonServiceTest). Minor.Stray blank line in
DocumentRepository.java— the diff shows a blank line added betweenfindByReceiversId()and the comment. Small but unnecessary noise.Test naming is slightly inconsistent — most tests use
search_*prefix (search_resolvesSingleName_asSender) but repository tests usesearchByPersonSpec_*. Not a real issue, just noting the pattern divergence.isAnyRolereturns true for null/unknown role — the logicrole == null || "any".equals(role) || (!\"sender\".equals(role) && !"receiver".equals(role))correctly defaults unknown values to "any" behavior. ThesanitiseRolemethod inRestClientOllamaClientensures the role is always one of the three valid values before it reachesNlQueryParserService, making the|| (!sender && !receiver)branch technically unreachable from production code. It's still harmless defensive code.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No security vulnerabilities found. The implementation applies defense-in-depth correctly. A couple of observations worth noting for future work.
What's correct
@RequirePermission(Permission.READ_ALL)on the POST endpoint — type-safe, AOP-enforced. No magic strings. ✅@Valid @RequestBody NlSearchRequestwith@NotBlank @Size(min=3, max=500)— validates at the controller boundary before any downstream calls. ✅log.warn("Ollama inference failed: {}", e.getClass().getSimpleName()). No JNDI injection risk (CWE-117). ✅NlQueryParserServicenever logs the query string. The C4 L3 diagram even notes "Never logs raw query content (PII)". ✅personNames,personRole,dateFrom,dateTo,keywords. This is a reasonable mitigation against prompt injection via structured output fields. ✅tooManyRequests()(HTTP 429) with structuredErrorCode— the error does not expose implementation details in the response. ✅SMART_SEARCH_UNAVAILABLErather than proceeding with partially parsed data. ✅Observations (not blockers)
Rate limit error message includes the caller's username:
"NL search rate limit exceeded for user: " + userKey. IfDomainException.messageis serialized in the error response body, this leaks the username back to the caller. Looking at the existingLoginRateLimiterpattern, this appears to be accepted practice in this codebase. The username is already known to the caller (they provided it), so the practical impact is minimal. Still worth auditing theGlobalExceptionHandlerto confirmmessageis not serialized.OllamaHealthClientis not wired to Spring Boot Actuator health. The interface exists and is tested, but a degraded Ollama won't surface in/actuator/health. Consider adding aHealthIndicatorbean in a future PR so monitoring can detect Ollama outages before users hit 503s.Input validation is duplicated between
@Sizeannotation andNlQueryParserService.search(). Both enforce min=3/max=500. The annotation fires first (Spring validation layer), so the service guard is technically redundant. This is defensive programming — not a security concern — but the duplication could be removed from the service layer.🛠️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ⚠️ Approved with concerns
The backend code itself is infrastructure-ready. My concern is that the PR adds an Ollama container to the C4 L2 diagram and DEPLOYMENT.md — but no actual
docker-compose.ymlchanges. The implementation is ahead of its infrastructure.What's correct
application.yamluseshttp://ollama:11434— references the Docker Compose service name, not a hardcoded IP. ✅application-dev.yamloverrides tohttp://localhost:11434— dev profile correctly points to local Ollama. ✅3.9.2is version-pinned inpom.xml. Jetty ee10 transitive deps are also pinned to12.1.8. ✅DEPLOYMENT.mdOllama section covers the first-run model pull, health check, and config table. Good runbook. ✅timeoutSeconds: 30/healthCheckTimeoutSeconds: 2— reasonable defaults. 30 seconds for CPU inference is generous; 2 seconds for health is tight enough to detect early. ✅Concerns
Ollama is in the C4 diagrams but not in
docker-compose.yml. The PR description says the Compose service is tracked as issue #735. This means:System_Boundary(archiv, "Familienarchiv (Docker Compose)")— but it's not actually there yetDEPLOYMENT.mdwill be told to pullqwen2.5:7b-instruct-q4_K_Mbut won't have an Ollama container to pull it intoThis is acceptable as long as issue #735 ships soon and the note in DEPLOYMENT.md makes clear that Ollama requires manual setup until then. I'd suggest adding a callout in DEPLOYMENT.md that Ollama must be started separately until the Compose definition is merged.
No health check on the Ollama container. When the Compose definition ships in #735, it should include:
And the backend should depend on it:
condition: service_healthy. Without this, the backend starts before the model is loaded and the first requests 503.ollama/ollama:latest— when the Compose definition is written for #735, pin the image tag.:latestis not a version.🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist
Verdict: ✅ Approved
Strong test pyramid. 72 tests across the right layers. A few gaps worth tracking.
What's correct
Correct test slices used:
NlQueryParserServiceTest—@ExtendWith(MockitoExtension.class)style (viaopenMocks), no Spring context ✅NlSearchControllerTest—@WebMvcTestslice, not full@SpringBootTest✅DocumentRepositoryTest— Testcontainers real PostgreSQL, not H2 ✅NlSearchRateLimiterTest— plain JUnit, no Spring context ✅RestClientOllamaClientTest— WireMock for HTTP boundary ✅Rate limiter test isolation —
@BeforeEachcallsrateLimiter.resetForTest()inNlSearchControllerTest. Prevents bucket state leaking between tests. ✅Boundary conditions tested in
NlQueryParserServiceTest:ambiguousPersonshas 10 entries ✅personNames/keywords→ no NPE ✅VALIDATION_ERROR✅VALIDATION_ERROR✅SMART_SEARCH_UNAVAILABLE→ propagated ✅DocumentRepositoryTestSpecification cases including DISTINCT deduplication ✅Security boundaries tested:
SMART_SEARCH_UNAVAILABLE✅makeResponse()factory function in controller test — clean test data setup. ✅Gaps (not blockers, worth tracking)
NlQueryParserServiceTestusesMockitoAnnotations.openMocks(this)in@BeforeEachrather than@ExtendWith(MockitoExtension.class)on the class. Both are functionally equivalent, but the annotation form is the project standard (seePersonServiceTest). TheopenMocksapproach also requires manually cleaning up mock state — theMockitoExtensiondoes this automatically.OllamaHealthClient.isHealthy()has no dedicated test for the false case in context — onlyRestClientOllamaClientTesttests that a request succeeds. The health check is a GET/api/tagscall. Consider adding a test thatwireMock.stubFor(get("/api/tags").willReturn(serverError()))causesisHealthy()to returnfalse. Low priority since the interface isn't wired to an actuator health contributor yet.No test for the 3-name case in
NlQueryParserServiceTestwhere the 3rd name is also ambiguous (e.g. names=[Walter→resolved, Emma→resolved, Heinrich→multi-match]). The existing test covers Heinrich resolving to a single person and being folded intoextraFragments. The ambiguous 3rd-name path (ambiguous short-circuits before the 2-resolved check) is implicitly covered by test #7, but an explicit 3-name-with-ambiguous-third test would make the intent clear.WireMock timeout test uses
withFixedDelay(6000)andclient.timeoutSeconds = 5— depends on the test executing within the fixed delay window. On a very slow CI machine this could be flaky. Worth noting but not critical at current CI speeds.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
The implementation traces cleanly to the acceptance criteria in issue #738. All specified behaviors are present.
Requirements coverage
POST /api/search/nlendpointNlSearchControllerRestClientOllamaClientwith grammar-constrained JSON schemaNlQueryParserService.resolveNames()buildSender()/buildReceiver()ambiguousPersons, skip searchsearch()noMatchFragmentsinbuildText()parseDate()handles both formsNlSearchRateLimiterwithBucket4jSMART_SEARCH_UNAVAILABLEwhen Ollama unreachableSMART_SEARCH_RATE_LIMITEDwhen limit exceededkeywordsAppliedflag in responseNlQueryInterpretation.keywordsAppliedapi.tsregeneratedObservations
smart_search_keywords_not_appliedkey is added to all message files but unused in this PR. It's defined pre-emptively for the frontend issue (#735). This is fine as a forward declaration, but worth noting that it will silently go unused until the frontend lands.The
personRolefield's behavior for 2-resolved names bypassesisAnyRolecheck — when 2 names resolve,resolved.size() >= 2routes directly tobuildSender(0)/buildReceiver(1)regardless ofpersonRole. This means if Ollama sayspersonRole="sender"but two names match, the implementation treats them as sender+receiver pair. The issue spec implies this is intentional ("3+ names → extra fragments; sender+receiver role detection"), but the behavior wherepersonRoleis overridden by the 2-name heuristic is worth documenting explicitly in the ADR or as a code comment. Not a defect — just an implicit design choice.🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead
Verdict: ⚠️ Approved with concerns
This is a backend-only PR — no Svelte components, no layout changes. I focused on the i18n strings, as those directly affect what users see and hear.
What's correct
smart_search_keywords_not_appliedis clear and non-technical for users. ✅Blocker
German i18n strings use informal "du" form while the existing error messages use formal "Sie":
The Familienarchiv's primary senior audience (60+) expects formal address ("Sie") in error messages. Switching to "du" creates a tone inconsistency that stands out to older German speakers. The fix is two lines:
This should be fixed before merge since it's the only UI-visible artifact in this PR. The strings are not yet referenced in any frontend component, so no other changes are needed.
marcel referenced this pull request2026-06-06 19:15:56 +02:00