diff --git a/CHANGES/1175.bugfix b/CHANGES/1175.bugfix new file mode 100644 index 000000000..20055fc36 --- /dev/null +++ b/CHANGES/1175.bugfix @@ -0,0 +1 @@ +Properly return 400 error when trying to upload a duplicate Collection. diff --git a/pulp_ansible/app/galaxy/serializers.py b/pulp_ansible/app/galaxy/serializers.py index 54687c1fe..f0b5c3720 100644 --- a/pulp_ansible/app/galaxy/serializers.py +++ b/pulp_ansible/app/galaxy/serializers.py @@ -6,6 +6,7 @@ from rest_framework.reverse import reverse from rest_framework import serializers +from pulpcore.plugin.models import Artifact from pulp_ansible.app.models import Collection, CollectionVersion, Role from pulp_ansible.app.galaxy.v3.serializers import CollectionMetadataSerializer @@ -187,3 +188,10 @@ class GalaxyCollectionUploadSerializer(serializers.Serializer): file = serializers.FileField( help_text=_("The file containing the Artifact binary data."), required=True ) + + def validate(self, data): + """Ensure duplicate artifact isn't uploaded.""" + sha256 = data["file"].hashers["sha256"].hexdigest() + artifact = Artifact.objects.filter(sha256=sha256).first() + if artifact: + raise serializers.ValidationError(_("Artifact already exists")) diff --git a/pulp_ansible/app/serializers.py b/pulp_ansible/app/serializers.py index 026b7a369..584f0040b 100644 --- a/pulp_ansible/app/serializers.py +++ b/pulp_ansible/app/serializers.py @@ -285,6 +285,15 @@ class CollectionOneShotSerializer(serializers.Serializer): default=None, ) + def validate(self, data): + """Ensure duplicate artifact isn't uploaded.""" + data = super().validate(data) + sha256 = data["file"].hashers["sha256"].hexdigest() + artifact = Artifact.objects.filter(sha256=sha256).first() + if artifact: + raise ValidationError(_("Artifact already exists")) + return data + class AnsibleDistributionSerializer(DistributionSerializer): """ @@ -383,7 +392,7 @@ def validate(self, data): sha256 = data["file"].hashers["sha256"].hexdigest() artifact = Artifact.objects.filter(sha256=sha256).first() if artifact: - ValidationError(_("Artifact already exists")) + raise ValidationError(_("Artifact already exists")) temp_file = PulpTemporaryFile.init_and_validate(data.pop("file")) temp_file.save() data["temp_file_pk"] = str(temp_file.pk) diff --git a/pulp_ansible/tests/functional/api/collection/v2/test_upload.py b/pulp_ansible/tests/functional/api/collection/v2/test_upload.py index 7ab39a223..ce1d8e260 100644 --- a/pulp_ansible/tests/functional/api/collection/v2/test_upload.py +++ b/pulp_ansible/tests/functional/api/collection/v2/test_upload.py @@ -2,10 +2,14 @@ import hashlib from tempfile import NamedTemporaryFile -from pulp_smash.pulp3.bindings import delete_orphans, monitor_task, PulpTestCase, PulpTaskError +from pulp_smash.pulp3.bindings import delete_orphans, monitor_task, PulpTestCase from pulp_smash.utils import http_get -from pulpcore.client.pulp_ansible import AnsibleCollectionsApi, ContentCollectionVersionsApi +from pulpcore.client.pulp_ansible import ( + AnsibleCollectionsApi, + ContentCollectionVersionsApi, + ApiException, +) from pulp_ansible.tests.functional.utils import gen_ansible_client from pulp_ansible.tests.functional.utils import set_up_module as setUpModule # noqa:F401 @@ -51,13 +55,10 @@ def test_collection_upload(self): self.assertEqual(response.sha256, self.collection_sha256, response) - with self.assertRaises(PulpTaskError) as exc: + with self.assertRaises(ApiException) as exc: self.upload_collection() - task_result = exc.exception.task.to_dict() - self.assertEqual(task_result["state"], "failed") - error = task_result["error"] - for key in ("artifact", "already", "exists"): - self.assertIn(key, task_result["error"]["description"].lower(), error) + assert exc.exception.status == 400 + assert "Artifact already exists" in exc.exception.reason delete_orphans() diff --git a/pulp_ansible/tests/functional/api/collection/v3/test_collection.py b/pulp_ansible/tests/functional/api/collection/v3/test_collection.py index 1ba00b77d..3ba7bc64e 100644 --- a/pulp_ansible/tests/functional/api/collection/v3/test_collection.py +++ b/pulp_ansible/tests/functional/api/collection/v3/test_collection.py @@ -20,7 +20,6 @@ ANSIBLE_COLLECTION_UPLOAD_FIXTURE_URL, ANSIBLE_DISTRIBUTION_PATH, ANSIBLE_REPO_PATH, - COLLECTION_METADATA, ) from pulp_ansible.tests.functional.utils import set_up_module as setUpModule # noqa:F401 @@ -87,15 +86,19 @@ def get_metadata_published(pulp_client, pulp_dist): return datetime.strptime(metadata["published"], "%Y-%m-%dT%H:%M:%S.%fZ") +def upload_collection(client, filename, base_path): + """Helper to upload collections to pulp_ansible/galaxy.""" + UPLOAD_PATH = get_galaxy_url(base_path, "/v3/artifacts/collections/") + collection = {"file": (open(filename, "rb"))} + + return client.using_handler(upload_handler).post(UPLOAD_PATH, files=collection) + + @pytest.fixture(scope="session") def collection_upload(pulp_client, artifact, pulp_dist): """Publish a new collection and return the processed response data.""" - UPLOAD_PATH = get_galaxy_url(pulp_dist["base_path"], "/v3/artifacts/collections/") published_before_upload = get_metadata_published(pulp_client, pulp_dist) - logging.info(f"Uploading collection to '{UPLOAD_PATH}'...") - collection = {"file": (ANSIBLE_COLLECTION_FILE_NAME, open(artifact.filename, "rb"))} - - response = pulp_client.using_handler(upload_handler).post(UPLOAD_PATH, files=collection) + response = upload_collection(pulp_client, artifact.filename, pulp_dist["base_path"]) published_after_upload = get_metadata_published(pulp_client, pulp_dist) assert published_after_upload > published_before_upload return response @@ -104,12 +107,8 @@ def collection_upload(pulp_client, artifact, pulp_dist): @pytest.fixture(scope="session") def collection_upload2(pulp_client, artifact2, pulp_dist): """Publish the second new collection and return the processed response data.""" - UPLOAD_PATH = get_galaxy_url(pulp_dist["base_path"], "/v3/artifacts/collections/") published_before_upload = get_metadata_published(pulp_client, pulp_dist) - logging.info(f"Uploading collection to '{UPLOAD_PATH}'...") - collection = {"file": (open(artifact2.filename, "rb"))} - - response = pulp_client.using_handler(upload_handler).post(UPLOAD_PATH, files=collection) + response = upload_collection(pulp_client, artifact2.filename, pulp_dist["base_path"]) published_after_upload = get_metadata_published(pulp_client, pulp_dist) assert published_after_upload > published_before_upload return response @@ -196,9 +195,10 @@ def test_collection_upload(collection_upload): assert "finished_at" in collection_upload assert "messages" in collection_upload - for key, value in collection_upload.items(): - if key in COLLECTION_METADATA.keys(): - assert COLLECTION_METADATA[key] == value, collection_upload + # TODO: Add this back when namespace, name, and version are apart of the CollectionImport + # for key, value in collection_upload.items(): + # if key in COLLECTION_METADATA.keys(): + # assert COLLECTION_METADATA[key] == value, collection_upload def test_collection_list( @@ -266,7 +266,7 @@ def test_collection_version(artifact, pulp_client, collection_detail): assert version["artifact"]["sha256"] == hashlib.sha256(tarball).hexdigest() assert version["artifact"]["size"] == len(tarball) - # assert version['artifact']['filename'] == artifact.filename + assert version["artifact"]["filename"] == artifact.filename.strip("/tmp/") assert "updated_at" in version assert "created_at" in version @@ -309,27 +309,17 @@ def test_collection_download(artifact, pulp_client, collection_detail): assert f.content == tarball -def test_collection_upload_repeat(pulp_client, known_collection, pulp_dist): +def test_collection_upload_repeat(pulp_client, artifact, pulp_dist, collection_upload): """Upload a duplicate collection. Should fail, because of the conflict of collection name and version. """ - cfg = config.get_config() - url = urljoin(cfg.get_base_url(), f"api/{pulp_dist['base_path']}/v3/artifacts/collections/") - with pytest.raises(HTTPError) as ctx: - response = pulp_client.post(url, files=known_collection) - - assert ctx.exception.response.json()["errors"][0] == { - "status": "400", - "code": "invalid", - "title": "Invalid input.", - "detail": "Artifact already exists.", - } + upload_collection(pulp_client, artifact.filename, pulp_dist["base_path"]) - for key, value in collection_upload.items(): - if key in COLLECTION_METADATA.keys(): - assert COLLECTION_METADATA[key] == value, response - - collection_sha256 = hashlib.sha256(known_collection["files"][1]).hexdigest() - assert response["sha256"] == collection_sha256, response + assert ctx.value.response.json()["errors"][0] == { + "status": "400", + "code": "invalid", + "title": "Invalid input.", + "detail": "Artifact already exists", + }