mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-13 22:32:04 +00:00
fix(search): bound the clinical kinds by the search budget, one search per actor, query in a POST body
This commit is contained in:
1 parent
138792d555
commit
41049b3837
19 files changed
+637
-178
No files matched your search
@@ -1,6 +1,6 @@
|
||||
/**
|
||||
* `GET /api/search` — admission, validation, the rate limit, and what the
|
||||
* route hands the search and the log.
|
||||
* `POST /api/search` — admission, validation, the rate limit, one search in
|
||||
* flight per actor, and what the route hands the search and the log.
|
||||
*
|
||||
* The search itself is `searchRecords`, covered by its own unit tests and by
|
||||
* `tests/integration/record-search.test.ts` against Postgres; here it is a
|
||||
@@ -49,12 +49,27 @@ vi.mock("@/lib/search/search-records", () => ({
|
||||
searchRecords: mocks.searchRecords,
|
||||
}));
|
||||
|
||||
import { GET } from "../route";
|
||||
import { POST } from "../route";
|
||||
import { searchesInFlight } from "@/lib/search/in-flight";
|
||||
|
||||
const handler = GET as unknown as (req: NextRequest) => Promise<Response>;
|
||||
const handler = POST as unknown as (req: NextRequest) => Promise<Response>;
|
||||
|
||||
function request(query: string): NextRequest {
|
||||
return new NextRequest(`http://localhost/api/search?${query}`);
|
||||
/**
|
||||
* A search request. Written as the query string the route used to take, so
|
||||
* every case reads as before; the route receives it as a JSON body.
|
||||
*/
|
||||
function request(query: string, init: { signal?: AbortSignal } = {}) {
|
||||
const params = new URLSearchParams(query);
|
||||
const body: Record<string, unknown> = {};
|
||||
for (const [key, value] of params) {
|
||||
body[key] = key === "limit" && /^\d+$/.test(value) ? Number(value) : value;
|
||||
}
|
||||
return new NextRequest("http://localhost/api/search", {
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify(body),
|
||||
signal: init.signal,
|
||||
});
|
||||
}
|
||||
|
||||
const OWNER = { id: "owner-1", locale: "de" };
|
||||
@@ -93,7 +108,7 @@ beforeEach(() => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("GET /api/search", () => {
|
||||
describe("POST /api/search", () => {
|
||||
it("admits the whole record only, at read", async () => {
|
||||
await handler(request("q=schulter"));
|
||||
expect(mocks.requireRecordAuth).toHaveBeenCalledWith("read", "record");
|
||||
@@ -137,6 +152,7 @@ describe("GET /api/search", () => {
|
||||
["a zero limit", "q=knie&limit=0"],
|
||||
["a limit above the cap", "q=knie&limit=21"],
|
||||
["a non-numeric limit", "q=knie&limit=lots"],
|
||||
["a fractional limit", "q=knie&limit=2.5"],
|
||||
])("refuses %s with 422 and never searches", async (_label, query) => {
|
||||
const res = await handler(request(query));
|
||||
expect(res.status).toBe(422);
|
||||
@@ -173,4 +189,108 @@ describe("GET /api/search", () => {
|
||||
delegated: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps the query out of the URL", async () => {
|
||||
const req = request("q=Psychotherapie");
|
||||
expect(req.url).not.toMatch(/psychotherapie/i);
|
||||
const res = await handler(req);
|
||||
expect(res.status).toBe(200);
|
||||
expect(mocks.searchRecords.mock.calls[0][0].query).toBe("Psychotherapie");
|
||||
});
|
||||
|
||||
it("refuses a body that is not JSON", async () => {
|
||||
const res = await handler(
|
||||
new NextRequest("http://localhost/api/search", {
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "text/plain" },
|
||||
body: "q=schulter",
|
||||
}),
|
||||
);
|
||||
expect(res.status).toBe(415);
|
||||
expect(mocks.searchRecords).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("allows 20 searches a minute per actor", async () => {
|
||||
await handler(request("q=schulter"));
|
||||
expect(mocks.checkRateLimit).toHaveBeenCalledWith(
|
||||
"search:actor-1",
|
||||
20,
|
||||
60_000,
|
||||
);
|
||||
});
|
||||
|
||||
it("hands the search an abort signal that follows the client", async () => {
|
||||
const client = new AbortController();
|
||||
let seen: AbortSignal | undefined;
|
||||
mocks.searchRecords.mockImplementationOnce(async (args) => {
|
||||
seen = args.signal;
|
||||
client.abort();
|
||||
return {
|
||||
groups: [],
|
||||
truncated: true,
|
||||
stats: { sources: 1, rows: 0, candidates: 0, elapsedMs: 1 },
|
||||
};
|
||||
});
|
||||
await handler(request("q=schulter", { signal: client.signal }));
|
||||
expect(seen?.aborted).toBe(true);
|
||||
});
|
||||
|
||||
it("stops an actor's earlier search when a newer one starts", async () => {
|
||||
let release: () => void = () => {};
|
||||
let firstSignal: AbortSignal | undefined;
|
||||
mocks.searchRecords.mockImplementationOnce(
|
||||
(args) =>
|
||||
new Promise((resolve) => {
|
||||
firstSignal = args.signal;
|
||||
release = () =>
|
||||
resolve({
|
||||
groups: [],
|
||||
truncated: true,
|
||||
stats: { sources: 1, rows: 0, candidates: 0, elapsedMs: 1 },
|
||||
});
|
||||
}),
|
||||
);
|
||||
const first = handler(request("q=schul"));
|
||||
await vi.waitFor(() => expect(firstSignal).toBeDefined());
|
||||
expect(firstSignal?.aborted).toBe(false);
|
||||
|
||||
const second = await handler(request("q=schulter"));
|
||||
expect(second.status).toBe(200);
|
||||
expect(firstSignal?.aborted).toBe(true);
|
||||
|
||||
release();
|
||||
const superseded = await first;
|
||||
expect(superseded.status).toBe(409);
|
||||
expect((await superseded.json()).meta?.errorCode).toBe("search.superseded");
|
||||
expect(searchesInFlight()).toBe(0);
|
||||
});
|
||||
|
||||
it("does not stop another actor's search", async () => {
|
||||
let firstSignal: AbortSignal | undefined;
|
||||
let release: () => void = () => {};
|
||||
mocks.searchRecords.mockImplementationOnce(
|
||||
(args) =>
|
||||
new Promise((resolve) => {
|
||||
firstSignal = args.signal;
|
||||
release = () =>
|
||||
resolve({
|
||||
groups: [],
|
||||
truncated: false,
|
||||
stats: { sources: 1, rows: 0, candidates: 0, elapsedMs: 1 },
|
||||
});
|
||||
}),
|
||||
);
|
||||
const first = handler(request("q=schul"));
|
||||
await vi.waitFor(() => expect(firstSignal).toBeDefined());
|
||||
|
||||
mocks.requireRecordAuth.mockResolvedValueOnce({
|
||||
user: { id: "owner-2", locale: "de" },
|
||||
actor: { id: "actor-2", locale: "de" },
|
||||
grantId: null,
|
||||
});
|
||||
await handler(request("q=knie"));
|
||||
expect(firstSignal?.aborted).toBe(false);
|
||||
release();
|
||||
expect((await first).status).toBe(200);
|
||||
});
|
||||
});
|
||||
+80
-42
@@ -1,5 +1,6 @@
|
||||
/**
|
||||
* `GET /api/search?q=&limit=` — one search over the record (Discussion #1117):
|
||||
* `POST /api/search` with `{ q, limit }` — one search over the record
|
||||
* (Discussion #1117):
|
||||
* visits and procedures, conditions, documents (title and indexed text),
|
||||
* vaccinations, lab results, medications, symptoms, workouts and the notes on
|
||||
* mood entries, readings and illness days. Each hit carries the in-app path
|
||||
@@ -12,35 +13,49 @@
|
||||
* switched-off module is not read at all.
|
||||
*
|
||||
* The query is health data ("Schulter", a drug, a diagnosis). It never
|
||||
* reaches a log: the annotation carries its length and the counts only, and a
|
||||
* refused query is answered without echoing it.
|
||||
* reaches a log: it travels in the JSON body rather than the URL, so neither
|
||||
* this app's request log nor a reverse proxy's access log records it; the
|
||||
* annotation carries its length and the counts only, and a refused query is
|
||||
* answered without echoing it.
|
||||
*
|
||||
* Rate limited per actor: the palette asks once per pause in typing, and a
|
||||
* manager searching someone's record spends their own allowance.
|
||||
* Bounded per actor: 20 searches a minute (the palette asks once per pause in
|
||||
* typing, and a manager searching someone's record spends their own
|
||||
* allowance), and one at a time. A newer search by the same actor aborts the
|
||||
* one before it, and a client that goes away aborts its own, so an abandoned
|
||||
* search stops reading at its next budget check instead of running out its
|
||||
* time (`in-flight.ts`).
|
||||
*/
|
||||
import { NextRequest } from "next/server";
|
||||
|
||||
import { apiHandler, requireRecordAuth } from "@/lib/api-handler";
|
||||
import { apiError, apiSuccess, returnAllZodIssues } from "@/lib/api-response";
|
||||
import {
|
||||
apiError,
|
||||
apiSuccess,
|
||||
returnAllZodIssues,
|
||||
safeJson,
|
||||
} from "@/lib/api-response";
|
||||
import { prisma } from "@/lib/db";
|
||||
import { defaultLocale, locales, type Locale } from "@/lib/i18n/config";
|
||||
import { annotate } from "@/lib/logging/context";
|
||||
import { resolveModuleMap } from "@/lib/modules/gate";
|
||||
import { checkRateLimit } from "@/lib/rate-limit";
|
||||
import { beginSearch, SUPERSEDED } from "@/lib/search/in-flight";
|
||||
import { searchRecords } from "@/lib/search/search-records";
|
||||
import { searchQuerySchema } from "@/lib/search/wire-schemas";
|
||||
import { actingDomainVisibility } from "@/lib/sharing/acting-domains";
|
||||
import { resolveUserTimezone } from "@/lib/tz/resolver";
|
||||
|
||||
/** Searches per actor per minute: a few per typed word, with room to spare. */
|
||||
const SEARCH_RATE_LIMIT = 60;
|
||||
/** Searches per actor per minute: one per pause in typing, with room to spare. */
|
||||
const SEARCH_RATE_LIMIT = 20;
|
||||
const SEARCH_RATE_WINDOW_MS = 60_000;
|
||||
/** The body is a query of at most 100 characters and a number. */
|
||||
const SEARCH_BODY_MAX_BYTES = 2048;
|
||||
|
||||
function displayLocale(value: string | null | undefined): Locale {
|
||||
return locales.includes(value as Locale) ? (value as Locale) : defaultLocale;
|
||||
}
|
||||
|
||||
export const GET = apiHandler(async (request: NextRequest) => {
|
||||
export const POST = apiHandler(async (request: NextRequest) => {
|
||||
const { user, actor, grantId } = await requireRecordAuth("read", "record");
|
||||
|
||||
const rl = await checkRateLimit(
|
||||
@@ -52,10 +67,14 @@ export const GET = apiHandler(async (request: NextRequest) => {
|
||||
return apiError("Too many searches. Please retry shortly.", 429);
|
||||
}
|
||||
|
||||
const params = new URL(request.url).searchParams;
|
||||
const { data: body, error: jsonError } = await safeJson<{
|
||||
q?: unknown;
|
||||
limit?: unknown;
|
||||
}>(request, { maxBytes: SEARCH_BODY_MAX_BYTES });
|
||||
if (jsonError) return jsonError;
|
||||
const parsed = searchQuerySchema.safeParse({
|
||||
q: params.get("q") ?? undefined,
|
||||
limit: params.get("limit") ?? undefined,
|
||||
q: body?.q ?? undefined,
|
||||
limit: body?.limit ?? undefined,
|
||||
});
|
||||
if (!parsed.success) {
|
||||
annotate({
|
||||
@@ -67,35 +86,54 @@ export const GET = apiHandler(async (request: NextRequest) => {
|
||||
});
|
||||
}
|
||||
|
||||
const [modules, domainVisible, timezone] = await Promise.all([
|
||||
resolveModuleMap(user.id),
|
||||
actingDomainVisibility(prisma, grantId),
|
||||
resolveUserTimezone(user.id),
|
||||
]);
|
||||
const result = await searchRecords({
|
||||
recordId: user.id,
|
||||
query: parsed.data.q,
|
||||
limit: parsed.data.limit,
|
||||
modules,
|
||||
domainVisible,
|
||||
displayLocale: displayLocale(actor.locale),
|
||||
timezone,
|
||||
});
|
||||
|
||||
annotate({
|
||||
action: { name: "search.query.run" },
|
||||
meta: {
|
||||
query_length: parsed.data.q.length,
|
||||
const slot = beginSearch(actor.id);
|
||||
try {
|
||||
const [modules, domainVisible, timezone] = await Promise.all([
|
||||
resolveModuleMap(user.id),
|
||||
actingDomainVisibility(prisma, grantId),
|
||||
resolveUserTimezone(user.id),
|
||||
]);
|
||||
const result = await searchRecords({
|
||||
recordId: user.id,
|
||||
query: parsed.data.q,
|
||||
limit: parsed.data.limit,
|
||||
sources: result.stats.sources,
|
||||
rows_scanned: result.stats.rows,
|
||||
matches: result.stats.candidates,
|
||||
groups: result.groups.length,
|
||||
hits: result.groups.reduce((n, g) => n + g.hits.length, 0),
|
||||
truncated: result.truncated,
|
||||
elapsed_ms: result.stats.elapsedMs,
|
||||
delegated: grantId !== null,
|
||||
},
|
||||
});
|
||||
return apiSuccess({ groups: result.groups, truncated: result.truncated });
|
||||
modules,
|
||||
domainVisible,
|
||||
displayLocale: displayLocale(actor.locale),
|
||||
timezone,
|
||||
signal: AbortSignal.any([request.signal, slot.signal]),
|
||||
});
|
||||
|
||||
if (slot.signal.aborted) {
|
||||
// A newer search by the same actor took over; this answer is partial
|
||||
// and nobody is waiting for it.
|
||||
annotate({
|
||||
action: { name: "search.query.superseded" },
|
||||
meta: { query_length: parsed.data.q.length },
|
||||
});
|
||||
return apiError("A newer search replaced this one", 409, {
|
||||
errorCode: SUPERSEDED,
|
||||
});
|
||||
}
|
||||
|
||||
annotate({
|
||||
action: { name: "search.query.run" },
|
||||
meta: {
|
||||
query_length: parsed.data.q.length,
|
||||
limit: parsed.data.limit,
|
||||
sources: result.stats.sources,
|
||||
rows_scanned: result.stats.rows,
|
||||
matches: result.stats.candidates,
|
||||
groups: result.groups.length,
|
||||
hits: result.groups.reduce((n, g) => n + g.hits.length, 0),
|
||||
truncated: result.truncated,
|
||||
elapsed_ms: result.stats.elapsedMs,
|
||||
delegated: grantId !== null,
|
||||
aborted: request.signal.aborted,
|
||||
},
|
||||
});
|
||||
return apiSuccess({ groups: result.groups, truncated: result.truncated });
|
||||
} finally {
|
||||
slot.end();
|
||||
}
|
||||
});
|
||||
Reference in new issue
Block a user