Repository navigation
feat: return full CSV results up to 100,000 rows; cap JSON limit at 5,000 - #11
Conversation
…,000 Same behavior as CLEANDB-api#20. CSV is not paginated: without a limit it returns every match, streamed through a server-side cursor into a spooled temp file, and a query matching more than 100,000 records gets a 400 instead of a partial file. An explicit JSON limit above the 5,000-record page size gets a 422. CSV skips the COUNT query. This replaces #8's silent cap on both formats and its CSV Link/X-Total-Count headers.
matthewberry
left a comment
There was a problem hiding this comment.
Looks good. Reproduced: full suite 44 passed, and against #10's app/ the new tests give 4 failures and 9 errors, matching the PR body.
Checked:
- Temp file closed on disconnect: holds for the installed versions. uvicorn 0.34.2 reports ASGI
spec_version2.3, which sends Starlette 0.46.2'sStreamingResponsedown its task-group path, and that path runsbackground()after a cancel. On spec 2.4 a disconnect would raiseClientDisconnectfirst and skip the background task. If a future uvicorn upgrade switches to 2.4, the file would then be closed by garbage collection instead. - Connection release: the nested
aclosing(initer_filtered_dataand inwrite_csv) releases the cursor's connection on the over-cap 400 and on errors. This is from reading the code; the 15-parallel-request run in the PR body is the evidence it works.
Minor: dropping #8's X-Total-Count/Link headers means a CSV client paging with an explicit limit has no total, so it can only stop when it gets a short page. That's reasonable, since without limit it gets everything, but it's worth a line in the docs.
Couldn't reproduce, and it predates this PR: the data query has no ORDER BY, so limit/offset pages aren't guaranteed to be consistent between requests. The new 400 message tells callers to page that way. With a 35k-row table it's unlikely to show up.
…nto mjberry/feat/full-csv-export
|
Thanks for checking the ASGI path. Fixed the docs point in 8605b9e (after merging #10's update in 5e576a9): the API description and the On the background task: agreed that on ASGI spec 2.4 a disconnect would skip On |
Stacked on #10; merge that first. If #10's branch is deleted on merge, GitHub retargets this PR to
main; otherwise change the base tomainbefore merging.Brings
/api/v1/datain line with moleculemaker/CLEANDB-api#20, so the two APIs treat large requests the same way.Problem
#8 capped every request at 5,000 records, in both formats, to stop single requests from exhausting memory. That made the two APIs inconsistent, and for CSV it removed something useful:
LinkandX-Total-Countheaders so a capped file isn't silently mistaken for a complete one, but a CSV client still has to page.limitabove 5,000 was quietly reduced to 5,000. CLEANDB-api rejects it with a 422 instead.The memory problem #8 fixed came from the
rich.printof every result set, which is gone. Streaming the CSV keeps a full export small.Changes
limit: returns every matching record, up toMAX_DATA_LIMIT(100,000). If the query matches more, it returns a 400 asking the caller to narrow the filters or page withlimit/offset; it never returns a partial file. The export requestscap + 1rows, andwrite_csvreturns the 400 if the extra row arrives, so the cap is checked against the rows actually written, in one query.Database.iterate, prefetch 1000) into aSpooledTemporaryFile(in memory up to 8 MB, then on disk). The DB connection is released before the response starts, so a slow client never holds it. The file is closed by aBackgroundTask, which also runs when the client disconnects. The row generators are closed withaclosing, so stopping early (the 400, or an error) releases the cursor's connection immediately.limit: returned as asked, up to 100,000 (fix: reject out-of-range limit and offset; previous past the end goes to the last records #10's bound onlimitis now 100,000 instead of the bigint maximum).COUNTquery, and no longer has fix: cap explicit page sizes and stop printing query results #8'sLink/X-Total-Countheaders: without a limit there is nothing to page, and with one the caller is paging already.columnsif given, otherwise the first row's keys, as before. (Unlike CLEANDB-api, this table has more columns than theOEDColumnenum lists.) An empty result withcolumnshas a header-only file; with neither, the file is empty.limitcapped at the page size: an explicit JSONlimitaboveAUTO_PAGINATION_THRESHOLD(5,000) gets a 422 shaped like thele=error. JSON is paginated, so nothing becomes unreachable. A JSON request withoutlimitis auto-paginated as before, with links on every page.get_filtered_data's query building moved intobuild_data_query, shared with the newiter_filtered_data.HTTPExceptionnow passes through the endpoint's catch-all instead of being rewrapped as a 500./datadocstring explain how each format behaves.API clients that send a JSON
limitabove 5,000 will get a 422. Within moleculemaker, nothing calls/datathat way (the OpenEnzymeDB frontend only links to the API docs, andoed-toolkitdoesn't call it).Testing
tests/test_data_csv.py, ported from CLEANDB-api'stest_search_csv.py: 12 tests with the DB calls mocked, covering the header (from rows, fromcolumns, and header-only when nothing matches), returning more than a JSON page without a limit, the cap (and that the row iterator is closed when it's passed), the explicit-limit path, CSV limits above the JSON cap, alimitabove 100,000 (422), a DB error partway through (500), output large enough to spill to disk, and the cursor and row generator being closed early.tests/test_auto_pagination.py: fix: cap explicit page sizes and stop printing query results #8's "explicit limit is capped" and "capped CSV has headers" tests are replaced by one asserting a JSONlimitof 10 is accepted and 11 gets a 422 with the threshold at 10.MockDatabasegainsiterate, recording queries likefetch, so the existing CSV integration test runs through the new path.oed.oed_datacreated fromdb_migration/oed_03_data.sql:offset=1the remaining 100,000 are returnedformat=csv&limit=100001, JSONlimit=5001andlimit=-1get 422s naminglimit; JSONlimit=5000returns a pageec=…), explicitly paged, andcolumns=CSV requests return the expected rows and headeridle, and a JSON request afterwards returned 200