From 4cb6f4e137c384a16ca48534f058b9cdfcdd3f68 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Tue, 13 Oct 2026 12:56:23 -0400 Subject: [PATCH 1/5] OpenConceptLab/ocl_online#454 | Load list-row lookups once per page for concepts and mappings The concept and mapping list serializers looked up several things for every row: the latest version's uri (version_url), latest_source_version, the from/to concepts' sources and the sources' owners, and created_by/updated_by. A 1,000-row page of mappings ran about 8,500 queries. A list serializer (SourceChildPageSerializer) now loads these for the whole page after pagination: prefetches for the relations, and one query each for the latest version uris and the latest source versions. Serializing a single object is unchanged. The response is the same; the new tests compare it with row-by-row serialization and check that the query count doesn't grow with the page size, for HEAD and repo-version lists, default and verbose. Co-Authored-By: Claude Opus 5.5 --- core/common/mixins.py | 37 ++++++++++- core/common/serializers.py | 11 +++- core/concepts/models.py | 4 ++ core/concepts/serializers.py | 5 +- core/integration_tests/tests_concepts.py | 79 ++++++++++++++++++++++ core/integration_tests/tests_mappings.py | 84 ++++++++++++++++++++++++ core/mappings/models.py | 7 ++ core/mappings/serializers.py | 5 +- 8 files changed, 227 insertions(+), 5 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index e628b9d88..917bcff89 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -8,7 +8,7 @@ from django.core.exceptions import ValidationError from django.core.paginator import Paginator from django.db import transaction, models -from django.db.models import Q, F, QuerySet +from django.db.models import Q, F, QuerySet, prefetch_related_objects from django.http import HttpResponseForbidden, Http404 from django.shortcuts import redirect from django.urls import resolve, Resolver404 @@ -635,9 +635,42 @@ class SourceChildMixin(ChecksumModel): _counted = models.BooleanField(default=True, null=True, blank=True) _index = models.BooleanField(default=True) + # relations the list serializers read on every row, prefetched once per page by load_list_fields + LIST_PREFETCHES = () + class Meta: abstract = True + @classmethod + def load_list_fields(cls, instances): + """ + Loads what the list serializers would otherwise query row by row, for a whole page at once: + LIST_PREFETCHES, the latest version's uri (version_url) and latest_source_version. + """ + if not instances: + return + prefetch_related_objects(instances, *cls.LIST_PREFETCHES) + + heads = [instance.id for instance in instances if instance.is_versioned_object] + latest_version_uris = dict(cls.objects.filter( + versioned_object_id__in=heads, is_active=True, is_latest_version=True + ).exclude(id=F('versioned_object_id')).order_by('versioned_object_id', '-created_at').distinct( + 'versioned_object_id').values_list('versioned_object_id', 'uri')) if heads else {} + + key = f'{cls._meta.model_name}_id' + latest_source_version_ids = dict(cls.sources.through.objects.filter( + **{f'{key}__in': [instance.id for instance in instances]} + ).exclude(source__version=HEAD).order_by(key, '-source__created_at').distinct(key).values_list( + key, 'source_id')) + source_versions = cls._meta.get_field('sources').related_model.objects.in_bulk( + set(latest_source_version_ids.values())) + + for instance in instances: + # pylint: disable=protected-access + instance._latest_source_version = source_versions.get(latest_source_version_ids.get(instance.id)) + if instance.id in latest_version_uris: + instance._latest_version_uri = latest_version_uris[instance.id] + def calculate_uri(self): uri = self.parent.uri + self.resource_type.lower() + 's/' + str(self.mnemonic) + '/' if not self.is_head: @@ -746,7 +779,7 @@ def is_versioned_object(self): @property def version_url(self): if self.is_versioned_object: - return self.get_latest_version().uri + return getattr(self, '_latest_version_uri', None) or self.get_latest_version().uri return self.uri @property diff --git a/core/common/serializers.py b/core/common/serializers.py index bc00f63e8..7a6d3c836 100644 --- a/core/common/serializers.py +++ b/core/common/serializers.py @@ -1,6 +1,7 @@ +from django.db.models.manager import BaseManager from pydash import get from rest_framework.fields import CharField, JSONField, SerializerMethodField, FloatField, ChoiceField -from rest_framework.serializers import Serializer, Field, ValidationError, ModelSerializer +from rest_framework.serializers import Serializer, Field, ValidationError, ModelSerializer, ListSerializer from core import settings from core.code_systems.constants import RESOURCE_TYPE as CODE_SYSTEM_RESOURCE_TYPE @@ -225,6 +226,14 @@ def get_search_highlight(obj): return cleaned_highlight +class SourceChildPageSerializer(ListSerializer): # pylint: disable=abstract-method + """Serializes a page of concepts or mappings, loading the per-row lookups for the whole page first.""" + def to_representation(self, data): + instances = list(data.all() if isinstance(data, BaseManager) else data) + self.child.Meta.model.load_list_fields(instances) + return super().to_representation(instances) + + class AbstractResourceSerializer(ModelSerializer): search_meta = SerializerMethodField() diff --git a/core/concepts/models.py b/core/concepts/models.py index 5628da87e..0ea8680bd 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -220,6 +220,8 @@ class HierarchicalConcepts(models.Model): class Concept(ConceptValidationMixin, SourceChildMixin, VersionedModel): # pylint: disable=too-many-public-methods + LIST_PREFETCHES = ('parent', 'created_by', 'updated_by') + class Meta: db_table = 'concepts' unique_together = ('mnemonic', 'version', 'parent') @@ -981,6 +983,8 @@ def is_existing_in_parent(self): @property def latest_source_version(self): + if hasattr(self, '_latest_source_version'): # loaded for the whole page by load_list_fields + return self._latest_source_version return self.sources.exclude(version=HEAD).order_by('-created_at').first() def get_source_version_before_creation(self): diff --git a/core/concepts/serializers.py b/core/concepts/serializers.py index 5e8229b91..aecffe228 100644 --- a/core/concepts/serializers.py +++ b/core/concepts/serializers.py @@ -9,7 +9,7 @@ INCLUDE_CHILD_CONCEPT_URLS, HEAD, INCLUDE_SUMMARY, INCLUDE_VERBOSE_REFERENCES, VERBOSE_PARAM, \ INCLUDE_RETIRED_PARAM, ACCESS_TYPE_NONE from core.common.fields import EncodedDecodedCharField -from core.common.serializers import AbstractResourceSerializer +from core.common.serializers import AbstractResourceSerializer, SourceChildPageSerializer from core.common.utils import to_parent_uri_from_kwargs, get_truthy_values from core.concepts.models import Concept, ConceptName @@ -323,6 +323,7 @@ class ConceptListSerializer(ConceptAbstractSerializer): class Meta: model = Concept + list_serializer_class = SourceChildPageSerializer fields = ConceptAbstractSerializer.Meta.fields + ( 'uuid', 'id', 'external_id', 'concept_class', 'datatype', 'url', 'retired', 'source', 'owner', 'owner_type', 'owner_url', 'display_name', 'display_locale', 'version', 'update_comment', @@ -339,6 +340,7 @@ class ConceptVersionListSerializer(ConceptListSerializer): class Meta: model = Concept + list_serializer_class = SourceChildPageSerializer fields = ConceptListSerializer.Meta.fields + ( 'previous_version_url', 'source_versions', 'collection_versions' ) @@ -485,6 +487,7 @@ class ConceptDetailSerializer(ConceptAbstractSerializer): class Meta: model = Concept + list_serializer_class = SourceChildPageSerializer fields = ConceptAbstractSerializer.Meta.fields + ( 'id', 'external_id', 'concept_class', 'datatype', 'url', 'retired', 'source', 'owner', 'owner_type', 'owner_url', 'display_name', 'display_locale', 'names', 'descriptions', diff --git a/core/integration_tests/tests_concepts.py b/core/integration_tests/tests_concepts.py index 2ef60d9db..074b81a63 100644 --- a/core/integration_tests/tests_concepts.py +++ b/core/integration_tests/tests_concepts.py @@ -1,11 +1,15 @@ +from datetime import timedelta from unittest.mock import patch import factory from celery.states import PENDING from django.core.cache import cache +from django.db import connection from django.db.models import F from django.test import override_settings +from django.test.utils import CaptureQueriesContext +from django.utils import timezone from mock import ANY from core.bundles.models import Bundle @@ -16,6 +20,7 @@ from core.concepts.constants import CONCEPT_HARD_DELETE_REQUIRES_HEAD_ONLY from core.concepts.documents import ConceptDocument from core.concepts.models import Concept +from core.concepts.serializers import ConceptListSerializer, ConceptDetailSerializer from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory, ConceptDescriptionFactory from core.mappings.models import Mapping from core.mappings.tests.factories import MappingFactory @@ -4175,3 +4180,77 @@ def test_post_success(self, bundle_clone_mock): self.concept, self.concept.parent, self.clone_to_source, self.user, ANY, False, resource_budget=100, **parameters # no group: the preview per-call budget (ocl_online#230) ) + + +class ConceptListQueryCountTest(OCLAPITestCase): + """A page of concepts costs the same number of queries whatever its size, and serializes as row by row did.""" + def setUp(self): + super().setUp() + self.source = OrganizationSourceFactory() + for _ in range(4): + ConceptFactory( + parent=self.source, + names=[ + ConceptNameFactory.build(locale='en', locale_preferred=True), ConceptNameFactory.build(locale='fr') + ], + descriptions=[ConceptDescriptionFactory.build(locale='en')]) + concepts = Concept.objects.filter(parent=self.source) + for concept in concepts: # a GET calculates and saves missing checksums, one row at a time + concept.set_checksums() + for locale in [*concept.names.all(), *concept.descriptions.all()]: + locale.set_checksums() + self.set_released_versions(concepts) + + def get_page(self, url, limit, params=''): + with CaptureQueriesContext(connection) as context: + response = self.client.get(f'{url}?limit={limit}{params}', format='json') + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data), limit) + return response, len(context.captured_queries) + + def assert_page(self, url, serializer_class, params=''): + """Same query count for a page of 2 and of 4; same output as serializing each row on its own.""" + _, small_page_queries = self.get_page(url, 2, params) + response, page_queries = self.get_page(url, 4, params) + self.assertEqual(page_queries, small_page_queries) + + context = {'request': response.renderer_context['request'], 'view': response.renderer_context['view']} + rows = [ + Concept.objects.get(versioned_object_id=row['versioned_object_id'], version=row['version']) + for row in response.data + ] + self.assertEqual(response.data, [serializer_class(row, context=context).data for row in rows]) + return response.data, rows + + def set_released_versions(self, rows): + """ + Newest head first by updated_at, so a page of 2 and a page of 4 hold the same kinds of rows. + v1 holds every latest version row, v2 every other one; both hold the newest head as well. + """ + self.heads = list(rows.filter(id=F('versioned_object_id')).order_by('id')) + now = timezone.now() + for age, head in enumerate(reversed(self.heads)): + Concept.objects.filter(id=head.id).update(updated_at=now - timedelta(minutes=age)) + versions = list(rows.exclude(id=F('versioned_object_id')).filter(is_latest_version=True).order_by('id')) + self.v1 = OrganizationSourceFactory( + mnemonic=self.source.mnemonic, organization=self.source.organization, version='v1') + self.v2 = OrganizationSourceFactory( + mnemonic=self.source.mnemonic, organization=self.source.organization, version='v2') + getattr(self.v1, 'concepts').add(*versions, self.heads[-1]) + getattr(self.v2, 'concepts').add(*versions[::2], self.heads[-1]) + self.in_v2 = {row.id for row in versions[::2]} | {self.heads[-1].id} + + def test_head_list(self): + data, rows = self.assert_page(self.source.concepts_url, ConceptListSerializer) + self.assert_page(self.source.concepts_url, ConceptDetailSerializer, '&verbose=true') + + for row, instance in zip(data, rows): + self.assertEqual(row['version_url'], instance.get_latest_version().uri) + self.assertEqual(row['latest_source_version'], 'v2' if instance.id == self.heads[-1].id else None) + + def test_version_list(self): + data, rows = self.assert_page(self.v1.concepts_url, ConceptListSerializer) + self.assert_page(self.v1.concepts_url, ConceptDetailSerializer, '&verbose=true') + + for row, instance in zip(data, rows): + self.assertEqual(row['latest_source_version'], 'v2' if instance.id in self.in_v2 else 'v1') diff --git a/core/integration_tests/tests_mappings.py b/core/integration_tests/tests_mappings.py index 0c9b5e38a..0385fffb2 100644 --- a/core/integration_tests/tests_mappings.py +++ b/core/integration_tests/tests_mappings.py @@ -1,5 +1,11 @@ +from datetime import timedelta + from django.core.cache import cache +from django.db import connection +from django.db.models import F from django.test import override_settings +from django.test.utils import CaptureQueriesContext +from django.utils import timezone from rest_framework.exceptions import ErrorDetail from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory @@ -8,6 +14,7 @@ from core.mappings.constants import SAME_AS from core.mappings.documents import MappingDocument from core.mappings.models import Mapping +from core.mappings.serializers import MappingListSerializer, MappingDetailSerializer from core.mappings.tests.factories import MappingFactory from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import UserSourceFactory, OrganizationSourceFactory @@ -1076,3 +1083,80 @@ def test_repo_version_mappings_with_search_are_not_cached(self): self.assertIsNone(cache.get(body_key)) self.assertIsNone(cache.get(headers_key)) self.assertIsNone(cache.get(body_key + '?q=123')) + + +class MappingListQueryCountTest(OCLAPITestCase): + """A page of mappings costs the same number of queries whatever its size, and serializes as row by row did.""" + def setUp(self): + super().setUp() + self.source = OrganizationSourceFactory() + target = OrganizationSourceFactory() + for index in range(4): # external, internal, external, internal + from_concept = ConceptFactory(parent=self.source, names=[ConceptNameFactory.build(locale_preferred=True)]) + if index % 2: + to_concept = ConceptFactory(parent=target, names=[ConceptNameFactory.build(locale_preferred=True)]) + MappingFactory( + parent=self.source, from_concept=from_concept, from_source=self.source, to_concept=to_concept, + to_source=target) + else: + MappingFactory( + parent=self.source, from_concept=from_concept, from_source=self.source, to_concept=None, + to_source=target, to_concept_code=f'X{index}', to_source_url=target.uri) + mappings = Mapping.objects.filter(parent=self.source) + for mapping in mappings: + mapping.set_checksums() # a GET calculates and saves missing checksums, one row at a time + self.set_released_versions(mappings) + + def get_page(self, url, limit, params=''): + with CaptureQueriesContext(connection) as context: + response = self.client.get(f'{url}?limit={limit}{params}', format='json') + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data), limit) + return response, len(context.captured_queries) + + def assert_page(self, url, serializer_class, params=''): + """Same query count for a page of 2 and of 4; same output as serializing each row on its own.""" + _, small_page_queries = self.get_page(url, 2, params) + response, page_queries = self.get_page(url, 4, params) + self.assertEqual(page_queries, small_page_queries) + + context = {'request': response.renderer_context['request'], 'view': response.renderer_context['view']} + rows = [ + Mapping.objects.get(versioned_object_id=row['versioned_object_id'], version=row['version']) + for row in response.data + ] + self.assertEqual(response.data, [serializer_class(row, context=context).data for row in rows]) + return response.data, rows + + def set_released_versions(self, rows): + """ + Newest head first by updated_at, so a page of 2 and a page of 4 hold the same kinds of rows. + v1 holds every latest version row, v2 every other one; both hold the newest head as well. + """ + self.heads = list(rows.filter(id=F('versioned_object_id')).order_by('id')) + now = timezone.now() + for age, head in enumerate(reversed(self.heads)): + Mapping.objects.filter(id=head.id).update(updated_at=now - timedelta(minutes=age)) + versions = list(rows.exclude(id=F('versioned_object_id')).filter(is_latest_version=True).order_by('id')) + self.v1 = OrganizationSourceFactory( + mnemonic=self.source.mnemonic, organization=self.source.organization, version='v1') + self.v2 = OrganizationSourceFactory( + mnemonic=self.source.mnemonic, organization=self.source.organization, version='v2') + getattr(self.v1, 'mappings').add(*versions, self.heads[-1]) + getattr(self.v2, 'mappings').add(*versions[::2], self.heads[-1]) + self.in_v2 = {row.id for row in versions[::2]} | {self.heads[-1].id} + + def test_head_list(self): + data, rows = self.assert_page(self.source.mappings_url, MappingListSerializer) + self.assert_page(self.source.mappings_url, MappingDetailSerializer, '&verbose=true') + + for row, instance in zip(data, rows): + self.assertEqual(row['version_url'], instance.get_latest_version().uri) + self.assertEqual(row['latest_source_version'], 'v2' if instance.id == self.heads[-1].id else None) + + def test_version_list(self): + data, rows = self.assert_page(self.v1.mappings_url, MappingListSerializer) + self.assert_page(self.v1.mappings_url, MappingDetailSerializer, '&verbose=true') + + for row, instance in zip(data, rows): + self.assertEqual(row['latest_source_version'], 'v2' if instance.id in self.in_v2 else 'v1') diff --git a/core/mappings/models.py b/core/mappings/models.py index 90d576546..2bc4ac315 100644 --- a/core/mappings/models.py +++ b/core/mappings/models.py @@ -20,6 +20,11 @@ class Mapping(MappingValidationMixin, SourceChildMixin, VersionedModel): + LIST_PREFETCHES = ( + 'from_concept__parent', 'to_concept__parent', 'from_source__organization', 'from_source__user', + 'to_source__organization', 'to_source__user', 'created_by', 'updated_by' + ) + class Meta: db_table = 'mappings' unique_together = ('mnemonic', 'version', 'parent') @@ -416,6 +421,8 @@ def is_existing_in_parent(self): @property def latest_source_version(self): + if hasattr(self, '_latest_source_version'): # loaded for the whole page by load_list_fields + return self._latest_source_version return self.sources.exclude(version=HEAD).order_by('-created_at').first() @classmethod diff --git a/core/mappings/serializers.py b/core/mappings/serializers.py index 74a674056..e53d5bb2d 100644 --- a/core/mappings/serializers.py +++ b/core/mappings/serializers.py @@ -7,7 +7,7 @@ MAPPING_LOOKUP_TO_CONCEPT, MAPPING_LOOKUP_FROM_SOURCE, MAPPING_LOOKUP_TO_SOURCE, INCLUDE_EXTRAS_PARAM, \ INCLUDE_SOURCE_VERSIONS, INCLUDE_COLLECTION_VERSIONS, INCLUDE_VERBOSE_REFERENCES from core.common.fields import EncodedDecodedCharField -from core.common.serializers import AbstractResourceSerializer +from core.common.serializers import AbstractResourceSerializer, SourceChildPageSerializer from core.common.utils import get_truthy_values from core.concepts.serializers import ConceptDetailSerializer from core.mappings.models import Mapping @@ -124,6 +124,7 @@ class MappingListSerializer(AbstractMappingSerializer): class Meta: model = Mapping + list_serializer_class = SourceChildPageSerializer fields = AbstractMappingSerializer.Meta.fields + ( 'external_id', 'retired', 'map_type', 'source', 'owner', 'owner_type', 'from_concept_code', 'from_concept_name', 'from_concept_url', @@ -145,6 +146,7 @@ class MappingVersionListSerializer(MappingListSerializer): class Meta: model = Mapping + list_serializer_class = SourceChildPageSerializer fields = MappingListSerializer.Meta.fields + ( 'previous_version_url', 'source_versions', 'collection_versions' ) @@ -252,6 +254,7 @@ class MappingDetailSerializer(MappingListSerializer): class Meta: model = Mapping + list_serializer_class = SourceChildPageSerializer fields = MappingListSerializer.Meta.fields + ( 'type', 'uuid', 'extras', 'created_on', 'updated_on', 'created_by', 'updated_by', 'parent_id', 'public_can_view', 'latest_source_version' From 61274f650b1b777ef873785c9675f62129e0ba12 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Tue, 13 Oct 2026 13:08:11 -0400 Subject: [PATCH 2/5] OpenConceptLab/ocl_online#454 | Skip the before-creation source lookup when a page found no released version Codex review F1.1: a concept revision in no released version (e.g. a search result saved since the last release) still made one query per row, through summary_properties -> get_parent_source_version() -> get_source_version_before_creation(). That lookup only searches the row's released versions, so when load_list_fields found none the answer is None. Co-Authored-By: Claude Opus 5.5 --- core/concepts/models.py | 2 ++ core/integration_tests/tests_concepts.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/core/concepts/models.py b/core/concepts/models.py index 0ea8680bd..1867a048c 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -988,6 +988,8 @@ def latest_source_version(self): return self.sources.exclude(version=HEAD).order_by('-created_at').first() def get_source_version_before_creation(self): + if hasattr(self, '_latest_source_version') and not self._latest_source_version: + return None # load_list_fields found no released version holding this row return self.sources.exclude(version=HEAD).filter( created_at__lte=self.created_at).order_by('-created_at').first() diff --git a/core/integration_tests/tests_concepts.py b/core/integration_tests/tests_concepts.py index 074b81a63..7c7584dc9 100644 --- a/core/integration_tests/tests_concepts.py +++ b/core/integration_tests/tests_concepts.py @@ -11,6 +11,8 @@ from django.test.utils import CaptureQueriesContext from django.utils import timezone from mock import ANY +from rest_framework.request import Request +from rest_framework.test import APIRequestFactory from core.bundles.models import Bundle from core.sources.models import CloneLimitExceeded @@ -4254,3 +4256,24 @@ def test_version_list(self): for row, instance in zip(data, rows): self.assertEqual(row['latest_source_version'], 'v2' if instance.id in self.in_v2 else 'v1') + + def test_revisions_in_no_released_version(self): + """Search results can hold revisions saved since the last release: no per-row lookup for those either.""" + self.v1.concepts.clear() + self.v2.concepts.clear() + revisions = list(Concept.objects.filter(parent=self.source, is_latest_version=True).exclude( + id=F('versioned_object_id')).order_by('id')) + context = {'request': Request(APIRequestFactory().get('/'))} + + def serialize(rows): # fetched as search results are: by id, names prefetched + rows = list( + Concept.objects.filter(id__in=[row.id for row in rows]).prefetch_related('names').order_by('id')) + with CaptureQueriesContext(connection) as queries: + data = ConceptListSerializer(rows, many=True, context=context).data + return data, len(queries.captured_queries) + + _, small_page_queries = serialize(revisions[:2]) + data, page_queries = serialize(revisions) + self.assertEqual(page_queries, small_page_queries) + self.assertEqual(data, [ConceptListSerializer(row, context=context).data for row in revisions]) + self.assertEqual({row['latest_source_version'] for row in data}, {None}) From 1744f47694ed9c16c4060048c1ebec71a3053392 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Tue, 13 Oct 2026 13:18:53 -0400 Subject: [PATCH 3/5] OpenConceptLab/ocl_online#454 | Simplify: one latest_source_version, shared list query-count tests From the simplification review of #936: - Concept and Mapping overrode SourceChildMixin.latest_source_version with the same body, leaving the mixin's unused. The shared body now lives on the mixin, next to load_list_fields, and the overrides are gone. - load_list_fields drops guards Django already short-circuits (empty page, no heads) and always sets _latest_version_uri. - The concept and mapping query-count tests share SourceChildListQueryCountMixin (core/integration_tests/test_utils.py). Co-Authored-By: Claude Opus 5.5 --- core/common/mixins.py | 13 ++-- core/concepts/models.py | 6 -- core/integration_tests/test_utils.py | 75 ++++++++++++++++++++++++ core/integration_tests/tests_concepts.py | 64 +++----------------- core/integration_tests/tests_mappings.py | 68 +++------------------ core/mappings/models.py | 6 -- 6 files changed, 94 insertions(+), 138 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index 917bcff89..8a790b73b 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -647,15 +647,13 @@ def load_list_fields(cls, instances): Loads what the list serializers would otherwise query row by row, for a whole page at once: LIST_PREFETCHES, the latest version's uri (version_url) and latest_source_version. """ - if not instances: - return prefetch_related_objects(instances, *cls.LIST_PREFETCHES) heads = [instance.id for instance in instances if instance.is_versioned_object] latest_version_uris = dict(cls.objects.filter( versioned_object_id__in=heads, is_active=True, is_latest_version=True ).exclude(id=F('versioned_object_id')).order_by('versioned_object_id', '-created_at').distinct( - 'versioned_object_id').values_list('versioned_object_id', 'uri')) if heads else {} + 'versioned_object_id').values_list('versioned_object_id', 'uri')) key = f'{cls._meta.model_name}_id' latest_source_version_ids = dict(cls.sources.through.objects.filter( @@ -668,8 +666,7 @@ def load_list_fields(cls, instances): for instance in instances: # pylint: disable=protected-access instance._latest_source_version = source_versions.get(latest_source_version_ids.get(instance.id)) - if instance.id in latest_version_uris: - instance._latest_version_uri = latest_version_uris[instance.id] + instance._latest_version_uri = latest_version_uris.get(instance.id) def calculate_uri(self): uri = self.parent.uri + self.resource_type.lower() + 's/' + str(self.mnemonic) + '/' @@ -684,9 +681,9 @@ def is_in_latest_source_version(self): @property def latest_source_version(self): - if self.is_in_latest_source_version: - return self._cached_latest_source_version - return None + if hasattr(self, '_latest_source_version'): # loaded for the whole page by load_list_fields + return self._latest_source_version + return self.sources.exclude(version=HEAD).order_by('-created_at').first() @cached_property def _cached_latest_source_version(self): diff --git a/core/concepts/models.py b/core/concepts/models.py index 1867a048c..4199fcfe4 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -981,12 +981,6 @@ def __clone_locales(locales): def is_existing_in_parent(self): return self.parent.concepts_set.filter(mnemonic__exact=self.mnemonic).exists() - @property - def latest_source_version(self): - if hasattr(self, '_latest_source_version'): # loaded for the whole page by load_list_fields - return self._latest_source_version - return self.sources.exclude(version=HEAD).order_by('-created_at').first() - def get_source_version_before_creation(self): if hasattr(self, '_latest_source_version') and not self._latest_source_version: return None # load_list_fields found no released version holding this row diff --git a/core/integration_tests/test_utils.py b/core/integration_tests/test_utils.py index a4ee2161b..31cd0f95f 100644 --- a/core/integration_tests/test_utils.py +++ b/core/integration_tests/test_utils.py @@ -1,8 +1,15 @@ import json import os +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 jsonpath_ng import parse +from core.sources.tests.factories import OrganizationSourceFactory + def load_json(file, parent_dir=None): if parent_dir: @@ -33,3 +40,71 @@ def ignore_json_paths(self, json_input, json_response, paths): for path in paths: self.update(json_input, path, None) self.update(json_response, path, None) + + +class SourceChildListQueryCountMixin: + """ + A page of concepts or mappings costs the same number of queries whatever its size, and serializes as row by row + did. Set model, relation ('concepts' or 'mappings'), list_serializer and detail_serializer; setUp creates 4 rows + in self.source and calls set_released_versions. + """ + model = None + relation = None + list_serializer = None + detail_serializer = None + + def get_page(self, url, limit, params=''): + with CaptureQueriesContext(connection) as context: + response = self.client.get(f'{url}?limit={limit}{params}', format='json') + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data), limit) + return response, len(context.captured_queries) + + def assert_page(self, url, serializer_class, params=''): + """Same query count for a page of 2 and of 4; same output as serializing each row on its own.""" + _, small_page_queries = self.get_page(url, 2, params) + response, page_queries = self.get_page(url, 4, params) + self.assertEqual(page_queries, small_page_queries) + + context = {'request': response.renderer_context['request'], 'view': response.renderer_context['view']} + rows = [ + self.model.objects.get(versioned_object_id=row['versioned_object_id'], version=row['version']) + for row in response.data + ] + self.assertEqual(response.data, [serializer_class(row, context=context).data for row in rows]) + return response.data, rows + + def set_released_versions(self, rows): + """ + Newest head first by updated_at, so a page of 2 and a page of 4 hold the same kinds of rows. + v1 holds every latest version row, v2 every other one; both hold the newest head as well. + """ + self.heads = list(rows.filter(id=F('versioned_object_id')).order_by('id')) + now = timezone.now() + for age, head in enumerate(reversed(self.heads)): + self.model.objects.filter(id=head.id).update(updated_at=now - timedelta(minutes=age)) + versions = list(rows.exclude(id=F('versioned_object_id')).filter(is_latest_version=True).order_by('id')) + self.v1 = OrganizationSourceFactory( + mnemonic=self.source.mnemonic, organization=self.source.organization, version='v1') + self.v2 = OrganizationSourceFactory( + mnemonic=self.source.mnemonic, organization=self.source.organization, version='v2') + getattr(self.v1, self.relation).add(*versions, self.heads[-1]) + getattr(self.v2, self.relation).add(*versions[::2], self.heads[-1]) + self.in_v2 = {row.id for row in versions[::2]} | {self.heads[-1].id} + + def test_head_list(self): + url = getattr(self.source, f'{self.relation}_url') + data, rows = self.assert_page(url, self.list_serializer) + self.assert_page(url, self.detail_serializer, '&verbose=true') + + for row, instance in zip(data, rows): + self.assertEqual(row['version_url'], instance.get_latest_version().uri) + self.assertEqual(row['latest_source_version'], 'v2' if instance.id == self.heads[-1].id else None) + + def test_version_list(self): + url = getattr(self.v1, f'{self.relation}_url') + data, rows = self.assert_page(url, self.list_serializer) + self.assert_page(url, self.detail_serializer, '&verbose=true') + + for row, instance in zip(data, rows): + self.assertEqual(row['latest_source_version'], 'v2' if instance.id in self.in_v2 else 'v1') diff --git a/core/integration_tests/tests_concepts.py b/core/integration_tests/tests_concepts.py index 7c7584dc9..eda91604a 100644 --- a/core/integration_tests/tests_concepts.py +++ b/core/integration_tests/tests_concepts.py @@ -1,4 +1,3 @@ -from datetime import timedelta from unittest.mock import patch import factory @@ -9,7 +8,6 @@ from django.db.models import F from django.test import override_settings from django.test.utils import CaptureQueriesContext -from django.utils import timezone from mock import ANY from rest_framework.request import Request from rest_framework.test import APIRequestFactory @@ -24,6 +22,7 @@ from core.concepts.models import Concept from core.concepts.serializers import ConceptListSerializer, ConceptDetailSerializer from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory, ConceptDescriptionFactory +from core.integration_tests.test_utils import SourceChildListQueryCountMixin from core.mappings.models import Mapping from core.mappings.tests.factories import MappingFactory from core.orgs.models import Organization @@ -4184,8 +4183,13 @@ def test_post_success(self, bundle_clone_mock): ) -class ConceptListQueryCountTest(OCLAPITestCase): +class ConceptListQueryCountTest(SourceChildListQueryCountMixin, OCLAPITestCase): """A page of concepts costs the same number of queries whatever its size, and serializes as row by row did.""" + model = Concept + relation = 'concepts' + list_serializer = ConceptListSerializer + detail_serializer = ConceptDetailSerializer + def setUp(self): super().setUp() self.source = OrganizationSourceFactory() @@ -4203,60 +4207,6 @@ def setUp(self): locale.set_checksums() self.set_released_versions(concepts) - def get_page(self, url, limit, params=''): - with CaptureQueriesContext(connection) as context: - response = self.client.get(f'{url}?limit={limit}{params}', format='json') - self.assertEqual(response.status_code, 200) - self.assertEqual(len(response.data), limit) - return response, len(context.captured_queries) - - def assert_page(self, url, serializer_class, params=''): - """Same query count for a page of 2 and of 4; same output as serializing each row on its own.""" - _, small_page_queries = self.get_page(url, 2, params) - response, page_queries = self.get_page(url, 4, params) - self.assertEqual(page_queries, small_page_queries) - - context = {'request': response.renderer_context['request'], 'view': response.renderer_context['view']} - rows = [ - Concept.objects.get(versioned_object_id=row['versioned_object_id'], version=row['version']) - for row in response.data - ] - self.assertEqual(response.data, [serializer_class(row, context=context).data for row in rows]) - return response.data, rows - - def set_released_versions(self, rows): - """ - Newest head first by updated_at, so a page of 2 and a page of 4 hold the same kinds of rows. - v1 holds every latest version row, v2 every other one; both hold the newest head as well. - """ - self.heads = list(rows.filter(id=F('versioned_object_id')).order_by('id')) - now = timezone.now() - for age, head in enumerate(reversed(self.heads)): - Concept.objects.filter(id=head.id).update(updated_at=now - timedelta(minutes=age)) - versions = list(rows.exclude(id=F('versioned_object_id')).filter(is_latest_version=True).order_by('id')) - self.v1 = OrganizationSourceFactory( - mnemonic=self.source.mnemonic, organization=self.source.organization, version='v1') - self.v2 = OrganizationSourceFactory( - mnemonic=self.source.mnemonic, organization=self.source.organization, version='v2') - getattr(self.v1, 'concepts').add(*versions, self.heads[-1]) - getattr(self.v2, 'concepts').add(*versions[::2], self.heads[-1]) - self.in_v2 = {row.id for row in versions[::2]} | {self.heads[-1].id} - - def test_head_list(self): - data, rows = self.assert_page(self.source.concepts_url, ConceptListSerializer) - self.assert_page(self.source.concepts_url, ConceptDetailSerializer, '&verbose=true') - - for row, instance in zip(data, rows): - self.assertEqual(row['version_url'], instance.get_latest_version().uri) - self.assertEqual(row['latest_source_version'], 'v2' if instance.id == self.heads[-1].id else None) - - def test_version_list(self): - data, rows = self.assert_page(self.v1.concepts_url, ConceptListSerializer) - self.assert_page(self.v1.concepts_url, ConceptDetailSerializer, '&verbose=true') - - for row, instance in zip(data, rows): - self.assertEqual(row['latest_source_version'], 'v2' if instance.id in self.in_v2 else 'v1') - def test_revisions_in_no_released_version(self): """Search results can hold revisions saved since the last release: no per-row lookup for those either.""" self.v1.concepts.clear() diff --git a/core/integration_tests/tests_mappings.py b/core/integration_tests/tests_mappings.py index 0385fffb2..db7e9a022 100644 --- a/core/integration_tests/tests_mappings.py +++ b/core/integration_tests/tests_mappings.py @@ -1,16 +1,11 @@ -from datetime import timedelta - from django.core.cache import cache -from django.db import connection -from django.db.models import F from django.test import override_settings -from django.test.utils import CaptureQueriesContext -from django.utils import timezone from rest_framework.exceptions import ErrorDetail from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory from core.common.tests import OCLAPITestCase from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory +from core.integration_tests.test_utils import SourceChildListQueryCountMixin from core.mappings.constants import SAME_AS from core.mappings.documents import MappingDocument from core.mappings.models import Mapping @@ -1085,8 +1080,13 @@ def test_repo_version_mappings_with_search_are_not_cached(self): self.assertIsNone(cache.get(body_key + '?q=123')) -class MappingListQueryCountTest(OCLAPITestCase): +class MappingListQueryCountTest(SourceChildListQueryCountMixin, OCLAPITestCase): """A page of mappings costs the same number of queries whatever its size, and serializes as row by row did.""" + model = Mapping + relation = 'mappings' + list_serializer = MappingListSerializer + detail_serializer = MappingDetailSerializer + def setUp(self): super().setUp() self.source = OrganizationSourceFactory() @@ -1106,57 +1106,3 @@ def setUp(self): for mapping in mappings: mapping.set_checksums() # a GET calculates and saves missing checksums, one row at a time self.set_released_versions(mappings) - - def get_page(self, url, limit, params=''): - with CaptureQueriesContext(connection) as context: - response = self.client.get(f'{url}?limit={limit}{params}', format='json') - self.assertEqual(response.status_code, 200) - self.assertEqual(len(response.data), limit) - return response, len(context.captured_queries) - - def assert_page(self, url, serializer_class, params=''): - """Same query count for a page of 2 and of 4; same output as serializing each row on its own.""" - _, small_page_queries = self.get_page(url, 2, params) - response, page_queries = self.get_page(url, 4, params) - self.assertEqual(page_queries, small_page_queries) - - context = {'request': response.renderer_context['request'], 'view': response.renderer_context['view']} - rows = [ - Mapping.objects.get(versioned_object_id=row['versioned_object_id'], version=row['version']) - for row in response.data - ] - self.assertEqual(response.data, [serializer_class(row, context=context).data for row in rows]) - return response.data, rows - - def set_released_versions(self, rows): - """ - Newest head first by updated_at, so a page of 2 and a page of 4 hold the same kinds of rows. - v1 holds every latest version row, v2 every other one; both hold the newest head as well. - """ - self.heads = list(rows.filter(id=F('versioned_object_id')).order_by('id')) - now = timezone.now() - for age, head in enumerate(reversed(self.heads)): - Mapping.objects.filter(id=head.id).update(updated_at=now - timedelta(minutes=age)) - versions = list(rows.exclude(id=F('versioned_object_id')).filter(is_latest_version=True).order_by('id')) - self.v1 = OrganizationSourceFactory( - mnemonic=self.source.mnemonic, organization=self.source.organization, version='v1') - self.v2 = OrganizationSourceFactory( - mnemonic=self.source.mnemonic, organization=self.source.organization, version='v2') - getattr(self.v1, 'mappings').add(*versions, self.heads[-1]) - getattr(self.v2, 'mappings').add(*versions[::2], self.heads[-1]) - self.in_v2 = {row.id for row in versions[::2]} | {self.heads[-1].id} - - def test_head_list(self): - data, rows = self.assert_page(self.source.mappings_url, MappingListSerializer) - self.assert_page(self.source.mappings_url, MappingDetailSerializer, '&verbose=true') - - for row, instance in zip(data, rows): - self.assertEqual(row['version_url'], instance.get_latest_version().uri) - self.assertEqual(row['latest_source_version'], 'v2' if instance.id == self.heads[-1].id else None) - - def test_version_list(self): - data, rows = self.assert_page(self.v1.mappings_url, MappingListSerializer) - self.assert_page(self.v1.mappings_url, MappingDetailSerializer, '&verbose=true') - - for row, instance in zip(data, rows): - self.assertEqual(row['latest_source_version'], 'v2' if instance.id in self.in_v2 else 'v1') diff --git a/core/mappings/models.py b/core/mappings/models.py index 2bc4ac315..27b50a948 100644 --- a/core/mappings/models.py +++ b/core/mappings/models.py @@ -419,12 +419,6 @@ def get_source(url): def is_existing_in_parent(self): return self.parent.mappings_set.filter(mnemonic__exact=self.mnemonic).exists() - @property - def latest_source_version(self): - if hasattr(self, '_latest_source_version'): # loaded for the whole page by load_list_fields - return self._latest_source_version - return self.sources.exclude(version=HEAD).order_by('-created_at').first() - @classmethod def create_new_version_for(cls, instance, data, user, cache=None): instance.populate_fields_from_relations(data, cache=cache, user=user) From 11caddf199be0a5c8a7e1c598994e2389e651174 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 07:09:35 -0400 Subject: [PATCH 4/5] OpenConceptLab/ocl_online#454 | Collection lists: resolved source version, batched and gated per field From Sunny's review of #936: - Collection-scoped concept and mapping lists return latest_source_version again, as the source version the row's references in that collection resolved to (what TBv3 computes from includeReferences today), not the latest release holding the row (ocl_issues#2770). HEAD resolves to null. It's loaded once per page; a single object computes it for itself. - load_list_fields only runs the lookups the serialized fields read, so a collection mapping list skips the latest-release lookup. - Collection pages no longer query per row for previous_version_url, references, names, descriptions, source_versions or collection_versions. The last three are now ordered (created_at; sorted uris); the order was arbitrary before. - Tests cover collection and global lists too. Co-Authored-By: Claude Opus 5.5 --- core/common/mixins.py | 140 ++++++++++++++++++++++++--- core/common/models.py | 2 + core/common/serializers.py | 26 ++++- core/concepts/models.py | 4 +- core/concepts/serializers.py | 23 +++-- core/integration_tests/test_utils.py | 26 ++++- core/mappings/models.py | 3 +- core/mappings/serializers.py | 14 +-- 8 files changed, 202 insertions(+), 36 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index 901c94ad4..843370e35 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -646,24 +646,83 @@ class SourceChildMixin(ChecksumModel): # relations the list serializers read on every row, prefetched once per page by load_list_fields LIST_PREFETCHES = () + # relations a serializer field reads on every row, prefetched by load_list_fields when that field is serialized + FIELD_PREFETCHES = {} + # serializer fields that read latest_source_version + LATEST_SOURCE_VERSION_FIELDS = ('latest_source_version', ) class Meta: abstract = True @classmethod - def load_list_fields(cls, instances): + def load_list_fields(cls, instances, fields, collection=None): """ - Loads what the list serializers would otherwise query row by row, for a whole page at once: - LIST_PREFETCHES, the latest version's uri (version_url) and latest_source_version. + Loads what the list serializers would otherwise query row by row, for a whole page at once, and only what + the serialized fields read: prefetched relations, the latest and previous versions' uris, the source and + collection versions holding each row, latest_source_version and, in a collection, the row's references and + the source versions they resolved to. """ - prefetch_related_objects(instances, *cls.LIST_PREFETCHES) + fields = set(fields) + prefetch_related_objects(instances, *cls.LIST_PREFETCHES, *( + lookup for field, lookups in cls.FIELD_PREFETCHES.items() if field in fields for lookup in lookups)) + loaders = { + 'version_url': cls.load_latest_version_uris, + 'previous_version_url': cls.load_previous_version_uris, + 'source_versions': cls.load_source_versions, + 'collection_versions': cls.load_collection_versions, + } + for field, loader in loaders.items(): + if field in fields: + loader(instances) + + reads_latest_source_version = fields & set(cls.LATEST_SOURCE_VERSION_FIELDS) + if collection: # there latest_source_version shows the collection's resolved version instead + reads_latest_source_version.discard('latest_source_version') + if 'references' in fields: + cls.load_collection_references(instances, collection) + if 'latest_source_version' in fields: + cls.load_collection_source_versions(instances, collection) + if reads_latest_source_version: + cls.load_latest_source_versions(instances) + @classmethod + def load_latest_version_uris(cls, instances): + """Sets each HEAD row's _latest_version_uri (version_url) from one query for the page.""" heads = [instance.id for instance in instances if instance.is_versioned_object] latest_version_uris = dict(cls.objects.filter( versioned_object_id__in=heads, is_active=True, is_latest_version=True ).exclude(id=F('versioned_object_id')).order_by('versioned_object_id', '-created_at').distinct( 'versioned_object_id').values_list('versioned_object_id', 'uri')) + for instance in instances: + instance._latest_version_uri = latest_version_uris.get(instance.id) # pylint: disable=protected-access + + @classmethod + def load_source_versions(cls, instances): + """Sets each row's _source_versions: the uris of the released source versions holding it.""" + key = f'{cls._meta.model_name}_id' + uris = {} + for row_id, uri in cls.sources.through.objects.filter( + **{f'{key}__in': [instance.id for instance in instances]} + ).exclude(source__version=HEAD).order_by('source__created_at').values_list(key, 'source__uri'): + uris.setdefault(row_id, []).append(uri) + for instance in instances: + instance._source_versions = uris.get(instance.id, []) # pylint: disable=protected-access + @classmethod + def load_collection_versions(cls, instances): + """Sets each row's _collection_versions: the uris of the collection versions whose expansions hold it.""" + key = f'{cls._meta.model_name}_id' + uris = {} + for row_id, uri in cls.expansion_set.through.objects.filter( + **{f'{key}__in': [instance.id for instance in instances]} + ).exclude(expansion__collection_version__version=HEAD).values_list(key, 'expansion__collection_version__uri'): + uris.setdefault(row_id, set()).add(uri) + for instance in instances: + instance._collection_versions = sorted(uris.get(instance.id, ())) # pylint: disable=protected-access + + @classmethod + def load_latest_source_versions(cls, instances): + """Sets each row's _latest_source_version: the latest released source version holding it.""" key = f'{cls._meta.model_name}_id' latest_source_version_ids = dict(cls.sources.through.objects.filter( **{f'{key}__in': [instance.id for instance in instances]} @@ -671,11 +730,67 @@ def load_list_fields(cls, instances): key, 'source_id')) source_versions = cls._meta.get_field('sources').related_model.objects.in_bulk( set(latest_source_version_ids.values())) - for instance in instances: # pylint: disable=protected-access instance._latest_source_version = source_versions.get(latest_source_version_ids.get(instance.id)) - instance._latest_version_uri = latest_version_uris.get(instance.id) + + @classmethod + def load_previous_version_uris(cls, instances): + """Sets each row's _prev_version_uri, as prev_version_uri would query it, from one query for the page.""" + siblings = {} + for row_id, versioned_object_id, created_at, uri in cls.objects.filter( + versioned_object_id__in={instance.versioned_object_id for instance in instances}, is_active=True + ).exclude(id=F('versioned_object_id')).values_list('id', 'versioned_object_id', 'created_at', 'uri'): + siblings.setdefault(versioned_object_id, []).append((created_at, row_id, uri)) + for instance in instances: + previous = [ + (created_at, uri) for created_at, row_id, uri in siblings.get(instance.versioned_object_id, []) + if row_id != instance.id and created_at <= instance.created_at + ] + # pylint: disable=protected-access + instance._prev_version_uri = max(previous, key=lambda version: version[0])[1] if previous else None + + @classmethod + def load_collection_references(cls, instances, collection): + """Sets each row's _collection_references: (collection id, ids of its references in that collection).""" + key = f'{cls._meta.model_name}_id' + reference_ids = {} + for row_id, reference_id in cls.references.through.objects.filter( + **{f'{key}__in': [instance.id for instance in instances]}, reference__collection_id=collection.id + ).order_by('reference_id').values_list(key, 'reference_id'): + reference_ids.setdefault(row_id, []).append(reference_id) + for instance in instances: + # pylint: disable=protected-access + instance._collection_references = (collection.id, reference_ids.get(instance.id, [])) + + def get_collection_reference_ids(self, collection): + loaded = getattr(self, '_collection_references', None) + if loaded and loaded[0] == collection.id: + return loaded[1] + return list(self.collection_references(collection).order_by('id').values_list('id', flat=True)) + + @classmethod + def load_collection_source_versions(cls, instances, collection): + """ + Sets each row's _collection_source_version: the newest source version its references in this collection + resolved to, as TBv3 shows a collection's rows, or None when they resolved to HEAD or to no source version. + """ + cls.load_collection_references( + [instance for instance in instances if get(instance, '_collection_references.0') != collection.id], + collection) + reference_ids = {instance.id: instance.get_collection_reference_ids(collection) for instance in instances} + references = cls._meta.get_field('references').related_model.objects.in_bulk( + {reference_id for ids in reference_ids.values() for reference_id in ids}) + + resolved = {} + for instance in instances: + versions = compact( + references[reference_id]._get_resolved_system_version(resolved) # pylint: disable=protected-access + for reference_id in reference_ids[instance.id] + ) + newest = max(versions, key=lambda version: version.created_at, default=None) + # pylint: disable=protected-access + instance._collection_source_version = None if get(newest, 'version') == HEAD else newest def calculate_uri(self): uri = self.parent.uri + self.resource_type.lower() + 's/' + str(self.mnemonic) + '/' @@ -766,12 +881,16 @@ def get_filters_for_criterion(params, prefix=None): @property def source_versions(self): - return self.sources.exclude(version=HEAD).values_list('uri', flat=True) + if hasattr(self, '_source_versions'): # loaded for a whole page by load_list_fields + return self._source_versions + return list(self.sources.exclude(version=HEAD).order_by('created_at').values_list('uri', flat=True)) @property def collection_versions(self): - return set(self.expansion_set.exclude( - collection_version__version=HEAD).values_list('collection_version__uri', flat=True)) + if hasattr(self, '_collection_versions'): # loaded for a whole page by load_list_fields + return self._collection_versions + return sorted(set(self.expansion_set.exclude( + collection_version__version=HEAD).values_list('collection_version__uri', flat=True))) @property def versions(self): @@ -946,8 +1065,7 @@ def save(self, *args, force_insert=False, force_update=False, using=None, update self.save(update_fields=['_counted']) def collection_references_uris(self, collection): - ids = self.collection_references(collection).values_list('id', flat=True) - return [f"{collection.uri}references/{_id}/" for _id in ids] + return [f"{collection.uri}references/{_id}/" for _id in self.get_collection_reference_ids(collection)] def collection_references(self, collection): return self.references.filter(collection=collection) diff --git a/core/common/models.py b/core/common/models.py index 4a54fbdec..782cde907 100644 --- a/core/common/models.py +++ b/core/common/models.py @@ -683,6 +683,8 @@ def prev_version(self): @property def prev_version_uri(self): + if hasattr(self, '_prev_version_uri'): # loaded for a whole page by SourceChildMixin.load_list_fields + return self._prev_version_uri return get(self, 'prev_version.uri') @property diff --git a/core/common/serializers.py b/core/common/serializers.py index 7a6d3c836..a2c7bb20d 100644 --- a/core/common/serializers.py +++ b/core/common/serializers.py @@ -226,11 +226,35 @@ def get_search_highlight(obj): return cleaned_highlight +def get_collection_in_scope(context): + """The collection (version) a concept or mapping view is scoped to, when the view has loaded it.""" + if 'collection' in get(context, 'view.kwargs', {}): + instance = get(context, 'request.instance') + if get(instance, 'resource_type') == 'Collection': + return instance + return None + + +class SourceVersionFieldMixin: + """ + latest_source_version for concepts and mappings: the latest released source version holding the row or, listed + in a collection, the source version the row's references there resolved to (ocl_issues#2770). + """ + def get_latest_source_version(self, obj): + collection = get_collection_in_scope(self.context) + if collection: + if not hasattr(obj, '_collection_source_version'): + obj.load_collection_source_versions([obj], collection) + return get(obj, '_collection_source_version.version') + return get(obj.latest_source_version, 'version') + + class SourceChildPageSerializer(ListSerializer): # pylint: disable=abstract-method """Serializes a page of concepts or mappings, loading the per-row lookups for the whole page first.""" def to_representation(self, data): instances = list(data.all() if isinstance(data, BaseManager) else data) - self.child.Meta.model.load_list_fields(instances) + self.child.Meta.model.load_list_fields( + instances, self.child.fields.keys(), get_collection_in_scope(self.context)) return super().to_representation(instances) diff --git a/core/concepts/models.py b/core/concepts/models.py index 4199fcfe4..cd54029ec 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -220,7 +220,9 @@ class HierarchicalConcepts(models.Model): class Concept(ConceptValidationMixin, SourceChildMixin, VersionedModel): # pylint: disable=too-many-public-methods - LIST_PREFETCHES = ('parent', 'created_by', 'updated_by') + LIST_PREFETCHES = ('parent', 'created_by', 'updated_by', 'names') + FIELD_PREFETCHES = {'descriptions': ('descriptions', )} + LATEST_SOURCE_VERSION_FIELDS = ('latest_source_version', 'property') # property: get_parent_source_version class Meta: db_table = 'concepts' diff --git a/core/concepts/serializers.py b/core/concepts/serializers.py index aecffe228..6c77557e3 100644 --- a/core/concepts/serializers.py +++ b/core/concepts/serializers.py @@ -9,7 +9,8 @@ INCLUDE_CHILD_CONCEPT_URLS, HEAD, INCLUDE_SUMMARY, INCLUDE_VERBOSE_REFERENCES, VERBOSE_PARAM, \ INCLUDE_RETIRED_PARAM, ACCESS_TYPE_NONE from core.common.fields import EncodedDecodedCharField -from core.common.serializers import AbstractResourceSerializer, SourceChildPageSerializer +from core.common.serializers import AbstractResourceSerializer, SourceChildPageSerializer, \ + SourceVersionFieldMixin, get_collection_in_scope from core.common.utils import to_parent_uri_from_kwargs, get_truthy_values from core.concepts.models import Concept, ConceptName @@ -135,7 +136,7 @@ def to_representation(self, instance): # used to be to_native return ret -class ConceptAbstractSerializer(AbstractResourceSerializer): +class ConceptAbstractSerializer(SourceVersionFieldMixin, AbstractResourceSerializer): uuid = CharField(source='id', read_only=True) mappings = SerializerMethodField() mappings_payload = ListField(child=JSONField(), write_only=True, required=False, allow_empty=True) @@ -204,7 +205,7 @@ def __init__(self, *args, **kwargs): # pylint: disable=too-many-branches self.fields.pop('references', None) if get(params, 'onlyParentLess') not in TRUTHY and get(params, 'onlyHierarchyRoot') not in TRUTHY: self.fields.pop('has_children', None) - if 'collection' in self.view_kwargs: + if 'collection' in self.view_kwargs and not get_collection_in_scope(get(kwargs, 'context')): self.fields.pop('latest_source_version', None) except: # pylint: disable=bare-except pass @@ -317,8 +318,7 @@ class ConceptListSerializer(ConceptAbstractSerializer): version_created_by = DateTimeField(source='created_by.username', read_only=True) version_updated_on = DateTimeField(source='updated_at', read_only=True) version_updated_by = DateTimeField(source='updated_by.username', read_only=True) - latest_source_version = CharField( - source='latest_source_version.version', allow_null=True, allow_blank=True, read_only=True, required=False) + latest_source_version = SerializerMethodField() property = JSONField(source='summary_properties', read_only=True) class Meta: @@ -357,7 +357,7 @@ def __init__(self, *args, **kwargs): self.fields.pop('source_versions', None) if not self.include_collection_versions: self.fields.pop('collection_versions', None) - if 'collection' in self.view_kwargs: + if 'collection' in self.view_kwargs and not get_collection_in_scope(get(kwargs, 'context')): self.fields.pop('latest_source_version', None) except: # pylint: disable=bare-except pass @@ -481,8 +481,7 @@ class ConceptDetailSerializer(ConceptAbstractSerializer): url = CharField(required=False, source='versioned_object_url') updated_by = DateTimeField(source='updated_by.username', read_only=True) created_by = DateTimeField(source='created_by.username', read_only=True) - latest_source_version = CharField( - source='latest_source_version.version', allow_null=True, allow_blank=True, read_only=True, required=False) + latest_source_version = SerializerMethodField() property = JSONField(source='properties', read_only=True) class Meta: @@ -576,7 +575,7 @@ def get_child_concept_urls(obj): return list(uris) -class ConceptVersionDetailSerializer(ModelSerializer): +class ConceptVersionDetailSerializer(SourceVersionFieldMixin, ModelSerializer): type = CharField(source='resource_type') uuid = CharField(source='id') id = EncodedDecodedCharField(source='mnemonic') @@ -603,8 +602,7 @@ class ConceptVersionDetailSerializer(ModelSerializer): source_versions = ListField(read_only=True) collection_versions = ListField(read_only=True) references = SerializerMethodField() - latest_source_version = CharField( - source='latest_source_version.version', allow_null=True, allow_blank=True, read_only=True, required=False) + latest_source_version = SerializerMethodField() def __init__(self, *args, **kwargs): request = get(kwargs, 'context.request') @@ -633,7 +631,7 @@ def __init__(self, *args, **kwargs): self.fields.pop('parent_concept_urls') if not get(request, 'instance'): self.fields.pop('references', None) - if 'collection' in self.view_kwargs: + if 'collection' in self.view_kwargs and not get_collection_in_scope(get(kwargs, 'context')): self.fields.pop('latest_source_version', None) except: # pylint: disable=bare-except pass @@ -642,6 +640,7 @@ def __init__(self, *args, **kwargs): class Meta: model = Concept + list_serializer_class = SourceChildPageSerializer fields = ( 'type', 'uuid', 'id', 'external_id', 'concept_class', 'datatype', 'display_name', 'display_locale', 'names', 'descriptions', 'extras', 'retired', 'source', 'source_url', 'owner', 'owner_name', 'owner_url', diff --git a/core/integration_tests/test_utils.py b/core/integration_tests/test_utils.py index 31cd0f95f..f5b23c724 100644 --- a/core/integration_tests/test_utils.py +++ b/core/integration_tests/test_utils.py @@ -8,6 +8,7 @@ from django.utils import timezone from jsonpath_ng import parse +from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory from core.sources.tests.factories import OrganizationSourceFactory @@ -76,13 +77,13 @@ def assert_page(self, url, serializer_class, params=''): def set_released_versions(self, rows): """ - Newest head first by updated_at, so a page of 2 and a page of 4 hold the same kinds of rows. + Newest first by updated_at in creation order, so a page of 2 and a page of 4 hold the same kinds of rows. v1 holds every latest version row, v2 every other one; both hold the newest head as well. """ self.heads = list(rows.filter(id=F('versioned_object_id')).order_by('id')) now = timezone.now() - for age, head in enumerate(reversed(self.heads)): - self.model.objects.filter(id=head.id).update(updated_at=now - timedelta(minutes=age)) + for age, row in enumerate(rows.order_by('-id')): + self.model.objects.filter(id=row.id).update(updated_at=now - timedelta(minutes=age)) versions = list(rows.exclude(id=F('versioned_object_id')).filter(is_latest_version=True).order_by('id')) self.v1 = OrganizationSourceFactory( mnemonic=self.source.mnemonic, organization=self.source.organization, version='v1') @@ -108,3 +109,22 @@ def test_version_list(self): for row, instance in zip(data, rows): self.assertEqual(row['latest_source_version'], 'v2' if instance.id in self.in_v2 else 'v1') + + def test_collection_list(self): + """In a collection, rows show the source version their references resolved to (v1), not the latest one.""" + collection = OrganizationCollectionFactory() + expansion = ExpansionFactory(collection_version=collection) + collection.expansion_uri = expansion.uri + collection.save() + collection.add_expressions( + {'expressions': [f'{self.v1.uri}{self.relation}/{head.mnemonic}/' for head in self.heads]}, + collection.created_by) + + url = getattr(collection, f'{self.relation}_url') + data, _ = self.assert_page(url, self.model.get_serializer_class(version=True)) + self.assert_page(url, self.model.get_serializer_class(verbose=True, version=True), '&verbose=true') + self.assertEqual({row['latest_source_version'] for row in data}, {'v1'}) + + def test_global_list(self): + self.assert_page(f'/{self.relation}/', self.list_serializer) + self.assert_page(f'/{self.relation}/', self.detail_serializer, '&verbose=true') diff --git a/core/mappings/models.py b/core/mappings/models.py index 27b50a948..e183d8370 100644 --- a/core/mappings/models.py +++ b/core/mappings/models.py @@ -22,7 +22,8 @@ class Mapping(MappingValidationMixin, SourceChildMixin, VersionedModel): LIST_PREFETCHES = ( 'from_concept__parent', 'to_concept__parent', 'from_source__organization', 'from_source__user', - 'to_source__organization', 'to_source__user', 'created_by', 'updated_by' + 'to_source__organization', 'to_source__user', 'created_by', 'updated_by', 'from_concept__names', + 'to_concept__names' ) class Meta: diff --git a/core/mappings/serializers.py b/core/mappings/serializers.py index e53d5bb2d..df6d17cd8 100644 --- a/core/mappings/serializers.py +++ b/core/mappings/serializers.py @@ -7,7 +7,8 @@ MAPPING_LOOKUP_TO_CONCEPT, MAPPING_LOOKUP_FROM_SOURCE, MAPPING_LOOKUP_TO_SOURCE, INCLUDE_EXTRAS_PARAM, \ INCLUDE_SOURCE_VERSIONS, INCLUDE_COLLECTION_VERSIONS, INCLUDE_VERBOSE_REFERENCES from core.common.fields import EncodedDecodedCharField -from core.common.serializers import AbstractResourceSerializer, SourceChildPageSerializer +from core.common.serializers import AbstractResourceSerializer, SourceChildPageSerializer, \ + SourceVersionFieldMixin, get_collection_in_scope from core.common.utils import get_truthy_values from core.concepts.serializers import ConceptDetailSerializer from core.mappings.models import Mapping @@ -17,7 +18,7 @@ TRUTHY = get_truthy_values() -class AbstractMappingSerializer(AbstractResourceSerializer): +class AbstractMappingSerializer(SourceVersionFieldMixin, AbstractResourceSerializer): from_concept = SerializerMethodField() to_concept = SerializerMethodField() from_source = SourceListSerializer() @@ -64,7 +65,7 @@ def __init__(self, *args, **kwargs): if not get(request, 'instance'): self.fields.pop('references', None) - if 'collection' in self.view_kwargs: + if 'collection' in self.view_kwargs and not get_collection_in_scope(get(kwargs, 'context')): self.fields.pop('latest_source_version', None) super().__init__(*args, **kwargs) @@ -119,8 +120,7 @@ class MappingListSerializer(AbstractMappingSerializer): sort_weight = FloatField(required=False, allow_null=True) version_updated_on = DateTimeField(source='updated_at', read_only=True) version_updated_by = DateTimeField(source='updated_by.username', read_only=True) - latest_source_version = CharField( - source='latest_source_version.version', allow_null=True, allow_blank=True, read_only=True, required=False) + latest_source_version = SerializerMethodField() class Meta: model = Mapping @@ -249,8 +249,7 @@ class MappingDetailSerializer(MappingListSerializer): to_source = SourceDetailSerializer() created_on = DateTimeField(source='created_at', read_only=True) updated_on = DateTimeField(source='updated_at', read_only=True) - latest_source_version = CharField( - source='latest_source_version.version', allow_null=True, allow_blank=True, read_only=True, required=False) + latest_source_version = SerializerMethodField() class Meta: model = Mapping @@ -325,6 +324,7 @@ class MappingVersionDetailSerializer(MappingDetailSerializer): class Meta: model = Mapping + list_serializer_class = SourceChildPageSerializer fields = MappingDetailSerializer.Meta.fields + ( 'previous_version_url', 'source_versions', 'collection_versions', ) From ba8e08606e13e6cfb968b8fed2d74ba862cc37ac Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 07:24:02 -0400 Subject: [PATCH 5/5] OpenConceptLab/ocl_online#454 | Simplify: one place loads collection references; plainer loaded attributes From the simplification review of 11caddf1: load_list_fields loads a page's collection references once for either field that needs them; the reference ids are a plain loaded attribute; FIELD_PREFETCHES is a tuple of relation names; a shorter load_list_fields docstring. Co-Authored-By: Claude Opus 5.5 --- core/common/mixins.py | 29 +++++++++-------------------- core/concepts/models.py | 2 +- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index 843370e35..04760a764 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -646,8 +646,8 @@ class SourceChildMixin(ChecksumModel): # relations the list serializers read on every row, prefetched once per page by load_list_fields LIST_PREFETCHES = () - # relations a serializer field reads on every row, prefetched by load_list_fields when that field is serialized - FIELD_PREFETCHES = {} + # relations named like the serializer field that reads them; prefetched by load_list_fields when it's serialized + FIELD_PREFETCHES = () # serializer fields that read latest_source_version LATEST_SOURCE_VERSION_FIELDS = ('latest_source_version', ) @@ -656,15 +656,9 @@ class Meta: @classmethod def load_list_fields(cls, instances, fields, collection=None): - """ - Loads what the list serializers would otherwise query row by row, for a whole page at once, and only what - the serialized fields read: prefetched relations, the latest and previous versions' uris, the source and - collection versions holding each row, latest_source_version and, in a collection, the row's references and - the source versions they resolved to. - """ + """Loads, for a whole page at once, what the serialized fields would otherwise query row by row.""" fields = set(fields) - prefetch_related_objects(instances, *cls.LIST_PREFETCHES, *( - lookup for field, lookups in cls.FIELD_PREFETCHES.items() if field in fields for lookup in lookups)) + prefetch_related_objects(instances, *cls.LIST_PREFETCHES, *fields.intersection(cls.FIELD_PREFETCHES)) loaders = { 'version_url': cls.load_latest_version_uris, 'previous_version_url': cls.load_previous_version_uris, @@ -678,7 +672,7 @@ def load_list_fields(cls, instances, fields, collection=None): reads_latest_source_version = fields & set(cls.LATEST_SOURCE_VERSION_FIELDS) if collection: # there latest_source_version shows the collection's resolved version instead reads_latest_source_version.discard('latest_source_version') - if 'references' in fields: + if fields & {'references', 'latest_source_version'}: cls.load_collection_references(instances, collection) if 'latest_source_version' in fields: cls.load_collection_source_versions(instances, collection) @@ -752,7 +746,7 @@ def load_previous_version_uris(cls, instances): @classmethod def load_collection_references(cls, instances, collection): - """Sets each row's _collection_references: (collection id, ids of its references in that collection).""" + """Sets each row's _collection_reference_ids: the ids of its references in that collection.""" key = f'{cls._meta.model_name}_id' reference_ids = {} for row_id, reference_id in cls.references.through.objects.filter( @@ -760,13 +754,11 @@ def load_collection_references(cls, instances, collection): ).order_by('reference_id').values_list(key, 'reference_id'): reference_ids.setdefault(row_id, []).append(reference_id) for instance in instances: - # pylint: disable=protected-access - instance._collection_references = (collection.id, reference_ids.get(instance.id, [])) + instance._collection_reference_ids = reference_ids.get(instance.id, []) # pylint: disable=protected-access def get_collection_reference_ids(self, collection): - loaded = getattr(self, '_collection_references', None) - if loaded and loaded[0] == collection.id: - return loaded[1] + if hasattr(self, '_collection_reference_ids'): # loaded for a whole page by load_list_fields + return self._collection_reference_ids return list(self.collection_references(collection).order_by('id').values_list('id', flat=True)) @classmethod @@ -775,9 +767,6 @@ def load_collection_source_versions(cls, instances, collection): Sets each row's _collection_source_version: the newest source version its references in this collection resolved to, as TBv3 shows a collection's rows, or None when they resolved to HEAD or to no source version. """ - cls.load_collection_references( - [instance for instance in instances if get(instance, '_collection_references.0') != collection.id], - collection) reference_ids = {instance.id: instance.get_collection_reference_ids(collection) for instance in instances} references = cls._meta.get_field('references').related_model.objects.in_bulk( {reference_id for ids in reference_ids.values() for reference_id in ids}) diff --git a/core/concepts/models.py b/core/concepts/models.py index cd54029ec..6cd515472 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -221,7 +221,7 @@ class HierarchicalConcepts(models.Model): class Concept(ConceptValidationMixin, SourceChildMixin, VersionedModel): # pylint: disable=too-many-public-methods LIST_PREFETCHES = ('parent', 'created_by', 'updated_by', 'names') - FIELD_PREFETCHES = {'descriptions': ('descriptions', )} + FIELD_PREFETCHES = ('descriptions', ) LATEST_SOURCE_VERSION_FIELDS = ('latest_source_version', 'property') # property: get_parent_source_version class Meta: