feat: RFC 9457 compatible #10
Conversation
WalkthroughThis pull request introduces a comprehensive overhaul of error handling and response management across multiple files in the project. The changes primarily focus on implementing a standardized error response mechanism based on RFC 9457, replacing the previous custom error structures with a more structured Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (11)
validation.go (4)
20-23: Consider broader error handling.
You could handle multiple error types (e.g., parse errors) distinctly, but falling back to a default bad-request detail is acceptable as a baseline.
25-32: Enhance debugging by including invalid values.
Capturing the field's value may help with detailed error diagnoses. For instance, you could storevErr.Value()inValidationErrorDetailwhere appropriate.errorDetails[i] = ValidationErrorDetail{ Field: vErr.Field(), Message: formatValidationMessage(vErr), + // Example approach: + // Value: fmt.Sprintf("%v", vErr.Value()), }
34-43: Use a custom type URI for more precise documentation.
Currently,Typeis set to a placeholder ("https://example.com/validation-error"). Consider linking to a real URI where possible.
45-47: Refine message for user-friendliness.
Translating"failed <tag> validation"into a user-oriented error message (e.g.,"field is required") can improve usability.response.go (2)
44-59: Partial fallback for error response.
Whencode >= 400andproblem != nil, we correctly delegate towriteProblemDetail. However, consider a fallback ifproblemis unexpectedlynilto avoid returning an unstructured JSON.if code >= 400 && problem != nil { writeProblemDetail(w, code, problem) return +} else if code >= 400 { + // Provide a default problem detail for unhandled error cases + fallback := NewProblemDetails(code, "Unknown Error", "No details provided.") + writeProblemDetail(w, code, fallback) + return }
83-88: Check code vs. problem status consistency.
writeProblemDetailreceivescodebut usesproblem.StatusforWriteHeader. Ensuring these values match helps avoid confusion.validation_test.go (2)
32-35: Consider making validation error messages more maintainable.The hardcoded validation error messages might be brittle if the underlying validation library changes its message format.
Consider extracting these messages into constants or using the actual messages from the validator:
- {Field: "Name", Message: "Name failed required validation"}, - {Field: "Age", Message: "Age failed required validation"}, + {Field: "Name", Message: validate.Struct(request).(validator.ValidationErrors)[0].Error()}, + {Field: "Age", Message: validate.Struct(request).(validator.ValidationErrors)[1].Error()},
47-71: Add test cases for edge cases.The test cases cover basic validation scenarios but could be enhanced with edge cases.
Consider adding these test cases:
- Zero values (e.g., Age = 0)
- Values at the boundary (e.g., Age = 18)
- Special characters in the Name field
Example:
tests := []testCase[TestRequest]{ + { + name: "Zero Age", + request: TestValidationRequest{Name: "Alice", Age: 0}, + expectedProblem: &ProblemDetails{ + Type: "https://example.com/validation-error", + Title: "Validation Error", + Status: 400, + Detail: "One or more fields failed validation.", + Extensions: map[string]interface{}{ + "errors": []ValidationErrorDetail{ + {Field: "Age", Message: "Age failed gt validation"}, + }, + }, + }, + },request_test.go (1)
51-57: Consider centralizing error type URLs.The test cases use hardcoded URLs for error types, which could lead to inconsistencies.
Consider defining these URLs as package constants:
+const ( + ValidationErrorType = "https://example.com/validation-error" + InvalidRequestType = "https://example.com/invalid-request" +)request.go (2)
64-71: Enhance error type information for JSON decoding errors.The error type URL should be more specific for JSON decoding errors.
Consider using a specific error type:
- problem := NewProblemDetails(http.StatusBadRequest, "Invalid Request", err.Error()) + problem := NewProblemDetails(http.StatusBadRequest, "JSON Parsing Error", err.Error()) + problem.Type = "https://example.com/errors/json-parse-error"
78-91: Consider using specific error types for different parameter errors.The error types could be more specific to distinguish between missing parameters and parameter setting errors.
Consider using different error types:
- problem := NewProblemDetails(http.StatusBadRequest, "Missing Parameter", "Parameter "+key+" not found in request") + problem := NewProblemDetails(http.StatusBadRequest, "Missing Parameter", "Parameter "+key+" not found in request") + problem.Type = "https://example.com/errors/missing-parameter" - problem := NewProblemDetails(http.StatusInternalServerError, "Parameter Error", "Failed to set field "+key) + problem := NewProblemDetails(http.StatusInternalServerError, "Parameter Setting Error", "Failed to set field "+key) + problem.Type = "https://example.com/errors/parameter-setting-error"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
examples/chi/go.sumis excluded by!**/*.sumexamples/gorillamux/go.sumis excluded by!**/*.sumexamples/stdmux/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
examples/stdmux/go.mod(1 hunks)request.go(1 hunks)request_test.go(6 hunks)response.go(1 hunks)response_test.go(3 hunks)validation.go(1 hunks)validation_test.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/stdmux/go.mod
🔇 Additional comments (15)
validation.go (4)
5-6: Imports look consistent with usage.
No issues observed with adding"net/http"since we rely onhttp.StatusBadRequest.
10-13: Structured error detail is well-defined.
DefiningValidationErrorDetailwithFieldandMessageprovides clarity and is easy to extend if needed.
16-18: Documentation comprehensively explains the function.
The comments effectively describe how the function generatesProblemDetailsfrom validation errors.
51-56: Validation approach is straightforward.
Returningnilon success andProblemDetailson failure keeps the usage clear and aligns with the new structured error responses.response.go (5)
10-13: Clear separation of success data and metadata.
Removing error fields fromResponse[T]keeps it focused on success payloads, which simplifies maintenance.
18-19: Meta fields look appropriately defined.
These fields (Page, PageSize, etc.) support pagination use cases well.
24-32: Adhering to RFC 9457 for structured problem details.
DefiningProblemDetailsimproves standardization and clarity for error responses.
34-43: Factory function is concise and readable.
Creating a simpleNewProblemDetailsconstructor centralizes error instantiation and fosters consistency.
60-77: Success response flow is straightforward.
Encoding the success response intoResponse[T]is well-handled, followed by fallback toProblemDetailsif encoding fails.response_test.go (4)
20-20: Testing with a dedicatedproblemfield is clean.
Replacing the olderrsslice withproblemclarifies error handling validation.
35-37: Ensures robust 404 response testing.
Verifies that aProblemDetailsobject is sent for a not found scenario.
48-74: Validates complex error detail structures.
Testing a 400 response with an array of validation issues ensures comprehensive verification of the new format.
82-92: Content-type assertions are consistent.
Differentiating between"application/problem+json"for errors and"application/json"for success is aligned with RFC 9457 best practices.request_test.go (1)
16-20: LGTM! Good validation rule for the ID field.The
gt=0validation rule ensures that IDs are always positive integers, which is a good practice.request.go (1)
73-76: LGTM! Good nil handling implementation.The
isRequestNilhelper function and the initialization logic are well implemented, ensuring proper handling of nil requests.Also applies to: 103-106
rluders
force-pushed
the
feat/rfc-9457-compliance
branch
from
376d62b to
90bc8c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
validation.go (2)
16-43: Consider using a more specific error type URI.The current implementation uses a generic example URL. Consider using a more specific URI that points to your API documentation.
- Type: "https://example.com/validation-error", + Type: "https://api.httpsuite.dev/problems/validation-error",
45-47: Enhance validation message readability.The current message format is technical. Consider making it more user-friendly.
- return vErr.Field() + " failed " + vErr.Tag() + " validation" + switch vErr.Tag() { + case "required": + return vErr.Field() + " is required" + case "min": + return vErr.Field() + " must be at least " + vErr.Param() + default: + return vErr.Field() + " is invalid" + }response_test.go (1)
48-74: Add test cases for edge cases.Consider adding test cases for:
- Empty problem details
- Problem details with empty extensions
- Problem details with non-string values in extensions
validation_test.go (1)
26-36: Consider using table-driven tests for problem details assertions.The repeated assertions could be simplified using a helper function.
+func assertProblemDetails(t *testing.T, expected, actual *ProblemDetails) { + assert.Equal(t, expected.Type, actual.Type) + assert.Equal(t, expected.Title, actual.Title) + assert.Equal(t, expected.Status, actual.Status) + assert.Equal(t, expected.Detail, actual.Detail) + assert.ElementsMatch(t, expected.Extensions["errors"], actual.Extensions["errors"]) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
examples/chi/go.sumis excluded by!**/*.sumexamples/gorillamux/go.sumis excluded by!**/*.sumexamples/stdmux/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
examples/stdmux/go.mod(1 hunks)request.go(1 hunks)request_test.go(6 hunks)response.go(1 hunks)response_test.go(3 hunks)validation.go(1 hunks)validation_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/stdmux/go.mod
- request_test.go
🔇 Additional comments (4)
validation.go (1)
10-14: LGTM! Well-structured error detail type.The
ValidationErrorDetailstruct provides a clear and concise representation of validation errors with appropriate JSON tags.response_test.go (1)
87-91: LGTM! Good content type verification.The test correctly verifies different content types based on the response status code.
response.go (1)
25-33: LGTM! Well-documented ProblemDetails struct.The struct follows RFC 9457 specifications with clear documentation.
request.go (1)
103-106: LGTM! Robust nil check implementation.The
isRequestNilfunction correctly handles both nil interfaces and nil pointers.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
validation.go (1)
19-46: Consider making the problem type URL configurable.The function implementation is solid, but the hardcoded URL "https://example.com/validation-error" should be configurable to allow customization.
Consider this approach:
+// ValidationProblemType defines the URI reference for validation problems +var ValidationProblemType = "https://example.com/validation-error" + func NewValidationProblemDetails(err error) *ProblemDetails { // ... return &ProblemDetails{ - Type: "https://example.com/validation-error", + Type: ValidationProblemType, Title: "Validation Error", // ... } }response.go (2)
25-33: Consider adding validation for required fields.While the ProblemDetails struct follows RFC 9457, consider adding validation to ensure required fields (type, title, status) are not empty.
Consider this approach:
+// Validate ensures that required fields are present +func (p *ProblemDetails) Validate() error { + if p.Type == "" { + return errors.New("type is required") + } + if p.Title == "" { + return errors.New("title is required") + } + if p.Status == 0 { + return errors.New("status is required") + } + return nil +}
45-83: Enhance error handling for write operations.While the function handles encoding errors well, the final write operation's error handling could be improved.
Consider this approach:
// Send the success response w.Header().Set("Content-Type", "application/json; charset=utf-8") w.WriteHeader(code) -if _, err := w.Write(buffer.Bytes()); err != nil { +written, err := w.Write(buffer.Bytes()) +if err != nil { log.Printf("Failed to write response body (status=%d): %v", code, err) +} else if written != buffer.Len() { + log.Printf("Incomplete write: wrote %d of %d bytes", written, buffer.Len()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
request_test.go(6 hunks)response.go(1 hunks)validation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- request_test.go
🔇 Additional comments (6)
validation.go (3)
10-11: LGTM! Validator instance optimization implemented.The package-level validator instance improves performance by avoiding repeated instantiation.
13-17: LGTM! Well-structured validation error details.The struct provides a clear and RFC 9457-compliant structure for validation errors with proper documentation and JSON tags.
54-61: LGTM! Clean and consistent validation implementation.The function effectively uses the package-level validator and follows the new RFC 9457-compliant error handling pattern.
response.go (3)
13-14: LGTM! Clean separation of success and error responses.The removal of the Errors field in favor of RFC 9457's ProblemDetails provides a clear distinction between success and error responses.
35-43: LGTM! RFC-compliant problem details creation.The function correctly uses "about:blank" as the default type, following RFC 9457 section 4.2.1.
85-91: LGTM! Improved error handling in writeProblemDetail.The function now properly handles encoding errors as suggested in the previous review.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
examples/gorillamux/main.go (1)
45-46: Consider making the base URL configurable.While hardcoding is acceptable for an example, in a real application, the base URL should be configurable through environment variables or configuration files.
- // Define the ProblemBaseURL - doesn't create the handlers - httpsuite.SetProblemBaseURL("http://localhost:8080") + // Define the ProblemBaseURL from configuration + baseURL := os.Getenv("PROBLEM_BASE_URL") + if baseURL == "" { + baseURL = "http://localhost:8080" // fallback for example + } + httpsuite.SetProblemBaseURL(baseURL)problem_details_test.go (2)
35-62: Consider adding edge case tests.While the current test cases are good, consider adding tests for:
- Empty path
- Path with special characters
- Path normalization (e.g., multiple slashes)
{ name: "Override existing path", errorKey: "validation_error", path: "/errors/new-validation-error", expected: "/errors/new-validation-error", }, + { + name: "Empty path", + errorKey: "empty_path", + path: "", + expected: "", + }, + { + name: "Path with special characters", + errorKey: "special_chars", + path: "/errors/special@#$", + expected: "/errors/special@#$", + }, + { + name: "Path with multiple slashes", + errorKey: "multiple_slashes", + path: "/errors//double///triple", + expected: "/errors/double/triple", + },
64-92: Consider testing with different base URL formats.Add test cases for:
- Base URL with trailing slash
- Base URL without scheme
- Base URL with query parameters
{ name: "Unknown error type", errorType: "unknown_error", expectedURL: BlankUrl, }, + { + name: "Base URL with trailing slash", + errorType: "validation_error", + expectedURL: "https://api.example.com//errors/validation-error", + setup: func() { + SetProblemBaseURL("https://api.example.com/") + }, + }, + { + name: "Base URL without scheme", + errorType: "validation_error", + expectedURL: "//api.example.com/errors/validation-error", + setup: func() { + SetProblemBaseURL("//api.example.com") + }, + },problem_details.go (1)
109-115: Optimize GetProblemTypeURL implementation.The function can be more efficient by avoiding multiple map lookups and mutex acquisitions.
Consider this optimization:
func GetProblemTypeURL(errorType string) string { + mu.RLock() + defer mu.RUnlock() if path, exists := errorTypePaths[errorType]; exists { - return getProblemBaseURL() + path + if problemBaseURL == BlankUrl { + return BlankUrl + } + return problemBaseURL + path } return BlankUrl }validation.go (2)
54-56: Enhance validation error messages.The current error message format is basic and could be more user-friendly.
Consider implementing a more descriptive message format:
func formatValidationMessage(vErr validator.FieldError) string { - return vErr.Field() + " failed " + vErr.Tag() + " validation" + switch vErr.Tag() { + case "required": + return vErr.Field() + " is required" + case "email": + return vErr.Field() + " must be a valid email address" + case "min": + return vErr.Field() + " must be at least " + vErr.Param() + case "max": + return vErr.Field() + " must not exceed " + vErr.Param() + default: + return vErr.Field() + " failed " + vErr.Tag() + " validation" + } }
42-49: Consider adding a validation count to the error response.Adding the total number of validation errors could be helpful for clients.
return &ProblemDetails{ Type: GetProblemTypeURL("validation_error"), Title: "Validation Error", Status: http.StatusBadRequest, - Detail: "One or more fields failed validation.", + Detail: fmt.Sprintf("%d field(s) failed validation.", len(errorDetails)), Extensions: map[string]interface{}{ "errors": errorDetails, + "total_errors": len(errorDetails), }, }response.go (1)
70-76: Consider adding retry logic for encoding errors.The current implementation only logs encoding errors but doesn't attempt to recover.
Consider implementing a fallback mechanism:
func writeProblemDetail(w http.ResponseWriter, code int, problem *ProblemDetails) { w.Header().Set("Content-Type", "application/problem+json; charset=utf-8") w.WriteHeader(problem.Status) - if err := json.NewEncoder(w).Encode(problem); err != nil { - log.Printf("Failed to encode problem details: %v", err) + if err := json.NewEncoder(w).Encode(problem); err != nil { + log.Printf("Failed to encode problem details: %v", err) + // Fallback to a simplified error response + fallback := map[string]interface{}{ + "type": problem.Type, + "title": problem.Title, + "status": problem.Status, + } + if err := json.NewEncoder(w).Encode(fallback); err != nil { + log.Printf("Failed to encode fallback error response: %v", err) + } } }response_test.go (1)
Line range hint
14-24: Add edge case tests for error scenarios.Consider adding test cases for:
- Internal server errors with encoding failures
- Empty problem details
- Maximum size responses
Example test case to add:
tests := []struct { name string code int data any problem *ProblemDetails meta *Meta expectedCode int expectedJSON string -}{ +}{ + { + name: "500 Internal Server Error with empty problem details", + code: http.StatusInternalServerError, + problem: NewProblemDetails( + http.StatusInternalServerError, + "", + "", + "", + ), + expectedCode: http.StatusInternalServerError, + expectedJSON: `{ + "type": "about:blank", + "title": "", + "status": 500 + }`, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
examples/chi/main.go(1 hunks)examples/gorillamux/main.go(1 hunks)examples/stdmux/main.go(1 hunks)problem_details.go(1 hunks)problem_details_test.go(1 hunks)request.go(1 hunks)request_test.go(6 hunks)response.go(1 hunks)response_test.go(2 hunks)validation.go(1 hunks)validation_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- validation_test.go
- request_test.go
🔇 Additional comments (8)
examples/stdmux/main.go (1)
49-50: Same configuration suggestion applies here.Similar to the gorillamux example, consider making the base URL configurable in a real application.
examples/chi/main.go (1)
55-56: Same configuration suggestion applies here.Similar to other examples, consider making the base URL configurable in a real application.
problem_details_test.go (1)
9-33: LGTM! Well-structured table-driven tests.The test cases effectively cover both valid and blank URL scenarios.
problem_details.go (1)
13-21: LGTM! Well-structured implementation of RFC 9457.The ProblemDetails struct is well-documented and correctly implements the RFC specification with appropriate JSON tags and extensibility support.
request.go (4)
64-75: Consider using http.MaxBytesReader for request body.Add protection against large request bodies to prevent potential DoS attacks.
I see that issue #11 has been created to track this investigation. Let me know if you need any assistance with implementing the solution once the investigation is complete.
78-81: LGTM! Good defensive programming.The initialization check ensures robust handling of requests without a JSON body.
110-114: LGTM! Clean error handling.The validation error handling is simplified and aligns well with the RFC 9457 specifications.
119-122: LGTM! Well-structured helper function.The function effectively encapsulates the nil check logic and handles both nil and uninitialized pointer cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
request.go (1)
119-122: Add return value documentation.Consider adding documentation about the return value to improve clarity:
// isRequestNil checks if a request object is nil or an uninitialized pointer. +// Returns true if the object is nil or an uninitialized pointer, false otherwise. func isRequestNil(i interface{}) bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
problem_details.go(1 hunks)request.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- problem_details.go
🔇 Additional comments (4)
request.go (4)
64-74: LGTM! Improved error handling with RFC 9457 compatibility.The error handling for JSON decoding now properly implements the RFC 9457 specification using ProblemDetails structure, providing more detailed and standardized error responses.
78-81: LGTM! Robust request initialization.The request initialization logic properly handles nil cases and uses reflection appropriately to create new instances.
83-105: LGTM! Comprehensive parameter handling with detailed error responses.The URL parameter handling now includes:
- Structured error responses using ProblemDetails
- Additional error context in the Extensions map
- Clear error messages for missing or invalid parameters
110-114: LGTM! Simplified validation error handling.The validation error handling is streamlined and properly integrated with the RFC 9457 compliant response structure.
|
If .Title is empty you could populate it based on .Status with https://pkg.go.dev/net/http#StatusText |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
problem_details.go (3)
21-29: Enhance Extensions field documentation.The Extensions field could benefit from more detailed documentation about its purpose and usage patterns.
- Extensions map[string]interface{} `json:"extensions,omitempty"` // Custom fields for additional details. + Extensions map[string]interface{} `json:"extensions,omitempty"` // Custom fields for additional problem-specific details as per RFC 9457 Section 3.2.
31-48: Add input validation for HTTP status code.The constructor should validate that the status code is within the valid HTTP status code range.
func NewProblemDetails(status int, problemType, title, detail string) *ProblemDetails { + if status < 100 || status > 599 { + status = http.StatusInternalServerError + } if problemType == "" { problemType = BlankUrl }
50-117: Add input validation for URLs and paths.The configuration functions should validate their input parameters to ensure they meet the expected format.
+import ( + "net/http" + "net/url" + "strings" +) func SetProblemBaseURL(baseURL string) { + if baseURL != "" { + if _, err := url.Parse(baseURL); err != nil { + return + } + baseURL = strings.TrimRight(baseURL, "/") + } mu.Lock() defer mu.Unlock() problemBaseURL = baseURL } func SetProblemErrorTypePath(errorType, path string) { + if !strings.HasPrefix(path, "/") { + path = "/" + path + } mu.Lock() defer mu.Unlock() errorTypePaths[errorType] = path }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
problem_details.go(1 hunks)
🔇 Additional comments (1)
problem_details.go (1)
1-19: LGTM! Well-structured package with proper thread safety.The package structure follows Go conventions, and thread safety concerns have been properly addressed using sync.RWMutex.
This PR implements the RFC 9457 to the Response helper when handling errors, adding the
ProblemDetailsto the response and also changing theContent-typetoapplication/problem+json; charset=utf-8.Extras
Fixes #9
Summary by CodeRabbit
New Features
Bug Fixes
Refactor