diff --git a/build.gradle b/build.gradle index 4e023ae..b2b0386 100644 --- a/build.gradle +++ b/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' diff --git a/src/main/java/bookShop/config/DatabaseInitializer.java b/src/main/java/bookShop/config/DatabaseInitializer.java index 096f288..8eb4449 100644 --- a/src/main/java/bookShop/config/DatabaseInitializer.java +++ b/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 { diff --git a/src/main/java/bookShop/config/GlobalStringTrimAdvice.java b/src/main/java/bookShop/config/GlobalStringTrimAdvice.java index b7a8432..ea8a45c 100644 --- a/src/main/java/bookShop/config/GlobalStringTrimAdvice.java +++ b/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 { diff --git a/src/main/java/bookShop/config/SwaggerConfig.java b/src/main/java/bookShop/config/SwaggerConfig.java index da26eb1..b1aaeb4 100644 --- a/src/main/java/bookShop/config/SwaggerConfig.java +++ b/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 diff --git a/src/main/java/bookShop/controller/AuthController.java b/src/main/java/bookShop/controller/AuthController.java index ef9feb7..2def2ff 100644 --- a/src/main/java/bookShop/controller/AuthController.java +++ b/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 = "Пользователь успешно зарегистрирован", diff --git a/src/main/java/bookShop/controller/BookController.java b/src/main/java/bookShop/controller/BookController.java index be41626..a2dd7b6 100644 --- a/src/main/java/bookShop/controller/BookController.java +++ b/src/main/java/bookShop/controller/BookController.java @@ -220,7 +220,7 @@ public ResponseEntity addBook(@Valid @RequestBody BookRequest reque } ) @PreAuthorize("hasRole('ADMIN')") - @PutMapping("/id/{id}") + @PutMapping("/id/{id}") // Думаю можно просто оставить /{id} public ResponseEntity 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 updateBook(@PathVariable Long id, @Valid @Req } ) @PreAuthorize("hasRole('ADMIN')") - @DeleteMapping("/id/{id}") + @DeleteMapping("/id/{id}") // Думаю можно просто оставить /{id} public ResponseEntity deleteBook(@PathVariable Long id) { bookService.deleteBook(id); return successMsg("Книга удалена"); @@ -346,7 +346,7 @@ public ResponseEntity deleteBook(@PathVariable Long id) { } ) @PreAuthorize("hasRole('ADMIN')") - @GetMapping("/id/{id}") + @GetMapping("/id/{id}") // Думаю можно просто оставить /{id} public ResponseEntity getBookById(@PathVariable Long id) { BookResponse data = BookResponse.from(bookService.getBookById(id)); return success(data); @@ -413,7 +413,7 @@ public ResponseEntity getBookById(@PathVariable Long id) { public ResponseEntity getBooksByTitle(@PathVariable String title) { List 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 getBooksByTitle(@PathVariable String title) { public ResponseEntity getBooksByAuthor(@PathVariable String author) { List data = bookService.getBooksByAuthor(author).stream() .map(BookResponse::from) - .collect(Collectors.toList()); + .collect(Collectors.toList()); // с 16 джавы есть более современный и лаконичный .toList() return success(data); } } \ No newline at end of file diff --git a/src/main/java/bookShop/controller/LoanController.java b/src/main/java/bookShop/controller/LoanController.java index b8bb349..0402344 100644 --- a/src/main/java/bookShop/controller/LoanController.java +++ b/src/main/java/bookShop/controller/LoanController.java @@ -329,7 +329,7 @@ public ResponseEntity getActiveLoansByBook(@PathVariable Long bookI List 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 getActiveLoansByUser(@PathVariable Long userI List data = loanService.getActiveLoans(userId) .stream() .map(LoanResponse::from) - .collect(Collectors.toList()); + .collect(Collectors.toList()); // с 16 джавы есть более современный и лаконичный .toList() return success(data); } } \ No newline at end of file diff --git a/src/main/java/bookShop/controller/swagger/SwaggerResponses.java b/src/main/java/bookShop/controller/swagger/SwaggerResponses.java index c4c868d..fd71a45 100644 --- a/src/main/java/bookShop/controller/swagger/SwaggerResponses.java +++ b/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 = """ diff --git a/src/main/java/bookShop/exception/ApiException.java b/src/main/java/bookShop/exception/ApiException.java index bd0e4a3..4066b37 100644 --- a/src/main/java/bookShop/exception/ApiException.java +++ b/src/main/java/bookShop/exception/ApiException.java @@ -2,6 +2,9 @@ import lombok.*; +// Я бы сказал ну слишком много отдельных классов исключений, хотя по сути все они тот же самый ApiException. +// В целом спорная тема (тут как тебе нравится), но я лично в коде где бросаю ошибку просто бросал бы ошибку с нужными параметрами +// @Getter public class ApiException extends RuntimeException { private final String errorCode; diff --git a/src/main/java/bookShop/model/AppUser.java b/src/main/java/bookShop/model/AppUser.java index f9624b6..dedd1b8 100644 --- a/src/main/java/bookShop/model/AppUser.java +++ b/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 loans; @Column(nullable = false, length = 11) private String phone; diff --git a/src/main/java/bookShop/model/Role.java b/src/main/java/bookShop/model/Role.java index d5512a8..7c3ae85 100644 --- a/src/main/java/bookShop/model/Role.java +++ b/src/main/java/bookShop/model/Role.java @@ -3,6 +3,7 @@ import lombok.AllArgsConstructor; import lombok.Getter; +// можно в отдельный пакет security @Getter @AllArgsConstructor public enum Role { diff --git a/src/main/java/bookShop/model/request/AuthRequest.java b/src/main/java/bookShop/model/request/AuthRequest.java index d2a92c9..043558d 100644 --- a/src/main/java/bookShop/model/request/AuthRequest.java +++ b/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 diff --git a/src/main/java/bookShop/model/response/BookResponse.java b/src/main/java/bookShop/model/response/BookResponse.java index cb4f1a3..c7fbb17 100644 --- a/src/main/java/bookShop/model/response/BookResponse.java +++ b/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()); diff --git a/src/main/java/bookShop/model/response/LoanResponse.java b/src/main/java/bookShop/model/response/LoanResponse.java index 58001f4..c99a86d 100644 --- a/src/main/java/bookShop/model/response/LoanResponse.java +++ b/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()); diff --git a/src/main/java/bookShop/model/response/UserResponse.java b/src/main/java/bookShop/model/response/UserResponse.java index 7c35138..f76d818 100644 --- a/src/main/java/bookShop/model/response/UserResponse.java +++ b/src/main/java/bookShop/model/response/UserResponse.java @@ -16,7 +16,7 @@ public class UserResponse { private String email; private List 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()); diff --git a/src/main/java/bookShop/security/SecurityConfig.java b/src/main/java/bookShop/security/SecurityConfig.java index ca74a93..0f31f06 100644 --- a/src/main/java/bookShop/security/SecurityConfig.java +++ b/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 diff --git a/src/main/java/bookShop/service/AuthService.java b/src/main/java/bookShop/service/AuthService.java index 2609fdc..08a43d0 100644 --- a/src/main/java/bookShop/service/AuthService.java +++ b/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()); } diff --git a/src/main/java/bookShop/service/BookService.java b/src/main/java/bookShop/service/BookService.java index b7c3c25..0e4f4fe 100644 --- a/src/main/java/bookShop/service/BookService.java +++ b/src/main/java/bookShop/service/BookService.java @@ -29,14 +29,14 @@ public Book getBookById(Long id) { } public List getBooksByTitle(String title) { - title = (title != null) ? title.trim() : null; + title = (title != null) ? title.trim() : null; //Можно еще добавить проверку на ввод пустой строки, возможно при запросе на пустую строку оператор LIKE выдаст все записи List books = bookRepository.findByTitleIgnoreCaseLike(title); if (books.isEmpty()) throw new BookNotFoundException("Книги с указанным названием не найдены"); return books; } public List getBooksByAuthor(String author) { - author = (author != null) ? author.trim() : null; + author = (author != null) ? author.trim() : null; //Можно еще добавить проверку на ввод пустой строки, возможно при запросе на пустую строку оператор LIKE выдаст все записи List 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); diff --git a/src/main/java/bookShop/service/LoyaltyService.java b/src/main/java/bookShop/service/LoyaltyService.java index 3ff762b..e5c2afd 100644 --- a/src/main/java/bookShop/service/LoyaltyService.java +++ b/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); diff --git a/src/main/java/bookShop/service/UserService.java b/src/main/java/bookShop/service/UserService.java index 1d61a99..c393839 100644 --- a/src/main/java/bookShop/service/UserService.java +++ b/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 getAllUsers() { List 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(); } diff --git a/src/main/java/bookShop/validation/LoanValidator.java b/src/main/java/bookShop/validation/LoanValidator.java index fe919bf..a3cc17e 100644 --- a/src/main/java/bookShop/validation/LoanValidator.java +++ b/src/main/java/bookShop/validation/LoanValidator.java @@ -8,6 +8,7 @@ import java.util.List; +// Думаю что вся эта логика должна быть в сервисном слое. В целом по классической структуре в БД делаем запросы только из сервисных классов @Component @RequiredArgsConstructor public class LoanValidator { diff --git a/src/main/java/bookShop/validation/NoEmojiNoXssValidator.java b/src/main/java/bookShop/validation/NoEmojiNoXssValidator.java index dd5e206..0a926bb 100644 --- a/src/main/java/bookShop/validation/NoEmojiNoXssValidator.java +++ b/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 { private static final Pattern XSS_PATTERN = Pattern.compile("[<>\"'&]"); private static final Pattern EMOJI_PATTERN = Pattern.compile( diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index a5af3f4..099c17b 100644 --- a/src/main/resources/application.properties +++ b/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 diff --git a/src/main/resources/data.sql b/src/main/resources/data.sql index e2e1670..2a0c461 100644 --- a/src/main/resources/data.sql +++ b/src/main/resources/data.sql @@ -1 +1,5 @@ INSERT INTO users (id, username, password) VALUES (1, 'admin', '$2a$10$xyz...'); + +/* + Для тестового проекта вполне достаточно, но в коммерческой разработке я бы прикрутил библиотеку миграций чтобы отслеживать запущенные скрипты и поддерживать целостность бд (например мы использует liquibase) + */ \ No newline at end of file