diff --git a/CHANGELOG.md b/CHANGELOG.md index 668ef15..66a8ffc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 genuinely part of a key. The same validation applies to the key inside a bound-action invocation, whose resolver checks the registry for the parent entity set only. +- `--dry-run` no longer reports success for a request the real command refuses. + `try_resolve_url` never raises by design, so an invalid `record_id` was + swallowed to `resolved_url: null` and the preview still rendered and exited 0 + while the real run exited 1 — the opposite of what a preview is for, and its + documented consumers are agents parsing that envelope to decide whether to + proceed. It now takes `strict=True` from the dry-run path, which re-raises + invalid *input* while still swallowing *incidental* resolution failures (a + registry miss keeps previewing with a null URL, as documented). `BCLIError` + subclasses don't inherit `ValueError`, which is what makes that split clean. + The failure is presented as the same `Error:` line the real run prints, not a + traceback. +- An **empty** `record_id` is now an error instead of silently addressing the + collection. `None` still means "operate on the entity set" (`bcli get + ` with no id is a collection read), but `""` previously took the same + path — so a caller that meant one record and supplied nothing got a request + against the whole set. `delete` and `patch` take `record_id` as a required + positional, so `bcli delete ""` composed a DELETE against the entity + set rather than a row. - `test_no_context_policy_path` no longer asserts against the developer's real `~/.config/bcli`, so it passes on a machine that has recorded a bcli error rather than only on a clean CI home. diff --git a/src/bcli/_url.py b/src/bcli/_url.py index f4d4567..0df07b4 100644 --- a/src/bcli/_url.py +++ b/src/bcli/_url.py @@ -87,7 +87,13 @@ def build_url( https://api.businesscentral.dynamics.com/v2.0/{env}/api/{pub}/{grp}/{ver}/companies({id})/{entity} """ validate_record_key("entity_set_name", entity_set_name) - if record_id: + # `None` means "address the collection" and is a supported call — `bcli get + # ` with no id reads the set. An *empty* key is not the same thing: + # it means the caller meant one record and supplied nothing, and silently + # dropping it would retarget the request at the whole collection (a DELETE + # or PATCH against an entity set rather than a row). Validate anything that + # was actually passed, including "". + if record_id is not None: validate_record_key("record_id", record_id) if publisher and group and version: @@ -100,7 +106,7 @@ def build_url( url = f"{BC_BASE_URL}/{environment}/{api_path}/companies({company_id})/{entity_set_name}" - if record_id: + if record_id is not None: url = f"{url}({record_id})" return url diff --git a/src/bcli_cli/_dry_run.py b/src/bcli_cli/_dry_run.py index 86b6de4..3ca97f5 100644 --- a/src/bcli_cli/_dry_run.py +++ b/src/bcli_cli/_dry_run.py @@ -57,14 +57,29 @@ def render_dry_run( profile = state.profile profile_name = state.active_profile_name - resolved_url = try_resolve_url( - endpoint, - record_id=record_id, - publisher=publisher, - group=group, - version=version, - force_standard=force_standard, - ) + # strict=True: a preview must not report success for a request the real + # command would refuse. An unresolvable endpoint still previews with a null + # URL (that is the documented behaviour); invalid input is fatal, so + # `--dry-run` fails exactly where the real run fails. + # + # Presented here rather than propagated, for two reasons: the dry-run branch + # in each write command sits above that command's own try/except, so a raw + # raise surfaces as a traceback instead of the `Error: ...` line the real run + # prints; and doing it in one place keeps post/patch/delete/attach/batch + # consistent without five edits. + try: + resolved_url = try_resolve_url( + endpoint, + record_id=record_id, + publisher=publisher, + group=group, + version=version, + force_standard=force_standard, + strict=True, + ) + except ValueError as exc: + _console.print(f"[red]Error:[/red] {exc}") + raise typer.Exit(1) from exc payload: dict[str, Any] = { "dry_run": True, diff --git a/src/bcli_cli/_url_resolve.py b/src/bcli_cli/_url_resolve.py index 48ba6e2..4d640eb 100644 --- a/src/bcli_cli/_url_resolve.py +++ b/src/bcli_cli/_url_resolve.py @@ -26,6 +26,7 @@ def try_resolve_url( group: str | None = None, version: str | None = None, force_standard: bool = False, + strict: bool = False, ) -> str | None: """Resolve ``endpoint`` to a full URL using the active profile. @@ -33,6 +34,18 @@ def try_resolve_url( ``disable_standard_api``, missing company id, malformed profile, etc.). Callers should treat ``None`` as "preview the rest, the user will see the gap and correct". + + ``strict=True`` keeps that for *incidental* failures but re-raises invalid + **input**. The two are not the same thing, and conflating them made + ``--dry-run`` report success for a request the real command refuses: an empty + or path-bearing ``record_id`` raises ``ValueError`` in the URL builder, this + helper swallowed it to ``None``, and the preview rendered a clean DELETE and + exited 0 while the real run exited 1. A preview whose entire job is to + predict must not disagree with what it predicts. + + ``BCLIError`` subclasses (``RegistryError``, ``ConfigError``) are the + incidental kind and stay swallowed even under ``strict`` — they do not + inherit ``ValueError``, which is what makes the split clean. """ try: if force_standard: @@ -53,5 +66,12 @@ def try_resolve_url( group=group, version=version, ) + except ValueError: + # Invalid input, not an incidental resolution failure. The real request + # validates the same value and fails, so a strict caller (the dry-run + # preview) must fail too rather than reporting a request it cannot make. + if strict: + raise + return None except Exception: return None diff --git a/tests/test_cli/test_dry_run.py b/tests/test_cli/test_dry_run.py index 903733d..98f9b4c 100644 --- a/tests/test_cli/test_dry_run.py +++ b/tests/test_cli/test_dry_run.py @@ -225,3 +225,110 @@ def test_exits_clean_for_human_format_too(self, configured_state): with pytest.raises(typer.Exit) as excinfo: render_dry_run("DELETE", "items", record_id="x") assert excinfo.value.exit_code == 0 + + +@pytest.fixture +def real_resolver_state(): + """Like ``configured_state`` but WITHOUT stubbing ``make_async_client``. + + ``configured_state``'s ``_StubClient._resolve_url`` ignores ``record_id`` + entirely and always returns a URL, which is fine for testing the renderer's + output shape — but it means the existing dry-run suite never exercises real + URL resolution. That is exactly why an empty ``record_id`` could render a + clean preview: nothing here ever built a real URL. These tests use the real + resolver. + """ + 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.format = "table" + yield + state._config = None + state._registry = None + state.format = "table" + + +class TestDryRunMustNotSucceedWhereTheRealRunFails: + """A preview that reports success for input the real request refuses is worse + than no preview: ``--dry-run`` exists so an agent can decide whether to + proceed, and its documented consumers parse the JSON envelope. + + ``try_resolve_url`` deliberately never raises, so a resolution failure + records ``resolved_url: null`` and the preview continues. That is right for + an incidental failure (registry miss, no company id) but wrong for invalid + *input*, because the real command validates the same value and exits 1. An + empty ``record_id`` used to render a clean DELETE preview and exit 0 while + ``bcli delete ""`` exited 1. + """ + + @pytest.mark.parametrize("empty", ["", " "]) + def test_empty_record_id_fails_the_preview(self, real_resolver_state, empty): + state.format = "json" + with pytest.raises(typer.Exit) as excinfo: + render_dry_run("DELETE", "items", record_id=empty) + assert excinfo.value.exit_code == 1 + + def test_traversing_record_id_fails_the_preview(self, real_resolver_state): + state.format = "json" + with pytest.raises(typer.Exit) as excinfo: + render_dry_run("DELETE", "items", record_id="1)/../../glEntries('X'") + assert excinfo.value.exit_code == 1 + + def test_the_failure_is_reported_not_traced(self, real_resolver_state, capsys): + """The dry-run branch sits above each command's own try/except, so a raw + raise would surface as a traceback rather than the ``Error:`` line the + real run prints.""" + state.format = "json" + with pytest.raises(typer.Exit): + render_dry_run("DELETE", "items", record_id="") + assert "must not be empty" in capsys.readouterr().err + + def test_none_record_id_still_previews_cleanly(self, real_resolver_state): + """A collection-targeted write is a real thing; don't break it.""" + state.format = "json" + with pytest.raises(typer.Exit) as excinfo: + render_dry_run("POST", "items", body={"x": 1}, record_id=None) + assert excinfo.value.exit_code == 0 + + def test_a_valid_key_still_previews_cleanly(self, real_resolver_state): + state.format = "json" + with pytest.raises(typer.Exit) as excinfo: + render_dry_run("DELETE", "items", record_id="'V00010'") + assert excinfo.value.exit_code == 0 + + +class TestTryResolveUrlStrictMode: + def test_strict_re_raises_input_validation(self, real_resolver_state): + from bcli_cli._url_resolve import try_resolve_url + + with pytest.raises(ValueError, match="must not be empty"): + try_resolve_url("items", record_id="", strict=True) + + def test_non_strict_still_swallows_input_validation(self, real_resolver_state): + """The audit path must never break a command that already ran.""" + from bcli_cli._url_resolve import try_resolve_url + + assert try_resolve_url("items", record_id="") is None + + def test_strict_still_swallows_incidental_failures(self, real_resolver_state): + """A registry miss is not the caller's input being wrong, so even strict + mode returns None — both the preview and audit paths want that. This + profile has disable_standard_api unset, so an unknown entity falls + through to the standard route and resolves; force the registry gate on + to get a genuine incidental failure.""" + from bcli_cli._url_resolve import try_resolve_url + + state.profile.disable_standard_api = True + state._registry = None + assert try_resolve_url("definitelyNotAnEndpoint", record_id="x", strict=True) is None diff --git a/tests/test_url/test_record_key_validation.py b/tests/test_url/test_record_key_validation.py index b3f79f7..ab2c628 100644 --- a/tests/test_url/test_record_key_validation.py +++ b/tests/test_url/test_record_key_validation.py @@ -90,6 +90,35 @@ def test_none_record_id_is_unchanged(self): assert url.endswith("engineOverviews") +class TestEmptyRecordIdIsNotTheSameAsNone: + """``None`` means "operate on the collection" and is legitimate — ``bcli get + `` with no id is a collection read. An *empty* key is different: it + means the caller meant to address one record and supplied nothing. + + The first version of this validation guarded with ``if record_id:``, so a + falsy key skipped validation *and* skipped appending the key — silently + turning a single-record operation into a collection one. ``delete`` and + ``patch`` take ``record_id`` as a required positional with no default, so + ``bcli delete engineOverviews ""`` composed a DELETE against the whole + entity set. Whether BC would honour that is not the point; the client must + not build it. + """ + + @pytest.mark.parametrize("empty", ["", " ", "\t", "\n"]) + def test_empty_record_id_is_rejected(self, empty: str): + with pytest.raises(ValueError, match="must not be empty"): + _build(entity_set_name="engineOverviews", record_id=empty) + + def test_empty_record_id_does_not_silently_become_a_collection_url(self): + with pytest.raises(ValueError): + _build(entity_set_name="engineOverviews", record_id="") + + def test_none_still_means_collection(self): + url = _build(entity_set_name="engineOverviews", record_id=None) + assert url.endswith("engineOverviews") + assert "(" not in url.rsplit("/", 1)[-1] + + class TestEntitySetNameIsValidatedToo: @pytest.mark.parametrize("bad", ["a/b", "..", ".", "a\\b"]) def test_rejects_path_syntax(self, bad: str):