fix(analytics): declare real error codes on unified-metric routes - #2134
Merged
Conversation
Contributor
|
📝 WalkthroughWalkthroughAnalytics routes now declare endpoint-specific OpenAPI responses. The committed specification adds metric and column operations, documents service-unavailable responses for admin threshold writes, and removes unsupported responses. E2E coverage rules and documentation now reflect the reduced exclusions. ChangesAnalytics API contract alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/components/backend/analytics/openapi.json (1)
2925-2965: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDeclare the
tablepath parameter.
/v1/columns/{table}has noparametersdeclaration. OpenAPI requires thetabletemplate parameter to be declared as a required path parameter. Generated clients cannot construct this operation reliably.Add the parameter through the Rust operation registration and regenerate this document.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/components/backend/analytics/openapi.json` around lines 2925 - 2965, Update the Rust registration for the columns list operation, identified by operationId analytics_api.columns.list_for_table, to declare table as a required path parameter, then regenerate the OpenAPI document so /v1/columns/{table} includes the corresponding parameters entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/domain/bronze-to-api-e2e/specs/DESIGN.md`:
- Line 423: The coverage-gate contract in the DESIGN.md description is stale:
update the verdict formula to subtract only UNIVERSAL_BOILERPLATE{429}, remove
401 from the universal exclusion because authentication is enabled, and state
that uncovered per-status codes are reported without causing a non-zero exit.
Preserve the existing descriptions of server-fault handling, BLOCKED entries,
and other gate failures.
In `@src/ingestion/tests/e2e/api/test_metric_results.py`:
- Line 67: Rename the affected test in the metric results API tests to
explicitly state that the invalid request returns 400 rather than 404, and
remove its explanatory docstring/comment. Preserve the test’s existing
assertions and behavior.
In `@src/ingestion/tests/e2e/lib/api_coverage.py`:
- Around line 41-63: The comments above UNIVERSAL_BOILERPLATE and BLOCKED,
including the per-route exclusion comments, are overly detailed and span
multiple lines. Reduce each exclusion’s comment to a single concise line stating
only its reason, while preserving the existing constants, route mappings, and
issue references; move broader policy context elsewhere if needed.
---
Outside diff comments:
In `@docs/components/backend/analytics/openapi.json`:
- Around line 2925-2965: Update the Rust registration for the columns list
operation, identified by operationId analytics_api.columns.list_for_table, to
declare table as a required path parameter, then regenerate the OpenAPI document
so /v1/columns/{table} includes the corresponding parameters entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ba917d1-dff4-459f-9110-f0895f08b60b
📥 Commits
Reviewing files that changed from the base of the PR and between d8c6888 and 198b98b85452f22c4998c3e64fe6abbf6723b3aa.
📒 Files selected for processing (6)
docs/components/backend/analytics/openapi.jsondocs/domain/bronze-to-api-e2e/specs/DESIGN.mdsrc/backend/services/analytics/src/api/mod.rssrc/ingestion/tests/e2e/README.mdsrc/ingestion/tests/e2e/api/test_metric_results.pysrc/ingestion/tests/e2e/lib/api_coverage.py
| @@ -420,7 +420,7 @@ Reachability alone does not prove the suite tests an operation's contract — a | |||
|
|
|||
| ##### Responsibility scope | |||
|
|
|||
| Attaches `record_response` as an httpx `response` event-hook on the single client `api-client` returns, recording `(method, path) -> {status codes}` into an in-process ledger (reading only metadata off the already-received response, never the body); dumps and merges that ledger to `.artifacts/observed_endpoints.json` at `pytest_sessionfinish`. As a gate (`python3 lib/api_coverage.py`, stdlib only), it loads the ledger plus the committed OpenAPI spec (`docs/components/backend/analytics/openapi.json` — the coverage universe), matches observed requests onto spec operations by path-template arity, and builds a `CoverageReport`. The verdict is per status code: for each operation, `required = declared - {codes >= 500} - UNIVERSAL_BOILERPLATE{401,429} - BLOCKED[op]`, and the operation passes iff `required` is a subset of the codes actually observed. Server-fault codes (>= 500) are declared for spec fidelity but never required. `UNIVERSAL_BOILERPLATE` (401/429) is subtracted on every route (gateway auth disabled, no rate limiter). `BLOCKED` is a per-operation set of declared codes the black-box rig provably cannot observe — either because the committed spec's `.standard_errors` boilerplate over-declares codes a route cannot answer (spec-fidelity bug, #1669), or because of a pinned product bug (#1663, #1664). Exits non-zero — blocking `./e2e.sh gates` and the `api-endpoint-coverage-gate` CI job — on any missing operation, uncovered required code, or stale/redundant `SKIP_LIST` / `BLOCKED` entry (an excluded code that is now observed, or a `BLOCKED` entry no longer in the spec, fails the gate so the exclusion list stays honest). | |||
| Attaches `record_response` as an httpx `response` event-hook on the single client `api-client` returns, recording `(method, path) -> {status codes}` into an in-process ledger (reading only metadata off the already-received response, never the body); dumps and merges that ledger to `.artifacts/observed_endpoints.json` at `pytest_sessionfinish`. As a gate (`python3 lib/api_coverage.py`, stdlib only), it loads the ledger plus the committed OpenAPI spec (`docs/components/backend/analytics/openapi.json` — the coverage universe), matches observed requests onto spec operations by path-template arity, and builds a `CoverageReport`. The verdict is per status code: for each operation, `required = declared - {codes >= 500} - UNIVERSAL_BOILERPLATE{401,429} - BLOCKED[op]`, and the operation passes iff `required` is a subset of the codes actually observed. Server-fault codes (>= 500) are declared for spec fidelity but never required. `UNIVERSAL_BOILERPLATE` (401/429) is subtracted on every route (gateway auth disabled, no rate limiter). `BLOCKED` is a per-operation set of declared codes the black-box rig provably cannot observe, each pinning a product bug where the handler answers something other than the code the route declares (#1663, #1664). Exits non-zero — blocking `./e2e.sh gates` and the `api-endpoint-coverage-gate` CI job — on any missing operation, uncovered required code, or stale/redundant `SKIP_LIST` / `BLOCKED` entry (an excluded code that is now observed, or a `BLOCKED` entry no longer in the spec, fails the gate so the exclusion list stays honest). | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the coverage-gate contract.
This text is stale. UNIVERSAL_BOILERPLATE now contains only 429. The rig runs with authentication enabled, so 401 is coverable. Uncovered per-status codes are reported and do not cause a non-zero exit.
Update the formula and gate behavior description to match lib/api_coverage.py.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/domain/bronze-to-api-e2e/specs/DESIGN.md` at line 423, The coverage-gate
contract in the DESIGN.md description is stale: update the verdict formula to
subtract only UNIVERSAL_BOILERPLATE{429}, remove 401 from the universal
exclusion because authentication is enabled, and state that uncovered per-status
codes are reported without causing a non-zero exit. Preserve the existing
descriptions of server-fault handling, BLOCKED entries, and other gate failures.
| @@ -64,7 +64,7 @@ def test_metric_results_400_reversed_period(api) -> None: | |||
| def test_metric_results_400_unknown_metric_key(api) -> None: | |||
| """An unknown `metric_key` is resolved against the catalog and rejected as a | |||
| 400 (`unavailable`) — NOT a 404. Pins that the compute endpoint has no | |||
| not-found path (the spec's declared 404 is `.standard_errors` boilerplate).""" | |||
| not-found path, which is why the spec declares no 404 for it.""" | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Put the behavioral rule in the test name.
The test guideline requires non-obvious behavior in the test name instead of a comment. Rename the test to state the 400-versus-404 rule and remove the docstring.
Proposed fix
-def test_metric_results_400_unknown_metric_key(api) -> None:
- """An unknown `metric_key` is resolved against the catalog and rejected as a
- 400 (`unavailable`) — NOT a 404. Pins that the compute endpoint has no
- not-found path, which is why the spec declares no 404 for it."""
+def test_metric_results_unknown_metric_key_returns_400_not_404(api) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| not-found path, which is why the spec declares no 404 for it.""" | |
| def test_metric_results_unknown_metric_key_returns_400_not_404(api) -> None: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tests/e2e/api/test_metric_results.py` at line 67, Rename the
affected test in the metric results API tests to explicitly state that the
invalid request returns 400 rather than 404, and remove its explanatory
docstring/comment. Preserve the test’s existing assertions and behavior.
Source: Coding guidelines
Comment on lines
+41
to
+63
| # The gate subtracts the codes a route provably cannot produce, or it would | ||
| # require statuses the API never returns. UNIVERSAL_BOILERPLATE drops from every | ||
| # route: 429, which only `POST /v1/metric-drilldown` declares (its own | ||
| # concurrency cap) and which no black-box test induces deterministically. | ||
| # 401 is REAL — the rig runs auth-ENABLED (the gears host's oidc-authn-plugin | ||
| # verifies the gateway JWT), so every route answers 401 to an anonymous call | ||
| # (api/test_unauthorized.py). | ||
| UNIVERSAL_BOILERPLATE = frozenset({429}) | ||
|
|
||
| # Per-route declared codes the rig cannot observe, subtracted from `required` on | ||
| # top of UNIVERSAL_BOILERPLATE — tagged per entry: `.standard_errors` boilerplate | ||
| # the handler can't answer (#1669), or a pinned rig/product bug (#1663 legacy- | ||
| # threshold reads 500; #1664 admin duplicate-create 500s not 409). Self-cleaning: | ||
| # an entry that becomes observed or leaves the spec fails the hygiene advisory. | ||
| # top of UNIVERSAL_BOILERPLATE. Every entry pins a product bug: the route | ||
| # declares the code it is contracted to answer, and the handler answers | ||
| # something else. Self-cleaning: an entry that becomes observed or leaves the | ||
| # spec fails the hygiene advisory. | ||
| BLOCKED: dict[str, frozenset[int]] = { | ||
| "GET /v1/metrics": frozenset({400, 403, 404, 409}), # boilerplate: list, no input/lookup/conflict | ||
| "POST /v1/metrics": frozenset({403, 404, 409}), # boilerplate | ||
| "GET /v1/metrics/{id}": frozenset({403, 409}), # boilerplate | ||
| "PUT /v1/metrics/{id}": frozenset({403, 409}), # boilerplate | ||
| "DELETE /v1/metrics/{id}": frozenset({403, 409}), # boilerplate | ||
| "POST /v1/metrics/{id}/query": frozenset({403, 409}), # boilerplate | ||
| "POST /v1/metrics/queries": frozenset({403, 404, 409}), # boilerplate (per-item errors embed in 200) | ||
| "GET /v1/columns": frozenset({400, 403, 404, 409}), # boilerplate | ||
| "GET /v1/columns/{table}": frozenset({400, 403, 404, 409}), # boilerplate: unknown table → empty 200 | ||
| "POST /v1/catalog/get_metrics": frozenset({403, 404, 409}), # boilerplate | ||
| "GET /v1/admin/metric-thresholds": frozenset({403, 404, 409}), # boilerplate | ||
| "GET /v1/admin/metric-thresholds/{id}": frozenset({403, 409}), # boilerplate | ||
| "PUT /v1/admin/metric-thresholds/{id}": frozenset({409}), # boilerplate (403 IS reachable: cross-tenant) | ||
| "DELETE /v1/admin/metric-thresholds/{id}": frozenset({409}), # boilerplate (403 reachable: cross-tenant) | ||
| # legacy thresholds: 403/409 boilerplate; the success code is #1663 (500 on read-back) | ||
| "GET /v1/metrics/{id}/thresholds": frozenset({200, 403, 409}), # 200=#1663 | ||
| "POST /v1/metrics/{id}/thresholds": frozenset({201, 403, 409}), # 201=#1663 | ||
| "PUT /v1/metrics/{id}/thresholds/{tid}": frozenset({200, 403, 409}), # 200=#1663 | ||
| "DELETE /v1/metrics/{id}/thresholds/{tid}": frozenset({204, 403, 409}), # 204=#1663 | ||
| "POST /v1/admin/metric-thresholds": frozenset({404, 409}), # 404 boilerplate; 409=#1664 | ||
| # persons 200/404 covered via the in-process Identity stub (#1691); rest boilerplate | ||
| "GET /v1/persons/{email}": frozenset({400, 403, 409}), | ||
| # 404/409 boilerplate; 403 IS reachable (person outside the caller's visible set) | ||
| "POST /v1/metric-results": frozenset({404, 409}), | ||
| # saved-query CRUD + run (#1965): 403 (no role gate — cross-tenant is 404 by | ||
| # opacity) and 409 (no conflict path) are `.standard_errors` boilerplate. | ||
| "GET /v1/queries": frozenset({400, 403, 404, 409}), # boilerplate: list, no input/lookup/conflict | ||
| "POST /v1/queries": frozenset({403, 404, 409}), # boilerplate (400 reachable: bad sql) | ||
| "GET /v1/queries/{id}": frozenset({403, 409}), # boilerplate | ||
| "PUT /v1/queries/{id}": frozenset({403, 409}), # boilerplate | ||
| "DELETE /v1/queries/{id}": frozenset({403, 409}), # boilerplate | ||
| "POST /v1/queries/{id}/run": frozenset({403, 409}), # boilerplate | ||
| # legacy per-metric thresholds: a read of a non-empty table 500s (#1663), so | ||
| # the success code never lands. | ||
| "GET /v1/metrics/{id}/thresholds": frozenset({200}), # #1663 | ||
| "POST /v1/metrics/{id}/thresholds": frozenset({201}), # #1663 | ||
| "PUT /v1/metrics/{id}/thresholds/{tid}": frozenset({200}), # #1663 | ||
| "DELETE /v1/metrics/{id}/thresholds/{tid}": frozenset({204}), # #1663 | ||
| # duplicate create answers 500 (unmapped UNIQUE violation), not 409 (#1664). | ||
| "POST /v1/admin/metric-thresholds": frozenset({409}), # #1664 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reduce the added comments to one line each.
The Python guideline permits comments only when code cannot express the reason. It also requires one-line comments. These multi-line policy comments exceed that limit.
Keep a short reason beside the exclusion. Move detailed policy context to the coverage design document or encode it in named values and tests.
Also applies to: 150-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tests/e2e/lib/api_coverage.py` around lines 41 - 63, The
comments above UNIVERSAL_BOILERPLATE and BLOCKED, including the per-route
exclusion comments, are overly detailed and span multiple lines. Reduce each
exclusion’s comment to a single concise line stating only its reason, while
preserving the existing constants, route mappings, and issue references; move
broader policy context elsewhere if needed.
Source: Coding guidelines
`.standard_errors` stamps a fixed {400,401,403,404,409,429,500} on an
operation with no way to select statuses, so the listing advertised six
codes it cannot answer: no license gate and no per-request authz (403),
no request body, no path or query params (400), no lookup (404), no
conflict path (409), no rate limiter (429).
The handler resolves tenant scope from the session and reaches only
`CanonicalError::internal`, so the operation answers 200, 500, or the
gateway's 401 when the bearer security requirement is unsatisfied.
Refs #1669
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
aleksdotbar
force-pushed
the
fix/1669-openapi-error-fidelity
branch
from
August 3, 2026 07:02
198b98b to
adefe78
Compare
aleksdotbar
changed the title
ktursunov
approved these changes
Aug 3, 2026
Both operations carried the same fixed `.standard_errors` set. Neither has a license gate or conflict path, so 403/409 were unanswerable on drilldown and 404/409 on metric-results — an unknown `metric_key` there resolves to a 400 (`unavailable`), pinned by its contract test. Declaring per route also picks up codes the boilerplate omitted: 415 on both (wrong Content-Type at the `Json` extractor), 429 on drilldown (its own concurrency cap), and 404 on drilldown alone, where an unknown `metric_key` is a genuine not-found. Drops the metric-results BLOCKED entry from the coverage gate: it excluded 404/409, neither of which the operation declares now. Refs #1669 Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
aleksdotbar
changed the title
aleksdotbar
enabled auto-merge
August 3, 2026 07:33
ktursunov
approved these changes
Aug 3, 2026
ktursunov
pushed a commit
that referenced
this pull request
Aug 4, 2026
… next ones Rebase picks up the first #1669 fidelity fixes. metric-definitions, metric-drilldown and metric-results now declare what they can actually answer instead of the seven-code `.standard_errors` stamp, which retires two of this gate's 403 exclusions: those operations no longer declare a 403 to subtract. The gate did not notice, and its own comment claimed it would. `stale_blocked` only asked whether the OPERATION had left the document — but a fidelity fix does not remove the operation, it stops the operation over-declaring. So an exclusion written against the old text goes on suppressing nothing while still reading as a live judgement about the route, which is the worst state for a suppression list to be in. `blocked_undeclared` closes that, and immediately named both entries. They are gone. `POST /v1/metric-drilldown/export` takes their place: it arrived with #2074 carrying the old boilerplate and the same absent gate, so its 403 is the over-declaration the others just shed. Reported, never blocking — a corrected document must not fail the gate. Generated models regenerated against the same spec: 72/150 coverable codes. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ktursunov
pushed a commit
that referenced
this pull request
Aug 4, 2026
…nothing can raise Both halves are corrections to what the gate DEMANDS, informed by the first fully green run (48/48 operations, 99 status codes observed). **409.** Declared on every route; producible by none. `already_exists`, `aborted` and `conflict` appear nowhere in the service, and the rig reached the same conclusion independently — it excludes 409 on all 26 operations it lists. So 27 operations were being held to a response the product has no code to send, which is the `.standard_errors` residue (#1669) the 403 pass already dealt with. Coverage 66.0% becomes 80.5% (99/123) with no test written, because the denominator was counting answers that cannot exist. One entry is a BUG rather than an absence: a duplicate admin threshold IS a conflict, and violates `uq_metric_threshold_scope_target` — it just arrives as a 500 (#1664). Excluded for that reason and tagged, with two independent notifications when it is fixed: the strict xfail in test_thresholds.py starts XPASSing, and this gate's blocked-now-observed advisory names the entry. The hygiene check earned its keep immediately. I also excluded 409 on `POST /v1/metric-results`, and the gate reported it stale on the spot — #2134 had already removed 409 from that declaration. Entry dropped; the exclusion I would otherwise have left behind never landed. **#1670.** The off-schema-body contract, per route and read off the handler signatures rather than inferred from file layout: `CanonicalJson` in api/admin/handlers.rs and api/catalog.rs answers the canonical 400 the spec declares, plain `axum::Json` answers Axum's own 422 for twelve others. Pinned as it BEHAVES with the intent named, so the suite keeps reporting what a caller receives today; the two canonical routes are what make the other twelve a fixable inconsistency rather than a limitation. `POST /v1/queries/{id}/run` is deliberately absent from that table — it binds `Option<Json<…>>`, where a rejected body's fate belongs to the Option wrapper rather than to this product's contract. The rig omits it for the same reason. 208 collected becomes 223. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
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
2 participants
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.
Problem
.standard_errors(openapi)is a fixed list in the toolkit — all-or-nothing, no status selection. Three unified-metric operations declared the same eight codes regardless of what their handlers can answer.GET /v1/metric-definitionsPOST /v1/metric-resultsPOST /v1/metric-drilldownBoth directions were wrong. Unanswerable codes: none of the three has a license gate or a conflict path;
metric-definitionstakes no body and no params; an unknownmetric_keyonmetric-resultsis a 400 (unavailable), not a 404, pinned by its contract test. Missing codes: 415 on the two body routes, and 429 on drilldown from its own concurrency cap.Fix
Per-route declaration via the toolkit's granular
error_4xx/error_5xxbuilders — sameProblemschema andapplication/problem+jsoncontent type as.standard_errors. Sets derived from the canonical-error kinds reachable from each handler plus its extractors.401 stays on all three: the operations carry a
bearerAuthrequirement, and an operation that declares auth must document the answer to failing it — the gateway emits it, not this process.Coverage gate
POST /v1/metric-resultsloses itsBLOCKEDentry (it excluded 404/409, neither now declared). No other entry moves; the other two operations never had one.No test changes needed — every newly required code already has a test (415 on both routes, 403 on metric-results, 401 via
api/test_unauthorized.py). Both routes end up with a strictly smaller required set than before, so no new coverage advisories.Scope
Three operations. The remaining 26 still call
.standard_errors, so #1669 stays open — including the admin threshold writes, which answer a real, undeclared 503 when the audit sink is down.Refs #1669