Dev/storage #8

Artyom321 · 2025-02-24T21:28:02Z

No description provided.

common/config/config.go

storage/filesystem/filesystem.go

storage/helpers.go

storage/storage.go

storage/handlers.go

grphil

Ещё из общего, постарайся сообщения об ошибках поподробнее писать, а то пока по ним не всегда можно понять что конкретно не так пошло со всеми подробностями

storage/storage.go

storage/filesystem/filesystem.go

common/connectors/storageconn/structs.go

storage/helpers.go

storage/handlers.go

storage/filesystem/filesystem.go

common/connectors/storageconn/connector.go

common/connectors/storageconn/structs.go

Copilot

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

go.mod: Language not supported

storage/filesystem/filesystem.go

grphil

Ну у меня тут главное замечание по тому, что формат запроса к стораджу довольно странный, после того, как это финализируем, досмотрю ревью

common/connectors/storageconn/structs.go

storage/storage.go

common/connectors/storageconn/connector.go

grphil

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

common/connectors/storageconn/structs.go

common/connectors/storageconn/connector.go

main.go

storage/filesystem/filesystem.go

storage/storage.go

storage/helpers.go

grphil · 2025-04-13T09:57:35Z

storage/helpers.go

+		if request.TestID == 0 {
+			return "", errors.New("TestID is not specified for test resource")
+		}
+		return fmt.Sprintf("tests/%s/%s", strconv.FormatUint(request.TestID, 10), request.StorageFilename), nil


А, я понял зачем тебе StorageFilename. Смотри, мне кажется проще будет захардкодить определенное имя для разных типов файлов, чтобы всем было проще пользоваться стораджем.

SourceCode это source/%s с именем файла (лучше имя сорса сохранить)

CompiledBinary это soultion

CompileOutput это compile.out

TestInput это tests/%02d с номером теста (тут общепринятое название с полигона)

TestAnswer это tests/%02d.a с номером теста (тут общепринятое название с полигона)

TestOutput это tests/%02d.out с номером теста

TestStderr это tests/%02d.err c номером теста

CheckerOutput это tests/%02d.check с номером теста

Checker это check

Interactor это interactor

А дальше можно делать так:
При загрузке файла у тебя кастомное имя просится только для сорса, а остальных случаях ты говоришь что не надо мне кастомных имен.
При скачивании файла если кастомное имя захардкожено, то берешь его, иначе смотришь на папку, содержащую файл - если там единственный файл, выдаешь его, иначе падаешь с ошибкой и просьбой подробнее указать имя файла

Я долго над этим думал, и решил сделать везде кастомное имя, потому что в полигоне например checker/validator/generator можно как угодно называть. Но если хочешь, давай докостыляю...

Ну можно checker, validator, generator, interactor в отдельные папочки поместить и как сорс брать (если в папке один файл, значит это он, иначе падаем с ошибкой)

Я не стал добавлять логику с проверкой что файл единственный в папке. Там и так код не очень, не очень хочется делать разную логику составления имени файла при upload и get)

Artyom321 · 2025-04-14T21:19:50Z

storage/helpers.go

