Fix duplicate collection upload error msg & tests

fixes: #1175

CHANGES/1175.bugfix

@@ -0,0 +1 @@
+Properly return 400 error when trying to upload a duplicate Collection.

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

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)

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.body
 
         delete_orphans()

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
 
@@ -59,7 +58,7 @@ def get_galaxy_url(base, path):
 
 
 @pytest.fixture(scope="session")
-def artifact():
+def collection_artifact():
     """Generate a randomized collection for testing."""
     # build_collection will only store one collection, so copy to new location and delete later
     artifact = build_collection("skeleton")
@@ -69,7 +68,7 @@ def artifact():
 
 
 @pytest.fixture(scope="session")
-def artifact2():
+def collection_artifact2():
     """
     Generate a second randomized collection for testing.
 
@@ -87,39 +86,40 @@ 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):
+def collection_upload(pulp_client, collection_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, collection_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
 
 
 @pytest.fixture(scope="session")
-def collection_upload2(pulp_client, artifact2, pulp_dist):
+def collection_upload2(pulp_client, collection_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, collection_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
 
 
 @pytest.fixture(scope="session")
-def collection_detail(collection_upload, pulp_client, pulp_dist, artifact):
+def collection_detail(collection_upload, pulp_client, pulp_dist, collection_artifact):
     """Fetch and parse a collection details response from an uploaded collection."""
     url = get_galaxy_url(
-        pulp_dist["base_path"], f"/v3/collections/{artifact.namespace}/{artifact.name}/"
+        pulp_dist["base_path"],
+        f"/v3/collections/{collection_artifact.namespace}/{collection_artifact.name}/",
     )
     response = pulp_client.using_handler(api.json_handler).get(url)
     return response
@@ -196,13 +196,19 @@ 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(
-    artifact, artifact2, collection_upload, collection_upload2, pulp_client, pulp_dist
+    collection_artifact,
+    collection_artifact2,
+    collection_upload,
+    collection_upload2,
+    pulp_client,
+    pulp_dist,
 ):
     """Tests the collection list endpoint after uploading both collections."""
     url = get_galaxy_url(pulp_dist["base_path"], "v3/collections/")
@@ -211,33 +217,35 @@ def test_collection_list(
     assert response["meta"]["count"] >= 2
     present_collections = {c["href"].split("collections/")[1] for c in response["data"]}
     uploaded_collections = {
-        f"index/{artifact.namespace}/{artifact.name}/",
-        f"index/{artifact2.namespace}/{artifact2.name}/",
+        f"index/{collection_artifact.namespace}/{collection_artifact.name}/",
+        f"index/{collection_artifact2.namespace}/{collection_artifact2.name}/",
     }
     assert uploaded_collections.issubset(present_collections)
 
 
-def test_collection_detail(artifact, collection_detail, pulp_dist):
+def test_collection_detail(collection_artifact, collection_detail, pulp_dist):
     """Test collection detail resulting from a successful upload of one version.
 
     Includes information of the most current version.
     """
     url = (
         f"plugin/ansible/content/{pulp_dist['base_path']}"
-        f"/collections/index/{artifact.namespace}/{artifact.name}/"
+        f"/collections/index/{collection_artifact.namespace}/{collection_artifact.name}/"
     )
 
     assert not collection_detail["deprecated"]
 
     # Check that the URL ends with the correct path so that this test doesn't fail
     # when galaxy_ng is installed
     assert collection_detail["href"].endswith(url)
-    assert collection_detail["namespace"] == artifact.namespace
-    assert collection_detail["name"] == artifact.name
+    assert collection_detail["namespace"] == collection_artifact.namespace
+    assert collection_detail["name"] == collection_artifact.name
     assert collection_detail["highest_version"]["version"] == "1.0.0"
 
 
-def test_collection_version_list(artifact, pulp_client, collection_detail, collection_upload2):
+def test_collection_version_list(
+    collection_artifact, pulp_client, collection_detail, collection_upload2
+):
     """Test the versions endpoint, listing the available versions of a given collection."""
     # Version List Endpoint
     versions = pulp_client.using_handler(api.json_handler).get(collection_detail["versions_url"])
@@ -248,7 +256,7 @@ def test_collection_version_list(artifact, pulp_client, collection_detail, colle
     assert version["href"] == collection_detail["highest_version"]["href"]
 
 
-def test_collection_version(artifact, pulp_client, collection_detail):
+def test_collection_version(collection_artifact, pulp_client, collection_detail):
     """Test collection version endpoint.
 
     Each collection version details a specific uploaded artifact for the collection.
@@ -258,15 +266,15 @@ def test_collection_version(artifact, pulp_client, collection_detail):
         collection_detail["highest_version"]["href"]
     )
 
-    assert version["name"] == artifact.name
-    assert version["namespace"] == {"name": artifact.namespace}
+    assert version["name"] == collection_artifact.name
+    assert version["namespace"] == {"name": collection_artifact.namespace}
     assert version["version"] == "1.0.0"
 
-    tarball = open(artifact.filename, "rb").read()
+    tarball = open(collection_artifact.filename, "rb").read()
     assert version["artifact"]["sha256"] == hashlib.sha256(tarball).hexdigest()
     assert version["artifact"]["size"] == len(tarball)
 
-    # assert version['artifact']['filename'] == artifact.filename
+    assert version["artifact"]["filename"] == collection_artifact.filename.strip("/tmp/")
 
     assert "updated_at" in version
     assert "created_at" in version
@@ -289,7 +297,7 @@ def test_collection_version(artifact, pulp_client, collection_detail):
 
 
 @pytest.mark.skip("Blocked by open ticket: https://pulp.plan.io/issues/5647")
-def test_collection_download(artifact, pulp_client, collection_detail):
+def test_collection_download(collection_artifact, pulp_client, collection_detail):
     """Test collection download URL.
 
     Should require authentication and redirect to a download location.
@@ -301,35 +309,25 @@ def test_collection_download(artifact, pulp_client, collection_detail):
     # Artifact Download Endoint
     url = version["download_url"]
 
-    tarball = open(artifact.filename, "rb").read()
+    tarball = open(collection_artifact.filename, "rb").read()
 
     c = pulp_client.using_handler(api.echo_handler)
     f = c.get(url)
     assert f.status_code == 200, (url, f.request.headers)
     assert f.content == tarball
 
 
-def test_collection_upload_repeat(pulp_client, known_collection, pulp_dist):
+def test_collection_upload_repeat(pulp_client, collection_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, collection_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",
+    }