From 990c074b54d3709fd78000f6d8bd5147ad4f267e Mon Sep 17 00:00:00 2001 From: Martin Stolle Date: Thu, 29 Feb 2024 07:58:07 +0100 Subject: [PATCH] ZO-4267: change in contract: copy and move raise CopyError and MoveError instead of KeyError if resources are missing Ideally we would have a exception hierachy CopyError - ResourceMissingError(CopyError) - TargetAlreadyExistError(CopyError) - etc. and then the consumer of the connector can decide what to do with the individual errors or if the catch all at once with CopyError --- core/src/zeit/connector/connector.py | 16 ++++++++++------ core/src/zeit/connector/interfaces.py | 13 +++++++++---- core/src/zeit/connector/mock.py | 12 ++++++++++-- core/src/zeit/connector/tests/test_contract.py | 8 +++++++- 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/core/src/zeit/connector/connector.py b/core/src/zeit/connector/connector.py index a91650d5bc..d02d8233e2 100644 --- a/core/src/zeit/connector/connector.py +++ b/core/src/zeit/connector/connector.py @@ -281,12 +281,13 @@ def copy(self, old_id, new_id): def move(self, old_id, new_id): """Move the resource with id `old_id` to `new_id`.""" - self._copy_or_move( - 'move', zeit.connector.interfaces.MoveError, old_id, new_id, resolve_conflicts=True - ) + self._copy_or_move('move', zeit.connector.interfaces.MoveError, old_id, new_id) - def _copy_or_move(self, method_name, exception, old_id, new_id, resolve_conflicts=False): - source = self[old_id] # Makes sure source exists. + def _copy_or_move(self, method_name, exception, old_id, new_id): + try: + source = self[old_id] # Makes sure source exists. + except KeyError as err: + raise exception(old_id, f'Can not copy or move non-existant resource {old_id}') from err if self._is_descendant(new_id, old_id): raise exception(old_id, 'Could not copy or move %s to a decendant of itself.' % old_id) @@ -325,7 +326,10 @@ def _copy_or_move(self, method_name, exception, old_id, new_id, resolve_conflict del self[old_id] else: token = self._get_my_locktoken(old_id) - method(old_loc, new_loc, locktoken=token) + try: + method(old_loc, new_loc, locktoken=token) + except zeit.connector.dav.interfaces.DAVNotFoundError as err: + raise exception('The resource %s does not exist.', old_id) from err self._invalidate_cache(old_id) self._invalidate_cache(new_id) diff --git a/core/src/zeit/connector/interfaces.py b/core/src/zeit/connector/interfaces.py index 53e99778d9..2637ed3555 100644 --- a/core/src/zeit/connector/interfaces.py +++ b/core/src/zeit/connector/interfaces.py @@ -130,17 +130,22 @@ def add(object, verify_etag=True): def copy(old_id, new_id): """Copy the resource old_id to new_id. - raises KeyError if ther is no resource `old_id` - raises CopyError if there was a problem with copying itself. + A COPY method invocation MUST NOT duplicate any write locks active on the source. + raises CopyError if there was a problem with copying itself. """ def move(old_id, new_id): """Move the resource with id `old_id` to `new_id`. - raises KeyError if ther is no resource `old_id` - raises MoveError if there was a problem with moving itself. + A successful MOVE request on a write locked resource MUST NOT move the + write lock with the resource. + However, the resource is subject to being added to an existing lock at the destination. + If the collection /a/b/ is write locked and the resource /c is moved to /a/b/c + then resource /a/b/c will be added to the write lock. + + raises MoveError if there was a problem with moving itself. """ def changeProperties(id, properties): diff --git a/core/src/zeit/connector/mock.py b/core/src/zeit/connector/mock.py index 19c17e8c48..3b0494f019 100644 --- a/core/src/zeit/connector/mock.py +++ b/core/src/zeit/connector/mock.py @@ -187,7 +187,10 @@ def add(self, object, verify_etag=True): def copy(self, old_id, new_id): self._prevent_overwrite(old_id, new_id, CopyError) - r = self[old_id] + try: + r = self[old_id] + except KeyError: + raise CopyError(old_id, '') r.id = new_id r.properties.pop(UUID_PROPERTY, None) self.add(r, verify_etag=False) @@ -199,7 +202,12 @@ def copy(self, old_id, new_id): def move(self, old_id, new_id): self._prevent_overwrite(old_id, new_id, MoveError) self._ignore_uuid_checks = True - r = self[old_id] + try: + r = self[old_id] + except KeyError: + raise MoveError(old_id, f'The resource {old_id} does not exist.') + if new_id in self: + raise MoveError(new_id, f'The resource {new_id} already exists.') r.id = new_id try: self.add(r, verify_etag=False) diff --git a/core/src/zeit/connector/tests/test_contract.py b/core/src/zeit/connector/tests/test_contract.py index 59e01dc735..73ecd1ff90 100644 --- a/core/src/zeit/connector/tests/test_contract.py +++ b/core/src/zeit/connector/tests/test_contract.py @@ -202,8 +202,14 @@ def test_move_collection_applies_to_all_children(self): self.assertNotIn('http://xml.zeit.de/testing/source/file', self.connector) self.assertIn('http://xml.zeit.de/testing/target/file', self.connector) + def test_move_nonexistent_raises(self): + with self.assertRaises(MoveError): + self.connector.move( + 'http://xml.zeit.de/nonexistent', 'http://xml.zeit.de/testing/target' + ) + def test_copy_nonexistent_raises(self): - with self.assertRaises(KeyError): + with self.assertRaises(CopyError): self.connector.copy( 'http://xml.zeit.de/nonexistent', 'http://xml.zeit.de/testing/target' )