Repository navigation
Add film director - #6
Conversation
ksupyl
left a comment
ksupyl
left a comment
There was a problem hiding this comment.
Рома, спасибо!
Тесты Практикума для ветки проходят, режиссёры подгружаются везде, где жанры - как договаривались.
Оставила комментарии прямо в коде.
Обязательные: неизвестный режиссёр в фильме (500 и полузаписанный фильм), 404 для фильмов несуществующего режиссёра и пустое имя режиссёра. Последние два проверяет общий набор тестов Практикума на финальном PR develop -> main - без них он упадёт. Остальное - мелочи.
И два общих момента:
— в Film, FilmService, FilmController, FilmStorage, FilmDbStorage и FilmDbStorageTest пропали пустые строки между группами импортов, а в тесте явные импорты заменились на model.*.
Верни, пожалуйста: эти файлы правят все, и лишние строки дают конфликты;
— в README добавь /directors, /films/director/{id} и две новые таблицы в ER-диаграмму, как Даша сделала для отзывов.
Если что-то непонятно — пиши, разберёмся вместе)
Когда поправишь и будет второй апрув - влей, пожалуйста, через Squash and merge с заголовком:
feat: add film directors (#6)
|
|
||
| Long id = keyHolder.getKeyAs(Long.class); | ||
| saveGenres(id, film.getGenres()); | ||
| saveDirector(id, film.getDirectors()); |
There was a problem hiding this comment.
Здесь фильм к этому моменту уже вставлен.
Если в запросе режиссёр, которого нет в базе (например "directors": [{"id": 999}]), этот insert падает на внешнем ключе - клиент получает 500, а фильм остаётся в базе.
С PUT то же самое: название успевает поменяться, а старые режиссёры удаляются.
Режиссёров нужно проверять в FilmService до записи - так же, как сейчас там проверяются жанры в validateMpaAndGenres. Тогда будет 404 и в базу ничего не попадёт.
| } | ||
|
|
||
| public Collection<Film> findByDirectorSorted(long directorId, String sortBy) { | ||
| return filmStorage.findFilmByDirector(directorId, sortBy); |
There was a problem hiding this comment.
Для несуществующего режиссёра сейчас 200 и пустой список, а везде в проекте на несуществующий id - 404: например, в getCommon сначала проверяются пользователи.
Здесь тоже нужно сначала проверить, что режиссёр существует.
Это проверяет общий набор тестов Практикума на финальном PR develop -> main ("Get films with deleted director").
| public class Director { | ||
|
|
||
| private Long id; | ||
| private String name; |
There was a problem hiding this comment.
Сейчас режиссёр с пустым именем сохраняется, а без имени падает с 500 из базы.
Нужна валидация имени - как у name в Film, - и @Valid в POST и PUT контроллера, тогда будет 400.
Это проверяет общий набор тестов Практикума на финальном PR develop -> main ("Create director Fail name").
| } | ||
|
|
||
| @PostMapping | ||
| public Director createDirector(@RequestBody Director director) { |
There was a problem hiding this comment.
Без @Valid проверки из модели не срабатывают-— как в FilmController, нужен @Valid перед @RequestBody. То же в updateDirector.
| return films; | ||
| } | ||
|
|
||
| // Собирает запрос популярных фильмов: базовое чтение + WHERE по условиям + группировка с лимитом |
There was a problem hiding this comment.
LIMIT отсюда вынесен в #5, а комментарий теперь говорит обратное.
Верни, пожалуйста, прежний: "Собирает запрос: чтение + WHERE по условиям + сортировка по числу лайков, без LIMIT".
| private static final String UPDATE_QUERY = "UPDATE directors SET name = ? WHERE id = ?"; | ||
| private static final String DELETE_QUERY = "DELETE FROM directors WHERE id = ?"; | ||
| private static final String SELECT_BY_ID_QUERY = "SELECT * FROM directors WHERE id = ?"; | ||
| private static final String SELECT_ALL_QUERY = "SELECT * FROM directors"; |
There was a problem hiding this comment.
Без ORDER BY порядок не гарантирован - в остальных запросах проекта стоит ORDER BY id.
|
|
||
| public Director getDirectorById(long id) { | ||
| return directorStorage.findDirectorById(id) | ||
| .orElseThrow(() -> new NotFoundException("Director with id " + id + " not found")); |
There was a problem hiding this comment.
В проекте сообщения об ошибках на русском - давай и здесь, и в removeDirector в DirectorDbStorage.
| return likes.remove(userId); | ||
| } | ||
|
|
||
| private Set<Director> directors = new LinkedHashSet<>(); |
There was a problem hiding this comment.
Поле лучше поднять к остальным полям - сейчас оно стоит после методов.
ksupyl
left a comment
ksupyl
left a comment
There was a problem hiding this comment.
Рома, всё поправлено и это крутяк)
Все три ошибки ушли, тесты Практикума проходят - в том числе общий набор для финального PR: раздел режиссёров 167 из 167.
Перед мержем один шаг: Даша уже влила отзывы, поэтому подтяни develop к себе. Будут конфликты:
— schema.sql — оставить оба блока;
— README — тоже оставить оба варианта, но в ER-диаграмме проверь, что у блока review_likes осталась закрывающая скобка } перед directors, иначе схема сломается.
И одна правка, которую Git не покажет: в ReviewDbStorageTest добавь DirectorRowMapper.class в @import (и импорт сверху), иначе упадут тесты отзывов.
Запушь и дождись зелёного CI - после второго апрува вливай через Squash and merge с заголовком
feat: add film directors (#6)
Если с конфликтами будет туго - пиши, будем кумекать вместе)
Kirill-Kazantcev
left a comment
Kirill-Kazantcev
left a comment
There was a problem hiding this comment.
Ждал правок после комментариев ksupyl
Все огонь можно мерджить
Добавление режиссёров в фильмы.
API
GET /films/director/{directorId}?sortBy=[year,likes]
Возвращает список фильмов режиссера отсортированных по количеству лайков или году выпуска.
GET /directors - Список всех режиссёров
GET /directors/{id}- Получение режиссёра по id
POST /directors - Создание режиссёра
PUT /directors - Изменение режиссёра
DELETE /directors/{id} - Удаление режиссёра
База данных:
directors - режиссёры
film_directors - связь фильма с режиссёром
Тесты
Создал 5 тестов для тестирование базовой логики работы режиссёров.
Так же были созданы 2 теста для проверки правильности сортировки фильмов режиссёра на основе лайков и даты выпуска соответсвенною