refactor: use WriteErr function for handling error returned by handler #640
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new function, Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
error.go (1)
283-306: LGTM with minor suggestions for error wrapping consistency.The implementation effectively handles status errors with proper content negotiation and special cases. Consider wrapping transformation and marshaling errors for better error context:
func writeStatusError(api API, ctx Context, err StatusError) error { ct, negotiateErr := api.Negotiate(ctx.Header("Accept")) if negotiateErr != nil { return fmt.Errorf("failed to write status error: %w", negotiateErr) } if ctf, ok := err.(ContentTypeFilter); ok { ct = ctf.ContentType(ct) } ctx.SetHeader("Content-Type", ct) status := err.GetStatus() ctx.SetStatus(status) // If request accept no output, just set the status code and return. if status == http.StatusNoContent || status == http.StatusNotModified { return nil } tval, terr := api.Transform(ctx, strconv.Itoa(status), err) if terr != nil { - return terr + return fmt.Errorf("failed to transform status error: %w", terr) } - return api.Marshal(ctx.BodyWriter(), ct, tval) + if err := api.Marshal(ctx.BodyWriter(), ct, tval); err != nil { + return fmt.Errorf("failed to marshal status error: %w", err) + } + return nil }huma.go (1)
Line range hint
965-971: Ensure type safety when setting string values for enumsThe new code supports slices of string subtypes (enums) by dynamically creating and setting values. However, there is a potential for a panic if
f.Type().Elem().Kind()is notreflect.String.Consider adding a type check before calling
SetString(val)to ensure thatf.Type().Elem().Kind()is indeedreflect.String.Apply this diff to add the type check:
+ if f.Type().Elem().Kind() != reflect.String { + res.Add(pb, value, "invalid enum type") + return + } enumVal := reflect.New(f.Type().Elem()).Elem() enumVal.SetString(val)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
error.go(2 hunks)huma.go(2 hunks)openapi.go(1 hunks)
🔇 Additional comments (4)
error.go (2)
400-400: LGTM! Clean simplification of error formatting.
Direct assignment to fmt.Sprintf is a good simplification that maintains the same functionality while reducing indirection.
283-306: Verify integration with existing error handling patterns.
Since this introduces a new error handling function, let's verify its integration:
✅ Verification successful
Integration pattern is consistent and properly implemented
The verification shows:
writeStatusErroris correctly used inhuma.goto handleStatusErrorinstances- It complements the existing
WriteErrfunction which is used extensively throughout the codebase - The error handling flow is consistent:
WriteErrcreates and writes structured errorswriteStatusErrorspecifically handlesStatusErrorimplementations
- The implementation aligns with the documented error handling patterns in the docs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of error handling functions to ensure consistent approach
# Check where writeStatusError is being called
echo "Checking writeStatusError usage:"
rg "writeStatusError\(" -A 2
# Check existing error handling patterns
echo -e "\nChecking WriteErr usage for comparison:"
rg "WriteErr\(" -A 2
# Look for potential places where writeStatusError should be used
echo -e "\nChecking StatusError handling patterns:"
rg "StatusError\)" -A 5
Length of output: 7218
openapi.go (1)
866-867: LGTM! Documentation update accurately reflects the error schema generation options.
The comment update properly documents that error schemas can now be generated from either huma.NewError() or huma.NewErrorWithContext, which aligns with the PR's objective to enhance error handling capabilities.
huma.go (1)
1405-1415: Refactored error handling aligns with error management improvements
The introduction of the writeStatusError function and its usage to handle StatusError enhances consistency in error responses and leverages content negotiation. This change aligns with the PR objectives to streamline error handling.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #640 +/- ##
==========================================
+ Coverage 92.70% 92.97% +0.26%
==========================================
Files 22 22
Lines 4879 4895 +16
==========================================
+ Hits 4523 4551 +28
+ Misses 308 300 -8
+ Partials 48 44 -4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
error_test.go (1)
Line range hint
1-143: Consider adding test cases for new error handling functionalityWhile the existing tests provide good coverage, consider adding specific test cases for:
- The new
writeStatusErrorfunction to verify its behavior with different status codes- Integration between
WriteErrandwriteStatusErrorto ensure they work together correctly- Edge cases where both plain errors and status errors are handled in the same request
This would help ensure the robustness of the new error handling implementation.
huma.go (2)
466-488: LGTM! Well-structured response writing logic.The function effectively centralizes response writing with proper content type negotiation and error handling. The fallback to JSON on negotiation failure is a good defensive programming practice.
Consider extracting the error response logic (lines 474-480) into a separate function to improve readability and reusability:
+func writeErrorResponse(api API, ctx Context, err error) error { + status := http.StatusInternalServerError + if se, ok := err.(StatusError); ok { + status = se.GetStatus() + } + return transformAndWrite(api, ctx, status, "application/json", err) +} func writeResponse(api API, ctx Context, status int, ct string, body any) error { if ct == "" { var err error ct, err = getContentType(api, ctx, body) if err != nil { - status := http.StatusInternalServerError - if se, ok := err.(StatusError); ok { - status = se.GetStatus() - } - if err := transformAndWrite(api, ctx, status, "application/json", err); err != nil { - return err - } + return writeErrorResponse(api, ctx, err) } } // ... rest of the function }
490-494: LGTM! Consider adding documentation about panic behavior.The function serves as a panic wrapper for writeResponse, which is useful in scenarios where error handling isn't feasible. However, it would benefit from clear documentation about its panic behavior.
Add a documentation comment explaining the panic behavior:
+// writeResponseWithPanic writes a response using writeResponse and panics if an error occurs. +// This should only be used in scenarios where normal error handling isn't feasible, +// such as in final error handlers or cleanup routines. func writeResponseWithPanic(api API, ctx Context, status int, ct string, body any) { if err := writeResponse(api, ctx, status, ct, body); err != nil { panic(err) } }huma_test.go (1)
1368-1383: LGTM! The test case effectively verifies generic error handling.The test case is well-structured and provides good coverage for generic error handling, ensuring that non-status errors are properly converted to 500 Internal Server Error responses.
Consider using
errors.Newfor better performance.Since the error message is a simple string without any formatting, using
errors.Newwould be more efficient thanfmt.Errorf.Apply this diff to improve performance:
- return nil, fmt.Errorf("whoops") + return nil, errors.New("whoops")🧰 Tools
🪛 golangci-lint
1375-1375: fmt.Errorf can be replaced with errors.New
(perfsprint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
error.go(2 hunks)error_test.go(2 hunks)huma.go(6 hunks)huma_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- error.go
🧰 Additional context used
🪛 golangci-lint
huma_test.go
1375-1375: fmt.Errorf can be replaced with errors.New
(perfsprint)
🔇 Additional comments (4)
error_test.go (2)
86-86: LGTM: Improved test context initialization
The change from nil to &huma.Operation{} makes the test more realistic by providing a concrete operation context, which better reflects real-world usage scenarios.
101-101: LGTM: Consistent context initialization
The change maintains consistency with other tests by using a proper Operation instance in the context. This helps ensure that error transformation is tested under realistic conditions.
huma.go (2)
496-507: LGTM! Well-designed content type negotiation.
The function effectively handles content type negotiation with proper error handling and extensibility through the ContentTypeFilter interface. The error message includes context which is helpful for debugging.
1449-1458: LGTM! Improved error handling with better context.
The changes effectively implement the PR objectives by:
- Using dedicated error handling for StatusError
- Adding request context to generic errors
- Maintaining consistent error response writing through writeResponseWithPanic
First of all, thank you for �maintating the Huma framework active. I'm using it for personal projects, and I can write code more conveniently and elegantly than before.
What is this PR?
This PR refactors error handling logic in
Registerfunction to useWriteErrfunction.Context
Registerfunction can register handler and include handling logic for overall request-response lifecycle. If the registered handler returns anerror, the error is handled according to the error type. For now, there is 2 types of error(StatusErrorand plain error).In the previous code, �if the error returned from the handler is
StatusErroruse it for response body, else(in case of plainerror) instantiate response body using theNewErrorfunction. And then write response by calling thetransformAndWritefunction within the same file.Why do this?
I think if the returned from the handler is plain
error, it should be useNewErrorWithContextinstead ofNewError�to ensure consistent behavior within framework. �That's exactly why theNewErrorWithContextandWriteErrfunctions exist for this as I understand. Error handling in other part of the function already useWriteErr, so the code in this PR may be changed together.What will be changed?
Add
writeStatusErrorfunctionFor
StatusError, it works as same before using newwriteStatusErrorfunction.writeStatusErrorfunction acts likeWriteErrfunction include content negotitation, set status code, but the only difference is thatNewErrorWithContextdoes not create a response body, and the receivedStatusErrorby function parameter is used immediately for the response.Use
WriteErrfunction for plainerrorWriteErrfunction do the almost same thing withtransformAndWritefunction except thetransformAndWritefunction receive the response body by parameter. So I refactor this part to useWriteErrfunction.Expectations
NewErrorWIthContext, Huma can intercept handler error. It will be a great support in implementing global error handler using Huma middleware or Sentry alerting usingcontext.Contextof request.What �can do in next step?
I think the
StatusErrorhandling in the same part should also be able to usehuma.Contextso that every error inside Huma can be handled with consistent behavior.Summary by CodeRabbit
New Features
Bug Fixes
Documentation