Skip to content

Commit 1166a0f

Browse files
deluandeluan
committed
fix(plugins): enhance error handling in checkErr function
Improved the error handling logic in the checkErr function to map specific error strings to their corresponding API error constants. This change ensures that errors from plugins are correctly identified and returned, enhancing the robustness of error reporting. Signed-off-by: Deluan <deluan@navidrome.org>
1 parent 9e97d0a commit 1166a0f

File tree

2 files changed

+135
-14
lines changed

2 files changed

+135
-14
lines changed

‎plugins/base_capability.go

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -119,17 +119,41 @@ type errorResponse interface {
119119

120120
// checkErr returns an updated error if the response implements errorResponse and contains an error message.
121121
// If the response is nil, it returns the original error. Otherwise, it wraps or creates an error as needed.
122+
// It also maps error strings to their corresponding api.Err* constants.
122123
func checkErr[T any](resp T, err error) (T, error) {
123124
if any(resp) == nil {
124-
return resp, err
125+
return resp, mapAPIError(err)
125126
}
126127
respErr, ok := any(resp).(errorResponse)
127128
if ok && respErr.GetError() != "" {
128-
if err == nil {
129-
err = errors.New(respErr.GetError())
130-
} else {
131-
err = fmt.Errorf("%s: %w", respErr.GetError(), err)
129+
respErrMsg := respErr.GetError()
130+
respErrErr := errors.New(respErrMsg)
131+
mappedErr := mapAPIError(respErrErr)
132+
// Check if the error was mapped to an API error (different from the temp error)
133+
if errors.Is(mappedErr, api.ErrNotImplemented) || errors.Is(mappedErr, api.ErrNotFound) {
134+
// Return the mapped API error instead of wrapping
135+
return resp, mappedErr
132136
}
137+
// For non-API errors, use wrap the original error if it is not nil
138+
return resp, errors.Join(respErrErr, err)
139+
}
140+
return resp, mapAPIError(err)
141+
}
142+
143+
// mapAPIError maps error strings to their corresponding api.Err* constants.
144+
// This is needed as errors from plugins may not be of type api.Error, due to serialization/deserialization.
145+
func mapAPIError(err error) error {
146+
if err == nil {
147+
return nil
148+
}
149+
150+
errStr := err.Error()
151+
switch errStr {
152+
case api.ErrNotImplemented.Error():
153+
return api.ErrNotImplemented
154+
case api.ErrNotFound.Error():
155+
return api.ErrNotFound
156+
default:
157+
return err
133158
}
134-
return resp, err
135159
}

‎plugins/base_capability_test.go

Lines changed: 105 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"errors"
66

7+
"github.com/navidrome/navidrome/plugins/api"
78
. "github.com/onsi/ginkgo/v2"
89
. "github.com/onsi/gomega"
910
)
@@ -34,7 +35,16 @@ var _ = Describe("baseCapability", func() {
3435

3536
var _ = Describe("checkErr", func() {
3637
Context("when resp is nil", func() {
37-
It("should return the original error unchanged", func() {
38+
It("should return nil error when both resp and err are nil", func() {
39+
var resp *testErrorResponse
40+
41+
result, err := checkErr(resp, nil)
42+
43+
Expect(result).To(BeNil())
44+
Expect(err).To(BeNil())
45+
})
46+
47+
It("should return original error unchanged for non-API errors", func() {
3848
var resp *testErrorResponse
3949
originalErr := errors.New("original error")
4050

@@ -44,13 +54,24 @@ var _ = Describe("checkErr", func() {
4454
Expect(err).To(Equal(originalErr))
4555
})
4656

47-
It("should return nil error when both resp and err are nil", func() {
57+
It("should return mapped API error for ErrNotImplemented", func() {
4858
var resp *testErrorResponse
59+
err := errors.New("plugin:not_implemented")
4960

50-
result, err := checkErr(resp, nil)
61+
result, mappedErr := checkErr(resp, err)
5162

5263
Expect(result).To(BeNil())
53-
Expect(err).To(BeNil())
64+
Expect(mappedErr).To(Equal(api.ErrNotImplemented))
65+
})
66+
67+
It("should return mapped API error for ErrNotFound", func() {
68+
var resp *testErrorResponse
69+
err := errors.New("plugin:not_found")
70+
71+
result, mappedErr := checkErr(resp, err)
72+
73+
Expect(result).To(BeNil())
74+
Expect(mappedErr).To(Equal(api.ErrNotFound))
5475
})
5576
})
5677

@@ -94,7 +115,49 @@ var _ = Describe("checkErr", func() {
94115
result, err := checkErr(resp, originalErr)
95116

96117
Expect(result).To(Equal(resp))
97-
Expect(err).To(MatchError("plugin error: original error"))
118+
Expect(err).To(HaveOccurred())
119+
// Check that both error messages are present in the joined error
120+
errStr := err.Error()
121+
Expect(errStr).To(ContainSubstring("plugin error"))
122+
Expect(errStr).To(ContainSubstring("original error"))
123+
})
124+
125+
It("should return mapped API error for ErrNotImplemented when no original error", func() {
126+
resp := &testErrorResponse{errorMsg: "plugin:not_implemented"}
127+
128+
result, err := checkErr(resp, nil)
129+
130+
Expect(result).To(Equal(resp))
131+
Expect(err).To(MatchError(api.ErrNotImplemented))
132+
})
133+
134+
It("should return mapped API error for ErrNotFound when no original error", func() {
135+
resp := &testErrorResponse{errorMsg: "plugin:not_found"}
136+
137+
result, err := checkErr(resp, nil)
138+
139+
Expect(result).To(Equal(resp))
140+
Expect(err).To(MatchError(api.ErrNotFound))
141+
})
142+
143+
It("should return mapped API error for ErrNotImplemented even with original error", func() {
144+
resp := &testErrorResponse{errorMsg: "plugin:not_implemented"}
145+
originalErr := errors.New("original error")
146+
147+
result, err := checkErr(resp, originalErr)
148+
149+
Expect(result).To(Equal(resp))
150+
Expect(err).To(MatchError(api.ErrNotImplemented))
151+
})
152+
153+
It("should return mapped API error for ErrNotFound even with original error", func() {
154+
resp := &testErrorResponse{errorMsg: "plugin:not_found"}
155+
originalErr := errors.New("original error")
156+
157+
result, err := checkErr(resp, originalErr)
158+
159+
Expect(result).To(Equal(resp))
160+
Expect(err).To(MatchError(api.ErrNotFound))
98161
})
99162
})
100163

@@ -106,7 +169,7 @@ var _ = Describe("checkErr", func() {
106169
result, err := checkErr(resp, originalErr)
107170

108171
Expect(result).To(Equal(resp))
109-
Expect(err).To(Equal(originalErr))
172+
Expect(err).To(MatchError(originalErr))
110173
})
111174

112175
It("should return nil error when both are empty/nil", func() {
@@ -117,6 +180,16 @@ var _ = Describe("checkErr", func() {
117180
Expect(result).To(Equal(resp))
118181
Expect(err).To(BeNil())
119182
})
183+
184+
It("should map original API error when response error is empty", func() {
185+
resp := &testErrorResponse{errorMsg: ""}
186+
originalErr := errors.New("plugin:not_implemented")
187+
188+
result, err := checkErr(resp, originalErr)
189+
190+
Expect(result).To(Equal(resp))
191+
Expect(err).To(MatchError(api.ErrNotImplemented))
192+
})
120193
})
121194

122195
Context("when resp does not implement errorResponse", func() {
@@ -138,6 +211,16 @@ var _ = Describe("checkErr", func() {
138211
Expect(result).To(Equal(resp))
139212
Expect(err).To(BeNil())
140213
})
214+
215+
It("should map original API error when response doesn't implement errorResponse", func() {
216+
resp := &testNonErrorResponse{data: "some data"}
217+
originalErr := errors.New("plugin:not_found")
218+
219+
result, err := checkErr(resp, originalErr)
220+
221+
Expect(result).To(Equal(resp))
222+
Expect(err).To(MatchError(api.ErrNotFound))
223+
})
141224
})
142225

143226
Context("when resp is a value type (not pointer)", func() {
@@ -148,7 +231,11 @@ var _ = Describe("checkErr", func() {
148231
result, err := checkErr(resp, originalErr)
149232

150233
Expect(result).To(Equal(resp))
151-
Expect(err).To(MatchError("value error: original error"))
234+
Expect(err).To(HaveOccurred())
235+
// Check that both error messages are present in the joined error
236+
errStr := err.Error()
237+
Expect(errStr).To(ContainSubstring("value error"))
238+
Expect(errStr).To(ContainSubstring("original error"))
152239
})
153240

154241
It("should handle value types with empty error", func() {
@@ -158,7 +245,17 @@ var _ = Describe("checkErr", func() {
158245
result, err := checkErr(resp, originalErr)
159246

160247
Expect(result).To(Equal(resp))
161-
Expect(err).To(Equal(originalErr))
248+
Expect(err).To(MatchError(originalErr))
249+
})
250+
251+
It("should handle value types with API error", func() {
252+
resp := testValueErrorResponse{errorMsg: "plugin:not_implemented"}
253+
originalErr := errors.New("original error")
254+
255+
result, err := checkErr(resp, originalErr)
256+
257+
Expect(result).To(Equal(resp))
258+
Expect(err).To(MatchError(api.ErrNotImplemented))
162259
})
163260
})
164261
})

0 commit comments

Comments
 (0)