+	}
+}
+
+func getFilepath(request *storageconn.Request) (string, error) {


Есть идеи как это написать лучше?

Тут красиво вряд ли получится написать, но я бы так вообще сделал бы: в getInfoFromRequest ты создаешь тип resourceInfo, сохраняя туда request. А дальше для resourceInfo делаешь методы, которые заполняют его поля (вместо текущих функций которые строки возвращают).

Соответственно у тебя получаются методы:

getDataType

getDataID

getFilepath

Теперь про getFilepath, тут можно разделить на 2 части: папка (getFilepathFolder) и название файла (parseFilepathFilename)

parseFilepathFolder - ты делаешь стандартный switch, в котором в куче случаев просто не делаешь ничего, а для особых случаев (тест, исходник решения, чекер, валидатор) в поле filepath пишешь имя папки.

parseFilepathFilename - тут заводишь переменную filename в случае фиксированных имен файлов, записываешь его. Если же тип ресурса позволяет указывать Filename, то в одном и том же case для них всех ты чекаешь, указан ли Filename. Если указан, дописываешь его, если не указан, то в отдельное поле внутри структуры resourceInfo указываешь, что файл надо найти внутри папки. А дальше этим поиском пусть filesystem занимается, где все будет консистентно. Дальше сответственно если folder не пустая, дописываешь к ней имя файла, иначе ставишь его сразу на место filepath.

Ещё кстати парсинг тестов можно как нибудь удобно объединить, типа там сделать отдельный case на них всех, проверить что Test есть, и потом в какой нибудь отдельной функции построить суффикс уже четким switch по всем видам тестов

При таком подходе количество switch case будет ещё больше, но зато минимизируется копипаста внутри них

Если указан, дописываешь его, если не указан, то в отдельное поле внутри структуры resourceInfo указываешь, что файл надо найти внутри папки. - Я всё-таки оставил логику, что в таком случае берётся файл с дефолтным названием (которое есть для любого типа). У меня нет понимания почему это плохо (ну только если ты боишься что не будешь его указывать когда надо, и при этом в папке несколько файлов), но если хочешь, то могу дописать.

storage/helpers.go

grphil · 2025-04-15T14:53:13Z

storage/helpers.go

+	}
+}
+
+func getFilepath(request *storageconn.Request) (string, error) {


Тут красиво вряд ли получится написать, но я бы так вообще сделал бы: в getInfoFromRequest ты создаешь тип resourceInfo, сохраняя туда request. А дальше для resourceInfo делаешь методы, которые заполняют его поля (вместо текущих функций которые строки возвращают).

Соответственно у тебя получаются методы:

getDataType

getDataID

getFilepath

Теперь про getFilepath, тут можно разделить на 2 части: папка (getFilepathFolder) и название файла (parseFilepathFilename)

parseFilepathFolder - ты делаешь стандартный switch, в котором в куче случаев просто не делаешь ничего, а для особых случаев (тест, исходник решения, чекер, валидатор) в поле filepath пишешь имя папки.

parseFilepathFilename - тут заводишь переменную filename в случае фиксированных имен файлов, записываешь его. Если же тип ресурса позволяет указывать Filename, то в одном и том же case для них всех ты чекаешь, указан ли Filename. Если указан, дописываешь его, если не указан, то в отдельное поле внутри структуры resourceInfo указываешь, что файл надо найти внутри папки. А дальше этим поиском пусть filesystem занимается, где все будет консистентно. Дальше сответственно если folder не пустая, дописываешь к ней имя файла, иначе ставишь его сразу на место filepath.

Ещё кстати парсинг тестов можно как нибудь удобно объединить, типа там сделать отдельный case на них всех, проверить что Test есть, и потом в какой нибудь отдельной функции построить суффикс уже четким switch по всем видам тестов

При таком подходе количество switch case будет ещё больше, но зато минимизируется копипаста внутри них

storage/helpers.go

storage/filesystem/filesystem.go

grphil · 2025-04-16T11:23:43Z

storage/helpers.go

+	return &req, nil
+}
+
+func parseDataType(resourseInfo *filesystem.ResourceInfo) error {


А может все функции вида parse* просто в методы ResourceInfo перенести? Зачем им отдельно лежать?

grphil · 2025-04-16T11:26:55Z

storage/helpers.go

+	case resource.CompiledBinary, resource.CompileOutput, resource.CheckerOutput:
+		return "", nil
+	case resource.SourceCode:
+		return "sources", nil


Вообще хз, зачем эту папку во множественном числе указывать. А ещё в идеале все такие имена в константы в одном месте вынести

storage/filesystem/filesystem.go

grphil · 2025-04-16T11:33:44Z

storage/helpers.go

+			return "", errors.New("TestID is not specified for test resource")
+		}
+		return getTestResourceFilename(request.TestID, request.Resource)
+	default:


А как же заифать что Filename нет для source, checker и interactor...

grphil · 2025-04-16T11:35:02Z

storage/helpers.go

