From cd98870b3f4d8063004f5c808dc7d7b4858a38c7 Mon Sep 17 00:00:00 2001 From: Josh Borrow Date: Thu, 29 Feb 2024 14:02:27 -0500 Subject: [PATCH] Cleanup for remote transfers --- librarian_server/api/clone.py | 25 ++++++++++++++++++++++--- librarian_server/orm/instance.py | 4 ++-- librarian_server/orm/transfer.py | 1 + tests/server_unit_test/test_clone.py | 17 +++++++++++++---- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/librarian_server/api/clone.py b/librarian_server/api/clone.py index 009d0fe..b5b481d 100644 --- a/librarian_server/api/clone.py +++ b/librarian_server/api/clone.py @@ -41,6 +41,7 @@ from ..logger import log from ..orm.file import File from ..orm.instance import RemoteInstance +from ..orm.librarian import Librarian from ..orm.storemetadata import StoreMetadata from ..orm.transfer import IncomingTransfer, OutgoingTransfer, TransferStatus from .auth import CallbackUserDependency, ReadappendUserDependency @@ -342,7 +343,8 @@ def complete( Possible response codes: 200 - OK. Transfer status updated. - 404 - Not found. Could not find transfer. + 400 - Not found. Could not find transfer. + 400 - Bad request. Could not find librarian. 406 - Not acceptable. Transfer is not in STAGED status. """ @@ -361,7 +363,7 @@ def complete( f"Could not find transfer with ID {request.source_transfer_id}. Returning error." ) - response.status_code = status.HTTP_404_NOT_FOUND + response.status_code = status.HTTP_400_BAD_REQUEST return CloneFailedResponse( reason="Could not find transfer.", suggested_remedy=( @@ -389,13 +391,30 @@ def complete( destination_transfer_id=request.destination_transfer_id, ) + librarian = session.query(Librarian).filter_by(name=transfer.destination).first() + + if librarian is None: + log.debug(f"Could not find librarian {transfer.destination}. Returning error.") + + response.status_code = status.HTTP_400_BAD_REQUEST + return CloneFailedResponse( + reason=f"Could not find librarian {transfer.destination}.", + suggested_remedy=( + f"Check your librarian configuration. The librarian {transfer.destination} needs " + "to be an entry in this database. No remote instances will be created, and the " + "transfer status will not be updated, so you can try again afterwards." + ), + source_transfer_id=request.source_transfer_id, + destination_transfer_id=request.destination_transfer_id, + ) + transfer.status = TransferStatus.COMPLETED # Create new remote instance for this file that was just completed. remote_instance = RemoteInstance.new_instance( file=transfer.file, store_id=request.store_id, - librarian=transfer.destination, + librarian=librarian, ) session.add(remote_instance) diff --git a/librarian_server/orm/instance.py b/librarian_server/orm/instance.py index 260e65c..4989da5 100644 --- a/librarian_server/orm/instance.py +++ b/librarian_server/orm/instance.py @@ -142,8 +142,8 @@ def new_instance( return RemoteInstance( file=file, store_id=store_id, + librarian_id=librarian.id, librarian=librarian, copy_time=datetime.utcnow(), - # TODO: This should somehow be our name? Not just the displayed site name. - sender=server_settings.displayed_site_name, + sender=server_settings.name, ) diff --git a/librarian_server/orm/transfer.py b/librarian_server/orm/transfer.py index 9f6a2c0..65fa585 100644 --- a/librarian_server/orm/transfer.py +++ b/librarian_server/orm/transfer.py @@ -159,6 +159,7 @@ def new_transfer( destination=destination, transfer_size=file.size, transfer_checksum=file.checksum, + file_name=file.name, instance_id=instance.id, start_time=datetime.datetime.utcnow(), ) diff --git a/tests/server_unit_test/test_clone.py b/tests/server_unit_test/test_clone.py index 30b7c4c..2c183a9 100644 --- a/tests/server_unit_test/test_clone.py +++ b/tests/server_unit_test/test_clone.py @@ -307,8 +307,16 @@ def test_incoming_transfer_endpoints( session.add(transfer) session.commit() + librarian = test_orm.Librarian.new_librarian( + name="test2", url="http://localhost", port=5000, check_connection=False + ) + librarian.authenticator = "does_not_authenticate" + session.add(librarian) + session.commit() + transfer_id = transfer.id instance_id = instance.id + store_id = store.id # We will first test the failure case where we have not set the transfer to be ongoing @@ -317,7 +325,7 @@ def test_incoming_transfer_endpoints( request = CloneCompleteRequest( source_transfer_id=transfer_id, destination_transfer_id=transfer_id, - store_id=store.id, + store_id=store_id, ) response = test_client.post_with_auth( @@ -351,10 +359,11 @@ def test_incoming_transfer_endpoints( assert transfer.status == test_orm.TransferStatus.COMPLETED - instance = session.get(test_orm.Instance, instance_id) file = session.get(test_orm.File, str(garbage_filename)) - session.delete(instance) + session.delete(*file.instances) + session.delete(*file.remote_instances) + session.delete(transfer) session.delete(file) session.commit() @@ -374,7 +383,7 @@ def test_complete_no_transfer(test_client, test_server, test_orm): "/api/v2/clone/complete", content=request.model_dump_json() ) - assert response.status_code == 404 + assert response.status_code == 400 decoded_response = CloneFailedResponse.model_validate_json(response.content)