Review PR

build.gradle

@@ -14,6 +14,8 @@ repositories {
 }
 
 dependencies {
+    //TODO лучше сгруппировать зависимости как ты сделал ниже с тестовыми библиотеками
+    //Отдельно выделить Spring dependencies, Database, Misc (сюда ломбок, jwt)
     implementation 'org.springframework.boot:spring-boot-starter-web'
     implementation 'org.springframework.boot:spring-boot-starter-security'
     implementation 'org.springframework.boot:spring-boot-starter-data-jpa'

src/main/java/bookShop/config/DatabaseInitializer.java

@@ -13,6 +13,7 @@
 import java.util.Arrays;
 import bookShop.model.AppUser;
 
+//TODO в целом опять же можно воспользоваться библиотекой миграции для заполнения БД, будет удобнее. Но подход прикольный)
 @Component
 @RequiredArgsConstructor
 public class DatabaseInitializer implements CommandLineRunner {

src/main/java/bookShop/config/GlobalStringTrimAdvice.java

@@ -5,6 +5,7 @@
 import org.springframework.web.bind.annotation.InitBinder;
 import org.springframework.beans.propertyeditors.StringTrimmerEditor;
 
+//TODO лучше перенести с пакет controller/advice, как будто бы точно не место в конфигурации
 @ControllerAdvice
 public class GlobalStringTrimAdvice {
 

src/main/java/bookShop/config/SwaggerConfig.java

@@ -7,6 +7,12 @@
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 
+// Чтобы избежать дублирования описания по сути одинаковых ответов для всех эндпойнтов (401, 403, 500 ошибка) можно добавить сюда глобальную конфигурацию стандартных ответов
+// .addResponses("401", new ApiResponse().description("Пользователь не авторизован")
+//                                .content(...)) // Указать содержание здесь
+//                        .addResponses("403", new ApiResponse().description("Доступ запрещён"))
+//                        .addResponses("500", new ApiResponse().description("Внутренняя ошибка сервера"))
+// Это сильно разгрузит обьем кода в контроллерах
 @Configuration
 public class SwaggerConfig {
     @Bean

src/main/java/bookShop/controller/AuthController.java

@@ -16,6 +16,13 @@
 import io.swagger.v3.oas.annotations.media.Schema;
 import io.swagger.v3.oas.annotations.media.ExampleObject;
 
+/*
+ 1. Коммент ко всем контроллерам: если делаем api, то должно быть его версионирование (не важно, будет в итоге когда-либо версия 2 или нет).
+    Так что лучше во все контроллеры добавить в @RequestMapping /api/v1 (например тут получится /api/v1/auth), а сами классы контроллеров выделить в отдельный пакет controller/v1/
+ 2. @ExampleObject получается очень массивным и ухудшает читаемость кода. В классе контроллера хочется в первую очередь видеть эндпойнты.
+    Возможно можно использовать тот же SwaggerResponses, как ты в других контроллерах сделал
+ */
+
 @Tag(name = "Аутентификация", description = "Регистрация и получение токена")
 @RestController
 @RequestMapping("/auth")
@@ -26,6 +33,7 @@ public class AuthController {
     @Operation(
             summary = "Регистрация пользователя",
             responses = {
+                    // чтобы не приходилось делать такие длинные квалифицированные вызовы с указанием библиотеки, я бы все таки твой ApiResponse переименовал бы в какой нить BookShopApiResponse и тут спокойно бы сделал импорт
                     @io.swagger.v3.oas.annotations.responses.ApiResponse(
                             responseCode = "200",
                             description = "Пользователь успешно зарегистрирован",

src/main/java/bookShop/controller/BookController.java

@@ -220,7 +220,7 @@ public ResponseEntity<ApiResponse> addBook(@Valid @RequestBody BookRequest reque
             }
     )
     @PreAuthorize("hasRole('ADMIN')")
-    @PutMapping("/id/{id}")
+    @PutMapping("/id/{id}") // Думаю можно просто оставить /{id}
     public ResponseEntity<ApiResponse> updateBook(@PathVariable Long id, @Valid @RequestBody BookRequest request) {
         BookResponse data = BookResponse.from(bookService.updateBook(id, request));
         return success(data, "Книга успешно обновлена");
@@ -283,7 +283,7 @@ public ResponseEntity<ApiResponse> updateBook(@PathVariable Long id, @Valid @Req
             }
     )
     @PreAuthorize("hasRole('ADMIN')")
-    @DeleteMapping("/id/{id}")
+    @DeleteMapping("/id/{id}") // Думаю можно просто оставить /{id}
     public ResponseEntity<ApiResponse> deleteBook(@PathVariable Long id) {
         bookService.deleteBook(id);
         return successMsg("Книга удалена");
@@ -346,7 +346,7 @@ public ResponseEntity<ApiResponse> deleteBook(@PathVariable Long id) {
             }
     )
     @PreAuthorize("hasRole('ADMIN')")
-    @GetMapping("/id/{id}")
+    @GetMapping("/id/{id}") // Думаю можно просто оставить /{id}
     public ResponseEntity<ApiResponse> getBookById(@PathVariable Long id) {
         BookResponse data = BookResponse.from(bookService.getBookById(id));
         return success(data);
@@ -413,7 +413,7 @@ public ResponseEntity<ApiResponse> getBookById(@PathVariable Long id) {
     public ResponseEntity<ApiResponse> getBooksByTitle(@PathVariable String title) {
         List<BookResponse> data = bookService.getBooksByTitle(title).stream()
                 .map(BookResponse::from)
-                .collect(Collectors.toList());
+                .collect(Collectors.toList()); // с 16 джавы есть более современный и лаконичный .toList()
         return success(data);
     }
 
@@ -478,7 +478,7 @@ public ResponseEntity<ApiResponse> getBooksByTitle(@PathVariable String title) {
     public ResponseEntity<ApiResponse> getBooksByAuthor(@PathVariable String author) {
         List<BookResponse> data = bookService.getBooksByAuthor(author).stream()
                 .map(BookResponse::from)
-                .collect(Collectors.toList());
+                .collect(Collectors.toList()); // с 16 джавы есть более современный и лаконичный .toList()
         return success(data);
     }
 }

src/main/java/bookShop/controller/LoanController.java

@@ -329,7 +329,7 @@ public ResponseEntity<ApiResponse> getActiveLoansByBook(@PathVariable Long bookI
         List<LoanResponse> data = loanService.getActiveLoansByBook(bookId)
                 .stream()
                 .map(LoanResponse::from)
-                .collect(Collectors.toList());
+                .collect(Collectors.toList()); // с 16 джавы есть более современный и лаконичный .toList()
         return success(data);
     }
 
@@ -402,7 +402,7 @@ public ResponseEntity<ApiResponse> getActiveLoansByUser(@PathVariable Long userI
         List<LoanResponse> data = loanService.getActiveLoans(userId)
                 .stream()
                 .map(LoanResponse::from)
-                .collect(Collectors.toList());
+                .collect(Collectors.toList()); // с 16 джавы есть более современный и лаконичный .toList()
         return success(data);
     }
 }

src/main/java/bookShop/controller/swagger/SwaggerResponses.java

@@ -1,5 +1,16 @@
 package bookShop.controller.swagger;
 
+/*
+Структура JSON во всех ответах в целом одинаковая, так что мне кажется можно сделать один метод generateResponse(error, message, status, timestamp, data)
+где через String.format подставлять значения в {
+          "error": %s,
+          "message": %s,
+          "status": %d,
+          "timestamp": "{{timestamp}}",
+          "data": %s
+        }
+        И потом условно public static final String STATUS_200_LIST = generateResponse(null, "Пользователь успешно создан", 200, "..." итд
+ */
 public class SwaggerResponses {
 
     public static final String STATUS_200_LIST = """

src/main/java/bookShop/exception/ApiException.java

@@ -2,6 +2,9 @@
 
 import lombok.*;
 
+// Я бы сказал ну слишком много отдельных классов исключений, хотя по сути все они тот же самый ApiException.
+// В целом спорная тема (тут как тебе нравится), но я лично в коде где бросаю ошибку просто бросал бы ошибку с нужными параметрами
+//
 @Getter
 public class ApiException extends RuntimeException {
     private final String errorCode;

src/main/java/bookShop/model/AppUser.java

@@ -1,10 +1,23 @@
 package bookShop.model;
 
-import lombok.*;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+import lombok.NoArgsConstructor;
 
-import javax.persistence.*;
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.EnumType;
+import javax.persistence.Enumerated;
+import javax.persistence.FetchType;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.OneToMany;
+import javax.persistence.Table;
 import java.util.List;
 
+// А все классы связанные с базой данных выделить в пакет /model/entity.
 @Entity
 @Table(name = "app_user")
 @Data
@@ -28,7 +41,7 @@ public class AppUser {
     @Column(nullable = false)
     @Builder.Default
     private LoyaltyLevel loyaltyLevel = LoyaltyLevel.NOVICE;
-    @OneToMany(mappedBy = "appUser", fetch = FetchType.EAGER)
+    @OneToMany(mappedBy = "appUser", fetch = FetchType.EAGER) //потенциальная N+1 проблема, возможно стоит рассмотреть FetchType.LAZY и в запросах уже тянуть через join fetch (почитай, есть разные способы решения)
     private List<Loan> loans;
     @Column(nullable = false, length = 11)
     private String phone;

src/main/java/bookShop/model/Role.java

@@ -3,6 +3,7 @@
 import lombok.AllArgsConstructor;
 import lombok.Getter;
 
+// можно в отдельный пакет security
 @Getter
 @AllArgsConstructor
 public enum Role {

src/main/java/bookShop/model/request/AuthRequest.java

@@ -8,6 +8,7 @@
 import bookShop.validation.NoEmojiNoXss;
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 
+// Коммент ко всем Request/Response классам: по сути это всё DTO (Data transfer object), поэтому лучше их в выделить в отдельный пакет /model/dto и тут уже /request и /response
 @JsonDeserialize(using = TrimmingAuthRequestDeserializer.class)
 @Data
 @NoArgsConstructor

src/main/java/bookShop/model/response/BookResponse.java

@@ -10,7 +10,9 @@ public class BookResponse {
     private int copiesAvailable;
     private double price;
 
+    //добавил бы импорт Book и более говорящее название метода, например mapfromBook. Тем более метод статический публичный, и если ты потом в одном классе будешь делать статический импорт например from из BookResponse и LoanResponse, получится мешанина
     public static BookResponse from(bookShop.model.Book book) {
+
         BookResponse dto = new BookResponse();
         dto.setId(book.getId());
         dto.setTitle(book.getTitle());

src/main/java/bookShop/model/response/LoanResponse.java

@@ -13,7 +13,7 @@ public class LoanResponse {
     private LocalDate dueDate;
     private LocalDate returnedDate;
 
-    public static LoanResponse from(bookShop.model.Loan loan) {
+    public static LoanResponse from(bookShop.model.Loan loan) { //добавил бы импорт Loan и более говорящее название метода, например mapfromLoan
         LoanResponse dto = new LoanResponse();
         dto.setId(loan.getId());
         dto.setBookId(loan.getBook().getId());

src/main/java/bookShop/model/response/UserResponse.java

@@ -16,7 +16,7 @@ public class UserResponse {
     private String email;
     private List<LoanResponse> activeLoans;
 
-    public static UserResponse from(AppUser user) {
+    public static UserResponse from(AppUser user) { //добавил бы более говорящее название метода, например mapfromAppUser
         UserResponse dto = new UserResponse();
         dto.setId(user.getId());
         dto.setUsername(user.getUsername());

src/main/java/bookShop/security/SecurityConfig.java

@@ -31,6 +31,8 @@ public class SecurityConfig {
     @Autowired
     private bookShop.security.CustomAccessDeniedHandler accessDeniedHandler;
 
+    // В целом всё отлично, но подход через .authorizeRequests() и .antMatchers подустарел и вроде в последних версиях спринга deprecated.
+    // Сейчас используется authorizeHttpRequests() и .requestMatchers(). Функционирует в целом так же
     @Bean
     public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
         http

src/main/java/bookShop/service/AuthService.java

@@ -29,7 +29,7 @@ public class AuthService {
     private final AppUserRepository userRepository;
     private final PasswordEncoder passwordEncoder;
     private final bookShop.security.JwtUtil jwtUtil;
-    private final Validator validator = Validation.buildDefaultValidatorFactory().getValidator();
+    private final Validator validator = Validation.buildDefaultValidatorFactory().getValidator(); // мне кажется лучше инжектить эту зависимость через конструктор, чтобы она потом закрылась автоматически
 
     public UserResponse register(RegisterRequest request) {
         log.info("Регистрация пользователя: {}", request.getUsername());
@@ -42,7 +42,7 @@ public UserResponse register(RegisterRequest request) {
                 throw new AdminLimitExceededException("В системе не может быть более 3-х админов");
             }
         }
-        var violations = validator.validateProperty(request, "password");
+        var violations = validator.validateProperty(request, "password"); // не очень понял зачем эта валидация, она вроде на уровне RegisterRequest должна автоматом происходить за счет @ValidPassword
         if (!violations.isEmpty()) {
             throw new ValidationException(violations.iterator().next().getMessage());
         }

src/main/java/bookShop/service/BookService.java

@@ -29,14 +29,14 @@ public Book getBookById(Long id) {
     }
 
     public List<Book> getBooksByTitle(String title) {
-        title = (title != null) ? title.trim() : null;
+        title = (title != null) ? title.trim() : null; //Можно еще добавить проверку на ввод пустой строки, возможно при запросе на пустую строку оператор LIKE выдаст все записи
         List<Book> books = bookRepository.findByTitleIgnoreCaseLike(title);
         if (books.isEmpty()) throw new BookNotFoundException("Книги с указанным названием не найдены");
         return books;
     }
 
     public List<Book> getBooksByAuthor(String author) {
-        author = (author != null) ? author.trim() : null;
+        author = (author != null) ? author.trim() : null; //Можно еще добавить проверку на ввод пустой строки, возможно при запросе на пустую строку оператор LIKE выдаст все записи
         List<Book> books = bookRepository.findByAuthorIgnoreCaseLike(author);
         if (books.isEmpty()) throw new BookNotFoundException("Книги с указанным автором не найдены");
         return books;
@@ -68,7 +68,7 @@ public Book updateBook(Long id, BookRequest request) {
 
     public void deleteBook(Long id) {
         log.warn("Попытка удаления админом книги [{}]", id);
-        if (!bookRepository.existsById(id)) {
+        if (!bookRepository.existsById(id)) { // я бы вообще не делал запрос на существование записи в этом случае, потому что это лишний запрос в бд если книга существует. Лучше просто удалять и либо проверять 0 или 1 вернулось, либо просто игнорировать ответ
             throw new BookNotFoundException();
         }
         bookRepository.deleteById(id);

src/main/java/bookShop/service/LoyaltyService.java

@@ -6,7 +6,7 @@
 
 @Slf4j
 @Service
-public class LoyaltyService {
+public class LoyaltyService { // Думаю тут отдельный сервисный класс излишен, можно просто через LoyaltyLevel.fromPoint хоть откуда получить нужный enum. А иной логики тут и нет
     public LoyaltyLevel calculateLevel(int points) {
         log.debug("Рассчитан уровень лояльности для {} баллов", points);
         return LoyaltyLevel.fromPoints(points);

src/main/java/bookShop/service/UserService.java

@@ -25,7 +25,7 @@ public class UserService {
     private final AppUserRepository userRepository;
     private final LoanRepository loanRepository;
     private final PasswordEncoder passwordEncoder;
-    private final Validator validator = Validation.buildDefaultValidatorFactory().getValidator();
+    private final Validator validator = Validation.buildDefaultValidatorFactory().getValidator(); // мне кажется лучше инжектить эту зависимость через конструктор, чтобы она потом закрылась автоматически
 
     public List<UserResponse> getAllUsers() {
         List<AppUser> users = userRepository.findAll();
@@ -59,7 +59,7 @@ public UserResponse getUserByUsername(String username) {
     }
 
     public void deleteUser(Long id) {
-        log.warn("Попытка удаления админом пользователя [{}]", id);
+        log.warn("Попытка удаления админом пользователя [{}]", id); //такая же история как в BookService, два запроса в бд вместо одного
         if (!userRepository.existsById(id)) {
             throw new UserNotFoundException();
         }

src/main/java/bookShop/validation/LoanValidator.java

@@ -8,6 +8,7 @@
 
 import java.util.List;
 
+// Думаю что вся эта логика должна быть в сервисном слое. В целом по классической структуре в БД делаем запросы только из сервисных классов
 @Component
 @RequiredArgsConstructor
 public class LoanValidator {

src/main/java/bookShop/validation/NoEmojiNoXssValidator.java

@@ -3,6 +3,8 @@
 import javax.validation.ConstraintValidator;
 import java.util.regex.Pattern;
 
+// Это конечно реально сильная валидация)
+
 public class NoEmojiNoXssValidator implements ConstraintValidator<NoEmojiNoXss, String> {
     private static final Pattern XSS_PATTERN = Pattern.compile("[<>\"'&]");
     private static final Pattern EMOJI_PATTERN = Pattern.compile(

src/main/resources/application.properties

@@ -5,6 +5,7 @@ spring.datasource.password=bookshop_pass
 spring.jpa.hibernate.ddl-auto=update
 spring.jpa.show-sql=true
 
+# ???????? ????????, ?? ??? ???? ??????????? ? ????? ?? ???? ?? ????????
 springdoc.api-docs.path=/v3/api-docs
 springdoc.swagger-ui.path=/swagger-ui.html