Skip to content
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

Merged
merged 37 commits into from
Feb 8, 2024
Merged

First Iteration #1

merged 37 commits into from
Feb 8, 2024

Conversation

adwski
Copy link
Owner

@adwski adwski commented Jan 8, 2024

Bare minimum of functions for MVP

@adwski adwski self-assigned this Jan 16, 2024
Copy link
Collaborator

@s-shpak s-shpak left a 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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Потенциально, паника может возникнуть и в middleware, потому я посоветовал зарегистрировать этот обработчик первым

return nil
}

func runMigrations(dsn string, migrationsDir *embed.FS) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мне не очень нравится сигнатура этой функции. Почему бы просто не возвращать ошибку и не проверять ее?


jwtCookieName = "vidiSessID"

SessionContextKey = "vUser"
Copy link
Collaborator

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/

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В echo контексте ключи могут быть только строками. Саму константу сделал неэкспортируемой


// salted prepends statically configured string to password.
func (s *Store) salted(pwd string) []byte {
// TODO I dunno if manual salting is helping anything here
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нет, при использовании bcrypt это не требуется


// 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?)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я бы не стал использовать enum: несмотря на то, что есть риск записать неверное значение в таблицу, если запись происхдит только из приложения, enum можно не использовать. Преимущество - отстутствие бизнес-логики в БД

// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
VideoStatusError = iota - 1
VideoStatusError Status = iota - 1

CONSTRAINT user_uid_not_empty CHECK (user_id != '')
);

CREATE UNIQUE INDEX videos_id On videos (id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это надо протестить. Я бы не стал полагаться на логику на стороне клиента.

Copy link
Owner Author

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, это хорошее улучшение на будущее

@adwski
Copy link
Owner Author

adwski commented Jan 23, 2024

Все круто, есть несколько минорных комментов. Я еще не запускал локально приложение, тебе осталось добавить какие-то фичи?

Нет, по фичам всё. Главное сейчас тесты. Если успею, добавлю сваггер

key: "key",
config: bytes.NewReader([]byte("key: /api/")),
},
err: "must not end with",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему бы не добавить в тест точный текст ошибки ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Согласен. пофиксил

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
Copy link
Collaborator

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) ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут я ошибся, нужно просто обработать pg ошибку. Пофиксил

@adwski adwski merged commit da80895 into main Feb 8, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants