Skip to content

Commit

Permalink
Add errorlint linter
Browse files Browse the repository at this point in the history
  • Loading branch information
deluan committed Sep 30, 2022
1 parent db67c12 commit 77dbaff
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 23 deletions.
1 change: 1 addition & 0 deletions .golangci.yml
Expand Up @@ -7,6 +7,7 @@ linters:
- depguard
- dogsled
- errcheck
- errorlint
- exportloopref
- gocyclo
- goprintffuncname
Expand Down
14 changes: 10 additions & 4 deletions core/agents/lastfm/agent.go
Expand Up @@ -2,6 +2,7 @@ package lastfm

import (
"context"
"errors"
"net/http"

"github.com/navidrome/navidrome/conf"
Expand Down Expand Up @@ -118,7 +119,9 @@ func (l *lastfmAgent) GetTopSongs(ctx context.Context, id, artistName, mbid stri

func (l *lastfmAgent) callArtistGetInfo(ctx context.Context, name string, mbid string) (*Artist, error) {
a, err := l.client.ArtistGetInfo(ctx, name, mbid)
lfErr, isLastFMError := err.(*lastFMError)
var lfErr *lastFMError
isLastFMError := errors.As(err, &lfErr)

if mbid != "" && ((err == nil && a.Name == "[unknown]") || (isLastFMError && lfErr.Code == 6)) {
log.Warn(ctx, "LastFM/artist.getInfo could not find artist by mbid, trying again", "artist", name, "mbid", mbid)
return l.callArtistGetInfo(ctx, name, "")
Expand All @@ -133,7 +136,8 @@ func (l *lastfmAgent) callArtistGetInfo(ctx context.Context, name string, mbid s

func (l *lastfmAgent) callArtistGetSimilar(ctx context.Context, name string, mbid string, limit int) ([]Artist, error) {
s, err := l.client.ArtistGetSimilar(ctx, name, mbid, limit)
lfErr, isLastFMError := err.(*lastFMError)
var lfErr *lastFMError
isLastFMError := errors.As(err, &lfErr)
if mbid != "" && ((err == nil && s.Attr.Artist == "[unknown]") || (isLastFMError && lfErr.Code == 6)) {
log.Warn(ctx, "LastFM/artist.getSimilar could not find artist by mbid, trying again", "artist", name, "mbid", mbid)
return l.callArtistGetSimilar(ctx, name, "", limit)
Expand All @@ -147,7 +151,8 @@ func (l *lastfmAgent) callArtistGetSimilar(ctx context.Context, name string, mbi

func (l *lastfmAgent) callArtistGetTopTracks(ctx context.Context, artistName, mbid string, count int) ([]Track, error) {
t, err := l.client.ArtistGetTopTracks(ctx, artistName, mbid, count)
lfErr, isLastFMError := err.(*lastFMError)
var lfErr *lastFMError
isLastFMError := errors.As(err, &lfErr)
if mbid != "" && ((err == nil && t.Attr.Artist == "[unknown]") || (isLastFMError && lfErr.Code == 6)) {
log.Warn(ctx, "LastFM/artist.getTopTracks could not find artist by mbid, trying again", "artist", artistName, "mbid", mbid)
return l.callArtistGetTopTracks(ctx, artistName, "", count)
Expand Down Expand Up @@ -204,7 +209,8 @@ func (l *lastfmAgent) Scrobble(ctx context.Context, userId string, s scrobbler.S
if err == nil {
return nil
}
lfErr, isLastFMError := err.(*lastFMError)
var lfErr *lastFMError
isLastFMError := errors.As(err, &lfErr)
if !isLastFMError {
log.Warn(ctx, "Last.fm client.scrobble returned error", "track", s.Title, err)
return scrobbler.ErrRetryLater
Expand Down
4 changes: 3 additions & 1 deletion core/agents/listenbrainz/agent.go
Expand Up @@ -2,6 +2,7 @@ package listenbrainz

import (
"context"
"errors"
"net/http"

"github.com/navidrome/navidrome/conf"
Expand Down Expand Up @@ -88,7 +89,8 @@ func (l *listenBrainzAgent) Scrobble(ctx context.Context, userId string, s scrob
if err == nil {
return nil
}
lbErr, isListenBrainzError := err.(*listenBrainzError)
var lbErr *listenBrainzError
isListenBrainzError := errors.As(err, &lbErr)
if !isListenBrainzError {
log.Warn(ctx, "ListenBrainz Scrobble returned HTTP error", "track", s.Title, err)
return scrobbler.ErrRetryLater
Expand Down
25 changes: 17 additions & 8 deletions persistence/user_repository_test.go
Expand Up @@ -95,7 +95,8 @@ var _ = Describe("UserRepository", func() {
user := *loggedUser
user.NewPassword = "new"
err := validatePasswordChange(&user, loggedUser)
verr := err.(*rest.ValidationError)
var verr *rest.ValidationError
errors.As(err, &verr)
Expect(verr.Errors).To(HaveLen(1))
Expect(verr.Errors).To(HaveKeyWithValue("currentPassword", "ra.validation.required"))
})
Expand All @@ -104,7 +105,8 @@ var _ = Describe("UserRepository", func() {
user := *loggedUser
user.CurrentPassword = "abc123"
err := validatePasswordChange(&user, loggedUser)
verr := err.(*rest.ValidationError)
var verr *rest.ValidationError
errors.As(err, &verr)
Expect(verr.Errors).To(HaveLen(1))
Expect(verr.Errors).To(HaveKeyWithValue("password", "ra.validation.required"))
})
Expand All @@ -114,7 +116,8 @@ var _ = Describe("UserRepository", func() {
user.CurrentPassword = "current"
user.NewPassword = "new"
err := validatePasswordChange(&user, loggedUser)
verr := err.(*rest.ValidationError)
var verr *rest.ValidationError
errors.As(err, &verr)
Expect(verr.Errors).To(HaveLen(1))
Expect(verr.Errors).To(HaveKeyWithValue("currentPassword", "ra.validation.passwordDoesNotMatch"))
})
Expand All @@ -136,7 +139,8 @@ var _ = Describe("UserRepository", func() {
user := *loggedUser
user.NewPassword = "new"
err := validatePasswordChange(&user, loggedUser)
verr := err.(*rest.ValidationError)
var verr *rest.ValidationError
errors.As(err, &verr)
Expect(verr.Errors).To(HaveLen(1))
Expect(verr.Errors).To(HaveKeyWithValue("currentPassword", "ra.validation.required"))
})
Expand All @@ -145,7 +149,8 @@ var _ = Describe("UserRepository", func() {
user := *loggedUser
user.CurrentPassword = "abc123"
err := validatePasswordChange(&user, loggedUser)
verr := err.(*rest.ValidationError)
var verr *rest.ValidationError
errors.As(err, &verr)
Expect(verr.Errors).To(HaveLen(1))
Expect(verr.Errors).To(HaveKeyWithValue("password", "ra.validation.required"))
})
Expand All @@ -155,7 +160,8 @@ var _ = Describe("UserRepository", func() {
user.CurrentPassword = "current"
user.NewPassword = "new"
err := validatePasswordChange(&user, loggedUser)
verr := err.(*rest.ValidationError)
var verr *rest.ValidationError
errors.As(err, &verr)
Expect(verr.Errors).To(HaveLen(1))
Expect(verr.Errors).To(HaveKeyWithValue("currentPassword", "ra.validation.passwordDoesNotMatch"))
})
Expand Down Expand Up @@ -186,8 +192,11 @@ var _ = Describe("UserRepository", func() {
It("returns ValidationError if username already exists", func() {
var newUser = &model.User{ID: "2", UserName: "johndoe"}
err := validateUsernameUnique(repo, newUser)
Expect(err).To(BeAssignableToTypeOf(&rest.ValidationError{}))
Expect(err.(*rest.ValidationError).Errors).To(HaveKeyWithValue("userName", "ra.validation.unique"))
var verr *rest.ValidationError
isValidationError := errors.As(err, &verr)

Expect(isValidationError).To(BeTrue())
Expect(verr.Errors).To(HaveKeyWithValue("userName", "ra.validation.unique"))
})
It("returns generic error if repository call fails", func() {
repo.Error = errors.New("fake error")
Expand Down
25 changes: 18 additions & 7 deletions server/subsonic/album_lists_test.go
Expand Up @@ -2,6 +2,7 @@ package subsonic

import (
"context"
"errors"
"net/http/httptest"

"github.com/navidrome/navidrome/server/subsonic/responses"
Expand Down Expand Up @@ -46,9 +47,12 @@ var _ = Describe("AlbumListController", func() {
It("should fail if missing type parameter", func() {
r := newGetRequest()
_, err := controller.GetAlbumList(w, r)
var subErr subError
isSubError := errors.As(err, &subErr)

Expect(err).To(MatchError("required 'type' parameter is missing"))
Expect(err.(subError).code).To(Equal(responses.ErrorMissingParameter))
Expect(isSubError).To(BeTrue())
Expect(subErr).To(MatchError("required 'type' parameter is missing"))
Expect(subErr.code).To(Equal(responses.ErrorMissingParameter))
})

It("should return error if call fails", func() {
Expand All @@ -58,7 +62,9 @@ var _ = Describe("AlbumListController", func() {
_, err := controller.GetAlbumList(w, r)

Expect(err).ToNot(BeNil())
Expect(err.(subError).code).To(Equal(responses.ErrorGeneric))
var subErr subError
errors.As(err, &subErr)
Expect(subErr.code).To(Equal(responses.ErrorGeneric))
})
})

Expand All @@ -82,8 +88,11 @@ var _ = Describe("AlbumListController", func() {
r := newGetRequest()
_, err := controller.GetAlbumList2(w, r)

Expect(err).To(MatchError("required 'type' parameter is missing"))
Expect(err.(subError).code).To(Equal(responses.ErrorMissingParameter))
var subErr subError
errors.As(err, &subErr)

Expect(subErr).To(MatchError("required 'type' parameter is missing"))
Expect(subErr.code).To(Equal(responses.ErrorMissingParameter))
})

It("should return error if call fails", func() {
Expand All @@ -92,8 +101,10 @@ var _ = Describe("AlbumListController", func() {

_, err := controller.GetAlbumList2(w, r)

Expect(err).ToNot(BeNil())
Expect(err.(subError).code).To(Equal(responses.ErrorGeneric))
var subErr subError
errors.As(err, &subErr)
Expect(subErr).ToNot(BeNil())
Expect(subErr.code).To(Equal(responses.ErrorGeneric))
})
})
})
8 changes: 5 additions & 3 deletions server/subsonic/api.go
Expand Up @@ -183,7 +183,8 @@ func h(r chi.Router, path string, f handler) {
res, err := f(w, r)
if err != nil {
// If it is not a Subsonic error, convert it to an ErrorGeneric
if _, ok := err.(subError); !ok {
var subErr subError
if !errors.As(err, &subErr) {
if errors.Is(err, model.ErrNotFound) {
err = newError(responses.ErrorDataNotFound, "data not found")
} else {
Expand Down Expand Up @@ -233,8 +234,9 @@ func h410(r chi.Router, paths ...string) {
func sendError(w http.ResponseWriter, r *http.Request, err error) {
response := newResponse()
code := responses.ErrorGeneric
if e, ok := err.(subError); ok {
code = e.code
var subErr subError
if errors.As(err, &subErr) {
code = subErr.code
}
response.Status = "failed"
response.Error = &responses.Error{Code: code, Message: err.Error()}
Expand Down

0 comments on commit 77dbaff

Please sign in to comment.