Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.capturecat.core.api.user.dto.TagRenameRequest;
import com.capturecat.core.service.auth.LoginUser;
import com.capturecat.core.service.image.TagResponse;
import com.capturecat.core.service.tag.ImageTagService;
import com.capturecat.core.service.user.UserTagService;
import com.capturecat.core.support.response.ApiResponse;
import com.capturecat.core.support.response.CursorResponse;
Expand All @@ -27,6 +28,7 @@
public class UserTagController {

private final UserTagService userTagService;
private final ImageTagService imageTagService;

@PostMapping
public ApiResponse<TagResponse> create(@AuthenticationPrincipal LoginUser loginUser, @RequestParam String tagName) {
Expand All @@ -44,16 +46,20 @@ public ApiResponse<CursorResponse<TagResponse>> getAll(@AuthenticationPrincipal
}

@PatchMapping
public ApiResponse<TagResponse> update(@AuthenticationPrincipal LoginUser loginUser,
@RequestBody TagRenameRequest request) {
public ApiResponse<TagResponse> update(
@AuthenticationPrincipal LoginUser loginUser,
@RequestBody TagRenameRequest request
) {
TagResponse response = userTagService.update(loginUser, request.currentTagId(), request.newTagName());
imageTagService.update(loginUser, request.currentTagId(), response.id());

return ApiResponse.success(response);
}

@DeleteMapping
public ApiResponse<?> delete(@AuthenticationPrincipal LoginUser loginUser, @RequestParam Long tagId) {
userTagService.delete(loginUser, tagId);
imageTagService.delete(loginUser, tagId);

return ApiResponse.success();
}
Comment on lines 59 to 65
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Reverse the delete operation order to avoid potential errors.

Deleting the user tag before the image tags can cause TAG_NOT_FOUND exception in ImageTagService.delete at line 37-38, which attempts to load the tag after it may have been deleted. This creates fragile transaction-dependent coupling.

Apply this diff to delete image tags first:

 @DeleteMapping
 public ApiResponse<?> delete(@AuthenticationPrincipal LoginUser loginUser, @RequestParam Long tagId) {
-	userTagService.delete(loginUser, tagId);
 	imageTagService.delete(loginUser, tagId);
+	userTagService.delete(loginUser, tagId);
 
 	return ApiResponse.success();
 }
📝 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.

Suggested change
@DeleteMapping
public ApiResponse<?> delete(@AuthenticationPrincipal LoginUser loginUser, @RequestParam Long tagId) {
userTagService.delete(loginUser, tagId);
imageTagService.delete(loginUser, tagId);
return ApiResponse.success();
}
@DeleteMapping
public ApiResponse<?> delete(@AuthenticationPrincipal LoginUser loginUser, @RequestParam Long tagId) {
imageTagService.delete(loginUser, tagId);
userTagService.delete(loginUser, tagId);
return ApiResponse.success();
}
🤖 Prompt for AI Agents
In
capturecat-core/src/main/java/com/capturecat/core/api/user/UserTagController.java
around lines 59 to 65, the controller currently deletes the user tag before
deleting related image tags which can cause ImageTagService.delete to throw
TAG_NOT_FOUND when it tries to load a tag that has already been removed; modify
the method to call imageTagService.delete(loginUser, tagId) before
userTagService.delete(loginUser, tagId) so image tags are removed while the tag
still exists, and ensure the operation remains within the same transaction (or
annotate with @Transactional at the controller/service level) to keep atomicity.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ public interface ImageTagRepository extends JpaRepository<ImageTag, Long> {
@Query("DELETE FROM ImageTag it WHERE it.tag = :tag AND it.image.user = :user")
void deleteByTagAndUser(Tag tag, User user);

@Modifying(clearAutomatically = true, flushAutomatically = true)
@Query("UPDATE ImageTag it SET it.tag.id = :newTagId WHERE it.tag.id = :oldTagId AND it.image.user.id = :userId")
void updateImageTagsForUser(Long userId, Long oldTagId, Long newTagId);

@Modifying(clearAutomatically = true, flushAutomatically = true)
@Query("DELETE FROM ImageTag it WHERE it.image.id IN (SELECT i.id FROM Image i WHERE i.user.id = :userId)")
void deleteAllTagsByUserId(Long userId);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package com.capturecat.core.service.tag;

import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import lombok.RequiredArgsConstructor;

import com.capturecat.core.domain.tag.ImageTagRepository;
import com.capturecat.core.domain.tag.Tag;
import com.capturecat.core.domain.tag.TagRepository;
import com.capturecat.core.domain.user.User;
import com.capturecat.core.domain.user.UserRepository;
import com.capturecat.core.service.auth.LoginUser;
import com.capturecat.core.support.error.CoreException;
import com.capturecat.core.support.error.ErrorType;

@Service
@RequiredArgsConstructor
public class ImageTagService {

private final ImageTagRepository imageTagRepository;
private final UserRepository userRepository;
private final TagRepository tagRepository;

@Transactional
public void update(LoginUser loginUser, Long oldTagId, Long newTagId) {
User user = userRepository.findByUsername(loginUser.getUsername())
.orElseThrow(() -> new CoreException(ErrorType.USER_NOT_FOUND));

imageTagRepository.updateImageTagsForUser(user.getId(), oldTagId, newTagId);
}
Comment on lines +25 to +31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Validate that newTagId exists before updating.

The update method doesn't verify that newTagId corresponds to an existing tag. If an invalid ID is passed, the repository update will create orphaned foreign key references, potentially corrupting data.

Apply this diff to add validation:

 @Transactional
 public void update(LoginUser loginUser, Long oldTagId, Long newTagId) {
 	User user = userRepository.findByUsername(loginUser.getUsername())
 		.orElseThrow(() -> new CoreException(ErrorType.USER_NOT_FOUND));
+	tagRepository.findById(newTagId)
+		.orElseThrow(() -> new CoreException(ErrorType.TAG_NOT_FOUND));
 
 	imageTagRepository.updateImageTagsForUser(user.getId(), oldTagId, newTagId);
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
capturecat-core/src/main/java/com/capturecat/core/service/tag/ImageTagService.java
around lines 25 to 31, the update method currently looks up the User but does
not validate that newTagId exists; add a check that the Tag with newTagId exists
(e.g., tagRepository.findById(newTagId).orElseThrow(() -> new
CoreException(ErrorType.TAG_NOT_FOUND))) before calling
imageTagRepository.updateImageTagsForUser, and throw an appropriate
CoreException if not found so the update cannot create invalid foreign-key
references.


@Transactional
public void delete(LoginUser loginUser, Long tagId) {
User user = userRepository.findByUsername(loginUser.getUsername())
.orElseThrow(() -> new CoreException(ErrorType.USER_NOT_FOUND));
Tag tag = tagRepository.findById(tagId)
.orElseThrow(() -> new CoreException(ErrorType.TAG_NOT_FOUND));

imageTagRepository.deleteByTagAndUser(tag, user);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
@RequiredArgsConstructor
public class UserTagService {

private static final int MAX_USER_TAG_COUNT = 30;
private static final int MAX_USER_TAG_COUNT = 40;

private final UserRepository userRepository;
private final UserTagRepository userTagRepository;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import com.capturecat.core.config.jwt.JwtUtil;
import com.capturecat.core.service.image.TagResponse;
import com.capturecat.core.service.tag.ImageTagService;
import com.capturecat.core.service.user.UserTagService;
import com.capturecat.core.support.response.CursorResponse;
import com.capturecat.test.api.RestDocsTest;
Expand All @@ -39,11 +40,13 @@ class UserTagControllerTest extends RestDocsTest {

private UserTagController userTagController;
private UserTagService userTagService;
private ImageTagService imageTagService;

@BeforeEach
void setUp() {
userTagService = mock(UserTagService.class);
userTagController = new UserTagController(userTagService);
imageTagService = mock(ImageTagService.class);
userTagController = new UserTagController(userTagService, imageTagService);
mockMvc = mockController(userTagController);
}

Expand Down Expand Up @@ -101,6 +104,7 @@ void setUp() {
void 유저_태그_수정() {
// given
BDDMockito.given(userTagService.update(any(), anyLong(), anyString())).willReturn(new TagResponse(1L, "java"));
BDDMockito.willDoNothing().given(imageTagService).update(any(), anyLong(), anyLong());

// when & then
given()
Expand All @@ -126,6 +130,7 @@ void setUp() {
void 유저_태그_삭제() {
// given
BDDMockito.given(userTagService.create(any(), anyString())).willReturn(new TagResponse(1L, "java"));
BDDMockito.willDoNothing().given(imageTagService).delete(any(), anyLong());

// when & then
given()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,63 @@ class ImageTagRepositoryTest {
assertThat(user2Result).hasSize(1);
assertThat(user2Result.get(0).getImage().getUser()).isEqualTo(user2);
}

@Test
void 지정한_사용자만_태그가_교체된다() {
// given
var user1 = userRepository.save(DummyObject.newUser("user1"));
var user2 = userRepository.save(DummyObject.newUser("user2"));

var image1 = imageRepository.save(DummyObject.newMockUserImage(user1));
var image2 = imageRepository.save(DummyObject.newMockUserImage(user2));

var oldTag = tagRepository.save(TagFixture.createTag("old"));
var newTag = tagRepository.save(TagFixture.createTag("new"));

imageTagRepository.save(new ImageTag(image1, oldTag));
imageTagRepository.save(new ImageTag(image2, oldTag));

entityManager.flush();
entityManager.clear();

// when
imageTagRepository.updateImageTagsForUser(user1.getId(), oldTag.getId(), newTag.getId());

entityManager.flush();
entityManager.clear();

// then
var it1 = imageTagRepository.findByImage(image1).get(0);
var it2 = imageTagRepository.findByImage(image2).get(0);

assertThat(it1.getTag().getId()).isEqualTo(newTag.getId());
assertThat(it2.getTag().getId()).isEqualTo(oldTag.getId());
}

@Test
void 태그가_없는_이미지에는_영향이_없다() {
// given
var user = userRepository.save(DummyObject.newUser("noTagUser"));
var imageWithNoTag = imageRepository.save(DummyObject.newMockUserImage(user));

var oldTag = tagRepository.save(TagFixture.createTag("old"));
var newTag = tagRepository.save(TagFixture.createTag("new"));

var imageWithTag = imageRepository.save(DummyObject.newMockUserImage(user));
imageTagRepository.save(new ImageTag(imageWithTag, oldTag));

entityManager.flush();
entityManager.clear();

// when
imageTagRepository.updateImageTagsForUser(user.getId(), oldTag.getId(), newTag.getId());

entityManager.flush();
entityManager.clear();

// then
assertThat(imageTagRepository.findByImage(imageWithNoTag)).isEmpty();
var updated = imageTagRepository.findByImage(imageWithTag).get(0);
assertThat(updated.getTag().getId()).isEqualTo(newTag.getId());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package com.capturecat.core.service.tag;

import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;

import java.util.Optional;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

import com.capturecat.core.DummyObject;
import com.capturecat.core.domain.tag.ImageTagRepository;
import com.capturecat.core.domain.tag.Tag;
import com.capturecat.core.domain.tag.TagFixture;
import com.capturecat.core.domain.tag.TagRepository;
import com.capturecat.core.domain.user.User;
import com.capturecat.core.domain.user.UserRepository;
import com.capturecat.core.service.auth.LoginUser;
import com.capturecat.core.support.error.CoreException;

@ExtendWith(MockitoExtension.class)
class ImageTagServiceTest {

@Mock
ImageTagRepository imageTagRepository;

@Mock
UserRepository userRepository;

@Mock
TagRepository tagRepository;

@InjectMocks
ImageTagService imageTagService;

@Test
void 업데이트_사용자_존재하면_레포지토리_호출한다() {
// given
User user = DummyObject.newMockUser(123L);

given(userRepository.findByUsername(anyString())).willReturn(Optional.of(user));

// when
imageTagService.update(new LoginUser(user), 10L, 20L);

// then
verify(imageTagRepository, times(1))
.updateImageTagsForUser(user.getId(), 10L, 20L);
}

@Test
void 업데이트_사용자_없으면_CoreException_던진다() {
// given
User user = DummyObject.newUser("noUser");

given(userRepository.findByUsername(anyString())).willReturn(Optional.empty());

// when & then
assertThatThrownBy(() -> imageTagService.update(new LoginUser(user), 1L, 2L))
.isInstanceOf(CoreException.class);
verifyNoInteractions(imageTagRepository);
}

@Test
void 삭제_사용자_태그_존재하면_레포지토리_호출한다() {
// given
User user = DummyObject.newUser("test");

given(userRepository.findByUsername(anyString())).willReturn(Optional.of(user));

Tag tag = TagFixture.createTag(10L, "testTag");
given(tagRepository.findById(anyLong())).willReturn(Optional.of(tag));

// when
imageTagService.delete(new LoginUser(user), tag.getId());

// then
verify(imageTagRepository, times(1)).deleteByTagAndUser(tag, user);
}

@Test
void 삭제_사용자_없으면_CoreException_던진다() {
// given
User user = DummyObject.newUser("noUser");

given(userRepository.findByUsername(anyString())).willReturn(Optional.empty());

// when & then
assertThatThrownBy(() -> imageTagService.delete(new LoginUser(user), 1L))
.isInstanceOf(CoreException.class);
verifyNoInteractions(imageTagRepository, tagRepository);
}

@Test
void 삭제_태그_없으면_CoreException_던진다() {
// given
User user = DummyObject.newUser("test");

given(userRepository.findByUsername(anyString())).willReturn(Optional.of(user));
given(tagRepository.findById(anyLong())).willReturn(Optional.empty());

// when & then
assertThatThrownBy(() -> imageTagService.delete(new LoginUser(user), 2L))
.isInstanceOf(CoreException.class);
verifyNoInteractions(imageTagRepository);
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class UserTagServiceTest {
given(userRepository.findByUsername(anyString())).willReturn(Optional.of(user));
given(tagRegister.registerTagsFor(anyString())).willReturn(tag);
given(userTagRepository.existsByUserAndTag(eq(user), eq(tag))).willReturn(false);
given(userTagRepository.countByUser(eq(user))).willReturn(30L);
given(userTagRepository.countByUser(eq(user))).willReturn(40L);

// when & then
assertThatThrownBy(() -> userTagService.create(new LoginUser(DummyObject.newUser("test")), "java"))
Expand Down