From 63564ff5092d22a2d57c418d09716e9816288a02 Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Tue, 25 Aug 2026 14:10:45 +0200 Subject: [PATCH 1/6] Fixes #119 --- src/simdb/remote/apis/files.py | 12 ++++-------- src/simdb/remote/core/path.py | 13 +++++++------ src/simdb/remote/models.py | 13 ++++++++++++- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/simdb/remote/apis/files.py b/src/simdb/remote/apis/files.py index cd7cad5e..7f0e7a56 100644 --- a/src/simdb/remote/apis/files.py +++ b/src/simdb/remote/apis/files.py @@ -58,15 +58,11 @@ def _verify_file( path_value = qs.get("path") if path_value is None: raise ValueError("The 'path' key is missing in the URI query") - if common_root == Path("/"): - path_value = str(staging_dir) + path_value - elif common_root is not None and common_root == path_value: - path_value = path_value.replace(str(common_root), str(staging_dir)) - - else: - path_value = str(staging_dir) + staged_dir = secure_path( + Path(path_value), common_root, staging_dir, is_file=False + ) new_uri = uri.build( - scheme=uri.scheme, path=uri.path, query=f"path={path_value}" + scheme=uri.scheme, path=uri.path, query=f"path={staged_dir.as_posix()}" ) checksum = imas_checksum(new_uri, ids_list or []) if sim_file.checksum != checksum: diff --git a/src/simdb/remote/core/path.py b/src/simdb/remote/core/path.py index bb02508d..a750a1ad 100644 --- a/src/simdb/remote/core/path.py +++ b/src/simdb/remote/core/path.py @@ -9,13 +9,14 @@ def secure_path( path: Path, common_root: Optional[Path], staging_dir: Path, is_file=True ) -> Path: if common_root is None: - directory = staging_dir - else: - directory = staging_dir / path.parent.relative_to(common_root) + return staging_dir / secure_filename(path.name) if is_file else staging_dir if is_file: - return directory / secure_filename(path.name) - else: - return directory + return ( + staging_dir + / path.parent.relative_to(common_root) + / secure_filename(path.name) + ) + return staging_dir / path.relative_to(common_root) def find_common_root(paths: Collection[Path]) -> Optional[Path]: diff --git a/src/simdb/remote/models.py b/src/simdb/remote/models.py index ae9565aa..d09df187 100644 --- a/src/simdb/remote/models.py +++ b/src/simdb/remote/models.py @@ -521,7 +521,7 @@ class FileUploadResponse(BaseModel): class FileRegistrationItem(BaseModel): """A single file entry in the file registration payload.""" - chunks: int + chunks: int = 0 """The amount of chunks to be processed.""" file_type: str """The file type.""" @@ -530,6 +530,17 @@ class FileRegistrationItem(BaseModel): ids_list: Optional[List[Any]] = None """List of IDS names associated with the file.""" + @field_validator("ids_list", mode="before") + @classmethod + def _coerce_ids_list(cls, v: Any) -> Any: + """Accept the display-string form of the IDS list, ``"[a, b, c]"``.""" + if not isinstance(v, str): + return v + text = v.strip() + if text.startswith("[") and text.endswith("]"): + text = text[1:-1] + return [name.strip() for name in text.split(",") if name.strip()] + class FileRegistrationData(BaseModel): """Payload for final file registration after chunk uploads.""" From 5b96ede0dd7003b4ddab5d86e1cacf9c47055c33 Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Tue, 1 Sep 2026 16:07:28 +0200 Subject: [PATCH 2/6] Support lower api versions --- src/simdb/cli/remote_api.py | 83 +++++++++++++++++++++++----- tests/cli/test_remote_api_version.py | 81 ++++++++++++++++++++++++++- 2 files changed, 150 insertions(+), 14 deletions(-) diff --git a/src/simdb/cli/remote_api.py b/src/simdb/cli/remote_api.py index 99f87363..61c94133 100644 --- a/src/simdb/cli/remote_api.py +++ b/src/simdb/cli/remote_api.py @@ -11,6 +11,7 @@ import sys import uuid from collections import defaultdict +from contextlib import contextmanager from io import BytesIO from pathlib import Path from typing import ( @@ -19,7 +20,9 @@ Any, Callable, Dict, + FrozenSet, Iterable, + Iterator, List, Optional, Tuple, @@ -124,10 +127,14 @@ def _push_simulation_v1_2(self, ...): compatible with. This keeps the current protocol as the primary, most-visible code path and confines backwards-compatibility handling to clearly named shims. + A method that does not support the negotiated version sends its requests to the + highest version it does support that the remote provides, so ``@versioned_method + ("v1.2")`` keeps working against a v1.3 remote. + The full set of supported versions is recorded on the method as ``_api_versions``, - and calling the method with a negotiated version that no implementation serves - raises a RemoteError. While no version has been negotiated yet, the default - implementation is used. + and calling the method against a remote that provides none of them raises a + RemoteError. While no version has been negotiated yet, the default implementation + is used. """ def decorator(default: Callable) -> Callable: @@ -136,17 +143,25 @@ def decorator(default: Callable) -> Callable: @functools.wraps(default) def wrapper(self, *args, **kwargs): - selected = getattr(self, "_api_version", None) + selected = getattr(self, "_request_version", None) or getattr( + self, "_api_version", None + ) if selected is None: return default(self, *args, **kwargs) - impl = registry.get(selected) - if impl is None: + + if selected in registry: + return registry[selected](self, *args, **kwargs) + + version = select_api_version(self._server_versions, registry) + if version is None: raise RemoteError( - f"'{default_name}' is not supported by the negotiated API " - f"version '{selected}'. It requires one of: " - f"{', '.join(sorted(registry))}." + f"'{default_name}' requires one of the API versions " + f"{', '.join(sorted(registry))}, none of which is provided by " + f"remote '{self._remote}' (it provides " + f"{', '.join(sorted(self._server_versions))})." ) - return impl(self, *args, **kwargs) + with self.api_version(version): + return registry[version](self, *args, **kwargs) def register(*impl_versions: str) -> Callable: def do_register(func: Callable) -> Callable: @@ -303,7 +318,10 @@ def __init__( if self._firewall is not None: self._load_cookies(remote, username, password) - self._api_url: str = f"{self._url}/" + self._base_url: str = f"{self._url}/" + self._api_version: Optional[str] = None + self._request_version: Optional[str] = None + self._server_versions: FrozenSet[str] = frozenset() self._server_auth = self.get_server_authentication() if self._firewall: self._server_auth = "None" @@ -328,6 +346,7 @@ def __init__( endpoints = self.get_endpoints() endpoint_versions = [endpoint.split("/")[-1] for endpoint in endpoints] + self._server_versions = frozenset(endpoint_versions) selected_version = select_api_version(endpoint_versions) if selected_version is None: @@ -341,7 +360,6 @@ def __init__( print(f"Selected API version {selected_version}") self._api_version = selected_version - self._api_url += f"{selected_version}/" self.version = Version.coerce(self.get_api_version()) self.server_version = Version.coerce(self.get_server_version()) @@ -394,6 +412,42 @@ def _load_cookies( else: raise ValueError(f"Unknown firewall option {self._firewall}") + @property + def _api_url(self) -> str: + """ + Return the base URL of the API version the current request targets. + """ + version = self._request_version or self._api_version + if version is None: + return self._base_url + return f"{self._base_url}{version}/" + + @contextmanager + def api_version(self, version: str) -> Iterator[None]: + """ + Send the requests made inside this context to the given API version. + + @param version: the API version, as it appears in the endpoint URL. + """ + if version not in CLIENT_API_VERSIONS: + raise RemoteError( + f"API version '{version}' is not supported by this client. It " + f"supports: {', '.join(CLIENT_API_VERSIONS)}." + ) + if version not in self._server_versions: + raise RemoteError( + f"API version '{version}' is not provided by remote " + f"'{self._remote}'. It provides: " + f"{', '.join(sorted(self._server_versions)) or 'none'}." + ) + + previous = self._request_version + self._request_version = version + try: + yield + finally: + self._request_version = previous + @property def remote(self) -> str: """ @@ -856,7 +910,7 @@ def _send_chunk( ] self.post("files", data={}, files=files) - @versioned_method("v1.2", "v1.3") + @versioned_method("v1.2") @try_request def push_simulation( self, @@ -870,6 +924,9 @@ def push_simulation( First we upload any files associated with the simulation, then push the simulation metadata. + Only supported on the v1.2 API: the chunked file upload is to be replaced by + a resumable HTTP upload, so it has not been ported to v1.3. + :param simulation: The Simulation to push to remote server :param out_stream: The IO stream to write messages to the user (default: stdout) :param add_watcher: Add the current user as a watcher of the simulation on the diff --git a/tests/cli/test_remote_api_version.py b/tests/cli/test_remote_api_version.py index 08b0b44b..05b4f3f3 100644 --- a/tests/cli/test_remote_api_version.py +++ b/tests/cli/test_remote_api_version.py @@ -1,4 +1,12 @@ -from simdb.cli.remote_api import select_api_version +import io +from unittest import mock + +import pytest + +from simdb.cli.manifest import Manifest +from simdb.cli.remote_api import RemoteAPI, RemoteError, select_api_version +from simdb.config import Config +from simdb.database.models import Simulation def test_selects_highest_common_version(): @@ -16,3 +24,74 @@ def test_no_common_version_returns_none(): def test_versions_compare_semantically_not_lexicographically(): assert select_api_version(["v1.2", "v1.10"], ("v1.2", "v1.10")) == "v1.10" + + +def _remote_api(endpoints=("v1.2", "v1.3")): + config = Config() + config.set_option("remote.test.url", "http://remote.test") + config.set_option("remote.test.token", "123ABC") + + with mock.patch.object( + RemoteAPI, "get_server_authentication", return_value="None" + ), mock.patch.object( + RemoteAPI, "get_endpoints", return_value=list(endpoints) + ), mock.patch.object( + RemoteAPI, "get_api_version", return_value="1.3" + ), mock.patch.object(RemoteAPI, "get_server_version", return_value="0.11"): + return RemoteAPI("test", None, None, config) + + +def test_requests_use_the_negotiated_version(): + api = _remote_api() + + assert api._api_url == "http://remote.test/v1.3/" + + +def test_api_version_switches_the_requested_version(): + api = _remote_api() + + with api.api_version("v1.2"): + assert api._api_url == "http://remote.test/v1.2/" + + assert api._api_url == "http://remote.test/v1.3/" + + +def test_api_version_not_provided_by_remote_raises(): + api = _remote_api(endpoints=("v1.3",)) + + with pytest.raises(RemoteError, match="not provided by remote"), api.api_version( + "v1.2" + ): + pass + + +def test_api_version_unknown_to_client_raises(): + api = _remote_api() + + with pytest.raises( + RemoteError, match="not supported by this client" + ), api.api_version("v1.1"): + pass + + +def test_push_simulation_uses_v1_2_when_v1_3_is_negotiated(): + api = _remote_api() + simulation = Simulation(Manifest()) + used_urls = [] + + with mock.patch.object( + RemoteAPI, "get_upload_options", return_value={"copy_files": False} + ), mock.patch.object( + RemoteAPI, "post", side_effect=lambda *a, **kw: used_urls.append(api._api_url) + ): + api.push_simulation(simulation, out_stream=io.StringIO()) + + assert used_urls == ["http://remote.test/v1.2/"] + assert api._api_url == "http://remote.test/v1.3/" + + +def test_push_simulation_requires_v1_2_on_the_remote(): + api = _remote_api(endpoints=("v1.3",)) + + with pytest.raises(RemoteError, match=r"requires one of the API versions v1\.2"): + api.push_simulation(Simulation(Manifest()), out_stream=io.StringIO()) From 0b6b49f9459cdf33fca3d5d76312a68a8438933c Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Tue, 1 Sep 2026 16:30:20 +0200 Subject: [PATCH 3/6] Fix issues on client side instead --- ...c7d94e02_normalise_ids_metadata_to_list.py | 92 +++++++++++++++++++ src/simdb/cli/commands/utils.py | 5 +- src/simdb/database/models/simulation.py | 8 +- src/simdb/remote/models.py | 11 --- 4 files changed, 102 insertions(+), 14 deletions(-) create mode 100644 alembic/versions/a3f1c7d94e02_normalise_ids_metadata_to_list.py diff --git a/alembic/versions/a3f1c7d94e02_normalise_ids_metadata_to_list.py b/alembic/versions/a3f1c7d94e02_normalise_ids_metadata_to_list.py new file mode 100644 index 00000000..96173b3d --- /dev/null +++ b/alembic/versions/a3f1c7d94e02_normalise_ids_metadata_to_list.py @@ -0,0 +1,92 @@ +"""normalise_ids_metadata_to_list + +Convert the ``ids`` and ``input_ids`` simulation metadata from their display-string +form, ``"[core_profiles, equilibrium]"``, to a real list of IDS names. + +Revision ID: a3f1c7d94e02 +Revises: 6fb9b8fbac38 +Create Date: 2026-09-01 00:00:00.000000 + +""" + +import json +from typing import Any, Sequence, Union + +from sqlalchemy import text + +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "a3f1c7d94e02" +down_revision: Union[str, Sequence[str], None] = "6fb9b8fbac38" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +IDS_KEYS = ("ids", "input_ids") + +_SELECT = text("SELECT id, metadata FROM simulations WHERE metadata IS NOT NULL") +_UPDATE = text("UPDATE simulations SET metadata = :metadata WHERE id = :sim_id") + + +def _as_dict(value: Any) -> Any: + """Return the metadata column value as a dict. + + SQLite hands back the raw JSON text while PostgreSQL decodes JSONB for us. + """ + if isinstance(value, (bytes, bytearray, memoryview)): + value = bytes(value).decode("utf-8") + if isinstance(value, str): + try: + value = json.loads(value) + except ValueError: + return None + return value if isinstance(value, dict) else None + + +def _split_ids(value: str) -> list: + text_value = value.strip() + if text_value.startswith("[") and text_value.endswith("]"): + text_value = text_value[1:-1] + return [name.strip() for name in text_value.split(",") if name.strip()] + + +def _convert(convert_value) -> None: + conn = op.get_bind() + rows = conn.execute(_SELECT).fetchall() + + for sim_id, metadata in rows: + meta_dict = _as_dict(metadata) + if not meta_dict: + continue + + changed = False + for key in IDS_KEYS: + if key not in meta_dict: + continue + new_value = convert_value(meta_dict[key]) + if new_value is not None and new_value != meta_dict[key]: + meta_dict[key] = new_value + changed = True + + if changed: + conn.execute(_UPDATE, {"metadata": json.dumps(meta_dict), "sim_id": sim_id}) + + +def upgrade() -> None: + """Turn stringified IDS lists into real lists.""" + + def to_list(value: Any) -> Any: + return _split_ids(value) if isinstance(value, str) else None + + _convert(to_list) + + +def downgrade() -> None: + """Restore the display-string form of the IDS lists.""" + + def to_string(value: Any) -> Any: + if isinstance(value, list): + return "[{}]".format(", ".join(str(el) for el in value)) + return None + + _convert(to_string) diff --git a/src/simdb/cli/commands/utils.py b/src/simdb/cli/commands/utils.py index df460ae3..10bb349b 100644 --- a/src/simdb/cli/commands/utils.py +++ b/src/simdb/cli/commands/utils.py @@ -241,7 +241,10 @@ def _format_meta_value(meta_value: Any, max_len: int) -> str: if isinstance(meta_value, list): values = [] for i, v in enumerate(meta_value): - values.append(f"{v:.2f}") + if isinstance(v, bool) or not isinstance(v, (int, float)): + values.append(str(v)) + else: + values.append(f"{v:.2f}") if i >= max_len - 1: values.append("...") break diff --git a/src/simdb/database/models/simulation.py b/src/simdb/database/models/simulation.py index ede09fdf..428a04ce 100644 --- a/src/simdb/database/models/simulation.py +++ b/src/simdb/database/models/simulation.py @@ -221,7 +221,7 @@ def __init__( self.inputs.append(file) if all_input_idss: - self.set_meta("input_ids", "[{}]".format(", ".join(all_input_idss))) + self.set_meta("input_ids", all_input_idss) all_output_idss = [] @@ -249,7 +249,7 @@ def __init__( self.outputs.append(file) if all_output_idss: - self.set_meta("ids", "[{}]".format(", ".join(all_output_idss))) + self.set_meta("ids", all_output_idss) flattened_dict = flatten_dict(manifest.metadata) @@ -293,6 +293,10 @@ def __str__(self): first_line = False elif isinstance(value, dict) and "min" in value and "max" in value: result += f" {element}: [{value['min']}, {value['max']}]\n" + elif isinstance(value, list): + result += " {}: [{}]\n".format( + element, ", ".join(str(el) for el in value) + ) else: result += f" {element}: {value}\n" result += "inputs:\n" diff --git a/src/simdb/remote/models.py b/src/simdb/remote/models.py index d09df187..973005d3 100644 --- a/src/simdb/remote/models.py +++ b/src/simdb/remote/models.py @@ -530,17 +530,6 @@ class FileRegistrationItem(BaseModel): ids_list: Optional[List[Any]] = None """List of IDS names associated with the file.""" - @field_validator("ids_list", mode="before") - @classmethod - def _coerce_ids_list(cls, v: Any) -> Any: - """Accept the display-string form of the IDS list, ``"[a, b, c]"``.""" - if not isinstance(v, str): - return v - text = v.strip() - if text.startswith("[") and text.endswith("]"): - text = text[1:-1] - return [name.strip() for name in text.split(",") if name.strip()] - class FileRegistrationData(BaseModel): """Payload for final file registration after chunk uploads.""" From 3181e41980d3ab3976da44978830f792b0e49291 Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Wed, 2 Sep 2026 11:41:45 +0200 Subject: [PATCH 4/6] Revert UDA removal from imas_files Fixes issue with sim pull --- src/simdb/imas/utils.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/simdb/imas/utils.py b/src/simdb/imas/utils.py index 7b9ff234..d4529f3e 100644 --- a/src/simdb/imas/utils.py +++ b/src/simdb/imas/utils.py @@ -331,6 +331,15 @@ def imas_files(uri: SimDBUrl) -> List[Path]: path = _get_path(uri) + if backend == "uda": + query_backend = dict(uri.query_params()).get("backend") + if query_backend is None: + raise ValueError( + "Invalid IMAS URI - 'backend' query argument not provided for UDA " + "backend" + ) + backend = query_backend + if backend == "hdf5": return [p.absolute() for p in path.glob("*.h5")] elif backend == "mdsplus": From 807adcf306683962da108da7111c0aabc0669e02 Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Wed, 2 Sep 2026 13:48:35 +0200 Subject: [PATCH 5/6] Raise error when no files available for download --- src/simdb/cli/remote_api.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/simdb/cli/remote_api.py b/src/simdb/cli/remote_api.py index 61c94133..92bf4142 100644 --- a/src/simdb/cli/remote_api.py +++ b/src/simdb/cli/remote_api.py @@ -1166,6 +1166,12 @@ def pull_simulation( all_paths = [] + if len(simulation.inputs) + len(simulation.outputs) == 0: + raise RemoteError( + f"Simulation '{sim_id}' on remote has no input or output files " + "registered, so there is nothing to download." + ) + for file in itertools.chain(simulation.inputs, simulation.outputs): info = self._get_file_info(file.uuid) all_paths += [path for (path, _) in info] From b359f6c7bb4a8b11542b6c50ea971b87c4895705 Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Wed, 2 Sep 2026 15:24:52 +0200 Subject: [PATCH 6/6] Handle ids lists from old remotes --- src/simdb/database/models/simulation.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/simdb/database/models/simulation.py b/src/simdb/database/models/simulation.py index 428a04ce..a57dd6d3 100644 --- a/src/simdb/database/models/simulation.py +++ b/src/simdb/database/models/simulation.py @@ -170,7 +170,25 @@ def _get_metadata_dict(self) -> Dict[str, Any]: return {} return self._metadata + def _coerce_ids_list(self, v: Any) -> Any: + """Repair ``ids``/``input_ids`` metadata written as a display string. + + SimDB <= 1.2 stored these as ``"[core_profiles, equilibrium]"`` rather than a + list, which fails validation when the simulation is pushed back (#119). + remains. + """ + if not isinstance(v, str): + return v + text = v.strip() + if text.startswith("[") and text.endswith("]"): + text = text[1:-1] + return [name.strip() for name in text.split(",") if name.strip()] + def _set_metadata_dict(self, meta_dict: Dict[str, Any]) -> None: + # Fix simulations pulled with odd formatted ids arrays: + for key in ("ids", "input_ids"): + if key in meta_dict: + meta_dict[key] = self._coerce_ids_list(meta_dict[key]) self._metadata = meta_dict def __init__(