Feature/fix tag bug (#175)

* fix : setTags -> updateTags 버그 수정 * feature : setTags 시에 문제의 태그가 비어있을 때만 생성하도록 예외처리 생성 * refactor : 태그 Upserter 리팩토링 * refactor : ProblemTagRepository 제거 & lint --------- Co-authored-by: marcus.min <[email protected]>
SW13-Monstera · Jan 21, 2024 · d69aaf6 · d69aaf6
1 parent 27ef61a
commit d69aaf6
Show file tree

Hide file tree

Showing 9 changed files with 109 additions and 76 deletions.
diff --git 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
@@ -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<ProblemTag> = 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<Tag>) {
+        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
@@ -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
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
@@ -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
@@ -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<String>) {
         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<String>) {
-        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<String>,
+    ) = problem.problemTags.map { it.tag.name }.toSet() == tagNames.toSet()
+
     private fun checkEveryTagExist(
         existTags: List<Tag>,
         tagNames: List<String>,

diff --git a/...broker/apiserver/controller/v1/admin/problem/AdminLongProblemControllerIntegrationTest.kt b/...broker/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,29 +14,23 @@ 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(),
         )
 
         // 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<StandardAnswer>(
                     "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<ProblemTag>(
+                    "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/.../io/csbroker/apiserver/controller/v1/admin/problem/AdminMultipleProblemIntegrationTest.kt b/.../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
@@ -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<TagRepository>()
-    private val problemTagRepository = mockk<ProblemTagRepository>()
-    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<List<ProblemTag>>()) } returns emptyList()
 
         // when
         tagUpserter.setTags(problem, tagNames)
 
         // then
-        verify {
-            tagRepository.findTagsByNameIn(tagNames)
-            problemTagRepository.saveAll(any<List<ProblemTag>>())
-        }
+        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<ConditionConflictException> { 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<ConditionConflictException> { tagUpserter.setTags(problem, listOf("newTag!!!")) }
+    }
+
     @Test
     fun `존재하지 않는 태그를 업데이트하려고 하면 예외가 발생한다`() {
         // given