+	request := resourseInfo.Request
+
+	switch request.Resource {
+	case resource.CompiledBinary, resource.CompileOutput, resource.CheckerOutput:


Так, а CheckerOutput это же ресурс для всех тестов. Он по сути как аутпут на тесте должен храниться, он же не общий для всей посылки

grphil · 2025-04-16T11:37:32Z

common/connectors/storageconn/connector.go

-	return nil
+func (s *Connector) Download(request *Request) *FileResponse {
+	response := NewFileResponse(*request)
+	if request.StorageFilename == "" {


Теперь у тебя вообще очень странно поддержан Filename, в половине случаев он есть, в половине нет, ты всегда требуешь его...

grphil · 2025-04-16T19:32:25Z

common/connectors/storageconn/connector.go

+	}
+
+	if resp.IsError() {
+		response.Error = fmt.Errorf("get request failed with status: %v", resp.Status())


О, а давай ещё как-то нормально захендлим, что файла не существует. Какую-то специальную ошибку можем например в этом случае возвращать

Вынес это в отдельный 404

А где? Просто текст писать не оч, это парсить больно. Лучше сделай отдельную ошибку ErrFileNotFound и если статус 404, то возвращай ее. Можешь посмотреть как в lib/cache это сделано, там тоже кастомные ошибки есть

grphil

Ну пока тут баги есть

grphil · 2025-04-17T13:30:11Z

storage/filesystem/filesystem.go

+	}
+
+	if resourceInfo.EmptyStorageFilename {
+		if len(entries) > 1 || (len(entries) == 1 && entries[0] != filepath.Base(resourceInfo.Filepath)) {


А ты же тут все ещё будешь пытаться достать именно определенное имя, которое дефолтно забрал из FilepathFolderMapping

Тут же такая логика: если нам не передали имя, то достаём из папки дефолтный если есть. Или ты хочешь доставать один всегда c любым названием (если не передали)?

Да. Вообще кажется лучше разделить на 2 типа все ресурсы: с фиксированным именем, и нефиксированным именем. Соответственно для ресурсов с фиксированным именем мы юзаем только его. Для ресурсов без фиксированного имени мы или пытаемся по filename найти (если есть), или пытаемся просто взять в папке (если такой один)

А всё-таки сохранять ресурсы с нефиксированным именем мы разрешаем, если StorageFilename не указан (с каким-то дефолтным)?

Давай запретим. Но оно скорее всего само запретится - в папке ничего не будет, поэтому и не сохранится

grphil · 2025-04-17T13:35:40Z

storage/filesystem/resource_info.go

+	resourseInfo.EmptyStorageFilename = true
+
+	filepathFilename, ok := resource.FilepathFilenameMapping[request.Resource]
+	if !ok {


Мне кажется лучше не добавлять дефолтное имя для тех файлов, которые не имеют StorageFilename, и как раз !ok тут будет индикатором того, что имя файла надо вычислять самому. И как раз только в этом случае будет осмысленно resourseInfo.EmptyStorageFilename делать равным true

Кстати у тебя сейчас есть ещё одна бага - ты для всех видов файлов указываешь EmptyStorageFilename, даже даже если у них есть дефолтное имя в сторадже (например тесты). И когда ты их берешь, то папка с ними содержит ещё файлы и ты кидаешь ошибку

Так здесь маппинг говорит дефолтный filename для ресурса, если нет StorageFilename. Ты типо хочешь, чтобы на уровне ResourceInfo было 2 варианта filename: есть переданный StorageFilename и нет?

Написал в другом комменте:
Вообще кажется лучше разделить на 2 типа все ресурсы: с фиксированным именем, и нефиксированным именем. Соответственно для ресурсов с фиксированным именем мы юзаем только его. Для ресурсов без фиксированного имени мы или пытаемся по filename найти (если есть), или пытаемся просто взять в папке (если такой один)

grphil · 2025-04-17T13:39:29Z

storage/filesystem/filesystem.go

+	}
+
+	dir := filepath.Dir(fullPath)
+	entries, err := filesystem.GetDirEntriesNames(dir)


Мне кажется все такие штуки лучше вызвать именно на этапе BuildFilePath, типа просто там чекаешь - если resourceInfo.EmptyStorageFilename = false, то берешь resourceInfo.Filepath как путь к файлу, иначе берешь resourceInfo.Filepath как путь к папке и дальше берешь все содержимое папки и там ищешь файл.

grphil · 2025-04-17T13:41:16Z

storage/filesystem/filesystem.go

+}
+
+func (filesystem *Filesystem) BuildFilePath(resourceInfo *ResourceInfo) (string, error) {
+	cleanFilename := filepath.Base(resourceInfo.Filepath)


Ты же тут теряешь полностью весь путь до resourceInfo.Filepath со всеми этими папочками

grphil · 2025-04-17T13:41:54Z

storage/filesystem/filesystem.go

+	subpathID := filesystem.generatePathFromID(resourceInfo.DataType.String(), strconv.FormatUint(resourceInfo.ID, 10))
+	fullPath := filepath.Join(filesystem.Basepath, subpathID, cleanFilename)
+
+	absBasepath, err := filepath.Abs(filesystem.Basepath)


А зачем тебе вообще все эти чеки с absBasepath и absFullPath?

Чтобы проверить, что мы случайно за пределы корня файловой системы не ушли (если какие-нибудь ../ случайно передали в StorageFilename).

Ну видимо действительно уберу всё это

grphil · 2025-04-17T13:45:17Z

common/connectors/storageconn/connector.go

+	}
+
+	if resp.IsError() {
+		response.Error = fmt.Errorf("get request failed with status: %v", resp.Status())


А где? Просто текст писать не оч, это парсить больно. Лучше сделай отдельную ошибку ErrFileNotFound и если статус 404, то возвращай ее. Можешь посмотреть как в lib/cache это сделано, там тоже кастомные ошибки есть

grphil

Ну сейчас тут пара фиксов осталось сделать

grphil · 2025-04-18T17:56:00Z

storage/handlers.go

+
+	err = s.filesystem.GetFile(resourceInfo)
+	if err != nil {
+		logger.Error("Failed to get file: id=%d, dataType=%s, filePath=%s\n %v",


Давай ошибку будем в лог писать если это не os.NotExistErr, а то если файла нет, то у нас ошибки то не произошло, просто запрос плохой. То есть перенести на 4 строчки вниз сообщение

grphil · 2025-04-18T18:02:29Z

storage/filesystem/filesystem.go

+			return "", fmt.Errorf("StorageFilename is not specified, but directory %s does not contain exactly one file", fullPath)
+		}
+		fullPath = filepath.Join(fullPath, entries[0].Name())
+		resourceInfo.Filepath = fullPath // It lets use resourceInfo.Filepath after successful SaveFile, RemoveFile and GetFile calls


Смотри, тут несколько проблем:

Во первых, ты в половине случаев обновляешь Filepath, а в половине случаев нет. У тебя кстати и download вроде не будет работать если не было EmptyStorageFilename.

Во вторых, в целом мне кажется не оч хорошо менять resourceInfo, оно же как бы константно нам передается. Мне кажется лучше только возвращать именно строку с полным путем до файла

Filepath всегда будет заполнен (при корректном вызове), потому что мы его в resouce_info.go заполняем ещё (и там если не EmptyStorageFilename, то мы полный путь запишем), поэтому Download кажется должен работать.

Вообще я эту штуку в последний момент поменял) мне очень захотелось сделать одинаковую сигнатуру всех методов filesystem + не писать путь в двух местах

Filepath всегда будет заполнен (при корректном вызове), потому что мы его в resouce_info.go заполняем ещё (и там если не EmptyStorageFilename, то мы полный путь запишем), поэтому Download кажется должен работать.

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

Согласен, хуйню написал

grphil · 2025-04-18T18:03:01Z

storage/filesystem/filesystem.go

+	return nil
+}
+
+func (filesystem *Filesystem) GetFile(resourceInfo *ResourceInfo) error {


Мне кажется тут скорее логично именно путь до файла возвращать, а не resourceInfo менять и туда куда-то этот путь проставлять

grphil · 2025-04-18T18:09:37Z

common/constants/resource/resource_type.go

+)
+
+// FilepathFolderMapping should contain all types
+var FilepathFolderMapping = map[Type]string{


А давай эти два мапа в storage перетащим, оно же никакой другой компоненте не нужно

Я хотел тут их оставить, чтобы точно не забыли. Но там при добавлении типа всё равно скорее всего придётся код в storage дописывать, так что ок

grphil · 2025-04-18T18:11:38Z

common/connectors/storageconn/connector.go

+		return response
+	}
+
+	filename := ""


Лучше пустые типы через var filename string создавать

grphil · 2025-04-18T18:15:38Z

common/connectors/storageconn/connector.go

+	}
+	defer file.Close()
+
+	_, err = io.Copy(file, bytes.NewReader(resp.Body()))


О, а я вроде уже писал раньше, видимо потерялось. Давай не будем целиком resp.Body() хранить. Оно будет очень сильно заполнять оперативку когда ты грузишь тесты по 100-200мб и там реально может просто оперативки не хватить. Лучше вместо этого в запросе у resty настрой чтобы body не сохранялось, а вместо этого был ридер по нему, и из него все вычитай. (не забудь ещё этот ридер закрыть)

grphil

Только один супермелкий коммент! Ну и порезолви там мердж конфликт

grphil · 2025-04-18T20:48:53Z

common/connectors/storageconn/connector.go

+	}
+	defer resp.RawBody().Close()
+
+	if resp.StatusCode() >= 400 {


Давай лучше лучше resp.StatusCode() == http.StatusOK

Наверно всё-таки !=)

grphil

Ладно, держи свой шип!

Add storage

5bff50e

Artyom321 force-pushed the dev/storage branch from c7a4bbf to 5bff50e Compare February 24, 2025 21:51

misha567889 self-requested a review April 4, 2025 15:51

misha567889 requested changes Apr 4, 2025

View reviewed changes

Merge branch 'main' into dev/storage

cafaf47

Artyom321 force-pushed the dev/storage branch from cd9aae4 to cafaf47 Compare April 5, 2025 19:52

grphil requested changes Apr 5, 2025

View reviewed changes

grphil reviewed Apr 5, 2025

View reviewed changes

common/connectors/storageconn/connector.go Outdated Show resolved Hide resolved

grphil reviewed Apr 5, 2025

View reviewed changes

common/connectors/storageconn/structs.go Outdated Show resolved Hide resolved

grphil requested a review from Copilot April 6, 2025 13:42

Copilot AI reviewed Apr 6, 2025

View reviewed changes

storage/filesystem/filesystem.go Outdated Show resolved Hide resolved

Merge branch 'main' into dev/storage

83fac79

Artyom321 force-pushed the dev/storage branch from 9217f3a to 83fac79 Compare April 7, 2025 21:36

grphil requested changes Apr 8, 2025

View reviewed changes

common/connectors/storageconn/structs.go Outdated Show resolved Hide resolved

storage/storage.go Outdated Show resolved Hide resolved

common/connectors/storageconn/connector.go Outdated Show resolved Hide resolved

common/connectors/storageconn/connector.go Outdated Show resolved Hide resolved

Artem Titov and others added 2 commits April 11, 2025 23:09

Add storage

b1f02c0

Merge remote-tracking branch 'origin/dev/storage' into dev/storage

49b4d2e

Artyom321 force-pushed the dev/storage branch 3 times, most recently from 5e3a5cc to 49b4d2e Compare April 12, 2025 13:11

Merge remote-tracking branch 'origin/main' into dev/storage

aa19956

grphil requested changes Apr 13, 2025

View reviewed changes

Artyom321 commented Apr 14, 2025

View reviewed changes

grphil requested changes Apr 15, 2025

View reviewed changes

Artyom321 force-pushed the dev/storage branch from 447fdf1 to afa69e8 Compare April 15, 2025 21:56

grphil requested changes Apr 16, 2025

View reviewed changes

grphil reviewed Apr 16, 2025

View reviewed changes

Artyom321 force-pushed the dev/storage branch 2 times, most recently from b702cc5 to 0ffa07f Compare April 16, 2025 22:12

grphil requested changes Apr 17, 2025

View reviewed changes

Artyom321 force-pushed the dev/storage branch from 0ffa07f to be6cc28 Compare April 17, 2025 22:40

grphil requested changes Apr 18, 2025

View reviewed changes

Artyom321 force-pushed the dev/storage branch from be6cc28 to 8a1ae77 Compare April 18, 2025 20:26

grphil requested changes Apr 18, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/storage

971d224

Artyom321 force-pushed the dev/storage branch from 8a1ae77 to 971d224 Compare April 18, 2025 21:09

Merge branch 'main' into dev/storage

6b84fe5

Artyom321 force-pushed the dev/storage branch from e9aab93 to 6b84fe5 Compare April 18, 2025 21:31

grphil approved these changes Apr 18, 2025

View reviewed changes

Artyom321 merged commit f90aa81 into main Apr 18, 2025
2 checks passed

misha567889 deleted the dev/storage branch May 5, 2025 17:06

Dev/storage #8

Dev/storage #8

Uh oh!

Conversation

Artyom321 commented Feb 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grphil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grphil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grphil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grphil Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Artyom321 Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Artyom321 Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

grphil Apr 13, 2025 •

edited

Loading

Artyom321 Apr 14, 2025 •

edited

Loading

Artyom321 Apr 15, 2025 •

edited

Loading

grphil Apr 17, 2025 •

edited

Loading