Feat/#137 테마 패키지 비동기 빌드 및 상태 조회 - #143
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
전반적으로 잘 작성된 코드라고 생각합니다. 다만 비즈니스 로직 관점이나 기술 선택 이유에 관련하여 궁금한 부분들이 있어 코멘트 남겼습니다.
테스트 작성 자체는 꼼꼼하게 해주셔서 인상깊었습니다. 다만 일부 테스트는 실제 요구사항이나 중요한 동작을 보호하는 테스트인지 검토가 필요해 보입니다.
테스트는 모든 코드를 실행하는 것보다 변경 시 깨지면 안 되는 동작을 명확히 보장하는 것이 목적이라고 생각합니다. 특히 프레임워크 기본 동작이나 구현 세부사항을 검증하는 테스트는 유지 비용 대비 효용이 낮을 수 있어, 테스트가 어떤 장애/회귀를 방지하는지 검토가 필요할 것으로 보입니다.
There was a problem hiding this comment.
현재 PR 내에서 HttpMEssageNotReadableException 예외의 발생 시점이 궁금합니다.
There was a problem hiding this comment.
테마 빌드 시작 요청의 platform 값을 Platform enum으로 역직렬화할 때 발생합니다. 정의되지 않은 값을 전달하면 DTO 변환 단계에서 HttpMessageNotReadableException이 발생합니다. 이는 DTO 변환 후 검증하는 MethodArgumentNotValidException이나 path/query parameter 변환을 처리하는 MethodArgumentTypeMismatchException의 대상이 아닙니다. 기존 공통 Exception 핸들러로 처리하면 500이 반환되므로, 잘못된 요청임을 나타내는 400을 반환하기 위해 전용 핸들러를 추가했습니다.
| private ThemeBuildJob(ThemeComponent themeComponent, Platform platform) { | ||
| this.themeComponent = themeComponent; | ||
| this.platform = platform; | ||
| this.status = ThemeBuildStatus.RUNNING; | ||
| } | ||
|
|
||
| public static ThemeBuildJob createRunning( | ||
| ThemeComponent themeComponent, | ||
| Platform platform | ||
| ) { | ||
| return new ThemeBuildJob(themeComponent, platform); | ||
| } |
There was a problem hiding this comment.
이 부분에서 private 생성자를 별도로 두는 이유가 궁금합니다. 현재 코드상으로는 생성자를 추가로 분리하지 않아도 동일하게 구현할 수 있을 것 같은데, 오히려 불필요한 추상화 계층이 하나 더 생긴 느낌입니다. 특별한 의도가 있는 구조라면 설명 부탁드립니다.
There was a problem hiding this comment.
생성에 필요한 themeComponent, platform과 RUNNING 초기 상태를 생성자에 모으고, createRunning이라는 이름으로 생성 의도를 드러내려는 목적이었습니다. 다만 말씀해주신대로 별도로 분리해 둘 필요가 없다고 생각합니다. 팩토리 메서드 내부에서 직접 초기화하도록 단순화하겠습니다.
There was a problem hiding this comment.
현재 Executor 빈이 하나뿐인 것 같은데, @bean(name = ...)과 상수를 별도로 두는 이유가 궁금합니다. 여러 Executor를 구분해서 사용할 예정이 아니라면 기본 빈 등록만으로도 충분해 보여서, 오히려 관리 포인트만 늘어난 느낌입니다.
There was a problem hiding this comment.
처음에는 테마 빌드 전용 Executor를 명확히 주입하고, 빈 이름의 불일치를 방지하려는 의도로 이름 상수와 @qualifier를 사용했습니다. 그러나 말씀해주신대로 현 구조에서는 별도 이름 지정이 동작상 이점 없이 관리 포인트만 늘어난다는 의견에 동의합니다. 전용 Executor 설정은 유지하되 상수, 명시적 빈 이름과 @qualifier는 제거하고, 추후 Executor가 여러 개가 되는 시점에 구분자를 추가하겠습니다.
| @Transactional | ||
| public ThemeBuildStartResponse startBuild(Integer themeComponentId, Platform platform) { | ||
| ThemeComponent themeComponent = themeComponentRepository.findByIdForUpdate(themeComponentId) | ||
| .orElseThrow(() -> new ResourceNotFoundException( | ||
| "Theme not found with id: " + themeComponentId)); | ||
| validateThemeAccess(themeComponent, "No permission to build theme package"); | ||
| ThemeBuildJob runningJob = themeBuildJobRepository | ||
| .findFirstByThemeComponent_ThemeComponentIdAndPlatformAndStatusOrderByCreatedAtDesc( | ||
| themeComponentId, | ||
| platform, | ||
| ThemeBuildStatus.RUNNING | ||
| ) | ||
| .orElse(null); | ||
| if (runningJob != null) { | ||
| return ThemeBuildStartResponse.from(runningJob); | ||
| } | ||
|
|
||
| ThemeBuildJob saved = themeBuildJobRepository.saveAndFlush( | ||
| ThemeBuildJob.createRunning(themeComponent, platform) | ||
| ); | ||
| registerBuildDispatch(saved.getBuildId()); | ||
| return ThemeBuildStartResponse.from(saved); | ||
| } |
There was a problem hiding this comment.
현재 로직은 실행 중인 빌드가 동일한 테마 상태에 대한 중복 요청인지, 테마 수정 이후 새롭게 생성해야 하는 빌드 요청인지 구분하지 못하는 것 같습니다.
테마 수정 후 재빌드 요청 시에도 기존 RUNNING Job을 반환할 가능성이 있어, 빌드 대상의 버전이나 변경 시점을 기준으로 구분하는 방식이 필요해 보입니다.
There was a problem hiding this comment.
놓친 부분이 있던 것 같습니다. 피드백 감사합니다. 빌드 대상의 버전이나 스냅샷을 어떻게 관리할 지, 수정된 테마로 다시 빌드를 요청했을 때 기존의 빌드가 이미 Running중이었을때는 어떤 방식으로 처리할 지 정도를 의논해봐야 할 것 같습니다. 제 생각에는 테마 변경시 버전(revision)을 증가시키고 Job에 해당 버전을 저장한 뒤, 동일한 버전의 Running Job을 재사용하는 방식이 적절해보입니다. 그리고 다른 버전의 Job이 실행중이라면 기존 작업 완료 후 순차적으로 실행하는 방향이 안전할 것 같습니다. 이에 대해 용민님의 의견은 어떤지 궁금합니다.
| @Modifying(flushAutomatically = true) | ||
| @Query(""" | ||
| update ThemeBuildJob job | ||
| set job.status = :successStatus, | ||
| job.packageUrl = :packageUrl, | ||
| job.updatedAt = :updatedAt | ||
| where job.buildId = :buildId | ||
| and job.status = :runningStatus | ||
| """) | ||
| int markSuccessIfRunning( | ||
| @Param("buildId") Long buildId, | ||
| @Param("packageUrl") String packageUrl, | ||
| @Param("updatedAt") LocalDateTime updatedAt, | ||
| @Param("runningStatus") ThemeBuildStatus runningStatus, | ||
| @Param("successStatus") ThemeBuildStatus successStatus | ||
| ); | ||
|
|
||
| @Modifying(flushAutomatically = true) | ||
| @Query(""" | ||
| update ThemeBuildJob job | ||
| set job.status = :failedStatus, | ||
| job.updatedAt = :updatedAt | ||
| where job.buildId = :buildId and job.status = :runningStatus | ||
| """) | ||
| int markFailedIfRunning( | ||
| @Param("buildId") Long buildId, | ||
| @Param("updatedAt") LocalDateTime updatedAt, | ||
| @Param("runningStatus") ThemeBuildStatus runningStatus, | ||
| @Param("failedStatus") ThemeBuildStatus failedStatus | ||
| ); | ||
| } |
There was a problem hiding this comment.
별도 조회 없이 빌드 상태와 갱신 시간을 변경하기 위한 벌크 업데이트 쿼리로 이해했습니다.
다만 상태 전이가 메서드명으로 이미 표현되어 있고 호출부에서 항상 동일한 enum 값을 전달한다면, 해당 파라미터를 유지할 필요가 있는지 검토해보면 좋겠습니다. 잘못된 상태 값 전달 가능성을 줄이는 방향에서는 Repository 내부에서 고정하는 방식도 충분히 좋아보이는데 의견이 궁금합니다.
There was a problem hiding this comment.
전이 전후 상태를 호출부에서 명시하고 쿼리를 파라미터화하려고 했는데 두 메서드가 각각 running -> success, runninng -> failed 전이만 담당하고 호출부에서도 항상 동일한 값을 전달하고 있어, 파라미터로 유지할 필요가 딱히 없어보인다는 점에 동의합니다. 상태값을 메서드에 고정하고 관련 파라미터를 제거하겠습니다.
| @Test | ||
| @DisplayName("인증하지 않은 build 조회는 401을 반환한다") | ||
| void findThemeBuild_unauthorized() throws Exception { | ||
| Long buildId = startAndReadBuildId(theme, owner); | ||
|
|
||
| mockMvc.perform(get("/api/theme-builds/{buildId}", buildId)) | ||
| .andExpect(status().isUnauthorized()); | ||
| } |
There was a problem hiding this comment.
해당 테스트는 현재 SecurityFilterChain의 인증 처리 동작을 검증하는 테스트로 보입니다. 이미 존재하는 JwtFilter 테스트와 비교했을 때, 두 테스트가 각각 어떤 책임을 검증하는지 구분이 필요해 보입니다.
현재 테스트가 별도로 존재해야 하는 이유(예: 특정 API의 인증 정책 보장)가 있다면 해당 의도를 명확히 하면 좋을 것 같고, 그렇지 않다면 중복 테스트가 될 가능성이 있어 보입니다.
There was a problem hiding this comment.
startThemeBuild_unauthorized()와 같은 이유로 작성했습니다. 특히 GET 경로의 whitelist 설정 변경으로 해당 API가 의도치않게 공개되는 회귀를 방지하기 위한 테스트입니다.
| private User saveUser(String email, UserRole role) { | ||
| return userRepository.save(User.builder() | ||
| .publicUserId(UUID.randomUUID().toString()) | ||
| .userEmail(email) | ||
| .name("theme build test user") | ||
| .role(role) | ||
| .build()); | ||
| } | ||
|
|
||
| private ThemeComponent saveTheme(User user) { | ||
| return themeComponentRepository.save(ThemeComponent.builder() | ||
| .userEmail(user.getUserEmail()) | ||
| .themeName("polling test theme") | ||
| .versionNumber("1") | ||
| .versionName("1.0.0") | ||
| .isDone(true) | ||
| .isPublic(false) | ||
| .build()); | ||
| } |
There was a problem hiding this comment.
테스트 코드 내부에서 Entity 생성 방식이 직접 노출되고 있는 부분은 유지보수 측면에서 개선이 필요해 보입니다. 현재 Entity 구조 변경이나 생성 로직 변경 시 여러 테스트를 함께 수정해야 하는 구조가 될 가능성이 있습니다.
이미 공통 fixture/seeder가 존재한다면 해당 기능을 활용해 테스트 데이터 생성 책임을 분리하는 방향이 좋을 것 같습니다.
There was a problem hiding this comment.
의견 감사합니다. 기존 공통 생성기를 확인했으나, UserDataGenerator는 역할이 USER로 고정되어 있고 ThemeComponentScenarioSupport는 메타데이터, 이미지, 스타일까지 생성해 이 테스트에는 과도한 의존성이 생긴다고 생각했습니다. 그래서 권한 및 빌드 테스트에 필요한 최소 엔티티만 생성하도록 로컬 헬퍼를 사용했습니다.
다만 동일한 생성 코드가 다른 ThemeBuild 테스트에도 반복되고 있어 유지보수 우려는 타당하다고 판단했습니다. 기존 시더를 그대로 사용하기보다는 필요한 필드만 받는 경량 fixture로 분리해 관련 테스트에서 재사용하도록 수정하겠습니다.
| private void assertStatusResponse( | ||
| ResultActions result, | ||
| String expectedStatus, | ||
| String expectedDownloadUrl | ||
| ) throws Exception { | ||
| result.andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.status").value(expectedStatus)); | ||
| JsonNode body = readBody(result); | ||
| assertThat(body.size()).isEqualTo(2); | ||
| assertThat(body.has("downloadUrl")).isTrue(); | ||
| if (expectedDownloadUrl == null) { | ||
| assertThat(body.get("downloadUrl").isNull()).isTrue(); | ||
| } else { | ||
| assertThat(body.get("downloadUrl").asText()).isEqualTo(expectedDownloadUrl); | ||
| } | ||
| } | ||
|
|
||
| private JsonNode readBody(ResultActions result) throws Exception { | ||
| return objectMapper.readTree(result.andReturn().getResponse().getContentAsString()); | ||
| } |
There was a problem hiding this comment.
기존 테스트에서는 응답 JSON을 DTO(Object)로 변환한 뒤 검증하는 방식을 사용한 것으로 이해했습니다. 이번 테스트에서는 JsonNode로 변환 후 필드 값을 검증하는 방식을 사용하고 있는데, 해당 방식으로 변경한 이유가 궁금합니다.
응답 스키마를 검증하는 목적이라면 기존 방식과 비교했을 때 JsonNode를 사용하는 방식의 장점이 있는지 확인하고 싶습니다.
There was a problem hiding this comment.
이번 테스트는 응답 값뿐 아니라 downloadUrl 필드가 명시적인 null로 포함되는지 등 실제 JSON 구조를 검증하기 위해 JsonNode를 사용했습니다. DTO 변환만으로는 필드 누락과 명시적인 null을 구분할 수 없기 때문입니다.
There was a problem hiding this comment.
현재 findByIdForUpdate에서 비관적 락을 사용하신 것으로 보입니다. 동시성 상황에서 발생할 수 있는 문제를 방지하기 위한 의도로 이해했는데, 해당 락이 필요한 구체적인 시나리오가 궁금합니다.
There was a problem hiding this comment.
동일한 테마·플랫폼으로 제작 요청이 동시에 들어오면, 두 요청이 모두 RUNNING job이 없다고 조회한 뒤 각각 job을 생성·dispatch할 수 있습니다.
RUNNING job이 없는 최초 생성 경로에는 잠글 대상 행이 없으므로, 요청 대상 ThemeComponent 행에 비관적 락을 걸어 조회부터 job 생성까지 직렬화했습니다. 두 번째 요청은 첫 트랜잭션의 커밋을 기다린 뒤, 기존 job이 여전히 RUNNING이면 이를 재사용합니다. 실제 패키지 제작은 커밋 이후 실행되므로 락은 startBuild 트랜잭션 동안만 유지됩니다.
| @Test | ||
| @Transactional | ||
| @DisplayName("테마를 삭제하면 DB cascade로 관련 build job도 삭제된다") | ||
| void deleteTheme_cascadesBuildJobs() { | ||
| ThemeBuildJob build = themeBuildJobRepository.saveAndFlush( | ||
| ThemeBuildJob.createRunning(theme, Platform.ANDROID)); | ||
| Integer themeComponentId = theme.getThemeComponentId(); | ||
| Long buildId = build.getBuildId(); | ||
| entityManager.clear(); | ||
|
|
||
| themeManageService.deleteTheme(themeComponentId); | ||
| entityManager.flush(); | ||
| entityManager.clear(); | ||
|
|
||
| assertThat(themeComponentRepository.existsById(themeComponentId)).isFalse(); | ||
| assertThat(themeBuildJobRepository.existsById(buildId)).isFalse(); | ||
| } |
There was a problem hiding this comment.
단위 테스트에서 DB casacde를 별도로 검증하는 이유가 궁금합니다
There was a problem hiding this comment.
해당 테스트는 단위 테스트가 아니라 @SpringBootTest와 H2를 사용하는 통합 테스트입니다. ThemeBuildJob을 애플리케이션 코드에서 직접 삭제하지 않고 DB의 ON DELETE CASCADE에 의존하고 있어, H2 환경에서 테마 삭제 시 관련 build job이 실제로 함께 삭제되는 persistence 계약을 검증하기 위해 추가했습니다.
There was a problem hiding this comment.
확인했습니다. 단순히 DB cascade가 아닌 테마 삭제 시 시나리오를 검증한 것으로 보입니다. 해당 테스트는 필요하다는 의견에 동의합니다.
kym8821
left a comment
kym8821
left a comment
There was a problem hiding this comment.
전반적으로 잘 작성되었습니다. 다만 ThemeBuild에 대한 Fixture 계층이 추가되었는데 도메인 간 구분이 명확하지 않은 것 같습니다. 해당 부분만 재확인 부탁드립니다.
There was a problem hiding this comment.
다시 확인해보니 ThemeFixture에서 User 생성까지 담당하고 있습니다. 만약 별도 Fixture 계층을 둘 것이라면 도메인별로 구분하는 것이 좋아보입니다.
There was a problem hiding this comment.
확인했습니다. user 생성을 UserFixture로 따로 분리하도록 하겠습니다.
| @Test | ||
| @Transactional | ||
| @DisplayName("테마를 삭제하면 DB cascade로 관련 build job도 삭제된다") | ||
| void deleteTheme_cascadesBuildJobs() { | ||
| ThemeBuildJob build = themeBuildJobRepository.saveAndFlush( | ||
| ThemeBuildJob.createRunning(theme, Platform.ANDROID)); | ||
| Integer themeComponentId = theme.getThemeComponentId(); | ||
| Long buildId = build.getBuildId(); | ||
| entityManager.clear(); | ||
|
|
||
| themeManageService.deleteTheme(themeComponentId); | ||
| entityManager.flush(); | ||
| entityManager.clear(); | ||
|
|
||
| assertThat(themeComponentRepository.existsById(themeComponentId)).isFalse(); | ||
| assertThat(themeBuildJobRepository.existsById(buildId)).isFalse(); | ||
| } |
There was a problem hiding this comment.
확인했습니다. 단순히 DB cascade가 아닌 테마 삭제 시 시나리오를 검증한 것으로 보입니다. 해당 테스트는 필요하다는 의견에 동의합니다.
kym8821
left a comment
kym8821
left a comment
There was a problem hiding this comment.
고생하셨습니다. 최종 점검 후 머지하셔도 좋습니다. 테스트는 한번 돌리고 머지해주세요
2 participants
PR 타입
작업 내용
테마 패키지 비동기 제작
기존 API 변경
관련 이슈: #137
테스트 결과
PR 체크리스트