Harden RepositoryVersion against memoization issues

CHANGES/7272.bugfix

@@ -0,0 +1 @@
+Harden RepositoryVersion memoization against bugs that could lead to incorrect counts and repository content.

pulpcore/app/management/commands/datarepair-2327.py → pulpcore/app/management/commands/datarepair.py

@@ -2,36 +2,87 @@
 
 from django.db import connection
 from django.conf import settings
-from django.core.management import BaseCommand
+from django.core.management import BaseCommand, CommandError
 from django.db.models import Q
 from django.utils.encoding import force_bytes, force_str
 
 import cryptography
 
-from pulpcore.app.models import Remote
+from pulpcore.app import models
 
 
 class Command(BaseCommand):
     """
-    Django management command for repairing incorrectly migrated remote data.
+    Django management command for repairing improper data mistakes and migrations.
     """
 
-    help = (
-        "Repairs issue #2327. A small number of configuration settings may have been "
-        "corrupted during an upgrade from a previous version of Pulp to a Pulp version "
-        "between 3.15-3.18, resulting in trouble when syncing or viewing certain remotes. "
-        "This script repairs the data (which was not lost)."
-    )
+    help = _("Repairs various data corruption issues known to Pulp.")
 
     def add_arguments(self, parser):
         """Set up arguments."""
+        parser.add_argument("issue", help=_("The github issue # of the issue to be fixed."))
         parser.add_argument(
             "--dry-run",
             action="store_true",
-            help=_("Don't modify anything, just collect results on how many Remotes are impacted."),
+            help=_(
+                "Don't modify anything, just show the results of what would happen if this "
+                "command were run."
+            ),
         )
 
     def handle(self, *args, **options):
+        """Implement the command."""
+        issue = options["issue"]
+
+        if issue == "2327":
+            self.repair_2327(options)
+        elif issue == "7272":
+            self.repair_7272(options)
+        else:
+            raise CommandError(_("Unknown issue: '{}'").format(issue))
+
+    def repair_7272(self, options):
+        dry_run = options["dry_run"]
+
+        print()
+
+        for repo in models.Repository.objects.all():
+            for rv in models.RepositoryVersion.objects.filter(repository=repo):
+                if rv.content_ids is not None:
+                    cached_id_set = set(rv.content_ids)
+                    repositorycontent_id_set = set(
+                        rv._content_relationships().values_list("content__pk", flat=True)
+                    )
+                    if cached_id_set != repositorycontent_id_set:
+                        print(
+                            f'Repository "{repo.name}" ("{repo.pulp_type}") version {rv.number} '
+                            "has a mismatch between the RepositoryContent and the cached ID set"
+                        )
+
+                        if not dry_run:
+                            rv.content_ids = list(
+                                rv._content_relationships().values_list("content__pk", flat=True)
+                            )
+                            rv.save()
+                            rv._compute_counts()
+
+                repositorycontent_id_count = rv._content_relationships().count()
+                if repositorycontent_id_count == 0:
+                    continue
+                rv_count_details = models.RepositoryVersionContentDetails.objects.get(
+                    repository_version=rv, count_type=models.RepositoryVersionContentDetails.PRESENT
+                )
+
+                if rv_count_details.count != repositorycontent_id_count:
+                    print(
+                        f'Repository "{repo.name}" ("{repo.pulp_type}") version {rv.number} has a '
+                        "mismatch between the RepositoryContent and the "
+                        "RepositoryVersionContentDetails"
+                    )
+
+            print()
+
+    def repair_2327(self, options):
         dry_run = options["dry_run"]
         fields = ("username", "password", "proxy_username", "proxy_password", "client_key")
 
@@ -49,11 +100,11 @@ def handle(self, *args, **options):
         number_unencrypted = 0
         number_multi_encrypted = 0
 
-        for remote_pk in Remote.objects.filter(possibly_affected_remotes).values_list(
+        for remote_pk in models.Remote.objects.filter(possibly_affected_remotes).values_list(
             "pk", flat=True
         ):
             try:
-                remote = Remote.objects.get(pk=remote_pk)
+                remote = models.Remote.objects.get(pk=remote_pk)
                 # if we can get the remote successfully, it is either OK or the fields are
                 # encrypted more than once
             except cryptography.fernet.InvalidToken:
@@ -73,7 +124,7 @@ def handle(self, *args, **options):
                     field_values[field] = value
 
                 if not dry_run:
-                    Remote.objects.filter(pk=remote_pk).update(**field_values)
+                    models.Remote.objects.filter(pk=remote_pk).update(**field_values)
                 number_unencrypted += 1
             else:
                 times_decrypted = 0

pulpcore/app/models/repository.py

@@ -1068,6 +1068,7 @@ def add_content(self, content):
                 complete RepositoryVersion
         """
 
+        assert issubclass(content.model, Content)
         if self.complete:
             raise ResourceImmutableError(self)
 
@@ -1076,6 +1077,7 @@ def add_content(self, content):
             .exclude(pulp_domain_id=get_domain_pk())
             .exists()
         )
+
         repo_content = []
         to_add = set(content.values_list("pk", flat=True)) - set(self._get_content_ids())
         with transaction.atomic():
@@ -1114,12 +1116,14 @@ def remove_content(self, content):
                 complete RepositoryVersion
         """
 
+        assert issubclass(content.model, Content)
         if self.complete:
             raise ResourceImmutableError(self)
 
         if not content or not content.count():
             return
 
+        # check that all content is within the current domain
         assert (
             not Content.objects.filter(pk__in=content)
             .exclude(pulp_domain_id=get_domain_pk())
@@ -1157,6 +1161,7 @@ def set_content(self, content):
             pulpcore.exception.ResourceImmutableError: if set_content is called on a
                 complete RepositoryVersion
         """
+        assert issubclass(content.model, Content)
         self.remove_content(self.content.exclude(pk__in=content))
         self.add_content(content.exclude(pk__in=self.content))
 
@@ -1306,6 +1311,9 @@ def _compute_counts(self):
         objects and makes new ones with each call.
         """
         with transaction.atomic():
+            # relatively inexpensive sanity check for memoization
+            assert len(self.content_ids) == self._content_relationships().count()
+            # delete existing content details and recompute them all
             RepositoryVersionContentDetails.objects.filter(repository_version=self).delete()
             counts_list = []
             for value, name in RepositoryVersionContentDetails.COUNT_TYPE_CHOICES: