DEAD-39: image-feat: add s3 repository #8

AlexNov03 · 2024-10-26T00:27:30Z

No description provided.

ilyushkaaa · 2024-10-28T18:35:48Z

docker-compose-minio.yaml

@@ -0,0 +1,20 @@
+services:


а зачем отдельный docker-compose файл под минио?

ilyushkaaa · 2024-10-28T18:42:57Z

internal/delivery/http/v1/avatar.go

+	SetAvatarImage(ctx context.Context, data *domain.ImageData, userID domain.UserID) (domain.ImageURL, error)
+	DeleteAvatarImage(ctx context.Context, userID domain.UserID) error


тут можно бы назвать исключив слово Avatar: SetImage, DeleteImage. Они находятся в интерфейсе юзкейса Аватарок, соответственно и так понятно, что метод относится к ним

ilyushkaaa · 2024-10-28T18:45:33Z

internal/delivery/http/v1/avatar.go

+
+	imageData := new(domain.ImageData)
+
+	file, fileHeader, multipartErr := utils.DecodeImage(r, h.cfg)


да можно было бы и просто err, она тут ни с чем не пересекается. Обычно ошибкам дают какие то названия кроме err, если только назвав err перетрется ранее присвоенное в err значение, которое пригодится в дальнейшем. А тут нет такой проблемы

там в другом проблема, это ошибка типа restErr а ниже err определяется типа internalError, там типы перекрываются, поэтому нейминг другой

можно было конечно сделать

var err error

и тогда под этот интерфейс оба типа бы подходили

ilyushkaaa · 2024-10-28T18:46:49Z

internal/delivery/http/v1/field.go

+	if errors.Is(err, interr.ErrNotFound) {
+		if errors.Is(err, interr.ErrNotFound) {


чтобы наверняка?)))

ilyushkaaa · 2024-10-28T18:49:17Z

internal/delivery/http/v1/fieldimage.go

+	SetFieldImage(ctx context.Context, data *domain.ImageData, fieldID domain.FieldID) (domain.ImageURL, error)
+	DeleteFieldImage(ctx context.Context, fieldID domain.FieldID) error


ну и здесь тоже аналогично тому что выше по неймингу

ilyushkaaa · 2024-10-28T18:51:32Z

internal/delivery/http/v1/fieldimage.go

+	err = h.UC.FieldImage.DeleteFieldImage(r.Context(), fieldID)
+
+	if err != nil {
+		utils.ProcessInternalServerError(h.log, w, err)
+		return
+	}


а тут у нас не может быть not found?

ilyushkaaa · 2024-10-28T18:53:38Z

internal/repository/image/image.go

+		return nil, interr.NewInternalError(err, "unable to upload photo")
+	}
+
+	url, err := r.MinioAdapter.PresignedGetObject(ctx, r.BucketName, imageID, time.Second*60*60*24, nil)


давай константыой хотя бы, а еще лучше конфигом время хранения объекта будем задавать

ilyushkaaa · 2024-10-28T18:54:23Z

internal/repository/image/image.go

+		} else {
+			return "", interr.NewInternalError(err, "error while viewing image by imageID")
+		}


else можем опустить

ilyushkaaa · 2024-10-28T18:57:17Z

internal/repository/local/field/field.go

+type Repository struct {
+	mu     sync.RWMutex
+	Fields map[domain.FieldID]*domain.ImageID
+}


уже бы хорошо такое хранить в каких либо inmemory хранилищах, например в redis. Рестартнется сервис и потеряются все записи в мапе

а вообще, почему мы это в бд не храним просто в виде поля к блоку?

а, ну этим еще параллельно Ваня занимается. Хорошо бы это потом поправить когда его логика будет готова

ilyushkaaa · 2024-10-28T19:03:08Z

internal/repository/pg/user/user.go

