HYPERFLEET-993 - feat: add typed error response models#56
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR standardizes RFC 9457 problem+json error payloads: adds per-status response models (400/401/403/404/409), extracts example error constants, moves error models into the HyperFleet namespace, enforces Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Mischulee
force-pushed
the
HYPERFLEET-993
branch
2 times, most recently
from
250e433 to
cd618e4
Compare
![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
🤖 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 `@schemas/core/openapi.yaml`:
- Around line 2144-2194: The OpenAPI spec includes an unused component
ForbiddenResponse causing Spectral failures; either remove the ForbiddenResponse
schema (and its example) from components/schemas or add explicit 403 responses
that reference it for the operations that should advertise permission-denied
behavior; locate the ForbiddenResponse definition in schemas/core/openapi.yaml
and either delete that object or add response objects with $ref to
'`#/components/schemas/ForbiddenResponse`' on the relevant paths/operations to
ensure the component is actually referenced.
In `@shared/models/common/model.tsp`:
- Around line 114-119: The ForbiddenResponse model is emitted but never
referenced; either remove or stop generating it, or reference it from
authenticated operations that should return 403. To fix, locate the
ForbiddenResponse model (model ForbiddenResponse) and either delete it from
shared/models/common/model.tsp (and any generator inclusion) or add a 403
response reference to the relevant operation definitions (e.g., attach
ForbiddenResponse as the 403 response in the operations that require
authentication/authorization) so the model is actually consumed by the API
surface.
- Around line 61-133: The Error model mixes transport metadata with the problem
body; remove transport-only decorators from Error (or rename it to
ProblemDetails) so it contains only problem fields and move the `@example`
annotations onto that body model, then replace the current response models
(BadRequestResponse, UnauthorizedResponse, ForbiddenResponse, NotFoundResponse,
ConflictResponse) with thin wrappers that declare `@statusCode` (and any `@header`)
and contain a single payload property typed to the body model (e.g., body:
ProblemDetails) instead of using the spread (...Error); adjust references
accordingly so examples reflect only the JSON problem body and transport
metadata lives on the response wrappers.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e0cd2900-eb01-41cc-a9e8-f07693556934
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
CHANGELOG.mdcore/services/force-delete-internal.tspcore/services/resources-internal.tspcore/services/statuses-internal.tspmain.tsppackage.jsonschemas/core/openapi.yamlshared/models/common/example_errors.tspshared/models/common/model.tspshared/services/clusters.tspshared/services/nodepools.tsp
| ### Fixed | ||
|
|
||
| - Typed error response models (`BadRequestResponse`, `UnauthorizedResponse`, `NotFoundResponse`, `ConflictResponse`) now emit an `application/problem+json` body schema per RFC 9457 (HYPERFLEET-993) | ||
| - `UnauthorizedResponse` (401) added to all status and force-delete endpoints |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Pattern
The six status upsert endpoints (createOrUpdateStatus for cluster, nodepool, and resource statuses) previously declared | ConflictResponse (409) in their return unions. This PR removes 409 from all of them — which the JIRA ticket explicitly permits ("or the ConflictResponse usage is removed if intentionally absent").
However, the CHANGELOG doesn't mention this removal. Since it changes the documented API contract, a line under ### Fixed or ### Changed noting it would help consumers who rely on the spec for error handling:
- `ConflictResponse` (409) removed from status create/update endpoints (upsert semantics — conflict is not applicable)Also, | Error (default error response) was added to these same endpoints — a good fix, as it was previously missing.
There was a problem hiding this comment.
Added both fixes to changelog, thanks for pointing it.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 05f859b
into
openshift-hyperfleet:main
2 participants
Summary
Introduces dedicated typed error response models for standard HTTP error codes and adds UnauthorizedResponse (401) to all API endpoints.
Test Plan
npm run buildpassesapplication/problem+jsonbody schema in generatedschemas/core/openapi.yamlspectral lintpasseshyperfleet-api-spec-templatebuilds successfully against api-spec 1.0.20