Fix bug in dump_path (#1346)

normanrz · web-flow · commit 456679a0582b · 2025-08-11T08:41:07.000Z
* add test to repro dump_path bug

* fixes bug in dump_path and prefix dataset names

* changelog

* clean

* refactor

* tests

* more tests

* rename resolve_if_fs to cheap_resolve
diff --git a/webknossos/Changelog.md b/webknossos/Changelog.md
@@ -19,6 +19,7 @@ For upgrade instructions, please check the respective _Breaking Changes_ section
 ### Changed
 
 ### Fixed
+- Fixed a bug where paths of references mags were wrong, if the target dataset's path was a prefix of the source dataset's path. [#1346](https://github.com/scalableminds/webknossos-libs/pull/1346)
 
 
 ## [2.4.8](https://github.com/scalableminds/webknossos-libs/releases/tag/v2.4.8) - 2025-08-06
diff --git a/webknossos/tests/dataset/test_dataset.py b/webknossos/tests/dataset/test_dataset.py
@@ -1815,7 +1815,10 @@ def test_writing_subset_of_chunked_compressed_data(
 
 
 @pytest.mark.parametrize("data_format,output_path", DATA_FORMATS_AND_OUTPUT_PATHS)
-def test_add_layer_as_ref(data_format: DataFormat, output_path: UPath) -> None:
+@pytest.mark.parametrize("as_object", [True, False])
+def test_add_layer_as_ref(
+    data_format: DataFormat, output_path: UPath, as_object: bool
+) -> None:
     ds_path = copy_simple_dataset(data_format, output_path, "original")
     new_path = prepare_dataset_path(data_format, output_path, "with_refs")
 
@@ -1832,10 +1835,17 @@ def test_add_layer_as_ref(data_format: DataFormat, output_path: UPath) -> None:
 
     ds = Dataset(new_path, voxel_size=(1, 1, 1))
     # add color layer
-    new_layer = ds.add_layer_as_ref(ds_path / "color")
+    new_layer = ds.add_layer_as_ref(
+        original_ds.get_layer("color") if as_object else ds_path / "color"
+    )
     mag = new_layer.get_mag("1")
     # add segmentation layer
-    new_segmentation_layer = ds.add_layer_as_ref(ds_path / "segmentation")
+    new_segmentation_layer = ds.add_layer_as_ref(
+        original_ds.get_layer("segmentation")
+        if as_object
+        else ds_path / "segmentation",
+        new_layer_name="seg",
+    )
 
     color_mag_path = original_mag.path.name
     assert ds._properties.data_layers[0].mags[0].path == dump_path(
@@ -1847,6 +1857,8 @@ def test_add_layer_as_ref(data_format: DataFormat, output_path: UPath) -> None:
     )
     assert not (new_path / "segmentation" / "1").exists()
     assert not (new_path / "segmentation").exists()
+    assert not (new_path / "seg" / "1").exists()
+    assert not (new_path / "seg").exists()
 
     assert len(ds.layers) == 2
     assert len(ds.get_layer("color").mags) == 1
@@ -1870,6 +1882,25 @@ def test_add_layer_as_ref(data_format: DataFormat, output_path: UPath) -> None:
     assure_exported_properties(ds)
 
 
+@pytest.mark.parametrize("output_path", OUTPUT_PATHS)
+def test_add_layer_as_ref_prefix(output_path: UPath) -> None:
+    source = Dataset(output_path / "name_with_suffix", (1, 1, 1))
+    source.add_layer(
+        "consensus", SEGMENTATION_CATEGORY, dtype_per_channel="uint8"
+    ).add_mag(1)
+
+    target = Dataset(output_path / "name", (1, 1, 1))
+    target.add_layer("raw", COLOR_CATEGORY, dtype_per_channel="uint8").add_mag(1)
+
+    glom = source.get_layer("consensus")
+    target.add_layer_as_ref(foreign_layer=glom, new_layer_name="glomeruli")
+
+    assert target._properties.data_layers[1].mags[0].path == dump_path(
+        source.get_layer("consensus").get_mag(1).path,
+        UPath.home() / "random",  # an unrelated path
+    )
+
+
 @pytest.mark.parametrize("data_format,output_path", DATA_FORMATS_AND_OUTPUT_PATHS)
 def test_ref_layer_add_mag(data_format: DataFormat, output_path: UPath) -> None:
     ds_path = copy_simple_dataset(data_format, output_path, "original")
diff --git a/webknossos/tests/test_utils.py b/webknossos/tests/test_utils.py
@@ -1,8 +1,10 @@
+from pathlib import Path
 from unittest.mock import Mock, patch
 
 import pytest
+from upath import UPath
 
-from webknossos.utils import call_with_retries
+from webknossos.utils import call_with_retries, dump_path
 
 
 def test_call_with_retries_success() -> None:
@@ -68,3 +70,122 @@ def test_call_with_retries_direct_failure(mock_sleep: Mock) -> None:
 
     assert mock_fn.call_count == 1  # Only called once, no retries
     assert mock_sleep.call_count == 0  # Sleep not called since it failed immediately
+
+
+def test_dump_path(tmp_path: Path) -> None:
+    tmp_path = UPath(tmp_path.resolve())
+
+    # relative enclosed
+    dataset_path = tmp_path / "test_dataset"
+    path = dataset_path / "test.txt"
+    assert dump_path(path, dataset_path) == "./test.txt"
+
+    # relative not enclosed
+    dataset_path = tmp_path / "test_dataset"
+    path = dataset_path / ".." / "test_dataset2" / "test.txt"
+    assert (
+        dump_path(path, dataset_path) == f"{tmp_path.as_posix()}/test_dataset2/test.txt"
+    )
+
+    # dataset path with ..
+    dataset_path = tmp_path / "test_dataset" / ".." / "test_dataset2"
+    path = tmp_path / "test_dataset" / "test.txt"
+    assert (
+        dump_path(path, dataset_path) == f"{tmp_path.as_posix()}/test_dataset/test.txt"
+    )
+
+    dataset_path = tmp_path / "test_dataset" / ".." / "test_dataset2"
+    path = tmp_path / "test_dataset2" / "test.txt"
+    assert dump_path(path, dataset_path) == "./test.txt"
+
+    # dataset path is a prefix
+    dataset_path = tmp_path / "test_dataset"
+    path = tmp_path / "test_dataset_longer" / "test.txt"
+    assert (
+        dump_path(path, dataset_path)
+        == f"{tmp_path.as_posix()}/test_dataset_longer/test.txt"
+    )
+
+    # s3 non relative
+    dataset_path = tmp_path / "test_dataset"
+    path = UPath(
+        "s3://bucket/test.txt",
+        client_kwargs={"endpoint_url": "https://s3.amazonaws.com"},
+    )
+    assert dump_path(path, dataset_path) == "s3://s3.amazonaws.com/bucket/test.txt"
+
+    # s3 relative
+    dataset_path = UPath(
+        "s3://bucket/test_dataset",
+        client_kwargs={"endpoint_url": "https://s3.amazonaws.com"},
+    )
+    path = dataset_path / "test.txt"
+    assert dump_path(path, dataset_path) == "./test.txt"
+
+    # s3 dataset path is a prefix
+    dataset_path = UPath(
+        "s3://bucket/test_dataset",
+        client_kwargs={"endpoint_url": "https://s3.amazonaws.com"},
+    )
+    path = (
+        UPath(
+            "s3://bucket/test_dataset_longer",
+            client_kwargs={"endpoint_url": "https://s3.amazonaws.com"},
+        )
+        / "test.txt"
+    )
+    assert (
+        dump_path(path, dataset_path)
+        == "s3://s3.amazonaws.com/bucket/test_dataset_longer/test.txt"
+    )
+
+    # s3 with ..
+    dataset_path = UPath(
+        "s3://bucket/test_dataset",
+        client_kwargs={"endpoint_url": "https://s3.amazonaws.com"},
+    )
+    path = dataset_path / ".." / "test_dataset2" / "test.txt"
+    assert (
+        dump_path(path, dataset_path)
+        == "s3://s3.amazonaws.com/bucket/test_dataset2/test.txt"
+    )
+
+    path = dataset_path / ".." / "test_dataset2" / "test.txt"
+    assert (
+        dump_path(path, dataset_path)
+        == "s3://s3.amazonaws.com/bucket/test_dataset2/test.txt"
+    )
+
+    path = dataset_path / ".." / "test_dataset" / "test.txt"
+    assert dump_path(path, dataset_path) == "./test.txt"
+
+    # s3 dataset with ..
+    dataset_path = (
+        UPath(
+            "s3://bucket/test_dataset",
+            client_kwargs={"endpoint_url": "https://s3.amazonaws.com"},
+        )
+        / ".."
+        / "test_dataset2"
+    )
+    path = dataset_path / "test.txt"
+    assert dump_path(path, dataset_path) == "./test.txt"
+
+    path = UPath(
+        "s3://bucket/test_dataset/test.txt",
+        client_kwargs={"endpoint_url": "https://s3.amazonaws.com"},
+    )
+    assert (
+        dump_path(path, dataset_path)
+        == "s3://s3.amazonaws.com/bucket/test_dataset/test.txt"
+    )
+
+    path = (
+        UPath(
+            "s3://bucket/",
+            client_kwargs={"endpoint_url": "https://s3.amazonaws.com"},
+        )
+        / "test_dataset2"
+        / "test.txt"
+    )
+    assert dump_path(path, dataset_path) == "./test.txt"
diff --git a/webknossos/webknossos/dataset/attachments.py b/webknossos/webknossos/dataset/attachments.py
@@ -9,11 +9,11 @@
 from upath import UPath
 
 from ..utils import (
+    cheap_resolve,
     copytree,
     dump_path,
     enrich_path,
     is_fs_path,
-    resolve_if_fs_path,
     snake_to_camel_case,
     warn_deprecated,
 )
@@ -307,7 +307,7 @@ def add_attachments(self, *other: Attachment) -> None:
 
     def add_attachment_as_ref(self, attachment: Attachment) -> None:
         new_attachment = type(attachment).from_path_and_name(
-            resolve_if_fs_path(attachment.path),
+            cheap_resolve(attachment.path),
             attachment.name,
             data_format=attachment.data_format,
             dataset_path=self._layer.dataset.resolved_path,
@@ -320,7 +320,7 @@ def add_copy_attachments(self, *other: Attachment) -> None:
             self.add_attachment_as_copy(*other)
 
     def add_attachment_as_copy(self, attachment: Attachment) -> None:
-        new_path = resolve_if_fs_path(
+        new_path = cheap_resolve(
             self._layer.path
             / snake_to_camel_case(TYPE_MAPPING[type(attachment)])
             / _maybe_add_suffix(attachment.name, attachment.data_format)
@@ -345,7 +345,7 @@ def add_symlink_attachments(
             stacklevel=2,
         )
         for attachment in other:
-            new_path = resolve_if_fs_path(attachment.path)
+            new_path = cheap_resolve(attachment.path)
             if is_fs_path(attachment.path):
                 new_path = (
                     self._layer.resolved_path
diff --git a/webknossos/webknossos/dataset/dataset.py b/webknossos/webknossos/dataset/dataset.py
@@ -66,14 +66,14 @@
     from ..client._upload_dataset import LayerToLink
 
 from ..utils import (
+    cheap_resolve,
     copytree,
     count_defined_values,
     dump_path,
     get_executor_for_args,
     infer_metadata_type,
     is_fs_path,
     named_partial,
-    resolve_if_fs_path,
     rmtree,
     strip_trailing_slash,
     wait_and_ensure_success,
@@ -323,7 +323,7 @@ def __init__(
 
         self._read_only = read_only
         self.path: UPath = path
-        self._resolved_path: UPath = resolve_if_fs_path(path)
+        self._resolved_path: UPath = cheap_resolve(path)
 
         if count_defined_values((voxel_size, voxel_size_with_unit)) > 1:
             raise ValueError(
@@ -456,7 +456,7 @@ def open(cls, dataset_path: str | PathLike, read_only: bool = False) -> "Dataset
         dataset = cls.__new__(cls)
         dataset.path = dataset_path
         dataset._read_only = read_only
-        dataset._resolved_path = resolve_if_fs_path(dataset_path)
+        dataset._resolved_path = cheap_resolve(dataset_path)
         return dataset._init_from_properties(dataset_properties)
 
     @property
@@ -1626,7 +1626,7 @@ def add_layer_for_existing_files(
             except ValueError:
                 continue
             # Mags are only writable if they are local to the dataset
-            resolved_mag_path = resolve_if_fs_path(mag_dir)
+            resolved_mag_path = cheap_resolve(mag_dir)
             read_only = resolved_mag_path.parent != self.resolved_path / layer_name
             layer._add_mag_for_existing_files(
                 mag_dir.name, mag_path=resolved_mag_path, read_only=read_only
diff --git a/webknossos/webknossos/dataset/layer.py b/webknossos/webknossos/dataset/layer.py
@@ -44,14 +44,14 @@
     from .dataset import Dataset
 
 from ..utils import (
+    cheap_resolve,
     copytree,
     dump_path,
     enrich_path,
     get_executor_for_args,
     is_fs_path,
     movetree,
     named_partial,
-    resolve_if_fs_path,
     rmtree,
     warn_deprecated,
 )
@@ -176,7 +176,7 @@ def _get_shard_shape(
 
 
 def _is_foreign_mag(dataset_path: UPath, layer_name: str, mag_path: UPath) -> bool:
-    return dataset_path / layer_name != resolve_if_fs_path(mag_path).parent
+    return dataset_path / layer_name != cheap_resolve(mag_path).parent
 
 
 def _find_mag_path(
@@ -191,9 +191,9 @@ def _find_mag_path(
     short_mag_file_path = layer_path / mag.to_layer_name()
     long_mag_file_path = layer_path / mag.to_long_layer_name()
     if short_mag_file_path.exists():
-        return resolve_if_fs_path(short_mag_file_path)
+        return cheap_resolve(short_mag_file_path)
     elif long_mag_file_path.exists():
-        return resolve_if_fs_path(long_mag_file_path)
+        return cheap_resolve(long_mag_file_path)
     else:
         raise FileNotFoundError(
             f"Could not find any valid mag `{mag}` in `{layer_path}`."
@@ -244,7 +244,7 @@ def __init__(
             properties.element_class, properties.num_channels
         )
         self._mags: dict[Mag, MagView] = {}
-        resolved_path = resolve_if_fs_path(self.dataset.resolved_path / self.name)
+        resolved_path = cheap_resolve(self.dataset.resolved_path / self.name)
         self._resolved_path: UPath = resolved_path
         self._read_only = read_only
 
@@ -355,9 +355,7 @@ def name(self, layer_name: str) -> None:
         if self.path.exists():
             self.path.rename(self.dataset.path / layer_name)
         self._path = self.dataset.path / layer_name
-        self._resolved_path = resolve_if_fs_path(
-            self.dataset.resolved_path / layer_name
-        )
+        self._resolved_path = cheap_resolve(self.dataset.resolved_path / layer_name)
         del self.dataset.layers[self.name]
         self.dataset.layers[layer_name] = self
         self._properties.name = layer_name
@@ -1121,7 +1119,7 @@ def _create_dir_for_mag(self, mag: MagLike) -> UPath:
         mag_name = Mag(mag).to_layer_name()
         full_path = self.resolved_path / mag_name
         full_path.mkdir(parents=True, exist_ok=True)
-        full_path = resolve_if_fs_path(full_path)
+        full_path = cheap_resolve(full_path)
         return full_path
 
     def _assert_mag_does_not_exist_yet(self, mag: MagLike) -> None:
diff --git a/webknossos/webknossos/utils.py b/webknossos/webknossos/utils.py
