Skip to content

Categories#22

Open
sp20ks wants to merge 9 commits intodatabasefrom
categories
Open

Categories#22
sp20ks wants to merge 9 commits intodatabasefrom
categories

Conversation

@sp20ks
Copy link
Collaborator

@sp20ks sp20ks commented Apr 28, 2024

в ручку получения списка достопримечательностей добавлена возможность передачи query параметра category_id

@sp20ks sp20ks requested review from GeorgiyX and MrDzhofik April 29, 2024 17:55
Comment on lines +36 to +41
CREATE TABLE category
(
id integer PRIMARY KEY GENERATED ALWAYS AS IDENTITY,
name text NOT NULL UNIQUE
);

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
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

миграции поправим уже отдельным пр, ок?

country_id integer REFERENCES country (id),
UNIQUE (name, city_id)
UNIQUE (name, city_id),
category_id integer REFERENCES category (id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Тут тоже самое - нужна инструкция ALTER TABLE и скрипт установки значений категории.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Если сейчас нет времени править - оставляйте как есть.

@@ -28,10 +29,19 @@ func (ss *SightStorage) GetSightsList() ([]entities.Sight, error) {
var sights []*entities.Sight
ctx := context.Background()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Контекст нужно прокидывать снаружи, чтобы иметь возможность отменить операцию и освободить коннтект к базе

Comment on lines 51 to 87

err := pgxscan.Select(ctx, ss.db, &sight, `SELECT sight.id, COALESCE(rating, 0) AS rating, sight.name, description, city_id, sight.country_id, im.path, city.city, country.country
FROM sight
INNER JOIN image_data AS im
ON sight.id = im.sight_id
INNER JOIN city
ON sight.city_id = city.id
INNER JOIN country
ON sight.country_id = country.id
WHERE sight.id = $1`, id)
err := pgxscan.Select(ctx, ss.db, &sight,
`SELECT sight.id,
COALESCE(rating, 0) AS rating,
sight.name,
description,
city_id,
sight.country_id,
im.path,
city.city,
country.country,
category_id,
category.name AS category
FROM sight
INNER JOIN image_data AS im
ON sight.id = im.sight_id
INNER JOIN city
ON sight.city_id = city.id
INNER JOIN country
ON sight.country_id = country.id
INNER JOIN category
ON sight.category_id = category.id
WHERE sight.id = $1`, id)
if err != nil {
return entities.Sight{}, err
}

return *sight[0], nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Для чтения одной строки стоит использовать pgxscan.Get

first = false
}
if key == "rating" || key == "city_id" || key == "country_id" || key == "category_id" {
query.WriteString(fmt.Sprintf("%s = %s", key, value))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Нужно все аргументы передавать через плейсхолдеры. Такое простроение запросов позволяет атаковать бек через sql инъекции.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Для построения "сложных" запросов можно использовать пакеты squirrel или xorm/builder

rows, err := ss.db.Query(context.Background(), query, "%"+str+"%")
if err != nil {
return entities.Sights{}, err
func (ss *SightStorage) SearchSights(searchParams map[string]string) (entities.Sights, error) {
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
Collaborator Author

Choose a reason for hiding this comment

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

то есть не мапой? а если у нас в будущем планируется поиск не только по имени, но и по стране, городу и тд? все 11 параметров передавать отдельно?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Да, отдельно или в структуре

Comment on lines 140 to 143
var sightList []entities.Sight
for _, s := range sights {
sightList = append(sightList, *s)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Можно сразу сканировать в []entities.Sight

}

func SetQueryParamToCtx(ctx context.Context, queryParams map[string]string) context.Context {
return context.WithValue(ctx, RequestQueryParamsKey, queryParams)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вроде в ранних PRах обсуждали, что лучше в качестве ключа использовать собственный тип, а не строку. Это необходимо делать чтобы избежать коллизий.

@GeorgiyX GeorgiyX mentioned this pull request May 5, 2024
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