merge:book api #12
merge:book api #12
Conversation
feat: λμ κΈ°λ₯μ μΆκ° κ°λ°ν©λλ€
WalkthroughThis update introduces a major refactor and feature expansion to the book management module. It adds external API integration for book search, new domain fields, advanced search/sort capabilities, and robust DTOs. The service layer is modularized with interfaces and implementations, controller endpoints are redesigned, and configuration is updated for new dependencies and API credentials. Legacy value objects and DTOs are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BookController
participant BookServiceImpl
participant BookRepository
participant BookClient
participant NaverBookClient
participant ExternalBookAPI
%% Book search via external API
User->>BookController: GET /naver/search?query=...&page=...
BookController->>BookClient: searchBooks(query, page)
BookClient->>NaverBookClient: searchBooks(query, page)
NaverBookClient->>ExternalBookAPI: HTTP GET with headers (ID, Secret)
ExternalBookAPI-->>NaverBookClient: XML Response
NaverBookClient-->>BookClient: List<BookApiResponse>
BookClient-->>BookController: List<BookApiResponse>
BookController-->>User: 200 OK, List<BookApiResponse>
%% Book creation (admin)
User->>BookController: POST /book/new (CreateBookRequest)
BookController->>BookServiceImpl: createBook(member, request)
BookServiceImpl->>BookRepository: save(Book)
BookRepository-->>BookServiceImpl: Book
BookServiceImpl-->>BookController: void
BookController-->>User: 200 OK
%% Book borrowing
User->>BookController: POST /book/{id}/borrow
BookController->>BookServiceImpl: rent(member, id)
BookServiceImpl->>BookRepository: findBookById(id)
BookRepository-->>BookServiceImpl: Optional<Book>
BookServiceImpl-->>BookRepository: save(Book)
BookRepository-->>BookServiceImpl: Book
BookServiceImpl-->>BookController: void
BookController-->>User: 200 OK
Poem
β¨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. πͺ§ TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
π Outside diff range comments (2)
src/main/java/org/poolc/api/activity/domain/Session.java (2)
74-82: Remove@Transactionalfrom entity methods β shift transaction boundaries to the service layer
Using Springβs@Transactionalannotation inside an entity couples domain objects to the infrastructure layer and makes unit-testing harder. Apply transactions in the service/repository layer instead.
79-82:attendmay violate DB unique constraint β deduplicate beforeaddAll
attendedMemberLoginIDsis persisted with a(session_id, member_loginid)unique constraint. Blindly appending may triggerConstraintViolationExceptionwhen the same member is passed twice or is already stored.- public void attend(List<String> attendances) { - this.attendedMemberLoginIDs.addAll(attendances); + public void attend(List<String> attendances) { + attendances.stream() + .filter(loginId -> !this.attendedMemberLoginIDs.contains(loginId)) + .forEach(this.attendedMemberLoginIDs::add); }Consider switching the collection to a
Set<String>(see lines 45-49) for O(1) look-ups and built-in de-duplication.
π§Ή Nitpick comments (16)
src/main/java/org/poolc/api/activity/domain/Session.java (1)
45-49: PreferSet<String>forattendedMemberLoginIDsto guarantee uniqueness
ASeteliminates duplicates by design and communicates intent better than aList.- private List<String> attendedMemberLoginIDs = new ArrayList<>(); + private Set<String> attendedMemberLoginIDs = new HashSet<>();Adjust corresponding getter and Hibernate mapping accordingly.
src/main/java/org/poolc/api/badge/service/BadgeService.java (1)
142-153: Good addition of @transactional annotation and code simplification.The
@Transactionalannotation is appropriate here since the method performs database operations (saving BadgeLog). The removal of the redundant internal check streamlines the code nicely.However, there's a potential logical issue: the method calls
getBadgeByBadgeId(badgeId)which uses.get()without checking if the Optional is present, even though the if condition already verifiesbadgeRepository.findBadgeById(badgeId).isPresent(). This could lead to an inconsistent state if the badge is deleted between the check and the retrieval.Consider this more robust approach:
public void badgeGiver(Member member, Long badgeId){ - if(duplicateBadgeLogCheck(badgeId, member)&&badgeRepository.findBadgeById(badgeId).isPresent()){ - Badge badge = getBadgeByBadgeId(badgeId); + if(duplicateBadgeLogCheck(badgeId, member)){ + Badge badge = badgeRepository.findBadgeById(badgeId) + .orElseThrow(() -> new NoSuchElementException("ν΄λΉνλ λ±μ§κ° μμ΅λλ€.")); badgeLogRepository.save(BadgeLog.builder() .member(member) .date(LocalDate.now()) .badge(badge) .build()); notificationService.createBadgeNotification(member); } }src/test/java/org/poolc/api/book/BookDataLoader.java (2)
14-14: Remove commented code or use a cleaner approach for disabling functionality.While commenting out the
implements CommandLineRunnerachieves the goal of disabling test data loading, this approach has several drawbacks:
- Commented code should generally be removed from the codebase
- The
@Profile("bookTest")annotation is still present but ineffective- Future developers might be confused by the commented implementation
Consider one of these alternatives:
Option 1: Remove the functionality entirely if it's no longer needed:
-@Component -@Profile("bookTest") -@RequiredArgsConstructor -public class BookDataLoader /*implements CommandLineRunner*/ { - // ... entire class content -}Option 2: Use a feature flag or conditional logic:
@Component @Profile("bookTest") @RequiredArgsConstructor public class BookDataLoader implements CommandLineRunner { @Value("${book.test.data.enabled:false}") private boolean testDataEnabled; @Override public void run(String... args) { if (!testDataEnabled) { return; } // ... existing logic } }
18-39: Clean up commented code block.The entire
runmethod is commented out, which leaves dead code in the repository. This reduces code readability and maintainability.If this functionality is truly no longer needed, remove the commented code entirely. If it might be needed in the future, consider using version control history to preserve it rather than keeping commented code in the active codebase.
src/main/resources/application.yml (1)
35-40: LGTM! Good use of environment variables for API configuration.The configuration structure is clean and follows Spring Boot conventions. Using environment variables for API credentials is a security best practice that prevents sensitive information from being committed to version control.
Consider adding validation or default values to make the configuration more robust:
book: api: url: ${NAVER_BOOK_API_URL:https://openapi.naver.com/v1/search/book.xml} id: ${NAVER_BOOK_CLIENT_ID:} secret: ${NAVER_BOOK_CLIENT_SECRET:} # Optional: Add timeout and retry configurations timeout: ${NAVER_BOOK_API_TIMEOUT:5000} max-retries: ${NAVER_BOOK_API_MAX_RETRIES:3}This would provide:
- Default URL for development/testing
- Clear indication when credentials are missing
- Additional configuration options for API behavior
src/main/java/org/poolc/api/book/domain/BorrowStatus.java (1)
1-5: LGTM! Clean enum definition for book borrowing states.The
BorrowStatusenum appropriately represents the lifecycle of a borrowed book with four well-defined states. The naming is clear and follows Java conventions.Consider adding JavaDoc documentation to clarify the meaning and transitions between states:
+/** + * Represents the status of a borrowed book throughout its borrowing lifecycle. + */ public enum BorrowStatus { + /** Book has been borrowed and is currently with the borrower */ BORROWED, + /** Book has been returned to the library */ RETURNED, + /** Borrowing period has been extended */ EXTENDED, + /** Book is past its due date */ OVERDUE }Additionally, if you need to associate behavior with states in the future, consider adding methods to the enum:
public enum BorrowStatus { BORROWED(true), RETURNED(false), EXTENDED(true), OVERDUE(true); private final boolean isActive; BorrowStatus(boolean isActive) { this.isActive = isActive; } public boolean isActive() { return isActive; } }src/test/java/org/poolc/api/book/BookAcceptanceTest.java (1)
20-333: Update and re-enable the acceptance tests.The entire test class has been commented out, which removes important test coverage for the book API. While this may be necessary during the refactoring process, these tests should be updated to use the new DTOs and API structure as soon as possible.
The tests reference old DTOs like
BookRequestandBookResponsethat have been replaced. Please update the tests to use the new DTOs from thedto.requestanddto.responsepackages and re-enable them.Would you like me to help generate updated test cases that work with the new API structure?
src/main/java/org/poolc/api/configuration/RestTemplateConfig.java (1)
10-14: Consider adding production-ready configuration to RestTemplate.The current configuration uses default RestTemplate settings, which may not be optimal for production use. Consider adding timeouts, connection pooling, and error handling.
Here's a more production-ready configuration:
@Bean public RestTemplate restTemplate() { - return new RestTemplate(); + RestTemplate restTemplate = new RestTemplate(); + + // Configure timeouts + HttpComponentsClientHttpRequestFactory factory = new HttpComponentsClientHttpRequestFactory(); + factory.setConnectTimeout(5000); + factory.setReadTimeout(10000); + + // Configure connection pooling + CloseableHttpClient httpClient = HttpClients.custom() + .setMaxConnTotal(50) + .setMaxConnPerRoute(20) + .build(); + factory.setHttpClient(httpClient); + + restTemplate.setRequestFactory(factory); + return restTemplate; }You'll also need to add these imports:
import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClients; import org.springframework.http.client.HttpComponentsClientHttpRequestFactory;And add this dependency to your
pom.xml:<dependency> <groupId>org.apache.httpcomponents</groupId> <artifactId>httpclient</artifactId> </dependency>src/main/java/org/poolc/api/book/dto/response/BookApiResponse.java (1)
1-39: LGTM! Clean DTO implementationThe DTO is well-structured with appropriate Jackson XML annotations for API response mapping.
Consider making the DTO immutable by using
@Valueinstead of@Getterwith the other annotations, or add@FieldDefaults(makeFinal = true, level = AccessLevel.PRIVATE):-@Getter +@Value @Builder @AllArgsConstructor -@NoArgsConstructor +@NoArgsConstructor(force = true) public class BookApiResponse {src/main/java/org/poolc/api/book/repository/BookRepository.java (1)
4-15: Organize imports for better readability.Group related imports together for consistency.
Reorganize imports:
import org.poolc.api.book.domain.Book; import org.poolc.api.book.domain.BookSortOption; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; import org.springframework.data.jpa.domain.Specification; import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.data.jpa.repository.JpaSpecificationExecutor; +import org.springframework.data.jpa.repository.Query; +import org.springframework.data.repository.query.Param; import java.util.Optional; - -import org.springframework.data.jpa.repository.JpaSpecificationExecutor; -import org.springframework.data.jpa.repository.Query; -import org.springframework.data.repository.query.Param;src/main/java/org/poolc/api/book/dto/request/CreateBookRequest.java (3)
3-10: Remove duplicate import statement.
NotNullis imported twice.Remove the duplicate import:
import javax.validation.constraints.NotBlank; import javax.validation.constraints.Size; import lombok.AllArgsConstructor; import lombok.Getter; import lombok.NoArgsConstructor; -import javax.validation.constraints.NotNull; import java.util.List;
40-41: Add URL validation for image and link fields.The image and link fields should contain valid URLs.
Add URL validation:
@NotBlank + @Pattern(regexp = "^https?://.*", message = "Must be a valid URL") private String image; @NotNull private Integer discount; @NotBlank + @Pattern(regexp = "^https?://.*", message = "Must be a valid URL") private String link;Also applies to: 46-47
43-44: Add range validation for discount field.Discount values should typically be within a reasonable range (e.g., 0-100 for percentage).
Add range validation:
@NotNull + @Min(0) + @Max(100) private Integer discount;You'll need to import
javax.validation.constraints.Minandjavax.validation.constraints.Max.src/main/java/org/poolc/api/book/domain/Book.java (1)
85-89: Consider preserving rental history instead of nullifying data.Setting
rentDateto null loses historical information. Consider keeping this data for analytics or audit purposes, or maintain a separate rental history table.src/main/java/org/poolc/api/book/service/BookServiceImpl.java (1)
21-21: Consider making PAGE_SIZE configurable.The hardcoded value of 10 could be moved to application properties for easier configuration across environments.
-private static final int PAGE_SIZE = 10; +@Value("${book.page.size:10}") +private int pageSize;src/main/java/org/poolc/api/book/controller/BookController.java (1)
44-44: Fix minor formatting inconsistencies.-@RequestParam(value = "search", required = true)BookSearchOption searchOption, +@RequestParam(value = "search", required = true) BookSearchOption searchOption, -return new ResponseEntity<>(bookService.searchBooks(page,searchOption,keyword,sortOption), HttpStatus.OK); +return new ResponseEntity<>(bookService.searchBooks(page, searchOption, keyword, sortOption), HttpStatus.OK);Also applies to: 48-48
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (30)
README.md(0 hunks)build.gradle(1 hunks)src/main/java/org/poolc/api/activity/domain/Session.java(1 hunks)src/main/java/org/poolc/api/auth/configurations/WebSecurityConfig.java(2 hunks)src/main/java/org/poolc/api/badge/service/BadgeService.java(1 hunks)src/main/java/org/poolc/api/book/client/BookClient.java(1 hunks)src/main/java/org/poolc/api/book/client/NaverBookClient.java(1 hunks)src/main/java/org/poolc/api/book/controller/BookController.java(1 hunks)src/main/java/org/poolc/api/book/domain/Book.java(3 hunks)src/main/java/org/poolc/api/book/domain/BookSearchOption.java(1 hunks)src/main/java/org/poolc/api/book/domain/BookSortOption.java(1 hunks)src/main/java/org/poolc/api/book/domain/BookSpecification.java(1 hunks)src/main/java/org/poolc/api/book/domain/BorrowStatus.java(1 hunks)src/main/java/org/poolc/api/book/dto/BookRequest.java(0 hunks)src/main/java/org/poolc/api/book/dto/BookResponse.java(0 hunks)src/main/java/org/poolc/api/book/dto/request/CreateBookRequest.java(1 hunks)src/main/java/org/poolc/api/book/dto/request/UpdateBookRequest.java(1 hunks)src/main/java/org/poolc/api/book/dto/response/BookApiResponse.java(1 hunks)src/main/java/org/poolc/api/book/dto/response/BookResponse.java(1 hunks)src/main/java/org/poolc/api/book/dto/response/NaverApiResponse.java(1 hunks)src/main/java/org/poolc/api/book/repository/BookRepository.java(1 hunks)src/main/java/org/poolc/api/book/service/BookService.java(1 hunks)src/main/java/org/poolc/api/book/service/BookServiceImpl.java(1 hunks)src/main/java/org/poolc/api/book/vo/BookCreateValues.java(0 hunks)src/main/java/org/poolc/api/book/vo/BookUpdateValues.java(0 hunks)src/main/java/org/poolc/api/book/vo/BorrowerValues.java(0 hunks)src/main/java/org/poolc/api/configuration/RestTemplateConfig.java(1 hunks)src/main/resources/application.yml(1 hunks)src/test/java/org/poolc/api/book/BookAcceptanceTest.java(2 hunks)src/test/java/org/poolc/api/book/BookDataLoader.java(2 hunks)
π€ Files with no reviewable changes (6)
- README.md
- src/main/java/org/poolc/api/book/dto/BookRequest.java
- src/main/java/org/poolc/api/book/vo/BookUpdateValues.java
- src/main/java/org/poolc/api/book/vo/BorrowerValues.java
- src/main/java/org/poolc/api/book/vo/BookCreateValues.java
- src/main/java/org/poolc/api/book/dto/BookResponse.java
π Additional comments (11)
src/main/java/org/poolc/api/activity/domain/Session.java (2)
85-93:equals/hashCoderely on lazy association β may trigger unintended DB hits & proxy issues
Comparingactivity(a LAZY proxy) inside equality can cause extra queries and inconsistent behavior across persistence contexts. Limit equality to immutable identifiers (e.g.,id) unless you have a strict requirement otherwise.
101-101: Added trailing newline β looks good
File now conforms to POSIX newline convention.build.gradle (1)
69-72: Confirm Jackson version compatibility and necessityBefore merging, please verify the following:
- Spring Boot 2.4.2 manages Jackson at version 2.11.4. Overriding to 2.18.1 may introduce API or serialization mismatches at runtime.
- If you need features in 2.18.1, either
β’ Upgrade Spring Boot to a release that officially supports Jackson 2.18.x (e.g. 3.4.0), or
β’ Manually override and exhaustively test all serialization/deserialization paths.- Spring Boot already brings in core Jackson modules transitively. You may only need to declare
jackson-dataformat-xml; the core, annotations, and databind modules could be redundant.Locations to review:
- build.gradle (Lines 69β72) β confirm which explicit artifacts are truly required.
To inspect your local dependency graph, set your JAVA_HOME appropriately and run:
export JAVA_HOME=/path/to/your/jdk ./gradlew dependencies --configuration compileClasspath | grep jackson ./gradlew dependencyInsight --dependency jackson-databind --configuration compileClasspathAfter confirming no conflicts or redundancies, approve or adjust the version as needed.
src/main/java/org/poolc/api/auth/configurations/WebSecurityConfig.java (2)
99-99: Security rule addition looks goodThe new security rule appropriately restricts POST operations on book endpoints to ADMIN users, maintaining consistency with the existing security model.
41-48: Fix syntax error: missing comma in allowed origins listThere's a missing comma at the end of line 44 that will cause a compilation error.
Apply this fix:
- ,"https://poolc.org/api", "http://poolc.org/api", - "https://dev.poolc.org", "http://dev.poolc.org" + ,"https://poolc.org/api", "http://poolc.org/api", + "https://dev.poolc.org", "http://dev.poolc.org",Likely an incorrect or invalid review comment.
src/main/java/org/poolc/api/book/dto/response/NaverApiResponse.java (1)
1-43: Well-structured XML response mappingThe DTO correctly models the Naver API XML response structure with appropriate annotations. Good use of
@JsonIgnoreProperties(ignoreUnknown = true)for forward compatibility.src/main/java/org/poolc/api/book/dto/response/BookResponse.java (1)
38-56: Excellent DTO implementation with proper null handlingThe static factory method correctly converts from the Book entity and handles the optional renter field gracefully. The comprehensive field mapping aligns well with the expanded book domain model.
src/main/java/org/poolc/api/book/domain/BookSpecification.java (1)
11-11: No SQL injection risk withCriteriaBuilder.likeβ inputs are bound as parametersThe
CriteriaBuilder.like(root.get(...), pattern)call uses JDBC parameter binding, so user input isnβt executed as SQL. If your goal is to prevent%or_in the keyword from acting as wildcards (rather than for security), you can optionally escape them:// src/main/java/org/poolc/api/book/domain/BookSpecification.java String escapedKeyword = keyword .replace("\\", "\\\\") .replace("%", "\\%") .replace("_", "\\_"); Predicate predicate = criteriaBuilder.like( root.get("title"), "%" + escapedKeyword + "%", '\\' );β’ Applies likewise in
findByAuthorAndSortOption
β’ No change needed forfindByTagsContainingAndSortOption(usesisMember)No fixes required.
Likely an incorrect or invalid review comment.
src/main/java/org/poolc/api/book/domain/Book.java (3)
21-23: Good use of Lombok annotations for builder pattern.The addition of
@AllArgsConstructorand@Builderannotations provides a clean API for entity creation while maintaining JPA compatibility with the protected no-args constructor.
32-70: Well-structured entity fields with appropriate column definitions.The field additions and renaming (borrowerβrenter) improve clarity. Good choices for column types:
- VARCHAR(600) for URLs accommodates long URLs
- TEXT for description handles large content
- @ElementCollection for tags provides simple many-to-many relationship
91-103: Excellent defensive programming in the update method.The null-checking pattern ensures only explicitly provided fields are updated, preventing accidental data loss.
| private final String value; | ||
|
|
||
| BookSearchOption(String value) { | ||
| this.value = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Add a getter method for the value field.
The enum follows a common pattern of having a private value field, but it's missing a public getter method to access this value. This is typically needed when the enum is used in other parts of the codebase.
Add this getter method:
BookSearchOption(String value) {
this.value = value;
}
+
+ public String getValue() {
+ return value;
+ }π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private final String value; | |
| BookSearchOption(String value) { | |
| this.value = value; | |
| } | |
| } | |
| private final String value; | |
| BookSearchOption(String value) { | |
| this.value = value; | |
| } | |
| public String getValue() { | |
| return value; | |
| } | |
| } |
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/domain/BookSearchOption.java between lines 8
and 13, the enum has a private final String field named value but lacks a public
getter method. Add a public method named getValue() that returns this.value to
allow external access to the value field.
| private final String value; | ||
|
|
||
| BookSortOption(String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Add a getter method for the value field.
Similar to BookSearchOption, this enum is missing a public getter method to access the value field. This is needed for consistency and proper usage throughout the codebase.
Add this getter method:
BookSortOption(String value) {
this.value = value;
}
+
+ public String getValue() {
+ return value;
+ }π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private final String value; | |
| BookSortOption(String value) { | |
| this.value = value; | |
| } | |
| } | |
| private final String value; | |
| BookSortOption(String value) { | |
| this.value = value; | |
| } | |
| public String getValue() { | |
| return value; | |
| } | |
| } |
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/domain/BookSortOption.java between lines 9
and 15, the enum has a private final field 'value' but lacks a public getter
method to access it. Add a public method named getValue() that returns the value
field to provide consistent access similar to BookSearchOption.
|
|
||
| import org.poolc.api.book.dto.response.BookApiResponse; | ||
|
|
||
| import javax.management.modelmbean.XMLParseException; |
There was a problem hiding this comment.
Use appropriate exception type for XML parsing.
The import javax.management.modelmbean.XMLParseException is incorrect for general XML parsing. This exception is specifically for JMX management beans, not for parsing XML responses from external APIs.
Consider using a more appropriate exception type:
-import javax.management.modelmbean.XMLParseException;
+import com.fasterxml.jackson.core.JsonProcessingException;And update the method signature:
- List<BookApiResponse> searchBooks(String query, int page) throws XMLParseException;
+ List<BookApiResponse> searchBooks(String query, int page) throws JsonProcessingException;Alternatively, you could use a more general exception like Exception or create a custom exception class for book client operations.
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/client/BookClient.java at line 5, replace
the incorrect import of javax.management.modelmbean.XMLParseException with a
more appropriate XML parsing exception such as org.xml.sax.SAXException or
javax.xml.parsers.ParserConfigurationException depending on the XML parsing
library used. Also, update any method signatures and exception handling code to
reflect this change, or alternatively use a general Exception or define a custom
exception class for book client errors.
| import org.springframework.stereotype.Component; | ||
| import org.springframework.web.client.RestTemplate; | ||
|
|
||
| import javax.management.modelmbean.XMLParseException; |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Use appropriate exception type for XML parsing
XMLParseException from javax.management.modelmbean is not intended for general XML parsing errors. Consider using a more appropriate exception type.
Options:
- Create a custom exception:
BookApiExceptionorXmlParsingException - Use
org.springframework.web.client.RestClientException - Use
com.fasterxml.jackson.core.JsonProcessingException(which XmlMapper actually throws)
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/client/NaverBookClient.java at line 14,
replace the import of javax.management.modelmbean.XMLParseException with a more
appropriate exception type for XML parsing errors. You can either create a
custom exception like BookApiException or XmlParsingException, or use an
existing relevant exception such as
org.springframework.web.client.RestClientException or
com.fasterxml.jackson.core.JsonProcessingException, which is actually thrown by
XmlMapper. Update the import and exception handling accordingly to reflect this
change.
| String url = new StringBuilder(this.url) | ||
| .append("?query=").append(query) | ||
| .append("&display=").append(PAGE_SIZE) | ||
| .append("&start=").append(page * PAGE_SIZE + 1) | ||
| .toString(); |
There was a problem hiding this comment.
URL encode query parameter to handle special characters
The query parameter should be URL encoded to properly handle special characters, spaces, and non-ASCII text.
Add URL encoding:
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
String url = new StringBuilder(this.url)
- .append("?query=").append(query)
+ .append("?query=").append(URLEncoder.encode(query, StandardCharsets.UTF_8))
.append("&display=").append(PAGE_SIZE)
.append("&start=").append(page * PAGE_SIZE + 1)
.toString();π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/client/NaverBookClient.java around lines 42
to 46, the query parameter in the URL is not URL encoded, which can cause issues
with special characters and spaces. Fix this by applying URL encoding to the
query string before appending it to the URL. Use a standard encoding method like
URLEncoder.encode(query, "UTF-8") to ensure proper encoding of the query
parameter.
| @NotBlank | ||
| @Size(max = 100) | ||
| private String isbn; |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Add ISBN format validation.
ISBN should follow specific formats (ISBN-10 or ISBN-13). The current validation only checks length.
Add pattern validation for ISBN:
@NotBlank
@Size(max = 100)
+ @Pattern(regexp = "^(97[89])?\\d{9}[\\dX]$", message = "Invalid ISBN format")
private String isbn;You'll need to import javax.validation.constraints.Pattern.
π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @NotBlank | |
| @Size(max = 100) | |
| private String isbn; | |
| @NotBlank | |
| @Size(max = 100) | |
| @Pattern(regexp = "^(97[89])?\\d{9}[\\dX]$", message = "Invalid ISBN format") | |
| private String isbn; |
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/dto/request/CreateBookRequest.java around
lines 29 to 31, the isbn field currently only has @NotBlank and @Size
validations, which do not enforce the correct ISBN format. Add a @Pattern
annotation with a regex that matches valid ISBN-10 or ISBN-13 formats to ensure
proper format validation. Also, import javax.validation.constraints.Pattern to
support this annotation.
| public void deleteBook(Member member, Long id) throws Exception { | ||
| checkAdmin(member); | ||
| Book book = bookRepository.findById(id) | ||
| .orElseThrow(() -> new Exception("μ± μ μ°Ύμ μμ΅λλ€. id: " + id)); |
There was a problem hiding this comment.
Fix typo in Korean error messages.
The Korean text is missing a character. "μ± μ μ°Ύμ μμ΅λλ€" should be "μ± μ μ°Ύμ μ μμ΅λλ€".
-.orElseThrow(() -> new Exception("μ±
μ μ°Ύμ μμ΅λλ€. id: " + id));
+.orElseThrow(() -> new Exception("μ±
μ μ°Ύμ μ μμ΅λλ€. id: " + id));Also applies to: 96-96, 104-104, 115-115, 129-129
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/service/BookServiceImpl.java at lines 87,
96, 104, 115, and 129, fix the typo in the Korean error messages by changing "μ±
μ
μ°Ύμ μμ΅λλ€" to "μ±
μ μ°Ύμ μ μμ΅λλ€" to correct the grammar in all these exception
messages.
| public void deleteBook(Member member, Long id) throws Exception { | ||
| checkAdmin(member); | ||
| Book book = bookRepository.findById(id) | ||
| .orElseThrow(() -> new Exception("μ± μ μ°Ύμ μμ΅λλ€. id: " + id)); | ||
| bookRepository.delete(book); | ||
| } |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Use specific exception types instead of generic Exception.
Consider using domain-specific exceptions like BookNotFoundException, UnauthorizedException, etc., instead of generic Exception for better error handling.
Also applies to: 93-98, 102-109, 113-123
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/service/BookServiceImpl.java around lines 84
to 89, replace the generic Exception with more specific custom exceptions such
as BookNotFoundException when a book is not found and UnauthorizedException for
authorization failures. Define these exceptions if they do not exist, and update
the method signatures and throw statements accordingly. Apply the same changes
to the other indicated line ranges (93-98, 102-109, 113-123) to improve error
handling clarity and specificity.
| @Override | ||
| public Page<BookResponse> getAllBooks(int page, BookSortOption option) { | ||
| Page<Book> books; | ||
| // System.out.println("option: " + option); |
There was a problem hiding this comment.
Remove commented debug code.
-// System.out.println("option: " + option);π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // System.out.println("option: " + option); |
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/service/BookServiceImpl.java at line 26,
remove the commented out debug statement "System.out.println("option: " +
option);" entirely to clean up the code and avoid leaving unnecessary commented
code.
| @GetMapping("/naver/search") | ||
| public ResponseEntity<List<BookApiResponse>> searchBooksFromAPI(@RequestParam String query, | ||
| @RequestParam(value = "page", defaultValue = "0") @Min(0) Integer page) { | ||
| try { | ||
| return new ResponseEntity<>(bookClient.searchBooks(query, page), HttpStatus.OK); | ||
| } catch (Exception e) { | ||
| return new ResponseEntity<>(null, HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } | ||
| } |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Improve error handling with specific HTTP status codes.
The generic catch-all exception handling returns 500 for all errors, making it difficult to distinguish between validation errors (400), not found (404), unauthorized (403), etc. Consider using @ExceptionHandler or returning appropriate status codes based on exception types.
Example approach:
@ExceptionHandler(IllegalArgumentException.class)
public ResponseEntity<String> handleBadRequest(IllegalArgumentException e) {
return ResponseEntity.badRequest().body(e.getMessage());
}
@ExceptionHandler(BookNotFoundException.class)
public ResponseEntity<String> handleNotFound(BookNotFoundException e) {
return ResponseEntity.notFound().build();
}Also applies to: 41-52, 54-63, 65-72, 74-83, 85-93, 95-104, 106-114, 116-124
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/book/controller/BookController.java around lines
31 to 39, the current error handling catches all exceptions and returns a
generic 500 status, which obscures specific error causes. Refactor by removing
the generic try-catch in the controller method and instead implement specific
@ExceptionHandler methods in the controller for different exception types like
IllegalArgumentException (return 400 Bad Request), BookNotFoundException (return
404 Not Found), and others as needed. This will allow returning precise HTTP
status codes based on the exception type and improve API error clarity. Apply
the same pattern to other similar methods in the specified line ranges.
π Issue Number
Close #
π μμ λ΄μ
π μμ μ ν
π 체ν¬λ¦¬μ€νΈ
π PR νΉμ΄ μ¬ν
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Documentation