-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Домашняя работа 4 #4
base: main
Are you sure you want to change the base?
Conversation
src/main/java/ru/qngdjas/habitstracker/api/controller/AuthController.java
Outdated
Show resolved
Hide resolved
src/main/java/ru/qngdjas/habitstracker/api/controller/HabitController.java
Outdated
Show resolved
Hide resolved
src/main/java/ru/qngdjas/habitstracker/api/controller/HabitController.java
Outdated
Show resolved
Hide resolved
|
||
@GetMapping(value = "/habits", produces = MediaType.APPLICATION_JSON_VALUE) | ||
public ResponseEntity<List<HabitDTO>> list() { | ||
List<HabitDTO> habits = habitService.getAll(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хардкодить идентификатор — это плохая практика. Если необходимо передавать константу, лучше вынести ее в отдельное место. В данном случае стоит получать идентификатор пользователя из токена аутентификации. Также я не вижу, чтобы проводилась проверка прав доступа к методам сервиса. Это важный аспект безопасности, который необходимо учитывать.
|
||
@RestController | ||
@RequiredArgsConstructor | ||
@RequestMapping("/habits") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зачем этот класс? Если он не содержит в себе никакой логики, лучше удалить.
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception { | ||
String email = (String) request.getSession().getAttribute("email"); | ||
if (email == null) { | ||
response.sendRedirect("api/v1/login"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Следуюет возвращать 401 вместо перенаправления для API. Это более стандартный подход.
src/main/java/ru/qngdjas/habitstracker/application/mapper/model/UserMapper.java
Outdated
Show resolved
Hide resolved
@Override | ||
public void addInterceptors(InterceptorRegistry registry) { | ||
registry.addInterceptor(authInterceptor) | ||
.excludePathPatterns("/api/v1/login, /api/v1/register"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Следует убрать кавычки и передать пути как отдельные аргументы.
API: 1. Да, использование GlobalExceptionHandler хороший способ для обработки ошибки, который позволяет централизовано обработать ошибки и вынести эту логику из контроллера. Config:
|
Спасибо за пояснения |
Частично разобрался с контекстами, встретился со следующими проблемами:
API:
1. Наилучшим вариантом обработки исключений является GlobalExceptionHandler'ом?
2. Права доступа (аутентифицирован или нет) лучше перехватывать AuthInterceptor'ом?
Config:
1. Не понимаю роли application.yml в чистом Spring MVC. Средства чтения yml доступны в Boot, поэтому либо парсить ручками, либо с помощью библиотек. Отсюда не до конца понятно требование вынести конфиг в yml - перспективнее выглядит properties, потому что работа с ним возможна из коробки.
Концепция инициализации параметров подключения в рамках RootContext'а или любого другого контекста понятна.
2. Избавился от web.xml в пользу JavaConfiguration но не до конца разобрался с разными подходами конфигурирования, в частности с аннотациями @configuration, @EnableWebMvc и связи между AppConfig и WebConfig.
3. Из п. 2 - не получилось сконфигурировать тестовое окружение для контроллеров с использованием MockMvc.