Conversation
|
""" WalkthroughA new bookmark feature has been introduced, including REST API endpoints, service and repository layers, JPA entity, and comprehensive tests. Documentation for the API and its error codes has been added and integrated into the documentation index. The changes cover both implementation and verification of the bookmark registration process. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BookmarkController
participant BookmarkService
participant SecurityContext
participant UserRepository
participant ImageRepository
participant BookmarkRepository
Client->>BookmarkController: POST /v1/bookmarks?imageId={id}
BookmarkController->>BookmarkService: addBookmark(imageId)
BookmarkService->>SecurityContext: getAuthentication()
SecurityContext-->>BookmarkService: Authentication (principal: username)
BookmarkService->>UserRepository: findByUsername(username)
UserRepository-->>BookmarkService: User or throw USER_NOT_FOUND
BookmarkService->>ImageRepository: findById(imageId)
ImageRepository-->>BookmarkService: Image or throw IMAGE_NOT_FOUND
BookmarkService->>BookmarkRepository: existsByUserAndImage(user, image)
BookmarkRepository-->>BookmarkService: boolean (exists or not)
alt Not exists
BookmarkService->>BookmarkRepository: save(new Bookmark(user, image))
BookmarkRepository-->>BookmarkService: saved Bookmark
else Exists
BookmarkService-->>BookmarkController: throw CoreException(BOOKMARK_DUPLICATION)
end
BookmarkService-->>BookmarkController: void
BookmarkController-->>Client: ApiResponse (success)
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 4
🧹 Nitpick comments (7)
capturecat-core/src/docs/asciidoc/error-codes.adoc (1)
36-40: Remove unnecessary blank lines.The extra blank lines before the bookmark registration section are not needed and affect documentation formatting consistency.
- - - - -capturecat-core/src/main/java/com/capturecat/core/domain/bookmark/BookmarkRepository.java (1)
5-6: Consider adding bookmark-specific query methods.While the basic JpaRepository provides CRUD operations, bookmark functionality typically requires additional methods. Consider adding methods like:
boolean existsByUserAndImage(User user, Image image); List<Bookmark> findByUserId(Long userId); void deleteByUserAndImage(User user, Image image);This would prevent duplicate bookmarks and enable efficient bookmark retrieval.
capturecat-core/src/test/java/com/capturecat/core/domain/bookmark/BookmarkTest.java (1)
13-24: Expand test coverage for the Bookmark entity.The current test only verifies object creation. Consider adding tests for:
- Bookmark entity properties and relationships
- Validation constraints if any
- Equals/hashCode methods if implemented
- Edge cases with null values
@Test void 북마크_엔티티_속성_검증() { // given User user = DummyObject.newMockUser(1L); Image image = DummyObject.newMockImage(1L); // when Bookmark bookmark = new Bookmark(user, image); // then assertThat(bookmark.getUser()).isEqualTo(user); assertThat(bookmark.getImage()).isEqualTo(image); assertThat(bookmark.getCreatedAt()).isNotNull(); }capturecat-core/src/main/java/com/capturecat/core/service/bookmark/BookmarkService.java (1)
33-38: Consider optimizing database queries if needed.The service makes three separate database calls. While this is acceptable for the current use case, consider optimizing if this becomes a performance bottleneck.
For future optimization, you could:
- Use batch operations if multiple bookmarks are added simultaneously
- Consider caching frequently accessed user data
- Use database-level constraints instead of application-level checks for duplicate prevention
capturecat-core/src/test/java/com/capturecat/core/service/bookmark/BookmarkServiceTest.java (3)
48-67: Consider verifying the actual bookmark object passed to save().The test correctly verifies the service behavior, but it could be more specific about the bookmark object being saved.
Consider using an ArgumentCaptor to verify the actual bookmark object:
+import org.mockito.ArgumentCaptor; @Test void 북마크를_한다() { // given var user = DummyObject.newMockUser(1L); var image = DummyObject.newMockImage(1L); setupLoggedInUser(user); given(userRepository.findByUsername(anyString())) .willReturn(Optional.of(user)); given(imageRepository.findById(anyLong())) .willReturn(Optional.of(image)); given(bookmarkRepository.save(any())) .willReturn(new Bookmark(user, image)); // when bookmarkService.addBookmark(image.getId()); // then + ArgumentCaptor<Bookmark> bookmarkCaptor = ArgumentCaptor.forClass(Bookmark.class); + verify(bookmarkRepository).save(bookmarkCaptor.capture()); + Bookmark savedBookmark = bookmarkCaptor.getValue(); + assertThat(savedBookmark.getUser()).isEqualTo(user); + assertThat(savedBookmark.getImage()).isEqualTo(image); - verify(bookmarkRepository).save(any(Bookmark.class)); }
84-99: Fix the typo in the test method name.There's a typo in the method name that should be corrected for consistency.
-void 북마크를_할_때_이미지가_존재하지_읺으면_실패한다() { +void 북마크를_할_때_이미지가_존재하지_않으면_실패한다() {The test logic itself is correct and properly handles the image not found scenario.
101-107: Security context setup is functional but could be more robust.The helper method correctly mocks the Spring Security context, but consider using a more explicit approach to avoid potential issues with static state.
Consider using
@WithMockUserannotation or cleaning up the SecurityContext after each test:+import org.junit.jupiter.api.AfterEach; +@AfterEach +void clearSecurityContext() { + SecurityContextHolder.clearContext(); +} private void setupLoggedInUser(User user) { Authentication authentication = Mockito.mock(Authentication.class); SecurityContext securityContext = Mockito.mock(SecurityContext.class); given(securityContext.getAuthentication()).willReturn(authentication); SecurityContextHolder.setContext(securityContext); given(authentication.getPrincipal()).willReturn(new LoginUser(user)); }Alternatively, consider using
@WithMockUserfor simpler authentication setup if it fits your testing strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
capturecat-core/src/docs/asciidoc/bookmark.adoc(1 hunks)capturecat-core/src/docs/asciidoc/error-codes.adoc(1 hunks)capturecat-core/src/docs/asciidoc/index.adoc(1 hunks)capturecat-core/src/main/java/com/capturecat/core/api/bookmark/BookmarkController.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/domain/bookmark/Bookmark.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/domain/bookmark/BookmarkRepository.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/service/bookmark/BookmarkService.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/api/bookmark/BookmarkControllerTest.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/domain/bookmark/BookmarkTest.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/service/bookmark/BookmarkServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
capturecat-core/src/test/java/com/capturecat/core/domain/bookmark/BookmarkTest.java (1)
capturecat-core/src/test/java/com/capturecat/core/DummyObject.java (1)
DummyObject(16-63)
capturecat-core/src/test/java/com/capturecat/core/api/bookmark/BookmarkControllerTest.java (1)
capturecat-tests/api-docs/src/main/java/com/capturecat/test/api/RestDocsUtil.java (1)
RestDocsUtil(7-22)
capturecat-core/src/test/java/com/capturecat/core/service/bookmark/BookmarkServiceTest.java (1)
capturecat-core/src/test/java/com/capturecat/core/DummyObject.java (1)
DummyObject(16-63)
🔇 Additional comments (7)
capturecat-core/src/docs/asciidoc/index.adoc (1)
18-18: LGTM! Documentation integration looks correct.The bookmark documentation is properly integrated into the main documentation index with correct placement and formatting.
capturecat-core/src/docs/asciidoc/error-codes.adoc (1)
41-43: Documentation structure looks good.The bookmark registration error codes section follows the established pattern and is properly formatted.
capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java (1)
89-94: Error code documentation test looks correct.The test method follows the established pattern and correctly documents the expected error codes (
USER_NOT_FOUNDandIMAGE_NOT_FOUND) for the bookmark registration API.capturecat-core/src/docs/asciidoc/bookmark.adoc (1)
1-13: Documentation structure looks good.The AsciiDoc documentation is well-organized with clear sections for success and failure scenarios. The Korean text is properly formatted and the REST Docs snippet references align with the test implementation.
capturecat-core/src/test/java/com/capturecat/core/service/bookmark/BookmarkServiceTest.java (3)
1-32: Import statements and dependencies look good.The imports are well-organized and include all necessary testing dependencies. The use of static imports for AssertJ and Mockito improves readability.
33-46: Test class setup is properly configured.The MockitoExtension and dependency injection setup is correct. The mocked repositories (BookmarkRepository, ImageRepository, UserRepository) cover all the dependencies needed for the BookmarkService.
69-82: Error handling test for user not found is well-implemented.The test properly simulates the scenario where a user is not found and verifies that no bookmark is saved. The use of
never()verification is appropriate.
| @Entity | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| public class Bookmark { | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| private Long id; | ||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "user_id") | ||
| private User user; | ||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "image_id") | ||
| private Image image; | ||
|
|
||
| public Bookmark(User user, Image image) { | ||
| this.user = user; | ||
| this.image = image; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add unique constraint to prevent duplicate bookmarks.
The entity allows multiple bookmarks for the same user-image combination, which could lead to data integrity issues and duplicate entries.
Add a unique constraint on the user-image combination:
@Entity
+@Table(uniqueConstraints = @UniqueConstraint(columnNames = {"user_id", "image_id"}))
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class Bookmark {You'll also need to add the @Table import:
+import jakarta.persistence.Table;
+import jakarta.persistence.UniqueConstraint;📝 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.
| @Entity | |
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | |
| public class Bookmark { | |
| @Id | |
| @GeneratedValue(strategy = GenerationType.IDENTITY) | |
| private Long id; | |
| @ManyToOne(fetch = FetchType.LAZY) | |
| @JoinColumn(name = "user_id") | |
| private User user; | |
| @ManyToOne(fetch = FetchType.LAZY) | |
| @JoinColumn(name = "image_id") | |
| private Image image; | |
| public Bookmark(User user, Image image) { | |
| this.user = user; | |
| this.image = image; | |
| } | |
| } | |
| // add these imports | |
| import jakarta.persistence.Table; | |
| import jakarta.persistence.UniqueConstraint; | |
| @Entity | |
| @Table(uniqueConstraints = @UniqueConstraint(columnNames = {"user_id", "image_id"})) | |
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | |
| public class Bookmark { | |
| @Id | |
| @GeneratedValue(strategy = GenerationType.IDENTITY) | |
| private Long id; | |
| @ManyToOne(fetch = FetchType.LAZY) | |
| @JoinColumn(name = "user_id") | |
| private User user; | |
| @ManyToOne(fetch = FetchType.LAZY) | |
| @JoinColumn(name = "image_id") | |
| private Image image; | |
| public Bookmark(User user, Image image) { | |
| this.user = user; | |
| this.image = image; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
capturecat-core/src/main/java/com/capturecat/core/domain/bookmark/Bookmark.java
around lines 17 to 37, the Bookmark entity currently allows duplicate entries
for the same user and image combination. To fix this, add a unique constraint on
the user and image columns by annotating the class with @Table and specifying a
uniqueConstraints attribute that enforces uniqueness on the user_id and image_id
columns. Also, import the javax.persistence.Table class to support this
annotation.
| @PostMapping | ||
| public ApiResponse<?> addBookmark(@RequestParam Long imageId) { | ||
| bookmarkService.addBookmark(imageId); | ||
| return ApiResponse.success(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the imageId parameter.
The controller lacks validation for the imageId parameter, which could allow invalid values to be passed to the service layer.
Add validation annotations to ensure data integrity:
@PostMapping
-public ApiResponse<?> addBookmark(@RequestParam Long imageId) {
+public ApiResponse<?> addBookmark(@RequestParam @Positive Long imageId) {
bookmarkService.addBookmark(imageId);
return ApiResponse.success();
}Add the validation import:
+import jakarta.validation.constraints.Positive;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
capturecat-core/src/main/java/com/capturecat/core/api/bookmark/BookmarkController.java
around lines 20 to 24, the addBookmark method lacks validation for the imageId
parameter. Add validation annotations such as @NotNull and @Positive to the
imageId parameter to ensure it is not null and is a positive number. Also,
include the necessary import statements for these validation annotations. This
will enforce input validation before the service layer is called.
| @Test | ||
| void 북마크를_한다() { | ||
| // given | ||
| willDoNothing().given(bookmarkService).addBookmark(anyLong()); | ||
|
|
||
| // when & then | ||
| given().contentType(ContentType.JSON) | ||
| .accept(ContentType.JSON) | ||
| .queryParam("imageId", 1L) | ||
| .when().post("/v1/bookmarks") | ||
| .then().status(HttpStatus.OK) | ||
| .apply(document("addBookmark", requestPreprocessor(), responsePreprocessor(), | ||
| queryParameters(parameterWithName("imageId").description("즐겨찾기할 이미지 ID")), | ||
| responseFields( | ||
| fieldWithPath("result").type(JsonFieldType.STRING).description("요청 성공 여부"), | ||
| fieldWithPath("data").type(JsonFieldType.NULL).optional().ignored(), | ||
| fieldWithPath("error").type(JsonFieldType.NULL).optional().ignored()))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error scenario tests for comprehensive coverage.
The test covers the happy path well, but consider adding tests for error scenarios to ensure proper error handling.
Add tests for error scenarios:
@Test
void 존재하지_않는_이미지_ID로_북마크_추가시_예외가_발생한다() {
// given
willThrow(new CoreException(ErrorType.IMAGE_NOT_FOUND))
.given(bookmarkService).addBookmark(anyLong());
// when & then
given().contentType(ContentType.JSON)
.accept(ContentType.JSON)
.queryParam("imageId", 999L)
.when().post("/v1/bookmarks")
.then().status(HttpStatus.BAD_REQUEST);
}🤖 Prompt for AI Agents
In
capturecat-core/src/test/java/com/capturecat/core/api/bookmark/BookmarkControllerTest.java
around lines 36 to 53, the current test only covers the successful bookmark
addition scenario. To improve test coverage, add a new test method that
simulates an error scenario where adding a bookmark with a non-existent image ID
throws a CoreException with ErrorType.IMAGE_NOT_FOUND. Mock the bookmarkService
to throw this exception when addBookmark is called with any long value, then
verify that the API responds with a BAD_REQUEST status. This will ensure proper
error handling is tested.
| @Transactional | ||
| public void addBookmark(Long imageId) { | ||
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); | ||
| LoginUser loginUser = (LoginUser)authentication.getPrincipal(); | ||
|
|
||
| User user = userRepository.findByUsername(loginUser.getUsername()) | ||
| .orElseThrow(() -> new CoreException(ErrorType.USER_NOT_FOUND)); | ||
| Image image = imageRepository.findById(imageId) | ||
| .orElseThrow(() -> new CoreException(ErrorType.IMAGE_NOT_FOUND)); | ||
|
|
||
| bookmarkRepository.save(new Bookmark(user, image)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Address race condition and add proper validation.
The service has several critical issues that need to be addressed:
- Race condition: Multiple concurrent requests could create duplicate bookmarks
- Missing null checks: Authentication and principal could be null
- Unsafe casting: Principal casting without type checking
Apply these fixes:
@Transactional
public void addBookmark(Long imageId) {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+ if (authentication == null || authentication.getPrincipal() == null) {
+ throw new CoreException(ErrorType.UNAUTHORIZED);
+ }
+
+ if (!(authentication.getPrincipal() instanceof LoginUser)) {
+ throw new CoreException(ErrorType.UNAUTHORIZED);
+ }
+
LoginUser loginUser = (LoginUser)authentication.getPrincipal();
User user = userRepository.findByUsername(loginUser.getUsername())
.orElseThrow(() -> new CoreException(ErrorType.USER_NOT_FOUND));
Image image = imageRepository.findById(imageId)
.orElseThrow(() -> new CoreException(ErrorType.IMAGE_NOT_FOUND));
+ // Check if bookmark already exists to prevent duplicates
+ if (bookmarkRepository.existsByUserAndImage(user, image)) {
+ return; // Or throw a specific exception if duplicates should be an error
+ }
bookmarkRepository.save(new Bookmark(user, image));
}You'll need to add the existsByUserAndImage method to BookmarkRepository:
boolean existsByUserAndImage(User user, Image image);🤖 Prompt for AI Agents
In
capturecat-core/src/main/java/com/capturecat/core/service/bookmark/BookmarkService.java
around lines 28 to 40, fix the race condition by checking if a bookmark already
exists using a new existsByUserAndImage method in BookmarkRepository before
saving. Add null checks for Authentication and principal to avoid
NullPointerExceptions. Also, verify the principal's type before casting to
LoginUser to prevent ClassCastException. Implement these validations and
conditional checks to ensure safe and correct bookmark creation.
# Conflicts: # capturecat-core/src/docs/asciidoc/error-codes.adoc # capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
capturecat-core/src/test/java/com/capturecat/core/service/bookmark/BookmarkServiceTest.java (1)
87-87: Fix typo in Korean text.There's a typo in the Korean text: "읺으면" should be "없으면".
- void 즐겨찾기를_할_때_이미지가_존재하지_읺으면_실패한다() { + void 즐겨찾기를_할_때_이미지가_존재하지_않으면_실패한다() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
capturecat-core/src/main/java/com/capturecat/core/domain/bookmark/BookmarkRepository.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/service/bookmark/BookmarkService.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorCode.java(1 hunks)capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/api/bookmark/BookmarkControllerTest.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/domain/bookmark/BookmarkTest.java(1 hunks)capturecat-core/src/test/java/com/capturecat/core/service/bookmark/BookmarkServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorCode.java
🚧 Files skipped from review as they are similar to previous changes (4)
- capturecat-core/src/test/java/com/capturecat/core/domain/bookmark/BookmarkTest.java
- capturecat-core/src/test/java/com/capturecat/core/api/bookmark/BookmarkControllerTest.java
- capturecat-core/src/test/java/com/capturecat/core/api/error/ErrorCodeControllerTest.java
- capturecat-core/src/main/java/com/capturecat/core/service/bookmark/BookmarkService.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
capturecat-core/src/test/java/com/capturecat/core/service/bookmark/BookmarkServiceTest.java (1)
capturecat-core/src/test/java/com/capturecat/core/DummyObject.java (1)
DummyObject(16-63)
🔇 Additional comments (4)
capturecat-core/src/main/java/com/capturecat/core/support/error/ErrorType.java (1)
34-35: LGTM! Well-integrated error type addition.The new
BOOKMARK_DUPLICATIONerror type follows the established pattern with appropriate HTTP status (400 Bad Request) and log level (WARN) for duplicate resource scenarios.capturecat-core/src/main/java/com/capturecat/core/domain/bookmark/BookmarkRepository.java (1)
8-11: LGTM! Clean repository interface following Spring Data conventions.The
BookmarkRepositoryinterface properly extendsJpaRepositoryand the custom methodexistsByUserAndImagefollows Spring Data naming conventions for existence checks, which is perfect for preventing duplicate bookmarks.capturecat-core/src/test/java/com/capturecat/core/service/bookmark/BookmarkServiceTest.java (2)
48-121: Excellent test coverage and structure.The test suite provides comprehensive coverage of all bookmark service scenarios:
- Successful bookmark creation
- User not found error handling
- Image not found error handling
- Duplicate bookmark prevention
The tests follow good practices with proper mocking, BDD-style structure, and verification that the repository save method is called only in success scenarios.
123-129: Well-structured security context setup.The
setupLoggedInUserhelper method properly mocks the Spring Security context with theLoginUserwrapper, enabling clean testing of authenticated service methods.
2 participants
📌 관련 이슈 (필수)
📝 작업 내용 (필수)
즐겨찾기 등록 API를 구현했습니다.
💬 리뷰 참고 사항 (선택)
Summary by CodeRabbit
New Features
Documentation
Tests