diff --git a/AGENTS.md b/AGENTS.md index bb81bba6..0b837835 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,6 +113,17 @@ Each package's docs live alongside its source. Skim the index when working in a read on its own (drop a prefix only when the class already conveys it). - Ruff linting is currently only enforced on the `bfabric` package (scripts, wrapper_creator, tests, noxfile are excluded via per-file-ignores) - Line length: 120 (ruff and black) +- Docstrings use Sphinx `:param:` / `:raises:`; Google-style `Args:` / `Returns:` blocks survive in a + few older modules (e.g. `entities/core/uri.py`, `entities/core/entity_reader.py`) — don't add more. + Keep them at the lowest useful altitude: one summary line by default, plus a short paragraph only for + a contract the signature cannot show (why this exists beside a similar function, a gotcha, an ordering + constraint). Skip `Returns:` blocks that restate the return annotation and `:param:` lines that re-say + the name and type. Avoid `>>>` examples: no session collects doctests, so they read as tested without + being tested — put a short example inline in the prose instead. +- Exception: a cyclopts command function's docstring **is** its `--help` text — the summary line becomes + the command description and each `:param:` line becomes that option's help (see + `bfabric-cli workunit not-available --help`). Those are user-facing copy: keep them complete and clear + rather than terse, and trim the internal helpers around them instead. - Do not restate a parameter's default value in its docstring when the signature already shows it (e.g. `client_id: str = DEFAULT_CLIENT_ID`). Writing `(default "CLI")` in the `:param:` line just duplicates the signature and drifts out of sync when the default changes. Keep notes that explain what a value *means* (e.g. `(``0`` = auto-assign)`), not ones that merely repeat it. This also applies to class/model docstrings that restate a field's default shown a few lines below (prefer "see `field_name`" over repeating the literal value). Note the common case where the signature default is a sentinel like `None` but the docstring explains what it resolves to at runtime (e.g. `max_results: int | None = 100` documented as `` (``None`` for all) ``, or `path: Path | None = None` documented as `` (``None`` writes to ``./output.yml``) ``) — that is the *meaning* case, not the restatement case, and should be kept; phrase it as "``None`` does/means X", not "(default: X)", so it isn't mistaken for a literal restatement. - basedpyright uses per-package baseline files at `.basedpyright/baseline.{package}.json` — **do not edit baseline files to silence new errors**; fix the code or add a targeted `# pyright: ignore[...]` comment on the offending line. Baselines only exist to grandfather in pre-existing errors. - Integration tests live in a separate repository diff --git a/bfabric/docs/api_reference/entity_uri/index.md b/bfabric/docs/api_reference/entity_uri/index.md index baef6b15..9b8b8e46 100644 --- a/bfabric/docs/api_reference/entity_uri/index.md +++ b/bfabric/docs/api_reference/entity_uri/index.md @@ -34,11 +34,18 @@ https:///bfabric//show.html?id= Example: `https://fgcz-bfabric.uzh.ch/bfabric/sample/show.html?id=123` +The instance must be served over `https`; `http` is accepted for `localhost` only (development +instances). + +The constructor is strict: it accepts exactly this canonical form. To accept a URL as a user copied +it out of the browser, use [`normalize`](#normalize-a-web-url). + ### Key Features - **Validation**: Automatic validation of URI format and structure - **Parsing**: Extract entity type and ID from URIs - **Construction**: Create URIs from components +- **Normalization**: Turn a B-Fabric web URL into the canonical URI - **Cross-instance**: Reference entities from any B-Fabric instance ## Usage Examples @@ -57,6 +64,29 @@ print(uri.components.entity_type) # "sample" print(uri.components.entity_id) # 123 ``` +(normalize-a-web-url)= +### Normalize a Web URL + +A URL copied from the browser usually carries extra query parameters (e.g. the selected tab), which +the constructor rejects. `EntityUri.normalize` accepts it instead: + +```python +from bfabric.entities.core.uri import EntityUri + +uri = EntityUri.normalize( + "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=346001&tab=details" +) +print(uri) # "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=346001" +``` + +Dropped: every query parameter except `id`, and the fragment. Normalized: host case and a default +port (`:443` on `https`). Everything else is still validated as strictly as in the constructor — the +path must be `/bfabric//show.html`, and `id` must be present exactly once (repeated +`id` parameters with conflicting values are an error) and a positive integer. + +Because the result is canonical, it compares and hashes equal to the same entity's `EntityUri`, so a +pasted URL can be used as an `EntityResult` or cache key. + ### Construct URI from Components ```python diff --git a/bfabric/docs/changelog.md b/bfabric/docs/changelog.md index 291e8bfd..f349a429 100644 --- a/bfabric/docs/changelog.md +++ b/bfabric/docs/changelog.md @@ -14,6 +14,7 @@ Minor breaking changes are still possible in `1.X.Y` but we try to announce them - PKCE login: the browser callback page now renders a distinct, styled "Login failed" page showing the provider's error (e.g. a two-factor-enrollment requirement) instead of always claiming "Login successful". - OAuth token-acquisition failures (expired/revoked refresh token, unreachable token endpoint) now raise a clear `BfabricOAuthError` instead of leaking an `authlib`/`requests` traceback. - `EntityReader` lookups (`read_id` / `read_ids` / `query` / `query_one`) now accept an entity **class** in place of the endpoint string — e.g. `client.reader.read_id(Resource, id)` — inferring both the endpoint and the result type; the string form (with optional `expected_type`) still works. +- `EntityUri.normalize` — normalize a B-Fabric web URL (as copied from the browser) into a canonical entity URI, dropping extra query parameters (e.g. `&tab=details`) and the fragment, and normalizing host case and a default port. The `EntityUri` constructor stays strict and now hints at `normalize` when it rejects such a URL. URI validation is also no longer regex-based (`urllib.parse` instead), so its error messages name the actual problem. This leaves the canonical form the constructor accepts unchanged, except that credentials in the URL (`https://user:pw@host/bfabric/…`) are now rejected and the localhost exemption is case-insensitive. - The id/URI `EntityReader` lookups (`read_ids` / `read_uris`) now return an `EntityResult` — a `dict[EntityUri, Entity | None]` subclass with `.present` (found entities as a list) and `.by_id` (found entities re-keyed by integer id) properties, so callers write `reader.read_ids(Resource, ids).present` instead of wrapping the result in a helper. Class→endpoint inference is centralized in `import_entity.entity_type_of` (the inverse of `import_entity`). ## \[1.20.0rc2\] - 2026-07-15 diff --git a/bfabric/src/bfabric/entities/core/uri.py b/bfabric/src/bfabric/entities/core/uri.py index d7f29119..d6f1cfde 100644 --- a/bfabric/src/bfabric/entities/core/uri.py +++ b/bfabric/src/bfabric/entities/core/uri.py @@ -1,6 +1,5 @@ from __future__ import annotations -import re import urllib.parse from collections import defaultdict from typing import TYPE_CHECKING, Annotated, Any @@ -19,9 +18,7 @@ if TYPE_CHECKING: from collections.abc import Iterator -_URI_REGEX = re.compile( - r"^(?P(https://[^/]+/bfabric/|http://localhost(:\d+)?/bfabric/))(?P\w+)/show\.html\?id=(?P\d+)$" -) +_NORMALIZE_HINT = "use EntityUri.normalize to accept a browser URL" def _validate_entity_uri(uri: str) -> str: @@ -29,11 +26,44 @@ def _validate_entity_uri(uri: str) -> str: return uri -def _parse_uri_components(uri: str) -> EntityUriComponents: - match = _URI_REGEX.match(uri) - if not match: - raise ValueError(f"Invalid Entity URI: {uri}") - return EntityUriComponents.model_validate(match.groupdict()) +def _parse_uri_components(uri: str, *, allow_extra_query: bool = False) -> EntityUriComponents: + """Parse a B-Fabric entity URI into its components. + + :param uri: the URI to parse + :param allow_extra_query: ignore query parameters other than ``id``, and any fragment, instead of + rejecting them (i.e. accept a URL as copied from the browser) + :raises ValueError: if the URI is not a valid entity URI + """ + + def invalid(reason: str) -> ValueError: + return ValueError(f"Invalid Entity URI: {uri} ({reason})") + + parsed = urllib.parse.urlsplit(uri) + host = (parsed.hostname or "").lower() + if not host or (parsed.scheme != "https" and (parsed.scheme, host) != ("http", "localhost")): + raise invalid("expected https:// or http://localhost") + if parsed.username or parsed.password: + raise invalid("credentials are not allowed") + + segments = parsed.path.split("/")[1:] + if len(segments) != 3 or segments[0] != "bfabric" or segments[2] != "show.html": + raise invalid("expected path /bfabric//show.html") + + entity_ids = set(urllib.parse.parse_qs(parsed.query).get("id", [])) + if len(entity_ids) != 1: + raise invalid("conflicting 'id' query parameters" if entity_ids else "missing 'id' query parameter") + entity_id = entity_ids.pop() + if not entity_id.isdigit(): + raise invalid("entity id must be a positive integer") + # Strict mode parses the same way, then insists the URI was canonical to begin with. + if not allow_extra_query and (parsed.query != f"id={entity_id}" or parsed.fragment): + raise invalid(f"expected query exactly 'id=' and no fragment; {_NORMALIZE_HINT}") + + return EntityUriComponents( + bfabric_instance=HttpUrl(f"{parsed.scheme}://{parsed.netloc.lower()}/bfabric/"), + entity_type=segments[1], + entity_id=int(entity_id), + ) ValidatedEntityUri = Annotated[str, AfterValidator(_validate_entity_uri)] @@ -83,6 +113,15 @@ def from_components(cls, bfabric_instance: str, entity_type: str, entity_id: int bfabric_instance=bfabric_instance, entity_type=entity_type, entity_id=entity_id ).as_uri() + @classmethod + def normalize(cls, url: str) -> EntityUri: + """Normalize a B-Fabric web URL, e.g. one copied from the browser, to a canonical EntityUri. + + Extra query parameters and the fragment are dropped, so unlike the constructor this accepts + ``.../show.html?id=123&tab=details``, returning the canonical URI of the referenced entity. + """ + return _parse_uri_components(url, allow_extra_query=True).as_uri() + @property def components(self) -> EntityUriComponents: """Access parsed URI components.""" diff --git a/bfabric_scripts/docs/changelog.md b/bfabric_scripts/docs/changelog.md index aca41ed2..2ebae99a 100644 --- a/bfabric_scripts/docs/changelog.md +++ b/bfabric_scripts/docs/changelog.md @@ -10,6 +10,8 @@ Versioning currently follows `X.Y.Z` semantic versioning, independent of the `bf ## \[Unreleased\] +- `bfabric-cli workunit diff REF1 REF2` — compare two workunits side by side (name, parameters, output/input resources, status, application, container, input dataset), highlighting differences in rich tables. Each reference is a numeric workunit ID or a workunit URL, including one copied straight from the browser (extra query parameters such as `&tab=details` are accepted); `--only-diff` collapses the output to just the differing rows. + ## \[1.16.0rc2\] - 2026-07-15 - `bfabric-cli auth` — OAuth authentication & client management. Login: `login` (browser), `device-code` (headless), `pat`; client registration: `register` / `register-webapp`; environment management: `default`, `list`, `status`, `logout`. Scope presets (`read-only` / `read-write` / `upload`) or a raw scope, via an interactive picker when `--scope` is omitted in a terminal; no baked-in default scope, so a headless run must pass `--scope` (registration keeps the OIDC-inclusive default webapps need). When `--config-env` is omitted it prompts for the environment (else targets the current default / `PRODUCTION`); unless `--set-default` / `--no-set-default` is given it asks (default yes) whether to make the env the default, and cancelling that prompt aborts the login. `status` reports an OAuth env's cached-token freshness and granted scope (annotated with the matching preset); `logout` removes an env's config entry and cached tokens (confirmation required). PATs are stored under a `pat` key (`auth_method: pat`), keeping the config parseable by ≤1.19.0 clients. diff --git a/bfabric_scripts/src/bfabric_scripts/cli/cli_workunit.py b/bfabric_scripts/src/bfabric_scripts/cli/cli_workunit.py index c229164e..495b16ae 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/cli_workunit.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/cli_workunit.py @@ -1,5 +1,6 @@ import cyclopts +from bfabric_scripts.cli.workunit.diff import cmd_workunit_diff from bfabric_scripts.cli.workunit.export_definition import cmd_workunit_export_definition from bfabric_scripts.cli.workunit.not_available import ( cmd_workunit_not_available, @@ -10,3 +11,4 @@ _ = cmd_workunit.command(cmd_workunit_not_available, name="not-available") _ = cmd_workunit.command(cmd_workunit_export_definition, name="export-definition") _ = cmd_workunit.command(cmd_workunit_upload, name="upload") +_ = cmd_workunit.command(cmd_workunit_diff, name="diff") diff --git a/bfabric_scripts/src/bfabric_scripts/cli/workunit/diff.py b/bfabric_scripts/src/bfabric_scripts/cli/workunit/diff.py new file mode 100644 index 00000000..fa643291 --- /dev/null +++ b/bfabric_scripts/src/bfabric_scripts/cli/workunit/diff.py @@ -0,0 +1,172 @@ +from __future__ import annotations + +from dataclasses import dataclass +from typing import TYPE_CHECKING, TypeVar + +from rich.console import Console +from rich.markup import escape +from rich.table import Table + +from bfabric import Bfabric +from bfabric.entities import Workunit +from bfabric.entities.core.uri import EntityUri +from bfabric.utils.cli_integration import use_client + +if TYPE_CHECKING: + from collections.abc import Iterable + + from bfabric.entities import Resource + +_MISSING = "—" + +KeyT = TypeVar("KeyT", str, tuple[str, str]) + + +def _text(value: object) -> str: + """Coerce a raw entity field value to a display string (``None`` -> empty).""" + return "" if value is None else str(value) + + +@dataclass(frozen=True) +class DiffRow: + """A single comparison row: a key and its value on each side (``None`` = absent on that side).""" + + key: tuple[str, ...] + left: str | None + right: str | None + + @property + def differs(self) -> bool: + return self.left != self.right + + +def diff_rows(left: dict[KeyT, str], right: dict[KeyT, str]) -> list[DiffRow]: + """Compare two dicts, one row per key in the sorted union of both key sets.""" + rows: list[DiffRow] = [] + for key in sorted(set(left) | set(right)): + key_tuple = key if isinstance(key, tuple) else (key,) + rows.append(DiffRow(key=tuple(str(k) for k in key_tuple), left=left.get(key), right=right.get(key))) + return rows + + +def _resolve_workunit(reference: str, *, client: Bfabric) -> Workunit: + """Resolve a workunit reference (numeric ID or workunit URL) to a ``Workunit``.""" + if reference.isdigit(): + workunit = client.reader.read_id(Workunit, int(reference)) + else: + try: + # Lenient parse, so a URL copied from the browser (with e.g. "&tab=details") is accepted. + uri = EntityUri.normalize(reference) + except ValueError as error: + # The parser's complaint is about URL syntax; the user needs to know an ID would do too. + raise ValueError(f"Not a workunit URI or numeric ID: {reference}") from error + if uri.components.entity_type != "workunit": + raise ValueError(f"Not a workunit URI: {reference}") + workunit = client.reader.read_uri(uri, expected_type=Workunit) + + if workunit is None: + raise ValueError(f"Workunit not found: {reference}") + return workunit + + +def _fields(workunit: Workunit) -> dict[str, str]: + """Scalar fields worth comparing between two workunits.""" + application = workunit.application + container = workunit.container + input_dataset = workunit.input_dataset + return { + "name": _text(workunit.get("name")), + "status": _text(workunit.get("status")), + "application": f"A{application.id} {_text(application.get('name'))}".strip(), + "container": f"{container.classname} {container.id}", + "input dataset": str(input_dataset.id) if input_dataset is not None else _MISSING, + } + + +def _parameters(workunit: Workunit) -> dict[tuple[str, str], str]: + """Parameters keyed by ``(context, key)`` so identically-named params in different contexts stay distinct.""" + return {(_text(parameter.get("context")), parameter.key): parameter.value for parameter in workunit.parameters} + + +def _resource_map(resources: Iterable[Resource]) -> dict[str, str]: + """Map each resource name to its checksum (``""`` when unknown) for presence/content comparison.""" + result: dict[str, str] = {} + for resource in resources: + name = _text(resource.get("name")) or f"resource {resource.id}" + result[name] = _text(resource.get("filechecksum")) + return result + + +def render_section( + console: Console, + title: str, + key_headers: list[str], + rows: list[DiffRow], + labels: tuple[str, str], + only_diff: bool, +) -> None: + """Render one comparison section as a rich table, styling and marking rows that differ.""" + shown = [row for row in rows if row.differs] if only_diff else rows + console.print(f"[bold]{title}[/bold]") + if not shown: + console.print(" (no differences)" if rows else " (empty)") + console.print() + return + + table = Table(show_header=True, header_style="bold") + for header in key_headers: + table.add_column(header) + table.add_column(labels[0]) + table.add_column(labels[1]) + table.add_column("") + + for row in shown: + style = "yellow" if row.differs else None + # Escape cell text: entity data (names, param values, checksums) may contain rich-markup + # characters, which would otherwise misrender or raise MarkupError (e.g. a value of "[/]"). + table.add_row( + *(escape(part) for part in row.key), + escape(row.left) if row.left is not None else _MISSING, + escape(row.right) if row.right is not None else _MISSING, + "≠" if row.differs else "", + style=style, + ) + console.print(table) + console.print() + + +@use_client +def cmd_workunit_diff(workunit1: str, workunit2: str, *, client: Bfabric, only_diff: bool = False) -> None: + """Compare two workunits (name, parameters, resources, and key fields) side by side. + + :param workunit1: first workunit — a numeric ID or a workunit URL (as copied from the browser) + :param workunit2: second workunit — a numeric ID or a workunit URL (as copied from the browser) + :param only_diff: show only fields/parameters/resources that differ + """ + wu1 = _resolve_workunit(workunit1, client=client) + wu2 = _resolve_workunit(workunit2, client=client) + + labels = (f"WU{wu1.id}", f"WU{wu2.id}") + console = Console() + console.print(f"[bold]Workunit diff: {labels[0]} vs {labels[1]}[/bold]\n") + + render_section(console, "Fields", ["Field"], diff_rows(_fields(wu1), _fields(wu2)), labels, only_diff) + render_section( + console, "Parameters", ["Context", "Key"], diff_rows(_parameters(wu1), _parameters(wu2)), labels, only_diff + ) + render_section( + console, + "Output resources", + ["Resource"], + diff_rows(_resource_map(wu1.resources), _resource_map(wu2.resources)), + labels, + only_diff, + ) + render_section( + console, + "Input resources", + ["Resource"], + diff_rows(_resource_map(wu1.input_resources), _resource_map(wu2.input_resources)), + labels, + only_diff, + ) diff --git a/tests/bfabric/entities/core/test_uri.py b/tests/bfabric/entities/core/test_uri.py index accca3b2..fb927d95 100644 --- a/tests/bfabric/entities/core/test_uri.py +++ b/tests/bfabric/entities/core/test_uri.py @@ -4,6 +4,8 @@ from bfabric.entities.core.uri import EntityUri, EntityUriComponents, GroupedUris from bfabric.entities.core.uri import _parse_uri_components +CANONICAL_URI = "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=346001" + class TestEntityUri: @pytest.mark.parametrize( @@ -24,6 +26,19 @@ def test_invalid(self): EntityUri(uri) assert "Invalid Entity URI" in str(error.value) + @pytest.mark.parametrize( + "uri", + [ + f"{CANONICAL_URI}&tab=details", + f"{CANONICAL_URI}#tab", + f"{CANONICAL_URI}&id=346001", + ], + ) + def test_non_canonical_rejected_with_hint(self, uri): + """The constructor stays strict, but points at the lenient entry point.""" + with pytest.raises(ValueError, match="EntityUri.normalize"): + EntityUri(uri) + def test_components_property(self): uri = "https://fgcz-bfabric.uzh.ch/bfabric/project/show.html?id=3000" entity_uri = EntityUri(uri) @@ -42,6 +57,67 @@ def test_from_components(self, bfabric_instance: str): assert isinstance(entity_uri, EntityUri) +class TestNormalize: + @pytest.mark.parametrize( + "url", + [ + CANONICAL_URI, + f"{CANONICAL_URI}&tab=details", + "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?tab=details&id=346001", + f"{CANONICAL_URI}#tab", + f"{CANONICAL_URI}&id=346001", + "https://FGCZ-Bfabric.UZH.ch/bfabric/workunit/show.html?id=346001", + "https://fgcz-bfabric.uzh.ch:443/bfabric/workunit/show.html?id=346001", + ], + ) + def test_normalizes_to_canonical(self, url): + uri = EntityUri.normalize(url) + assert uri == CANONICAL_URI + assert isinstance(uri, EntityUri) + + def test_keeps_explicit_non_default_port(self): + uri = EntityUri.normalize("http://localhost:8080/bfabric/project/show.html?id=3000&tab=details") + assert uri == "http://localhost:8080/bfabric/project/show.html?id=3000" + + def test_idempotent_on_entity_uri(self): + assert EntityUri.normalize(EntityUri(CANONICAL_URI)) == CANONICAL_URI + + def test_components(self): + components = EntityUri.normalize(f"{CANONICAL_URI}&tab=details").components + assert components.bfabric_instance == HttpUrl("https://fgcz-bfabric.uzh.ch/bfabric/") + assert components.entity_type == "workunit" + assert components.entity_id == 346001 + + def test_keys_dict_like_canonical(self): + """Normalization is what makes a pasted URL usable as an EntityResult / cache key.""" + uri = EntityUri.normalize(f"{CANONICAL_URI}&tab=details") + assert hash(uri) == hash(EntityUri(CANONICAL_URI)) + assert len({uri, EntityUri(CANONICAL_URI)}) == 1 + + @pytest.mark.parametrize( + "url", + [ + "http://example.com/bfabric/workunit/show.html?id=346001", + "ftp://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=346001", + "https://user:pw@fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=346001", + "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html", + "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?tab=details", + "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=abc", + "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=0", + "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=346001&id=346002", + "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.htm?id=346001", + "https://fgcz-bfabric.uzh.ch/lims/bfabric/workunit/show.html?id=346001", + "https://fgcz-bfabric.uzh.ch/bfabric/Workunit/show.html?id=346001", + "https://example.com/invalid/uri", + "not-a-url", + "", + ], + ) + def test_invalid(self, url): + with pytest.raises(ValueError): + EntityUri.normalize(url) + + class TestEntityUriComponents: @pytest.mark.parametrize( "bfabric_instance", diff --git a/tests/bfabric_scripts/cli/workunit/test_cmd_workunit_diff.py b/tests/bfabric_scripts/cli/workunit/test_cmd_workunit_diff.py new file mode 100644 index 00000000..78913a9d --- /dev/null +++ b/tests/bfabric_scripts/cli/workunit/test_cmd_workunit_diff.py @@ -0,0 +1,247 @@ +import os + +import pytest +from bfabric import Bfabric +from bfabric.entities import Workunit +from bfabric_scripts.cli.workunit.diff import ( + DiffRow, + _resolve_workunit, + cmd_workunit_diff, + diff_rows, +) + +WORKUNIT_URI = "https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=123" +SAMPLE_URI = "https://fgcz-bfabric.uzh.ch/bfabric/sample/show.html?id=5" + + +@pytest.fixture +def mock_client(mocker): + client = mocker.Mock(spec=Bfabric) + client.config.base_url = "https://fgcz-bfabric.uzh.ch/bfabric/" + return client + + +def _make_param(mocker, context, key, value): + param = mocker.MagicMock() + param.get.side_effect = lambda k, d=None: {"context": context}.get(k, d) + param.key = key + param.value = value + return param + + +def _make_resource(mocker, name, checksum): + resource = mocker.MagicMock() + resource.get.side_effect = lambda k, d=None: {"name": name, "filechecksum": checksum}.get(k, d) + return resource + + +def _make_workunit( + mocker, + *, + id, + name, + status, + app_id, + app_name, + container_class="project", + container_id=1, + input_dataset_id=None, + parameters=(), + resources=(), + input_resources=(), +): + workunit = mocker.MagicMock() + workunit.id = id + workunit.get.side_effect = lambda k, d=None: {"name": name, "status": status}.get(k, d) + workunit.application.id = app_id + workunit.application.get.side_effect = lambda k, d=None: {"name": app_name}.get(k, d) + workunit.container.classname = container_class + workunit.container.id = container_id + if input_dataset_id is None: + workunit.input_dataset = None + else: + workunit.input_dataset.id = input_dataset_id + workunit.parameters = [_make_param(mocker, ctx, key, val) for ctx, key, val in parameters] + workunit.resources = [_make_resource(mocker, n, c) for n, c in resources] + workunit.input_resources = [_make_resource(mocker, n, c) for n, c in input_resources] + return workunit + + +class TestDiffRows: + def test_equal_values(self): + rows = diff_rows({"a": "1"}, {"a": "1"}) + assert rows == [DiffRow(key=("a",), left="1", right="1")] + assert not rows[0].differs + + def test_changed_value(self): + rows = diff_rows({"a": "1"}, {"a": "2"}) + assert rows[0].differs + + def test_added_and_removed(self): + rows = diff_rows({"only_left": "1"}, {"only_right": "2"}) + by_key = {row.key: row for row in rows} + assert by_key[("only_left",)].left == "1" + assert by_key[("only_left",)].right is None + assert by_key[("only_right",)].left is None + assert by_key[("only_left",)].differs + assert by_key[("only_right",)].differs + + def test_sorted_union_of_keys(self): + rows = diff_rows({"b": "1", "a": "1"}, {"c": "1"}) + assert [row.key for row in rows] == [("a",), ("b",), ("c",)] + + def test_tuple_keys(self): + rows = diff_rows({("APPLICATION", "threads"): "4"}, {("APPLICATION", "threads"): "8"}) + assert rows[0].key == ("APPLICATION", "threads") + assert rows[0].differs + + +class TestResolveWorkunit: + def test_resolve_by_uri(self, mock_client, mocker): + expected = mocker.Mock(spec=Workunit) + mock_client.reader.read_uri.return_value = expected + + result = _resolve_workunit(WORKUNIT_URI, client=mock_client) + + assert result is expected + mock_client.reader.read_uri.assert_called_once_with(WORKUNIT_URI, expected_type=Workunit) + + def test_resolve_by_id(self, mock_client, mocker): + expected = mocker.Mock(spec=Workunit) + mock_client.reader.read_id.return_value = expected + + result = _resolve_workunit("123", client=mock_client) + + assert result is expected + mock_client.reader.read_id.assert_called_once_with(Workunit, 123) + + def test_resolve_by_browser_url(self, mock_client, mocker): + """A URL copied from the browser carries extra query parameters and must still resolve.""" + expected = mocker.Mock(spec=Workunit) + mock_client.reader.read_uri.return_value = expected + + result = _resolve_workunit(f"{WORKUNIT_URI}&tab=details", client=mock_client) + + assert result is expected + mock_client.reader.read_uri.assert_called_once_with(WORKUNIT_URI, expected_type=Workunit) + + def test_non_workunit_uri_raises(self, mock_client): + with pytest.raises(ValueError, match="Not a workunit URI"): + _resolve_workunit(SAMPLE_URI, client=mock_client) + + def test_non_workunit_browser_url_raises(self, mock_client): + # Anchored on the colon: the "or numeric ID" fallback message must not satisfy this. + with pytest.raises(ValueError, match="Not a workunit URI: "): + _resolve_workunit(f"{SAMPLE_URI}&tab=details", client=mock_client) + + def test_invalid_reference_raises(self, mock_client): + with pytest.raises(ValueError, match="Not a workunit URI or numeric ID"): + _resolve_workunit("not-a-ref", client=mock_client) + + def test_not_found_raises(self, mock_client): + mock_client.reader.read_id.return_value = None + with pytest.raises(ValueError, match="Workunit not found"): + _resolve_workunit("999", client=mock_client) + + +class TestCmdWorkunitDiff: + @pytest.fixture(autouse=True) + def _wide_console(self, mocker): + # Widen rich's output so table cells aren't wrapped/truncated in captured (non-tty) output. + mocker.patch.dict(os.environ, {"COLUMNS": "200"}) + + def test_renders_fields_and_marks_differences(self, mocker, mock_client, capsys): + wu1 = _make_workunit( + mocker, + id=100, + name="run", + status="available", + app_id=170, + app_name="MaxQuant", + parameters=[("APPLICATION", "threads", "4"), ("APPLICATION", "fdr", "0.01")], + resources=[("out.zip", "aaa")], + ) + wu2 = _make_workunit( + mocker, + id=200, + name="run", + status="FAILED", + app_id=170, + app_name="MaxQuant", + parameters=[("APPLICATION", "threads", "8"), ("APPLICATION", "fdr", "0.01")], + resources=[("out.zip", "aaa"), ("extra.zip", "bbb")], + ) + mocker.patch("bfabric_scripts.cli.workunit.diff._resolve_workunit", side_effect=[wu1, wu2]) + + cmd_workunit_diff("100", "200", client=mock_client) + + out = capsys.readouterr().out + assert "Workunit diff: WU100 vs WU200" in out + assert "≠" in out + assert "FAILED" in out + assert "threads" in out + assert "extra.zip" in out + + def test_only_diff_hides_identical_rows(self, mocker, mock_client, capsys): + wu1 = _make_workunit( + mocker, + id=100, + name="run", + status="available", + app_id=170, + app_name="MaxQuant", + parameters=[("APPLICATION", "threads", "4"), ("APPLICATION", "fdr", "0.01")], + ) + wu2 = _make_workunit( + mocker, + id=200, + name="run", + status="available", + app_id=170, + app_name="MaxQuant", + parameters=[("APPLICATION", "threads", "8"), ("APPLICATION", "fdr", "0.01")], + ) + mocker.patch("bfabric_scripts.cli.workunit.diff._resolve_workunit", side_effect=[wu1, wu2]) + + cmd_workunit_diff("100", "200", client=mock_client, only_diff=True) + + out = capsys.readouterr().out + # threads differs -> shown; fdr is identical -> hidden; name/status identical -> "(no differences)". + assert "threads" in out + assert "fdr" not in out + assert "(no differences)" in out + + def test_markup_like_values_are_escaped_not_interpreted(self, mocker, mock_client, capsys): + # rich parses "[/]" as a closing markup tag and raises MarkupError unless the cell is escaped. + wu1 = _make_workunit( + mocker, + id=100, + name="run", + status="ok", + app_id=170, + app_name="MaxQuant", + parameters=[("APPLICATION", "expr", "[/]")], + resources=[("[bold]a.zip", "aaa")], + ) + wu2 = _make_workunit( + mocker, + id=200, + name="run", + status="ok", + app_id=170, + app_name="MaxQuant", + parameters=[("APPLICATION", "expr", "value")], + resources=[], + ) + mocker.patch("bfabric_scripts.cli.workunit.diff._resolve_workunit", side_effect=[wu1, wu2]) + + cmd_workunit_diff("100", "200", client=mock_client) + + out = capsys.readouterr().out + # No crash, and the literal characters survive rather than being swallowed as markup. + assert "[/]" in out + assert "[bold]a.zip" in out + + +if __name__ == "__main__": + pytest.main(["-vv", __file__])