refactor: refactoring http error handling #254
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors HTTP error handling throughout the codebase to provide better control over HTTP status codes in error responses. The refactoring introduces new error handling functions that can attach HTTP status codes directly to errors, and updates the panic recovery middleware to extract and use these codes when responding to panicked requests.
Key Changes:
- Introduced
ErrorWithHTTPCodeandNewWithHTTPCodefunctions to create errors with HTTP status codes - Updated panic recovery handler to extract HTTP codes from errors
- Added context parameter to
EntityProviderinterface methods for better request tracing - Refactored
OrganizationPrincipalschema and introducedOrganizationPrincipalCache - Added comprehensive test coverage for new Elasticsearch document handling functions
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| core/errors/error.go | Added HTTP code support to error types and functions; refactored Cause/Code functions |
| core/api/recovery.go | Updated panic handler to extract and use HTTP codes from errors |
| core/api/util.go | Added helper functions for common HTTP error responses (401, 403, 400) |
| core/util/http.go | Updated to use new ErrorWithCode function name |
| core/security/*.go | Updated error handling to use HTTP code errors; added context to functions |
| core/entity_card/api.go | Added context parameter to entity provider interface methods |
| core/orm/*.go | Added debug-mode panics for validation errors; added system fields support |
| core/elastic/*.go | Added generic document helpers and tests; refactored response structures |
| core/global/register.go | Added InitialDelay field to BackgroundTask for better task scheduling |
| modules/security/rbac/*.go | Updated to use context in entity providers; refactored principal caching |
| modules/elastic/schema.go | Consolidated log messages for better readability |
| modules/queue/disk_queue/module.go | Fixed formatting (spacing after else) |
| docs/content.en/docs/release-notes/_index.md | Added release note for principal cache refactoring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/errors/error.go
Outdated
| if err != nil { | ||
| cause, ok := err.(causer) | ||
| if !ok { | ||
| break | ||
| if ok { | ||
| err = cause.Cause() | ||
| } | ||
| err = cause.Cause() | ||
| } | ||
| return err |
There was a problem hiding this comment.
The refactored Cause() function only unwraps one level of error causes, whereas the original implementation would recursively unwrap all nested causes until reaching the root cause. This changes the behavior of the function and may break code that depends on getting the root cause. Consider restoring the loop to maintain backward compatibility:
for err != nil {
cause, ok := err.(causer)
if !ok {
break
}
err = cause.Cause()
}| v := u.Provider != "" && u.Login != "" && u.UserID != "" | ||
| if !v { | ||
| if global.Env().IsDebug { | ||
| log.Error(util.MustToJSON(u), u.UserID) |
There was a problem hiding this comment.
Incorrect indentation for the log.Error statement. It should be indented to be inside the if block.
| log.Error(util.MustToJSON(u), u.UserID) | |
| log.Error(util.MustToJSON(u), u.UserID) |
| if global.Env().IsDebug { | ||
| panic(errors.NewWithHTTPCode(401, "user not found")) | ||
| } | ||
|
|
There was a problem hiding this comment.
Inconsistent behavior between debug and non-debug modes. In debug mode, the function panics when user is not found, but in non-debug mode it returns an error. This makes the function behavior unpredictable and can cause issues when moving code between environments. Consider either always panicking (since the caller can use MustGetUserFromContext for that) or always returning an error consistently.
| if global.Env().IsDebug { | |
| panic(errors.NewWithHTTPCode(401, "user not found")) | |
| } |
core/errors/error.go
Outdated
| if err != nil { | ||
| cause, ok := err.(causer) | ||
| if ok { | ||
| code = cause.Code() | ||
| } | ||
| } | ||
| return code |
There was a problem hiding this comment.
The refactored Code() function only checks one level of error wrapping, whereas the original implementation would loop through all nested causes to find an error code. This changes the behavior and may not find the error code when errors are wrapped multiple times. Consider restoring the loop to maintain backward compatibility:
for err != nil {
cause, ok := err.(causer)
if !ok {
break
}
code = cause.Code()
err = cause.Cause()
}
core/errors/error.go
Outdated
| if err != nil { | ||
| cause, ok := err.(causer) | ||
| if !ok { | ||
| break | ||
| if ok { | ||
| code = cause.HTTPCode() | ||
| } | ||
| code = cause.Code() | ||
| } | ||
| return code |
There was a problem hiding this comment.
The HTTPCode() function only checks one level of error wrapping. For consistency with how the original Code() and Cause() functions worked, this should loop through nested causes to find the HTTP code. Consider implementing it similarly:
for err != nil {
cause, ok := err.(causer)
if !ok {
break
}
code = cause.HTTPCode()
err = cause.Cause()
}This ensures HTTP codes are properly extracted from deeply nested error chains.
What does this PR do
Rationale for this change
Standards checklist