fix(files)!: follow the api's verbatim file external ids and deletedAt trash

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: ...

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_..._<epochMillis>` 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>,

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<DateTime<Utc>> {
+        self.inner.deleted_at
+    }
+    #[getter]
     pub fn parent_id(&self) -> Option<i64> {
         self.inner.parent_id
     }

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_..._<epochMillis>` 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>,

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
 
@@ -139,13 +138,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:
@@ -154,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")

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_<checksum>_<id>_<epochMillis>`. 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<DataWrapper<INode>, 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_<checksum>_<id>_<epochMillis>` 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<Self> {
@@ -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)> {

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<dyn std::error::Error>> {
         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,27 +488,22 @@ 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_<checksum>_<id>_<epochMillis>.
-        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));
 
         // 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()
+        );
+    }
 }

src/generic.rs

@@ -1122,6 +1122,9 @@ pub struct INode {
     pub date_created: DateTime<Utc>,
     #[serde(rename = "lastUpdated")]
     pub last_updated: DateTime<Utc>,
+    /// Set only on a file listed by [`list_trash`](crate::files::FileService::list_trash).
+    #[serde(rename = "deletedAt")]
+    pub deleted_at: Option<DateTime<Utc>>,
     #[serde(rename = "parentId")]
     #[serde(default, with = "crate::serde_helper::opt_string_id_i64")]
     pub parent_id: Option<i64>,