From 38a49c9a99b467093f971f0896e5dd6cf26a378c Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Wed, 2 Sep 2026 16:19:08 +0200 Subject: [PATCH 1/6] Type remote API client payloads with pydantic models Replace the hand-rolled json.dumps/CustomEncoder request bodies and res.json() dict indexing in RemoteAPI with the shared models from simdb.remote.models, so the client and server validate against one contract. Request bodies are now pydantic models serialised by serialise_request_data; responses are parsed with model_validate_json and try_request maps pydantic ValidationError onto the existing FailedConnection/RemoteError reporting. Watcher.to_model now maps the notification enum onto the single character the API reports, instead of emitting str(Notification.X). --- src/simdb/cli/commands/remote.py | 13 +- src/simdb/cli/commands/simulation.py | 27 +- src/simdb/cli/commands/utils.py | 72 ++-- src/simdb/cli/remote_api.py | 411 +++++++++++++-------- src/simdb/database/models/watcher.py | 10 +- src/simdb/remote/models.py | 82 +++- tests/cli/test_cli_remote_command.py | 10 +- tests/cli/test_remote_api_version.py | 82 +++- tests/remote/api/test_files.py | 15 + tests/remote/api/test_remote_api_client.py | 345 +++++++++++++++++ tests/remote/api/test_watchers.py | 24 +- 11 files changed, 882 insertions(+), 209 deletions(-) create mode 100644 tests/remote/api/test_remote_api_client.py diff --git a/src/simdb/cli/commands/remote.py b/src/simdb/cli/commands/remote.py index 4401464b..40768d30 100644 --- a/src/simdb/cli/commands/remote.py +++ b/src/simdb/cli/commands/remote.py @@ -11,6 +11,7 @@ from simdb.cli.remote_api import RemoteAPI from simdb.database.models.simulation import Simulation +from simdb.database.models.watcher import Watcher from simdb.notifications import Notification from . import check_meta_args, pass_config @@ -270,6 +271,13 @@ def config_set_option(config: "Config", name: str, option: str, value: str): config.save() +_NOTIFICATION_NAMES = { + value: notification.name + for notification, value in Watcher.NOTIFICATION_CHOICES.items() +} +"""Notification names by the single character the remote reports them as.""" + + @remote.group(cls=RemoteSubGroup) def watcher(): """Manage simulation watchers on REMOTE SimDB server.""" @@ -285,7 +293,10 @@ def list_watchers(api: RemoteAPI, sim_id: str): if watchers: click.echo(f"Watchers for simulation {sim_id}:") for watcher in watchers: - click.echo(watcher) + notification = _NOTIFICATION_NAMES.get( + watcher.notification, watcher.notification + ) + click.echo(f"{watcher.username} <{watcher.email}> ({notification})") else: click.echo(f"no watchers found for simulation {sim_id}") diff --git a/src/simdb/cli/commands/simulation.py b/src/simdb/cli/commands/simulation.py index 59bd0bdd..551dde77 100644 --- a/src/simdb/cli/commands/simulation.py +++ b/src/simdb/cli/commands/simulation.py @@ -418,34 +418,33 @@ def simulation_data( except Exception as err: raise click.ClickException(str(err)) from err - click.echo(f"simulation : {result['simulation']}") - click.echo(f"path : {result['path']} (occurrence {result['occurrence']})") + click.echo(f"simulation : {result.simulation}") + click.echo(f"path : {result.path} (occurrence {result.occurrence})") - coordinates = result.get("coordinates") or [] + field = result.field + coordinates = result.coordinates plot_coordinate = next( ( coord for coord in coordinates - if isinstance(coord.get("data"), list) - and isinstance(result["field"].get("data"), list) - and len(coord["data"]) == len(result["field"]["data"]) + if isinstance(coord.data, list) + and isinstance(field.data, list) + and len(coord.data) == len(field.data) ), None, ) - field_is_1d = is_numeric_1d(result["field"].get("data")) + field_is_1d = is_numeric_1d(field.data) if field_is_1d: - show_quantity_textual_plot( - result["field"], label="field", x_quantity=plot_coordinate - ) + show_quantity_textual_plot(field, label="field", x_quantity=plot_coordinate) else: - print_quantity(result["field"], label="field") + print_quantity(field, label="field") if config.verbose and coordinates: for coord in coordinates: - if field_is_1d and is_numeric_1d(coord.get("data")): + if field_is_1d and is_numeric_1d(coord.data): continue - if isinstance(coord.get("data"), list): - print_quantity(coord, label=f"coord {coord['name']}", show_stats=False) + if isinstance(coord.data, list): + print_quantity(coord, label=f"coord {coord.name}", show_stats=False) @simulation.command("validate", cls=n_required_args_adaptor(1)) diff --git a/src/simdb/cli/commands/utils.py b/src/simdb/cli/commands/utils.py index df460ae3..067c460a 100644 --- a/src/simdb/cli/commands/utils.py +++ b/src/simdb/cli/commands/utils.py @@ -8,6 +8,8 @@ from rich.table import Table from rich.text import Text +from simdb.remote.models import QuantityData, SimulationTraceData + if TYPE_CHECKING: # Only importing these for type checking and documentation generation in order to # speed up runtime startup. @@ -50,9 +52,9 @@ def is_numeric_1d(data: Any) -> bool: return isinstance(data, list) and bool(data) and all(_is_numeric(v) for v in data) -def _quantity_axis_label(q: dict, fallback: str = "") -> str: - name = q.get("name") or fallback - units = q.get("units") or "-" +def _quantity_axis_label(q: QuantityData, fallback: str = "") -> str: + name = q.name or fallback + units = q.units or "-" label = str(name).rsplit("/", 1)[-1] or str(name) return f"{label} [{units}]" @@ -140,14 +142,14 @@ def _plot_panel( def show_quantity_textual_plot( - q: dict, + q: QuantityData, label: str = "", - x_quantity: Optional[dict] = None, + x_quantity: Optional[QuantityData] = None, ) -> None: - """Print line plot for a 1-D numeric QuantityData dict.""" - name = q["name"] - units = q["units"] or "-" - data = q["data"] + """Print line plot for a 1-D numeric QuantityData.""" + name = q.name + units = q.units or "-" + data = q.data if not is_numeric_1d(data): print_quantity(q, label=label) return @@ -158,10 +160,10 @@ def show_quantity_textual_plot( xlabel = "index [-]" if ( x_quantity - and is_numeric_1d(x_quantity.get("data")) - and len(x_quantity["data"]) == len(y_values) + and is_numeric_1d(x_quantity.data) + and len(x_quantity.data) == len(y_values) ): - x_values = [float(value) for value in x_quantity["data"]] + x_values = [float(value) for value in x_quantity.data] xlabel = _quantity_axis_label(x_quantity, fallback="x") title = label or name @@ -191,11 +193,11 @@ def show_quantity_textual_plot( print_quantity(q, label=label) -def print_quantity(q: dict, label: str = "", show_stats: bool = True) -> None: - """Print a QuantityData dict with array display and stats.""" - name = q["name"] - units = q["units"] or "-" - data = q["data"] +def print_quantity(q: QuantityData, label: str = "", show_stats: bool = True) -> None: + """Print a QuantityData with array display and stats.""" + name = q.name + units = q.units or "-" + data = q.data title = f"[bold]{label or name}[/bold] [dim]\\[{units}][/dim]" if not isinstance(data, list): @@ -334,44 +336,36 @@ def print_simulations( ) -def _print_trace_sim(trace_data: dict, indentation: int): +def _print_trace_sim(trace_data: SimulationTraceData, indentation: int): spaces = " " * indentation - if "error" in trace_data: - error = trace_data["error"] - click.echo(f"{spaces}{error}") - return - - uuid = trace_data["uuid"] - alias = trace_data["alias"] - status = trace_data.get("status", "unknown") + status = trace_data.status or "unknown" - click.echo(f"{spaces}Simulation: {uuid}") - click.echo(f"{spaces} Alias: {alias}") + click.echo(f"{spaces}Simulation: {trace_data.uuid}") + click.echo(f"{spaces} Alias: {trace_data.alias}") click.echo(f"{spaces} Status: {status}") - status_on_name = status + "_on" - if status_on_name in trace_data: - status_on = trace_data[status_on_name] + status_on_name = status.replace(" ", "_") + "_on" + status_on = getattr(trace_data, status_on_name, None) + if status_on is not None: label = status_on_name.replace("_", " ").capitalize() click.echo(f"{spaces}{label}: {status_on}") - if "replaces" in trace_data: - if "replaces_reason" in trace_data: - replaces_reason = trace_data["replaces_reason"] - click.echo(f"{spaces}Replaces: (reason: {replaces_reason})") + if trace_data.replaces is not None: + if trace_data.replaces_reason is not None: + click.echo(f"{spaces}Replaces: (reason: {trace_data.replaces_reason})") else: click.echo(f"{spaces}Replaces:") - _print_trace_sim(trace_data["replaces"], indentation + 2) + _print_trace_sim(trace_data.replaces, indentation + 2) -def print_trace(trace_data: dict) -> None: +def print_trace(trace_data: Optional[SimulationTraceData]) -> None: """ Print the simulation trace data to the console. - :param trace_data: A dictionary containing the simulation trace data. + :param trace_data: The trace data of the simulation. :return: None """ - if not trace_data: + if trace_data is None: click.echo("No simulations trace found") return diff --git a/src/simdb/cli/remote_api.py b/src/simdb/cli/remote_api.py index 97348cdb..0e173b3e 100644 --- a/src/simdb/cli/remote_api.py +++ b/src/simdb/cli/remote_api.py @@ -4,7 +4,6 @@ import hashlib import io import itertools -import json import os import pickle import shutil @@ -30,14 +29,50 @@ import appdirs import click import requests +from pydantic import BaseModel, ValidationError from requests.auth import AuthBase from semantic_version import Version from simdb.config import Config from simdb.database.models import Simulation from simdb.imas.utils import SimDBUrl, imas_files -from simdb.json import CustomDecoder, CustomEncoder -from simdb.remote import CLIENT_API_VERSIONS, APIConstants +from simdb.remote import CLIENT_API_VERSIONS +from simdb.remote.models import ( + ChunkInfo, + FileGetDataResponse, + FileRegistrationData, + FileRegistrationItem, + FileUploadData, + ImasDataQueryParams, + ImasDataResponse, + IndexResponse, + MetadataDataList, + MetadataDeleteData, + MetadataDeleteResponse, + MetadataPatchData, + MetadataValue, + PaginatedResponse, + PaginationData, + SimulationData, + SimulationDataResponse, + SimulationDeleteResponse, + SimulationListItem, + SimulationPatchResponse, + SimulationPostData, + SimulationTraceData, + StagingDirectoryResponse, + StatusPatchData, + TokenResponse, + UploadOptions, + ValidationResult, + ValidationSchemaList, + WatcherData, + WatcherDeleteRequest, + WatcherDeleteResponse, + WatcherGetResponse, + WatcherPostRequest, + WatcherPostResponse, +) from .manifest import DataType @@ -93,6 +128,18 @@ def wrapped_func(*args, **kwargs): This might indicate an invalid SimDB URL or the existence of a firewall. """ ) from None + except ValidationError as ex: + if any(error["type"] == "json_invalid" for error in ex.errors()): + raise FailedConnection( + """\ +Invalid JSON returned from request endpoint + +This might indicate an invalid SimDB URL or the existence of a firewall. + """ + ) from None + raise RemoteError( + f"Unexpected data exchanged with the remote:\n{ex}" + ) from None return wrapped_func @@ -214,6 +261,23 @@ def select_api_version( return max(common_versions, key=lambda v: Version.coerce(v.lstrip("v"))) +RequestData = Optional[BaseModel] +"""Body of a request, described by the pydantic model of the endpoint.""" + + +def serialise_request_data(data: RequestData) -> Union[str, Dict]: + """ + Serialise the body of a request to JSON. + + @param data: the request body as the pydantic model of the endpoint, or None + for a request without a body. + @return: the JSON encoded body, or an empty dict if there is no body. + """ + if data is None: + return {} + return data.model_dump_json() + + def check_return(res: "requests.Response") -> None: if res.status_code != 200: try: @@ -226,6 +290,46 @@ def check_return(res: "requests.Response") -> None: res.raise_for_status() +def _pagination_headers(limit: int, page: int = 1) -> Dict[str, str]: + """ + Return the pagination request headers, leaving out the server side defaults. + """ + pagination = PaginationData.model_validate({"limit": limit, "page": page}) + return { + name: str(value) + for name, value in pagination.model_dump( + by_alias=True, exclude_defaults=True + ).items() + } + + +def _meta_list(value: Any) -> List[Any]: + """ + Return a metadata value as the list of names the files endpoint expects. + """ + if value is None: + return [] + if isinstance(value, (list, tuple)): + return list(value) + return [value] + + +def _simulation_from_list_item(item: SimulationListItem) -> "Simulation": + """ + Build a Simulation from the summary returned by the simulation list endpoint. + """ + return Simulation.from_data_model( + SimulationData.model_validate( + { + "uuid": item.uuid, + "alias": item.alias, + "datetime": item.datetime, + "metadata": item.metadata, + } + ) + ) + + def _get_paths(file: "File") -> Iterable[Path]: if file.type == DataType.FILE: if file.uri and file.uri.path: @@ -462,23 +566,24 @@ def get( check_return(res) return res - def put(self, url: str, data: Dict, **kwargs) -> "requests.Response": + def put(self, url: str, data: RequestData, **kwargs) -> "requests.Response": """ Perform an HTTP PUT request. @param url: the URL of the request. - @param data: the PUT data to send. + @param data: the PUT data to send, as the pydantic model of the endpoint. @param kwargs: any additional keyword arguments to add to the request. @return: """ headers = {"Content-type": "application/json"} headers["User-Agent"] = "it_script_basic" + put_data = serialise_request_data(data) if self._server_auth != "None": res = requests.put( self._api_url + url, - data=json.dumps(data, cls=CustomEncoder), + data=put_data, headers=headers, auth=self._get_auth(), cookies=self._cookies, @@ -487,7 +592,7 @@ def put(self, url: str, data: Dict, **kwargs) -> "requests.Response": else: res = requests.put( self._api_url + url, - data=json.dumps(data, cls=CustomEncoder), + data=put_data, headers=headers, cookies=self._cookies, **kwargs, @@ -496,12 +601,12 @@ def put(self, url: str, data: Dict, **kwargs) -> "requests.Response": check_return(res) return res - def post(self, url: str, data: Dict, **kwargs) -> "requests.Response": + def post(self, url: str, data: RequestData, **kwargs) -> "requests.Response": """ Perform an HTTP POST request. @param url: the URL of the request. - @param data: the POST data to send. + @param data: the POST data to send, as the pydantic model of the endpoint. @param kwargs: any additional keyword arguments to add to the request. @return: """ @@ -512,7 +617,7 @@ def post(self, url: str, data: Dict, **kwargs) -> "requests.Response": headers = {} else: headers = {"Content-type": "application/json"} - post_data = json.dumps(data, cls=CustomEncoder, indent=2) if data else {} + post_data = serialise_request_data(data) headers["User-Agent"] = "it_script_basic" # Compress the data if it is larger than 2 MB and the URL is for simulations @@ -549,23 +654,24 @@ def post(self, url: str, data: Dict, **kwargs) -> "requests.Response": check_return(res) return res - def patch(self, url: str, data: Dict, **kwargs) -> "requests.Response": + def patch(self, url: str, data: RequestData, **kwargs) -> "requests.Response": """ Perform an HTTP PATCH request. @param url: the URL of the request. - @param data: the PATCH data to send. + @param data: the PATCH data to send, as the pydantic model of the endpoint. @param kwargs: any additional keyword arguments to add to the request. @return: """ headers = {"Content-type": "application/json"} headers["User-Agent"] = "it_script_basic" + patch_data = serialise_request_data(data) if self._server_auth != "None": res = requests.patch( self._api_url + url, - data=json.dumps(data, cls=CustomEncoder), + data=patch_data, headers=headers, auth=self._get_auth(), cookies=self._cookies, @@ -574,7 +680,7 @@ def patch(self, url: str, data: Dict, **kwargs) -> "requests.Response": else: res = requests.patch( self._api_url + url, - data=json.dumps(data, cls=CustomEncoder), + data=patch_data, headers=headers, cookies=self._cookies, **kwargs, @@ -583,23 +689,24 @@ def patch(self, url: str, data: Dict, **kwargs) -> "requests.Response": check_return(res) return res - def delete(self, url: str, data: Dict[Any, Any], **kwargs) -> "requests.Response": + def delete(self, url: str, data: RequestData, **kwargs) -> "requests.Response": """ Perform an HTTP DELETE request. @param url: the URL of the request. - @param data: the DELETE data to send. + @param data: the DELETE data to send, as the pydantic model of the endpoint. @param kwargs: any additional keyword arguments to add to the request. @return: """ headers = {"Content-type": "application/json"} headers["User-Agent"] = "it_script_basic" + delete_data = serialise_request_data(data) if self._server_auth != "None": res = requests.delete( self._api_url + url, - data=json.dumps(data, cls=CustomEncoder), + data=delete_data, headers=headers, auth=self._get_auth(), cookies=self._cookies, @@ -608,7 +715,7 @@ def delete(self, url: str, data: Dict[Any, Any], **kwargs) -> "requests.Response else: res = requests.delete( self._api_url + url, - data=json.dumps(data, cls=CustomEncoder), + data=delete_data, headers=headers, cookies=self._cookies, **kwargs, @@ -619,56 +726,59 @@ def delete(self, url: str, data: Dict[Any, Any], **kwargs) -> "requests.Response def has_url(self) -> bool: return bool(self._url) + @try_request + def _get_index(self) -> IndexResponse: + """ + Return the index of the endpoint the current request targets. + """ + res = self.get("", authenticate=False) + return IndexResponse.model_validate_json(res.content) + @versioned_method("v1.2", "v1.3") @try_request def get_token(self) -> str: res = self.get("token") - data = res.json() - return data["token"] + return TokenResponse.model_validate_json(res.content).token @versioned_method("v1.2", "v1.3") - @try_request def get_endpoints(self) -> List[str]: - res = self.get("", authenticate=False) - data = res.json() - return data["endpoints"] + return self._get_index().endpoints @versioned_method("v1.2", "v1.3") - @try_request def get_server_authentication(self) -> Optional[str]: - res = self.get("", authenticate=False) - data = res.json() - return data.get("authentication") + return self._get_index().authentication @versioned_method("v1.2", "v1.3") - @try_request def get_api_version(self) -> str: - res = self.get("", authenticate=False) - data = res.json() - return data["api_version"] + version = self._get_index().api_version + if version is None: + raise RemoteError(f"Remote '{self._remote}' did not report an API version.") + return version @versioned_method("v1.2", "v1.3") - @try_request def get_server_version(self) -> str: - res = self.get("", authenticate=False) - data = res.json() - return data["server_version"] + version = self._get_index().server_version + if version is None: + raise RemoteError( + f"Remote '{self._remote}' did not report a server version." + ) + return version @versioned_method("v1.2", "v1.3") @try_request def get_validation_schemas(self) -> List[Dict]: res = self.get("validation_schema") - return res.json() + return ValidationSchemaList.model_validate_json(res.content).root @versioned_method("v1.2", "v1.3") @try_request - def get_upload_options(self) -> Dict[str, Any]: + def get_upload_options(self) -> UploadOptions: try: res = self.get("upload_options") - return res.json() + return UploadOptions.model_validate_json(res.content) except FailedConnection: # old remotes may not provide this endpoint - return {} + return UploadOptions() @versioned_method("v1.2", "v1.3") @try_request @@ -676,22 +786,24 @@ def list_simulations( self, meta: Optional[List[str]] = None, limit: int = 0 ) -> List["Simulation"]: args = "?" + "&".join(meta) if meta else "" - headers = {"simdb-result-limit": str(limit)} + headers = _pagination_headers(limit) res = self.get("simulations" + args, headers=headers) - data = res.json(cls=CustomDecoder) - return [Simulation.from_data(sim) for sim in data["results"]] + data = PaginatedResponse[SimulationListItem].model_validate_json(res.content) + return [_simulation_from_list_item(sim) for sim in data.results] @versioned_method("v1.2", "v1.3") @try_request def get_simulation(self, sim_id: str) -> "Simulation": res = self.get("simulation/" + sim_id) - return Simulation.from_data(res.json(cls=CustomDecoder)) + return Simulation.from_data_model( + SimulationDataResponse.model_validate_json(res.content) + ) @versioned_method("v1.2", "v1.3") @try_request - def trace_simulation(self, sim_id: str) -> dict: + def trace_simulation(self, sim_id: str) -> SimulationTraceData: res = self.get("trace/" + sim_id) - return res.json(cls=CustomDecoder) + return SimulationTraceData.model_validate_json(res.content) @versioned_method("v1.2", "v1.3") @try_request @@ -703,92 +815,104 @@ def query_simulations( (key, value) = item.split("=") params[key].append(value) args = "?" + "&".join(meta) if meta else "" - headers = { - APIConstants.LIMIT_HEADER: str(limit), - APIConstants.PAGE_HEADER: str(1), - } + headers = _pagination_headers(limit, page=1) res = self.get("simulations" + args, params, headers=headers) - data = res.json(cls=CustomDecoder) - return [Simulation.from_data(sim) for sim in data["results"]] + data = PaginatedResponse[SimulationListItem].model_validate_json(res.content) + return [_simulation_from_list_item(sim) for sim in data.results] @versioned_method("v1.2", "v1.3") @try_request - def delete_simulation(self, sim_id: str) -> Dict: - res = self.delete("simulation/" + sim_id, {}) - return res.json() + def delete_simulation(self, sim_id: str) -> SimulationDeleteResponse: + res = self.delete("simulation/" + sim_id, None) + return SimulationDeleteResponse.model_validate_json(res.content) @versioned_method("v1.2", "v1.3") @try_request def update_simulation(self, sim_id: str, update_type: "Simulation.Status") -> None: - self.patch("simulation/" + sim_id, {"status": update_type.value}) + res = self.patch( + "simulation/" + sim_id, StatusPatchData(status=update_type.value) + ) + SimulationPatchResponse.model_validate_json(res.content) @versioned_method("v1.2", "v1.3") @try_request def validate_simulation(self, sim_id: str) -> Tuple[bool, str]: - res = self.post("validate/" + sim_id, {}) - data = res.json() - if data["passed"]: + res = self.post("validate/" + sim_id, None) + result = ValidationResult.model_validate_json(res.content) + if result.passed: return True, "" else: - return False, data["error"] + return False, result.error or "" @versioned_method("v1.2", "v1.3") @try_request def add_watcher( self, sim_id: str, user: str, email: str, notification: "Watcher.Notification" ) -> None: - self.post( + res = self.post( "watchers/" + sim_id, - {"user": user, "email": email, "notification": notification.name}, + WatcherPostRequest(user=user, email=email, notification=notification.name), ) + WatcherPostResponse.model_validate_json(res.content) @versioned_method("v1.2", "v1.3") @try_request def remove_watcher(self, sim_id: str, user: str) -> None: - self.delete("watchers/" + sim_id, {"user": user}) + res = self.delete("watchers/" + sim_id, WatcherDeleteRequest(user=user)) + WatcherDeleteResponse.model_validate_json(res.content) @versioned_method("v1.2", "v1.3") @try_request - def list_watchers(self, sim_id: str) -> List[Tuple]: + def list_watchers(self, sim_id: str) -> List[WatcherData]: res = self.get("watchers/" + sim_id) - return [(d["username"], d["email"], d["notification"]) for d in res.json()] + return WatcherGetResponse.model_validate_json(res.content).root @versioned_method("v1.2", "v1.3") @try_request def set_metadata( self, sim_id: str, key: str, value: Union[str, uuid.UUID, int, float] - ) -> List[str]: - res = self.patch("simulation/metadata/" + sim_id, {"key": key, "value": value}) - return [data["value"] for data in res.json()] + ) -> List[MetadataValue]: + """ + Set a metadata value on the remote, returning the values it replaced. + """ + res = self.patch( + "simulation/metadata/" + sim_id, + MetadataPatchData(key=key, value=str(value)), + ) + return [ + meta.value + for meta in MetadataDataList.model_validate_json(res.content).root + ] @versioned_method("v1.2", "v1.3") @try_request - def delete_metadata(self, sim_id: str, key: str) -> List[str]: - res = self.delete("simulation/metadata/" + sim_id, {"key": key}) - return [data["value"] for data in res.json()] + def delete_metadata(self, sim_id: str, key: str) -> None: + res = self.delete("simulation/metadata/" + sim_id, MetadataDeleteData(key=key)) + MetadataDeleteResponse.model_validate_json(res.content) @versioned_method("v1.3") @try_request def get_simulation_data( self, sim_id: str, path: str, dd_version: Optional[str] = None - ) -> Dict[str, Any]: - params = {"path": path} - if dd_version is not None: - params["dd_version"] = dd_version - res = self.get(f"simulation/{sim_id}/data", params=params) - return res.json() + ) -> ImasDataResponse: + query = ImasDataQueryParams(path=path, dd_version=dd_version) + res = self.get( + f"simulation/{sim_id}/data", + params=query.model_dump(exclude_none=True), + ) + return ImasDataResponse.model_validate_json(res.content) @try_request - def get_directory(self) -> str: + def get_directory(self) -> Path: res = self.get("staging_dir") - return res.json()["staging_dir"] + return StagingDirectoryResponse.model_validate_json(res.content).staging_dir def _push_file( self, path: Path, uuid: uuid.UUID, file_type: str, - sim_data: Dict[str, Any], + sim_data: SimulationData, chunk_size: int, out_stream: IO, type: DataType, @@ -808,18 +932,18 @@ def _push_file( if type == DataType.FILE: self.post( "files", - data={ - "simulation": sim_data, - "obj_type": DataType.FILE, - "files": [ - { - "chunks": num_chunks, - "file_type": file_type, - "file_uuid": uuid.hex, - "ids_list": None, - } + data=FileRegistrationData( + simulation=sim_data, + obj_type=DataType.FILE, + files=[ + FileRegistrationItem( + chunks=num_chunks, + file_type=file_type, + file_uuid=uuid, + ids_list=None, + ) ], - }, + ), ) print(f"\r{msg}", file=out_stream, end="") print( @@ -835,25 +959,25 @@ def _send_chunk( chunk_size: int, uuid: uuid.UUID, file_type: str, - sim_data: dict, + sim_data: SimulationData, ): - data = { - "simulation": sim_data, - "file_type": file_type, - "chunk_info": {uuid.hex: {"chunk_size": chunk_size, "chunk": chunk_index}}, - } + data = FileUploadData( + simulation=sim_data, + file_type=file_type, + chunk_info={uuid.hex: ChunkInfo(chunk_size=chunk_size, chunk=chunk_index)}, + ) files: List[Tuple[str, Tuple[str, bytes, str]]] = [ ( "data", ( "data", - json.dumps(data, cls=CustomEncoder).encode(), + data.model_dump_json().encode(), "text/json", ), ), ("files", (uuid.hex, chunk, "application/octet-stream")), ] - self.post("files", data={}, files=files) + self.post("files", data=None, files=files) @versioned_method("v1.2", "v1.3") @try_request @@ -875,13 +999,10 @@ def push_simulation( remote server """ - sim_data = simulation.data(recurse=True) + sim_data = simulation.to_model(recurse=True) try: - sim_json = json.dumps( - sim_data, cls=CustomEncoder, separators=(",", ":") - ).encode("utf-8") - sim_json_size = len(sim_json) + sim_json_size = len(sim_data.model_dump_json().encode("utf-8")) except Exception: sim_json_size = 0 @@ -897,17 +1018,17 @@ def push_simulation( ) options = self.get_upload_options() - if options.get("copy_files", True): + if options.copy_files: chunk_size = allowed_chunk # 10 MB limit on ITER network - copy_ids = options.get("copy_ids", True) + copy_ids = options.copy_ids for file in simulation.inputs: if file.type == DataType.IMAS: if not copy_ids: print(f"Skipping IDS data {file}", file=out_stream, flush=True) continue - ids_list = simulation.meta_dict().get("input_ids", []) + ids_list = _meta_list(simulation.meta_dict().get("input_ids")) for path in imas_files(file.uri): # Check if hdf5 ids_name is in ids_list ids_name = Path(path).name.split(".") @@ -918,9 +1039,9 @@ def push_simulation( ): continue sim_file = next( - f for f in sim_data["inputs"] if f.get("uuid") == file.uuid + f for f in sim_data.inputs.root if f.uuid == file.uuid ) - sim_file["uri"] = f"file:{path}" + sim_file.uri = f"file:{path}" self._push_file( path, file.uuid, @@ -933,17 +1054,17 @@ def push_simulation( self.post( "files", - data={ - "simulation": simulation.data(recurse=True), - "obj_type": file.type, - "files": [ - { - "file_type": "input", - "file_uuid": file.uuid.hex, - "ids_list": ids_list, - } + data=FileRegistrationData( + simulation=simulation.to_model(recurse=True), + obj_type=file.type, + files=[ + FileRegistrationItem( + file_type="input", + file_uuid=file.uuid, + ids_list=ids_list, + ) ], - }, + ), ) else: @@ -964,7 +1085,7 @@ def push_simulation( print(f"Skipping IDS data {file}", file=out_stream, flush=True) continue - ids_list = simulation.meta_dict().get("ids", []) + ids_list = _meta_list(simulation.meta_dict().get("ids")) for path in imas_files(file.uri): # Check if hdf5 ids_name is in ids_list ids_name = Path(path).name.split(".") @@ -975,15 +1096,11 @@ def push_simulation( ): continue sim_file = next( - ( - f - for f in sim_data["outputs"] - if f.get("uuid") == file.uuid - ), + (f for f in sim_data.outputs.root if f.uuid == file.uuid), None, ) if sim_file: - sim_file["uri"] = f"file:{path}" + sim_file.uri = f"file:{path}" self._push_file( path, file.uuid, @@ -996,17 +1113,17 @@ def push_simulation( self.post( "files", - data={ - "simulation": simulation.data(recurse=True), - "obj_type": file.type, - "files": [ - { - "file_type": "output", - "file_uuid": file.uuid.hex, - "ids_list": ids_list, - } + data=FileRegistrationData( + simulation=simulation.to_model(recurse=True), + obj_type=file.type, + files=[ + FileRegistrationItem( + file_type="output", + file_uuid=file.uuid, + ids_list=ids_list, + ) ], - }, + ), ) else: if file.uri and file.uri.path: @@ -1020,24 +1137,22 @@ def push_simulation( file.type, ) - sim_data = simulation.data(recurse=True) - uploaded_by = simulation.meta_dict().get("uploaded_by", None) + uploaded_by = simulation.meta_dict().get("uploaded_by") print("Uploading simulation data ... ", file=out_stream, end="", flush=True) self.post( "simulations", - data={ - "simulation": sim_data, - "add_watcher": add_watcher, - "uploaded_by": uploaded_by, - }, + data=SimulationPostData( + simulation=simulation.to_model(recurse=True), + add_watcher=add_watcher, + uploaded_by=str(uploaded_by) if uploaded_by is not None else None, + ), ) print("Success", file=out_stream, flush=True) def _get_file_info(self, uuid: uuid.UUID) -> List[Tuple[Path, str]]: - r = self.get(f"file/{uuid.hex}") - data = r.json() - files = data["files"] - return [(Path(file["path"]), file["checksum"]) for file in files] + res = self.get(f"file/{uuid.hex}") + data = FileGetDataResponse.model_validate_json(res.content) + return [(file.path, file.checksum) for file in data.files] def _pull_file( self, @@ -1145,4 +1260,4 @@ def pull_simulation( @versioned_method("v1.2", "v1.3") @try_request def reset_database(self) -> None: - self.post("reset", {}) + self.post("reset", None) diff --git a/src/simdb/database/models/watcher.py b/src/simdb/database/models/watcher.py index c7532b72..c64e9cd8 100644 --- a/src/simdb/database/models/watcher.py +++ b/src/simdb/database/models/watcher.py @@ -62,4 +62,12 @@ def data(self, recurse: bool = False) -> Dict[str, str]: return data def to_model(self) -> WatcherData: - return WatcherData.model_validate(self.data()) + return WatcherData.model_validate( + { + "username": self.username, + "email": self.email, + "notification": self.NOTIFICATION_CHOICES[ + Notification(self.notification) + ], + } + ) diff --git a/src/simdb/remote/models.py b/src/simdb/remote/models.py index ae9565aa..12bc21bf 100644 --- a/src/simdb/remote/models.py +++ b/src/simdb/remote/models.py @@ -240,6 +240,28 @@ def convert_array_to_range(cls, data: Any) -> Any: data["value"] = _array_to_range(data["value"]) return data + @staticmethod + def _coerce_ids_list(v: str) -> list[str]: + """Accept the display-string form of the IDS list, ``"[a, b, c]"``.""" + text = v.strip() + if text.startswith("[") and text.endswith("]"): + text = text[1:-1] + return [name.strip() for name in text.split(",") if name.strip()] + + @model_validator(mode="before") + @classmethod + def fix_input_ids_string(cls, data: Any) -> Any: + """Convert funky ids input list strings to actual lists.""" + if ( + isinstance(data, dict) + and "value" in data + and "element" in data + and data["element"] == "input_ids" + and isinstance(data["value"], str) + ): + data["value"] = MetadataData._coerce_ids_list(data["value"]) + return data + def as_dict(self) -> dict: """Convert to dictionary.""" return {self.element: self.value} @@ -521,8 +543,11 @@ class FileUploadResponse(BaseModel): class FileRegistrationItem(BaseModel): """A single file entry in the file registration payload.""" - chunks: int - """The amount of chunks to be processed.""" + chunks: Optional[int] = None + """The amount of chunks to be processed. + + Only sent for plain file uploads; omitted for IMAS registrations, where a + single item covers the many files pushed under one UUID.""" file_type: str """The file type.""" file_uuid: HexUUID @@ -662,6 +687,59 @@ class ImasDataResponse(BaseModel): """Coordinates for each dimension of *field*, in dimension order.""" +class IndexResponse(BaseModel): + """Response from an API index endpoint. + + Both the server root (``/``) and the versioned API roots (``/v1.x/``) are + described by this model; each populates only the fields it provides, so every + field carries a default. + """ + + api: Optional[str] = None + """Name of the API.""" + api_version: Optional[str] = None + """Version of the API served by this endpoint.""" + server_version: Optional[str] = None + """Version of the SimDB server.""" + endpoints: List[str] = [] + """URLs of the endpoints provided by this endpoint.""" + documentation: Optional[str] = None + """URL of the API documentation.""" + authentication: Optional[str] = None + """Name of the authenticator the server uses by default.""" + authenticators: List[str] = [] + """Names of all the authenticators the server accepts.""" + + +class TokenResponse(BaseModel): + """Response from the get token endpoint.""" + + token: str + """The JWT token to authenticate subsequent requests with.""" + status: Optional[str] = None + """Status of the token request.""" + + +class UploadOptions(BaseModel): + """Response from the upload options endpoint. + + Describes how the server expects simulation data to be uploaded. Remotes that + do not provide the endpoint are treated as if they returned the defaults. + """ + + copy_files: bool = True + """Whether files have to be uploaded to the server.""" + copy_ids: bool = True + """Whether IMAS data has to be uploaded to the server.""" + + +class ValidationSchemaList(RootModel): + """Response from the validation schema endpoint.""" + + root: List[Dict[str, Any]] = [] + """The validation schemas the server validates simulations against.""" + + class ErrorResponse(BaseModel): """Response model for server errors.""" diff --git a/tests/cli/test_cli_remote_command.py b/tests/cli/test_cli_remote_command.py index 0e626042..79badcbc 100644 --- a/tests/cli/test_cli_remote_command.py +++ b/tests/cli/test_cli_remote_command.py @@ -5,6 +5,7 @@ from simdb.cli.simdb import cli from simdb.notifications import Notification +from simdb.remote.models import WatcherData @mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_authentication") @@ -24,7 +25,11 @@ def test_remote_watchers_list_command( get_server_version.return_value = "0.11" get_server_authentication.return_value = "None" sim_id = "acbd1234" - watchers = ["a", "b", "c"] + watchers = [ + WatcherData(username="a", email="a@simdb.test", notification="A"), + WatcherData(username="b", email="b@simdb.test", notification="V"), + WatcherData(username="c", email="c@simdb.test", notification="R"), + ] list_watchers.return_value = watchers config_file = config_test_file() runner = CliRunner() @@ -34,7 +39,8 @@ def test_remote_watchers_list_command( assert result.exception is None assert sim_id in result.output for watcher in watchers: - assert watcher in result.output + assert watcher.username in result.output + assert watcher.email in result.output assert list_watchers.called assert get_api_version.called diff --git a/tests/cli/test_remote_api_version.py b/tests/cli/test_remote_api_version.py index 08b0b44b..80fa031f 100644 --- a/tests/cli/test_remote_api_version.py +++ b/tests/cli/test_remote_api_version.py @@ -1,4 +1,13 @@ -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 +from simdb.remote.models import UploadOptions def test_selects_highest_common_version(): @@ -16,3 +25,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=UploadOptions(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()) diff --git a/tests/remote/api/test_files.py b/tests/remote/api/test_files.py index e4e6e328..29d6c20d 100644 --- a/tests/remote/api/test_files.py +++ b/tests/remote/api/test_files.py @@ -3,6 +3,7 @@ import io import json import tarfile +import uuid from datetime import datetime, timezone from pathlib import Path @@ -189,3 +190,17 @@ def test_download_file(client): assert rv.status_code == 200 assert rv.data == file_content + + +def test_file_registration_item_without_chunks(): + """The IMAS registration payload omits ``chunks``. + + A single item covers all the files pushed under one file UUID, so the + client has no meaningful chunk count to report and leaves it unset. + """ + item = FileRegistrationItem( + file_type="input", + file_uuid=uuid.uuid4(), + ids_list=["core_profiles"], + ) + assert item.chunks is None diff --git a/tests/remote/api/test_remote_api_client.py b/tests/remote/api/test_remote_api_client.py new file mode 100644 index 00000000..340c85b6 --- /dev/null +++ b/tests/remote/api/test_remote_api_client.py @@ -0,0 +1,345 @@ +"""Round-trip tests for the CLI RemoteAPI client against the real server. + +The client talks to the Flask test client instead of the network, so every +request body, query parameter and response is validated by the pydantic models +of both sides. +""" + +import base64 +import gzip +import io +import json +import os +import shutil +import tempfile +from functools import partial +from pathlib import Path +from unittest import mock +from urllib.parse import urlencode + +import pytest +from conftest import TEST_PASSWORD, has_flask + +from simdb.cli.manifest import Manifest +from simdb.cli.remote_api import FailedConnection, RemoteAPI, RemoteError +from simdb.config import Config +from simdb.database.models import Simulation +from simdb.notifications import Notification +from simdb.remote.app import create_app +from simdb.remote.models import ( + SimulationDeleteResponse, + SimulationTraceData, + UploadOptions, + WatcherData, +) + +REMOTE_URL = "http://remote.test" + +VALIDATION_SCHEMA = """\ +status: + type: string +""" + + +class _Response: + """The parts of a requests.Response the RemoteAPI uses.""" + + def __init__(self, response, url): + self.status_code = response.status_code + self.headers = response.headers + self.url = url + content = response.data + if (response.headers.get("Content-Encoding") or "").lower() == "gzip": + # requests transparently decodes this for the real client + content = gzip.decompress(content) + self.content = content + + def json(self, **kwargs): + return json.loads(self.content.decode(), **kwargs) + + def raise_for_status(self): + if self.status_code >= 400: + raise AssertionError(f"HTTP {self.status_code} for {self.url}") + + def iter_content(self, chunk_size=1): + for start in range(0, len(self.content), chunk_size): + yield self.content[start : start + chunk_size] + + +def _requests_shim(client): + """Return a stand-in for the requests module routed to *client*.""" + + def request( + method, + url, + params=None, + data=None, + headers=None, + auth=None, + files=None, + **_kwargs, + ): + path = url[len(REMOTE_URL) :] + query = None + if "?" in path: + path, _, query = path.partition("?") + if params: + extra = urlencode(params, doseq=True) + query = f"{query}&{extra}" if query else extra + + body = data + if files: + fields = {} + for key, (name, content, _content_type) in files: + fields.setdefault(key, []).append((io.BytesIO(content), name)) + body = {k: v[0] if len(v) == 1 else v for k, v in fields.items()} + + request_headers = dict(headers or {}) + if isinstance(auth, tuple): + credentials = base64.b64encode(f"{auth[0]}:{auth[1]}".encode()).decode() + request_headers["Authorization"] = f"Basic {credentials}" + + return _Response( + client.open( + path, + method=method, + query_string=query, + data=body, + headers=request_headers, + ), + url, + ) + + shim = mock.MagicMock() + for method in ("get", "post", "put", "patch", "delete"): + shim.configure_mock(**{method: partial(request, method.upper())}) + shim.ConnectionError = ConnectionError + shim.HTTPError = type("HTTPError", (Exception,), {}) + shim.JSONDecodeError = json.JSONDecodeError + return shim + + +@pytest.fixture(scope="function") +def remote(tmp_path): + """Yield a (RemoteAPI, admin RemoteAPI) pair talking to a live server.""" + if not has_flask: + pytest.skip("Flask not installed") + + schema_file = tmp_path / "validation.yaml" + schema_file.write_text(VALIDATION_SCHEMA) + + db_fd, db_file = tempfile.mkstemp() + upload_dir = tempfile.mkdtemp() + + config = Config() + config.load() + config.set_option("database.type", "sqlite") + config.set_option("database.file", db_file) + config.set_option("server.admin_password", TEST_PASSWORD) + config.set_option("server.upload_folder", upload_dir) + config.set_option("authentication.type", "None") + config.set_option("server.copy_files", True) + config.set_option("role.admin.users", "admin") + config.set_option("validation.path", str(schema_file)) + config.set_option("flask.secret_key", "test-secret") + app = create_app(config=config, testing=True, debug=True) + app.testing = True + + client_config = Config() + client_config.set_option(f"remote.{'test'}.url", REMOTE_URL) + + with app.test_client() as client, mock.patch( + "simdb.cli.remote_api.requests", _requests_shim(client) + ): + api = RemoteAPI("test", None, None, client_config, use_token=False) + admin = RemoteAPI( + "test", "admin", TEST_PASSWORD, client_config, use_token=False + ) + yield api, admin + + os.close(db_fd) + Path(db_file).unlink() + shutil.rmtree(upload_dir) + + +def _push_simulation(api, tmp_path, alias="client-sim"): + data_file = tmp_path / "input.txt" + data_file.write_text("hello simdb\n") + manifest_file = tmp_path / "manifest.yaml" + manifest_file.write_text( + "manifest_version: 2\n" + f"alias: {alias}\n" + "inputs:\n" + f" - uri: file://{data_file}\n" + "outputs:\n" + f" - uri: file://{data_file}\n" + "metadata:\n" + "- values:\n" + " code: my-code\n" + ) + simulation = Simulation(Manifest.load_from_file(manifest_file)) + api.push_simulation(simulation, out_stream=io.StringIO(), add_watcher=False) + return simulation + + +def test_index_endpoints(remote): + api, _ = remote + + assert str(api.version).startswith("1.3") + assert any(url.endswith("simulations") for url in api.get_endpoints()) + assert api.get_upload_options() == UploadOptions(copy_files=True, copy_ids=True) + assert isinstance(api.get_directory(), Path) + assert api.get_validation_schemas() == [{"status": {"type": "string"}}] + + +def test_token(remote): + _, admin = remote + + assert admin.get_token() + + +def test_push_and_get_simulation(remote, tmp_path): + api, _ = remote + + simulation = _push_simulation(api, tmp_path) + pushed = api.get_simulation(simulation.uuid.hex) + + assert pushed.uuid == simulation.uuid + assert pushed.alias == "client-sim" + assert pushed.meta_dict()["values"]["code"] == "my-code" + # the file has been staged on the server, so the URI now points there + assert len(pushed.inputs) == 1 + assert Path(pushed.inputs[0].uri.path).read_text() == "hello simdb\n" + + +def test_list_and_query_simulations(remote, tmp_path): + api, _ = remote + + simulation = _push_simulation(api, tmp_path) + + listed = api.list_simulations(limit=0) + assert [sim.uuid for sim in listed] == [simulation.uuid] + + queried = api.query_simulations(["values.code=my-code"], ["values.code"]) + assert [sim.uuid for sim in queried] == [simulation.uuid] + assert queried[0].find_meta("values.code") == ["my-code"] + + assert api.query_simulations(["values.code=other"], []) == [] + + +def test_trace_simulation(remote, tmp_path): + api, _ = remote + + simulation = _push_simulation(api, tmp_path) + trace = api.trace_simulation(simulation.uuid.hex) + + assert isinstance(trace, SimulationTraceData) + assert trace.uuid == simulation.uuid + assert trace.status == "not validated" + assert trace.replaces is None + + +def test_validate_simulation(remote, tmp_path): + api, _ = remote + + simulation = _push_simulation(api, tmp_path) + + assert api.validate_simulation(simulation.uuid.hex) == (True, "") + + +def test_watchers(remote, tmp_path): + api, _ = remote + + simulation = _push_simulation(api, tmp_path) + sim_id = simulation.uuid.hex + + assert api.list_watchers(sim_id) == [] + + api.add_watcher(sim_id, "watcher1", "example@iter.org", Notification.ALL) + assert api.list_watchers(sim_id) == [ + WatcherData(username="watcher1", email="example@iter.org", notification="A") + ] + + api.remove_watcher(sim_id, "watcher1") + assert api.list_watchers(sim_id) == [] + + +def test_metadata(remote, tmp_path): + api, admin = remote + + simulation = _push_simulation(api, tmp_path) + sim_id = simulation.uuid.hex + + assert admin.set_metadata(sim_id, "code", "first") == [] + assert admin.set_metadata(sim_id, "code", "second") == ["first"] + + assert admin.delete_metadata(sim_id, "code") is None + assert "code" not in api.get_simulation(sim_id).meta_dict() + + +def test_update_simulation_status(remote, tmp_path): + api, admin = remote + + simulation = _push_simulation(api, tmp_path) + sim_id = simulation.uuid.hex + + admin.update_simulation(sim_id, Simulation.Status.ACCEPTED) + + assert api.get_simulation(sim_id).status == Simulation.Status.ACCEPTED + + +def test_delete_simulation(remote, tmp_path): + api, admin = remote + + simulation = _push_simulation(api, tmp_path) + + deleted = admin.delete_simulation(simulation.uuid.hex) + + assert isinstance(deleted, SimulationDeleteResponse) + assert deleted.deleted.simulation == simulation.uuid + assert deleted.deleted.files + assert api.list_simulations() == [] + + +def test_pull_simulation(remote, tmp_path): + api, _ = remote + + simulation = _push_simulation(api, tmp_path) + + pulled = api.pull_simulation( + simulation.uuid.hex, tmp_path / "pulled", out_stream=io.StringIO() + ) + + assert pulled.uuid == simulation.uuid + assert Path(pulled.inputs[0].uri.path).read_text() == "hello simdb\n" + + +def test_simulation_data_reports_remote_errors(remote, tmp_path): + """The v1.3 IMAS data endpoint reports errors as a RemoteError, not a model.""" + api, _ = remote + + simulation = _push_simulation(api, tmp_path) + + with pytest.raises(RemoteError): + # the simulation holds no IMAS data, so the remote reports an error + api.get_simulation_data(simulation.uuid.hex, "core_profiles/time") + + +def test_invalid_json_from_remote_is_reported(remote): + """A non-JSON body is reported as a failed connection, not a pydantic error.""" + api, _ = remote + + with mock.patch.object( + RemoteAPI, "get", return_value=mock.Mock(content=b"login") + ), pytest.raises(FailedConnection, match="Invalid JSON"): + api.get_simulation("does-not-matter") + + +def test_unexpected_response_from_remote_is_reported(remote): + """A JSON body that does not match the model is reported as a remote error.""" + api, _ = remote + + with mock.patch.object( + RemoteAPI, "get", return_value=mock.Mock(content=b'{"unexpected": true}') + ), pytest.raises(RemoteError, match="Unexpected data exchanged"): + api.get_simulation("does-not-matter") diff --git a/tests/remote/api/test_watchers.py b/tests/remote/api/test_watchers.py index 7ec11a01..68f02565 100644 --- a/tests/remote/api/test_watchers.py +++ b/tests/remote/api/test_watchers.py @@ -26,7 +26,29 @@ def test_get_watchers(client): ) assert rv.status_code == 200 - WatcherGetResponse.model_validate(rv.json) + assert WatcherGetResponse.model_validate(rv.json).root == [] + + # Watchers that have been added are reported with the notification character + post_data = WatcherPostRequest( + user="testuser", email="example@iter.org", notification="ALL" + ) + rv_add = client.post( + f"/v1.2/watchers/{simulation_data.simulation.uuid.hex}", + json=post_data.model_dump(mode="json"), + headers=HEADERS, + content_type="application/json", + ) + assert rv_add.status_code == 200 + + rv = client.get( + f"/v1.2/watchers/{simulation_data.simulation.uuid.hex}", headers=HEADERS + ) + + assert rv.status_code == 200 + watchers = WatcherGetResponse.model_validate(rv.json).root + assert [(w.username, w.email, w.notification) for w in watchers] == [ + ("testuser", "example@iter.org", "A") + ] def test_post_watchers(client): From 382cf90fc60adcb21e7fb33eb676c8d39689d4f6 Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Wed, 2 Sep 2026 16:38:56 +0200 Subject: [PATCH 2/6] Consolidate ids list fix --- src/simdb/database/models/simulation.py | 24 ++++--------- src/simdb/remote/models.py | 46 ++++++++++++++++++------- tests/remote/api/test_metadata.py | 24 ++++++++++++- 3 files changed, 63 insertions(+), 31 deletions(-) diff --git a/src/simdb/database/models/simulation.py b/src/simdb/database/models/simulation.py index 3d804218..495873b2 100644 --- a/src/simdb/database/models/simulation.py +++ b/src/simdb/database/models/simulation.py @@ -17,12 +17,14 @@ from simdb.enums import IngestionStatus from simdb.imas.utils import SimDBUrl from simdb.remote.models import ( + IDS_LIST_KEYS, FileDataList, MetadataData, MetadataDataList, SimulationData, SimulationDataResponse, SimulationTraceData, + coerce_ids_list, ) if "sphinx" in sys.modules: @@ -163,25 +165,13 @@ 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"): + # Repair simulations pulled with odd formatted ids arrays. This is the only + # coercion on the raw-dict path (v1/v1.1 endpoints), which never builds a + # MetadataData. + for key in IDS_LIST_KEYS: if key in meta_dict: - meta_dict[key] = self._coerce_ids_list(meta_dict[key]) + meta_dict[key] = coerce_ids_list(meta_dict[key]) self._metadata = meta_dict def __init__( diff --git a/src/simdb/remote/models.py b/src/simdb/remote/models.py index 12bc21bf..08e8ee53 100644 --- a/src/simdb/remote/models.py +++ b/src/simdb/remote/models.py @@ -12,6 +12,7 @@ List, Literal, Optional, + Tuple, TypeVar, Union, ) @@ -224,6 +225,31 @@ def _serialize_numpy(o: np.ndarray) -> dict: validation.""" +IDS_LIST_KEYS: Tuple[str, ...] = ("ids", "input_ids") +"""Metadata keys whose value is a list of IDS names.""" + + +def coerce_ids_list(value: Any) -> Any: + """Repair an IDS list written as the display string ``"[a, b, c]"``. + + SimDB <= 1.2 stored the :data:`IDS_LIST_KEYS` metadata that way rather than as a + list (#119), so both forms have to be accepted wherever such a value crosses into + the models. Values that are not strings are returned unchanged. + + Kept as a single definition because it is applied at three separate boundaries: + :meth:`MetadataData.fix_ids_list_string` on the way out to the API, + ``Simulation._set_metadata_dict`` on the way in to the ORM, and the + ``a3f1c7d94e02`` migration for rows already at rest. The migration carries its own + copy on purpose -- migrations must stay frozen against later changes here. + """ + if not isinstance(value, str): + return value + text = value.strip() + if text.startswith("[") and text.endswith("]"): + text = text[1:-1] + return [name.strip() for name in text.split(",") if name.strip()] + + class MetadataData(BaseModel): """Key-value pair for simulation metadata.""" @@ -240,26 +266,20 @@ def convert_array_to_range(cls, data: Any) -> Any: data["value"] = _array_to_range(data["value"]) return data - @staticmethod - def _coerce_ids_list(v: str) -> list[str]: - """Accept the display-string form of the IDS list, ``"[a, b, c]"``.""" - text = v.strip() - if text.startswith("[") and text.endswith("]"): - text = text[1:-1] - return [name.strip() for name in text.split(",") if name.strip()] - @model_validator(mode="before") @classmethod - def fix_input_ids_string(cls, data: Any) -> Any: - """Convert funky ids input list strings to actual lists.""" + def fix_ids_list_string(cls, data: Any) -> Any: + """Convert funky ids list strings to actual lists. + + Applies to every key in :data:`IDS_LIST_KEYS`; see :func:`coerce_ids_list`. + """ if ( isinstance(data, dict) and "value" in data and "element" in data - and data["element"] == "input_ids" - and isinstance(data["value"], str) + and data["element"] in IDS_LIST_KEYS ): - data["value"] = MetadataData._coerce_ids_list(data["value"]) + data["value"] = coerce_ids_list(data["value"]) return data def as_dict(self) -> dict: diff --git a/tests/remote/api/test_metadata.py b/tests/remote/api/test_metadata.py index dbd99cee..6fdbe79c 100644 --- a/tests/remote/api/test_metadata.py +++ b/tests/remote/api/test_metadata.py @@ -1,10 +1,19 @@ +import pytest from conftest import ( HEADERS, generate_simulation_data, post_simulation, ) -from simdb.remote.models import MetadataKeyInfoList, MetadataValueList, RangeValue +from simdb.database.models.simulation import Simulation +from simdb.remote.models import ( + IDS_LIST_KEYS, + MetadataData, + MetadataKeyInfoList, + MetadataValueList, + RangeValue, + coerce_ids_list, +) def test_get_metadata_keys(client): @@ -85,3 +94,16 @@ def test_get_metadata_values_nonexistent_key(client): assert rv.status_code == 200 # Should return an empty list or list without the key assert isinstance(rv.json, list) + + +@pytest.mark.parametrize("element", ["ids", "input_ids"]) +def test_ids_list_display_string_is_coerced(element): + """Both IDS list keys accept the display-string form written by SimDB <= 1.2. + + These were stored as ``"[core_profiles, equilibrium]"`` rather than a list + (#119), and the coercion has to cover ``ids`` as well as ``input_ids``. + """ + meta = MetadataData.model_validate( + {"element": element, "value": "[core_profiles, equilibrium]"} + ) + assert meta.value == ["core_profiles", "equilibrium"] From 4313752e1412fa63207e91e7d7cd6698abf34f6c Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Thu, 3 Sep 2026 10:29:55 +0200 Subject: [PATCH 3/6] Self review --- src/simdb/cli/remote_api.py | 12 ++--- src/simdb/remote/apis/v1_2/simulations.py | 22 +++++++-- src/simdb/remote/core/path.py | 59 ++++++++++++++++++++--- src/simdb/remote/models.py | 3 +- 4 files changed, 75 insertions(+), 21 deletions(-) diff --git a/src/simdb/cli/remote_api.py b/src/simdb/cli/remote_api.py index 87d9bf6a..13e46398 100644 --- a/src/simdb/cli/remote_api.py +++ b/src/simdb/cli/remote_api.py @@ -75,6 +75,7 @@ WatcherGetResponse, WatcherPostRequest, WatcherPostResponse, + coerce_ids_list, ) from .manifest import DataType @@ -320,8 +321,9 @@ def _pagination_headers(limit: int, page: int = 1) -> Dict[str, str]: def _meta_list(value: Any) -> List[Any]: """ - Return a metadata value as the list of names the files endpoint expects. + Return an IDS metadata value as the list of names the files endpoint expects. """ + value = coerce_ids_list(value) if value is None: return [] if isinstance(value, (list, tuple)): @@ -827,12 +829,8 @@ def get_validation_schemas(self) -> List[Dict]: @versioned_method("v1.2", "v1.3") @try_request def get_upload_options(self) -> UploadOptions: - try: - res = self.get("upload_options") - return UploadOptions.model_validate_json(res.content) - except FailedConnection: - # old remotes may not provide this endpoint - return UploadOptions() + res = self.get("upload_options") + return UploadOptions.model_validate_json(res.content) @versioned_method("v1.2", "v1.3") @try_request diff --git a/src/simdb/remote/apis/v1_2/simulations.py b/src/simdb/remote/apis/v1_2/simulations.py index a8594490..62e14f13 100644 --- a/src/simdb/remote/apis/v1_2/simulations.py +++ b/src/simdb/remote/apis/v1_2/simulations.py @@ -20,7 +20,11 @@ from simdb.remote.core.auth import User, requires_auth from simdb.remote.core.cache import cache, cache_key, clear_cache from simdb.remote.core.errors import error -from simdb.remote.core.path import find_common_root, secure_path +from simdb.remote.core.path import ( + UnsafePathError, + find_common_root, + secure_path, +) from simdb.remote.core.pydantic_utils import ( Body, Header, @@ -163,6 +167,16 @@ def get_meta_val(key, default=None): return data +def _staging_path( + path: Path, common_root: Optional[Path], staging_dir: Path, is_file: bool = True +) -> Path: + """secure_path, reporting a path it refuses to stage as a client error.""" + try: + return secure_path(path, common_root, staging_dir, is_file=is_file) + except UnsafePathError as err: + raise ResponseException(str(err)) from err + + @api.route("/simulations") class SimulationList(Resource): @requires_auth() @@ -267,7 +281,7 @@ def post( and sim_file.uri.scheme == "file" and sim_file.uri.path is not None ): - path = secure_path( + path = _staging_path( Path(sim_file.uri.path), common_root, staging_dir ) if not path.exists(): @@ -278,11 +292,11 @@ def post( elif sim_file.uri.scheme == "imas": qs = dict(sim_file.uri.query_params()) if copy_files: - path = secure_path( + path = _staging_path( Path(qs["path"]), common_root, staging_dir, - is_file=common_root is not None, + is_file=False, ) else: path = Path(qs["path"]) diff --git a/src/simdb/remote/core/path.py b/src/simdb/remote/core/path.py index a750a1ad..505340a7 100644 --- a/src/simdb/remote/core/path.py +++ b/src/simdb/remote/core/path.py @@ -5,18 +5,61 @@ from werkzeug.utils import secure_filename +class UnsafePathError(ValueError): + """Raised for an uploaded path that would be staged outside the staging dir.""" + + +def _staged_relative(path: Path, common_root: Path) -> Path: + """ + Return *path* relative to *common_root*, as a path that stays inside it. + + Path.relative_to is purely lexical and keeps ".." segments, so "/data/../etc" + relative to "/data" is "../etc", which would stage the file outside the staging + directory entirely. The paths come from the uploaded simulation, and neither + they nor the common root derived from them are to be trusted. + """ + try: + relative = path.relative_to(common_root) + except ValueError as err: + raise UnsafePathError(f"path {path} is not below {common_root}") from err + + # normpath resolves the ".." segments that do stay inside, so that a path + # written as "run/../run" stages next to the one written as "run". + normalised = Path(os.path.normpath(relative)) + if normalised.is_absolute() or ".." in normalised.parts: + raise UnsafePathError(f"path {path} escapes {common_root}") + return normalised + + def secure_path( path: Path, common_root: Optional[Path], staging_dir: Path, is_file=True ) -> Path: + """ + Return the location under *staging_dir* that *path* is uploaded to. + + Directories keep their name so that the directory form agrees with the file + form: the parent of a staged file is the staged form of its directory. + + :param path: the path on the client, from the simulation being uploaded. + :param common_root: the root the uploaded paths share, or None if they share + none, in which case everything is flattened into *staging_dir*. + :param staging_dir: the directory the simulation is staged in. + :param is_file: True if *path* is a file, False if it is a directory. + :raises UnsafePathError: if *path* would be staged outside *staging_dir*. + """ + if not is_file: + if common_root is None: + return staging_dir + return staging_dir / _staged_relative(path, common_root) + + # secure_filename() empties a name that is entirely unusable, such as "..", + # which would otherwise hand back the directory holding the file. + name = secure_filename(path.name) + if not name: + raise UnsafePathError(f"path {path} has no usable file name") if common_root is None: - return staging_dir / secure_filename(path.name) if is_file else staging_dir - if is_file: - return ( - staging_dir - / path.parent.relative_to(common_root) - / secure_filename(path.name) - ) - return staging_dir / path.relative_to(common_root) + return staging_dir / name + return staging_dir / _staged_relative(path.parent, common_root) / name def find_common_root(paths: Collection[Path]) -> Optional[Path]: diff --git a/src/simdb/remote/models.py b/src/simdb/remote/models.py index 08e8ee53..eb345ef9 100644 --- a/src/simdb/remote/models.py +++ b/src/simdb/remote/models.py @@ -743,8 +743,7 @@ class TokenResponse(BaseModel): class UploadOptions(BaseModel): """Response from the upload options endpoint. - Describes how the server expects simulation data to be uploaded. Remotes that - do not provide the endpoint are treated as if they returned the defaults. + Describes how the server expects simulation data to be uploaded. """ copy_files: bool = True From ce8a23421c197a103d64a2dc8fa4ef221590614d Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Thu, 3 Sep 2026 10:33:28 +0200 Subject: [PATCH 4/6] Ruff --- tests/remote/api/test_metadata.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/remote/api/test_metadata.py b/tests/remote/api/test_metadata.py index 6fdbe79c..82ee1b32 100644 --- a/tests/remote/api/test_metadata.py +++ b/tests/remote/api/test_metadata.py @@ -5,14 +5,11 @@ post_simulation, ) -from simdb.database.models.simulation import Simulation from simdb.remote.models import ( - IDS_LIST_KEYS, MetadataData, MetadataKeyInfoList, MetadataValueList, RangeValue, - coerce_ids_list, ) From b30f593a352a0cb444af06abb5f4e513f02d4214 Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Thu, 3 Sep 2026 11:17:36 +0200 Subject: [PATCH 5/6] Add CLI tests --- tests/cli/cli_helpers.py | 34 + tests/cli/conftest.py | 159 ++++ tests/cli/test_cli_alias_command.py | 166 ++-- tests/cli/test_cli_config_command.py | 116 ++- tests/cli/test_cli_display.py | 337 ++++++++ tests/cli/test_cli_manifest_command.py | 105 +-- .../cli/test_cli_optional_remote_argument.py | 105 +++ tests/cli/test_cli_provenance_command.py | 70 +- tests/cli/test_cli_remote_api_client.py | 374 +++++++++ tests/cli/test_cli_remote_command.py | 730 +++++++++++------- tests/cli/test_cli_remote_config_command.py | 152 ++++ tests/cli/test_cli_root.py | 133 ++++ tests/cli/test_cli_simulation_command.py | 579 ++++++++++++-- tests/cli/test_cli_validators.py | 28 + tests/cli/test_remote_api_helpers.py | 42 + tests/cli/utils.py | 51 -- 16 files changed, 2594 insertions(+), 587 deletions(-) create mode 100644 tests/cli/cli_helpers.py create mode 100644 tests/cli/conftest.py create mode 100644 tests/cli/test_cli_display.py create mode 100644 tests/cli/test_cli_optional_remote_argument.py create mode 100644 tests/cli/test_cli_remote_api_client.py create mode 100644 tests/cli/test_cli_remote_config_command.py create mode 100644 tests/cli/test_cli_root.py create mode 100644 tests/cli/test_cli_validators.py create mode 100644 tests/cli/test_remote_api_helpers.py delete mode 100644 tests/cli/utils.py diff --git a/tests/cli/cli_helpers.py b/tests/cli/cli_helpers.py new file mode 100644 index 00000000..18a69a92 --- /dev/null +++ b/tests/cli/cli_helpers.py @@ -0,0 +1,34 @@ +"""Helpers shared by the CLI tests. + +Kept out of ``conftest.py`` because pytest imports every ``conftest`` under +the same module name, so importing from it directly picks up whichever one +was loaded first. +""" + +from typing import Optional +from unittest import mock + + +def make_simulation( + alias: str, + uuid: str = "0123456789abcdef0123456789abcdef", + datetime: str = "2000-01-01 00:00:00", + status: str = "not validated", + meta: Optional[dict] = None, +) -> mock.Mock: + """Build a stand-in for a :class:`~simdb.database.models.Simulation`. + + Only the attributes the CLI display code touches are set; ``find_meta`` + answers from ``meta`` the same way the real model does (a list of objects + with a ``value``, empty when the name is unknown). + """ + simulation = mock.Mock() + simulation.alias = alias + simulation.uuid = uuid + simulation.datetime = datetime + simulation.status = status + meta = meta or {} + simulation.find_meta.side_effect = lambda name: ( + [mock.Mock(value=meta[name])] if name in meta else [] + ) + return simulation diff --git a/tests/cli/conftest.py b/tests/cli/conftest.py new file mode 100644 index 00000000..ff2c5342 --- /dev/null +++ b/tests/cli/conftest.py @@ -0,0 +1,159 @@ +"""Shared fixtures for the ``simdb`` command line interface tests. + +Every test here drives the CLI through :class:`click.testing.CliRunner`, so the +fixtures below take care of the two things that would otherwise leak between +tests and into the developer's machine: the configuration that the CLI reads at +startup, and the handshake :class:`~simdb.cli.remote_api.RemoteAPI` performs +against a remote when it is constructed. +""" + +import os +from pathlib import Path +from types import SimpleNamespace +from unittest import mock + +import pytest +from click.testing import CliRunner, Result + +from simdb.cli.remote_api import RemoteAPI +from simdb.cli.simdb import cli + +REMOTE_NAME = "test" +REMOTE_URL = "http://0.0.0.0:5000/" +REMOTE_TOKEN = "123ABC" + +SERVER_ENDPOINTS = ["v1", "v1.1", "v1.1.1", "v1.2", "v1.3"] +"""API versions the fake remote advertises.""" + + +@pytest.fixture(autouse=True) +def isolated_config_environment(tmp_path, monkeypatch): + """Point the CLI at throw-away site and user configuration files. + + :class:`~simdb.config.config.Config` reads ``simdb.cfg`` from the platform + config directories unless ``SIMDB_SITE_CONFIG_PATH``/ + ``SIMDB_USER_CONFIG_PATH`` say otherwise. Without this fixture the outcome of + a test depends on whether the machine running it happens to have a real + SimDB configuration, which is exactly the kind of difference that makes a + suite pass locally and fail in CI. + """ + for variable in [name for name in os.environ if name.startswith("SIMDB_")]: + monkeypatch.delenv(variable) + monkeypatch.setenv("SIMDB_SITE_CONFIG_PATH", str(tmp_path / "site-simdb.cfg")) + monkeypatch.setenv("SIMDB_USER_CONFIG_PATH", str(tmp_path / "user-simdb.cfg")) + + +@pytest.fixture +def config_file(tmp_path) -> Path: + """A configuration file declaring a single, default, token-authenticated remote.""" + config_path = tmp_path / "simdb.cfg" + config_path.write_text( + f'[remote "{REMOTE_NAME}"]\n' + f"url = {REMOTE_URL}\n" + "default = True\n" + f"token = {REMOTE_TOKEN}\n" + "\n" + "[db]\n" + # Keep any command that reaches the real database away from the local + # one in the user's data directory. + f"file = {tmp_path / 'sim.db'}\n" + ) + return config_path + + +@pytest.fixture +def runner() -> CliRunner: + return CliRunner() + + +@pytest.fixture +def invoke(runner, config_file): + """Invoke the ``simdb`` CLI against the throw-away :func:`config_file`. + + ``invoke("simulation", "list")`` runs ``simdb --config-file=... simulation + list``. Any keyword argument is forwarded to + :meth:`click.testing.CliRunner.invoke`, so ``input=`` can be used to answer + prompts. + """ + + def _invoke(*args: str, **kwargs) -> Result: + return runner.invoke(cli, [f"--config-file={config_file}", *args], **kwargs) + + return _invoke + + +@pytest.fixture +def remote_handshake(): + """Stub the requests :class:`RemoteAPI` makes while it is being constructed. + + ``RemoteAPI.__init__`` asks the remote for its authentication scheme, its + endpoints, and its server version before any command specific request is + made. Tests that only care about the command itself get all three stubbed + here, and can still assert on them through the returned namespace:: + + def test_something(invoke, remote_handshake): + ... + assert remote_handshake.get_endpoints.called + """ + with mock.patch.object( + RemoteAPI, "get_server_authentication", return_value="None" + ) as get_server_authentication, mock.patch.object( + RemoteAPI, "get_endpoints", return_value=list(SERVER_ENDPOINTS) + ) as get_endpoints, mock.patch.object( + RemoteAPI, "get_server_version", return_value="0.11" + ) as get_server_version: + yield SimpleNamespace( + get_server_authentication=get_server_authentication, + get_endpoints=get_endpoints, + get_server_version=get_server_version, + ) + + +@pytest.fixture +def local_db(): + """Replace the local database with a mock in every module that looks it up. + + ``get_local_db`` is imported into each command module, so patching a single + import site silently leaves the other commands talking to the real database + in the user's data directory. + """ + db = mock.Mock() + with mock.patch( + "simdb.cli.commands.alias.get_local_db", return_value=db + ), mock.patch("simdb.cli.commands.simulation.get_local_db", return_value=db): + yield db + + +@pytest.fixture +def data_file(tmp_path) -> Path: + """A small file a manifest can reference as an input or output.""" + path = tmp_path / "data.txt" + path.write_text("simulation data\n") + return path + + +@pytest.fixture +def manifest_file(tmp_path, data_file) -> Path: + """A minimal, valid manifest referencing only local files.""" + manifest_path = tmp_path / "manifest.yaml" + manifest_path.write_text( + f"""\ +manifest_version: 2 +alias: simulation-alias + +inputs: + - uri: file://{data_file} + +outputs: + - uri: file://{data_file} + +metadata: +- values: + workflow: + name: Workflow Name + git: ssh://git@git.iter.org/wf/workflow.git + branch: master + commit: 079e84d5ae8a0eec6dcf3819c98f3c05f48e952f +""" + ) + return manifest_path diff --git a/tests/cli/test_cli_alias_command.py b/tests/cli/test_cli_alias_command.py index c173dfd6..a85a6efd 100644 --- a/tests/cli/test_cli_alias_command.py +++ b/tests/cli/test_cli_alias_command.py @@ -1,80 +1,110 @@ -from unittest import mock +"""Tests for ``simdb alias``.""" -from click.testing import CliRunner -from utils import config_test_file +from unittest import mock -from simdb.cli.simdb import cli +import pytest +from cli_helpers import make_simulation LOCAL_ALIASES = ["hello", "world", "foo-123"] REMOTE_ALIASES = ["foo#1", "bar", "barfoo", "123foo", "barbaz"] -def _generate_mock_data(get_local_db, remote_list_simulations): - simulations = [] - - for alias in REMOTE_ALIASES: - sim = mock.Mock() - sim.alias = alias - simulations.append(sim) - remote_list_simulations.return_value = simulations - simulations = [] - - for alias in LOCAL_ALIASES: - sim = mock.Mock() - sim.alias = alias - simulations.append(sim) - get_local_db.return_value.list_simulations.return_value = simulations - - -@mock.patch("simdb.cli.commands.alias.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI.list_simulations") -@mock.patch("simdb.cli.remote_api.RemoteAPI.__init__") -def test_alias_search_command(init, remote_list_simulations, get_local_db): - init.return_value = None - _generate_mock_data(get_local_db, remote_list_simulations) - - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, [f"--config-file={config_file}", "alias", "search", "foo"] - ) - assert result.exception is None - expected_sims = ["foo#1", "barfoo", "123foo", "foo-123"] - assert "\n".join(expected_sims) in result.output - - -@mock.patch("simdb.cli.commands.alias.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI.list_simulations") -@mock.patch("simdb.cli.remote_api.RemoteAPI.has_url") -@mock.patch("simdb.cli.remote_api.RemoteAPI.__init__") -def test_alias_list_command(init, has_url, remote_list_simulations, get_local_db): - init.return_value = None - has_url.return_value = True - _generate_mock_data(get_local_db, remote_list_simulations) - - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "alias", "list"]) - assert result.exception is None +@pytest.fixture +def aliases(local_db, remote_handshake): + """A local database and a remote, each holding a known set of aliases.""" + local_db.list_simulations.return_value = [ + make_simulation(alias) for alias in LOCAL_ALIASES + ] + with mock.patch( + "simdb.cli.remote_api.RemoteAPI.list_simulations", + return_value=[make_simulation(alias) for alias in REMOTE_ALIASES], + ), mock.patch("simdb.cli.remote_api.RemoteAPI.has_url", return_value=True): + yield + + +def test_search_returns_local_and_remote_matches(invoke, aliases): + result = invoke("alias", "search", "foo") + + assert result.exit_code == 0 + assert "\n".join(["foo#1", "barfoo", "123foo", "foo-123"]) in result.output + + +def test_search_without_matches_prints_nothing(invoke, aliases): + result = invoke("alias", "search", "nothing-matches-this") + + assert result.exit_code == 0 + for alias in LOCAL_ALIASES + REMOTE_ALIASES: + assert alias not in result.output + + +def test_list_shows_the_local_aliases(invoke, aliases): + result = invoke("alias", "list") + + assert result.exit_code == 0 assert "\n ".join(LOCAL_ALIASES) in result.output -@mock.patch("simdb.cli.commands.alias.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI.list_simulations") -@mock.patch("simdb.cli.remote_api.RemoteAPI.has_url") -@mock.patch("simdb.cli.remote_api.RemoteAPI.__init__") -def test_alias_list_command_with_remote_name( - init, has_url, remote_list_simulations, get_local_db -): - init.return_value = None - has_url.return_value = True - _generate_mock_data(get_local_db, remote_list_simulations) - - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, [f"--config-file={config_file}", "alias", "test", "list"] - ) - assert result.exception is None +def test_list_with_a_remote_name_shows_both_sides(invoke, aliases): + result = invoke("alias", "test", "list") + + assert result.exit_code == 0 assert "\n ".join(REMOTE_ALIASES) in result.output assert "\n ".join(LOCAL_ALIASES) in result.output + + +def test_list_can_skip_the_remote(invoke, aliases): + result = invoke("alias", "list", "--local") + + assert result.exit_code == 0 + assert "Remote:" not in result.output + assert "\n ".join(LOCAL_ALIASES) in result.output + + +def test_list_explains_a_remote_without_a_url(invoke, local_db, remote_handshake): + local_db.list_simulations.return_value = [] + with mock.patch("simdb.cli.remote_api.RemoteAPI.has_url", return_value=False): + result = invoke("alias", "list") + + assert result.exit_code == 0 + assert "The Remote Server has not been specified" in result.output + + +def test_make_unique_returns_an_unused_alias_unchanged(invoke, aliases): + result = invoke("alias", "make-unique", "brand-new") + + assert result.exit_code == 0 + assert result.output.strip() == "brand-new" + + +def test_make_unique_appends_a_counter_to_a_taken_alias(invoke, aliases): + result = invoke("alias", "make-unique", "bar") + + assert result.exit_code == 0 + assert result.output.strip() == "bar-1" + + +def test_make_unique_replaces_reserved_characters(invoke, aliases): + result = invoke("alias", "make-unique", "a#b/c(d)e=f,g*h%i") + + assert result.exit_code == 0 + assert result.output.strip() == "a_b_c_d_e_f_g_h_i" + + +def test_make_unique_keeps_counting_past_a_taken_suffix(invoke, aliases, local_db): + local_db.list_simulations.return_value = [ + make_simulation("bar"), + make_simulation("bar-1"), + ] + + result = invoke("alias", "make-unique", "bar") + + assert result.exit_code == 0 + assert result.output.strip() == "bar-2" + + +def test_the_group_shows_help_when_no_subcommand_is_given(invoke): + result = invoke("alias") + + assert result.exit_code == 0 + assert "Query remote and local aliases." in result.output + assert "make-unique" in result.output diff --git a/tests/cli/test_cli_config_command.py b/tests/cli/test_cli_config_command.py index 1d278d9b..853c1b10 100644 --- a/tests/cli/test_cli_config_command.py +++ b/tests/cli/test_cli_config_command.py @@ -1,36 +1,96 @@ +"""Tests for ``simdb config``.""" + from unittest import mock -from click.testing import CliRunner -from utils import config_test_file +import pytest from simdb.cli.simdb import cli -@mock.patch("simdb.config.config.Config.get_option") -def test_config_get(get_option): - config_file = config_test_file() - get_option.return_value = "bar" - runner = CliRunner() - result = runner.invoke( - cli, [f"--config-file={config_file}", "config", "get", "foo"] - ) - assert result.exception is None +def test_get_prints_the_option_value(invoke): + with mock.patch( + "simdb.config.config.Config.get_option", return_value="bar" + ) as get_option: + result = invoke("config", "get", "foo") + + assert result.exit_code == 0 assert "bar" in result.output - (args, kwargs) = get_option.call_args - assert args == ("foo",) - assert kwargs == {} - - -@mock.patch("simdb.config.config.Config.save") -@mock.patch("simdb.config.config.Config.set_option") -def test_config_set(set_option, save): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, [f"--config-file={config_file}", "config", "set", "foo", "bar"] - ) - assert result.exception is None - (args, kwargs) = set_option.call_args - assert args == ("foo", "bar") - assert kwargs == {} + # Config.load also reads options, so only the final call is the command's. + assert get_option.call_args.args == ("foo",) + + +def test_get_reads_from_the_loaded_config_file(invoke): + result = invoke("config", "get", "remote.test.url") + + assert result.exit_code == 0 + assert "http://0.0.0.0:5000/" in result.output + + +def test_get_fails_for_an_unknown_option(invoke): + result = invoke("config", "get", "no.such.option") + + assert result.exit_code != 0 + + +def test_set_stores_the_option_and_saves(invoke): + with mock.patch("simdb.config.config.Config.save") as save, mock.patch( + "simdb.config.config.Config.set_option" + ) as set_option: + result = invoke("config", "set", "foo", "bar") + + assert result.exit_code == 0 + # Config.load sets options for the SIMDB_* environment variables first. + assert set_option.call_args.args == ("foo", "bar") + assert save.called + + +def test_delete_removes_the_option_and_saves(invoke): + with mock.patch("simdb.config.config.Config.save") as save, mock.patch( + "simdb.config.config.Config.delete_option" + ) as delete_option: + result = invoke("config", "delete", "foo") + + assert result.exit_code == 0 + assert "Success." in result.output + delete_option.assert_called_once_with("foo") assert save.called + + +def test_list_shows_the_configured_options(invoke): + result = invoke("config", "list") + + assert result.exit_code == 0 + assert "remote.test.url: http://0.0.0.0:5000/" in result.output + + +def test_list_masks_remote_tokens(invoke): + """A token in the configuration must never be echoed back in full.""" + result = invoke("config", "list") + + assert result.exit_code == 0 + assert "remote.test.token: ********" in result.output + assert "123ABC" not in result.output + + +def test_path_prints_the_config_file_that_was_loaded(invoke, tmp_path): + """``--config-file`` replaces the user configuration rather than adding to it.""" + result = invoke("config", "path") + + assert result.exit_code == 0 + assert str(tmp_path / "simdb.cfg") in result.output + + +def test_path_falls_back_to_the_user_configuration(runner, tmp_path): + """Without ``--config-file`` the location comes from SIMDB_USER_CONFIG_PATH.""" + result = runner.invoke(cli, ["config", "path"]) + + assert result.exit_code == 0 + assert str(tmp_path / "user-simdb.cfg") in result.output + + +@pytest.mark.parametrize("subcommand", ["get", "set", "delete"]) +def test_missing_arguments_are_reported(invoke, subcommand): + result = invoke("config", subcommand) + + assert result.exit_code == 2 + assert "Missing argument" in result.output diff --git a/tests/cli/test_cli_display.py b/tests/cli/test_cli_display.py new file mode 100644 index 00000000..f88bf18b --- /dev/null +++ b/tests/cli/test_cli_display.py @@ -0,0 +1,337 @@ +"""Tests for the console output helpers in ``simdb.cli.commands.utils``. + +These are the functions that turn simulations and IDS quantities into what the +user actually sees. They are pure enough to call directly, so they are tested +here without going through a command. +""" + +import pytest +from cli_helpers import make_simulation +from rich.console import Console + +from simdb.cli.commands import utils +from simdb.cli.commands.utils import ( + is_numeric_1d, + print_quantity, + print_simulations, + print_trace, + show_quantity_textual_plot, +) +from simdb.remote.models import QuantityData, SimulationTraceData + + +@pytest.fixture(autouse=True) +def fixed_width_console(monkeypatch): + """Give the rich output a stable width so assertions do not depend on the + terminal the tests happen to run in.""" + monkeypatch.setattr( + utils, "_RICH_CONSOLE", Console(width=120, legacy_windows=False) + ) + + +def quantity(data, name="grid/rho_tor_norm", units="-") -> QuantityData: + return QuantityData(name=name, units=units, data=data) + + +# --------------------------------------------------------------------------- +# is_numeric_1d +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("data", "expected"), + [ + ([1, 2, 3], True), + ([1.0, 2.5], True), + ([], False), + ([[1, 2], [3, 4]], False), + (["a", "b"], False), + # bool is an int subclass, but plotting booleans is not what the caller + # means by "numeric". + ([True, False], False), + (1.0, False), + (None, False), + ], +) +def test_is_numeric_1d(data, expected): + assert is_numeric_1d(data) is expected + + +# --------------------------------------------------------------------------- +# print_quantity +# --------------------------------------------------------------------------- + + +def test_print_quantity_renders_a_scalar(capsys): + print_quantity(quantity(1.23456789, name="time", units="s")) + + output = capsys.readouterr().out + assert "1.23457" in output + assert "scalar" in output + + +def test_print_quantity_renders_a_one_dimensional_array_with_stats(capsys): + print_quantity(quantity([0.0, 0.5, 1.0])) + + output = capsys.readouterr().out + assert "shape (3,)" in output + for column in ("n", "min", "max", "mean", "std", "median"): + assert column in output + assert "0.5" in output + + +def test_print_quantity_truncates_long_rows(capsys): + print_quantity(quantity(list(range(100)))) + + output = capsys.readouterr().out + assert "..." in output + assert "shape (100,)" in output + # Head and tail are kept, the middle is not. + assert "0 1 2" in output + assert "97 98 99" in output + assert "50" not in output + + +def test_print_quantity_renders_a_two_dimensional_array(capsys): + print_quantity(quantity([[1, 2], [3, 4]])) + + output = capsys.readouterr().out + assert "shape (2, 2)" in output + + +def test_print_quantity_truncates_tall_two_dimensional_arrays(capsys): + print_quantity(quantity([[row, row] for row in range(20)])) + + output = capsys.readouterr().out + assert "shape (20, 2)" in output + assert "..." in output + + +def test_print_quantity_summarises_higher_dimensional_arrays(capsys): + print_quantity(quantity([[[1, 2], [3, 4]], [[5, 6], [7, 8]]])) + + output = capsys.readouterr().out + assert "3-D array" in output + + +def test_print_quantity_can_omit_the_stats_table(capsys): + print_quantity(quantity([0.0, 0.5, 1.0]), show_stats=False) + + output = capsys.readouterr().out + assert "median" not in output + + +def test_print_quantity_uses_the_label_over_the_name(capsys): + print_quantity(quantity([1, 2, 3], name="grid/rho_tor_norm"), label="field") + + output = capsys.readouterr().out + assert "field" in output + assert "rho_tor_norm" not in output + + +def test_print_quantity_falls_back_to_a_dash_for_missing_units(capsys): + print_quantity(quantity(1.0, units="")) + + assert "[-]" in capsys.readouterr().out + + +def test_stats_are_omitted_for_a_single_value(capsys): + print_quantity(quantity([42.0])) + + output = capsys.readouterr().out + assert "shape (1,)" in output + assert "median" not in output + + +# --------------------------------------------------------------------------- +# show_quantity_textual_plot +# --------------------------------------------------------------------------- + + +def test_plot_is_drawn_for_a_numeric_field(capsys): + show_quantity_textual_plot(quantity([0.0, 1.0, 4.0, 9.0]), label="field") + + output = capsys.readouterr().out + assert "index [-]" in output + assert "shape (4,)" in output + + +def test_plot_uses_a_matching_coordinate_as_the_x_axis(capsys): + show_quantity_textual_plot( + quantity([0.0, 1.0, 4.0, 9.0]), + label="field", + x_quantity=quantity([0, 1, 2, 3], name="profiles_1d/time", units="s"), + ) + + output = capsys.readouterr().out + assert "time [s]" in output + assert "index [-]" not in output + + +def test_plot_ignores_a_coordinate_of_a_different_length(capsys): + show_quantity_textual_plot( + quantity([0.0, 1.0, 4.0]), + x_quantity=quantity([0, 1], name="time", units="s"), + ) + + assert "index [-]" in capsys.readouterr().out + + +def test_non_numeric_data_is_printed_instead_of_plotted(capsys): + show_quantity_textual_plot(quantity(["a", "b"]), label="field") + + output = capsys.readouterr().out + assert "index [-]" not in output + assert "shape (2,)" in output + + +# --------------------------------------------------------------------------- +# print_simulations +# --------------------------------------------------------------------------- + + +def test_print_simulations_reports_an_empty_list(capsys): + print_simulations([]) + + assert "No simulations found" in capsys.readouterr().out + + +def test_print_simulations_prints_a_single_alias_column(capsys): + print_simulations([make_simulation("first"), make_simulation("second")]) + + output = capsys.readouterr().out + assert "alias" in output + assert "first" in output + assert "second" in output + assert "UUID" not in output + assert "status" not in output + + +def test_print_simulations_adds_datetime_and_status_when_verbose(capsys): + print_simulations([make_simulation("first", status="passed")], verbose=True) + + output = capsys.readouterr().out + assert "datetime" in output + assert "status" in output + assert "passed" in output + + +def test_print_simulations_adds_the_uuid_column_on_request(capsys): + print_simulations([make_simulation("first", uuid="abcd1234")], show_uuid=True) + + output = capsys.readouterr().out + assert "UUID" in output + assert "abcd1234" in output + + +def test_print_simulations_adds_a_column_per_metadata_name(capsys): + simulations = [ + make_simulation("first", meta={"pulse": 134173}), + make_simulation("second", meta={}), + ] + + print_simulations(simulations, metadata_names=["pulse"]) + + output = capsys.readouterr().out + assert "pulse" in output + assert "134173" in output + # A simulation without the metadata still gets a row. + assert "second" in output + + +def test_print_simulations_handles_a_missing_alias(capsys): + print_simulations([make_simulation(None, uuid="abcd1234")], show_uuid=True) + + assert "abcd1234" in capsys.readouterr().out + + +def test_print_simulations_hints_at_the_limit_when_a_full_page_is_returned(capsys): + simulations = [make_simulation(f"sim{index}") for index in range(100)] + + print_simulations(simulations) + + assert "first 100 entries shown" in capsys.readouterr().out + + +def test_print_simulations_does_not_hint_below_the_limit(capsys): + print_simulations([make_simulation(f"sim{index}") for index in range(99)]) + + assert "first 100 entries shown" not in capsys.readouterr().out + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + ({"min": 1, "max": 5}, "[1, 5]"), + ([1.0, 2.0], "[1.00, 2.00]"), + ([True, False], "[True, False]"), + (["a", "b"], "[a, b]"), + # Long lists are truncated to five entries. + (list(range(10)), "[0.00, 1.00, 2.00, 3.00, 4.00, ...]"), + ("plain", "plain"), + ], +) +def test_metadata_values_are_formatted_for_the_table(capsys, value, expected): + print_simulations( + [make_simulation("sim", meta={"field": value})], metadata_names=["field"] + ) + + assert expected in capsys.readouterr().out + + +# --------------------------------------------------------------------------- +# print_trace +# --------------------------------------------------------------------------- + + +def test_print_trace_reports_a_missing_trace(capsys): + print_trace(None) + + assert "No simulations trace found" in capsys.readouterr().out + + +def test_print_trace_prints_the_simulation_and_its_status_date(capsys): + trace = SimulationTraceData( + alias="current", status="passed", passed_on="2024-01-01" + ) + + print_trace(trace) + + output = capsys.readouterr().out + assert "current" in output + assert "passed" in output + assert "Passed on: 2024-01-01" in output + + +def test_print_trace_reports_an_unknown_status(capsys): + print_trace(SimulationTraceData(alias="current")) + + assert "Status: unknown" in capsys.readouterr().out + + +def test_print_trace_indents_the_replaced_simulation(capsys): + trace = SimulationTraceData( + alias="current", + status="passed", + replaces=SimulationTraceData(alias="older", status="deprecated"), + replaces_reason="superseded", + ) + + print_trace(trace) + + output = capsys.readouterr().out + assert "Replaces: (reason: superseded)" in output + assert " Simulation:" in output + assert "older" in output + + +def test_print_trace_handles_a_replacement_without_a_reason(capsys): + trace = SimulationTraceData( + alias="current", replaces=SimulationTraceData(alias="older") + ) + + print_trace(trace) + + output = capsys.readouterr().out + assert "Replaces:" in output + assert "reason" not in output diff --git a/tests/cli/test_cli_manifest_command.py b/tests/cli/test_cli_manifest_command.py index efc96740..6408ab21 100644 --- a/tests/cli/test_cli_manifest_command.py +++ b/tests/cli/test_cli_manifest_command.py @@ -1,54 +1,65 @@ -from unittest import mock +"""Tests for ``simdb manifest``.""" -from click.testing import CliRunner -from utils import config_test_file, create_manifest, get_file_path +import yaml -from simdb.cli.simdb import cli +from simdb.cli.manifest import Manifest -@mock.patch("simdb.cli.commands.manifest.Manifest") -def test_manifest_check_command(manifest): - config_file = config_test_file() - runner = CliRunner() - file_name = create_manifest() - result = runner.invoke( - cli, [f"--config-file={config_file}", "manifest", "check", str(file_name)] - ) - assert result.exception is None - assert "ok" in result.output - assert manifest.load_from_file.called - (args, kwargs) = manifest.load_from_file.call_args - assert str(args[0]) == str(file_name) - assert kwargs == {} - - -def test_manifest_check_command_integration(): - """Integration test that actually runs the manifest check without mocking.""" - config_file = config_test_file() - runner = CliRunner() - file_name = create_manifest() - result = runner.invoke( - cli, [f"--config-file={config_file}", "manifest", "check", str(file_name)] - ) - assert result.exception is None, f"Unexpected exception: {result.exception}" - assert result.exit_code == 0, ( - f"Exit code: {result.exit_code}, Output: {result.output}" - ) +def test_check_accepts_a_valid_manifest(invoke, manifest_file): + result = invoke("manifest", "check", str(manifest_file)) + + assert result.exit_code == 0 assert "ok" in result.output -@mock.patch("simdb.cli.commands.manifest.Manifest") -def test_manifest_create_command(manifest): - config_file = config_test_file() - runner = CliRunner() - file_name = get_file_path("manifest.yaml") - result = runner.invoke( - cli, [f"--config-file={config_file}", "manifest", "create", str(file_name)] - ) - assert result.exception is None - assert str(file_name) in result.output - assert manifest.from_template.called - assert manifest.from_template().save.called - (args, kwargs) = manifest.from_template().save.call_args - assert args[0].name == str(file_name) - assert kwargs == {} +def test_check_rejects_an_invalid_manifest(invoke, tmp_path): + manifest_file = tmp_path / "broken.yaml" + manifest_file.write_text("manifest_version: 2\nalias: 'not a valid alias'\n") + + result = invoke("manifest", "check", str(manifest_file)) + + assert result.exit_code != 0 + assert "illegal characters in alias" in str(result.exception) + + +def test_check_requires_the_file_to_exist(invoke, tmp_path): + result = invoke("manifest", "check", str(tmp_path / "missing.yaml")) + + assert result.exit_code == 2 + assert "does not exist" in result.output + + +def test_create_writes_a_manifest_from_the_template(invoke, tmp_path): + manifest_file = tmp_path / "new-manifest.yaml" + + result = invoke("manifest", "create", str(manifest_file)) + + assert result.exit_code == 0 + assert str(manifest_file) in result.output + assert manifest_file.exists() + assert yaml.safe_load(manifest_file.read_text())["manifest_version"] == 2 + + +def test_a_created_manifest_carries_the_template_placeholders(invoke, tmp_path): + """``create`` writes a skeleton, so ``check`` still has something to report. + + The template points at ``/home/user/path/to/a/file1`` and friends, which the + user is expected to replace; checking it unedited must say so rather than + pass silently. + """ + manifest_file = tmp_path / "new-manifest.yaml" + assert invoke("manifest", "create", str(manifest_file)).exit_code == 0 + + result = invoke("manifest", "check", str(manifest_file)) + + assert result.exit_code != 0 + assert "No files found matching path" in str(result.exception) + + +def test_check_loads_the_manifest_it_was_given(invoke, manifest_file): + """``check`` reports on the requested file, not on a default one.""" + loaded = Manifest.load_from_file(manifest_file) + + assert loaded.alias == "simulation-alias" + assert len(loaded.inputs) == 1 + assert len(loaded.outputs) == 1 diff --git a/tests/cli/test_cli_optional_remote_argument.py b/tests/cli/test_cli_optional_remote_argument.py new file mode 100644 index 00000000..25d9e4a1 --- /dev/null +++ b/tests/cli/test_cli_optional_remote_argument.py @@ -0,0 +1,105 @@ +"""The optional ``REMOTE`` argument of ``simdb simulation`` sub-commands. + +``simulation push``, ``pull``, ``data`` and ``validate`` all take an optional +REMOTE before their required arguments. Click cannot express "optional first +argument", so :class:`OptionalRemoteCommand` fills in an empty REMOTE when the +command line does not provide a value for every argument. + +Counting raw command line entries instead is what made ``push SIM_ID --replaces +=x`` fail with "Missing argument 'SIM_ID'": the option was counted as an +argument, so no empty REMOTE was inserted. +""" + +from unittest import mock + +import pytest +from cli_helpers import make_simulation + +SUBCOMMANDS = [ + ("push", ()), + ("validate", ()), + ("pull", ("directory",)), + ("data", ("core_profiles/time",)), +] +"""Each sub-command and the arguments that follow its SIM_ID.""" + + +@pytest.fixture +def remote_api(): + with mock.patch("simdb.cli.commands.simulation.RemoteAPI") as remote_api_cls: + remote_api_cls.return_value.get_validation_schemas.return_value = [] + yield remote_api_cls + + +@pytest.fixture(autouse=True) +def a_simulation_exists(local_db): + local_db.get_simulation.return_value = make_simulation("sim") + local_db.get_simulation.side_effect = None + return local_db + + +@pytest.mark.parametrize(("subcommand", "trailing"), SUBCOMMANDS) +@pytest.mark.parametrize( + "options", [("--username", "bob"), ()], ids=["username", "no-options"] +) +@pytest.mark.parametrize( + "options_first", [True, False], ids=["options-first", "options-last"] +) +@pytest.mark.parametrize( + "remote", [("test",), ()], ids=["named-remote", "default-remote"] +) +def test_the_remote_may_be_omitted( + invoke, remote_api, remote, options_first, options, subcommand, trailing +): + """REMOTE may be left out, wherever the options appear on the command line.""" + arguments = (*remote, "sim", *trailing) + argv = (*options, *arguments) if options_first else (*arguments, *options) + + result = invoke("simulation", subcommand, *argv) + + assert remote_api.called, result.output + used_remote, used_username = remote_api.call_args.args[:2] + assert used_remote == (remote[0] if remote else "") + assert used_username == ("bob" if options else None) + + +@pytest.mark.parametrize( + ("subcommand", "arguments", "option"), + [ + ("push", ["sim"], "--replaces=older"), + ("push", ["sim"], "--add-watcher"), + ("data", ["sim", "core_profiles/time"], "--dd-version=4.1.1"), + ], +) +def test_a_command_specific_option_does_not_consume_the_remote( + invoke, remote_api, subcommand, arguments, option +): + """Options that are not shared by every sub-command must count the same way.""" + result = invoke("simulation", subcommand, *arguments, option) + + assert "Missing argument" not in result.output + assert remote_api.call_args.args[0] == "" + + +@pytest.mark.parametrize( + ("subcommand", "trailing"), + [(subcommand, trailing) for subcommand, trailing in SUBCOMMANDS if trailing], +) +def test_a_genuinely_missing_argument_is_still_reported( + invoke, remote_api, subcommand, trailing +): + """Filling in the REMOTE must not paper over an argument the user forgot.""" + result = invoke("simulation", subcommand, "sim", *trailing[:-1]) + + assert result.exit_code == 2 + assert "Missing argument" in result.output + assert not remote_api.called + + +@pytest.mark.parametrize("subcommand", [subcommand for subcommand, _ in SUBCOMMANDS]) +def test_a_command_with_no_arguments_at_all_is_reported(invoke, remote_api, subcommand): + result = invoke("simulation", subcommand) + + assert result.exit_code == 2 + assert "Missing argument" in result.output + assert not remote_api.called diff --git a/tests/cli/test_cli_provenance_command.py b/tests/cli/test_cli_provenance_command.py index a68026f7..7d4cfe20 100644 --- a/tests/cli/test_cli_provenance_command.py +++ b/tests/cli/test_cli_provenance_command.py @@ -1,22 +1,48 @@ -from unittest import mock - -from click.testing import CliRunner -from utils import config_test_file, get_file_path - -from simdb.cli.simdb import cli - - -@mock.patch("yaml.dump") -def test_provenance_command(dump): - config_file = config_test_file() - runner = CliRunner() - file_name = get_file_path("provenance.yaml") - result = runner.invoke( - cli, [f"--config-file={config_file}", "provenance", str(file_name)] - ) - assert result.exception is None - assert str(file_name) in result.output - assert dump.called - (args, kwargs) = dump.call_args - assert args[1].name == str(file_name) - assert kwargs == {"default_flow_style": False} +"""Tests for ``simdb provenance``.""" + +import yaml + + +def test_provenance_writes_a_yaml_description_of_the_system(invoke, tmp_path): + provenance_file = tmp_path / "provenance.yaml" + + result = invoke("provenance", str(provenance_file)) + + assert result.exit_code == 0 + assert str(provenance_file) in result.output + + provenance = yaml.safe_load(provenance_file.read_text()) + assert set(provenance) == {"environment", "platform"} + assert provenance["platform"]["system"] + assert provenance["platform"]["python_version"] + + +def test_path_like_environment_variables_are_split_into_lists( + invoke, tmp_path, monkeypatch +): + monkeypatch.setenv("SIMDB_TEST_PATH", "/first:/second") + provenance_file = tmp_path / "provenance.yaml" + + assert invoke("provenance", str(provenance_file)).exit_code == 0 + + environment = yaml.safe_load(provenance_file.read_text())["environment"] + assert environment["SIMDB_TEST_PATH"] == ["/first", "/second"] + + +def test_other_environment_variables_are_kept_as_strings(invoke, tmp_path, monkeypatch): + monkeypatch.setenv("SIMDB_TEST_VALUE", "plain") + provenance_file = tmp_path / "provenance.yaml" + + assert invoke("provenance", str(provenance_file)).exit_code == 0 + + environment = yaml.safe_load(provenance_file.read_text())["environment"] + assert environment["SIMDB_TEST_VALUE"] == "plain" + + +def test_the_file_is_overwritten_on_a_second_run(invoke, tmp_path): + provenance_file = tmp_path / "provenance.yaml" + provenance_file.write_text("stale: true\n") + + assert invoke("provenance", str(provenance_file)).exit_code == 0 + + assert "stale" not in provenance_file.read_text() diff --git a/tests/cli/test_cli_remote_api_client.py b/tests/cli/test_cli_remote_api_client.py new file mode 100644 index 00000000..39116a9c --- /dev/null +++ b/tests/cli/test_cli_remote_api_client.py @@ -0,0 +1,374 @@ +"""Tests for the HTTP layer of :class:`simdb.cli.remote_api.RemoteAPI`. + +The other CLI tests stub ``RemoteAPI`` methods so they can concentrate on the +commands. Here it is the client itself that is under test, so only ``requests`` +is replaced: construction, authentication, URL building, error translation and +response parsing all run for real. +""" + +import json +from unittest import mock + +import pytest +import requests +from pydantic import ValidationError + +from simdb.cli.remote_api import ( + FailedConnection, + RemoteAPI, + RemoteError, + check_return, + try_request, +) +from simdb.config import Config +from simdb.remote.models import TokenResponse + +INDEX = { + "api": "SimDB", + "api_version": "1.3", + "server_version": "0.11", + "endpoints": ["http://remote.test/v1.2", "http://remote.test/v1.3"], + "authentication": "None", +} + + +def response(payload=None, status=200, content=None) -> requests.Response: + """Build a real :class:`requests.Response` around a JSON payload.""" + res = requests.Response() + res.status_code = status + res.url = "http://remote.test/" + res.reason = "OK" if status == 200 else "Error" + if content is None: + content = json.dumps(payload if payload is not None else {}).encode() + res._content = content + return res + + +class FakeHttp: + """Answer ``requests.get``/``post`` from a URL suffix to payload mapping.""" + + def __init__(self, index=None): + self.routes = {"": index if index is not None else dict(INDEX)} + self.calls = [] + + def route(self, suffix, payload=None, status=200, content=None): + self.routes[suffix] = response(payload, status=status, content=content) + + def _respond(self, method, url, **kwargs): + self.calls.append(mock.call(method, url, **kwargs)) + suffix = url.split("/v1.3/", 1)[-1] if "/v1.3/" in url else "" + if url.rstrip("/") in ("http://remote.test", "http://remote.test/"): + suffix = "" + route = self.routes.get(suffix) + if route is None: + return response({"error": f"no route for {url!r}"}, status=404) + return route if isinstance(route, requests.Response) else response(route) + + def request_for(self, suffix): + """The recorded call whose URL ends with the given suffix.""" + for call in self.calls: + if call.args[1].endswith(suffix): + return call + raise AssertionError(f"no request was made to {suffix!r}") + + +@pytest.fixture +def http(monkeypatch): + fake = FakeHttp() + for method in ("get", "post", "put", "delete"): + monkeypatch.setattr( + requests, + method, + lambda url, _method=method, **kwargs: fake._respond(_method, url, **kwargs), + ) + return fake + + +def make_config(**options) -> Config: + config = Config() + config.set_option("remote.test.url", "http://remote.test") + for name, value in options.items(): + config.set_option(f"remote.test.{name}", value) + return config + + +# --------------------------------------------------------------------------- +# construction +# --------------------------------------------------------------------------- + + +def test_the_negotiated_version_is_used_for_requests(http): + api = RemoteAPI("test", None, None, make_config()) + + assert api._api_url == "http://remote.test/v1.3/" + assert api.remote == "test" + assert api.has_url() is True + + +def test_the_default_remote_is_used_when_no_name_is_given(http): + config = make_config() + config.default_remote = "test" + + api = RemoteAPI(None, None, None, config) + + assert api.remote == "test" + + +def test_a_missing_remote_name_without_a_default_is_reported(http): + with pytest.raises(KeyError, match="no default remote"): + RemoteAPI(None, None, None, Config()) + + +def test_an_unknown_remote_is_reported(http): + with pytest.raises(ValueError, match="Remote 'other' not found"): + RemoteAPI("other", None, None, make_config()) + + +def test_a_password_without_a_username_is_rejected(http): + with pytest.raises(ValueError, match="Password given but no username"): + RemoteAPI("test", None, "secret", make_config()) + + +def test_authentication_without_credentials_or_a_token_is_rejected(http): + http.routes[""] = response({**INDEX, "authentication": "LDAP"}) + + with pytest.raises(ValueError, match="No username or password given"): + RemoteAPI("test", None, None, make_config()) + + +def test_a_remote_without_a_usable_version_is_reported(http): + http.routes[""] = response({**INDEX, "endpoints": ["http://remote.test/v9"]}) + + with pytest.raises(RemoteError, match="No compatible API version"): + RemoteAPI("test", None, None, make_config()) + + +def test_the_api_version_comes_from_the_negotiated_endpoint(http): + http.routes[""] = response({**INDEX, "api_version": None}) + + api = RemoteAPI("test", None, None, make_config()) + + # The negotiated endpoint decides the version, so a remote that reports no + # api_version of its own is still usable. + assert str(api.version) == "1.3.0" + + +def test_a_remote_that_reports_no_server_version_is_reported(http): + http.routes[""] = response({**INDEX, "server_version": None}) + + with pytest.raises(RemoteError, match="did not report a server version"): + RemoteAPI("test", None, None, make_config()) + + +def test_the_selected_version_is_announced_when_verbose(http, capsys): + config = make_config() + config.verbose = True + + RemoteAPI("test", None, None, config) + + assert "Selected API version v1.3" in capsys.readouterr().out + + +# --------------------------------------------------------------------------- +# authentication +# --------------------------------------------------------------------------- + + +def test_no_credentials_are_sent_to_an_unauthenticated_remote(http): + api = RemoteAPI("test", None, None, make_config()) + http.route("validation_schema", []) + + api.get_validation_schemas() + + assert "auth" not in http.request_for("validation_schema").kwargs + + +def test_a_token_is_sent_as_a_jwt_header(http): + http.routes[""] = response({**INDEX, "authentication": "LDAP"}) + api = RemoteAPI("test", None, None, make_config(token="123ABC")) + http.route("validation_schema", []) + + api.get_validation_schemas() + + auth = http.request_for("validation_schema").kwargs["auth"] + request = auth(mock.Mock(headers={})) + assert request.headers["Authorization"] == "JWT-Token 123ABC" + + +def test_a_username_and_password_are_sent_as_basic_auth(http): + http.routes[""] = response({**INDEX, "authentication": "LDAP"}) + api = RemoteAPI("test", "user", "secret", make_config()) + http.route("validation_schema", []) + + api.get_validation_schemas() + + assert http.request_for("validation_schema").kwargs["auth"] == ("user", "secret") + + +def test_credentials_are_prompted_for_when_the_remote_authenticates(http): + http.routes[""] = response({**INDEX, "authentication": "LDAP"}) + + with mock.patch("click.prompt", side_effect=["user", "secret"]) as prompt: + api = RemoteAPI("test", None, None, make_config(), use_token=False) + + assert prompt.call_count == 2 + assert api._username == "user" + assert api._password == "secret" + + +# --------------------------------------------------------------------------- +# requests and responses +# --------------------------------------------------------------------------- + + +def test_requests_are_sent_to_the_versioned_url(http): + api = RemoteAPI("test", None, None, make_config()) + http.route("token", {"token": "NEWTOKEN"}) + + assert api.get_token() == "NEWTOKEN" + assert http.request_for("token").args[1] == "http://remote.test/v1.3/token" + + +def test_the_api_version_context_redirects_requests(http): + api = RemoteAPI("test", None, None, make_config()) + http.route("validation_schema", []) + + 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_list_simulations_sends_the_pagination_headers(http): + api = RemoteAPI("test", None, None, make_config()) + http.route( + "simulations", + { + "count": 1, + "page": 1, + "limit": 10, + "results": [ + { + "uuid": {"_type": "uuid.UUID", "hex": "0" * 32}, + "alias": "sim", + "datetime": "2000-01-01", + } + ], + }, + ) + + simulations = api.list_simulations(limit=10) + + assert [simulation.alias for simulation in simulations] == ["sim"] + headers = http.request_for("simulations").kwargs["headers"] + assert headers["simdb-result-limit"] == "10" + + +def test_list_simulations_appends_the_requested_metadata(http): + api = RemoteAPI("test", None, None, make_config()) + http.route( + "simulations?pulse&run", {"count": 0, "page": 1, "limit": 0, "results": []} + ) + + api.list_simulations(meta=["pulse", "run"]) + + assert http.request_for("simulations?pulse&run") + + +def test_an_error_payload_becomes_a_remote_error(http): + api = RemoteAPI("test", None, None, make_config()) + http.route("validation_schema", {"error": "you shall not pass"}, status=403) + + with pytest.raises(RemoteError, match="you shall not pass"): + api.get_validation_schemas() + + +def test_an_error_without_a_payload_becomes_a_failed_connection(http): + api = RemoteAPI("test", None, None, make_config()) + http.route("validation_schema", status=500, content=b"oops") + + with pytest.raises(FailedConnection, match="HTTP error 500"): + api.get_validation_schemas() + + +def test_a_non_json_response_becomes_a_failed_connection(http): + """A firewall login page is HTML where the API promised JSON.""" + api = RemoteAPI("test", None, None, make_config()) + http.route("validation_schema", content=b"login") + + with pytest.raises(FailedConnection, match="Invalid JSON"): + api.get_validation_schemas() + + +def test_unexpected_json_becomes_a_remote_error(http): + api = RemoteAPI("test", None, None, make_config()) + http.route("token", {"not_a_token": True}) + + with pytest.raises(RemoteError, match="Unexpected data exchanged"): + api.get_token() + + +# --------------------------------------------------------------------------- +# check_return and try_request, tested on their own +# --------------------------------------------------------------------------- + + +def test_check_return_accepts_a_successful_response(): + assert check_return(response({"ok": True})) is None + + +def test_check_return_raises_for_status_without_an_error_field(): + with pytest.raises(requests.HTTPError): + check_return(response({"detail": "nope"}, status=404)) + + +def test_try_request_translates_a_connection_error(): + request = mock.Mock(url="http://remote.test/v1.3/simulations") + + @try_request + def failing(): + raise requests.ConnectionError(request=request) + + with pytest.raises(FailedConnection, match="Connection failed to"): + failing() + + +def test_try_request_reports_an_unknown_url_for_a_request_less_error(): + @try_request + def failing(): + raise requests.ConnectionError(request=None) + + with pytest.raises(FailedConnection, match="undefined"): + failing() + + +def test_try_request_translates_a_json_decode_error(): + @try_request + def failing(): + raise requests.JSONDecodeError("bad", "doc", 0) + + with pytest.raises(FailedConnection, match="Invalid JSON"): + failing() + + +def test_try_request_translates_a_validation_error(): + @try_request + def failing(): + TokenResponse.model_validate({"wrong": "shape"}) + + with pytest.raises(RemoteError, match="Unexpected data exchanged"): + failing() + + +def test_try_request_passes_a_successful_call_through(): + @try_request + def succeeding(value): + return value + + assert succeeding(42) == 42 + + +def test_a_validation_error_is_not_swallowed_as_something_else(): + """Only ``json_invalid`` errors mean the response was not JSON at all.""" + with pytest.raises(ValidationError): + TokenResponse.model_validate_json(b"{}") diff --git a/tests/cli/test_cli_remote_command.py b/tests/cli/test_cli_remote_command.py index 00055fad..b03d7941 100644 --- a/tests/cli/test_cli_remote_command.py +++ b/tests/cli/test_cli_remote_command.py @@ -1,311 +1,469 @@ +"""Tests for ``simdb remote``. + +The remote handshake is stubbed by the ``remote_handshake`` fixture; each test +stubs only the one API call its command makes. +""" + +import uuid from unittest import mock -from click.testing import CliRunner -from utils import config_test_file +import pytest +from cli_helpers import make_simulation -from simdb.cli.simdb import cli +from simdb.cli.remote_api import RemoteAPI +from simdb.database.models.simulation import Simulation from simdb.notifications import Notification -from simdb.remote.models import WatcherData - - -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_authentication") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_endpoints") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_version") -@mock.patch("simdb.cli.remote_api.RemoteAPI.list_watchers") -def test_remote_watchers_list_command( - list_watchers, - get_server_version, - get_endpoints, - get_server_authentication, -): - get_endpoints.return_value = ["v1", "v1.1", "v1.1.1", "v1.2"] - get_server_version.return_value = "0.11" - get_server_authentication.return_value = "None" - sim_id = "acbd1234" +from simdb.remote.models import SimulationTraceData, WatcherData + +pytestmark = pytest.mark.usefixtures("remote_handshake") + + +@pytest.fixture +def api_call(): + """Stub a single :class:`RemoteAPI` method by name. + + ``api_call("list_simulations", return_value=[...])`` patches the method for + the duration of the test and hands back the mock to assert on. + """ + patches = [] + + def _api_call(name, **kwargs): + patcher = mock.patch.object(RemoteAPI, name, **kwargs) + patches.append(patcher) + return patcher.start() + + yield _api_call + + for patcher in reversed(patches): + patcher.stop() + + +# --------------------------------------------------------------------------- +# remote test / directory +# --------------------------------------------------------------------------- + + +def test_test_command_reports_the_remote_api_version(invoke, remote_handshake): + result = invoke("remote", "test") + + assert result.exit_code == 0 + assert "Remote is valid" in result.output + assert "1.3" in result.output + + +def test_directory_prints_the_remote_storage_directory(invoke, api_call): + api_call("get_directory", return_value="/srv/simdb/data") + + result = invoke("remote", "directory") + + assert result.exit_code == 0 + assert "/srv/simdb/data" in result.output + + +# --------------------------------------------------------------------------- +# remote watcher +# --------------------------------------------------------------------------- + + +def test_watcher_list_prints_every_watcher(invoke, api_call, remote_handshake): watchers = [ WatcherData(username="a", email="a@simdb.test", notification="A"), WatcherData(username="b", email="b@simdb.test", notification="V"), WatcherData(username="c", email="c@simdb.test", notification="R"), ] - list_watchers.return_value = watchers - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, [f"--config-file={config_file}", "remote", "watcher", "list", sim_id] - ) - assert result.exception is None - assert sim_id in result.output + list_watchers = api_call("list_watchers", return_value=watchers) + + result = invoke("remote", "watcher", "list", "acbd1234") + + assert result.exit_code == 0 + assert "acbd1234" in result.output for watcher in watchers: assert watcher.username in result.output assert watcher.email in result.output assert list_watchers.called + assert remote_handshake.get_endpoints.called -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_authentication") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_endpoints") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_version") -@mock.patch("simdb.cli.remote_api.RemoteAPI.remove_watcher") -def test_remote_watcher_remove_command( - remove_watcher, - get_server_version, - get_endpoints, - get_server_authentication, -): - get_endpoints.return_value = ["v1", "v1.1", "v1.1.1", "v1.2"] - get_server_version.return_value = "0.11" - get_server_authentication.return_value = "None" - user = "test" - sim_id = "acbd1234" - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, - [ - f"--config-file={config_file}", - "remote", - "watcher", - "remove", - sim_id, - f"--user={user}", - ], +def test_watcher_list_reports_no_watchers(invoke, api_call): + api_call("list_watchers", return_value=[]) + + result = invoke("remote", "watcher", "list", "acbd1234") + + assert result.exit_code == 0 + assert "no watchers found for simulation acbd1234" in result.output + + +def test_watcher_remove_passes_the_user_on(invoke, api_call): + remove_watcher = api_call("remove_watcher") + + result = invoke("remote", "watcher", "remove", "acbd1234", "--user=test") + + assert result.exit_code == 0 + assert "acbd1234" in result.output + assert remove_watcher.call_args.args == ("acbd1234", "test") + + +def test_watcher_remove_fails_without_a_user(invoke, api_call): + remove_watcher = api_call("remove_watcher") + + result = invoke("remote", "watcher", "remove", "acbd1234") + + assert result.exit_code != 0 + assert not remove_watcher.called + + +@pytest.mark.xfail( + strict=True, + reason=( + "remove_watcher calls get_string_option('user.name') without default=None, " + "so the config lookup raises KeyError before its own error message is built" + ), +) +def test_watcher_remove_explains_a_missing_user(invoke, api_call): + """``watcher add`` reports this cleanly; ``watcher remove`` should match it.""" + api_call("remove_watcher") + + result = invoke("remote", "watcher", "remove", "acbd1234") + + assert "User not provided and user.name not found in config" in result.output + + +def test_watcher_add_passes_user_email_and_notification(invoke, api_call): + add_watcher = api_call("add_watcher") + + result = invoke( + "remote", + "watcher", + "add", + "acbd1234", + "--user=test", + "--email=test@iter.org", + "--notification=all", ) - assert result.exception is None - assert sim_id in result.output - assert remove_watcher.called - (args, kwargs) = remove_watcher.call_args - assert args == (sim_id, user) - assert kwargs == {} - - -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_authentication") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_endpoints") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_version") -@mock.patch("simdb.cli.remote_api.RemoteAPI.add_watcher") -def test_remote_watcher_add_command( - add_watcher, - get_server_version, - get_endpoints, - get_server_authentication, -): - get_endpoints.return_value = ["v1", "v1.1", "v1.1.1", "v1.2"] - get_server_version.return_value = "0.11" - get_server_authentication.return_value = "None" - user = "test" - email = "test@iter.org" - sim_id = "acbd1234" - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, - [ - f"--config-file={config_file}", - "remote", - "watcher", - "add", - sim_id, - f"--user={user}", - f"--email={email}", - "--notification=all", - ], + + assert result.exit_code == 0 + assert "acbd1234" in result.output + assert add_watcher.call_args.args == ( + "acbd1234", + "test", + "test@iter.org", + Notification.ALL, ) - assert result.exception is None - assert sim_id in result.output - assert add_watcher.called - (args, kwargs) = add_watcher.call_args - assert args == (sim_id, user, email, Notification.ALL) - assert kwargs == {} - - -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_authentication") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_endpoints") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_version") -@mock.patch("simdb.cli.remote_api.RemoteAPI.list_simulations") -def test_remote_list_command( - list_simulations, - get_server_version, - get_endpoints, - get_server_authentication, -): - get_endpoints.return_value = ["v1", "v1.1", "v1.1.1", "v1.2"] - get_server_version.return_value = "0.11" - get_server_authentication.return_value = "None" - data = [ - ("abcd1234", "test"), - ("abcd5678", "test"), - ("abcd4321", "test"), - ] - sims = [] - for el in data: - sim = mock.Mock() - sim.uuid = el[0] - sim.alias = el[1] - sims.append(sim) - list_simulations.return_value = sims - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, [f"--config-file={config_file}", "remote", "list", "--uuid"] + + +def test_watcher_add_needs_an_email(invoke, api_call): + add_watcher = api_call("add_watcher") + + result = invoke("remote", "watcher", "add", "acbd1234", "--user=test") + + assert result.exit_code != 0 + assert "Email not provided and user.email not found in config" in result.output + assert not add_watcher.called + + +def test_watcher_add_rejects_an_unknown_notification(invoke, api_call): + add_watcher = api_call("add_watcher") + + result = invoke( + "remote", "watcher", "add", "acbd1234", "--user=t", "-e=t@x", "-n=sometimes" ) - assert result.exception is None - assert list_simulations.called - for el in data: - for i in el: - assert i in result.output - - -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_authentication") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_endpoints") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_version") -@mock.patch("simdb.cli.remote_api.RemoteAPI.list_simulations") -def test_remote_list_command_with_verbose( - list_simulations, - get_server_version, - get_endpoints, - get_server_authentication, -): - get_endpoints.return_value = ["v1", "v1.1", "v1.1.1", "v1.2"] - get_server_version.return_value = "0.11" - get_server_authentication.return_value = "None" - data = [ - ("abcd1234", "test", "2000-01-01-01", "Validated"), - ("abcd5678", "test", "2000-02-02-02", "Validated"), - ("abcd4321", "test", "2000-03-03-03", "Validated"), + + assert result.exit_code == 2 + assert not add_watcher.called + + +# --------------------------------------------------------------------------- +# remote list / info / query +# --------------------------------------------------------------------------- + + +def test_list_prints_the_returned_simulations(invoke, api_call, remote_handshake): + simulations = [ + make_simulation("test", uuid="abcd1234"), + make_simulation("test", uuid="abcd5678"), + make_simulation("test", uuid="abcd4321"), ] - sims = [] - for el in data: - sim = mock.Mock() - sim.uuid = el[0] - sim.alias = el[1] - sim.datetime = el[2] - sim.status = el[3] - sims.append(sim) - list_simulations.return_value = sims - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, [f"--config-file={config_file}", "--verbose", "remote", "list", "--uuid"] - ) - assert result.exception is None + list_simulations = api_call("list_simulations", return_value=simulations) + + result = invoke("remote", "list", "--uuid") + + assert result.exit_code == 0 + for simulation in simulations: + assert simulation.uuid in result.output assert list_simulations.called - for el in data: - for i in el: - assert i in result.output - - -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_authentication") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_endpoints") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_version") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_simulation") -def test_remote_info_command( - get_simulation, - get_server_version, - get_endpoints, - get_server_authentication, -): - get_endpoints.return_value = ["v1", "v1.1", "v1.1.1", "v1.2"] - get_server_version.return_value = "0.11" - get_server_authentication.return_value = "None" - sim_id = "abcd1234" - sim = ("abcd1234", "test", "2000-01-01-01", "Validated") - get_simulation.return_value = sim - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, [f"--config-file={config_file}", "remote", "info", sim_id] + assert remote_handshake.get_endpoints.called + + +def test_list_shows_the_extra_columns_when_verbose(invoke, api_call): + api_call( + "list_simulations", + return_value=[ + make_simulation( + "test", uuid="abcd1234", datetime="2000-01-01-01", status="passed" + ) + ], ) - assert result.exception is None - assert str(sim) in result.output - assert get_simulation.called - (args, kwargs) = get_simulation.call_args - assert args == (sim_id,) - assert kwargs == {} - - -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_authentication") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_endpoints") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_version") -@mock.patch("simdb.cli.remote_api.RemoteAPI.query_simulations") -def test_remote_query_command( - query_simulations, - get_server_version, - get_endpoints, - get_server_authentication, -): - get_endpoints.return_value = ["v1", "v1.1", "v1.1.1", "v1.2"] - get_server_version.return_value = "0.11" - get_server_authentication.return_value = "None" - data = [ - ("abcd1234", "123"), - ] - sims = [] - for el in data: - sim = mock.Mock() - sim.uuid = el[0] - sim.alias = el[1] - sim.find_meta.return_value = [] - sims.append(sim) + + result = invoke("--verbose", "remote", "list", "--uuid") + + assert result.exit_code == 0 + assert "2000-01-01-01" in result.output + assert "passed" in result.output + + +def test_list_rejects_a_negative_limit(invoke, api_call): + list_simulations = api_call("list_simulations") + + result = invoke("remote", "list", "--limit=-1") + + assert result.exit_code == 2 + assert not list_simulations.called + + +def test_info_prints_the_simulation(invoke, api_call): + get_simulation = api_call("get_simulation", return_value="simulation description") + + result = invoke("remote", "info", "abcd1234") + + assert result.exit_code == 0 + assert "simulation description" in result.output + assert get_simulation.call_args.args == ("abcd1234",) + + +def test_query_forwards_the_constraints(invoke, api_call): constraints = ("alias=123", "description=in:test") - query_simulations.return_value = sims - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, [f"--config-file={config_file}", "remote", "query", "--uuid", *constraints] + query_simulations = api_call( + "query_simulations", return_value=[make_simulation("123", uuid="abcd1234")] ) - assert result.exception is None - for el in data: - for i in el: - assert i in result.output - assert query_simulations.called - (args, kwargs) = query_simulations.call_args - assert args == (constraints, (), 100) - assert kwargs == {} - - -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_authentication") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_endpoints") -@mock.patch("simdb.cli.remote_api.RemoteAPI.get_server_version") -@mock.patch("simdb.cli.remote_api.RemoteAPI.query_simulations") -def test_remote_query_command_with_verbose( - query_simulations, - get_server_version, - get_endpoints, - get_server_authentication, -): - get_endpoints.return_value = ["v1", "v1.1", "v1.1.1", "v1.2"] - get_server_version.return_value = "0.11" - get_server_authentication.return_value = "None" - data = [ - ("abcd1234", "123", "2000-01-01-01", "Validated"), - ] - sims = [] - for el in data: - sim = mock.Mock() - sim.uuid = el[0] - sim.alias = el[1] - sim.datetime = el[2] - sim.status = el[3] - sim.find_meta.return_value = [] - sims.append(sim) - constraints = ("alias=123", "description=in:test") - query_simulations.return_value = sims - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke( - cli, - [ - f"--config-file={config_file}", - "--verbose", - "remote", - "query", - "--uuid", - *constraints, + + result = invoke("remote", "query", "--uuid", *constraints) + + assert result.exit_code == 0 + assert "abcd1234" in result.output + assert query_simulations.call_args.args == (constraints, (), 100) + + +def test_query_shows_the_extra_columns_when_verbose(invoke, api_call): + api_call( + "query_simulations", + return_value=[ + make_simulation( + "123", uuid="abcd1234", datetime="2000-01-01-01", status="passed" + ) ], ) - assert result.exception is None - for el in data: - for i in el: - assert i in result.output - assert query_simulations.called - (args, kwargs) = query_simulations.call_args - assert args == (constraints, (), 100) - assert kwargs == {} + + result = invoke("--verbose", "remote", "query", "--uuid", "alias=123") + + assert result.exit_code == 0 + assert "2000-01-01-01" in result.output + assert "passed" in result.output + + +# --------------------------------------------------------------------------- +# remote version / trace +# --------------------------------------------------------------------------- + + +def test_version_prints_the_remote_simdb_version(invoke): + result = invoke("remote", "version") + + assert result.exit_code == 0 + assert "Remote 'test' SimDB version: 0.11" in result.output + + +def test_trace_prints_the_provenance_chain(invoke, api_call): + trace_simulation = api_call( + "trace_simulation", + return_value=SimulationTraceData( + alias="current", + status="passed", + replaces=SimulationTraceData(alias="older", status="deprecated"), + ), + ) + + result = invoke("remote", "trace", "abcd1234") + + assert result.exit_code == 0 + assert "current" in result.output + assert "older" in result.output + assert trace_simulation.call_args.args == ("abcd1234",) + + +def test_trace_reports_a_missing_trace(invoke, api_call): + api_call("trace_simulation", return_value=None) + + result = invoke("remote", "trace", "abcd1234") + + assert result.exit_code == 0 + assert "No simulations trace found" in result.output + + +# --------------------------------------------------------------------------- +# remote schema +# --------------------------------------------------------------------------- + + +def test_schema_prints_the_validation_schemas(invoke, api_call): + api_call("get_validation_schemas", return_value=[{"alias": {"required": True}}]) + + result = invoke("remote", "schema") + + assert result.exit_code == 0 + assert "alias" in result.output + + +def test_schema_rejects_a_non_positive_depth(invoke, api_call): + get_validation_schemas = api_call("get_validation_schemas", return_value=[]) + + result = invoke("remote", "schema", "--depth=0") + + assert result.exit_code == 2 + assert "must be greater than zero" in result.output + assert not get_validation_schemas.called + + +# --------------------------------------------------------------------------- +# remote token +# --------------------------------------------------------------------------- + + +def test_token_new_stores_the_token_in_the_configuration(invoke, api_call, config_file): + api_call("get_token", return_value="NEWTOKEN") + + result = invoke("remote", "token", "new") + + assert result.exit_code == 0 + assert "Token added for remote test." in result.output + assert "NEWTOKEN" in config_file.read_text() + + +def test_token_delete_removes_the_token(invoke, config_file): + result = invoke("remote", "token", "delete") + + assert result.exit_code == 0 + assert "Token for remote test deleted." in result.output + assert "123ABC" not in config_file.read_text() + + +def test_token_delete_can_be_repeated(invoke): + assert invoke("remote", "token", "delete").exit_code == 0 + + result = invoke("remote", "token", "delete") + + assert result.exit_code == 0 + + +@pytest.mark.xfail( + strict=True, + reason=( + "Config.delete_option only raises KeyError for a missing section; " + "configparser.remove_option returns False for a missing option instead " + "of raising, so the deletion is reported as successful" + ), +) +def test_token_delete_says_when_there_was_no_token(invoke): + assert invoke("remote", "token", "delete").exit_code == 0 + + result = invoke("remote", "token", "delete") + + assert "No token for remote test found." in result.output + + +# --------------------------------------------------------------------------- +# remote admin +# --------------------------------------------------------------------------- + + +def test_admin_set_meta_reports_an_update(invoke, api_call): + set_metadata = api_call("set_metadata", return_value="old") + + result = invoke("remote", "admin", "set-meta", "abcd1234", "pulse", "134173") + + assert result.exit_code == 0 + assert "old -> 134173" in result.output + assert set_metadata.call_args.args == ("abcd1234", "pulse", "134173") + + +def test_admin_set_meta_reports_a_new_value(invoke, api_call): + api_call("set_metadata", return_value=None) + + result = invoke("remote", "admin", "set-meta", "abcd1234", "pulse", "134173") + + assert result.exit_code == 0 + assert "Added pulse for simulation abcd1234 with value '134173'" in result.output + + +@pytest.mark.parametrize( + ("type_name", "value", "expected"), + [ + ("int", "42", 42), + ("float", "1.5", 1.5), + ("string", "42", "42"), + ], +) +def test_admin_set_meta_converts_the_value( + invoke, api_call, type_name, value, expected +): + set_metadata = api_call("set_metadata", return_value=None) + + result = invoke( + "remote", "admin", "set-meta", "abcd1234", "key", value, f"--type={type_name}" + ) + + assert result.exit_code == 0 + assert set_metadata.call_args.args[2] == expected + + +def test_admin_set_meta_converts_a_uuid(invoke, api_call): + set_metadata = api_call("set_metadata", return_value=None) + value = "12345678123456781234567812345678" + + result = invoke( + "remote", "admin", "set-meta", "abcd1234", "key", value, "--type=UUID" + ) + + assert result.exit_code == 0 + assert set_metadata.call_args.args[2] == uuid.UUID(value) + + +def test_admin_set_status_updates_the_status(invoke, api_call): + update_simulation = api_call("update_simulation", return_value="not validated") + + result = invoke("remote", "admin", "set-status", "abcd1234", "PASSED") + + assert result.exit_code == 0 + assert "not validated -> PASSED" in result.output + assert update_simulation.call_args.args == ( + "abcd1234", + Simulation.Status.PASSED, + ) + + +def test_admin_set_status_rejects_an_unknown_status(invoke, api_call): + update_simulation = api_call("update_simulation") + + result = invoke("remote", "admin", "set-status", "abcd1234", "excellent") + + assert result.exit_code == 2 + assert not update_simulation.called + + +def test_admin_del_meta_removes_the_key(invoke, api_call): + delete_metadata = api_call("delete_metadata") + + result = invoke("remote", "admin", "del-meta", "abcd1234", "pulse") + + assert result.exit_code == 0 + assert "Deleted pulse for simulation abcd1234" in result.output + assert delete_metadata.call_args.args == ("abcd1234", "pulse") + + +def test_admin_delete_removes_the_simulation(invoke, api_call): + delete_simulation = api_call("delete_simulation") + + result = invoke("remote", "admin", "delete", "abcd1234") + + assert result.exit_code == 0 + assert "Deleted simulation abcd1234" in result.output + assert delete_simulation.call_args.args == ("abcd1234",) diff --git a/tests/cli/test_cli_remote_config_command.py b/tests/cli/test_cli_remote_config_command.py new file mode 100644 index 00000000..02751570 --- /dev/null +++ b/tests/cli/test_cli_remote_config_command.py @@ -0,0 +1,152 @@ +"""Tests for ``simdb remote config``. + +These commands only touch the configuration file, so no remote is involved. The +file they write back to is the throw-away one the ``config_file`` fixture +creates inside ``tmp_path``. +""" + +import configparser + +import pytest + + +@pytest.fixture +def saved_config(config_file): + """Read back what ``Config.save`` wrote. + + ``Config.load`` treats a ``--config-file`` as the user configuration, so + that is the file ``save`` writes back to. + """ + + def _saved_config() -> configparser.ConfigParser: + parser = configparser.ConfigParser() + parser.read(config_file) + return parser + + return _saved_config + + +def test_list_shows_the_configured_remotes(invoke): + result = invoke("remote", "config", "list") + + assert result.exit_code == 0 + assert "test: http://0.0.0.0:5000/ (default)" in result.output + + +def test_list_shows_the_username_and_firewall_of_a_remote(invoke): + assert ( + invoke("remote", "config", "set-option", "test", "username", "me").exit_code + == 0 + ) + assert ( + invoke("remote", "config", "set-option", "test", "firewall", "F5").exit_code + == 0 + ) + + result = invoke("remote", "config", "list") + + assert result.exit_code == 0 + assert "firewall: F5" in result.output + assert "username: me" in result.output + + +def test_default_prints_the_default_remote(invoke): + result = invoke("remote", "config", "default") + + assert result.exit_code == 0 + assert result.output.strip() == "test" + + +def test_get_default_prints_the_default_remote(invoke): + result = invoke("remote", "config", "get-default") + + assert result.exit_code == 0 + assert result.output.strip() == "test" + + +def test_new_adds_a_remote(invoke, saved_config): + result = invoke("remote", "config", "new", "other", "http://other.test") + + assert result.exit_code == 0 + assert saved_config()['remote "other"']["url"] == "http://other.test" + + +def test_new_can_record_a_username_and_firewall(invoke, saved_config): + result = invoke( + "remote", + "config", + "new", + "other", + "http://other.test", + "--username=me", + "--firewall=F5", + ) + + assert result.exit_code == 0 + section = saved_config()['remote "other"'] + assert section["username"] == "me" + assert section["firewall"] == "F5" + + +def test_new_rejects_an_unknown_firewall(invoke): + result = invoke( + "remote", "config", "new", "other", "http://other.test", "--firewall=nope" + ) + + assert result.exit_code == 2 + + +def test_new_can_make_the_remote_the_default(invoke, saved_config): + result = invoke( + "remote", "config", "new", "other", "http://other.test", "--default" + ) + + assert result.exit_code == 0 + config = saved_config() + assert config.getboolean('remote "other"', "default") is True + assert config.getboolean('remote "test"', "default") is False + + +def test_set_default_switches_the_default_remote(invoke, saved_config): + assert ( + invoke("remote", "config", "new", "other", "http://other.test").exit_code == 0 + ) + + result = invoke("remote", "config", "set-default", "other") + + assert result.exit_code == 0 + assert saved_config().getboolean('remote "other"', "default") is True + + +def test_delete_removes_a_remote(invoke, saved_config): + assert ( + invoke("remote", "config", "new", "other", "http://other.test").exit_code == 0 + ) + + result = invoke("remote", "config", "delete", "other") + + assert result.exit_code == 0 + assert 'remote "other"' not in saved_config().sections() + + +def test_set_option_stores_an_arbitrary_option(invoke, saved_config): + result = invoke("remote", "config", "set-option", "test", "token", "NEWTOKEN") + + assert result.exit_code == 0 + assert saved_config()['remote "test"']["token"] == "NEWTOKEN" + + +@pytest.mark.parametrize( + ("subcommand", "arguments"), + [ + ("new", ["only-a-name"]), + ("delete", []), + ("set-default", []), + ("set-option", ["test", "token"]), + ], +) +def test_missing_arguments_are_reported(invoke, subcommand, arguments): + result = invoke("remote", "config", subcommand, *arguments) + + assert result.exit_code == 2 + assert "Missing argument" in result.output diff --git a/tests/cli/test_cli_root.py b/tests/cli/test_cli_root.py new file mode 100644 index 00000000..99000ef5 --- /dev/null +++ b/tests/cli/test_cli_root.py @@ -0,0 +1,133 @@ +"""Tests for the top level ``simdb`` group in :mod:`simdb.cli.simdb`.""" + +from unittest import mock + +import pytest +from cli_helpers import make_simulation + +from simdb import __version__ +from simdb.cli import simdb as simdb_cli +from simdb.cli.simdb import cli + + +def test_version_option_reports_the_package_version(runner): + result = runner.invoke(cli, ["--version"]) + + assert result.exit_code == 0 + assert __version__ in result.output + + +def test_help_lists_every_command(runner): + result = runner.invoke(cli, ["--help"]) + + assert result.exit_code == 0 + commands = ("alias", "config", "manifest", "provenance", "remote", "simulation") + for command in commands: + assert command in result.output + + +def test_commands_are_listed_in_alphabetical_order(): + listed = cli.list_commands(None) + + assert listed == sorted(listed) + assert "sim" in listed + + +def test_sim_is_an_alias_for_simulation(runner): + result = runner.invoke(cli, ["--help"]) + + assert "Alias for simulation." in result.output + assert cli.get_command(None, "sim") is not None + + +def test_the_alias_reaches_the_same_command(invoke, local_db): + local_db.list_simulations.return_value = [] + + assert invoke("sim", "list").exit_code == 0 + assert local_db.list_simulations.called + + +def test_unknown_commands_are_rejected(runner): + result = runner.invoke(cli, ["nonsense"]) + + assert result.exit_code == 2 + + +def test_hidden_dump_help_prints_help_for_every_subcommand(runner): + result = runner.invoke(cli, ["dump-help"]) + + assert result.exit_code == 0 + # Both a group and one of its sub-commands are documented. + assert "Manage ingested simulations." in result.output + assert "List ingested simulations." in result.output + assert "Query/update application configuration." in result.output + + +def test_dump_help_is_hidden_from_the_command_list(runner): + assert "dump-help" not in runner.invoke(cli, ["--help"]).output + + +def test_config_file_option_is_loaded(invoke): + """The remote declared in the config file is visible to the commands.""" + result = invoke("remote", "config", "list") + + assert result.exit_code == 0 + assert "test: http://0.0.0.0:5000/" in result.output + assert "(default)" in result.output + + +def test_a_missing_config_file_is_reported(runner, tmp_path): + result = runner.invoke( + cli, [f"--config-file={tmp_path / 'nope.cfg'}", "config", "path"] + ) + + assert result.exit_code == 2 + assert "No such file or directory" in result.output + + +def test_verbose_flag_reaches_the_commands(invoke, local_db): + """``--verbose`` is what turns on the extra columns of ``simulation list``.""" + local_db.list_simulations.return_value = [make_simulation("sim")] + + assert "status" not in invoke("simulation", "list").output + assert "status" in invoke("--verbose", "simulation", "list").output + + +class TestMain: + """``main`` is the console-script entry point and the CLI's last error handler.""" + + def test_successful_runs_do_not_raise(self): + with mock.patch.object(simdb_cli, "cli") as command: + simdb_cli.main() + + assert command.called + + def test_errors_are_reported_and_exit_non_zero(self, capsys): + with mock.patch.object( + simdb_cli, "cli", side_effect=RuntimeError("boom") + ), pytest.raises(SystemExit) as exit_info: + simdb_cli.main() + + assert exit_info.value.code == 1 + assert "Error: boom" in capsys.readouterr().err + + def test_debug_mode_re_raises_for_a_traceback(self, monkeypatch): + monkeypatch.setattr(simdb_cli, "g_debug", True) + + with mock.patch.object( + simdb_cli, "cli", side_effect=RuntimeError("boom") + ), pytest.raises(RuntimeError, match="boom"): + simdb_cli.main() + + def test_the_debug_flag_switches_debug_mode_on(self, invoke): + """``-d`` is what makes :func:`main` re-raise instead of printing. + + ``--help`` is eager and exits before the group callback runs, so the + flag only takes effect when a command is actually invoked. + """ + invoke("-d", "config", "path") + assert simdb_cli.g_debug is True + + # Leave the module global as the other tests expect to find it. + invoke("config", "path") + assert simdb_cli.g_debug is False diff --git a/tests/cli/test_cli_simulation_command.py b/tests/cli/test_cli_simulation_command.py index 0120fc02..7541cca1 100644 --- a/tests/cli/test_cli_simulation_command.py +++ b/tests/cli/test_cli_simulation_command.py @@ -1,87 +1,496 @@ +"""Tests for ``simdb simulation``. + +The local database and the remote are stubbed; what is exercised here is the +command layer itself: argument parsing, the branches each command takes, the +calls it makes, and what it reports back to the user. +""" + from unittest import mock -from click.testing import CliRunner -from utils import config_test_file - -from simdb.cli.simdb import cli - - -@mock.patch("simdb.database.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI") -def test_simulation_alias_command(remote_api, get_local_db): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "simulation"]) - assert result.exception is None - - -@mock.patch("simdb.database.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI") -def test_simulation_delete_command(remote_api, get_local_db): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "simulation"]) - assert result.exception is None - - -@mock.patch("simdb.database.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI") -def test_simulation_info_command(remote_api, get_local_db): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "simulation"]) - assert result.exception is None - - -@mock.patch("simdb.database.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI") -def test_simulation_list_command(remote_api, get_local_db): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "simulation"]) - assert result.exception is None - - -@mock.patch("simdb.database.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI") -def test_simulation_modify_command(remote_api, get_local_db): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "simulation"]) - assert result.exception is None - - -@mock.patch("simdb.database.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI") -def test_simulation_new_command(remote_api, get_local_db): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "simulation"]) - assert result.exception is None - - -@mock.patch("simdb.database.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI") -def test_simulation_push_command(remote_api, get_local_db): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "simulation"]) - assert result.exception is None - - -@mock.patch("simdb.database.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI") -def test_simulation_query_command(remote_api, get_local_db): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "simulation"]) - assert result.exception is None - - -@mock.patch("simdb.database.get_local_db") -@mock.patch("simdb.cli.remote_api.RemoteAPI") -def test_simulation_validate_command(remote_api, get_local_db): - config_file = config_test_file() - runner = CliRunner() - result = runner.invoke(cli, [f"--config-file={config_file}", "simulation"]) - assert result.exception is None +import pytest +from cli_helpers import make_simulation + +from simdb.cli.remote_api import RemoteError +from simdb.database import DatabaseError +from simdb.remote.models import ImasDataResponse, QuantityData +from simdb.validation import ValidationError + + +@pytest.fixture +def remote_api(): + """Replace the ``RemoteAPI`` the simulation commands construct.""" + with mock.patch("simdb.cli.commands.simulation.RemoteAPI") as remote_api_cls: + yield remote_api_cls.return_value + + +# --------------------------------------------------------------------------- +# simulation list +# --------------------------------------------------------------------------- + + +def test_list_prints_every_alias(invoke, local_db): + local_db.list_simulations.return_value = [ + make_simulation("first"), + make_simulation("second"), + ] + + result = invoke("simulation", "list") + + assert result.exit_code == 0 + assert "first" in result.output + assert "second" in result.output + assert local_db.list_simulations.call_args.kwargs == { + "meta_keys": (), + "limit": 100, + } + + +def test_list_reports_an_empty_database(invoke, local_db): + local_db.list_simulations.return_value = [] + + result = invoke("simulation", "list") + + assert result.exit_code == 0 + assert "No simulations found" in result.output + + +def test_list_shows_uuid_and_metadata_columns_on_request(invoke, local_db): + local_db.list_simulations.return_value = [ + make_simulation("first", uuid="abcd1234", meta={"pulse": 134173}) + ] + + result = invoke("simulation", "list", "--uuid", "--meta-data=pulse") + + assert result.exit_code == 0 + assert "abcd1234" in result.output + assert "134173" in result.output + assert local_db.list_simulations.call_args.kwargs["meta_keys"] == ("pulse",) + + +def test_list_passes_the_requested_limit(invoke, local_db): + local_db.list_simulations.return_value = [] + + assert invoke("simulation", "list", "--limit=5").exit_code == 0 + + assert local_db.list_simulations.call_args.kwargs["limit"] == 5 + + +def test_list_rejects_a_negative_limit(invoke, local_db): + result = invoke("simulation", "list", "--limit=-1") + + assert result.exit_code == 2 + assert "must be non-negative" in result.output + assert not local_db.list_simulations.called + + +# --------------------------------------------------------------------------- +# simulation modify +# --------------------------------------------------------------------------- + + +def test_modify_sets_a_new_alias(invoke, local_db): + simulation = make_simulation("old") + local_db.get_simulation.return_value = simulation + + result = invoke("simulation", "modify", "old", "--alias=new") + + assert result.exit_code == 0 + assert "alias updated" in result.output + assert simulation.alias == "new" + assert local_db.session.commit.called + + +def test_modify_sets_metadata(invoke, local_db): + simulation = make_simulation("sim") + local_db.get_simulation.return_value = simulation + + result = invoke("simulation", "modify", "sim", "--set-meta=pulse=134173") + + assert result.exit_code == 0 + assert "metadata updated" in result.output + simulation.set_meta.assert_called_once_with("pulse", "134173") + assert local_db.session.commit.called + + +def test_modify_rejects_metadata_without_a_value(invoke, local_db): + result = invoke("simulation", "modify", "sim", "--set-meta=pulse") + + assert result.exit_code == 2 + assert "must be of form NAME=VALUE" in result.output + assert not local_db.session.commit.called + + +def test_modify_deletes_metadata(invoke, local_db): + simulation = make_simulation("sim") + local_db.get_simulation.return_value = simulation + + result = invoke("simulation", "modify", "sim", "--del-meta=pulse") + + assert result.exit_code == 0 + assert "metadata deleted" in result.output + simulation.remove_meta.assert_called_once_with("pulse") + + +def test_modify_without_options_changes_nothing(invoke, local_db): + result = invoke("simulation", "modify", "sim") + + assert result.exit_code == 0 + assert "nothing to do" in result.output + assert not local_db.get_simulation.called + + +# --------------------------------------------------------------------------- +# simulation delete +# --------------------------------------------------------------------------- + + +def test_delete_removes_a_single_simulation(invoke, local_db): + simulation = make_simulation("sim") + simulation.uuid = mock.Mock(hex="abcd1234") + local_db.delete_simulation.return_value = simulation + + result = invoke("simulation", "delete", "sim") + + assert result.exit_code == 0 + assert "abcd1234 deleted" in result.output + local_db.delete_simulation.assert_called_once_with("sim") + + +def test_delete_requires_a_simulation_or_all(invoke, local_db): + result = invoke("simulation", "delete") + + assert result.exit_code != 0 + assert "Either SIM_ID or --all must be provided" in result.output + + +def test_delete_all_removes_the_database_file_once_confirmed( + invoke, local_db, tmp_path +): + database_file = tmp_path / "sim.db" + database_file.write_text("not really a database") + + with mock.patch( + "simdb.cli.commands.simulation.Confirm.ask", return_value=True + ) as ask: + result = invoke("simulation", "delete", "--all") + + assert result.exit_code == 0 + assert "Local database reset." in result.output + assert ask.called + assert not database_file.exists() + + +def test_delete_all_keeps_the_database_when_not_confirmed(invoke, local_db, tmp_path): + database_file = tmp_path / "sim.db" + database_file.write_text("not really a database") + + with mock.patch("simdb.cli.commands.simulation.Confirm.ask", return_value=False): + result = invoke("simulation", "delete", "--all") + + # Declining the reset falls through to the single-simulation path, which has + # no SIM_ID to work with. + assert result.exit_code != 0 + assert "Either SIM_ID or --all must be provided" in result.output + assert database_file.exists() + + +# --------------------------------------------------------------------------- +# simulation info +# --------------------------------------------------------------------------- + + +def test_info_prints_the_simulation(invoke, local_db): + local_db.get_simulation.return_value = "simulation description" + + result = invoke("simulation", "info", "sim") + + assert result.exit_code == 0 + assert "simulation description" in result.output + local_db.get_simulation.assert_called_once_with("sim") + + +def test_info_fails_when_the_simulation_is_unknown(invoke, local_db): + local_db.get_simulation.return_value = None + + result = invoke("simulation", "info", "sim") + + assert result.exit_code != 0 + assert isinstance(result.exception, KeyError) + + +# --------------------------------------------------------------------------- +# simulation ingest +# --------------------------------------------------------------------------- + + +def test_ingest_stores_the_manifest_and_reports_the_alias( + invoke, local_db, manifest_file +): + result = invoke("simulation", "ingest", str(manifest_file)) + + assert result.exit_code == 0 + assert "ALIAS: simulation-alias" in result.output + assert local_db.insert_simulation.called + (simulation,) = local_db.insert_simulation.call_args.args + assert simulation.alias == "simulation-alias" + assert len(simulation.inputs) == 1 + assert len(simulation.outputs) == 1 + + +def test_ingest_alias_option_overrides_the_manifest(invoke, local_db, manifest_file): + result = invoke("simulation", "ingest", str(manifest_file), "--alias=override") + + assert result.exit_code == 0 + assert "ALIAS: override" in result.output + (simulation,) = local_db.insert_simulation.call_args.args + assert simulation.alias == "override" + + +def test_ingest_rejects_an_alias_with_reserved_characters( + invoke, local_db, manifest_file +): + """The manifest refuses the alias before the simulation is built. + + ``simulation_ingest`` also has a "warning: alias contains reserved + characters" branch, but ``Manifest.validate_alias`` applies the identical + ``urllib.parse.quote`` check to both the manifest alias and the ``--alias`` + override, so that branch cannot be reached. + """ + result = invoke("simulation", "ingest", str(manifest_file), "--alias=with space") + + assert result.exit_code != 0 + assert "illegal characters in alias" in str(result.exception) + assert not local_db.insert_simulation.called + + +def test_ingest_requires_an_existing_manifest(invoke, local_db, tmp_path): + result = invoke("simulation", "ingest", str(tmp_path / "missing.yaml")) + + assert result.exit_code == 2 + assert not local_db.insert_simulation.called + + +# --------------------------------------------------------------------------- +# simulation query +# --------------------------------------------------------------------------- + + +def test_query_parses_constraints_and_prints_matches(invoke, local_db): + local_db.query_meta.return_value = [make_simulation("match")] + + result = invoke("simulation", "query", "pulse=gt:1000", "run=0") + + assert result.exit_code == 0 + assert "match" in result.output + (constraints,) = local_db.query_meta.call_args.args + assert [(name, value) for name, value, _ in constraints] == [ + ("pulse", "1000"), + ("run", "0"), + ] + + +def test_query_requires_a_constraint(invoke, local_db): + result = invoke("simulation", "query") + + assert result.exit_code != 0 + assert "At least one constraint must be provided" in result.output + assert not local_db.query_meta.called + + +def test_query_rejects_a_constraint_without_a_value(invoke, local_db): + result = invoke("simulation", "query", "pulse") + + assert result.exit_code != 0 + assert "Invalid constraint pulse" in result.output + assert not local_db.query_meta.called + + +# --------------------------------------------------------------------------- +# simulation push +# --------------------------------------------------------------------------- + + +def test_push_validates_and_uploads_the_simulation(invoke, local_db, remote_api): + simulation = make_simulation("sim") + local_db.get_simulation.return_value = simulation + remote_api.get_validation_schemas.return_value = [] + + result = invoke("simulation", "push", "sim") + + assert result.exit_code == 0 + assert "Successfully pushed simulation" in result.output + assert remote_api.push_simulation.call_args.args == (simulation,) + assert remote_api.push_simulation.call_args.kwargs["add_watcher"] is False + + +def test_push_records_the_replaced_simulation(invoke, local_db, remote_api): + simulation = make_simulation("sim") + local_db.get_simulation.return_value = simulation + remote_api.get_validation_schemas.return_value = [] + + result = invoke("simulation", "push", "sim", "--replaces=older") + + assert result.exit_code == 0 + simulation.set_meta.assert_called_once_with("replaces", "older") + + +def test_push_adds_a_watcher_on_request(invoke, local_db, remote_api): + local_db.get_simulation.return_value = make_simulation("sim") + remote_api.get_validation_schemas.return_value = [] + + result = invoke("simulation", "push", "sim", "--add-watcher") + + assert result.exit_code == 0 + assert remote_api.push_simulation.call_args.kwargs["add_watcher"] is True + + +def test_push_fails_when_the_simulation_is_unknown(invoke, local_db, remote_api): + local_db.get_simulation.return_value = None + + result = invoke("simulation", "push", "sim") + + assert result.exit_code != 0 + assert "Failed to find simulation: sim" in result.output + assert not remote_api.push_simulation.called + + +def test_push_reports_validation_failures_without_uploading( + invoke, local_db, remote_api +): + local_db.get_simulation.return_value = make_simulation("sim") + remote_api.get_validation_schemas.return_value = [{"alias": {"type": "string"}}] + + with mock.patch("simdb.cli.commands.simulation.Validator") as validator: + validator.return_value.validate.side_effect = ValidationError("bad metadata") + result = invoke("simulation", "push", "sim") + + assert result.exit_code != 0 + assert "Simulation does not validate: bad metadata" in result.output + assert not remote_api.push_simulation.called + + +# --------------------------------------------------------------------------- +# simulation pull +# --------------------------------------------------------------------------- + + +def test_pull_stores_the_simulation_locally(invoke, local_db, remote_api, tmp_path): + local_db.get_simulation.side_effect = DatabaseError("not found") + pulled = make_simulation("pulled") + remote_api.pull_simulation.return_value = pulled + + result = invoke("simulation", "pull", "sim", str(tmp_path / "out")) + + assert result.exit_code == 0 + assert "Successfully pulled simulation" in result.output + local_db.insert_simulation.assert_called_once_with(pulled) + + +def test_pull_refuses_to_overwrite_an_existing_simulation( + invoke, local_db, remote_api, tmp_path +): + local_db.get_simulation.return_value = make_simulation("sim") + + result = invoke("simulation", "pull", "sim", str(tmp_path / "out")) + + assert result.exit_code != 0 + assert "already exists" in result.output + assert not remote_api.pull_simulation.called + assert not local_db.insert_simulation.called + + +def test_pull_reports_remote_errors(invoke, local_db, remote_api, tmp_path): + local_db.get_simulation.side_effect = DatabaseError("not found") + remote_api.pull_simulation.side_effect = RemoteError("remote is down") + + result = invoke("simulation", "pull", "sim", str(tmp_path / "out")) + + assert result.exit_code != 0 + assert "remote is down" in result.output + assert not local_db.insert_simulation.called + + +# --------------------------------------------------------------------------- +# simulation data +# --------------------------------------------------------------------------- + + +def _data_response(field_data, coordinates=()): + return ImasDataResponse( + simulation="abcd1234", + path="core_profiles/profiles_1d[0]/grid/rho_tor_norm", + occurrence=0, + field=QuantityData(name="grid/rho_tor_norm", units="-", data=field_data), + coordinates=list(coordinates), + ) + + +def test_data_plots_a_one_dimensional_field(invoke, remote_api): + remote_api.get_simulation_data.return_value = _data_response([0.0, 0.5, 1.0]) + + result = invoke("simulation", "data", "sim", "core_profiles/grid/rho_tor_norm") + + assert result.exit_code == 0 + assert "abcd1234" in result.output + assert "occurrence 0" in result.output + assert remote_api.get_simulation_data.call_args.args == ( + "sim", + "core_profiles/grid/rho_tor_norm", + ) + assert remote_api.get_simulation_data.call_args.kwargs == {"dd_version": None} + + +def test_data_forwards_the_requested_dd_version(invoke, remote_api): + remote_api.get_simulation_data.return_value = _data_response(1.5) + + result = invoke( + "simulation", "data", "sim", "core_profiles/time", "--dd-version=4.1.1" + ) + + assert result.exit_code == 0 + assert remote_api.get_simulation_data.call_args.kwargs == {"dd_version": "4.1.1"} + + +def test_data_reports_remote_failures_as_a_clean_error(invoke, remote_api): + remote_api.get_simulation_data.side_effect = RemoteError("no such field") + + result = invoke("simulation", "data", "sim", "core_profiles/nope") + + assert result.exit_code != 0 + assert "no such field" in result.output + + +# --------------------------------------------------------------------------- +# simulation validate +# --------------------------------------------------------------------------- + + +def test_validate_checks_metadata_and_file_checksums(invoke, local_db, remote_api): + file = mock.Mock(uri="file:///data", checksum="abc") + file.generate_checksum.return_value = "abc" + simulation = make_simulation("sim") + simulation.inputs = [file] + simulation.outputs = [] + local_db.get_simulation.return_value = simulation + remote_api.get_validation_schemas.return_value = [] + + result = invoke("simulation", "validate", "sim") + + assert result.exit_code == 0 + assert "validation successful" in result.output + + +def test_validate_fails_on_a_checksum_mismatch(invoke, local_db, remote_api): + file = mock.Mock(uri="file:///data", checksum="abc") + file.generate_checksum.return_value = "different" + simulation = make_simulation("sim") + simulation.inputs = [file] + simulation.outputs = [] + local_db.get_simulation.return_value = simulation + remote_api.get_validation_schemas.return_value = [] + + result = invoke("simulation", "validate", "sim") + + assert result.exit_code != 0 + assert isinstance(result.exception, ValidationError) + assert "file:///data" in str(result.exception) diff --git a/tests/cli/test_cli_validators.py b/tests/cli/test_cli_validators.py new file mode 100644 index 00000000..614516e4 --- /dev/null +++ b/tests/cli/test_cli_validators.py @@ -0,0 +1,28 @@ +"""Tests for the shared click parameter validators.""" + +import click +import pytest + +from simdb.cli.commands.validators import validate_non_negative, validate_positive + + +@pytest.mark.parametrize("value", [0, 1, 100]) +def test_non_negative_accepts_zero_and_above(value): + assert validate_non_negative(None, None, value) == value + + +@pytest.mark.parametrize("value", [-1, -100]) +def test_non_negative_rejects_negative_values(value): + with pytest.raises(click.BadParameter, match="must be non-negative"): + validate_non_negative(None, None, value) + + +@pytest.mark.parametrize("value", [1, 100]) +def test_positive_accepts_values_above_zero(value): + assert validate_positive(None, None, value) == value + + +@pytest.mark.parametrize("value", [0, -1]) +def test_positive_rejects_zero_and_below(value): + with pytest.raises(click.BadParameter, match="must be greater than zero"): + validate_positive(None, None, value) diff --git a/tests/cli/test_remote_api_helpers.py b/tests/cli/test_remote_api_helpers.py new file mode 100644 index 00000000..73c76cae --- /dev/null +++ b/tests/cli/test_remote_api_helpers.py @@ -0,0 +1,42 @@ +"""Tests for the IDS metadata helper used when pushing IMAS data.""" + +import pytest + +from simdb.cli.remote_api import _meta_list + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + # The list form written since #119. + (["core_profiles", "equilibrium"], ["core_profiles", "equilibrium"]), + (("core_profiles", "equilibrium"), ["core_profiles", "equilibrium"]), + # The display string written by SimDB <= 1.2, which reaches the client + # from a remote that has not been migrated. + ("[core_profiles, equilibrium]", ["core_profiles", "equilibrium"]), + ("core_profiles", ["core_profiles"]), + # Nothing to filter on. + (None, []), + ("", []), + ("[]", []), + ], +) +def test_meta_list_normalises_both_stored_forms(value, expected): + assert _meta_list(value) == expected + + +@pytest.mark.parametrize( + "value", [["core_profiles", "equilibrium"], "[core_profiles, equilibrium]"] +) +def test_ids_names_match_however_the_value_was_stored(value): + """push_simulation skips any IDS whose name is not in this list. + + Wrapping the display string instead of splitting it yields + ``["[core_profiles, equilibrium]"]``, which matches no IDS name at all and so + silently skips every file of the simulation. + """ + ids_list = _meta_list(value) + + assert "core_profiles" in ids_list + assert "equilibrium" in ids_list + assert "core_sources" not in ids_list diff --git a/tests/cli/utils.py b/tests/cli/utils.py deleted file mode 100644 index 7c99c36f..00000000 --- a/tests/cli/utils.py +++ /dev/null @@ -1,51 +0,0 @@ -from pathlib import Path - - -def config_test_file() -> Path: - config = """\ -[remote "test"] -url = http://0.0.0.0:5000/ -default = True -token = 123ABC -""" - config_file = Path(__file__).parent / "test.cfg" - config_file.write_text(config) - return config_file - - -def create_manifest() -> Path: - manifest = """\ -manifest_version: 2 -alias: simulation-alias - -# Data and configuration files -inputs: -# - uri: simdb://simdb.iter.org/123e4567-e89b-12d3-a456-426655440000 - - uri: file:///$MANIFEST_DIR/utils.py - - uri: imas:///user?shot=10000&run=0&database=west -# - uri: imas+uda:///TOKAMAK?shot=10000&run=0&server=uda.server.org:56565 - -# Data and log files. -outputs: - - uri: file:///$MANIFEST_DIR/utils.py - - uri: imas:///user?shot=10000&run=1&database=west - -metadata: -- values: - workflow: - name: Workflow Name - git: ssh://git@git.iter.org/wf/workflow.git - branch: master - commit: 079e84d5ae8a0eec6dcf3819c98f3c05f48e952f - codes: - - Code 1: - git: ssh://git@git.iter.org/eq/code.git - commit: 079e84d5ae8a0eec6dcf3819c98f3c05f48e952f -""" - manifest_file = Path(__file__).parent / "manifest.yaml" - manifest_file.write_text(manifest) - return manifest_file - - -def get_file_path(file_name) -> Path: - return Path(__file__).parent / file_name From a974a8d4336d2c4aef14abcd0226d5d7191b8ed3 Mon Sep 17 00:00:00 2001 From: Yannick de Jong Date: Wed, 26 Aug 2026 10:57:51 +0200 Subject: [PATCH 6/6] Fix default remote handling when using options --- src/simdb/cli/commands/simulation.py | 29 +++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/simdb/cli/commands/simulation.py b/src/simdb/cli/commands/simulation.py index 551dde77..c6f523b3 100644 --- a/src/simdb/cli/commands/simulation.py +++ b/src/simdb/cli/commands/simulation.py @@ -3,7 +3,7 @@ import urllib.parse from itertools import chain from pathlib import Path -from typing import Any, List, Optional, Tuple, Type +from typing import Any, List, Optional, Tuple import appdirs import click @@ -200,19 +200,22 @@ def simulation_ingest(config: Config, manifest_file: str, alias: str): click.echo("ALIAS: " + simulation.alias + "\nUUID: " + str(simulation.uuid)) -def n_required_args_adaptor(n) -> Type[click.Command]: - class NRequiredArgs(click.Command): - NArgs = n +class OptionalRemoteCommand(click.Command): + """A command declared as `[REMOTE] ARG...` whose REMOTE may be left out.""" - def parse_args(self, ctx, args): - if len(args) == self.NArgs: - args.insert(0, "") - super().parse_args(ctx, args) + def parse_args(self, ctx, args): + arguments = [p for p in self.get_params(ctx) if isinstance(p, click.Argument)] + if self._count_values_given(ctx, args, arguments) < len(arguments): + args = ["", *args] + super().parse_args(ctx, args) - return NRequiredArgs + def _count_values_given(self, ctx, args, arguments) -> int: + """Count how many of the ARGUMENTS the command line provides a value for.""" + values = self.make_parser(ctx).parse_args(list(args))[0] + return sum(1 for argument in arguments if values.get(argument.name) is not None) -@simulation.command("push", cls=n_required_args_adaptor(1)) +@simulation.command("push", cls=OptionalRemoteCommand) @pass_config @click.argument("remote", required=False) @click.argument("sim_id") @@ -257,7 +260,7 @@ def simulation_push( click.echo(f"Successfully pushed simulation {simulation.uuid}") -@simulation.command("pull", cls=n_required_args_adaptor(2)) +@simulation.command("pull", cls=OptionalRemoteCommand) @pass_config @click.argument("remote", required=False) @click.argument("sim_id") @@ -380,7 +383,7 @@ def simulation_query( ) -@simulation.command("data", cls=n_required_args_adaptor(2)) +@simulation.command("data", cls=OptionalRemoteCommand) @pass_config @click.argument("remote", required=False) @click.argument("sim_id") @@ -447,7 +450,7 @@ def simulation_data( print_quantity(coord, label=f"coord {coord.name}", show_stats=False) -@simulation.command("validate", cls=n_required_args_adaptor(1)) +@simulation.command("validate", cls=OptionalRemoteCommand) @pass_config @click.argument("remote", required=False) @click.argument("sim_id")