serve: mirror describe-batch + unload-table, expose job idempotency (#657) - #683
Draft
padak wants to merge 1 commit into
Draft
serve: mirror describe-batch + unload-table, expose job idempotency (#657)#683padak wants to merge 1 commit into
padak wants to merge 1 commit into
Conversation
…ency (#657) Three MEDIUM gaps from the 0.89.0 serve audit. Each mirror deliberately differs from its CLI command where the CLI option names the CALLER's filesystem -- which, over serve, is the server's. - `POST /storage/describe-batch/{project}`: the whole bulk-documentation path was CLI-only. Sections travel inline in the body instead of a `--from-file` path. `parse_describe_batch_file` is split into a shared `parse_describe_batch_document`, and the write loop into `_apply_describe_batch`, so the two surfaces cannot drift into accepting different documents -- the half that drifted would be the one letting a malformed section reach the write loop. The sections are typed `Any` on purpose: declaring them `dict[str, str]` would have pydantic reject a wrong shape first with its generic 422, discarding #645's messages (offending key, actual type, copy-pasteable example). - `POST /storage/tables/{project}/{table_id}/unload`: no `--download` / `--output`; the response carries the Storage `file_id` and `GET /storage/files/{p}/{file_id}/download` fetches the bytes. That is also the only shape that works for parquet, whose export is sliced. - `POST /jobs/{p}/run` now accepts `idempotency_key` / `force_rerun`. `JobService.run_job` has taken both since #427, and retrying a POST is the canonical case that store was built for, so the mirror omitting them left the caller who needs them most without them. Omitting both produces a byte-identical call to the pre-change one. The issue's bonus ask, `GET /permissions/show`, is in the #655 PR instead -- it belongs with the enforcement it describes. Tests: 20 new in tests/test_server_missing_mirrors.py, including an exact-kwargs assertion that `unload` never passes `output_path`, a greedy-`{table_id:path}` case proving `/unload` is not eaten as part of a dotted table id, and a parametrised check that the shared validator rejects and accepts the same documents on both paths. Fixes #657
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
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.
Fixes #657.
Three MEDIUM gaps from the 0.89.0 serve audit. Each mirror deliberately differs from its CLI command in one place — wherever the CLI option names the caller's filesystem, which over
serveis the server's.1.
storage describe-batchhad no route at allThe whole bulk-documentation path was CLI-only.
POST /storage/describe-batch/{project}takes thebuckets/tables/columnssections inline in the body (plusbranch_id) instead of a--from-filepath.The issue noted the validation core was already factored out. It was factored out of the file reader, not of the write loop, so this splits both:
parse_describe_batch_file→parse_describe_batch_document(raw, source_label)+ a thin YAML-loading wrapper.describe_batch→_apply_describe_batch(alias, parsed, ...), shared with the newdescribe_batch_document.Both surfaces therefore run literally the same rules and the same write loop. Two copies would drift, and the half that drifted would be the one letting a malformed section reach the write loop.
One typing choice worth a reviewer's eye: the body's three sections are declared
Any, notdict[str, str]. The stricter annotation looks better and is worse — pydantic would reject a wrong shape first with its generic 422, discarding exactly what #645 built: the offending key, its actual type, and a copy-pasteable example. Typing it loose is what lets the shared validator be the one that speaks.Error-shape note: a malformed document answers 422 (
HTTP_ERROR) where the CLI exits 2 (INVALID_ARGUMENT) — same message, different code. It is a malformed request body, which is the class of problem FastAPI already answers 422 for on this surface, and the alternatives were worse: a globalValueErrorhandler would turn genuine bugs into 400s, andKeboolaApiErrormaps to 502. Documented indocs/web-server.mdandgotchas.mdso a caller branches on the status here rather than the code. Unifying validation error codes across the two surfaces is a broader cleanup, not this PR.2.
storage unload-tablehad no mirrorPOST /storage/tables/{project}/{table_id}/unload, withdownloadhard-wired False and nooutput_path: writing to "the caller's disk" means writing to the server's. The response carries the Storagefile_id, andGET /storage/files/{project}/{file_id}/downloadfetches the bytes.That is also the only shape that works for
file_type: "parquet", whose export is sliced and has no single-file download even on the CLI.The body is optional — an unload with no options is the common case, and requiring
{}would be noise.3.
POST /jobs/{p}/rundropped client-side idempotencyJobService.run_jobhas takenidempotency_key/force_rerunsince #427, and retrying a POST over HTTP is the canonical case that store was built for — so the REST mirror omitting them left the caller who needs them most without them. Both are now body fields. Omitting them produces a byte-identical call to the pre-change one (a test asserts exactly that).Dedup is scoped to the served config dir, i.e. per machine, same as on the CLI.
The bonus ask went elsewhere
GET /permissions/showis in #682 (issue #655) rather than here: a read-only view of the policy belongs with the enforcement it describes, not on its own.Testing
make checkgreen: 6137 passed, 12 skipped.tyclean (the one remaining diagnostic is the pre-existingscripts/hatch_build.pyunresolved import). Lint, format, command-sync, version-gates, sentinel-guards, error-codes and the endpoint-reference gate all pass;docs/web-server-endpoints.mdregenerated and committed.20 new tests in
tests/test_server_missing_mirrors.py, service layer mocked — what is under test is the router → service contract, not Keboola behaviour:Nonerather than becoming an invented{}; a shape error is 422 carrying the CLI's message; a per-item API failure is a 200 witherrors, not a 422 (shape is a usage error, an API refusal is not).output_pathfails rather than quietly writing to the server's disk; body-optional; all options threaded; and a greedy-{table_id:path}case proving/unloadis not swallowed as part ofout.c-my.deeply.dotted.run_job; omitting them preserves the pre-change call.source_labelnames the body rather than a filename.Merge-order note
#682 (issue #655, open) adds an app-wide permission firewall whose completeness test requires every route to have a
ROUTE_OPERATIONSentry. Whichever of the two lands second needs three entries added:(
POST /jobs/{p}/runis already mapped — this PR only adds fields to its body.) Both operations already exist inOPERATION_REGISTRY. The completeness test names each missing route, so the failure is self-explaining rather than mysterious.No version bump, no changelog entry (per CONTRIBUTING: those belong to the release PR). New behaviour gated with the literal
(since vNEXT)placeholder.Doc surfaces
docs/web-server.md(new "CLI options that a REST mirror deliberately drops" table, plus the stale "mirrors still considered missing" sentence),docs/web-server-endpoints.md(regenerated), andgotchas.md. No CLI command added, renamed, or removed, soCLAUDE.md's command list,context.py,keboola-expert.mdandSKILL.mdneed no change.