diff --git a/src/bcli/client/_async.py b/src/bcli/client/_async.py index 835ce14..830be3b 100644 --- a/src/bcli/client/_async.py +++ b/src/bcli/client/_async.py @@ -2,6 +2,7 @@ from __future__ import annotations +import re from pathlib import Path from typing import Any @@ -17,6 +18,56 @@ from bcli.registry._registry import EndpointRegistry +# OData v4 *bound action* (and bound function) invocation pattern: +# +# ()/.<...>. +# +# - ```` is a normal identifier; it's the parent entity set +# that the registry validator looks up. +# - ```` is anything inside the parentheses — a GUID, a single- +# quoted string, an int, or a composite ``k1='a',k2='b'``. We do not +# over-validate it; BC will reject malformed keys server-side and +# passing through preserves the user's spelling for debugging. +# - The tail is a *qualified identifier* — at least one dot separates +# the namespace from the action/function name. We intentionally don't +# hardcode ``Microsoft.NAV`` so the parser works with any tenant's +# custom action namespaces. +_BOUND_ACTION_RE = re.compile( + r"^(?P[A-Za-z_][A-Za-z0-9_]*)" + r"\((?P.+)\)" + r"/(?P[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)+)$" +) + +# An unbound action at the OData service root: ``Namespace.action`` +# with no parent entity. v0.1 explicitly rejects these — a future +# revision could route them past the company-id URL builder, but the +# bound-action case is the only one in the bug report this PR fixes. +_UNBOUND_ACTION_RE = re.compile( + r"^[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)+$" +) + + +def _parse_bound_action(entity_set_name: str) -> tuple[str, str, str] | None: + """Recognise a bound-action invocation in ``entity_set_name``. + + Returns ``(parent_entity_set, key, qualified_action)`` if the + string matches the OData v4 bound-action shape, ``None`` otherwise. + A plain entity-set name (``customers``) or a record URL with no + action tail (``customers(123)``) also returns ``None`` — those go + through the standard validator path unchanged. + """ + m = _BOUND_ACTION_RE.match(entity_set_name) + if m is None: + return None + return m.group("entity"), m.group("key"), m.group("qualified") + + +def _is_unbound_action(entity_set_name: str) -> bool: + """Return ``True`` for a service-root unbound-action invocation + (``Namespace.action`` with no parent entity set).""" + return bool(_UNBOUND_ACTION_RE.match(entity_set_name)) + + class AsyncBCClient: """Async client for Business Central APIs. @@ -461,6 +512,67 @@ def _resolve_url_for_target( "No company_id configured. Run 'bcli config init' or 'bcli company use '." ) + # ─── OData v4 bound action / function invocation ────────────── + # + # Pattern: ``()/.<...>.``. + # The registry validator should look up *only the parent + # entity set*; everything from the ``(`` onward is opaque to + # the registry and gets stitched back onto the resolved URL. + # This keeps ``disable_standard_api`` enforcement intact (gated + # on the parent, which is the security-relevant identity) and + # honours custom-route registry entries on the parent. + bound = _parse_bound_action(entity_set_name) + if bound is not None: + parent, key, qualified = bound + # Composing a parent URL with ``record_id`` here would + # double-append parens — actions encode the key inside the + # bound-action string, so we resolve the parent alone and + # then splice the ``(key)/qualified`` tail. + parent_url = self._resolve_url_for_target( + environment, + company_id, + parent, + record_id=None, + publisher=publisher, + group=group, + version=version, + ) + return f"{parent_url}({key})/{qualified}" + + # Service-root unbound action: ``Namespace.action`` with no + # parent entity. Resolves to ``companies()/.`` + # under the chosen API route. Registry lookup is skipped (unbound + # actions aren't entity sets and can't be registered), but the + # ``disable_standard_api`` security gate still applies when no + # explicit publisher/group/version override is supplied. + if _is_unbound_action(entity_set_name): + if publisher and group and version: + return build_url( + environment=environment, + company_id=company_id, + entity_set_name=entity_set_name, + record_id=None, + publisher=publisher, + group=group, + version=version, + ) + if self._profile.disable_standard_api: + from bcli.errors import RegistryError + + raise RegistryError( + f"Unbound action '{entity_set_name}' cannot be routed: " + f"'disable_standard_api = true' blocks the standard v2.0 " + f"fallback, and unbound actions are not registry entries. " + f"Pass --publisher/--group/--version to target a custom " + f"API route." + ) + return build_url( + environment=environment, + company_id=company_id, + entity_set_name=entity_set_name, + record_id=None, + ) + # Explicit override takes priority if publisher and group and version: return build_url( diff --git a/src/bcli/workflow/_models.py b/src/bcli/workflow/_models.py index abc7a5a..4a50dea 100644 --- a/src/bcli/workflow/_models.py +++ b/src/bcli/workflow/_models.py @@ -5,7 +5,7 @@ from dataclasses import dataclass, field from typing import Any, Literal -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, model_validator # ─── Pydantic models (YAML → validated structure) ──────────────────── @@ -20,7 +20,18 @@ class ParamDef(BaseModel): class StepDef(BaseModel): - """A single step as declared in a workflow YAML file.""" + """A single step as declared in a workflow YAML file. + + Steps may specify the HTTP verb under either of two keys: + + * ``action: post`` — the original spelling, lowercased. + * ``method: POST`` — alias kept because authors copy-pasting OData + examples (especially bound-action invocations) reach for ``method`` + out of habit. The model lowercases the value before assigning it + to ``action``. + + Both keys at once are rejected to keep YAML files unambiguous. + """ name: str = "" action: Literal["get", "post", "patch", "delete"] = "get" @@ -30,6 +41,41 @@ class StepDef(BaseModel): id: str | None = None etag: str = "*" + @model_validator(mode="before") + @classmethod + def _accept_method_alias(cls, data: Any) -> Any: + """Translate ``method:`` → ``action:`` before field validation. + + Runs in ``mode="before"`` so the ``Literal[...]`` validator on + ``action`` sees the normalised lowercase value and the unknown + ``method`` field is consumed (it would otherwise raise an + ``extra inputs`` error under ``model_config = ConfigDict(extra= + "forbid")`` — kept open so existing YAML stays valid, but the + validator still needs to drop the alias). + """ + if not isinstance(data, dict): + return data + if "method" not in data: + return data + method_raw = data["method"] + # Default to checking against the literal set the field accepts. + # Anything else gets normalised and passed through so pydantic's + # ``Literal[...]`` validator raises with a clear message. + if isinstance(method_raw, str): + method_norm = method_raw.lower() + else: + method_norm = method_raw + if "action" in data: + raise ValueError( + "Step declares both 'action' and 'method'. Pick one key — " + "'action' is the canonical spelling, 'method' is an alias " + "accepted for OData copy-paste habits." + ) + # Strip the alias and inject the normalised action. + normalised = {k: v for k, v in data.items() if k != "method"} + normalised["action"] = method_norm + return normalised + class WorkflowDef(BaseModel): """Top-level workflow definition parsed from YAML.""" diff --git a/src/bcli_cli/app.py b/src/bcli_cli/app.py index 8cc34a8..a55db5c 100644 --- a/src/bcli_cli/app.py +++ b/src/bcli_cli/app.py @@ -158,6 +158,7 @@ def _emit_command_summary() -> None: # Import and register command groups from bcli_cli.commands import ( # noqa: E402 + action_cmd, attach_cmd, auth_cmd, batch_cmd, @@ -197,6 +198,10 @@ def _emit_command_summary() -> None: app.command(name="post")(post_cmd.post_command) app.command(name="patch")(patch_cmd.patch_command) app.command(name="delete")(delete_cmd.delete_command) +app.command( + name="action", + help="Invoke an OData v4 bound action on a record", +)(action_cmd.action_command) app.command(name="q", help="Run a saved query (no OData required)")(query_cmd.query_command) app.command(name="ai-context")(context_cmd.ai_context_command) app.command(name="doctor", help="Diagnose your bcli install (self-rescue for team users)")(doctor_cmd.doctor_command) diff --git a/src/bcli_cli/commands/action_cmd.py b/src/bcli_cli/commands/action_cmd.py new file mode 100644 index 0000000..bc40cb4 --- /dev/null +++ b/src/bcli_cli/commands/action_cmd.py @@ -0,0 +1,215 @@ +"""bcli action — invoke an OData v4 bound action. + +OData v4 *actions* are POST requests against a URL of the form + + ()/. + +where ``.`` is a server-side declared action +bound to the record at ``()``. Hand-writing that string +is error-prone (the parentheses + dot escaping in particular trips up +shells), so this verb composes the URL from named arguments and +forwards to the same client.post path that ``bcli post`` uses. + +Spec-conformance notes: + +* Actions are always POST regardless of any caller intent — OData v4 + does not allow GET on an action invocation. +* The default namespace is ``Microsoft.NAV`` (the BC convention); the + ``--namespace`` flag overrides it for non-BC tenants. The registry + validator itself is namespace-agnostic — see + ``bcli.client._async._parse_bound_action``. +* ``--data`` defaults to an empty body when omitted (matching + ``bcli post`` semantics). ``--no-data`` is retained as an explicit + no-op alias for callers who want to spell the intent out; passing + both ``--data`` and ``--no-data`` is still an error. +""" + +from __future__ import annotations + +import asyncio +import json +from pathlib import Path +from typing import Optional + +import typer +from rich.console import Console + +from bcli_cli._envelope_wrap import capture, validate_flags +from bcli_cli._state import state +from bcli_cli.output import format_output, print_context_banner + +console = Console(stderr=True) + + +DEFAULT_NAMESPACE = "Microsoft.NAV" + + +def action_command( + entity_set: str = typer.Argument( + ..., help="Parent entity set name (e.g. 'examples')", + ), + key: str = typer.Argument( + ..., + help=( + "Record key inside the parens. Pass a GUID/int as bare text " + "or a string key with explicit single quotes: \"'ALFKI'\"." + ), + ), + action_name: str = typer.Argument( + ..., help="Action identifier (e.g. 'archive')", + ), + data: Optional[str] = typer.Option( + None, + "--data", + "-d", + help="JSON body for the action (literal or @filename).", + ), + no_data: bool = typer.Option( + False, + "--no-data", + help="Explicitly send an empty body. Same as omitting --data; " + "kept for callers who want to spell the intent out.", + ), + namespace: Optional[str] = typer.Option( + None, + "--namespace", + "-n", + help=f"Action namespace (default: {DEFAULT_NAMESPACE}).", + ), + format: Optional[str] = typer.Option( + None, "--format", "-f", + help="Output format: table, json, csv, ndjson, raw", + ), + publisher: Optional[str] = typer.Option(None, "--publisher", hidden=True), + group: Optional[str] = typer.Option(None, "--group", hidden=True), + version: Optional[str] = typer.Option(None, "--version", hidden=True), + yes: bool = typer.Option( + False, "--yes", "-y", + help="Skip the read-only-profile warning prompt", + ), + result_out: Optional[Path] = typer.Option( + None, "--result-out", + help="Write a JSON result envelope to this path (atomic). See AIP §Phase 2.", + ), + result_fd: Optional[int] = typer.Option( + None, "--result-fd", + help="Write the JSON result envelope to this file descriptor and close it.", + ), + idempotency_key: Optional[str] = typer.Option( + None, "--idempotency-key", + help="Opaque token forwarded as Idempotency-Key header (AIP §Phase 4d).", + ), +) -> None: + """Invoke an OData v4 bound action on a record. + + \b + Examples: + bcli action examples 42 archive + bcli action items "'ALFKI'" doSomething --data '{"flag": true}' + bcli action widgets 7 cancel --namespace Custom.Ns --data @payload.json + """ + validate_flags(result_out, result_fd) + + # ``--data`` and ``--no-data`` may not both be set. Either alone, or + # neither (defaults to ``{}``), is fine — matches ``bcli post``. + if data is not None and no_data: + raise typer.BadParameter( + "--data and --no-data are mutually exclusive. " + "Pass one or the other (or neither — empty body is the default).", + ) + + body: dict = {} if data is None else _parse_data(data) + + ns = namespace or DEFAULT_NAMESPACE + # Compose the synthetic bound-action string. The registry validator + # downstream (``_parse_bound_action``) will split it back into + # parent + key + qualified-action; we don't try to second-guess that + # parse here — keeping the bound-action shape the single source of + # truth. + composed = f"{entity_set}({key})/{ns}.{action_name}" + + output_format = format or state.format + state.format = output_format + if output_format in ("json", "csv", "ndjson", "raw"): + state.quiet = True + + print_context_banner() + + with capture( + method="POST", + endpoint=composed, + result_out=result_out, + result_fd=result_fd, + ) as cap: + from bcli_cli._safety import confirm_write_or_exit + from bcli_cli._url_resolve import try_resolve_url + + cap.set_resolved_url(try_resolve_url( + composed, + publisher=publisher, group=group, version=version, + )) + + # disable_writes / read-only-profile gate. Same policy as ``post`` + # — actions are mutating writes from the agent's perspective. + confirm_write_or_exit("ACTION", composed, yes=yes) + + if state.dry_run: + from bcli_cli._dry_run import render_dry_run + cap.mark_dry_run() + cap.emit_success() + render_dry_run( + "POST", composed, body=body, + publisher=publisher, group=group, version=version, + ) + + try: + result = asyncio.run(_audited_post( + composed, body, + publisher=publisher, group=group, version=version, + idempotency_key=idempotency_key, + )) + cap.extract_record_id_from(result) + cap.emit_success() + if result: + format_output([result], output_format) + else: + # 204 No Content — common for actions that mutate but + # don't return a payload. Stay quiet on machine-readable + # formats, print a friendly line on table/markdown. + if output_format in (None, "table", "markdown"): + console.print("[green]✓[/green] Action invoked (204 No Content)") + except Exception as e: + cap.emit_failure(e) + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(1) + + +async def _audited_post(endpoint, body, **kwargs): + from bcli_cli._audit_wrap import audited_write + from bcli_cli._url_resolve import try_resolve_url + resolved_url = try_resolve_url( + endpoint, + publisher=kwargs.get("publisher"), + group=kwargs.get("group"), + version=kwargs.get("version"), + ) + return await audited_write( + _execute_post(endpoint, body, **kwargs), + method="POST", endpoint=endpoint, body=body, + resolved_url=resolved_url, + ) + + +async def _execute_post(endpoint, body, **kwargs): + async with state.make_async_client() as client: + return await client.post(endpoint, body, **kwargs) + + +def _parse_data(data: str) -> dict: + """Parse --data argument: JSON string or @filename.""" + if data.startswith("@"): + path = Path(data[1:]) + if not path.is_file(): + raise typer.BadParameter(f"File not found: {path}") + return json.loads(path.read_text(encoding="utf-8")) + return json.loads(data) diff --git a/src/bcli_cli/commands/batch_cmd.py b/src/bcli_cli/commands/batch_cmd.py index 173cb0b..e3439ff 100644 --- a/src/bcli_cli/commands/batch_cmd.py +++ b/src/bcli_cli/commands/batch_cmd.py @@ -194,6 +194,15 @@ def _compose_rollback_url(client: Any, entity: str, post_result: Any) -> str | N """ if not isinstance(post_result, dict): return None + # OData actions (bound or unbound) have no inverse — there's no + # "DELETE the archiving" of ``examples(42)/Microsoft.NAV.archive``, + # nor of ``Microsoft.NAV.refreshAll``. If an action happens to + # return a body with an ``id`` field, composing a rollback URL by + # appending ``(id)`` would yield a nonsense path. The batch ledger + # should record ``rollback_skipped`` instead. + from bcli.client._async import _is_unbound_action, _parse_bound_action + if _parse_bound_action(entity) is not None or _is_unbound_action(entity): + return None record_id = post_result.get("id") or post_result.get("systemId") if not record_id: return None @@ -353,15 +362,25 @@ def run_batch( # update; the BaseException branch below derives a truthful # state from the (empty) step table when the gate fires. mutating_actions = {"post", "patch", "delete"} + + def _step_action(step: dict) -> str: + """Mirror the alias logic from ``_execute_batch`` so the + disable_writes gate doesn't miss a step that uses + ``method: POST`` instead of ``action: post``.""" + raw = step.get("action") + if raw is None and "method" in step: + m = step.get("method") + raw = m.lower() if isinstance(m, str) else None + return (raw or "get").lower() + mutating_steps = [ - step for step in steps - if (step.get("action") or "get").lower() in mutating_actions + step for step in steps if _step_action(step) in mutating_actions ] if mutating_steps: from bcli_cli._safety import confirm_write_or_exit preview = ", ".join( - f"{(step.get('action') or 'get').upper()} {step.get('endpoint', '?')}" + f"{_step_action(step).upper()} {step.get('endpoint', '?')}" for step in mutating_steps[:3] ) if len(mutating_steps) > 3: @@ -488,7 +507,11 @@ def _print_dry_run(steps: list[dict], context: Any | None) -> None: # Step references can't resolve at dry-run time — show raw step_to_show = step - action = (step_to_show.get("action") or "get").upper() + action_raw = step_to_show.get("action") + if action_raw is None and "method" in step_to_show: + method_raw = step_to_show.get("method") + action_raw = method_raw.lower() if isinstance(method_raw, str) else None + action = (action_raw or "get").upper() endpoint = step_to_show.get("endpoint", "?") name = step_to_show.get("name", "") data = step_to_show.get("data") @@ -554,7 +577,18 @@ def emit(self, **_: Any) -> None: ... ) continue - action = (step.get("action") or "get").lower() + # Accept the ``method:`` alias for ``action:``. Authors who + # copy-paste OData examples (especially bound-action + # invocations) reach for ``method`` — silently treating that + # as a missing ``action`` and downgrading to GET would let a + # mutating POST step land as a GET round-trip. See + # ``bcli.workflow._models.StepDef`` for the equivalent + # normalisation at schema-validation time. + action_raw = step.get("action") + if action_raw is None and "method" in step: + method_raw = step.get("method") + action_raw = method_raw.lower() if isinstance(method_raw, str) else None + action = (action_raw or "get").lower() endpoint = step.get("endpoint", "") step_name = step.get("name") or endpoint data = step.get("data") diff --git a/tests/test_cli/test_action_cmd.py b/tests/test_cli/test_action_cmd.py new file mode 100644 index 0000000..d16e63c --- /dev/null +++ b/tests/test_cli/test_action_cmd.py @@ -0,0 +1,188 @@ +"""Tests for the ``bcli action`` verb (OData v4 bound action invocation). + +The action verb is a thin sugar layer over ``bcli post``. Internally it +composes the bound-action URL string +``()/.`` and forwards it to the +same client.post path. The verb takes care of: + +* default namespace ``Microsoft.NAV`` (the BC convention; the registry + validator itself remains namespace-agnostic). +* ``--namespace`` override for non-BC tenants. +* ``--data`` (JSON literal or @file) for a body, or no flag at all for + an empty body (matching ``bcli post``). ``--no-data`` is retained as + an explicit no-op alias; ``--data`` and ``--no-data`` may not both be + supplied. +* honouring ``--profile``/``--company``/``--publisher``/``--group``/ + ``--version`` overrides via the standard CLI plumbing. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from unittest.mock import AsyncMock + +import pytest +import typer + +from bcli.config._model import BCConfig, BCDefaults, BCProfile +from bcli_cli._state import state +from bcli_cli.commands import action_cmd + + +@pytest.fixture +def cli_state(): + cfg = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={ + "dev": BCProfile( + tenant_id="t1", + environment="Sandbox", + company_id="c-123", + disable_writes=False, + ), + }, + ) + state._config = cfg + state._registry = None + state.profile_name = None + state.env_override = None + state.company_override = None + state.format = "table" + state.dry_run = False + state.quiet = True + yield state + state._config = None + state._registry = None + state.profile_name = None + state.dry_run = False + + +@pytest.fixture +def fake_client(monkeypatch): + c = AsyncMock() + c.__aenter__ = AsyncMock(return_value=c) + c.__aexit__ = AsyncMock(return_value=False) + c.post = AsyncMock(return_value={"result": "ok"}) + c._resolve_url = lambda entity, **kw: f"https://example.test/{entity}" + monkeypatch.setattr(state, "make_async_client", lambda **_: c) + return c + + +@pytest.fixture(autouse=True) +def non_interactive(monkeypatch): + import sys + monkeypatch.setattr(sys.stdin, "isatty", lambda: False) + + +def _run( + *, + entity="examples", + key="42", + action="archive", + data='{"flag": true}', + no_data=False, + namespace=None, + yes=True, + **kwargs, +): + kwargs.setdefault("format", None) + kwargs.setdefault("publisher", None) + kwargs.setdefault("group", None) + kwargs.setdefault("version", None) + kwargs.setdefault("result_out", None) + kwargs.setdefault("result_fd", None) + kwargs.setdefault("idempotency_key", None) + return action_cmd.action_command( + entity_set=entity, + key=key, + action_name=action, + data=data, + no_data=no_data, + namespace=namespace, + yes=yes, + **kwargs, + ) + + +class TestURLComposition: + def test_default_namespace_is_microsoft_nav(self, cli_state, fake_client): + _run() + endpoint_arg = fake_client.post.await_args.args[0] + assert endpoint_arg == "examples(42)/Microsoft.NAV.archive" + + def test_namespace_override(self, cli_state, fake_client): + _run(namespace="Custom.Ns") + endpoint_arg = fake_client.post.await_args.args[0] + assert endpoint_arg == "examples(42)/Custom.Ns.archive" + + def test_quoted_string_key_passed_through(self, cli_state, fake_client): + _run(key="'ALFKI'") + endpoint_arg = fake_client.post.await_args.args[0] + assert endpoint_arg == "examples('ALFKI')/Microsoft.NAV.archive" + + def test_action_uses_post_not_get(self, cli_state, fake_client): + """OData actions are always POST. The verb must NOT call .get, + even if a future flag suggests otherwise.""" + _run() + assert fake_client.post.await_count == 1 + + +class TestDataHandling: + def test_data_json_literal_parsed(self, cli_state, fake_client): + _run(data='{"a": 1, "b": "two"}') + body_arg = fake_client.post.await_args.args[1] + assert body_arg == {"a": 1, "b": "two"} + + def test_data_from_file(self, cli_state, fake_client, tmp_path: Path): + f = tmp_path / "payload.json" + f.write_text('{"loaded": "from-file"}', encoding="utf-8") + _run(data=f"@{f}") + body_arg = fake_client.post.await_args.args[1] + assert body_arg == {"loaded": "from-file"} + + def test_no_data_flag_sends_empty_body(self, cli_state, fake_client): + """Explicit --no-data sends an empty body.""" + _run(data=None, no_data=True) + body_arg = fake_client.post.await_args.args[1] + assert body_arg == {} + + def test_neither_flag_defaults_to_empty_body(self, cli_state, fake_client): + """Omitting both flags is equivalent to --no-data — matches + ``bcli post`` semantics so AI agents calling + ``bcli action examples 42 archive`` don't get a BadParameter.""" + _run(data=None, no_data=False) + body_arg = fake_client.post.await_args.args[1] + assert body_arg == {} + + def test_data_and_no_data_mutually_exclusive(self, cli_state, fake_client): + with pytest.raises(typer.BadParameter): + _run(data='{"x": 1}', no_data=True) + + +class TestProfileOverrides: + def test_publisher_group_version_forwarded(self, cli_state, fake_client): + _run(publisher="acme", group="custom", version="v1.0") + kwargs = fake_client.post.await_args.kwargs + assert kwargs.get("publisher") == "acme" + assert kwargs.get("group") == "custom" + assert kwargs.get("version") == "v1.0" + + def test_idempotency_key_forwarded(self, cli_state, fake_client): + _run(idempotency_key="k-123") + kwargs = fake_client.post.await_args.kwargs + assert kwargs.get("idempotency_key") == "k-123" + + +class TestEnvelope: + def test_envelope_written_on_success( + self, cli_state, fake_client, tmp_path: Path, + ): + out = tmp_path / "env.json" + _run(result_out=out) + env = json.loads(out.read_text()) + assert env["status"] == "succeeded" + assert env["method"] == "POST" + # Envelope's endpoint field captures the synthetic bound-action + # string so an agent can audit *which* action was invoked. + assert env["endpoint"].endswith("/Microsoft.NAV.archive") diff --git a/tests/test_url/test_bound_action_resolve.py b/tests/test_url/test_bound_action_resolve.py new file mode 100644 index 0000000..25421c6 --- /dev/null +++ b/tests/test_url/test_bound_action_resolve.py @@ -0,0 +1,204 @@ +"""Tests for OData v4 bound-action URL recognition in the registry resolver. + +A URL of the form ``()/.`` is an +OData v4 *bound action invocation* (one example among many: BC's +``Microsoft.NAV.*`` actions on a per-record entity). The registry +validator must: + +1. Recognise the shape and lookup *only the parent entity set* in the + registry — not the whole literal string (which has never been an + entity set name). +2. Still enforce ``disable_standard_api`` based on the parent entity + set's registry status, exactly like a plain ``get`` would. +3. Surface a parent-entity-missing error (not a "whole URL not found" + error) when the parent isn't registered on a locked-down profile. + +The namespace identifier is *agnostic* — these tests deliberately +exercise ``Custom.Ns.doSomething`` alongside ``Microsoft.NAV.archive`` +so any hardcoded namespace check would surface as a failure. +""" + +from __future__ import annotations + +import pytest + +from bcli.client._async import AsyncBCClient +from bcli.config._model import BCConfig, BCDefaults, BCProfile +from bcli.errors import RegistryError +from bcli.registry._schema import EndpointMetadata + + +def _make_client( + *, + disable_standard: bool = False, + custom_endpoints: list[EndpointMetadata] | None = None, + env: str = "Production", + company_id: str = "company-1", +) -> AsyncBCClient: + """Build a client wired to an in-memory profile + registry.""" + cfg = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={ + "dev": BCProfile( + tenant_id="t1", + environment=env, + company_id=company_id, + disable_standard_api=disable_standard, + ), + }, + ) + client = AsyncBCClient(profile="dev", config=cfg) + if custom_endpoints: + for ep in custom_endpoints: + client._registry._custom[ep.entity_set_name.lower()] = ep + return client + + +class TestBoundActionURLRecognition: + """The resolver should pass bound-action URLs through to the server + once the parent entity set is registered (or allowed by the standard + fallback).""" + + def test_bound_action_resolves_against_parent_standard_entity(self): + """``customers()/Microsoft.NAV.archive`` — parent is in the + standard v2.0 registry, so the URL should compose cleanly.""" + client = _make_client(disable_standard=False) + url = client._resolve_url("customers(abc-123)/Microsoft.NAV.archive") + assert url.endswith( + "/companies(company-1)/customers(abc-123)/Microsoft.NAV.archive" + ), url + + def test_bound_action_resolves_against_parent_custom_entity(self): + """Custom-route parent entities should keep their custom route + (publisher/group/version) in front of the bound-action tail.""" + client = _make_client( + custom_endpoints=[ + EndpointMetadata( + entity_set_name="widgets", + api_publisher="acme", + api_group="custom", + api_version="v1.0", + ), + ], + ) + url = client._resolve_url("widgets(42)/Microsoft.NAV.cancel") + assert "/api/acme/custom/v1.0/" in url + assert url.endswith("/widgets(42)/Microsoft.NAV.cancel") + + def test_bound_action_namespace_is_agnostic(self): + """The validator must not hardcode ``Microsoft.NAV`` — any + dotted qualified identifier should pass.""" + client = _make_client() + url = client._resolve_url("customers(abc-123)/Custom.Ns.doSomething") + assert url.endswith("/customers(abc-123)/Custom.Ns.doSomething") + + def test_bound_action_with_quoted_key(self): + """OData v4 string keys are single-quoted; the validator should + not interpret the quotes — just pass them through.""" + client = _make_client() + url = client._resolve_url("customers('ALFKI')/Microsoft.NAV.archive") + assert url.endswith("/customers('ALFKI')/Microsoft.NAV.archive") + + def test_bound_action_with_composite_key(self): + """OData v4 supports ``(k1='a',k2='b')`` composite keys — the + validator should not over-validate the inside of the parens.""" + client = _make_client() + url = client._resolve_url( + "items(k1='a',k2='b')/Microsoft.NAV.doStuff" + ) + assert url.endswith("/items(k1='a',k2='b')/Microsoft.NAV.doStuff") + + def test_bound_action_with_multi_segment_namespace(self): + """Some namespaces are multi-segment (``Foo.Bar.Baz.action``) + — the parse should still accept them, since ``Namespace`` is + explicitly multi-part in the OData v4 spec.""" + client = _make_client() + url = client._resolve_url("widgets(7)/A.B.C.doStuff") + assert url.endswith("/widgets(7)/A.B.C.doStuff") + + +class TestRegistryGateOnBoundActions: + """The ``disable_standard_api`` profile gate must still apply — but + it should fire on the *parent entity name*, not on the full URL. + """ + + def test_disable_standard_api_blocks_unregistered_parent(self): + """Parent entity ``examples`` is not in the standard registry + and not in the custom registry, and the profile has the standard + catalogue disabled → reject.""" + client = _make_client(disable_standard=True) + with pytest.raises(RegistryError) as exc: + client._resolve_url("examples(99)/Microsoft.NAV.archive") + # The error message should name the *parent* entity, not the + # whole bound-action string. This is the actionable signal — an + # operator needs to know which entry to add to the registry. + msg = str(exc.value) + assert "examples" in msg + # Sanity: the verbose composed string shouldn't dominate the + # error message — the parent name is what the operator looks up. + assert "Microsoft.NAV" not in msg or msg.index("examples") < msg.index( + "Microsoft.NAV" + ) + + def test_disable_standard_api_allows_registered_custom_parent(self): + """Parent ``widgets`` is in the custom registry → action passes + through even with the standard catalogue disabled.""" + client = _make_client( + disable_standard=True, + custom_endpoints=[ + EndpointMetadata( + entity_set_name="widgets", + api_publisher="acme", + api_group="custom", + api_version="v1.0", + ), + ], + ) + url = client._resolve_url("widgets(7)/Custom.Ns.archive") + assert url.endswith("/widgets(7)/Custom.Ns.archive") + + +class TestUnboundActionAtServiceRoot: + """An unbound action (``Namespace.action`` with no parent entity) + resolves to ``companies()/.`` under the + chosen API route. Unbound actions aren't entity sets and cannot be + registered — the ``disable_standard_api`` security gate therefore + still applies when no explicit publisher/group/version override is + supplied, mirroring the protection on standard-v2.0 entity sets.""" + + def test_unbound_action_resolves_via_standard_route(self): + client = _make_client(disable_standard=False) + url = client._resolve_url("Microsoft.NAV.refreshAll") + assert url.endswith( + "/companies(company-1)/Microsoft.NAV.refreshAll" + ), url + assert "/api/v2.0/" in url + + def test_unbound_action_resolves_via_custom_route_override(self): + """Explicit publisher/group/version routes the unbound action + under the matching custom API path.""" + client = _make_client(disable_standard=True) + url = client._resolve_url( + "Custom.Ns.refreshAll", + publisher="acme", group="custom", version="v1.0", + ) + assert "/api/acme/custom/v1.0/" in url + assert url.endswith("/companies(company-1)/Custom.Ns.refreshAll") + + def test_unbound_action_blocked_when_disable_standard_api(self): + """A locked-down profile without an explicit custom-route + override must refuse the unbound action — there's nowhere to + route it that doesn't fall through to the suppressed standard + v2.0 surface.""" + client = _make_client(disable_standard=True) + with pytest.raises(RegistryError) as exc: + client._resolve_url("Microsoft.NAV.refreshAll") + assert "disable_standard_api" in str(exc.value) + assert "publisher" in str(exc.value).lower() + + def test_unbound_action_namespace_agnostic(self): + """A non-Microsoft namespace must also route, not be rejected + for ``not yet supported``.""" + client = _make_client(disable_standard=False) + url = client._resolve_url("Custom.Ns.doStuff") + assert url.endswith("/companies(company-1)/Custom.Ns.doStuff") diff --git a/tests/test_workflow/test_method_alias.py b/tests/test_workflow/test_method_alias.py new file mode 100644 index 0000000..2adb8d6 --- /dev/null +++ b/tests/test_workflow/test_method_alias.py @@ -0,0 +1,153 @@ +"""Regression: ``method: POST`` in a YAML batch step is silently +downgraded to GET. + +The workflow schema documents ``action: get|post|patch|delete`` but the +batch runner (``_execute_batch``) reads ``step.get("action") or "get"`` +— a step that writes ``method: POST`` instead of ``action: post`` is +silently treated as a GET because ``action`` is absent. The bug surfaces +when users copy-paste OData bound-action examples (where the conventional +HTTP method key is ``method``) into a bcli workflow file. + +These tests pin the fix: ``method`` is accepted as an alias for +``action`` (lowercased + normalised), and the schema validator rejects +the *combination* of both keys to keep YAML files unambiguous. +""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from bcli.workflow._models import StepDef, WorkflowDef + + +class TestMethodKeyAlias: + def test_method_key_normalises_to_action(self): + """``method: POST`` (no ``action:``) → ``action == 'post'``.""" + step = StepDef(endpoint="examples", method="POST") + assert step.action == "post" + + def test_method_key_lowercased(self): + """Authors write HTTP methods in uppercase (``POST``, ``PATCH``) + out of habit; the model should normalise to the lowercase form + the rest of bcli expects.""" + for raw, expected in [ + ("POST", "post"), + ("Patch", "patch"), + ("delete", "delete"), + ("GET", "get"), + ]: + step = StepDef(endpoint="examples", method=raw) + assert step.action == expected + + def test_unknown_method_rejected(self): + with pytest.raises(ValidationError): + StepDef(endpoint="examples", method="PUT") + + def test_action_and_method_both_set_rejected(self): + """An author who sets both ``action:`` and ``method:`` to the + same value is harmless, but to keep the YAML one-way we reject + the combination — the author should pick one key.""" + with pytest.raises(ValidationError): + StepDef(endpoint="examples", action="post", method="POST") + + def test_action_alone_still_works(self): + """The existing ``action:`` spelling must not regress.""" + step = StepDef(endpoint="examples", action="post") + assert step.action == "post" + + def test_workflow_def_accepts_method_in_step(self): + wf = WorkflowDef( + steps=[{"endpoint": "examples", "method": "POST"}], + ) + assert wf.steps[0].action == "post" + + +class TestBatchRunnerHonoursMethodAlias: + """End-to-end: a YAML step that uses ``method:`` instead of + ``action:`` must produce the right HTTP verb on the client, not a + silent GET.""" + + def test_method_post_calls_client_post( + self, tmp_path, monkeypatch, + ): + from pathlib import Path + from unittest.mock import AsyncMock + + from bcli.config._model import BCConfig, BCDefaults, BCProfile + from bcli_cli._state import state + from bcli_cli.commands.batch_cmd import run_batch + + # Isolate HOME so the ledger lands in tmp_path. + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + + cfg = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={ + "dev": BCProfile( + tenant_id="t1", + environment="Sandbox", + company_id="c-1", + disable_writes=False, + ), + }, + ) + state._config = cfg + state._registry = None + state.profile_name = None + state.dry_run = False + state.quiet = True + + fake = AsyncMock() + fake.__aenter__ = AsyncMock(return_value=fake) + fake.__aexit__ = AsyncMock(return_value=False) + fake.post = AsyncMock(return_value={"id": "x-1"}) + fake.get = AsyncMock(side_effect=AssertionError( + "client.get must NOT be called — method: POST should reach " + "client.post via the action alias." + )) + fake._resolve_url = lambda entity, **kw: f"https://x/{entity}" + + monkeypatch.setattr(state, "make_async_client", lambda **_: fake) + + # Non-interactive: bypass the --yes prompt. + import sys + monkeypatch.setattr(sys.stdin, "isatty", lambda: False) + + yaml_file = tmp_path / "workflow.yaml" + yaml_file.write_text( + "name: method-alias\n" + "steps:\n" + " - name: archive_one\n" + " method: POST\n" + " endpoint: examples\n" + " data: {flag: true}\n", + encoding="utf-8", + ) + + try: + run_batch( + file=yaml_file, + dry_run=False, + output=None, + format=None, + set_params=None, + params_file=None, + yes=True, + result_out=None, + result_fd=None, + progress_fd=None, + ) + except SystemExit: + pass + finally: + state._config = None + state._registry = None + + # The fix: client.post is what should have been called. The + # AsyncMock raises in client.get if the runner downgraded. + assert fake.post.await_count == 1, ( + f"Expected client.post to be called once, " + f"got post={fake.post.await_count}, get={fake.get.await_count}" + ) diff --git a/tests/test_workflow/test_rollback_url_bound_action.py b/tests/test_workflow/test_rollback_url_bound_action.py new file mode 100644 index 0000000..d2f6158 --- /dev/null +++ b/tests/test_workflow/test_rollback_url_bound_action.py @@ -0,0 +1,61 @@ +"""Regression test for ``_compose_rollback_url`` and bound-action endpoints. + +When the batch runner records the outcome of a POST step, it tries to +compose a rollback URL (``DELETE ()``) so a subsequent +``bcli batch rollback`` can undo the create. Bound-action endpoints +have no such inverse — there is no "DELETE the archiving" of +``examples(42)/Microsoft.NAV.archive``. If the action happened to +return a body containing an ``id`` (unusual but legal for OData +actions), naïve URL composition would yield a nonsensical path like +``examples(42)/Microsoft.NAV.archive(new-id-789)``. + +The composer must detect bound-action endpoints and return ``None``, +which causes the ledger to record ``rollback_skipped`` instead. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from bcli_cli.commands.batch_cmd import _compose_rollback_url + + +def test_rollback_url_is_none_for_bound_action_with_id_in_result(): + """Even if a bound action returns ``{"id": ...}``, no rollback URL + should be composed — the action has no inverse.""" + client = MagicMock() + client._resolve_url.return_value = ( + "https://api.example.test/api/v2.0/companies(c-1)/" + "examples(42)/Microsoft.NAV.archive" + ) + post_result = {"id": "new-id-789"} + rb_url = _compose_rollback_url( + client, "examples(42)/Microsoft.NAV.archive", post_result, + ) + assert rb_url is None + # And the early guard means we never even reached the resolver. + client._resolve_url.assert_not_called() + + +def test_rollback_url_is_none_for_unbound_action_with_id_in_result(): + """Same guard applies to unbound (service-root) actions.""" + client = MagicMock() + rb_url = _compose_rollback_url( + client, "Microsoft.NAV.refreshAll", {"id": "irrelevant"}, + ) + assert rb_url is None + client._resolve_url.assert_not_called() + + +def test_rollback_url_normal_post_unchanged(): + """A plain entity-set POST still gets a composed rollback URL.""" + client = MagicMock() + client._resolve_url.return_value = ( + "https://api.example.test/api/v2.0/companies(c-1)/examples" + ) + rb_url = _compose_rollback_url( + client, "examples", {"id": "new-id-789"}, + ) + assert rb_url == ( + "https://api.example.test/api/v2.0/companies(c-1)/examples(new-id-789)" + ) diff --git a/uv.lock b/uv.lock index 22160c6..7d9389e 100644 --- a/uv.lock +++ b/uv.lock @@ -321,7 +321,7 @@ wheels = [ [[package]] name = "bc-cli" -version = "0.3.0" +version = "0.4.0" source = { editable = "." } dependencies = [ { name = "httpx" },