From d69aaf67560a417bb1b04b86807acd7d31530db3 Mon Sep 17 00:00:00 2001 From: Jaewon Min <67869514+ekzm8523@users.noreply.github.com> Date: Sun, 21 Jan 2024 22:33:43 +0900 Subject: [PATCH] Feature/fix tag bug (#175) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix : setTags -> updateTags 버그 수정 * feature : setTags 시에 문제의 태그가 비어있을 때만 생성하도록 예외처리 생성 * refactor : 태그 Upserter 리팩토링 * refactor : ProblemTagRepository 제거 & lint --------- Co-authored-by: marcus.min --- .../apiserver/common/enums/ErrorCode.kt | 1 + .../io/csbroker/apiserver/model/Problem.kt | 30 +++++++++- .../kotlin/io/csbroker/apiserver/model/Tag.kt | 2 +- .../problem/ProblemTagRepository.kt | 6 -- .../admin/AdminLongProblemServiceImpl.kt | 2 +- .../service/problem/admin/TagUpserter.kt | 39 ++++-------- ...minLongProblemControllerIntegrationTest.kt | 59 +++++++++++++++---- .../AdminMultipleProblemIntegrationTest.kt | 23 ++------ .../service/problem/admin/TagUpserterTest.kt | 23 +++++--- 9 files changed, 109 insertions(+), 76 deletions(-) delete mode 100644 src/main/kotlin/io/csbroker/apiserver/repository/problem/ProblemTagRepository.kt diff --git a/src/main/kotlin/io/csbroker/apiserver/common/enums/ErrorCode.kt b/src/main/kotlin/io/csbroker/apiserver/common/enums/ErrorCode.kt index b60a0bd4..f1eebb81 100644 --- a/src/main/kotlin/io/csbroker/apiserver/common/enums/ErrorCode.kt +++ b/src/main/kotlin/io/csbroker/apiserver/common/enums/ErrorCode.kt @@ -16,6 +16,7 @@ enum class ErrorCode( FORBIDDEN(403, "이 작업에 대한 권한이 없습니다."), NOT_FOUND_ENTITY(404, "대상을 찾을 수 없습니다."), USERNAME_DUPLICATED(409, "닉네임이 중복되었습니다."), + TAG_DUPLICATED(409, "태그가 중복되었습니다."), EMAIL_DUPLICATED(409, "이메일이 중복되었습니다."), PROVIDER_MISS_MATCH(409, "올바르지 않은 provider입니다."), SERVER_ERROR(500, "서버에서 오류가 발생했습니다."), diff --git a/src/main/kotlin/io/csbroker/apiserver/model/Problem.kt b/src/main/kotlin/io/csbroker/apiserver/model/Problem.kt index ef28af2b..b48e3d12 100644 --- a/src/main/kotlin/io/csbroker/apiserver/model/Problem.kt +++ b/src/main/kotlin/io/csbroker/apiserver/model/Problem.kt @@ -1,5 +1,8 @@ package io.csbroker.apiserver.model +import io.csbroker.apiserver.common.enums.ErrorCode +import io.csbroker.apiserver.common.exception.ConditionConflictException +import io.csbroker.apiserver.common.exception.EntityNotFoundException import io.csbroker.apiserver.dto.problem.GradingHistoryStats import io.csbroker.apiserver.dto.problem.ProblemResponseDto import jakarta.persistence.CascadeType @@ -49,7 +52,7 @@ abstract class Problem( @Column(name = "score") var score: Double, - @OneToMany(mappedBy = "problem", cascade = [CascadeType.ALL], fetch = FetchType.LAZY) + @OneToMany(mappedBy = "problem", cascade = [CascadeType.ALL], fetch = FetchType.LAZY, orphanRemoval = true) val problemTags: MutableSet = mutableSetOf(), @OneToMany(mappedBy = "problem", cascade = [CascadeType.ALL], fetch = FetchType.LAZY) @@ -77,4 +80,29 @@ abstract class Problem( dtype, ) } + + fun addTag(tag: Tag) { + val problemTag = ProblemTag(problem = this, tag = tag) + if (problemTags.map { it.tag.name }.contains(tag.name)) { + throw ConditionConflictException(ErrorCode.TAG_DUPLICATED, "해당 태그가 이미 존재합니다. tagName: ${tag.name}") + } + problemTags.add(problemTag) + } + + fun addTags(tags: List) { + val newProblemTags = tags.map { ProblemTag(problem = this, tag = it) } + val existProblemTags = problemTags.map { it.tag.name }.intersect(tags.map { it.name }.toSet()) + if (existProblemTags.isNotEmpty()) { + throw ConditionConflictException(ErrorCode.TAG_DUPLICATED, "해당 태그가 이미 존재합니다. tagName: $existProblemTags") + } + problemTags.addAll(newProblemTags) + } + + fun deleteTag(tag: Tag) { + val problemTag = problemTags.find { + it.tag.name == tag.name + } ?: throw EntityNotFoundException("해당 태그가 존재하지 않습니다. tagName: ${tag.name}") + + problemTags.remove(problemTag) + } } diff --git a/src/main/kotlin/io/csbroker/apiserver/model/Tag.kt b/src/main/kotlin/io/csbroker/apiserver/model/Tag.kt index c5120aaa..7798969f 100644 --- a/src/main/kotlin/io/csbroker/apiserver/model/Tag.kt +++ b/src/main/kotlin/io/csbroker/apiserver/model/Tag.kt @@ -16,7 +16,7 @@ class Tag( @Column(name = "tag_id") val id: Long = 0, - @Column(name = "tag_name", columnDefinition = "VARCHAR(30)") + @Column(name = "tag_name", columnDefinition = "VARCHAR(30)", unique = true) val name: String, @OneToMany(mappedBy = "tag") diff --git a/src/main/kotlin/io/csbroker/apiserver/repository/problem/ProblemTagRepository.kt b/src/main/kotlin/io/csbroker/apiserver/repository/problem/ProblemTagRepository.kt deleted file mode 100644 index a609cd47..00000000 --- a/src/main/kotlin/io/csbroker/apiserver/repository/problem/ProblemTagRepository.kt +++ /dev/null @@ -1,6 +0,0 @@ -package io.csbroker.apiserver.repository.problem - -import io.csbroker.apiserver.model.ProblemTag -import org.springframework.data.jpa.repository.JpaRepository - -interface ProblemTagRepository : JpaRepository diff --git a/src/main/kotlin/io/csbroker/apiserver/service/problem/admin/AdminLongProblemServiceImpl.kt b/src/main/kotlin/io/csbroker/apiserver/service/problem/admin/AdminLongProblemServiceImpl.kt index 0f6b0e9d..2e0373bf 100644 --- a/src/main/kotlin/io/csbroker/apiserver/service/problem/admin/AdminLongProblemServiceImpl.kt +++ b/src/main/kotlin/io/csbroker/apiserver/service/problem/admin/AdminLongProblemServiceImpl.kt @@ -77,7 +77,7 @@ class AdminLongProblemServiceImpl( } longProblem.updateFromDto(updateRequestDto) - tagUpserter.setTags(longProblem, updateRequestDto.tags) + tagUpserter.updateTags(longProblem, updateRequestDto.tags.toMutableList()) return id } } diff --git a/src/main/kotlin/io/csbroker/apiserver/service/problem/admin/TagUpserter.kt b/src/main/kotlin/io/csbroker/apiserver/service/problem/admin/TagUpserter.kt index 87c1c142..c220be3b 100644 --- a/src/main/kotlin/io/csbroker/apiserver/service/problem/admin/TagUpserter.kt +++ b/src/main/kotlin/io/csbroker/apiserver/service/problem/admin/TagUpserter.kt @@ -5,57 +5,42 @@ import io.csbroker.apiserver.common.exception.ConditionConflictException import io.csbroker.apiserver.model.Problem import io.csbroker.apiserver.model.ProblemTag import io.csbroker.apiserver.model.Tag -import io.csbroker.apiserver.repository.problem.ProblemTagRepository import io.csbroker.apiserver.repository.problem.TagRepository import org.springframework.stereotype.Component @Component class TagUpserter( private val tagRepository: TagRepository, - private val problemTagRepository: ProblemTagRepository, ) { fun setTags(problem: Problem, tagNames: List) { if (tagNames.isEmpty()) { throw ConditionConflictException(ErrorCode.CONDITION_NOT_FULFILLED, "태그의 개수는 1개 이상이여야합니다.") } - + if (problem.problemTags.isNotEmpty()) { + throw ConditionConflictException(ErrorCode.CONDITION_NOT_FULFILLED, "태그가 이미 존재합니다.") + } val tags = tagRepository.findTagsByNameIn(tagNames) checkEveryTagExist(tags, tagNames) - - val problemTags = tags.map { - ProblemTag(problem = problem, tag = it) - } - - problemTagRepository.saveAll(problemTags) - problem.problemTags.addAll(problemTags) + problem.addTags(tags) } fun updateTags(problem: Problem, tagNames: MutableList) { - problem.problemTags.removeIf { - if (it.tag.name !in tagNames) { - problemTagRepository.delete(it) - return@removeIf true - } - return@removeIf false - } - - tagNames.removeIf { - it in problem.problemTags.map { pt -> - pt.tag.name - } - } + if (isNotChanged(problem, tagNames)) return + problem.problemTags.clear() val tags = tagRepository.findTagsByNameIn(tagNames) checkEveryTagExist(tags, tagNames) - val problemTags = tags.map { - ProblemTag(problem = problem, tag = it) - } - + val problemTags = tags.map { ProblemTag(problem = problem, tag = it) } problem.problemTags.addAll(problemTags) } + private fun isNotChanged( + problem: Problem, + tagNames: MutableList, + ) = problem.problemTags.map { it.tag.name }.toSet() == tagNames.toSet() + private fun checkEveryTagExist( existTags: List, tagNames: List, diff --git a/src/test/kotlin/io/csbroker/apiserver/controller/v1/admin/problem/AdminLongProblemControllerIntegrationTest.kt b/src/test/kotlin/io/csbroker/apiserver/controller/v1/admin/problem/AdminLongProblemControllerIntegrationTest.kt index 2bb3c577..c7023935 100644 --- a/src/test/kotlin/io/csbroker/apiserver/controller/v1/admin/problem/AdminLongProblemControllerIntegrationTest.kt +++ b/src/test/kotlin/io/csbroker/apiserver/controller/v1/admin/problem/AdminLongProblemControllerIntegrationTest.kt @@ -3,6 +3,7 @@ package io.csbroker.apiserver.controller.v1.admin.problem import io.csbroker.apiserver.controller.IntegrationTest import io.csbroker.apiserver.dto.problem.longproblem.LongProblemUpsertRequestDto import io.csbroker.apiserver.model.LongProblem +import io.csbroker.apiserver.model.ProblemTag import io.csbroker.apiserver.model.StandardAnswer import io.csbroker.apiserver.model.Tag import io.kotest.matchers.shouldBe @@ -13,21 +14,15 @@ import org.springframework.test.web.servlet.result.MockMvcResultMatchers.status class AdminLongProblemControllerIntegrationTest : IntegrationTest() { @Test - fun `문제 업데이트`() { + fun `문제 업데이트 - 모범 답안`() { // given - val problem = save( - LongProblem( - title = "문제 제목", - description = "문제 설명", - creator = adminUser, - ), - ) - save(StandardAnswer(content = "삭제될 모범 답안", longProblem = problem)) - save(Tag(name = "tag1")) + val longProblem = LongProblem(title = "문제 제목", description = "문제 설명", creator = adminUser) + save(longProblem) + save(StandardAnswer(content = "삭제될 모범 답안", longProblem = longProblem)) val updateRequestDto = LongProblemUpsertRequestDto( title = "Test problem", description = "This is a test problem", - tags = mutableListOf("tag1"), + tags = mutableListOf(), standardAnswers = listOf("업데이트될 모범 답안"), gradingStandards = mutableListOf(), ) @@ -35,7 +30,7 @@ class AdminLongProblemControllerIntegrationTest : IntegrationTest() { // when val response = request( method = HttpMethod.PUT, - url = "/api/admin/problems/long/${problem.id}", + url = "/api/admin/problems/long/${longProblem.id}", isAdmin = true, body = updateRequestDto, ) @@ -45,9 +40,47 @@ class AdminLongProblemControllerIntegrationTest : IntegrationTest() { .andExpect { val standardAnswers = findAll( "SELECT s FROM StandardAnswer s where s.longProblem.id = :id", - mapOf("id" to problem.id), + mapOf("id" to longProblem.id), ) standardAnswers.map { it.content }.toSet() shouldBe updateRequestDto.standardAnswers.toSet() + standardAnswers.size shouldBe updateRequestDto.standardAnswers.size + } + } + + @Test + fun `문제 업데이트 - 태그`() { + // given + val oldTag = save(Tag(name = "long-problem-update-tag1")) + val longProblem = LongProblem(title = "문제 제목", description = "문제 설명", creator = adminUser) + longProblem.addTag(oldTag) + save(longProblem) + val newTag1 = save(Tag(name = "${longProblem.id}-tag2")) + val newTag2 = save(Tag(name = "${longProblem.id}-tag3")) + val updateRequestDto = LongProblemUpsertRequestDto( + title = longProblem.title, + description = longProblem.description, + tags = mutableListOf(newTag1.name, newTag2.name), + standardAnswers = emptyList(), + gradingStandards = mutableListOf(), + ) + + // when + val response = request( + method = HttpMethod.PUT, + url = "/api/admin/problems/long/${longProblem.id}", + isAdmin = true, + body = updateRequestDto, + ) + + // then + response.andExpect(status().isOk) + .andExpect { + val problemTags = findAll( + "SELECT pt FROM ProblemTag pt JOIN FETCH pt.tag where pt.problem.id = :id", + mapOf("id" to longProblem.id), + ) + problemTags.map { it.tag.name }.toSet() shouldBe updateRequestDto.tags.toSet() + problemTags.size shouldBe updateRequestDto.tags.size } } } diff --git a/src/test/kotlin/io/csbroker/apiserver/controller/v1/admin/problem/AdminMultipleProblemIntegrationTest.kt b/src/test/kotlin/io/csbroker/apiserver/controller/v1/admin/problem/AdminMultipleProblemIntegrationTest.kt index c2410afa..598e4995 100644 --- a/src/test/kotlin/io/csbroker/apiserver/controller/v1/admin/problem/AdminMultipleProblemIntegrationTest.kt +++ b/src/test/kotlin/io/csbroker/apiserver/controller/v1/admin/problem/AdminMultipleProblemIntegrationTest.kt @@ -23,22 +23,9 @@ class AdminMultipleProblemIntegrationTest : IntegrationTest() { isMultiple = false, ), ) - val tag = save( - Tag( - name = "tag1", - ), - ) - save( - Tag( - name = "tag2", - ), - ) - save( - ProblemTag( - problem = problem, - tag = tag, - ), - ) + val tag = save(Tag(name = "${problem.id}-tag1")) + val newTag = save(Tag(name = "${problem.id}-tag2")) + save(ProblemTag(problem = problem, tag = tag)) // when val response = request( @@ -58,7 +45,7 @@ class AdminMultipleProblemIntegrationTest : IntegrationTest() { isAnswer = false, ), ), - tags = listOf("tag2"), + tags = listOf(newTag.name), score = problem.score, ), ) @@ -71,7 +58,7 @@ class AdminMultipleProblemIntegrationTest : IntegrationTest() { mapOf("problemId" to problem.id), ) problemTags.size shouldBe 1 - problemTags[0].tag.name shouldBe "tag2" + problemTags[0].tag.name shouldBe newTag.name } } } diff --git a/src/test/kotlin/io/csbroker/apiserver/service/problem/admin/TagUpserterTest.kt b/src/test/kotlin/io/csbroker/apiserver/service/problem/admin/TagUpserterTest.kt index f1be38e8..60b9097e 100644 --- a/src/test/kotlin/io/csbroker/apiserver/service/problem/admin/TagUpserterTest.kt +++ b/src/test/kotlin/io/csbroker/apiserver/service/problem/admin/TagUpserterTest.kt @@ -6,7 +6,6 @@ import io.csbroker.apiserver.model.LongProblem import io.csbroker.apiserver.model.ProblemTag import io.csbroker.apiserver.model.Tag import io.csbroker.apiserver.model.User -import io.csbroker.apiserver.repository.problem.ProblemTagRepository import io.csbroker.apiserver.repository.problem.TagRepository import io.mockk.clearMocks import io.mockk.every @@ -20,8 +19,7 @@ import java.util.UUID class TagUpserterTest { private val tagRepository = mockk() - private val problemTagRepository = mockk() - private val tagUpserter = TagUpserter(tagRepository, problemTagRepository) + private val tagUpserter = TagUpserter(tagRepository) private val user = User( id = UUID.randomUUID(), @@ -33,7 +31,7 @@ class TagUpserterTest { @BeforeEach fun setUp() { - clearMocks(tagRepository, problemTagRepository) + clearMocks(tagRepository) } @Test @@ -44,16 +42,12 @@ class TagUpserterTest { val tag1 = Tag(id = 1, name = "tag1") val tag2 = Tag(id = 2, name = "tag2") every { tagRepository.findTagsByNameIn(tagNames) } returns listOf(tag1, tag2) - every { problemTagRepository.saveAll(any>()) } returns emptyList() // when tagUpserter.setTags(problem, tagNames) // then - verify { - tagRepository.findTagsByNameIn(tagNames) - problemTagRepository.saveAll(any>()) - } + verify { tagRepository.findTagsByNameIn(tagNames) } assertEquals(2, problem.problemTags.size) val updatedTagSet = problem.problemTags.map { it.tag.name }.toSet() assert(updatedTagSet.contains("tag1")) @@ -82,6 +76,17 @@ class TagUpserterTest { assertThrows { tagUpserter.setTags(problem, tagNames) } } + @Test + fun `이미 태그가 존재하는 문제에 태그를 생성할 시 예외가 발생한다`() { + // given + val problem = getLongProblem() + val existTag = Tag(name = "tag1") + problem.problemTags.add(ProblemTag(problem = problem, tag = existTag)) + + // when, then + assertThrows { tagUpserter.setTags(problem, listOf("newTag!!!")) } + } + @Test fun `존재하지 않는 태그를 업데이트하려고 하면 예외가 발생한다`() { // given