From d6d1b037e8b2e48e03603062d141969f6139ee05 Mon Sep 17 00:00:00 2001 From: Gerrod Ubben Date: Wed, 17 Aug 2022 18:07:18 -0400 Subject: [PATCH] Fix duplicate collection upload error msg & tests fixes: #1175 --- CHANGES/1175.bugfix | 1 + pulp_ansible/app/galaxy/serializers.py | 8 ++ pulp_ansible/app/serializers.py | 11 +- .../api/collection/v2/test_upload.py | 17 +-- .../api/collection/v3/test_collection.py | 104 +++++++++--------- 5 files changed, 79 insertions(+), 62 deletions(-) create mode 100644 CHANGES/1175.bugfix 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..b0db9e183 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.body 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..a18968a55 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 @@ -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,20 +217,20 @@ 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"] @@ -232,12 +238,14 @@ def test_collection_detail(artifact, collection_detail, pulp_dist): # 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,7 +309,7 @@ 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) @@ -309,27 +317,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, 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", + }