Issue #2973 Return structured JSON errors from config API#5143
Issue #2973 Return structured JSON errors from config API#5143sebrandon1 wants to merge 1 commit intocrc-org:mainfrom
Conversation
|
Hi @sebrandon1. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds structured validation error handling: server detects Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant APIServer as API Server
participant Helper as writeMultiError
participant Encoder as JSON Encoder
Client->>APIServer: POST /config (invalid fields)
APIServer->>APIServer: validate -> returns errors.MultiError
APIServer->>Helper: invoke writeMultiError(multiErr)
Helper->>Helper: map each error -> client.ValidationError
Helper->>Encoder: encode []client.ValidationError
Encoder-->>Client: HTTP 422 + application/json body (array of validation errors)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
b811b3f to
9581ba1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/crc/api/helpers.go (1)
124-127:⚠️ Potential issue | 🟠 MajorPre-existing bug: response headers are set after
WriteHeader.In Go's
net/http, callingw.WriteHeader()sends the status line and headers immediately. Anyw.Header().Set()calls after that are silently ignored. The newwriteMultiErrorcorrectly sets headers beforeWriteHeader(lines 141–142), but the existing success path here does it in the wrong order.Not introduced by this PR, but worth fixing for consistency — especially since the new code demonstrates the correct pattern.
Proposed fix
- w.WriteHeader(c.code) for k, v := range c.headers { w.Header().Set(k, v) } + w.WriteHeader(c.code)
🧹 Nitpick comments (1)
pkg/crc/api/client/types.go (1)
70-73: Consider enrichingValidationErrorwith additional fields per issue requirements.The linked issue
#2973calls for structured errors that include the property name, provided value, expected/default value, and reason — enabling clients to programmatically present specific field-level feedback. Currently, onlyMessageis provided, which is essentially the same information as the plain-text error but wrapped in JSON.Consider expanding the struct, e.g.:
type ValidationError struct { Property string `json:"property"` Value interface{} `json:"value,omitempty"` Reason string `json:"reason"` }This would give client applications (like the Electron tray app) something truly machine-parseable rather than requiring them to parse a human-readable message string.
|
We need to check this change with podman desktop which uses api endpoint for any operation for crc side. |
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
9581ba1 to
2fd8b3b
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
@praveenkumar I tested this change against the crc-extension source to assess Podman Desktop compatibility. Here are the findings: What changes: The How crc-extension handles it: In Impact: Not a crash — but users would see Test evidence: Options to address:
Let me know which approach you'd prefer and I can implement it. |
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
2fd8b3b to
15a920f
Compare
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/crc/api/client/types.go (1)
70-73: Consider expandingValidationErrorto structured fields for true machine-readability.The PR's linked issue (
#2973) asks for a schema with discrete fields (property name, invalid value, required/default value, reason). The current singleMessagefield still forces clients to do brittle string parsing — the same root problem as the plain-text concatenation it replaces.♻️ Optional schema expansion
// ValidationError represents a single validation error in a structured JSON error response. type ValidationError struct { - Message string `json:"message"` + Message string `json:"message"` // human-readable summary (kept for backwards compat) + Property string `json:"property,omitempty"` // the configuration key that failed validation + Value string `json:"value,omitempty"` // the value that was rejected + Reason string `json:"reason,omitempty"` // machine-readable reason for the failure }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/api/client/types.go` around lines 70 - 73, The ValidationError struct currently only has Message which forces clients to parse text; change the ValidationError type to use discrete fields (e.g., Property string, InvalidValue interface{}, Expected interface{} or Required bool/Default interface{}, and Reason string) so the API returns machine-readable JSON; update all marshaling/unmarshaling usage and any code that constructs ValidationError (search for ValidationError, NewValidationError, or places that set ValidationError.Message) to populate the new fields and adjust consumer code to read those fields instead of parsing Message.pkg/crc/api/api_http_test.go (1)
514-539: LGTM — consider also assertingContent-Type: application/jsonin the response.The non-deterministic map iteration handling is well-designed. The one gap is that
writeMultiErrorexplicitly setsContent-Type: application/json, but the test doesn't verify it. Since this is part of the new API contract, adding an assertion here (and in the table case at line 306) would lock it in:require.Equal(t, "application/json", resp.Header.Get("Content-Type"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/api/api_http_test.go` around lines 514 - 539, The test TestSetConfigMultipleErrors should also assert the response Content-Type to lock in the API contract set by writeMultiError; after you send the request and have resp (e.g., after resp.StatusCode or after reading resp.Body) add a check like require.Equal(t, "application/json", resp.Header.Get("Content-Type")). Do the same assertion in the table-driven test case that exercises writeMultiError (the earlier table case that validates multi-error responses) so both paths explicitly verify the Content-Type header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/crc/api/api_http_test.go`:
- Around line 306-309: The test is brittle because it asserts a raw JSON string
with HTML-escaped characters; change the assertion to be encoder-agnostic by
decoding the response body into []client.ValidationError and comparing the
resulting slice to the expected slice (same approach used in
TestSetConfigMultipleErrors). Locate the failing case that uses
post("config").withBody(`{"properties":{"cpus":0}}`) and the expected
httpError(500).withBody(...), read the response body produced in the test,
json.Unmarshal it into []client.ValidationError, and assert equality against the
expected []client.ValidationError (rather than matching the escaped string);
this will make the test resilient to changes in writeMultiError or
enc.SetEscapeHTML.
In `@pkg/crc/api/helpers.go`:
- Around line 135-147: The function writeMultiError builds validationErrors from
multiErr.Errors and currently calls e.Error() without checking for nil, which
can panic; also it sets http.StatusInternalServerError but these are client
validation issues and should use a 4xx status. Fix by guarding nil elements in
multiErr.Errors inside writeMultiError (skip or provide a safe message when e ==
nil before calling e.Error()) when constructing client.ValidationError entries,
and change the response status from http.StatusInternalServerError to an
appropriate 4xx (choose http.StatusBadRequest or http.StatusUnprocessableEntity)
while noting this is a behavioral change; ensure the Content-Type header and
JSON encoding of validationErrors (client.ValidationError) remain unchanged and
retain the logging path on Encode error.
---
Nitpick comments:
In `@pkg/crc/api/api_http_test.go`:
- Around line 514-539: The test TestSetConfigMultipleErrors should also assert
the response Content-Type to lock in the API contract set by writeMultiError;
after you send the request and have resp (e.g., after resp.StatusCode or after
reading resp.Body) add a check like require.Equal(t, "application/json",
resp.Header.Get("Content-Type")). Do the same assertion in the table-driven test
case that exercises writeMultiError (the earlier table case that validates
multi-error responses) so both paths explicitly verify the Content-Type header.
In `@pkg/crc/api/client/types.go`:
- Around line 70-73: The ValidationError struct currently only has Message which
forces clients to parse text; change the ValidationError type to use discrete
fields (e.g., Property string, InvalidValue interface{}, Expected interface{} or
Required bool/Default interface{}, and Reason string) so the API returns
machine-readable JSON; update all marshaling/unmarshaling usage and any code
that constructs ValidationError (search for ValidationError, NewValidationError,
or places that set ValidationError.Message) to populate the new fields and
adjust consumer code to read those fields instead of parsing Message.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/crc/api/api_http_test.gopkg/crc/api/client/client.gopkg/crc/api/client/types.gopkg/crc/api/helpers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/crc/api/client/client.go
@evidolob can you take a look to this one and suggest what would be better for extension side? |
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
15a920f to
79819fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/crc/api/client/types.go (1)
70-73: Consider extendingValidationErrorwith stable machine-readable fields.
messagealone is human-friendly but brittle for client logic. Optional fields likeproperty,value, andreasonwould make integrations more robust without breaking existing consumers ofmessage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/api/client/types.go` around lines 70 - 73, The ValidationError struct only has a human-friendly Message which is brittle for client logic; update the ValidationError type to include stable, optional machine-readable fields (e.g. Property string `json:"property,omitempty"`, Value interface{} `json:"value,omitempty"`, Reason string `json:"reason,omitempty"`) while keeping the existing Message field and JSON tag to preserve backward compatibility; add omitempty to new fields so existing consumers are unaffected and update any code that constructs ValidationError (e.g., places that instantiate ValidationError) to populate these fields where available.pkg/crc/api/helpers.go (1)
145-147: Consider a migration-friendly error shape for legacy clients.Always returning a JSON array can surface raw JSON strings in older clients. A transitional envelope (for example,
{"error":"...","details":[...]}) orAccept-based fallback would preserve readability while keeping structured details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/api/helpers.go` around lines 145 - 147, The handler currently encodes validationErrors directly as a JSON array; change it to write a migration-friendly envelope object (e.g., {"error":"validation failed","details": validationErrors}) by creating an envelope map or struct and encoding that instead of validationErrors; additionally, inspect the request Accept header and, if the client prefers text/plain or lacks JSON support, fall back to a simple human-readable error string (or the "error" field) so legacy clients see a readable message while newer clients get structured details; update the write logic where validationErrors is encoded to use the new envelope and Accept-based fallback.pkg/crc/api/api_http_test.go (1)
523-530: Optional: assertContent-Typeto lock the wire contract.Since this PR formalizes JSON validation errors, adding a header assertion here would prevent regressions in response media type.
🧪 Suggested assertion
require.Equal(t, http.StatusUnprocessableEntity, resp.StatusCode) + require.Contains(t, resp.Header.Get("Content-Type"), "application/json") body, err := io.ReadAll(resp.Body) require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/api/api_http_test.go` around lines 523 - 530, Add an assertion that the response Content-Type is the expected JSON media type to lock the wire contract: check resp.Header.Get("Content-Type") (using the exact expected value used elsewhere in tests, e.g., "application/json" or "application/json; charset=utf-8") before reading the body in the test (the block that currently reads resp and unmarshals into validationErrors), so the test asserts the response media type alongside the status and validation error payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/crc/api/client/client.go`:
- Around line 261-262: The error paths currently ignore errors from io.ReadAll
and return an HTTPError with an empty Body; update the error-path reads to
mirror the success path: call body, err := io.ReadAll(res.Body) and if err !=
nil return nil, fmt.Errorf("reading response body: %w", err) (or propagate the
read error) instead of discarding it, then include the body in the returned
&HTTPError{... Body: string(body)}; apply this change in the request-handling
function where HTTPError is constructed so all branches consistently handle
io.ReadAll errors.
---
Nitpick comments:
In `@pkg/crc/api/api_http_test.go`:
- Around line 523-530: Add an assertion that the response Content-Type is the
expected JSON media type to lock the wire contract: check
resp.Header.Get("Content-Type") (using the exact expected value used elsewhere
in tests, e.g., "application/json" or "application/json; charset=utf-8") before
reading the body in the test (the block that currently reads resp and unmarshals
into validationErrors), so the test asserts the response media type alongside
the status and validation error payload.
In `@pkg/crc/api/client/types.go`:
- Around line 70-73: The ValidationError struct only has a human-friendly
Message which is brittle for client logic; update the ValidationError type to
include stable, optional machine-readable fields (e.g. Property string
`json:"property,omitempty"`, Value interface{} `json:"value,omitempty"`, Reason
string `json:"reason,omitempty"`) while keeping the existing Message field and
JSON tag to preserve backward compatibility; add omitempty to new fields so
existing consumers are unaffected and update any code that constructs
ValidationError (e.g., places that instantiate ValidationError) to populate
these fields where available.
In `@pkg/crc/api/helpers.go`:
- Around line 145-147: The handler currently encodes validationErrors directly
as a JSON array; change it to write a migration-friendly envelope object (e.g.,
{"error":"validation failed","details": validationErrors}) by creating an
envelope map or struct and encoding that instead of validationErrors;
additionally, inspect the request Accept header and, if the client prefers
text/plain or lacks JSON support, fall back to a simple human-readable error
string (or the "error" field) so legacy clients see a readable message while
newer clients get structured details; update the write logic where
validationErrors is encoded to use the new envelope and Accept-based fallback.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/crc/api/api_http_test.gopkg/crc/api/client/client.gopkg/crc/api/client/types.gopkg/crc/api/helpers.go
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
79819fc to
8c33284
Compare
|
I think it is better to update the crc-extension to handle/parse json error messages, it would improve user averseness if something going wrong. But I not sure who is responsible to made thous changes in extension. |
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
8c33284 to
9037c0b
Compare
extension still part of crc org so if you want to update it once this PR merge, that should be doable. @sebrandon1 another thing which I want to point out is we are in progress to change some of the component around how crc works today.
So if that is not that urgent or breaking something , I would say let's not spend time on that and extension side and in future this will not even present. |
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
9037c0b to
8189bd8
Compare
|
Okay no worries! Good to know. |
sebrandon1
force-pushed
the
structured-json-config-errors
branch
2 times, most recently
from
d05a5b7 to
90f0947
Compare
|
/hold |
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
90f0947 to
5d2bb04
Compare
sebrandon1
force-pushed
the
structured-json-config-errors
branch
2 times, most recently
from
fc2e4d9 to
de776f9
Compare
sebrandon1
force-pushed
the
structured-json-config-errors
branch
3 times, most recently
from
13aa381 to
3ac6bfd
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
3ac6bfd to
eabb0f6
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
d4a832c to
84ce09c
Compare
When setting config via the API with invalid values, detect MultiError at the HTTP response layer and return a JSON array of structured ValidationError objects with HTTP 422 and Content-Type: application/json. Preserve error response body in the API client via HTTPError. Resolves crc-org#2973
sebrandon1
force-pushed
the
structured-json-config-errors
branch
from
84ce09c to
79fcb38
Compare
|
@sebrandon1: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
MultiErrorat the HTTP response layer and return a JSON array of structuredValidationErrorobjects withContent-Type: application/jsonsendRequest()viaHTTPError(previously discarded)Resolves #2973
Jira: https://issues.redhat.com/browse/CNFCERT-1343
Examples
Single invalid value:
[{"message":"Value '0' for configuration property 'cpus' is invalid, reason: requires CPUs >= 4"}]Multiple invalid values:
[ {"message":"Value '0' for configuration property 'cpus' is invalid, reason: requires CPUs >= 4"}, {"message":"Value '1' for configuration property 'memory' is invalid, reason: requires memory in MiB >= 10752"} ]Non-validation errors remain plain text (unchanged):
Test plan
go test -race --tags "build containers_image_openpgp" ./pkg/crc/api/...)make testpassesmake lintpasses (0 issues)Summary by CodeRabbit
Bug Fixes
Tests