Conversation
WalkthroughThis PR introduces a new public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
🧹 Nitpick comments (3)
werror/error_test.go (3)
61-114: Unused fieldcheckInternalin test struct.The
checkInternalfield is defined on line 68 and set totrueon line 88, but it's never used in the test logic. Either remove it or add the corresponding assertion.🔎 Either remove the unused field or use it:
tests := []struct { name string input interface{} wantNil bool wantCode string wantStatus int - checkInternal bool // check if it converted to InternalServerError }{Or add logic to use it:
if tt.checkInternal { if !errors.Is(werr, ErrInternalServerError) { t.Error("Expected conversion to InternalServerError") } }
165-172: Remove development notes from test code.These comments read like internal deliberation rather than test documentation. Consider removing or converting to a concise explanation of the test's intent.
🔎 Suggested cleanup:
- // Current implementation LEAKS the detail into Message. - // We test what it DOES, or what it SHOULD DO? - // User asked to build test "based on this version". - // If I test for correctness (no leak), it fails. - // If I test for current behavior (leak), it passes but solidifies bad practice. - - // I will check the properties are set, without asserting exact message content policy - // to avoid failing on policy disagreement, but print it. + // Verify code is set correctly; message composition is logged for visibility.
159-185: Consider adding a test case for nil base parameter.Given the PR's goal of fixing the "golang interface nil trap", adding test coverage for nil base scenarios in the constructors would help validate the fix and prevent regressions.
func TestNewErr_NilBase(t *testing.T) { // This test would fail currently but documents expected behavior defer func() { if r := recover(); r != nil { t.Error("NewErr panicked with nil base - nil trap not handled") } }() var nilBase WError = nil _ = NewErr(nilBase, "test", "detail") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.golangci.yml(0 hunks)werror/error.go(5 hunks)werror/error_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- .golangci.yml
🧰 Additional context used
🧬 Code graph analysis (1)
werror/error_test.go (1)
werror/error.go (6)
ErrInternalServerError(251-255)ToError(16-28)ErrBadRequest(180-180)ToErr(57-67)NewErrFromError(115-130)Err(42-53)
🔇 Additional comments (8)
werror/error.go (5)
17-19: Good: Nil guard added to ToError.This correctly addresses the nil input case, returning nil instead of falling through to the default case.
30-38: WError interface design looks good.The interface properly embeds
errorand includes the standardIs/Asmethods alongside the accessor methods. This allows polymorphic error handling while maintaining compatibility with the standard library's error wrapping conventions.
57-67: Good: Nil guard added to ToErr.This correctly handles the nil input case by returning nil early, preventing the interface nil trap when the input is explicitly nil.
136-141: Code-based equality is a reasonable semantic choice.The
Ismethod now compares error codes when the target is an*Err, which aligns with the intention of matching error categories rather than exact instances. The fallback toerrors.Ison the inner error preserves standard unwrapping behavior.
147-161: Accessor methods correctly implement the WError interface.These getters provide the necessary abstraction for the interface-based design.
werror/error_test.go (3)
17-59: Good test coverage for ToError.The table-driven tests cover the key branches: nil input, error type, Stringer type, and unknown type fallback.
116-146: Good coverage of Is semantics.The tests cover identity, wrapped error matching, self-matching, different codes, and inner error matching. This thoroughly validates the updated
Isimplementation.
148-157: Good basic As test.Validates that
errors.Ascorrectly extracts the*Errtype and preserves the code.
| func NewBaseErrFrom(base WError, code, msg string) WError { | ||
| if strings.TrimSpace(code) == "" { | ||
| code = base.Code | ||
| code = base.GetCode() | ||
| } | ||
| if strings.TrimSpace(msg) == "" { | ||
| msg = base.Message | ||
| msg = base.GetMessage() | ||
| } | ||
| err := &Err{ | ||
| error: fmt.Errorf("%w: %s %s", base.error, code, msg), | ||
| HttpStatus: base.HttpStatus, | ||
| error: fmt.Errorf("%w: %s %s", base, code, msg), | ||
| HttpStatus: base.GetHttpStatus(), | ||
| Code: code, | ||
| Message: msg, | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
Nil base parameter could still trigger the interface nil trap.
While the PR fixes nil handling in ToError/ToErr, this constructor does not guard against a nil base. If caller passes a typed nil (e.g., var e *Err = nil; NewBaseErrFrom(e, ...)) the interface value is non-nil but base.GetCode() will panic.
🔎 Consider adding a nil guard:
func NewBaseErrFrom(base WError, code, msg string) WError {
+ if base == nil {
+ base = ErrInternalServerError
+ }
if strings.TrimSpace(code) == "" {
code = base.GetCode()
}📝 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 NewBaseErrFrom(base WError, code, msg string) WError { | |
| if strings.TrimSpace(code) == "" { | |
| code = base.Code | |
| code = base.GetCode() | |
| } | |
| if strings.TrimSpace(msg) == "" { | |
| msg = base.Message | |
| msg = base.GetMessage() | |
| } | |
| err := &Err{ | |
| error: fmt.Errorf("%w: %s %s", base.error, code, msg), | |
| HttpStatus: base.HttpStatus, | |
| error: fmt.Errorf("%w: %s %s", base, code, msg), | |
| HttpStatus: base.GetHttpStatus(), | |
| Code: code, | |
| Message: msg, | |
| } | |
| return err | |
| } | |
| func NewBaseErrFrom(base WError, code, msg string) WError { | |
| if base == nil { | |
| base = ErrInternalServerError | |
| } | |
| if strings.TrimSpace(code) == "" { | |
| code = base.GetCode() | |
| } | |
| if strings.TrimSpace(msg) == "" { | |
| msg = base.GetMessage() | |
| } | |
| err := &Err{ | |
| error: fmt.Errorf("%w: %s %s", base, code, msg), | |
| HttpStatus: base.GetHttpStatus(), | |
| Code: code, | |
| Message: msg, | |
| } | |
| return err | |
| } |
🤖 Prompt for AI Agents
In werror/error.go around lines 80 to 94, NewBaseErrFrom currently calls
base.GetCode(), base.GetMessage() and base.GetHttpStatus() without guarding
against a typed nil interface which can panic; add a nil guard that treats base
as absent when either base == nil or reflect.ValueOf(base).IsNil() is true, and
only call base.GetCode()/GetMessage()/GetHttpStatus() when base is non-nil; when
base is absent, fall back to the provided code/msg (or sensible defaults) and
avoid calling any methods on base to prevent the interface nil trap.
| func NewErr(base WError, msg, msgDetail string) *Err { | ||
| msg = strings.TrimSpace(msg) | ||
| if msg == "" { | ||
| msg = base.Message | ||
| msg = base.GetMessage() | ||
| } | ||
| msgDetail = strings.TrimSpace(msgDetail) | ||
| if msgDetail != "" { | ||
| msg = msg + ": " + msgDetail | ||
| } | ||
| return &Err{ | ||
| error: fmt.Errorf("%w: %s", base.error, msg), | ||
| HttpStatus: base.HttpStatus, | ||
| Code: base.Code, | ||
| error: fmt.Errorf("%w: %s", base, msg), | ||
| HttpStatus: base.GetHttpStatus(), | ||
| Code: base.GetCode(), | ||
| Message: msg, | ||
| } | ||
| } |
There was a problem hiding this comment.
Same nil base risk applies here.
NewErr will panic if base is nil when calling base.GetMessage(), base.GetHttpStatus(), or base.GetCode().
🔎 Consider adding a nil guard:
func NewErr(base WError, msg, msgDetail string) *Err {
+ if base == nil {
+ base = ErrInternalServerError
+ }
msg = strings.TrimSpace(msg)| func NewErrFromError(base WError, err error) *Err { | ||
| msgDetail := err.Error() | ||
| werr := &Err{} | ||
| if errors.As(err, &werr) { | ||
| if werr.Code == base.Code && werr.Message == base.Message { | ||
| if werr.Code == base.GetCode() && werr.Message == base.GetMessage() { | ||
| return werr | ||
| } | ||
| msgDetail = werr.Message | ||
| } | ||
| return &Err{ | ||
| error: err, | ||
| HttpStatus: base.HttpStatus, | ||
| Code: base.Code, | ||
| Message: base.Message + ": " + msgDetail, | ||
| HttpStatus: base.GetHttpStatus(), | ||
| Code: base.GetCode(), | ||
| Message: base.GetMessage() + ": " + msgDetail, | ||
| } | ||
| } |
There was a problem hiding this comment.
Same nil base risk applies here.
NewErrFromError will panic if base is nil when accessing its methods.
🔎 Consider adding a nil guard:
func NewErrFromError(base WError, err error) *Err {
+ if base == nil {
+ base = ErrInternalServerError
+ }
msgDetail := err.Error()📝 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 NewErrFromError(base WError, err error) *Err { | |
| msgDetail := err.Error() | |
| werr := &Err{} | |
| if errors.As(err, &werr) { | |
| if werr.Code == base.Code && werr.Message == base.Message { | |
| if werr.Code == base.GetCode() && werr.Message == base.GetMessage() { | |
| return werr | |
| } | |
| msgDetail = werr.Message | |
| } | |
| return &Err{ | |
| error: err, | |
| HttpStatus: base.HttpStatus, | |
| Code: base.Code, | |
| Message: base.Message + ": " + msgDetail, | |
| HttpStatus: base.GetHttpStatus(), | |
| Code: base.GetCode(), | |
| Message: base.GetMessage() + ": " + msgDetail, | |
| } | |
| } | |
| func NewErrFromError(base WError, err error) *Err { | |
| if base == nil { | |
| base = ErrInternalServerError | |
| } | |
| msgDetail := err.Error() | |
| werr := &Err{} | |
| if errors.As(err, &werr) { | |
| if werr.Code == base.GetCode() && werr.Message == base.GetMessage() { | |
| return werr | |
| } | |
| msgDetail = werr.Message | |
| } | |
| return &Err{ | |
| error: err, | |
| HttpStatus: base.GetHttpStatus(), | |
| Code: base.GetCode(), | |
| Message: base.GetMessage() + ": " + msgDetail, | |
| } | |
| } |
🤖 Prompt for AI Agents
In werror/error.go around lines 115 to 130, NewErrFromError calls
base.GetCode/GetMessage/GetHttpStatus without guarding against base being nil
which will panic; add a nil check at the start (if base == nil) and handle it
deterministically (e.g., use default HttpStatus/Code/Message constants or return
a wrapped Err that indicates a nil base) before accessing base methods; keep
existing behavior for non-nil base and ensure the returned *Err uses the chosen
defaults when base is nil.
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
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.