-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First Iteration #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Все круто, есть несколько минорных комментов. Я еще не запускал локально приложение, тебе осталось добавить какие-то фичи?
`"status":${status},"error":"${error}","latency":"${latency_human}"` + | ||
`,"bytes_in":${bytes_in},"bytes_out":${bytes_out}}` + "\n" | ||
e.Use(middleware.LoggerWithConfig(loggerCfg)) | ||
e.Use(middleware.Recover()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Потенциально, паника может возникнуть и в middleware, потому я посоветовал зарегистрировать этот обработчик первым
internal/api/store/migrate.go
Outdated
return nil | ||
} | ||
|
||
func runMigrations(dsn string, migrationsDir *embed.FS) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Мне не очень нравится сигнатура этой функции. Почему бы просто не возвращать ошибку и не проверять ее?
internal/api/user/auth/auth.go
Outdated
|
||
jwtCookieName = "vidiSessID" | ||
|
||
SessionContextKey = "vUser" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не стоит экспортировать эту константу + для сохранения значений в контексте стоит использовать iota: https://splice.com/blog/iota-elegant-constants-golang/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В echo контексте ключи могут быть только строками. Саму константу сделал неэкспортируемой
internal/api/user/store/store.go
Outdated
|
||
// salted prepends statically configured string to password. | ||
func (s *Store) salted(pwd string) []byte { | ||
// TODO I dunno if manual salting is helping anything here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нет, при использовании bcrypt
это не требуется
internal/api/video/model/status.go
Outdated
|
||
// Video statuses. | ||
// TODO: May be transitive statuses are redundant? | ||
// TODO: How these statuses will map to DB enums? (or don't use enums may be?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я бы не стал использовать enum
: несмотря на то, что есть риск записать неверное значение в таблицу, если запись происхдит только из приложения, enum
можно не использовать. Преимущество - отстутствие бизнес-логики в БД
internal/api/video/model/status.go
Outdated
// TODO: May be transitive statuses are redundant? | ||
// TODO: How these statuses will map to DB enums? (or don't use enums may be?) | ||
const ( | ||
VideoStatusError = iota - 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VideoStatusError = iota - 1 | |
VideoStatusError Status = iota - 1 |
CONSTRAINT user_uid_not_empty CHECK (user_id != '') | ||
); | ||
|
||
CREATE UNIQUE INDEX videos_id On videos (id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CREATE UNIQUE INDEX videos_id On videos (id); | |
CREATE UNIQUE INDEX videos_id ON videos (id); |
Segment: &meta.SegmentConfig{ | ||
Init: mp4.SegmentSuffixInit, | ||
StartNumber: 1, | ||
// TODO Last segment duration most probably will not be equal to segmentDuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это надо протестить. Я бы не стал полагаться на логику на стороне клиента.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
На данный момент видео проигрывается в Chrome без compliance ошибок. На точную проверку по стандарту и коду reference-плеера потребуется дополнительное время.
// It also generates StaticMPD schema. | ||
func (p *Processor) ProcessFileFromReader(ctx context.Context, r io.Reader, location string) error { | ||
p.logger.Info("mp4 processing started") | ||
// TODO This approach reads whole file into memory, should use lazy read in the future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, это хорошее улучшение на будущее
Нет, по фичам всё. Главное сейчас тесты. Если успею, добавлю сваггер |
pkg/config/config_test.go
Outdated
key: "key", | ||
config: bytes.NewReader([]byte("key: /api/")), | ||
}, | ||
err: "must not end with", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почему бы не добавить в тест точный текст ошибки ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Согласен. пофиксил
internal/api/video/store/store.go
Outdated
query := `select id, location, status, created_at from videos where user_id = $1` | ||
rows, err := s.Pool().Query(ctx, query, userID) | ||
if err != nil { | ||
err = model.ErrNotFound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ты теряешь ошибку здесь, почему бы не return nil, fmt.Errorf("%w: %w", err, model.ErrNotFound)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут я ошибся, нужно просто обработать pg ошибку. Пофиксил
Bare minimum of functions for MVP