From e95aded066814855eb72d0e77feafbe181a815ce Mon Sep 17 00:00:00 2001 From: ehennestad Date: Thu, 8 Oct 2026 17:57:42 +0200 Subject: [PATCH 1/2] Check the source and the destination before a copy The data-proxy answers 201 to every copy and reports a failure only by email, so copy_to() returned True for a copy of an object that no longer exists, and replaced an object at the destination without warning. copy_to() now raises DoesNotExist when the object is no longer in the bucket, and FileExistsError when the destination object exists, in the same bucket or in dst_bucket, unless overwrite=True. Both checks run before the request. send_success_email is passed to the endpoint and defaults to False; the data-proxy emails about a failed copy either way. This follows DataproxyFile.rename(). Co-Authored-By: Claude Opus 5.5 --- doc.md | 22 +++++++-- ebrains_drive/files.py | 45 +++++++++++++---- tests/test_harmonization.py | 96 ++++++++++++++++++++++++++++--------- 3 files changed, 129 insertions(+), 34 deletions(-) diff --git a/doc.md b/doc.md index 67facd8c50..cff52ee08e 100644 --- a/doc.md +++ b/doc.md @@ -1077,11 +1077,23 @@ None Server-side copy — no client-side data transfer. Uses the data-proxy's native `PUT /v1/buckets/{name}/{object}/copy` endpoint. -**Request Parameters** (at least one must be supplied) +The data-proxy accepts the copy and completes it in the background, and +reports a copy that fails only by email. So before the request, `copy_to` +checks that the object still exists and, unless `overwrite` is set, that no +object at the destination has the destination name. The destination object +appears when the copy completes. + +**Request Parameters** (at least one of `dst_name` and `dst_bucket` must be supplied) * dst_name — destination object name (within the same bucket, unless `dst_bucket` is also set) * dst_bucket (keyword-only) — destination bucket name +* overwrite (keyword-only, default `False`) — when `False`, raise + `FileExistsError` if the destination object exists. When `True`, the copy + replaces it. +* send_success_email (keyword-only, default `False`) — whether the data-proxy + emails the user when the copy completes. It emails the user about a failed + copy either way. **Sample Case** @@ -1096,17 +1108,19 @@ native `PUT /v1/buckets/{name}/{object}/copy` endpoint. # Copy to a different bucket (same name) f.copy_to(dst_bucket="archive-bucket") - # Copy to a different bucket under a new name - f.copy_to(dst_name="report-archived.csv", dst_bucket="archive-bucket") + # Copy to a different bucket under a new name, replacing an object of that name + f.copy_to(dst_name="report-archived.csv", dst_bucket="archive-bucket", overwrite=True) ``` **Return Type** -`bool` — `True` on success. +`bool` — `True` once the data-proxy has accepted the copy. **Exceptions** * `ValueError` — when neither `dst_name` nor `dst_bucket` is supplied. +* `DoesNotExist` — the object is no longer in the bucket. +* `FileExistsError` — the destination object exists and `overwrite` is `False`. * Unauthorized ### Rename File ### diff --git a/ebrains_drive/files.py b/ebrains_drive/files.py index 6a701196bd..3871330f83 100644 --- a/ebrains_drive/files.py +++ b/ebrains_drive/files.py @@ -428,28 +428,57 @@ def delete(self): assert "has been removed" in json_resp["detail"] @on_401_raise_unauthorized("Unauthorized") - def copy_to(self, dst_name: str = None, *, dst_bucket: str = None): + def copy_to( + self, + dst_name: str = None, + *, + dst_bucket: str = None, + overwrite: bool = False, + send_success_email: bool = False, + ): """Copy this object to another location. Uses the data-proxy's native copy endpoint (``PUT /v1/buckets/{name}/{object}/copy``). Roughly analogous to :meth:`ebrains_drive.files._SeafDirentBase.copy_to`. + The data-proxy accepts the copy and completes it in the background, + and reports a copy that fails only by email. So before the request, + this method checks that the object still exists and, unless + ``overwrite`` is set, that no object at the destination has the + destination name. The destination object appears when the copy + completes. + :param dst_name: destination object name; defaults to the same name (only meaningful when ``dst_bucket`` is set). :param dst_bucket: destination bucket name; defaults to the same bucket (only meaningful when ``dst_name`` is set). - - The data-proxy accepts the copy and completes it in the background, - and reports a copy that fails, such as one of an object that no - longer exists, only by email. The destination object appears when - the copy completes, and replaces an object of that name. - + :param overwrite: when ``False`` (default), raise + :class:`FileExistsError` if the destination object exists. When + ``True``, the copy replaces it. + :param send_success_email: whether the data-proxy emails the user when + the copy completes. It emails the user about a failed copy either + way. :returns: ``True`` once the data-proxy has accepted the copy. + :raises DoesNotExist: if this object is no longer in the bucket. + :raises FileExistsError: if ``overwrite`` is ``False`` and the + destination object exists. """ if dst_name is None and dst_bucket is None: raise ValueError("copy_to requires at least one of dst_name or dst_bucket") - params = {} + self.bucket.get_file(self.name) + if not overwrite: + target_bucket = self.bucket if dst_bucket is None else self.client.buckets.get_bucket(dst_bucket) + target_name = self.name if dst_name is None else dst_name + try: + target_bucket.get_file(target_name) + except DoesNotExist: + pass + else: + raise FileExistsError( + f"Object with name = `{target_name}` already exists in bucket `{target_bucket.name}`." + ) + params = {"send_success_email": str(send_success_email).lower()} if dst_bucket is not None: params["to"] = dst_bucket if dst_name is not None: diff --git a/tests/test_harmonization.py b/tests/test_harmonization.py index 01dc0eced1..87977471f4 100644 --- a/tests/test_harmonization.py +++ b/tests/test_harmonization.py @@ -312,15 +312,6 @@ def test_bucket_dir_upload_qualifies_filename(mock_client): # --------------------------- copy_to -------------------------------- # -def test_dataproxy_file_copy_to_calls_native_endpoint(mock_client): - bucket = Bucket.from_json(mock_client, bucket_json) - f = DataproxyFile(mock_client, bucket, hash="h", last_modified="n", bytes=1, name="src", content_type="t") - mock_client.put.return_value = MockResp({}, status_code=200) - ok = f.copy_to(dst_name="dst") - assert ok is True - mock_client.put.assert_called_with("/v1/buckets/foo/src/copy", params={"name": "dst"}, expected=(200, 201)) - - def _bucket_client_answering(status_code): """A BucketApiClient whose HTTP session answers every request with ``status_code``. @@ -333,37 +324,98 @@ def _bucket_client_answering(status_code): return client +def _copyable_file(client, existing, other_bucket_existing=()): + """A DataproxyFile named "src" in bucket "foo", which holds the objects in ``existing``. + + Bucket "otherbucket" holds the objects in ``other_bucket_existing``. + """ + + def bucket_holding(fields, names): + bucket = Bucket.from_json(client, fields) + + def get_file(name): + if name not in names: + raise DoesNotExist(f"Cannot find {name}.") + return MagicMock(name=name) + + bucket.get_file = MagicMock(side_effect=get_file) + return bucket + + bucket = bucket_holding(bucket_json, set(existing)) + other = bucket_holding({**bucket_json, "name": "otherbucket"}, set(other_bucket_existing)) + client.buckets = MagicMock() + client.buckets.get_bucket.side_effect = lambda name: {"otherbucket": other}[name] + return DataproxyFile(client, bucket, hash="h", last_modified="n", bytes=1, name="src", content_type="t") + + +def test_dataproxy_file_copy_to_calls_native_endpoint(mock_client): + f = _copyable_file(mock_client, {"src"}) + assert f.copy_to(dst_name="dst") is True + mock_client.put.assert_called_once_with( + "/v1/buckets/foo/src/copy", + params={"send_success_email": "false", "name": "dst"}, + expected=(200, 201), + ) + + @pytest.mark.parametrize("status_code", [200, 201]) def test_dataproxy_file_copy_to_accepts_the_statuses_of_the_endpoint(status_code): - client = _bucket_client_answering(status_code) - bucket = Bucket.from_json(client, bucket_json) - f = DataproxyFile(client, bucket, hash="h", last_modified="n", bytes=1, name="src", content_type="t") + f = _copyable_file(_bucket_client_answering(status_code), {"src"}) assert f.copy_to(dst_name="dst") is True def test_dataproxy_file_copy_to_raises_for_another_status(): - client = _bucket_client_answering(403) - bucket = Bucket.from_json(client, bucket_json) - f = DataproxyFile(client, bucket, hash="h", last_modified="n", bytes=1, name="src", content_type="t") + f = _copyable_file(_bucket_client_answering(403), {"src"}) with pytest.raises(ClientHttpError): f.copy_to(dst_name="dst") def test_dataproxy_file_copy_to_other_bucket(mock_client): - bucket = Bucket.from_json(mock_client, bucket_json) - f = DataproxyFile(mock_client, bucket, hash="h", last_modified="n", bytes=1, name="src", content_type="t") - mock_client.put.return_value = MockResp({}, status_code=200) - f.copy_to(dst_bucket="otherbucket") - mock_client.put.assert_called_with("/v1/buckets/foo/src/copy", params={"to": "otherbucket"}, expected=(200, 201)) + f = _copyable_file(mock_client, {"src"}) + f.copy_to(dst_bucket="otherbucket", send_success_email=True) + mock_client.put.assert_called_once_with( + "/v1/buckets/foo/src/copy", + params={"send_success_email": "true", "to": "otherbucket"}, + expected=(200, 201), + ) def test_dataproxy_file_copy_to_requires_argument(mock_client): - bucket = Bucket.from_json(mock_client, bucket_json) - f = DataproxyFile(mock_client, bucket, hash="h", last_modified="n", bytes=1, name="src", content_type="t") + f = _copyable_file(mock_client, {"src"}) with pytest.raises(ValueError): f.copy_to() +def test_dataproxy_file_copy_of_a_missing_object_raises_before_the_request(mock_client): + f = _copyable_file(mock_client, set()) + with pytest.raises(DoesNotExist): + f.copy_to(dst_name="dst") + mock_client.put.assert_not_called() + + +@pytest.mark.parametrize( + "existing,other_bucket_existing,kwargs", + [ + pytest.param({"src", "dst"}, (), {"dst_name": "dst"}, id="same-bucket"), + pytest.param({"src"}, {"src"}, {"dst_bucket": "otherbucket"}, id="other-bucket-same-name"), + pytest.param({"src"}, {"dst"}, {"dst_name": "dst", "dst_bucket": "otherbucket"}, id="other-bucket-new-name"), + ], +) +def test_dataproxy_file_copy_onto_an_existing_object_raises_before_the_request( + mock_client, existing, other_bucket_existing, kwargs +): + f = _copyable_file(mock_client, existing, other_bucket_existing) + with pytest.raises(FileExistsError): + f.copy_to(**kwargs) + mock_client.put.assert_not_called() + + +def test_dataproxy_file_copy_to_with_overwrite_replaces_an_existing_object(mock_client): + f = _copyable_file(mock_client, {"src", "dst"}) + assert f.copy_to(dst_name="dst", overwrite=True) is True + mock_client.put.assert_called_once() + + # --------------------------- rename --------------------------------- # From 612d5785156356ae44e82b6534a9ca54caf76bfb Mon Sep 17 00:00:00 2001 From: ehennestad Date: Thu, 8 Oct 2026 20:57:00 +0200 Subject: [PATCH 2/2] Test the source check of copy_to with overwrite, and return plain stubs The source check runs whether or not overwrite is set, but only the case without overwrite was tested, so moving the check under "if not overwrite" passed every test. The missing-source test now also runs with the destination taken, and with overwrite=True. The stand-in for Bucket.get_file returned MagicMock(name=name), which names the mock rather than setting its name attribute; it now returns a SimpleNamespace with that name. Co-Authored-By: Claude Opus 5.5 --- tests/test_harmonization.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/test_harmonization.py b/tests/test_harmonization.py index 87977471f4..040f1f2382 100644 --- a/tests/test_harmonization.py +++ b/tests/test_harmonization.py @@ -336,7 +336,7 @@ def bucket_holding(fields, names): def get_file(name): if name not in names: raise DoesNotExist(f"Cannot find {name}.") - return MagicMock(name=name) + return SimpleNamespace(name=name) bucket.get_file = MagicMock(side_effect=get_file) return bucket @@ -386,10 +386,18 @@ def test_dataproxy_file_copy_to_requires_argument(mock_client): f.copy_to() -def test_dataproxy_file_copy_of_a_missing_object_raises_before_the_request(mock_client): - f = _copyable_file(mock_client, set()) +@pytest.mark.parametrize( + "existing,kwargs", + [ + pytest.param(set(), {"dst_name": "dst"}, id="destination-free"), + pytest.param({"dst"}, {"dst_name": "dst"}, id="destination-taken"), + pytest.param({"dst"}, {"dst_name": "dst", "overwrite": True}, id="overwrite"), + ], +) +def test_dataproxy_file_copy_of_a_missing_object_raises_before_the_request(mock_client, existing, kwargs): + f = _copyable_file(mock_client, existing) with pytest.raises(DoesNotExist): - f.copy_to(dst_name="dst") + f.copy_to(**kwargs) mock_client.put.assert_not_called()