diff --git a/CHANGES/7831.feature b/CHANGES/7831.feature new file mode 100644 index 00000000000..82c18216388 --- /dev/null +++ b/CHANGES/7831.feature @@ -0,0 +1,5 @@ +Added optional `added_between` and `removed_between` filters to the content list endpoints. Each +takes two repository versions (by HREF/PRN) as `base,target` and returns the net set of content +added or removed going from the base version to the target version, allowing diffs between two +arbitrary (possibly non-adjacent) repository versions instead of only the single-step difference +against the immediate predecessor. diff --git a/pulpcore/app/models/repository.py b/pulpcore/app/models/repository.py index 5be034f5ffa..1537449f9a7 100644 --- a/pulpcore/app/models/repository.py +++ b/pulpcore/app/models/repository.py @@ -997,15 +997,24 @@ def get_content(self, content_qs=None): if content_qs is None: content_qs = Content.objects - content_ids = self.content_ids - if len(content_ids) >= 65535: - # Workaround for PostgreSQL's limit on the number of parameters in a query - content_ids = ( - RepositoryVersion.objects.filter(pk=self.pk) - .annotate(cids=Func(F("content_ids"), function="unnest")) - .values_list("cids", flat=True) - ) - return content_qs.filter(pk__in=content_ids) + return content_qs.filter(pk__in=self.content_ids_subquery()) + + def content_ids_subquery(self): + """ + Return this version's ``content_ids`` as a database-side ``unnest`` subquery. + + Using a subquery keeps the content unit UUIDs inside PostgreSQL instead of loading the + whole array into Python and passing each UUID as a bound query parameter. This avoids the + per-query parameter limit and the memory/serialization cost for large repository versions. + + Returns: + django.db.models.QuerySet: A values queryset yielding the content unit UUIDs. + """ + return ( + RepositoryVersion.objects.filter(pk=self.pk) + .annotate(cids=Func(F("content_ids"), function="unnest")) + .values_list("cids", flat=True) + ) @property def content(self): @@ -1119,8 +1128,8 @@ def added(self, base_version=None): if not base_version: return Content.objects.filter(version_memberships__version_added=self) - return Content.objects.filter(pk__in=self.content_ids).exclude( - pk__in=base_version.content_ids + return Content.objects.filter(pk__in=self.content_ids_subquery()).exclude( + pk__in=base_version.content_ids_subquery() ) def removed(self, base_version=None): @@ -1134,8 +1143,8 @@ def removed(self, base_version=None): if not base_version: return Content.objects.filter(version_memberships__version_removed=self) - return Content.objects.filter(pk__in=base_version.content_ids).exclude( - pk__in=self.content_ids + return Content.objects.filter(pk__in=base_version.content_ids_subquery()).exclude( + pk__in=self.content_ids_subquery() ) def contains(self, content): diff --git a/pulpcore/app/viewsets/base.py b/pulpcore/app/viewsets/base.py index 9f302103c5f..830acb69606 100644 --- a/pulpcore/app/viewsets/base.py +++ b/pulpcore/app/viewsets/base.py @@ -157,7 +157,7 @@ def get_resource_model(uri): return match.func.cls.queryset.model @staticmethod - def get_resource(uri, model=None): + def get_resource(uri, model=None, deferred_fields=None): """ Resolve a resource URI/PRN to an instance of the resource. @@ -168,6 +168,9 @@ def get_resource(uri, model=None): uri (str): A resource URI/PRN. model (django.models.Model): A model class. If not provided, the method automatically determines the used model from the resource URI/PRN. + deferred_fields (iterable): Optional field names to defer when loading the resource, so + large columns are not fetched from the database. Field names that do not exist on + the resolved model are ignored. Returns: django.models.Model: The resource fetched from the DB. @@ -209,7 +212,13 @@ def get_resource(uri, model=None): kwargs[key] = value try: - return model.objects.get(**kwargs) + manager = model.objects + if deferred_fields: + model_fields = {field.name for field in model._meta.concrete_fields} + to_defer = [name for name in deferred_fields if name in model_fields] + if to_defer: + manager = manager.defer(*to_defer) + return manager.get(**kwargs) except model.MultipleObjectsReturned: raise DRFValidationError( detail=_("URI {u} matches more than one {m}.").format( diff --git a/pulpcore/app/viewsets/content.py b/pulpcore/app/viewsets/content.py index a23ad0e13dd..56a786c399b 100644 --- a/pulpcore/app/viewsets/content.py +++ b/pulpcore/app/viewsets/content.py @@ -23,6 +23,8 @@ ContentAddedRepositoryVersionFilter, ContentRemovedRepositoryVersionFilter, ContentRepositoryVersionFilter, + RepositoryVersionAddedBetweenFilter, + RepositoryVersionRemovedBetweenFilter, ) @@ -125,6 +127,14 @@ class ContentFilter(BaseFilterSet): Return Content which was added in this repository version. repository_version_removed: Return Content which was removed from this repository version. + added_between: + Given two Repository Version HREFs/PRNs as 'base,target', return the net set of + Content added going from the base version to the target version (present in target + but not in base). Diffs two arbitrary (possibly non-adjacent) versions. + removed_between: + Given two Repository Version HREFs/PRNs as 'base,target', return the net set of + Content removed going from the base version to the target version (present in base + but not in target). Diffs two arbitrary (possibly non-adjacent) versions. orphaned_for: Return Content which has been orphaned for a given number of minutes; -1 uses ORPHAN_PROTECTION_TIME value. @@ -135,6 +145,8 @@ class ContentFilter(BaseFilterSet): repository_version = ContentRepositoryVersionFilter() repository_version_added = ContentAddedRepositoryVersionFilter() repository_version_removed = ContentRemovedRepositoryVersionFilter() + added_between = RepositoryVersionAddedBetweenFilter() + removed_between = RepositoryVersionRemovedBetweenFilter() orphaned_for = OrphanedFilter( help_text="Minutes Content has been orphaned for. -1 uses ORPHAN_PROTECTION_TIME." ) diff --git a/pulpcore/app/viewsets/custom_filters.py b/pulpcore/app/viewsets/custom_filters.py index 72db8022db7..0daa9dc642c 100644 --- a/pulpcore/app/viewsets/custom_filters.py +++ b/pulpcore/app/viewsets/custom_filters.py @@ -9,7 +9,7 @@ from django.conf import settings from django.db.models import Q -from django_filters import BaseInFilter, CharFilter, Filter +from django_filters import BaseInFilter, BaseRangeFilter, CharFilter, Filter from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import extend_schema_field from rest_framework import serializers @@ -145,9 +145,12 @@ def get_repository_version(value): detail=_("No value supplied for repository version filter") ) - object = NamedModelViewSet.get_resource(value) + # content_ids is only consumed as a database-side subquery (see + # RepositoryVersion.content_ids_subquery), so defer the potentially huge array column here + # to keep it from being loaded into Python when resolving the version. + object = NamedModelViewSet.get_resource(value, deferred_fields=("content_ids",)) if isinstance(object, Repository): - object = object.latest_version() + object = object.versions.complete().defer("content_ids").last() if not isinstance(object, RepositoryVersion): raise serializers.ValidationError( detail=_("URI {u} not found for {m}.").format(u=value, m="repositoryversion") @@ -163,6 +166,90 @@ def filter(self, qs, value): raise NotImplementedError() +class RepositoryVersionBetweenFilter(BaseRangeFilter, RepoVersionHrefPrnFilter): + """ + Base class for range filters that diff content between two arbitrary repository versions. + + The filter accepts exactly two comma-separated Repository Version (or Repository) HREF/PRN + values in ``base,target`` order: the first is the base version and the second is the target + version. Subclasses implement :meth:`diff` to return the appropriate net set of content. + """ + + def diff(self, base_version, target_version): + """ + Return the content that differs between ``base_version`` and ``target_version``. + + Args: + base_version (pulpcore.app.models.RepositoryVersion): The base repository version. + target_version (pulpcore.app.models.RepositoryVersion): The target repository version. + + Returns: + django.db.models.query.QuerySet: A Content queryset with the diff. + """ + raise NotImplementedError() + + def filter(self, qs, value): + """ + Args: + qs (django.db.models.query.QuerySet): The Content Queryset + value (list): The ``[base, target]`` RepositoryVersion href/prn pair to diff between. + """ + if not value: + return qs + + base_version = self.get_repository_version(value[0]) + target_version = self.get_repository_version(value[1]) + return qs.filter(pk__in=self.diff(base_version, target_version)) + + +class RepositoryVersionAddedBetweenFilter(RepositoryVersionBetweenFilter): + """ + Filter Content to the net set added going from a base repository version to a target version. + + Given ``base,target``, returns the content present in ``target`` but not in ``base`` -- the + net difference across two arbitrary (possibly non-adjacent) repository versions, rather than + the single-step difference computed by ``repository_version_added``. + """ + + def __init__(self, *args, **kwargs): + kwargs.setdefault( + "help_text", + _( + "Two Repository Versions referenced by HREF/PRN as 'base,target'. Returns the " + "content added going from the base version to the target version (present in " + "target but not in base)." + ), + ) + super().__init__(*args, **kwargs) + + def diff(self, base_version, target_version): + return target_version.added(base_version=base_version) + + +class RepositoryVersionRemovedBetweenFilter(RepositoryVersionBetweenFilter): + """ + Filter Content to the net set removed going from a base repository version to a target version. + + Given ``base,target``, returns the content present in ``base`` but not in ``target`` -- the + net difference across two arbitrary (possibly non-adjacent) repository versions, rather than + the single-step difference computed by ``repository_version_removed``. + """ + + def __init__(self, *args, **kwargs): + kwargs.setdefault( + "help_text", + _( + "Two Repository Versions referenced by HREF/PRN as 'base,target'. Returns the " + "content removed going from the base version to the target version (present in " + "base but not in target)." + ), + ) + super().__init__(*args, **kwargs) + + def diff(self, base_version, target_version): + return target_version.removed(base_version=base_version) + + class RepositoryVersionFilter(RepoVersionHrefPrnFilter): """ Filter by RepositoryVersion href/prn. diff --git a/pulpcore/tests/functional/api/using_plugin/test_repo_versions.py b/pulpcore/tests/functional/api/using_plugin/test_repo_versions.py index 0ad7d065380..1caeffc1fba 100644 --- a/pulpcore/tests/functional/api/using_plugin/test_repo_versions.py +++ b/pulpcore/tests/functional/api/using_plugin/test_repo_versions.py @@ -188,6 +188,98 @@ def test_add_remove_content( assert latest_version.content_summary.removed == {} +@pytest.fixture +def file_repo_three_versions( + file_bindings, + file_repository_factory, + file_9_contents, + monitor_task, +): + """Build a repo with three versions whose diffs span more than one step. + + Using the content units "A" through "E", the versions end up as:: + + v1: add A, B, C -> present {A, B, C} + v2: add D, remove A -> present {B, C, D} + v3: add E, remove B -> present {C, D, E} + + So a diff between the non-adjacent versions v1 and v3 differs from the single-step diff at v3. + """ + repo = file_repository_factory() + contents = file_9_contents + + def modify(add=(), remove=()): + body = { + "add_content_units": [contents[name].pulp_href for name in add], + "remove_content_units": [contents[name].pulp_href for name in remove], + } + monitor_task(file_bindings.RepositoriesFileApi.modify(repo.pulp_href, body).task) + return file_bindings.RepositoriesFileApi.read(repo.pulp_href).latest_version_href + + v1 = modify(add=["A", "B", "C"]) + v2 = modify(add=["D"], remove=["A"]) + v3 = modify(add=["E"], remove=["B"]) + return repo, v1, v2, v3 + + +def _relative_paths(list_response): + return {content.relative_path for content in list_response.results} + + +@pytest.mark.parallel +def test_added_between_filter(file_bindings, file_repo_three_versions): + """``added_between`` returns the net content added between two arbitrary versions.""" + _, v1, v2, v3 = file_repo_three_versions + + # Single-step diff against the immediate predecessor (v2 -> v3) for reference. + single_step = file_bindings.ContentFilesApi.list(repository_version_added=v3) + assert _relative_paths(single_step) == {"E"} + + # added_between=base,target => content in target (v3) but not in base (v1). + # {C, D, E} - {A, B, C} => {D, E}. + net = file_bindings.ContentFilesApi.list(added_between=[v1, v3]) + assert _relative_paths(net) == {"D", "E"} + + +@pytest.mark.parallel +def test_removed_between_filter(file_bindings, file_repo_three_versions): + """``removed_between`` returns the net content removed between two arbitrary versions.""" + _, v1, v2, v3 = file_repo_three_versions + + # Single-step diff against the immediate predecessor (v2 -> v3) for reference. + single_step = file_bindings.ContentFilesApi.list(repository_version_removed=v3) + assert _relative_paths(single_step) == {"B"} + + # removed_between=base,target => content in base (v1) but not in target (v3). + # {A, B, C} - {C, D, E} => {A, B}. + net = file_bindings.ContentFilesApi.list(removed_between=[v1, v3]) + assert _relative_paths(net) == {"A", "B"} + + +@pytest.mark.parallel +def test_added_removed_between_are_symmetric(file_bindings, file_repo_three_versions): + """``added_between=a,b`` yields the same set as ``removed_between=b,a`` (order reversed).""" + _, v1, v2, v3 = file_repo_three_versions + + added = file_bindings.ContentFilesApi.list(added_between=[v1, v3]) + removed_reversed = file_bindings.ContentFilesApi.list(removed_between=[v3, v1]) + assert _relative_paths(added) == _relative_paths(removed_reversed) == {"D", "E"} + + +@pytest.mark.parallel +def test_between_filter_requires_two_versions(file_bindings, file_repo_three_versions): + """``added_between`` / ``removed_between`` reject anything other than exactly two versions.""" + _, v1, v2, v3 = file_repo_three_versions + + with pytest.raises(file_bindings.ApiException) as ctx: + file_bindings.ContentFilesApi.list(added_between=[v1]) + assert ctx.value.status == 400 + + with pytest.raises(file_bindings.ApiException) as ctx: + file_bindings.ContentFilesApi.list(removed_between=[v1, v2, v3]) + assert ctx.value.status == 400 + + @pytest.mark.parallel def test_add_remove_repo_version( file_bindings,