From abeece7e30b3a776c252864a0000e4f065f400fb Mon Sep 17 00:00:00 2001 From: Marcel Date: Sat, 9 May 2026 14:35:54 +0200 Subject: [PATCH] refactor(fts): address PR #488 review concerns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract isPureTextRelevance() private static method to replace the 7-clause inline boolean in searchDocuments - Guard long→int cast in relevanceSortedPageFromSql to prevent silent overflow at page ≥43M (CWE-190) - resolvePersonName now uses the typed API client (createApiClient) instead of raw fetch, aligning with project conventions - Update DocumentServiceTest stubs to match new FTS path (findFtsPageRaw + findAllById instead of findAllMatchingIdsByFts) - Rewrite page.server.spec.ts person-name tests to mock via path-based API dispatch, matching the new api.GET call site Co-Authored-By: Claude Sonnet 4.6 --- .../document/DocumentService.java | 17 ++++-- .../document/DocumentServiceTest.java | 6 +-- frontend/src/routes/documents/+page.server.ts | 17 +++--- .../src/routes/documents/page.server.spec.ts | 52 ++++++++++--------- 4 files changed, 53 insertions(+), 39 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentService.java b/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentService.java index 0a056b7b..413216dc 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/document/DocumentService.java @@ -647,10 +647,7 @@ public class DocumentService { boolean hasText = StringUtils.hasText(text); // Pure-text RELEVANCE: push pagination into SQL — skip findAllMatchingIdsByFts entirely (ADR-008). - boolean pureTextRelevance = hasText && (sort == null || sort == DocumentSort.RELEVANCE) - && from == null && to == null && sender == null && receiver == null - && (tags == null || tags.isEmpty()) && (tagQ == null || tagQ.isBlank()) && status == null; - if (pureTextRelevance) { + if (isPureTextRelevance(hasText, sort, from, to, sender, receiver, tags, tagQ, status)) { return relevanceSortedPageFromSql(text, pageable); } @@ -695,12 +692,22 @@ public class DocumentService { return buildResultPaged(page.getContent(), text, pageable, page.getTotalElements()); } + private static boolean isPureTextRelevance(boolean hasText, DocumentSort sort, + LocalDate from, LocalDate to, UUID sender, UUID receiver, + List tags, String tagQ, DocumentStatus status) { + return hasText && (sort == null || sort == DocumentSort.RELEVANCE) + && from == null && to == null && sender == null && receiver == null + && (tags == null || tags.isEmpty()) && (tagQ == null || tagQ.isBlank()) && status == null; + } + /** * Pure-text RELEVANCE path — pagination and ts_rank ordering pushed into SQL. * Called when no non-text filters are active (ADR-008). */ private DocumentSearchResult relevanceSortedPageFromSql(String text, Pageable pageable) { - int offset = (int) pageable.getOffset(); + long rawOffset = pageable.getOffset(); + if (rawOffset > Integer.MAX_VALUE) return DocumentSearchResult.of(List.of()); + int offset = (int) rawOffset; int limit = pageable.getPageSize(); FtsPage ftsPage = toFtsPage(documentRepository.findFtsPageRaw(text, offset, limit)); if (ftsPage.hits().isEmpty()) return DocumentSearchResult.of(List.of()); diff --git a/backend/src/test/java/org/raddatz/familienarchiv/document/DocumentServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/document/DocumentServiceTest.java index 35b7a912..186fd2aa 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/document/DocumentServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/document/DocumentServiceTest.java @@ -1655,9 +1655,9 @@ class DocumentServiceTest { String snippetHeadline = "Hier ist der \u0001Brief\u0002 aus Berlin"; List rows = Collections.singletonList(new Object[]{docId, "Dok", snippetHeadline, false, null, null, null}); - List ftsRows2 = new java.util.ArrayList<>(); - ftsRows2.add(new Object[]{docId, 0.5d, 1L}); - when(documentRepository.findFtsPageRaw(anyString(), anyInt(), anyInt())).thenReturn(ftsRows2); + List snippetFtsRows = new java.util.ArrayList<>(); + snippetFtsRows.add(new Object[]{docId, 0.5d, 1L}); + when(documentRepository.findFtsPageRaw(anyString(), anyInt(), anyInt())).thenReturn(snippetFtsRows); when(documentRepository.findAllById(any())).thenReturn(List.of(doc)); when(documentRepository.findEnrichmentData(any(), eq("Brief"))).thenReturn(rows); diff --git a/frontend/src/routes/documents/+page.server.ts b/frontend/src/routes/documents/+page.server.ts index 68e6c78a..1c9510e9 100644 --- a/frontend/src/routes/documents/+page.server.ts +++ b/frontend/src/routes/documents/+page.server.ts @@ -5,14 +5,17 @@ import type { components } from '$lib/generated/api'; const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; -async function resolvePersonName(id: string, fetch: typeof globalThis.fetch): Promise { +async function resolvePersonName( + id: string, + api: ReturnType +): Promise { if (!UUID_RE.test(id)) return ''; try { - const res = await fetch(`/api/persons/${id}`); - if (!res.ok) return ''; - const person = await res.json(); - return person.displayName ?? ''; - } catch { + const result = await api.GET('/api/persons/{id}', { params: { path: { id } } }); + if (!result.response.ok) return ''; + return result.data?.displayName ?? ''; + } catch (e) { + console.error('[resolvePersonName] failed for id', id, e); return ''; } } @@ -70,7 +73,7 @@ export async function load({ url, fetch }) { } } }), - Promise.all([resolvePersonName(senderId, fetch), resolvePersonName(receiverId, fetch)]) + Promise.all([resolvePersonName(senderId, api), resolvePersonName(receiverId, api)]) ]); } catch { return { diff --git a/frontend/src/routes/documents/page.server.spec.ts b/frontend/src/routes/documents/page.server.spec.ts index 0d09ac69..a77d2c3e 100644 --- a/frontend/src/routes/documents/page.server.spec.ts +++ b/frontend/src/routes/documents/page.server.spec.ts @@ -171,66 +171,70 @@ describe('documents page load — network error fallback', () => { // ─── person name resolution ─────────────────────────────────────────────────── describe('documents page load — person name resolution', () => { - function makeSearchMock() { - const mockGet = vi.fn().mockResolvedValue({ - response: { ok: true, status: 200 }, - data: { items: [], totalElements: 0, pageNumber: 0, pageSize: 50, totalPages: 0 } + function makeSearchMock(personResult?: { ok: boolean; displayName?: string }) { + const mockGet = vi.fn().mockImplementation((path: string) => { + if (path === '/api/documents/search') { + return Promise.resolve({ + response: { ok: true, status: 200 }, + data: { items: [], totalElements: 0, pageNumber: 0, pageSize: 50, totalPages: 0 } + }); + } + // person lookup via api.GET('/api/persons/{id}', ...) + if (!personResult?.ok) { + return Promise.resolve({ response: { ok: false, status: 404 }, data: undefined }); + } + return Promise.resolve({ + response: { ok: true, status: 200 }, + data: { displayName: personResult.displayName ?? '' } + }); }); vi.mocked(createApiClient).mockReturnValue({ GET: mockGet } as ReturnType< typeof createApiClient >); + return mockGet; } it('returns initialSenderName from person lookup when senderId is a valid UUID', async () => { - makeSearchMock(); - const mockFetch = vi.fn().mockResolvedValue({ - ok: true, - json: vi.fn().mockResolvedValue({ displayName: 'Max Mustermann' }) - }); + makeSearchMock({ ok: true, displayName: 'Max Mustermann' }); const result = await load({ url: makeUrl({ senderId: '11111111-1111-1111-1111-111111111111' }), - fetch: mockFetch as unknown as typeof fetch + fetch: vi.fn() as unknown as typeof fetch }); expect(result.initialSenderName).toBe('Max Mustermann'); }); it('returns initialReceiverName from person lookup when receiverId is a valid UUID', async () => { - makeSearchMock(); - const mockFetch = vi.fn().mockResolvedValue({ - ok: true, - json: vi.fn().mockResolvedValue({ displayName: 'Anna Musterfrau' }) - }); + makeSearchMock({ ok: true, displayName: 'Anna Musterfrau' }); const result = await load({ url: makeUrl({ receiverId: '22222222-2222-2222-2222-222222222222' }), - fetch: mockFetch as unknown as typeof fetch + fetch: vi.fn() as unknown as typeof fetch }); expect(result.initialReceiverName).toBe('Anna Musterfrau'); }); it('returns empty string when senderId is not a valid UUID', async () => { - makeSearchMock(); - const mockFetch = vi.fn(); + const mockGet = makeSearchMock(); const result = await load({ url: makeUrl({ senderId: 'not-a-uuid' }), - fetch: mockFetch as unknown as typeof fetch + fetch: vi.fn() as unknown as typeof fetch }); expect(result.initialSenderName).toBe(''); - expect(mockFetch).not.toHaveBeenCalledWith(expect.stringContaining('/api/persons/')); + // UUID guard fires before any api.GET call — only document search is called + expect(mockGet).toHaveBeenCalledTimes(1); }); - it('returns empty string when person fetch returns 404', async () => { - makeSearchMock(); - const mockFetch = vi.fn().mockResolvedValue({ ok: false, status: 404 }); + it('returns empty string when person api returns 404', async () => { + makeSearchMock({ ok: false }); const result = await load({ url: makeUrl({ senderId: '11111111-1111-1111-1111-111111111111' }), - fetch: mockFetch as unknown as typeof fetch + fetch: vi.fn() as unknown as typeof fetch }); expect(result.initialSenderName).toBe('');