From 811b09380a1880f952a8c54dd81189a7864688da Mon Sep 17 00:00:00 2001 From: jgjesdal Date: Mon, 28 Sep 2026 15:24:15 +0200 Subject: [PATCH 1/2] fix(files)!: follow the api's verbatim file external ids and deletedAt trash The api now stores a file's external id as sent (charset-checked, looked up case-insensitively) and soft-deletes by stamping deletedAt instead of rewriting the external id to a DELETED_ tombstone. - INode gains deleted_at (Python: INode.deleted_at). - FileUpload::new defaults the external id to the file name verbatim, the server's own default, rather than a snake-lowercased slug. A name outside [A-Za-z0-9._:+=-] needs an explicit external id or is a 400. - restore by external id works; the docs no longer steer callers to ids. - The lifecycle tests assert the preserved external id, deletedAt and a restore by external id; new tests pin the default id and the 400. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: jgjesdal --- .../intellistream_datahub_sdk/__init__.pyi | 2 + .../src/files/async_service.rs | 5 +- datahub_python_bindings/src/files/mod.rs | 5 ++ .../src/files/sync_service.rs | 5 +- python_tests/test_files.py | 11 ++-- src/files/mod.rs | 24 ++++----- src/files/test.rs | 54 ++++++++++++++----- src/generic.rs | 3 ++ 8 files changed, 73 insertions(+), 36 deletions(-) diff --git a/datahub_python_bindings/python/intellistream_datahub_sdk/__init__.pyi b/datahub_python_bindings/python/intellistream_datahub_sdk/__init__.pyi index 73a8436..7a233fa 100644 --- a/datahub_python_bindings/python/intellistream_datahub_sdk/__init__.pyi +++ b/datahub_python_bindings/python/intellistream_datahub_sdk/__init__.pyi @@ -2056,6 +2056,8 @@ class INode: @property def last_updated(self) -> datetime.datetime: ... @property + def deleted_at(self) -> datetime.datetime | None: ... + @property def parent_id(self) -> int | None: ... @property def parent_external_id(self) -> str | None: ... diff --git a/datahub_python_bindings/src/files/async_service.rs b/datahub_python_bindings/src/files/async_service.rs index 06c884c..b1f94bd 100644 --- a/datahub_python_bindings/src/files/async_service.rs +++ b/datahub_python_bindings/src/files/async_service.rs @@ -165,9 +165,8 @@ impl PyFilesServiceAsync { }) } - /// Restore soft-deleted files. Identify each by numeric id: the trashed - /// `DELETED_..._` external id does not round-trip through the server's - /// lowercasing hash, so that route answers 404. See `FileService::restore` in the SDK. + /// Restore soft-deleted files, by id or external id. By external id the most recently + /// deleted copy comes back. See `FileService::restore` in the SDK. fn restore<'py>( &self, py: Python<'py>, diff --git a/datahub_python_bindings/src/files/mod.rs b/datahub_python_bindings/src/files/mod.rs index f4cfc39..f38b689 100644 --- a/datahub_python_bindings/src/files/mod.rs +++ b/datahub_python_bindings/src/files/mod.rs @@ -121,6 +121,7 @@ impl PyINode { source_last_updated, date_created: DateTime::default(), last_updated: DateTime::default(), + deleted_at: None, parent_id, parent_external_id, data_set_id, @@ -189,6 +190,10 @@ impl PyINode { self.inner.last_updated } #[getter] + pub fn deleted_at(&self) -> Option> { + self.inner.deleted_at + } + #[getter] pub fn parent_id(&self) -> Option { self.inner.parent_id } diff --git a/datahub_python_bindings/src/files/sync_service.rs b/datahub_python_bindings/src/files/sync_service.rs index d4bdaf3..0b67ee9 100644 --- a/datahub_python_bindings/src/files/sync_service.rs +++ b/datahub_python_bindings/src/files/sync_service.rs @@ -151,9 +151,8 @@ impl PyFilesServiceSync { }) } - /// Restore soft-deleted files. Identify each by numeric id: the trashed - /// `DELETED_..._` external id does not round-trip through the server's - /// lowercasing hash, so that route answers 404. See `FileService::restore` in the SDK. + /// Restore soft-deleted files, by id or external id. By external id the most recently + /// deleted copy comes back. See `FileService::restore` in the SDK. fn restore<'py>( &self, py: Python<'py>, diff --git a/python_tests/test_files.py b/python_tests/test_files.py index 3e2748c..ee5c3cf 100644 --- a/python_tests/test_files.py +++ b/python_tests/test_files.py @@ -139,13 +139,14 @@ def test_get_search_update_download_trash_restore(sync_client, tmp_path): trashed = [n for n in sync_client.files.list_trash() if n.id == node_id] assert trashed, "the deleted file should be in the trash" - assert trashed[0].external_id.startswith("DELETED_") + assert trashed[0].external_id == ext_id + assert trashed[0].deleted_at is not None - # Restore by numeric id: the trashed `DELETED_...` external id does not round-trip - # through the server's lowercasing hash. See the Rust test for the detail. - restored = sync_client.files.restore([node_id]) + restored = sync_client.files.restore([ext_id]) assert restored[0].id == node_id - assert sync_client.files.get_by_id(node_id)[0].external_id == ext_id + after = sync_client.files.get_by_id(node_id)[0] + assert after.external_id == ext_id + assert after.deleted_at is None finally: for name in leaked: try: diff --git a/src/files/mod.rs b/src/files/mod.rs index 810d0fa..ee5c25f 100644 --- a/src/files/mod.rs +++ b/src/files/mod.rs @@ -1,6 +1,5 @@ mod test; -use crate::datahub::to_snake_lower_cased_allow_start_with_digits; use crate::generic::{ApiServiceProvider, DataWrapper, INode, IdAndExtId}; use crate::http::ResponseError; use crate::ApiService; @@ -107,10 +106,8 @@ impl FileService { /// `GET /files/trash` — the soft-deleted files the caller can read. /// - /// Folders are never listed: only files are soft-deleted. `name` and `path` are the - /// pre-deletion values, while the `external_id` has been rewritten to - /// `DELETED___`. Use the `id` to [`restore`](Self::restore) — - /// see there for why the rewritten external id does not round-trip. + /// Folders are never listed, since only files can be restored. `external_id`, `name` and + /// `path` are the pre-deletion values, and `deleted_at` says when the file was deleted. pub async fn list_trash(&self) -> Result, ResponseError> { let full_path = format!("{}/trash", self.base_url.as_str()); self.execute_get_request(full_path.as_str(), None::<&str>) @@ -120,11 +117,9 @@ impl FileService { /// `POST /files/restore` — move soft-deleted files out of the trash back to their original /// location. /// - /// **Identify each file by numeric id.** The external-id route does not currently work for - /// trashed files: the server hashes the supplied id through `ExternalIds.hash`, which - /// lowercases, while the stored hash for a `DELETED___` id was not - /// lowercased — so the lookup misses and the call answers 404. That is a server-side bug; the - /// id route sidesteps the hash entirely. + /// Identify each file by id or external id. A deleted file keeps its external id, so several + /// deleted copies may share one: by external id the most recently deleted copy comes back, and + /// an older one needs its id. /// /// The call never overwrites: if a file's original path or external id is taken, or its /// original folder is gone, the whole request is refused with 409 and nothing is restored. @@ -379,6 +374,11 @@ impl FileUpload { Ok(f) } + /// The external id defaults to the file name as-is, the same default the server applies. The + /// server stores it verbatim and accepts only letters, digits and `. _ : + = -`, so a file name + /// with a space or other character outside that set needs + /// [`set_external_id`](Self::set_external_id), or the upload is a 400 naming `externalId`. + /// /// Fails with the underlying `io::Error` when `file_path` cannot be read, and with /// `ErrorKind::IsADirectory` or `InvalidInput` when it is not a regular file. pub fn new(file_path: &str) -> io::Result { @@ -435,7 +435,7 @@ impl FileUpload { }; Ok(Self { - external_id: to_snake_lower_cased_allow_start_with_digits(file_name.as_str()), + external_id: file_name.clone(), file_path: file_path.to_string(), destination_path: None, name: file_name, @@ -461,7 +461,7 @@ impl FileUpload { /// Builds the `X-Datahub-*` and `Content-Type` headers the upload endpoint reads before it /// touches the body. Every value is percent-encoded the way the server decodes it (the path /// segment-by-segment, everything else with `URLDecoder.decode` — including the external id, - /// which the server then slug-sanitizes). `metadata` and `relatedResources` go as + /// which the server stores verbatim). `metadata` and `relatedResources` go as /// percent-encoded JSON, and the two source dates as percent-encoded ISO-8601 (RFC 3339). An /// omitted/octet-stream content type makes the server auto-detect the MIME type. pub fn upload_headers(&self) -> Vec<(&'static str, String)> { diff --git a/src/files/test.rs b/src/files/test.rs index cbc18b2..1ca65d0 100644 --- a/src/files/test.rs +++ b/src/files/test.rs @@ -290,7 +290,7 @@ mod tests { let id_collection = DataWrapper::from_vec(vec![ IdAndExtId::from_external_id("datahub_folder_foo"), IdAndExtId::from_external_id("datahub_folder_bar"), - IdAndExtId::from_external_id("random_values_csv"), + IdAndExtId::from_external_id("random_values.csv"), IdAndExtId::from_external_id("datahub_folder_images"), IdAndExtId::from_external_id("image_sola_jpg"), IdAndExtId::from_external_id("datahub_folder_insects"), @@ -385,7 +385,8 @@ mod tests { async fn file_lifecycle_get_search_update_download_trash_restore( ) -> Result<(), Box> { let api_service = create_api_service(); - let ext_id = "lifecycle_sola_jpg"; + // Mixed case and punctuation: stored verbatim, looked up case-insensitively. + let ext_id = "Lifecycle-Sola.JPG"; // Start from a clean slate; the file may be left over from a failed run. let _ = api_service @@ -415,6 +416,12 @@ mod tests { let by_ext = api_service.files.get_by_external_id(ext_id).await?; assert_eq!(by_ext.get_items()[0].id, Some(id)); + let by_other_case = api_service + .files + .get_by_external_id(&ext_id.to_lowercase()) + .await?; + assert_eq!(by_other_case.get_items()[0].id, Some(id)); + assert_eq!(by_other_case.get_items()[0].external_id, ext_id); // --- search --- let found = api_service.files.search("sola").await?; @@ -481,20 +488,14 @@ mod tests { .into_iter() .find(|n| n.id == Some(id)) .expect("the deleted file should be in the trash"); - // The trashed external id is rewritten to DELETED___. - assert!( - trashed.external_id.starts_with("DELETED_"), - "expected a trashed externalId, got {}", - trashed.external_id - ); + assert_eq!(trashed.external_id, ext_id); + assert!(trashed.deleted_at.is_some(), "a trashed file carries deletedAt"); - // Restore by numeric id. The external-id route does not currently work for trashed files: - // the server hashes the supplied id through ExternalIds.hash, which lowercases, while the - // stored hash for a `DELETED_...` id was not lowercased — so the lookup misses and the - // call 404s. Numeric id sidesteps the hash entirely. let restored = api_service .files - .restore(&DataWrapper::from_vec(vec![IdAndExtId::from_id(id)])) + .restore(&DataWrapper::from_vec(vec![IdAndExtId::from_external_id( + ext_id, + )])) .await?; assert_eq!(restored.get_http_status_code().unwrap(), 200); assert_eq!(restored.get_items()[0].id, Some(id)); @@ -502,6 +503,7 @@ mod tests { // Restored under its original external id, so the guard can clean it up. let after = api_service.files.get_by_id(id).await?; assert_eq!(after.get_items()[0].external_id, ext_id); + assert_eq!(after.get_items()[0].deleted_at, None); let _ = api_service .files @@ -516,4 +518,30 @@ mod tests { Ok(()) } + #[test] + fn default_external_id_is_the_file_name() { + let upload = FileUpload::new("resources/test/random_values.csv").unwrap(); + assert_eq!(upload.external_id, "random_values.csv"); + } + + #[tokio::test] + async fn upload_with_an_external_id_outside_the_charset_is_a_400() { + let api_service = create_api_service(); + let mut upload = + FileUpload::new_with_destination_path("resources/test/image.jpg", "/charset").unwrap(); + upload.set_external_id("has space.jpg".to_string()); + + let err = api_service + .files + .upload_file(upload) + .await + .expect_err("a space is outside the external id charset"); + assert_eq!(err.status.as_u16(), 400); + let problem = err.problem().expect("a problem document"); + assert!( + problem.fields().iter().any(|f| f.field.as_deref() == Some("externalId")), + "the problem should name externalId, got {:?}", + problem.fields() + ); + } } diff --git a/src/generic.rs b/src/generic.rs index 88c7354..c3a5d81 100644 --- a/src/generic.rs +++ b/src/generic.rs @@ -1122,6 +1122,9 @@ pub struct INode { pub date_created: DateTime, #[serde(rename = "lastUpdated")] pub last_updated: DateTime, + /// Set only on a file listed by [`list_trash`](crate::files::FileService::list_trash). + #[serde(rename = "deletedAt")] + pub deleted_at: Option>, #[serde(rename = "parentId")] #[serde(default, with = "crate::serde_helper::opt_string_id_i64")] pub parent_id: Option, From 3aafb33035a004b5db88495052f56f57ccf44cac Mon Sep 17 00:00:00 2001 From: jgjesdal Date: Mon, 28 Sep 2026 15:31:57 +0200 Subject: [PATCH 2/2] test(python): pin verbatim file external ids and trash restore Default external id is the file name; ids are stored verbatim and looked up case-insensitively; a charset violation is a 400 naming externalId; restore by external id takes the most recently deleted copy; and the async client's trash and restore round-trip with deleted_at. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: jgjesdal --- python_tests/test_files.py | 99 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 95 insertions(+), 4 deletions(-) diff --git a/python_tests/test_files.py b/python_tests/test_files.py index ee5c3cf..52ffcff 100644 --- a/python_tests/test_files.py +++ b/python_tests/test_files.py @@ -1,9 +1,8 @@ """Tests for the Python files module. -Mirrors `src/files/test.rs` (`test_file_upload`, `list_folders`). The clients now -expose the full `FilesServiceSync` / `FilesServiceAsync` with `upload_file`, -`list_root_directory`, and `list_directory_by_path`. The SDK uploads a single -`FileUpload` per call and echoes back the server-assigned metadata as a list. +Mirrors `src/files/test.rs`. The SDK uploads a single `FileUpload` per call and +echoes back the server-assigned metadata as a list. File external ids are stored +verbatim and looked up case-insensitively; a deleted file keeps its external id. """ import os @@ -155,6 +154,98 @@ def test_get_search_update_download_trash_restore(sync_client, tmp_path): pass +def _delete_quietly(sync_client, *names): + for name in names: + try: + sync_client.files.delete([name]) + except Exception: + pass + + +def test_default_external_id_is_the_file_name(): + # The server's own default when no external id is sent, and stored verbatim like it. + upload = intellistream_datahub_sdk.FileUpload(_IMAGE_PATH) + assert upload.external_id == "image.jpg" + + +def test_external_id_is_stored_verbatim_and_matched_case_insensitively(sync_client): + ext_id = unique_id("file") + "-Sola.JPG" + folder = "datahub_folder_pyverbatim" + upload = intellistream_datahub_sdk.FileUpload( + path=_IMAGE_PATH, destination_path="/pyverbatim/", external_id=ext_id, name=ext_id + ) + try: + node = sync_client.files.upload_file(upload)[0] + assert node.external_id == ext_id + + for spelling in (ext_id, ext_id.lower(), ext_id.upper()): + found = sync_client.files.get_by_external_id(spelling) + assert found[0].id == node.id, spelling + assert found[0].external_id == ext_id + finally: + _delete_quietly(sync_client, ext_id, folder) + + +def test_external_id_outside_the_charset_is_a_400(sync_client): + upload = intellistream_datahub_sdk.FileUpload( + path=_IMAGE_PATH, destination_path="/pycharset/", external_id="has space.jpg" + ) + with pytest.raises(intellistream_datahub_sdk.DataHubException) as err: + sync_client.files.upload_file(upload) + assert err.value.status_code == 400 + fields = (err.value.problem or {}).get("fields", []) + assert any(f.get("field") == "externalId" for f in fields), err.value.message + + +def test_restore_by_external_id_takes_the_most_recently_deleted_copy(sync_client): + # A deleted file keeps its external id, so the trash can hold several copies of one. + ext_id = unique_id("file_restore") + folder = "datahub_folder_pyrestore" + ids = [] + try: + for _ in range(2): + upload = intellistream_datahub_sdk.FileUpload( + path=_IMAGE_PATH, destination_path="/pyrestore/", external_id=ext_id, name=f"{ext_id}.jpg" + ) + ids.append(sync_client.files.upload_file(upload)[0].id) + sync_client.files.delete([ext_id]) + + trashed = {n.id: n for n in sync_client.files.list_trash() if n.id in ids} + assert set(trashed) == set(ids) + assert all(n.external_id == ext_id for n in trashed.values()) + assert trashed[ids[0]].deleted_at <= trashed[ids[1]].deleted_at + + restored = sync_client.files.restore([ext_id]) + assert [n.id for n in restored] == [ids[1]] + assert sync_client.files.get_by_external_id(ext_id)[0].id == ids[1] + assert ids[0] in {n.id for n in sync_client.files.list_trash()} + finally: + _delete_quietly(sync_client, ext_id, folder) + + +@pytest.mark.asyncio +async def test_async_trash_and_restore_by_external_id(async_client, sync_client): + ext_id = unique_id("file_async") + folder = "datahub_folder_pyasynctrash" + upload = intellistream_datahub_sdk.FileUpload( + path=_IMAGE_PATH, destination_path="/pyasynctrash/", external_id=ext_id, name=f"{ext_id}.jpg" + ) + try: + node = (await async_client.files.upload_file(upload))[0] + assert node.deleted_at is None + + await async_client.files.delete([ext_id]) + trashed = [n for n in await async_client.files.list_trash() if n.id == node.id] + assert trashed and trashed[0].external_id == ext_id + assert trashed[0].deleted_at is not None + + restored = await async_client.files.restore([ext_id]) + assert restored[0].id == node.id + assert (await async_client.files.get_by_id(node.id))[0].deleted_at is None + finally: + _delete_quietly(sync_client, ext_id, folder) + + def test_file_update_requires_a_selector(): with pytest.raises(ValueError): intellistream_datahub_sdk.FileUpdate(name="renamed.jpg")