Fix constraint violation in is_latest

CHANGES/1571.bugfix

@@ -0,0 +1 @@
+Prevented a race condition that lead to contraint violations when calculating the highest version of a collection.

pulp_ansible/app/migrations/0055_alter_collectionversion_version_alter_role_version.py

@@ -0,0 +1,27 @@
+# Generated by Django 4.2.4 on 2023-09-15 09:23
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("ansible", "0054_split_collection_version_numbers"),
+    ]
+
+    operations = [
+        migrations.RunSQL(
+            # Custom roles are only allowed with postgreSQL 16.
+            sql="CREATE COLLATION pulp_ansible_semver (provider = 'icu', locale = 'en-US-u-kn-true')",
+            reverse_sql="DROP COLLATION pulp_ansible_semver",
+        ),
+        migrations.AlterField(
+            model_name="collectionversion",
+            name="version",
+            field=models.CharField(db_collation="pulp_ansible_semver", max_length=128),
+        ),
+        migrations.AlterField(
+            model_name="role",
+            name="version",
+            field=models.CharField(db_collation="pulp_ansible_semver", max_length=128),
+        ),
+    ]

pulp_ansible/app/models.py

@@ -48,7 +48,7 @@ class Role(Content):
 
     namespace = models.CharField(max_length=64)
     name = models.CharField(max_length=64)
-    version = models.CharField(max_length=128)
+    version = models.CharField(max_length=128, db_collation="pulp_ansible_semver")
 
     @property
     def relative_path(self):
@@ -179,7 +179,7 @@ class CollectionVersion(Content):
     repository = models.CharField(default="", blank=True, max_length=2000, editable=False)
     requires_ansible = models.CharField(null=True, max_length=255)
 
-    version = models.CharField(max_length=128, editable=False)
+    version = models.CharField(max_length=128, db_collation="pulp_ansible_semver")
     version_major = models.IntegerField()
     version_minor = models.IntegerField()
     version_patch = models.IntegerField()

pulp_ansible/app/tasks/collections.py

@@ -15,7 +15,7 @@
 from async_lru import alru_cache
 from django.conf import settings
 from django.db import transaction
-from django.db.models import Q
+from django.db.models import F, Q
 from django.db.utils import IntegrityError
 from django.urls import reverse
 from django.utils.dateparse import parse_datetime
@@ -344,9 +344,8 @@ def create_collection_from_importer(importer_result, metadata_only=False):
             tag, created = Tag.objects.get_or_create(name=name)
             collection_version.tags.add(tag)
 
-        _update_highest_version(collection_version)
-
         collection_version.save()  # Save the FK updates
+        _update_highest_version(collection_version)
     return collection_version
 
 
@@ -421,60 +420,20 @@ def _get_backend_storage_url(artifact_file):
     return url
 
 
-def _update_highest_version(collection_version, save=False):
+def _update_highest_version(collection_version):
     """
     Checks if this version is greater than the most highest one.
 
-    If this version is the first version in collection, is_highest is set to True.
-    If this version is greater than the highest version in collection, set is_highest
-    equals False on the last highest version and True on this version.
-    Otherwise does nothing. The collection version is updated to the database if `save=True`,
-    otherwise only the `is_highest` field is updated on the instance. This is an optimization
-    to prevent double saving when this method is called during syncs and uploads.
+    TODO: This function violates content immutability. It must be deprecated.
     """
 
-    def is_new_highest(new, old):
-        if bool(new.prerelease) == bool(old.prerelease):
-            return new > old
-        return bool(old.prerelease)
-
-    # did we have one set previously for this collection?
-    last_highest = collection_version.collection.versions.filter(is_highest=True).first()
-
-    if not last_highest:
-        # we only have one version, so mark it as the highest
-        if collection_version.collection.versions.count() == 1:
-            collection_version.is_highest = True
-            if save:
-                collection_version.save(update_fields=["is_highest"])
-            return
-
-        # previous highest must have been removed, re-compute highest from the whole list ...
-        highest = None
-        for cv in collection_version.collection.versions.all():
-            sv = Version(cv.version)
-            if highest is None or is_new_highest(sv, highest[0]):
-                highest = (sv, cv)
-
-        highest[1].is_highest = True
-        if highest[1] != collection_version or save:
-            highest[1].save()
-        return
-
-    # exit if the new CV is not higher
-    if not is_new_highest(Version(collection_version.version), Version(last_highest.version)):
-        # ensure that this collection_version doesn't have is_highest improperly set
-        if collection_version.is_highest:
-            collection_version.is_highest = False
-            if save:
-                collection_version.save(update_fields=["is_highest"])
-        return
-
-    last_highest.is_highest = False
-    last_highest.save(update_fields=["is_highest"])
-    collection_version.is_highest = True
-    if save:
-        collection_version.save(update_fields=["is_highest"])
+    qs = collection_version.collection.versions.annotate(prerelease=Q(version_prerelease__ne=""))
+    highest_subq = qs.order_by("prerelease", "-version")[0:1].values("pk")
+    # Order them in such a way, that only the latest updated record will recieve true.
+    # This should prevent hitting the uniquenes constraint.
+    qs.annotate(new_is_highest=Q(pk=highest_subq)).order_by("-prerelease", "version").update(
+        is_highest=F("new_is_highest")
+    )
 
 
 class AnsibleDeclarativeVersion(DeclarativeVersion):
@@ -1244,6 +1203,5 @@ def _post_save(self, batch):
                         continue
                     setattr(collection_version, attr_name, attr_value)
 
-                _update_highest_version(collection_version)
-
                 collection_version.save()
+                _update_highest_version(collection_version)

pulp_ansible/tests/unit/test_tasks.py

@@ -114,7 +114,7 @@ def test_is_highest_set(self):
 
         collection_versions = build_cvs_from_specs(specs, build_artifacts=False)
         for cv in collection_versions:
-            _update_highest_version(cv, save=True)
+            _update_highest_version(cv)
 
         assert (
             CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1")
@@ -133,7 +133,7 @@ def test_is_highest_set_on_multple_stable_releases(self):
 
         collection_versions = build_cvs_from_specs(specs, build_artifacts=False)
         for cv in collection_versions:
-            _update_highest_version(cv, save=True)
+            _update_highest_version(cv)
 
         assert (
             CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1")
@@ -158,7 +158,7 @@ def test_is_highest_set_on_single_prerelease(self):
 
         collection_versions = build_cvs_from_specs(specs, build_artifacts=False)
         for cv in collection_versions:
-            _update_highest_version(cv, save=True)
+            _update_highest_version(cv)
 
         assert (
             CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1-rc2")
@@ -178,7 +178,7 @@ def test_is_highest_set_on_multiple_prereleases(self):
 
         collection_versions = build_cvs_from_specs(specs, build_artifacts=False)
         for cv in collection_versions:
-            _update_highest_version(cv, save=True)
+            _update_highest_version(cv)
 
         assert (
             CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1-rc1")
@@ -211,7 +211,7 @@ def test_is_highest_set_on_multiple_prereleases_one_pass(self):
         ]
 
         collection_versions = build_cvs_from_specs(specs, build_artifacts=False)
-        _update_highest_version(collection_versions[1], save=True)
+        _update_highest_version(collection_versions[1])
 
         assert (
             CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1-rc1")
@@ -246,7 +246,7 @@ def test_is_highest_set_on_multiple_prereleases_backwards_order(self):
 
         collection_versions = build_cvs_from_specs(specs, build_artifacts=False)
         for cv in collection_versions[::-1]:
-            _update_highest_version(cv, save=True)
+            _update_highest_version(cv)
 
         assert (
             CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1-rc1")
@@ -286,7 +286,7 @@ def test_prerelease_not_higher_than_stable(self):
 
         collection_versions = build_cvs_from_specs(specs, build_artifacts=False)
         for cv in collection_versions:
-            _update_highest_version(cv, save=True)
+            _update_highest_version(cv)
 
         assert (
             CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1")
@@ -312,7 +312,7 @@ def test_prerelease_matches_stable(self):
 
         collection_versions = build_cvs_from_specs(specs, build_artifacts=False)
         for cv in collection_versions:
-            _update_highest_version(cv, save=True)
+            _update_highest_version(cv)
 
         assert (
             CollectionVersion.objects.filter(namespace=namespace, name=name, version="2.0.0-rc1")