diff --git a/CHANGELOG.md b/CHANGELOG.md index ccc61cb..2484c82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [4.2.1] - 2019-05-06 +### Changed +- Improved Keep-Alive settings for HTTP client used to perform requests to Mojang's APIs. +- Mojang's textures queue now has static delay of 1 second after each iteration to prevent strange `429` errors. +- Mojang's textures queue now caches even errored responses for signed textures to avoid `429` errors. +- Mojang's textures queue now caches textures data for 70 seconds to avoid strange `429` errors. +- Mojang's textures queue now doesn't log timeout errors. + +### Fixed +- Panic when Redis connection is broken. +- Duplication of Redis connections pool for Mojang's textures queue. +- Removed validation rules for `hash` field. + ## [4.2.0] - 2019-05-02 ### Added - `CHANGELOG.md` file. @@ -44,5 +57,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 from the textures link instead. - `hash` field from `POST /api/skins` endpoint. -[Unreleased]: https://github.com/elyby/chrly/compare/4.2.0...HEAD +[Unreleased]: https://github.com/elyby/chrly/compare/4.2.1...HEAD +[4.2.1]: https://github.com/elyby/chrly/compare/4.2.0...4.2.1 [4.2.0]: https://github.com/elyby/chrly/compare/4.1.1...4.2.0 diff --git a/README.md b/README.md index ee90824..450a5b8 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,7 @@ [![Written in Go][ico-lang]][link-go] [![Build Status][ico-build]][link-build] +[![Coverage][ico-coverage]][link-coverage] [![Keep a Changelog][ico-changelog]](CHANGELOG.md) [![Software License][ico-license]](LICENSE) @@ -260,8 +261,10 @@ To run tests execute `go test ./...`. If your Go version is older than 1.9, then [ico-lang]: https://img.shields.io/badge/lang-go%201.12-blue.svg?style=flat-square [ico-build]: https://img.shields.io/travis/elyby/chrly.svg?style=flat-square +[ico-coverage]: https://img.shields.io/codecov/c/github/elyby/chrly.svg?style=flat-square [ico-changelog]: https://img.shields.io/badge/keep%20a-changelog-orange.svg?style=flat-square [ico-license]: https://img.shields.io/github/license/elyby/chrly.svg?style=flat-square [link-go]: https://golang.org [link-build]: https://travis-ci.org/elyby/chrly +[link-coverage]: https://codecov.io/gh/elyby/chrly diff --git a/api/mojang/mojang.go b/api/mojang/mojang.go index a10ad53..dd4132f 100644 --- a/api/mojang/mojang.go +++ b/api/mojang/mojang.go @@ -10,6 +10,9 @@ import ( var HttpClient = &http.Client{ Timeout: 3 * time.Second, + Transport: &http.Transport{ + MaxIdleConnsPerHost: 1024, + }, } type SignedTexturesResponse struct { diff --git a/api/mojang/queue/in_memory_textures_storage.go b/api/mojang/queue/in_memory_textures_storage.go index 0abc05b..1b8bc00 100644 --- a/api/mojang/queue/in_memory_textures_storage.go +++ b/api/mojang/queue/in_memory_textures_storage.go @@ -9,8 +9,8 @@ import ( "github.com/tevino/abool" ) -var inMemoryStorageGCPeriod = time.Second -var inMemoryStoragePersistPeriod = time.Second * 60 +var inMemoryStorageGCPeriod = 10 * time.Second +var inMemoryStoragePersistPeriod = time.Minute + 10*time.Second var now = time.Now type inMemoryItem struct { @@ -59,25 +59,33 @@ func (s *inMemoryTexturesStorage) GetTextures(uuid string) (*mojang.SignedTextur defer s.lock.Unlock() item, exists := s.data[uuid] - if !exists || now().Add(inMemoryStoragePersistPeriod*time.Duration(-1)).UnixNano()/10e5 > item.timestamp { + validRange := getMinimalNotExpiredTimestamp() + if !exists || validRange > item.timestamp { return nil, &ValueNotFound{} } return item.textures, nil } -func (s *inMemoryTexturesStorage) StoreTextures(textures *mojang.SignedTexturesResponse) { +func (s *inMemoryTexturesStorage) StoreTextures(uuid string, textures *mojang.SignedTexturesResponse) { + var timestamp int64 + if textures != nil { + decoded := textures.DecodeTextures() + if decoded == nil { + panic("unable to decode textures") + } + + timestamp = decoded.Timestamp + } else { + timestamp = unixNanoToUnixMicro(now().UnixNano()) + } + s.lock.Lock() defer s.lock.Unlock() - decoded := textures.DecodeTextures() - if decoded == nil { - panic("unable to decode textures") - } - - s.data[textures.Id] = &inMemoryItem{ + s.data[uuid] = &inMemoryItem{ textures: textures, - timestamp: decoded.Timestamp, + timestamp: timestamp, } } @@ -85,10 +93,18 @@ func (s *inMemoryTexturesStorage) gc() { s.lock.Lock() defer s.lock.Unlock() - maxTime := now().Add(inMemoryStoragePersistPeriod*time.Duration(-1)).UnixNano() / 10e5 + maxTime := getMinimalNotExpiredTimestamp() for uuid, value := range s.data { if maxTime > value.timestamp { delete(s.data, uuid) } } } + +func getMinimalNotExpiredTimestamp() int64 { + return unixNanoToUnixMicro(now().Add(inMemoryStoragePersistPeriod * time.Duration(-1)).UnixNano()) +} + +func unixNanoToUnixMicro(unixNano int64) int64 { + return unixNano / 10e5 +} diff --git a/api/mojang/queue/in_memory_textures_storage_test.go b/api/mojang/queue/in_memory_textures_storage_test.go index 27bce9f..1e90812 100644 --- a/api/mojang/queue/in_memory_textures_storage_test.go +++ b/api/mojang/queue/in_memory_textures_storage_test.go @@ -59,7 +59,7 @@ func TestInMemoryTexturesStorage_GetTextures(t *testing.T) { assert := testify.New(t) storage := CreateInMemoryTexturesStorage() - storage.StoreTextures(texturesWithSkin) + storage.StoreTextures("dead24f9a4fa4877b7b04c8c6c72bb46", texturesWithSkin) result, err := storage.GetTextures("dead24f9a4fa4877b7b04c8c6c72bb46") assert.Equal(texturesWithSkin, result) @@ -70,7 +70,7 @@ func TestInMemoryTexturesStorage_GetTextures(t *testing.T) { assert := testify.New(t) storage := CreateInMemoryTexturesStorage() - storage.StoreTextures(texturesWithSkin) + storage.StoreTextures("dead24f9a4fa4877b7b04c8c6c72bb46", texturesWithSkin) now = func() time.Time { return time.Now().Add(time.Minute * 2) @@ -90,7 +90,7 @@ func TestInMemoryTexturesStorage_StoreTextures(t *testing.T) { assert := testify.New(t) storage := CreateInMemoryTexturesStorage() - storage.StoreTextures(texturesWithSkin) + storage.StoreTextures("dead24f9a4fa4877b7b04c8c6c72bb46", texturesWithSkin) result, err := storage.GetTextures("dead24f9a4fa4877b7b04c8c6c72bb46") assert.Equal(texturesWithSkin, result) @@ -101,8 +101,8 @@ func TestInMemoryTexturesStorage_StoreTextures(t *testing.T) { assert := testify.New(t) storage := CreateInMemoryTexturesStorage() - storage.StoreTextures(texturesWithoutSkin) - storage.StoreTextures(texturesWithSkin) + storage.StoreTextures("dead24f9a4fa4877b7b04c8c6c72bb46", texturesWithoutSkin) + storage.StoreTextures("dead24f9a4fa4877b7b04c8c6c72bb46", texturesWithSkin) result, err := storage.GetTextures("dead24f9a4fa4877b7b04c8c6c72bb46") assert.NotEqual(texturesWithoutSkin, result) @@ -110,6 +110,17 @@ func TestInMemoryTexturesStorage_StoreTextures(t *testing.T) { assert.Nil(err) }) + t.Run("store nil textures", func(t *testing.T) { + assert := testify.New(t) + + storage := CreateInMemoryTexturesStorage() + storage.StoreTextures("dead24f9a4fa4877b7b04c8c6c72bb46", nil) + result, err := storage.GetTextures("dead24f9a4fa4877b7b04c8c6c72bb46") + + assert.Nil(result) + assert.Nil(err) + }) + t.Run("should panic if textures prop is not decoded", func(t *testing.T) { assert := testify.New(t) @@ -121,7 +132,7 @@ func TestInMemoryTexturesStorage_StoreTextures(t *testing.T) { assert.PanicsWithValue("unable to decode textures", func() { storage := CreateInMemoryTexturesStorage() - storage.StoreTextures(toStore) + storage.StoreTextures("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", toStore) }) }) } @@ -164,8 +175,8 @@ func TestInMemoryTexturesStorage_GarbageCollection(t *testing.T) { } storage := CreateInMemoryTexturesStorage() - storage.StoreTextures(textures1) - storage.StoreTextures(textures2) + storage.StoreTextures("dead24f9a4fa4877b7b04c8c6c72bb46", textures1) + storage.StoreTextures("b5d58475007d4f9e9ddd1403e2497579", textures2) storage.Start() diff --git a/api/mojang/queue/queue.go b/api/mojang/queue/queue.go index 4a67992..ff9900d 100644 --- a/api/mojang/queue/queue.go +++ b/api/mojang/queue/queue.go @@ -2,6 +2,7 @@ package queue import ( "net" + "net/url" "regexp" "strings" "sync" @@ -15,7 +16,7 @@ import ( var usernamesToUuids = mojang.UsernamesToUuids var uuidToTextures = mojang.UuidToTextures -var uuidsQueuePeriod = time.Second +var uuidsQueueIterationDelay = time.Second var forever = func() bool { return true } @@ -96,13 +97,13 @@ func (ctx *JobsQueue) GetTexturesForUsername(username string) chan *mojang.Signe func (ctx *JobsQueue) startQueue() { go func() { - time.Sleep(uuidsQueuePeriod) + time.Sleep(uuidsQueueIterationDelay) for forever() { start := time.Now() ctx.queueRound() elapsed := time.Since(start) ctx.Logger.RecordTimer("mojang_textures.usernames.round_time", elapsed) - time.Sleep(uuidsQueuePeriod - elapsed) + time.Sleep(uuidsQueueIterationDelay) } }() } @@ -123,7 +124,7 @@ func (ctx *JobsQueue) queueRound() { profiles, err := usernamesToUuids(usernames) if err != nil { - ctx.handleResponseError(err) + ctx.handleResponseError(err, "usernames") for _, job := range jobs { job.RespondTo <- nil } @@ -134,7 +135,7 @@ func (ctx *JobsQueue) queueRound() { for _, job := range jobs { go func(job *jobItem) { var uuid string - // Profiles in response not ordered, so we must search each username over full array + // The profiles in the response are not ordered, so we must search each username over full array for _, profile := range profiles { if strings.EqualFold(job.Username, profile.Name) { uuid = profile.Id @@ -142,7 +143,7 @@ func (ctx *JobsQueue) queueRound() { } } - ctx.Storage.StoreUuid(job.Username, uuid) + _ = ctx.Storage.StoreUuid(job.Username, uuid) if uuid == "" { job.RespondTo <- nil ctx.Logger.IncCounter("mojang_textures.usernames.uuid_miss", 1) @@ -166,26 +167,23 @@ func (ctx *JobsQueue) getTextures(uuid string) *mojang.SignedTexturesResponse { start := time.Now() result, err := uuidToTextures(uuid, true) ctx.Logger.RecordTimer("mojang_textures.textures.request_time", time.Since(start)) - shouldCache := true if err != nil { - ctx.handleResponseError(err) - shouldCache = false + ctx.handleResponseError(err, "textures") } - if shouldCache && result != nil { - ctx.Storage.StoreTextures(result) - } + // Mojang can respond with an error, but count it as a hit, so store result even if the textures is nil + ctx.Storage.StoreTextures(uuid, result) return result } -func (ctx *JobsQueue) handleResponseError(err error) { - ctx.Logger.Debug("Got response error :err", wd.ErrParam(err)) +func (ctx *JobsQueue) handleResponseError(err error, threadName string) { + ctx.Logger.Debug(":name: Got response error :err", wd.NameParam(threadName), wd.ErrParam(err)) switch err.(type) { case mojang.ResponseError: if _, ok := err.(*mojang.TooManyRequestsError); ok { - ctx.Logger.Warning("Got 429 Too Many Requests :err", wd.ErrParam(err)) + ctx.Logger.Warning(":name: Got 429 Too Many Requests :err", wd.NameParam(threadName), wd.ErrParam(err)) } return @@ -194,6 +192,10 @@ func (ctx *JobsQueue) handleResponseError(err error) { return } + if _, ok := err.(*url.Error); ok { + return + } + if opErr, ok := err.(*net.OpError); ok && (opErr.Op == "dial" || opErr.Op == "read") { return } @@ -203,5 +205,5 @@ func (ctx *JobsQueue) handleResponseError(err error) { } } - ctx.Logger.Emergency("Unknown Mojang response error: :err", wd.ErrParam(err)) + ctx.Logger.Emergency(":name: Unknown Mojang response error: :err", wd.NameParam(threadName), wd.ErrParam(err)) } diff --git a/api/mojang/queue/queue_test.go b/api/mojang/queue/queue_test.go index 4372203..8864df2 100644 --- a/api/mojang/queue/queue_test.go +++ b/api/mojang/queue/queue_test.go @@ -5,16 +5,19 @@ import ( "encoding/base64" "errors" "net" + "net/url" "strings" "syscall" "time" "github.com/elyby/chrly/api/mojang" - mocks "github.com/elyby/chrly/tests" + "testing" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" - "testing" + + mocks "github.com/elyby/chrly/tests" ) type mojangApiMocks struct { @@ -50,8 +53,9 @@ func (m *mockStorage) GetUuid(username string) (string, error) { return args.String(0), args.Error(1) } -func (m *mockStorage) StoreUuid(username string, uuid string) { - m.Called(username, uuid) +func (m *mockStorage) StoreUuid(username string, uuid string) error { + args := m.Called(username, uuid) + return args.Error(0) } func (m *mockStorage) GetTextures(uuid string) (*mojang.SignedTexturesResponse, error) { @@ -64,8 +68,8 @@ func (m *mockStorage) GetTextures(uuid string) (*mojang.SignedTexturesResponse, return result, args.Error(1) } -func (m *mockStorage) StoreTextures(textures *mojang.SignedTexturesResponse) { - m.Called(textures) +func (m *mockStorage) StoreTextures(uuid string, textures *mojang.SignedTexturesResponse) { + m.Called(uuid, textures) } type queueTestSuite struct { @@ -81,7 +85,7 @@ type queueTestSuite struct { } func (suite *queueTestSuite) SetupSuite() { - uuidsQueuePeriod = 0 + uuidsQueueIterationDelay = 0 } func (suite *queueTestSuite) SetupTest() { @@ -130,9 +134,9 @@ func (suite *queueTestSuite) TestReceiveTexturesForOneUsernameWithoutAnyCache() suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Once() suite.Storage.On("GetUuid", "maksimkurb").Once().Return("", &ValueNotFound{}) - suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once() + suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil) suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil, &ValueNotFound{}) - suite.Storage.On("StoreTextures", expectedResult).Once() + suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", expectedResult).Once() suite.MojangApi.On("UsernamesToUuids", []string{"maksimkurb"}).Once().Return([]*mojang.ProfileInfo{ {Id: "0d252b7218b648bfb86c2ae476954d32", Name: "maksimkurb"}, @@ -163,12 +167,12 @@ func (suite *queueTestSuite) TestReceiveTexturesForFewUsernamesWithoutAnyCache() suite.Storage.On("GetUuid", "maksimkurb").Once().Return("", &ValueNotFound{}) suite.Storage.On("GetUuid", "Thinkofdeath").Once().Return("", &ValueNotFound{}) - suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once() - suite.Storage.On("StoreUuid", "Thinkofdeath", "4566e69fc90748ee8d71d7ba5aa00d20").Once() + suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil) + suite.Storage.On("StoreUuid", "Thinkofdeath", "4566e69fc90748ee8d71d7ba5aa00d20").Once().Return(nil) suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil, &ValueNotFound{}) suite.Storage.On("GetTextures", "4566e69fc90748ee8d71d7ba5aa00d20").Once().Return(nil, &ValueNotFound{}) - suite.Storage.On("StoreTextures", expectedResult1).Once() - suite.Storage.On("StoreTextures", expectedResult2).Once() + suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", expectedResult1).Once() + suite.Storage.On("StoreTextures", "4566e69fc90748ee8d71d7ba5aa00d20", expectedResult2).Once() suite.MojangApi.On("UsernamesToUuids", []string{"maksimkurb", "Thinkofdeath"}).Once().Return([]*mojang.ProfileInfo{ {Id: "0d252b7218b648bfb86c2ae476954d32", Name: "maksimkurb"}, @@ -198,7 +202,7 @@ func (suite *queueTestSuite) TestReceiveTexturesForUsernameWithCachedUuid() { suite.Storage.On("GetUuid", "maksimkurb").Once().Return("0d252b7218b648bfb86c2ae476954d32", nil) // Storage.StoreUuid shouldn't be called suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil, &ValueNotFound{}) - suite.Storage.On("StoreTextures", expectedResult).Once() + suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", expectedResult).Once() // MojangApi.UsernamesToUuids shouldn't be called suite.MojangApi.On("UuidToTextures", "0d252b7218b648bfb86c2ae476954d32", true).Once().Return(expectedResult, nil) @@ -271,7 +275,7 @@ func (suite *queueTestSuite) TestReceiveTexturesForMoreThan100Usernames() { suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Times(120) suite.Storage.On("GetUuid", mock.Anything).Times(120).Return("", &ValueNotFound{}) - suite.Storage.On("StoreUuid", mock.Anything, "").Times(120) // if username is not compared to uuid, then receive "" + suite.Storage.On("StoreUuid", mock.Anything, "").Times(120).Return(nil) // should be called with "" if username is not compared to uuid // Storage.GetTextures and Storage.SetTextures shouldn't be called suite.MojangApi.On("UsernamesToUuids", usernames[0:100]).Once().Return([]*mojang.ProfileInfo{}, nil) @@ -305,9 +309,9 @@ func (suite *queueTestSuite) TestReceiveTexturesForTheSameUsernames() { suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Once() suite.Storage.On("GetUuid", "maksimkurb").Twice().Return("", &ValueNotFound{}) - suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once() + suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil) suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil, &ValueNotFound{}) - suite.Storage.On("StoreTextures", expectedResult).Once() + suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", expectedResult).Once() suite.MojangApi.On("UsernamesToUuids", []string{"maksimkurb"}).Once().Return([]*mojang.ProfileInfo{ {Id: "0d252b7218b648bfb86c2ae476954d32", Name: "maksimkurb"}, }, nil) @@ -337,9 +341,9 @@ func (suite *queueTestSuite) TestReceiveTexturesForUsernameThatAlreadyProcessing suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Once() suite.Storage.On("GetUuid", "maksimkurb").Twice().Return("", &ValueNotFound{}) - suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once() + suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil) suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Once().Return(nil, &ValueNotFound{}) - suite.Storage.On("StoreTextures", expectedResult).Once() + suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", expectedResult).Once() suite.MojangApi.On("UsernamesToUuids", []string{"maksimkurb"}).Once().Return([]*mojang.ProfileInfo{ {Id: "0d252b7218b648bfb86c2ae476954d32", Name: "maksimkurb"}, @@ -373,7 +377,7 @@ func (suite *queueTestSuite) TestDoNothingWhenNoTasks() { suite.Logger.On("RecordTimer", "mojang_textures.result_time", mock.Anything).Once() suite.Storage.On("GetUuid", "maksimkurb").Once().Return("", &ValueNotFound{}) - suite.Storage.On("StoreUuid", "maksimkurb", "").Once() + suite.Storage.On("StoreUuid", "maksimkurb", "").Once().Return(nil) // Storage.GetTextures and Storage.StoreTextures shouldn't be called suite.MojangApi.On("UsernamesToUuids", []string{"maksimkurb"}).Once().Return([]*mojang.ProfileInfo{}, nil) @@ -402,6 +406,7 @@ var expectedErrors = []error{ &mojang.TooManyRequestsError{}, &mojang.ServerError{}, &timeoutError{}, + &url.Error{Op: "GET", URL: "http://localhost"}, &net.OpError{Op: "read"}, &net.OpError{Op: "dial"}, syscall.ECONNREFUSED, @@ -411,8 +416,8 @@ func (suite *queueTestSuite) TestShouldNotLogErrorWhenExpectedErrorReturnedFromU suite.Logger.On("IncCounter", mock.Anything, mock.Anything) suite.Logger.On("UpdateGauge", mock.Anything, mock.Anything) suite.Logger.On("RecordTimer", mock.Anything, mock.Anything) - suite.Logger.On("Debug", "Got response error :err", mock.Anything).Times(len(expectedErrors)) - suite.Logger.On("Warning", "Got 429 Too Many Requests :err", mock.Anything).Once() + suite.Logger.On("Debug", ":name: Got response error :err", mock.Anything, mock.Anything).Times(len(expectedErrors)) + suite.Logger.On("Warning", ":name: Got 429 Too Many Requests :err", mock.Anything, mock.Anything).Once() suite.Storage.On("GetUuid", "maksimkurb").Return("", &ValueNotFound{}) @@ -431,8 +436,8 @@ func (suite *queueTestSuite) TestShouldLogEmergencyOnUnexpectedErrorReturnedFrom suite.Logger.On("IncCounter", mock.Anything, mock.Anything) suite.Logger.On("UpdateGauge", mock.Anything, mock.Anything) suite.Logger.On("RecordTimer", mock.Anything, mock.Anything) - suite.Logger.On("Debug", "Got response error :err", mock.Anything).Once() - suite.Logger.On("Emergency", "Unknown Mojang response error: :err", mock.Anything).Once() + suite.Logger.On("Debug", ":name: Got response error :err", mock.Anything, mock.Anything).Once() + suite.Logger.On("Emergency", ":name: Unknown Mojang response error: :err", mock.Anything, mock.Anything).Once() suite.Storage.On("GetUuid", "maksimkurb").Return("", &ValueNotFound{}) @@ -447,13 +452,13 @@ func (suite *queueTestSuite) TestShouldNotLogErrorWhenExpectedErrorReturnedFromU suite.Logger.On("IncCounter", mock.Anything, mock.Anything) suite.Logger.On("UpdateGauge", mock.Anything, mock.Anything) suite.Logger.On("RecordTimer", mock.Anything, mock.Anything) - suite.Logger.On("Debug", "Got response error :err", mock.Anything).Times(len(expectedErrors)) - suite.Logger.On("Warning", "Got 429 Too Many Requests :err", mock.Anything).Once() + suite.Logger.On("Debug", ":name: Got response error :err", mock.Anything, mock.Anything).Times(len(expectedErrors)) + suite.Logger.On("Warning", ":name: Got 429 Too Many Requests :err", mock.Anything, mock.Anything).Once() suite.Storage.On("GetUuid", "maksimkurb").Return("", &ValueNotFound{}) - suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32") + suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Return(nil) suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Return(nil, &ValueNotFound{}) - // Storage.StoreTextures shouldn't be called + suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", (*mojang.SignedTexturesResponse)(nil)) for _, err := range expectedErrors { suite.MojangApi.On("UsernamesToUuids", []string{"maksimkurb"}).Once().Return([]*mojang.ProfileInfo{ @@ -473,13 +478,13 @@ func (suite *queueTestSuite) TestShouldLogEmergencyOnUnexpectedErrorReturnedFrom suite.Logger.On("IncCounter", mock.Anything, mock.Anything) suite.Logger.On("UpdateGauge", mock.Anything, mock.Anything) suite.Logger.On("RecordTimer", mock.Anything, mock.Anything) - suite.Logger.On("Debug", "Got response error :err", mock.Anything).Once() - suite.Logger.On("Emergency", "Unknown Mojang response error: :err", mock.Anything).Once() + suite.Logger.On("Debug", ":name: Got response error :err", mock.Anything, mock.Anything).Once() + suite.Logger.On("Emergency", ":name: Unknown Mojang response error: :err", mock.Anything, mock.Anything).Once() suite.Storage.On("GetUuid", "maksimkurb").Return("", &ValueNotFound{}) - suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32") + suite.Storage.On("StoreUuid", "maksimkurb", "0d252b7218b648bfb86c2ae476954d32").Return(nil) suite.Storage.On("GetTextures", "0d252b7218b648bfb86c2ae476954d32").Return(nil, &ValueNotFound{}) - // Storage.StoreTextures shouldn't be called + suite.Storage.On("StoreTextures", "0d252b7218b648bfb86c2ae476954d32", (*mojang.SignedTexturesResponse)(nil)) suite.MojangApi.On("UsernamesToUuids", []string{"maksimkurb"}).Once().Return([]*mojang.ProfileInfo{ {Id: "0d252b7218b648bfb86c2ae476954d32", Name: "maksimkurb"}, diff --git a/api/mojang/queue/storage.go b/api/mojang/queue/storage.go index f0ffd50..2629e58 100644 --- a/api/mojang/queue/storage.go +++ b/api/mojang/queue/storage.go @@ -4,15 +4,15 @@ import "github.com/elyby/chrly/api/mojang" type UuidsStorage interface { GetUuid(username string) (string, error) - StoreUuid(username string, uuid string) + StoreUuid(username string, uuid string) error } +// nil value can be passed to the storage to indicate that there is no textures +// for uuid and we know about it. Return err only in case, when storage completely +// unable to load any information about textures type TexturesStorage interface { - // nil can be returned to indicate that there is no textures for uuid - // and we know about it. Return err only in case, when storage completely - // don't know anything about uuid GetTextures(uuid string) (*mojang.SignedTexturesResponse, error) - StoreTextures(textures *mojang.SignedTexturesResponse) + StoreTextures(uuid string, textures *mojang.SignedTexturesResponse) } type Storage interface { @@ -31,16 +31,16 @@ func (s *SplittedStorage) GetUuid(username string) (string, error) { return s.UuidsStorage.GetUuid(username) } -func (s *SplittedStorage) StoreUuid(username string, uuid string) { - s.UuidsStorage.StoreUuid(username, uuid) +func (s *SplittedStorage) StoreUuid(username string, uuid string) error { + return s.UuidsStorage.StoreUuid(username, uuid) } func (s *SplittedStorage) GetTextures(uuid string) (*mojang.SignedTexturesResponse, error) { return s.TexturesStorage.GetTextures(uuid) } -func (s *SplittedStorage) StoreTextures(textures *mojang.SignedTexturesResponse) { - s.TexturesStorage.StoreTextures(textures) +func (s *SplittedStorage) StoreTextures(uuid string, textures *mojang.SignedTexturesResponse) { + s.TexturesStorage.StoreTextures(uuid, textures) } // This error can be used to indicate, that requested diff --git a/api/mojang/queue/storage_test.go b/api/mojang/queue/storage_test.go index fb93767..b9ee64c 100644 --- a/api/mojang/queue/storage_test.go +++ b/api/mojang/queue/storage_test.go @@ -17,8 +17,9 @@ func (m *uuidsStorageMock) GetUuid(username string) (string, error) { return args.String(0), args.Error(1) } -func (m *uuidsStorageMock) StoreUuid(username string, uuid string) { +func (m *uuidsStorageMock) StoreUuid(username string, uuid string) error { m.Called(username, uuid) + return nil } type texturesStorageMock struct { @@ -35,8 +36,8 @@ func (m *texturesStorageMock) GetTextures(uuid string) (*mojang.SignedTexturesRe return result, args.Error(1) } -func (m *texturesStorageMock) StoreTextures(textures *mojang.SignedTexturesResponse) { - m.Called(textures) +func (m *texturesStorageMock) StoreTextures(uuid string, textures *mojang.SignedTexturesResponse) { + m.Called(uuid, textures) } func TestSplittedStorage(t *testing.T) { @@ -59,7 +60,7 @@ func TestSplittedStorage(t *testing.T) { t.Run("StoreUuid", func(t *testing.T) { storage, uuidsMock, _ := createMockedStorage() uuidsMock.On("StoreUuid", "username", "result").Once() - storage.StoreUuid("username", "result") + _ = storage.StoreUuid("username", "result") uuidsMock.AssertExpectations(t) }) @@ -74,10 +75,10 @@ func TestSplittedStorage(t *testing.T) { }) t.Run("StoreTextures", func(t *testing.T) { - toStore := &mojang.SignedTexturesResponse{Id: "mock id"} + toStore := &mojang.SignedTexturesResponse{} storage, _, texturesMock := createMockedStorage() - texturesMock.On("StoreTextures", toStore).Once() - storage.StoreTextures(toStore) + texturesMock.On("StoreTextures", "mock id", toStore).Once() + storage.StoreTextures("mock id", toStore) texturesMock.AssertExpectations(t) }) } diff --git a/db/redis.go b/db/redis.go index 977c134..df07135 100644 --- a/db/redis.go +++ b/db/redis.go @@ -6,7 +6,6 @@ import ( "encoding/json" "fmt" "io" - "log" "strconv" "strings" "time" @@ -21,36 +20,35 @@ import ( ) type RedisFactory struct { - Host string - Port int - PoolSize int - connection *pool.Pool + Host string + Port int + PoolSize int + pool *pool.Pool } -// TODO: Why isn't a pointer used here? -func (f RedisFactory) CreateSkinsRepository() (interfaces.SkinsRepository, error) { +func (f *RedisFactory) CreateSkinsRepository() (interfaces.SkinsRepository, error) { return f.createInstance() } -func (f RedisFactory) CreateCapesRepository() (interfaces.CapesRepository, error) { +func (f *RedisFactory) CreateCapesRepository() (interfaces.CapesRepository, error) { panic("capes repository not supported for this storage type") } -func (f RedisFactory) CreateMojangUuidsRepository() (queue.UuidsStorage, error) { +func (f *RedisFactory) CreateMojangUuidsRepository() (queue.UuidsStorage, error) { return f.createInstance() } -func (f RedisFactory) createInstance() (*redisDb, error) { - connection, err := f.getConnection() +func (f *RedisFactory) createInstance() (*redisDb, error) { + p, err := f.getPool() if err != nil { return nil, err } - return &redisDb{connection}, nil + return &redisDb{p}, nil } -func (f RedisFactory) getConnection() (*pool.Pool, error) { - if f.connection == nil { +func (f *RedisFactory) getPool() (*pool.Pool, error) { + if f.pool == nil { if f.Host == "" { return nil, &ParamRequired{"host"} } @@ -65,88 +63,87 @@ func (f RedisFactory) getConnection() (*pool.Pool, error) { return nil, err } - f.connection = conn - - go func() { - period := 5 - for { - time.Sleep(time.Duration(period) * time.Second) - resp := f.connection.Cmd("PING") - if resp.Err == nil { - continue - } - - log.Println("Redis not pinged. Try to reconnect") - conn, err := pool.New("tcp", addr, f.PoolSize) - if err != nil { - log.Printf("Cannot reconnect to redis: %v\n", err) - log.Printf("Waiting %d seconds to retry\n", period) - continue - } - - f.connection = conn - log.Println("Reconnected") - } - }() + f.pool = conn } - return f.connection, nil + return f.pool, nil } type redisDb struct { - conn *pool.Pool + pool *pool.Pool } const accountIdToUsernameKey = "hash:username-to-account-id" const mojangUsernameToUuidKey = "hash:mojang-username-to-uuid" func (db *redisDb) FindByUsername(username string) (*model.Skin, error) { - conn, _ := db.conn.Get() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + if err != nil { + return nil, err + } + defer db.pool.Put(conn) return findByUsername(username, conn) } func (db *redisDb) FindByUserId(id int) (*model.Skin, error) { - conn, _ := db.conn.Get() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + if err != nil { + return nil, err + } + defer db.pool.Put(conn) return findByUserId(id, conn) } func (db *redisDb) Save(skin *model.Skin) error { - conn, _ := db.conn.Get() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + if err != nil { + return err + } + defer db.pool.Put(conn) return save(skin, conn) } func (db *redisDb) RemoveByUserId(id int) error { - conn, _ := db.conn.Get() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + if err != nil { + return err + } + defer db.pool.Put(conn) return removeByUserId(id, conn) } func (db *redisDb) RemoveByUsername(username string) error { - conn, _ := db.conn.Get() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + if err != nil { + return err + } + defer db.pool.Put(conn) return removeByUsername(username, conn) } func (db *redisDb) GetUuid(username string) (string, error) { - conn, _ := db.conn.Get() - defer db.conn.Put(conn) + conn, err := db.pool.Get() + if err != nil { + return "", err + } + defer db.pool.Put(conn) return findMojangUuidByUsername(username, conn) } -func (db *redisDb) StoreUuid(username string, uuid string) { - conn, _ := db.conn.Get() - defer db.conn.Put(conn) +func (db *redisDb) StoreUuid(username string, uuid string) error { + conn, err := db.pool.Get() + if err != nil { + return err + } + defer db.pool.Put(conn) - storeMojangUuid(username, uuid, conn) + return storeMojangUuid(username, uuid, conn) } func findByUsername(username string, conn util.Cmder) (*model.Skin, error) { @@ -156,7 +153,7 @@ func findByUsername(username string, conn util.Cmder) (*model.Skin, error) { redisKey := buildUsernameKey(username) response := conn.Cmd("GET", redisKey) - if response.IsType(redis.Nil) { + if !response.IsType(redis.Str) { return nil, &SkinNotFoundError{username} } @@ -183,7 +180,7 @@ 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.Nil) { + if !response.IsType(redis.Str) { return nil, &SkinNotFoundError{"unknown"} } @@ -215,17 +212,17 @@ 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.(*SkinNotFoundError); !ok { - return err + if _, ok := err.(*SkinNotFoundError); ok { + return nil } + + return err } conn.Cmd("MULTI") conn.Cmd("DEL", buildUsernameKey(record.Username)) - if record != nil { - conn.Cmd("HDEL", accountIdToUsernameKey, record.UserId) - } + conn.Cmd("HDEL", accountIdToUsernameKey, record.UserId) conn.Cmd("EXEC") @@ -272,9 +269,14 @@ func findMojangUuidByUsername(username string, conn util.Cmder) (string, error) return parts[0], nil } -func storeMojangUuid(username string, uuid string, conn util.Cmder) { +func storeMojangUuid(username string, uuid string, conn util.Cmder) error { value := uuid + ":" + strconv.FormatInt(time.Now().Unix(), 10) - conn.Cmd("HSET", mojangUsernameToUuidKey, strings.ToLower(username), value) + res := conn.Cmd("HSET", mojangUsernameToUuidKey, strings.ToLower(username), value) + if res.IsType(redis.Err) { + return res.Err + } + + return nil } func buildUsernameKey(username string) string { @@ -284,8 +286,8 @@ func buildUsernameKey(username string) string { func zlibEncode(str []byte) []byte { var buff bytes.Buffer writer := zlib.NewWriter(&buff) - writer.Write(str) - writer.Close() + _, _ = writer.Write(str) + _ = writer.Close() return buff.Bytes() } @@ -298,7 +300,7 @@ func zlibDecode(bts []byte) ([]byte, error) { } resultBuffer := new(bytes.Buffer) - io.Copy(resultBuffer, reader) + _, _ = io.Copy(resultBuffer, reader) reader.Close() return resultBuffer.Bytes(), nil diff --git a/http/api.go b/http/api.go index 55b949b..9a41401 100644 --- a/http/api.go +++ b/http/api.go @@ -152,7 +152,7 @@ func validatePostSkinRequest(request *http.Request) map[string][]string { const maxMultipartMemory int64 = 32 << 20 const oneOfSkinOrUrlMessage = "One of url or skin should be provided, but not both" - request.ParseMultipartForm(maxMultipartMemory) + _ = request.ParseMultipartForm(maxMultipartMemory) validationRules := govalidator.MapData{ "identityId": {"required", "numeric", "min:1"}, @@ -161,7 +161,6 @@ func validatePostSkinRequest(request *http.Request) map[string][]string { "skinId": {"required", "numeric", "min:1"}, "url": {"url"}, "file:skin": {"ext:png", "size:24576", "mime:image/png"}, - "hash": {}, "is1_8": {"bool"}, "isSlim": {"bool"}, } @@ -174,7 +173,6 @@ func validatePostSkinRequest(request *http.Request) map[string][]string { } else if skinErr == nil { validationRules["file:skin"] = append(validationRules["file:skin"], "skinUploadingNotAvailable") } else if url != "" { - validationRules["hash"] = append(validationRules["hash"], "required") validationRules["is1_8"] = append(validationRules["is1_8"], "required") validationRules["isSlim"] = append(validationRules["isSlim"], "required") } @@ -213,7 +211,7 @@ func findIdentity(repo interfaces.SkinsRepository, identityId int, username stri record, err = repo.FindByUsername(username) if err == nil { - repo.RemoveByUsername(username) + _ = repo.RemoveByUsername(username) record.UserId = identityId } else { record = &model.Skin{ @@ -222,7 +220,7 @@ func findIdentity(repo interfaces.SkinsRepository, identityId int, username stri } } } else if record.Username != username { - repo.RemoveByUserId(identityId) + _ = repo.RemoveByUserId(identityId) record.Username = username } @@ -235,7 +233,7 @@ func apiBadRequest(resp http.ResponseWriter, errorsPerField map[string][]string) result, _ := json.Marshal(map[string]interface{}{ "errors": errorsPerField, }) - resp.Write(result) + _, _ = resp.Write(result) } func apiForbidden(resp http.ResponseWriter, reason string) { @@ -244,7 +242,7 @@ func apiForbidden(resp http.ResponseWriter, reason string) { result, _ := json.Marshal(map[string]interface{}{ "error": reason, }) - resp.Write(result) + _, _ = resp.Write(result) } func apiNotFound(resp http.ResponseWriter, reason string) { @@ -253,7 +251,7 @@ func apiNotFound(resp http.ResponseWriter, reason string) { result, _ := json.Marshal([]interface{}{ reason, }) - resp.Write(result) + _, _ = resp.Write(result) } func apiServerError(resp http.ResponseWriter) { diff --git a/http/api_test.go b/http/api_test.go index 24d9f67..01cd917 100644 --- a/http/api_test.go +++ b/http/api_test.go @@ -28,7 +28,7 @@ func TestConfig_PostSkin(t *testing.T) { resultModel := createSkinModel("mock_user", false) resultModel.SkinId = 5 - resultModel.Url = "http://ely.by/minecraft/skins/default.png" + resultModel.Url = "http://example.com/skin.png" resultModel.MojangTextures = "" resultModel.MojangSignature = "" @@ -37,13 +37,12 @@ func TestConfig_PostSkin(t *testing.T) { "username": {"mock_user"}, "uuid": {"0f657aa8-bfbe-415d-b700-5750090d3af3"}, "skinId": {"5"}, - "hash": {"94a457d92a61460cb9cb5d6f29732d2a"}, "is1_8": {"0"}, "isSlim": {"0"}, - "url": {"http://ely.by/minecraft/skins/default.png"}, + "url": {"http://example.com/skin.png"}, } - req := httptest.NewRequest("POST", "http://skinsystem.ely.by/api/skins", bytes.NewBufferString(form.Encode())) + req := httptest.NewRequest("POST", "http://chrly/api/skins", bytes.NewBufferString(form.Encode())) req.Header.Add("Content-Type", "application/x-www-form-urlencoded") w := httptest.NewRecorder() @@ -89,7 +88,7 @@ func TestConfig_PostSkin(t *testing.T) { panic(err) } - req := httptest.NewRequest("POST", "http://skinsystem.ely.by/api/skins", body) + req := httptest.NewRequest("POST", "http://chrly/api/skins", body) req.Header.Add("Content-Type", writer.FormDataContentType()) w := httptest.NewRecorder() @@ -141,7 +140,6 @@ func TestConfig_PostSkin(t *testing.T) { "username": {"mock_user"}, "uuid": {"0f657aa8-bfbe-415d-b700-5750090d3af3"}, "skinId": {"5"}, - "hash": {"94a457d92a61460cb9cb5d6f29732d2a"}, "is1_8": {"0"}, "isSlim": {"0"}, "url": {"http://textures-server.com/skin.png"}, @@ -171,7 +169,7 @@ func TestConfig_PostSkin(t *testing.T) { resultModel := createSkinModel("mock_user", false) resultModel.UserId = 2 resultModel.SkinId = 5 - resultModel.Url = "http://ely.by/minecraft/skins/default.png" + resultModel.Url = "http://example.com/skin.png" resultModel.MojangTextures = "" resultModel.MojangSignature = "" @@ -180,13 +178,12 @@ func TestConfig_PostSkin(t *testing.T) { "username": {"mock_user"}, "uuid": {"0f657aa8-bfbe-415d-b700-5750090d3af3"}, "skinId": {"5"}, - "hash": {"94a457d92a61460cb9cb5d6f29732d2a"}, "is1_8": {"0"}, "isSlim": {"0"}, - "url": {"http://ely.by/minecraft/skins/default.png"}, + "url": {"http://example.com/skin.png"}, } - req := httptest.NewRequest("POST", "http://skinsystem.ely.by/api/skins", bytes.NewBufferString(form.Encode())) + req := httptest.NewRequest("POST", "http://chrly/api/skins", bytes.NewBufferString(form.Encode())) req.Header.Add("Content-Type", "application/x-www-form-urlencoded") w := httptest.NewRecorder() @@ -219,7 +216,7 @@ func TestConfig_PostSkin(t *testing.T) { resultModel := createSkinModel("changed_username", false) resultModel.SkinId = 5 - resultModel.Url = "http://ely.by/minecraft/skins/default.png" + resultModel.Url = "http://example.com/skin.png" resultModel.MojangTextures = "" resultModel.MojangSignature = "" @@ -228,13 +225,12 @@ func TestConfig_PostSkin(t *testing.T) { "username": {"changed_username"}, "uuid": {"0f657aa8-bfbe-415d-b700-5750090d3af3"}, "skinId": {"5"}, - "hash": {"94a457d92a61460cb9cb5d6f29732d2a"}, "is1_8": {"0"}, "isSlim": {"0"}, - "url": {"http://ely.by/minecraft/skins/default.png"}, + "url": {"http://example.com/skin.png"}, } - req := httptest.NewRequest("POST", "http://skinsystem.ely.by/api/skins", bytes.NewBufferString(form.Encode())) + req := httptest.NewRequest("POST", "http://chrly/api/skins", bytes.NewBufferString(form.Encode())) req.Header.Add("Content-Type", "application/x-www-form-urlencoded") w := httptest.NewRecorder() @@ -268,7 +264,7 @@ func TestConfig_PostSkin(t *testing.T) { "mojangTextures": {"someBase64EncodedString"}, } - req := httptest.NewRequest("POST", "http://skinsystem.ely.by/api/skins", bytes.NewBufferString(form.Encode())) + req := httptest.NewRequest("POST", "http://chrly/api/skins", bytes.NewBufferString(form.Encode())) req.Header.Add("Content-Type", "application/x-www-form-urlencoded") w := httptest.NewRecorder() @@ -324,7 +320,7 @@ func TestConfig_PostSkin(t *testing.T) { config, mocks := setupMocks(ctrl) - req := httptest.NewRequest("POST", "http://skinsystem.ely.by/api/skins", nil) + req := httptest.NewRequest("POST", "http://chrly/api/skins", nil) req.Header.Add("Authorization", "Bearer invalid.jwt.token") w := httptest.NewRecorder() @@ -353,7 +349,7 @@ func TestConfig_DeleteSkinByUserId(t *testing.T) { config, mocks := setupMocks(ctrl) - req := httptest.NewRequest("DELETE", "http://skinsystem.ely.by/api/skins/id:1", nil) + req := httptest.NewRequest("DELETE", "http://chrly/api/skins/id:1", nil) w := httptest.NewRecorder() mocks.Auth.EXPECT().Check(gomock.Any()).Return(nil) @@ -381,7 +377,7 @@ func TestConfig_DeleteSkinByUserId(t *testing.T) { config, mocks := setupMocks(ctrl) - req := httptest.NewRequest("DELETE", "http://skinsystem.ely.by/api/skins/id:2", nil) + req := httptest.NewRequest("DELETE", "http://chrly/api/skins/id:2", nil) w := httptest.NewRecorder() mocks.Auth.EXPECT().Check(gomock.Any()).Return(nil) @@ -412,7 +408,7 @@ func TestConfig_DeleteSkinByUsername(t *testing.T) { config, mocks := setupMocks(ctrl) - req := httptest.NewRequest("DELETE", "http://skinsystem.ely.by/api/skins/mock_user", nil) + req := httptest.NewRequest("DELETE", "http://chrly/api/skins/mock_user", nil) w := httptest.NewRecorder() mocks.Auth.EXPECT().Check(gomock.Any()).Return(nil) @@ -440,7 +436,7 @@ func TestConfig_DeleteSkinByUsername(t *testing.T) { config, mocks := setupMocks(ctrl) - req := httptest.NewRequest("DELETE", "http://skinsystem.ely.by/api/skins/mock_user_2", nil) + req := httptest.NewRequest("DELETE", "http://chrly/api/skins/mock_user_2", nil) w := httptest.NewRecorder() mocks.Auth.EXPECT().Check(gomock.Any()).Return(nil)