diff --git a/swh/deposit/api/common.py b/swh/deposit/api/common.py --- a/swh/deposit/api/common.py +++ b/swh/deposit/api/common.py @@ -218,7 +218,9 @@ # find a deposit parent (same external id, status load to success) deposit_parent = ( Deposit.objects.filter( - external_id=external_id, status=DEPOSIT_STATUS_LOAD_SUCCESS + client=self._client, + external_id=external_id, + status=DEPOSIT_STATUS_LOAD_SUCCESS, ) .order_by("-id")[0:1] .get() diff --git a/swh/deposit/tests/api/test_collection.py b/swh/deposit/tests/api/test_collection.py --- a/swh/deposit/tests/api/test_collection.py +++ b/swh/deposit/tests/api/test_collection.py @@ -20,6 +20,8 @@ from swh.deposit.models import Deposit from swh.deposit.parsers import parse_xml +from ..conftest import create_deposit + def test_deposit_post_will_fail_with_401(client): """Without authentication, endpoint refuses access with 401 response @@ -192,3 +194,95 @@ assert new_deposit != deposit assert new_deposit.parent == deposit + + +def test_add_deposit_external_id_conflict_no_parent( + authenticated_client, + another_authenticated_client, + deposit_collection, + deposit_another_collection, + atom_dataset, + sample_archive, +): + """Posting a deposit with an external_id conflicting with an external_id + of a different client does not create a parent relationship + + """ + external_id = "foobar" + + # create a deposit for that other user, with the same slug + other_deposit = create_deposit( + another_authenticated_client, + deposit_another_collection.name, + sample_archive, + external_id, + DEPOSIT_STATUS_LOAD_SUCCESS, + ) + + # adding a new deposit with the same external id as a completed deposit + response = authenticated_client.post( + reverse(COL_IRI, args=[deposit_collection.name]), + content_type="application/atom+xml;type=entry", + data=atom_dataset["entry-data0"], + HTTP_SLUG=external_id, + ) + + assert response.status_code == status.HTTP_201_CREATED + response_content = parse_xml(BytesIO(response.content)) + deposit_id = response_content["swh:deposit_id"] + + assert other_deposit.id != deposit_id + + new_deposit = Deposit.objects.get(pk=deposit_id) + + assert new_deposit.parent is None + + +def test_add_deposit_external_id_conflict_with_parent( + authenticated_client, + another_authenticated_client, + deposit_collection, + deposit_another_collection, + completed_deposit, + atom_dataset, + sample_archive, +): + """Posting a deposit with an external_id conflicting with an external_id + of a different client creates a parent relationship with the deposit + of the right client instead of the last matching deposit + + """ + # given multiple deposit already loaded + deposit = completed_deposit + assert deposit.status == DEPOSIT_STATUS_LOAD_SUCCESS + + # create a deposit for that other user, with the same slug + other_deposit = create_deposit( + another_authenticated_client, + deposit_another_collection.name, + sample_archive, + deposit.external_id, + DEPOSIT_STATUS_LOAD_SUCCESS, + ) + + # adding a new deposit with the same external id as a completed deposit + response = authenticated_client.post( + reverse(COL_IRI, args=[deposit_collection.name]), + content_type="application/atom+xml;type=entry", + data=atom_dataset["entry-data0"], + HTTP_SLUG=deposit.external_id, + ) + + assert response.status_code == status.HTTP_201_CREATED + response_content = parse_xml(BytesIO(response.content)) + deposit_id = response_content["swh:deposit_id"] + + assert deposit_id != deposit.id + assert other_deposit.id != deposit.id + + new_deposit = Deposit.objects.get(pk=deposit_id) + assert deposit.collection == new_deposit.collection + assert deposit.external_id == new_deposit.external_id + + assert new_deposit != deposit + assert new_deposit.parent == deposit diff --git a/swh/deposit/tests/conftest.py b/swh/deposit/tests/conftest.py --- a/swh/deposit/tests/conftest.py +++ b/swh/deposit/tests/conftest.py @@ -5,6 +5,7 @@ import base64 from functools import partial +from io import BytesIO import os import re from typing import Mapping @@ -50,6 +51,16 @@ } +ANOTHER_TEST_USER = { + "username": "test2", + "password": "password2", + "email": "test@example2.org", + "provider_url": "https://hal-test.archives-ouvertes.example/", + "domain": "archives-ouvertes.example/", + "collection": {"name": "another-collection"}, +} + + def pytest_configure(): setup_django_for("testing") @@ -174,28 +185,37 @@ deposit_another_collection = deposit_collection_factory("another-collection") -@pytest.fixture -def deposit_user(db, deposit_collection): +def _create_deposit_user(db, collection, user_data): """Create/Return the test_user "test" """ from swh.deposit.models import DepositClient try: - user = DepositClient._default_manager.get(username=TEST_USER["username"]) + user = DepositClient._default_manager.get(username=user_data["username"]) except DepositClient.DoesNotExist: user = DepositClient._default_manager.create_user( - username=TEST_USER["username"], - email=TEST_USER["email"], - password=TEST_USER["password"], - provider_url=TEST_USER["provider_url"], - domain=TEST_USER["domain"], + username=user_data["username"], + email=user_data["email"], + password=user_data["password"], + provider_url=user_data["provider_url"], + domain=user_data["domain"], ) - user.collections = [deposit_collection.id] + user.collections = [collection.id] user.save() return user +@pytest.fixture +def deposit_user(db, deposit_collection): + return _create_deposit_user(db, deposit_collection, TEST_USER) + + +@pytest.fixture +def deposit_another_user(db, deposit_another_collection): + return _create_deposit_user(db, deposit_another_collection, ANOTHER_TEST_USER) + + @pytest.fixture def client(): """Override pytest-django one which does not work for djangorestframework. @@ -204,23 +224,35 @@ return APIClient() # <- drf's client -@pytest.fixture -def authenticated_client(client, deposit_user): +def _create_authenticated_client(client, user, user_data): """Returned a logged client This also patched the client instance to keep a reference on the associated deposit_user. """ - _token = "%s:%s" % (deposit_user.username, TEST_USER["password"]) + _token = "%s:%s" % (user.username, user_data["password"]) token = base64.b64encode(_token.encode("utf-8")) authorization = "Basic %s" % token.decode("utf-8") client.credentials(HTTP_AUTHORIZATION=authorization) - client.deposit_client = deposit_user + client.deposit_client = user yield client client.logout() +@pytest.fixture +def authenticated_client(client, deposit_user): + yield from _create_authenticated_client(client, deposit_user, TEST_USER) + + +@pytest.fixture +def another_authenticated_client(deposit_another_user): + client = APIClient() + yield from _create_authenticated_client( + client, deposit_another_user, ANOTHER_TEST_USER + ) + + @pytest.fixture def sample_archive(tmp_path): """Returns a sample archive @@ -282,10 +314,12 @@ ) # then - assert response.status_code == status.HTTP_201_CREATED + assert response.status_code == status.HTTP_201_CREATED, response.content.decode() from swh.deposit.models import Deposit - deposit = Deposit._default_manager.get(external_id=external_id) + response_content = parse_xml(BytesIO(response.content)) + deposit_id = response_content["swh:deposit_id"] + deposit = Deposit._default_manager.get(id=deposit_id) if deposit.status != deposit_status: deposit.status = deposit_status