Инкремент №9 #9
Инкремент №9 #9
Conversation
internal/agent/api.go
Outdated
| var requestID = utils.GenerateRequestID() | ||
| var ctx = setupLoggerCtx(requestID) | ||
| var mex metrics.MetricExchange |
There was a problem hiding this comment.
из этих 3 строк, var нужен только в последней, просто правило как выбрать между := и var
- если переменная инициализируется как zero-value то рекомендуется использовать var
- во всех остальных случаях разумно использовать :=
internal/agent/api.go
Outdated
| // HELP: можно ли тут вместо приведения типов | ||
| // использовать рефлексию через metric.(type) ? |
There was a problem hiding this comment.
можно но не стоит)
рефлексия оч специфический инструмент, если есть набор конечных состояний то лучше ее избежать
There was a problem hiding this comment.
тут вполне можно обойтись type switch
internal/agent/api.go
Outdated
| case "counter": | ||
| mex = metrics.NewUpdateCounterMex(name, metric.(metrics.Counter)) | ||
| case "gauge": |
There was a problem hiding this comment.
строки "counter" и "gauge" встречаются повсеместно в проекте, есть смысл сделать их константами и переиспользовать
|
|
||
| logging.LogDebugCtx(d.context, "got request compressed with "+encoding) | ||
|
|
||
| if _, ok := d.supportedEncodings[encoding]; !ok { |
There was a problem hiding this comment.
не самый надежный способ, потому что в заголовке может быть несколько значений
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding#compressing_with_gzip
There was a problem hiding this comment.
@tmvrus не очень понял тут. Это декомпрессия входящего запроса, мы проверяем Content-Encoding, там либо одна кодировка, либо несколько, в последовательном порядке их применения. Т.к. мы умеем распаковывать только gzip, то и проверяем только его.
Если речь про Accept-Encoding, то там проверяю все заголовки - https://github.com/ex0rcist/metflix/pull/9/files/42a295da31674147c0c10eb19ec8b95e85e8a52a#diff-3413224df30ede06962678504193d621a87171fd26eed154919225315109efd7R111
There was a problem hiding this comment.
Если там будет Content-Encoding: deflate, gzip код отработает?
There was a problem hiding this comment.
Нет, а должен? Гугл говорит что Content-Encoding: deflate это данные в формате zlib, тогда как Content-Encoding: gzip это данные в формате gzip, и обе этих либы используют алгоритм сжатия deflate. Но для распаковки zlib надо использовать другую библиотеку, т.к.заголовки у сжатых данных отличаются
internal/metrics/types.go
Outdated
| // Counter | ||
|
|
| logger.Info(). | ||
| Str("method", r.Method). | ||
| Str("url", r.URL.String()). | ||
| Str("remote-addr", r.RemoteAddr). // middleware.RealIP |
There was a problem hiding this comment.
На всякий случай предупрежу, в реальной системе тут врядли будет адрес автора запроса. Потому что в реальных системах запросы идут через один или несколько прокси, например nginx или haproxy. Чаще всего настроящий IP-адресс клиента будет в заголовке X-Forwarded-For
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For
There was a problem hiding this comment.
Спасибо, последнее время у нас этим занимались системные администраторы, и мы в конфигурациях использовали x-real-ip (от nginx), потому что знали что ему можно доверять. Тема стоит отдельного изучения
internal/server/handlers.go
Outdated
| // resp := fmt.Sprintf("%d %v", code, err) | ||
| // http.Error(w, resp, code) |
There was a problem hiding this comment.
не стоит оставлять закоменченый код в проекте, это дурной тон, засоряет проект и усложняет чтение
internal/server/server.go
Outdated
| func (s *Server) restoreStorage() error { | ||
| // HELP: не уверен что тут корректное решение... Но иначе нужно добавлять Restore() в интерфейс, а его логически нет у MemStorage | ||
| return s.Storage.(*storage.FileStorage).Restore() | ||
| } |
There was a problem hiding this comment.
Server вообще ничего не должен знать про загрузку с диска, разумнее всего это делать при создании экземпляра хранилища, передавая ему конфигурацию. Такая реализация это пример "abstraction leak" когда особенности хранения "протекают" на другие слои.
There was a problem hiding this comment.
Вообще я так и сделал изначально, но мне не понравилось что после создания Storage начинает сам себя дампать на диск, пока сервер еще формально не запущен ( Run())
Я перенес обратно в storage как было, а в сервере возможно стоит сделать CreateAndRun() вместо New() и Run() ? Или не стоит заморачиваться вообще
There was a problem hiding this comment.
ТАк а зачем серверу вообще знать про какой-то Run, есть для сервера в этом хоть какой-то смысл? Если нет, то и знать незачем.
internal/server/server.go
Outdated
| // HELP: не уверен что тут корректное решение... Но иначе нужно добавлять Dump() в интерфейс, а его логически нет у MemStorage | ||
| if err := s.Storage.(*storage.FileStorage).Dump(); err != nil { | ||
| logging.LogError(fmt.Errorf("error during FileStorage Dump(): %s", err.Error())) | ||
| } |
There was a problem hiding this comment.
все тоже самое, сохраннеие можно было реализовать внутри сторадажа, и ничего не говорить об этом серверу
internal/storage/storage.go
Outdated
| Push(id string, record Record) error | ||
| Get(id string) (Record, error) | ||
| List() ([]Record, error) | ||
| Kind() string |
There was a problem hiding this comment.
само по себе наличие этого метода нарушает принцип инкапсуляции, рассказывать вызывающему коду о внутреннем устройстве это лишнее. Боле того, добавлять в специальные состояние/флаги которые говорят о том, что это "тест" (mock) это повод задуматься редизайне приложения.
Прошу прощения за большой diff, привёл в порядок логирование, дописал и переписал много тестов.