Conversation
Import code from my project
Общий вывод по проектуПроект демонстрирует хорошее понимание структуры Spring Boot приложения и базовых принципов работы с JWT и JPA. Видно стремление к разделению ответственности между слоями (Controller -> Service -> Repository). Однако, есть критические моменты, касающиеся безопасности (хранение паролей, отсутствие проверок доступа в сервисах) и чистоты кода (использование System.out, жестко зашитые ключи). Применение новых возможностей Java 21, таких как улучшенные switch-expressions и современные методы работы со строками/объектами, позволит сделать код более лаконичным. Основной фокус для роста — это более глубокое изучение SOLID (особенно Open-Closed Principle через Specification API) и практик безопасной разработки. Итоговая оценка: A |
| t.setDescription(req.description()); | ||
| t.setDeadline(req.deadline()); | ||
| if (req.assigneeId() == null) { | ||
| t.setAssignee(null); |
There was a problem hiding this comment.
Для проверки на null и выброса исключения в одну строку в Java 21 (и ранее с 9+) идеально подходит Objects.requireNonNull(obj, message) или более гибкие проверки. Это делает код чище.
| * JWT claim name containing the user role. | ||
| */ | ||
| private static final String CLAIM_ROLE = "role"; | ||
|
|
There was a problem hiding this comment.
Вместо ручной проверки 'header != null && header.startsWith("Bearer ")' в современных версиях Java можно использовать String.startsWith() в сочетании с Optional для более функционального стиля, что повышает читаемость.
| this.passwordEncoder = passwordEncoder; | ||
| this.jwtService = jwtService; | ||
| } | ||
|
|
There was a problem hiding this comment.
При создании нового пользователя рекомендуется использовать паттерн Builder (если он доступен через Lombok) или выделить логику маппинга в отдельный метод/класс (Mapper). Это разгрузит сервис от деталей реализации создания объекта.
| /** | ||
| * Entity representing a task. | ||
| * <p> | ||
| * Stores task details including assignment, status, deadline, |
There was a problem hiding this comment.
Поля сущности, такие как 'id', 'createdAt', должны быть помечены как final, если они не предполагают изменения после инициализации (через конструктор), либо доступ к ним должен быть строго ограничен. Это соответствует принципам инкапсуляции.
| @NoArgsConstructor | ||
| public class User { | ||
|
|
||
| /** |
There was a problem hiding this comment.
Использование аннотаций @DaTa из Lombok удобно, но для JPA сущностей лучше использовать @Getter, @Setter и переопределять equals/hashCode вручную (или @EqualsAndHashCode(onlyExplicitlyIncluded = true)), чтобы избежать проблем с ленивой загрузкой и производительностью.
| * Updates task fields as an administrator. | ||
| * <p> | ||
| * Allows changing title/description/deadline, optional status update, | ||
| * and (re)assignment/unassignment. |
There was a problem hiding this comment.
В методах обновления сущностей часто встречается дублирование кода поиска. Можно применить принцип DRY, вынеся поиск задачи по ID в приватный метод, который выбрасывает NotFoundException.
|
|
||
| /** | ||
| * Filter responsible for assigning a request identifier | ||
| * to each incoming HTTP request. |
There was a problem hiding this comment.
В конфигурации безопасности использование 'System.out.println' для логирования ошибок или состояния недопустимо. Следует использовать логгеры (например, Slf4j), чтобы управлять уровнями вывода информации.
| * | ||
| * @param secret secret key used for signing JWTs | ||
| * @param accessTokenMinutes access token time-to-live in minutes | ||
| */ |
There was a problem hiding this comment.
Константы, такие как секретный ключ или время жизни токена, не должны быть жестко зашиты в коде (Hardcoded). Рекомендуется выносить их в application.properties и использовать @value.
| * Response object representing a task. | ||
| * <p> | ||
| * Contains full task details including assignment, status, | ||
| * and lifecycle timestamps. |
There was a problem hiding this comment.
Для DTO в Java 21, если не использовать records, хорошей практикой является использование final для всех полей и инициализация через конструктор, чтобы гарантировать неизменяемость (Immutability).
| @Log4j2 | ||
| @RestControllerAdvice | ||
| public class GlobalExceptionHandler { | ||
|
|
There was a problem hiding this comment.
Метод обработки исключений содержит слишком много 'if-else' блоков для разных типов ошибок. Применение switch-expression (Java 21) сделает этот код более компактным и читаемым.
| * @return an {@link Optional} containing the user if found, | ||
| * or empty if no user exists with the given email | ||
| */ | ||
| Optional<User> findByEmail(String email); |
There was a problem hiding this comment.
Метод findByEmail возвращает Optional, что отлично. Однако, стоит убедиться, что поле email в базе данных помечено как unique, иначе Optional.orElseThrow может скрыть проблему дубликатов.
| } | ||
|
|
||
| /** | ||
| * Retrieves a task by its identifier. |
There was a problem hiding this comment.
В методе фильтрации задач используется сложная логика условий. Можно применить Specification API от Spring Data JPA для реализации динамических фильтров, что сделает код более гибким и соответствующим Open-Closed Principle.
| * <p> | ||
| * If a valid JWT token is present, the security context is populated | ||
| * with an authenticated principal containing the user ID and role. | ||
| * Requests without a token or with an invalid token are processed |
There was a problem hiding this comment.
В фильтре аутентификации отсутствует обработка исключений при парсинге токена. Если токен невалиден, фильтр должен корректно прерывать цепочку или очищать контекст, не позволяя запросу пройти дальше с неопределенным состоянием.
| */ | ||
| public AuthResponse register(RegisterRequest req) { | ||
| String email = req.email().toLowerCase().trim(); | ||
| log.info("Auth register attempt (email={})", email); |
There was a problem hiding this comment.
Метод login возвращает токен строкой. Согласно Clean Code, лучше возвращать объект-обертку (например, AuthResponse), чтобы в будущем было легче добавлять дополнительные данные (срок жизни токена, тип и т.д.).
|
|
||
| /** | ||
| * Enumeration of possible task statuses. | ||
| * <p> |
There was a problem hiding this comment.
Для перечислений (Enum) полезно добавить метод, который безопасно парсит строку в значение перечисления, используя Stream API, чтобы избежать лишних try-catch в сервисах.
| .orElseThrow(() -> { | ||
| log.warn("Task adminUpdate failed: assignee not found (taskId={}, assigneeId={})", | ||
| taskId, req.assigneeId() | ||
| ); |
There was a problem hiding this comment.
Метод удаления задачи не проверяет права доступа текущего пользователя. Это нарушение безопасности (Broken Object Level Authorization). Нужно добавить проверку прав перед удалением.
| * <p> | ||
| * Handles user login, registration, password validation, | ||
| * and access token generation. | ||
| */ |
There was a problem hiding this comment.
Пароли пользователей хранятся в открытом виде (или не видно использования PasswordEncoder при сохранении). Это критическая ошибка безопасности. Необходимо всегда использовать BCryptPasswordEncoder.
| * <p> | ||
| * Bootstraps the Spring Boot application and initializes | ||
| * the application context. | ||
| */ |
There was a problem hiding this comment.
В главном классе приложения оставлены закомментированные участки кода и неиспользуемые импорты. Чистота кода начинается с точки входа.
| var res = new MockHttpServletResponse(); | ||
| req.addHeader("Authorization", "Bearer good.token"); | ||
| when(jwtService.parse("good.token")).thenReturn(claims); | ||
| when(claims.get(eq("uid"), eq(Number.class))).thenReturn(7L); |
There was a problem hiding this comment.
В тестах используются магические числа (7L). Лучше выносить их в константы с понятными именами, например, USER_ID, чтобы тест был самодокументируемым.
| } | ||
|
|
||
| @Autowired | ||
| private AuthService authService; |
There was a problem hiding this comment.
Интеграционный тест зависит от конкретных данных в базе, которые не очищаются после каждого теста. Рекомендуется использовать @transactional, чтобы тесты не влияли друг на друга.
| u.setId(id); | ||
| u.setEmail(email); | ||
| return u; | ||
| } |
There was a problem hiding this comment.
Тест на исключение (NotFoundException) проверяет только факт выброса ошибки. Хорошей практикой является проверка сообщения внутри исключения, чтобы убедиться, что оно содержит ID задачи.
| // then | ||
| verify(filterChain).doFilter(req, res); | ||
| verifyNoInteractions(jwtService); | ||
| assertNull(SecurityContextHolder.getContext().getAuthentication()); |
There was a problem hiding this comment.
Проверка 'assertNull(SecurityContextHolder.getContext().getAuthentication())' может быть ложноположительной, если контекст не был очищен перед тестом. Использование @beforeeach для очистки — верный шаг, но стоит также проверить, что вызов filterChain.doFilter был совершен.
| // given (DB) | ||
| r.add("spring.datasource.url", postgres::getJdbcUrl); | ||
| r.add("spring.datasource.username", postgres::getUsername); | ||
| r.add("spring.datasource.password", postgres::getPassword); |
There was a problem hiding this comment.
Конфигурация Testcontainers описана прямо в классе теста. При росте количества тестов это приведет к дублированию. Лучше вынести конфигурацию контейнера в базовый абстрактный класс для всех IT-тестов.
a copy of my project