-func (r *Repository) GetUserInfo(ctx context.Context, userID domain.UserID) (*domain.UserInfo, error) {
-	q := `SELECT (registration_date, extra_info, num_subscribers, num_subscriptions, 
-	avatar_url, first_name, last_name) 
+func (r *Repository) GetUserInfo(ctx context.Context, userID domain.UserID) (*domain.UserInfo, *domain.ImageID, error) {


хорошо бы завести вообще специальные объекты, с которыми мы будем оперировать на уровне базы- dto, чтобы туда из SQL запросов инфа о бюзере складывалась, а то странновато что мы imageID прям каким то отдельным полем возвращаем, это же тоже к юзеру относится

ну это ладно, посмотрим насколько успевать будем

ilyushkaaa · 2024-10-28T19:17:20Z

internal/repository/pg/user/user.go

+	_, err := r.PG.Exec(ctx, q, avatarID, userID)
+	if err != nil {
+		return interr.NewInternalError(err, "user Repository.UpdateUserAvatarID pg.Exec")
+	}


то что не найден такой аккаунт хорошо б обработать

как и в других таких местах, по примеру как ты сделал GetUserAvatarID

ilyushkaaa · 2024-10-28T19:25:33Z

internal/usecase/avatar/avatar.go

+	err = uc.repo.ImageRepo.DeleteImage(ctx, *imageID)
+	if err != nil {
+		return err
+	}
+
+	return uc.repo.UserRepo.ClearUserAvatarID(ctx, userID)


не очень нравится, что если не получилось очистить Avatar id, то мы будем на ошибки нарываться потом при получении его. Хорошо бы обработать этот момент конечно. Но ладно, посмотрим по времени

так он просто ошибку выше прокинет и на уровне хэндлеров мы ее обработаем, разве отличается от конструкции

if err := uc.repo.UserRepo.ClearUserAvatarID(ctx, userID); err != nil { return err } return nil

ilyushkaaa · 2024-10-28T19:33:11Z

internal/usecase/fieldimage/fieldimage.go

+	err = uc.repo.ImageRepo.DeleteImage(ctx, *imageID)
+	if err != nil {
+		return err
+	}


Suggested change

err = uc.repo.ImageRepo.DeleteImage(ctx, *imageID)

if err != nil {

return err

}

if err = uc.repo.ImageRepo.DeleteImage(ctx, *imageID); err != nil {

return err

}

ilyushkaaa · 2024-10-28T19:34:30Z

internal/utils/multipart.go

+)
+
+func DecodeImage(r *http.Request, cfg *bootstrap.Config) (multipart.File, *multipart.FileHeader, resterr.RestErr) {
+	err := r.ParseMultipartForm(10 << 20)


это тоже хорошо бы в константу, а лучше в конфиг.

ilyushkaaa · 2024-10-28T19:35:57Z

internal/utils/multipart.go

+		return resterr.NewBadRequestError("formfile is not valid")
+	}
+
+	_, _ = f.Seek(0, 0)


странновато выглядит. Надо бы ошибку обработать как будто бы если возвращает

ну я не обработал потому что сдвиг курсора в начало на 0 символов по идее никогда не должен ошибки возвращать, там проблемы обычно бывают если от текущей позиции курсор сдвигаем и он перескакивает через конец содержимого

ilyushkaaa · 2024-10-28T19:37:54Z

internal/utils/multipart.go

+	for _, validType := range cfg.Image.ValidTypes {
+		if mimeType == validType {
+			return nil
+		}
+	}


в конфигах можно мапы делать и в них парсить, так было бы чуть оптимальнее, за o(1) времени всегда бы здесь разбирались

жесткий хайлоад)))

ну здесь больше 7-10 типов точно не будет, так что я не стал, по факту и так константа

Ну это по факту такой цикл будет на каждый запрос связанный с картинкой. Это да, понятно, что мы не ляжем конечно же от такого. Но по сложности реализации это равноценные решения, никаких проблем в том, чтобы сделать немного оптимальнее я не вижу

ilyushkaaa · 2024-10-28T19:40:29Z

internal/usecase/avatar/avatar.go

+	if curAvatarID != nil {
+		err := uc.repo.ImageRepo.DeleteImage(ctx, *curAvatarID)
+		if err != nil {
+			return "", err
+		}
+	}


а там не получится просто обновить, без удаления?

я просто метод обновить картинку убрал из слоя репозиториев, хз сильно ли он нужен

с одной стороны чуть побыстрее будет, вместо двух операций одна, с другой стороны чуть побольше логики придется дописать

ну могу просто свой удаленный код вернуть и update делать

давай одним методом обновлять. Раз есть возможность, надо его использовать. Когда я говорил про то, что можно обновление и добавление картинки совместить, я имел в виду апи конец. Внутри на уровне репозитория конечно же CRUD должен быть

ну да, ты прав тем более здесь по идее может сильно больше времени тратиться

AlexNov03 added 3 commits October 25, 2024 20:33

added error api

d72d77e

commit 2

4cdd22a

image feat: add s3 repository

58f6f87

AlexNov03 changed the base branch from main to DEAD-15 October 26, 2024 00:27

image feat: change data type to multipart/formdata

3a17120

AlexNov03 requested a review from ilyushkaaa October 26, 2024 19:41

ilyushkaaa reviewed Oct 28, 2024

View reviewed changes

image fix: fix bugs in minio storage

48f6861

ilyushkaaa reviewed Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEAD-39: image-feat: add s3 repository #8

DEAD-39: image-feat: add s3 repository #8

AlexNov03 commented Oct 26, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024 •

edited

Loading

ilyushkaaa Oct 28, 2024

AlexNov03 Oct 28, 2024

AlexNov03 Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

AlexNov03 Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024 •

edited

Loading

ilyushkaaa Oct 28, 2024

AlexNov03 Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

AlexNov03 Oct 28, 2024

ilyushkaaa Oct 28, 2024

ilyushkaaa Oct 28, 2024

AlexNov03 Oct 28, 2024

AlexNov03 Oct 28, 2024

AlexNov03 Oct 28, 2024

ilyushkaaa Oct 28, 2024

AlexNov03 Oct 28, 2024

		SetAvatarImage(ctx context.Context, data *domain.ImageData, userID domain.UserID) (domain.ImageURL, error)
		DeleteAvatarImage(ctx context.Context, userID domain.UserID) error


		imageData := new(domain.ImageData)

		file, fileHeader, multipartErr := utils.DecodeImage(r, h.cfg)

		if errors.Is(err, interr.ErrNotFound) {
		if errors.Is(err, interr.ErrNotFound) {

		SetFieldImage(ctx context.Context, data *domain.ImageData, fieldID domain.FieldID) (domain.ImageURL, error)
		DeleteFieldImage(ctx context.Context, fieldID domain.FieldID) error

DEAD-39: image-feat: add s3 repository #8

Are you sure you want to change the base?

DEAD-39: image-feat: add s3 repository #8

Conversation

AlexNov03 commented Oct 26, 2024

Choose a reason for hiding this comment

ilyushkaaa Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyushkaaa Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyushkaaa Oct 28, 2024 •

edited

Loading

ilyushkaaa Oct 28, 2024 •

edited

Loading