Drop usage of the SkinNotFoundError and CapeNotFoundError

More accurate redis results checking
Return correct errors from filesystem db driver

db/filesystem.go

@@ -49,13 +49,17 @@ type filesStorage struct {
 
 func (repository *filesStorage) FindByUsername(username string) (*model.Cape, error) {
 	if username == "" {
-		return nil, &http.CapeNotFoundError{Who: username}
+		return nil, nil
 	}
 
 	capePath := path.Join(repository.path, strings.ToLower(username)+".png")
 	file, err := os.Open(capePath)
 	if err != nil {
-		return nil, &http.CapeNotFoundError{Who: username}
+		if os.IsNotExist(err) {
+			return nil, nil
+		}
+
+		return nil, err
 	}
 
 	return &model.Cape{

db/redis.go

@@ -5,7 +5,6 @@ import (
 	"compress/zlib"
 	"encoding/json"
 	"fmt"
-	"github.com/elyby/chrly/http"
 	"io"
 	"strconv"
 	"strings"
@@ -15,6 +14,7 @@ import (
 	"github.com/mediocregopher/radix.v2/redis"
 	"github.com/mediocregopher/radix.v2/util"
 
+	"github.com/elyby/chrly/http"
 	"github.com/elyby/chrly/model"
 	"github.com/elyby/chrly/mojangtextures"
 )
@@ -147,14 +147,10 @@ func (db *redisDb) StoreUuid(username string, uuid string) error {
 }
 
 func findByUsername(username string, conn util.Cmder) (*model.Skin, error) {
-	if username == "" {
-		return nil, &http.SkinNotFoundError{Who: username}
-	}
-
 	redisKey := buildUsernameKey(username)
 	response := conn.Cmd("GET", redisKey)
-	if !response.IsType(redis.Str) {
-		return nil, &http.SkinNotFoundError{Who: username}
+	if response.IsType(redis.Nil) {
+		return nil, nil
 	}
 
 	encodedResult, err := response.Bytes()
@@ -180,11 +176,14 @@ func findByUsername(username string, conn util.Cmder) (*model.Skin, error) {
 
 func findByUserId(id int, conn util.Cmder) (*model.Skin, error) {
 	response := conn.Cmd("HGET", accountIdToUsernameKey, id)
-	if !response.IsType(redis.Str) {
-		return nil, &http.SkinNotFoundError{Who: "unknown"}
+	if response.IsType(redis.Nil) {
+		return nil, nil
 	}
 
-	username, _ := response.Str()
+	username, err := response.Str()
+	if err != nil {
+		return nil, err
+	}
 
 	return findByUsername(username, conn)
 }
@@ -192,9 +191,7 @@ func findByUserId(id int, conn util.Cmder) (*model.Skin, error) {
 func removeByUserId(id int, conn util.Cmder) error {
 	record, err := findByUserId(id, conn)
 	if err != nil {
-		if _, ok := err.(*http.SkinNotFoundError); !ok {
-			return err
-		}
+		return err
 	}
 
 	conn.Cmd("MULTI")
@@ -212,13 +209,13 @@ func removeByUserId(id int, conn util.Cmder) error {
 func removeByUsername(username string, conn util.Cmder) error {
 	record, err := findByUsername(username, conn)
 	if err != nil {
-		if _, ok := err.(*http.SkinNotFoundError); ok {
-			return nil
-		}
-
 		return err
 	}
 
+	if record == nil {
+		return nil
+	}
+
 	conn.Cmd("MULTI")
 
 	conn.Cmd("DEL", buildUsernameKey(record.Username))

http/api.go

@@ -73,6 +73,13 @@ func (ctx *Api) postSkinHandler(resp http.ResponseWriter, req *http.Request) {
 		return
 	}
 
+	if record == nil {
+		record = &model.Skin{
+			UserId:   identityId,
+			Username: username,
+		}
+	}
+
 	skinId, _ := strconv.Atoi(req.Form.Get("skinId"))
 	is18, _ := strconv.ParseBool(req.Form.Get("is1_8"))
 	isSlim, _ := strconv.ParseBool(req.Form.Get("isSlim"))
@@ -109,13 +116,13 @@ func (ctx *Api) deleteSkinByUsernameHandler(resp http.ResponseWriter, req *http.
 
 func (ctx *Api) deleteSkin(skin *model.Skin, err error, resp http.ResponseWriter) {
 	if err != nil {
-		if _, ok := err.(*SkinNotFoundError); ok {
-			apiNotFound(resp, "Cannot find record for the requested identifier")
-		} else {
-			ctx.Emit("skinsystem:error", fmt.Errorf("unable to find skin info from the repository: %w", err))
-			apiServerError(resp)
-		}
+		ctx.Emit("skinsystem:error", fmt.Errorf("unable to find skin info from the repository: %w", err))
+		apiServerError(resp)
+		return
+	}
 
+	if skin == nil {
+		apiNotFound(resp, "Cannot find record for the requested identifier")
 		return
 	}
 
@@ -130,29 +137,38 @@ func (ctx *Api) deleteSkin(skin *model.Skin, err error, resp http.ResponseWriter
 }
 
 func (ctx *Api) findIdentityOrCleanup(identityId int, username string) (*model.Skin, error) {
-	var record *model.Skin
 	record, err := ctx.SkinsRepo.FindByUserId(identityId)
 	if err != nil {
-		if _, isSkinNotFound := err.(*SkinNotFoundError); !isSkinNotFound {
-			return nil, err
-		}
-
-		record, err = ctx.SkinsRepo.FindByUsername(username)
-		if err == nil {
-			_ = ctx.SkinsRepo.RemoveByUsername(username)
-			record.UserId = identityId
-		} else {
-			record = &model.Skin{
-				UserId:   identityId,
-				Username: username,
-			}
-		}
-	} else if record.Username != username {
-		_ = ctx.SkinsRepo.RemoveByUserId(identityId)
-		record.Username = username
+		return nil, err
 	}
 
-	return record, nil
+	if record != nil {
+		// The username may have changed in the external database,
+		// so we need to remove the old association
+		if record.Username != username {
+			_ = ctx.SkinsRepo.RemoveByUserId(identityId)
+			record.Username = username
+		}
+
+		return record, nil
+	}
+
+	// If the requested id was not found, then username was reassigned to another user
+	// who has not uploaded his data to Chrly yet
+	record, err = ctx.SkinsRepo.FindByUsername(username)
+	if err != nil {
+		return nil, err
+	}
+
+	// If the target username does exist, clear it as it will be reassigned to the new user
+	if record != nil {
+		_ = ctx.SkinsRepo.RemoveByUsername(username)
+		record.UserId = identityId
+
+		return record, nil
+	}
+
+	return nil, nil
 }
 
 func validatePostSkinRequest(request *http.Request) map[string][]string {

http/api_test.go

@@ -88,8 +88,8 @@ var postSkinTestsCases = []*postSkinTestCase{
 			"url":        {"http://example.com/skin.png"},
 		}.Encode()),
 		BeforeTest: func(suite *apiTestSuite) {
-			suite.SkinsRepository.On("FindByUserId", 1).Return(nil, &SkinNotFoundError{Who: "unknown"})
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"})
+			suite.SkinsRepository.On("FindByUserId", 1).Return(nil, nil)
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.SkinsRepository.On("Save", mock.MatchedBy(func(model *model.Skin) bool {
 				suite.Equal(1, model.UserId)
 				suite.Equal("mock_username", model.Username)
@@ -151,7 +151,7 @@ var postSkinTestsCases = []*postSkinTestCase{
 			"url":        {"http://example.com/skin.png"},
 		}.Encode()),
 		BeforeTest: func(suite *apiTestSuite) {
-			suite.SkinsRepository.On("FindByUserId", 2).Return(nil, &SkinNotFoundError{Who: "unknown"})
+			suite.SkinsRepository.On("FindByUserId", 2).Return(nil, nil)
 			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(createSkinModel("mock_username", false), nil)
 			suite.SkinsRepository.On("RemoveByUsername", "mock_username").Times(1).Return(nil)
 			suite.SkinsRepository.On("Save", mock.MatchedBy(func(model *model.Skin) bool {
@@ -368,7 +368,7 @@ func (suite *apiTestSuite) TestDeleteByUserId() {
 	})
 
 	suite.RunSubTest("Try to remove not exists identity id", func() {
-		suite.SkinsRepository.On("FindByUserId", 1).Return(nil, &SkinNotFoundError{Who: "unknown"})
+		suite.SkinsRepository.On("FindByUserId", 1).Return(nil, nil)
 
 		req := httptest.NewRequest("DELETE", "http://chrly/skins/id:1", nil)
 		w := httptest.NewRecorder()
@@ -407,7 +407,7 @@ func (suite *apiTestSuite) TestDeleteByUsername() {
 	})
 
 	suite.RunSubTest("Try to remove not exists identity username", func() {
-		suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"})
+		suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 
 		req := httptest.NewRequest("DELETE", "http://chrly/skins/mock_username", nil)
 		w := httptest.NewRecorder()

http/skinsystem.go

@@ -25,24 +25,6 @@ type CapesRepository interface {
 	FindByUsername(username string) (*model.Cape, error)
 }
 
-// TODO: can I get rid of this?
-type SkinNotFoundError struct {
-	Who string
-}
-
-func (e SkinNotFoundError) Error() string {
-	return "skin data not found"
-}
-
-type CapeNotFoundError struct {
-	Who string
-}
-
-// TODO: can I get rid of this?
-func (e CapeNotFoundError) Error() string {
-	return "cape file not found"
-}
-
 type MojangTexturesProvider interface {
 	GetForUsername(username string) (*mojang.SignedTexturesResponse, error)
 }
@@ -73,7 +55,7 @@ func (ctx *Skinsystem) Handler() *mux.Router {
 func (ctx *Skinsystem) skinHandler(response http.ResponseWriter, request *http.Request) {
 	username := parseUsername(mux.Vars(request)["username"])
 	rec, err := ctx.SkinsRepo.FindByUsername(username)
-	if err == nil && rec.SkinId != 0 {
+	if err == nil && rec != nil && rec.SkinId != 0 {
 		http.Redirect(response, request, rec.Url, 301)
 		return
 	}
@@ -110,7 +92,7 @@ func (ctx *Skinsystem) skinGetHandler(response http.ResponseWriter, request *htt
 func (ctx *Skinsystem) capeHandler(response http.ResponseWriter, request *http.Request) {
 	username := parseUsername(mux.Vars(request)["username"])
 	rec, err := ctx.CapesRepo.FindByUsername(username)
-	if err == nil {
+	if err == nil && rec != nil {
 		request.Header.Set("Content-Type", "image/png")
 		_, _ = io.Copy(response, rec.File)
 		return
@@ -150,11 +132,10 @@ func (ctx *Skinsystem) texturesHandler(response http.ResponseWriter, request *ht
 
 	var textures *mojang.TexturesResponse
 	skin, skinErr := ctx.SkinsRepo.FindByUsername(username)
-	_, capeErr := ctx.CapesRepo.FindByUsername(username)
-	if (skinErr == nil && skin.SkinId != 0) || capeErr == nil {
+	cape, capeErr := ctx.CapesRepo.FindByUsername(username)
+	if (skinErr == nil && skin != nil && skin.SkinId != 0) || (capeErr == nil && cape != nil) {
 		textures = &mojang.TexturesResponse{}
-
-		if skinErr == nil && skin.SkinId != 0 {
+		if skinErr == nil && skin != nil && skin.SkinId != 0 {
 			skinTextures := &mojang.SkinTexturesResponse{
 				Url: skin.Url,
 			}
@@ -168,7 +149,7 @@ func (ctx *Skinsystem) texturesHandler(response http.ResponseWriter, request *ht
 			textures.Skin = skinTextures
 		}
 
-		if capeErr == nil {
+		if capeErr == nil && cape != nil {
 			textures.Cape = &mojang.CapeTexturesResponse{
 				Url: request.URL.Scheme + "://" + request.Host + "/cloaks/" + username,
 			}
@@ -202,7 +183,7 @@ func (ctx *Skinsystem) signedTexturesHandler(response http.ResponseWriter, reque
 	var responseData *mojang.SignedTexturesResponse
 
 	rec, err := ctx.SkinsRepo.FindByUsername(username)
-	if err == nil && rec.SkinId != 0 && rec.MojangTextures != "" {
+	if err == nil && rec != nil && rec.SkinId != 0 && rec.MojangTextures != "" {
 		responseData = &mojang.SignedTexturesResponse{
 			Id:   strings.Replace(rec.Uuid, "-", "", -1),
 			Name: rec.Username,

http/skinsystem_test.go

@@ -163,7 +163,7 @@ var skinsTestsCases = []*skinsystemTestCase{
 	{
 		Name: "Username doesn't exists on the local storage, but exists on Mojang and has textures",
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"})
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(true, false), nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -174,7 +174,7 @@ var skinsTestsCases = []*skinsystemTestCase{
 	{
 		Name: "Username doesn't exists on the local storage, but exists on Mojang and has no textures",
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"})
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(false, false), nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -184,7 +184,7 @@ var skinsTestsCases = []*skinsystemTestCase{
 	{
 		Name: "Username doesn't exists on the local storage and doesn't exists on Mojang",
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"})
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(nil, nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -267,7 +267,7 @@ var capesTestsCases = []*skinsystemTestCase{
 	{
 		Name: "Username doesn't exists on the local storage, but exists on Mojang and has textures",
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"})
+			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(true, true), nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -278,7 +278,7 @@ var capesTestsCases = []*skinsystemTestCase{
 	{
 		Name: "Username doesn't exists on the local storage, but exists on Mojang and has no textures",
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"})
+			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(false, false), nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -288,7 +288,7 @@ var capesTestsCases = []*skinsystemTestCase{
 	{
 		Name: "Username doesn't exists on the local storage and doesn't exists on Mojang",
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"})
+			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(nil, nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -362,7 +362,7 @@ var texturesTestsCases = []*skinsystemTestCase{
 		Name: "Username exists and has skin, no cape",
 		BeforeTest: func(suite *skinsystemTestSuite) {
 			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(createSkinModel("mock_username", false), nil)
-			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"})
+			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
 			suite.Equal(200, response.StatusCode)
@@ -379,7 +379,7 @@ var texturesTestsCases = []*skinsystemTestCase{
 		Name: "Username exists and has slim skin, no cape",
 		BeforeTest: func(suite *skinsystemTestSuite) {
 			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(createSkinModel("mock_username", true), nil)
-			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"})
+			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
 			suite.Equal(200, response.StatusCode)
@@ -398,7 +398,7 @@ var texturesTestsCases = []*skinsystemTestCase{
 	{
 		Name: "Username exists and has cape, no skin",
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"})
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.CapesRepository.On("FindByUsername", "mock_username").Return(createCapeModel(), nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -435,8 +435,8 @@ var texturesTestsCases = []*skinsystemTestCase{
 	{
 		Name: "Username not exists, but Mojang profile available",
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{})
-			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{})
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
+			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Once().Return(createMojangResponse(true, true), nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -456,8 +456,8 @@ var texturesTestsCases = []*skinsystemTestCase{
 	{
 		Name: "Username not exists and Mojang profile unavailable",
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{})
-			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{})
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
+			suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Once().Return(nil, nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -524,7 +524,7 @@ var signedTexturesTestsCases = []*signedTexturesTestCase{
 		Name:       "Username not exists",
 		AllowProxy: false,
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"})
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
 			suite.Equal(204, response.StatusCode)
@@ -551,7 +551,7 @@ var signedTexturesTestsCases = []*signedTexturesTestCase{
 		Name:       "Username not exists, but Mojang profile is available and proxying is enabled",
 		AllowProxy: true,
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"})
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(true, false), nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {
@@ -578,7 +578,7 @@ var signedTexturesTestsCases = []*signedTexturesTestCase{
 		Name:       "Username not exists, Mojang profile is unavailable too and proxying is enabled",
 		AllowProxy: true,
 		BeforeTest: func(suite *skinsystemTestSuite) {
-			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"})
+			suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil)
 			suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(nil, nil)
 		},
 		AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {