Adds Error envelope#62
Merged
Merged
Conversation
- Implement reusable `ErrorEnvelope` struct for standardized error responses. - Add `HandleHttpError` function to write error details as `application/problem+json`. - Include unit tests to validate behavior with various scenarios.
…andleHttpError` for standardized responses. Update tests to validate error structure.
…log unknown codes. Update tests with additional cases for zero and negative status codes.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughProduct handlers now emit RFC 7807 Problem Details via a new ErrorEnvelope/HandleHttpError and buffer JSON encodes before writing responses; all product CRUD error paths were converted from plain text http.Error to structured JSON envelopes. ChangesProduct Handler Error Integration
Sequence DiagramsequenceDiagram
participant Client
participant ProductHandler
participant ProductStore
participant JSONEncoder
participant HTTPResponse
Client->>ProductHandler: HTTP request (create/get/list/update/delete)
ProductHandler->>ProductStore: DB query/command
alt DB returns error (pgx.ErrNoRows or other)
ProductStore-->>ProductHandler: error
ProductHandler->>HTTPResponse: HandleHttpError(ErrorEnvelope, status)
else DB returns data
ProductStore-->>ProductHandler: data
ProductHandler->>JSONEncoder: encode data into bytes.Buffer
alt encode success
JSONEncoder-->>ProductHandler: buf.Bytes()
ProductHandler->>HTTPResponse: set Content-Type and Write(buf.Bytes())
else encode failure
JSONEncoder-->>ProductHandler: encode error
ProductHandler->>HTTPResponse: HandleHttpError(ErrorEnvelope, 500)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/platform/handler_test.go (1)
62-126:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftInsufficient test coverage for ErrorEnvelope structure.
Tests for
CreatePlatform,UpdatePlatform,GetPlatform, andGetPlatformsverify HTTP status codes but do not verify the RFC 7807 ErrorEnvelope structure for error responses. This leaves the error envelope integration untested for these handlers.Each error test case should verify:
Content-Type: application/problem+jsonheader- ErrorEnvelope fields:
type,title,status,detailstatusfield matches HTTP status codeAs per coding guidelines: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Also applies to: 128-189, 191-275, 371-527
🤖 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 `@internal/platform/handler_test.go` around lines 62 - 126, The tests in TestCreatePlatform (and analogous tests for UpdatePlatform/GetPlatform/GetPlatforms) only assert HTTP status codes but not the RFC 7807 ErrorEnvelope; update the error-case branches in TestCreatePlatform (and the other test functions) to, after calling platformHandler.CreatePlatform via rr, assert that for error responses rr.Header().Get("Content-Type") == "application/problem+json" and unmarshal rr.Body into the ErrorEnvelope struct and assert its fields type, title, status, and detail exist and that envelope.Status equals rr.Code (use the mockPlatformQuerier error cases and the "Invalid JSON"/"Missing Name"/"DB Error" scenarios to drive these assertions); reference the platformHandler.CreatePlatform handler, the tests' rr (httptest.ResponseRecorder), and the ErrorEnvelope type when locating where to add the checks.
🤖 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 `@internal/error_envelope_test.go`:
- Around line 11-18: The table-driven tests initialize ErrorEnvelope values but
never assert the Detail and Instance fields; extend the test struct (the tests
variable) to include expectedDetail and expectedInstance fields, populate them
in each case, and add assertions in the test loop that compare the envelope's
Detail and Instance (from the ErrorEnvelope under test/and/or the rendered
output) against expectedDetail and expectedInstance so the RFC7807 fields are
covered; locate the tests slice and the loop that checks
expectedBody/expectedTitle/expectedType and add the two new checks there
referencing ErrorEnvelope.Detail and ErrorEnvelope.Instance.
In `@internal/platform/handler_test.go`:
- Around line 355-365: The test currently only checks ErrorEnvelope.Detail;
extend the assertion block that runs when rr.Code >= 400 to also validate the
Content-Type header equals "application/problem+json" and assert envelope.Type
== "about:blank", envelope.Title is non-empty (or matches expected short
summary), and envelope.Status equals rr.Code (or tt.expectedStatus); locate the
checks around rr, the ErrorEnvelope type and variable envelope in
handler_test.go and add header check using rr.Header().Get("Content-Type") and
the additional field assertions for Type, Title and Status so the test fully
conforms to RFC 7807.
In `@internal/platform/handler.go`:
- Around line 118-122: The handler currently sets w.Header().Set("Content-Type",
"application/json") then calls json.NewEncoder(w).Encode(platforms), which can
write partial data and prevent internal.HandleHttpError from changing headers to
application/problem+json; fix by encoding into a temporary buffer (e.g.,
bytes.Buffer or json.Marshal) inside the same function (the scope where
platforms is available), check for encoding error before writing any
headers/body, and only on success set Content-Type to application/json and write
the status and buffer to w; keep calls to internal.HandleHttpError and
ErrorEnvelope for the error path so the handler can set the proper
application/problem+json content type and status.
- Around line 145-149: The response currently sets Content-Type and streams json
directly with json.NewEncoder(w).Encode(platform) which can leave Content-Type
set to application/json even if encoding fails; change to encode into a
temporary buffer first (e.g., bytes.Buffer) inside the same handler (the
function containing the json.NewEncoder call), check for encoding error, call
internal.HandleHttpError(w, internal.ErrorEnvelope{...}) on failure, and only if
encoding succeeds set w.Header().Set("Content-Type", "application/json") and
write the buffer to w so HandleHttpError can set the correct RFC 7807 headers;
update the code around json.NewEncoder(w).Encode(platform) and the Content-Type
header accordingly.
In `@internal/product/handler.go`:
- Around line 127-129: The current code calls
json.NewEncoder(w).Encode(products) (and json.NewEncoder(w).Encode(product))
which may write response bytes before errors are detected so
internal.HandleHttpError cannot set headers/status; instead, encode into an
in-memory buffer first (e.g., bytes.Buffer or bytes.NewBuffer), check the Encode
error, and only if successful set Content-Type and write the status and buffer
bytes to w; on encode error call internal.HandleHttpError with the error detail.
Target fixes around the json.NewEncoder(w).Encode(products)/Encode(product)
sites and ensure you perform the buffer-then-write pattern before writing any
headers or body.
---
Outside diff comments:
In `@internal/platform/handler_test.go`:
- Around line 62-126: The tests in TestCreatePlatform (and analogous tests for
UpdatePlatform/GetPlatform/GetPlatforms) only assert HTTP status codes but not
the RFC 7807 ErrorEnvelope; update the error-case branches in TestCreatePlatform
(and the other test functions) to, after calling platformHandler.CreatePlatform
via rr, assert that for error responses rr.Header().Get("Content-Type") ==
"application/problem+json" and unmarshal rr.Body into the ErrorEnvelope struct
and assert its fields type, title, status, and detail exist and that
envelope.Status equals rr.Code (use the mockPlatformQuerier error cases and the
"Invalid JSON"/"Missing Name"/"DB Error" scenarios to drive these assertions);
reference the platformHandler.CreatePlatform handler, the tests' rr
(httptest.ResponseRecorder), and the ErrorEnvelope type when locating where to
add the checks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef1dbb21-691c-4633-bb06-bbe659d4c58d
📒 Files selected for processing (5)
internal/error_envelope.gointernal/error_envelope_test.gointernal/platform/handler.gointernal/platform/handler_test.gointernal/product/handler.go
…e a `bytes.Buffer` for improved reliability
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/product/handler.go (1)
158-165:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame
errcheckfinding at line 165.Apply the same fix as for
GetProductsByPlatform: capture or explicitly discard thew.Writereturn value to satisfyerrcheck.🤖 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 `@internal/product/handler.go` around lines 158 - 165, The w.Write call's error return is ignored; update the response write in the same way as GetProductsByPlatform by capturing or explicitly discarding the returned (int, error) from w.Write(buf.Bytes()) — e.g., assign to _ , err or _ , _ — and if err != nil call internal.HandleHttpError with a suitable internal.ErrorEnvelope and http.StatusInternalServerError; target the write after encoding (bytes.Buffer / json.NewEncoder.Encode and the w.Write call) to satisfy errcheck.
🤖 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 `@internal/product/handler.go`:
- Around line 127-134: The call to w.Write(buf.Bytes()) in the response path
(inside the product list handler in internal/product/handler.go) currently
ignores its returned error; update that code to capture and handle the error
(either assign to _ if you intentionally want to ignore it or, preferably, check
the error and log it via existing logging/error handling utilities or call
internal.HandleHttpError for unexpected write failures). Locate the block that
encodes products with json.NewEncoder(&buf).Encode(products) and the subsequent
w.Header().Set(...); w.Write(buf.Bytes()) line and modify the w.Write call to
capture the returned (written, err) result and handle/log the err appropriately.
---
Duplicate comments:
In `@internal/product/handler.go`:
- Around line 158-165: The w.Write call's error return is ignored; update the
response write in the same way as GetProductsByPlatform by capturing or
explicitly discarding the returned (int, error) from w.Write(buf.Bytes()) —
e.g., assign to _ , err or _ , _ — and if err != nil call
internal.HandleHttpError with a suitable internal.ErrorEnvelope and
http.StatusInternalServerError; target the write after encoding (bytes.Buffer /
json.NewEncoder.Encode and the w.Write call) to satisfy errcheck.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1f24a65-16d8-437c-9de6-c51782904559
📒 Files selected for processing (3)
.gitignoreinternal/platform/handler.gointernal/product/handler.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
…ers using `HandleHttpError` for consistent error reporting
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.
Description
Code Rabbit Summary
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
Fixes
Closes #58
Post Deployment Tasks?