refactor(fts): address PR #488 review concerns
Some checks failed
CI / Unit & Component Tests (push) Has been cancelled
CI / OCR Service Tests (push) Has been cancelled
CI / Backend Unit Tests (push) Has been cancelled
CI / Unit & Component Tests (pull_request) Failing after 4m48s
CI / OCR Service Tests (pull_request) Successful in 53s
CI / Backend Unit Tests (pull_request) Failing after 3m31s

- 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 <noreply@anthropic.com>
This commit is contained in:
Marcel
2026-05-09 14:35:54 +02:00
parent df1f56ca6d
commit abeece7e30
4 changed files with 53 additions and 39 deletions

View File

@@ -647,10 +647,7 @@ public class DocumentService {
boolean hasText = StringUtils.hasText(text); boolean hasText = StringUtils.hasText(text);
// Pure-text RELEVANCE: push pagination into SQL — skip findAllMatchingIdsByFts entirely (ADR-008). // Pure-text RELEVANCE: push pagination into SQL — skip findAllMatchingIdsByFts entirely (ADR-008).
boolean pureTextRelevance = hasText && (sort == null || sort == DocumentSort.RELEVANCE) if (isPureTextRelevance(hasText, sort, from, to, sender, receiver, tags, tagQ, status)) {
&& from == null && to == null && sender == null && receiver == null
&& (tags == null || tags.isEmpty()) && (tagQ == null || tagQ.isBlank()) && status == null;
if (pureTextRelevance) {
return relevanceSortedPageFromSql(text, pageable); return relevanceSortedPageFromSql(text, pageable);
} }
@@ -695,12 +692,22 @@ public class DocumentService {
return buildResultPaged(page.getContent(), text, pageable, page.getTotalElements()); 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<String> 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. * Pure-text RELEVANCE path — pagination and ts_rank ordering pushed into SQL.
* Called when no non-text filters are active (ADR-008). * Called when no non-text filters are active (ADR-008).
*/ */
private DocumentSearchResult relevanceSortedPageFromSql(String text, Pageable pageable) { 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(); int limit = pageable.getPageSize();
FtsPage ftsPage = toFtsPage(documentRepository.findFtsPageRaw(text, offset, limit)); FtsPage ftsPage = toFtsPage(documentRepository.findFtsPageRaw(text, offset, limit));
if (ftsPage.hits().isEmpty()) return DocumentSearchResult.of(List.of()); if (ftsPage.hits().isEmpty()) return DocumentSearchResult.of(List.of());

View File

@@ -1655,9 +1655,9 @@ class DocumentServiceTest {
String snippetHeadline = "Hier ist der \u0001Brief\u0002 aus Berlin"; String snippetHeadline = "Hier ist der \u0001Brief\u0002 aus Berlin";
List<Object[]> rows = Collections.singletonList(new Object[]{docId, "Dok", snippetHeadline, false, null, null, null}); List<Object[]> rows = Collections.singletonList(new Object[]{docId, "Dok", snippetHeadline, false, null, null, null});
List<Object[]> ftsRows2 = new java.util.ArrayList<>(); List<Object[]> snippetFtsRows = new java.util.ArrayList<>();
ftsRows2.add(new Object[]{docId, 0.5d, 1L}); snippetFtsRows.add(new Object[]{docId, 0.5d, 1L});
when(documentRepository.findFtsPageRaw(anyString(), anyInt(), anyInt())).thenReturn(ftsRows2); when(documentRepository.findFtsPageRaw(anyString(), anyInt(), anyInt())).thenReturn(snippetFtsRows);
when(documentRepository.findAllById(any())).thenReturn(List.of(doc)); when(documentRepository.findAllById(any())).thenReturn(List.of(doc));
when(documentRepository.findEnrichmentData(any(), eq("Brief"))).thenReturn(rows); when(documentRepository.findEnrichmentData(any(), eq("Brief"))).thenReturn(rows);

View File

@@ -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; 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<string> { async function resolvePersonName(
id: string,
api: ReturnType<typeof createApiClient>
): Promise<string> {
if (!UUID_RE.test(id)) return ''; if (!UUID_RE.test(id)) return '';
try { try {
const res = await fetch(`/api/persons/${id}`); const result = await api.GET('/api/persons/{id}', { params: { path: { id } } });
if (!res.ok) return ''; if (!result.response.ok) return '';
const person = await res.json(); return result.data?.displayName ?? '';
return person.displayName ?? ''; } catch (e) {
} catch { console.error('[resolvePersonName] failed for id', id, e);
return ''; 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 { } catch {
return { return {

View File

@@ -171,66 +171,70 @@ describe('documents page load — network error fallback', () => {
// ─── person name resolution ─────────────────────────────────────────────────── // ─── person name resolution ───────────────────────────────────────────────────
describe('documents page load — person name resolution', () => { describe('documents page load — person name resolution', () => {
function makeSearchMock() { function makeSearchMock(personResult?: { ok: boolean; displayName?: string }) {
const mockGet = vi.fn().mockResolvedValue({ const mockGet = vi.fn().mockImplementation((path: string) => {
if (path === '/api/documents/search') {
return Promise.resolve({
response: { ok: true, status: 200 }, response: { ok: true, status: 200 },
data: { items: [], totalElements: 0, pageNumber: 0, pageSize: 50, totalPages: 0 } 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< vi.mocked(createApiClient).mockReturnValue({ GET: mockGet } as ReturnType<
typeof createApiClient typeof createApiClient
>); >);
return mockGet;
} }
it('returns initialSenderName from person lookup when senderId is a valid UUID', async () => { it('returns initialSenderName from person lookup when senderId is a valid UUID', async () => {
makeSearchMock(); makeSearchMock({ ok: true, displayName: 'Max Mustermann' });
const mockFetch = vi.fn().mockResolvedValue({
ok: true,
json: vi.fn().mockResolvedValue({ displayName: 'Max Mustermann' })
});
const result = await load({ const result = await load({
url: makeUrl({ senderId: '11111111-1111-1111-1111-111111111111' }), 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'); expect(result.initialSenderName).toBe('Max Mustermann');
}); });
it('returns initialReceiverName from person lookup when receiverId is a valid UUID', async () => { it('returns initialReceiverName from person lookup when receiverId is a valid UUID', async () => {
makeSearchMock(); makeSearchMock({ ok: true, displayName: 'Anna Musterfrau' });
const mockFetch = vi.fn().mockResolvedValue({
ok: true,
json: vi.fn().mockResolvedValue({ displayName: 'Anna Musterfrau' })
});
const result = await load({ const result = await load({
url: makeUrl({ receiverId: '22222222-2222-2222-2222-222222222222' }), 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'); expect(result.initialReceiverName).toBe('Anna Musterfrau');
}); });
it('returns empty string when senderId is not a valid UUID', async () => { it('returns empty string when senderId is not a valid UUID', async () => {
makeSearchMock(); const mockGet = makeSearchMock();
const mockFetch = vi.fn();
const result = await load({ const result = await load({
url: makeUrl({ senderId: 'not-a-uuid' }), 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(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 () => { it('returns empty string when person api returns 404', async () => {
makeSearchMock(); makeSearchMock({ ok: false });
const mockFetch = vi.fn().mockResolvedValue({ ok: false, status: 404 });
const result = await load({ const result = await load({
url: makeUrl({ senderId: '11111111-1111-1111-1111-111111111111' }), 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(''); expect(result.initialSenderName).toBe('');