errs: conform HTTP responses to problem+json - #9
Open
Peyton-Spencer wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe error handling is migrated from simple JSON responses to RFC 7807 "application/problem+json" format. Changes include a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 1
🧹 Nitpick comments (2)
errs/json_test.go (1)
55-66: Consider closing response body for best practices consistency.While
httptest.ResponseRecorderdoesn't leak resources if the body isn't closed, it's good practice to closeresponse.Bodyto maintain consistency with real HTTP client usage patterns and prevent issues if tests are later refactored.🧹 Optional: Add defer to close response body
response := recorder.Result() + defer response.Body.Close() if response.StatusCode != http.StatusNotFound {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@errs/json_test.go` around lines 55 - 66, The test fetches the response via recorder.Result() into the variable response but never closes response.Body; after obtaining response (the result of recorder.Result()) add a defer response.Body.Close() immediately before decoding to ensure the response body is closed consistently (i.e., insert a defer response.Body.Close() right after response := recorder.Result() and before json.NewDecoder(response.Body).Decode(&body)).errs/errs.go (1)
111-114: Consider logging marshal errors for observability.While
json.Marshalfailing is extremely unlikely here (the struct contains only basic types from controlled sources), silently discarding the error could make debugging difficult if it ever does occur. The client would receive an empty body with the correct status code and headers.🔧 Optional: Log marshal errors for debugging
data, err := json.Marshal(e) - if err == nil { + if err != nil { + // Log unexpected marshal failure; body will be empty but headers/status are already sent + zerolog.Ctx(e.ctx).Error().Err(err).Msg("failed to marshal problem+json response") + } else { _, _ = w.Write(data) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@errs/errs.go` around lines 111 - 114, The json.Marshal call that serializes variable e into data ignores marshal errors (data, err := json.Marshal(e)) and silently skips writing a response body; update the error handling to log the marshal error and write a safe fallback response body so the client still gets useful content. Specifically, in the function that writes the error response (the block using variables data, err and writer w), if json.Marshal returns a non-nil err, call the appropriate logger (or fmt.Fprintf to w as fallback) to record the marshal error and then write a simple JSON or plain-text fallback message to w before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@errs/errs.go`:
- Line 4: Remove the unused bytes package import and the unused struct field:
delete the "bytes" import entry and remove the `b bytes.Buffer` field
declaration (the `b` field) from the relevant error response struct so the code
no longer references an unused Buffer after `Abort()` was changed to use
`json.Marshal()` directly; search for the struct that defines `b bytes.Buffer`
and remove that field and the import.
---
Nitpick comments:
In `@errs/errs.go`:
- Around line 111-114: The json.Marshal call that serializes variable e into
data ignores marshal errors (data, err := json.Marshal(e)) and silently skips
writing a response body; update the error handling to log the marshal error and
write a safe fallback response body so the client still gets useful content.
Specifically, in the function that writes the error response (the block using
variables data, err and writer w), if json.Marshal returns a non-nil err, call
the appropriate logger (or fmt.Fprintf to w as fallback) to record the marshal
error and then write a simple JSON or plain-text fallback message to w before
returning.
In `@errs/json_test.go`:
- Around line 55-66: The test fetches the response via recorder.Result() into
the variable response but never closes response.Body; after obtaining response
(the result of recorder.Result()) add a defer response.Body.Close() immediately
before decoding to ensure the response body is closed consistently (i.e., insert
a defer response.Body.Close() right after response := recorder.Result() and
before json.NewDecoder(response.Body).Decode(&body)).
🪄 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
Run ID: c4c8c804-7226-48f6-aeb4-867c2d193c46
📒 Files selected for processing (4)
errs/README.mderrs/errs.goerrs/json.goerrs/json_test.go
| @@ -3,6 +3,7 @@ package errs | |||
| import ( | |||
| "bytes" | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if bytes.Buffer field 'b' or bytes package is used anywhere in errs package
echo "=== Checking usage of bytes.Buffer field 'b' ==="
rg -n '\.b\.' --type=go errs/
echo "=== Checking usage of bytes package functions ==="
rg -n 'bytes\.' --type=go errs/
echo "=== Checking if 'b' field is accessed ==="
ast-grep --pattern 'e.b.$_($$$)'Repository: omniaura/go-kit
Length of output: 244
Remove unused bytes import and b bytes.Buffer field.
The bytes package (line 4) and b bytes.Buffer field (line 26) are no longer used after Abort() was updated to use json.Marshal() directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@errs/errs.go` at line 4, Remove the unused bytes package import and the
unused struct field: delete the "bytes" import entry and remove the `b
bytes.Buffer` field declaration (the `b` field) from the relevant error response
struct so the code no longer references an unused Buffer after `Abort()` was
changed to use `json.Marshal()` directly; search for the struct that defines `b
bytes.Buffer` and remove that field and the import.
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
1 participant
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.
Summary
errsresponses to RFC 7807 problem detailsContent-Type: application/problem+jsoninAbortand map errors totype,title,status, anddetailTesting
go test ./errs/...Closes #2
Summary by CodeRabbit