🐛 Address Copilot comments: JSON body validation, SSE comment, error chains#4170
🐛 Address Copilot comments: JSON body validation, SSE comment, error chains#4170clubanderson merged 1 commit intomainfrom
Conversation
- server.go: Remove ContentLength!=0 guard — use Body!=nil with io.EOF tolerance for empty bodies. JSON error response instead of plain text on /predictions/analyze (#4157) - sse.go: Fix comment to reflect namespace is always in cache key (#4157) - gateway.go: Use errors.Join for unwrappable error chains (#4159) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
bot
added
the
dco-signoff: yes
|
[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 |
✅ Deploy Preview for kubestellarconsole canceled.
|
kubestellar-prow
bot
added
the
size/S
There was a problem hiding this comment.
Pull request overview
Updates several handler and client behaviors to address prior review feedback: body validation on chunked/empty requests, JSON error format consistency, corrected SSE cache-key documentation, and improved error-chain composition.
Changes:
- Decode and validate optional JSON request bodies even for chunked/empty bodies (tolerating
io.EOF), and return JSON on 400s for/predictions/analyze. - Fix SSE cache-key comment to reflect that namespace is always part of the key (even when empty).
- Replace
%vformatting of multi-errors witherrors.Join()for better error chaining.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/k8s/gateway.go | Switches combined list errors to use errors.Join() for chainable errors. |
| pkg/api/handlers/sse.go | Corrects comment about namespace inclusion in SSE cache keys. |
| pkg/agent/server.go | Adjusts JSON body decoding/validation to handle chunked/empty bodies; returns JSON error payload for analyze 400s. |
| var combinedErr error | ||
| if len(errs) > 0 { | ||
| combinedErr = fmt.Errorf("gateway list errors: %v", errs) | ||
| combinedErr = fmt.Errorf("gateway list errors: %w", errors.Join(errs...)) |
There was a problem hiding this comment.
Wrapping errors.Join(errs...) with fmt.Errorf(... %w ...) changes the exposed unwrap shape: callers can no longer type-assert the returned error to interface{ Unwrap() []error } to retrieve all constituent errors directly (the wrapper only provides Unwrap() error). If consumers need access to the full joined list, prefer returning a joined error that includes context as one of the joined elements (e.g., join a contextual error with errs...) rather than wrapping the joined error.
| combinedErr = fmt.Errorf("gateway list errors: %w", errors.Join(errs...)) | |
| ctxErr := fmt.Errorf("gateway list errors") | |
| combinedErr = errors.Join(append([]error{ctxErr}, errs...)...) |
| var combinedErr error | ||
| if len(errs) > 0 { | ||
| combinedErr = fmt.Errorf("httproute list errors: %v", errs) | ||
| combinedErr = fmt.Errorf("httproute list errors: %w", errors.Join(errs...)) |
There was a problem hiding this comment.
Wrapping errors.Join(errs...) with fmt.Errorf(... %w ...) changes the exposed unwrap shape: callers can no longer type-assert the returned error to interface{ Unwrap() []error } to retrieve all constituent errors directly (the wrapper only provides Unwrap() error). If consumers need access to the full joined list, prefer returning a joined error that includes context as one of the joined elements (e.g., join a contextual error with errs...) rather than wrapping the joined error.
| combinedErr = fmt.Errorf("httproute list errors: %w", errors.Join(errs...)) | |
| contextErr := fmt.Errorf("httproute list errors") | |
| combinedErr = errors.Join(append([]error{contextErr}, errs...)...) |
| if r.Body != nil && r.ContentLength != 0 { | ||
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil { | ||
| if r.Body != nil { | ||
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil && err != io.EOF { |
There was a problem hiding this comment.
This 400 response encodes a JSON body but does not set Content-Type: application/json (unlike the updated /predictions/analyze path). To keep error responses consistent and easier for clients to parse, set the JSON content type header before writing the status code here as well.
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil && err != io.EOF { | |
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil && err != io.EOF { | |
| w.Header().Set("Content-Type", "application/json") |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 3 code suggestion(s) and 0 general comment(s). @copilot Please apply all of the following code review suggestions:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Summary
Addresses Copilot review comments from PRs #4157, #4158, #4159:
ContentLength != 0guard that skipped validation for chunked transfers. Now decodesr.Bodywhen present, toleratesio.EOFfor empty bodies. Returns JSON (not plain text) on/predictions/analyze400 errors.errors.Join()instead offmt.Errorf("%v", errs)for unwrappable error chains.