From 8c70ec816f211ece858f85939358b94f62d87057 Mon Sep 17 00:00:00 2001 From: Nicolas Harraudeau Date: Thu, 17 Dec 2015 18:03:51 +0100 Subject: [PATCH] api: DELETE support for REST API * NEW Adds DELETE support to the records REST API. Signed-off-by: Nicolas Harraudeau --- .editorconfig | 2 +- invenio_records_rest/views.py | 43 ++++- setup.py | 4 +- tests/test_invenio_records_rest.py | 250 ++++++++++++++++++++++++++++- 4 files changed, 287 insertions(+), 12 deletions(-) diff --git a/.editorconfig b/.editorconfig index dc568185..0dcf6f1d 100644 --- a/.editorconfig +++ b/.editorconfig @@ -37,7 +37,7 @@ charset = utf-8 indent_size = 4 # isort plugin configuration known_first_party = invenio_records_rest -known_third_party = invenio_records, invenio_rest +known_third_party = invenio_records, invenio_rest, invenio_pidstore multi_line_output = 2 default_section = THIRDPARTY diff --git a/invenio_records_rest/views.py b/invenio_records_rest/views.py index 6bb4b9a4..d5bff11b 100644 --- a/invenio_records_rest/views.py +++ b/invenio_records_rest/views.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # # This file is part of Invenio. -# Copyright (C) 2015 CERN. +# Copyright (C) 2015, 2016 CERN. # # Invenio is free software; you can redistribute it # and/or modify it under the terms of the GNU General Public License as @@ -27,7 +27,7 @@ from __future__ import absolute_import, print_function import uuid -from functools import wraps +from functools import partial, wraps from flask import Blueprint, abort, current_app, jsonify, make_response, \ request, url_for @@ -35,19 +35,19 @@ from invenio_pidstore import current_pidstore from invenio_pidstore.errors import PIDDeletedError, PIDDoesNotExistError, \ PIDMissingObjectError, PIDRedirectedError, PIDUnregistered +from invenio_pidstore.models import PersistentIdentifier from invenio_pidstore.resolver import Resolver from invenio_records.api import Record from invenio_rest import ContentNegotiatedMethodView from invenio_rest.decorators import require_content_types from jsonpatch import JsonPatchException, JsonPointerException from sqlalchemy.exc import SQLAlchemyError -from werkzeug.routing import BuildError from werkzeug.local import LocalProxy +from werkzeug.routing import BuildError from werkzeug.utils import import_string from .serializers import record_to_json_serializer - current_records_rest = LocalProxy( lambda: current_app.extensions['invenio-records-rest']) @@ -108,7 +108,7 @@ def create_url_rules(endpoint, list_route=None, item_route=None, if delete_permission_factory_imp else None resolver = Resolver(pid_type=pid_type, object_type='rec', - getter=Record.get_record) + getter=partial(Record.get_record, with_deleted=True)) serializers = {'application/json': record_to_json_serializer, } @@ -273,12 +273,43 @@ def __init__(self, resolver=None, read_permission_factory=None, :param resolver: Persistent identifier resolver instance. """ - super(RecordResource, self).__init__(**kwargs) + super(RecordResource, self).__init__(method_serializers={ + 'DELETE': { + '*/*': lambda *args: make_response(*args), + }, + }, **kwargs) self.resolver = resolver self.read_permission_factory = read_permission_factory self.update_permission_factory = update_permission_factory self.delete_permission_factory = delete_permission_factory + @pass_record + @need_record_permission('delete_permission_factory') + def delete(self, pid, record, **kwargs): + """Delete a record. + + :param pid: Persistent identifier for record. + :param record: Record object. + """ + self.check_etag(str(record.model.version_id)) + + try: + record.delete() + # mark all PIDs as DELETED + all_pids = PersistentIdentifier.query \ + .filter(PersistentIdentifier.object_type == pid.object_type) \ + .filter(PersistentIdentifier.object_uuid == pid.object_uuid) \ + .all() + for rec_pid in all_pids: + if not rec_pid.is_deleted(): + rec_pid.delete() + db.session.commit() + except Exception: + db.session.rollback() + current_app.logger.exception("Failed to delete record.") + abort(500) + return '', 204 + @pass_record @need_record_permission('read_permission_factory') def get(self, pid, record, **kwargs): diff --git a/setup.py b/setup.py index 3459e119..47f9b3b6 100644 --- a/setup.py +++ b/setup.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # # This file is part of Invenio. -# Copyright (C) 2015 CERN. +# Copyright (C) 2015, 2016 CERN. # # Invenio is free software; you can redistribute it # and/or modify it under the terms of the GNU General Public License as @@ -64,7 +64,7 @@ install_requires = [ 'Flask-CLI>=0.2.1', 'six>=1.10', - 'invenio-rest>=1.0.0a3', + 'invenio-rest>=1.0.0a4', 'invenio-records>=1.0.0a7', 'invenio-pidstore>=1.0.0a2', ] diff --git a/tests/test_invenio_records_rest.py b/tests/test_invenio_records_rest.py index 46bff930..96445b19 100644 --- a/tests/test_invenio_records_rest.py +++ b/tests/test_invenio_records_rest.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # # This file is part of Invenio. -# Copyright (C) 2015 CERN. +# Copyright (C) 2015, 2016 CERN. # # Invenio is free software; you can redistribute it # and/or modify it under the terms of the GNU General Public License as @@ -31,15 +31,18 @@ import json import uuid +import pytest from flask import Flask, url_for from invenio_db import db -from invenio_pidstore import current_pidstore from invenio_records import Record from jsonpatch import apply_patch from mock import patch from six import string_types from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm.exc import NoResultFound +from invenio_pidstore import current_pidstore +from invenio_pidstore.models import PersistentIdentifier from invenio_records_rest import InvenioRecordsREST @@ -81,6 +84,18 @@ def create_record(data): return pid, record +def delete_record(pid, record): + with db.session.begin_nested(): + record.delete() + # mark all PIDs as DELETED + all_pids = PersistentIdentifier.query \ + .filter(PersistentIdentifier.object_type == pid.object_type) \ + .filter(PersistentIdentifier.object_uuid == pid.object_uuid) \ + .all() + for rec_pid in all_pids: + rec_pid.delete() + + def control_num(data, cn=1): """Inject a control number in data.""" data = copy.deepcopy(data) @@ -88,6 +103,135 @@ def control_num(data, cn=1): return data +def test_valid_delete(app): + """Test VALID record delete request (DELETE .../records/).""" + with app.app_context(): + # create the record using the internal API + pid, record = create_record(test_data) + + with app.test_client() as client: + headers = [('Accept', 'application/json')] + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + headers=headers) + assert res.status_code == 204 + # check database state + with pytest.raises(NoResultFound): + Record.get_record(record.id) + assert pid.is_deleted() + + # check that DELETE with non JSON accepted format will work + # as it returns nothing + pid, record = create_record(test_data) + headers = [('Accept', 'video/mp4')] + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + headers=headers) + assert res.status_code == 204 + + +def test_delete_deleted(app): + """Test deleting a perviously deleted record.""" + with app.app_context(): + # create the record using the internal API + pid, record = create_record(test_data) + delete_record(pid, record) + + with app.test_client() as client: + headers = [('Accept', 'application/json')] + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + headers=headers) + assert res.status_code == 410 + # check that the returned record matches the given data + response_data = json.loads(res.get_data(as_text=True)) + assert response_data['status'] == 410 + assert 'no longer available' in response_data['message'] + + +def test_invalid_delete(app): + """Test INVALID record delete request (DELETE .../records/).""" + with app.app_context(): + with app.test_client() as client: + # check that GET with non existing id will return 404 + headers = [('Accept', 'application/json')] + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=0), + headers=headers) + assert res.status_code == 404 + + # check that deleting a deleted record returns 410 + pid, record = create_record(test_data) + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + headers=headers) + assert res.status_code == 204 + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + headers=headers) + assert res.status_code == 410 + + +def test_delete_with_sqldatabase_error(app): + """Test VALID record delete request (GET .../records/).""" + with app.app_context(): + # create the record using the internal API + pid, record = create_record(test_data) + db.session.expire(record.model) + pid_value = pid.pid_value + pid_type = pid.pid_type + record_id = record.id + + db.session.commit() + Record.get_record(record_id) + + def raise_exception(): + raise SQLAlchemyError() + + with app.test_client() as client: + # start a new SQLAlchemy session so that it will rollback + # everything + nested_transaction = db.session().transaction + orig_rollback = nested_transaction.rollback + flags = {'rollbacked': False} + + def custom_rollback(*args, **kwargs): + flags['rollbacked'] = True + orig_rollback(*args, **kwargs) + nested_transaction.rollback = custom_rollback + + with patch.object(PersistentIdentifier, 'delete', + side_effect=raise_exception): + headers = [('Accept', 'application/json')] + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid_value), + headers=headers) + assert res.status_code == 500 + # check that the transaction is finished + assert db.session().transaction is not nested_transaction + # check that the session has rollbacked + assert flags['rollbacked'] + + with app.app_context(): + with app.test_client() as client: + # check that the record and PID have not been deleted + Record.get_record(record_id) + assert not PersistentIdentifier.get(pid_type, + pid_value).is_deleted() + # try to delete without exception, the transaction should have been + # rollbacked + headers = [('Accept', 'application/json')] + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid_value), + headers=headers) + assert res.status_code == 204 + # check database state + with pytest.raises(NoResultFound): + Record.get_record(record_id) + assert PersistentIdentifier.get(pid_type, + pid_value).is_deleted() + + def test_valid_create(app, resolver): """Test VALID record creation request (POST .../records/).""" with app.app_context(): @@ -184,6 +328,26 @@ def test_valid_get(app): subtest_self_link(response_data, res.headers, client) +def test_get_deleted(app): + """Test getting deleted record.""" + with app.app_context(): + # create the record using the internal API + pid, record = create_record(test_data) + delete_record(pid, record) + + with app.test_client() as client: + headers = [('Accept', 'application/json')] + # check get response for deleted resource + get_res = client.get(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + headers=headers) + assert get_res.status_code == 410 + # check that the returned record matches the given data + response_data = json.loads(get_res.get_data(as_text=True)) + assert response_data['status'] == 410 + assert 'no longer available' in response_data['message'] + + def test_invalid_get(app): """Test INVALID record get request (GET .../records/).""" with app.app_context(): @@ -235,6 +399,28 @@ def test_valid_patch(app, resolver): subtest_self_link(response_data, res.headers, client) +def test_patch_deleted(app): + """Test patching deleted record.""" + with app.app_context(): + # create the record using the internal API + pid, record = create_record(test_data) + delete_record(pid, record) + + with app.test_client() as client: + headers = [('Content-Type', 'application/json-patch+json'), + ('Accept', 'application/json')] + # check patch response for deleted resource + patch_res = client.patch(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + data=json.dumps(test_patch), + headers=headers) + assert patch_res.status_code == 410 + # check that the returned record matches the given data + response_data = json.loads(patch_res.get_data(as_text=True)) + assert response_data['status'] == 410 + assert 'no longer available' in response_data['message'] + + def test_invalid_patch(app): """Test INVALID record patch request (PATCH .../records/).""" with app.app_context(): @@ -244,7 +430,6 @@ def test_invalid_patch(app): ('Accept', 'application/json')] res = client.patch(url_for('invenio_records_rest.recid_item', pid_value=0), - data=json.dumps(test_patch), headers=headers) assert res.status_code == 404 @@ -317,6 +502,28 @@ def test_valid_put(app, resolver): subtest_self_link(response_data, res.headers, client) +def test_put_deleted(app): + """Test putting deleted record.""" + with app.app_context(): + # create the record using the internal API + pid, record = create_record(test_data) + delete_record(pid, record) + + with app.test_client() as client: + headers = [('Content-Type', 'application/json'), + ('Accept', 'application/json')] + # check put response for deleted resource + put_res = client.put(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + data=json.dumps(test_data_patched), + headers=headers) + assert put_res.status_code == 410 + # check that the returned record matches the given data + response_data = json.loads(put_res.get_data(as_text=True)) + assert response_data['status'] == 410 + assert 'no longer available' in response_data['message'] + + def test_invalid_put(app): """Test INVALID record put request (PUT .../records/).""" with app.app_context(): @@ -533,6 +740,43 @@ def test_put_one_permissions(app, user_factory, resolver): assert response_data['metadata'] == test_data_patched +def test_delete_one_permissions(app, user_factory, resolver): + with app.app_context(): + # create the record using the internal API + pid, internal_record = create_record(test_data) + with user_factory('allowed') as allowed_user, \ + user_factory('forbidden') as forbidden_user: + # create one user allowed to delete the record + allowed_user.delete_access(True, str(internal_record.id)) + allowed_login = allowed_user.login_function() + # create one user who is not allowed to delete the record + forbidden_user.delete_access(False, str(internal_record.id)) + forbidden_login = forbidden_user.login_function() + db.session.commit() + + headers = [('Content-Type', 'application/json')] + # test get without being authenticated + with app.test_client() as client: + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + headers=headers) + assert res.status_code == 401 + # test not allowed get + with app.test_client() as client: + forbidden_login(client) + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + headers=headers) + assert res.status_code == 403 + # test allowed get + with app.test_client() as client: + allowed_login(client) + res = client.delete(url_for('invenio_records_rest.recid_item', + pid_value=pid.pid_value), + headers=headers) + assert res.status_code == 204 + + def subtest_self_link(response_data, response_headers, client): """Check that the returned self link returns the same data.