diff --git a/MANIFEST.in b/MANIFEST.in --- a/MANIFEST.in +++ b/MANIFEST.in @@ -4,6 +4,7 @@ recursive-include swh/deposit/static * recursive-include swh/deposit/fixtures * recursive-include swh/deposit/templates * +recursive-include swh/deposit/tests/data * recursive-include swh/deposit/tests/*/data * recursive-include swh py.typed include tox.ini diff --git a/swh/deposit/api/checks.py b/swh/deposit/api/checks.py new file mode 100644 --- /dev/null +++ b/swh/deposit/api/checks.py @@ -0,0 +1,54 @@ +# Copyright (C) 2017-2020 The Software Heritage developers +# See the AUTHORS file at the top-level directory of this distribution +# License: GNU General Public License version 3, or any later version +# See top-level LICENSE file for more information + +from typing import Dict, Optional, Tuple + +MANDATORY_FIELDS_MISSING = "Mandatory fields are missing" +ALTERNATE_FIELDS_MISSING = "Mandatory alternate fields are missing" + + +def check_metadata(metadata: Dict) -> Tuple[bool, Optional[Dict]]: + """Check metadata for mandatory field presence. + + Args: + metadata: Metadata dictionary to check for mandatory fields + + Returns: + tuple (status, error_detail): True, None if metadata are + ok (False, ) otherwise. + + """ + required_fields = { + "author": False, + } + alternate_fields = { + ("name", "title"): False, # alternate field, at least one + # of them must be present + } + + for field, value in metadata.items(): + for name in required_fields: + if name in field: + required_fields[name] = True + + for possible_names in alternate_fields: + for possible_name in possible_names: + if possible_name in field: + alternate_fields[possible_names] = True + continue + + mandatory_result = [k for k, v in required_fields.items() if not v] + optional_result = [" or ".join(k) for k, v in alternate_fields.items() if not v] + + if mandatory_result == [] and optional_result == []: + return True, None + detail = [] + if mandatory_result != []: + detail.append({"summary": MANDATORY_FIELDS_MISSING, "fields": mandatory_result}) + if optional_result != []: + detail.append( + {"summary": ALTERNATE_FIELDS_MISSING, "fields": optional_result,} + ) + return False, {"metadata": detail} diff --git a/swh/deposit/api/deposit_update.py b/swh/deposit/api/deposit_update.py --- a/swh/deposit/api/deposit_update.py +++ b/swh/deposit/api/deposit_update.py @@ -8,6 +8,8 @@ from rest_framework import status from rest_framework.request import Request +from swh.deposit.api.checks import check_metadata +from swh.deposit.api.converters import convert_status_detail from swh.deposit.models import Deposit from swh.model.hashutil import hash_to_bytes from swh.model.identifiers import parse_swhid @@ -290,6 +292,16 @@ ) return (status.HTTP_400_BAD_REQUEST, EM_IRI, data) + metadata_ok, error_details = check_metadata(metadata) + if not metadata_ok: + assert error_details, "Details should be set when a failure occurs" + data = make_error_dict( + BAD_REQUEST, + "Functional metadata checks failure", + convert_status_detail(error_details), + ) + return (status.HTTP_400_BAD_REQUEST, EM_IRI, data) + metadata_authority = MetadataAuthority( type=MetadataAuthorityType.DEPOSIT_CLIENT, url=deposit.client.provider_url, diff --git a/swh/deposit/api/private/deposit_check.py b/swh/deposit/api/private/deposit_check.py --- a/swh/deposit/api/private/deposit_check.py +++ b/swh/deposit/api/private/deposit_check.py @@ -17,10 +17,9 @@ from . import APIPrivateView, DepositReadMixin from ...config import ARCHIVE_TYPE, DEPOSIT_STATUS_REJECTED, DEPOSIT_STATUS_VERIFIED from ...models import Deposit, DepositRequest +from ..checks import check_metadata from ..common import APIGet -MANDATORY_FIELDS_MISSING = "Mandatory fields are missing" -ALTERNATE_FIELDS_MISSING = "Mandatory alternate fields are missing" MANDATORY_ARCHIVE_UNREADABLE = ( "At least one of its associated archives is not readable" # noqa ) @@ -130,52 +129,6 @@ return False, MANDATORY_ARCHIVE_INVALID return True, None - def _check_metadata(self, metadata: Dict) -> Tuple[bool, Optional[Dict]]: - """Check to execute on all metadata for mandatory field presence. - - Args: - metadata (dict): Metadata dictionary to check for mandatory fields - - Returns: - tuple (status, error_detail): True, None if metadata are - ok (False, ) otherwise. - - """ - required_fields = { - "author": False, - } - alternate_fields = { - ("name", "title"): False, # alternate field, at least one - # of them must be present - } - - for field, value in metadata.items(): - for name in required_fields: - if name in field: - required_fields[name] = True - - for possible_names in alternate_fields: - for possible_name in possible_names: - if possible_name in field: - alternate_fields[possible_names] = True - continue - - mandatory_result = [k for k, v in required_fields.items() if not v] - optional_result = [" or ".join(k) for k, v in alternate_fields.items() if not v] - - if mandatory_result == [] and optional_result == []: - return True, None - detail = [] - if mandatory_result != []: - detail.append( - {"summary": MANDATORY_FIELDS_MISSING, "fields": mandatory_result} - ) - if optional_result != []: - detail.append( - {"summary": ALTERNATE_FIELDS_MISSING, "fields": optional_result,} - ) - return False, {"metadata": detail} - def process_get( self, req, collection_name: str, deposit_id: int ) -> Tuple[int, Dict, str]: @@ -201,7 +154,7 @@ assert error_detail is not None problems.update(error_detail) - metadata_status, error_detail = self._check_metadata(metadata) + metadata_status, error_detail = check_metadata(metadata) if not metadata_status: assert error_detail is not None problems.update(error_detail) diff --git a/swh/deposit/tests/api/test_checks.py b/swh/deposit/tests/api/test_checks.py new file mode 100644 --- /dev/null +++ b/swh/deposit/tests/api/test_checks.py @@ -0,0 +1,78 @@ +# Copyright (C) 2017-2020 The Software Heritage developers +# See the AUTHORS file at the top-level directory of this distribution +# License: GNU General Public License version 3, or any later version +# See top-level LICENSE file for more information + +from swh.deposit.api.checks import check_metadata + + +def test_api_checks_check_metadata_ok(swh_checks_deposit): + actual_check, detail = check_metadata( + { + "url": "something", + "external_identifier": "something-else", + "name": "foo", + "author": "someone", + } + ) + + assert actual_check is True + assert detail is None + + +def test_api_checks_check_metadata_ok2(swh_checks_deposit): + actual_check, detail = check_metadata( + { + "url": "something", + "external_identifier": "something-else", + "title": "bar", + "author": "someone", + } + ) + + assert actual_check is True + assert detail is None + + +def test_api_checks_check_metadata_ko(swh_checks_deposit): + """Missing optional field should be caught + + """ + actual_check, error_detail = check_metadata( + { + "url": "something", + "external_identifier": "something-else", + "author": "someone", + } + ) + + expected_error = { + "metadata": [ + { + "summary": "Mandatory alternate fields are missing", + "fields": ["name or title"], + } + ] + } + assert actual_check is False + assert error_detail == expected_error + + +def test_api_checks_check_metadata_ko2(swh_checks_deposit): + """Missing mandatory fields should be caught + + """ + actual_check, error_detail = check_metadata( + { + "url": "something", + "external_identifier": "something-else", + "title": "foobar", + } + ) + + expected_error = { + "metadata": [{"summary": "Mandatory fields are missing", "fields": ["author"],}] + } + + assert actual_check is False + assert error_detail == expected_error diff --git a/swh/deposit/tests/api/test_deposit_private_check.py b/swh/deposit/tests/api/test_deposit_private_check.py --- a/swh/deposit/tests/api/test_deposit_private_check.py +++ b/swh/deposit/tests/api/test_deposit_private_check.py @@ -1,4 +1,4 @@ -# Copyright (C) 2017-2019 The Software Heritage developers +# Copyright (C) 2017-2020 The Software Heritage developers # See the AUTHORS file at the top-level directory of this distribution # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information @@ -7,12 +7,11 @@ import pytest from rest_framework import status +from swh.deposit.api.checks import ALTERNATE_FIELDS_MISSING, MANDATORY_FIELDS_MISSING from swh.deposit.api.private.deposit_check import ( - ALTERNATE_FIELDS_MISSING, MANDATORY_ARCHIVE_INVALID, MANDATORY_ARCHIVE_MISSING, MANDATORY_ARCHIVE_UNSUPPORTED, - MANDATORY_FIELDS_MISSING, ) from swh.deposit.config import ( COL_IRI, @@ -171,78 +170,6 @@ deposit.save() -def test_check_metadata_ok(swh_checks_deposit): - actual_check, detail = swh_checks_deposit._check_metadata( - { - "url": "something", - "external_identifier": "something-else", - "name": "foo", - "author": "someone", - } - ) - - assert actual_check is True - assert detail is None - - -def test_check_metadata_ok2(swh_checks_deposit): - actual_check, detail = swh_checks_deposit._check_metadata( - { - "url": "something", - "external_identifier": "something-else", - "title": "bar", - "author": "someone", - } - ) - - assert actual_check is True - assert detail is None - - -def test_check_metadata_ko(swh_checks_deposit): - """Missing optional field should be caught - - """ - actual_check, error_detail = swh_checks_deposit._check_metadata( - { - "url": "something", - "external_identifier": "something-else", - "author": "someone", - } - ) - - expected_error = { - "metadata": [ - { - "summary": "Mandatory alternate fields are missing", - "fields": ["name or title"], - } - ] - } - assert actual_check is False - assert error_detail == expected_error - - -def test_check_metadata_ko2(swh_checks_deposit): - """Missing mandatory fields should be caught - - """ - actual_check, error_detail = swh_checks_deposit._check_metadata( - { - "url": "something", - "external_identifier": "something-else", - "title": "foobar", - } - ) - - expected_error = { - "metadata": [{"summary": "Mandatory fields are missing", "fields": ["author"],}] - } - - assert actual_check is False - assert error_detail == expected_error - - def create_deposit_archive_with_archive( root_path, archive_extension, client, collection_name ): diff --git a/swh/deposit/tests/api/test_deposit_update.py b/swh/deposit/tests/api/test_deposit_update.py --- a/swh/deposit/tests/api/test_deposit_update.py +++ b/swh/deposit/tests/api/test_deposit_update.py @@ -759,3 +759,37 @@ assert response.status_code == status.HTTP_400_BAD_REQUEST assert b"Empty body request is not supported" in response.content + + +def test_post_update_metadata_done_deposit_failure_functional_checks( + tmp_path, + authenticated_client, + complete_deposit, + deposit_collection, + atom_dataset, + swh_storage, +): + """failure: client updates metadata on deposit done without required incomplete metadata + + Response: 400 + + """ + update_uri = reverse( + EDIT_SE_IRI, args=[deposit_collection.name, complete_deposit.id] + ) + + response = authenticated_client.post( + update_uri, + content_type="application/atom+xml;type=entry", + # no title, nor author, nor name fields + data=atom_dataset["entry-data-fail-metadata-functional-checks"], + HTTP_X_CHECK_SWHID=complete_deposit.swh_id, + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert b"Functional metadata checks failure" in response.content + # detail on the errors + assert b"- Mandatory fields are missing (author)" in response.content + assert ( + b"- Mandatory alternate fields are missing (name or title)" in response.content + ) diff --git a/swh/deposit/tests/data/atom/entry-data-fail-metadata-functional-checks.xml b/swh/deposit/tests/data/atom/entry-data-fail-metadata-functional-checks.xml new file mode 100644 --- /dev/null +++ b/swh/deposit/tests/data/atom/entry-data-fail-metadata-functional-checks.xml @@ -0,0 +1,7 @@ + + + + hal + urn:uuid:1225c695-cfb8-4ebb-aaaa-80da344efa6a + 2017-10-07T15:17:08Z +