Fix recursive add/remove M2M queries avoiding values_list()

CHANGES/460.bugfix

@@ -0,0 +1 @@
+Fix recursive add/remove to resolve many-to-many relations via through models instead of ``values_list()``.

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))
     )
 

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)

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)