From 94e5ef03885f89460913e5de467b9e011933117a Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Tue, 13 Oct 2026 14:53:35 -0400 Subject: [PATCH 1/2] OpenConceptLab/ocl_online#454 | Page HEAD concept and mapping lists by ids first A source's HEAD concept or mapping list sliced its page out of the joined query (select_related from/to concepts, or created_by/updated_by), so a deep page ran those joins for every row its OFFSET skipped. CustomPaginator(page_by_ids=True) now filters the list by the sliced ids instead, and joins only the page's rows. ConceptListView and MappingListView turn it on for a source's HEAD list; repo-version lists already select their ids first and search results are paged by Elasticsearch. Pages return the same rows in the same order. Co-Authored-By: Claude Opus 5.5 --- core/common/mixins.py | 15 ++++- core/concepts/views.py | 4 ++ core/integration_tests/tests_list_pages.py | 73 ++++++++++++++++++++++ core/mappings/views.py | 4 ++ 4 files changed, 93 insertions(+), 3 deletions(-) create mode 100644 core/integration_tests/tests_list_pages.py diff --git a/core/common/mixins.py b/core/common/mixins.py index e628b9d8..927f3c16 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -43,7 +43,7 @@ class CustomPaginator: def __init__( # pylint: disable=too-many-arguments self, request, total_count, queryset, page_size, is_sliced=False, max_score=None, search_scores=None, - highlights=None + highlights=None, page_by_ids=False ): self.request = request self.queryset = queryset @@ -55,7 +55,12 @@ def __init__( # pylint: disable=too-many-arguments bottom = (self.page_number - 1) * self.page_size top = bottom + self.page_size top = min(top, self.total) - self.queryset = self.queryset[bottom:top] + if page_by_ids and isinstance(self.queryset, QuerySet): + # select the page's ids first, so joins (select_related) run for the page's rows only, + # not for every row a deep page's OFFSET skips + self.queryset = self.queryset.filter(id__in=self.queryset.values('id')[bottom:top]) + else: + self.queryset = self.queryset[bottom:top] if isinstance(self.queryset, QuerySet): self.queryset.count = None self.paginator = Paginator(self.queryset, self.page_size) @@ -247,7 +252,8 @@ def list(self, request, *args, **kwargs): # pylint:disable=too-many-locals,too- paginator = CustomPaginator( request=request, queryset=sorted_list, page_size=self.limit, total_count=self.total_count, is_sliced=self.is_sliced(), max_score=get(self, '_max_score'), - search_scores=get(self, '_scores'), highlights=get(self, '_highlights') + search_scores=get(self, '_scores'), highlights=get(self, '_highlights'), + page_by_ids=self.should_page_by_ids() ) headers = paginator.headers results = paginator.current_page_results @@ -265,6 +271,9 @@ def list(self, request, *args, **kwargs): # pylint:disable=too-many-locals,too- response['num_found'] = len(sorted_list) return response + def should_page_by_ids(self): # see CustomPaginator + return False + def serialize_list(self, results, paginator=None): result_dict = self.get_serializer(results, many=True).data if self.should_include_facets(): diff --git a/core/concepts/views.py b/core/concepts/views.py index 01c8af7e..98bcc0eb 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -174,6 +174,10 @@ def get_serializer_class(self): return ConceptListSerializer + def should_page_by_ids(self): + parent = get(self, 'parent_resource') + return 'source' in self.kwargs and bool(parent) and parent.is_head + def is_sliced(self): result = super().is_sliced() if result: diff --git a/core/integration_tests/tests_list_pages.py b/core/integration_tests/tests_list_pages.py new file mode 100644 index 00000000..c2c6967d --- /dev/null +++ b/core/integration_tests/tests_list_pages.py @@ -0,0 +1,73 @@ +from datetime import timedelta + +from django.db import connection +from django.db.models import F +from django.test.utils import CaptureQueriesContext +from django.utils import timezone + +from core.common.tests import OCLAPITestCase +from core.concepts.models import Concept +from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory +from core.mappings.models import Mapping +from core.mappings.tests.factories import MappingFactory +from core.sources.tests.factories import OrganizationSourceFactory + + +class SourceHeadListPagesTest(OCLAPITestCase): + """ + A source's HEAD concept and mapping lists select a page's ids first (CustomPaginator page_by_ids), so the page + query's joins cover the page's rows only, not every row a deep page's OFFSET skips. + """ + def setUp(self): + super().setUp() + self.source = OrganizationSourceFactory() + + def concept(): + return ConceptFactory(parent=self.source, names=[ConceptNameFactory.build(locale_preferred=True)]) + + for _ in range(5): + MappingFactory(parent=self.source, from_concept=concept(), to_concept=concept()) + now = timezone.now() + for model in (Concept, Mapping): + for minutes, row in enumerate(model.objects.filter(parent=self.source, id=F('versioned_object_id'))): + model.objects.filter(id=row.id).update(updated_at=now - timedelta(minutes=minutes)) + row.set_checksums() + + def assert_pages(self, url, table, total): + everything = self.client.get(f'{url}?limit={total}', format='json') + self.assertEqual(len(everything.data), total) + + rows = [] + for page in range(1, total // 4 + 2): + with CaptureQueriesContext(connection) as context: + response = self.client.get(f'{url}?limit=4&page={page}', format='json') + self.assertEqual(response.status_code, 200) + self.assertEqual(response['num_found'], str(total)) + rows += response.data + + page_queries = [ + query['sql'] for query in context.captured_queries + if query['sql'].startswith('SELECT') and f'FROM "{table}"' in query['sql'] and ' JOIN ' in query['sql'] + ] + self.assertEqual(len(page_queries), 1) + self.assertIn(f'"{table}"."id" IN (SELECT', page_queries[0]) + self.assertNotIn('LIMIT', page_queries[0].rsplit(')', 1)[-1]) # paged inside the ids subquery + + self.assertEqual(rows, everything.data) + + def test_mappings(self): + self.assert_pages(self.source.mappings_url, 'mappings', 5) + + def test_concepts(self): + self.assert_pages(self.source.concepts_url, 'concepts', 10) + + def test_version_list_unchanged(self): + """Repo-version lists page their ids already (is_sliced) and don't go through page_by_ids.""" + version = OrganizationSourceFactory( + mnemonic=self.source.mnemonic, organization=self.source.organization, version='v1') + version.mappings.add(*Mapping.objects.filter(parent=self.source, is_latest_version=True).exclude( + id=F('versioned_object_id'))) + response = self.client.get(f'{version.mappings_url}?limit=2&page=2', format='json') + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data), 2) + self.assertEqual(response['num_found'], '5') diff --git a/core/mappings/views.py b/core/mappings/views.py index 264db9de..9290771f 100644 --- a/core/mappings/views.py +++ b/core/mappings/views.py @@ -68,6 +68,10 @@ def get_serializer_class(self): def apply_filters(self, queryset): return queryset + def should_page_by_ids(self): + parent = get(self, 'parent_resource') + return 'source' in self.kwargs and bool(parent) and parent.is_head + def is_sliced(self): result = super().is_sliced() if result: From 71095aa89fd644c27f597a50b918da967e80594a Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Tue, 13 Oct 2026 15:01:24 -0400 Subject: [PATCH 2/2] OpenConceptLab/ocl_online#454 | Simplify: shorter should_page_by_ids, trim paging tests From the simplification review of #937: should_page_by_ids reads parent_resource.is_head through pydash get; the paging tests drop a redundant set_checksums and a version-list test this change can't affect. Co-Authored-By: Claude Opus 5.5 --- core/concepts/views.py | 3 +-- core/integration_tests/tests_list_pages.py | 12 ------------ core/mappings/views.py | 3 +-- 3 files changed, 2 insertions(+), 16 deletions(-) diff --git a/core/concepts/views.py b/core/concepts/views.py index 98bcc0eb..711c22dd 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -175,8 +175,7 @@ def get_serializer_class(self): return ConceptListSerializer def should_page_by_ids(self): - parent = get(self, 'parent_resource') - return 'source' in self.kwargs and bool(parent) and parent.is_head + return 'source' in self.kwargs and bool(get(self, 'parent_resource.is_head')) def is_sliced(self): result = super().is_sliced() diff --git a/core/integration_tests/tests_list_pages.py b/core/integration_tests/tests_list_pages.py index c2c6967d..f21a53ed 100644 --- a/core/integration_tests/tests_list_pages.py +++ b/core/integration_tests/tests_list_pages.py @@ -31,7 +31,6 @@ def concept(): for model in (Concept, Mapping): for minutes, row in enumerate(model.objects.filter(parent=self.source, id=F('versioned_object_id'))): model.objects.filter(id=row.id).update(updated_at=now - timedelta(minutes=minutes)) - row.set_checksums() def assert_pages(self, url, table, total): everything = self.client.get(f'{url}?limit={total}', format='json') @@ -60,14 +59,3 @@ def test_mappings(self): def test_concepts(self): self.assert_pages(self.source.concepts_url, 'concepts', 10) - - def test_version_list_unchanged(self): - """Repo-version lists page their ids already (is_sliced) and don't go through page_by_ids.""" - version = OrganizationSourceFactory( - mnemonic=self.source.mnemonic, organization=self.source.organization, version='v1') - version.mappings.add(*Mapping.objects.filter(parent=self.source, is_latest_version=True).exclude( - id=F('versioned_object_id'))) - response = self.client.get(f'{version.mappings_url}?limit=2&page=2', format='json') - self.assertEqual(response.status_code, 200) - self.assertEqual(len(response.data), 2) - self.assertEqual(response['num_found'], '5') diff --git a/core/mappings/views.py b/core/mappings/views.py index 9290771f..e02c8aef 100644 --- a/core/mappings/views.py +++ b/core/mappings/views.py @@ -69,8 +69,7 @@ def apply_filters(self, queryset): return queryset def should_page_by_ids(self): - parent = get(self, 'parent_resource') - return 'source' in self.kwargs and bool(parent) and parent.is_head + return 'source' in self.kwargs and bool(get(self, 'parent_resource.is_head')) def is_sliced(self): result = super().is_sliced()