Conversation
EugeneLenkevich
left a comment
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Привет, Вячеслав! Спасибо за PR.
Здорово разруливаешь вызовы клиентов и транзакции. Все на самом деле отлично, но можно попробовать завести отдельную конфигурацию для клиентов и точно такую же конфигурацию, но с моками для тестов, а развести им навесив нужные профили. Но это опционально :)
| import org.springframework.boot.autoconfigure.SpringBootApplication; | ||
| import org.springframework.cloud.openfeign.EnableFeignClients; | ||
|
|
||
| @EnableFeignClients |
There was a problem hiding this comment.
Можно завести конфигурацию, повестить на нее эту аннотацию (+ еще перечислить конкретные клиенты, иначе он поднимет все что есть в класспассе)
Что это даст. С помощью профиля мы сможем развести фейн-клиенты для прода/стейджа и для тестов. Для тестов мы вообще можем держать конфигурацию с моками клиентов и спокойно тестировать наш сервис изолированно от других :)
https://www.baeldung.com/spring-cloud-feign-integration-tests (все что по ссылке ест не предлагаю тут сделать, скорее презентация того что можно сделать)
There was a problem hiding this comment.
There was a problem hiding this comment.
Спасибо за ревью. Я ознакомился с материалом, но пока хочу оставить так. На будущее сохраню. А ещё я забыл ридми добавить. Вот добавляю
There was a problem hiding this comment.
Ок, да, в общем имей в виду. А про редми я тоде забыл :)
| import ru.practicum.api.user.UserApi; | ||
|
|
||
| @FeignClient(name = "user-service") | ||
| public interface UserClient extends UserApi { |
There was a problem hiding this comment.
А сами клиенты тоде наверно можно вынести в какой то общий модуль. Смысл тут в чем, это же контракт какого то сервиса, мы его импортируем и делаем запросы к этому сервису, будь то бы проще чем каждый раз клиент пилить :)
| @Component | ||
| public class UserClientHelper extends UserClientAbstractHelper { | ||
|
|
||
| public UserClientHelper(UserApi userApiClient) { |
There was a problem hiding this comment.
Просто интересно, а в чем смысл именно через интерфейс цеплять клиент, не, ну так можно, все понятно, но можно же было напрямую инжектить сам клиент и было бы чутка проще, но, хотя, может тут дело вкуса :)
There was a problem hiding this comment.
Вроде как через интерфейсы надёжнее для будущего улучшения
There was a problem hiding this comment.
Фейн-клиент - это уже интерфейс, спринг создает ему реализацию в рантайме, потому, наверно, и так уже достаточно, мы же можем реализовать сами этот интерфейс и иметь несколько реализаций, ту же реализацию на моках для тестов
| public String delete(Long comId) { | ||
| if (!commentRepository.existsById(comId)) throw new NotFoundException("Not found Comment " + comId); | ||
| commentRepository.deleteById(comId); | ||
| return "deleted comment " + comId; |
There was a problem hiding this comment.
Такая строка не машинночитаема на самом деле, для дальнейшей обработки в коде проще вернуть что то более конкретное, кол-во удаленный строк, true/false
| if (!Objects.equals(eventCommentDto.getState(), State.PUBLISHED)) | ||
| throw new ConflictException("Unable to comment unpublished Event " + eventId); | ||
|
|
||
| return transactionTemplate.execute(status -> { |
There was a problem hiding this comment.
Во, супер, то есть мы сначала собираем с фейн-клиентов все что надо а только лишь потом открываем транзакцию, это круто :)
Единственное, можно было завести два сервиса, первый бы оркестрировал вызовы клиентов, второй бы сохранял запись и управлял транзакцией(у него б стояли соответствующие аннотации). Ну чтобы вручную не открывать транзакцию, хотя, это мелочи
EugeneLenkevich
left a comment
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Очень качественная работа, спасибо :)
3 participants
No description provided.