diff --git a/CHANGES/1571.bugfix b/CHANGES/1571.bugfix new file mode 100644 index 000000000..5277ad326 --- /dev/null +++ b/CHANGES/1571.bugfix @@ -0,0 +1 @@ +Prevented a race condition that lead to contraint violations when calculating the highest version of a collection. diff --git a/pulp_ansible/app/tasks/collections.py b/pulp_ansible/app/tasks/collections.py index a7ac663c2..f648efa6a 100644 --- a/pulp_ansible/app/tasks/collections.py +++ b/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,53 +420,22 @@ 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. + 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 - - # 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)): - 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_major", "-version_minor", "-version_patch", "-version_prerelease" + )[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_major", "version_minor", "version_patch", "version_prerelease" + ).update(is_highest=F("new_is_highest")) class AnsibleDeclarativeVersion(DeclarativeVersion): @@ -1209,6 +1177,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) diff --git a/pulp_ansible/tests/unit/test_tasks.py b/pulp_ansible/tests/unit/test_tasks.py index 6b3f68528..7a9d608f1 100644 --- a/pulp_ansible/tests/unit/test_tasks.py +++ b/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")