feat(werror): 添加param参数以及携带params参数的function函数#2
Conversation
- 实现 ConvertToWError 函数，内部使用go底层优化的断言进行类型判断 - 扩展 WError 接口，添加 SetDetails、GetParams、SetParams 和 SetMessage 方法 - 在 Err 结构体中添加 params 字段用于存储参数信息 - 实现 NewErrWithParams 函数，支持创建带参数的错误 - 添加 GetParams 和 SetParams 方法的完整实现 - 为 Err 结构体初始化 Details 和 params 字段为 nil
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 (3)
werror/error.go (3)
29-58: Consider handlingWErrorinterface implementations that aren't*Err.The function checks for
*Errconcrete type but doesn't handle cases whereximplements theWErrorinterface through a different type. If another type implementsWError, it will be wrapped as an internal server error rather than preserving its error semantics.Also, the comment on line 55 is in Chinese while other comments in the file are in English—consider using consistent language.
🔎 Proposed enhancement to handle WError interface
func ConvertToWError(x interface{}) *Err { if x == nil { return nil } if werr, ok := x.(*Err); ok { return werr } + // Handle other WError implementations + if werr, ok := x.(WError); ok { + return &Err{ + error: werr, + HttpStatus: werr.GetHttpStatus(), + Code: werr.GetCode(), + Message: werr.GetMessage(), + Details: werr.GetDetails(), + params: nil, + } + } + var rawErr error switch v := x.(type) { case error: rawErr = v default: rawErr = fmt.Errorf("%v", x) } detailErr := &Err{ Message: rawErr.Error(), } return &Err{ error: rawErr, HttpStatus: ErrInternalServerError.HttpStatus, Code: ErrInternalServerError.Code, Message: ErrInternalServerError.Message, - Details: []WError{detailErr}, // 有技术详情，非nil + Details: []WError{detailErr}, // Has technical details, non-nil params: nil, } }
218-220: Consider defensive copy for consistency withSetParams.
SetParamsmakes a defensive copy of the map, butSetDetailsassigns the slice directly. For consistency and to prevent external mutations, consider copying the slice.🔎 Proposed defensive copy
func (e *Err) SetDetails(details []WError) { - e.Details = details + if details == nil { + e.Details = nil + return + } + e.Details = make([]WError, len(details)) + copy(e.Details, details) }
233-242: Inconsistent nil handling inSetParams.When
paramsis nil,SetParamscreates an empty map rather than settinge.params = nil. This creates an inconsistency:
- After
NewBaseErr,GetParams()returnsnil- After
SetParams(nil),GetParams()returns an empty map{}If the intent is to clear params, consider setting to
nilinstead. If the intent is to ensure params is never nil after being set, document this behavior.🔎 Option A: Set to nil when nil is passed
func (e *Err) SetParams(params map[string]any) { if params == nil { - e.params = make(map[string]any) + e.params = nil return } e.params = make(map[string]any, len(params)) for k, v := range params { e.params[k] = v } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
werror/error.go
🔇 Additional comments (5)
werror/error.go (5)
86-86: Clarify whetherparamsshould be JSON-serializable.The
paramsfield lacks a JSON tag, so it won't be included in JSON serialization. If this is intentional (internal use only), consider adding a comment. Ifparamsshould appear in API error responses, addjson:"params,omitempty".
110-111: LGTM!Explicit initialization of
Detailsandparamstonilimproves code clarity.
212-214: LGTM!Simple and straightforward setter implementation.
222-231: LGTM!Good defensive copy implementation to prevent external mutations of the internal params map.
68-71: Adding methods to an existing interface is a breaking change for external implementations.Adding
SetDetails,GetParams,SetParams, andSetMessageto theWErrorinterface will break any external types implementing it. No internal implementations were found in the codebase, but external users with customWErrorimplementations will need to update their code.
| func NewErrWithParams(base *Err, code string, params map[string]any, msgDetail string) *Err { | ||
| if strings.TrimSpace(code) == "" { | ||
| code = base.Code | ||
| } | ||
| msg := base.Message | ||
|
|
||
| detailErr := &Err{ | ||
| Message: base.Message + msgDetail, | ||
| } | ||
| return &Err{ | ||
| error: fmt.Errorf("%w: %s", base.error, msg), | ||
| HttpStatus: base.HttpStatus, | ||
| Code: code, | ||
| Message: msg, | ||
| Details: []WError{detailErr}, | ||
| params: params, | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent behavior: params not defensively copied, and message handling differs from NewErr.
Two issues:
-
Direct map assignment (line 183):
paramsis assigned directly without copying, butSetParamsmakes a defensive copy. This inconsistency could lead to unexpected mutations if the caller modifies the map after creating the error. -
Message inconsistency: Unlike
NewErrwhich appendsmsgDetailto the outerMessagewith a:separator, heremsgDetailonly appears indetailErr.Message(line 175), and the outerMessage(line 181) excludes it. Also,detailErrconcatenates without a separator.
🔎 Proposed fix for consistency
func NewErrWithParams(base *Err, code string, params map[string]any, msgDetail string) *Err {
if strings.TrimSpace(code) == "" {
code = base.Code
}
msg := base.Message
+ msgDetail = strings.TrimSpace(msgDetail)
+ if msgDetail != "" {
+ msg = msg + ": " + msgDetail
+ }
detailErr := &Err{
- Message: base.Message + msgDetail,
+ Message: msg,
}
+
+ // Defensive copy of params
+ var paramsCopy map[string]any
+ if params != nil {
+ paramsCopy = make(map[string]any, len(params))
+ for k, v := range params {
+ paramsCopy[k] = v
+ }
+ }
+
return &Err{
error: fmt.Errorf("%w: %s", base.error, msg),
HttpStatus: base.HttpStatus,
Code: code,
- Message: msg,
+ Message: msg,
Details: []WError{detailErr},
- params: params,
+ params: paramsCopy,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewErrWithParams(base *Err, code string, params map[string]any, msgDetail string) *Err { | |
| if strings.TrimSpace(code) == "" { | |
| code = base.Code | |
| } | |
| msg := base.Message | |
| detailErr := &Err{ | |
| Message: base.Message + msgDetail, | |
| } | |
| return &Err{ | |
| error: fmt.Errorf("%w: %s", base.error, msg), | |
| HttpStatus: base.HttpStatus, | |
| Code: code, | |
| Message: msg, | |
| Details: []WError{detailErr}, | |
| params: params, | |
| } | |
| } | |
| func NewErrWithParams(base *Err, code string, params map[string]any, msgDetail string) *Err { | |
| if strings.TrimSpace(code) == "" { | |
| code = base.Code | |
| } | |
| msg := base.Message | |
| msgDetail = strings.TrimSpace(msgDetail) | |
| if msgDetail != "" { | |
| msg = msg + ": " + msgDetail | |
| } | |
| detailErr := &Err{ | |
| Message: msg, | |
| } | |
| // Defensive copy of params | |
| var paramsCopy map[string]any | |
| if params != nil { | |
| paramsCopy = make(map[string]any, len(params)) | |
| for k, v := range params { | |
| paramsCopy[k] = v | |
| } | |
| } | |
| return &Err{ | |
| error: fmt.Errorf("%w: %s", base.error, msg), | |
| HttpStatus: base.HttpStatus, | |
| Code: code, | |
| Message: msg, | |
| Details: []WError{detailErr}, | |
| params: paramsCopy, | |
| } | |
| } |
🤖 Prompt for AI Agents
In werror/error.go around lines 168-185, make NewErrWithParams consistent with
NewErr: defensively copy the incoming params map into a newly allocated map and
assign that copy to params to avoid external mutation; build the outer Message
by appending msgDetail to base.Message using the same ": " separator logic
NewErr uses (only when msgDetail is non-empty), set detailErr.Message
consistently (use the same concatenation pattern or the same base+detail
formatting used elsewhere), and update the fmt.Errorf call to include the final
outer message variable (not the original base.Message) so the returned error
text matches the Message field.
2 participants
Description
Type of Change
Checklist
Code of Conductand this pull request adheres to it.CONTRIBUTING.mdguide.make test-suite, and it passed with no errors.Review Process
Reviewees:
Reviewers:
This change is
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.