diff --git a/CHANGES/460.bugfix b/CHANGES/460.bugfix new file mode 100644 index 000000000..be0d780e0 --- /dev/null +++ b/CHANGES/460.bugfix @@ -0,0 +1 @@ +Fix recursive add/remove to resolve many-to-many relations via through models instead of ``values_list()``. diff --git a/pulp_container/app/tasks/recursive_add.py b/pulp_container/app/tasks/recursive_add.py index da0254bf7..c327a5ccd 100644 --- a/pulp_container/app/tasks/recursive_add.py +++ b/pulp_container/app/tasks/recursive_add.py @@ -1,8 +1,10 @@ from pulp_container.app.models import ( MEDIA_TYPE, Blob, + BlobManifest, ContainerRepository, Manifest, + ManifestListManifest, Tag, ) @@ -33,6 +35,11 @@ def recursive_add_content(repository_pk, content_units): media_type__in=[MEDIA_TYPE.MANIFEST_LIST, MEDIA_TYPE.INDEX_OCI], ) + # Avoid values_list() on ManyToManyField; query the through model instead. + listed_manifest_pks = ManifestListManifest.objects.filter( + image_manifest__in=manifest_lists_to_add + ).values_list("manifest_list", flat=True) + manifests_to_add = ( Manifest.objects.filter( pk__in=content_units, @@ -43,9 +50,7 @@ def recursive_add_content(repository_pk, content_units): MEDIA_TYPE.MANIFEST_OCI, ], ) - | Manifest.objects.filter( - pk__in=manifest_lists_to_add.values_list("listed_manifests", flat=True) - ) + | Manifest.objects.filter(pk__in=listed_manifest_pks) | Manifest.objects.filter( pk__in=tags_to_add.values_list("tagged_manifest", flat=True), media_type__in=[ @@ -57,9 +62,12 @@ def recursive_add_content(repository_pk, content_units): ) ) + listed_blob_pks = BlobManifest.objects.filter(manifest__in=manifests_to_add).values_list( + "manifest_blob", flat=True + ) blobs_to_add = ( Blob.objects.filter(pk__in=content_units) - | Blob.objects.filter(pk__in=manifests_to_add.values_list("blobs", flat=True)) + | Blob.objects.filter(pk__in=listed_blob_pks) | Blob.objects.filter(pk__in=manifests_to_add.values_list("config_blob", flat=True)) ) diff --git a/pulp_container/app/tasks/recursive_remove.py b/pulp_container/app/tasks/recursive_remove.py index 72835e227..36a92251f 100644 --- a/pulp_container/app/tasks/recursive_remove.py +++ b/pulp_container/app/tasks/recursive_remove.py @@ -5,7 +5,9 @@ from pulp_container.app.models import ( MEDIA_TYPE, Blob, + BlobManifest, Manifest, + ManifestListManifest, ManifestSignature, Tag, ) @@ -79,15 +81,20 @@ def recursive_remove_content(repository_pk, content_units): manifests_in_repo & type_manifest_list ).exclude(pk__in=manifest_lists_to_remove) + # Avoid values_list() on ManyToManyField; query the through model instead. listed_manifests_must_remain = Q( - pk__in=manifest_lists_to_remain.values_list("listed_manifests", flat=True) + pk__in=ManifestListManifest.objects.filter( + image_manifest__in=manifest_lists_to_remain + ).values_list("manifest_list", flat=True) ) manifests_must_remain = Manifest.objects.filter( tagged_manifests_must_remain | listed_manifests_must_remain ).filter(type_manifest & manifests_in_repo) listed_manifests_to_remove = Q( - pk__in=manifest_lists_to_remove.values_list("listed_manifests", flat=True) + pk__in=ManifestListManifest.objects.filter( + image_manifest__in=manifest_lists_to_remove + ).values_list("manifest_list", flat=True) ) manifests_to_remove = ( Manifest.objects.filter( @@ -102,11 +109,15 @@ def recursive_remove_content(repository_pk, content_units): ) listed_blobs_must_remain = Q( - pk__in=manifests_to_remain.values_list("blobs", flat=True) + pk__in=BlobManifest.objects.filter(manifest__in=manifests_to_remain).values_list( + "manifest_blob", flat=True + ) ) | Q(pk__in=manifests_to_remain.values_list("config_blob", flat=True)) - listed_blobs_to_remove = Q(pk__in=manifests_to_remove.values_list("blobs", flat=True)) | Q( - pk__in=manifests_to_remove.values_list("config_blob", flat=True) - ) + listed_blobs_to_remove = Q( + pk__in=BlobManifest.objects.filter(manifest__in=manifests_to_remove).values_list( + "manifest_blob", flat=True + ) + ) | Q(pk__in=manifests_to_remove.values_list("config_blob", flat=True)) blobs_to_remove = ( Blob.objects.filter(user_provided_content | listed_blobs_to_remove) diff --git a/pulp_container/tests/unit/test_recursive_content.py b/pulp_container/tests/unit/test_recursive_content.py new file mode 100644 index 000000000..bf20018a1 --- /dev/null +++ b/pulp_container/tests/unit/test_recursive_content.py @@ -0,0 +1,97 @@ +from django.test import TestCase + +from pulp_container.app.models import ( + MEDIA_TYPE, + Blob, + BlobManifest, + ContainerRepository, + Manifest, + ManifestListManifest, +) +from pulp_container.app.tasks.recursive_add import recursive_add_content +from pulp_container.app.tasks.recursive_remove import recursive_remove_content + + +def _make_blob(digest): + return Blob.objects.create(digest=digest) + + +def _make_manifest(digest, media_type=MEDIA_TYPE.MANIFEST_V2, config_blob=None): + return Manifest.objects.create( + digest=digest, + schema_version=2, + media_type=media_type, + config_blob=config_blob, + data="{}", + ) + + +class TestRecursiveM2MQueries(TestCase): + """Ensure recursive add/remove resolve M2M relations without values_list on M2M fields.""" + + def setUp(self): + self.repo = ContainerRepository.objects.create(name="recursive-m2m-test") + + # Two manifest lists, each with a distinct listed manifest and blob set. + # This is the scenario where values_list("listed_manifests"/"blobs") can miss rows. + self.config_a = _make_blob("sha256:" + "a" * 64) + self.config_b = _make_blob("sha256:" + "b" * 64) + self.blob_a1 = _make_blob("sha256:" + "c" * 64) + self.blob_a2 = _make_blob("sha256:" + "d" * 64) + self.blob_b1 = _make_blob("sha256:" + "e" * 64) + + self.manifest_a = _make_manifest("sha256:" + "1" * 64, config_blob=self.config_a) + self.manifest_b = _make_manifest("sha256:" + "2" * 64, config_blob=self.config_b) + BlobManifest.objects.create(manifest=self.manifest_a, manifest_blob=self.blob_a1) + BlobManifest.objects.create(manifest=self.manifest_a, manifest_blob=self.blob_a2) + BlobManifest.objects.create(manifest=self.manifest_b, manifest_blob=self.blob_b1) + + self.list_a = _make_manifest("sha256:" + "3" * 64, media_type=MEDIA_TYPE.MANIFEST_LIST) + self.list_b = _make_manifest("sha256:" + "4" * 64, media_type=MEDIA_TYPE.MANIFEST_LIST) + ManifestListManifest.objects.create( + image_manifest=self.list_a, manifest_list=self.manifest_a + ) + ManifestListManifest.objects.create( + image_manifest=self.list_b, manifest_list=self.manifest_b + ) + + def test_recursive_add_includes_all_listed_manifests_and_blobs(self): + recursive_add_content(self.repo.pk, [self.list_a.pk, self.list_b.pk]) + + content_pks = set(self.repo.latest_version().content.values_list("pk", flat=True)) + expected = { + self.list_a.pk, + self.list_b.pk, + self.manifest_a.pk, + self.manifest_b.pk, + self.config_a.pk, + self.config_b.pk, + self.blob_a1.pk, + self.blob_a2.pk, + self.blob_b1.pk, + } + self.assertTrue(expected.issubset(content_pks), content_pks) + + def test_recursive_remove_drops_unshared_listed_content(self): + # Seed the repository with both lists and their related content. + recursive_add_content(self.repo.pk, [self.list_a.pk, self.list_b.pk]) + + # Remove only list_b; list_a's tree must remain, list_b's tree must go. + recursive_remove_content(self.repo.pk, [self.list_b.pk]) + + content_pks = set(self.repo.latest_version().content.values_list("pk", flat=True)) + remaining = { + self.list_a.pk, + self.manifest_a.pk, + self.config_a.pk, + self.blob_a1.pk, + self.blob_a2.pk, + } + removed = { + self.list_b.pk, + self.manifest_b.pk, + self.config_b.pk, + self.blob_b1.pk, + } + self.assertTrue(remaining.issubset(content_pks), content_pks) + self.assertTrue(removed.isdisjoint(content_pks), content_pks)