Repository navigation
OpenConceptLab/ocl_online#470 | Cap concept, mapping and reference lists at LIST_MAX_LIMIT rows per page - #951
Conversation
…sts at LIST_MAX_LIMIT rows per page
8885762 to
1042cb6
Compare
snyaggarwal
left a comment
There was a problem hiding this comment.
I found two pagination regressions that need changes before this is merged. CI currently reports passing tests and pylint; these cases are not covered by the new tests.
…ermitted Compress lists uncapped
snyaggarwal
left a comment
There was a problem hiding this comment.
Rereviewed at 8689051. Both findings from my prior review are addressed: requests without limit retain the configured cap, and permitted compressed search/released-version requests avoid pre-slicing. The new tests cover both paths, and the current Tests and Pylint checks pass. I found no further blocking issue in this revision.
paynejd
left a comment
There was a problem hiding this comment.
Tests fail on one test, because of this morning's master merge, not your change. f24904c added the TEST_ES=off guard. MappingListViewTest.test_page_size_capped indexes into Elasticsearch (MappingDocument().update(...)), so the non-ES run stops it:
core.integration_tests.tests_mappings.MappingListViewTest sends PUT /_bulk?refresh=true to Elasticsearch, which TEST_ES=off forbids: tag its class @tag('es')
Master tags whole classes, so I'd move this test into its own @tag('es') class. Tagging MappingListViewTest itself would move its other tests into the serial ES run.
Go-live: the 100-row cap shouldn't reach production before the integrator notice (ocl_online#473) goes out. Merging is fine, but any later production deploy would carry it. Until the notice is out, set LIST_MAX_LIMIT=1000 in production, today's documented maximum, or hold the merge. #473 is with me; I'll comment there when it's sent.
Re-request my review once CI is green.
|
@snyaggarwal the integrator notice (ocl_online#473) is out. The cap goes live on Tuesday, November 10.
The paging docs the notice links are live (OpenConceptLab/ocl-docs#77). |
Part of OpenConceptLab/ocl_online#470. Related: OpenConceptLab/ocl_online#532 — the cap is one fixed setting, with no per-account logic in this repo.
Summary
Concept, mapping and reference lists now serve at most
LIST_MAX_LIMITrows per page (new env setting, default 100), so one page of a heavy list stays a short request. Alimitover the max is capped to it instead of falling back to 25; the paging headers describe the page actually served.What changes
is_page_size_capped— concept and mapping lists (sources, source versions, collections, collection versions, expansions, the global lists, search), version histories, a concept's mappings / children / parents, and references.limitis capped once, inBaseAPIView.initialize, so search (ES) and database lists agree.limit, so withlimit=2000page 2 came back empty. They now use the capped limit, and followingnextreaches every row.includeConcepts/includeMappings/includeReferencescapslimittoo.Compress: trueon a capped list is refused aboveLIST_MAX_LIMITrows (was 1,000);users.list_unpaginatedstays exempt.csv=true;brief=true&checksums=true; FHIR$translate/_count. A missinglimitstill returns 25.Env
LIST_MAX_LIMIT(optional, default 100).Verification
docker exec oclapi2-api-1 python manage.py test core.integration_tests.tests_concepts core.integration_tests.tests_mappings core.integration_tests.tests_collections core.integration_tests.tests_orgs core.common … --keepdb: new tests pass.ConceptPropertyFilterViewTestfails on master as well.docker exec oclapi2-api-1 pylint -j2 core/common …: 10.00/10.Tests, added to existing files: capped pages and following
next(including on a released version), search, global list, version history,limit1–100 unchanged, the checksums exemption,Compress, references, repo detail, an uncapped orgs list, andcap_page_size.Before go-live
Per the ticket: the Mapper's Locales fetch moves off
limit=500first, and integrators get notice (OpenConceptLab/ocl_online#473).🤖 Generated with Claude Code