An empty record_id must be an error, not a collection request - #28
Merged
Conversation
…equest The validation added in the previous commit guarded with `if record_id:`, so a falsy key skipped validation *and* skipped being appended — silently retargeting a single-record operation at the whole entity set. `None` legitimately means "address the collection": `bcli get <entity>` with no id is a collection read, and that is unchanged. But `""` took the same path, and `delete`/`patch` declare `record_id` as a required positional with no default, so `bcli delete <entity> ""` built a DELETE against the entity set instead of a row. Whether BC would honour that is beside the point — the client should not construct it. Both the validation and the append now key on `is not None`, so anything actually passed is validated, including the empty string.
`--dry-run` reported success for an empty or path-bearing record_id and exited 0, while the same command without the flag exited 1. A preview whose entire job is to predict must not disagree with what it predicts, and the documented consumers of the JSON envelope are agents deciding whether to proceed — so this was a false green. The cause was `try_resolve_url`'s bare `except Exception: return None`. Never raising is right for the audit path (a broken URL build shouldn't retroactively fail a command that already ran) and right for incidental resolution failures (a registry miss records a null URL and lets the preview through, as documented). It is wrong for invalid input, which the real request validates and rejects. `strict=True`, used only by the dry-run renderer, re-raises ValueError while still swallowing BCLIError subclasses. Those don't inherit ValueError, which is what makes the split clean rather than a guess. The renderer catches it and prints the same `Error:` line the real run prints, because each write command's dry-run branch sits above its own try/except, so a raw raise surfaced as a traceback. Tests needed a fixture without the existing `_StubClient`, whose `_resolve_url` ignores record_id and always returns a URL. That stub is why this survived: the dry-run suite never built a real URL, so nothing here could have caught it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Follow-up to #27. The key validation added there guarded with
if record_id:, so afalsy key skipped validation and skipped being appended — silently retargeting a
single-record operation at the whole entity set. Found by a stop-time review gate after
#27 had already merged, so 0.7.0 on
maincurrently has it.Nonelegitimately means "address the collection" —bcli get <entity>with no id isa collection read, and that behaviour is unchanged.
""took the same path, anddelete/patchdeclarerecord_idas a required positional with no default:built
DELETE .../engineOverviewsrather thanDELETE .../engineOverviews(id).Whether BC honours a collection DELETE is beside the point — the client should not
construct it.
Changes
Both the validation and the append now key on
is not None, so anything actuallypassed gets validated, including the empty string. Nothing in the tree passed a literal
record_id="", andget_cmdalready usesOptional[str] = typer.Argument(None)forcollection reads, so no caller changes behaviour.
Test plan
uv run pytest— 1066 passed, 5 skipped (+6 over Embeddable auth and saved queries, plus stricter URL key validation (0.7.0) #27)uv run ruff check src tests— cleanrecord_id=""and whitespace-only now raise "must not be empty";record_id=Nonestill produces the collection URL with no parensmainpost-merge, so the suite is green against the released 0.7.0 treeNote on release
Worth landing this before 0.7.0 goes to PyPI, so the published wheel doesn't carry the
empty-id path.