diff --git a/plugins/base_capability.go b/plugins/base_capability.go index 7a67b146..6572a25e 100644 --- a/plugins/base_capability.go +++ b/plugins/base_capability.go @@ -119,17 +119,41 @@ type errorResponse interface { // checkErr returns an updated error if the response implements errorResponse and contains an error message. // If the response is nil, it returns the original error. Otherwise, it wraps or creates an error as needed. +// It also maps error strings to their corresponding api.Err* constants. func checkErr[T any](resp T, err error) (T, error) { if any(resp) == nil { - return resp, err + return resp, mapAPIError(err) } respErr, ok := any(resp).(errorResponse) if ok && respErr.GetError() != "" { - if err == nil { - err = errors.New(respErr.GetError()) - } else { - err = fmt.Errorf("%s: %w", respErr.GetError(), err) + respErrMsg := respErr.GetError() + respErrErr := errors.New(respErrMsg) + mappedErr := mapAPIError(respErrErr) + // Check if the error was mapped to an API error (different from the temp error) + if errors.Is(mappedErr, api.ErrNotImplemented) || errors.Is(mappedErr, api.ErrNotFound) { + // Return the mapped API error instead of wrapping + return resp, mappedErr } + // For non-API errors, use wrap the original error if it is not nil + return resp, errors.Join(respErrErr, err) + } + return resp, mapAPIError(err) +} + +// mapAPIError maps error strings to their corresponding api.Err* constants. +// This is needed as errors from plugins may not be of type api.Error, due to serialization/deserialization. +func mapAPIError(err error) error { + if err == nil { + return nil + } + + errStr := err.Error() + switch errStr { + case api.ErrNotImplemented.Error(): + return api.ErrNotImplemented + case api.ErrNotFound.Error(): + return api.ErrNotFound + default: + return err } - return resp, err } diff --git a/plugins/base_capability_test.go b/plugins/base_capability_test.go index da285079..3bece8dc 100644 --- a/plugins/base_capability_test.go +++ b/plugins/base_capability_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" + "github.com/navidrome/navidrome/plugins/api" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -34,7 +35,16 @@ var _ = Describe("baseCapability", func() { var _ = Describe("checkErr", func() { Context("when resp is nil", func() { - It("should return the original error unchanged", func() { + It("should return nil error when both resp and err are nil", func() { + var resp *testErrorResponse + + result, err := checkErr(resp, nil) + + Expect(result).To(BeNil()) + Expect(err).To(BeNil()) + }) + + It("should return original error unchanged for non-API errors", func() { var resp *testErrorResponse originalErr := errors.New("original error") @@ -44,13 +54,24 @@ var _ = Describe("checkErr", func() { Expect(err).To(Equal(originalErr)) }) - It("should return nil error when both resp and err are nil", func() { + It("should return mapped API error for ErrNotImplemented", func() { var resp *testErrorResponse + err := errors.New("plugin:not_implemented") - result, err := checkErr(resp, nil) + result, mappedErr := checkErr(resp, err) Expect(result).To(BeNil()) - Expect(err).To(BeNil()) + Expect(mappedErr).To(Equal(api.ErrNotImplemented)) + }) + + It("should return mapped API error for ErrNotFound", func() { + var resp *testErrorResponse + err := errors.New("plugin:not_found") + + result, mappedErr := checkErr(resp, err) + + Expect(result).To(BeNil()) + Expect(mappedErr).To(Equal(api.ErrNotFound)) }) }) @@ -94,7 +115,49 @@ var _ = Describe("checkErr", func() { result, err := checkErr(resp, originalErr) Expect(result).To(Equal(resp)) - Expect(err).To(MatchError("plugin error: original error")) + Expect(err).To(HaveOccurred()) + // Check that both error messages are present in the joined error + errStr := err.Error() + Expect(errStr).To(ContainSubstring("plugin error")) + Expect(errStr).To(ContainSubstring("original error")) + }) + + It("should return mapped API error for ErrNotImplemented when no original error", func() { + resp := &testErrorResponse{errorMsg: "plugin:not_implemented"} + + result, err := checkErr(resp, nil) + + Expect(result).To(Equal(resp)) + Expect(err).To(MatchError(api.ErrNotImplemented)) + }) + + It("should return mapped API error for ErrNotFound when no original error", func() { + resp := &testErrorResponse{errorMsg: "plugin:not_found"} + + result, err := checkErr(resp, nil) + + Expect(result).To(Equal(resp)) + Expect(err).To(MatchError(api.ErrNotFound)) + }) + + It("should return mapped API error for ErrNotImplemented even with original error", func() { + resp := &testErrorResponse{errorMsg: "plugin:not_implemented"} + originalErr := errors.New("original error") + + result, err := checkErr(resp, originalErr) + + Expect(result).To(Equal(resp)) + Expect(err).To(MatchError(api.ErrNotImplemented)) + }) + + It("should return mapped API error for ErrNotFound even with original error", func() { + resp := &testErrorResponse{errorMsg: "plugin:not_found"} + originalErr := errors.New("original error") + + result, err := checkErr(resp, originalErr) + + Expect(result).To(Equal(resp)) + Expect(err).To(MatchError(api.ErrNotFound)) }) }) @@ -106,7 +169,7 @@ var _ = Describe("checkErr", func() { result, err := checkErr(resp, originalErr) Expect(result).To(Equal(resp)) - Expect(err).To(Equal(originalErr)) + Expect(err).To(MatchError(originalErr)) }) It("should return nil error when both are empty/nil", func() { @@ -117,6 +180,16 @@ var _ = Describe("checkErr", func() { Expect(result).To(Equal(resp)) Expect(err).To(BeNil()) }) + + It("should map original API error when response error is empty", func() { + resp := &testErrorResponse{errorMsg: ""} + originalErr := errors.New("plugin:not_implemented") + + result, err := checkErr(resp, originalErr) + + Expect(result).To(Equal(resp)) + Expect(err).To(MatchError(api.ErrNotImplemented)) + }) }) Context("when resp does not implement errorResponse", func() { @@ -138,6 +211,16 @@ var _ = Describe("checkErr", func() { Expect(result).To(Equal(resp)) Expect(err).To(BeNil()) }) + + It("should map original API error when response doesn't implement errorResponse", func() { + resp := &testNonErrorResponse{data: "some data"} + originalErr := errors.New("plugin:not_found") + + result, err := checkErr(resp, originalErr) + + Expect(result).To(Equal(resp)) + Expect(err).To(MatchError(api.ErrNotFound)) + }) }) Context("when resp is a value type (not pointer)", func() { @@ -148,7 +231,11 @@ var _ = Describe("checkErr", func() { result, err := checkErr(resp, originalErr) Expect(result).To(Equal(resp)) - Expect(err).To(MatchError("value error: original error")) + Expect(err).To(HaveOccurred()) + // Check that both error messages are present in the joined error + errStr := err.Error() + Expect(errStr).To(ContainSubstring("value error")) + Expect(errStr).To(ContainSubstring("original error")) }) It("should handle value types with empty error", func() { @@ -158,7 +245,17 @@ var _ = Describe("checkErr", func() { result, err := checkErr(resp, originalErr) Expect(result).To(Equal(resp)) - Expect(err).To(Equal(originalErr)) + Expect(err).To(MatchError(originalErr)) + }) + + It("should handle value types with API error", func() { + resp := testValueErrorResponse{errorMsg: "plugin:not_implemented"} + originalErr := errors.New("original error") + + result, err := checkErr(resp, originalErr) + + Expect(result).To(Equal(resp)) + Expect(err).To(MatchError(api.ErrNotImplemented)) }) }) })