-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feat/#453][FEW2.0] GPT 기반 컨텐츠 데이터 생성 #605
Conversation
- GeneratorConfig 생성 - Generator Prefix 추가
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiErrorDecoder.kt (2)
43-44
: 예외 메시지 내용이 실제 로직과 상충될 수 있습니다.
if (refusal != null)
인 상황에서 “choices count is 0”라는 문구가 사용되면, 거부(refusal)의 유무와 무관하게 응답 선택지가 0개라는 인상을 줄 수 있습니다. 해당 문구를 더 명확하게 바꾸어 실제 거부 상태를 반영하도록 개선하는 것이 좋겠습니다.
51-55
: 커스텀 예외 클래스를 도입하여 예외 의도를 명확히 전달하세요.
현재는RuntimeException
에 단일 메시지를 담아서 던지는데,RateLimitExceededException
,InvalidApiKeyException
,OpenAiRefusalException
등 용도에 부합하는 예외 클래스를 추가하면 오류 추적 및 처리 로직이 보다 명확해집니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiErrorDecoder.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/core/gpt/completion/ChatCompletion.kt
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- domain/generator/src/main/kotlin/com/few/generator/core/gpt/completion/ChatCompletion.kt
🔇 Additional comments (1)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiErrorDecoder.kt (1)
25-27
: OpenAI 응답 상태코드에 따른 세분화된 예외 처리를 고려해주세요.
현재 단순 로그 및 기본 디코더에 위임만 하고 있어 응답 상태코드별로 적절한 예외를 던지지 못합니다. 429, 401 등 OpenAI가 반환하는 대표적인 에러 코드에 따라 맞춤 예외를 던지면 디버깅이 더욱 편리해집니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
domain/generator/src/main/kotlin/com/few/generator/core/Scrapper.kt (3)
27-41
: 재시도 로직 개선 제안재시도 로직이 구현되어 있지만, 지수 백오프(exponential backoff) 방식을 적용하면 더 효율적일 수 있습니다. 또한 실패 원인을 더 상세히 로깅하면 디버깅에 도움이 될 것입니다.
fun get(url: String): Document? { + var currentSleepTime = sleepTime repeat(retryCount) { attempt -> try { val response = connectionFactory .createConnection(url) .get() Thread.sleep(sleepTime) return response } catch (e: Exception) { - log.error { "Request failed: ${e.message}" } + log.error { "Request failed (attempt ${attempt+1}/$retryCount) for URL: $url - Error: ${e.message}" } + currentSleepTime *= 2 // 지수 백오프 + Thread.sleep(currentSleepTime) } } log.warn { "최대 재시도 횟수($retryCount)를 초과하여 URL 접근에 실패했습니다: $url" } return null }
48-62
: 텍스트 처리 로직 최적화 제안텍스트 처리 로직에서 문장 분리 및 필터링 과정이 다소 복잡합니다. 중복 검증 로직이 있으며(61번 줄에서 단어 수 재확인), 메소드를 더 작은 단위로 분리하면 가독성이 향상될 것입니다.
fun getTexts(text: String): List<String> { val texts = mutableListOf<String>() val sanitizedText = text.replace("\u00A0", " ").replace("\t", " ") val paragraphs = sanitizedText.split("\n").map { it.trim() }.filter { it.isNotEmpty() } paragraphs.forEach { paragraph -> val separators = listOf("... ", ". ", "? ", "! ", "; ", "。", "?", "!") var processedText = paragraph separators.forEach { sep -> processedText = processedText.replace(sep, "$sep|") } val sentences = processedText.split("|").map { it.trim() } - texts.addAll(sentences.filter { isValidSentence(it) && it.length >= 10 }) + texts.addAll(sentences.filter { it.length >= 10 && isValidSentence(it) }) } - return texts.distinct().filter { it.split(" ").size >= 4 } + return texts.distinct() // isValidSentence에서 이미 단어 수를 검증했으므로 중복 검증 제거 }
72-105
: 이미지 추출 로직 모듈화 제안이미지 추출 로직이 너무 복잡하고 길어 가독성이 떨어집니다. 각 추출 방식(img 태그, 스타일 속성, 정규식)을 별도 메소드로 분리하면 유지보수성이 향상될 것입니다.
다음과 같이 분리하는 것을 제안합니다:
fun getImages(soup: Document): List<String> { val imageUrls = mutableSetOf<String>() + imageUrls.addAll(getImagesFromTags(soup)) + imageUrls.addAll(getImagesFromStyles(soup)) + imageUrls.addAll(getImagesFromHtml(soup)) + return imageUrls.toList() +} + +private fun getImagesFromTags(soup: Document): Set<String> { + val imageUrls = mutableSetOf<String>() soup.select("img").forEach { img -> img.attr("src")?.takeIf { isValidImageUrl(it) }?.let { imageUrls.add(it) } img.attr("srcset")?.split(",")?.map { it.trim().split(" ")[0] }?.forEach { if (isValidImageUrl(it)) imageUrls.add(it) } img.attr("data-src")?.takeIf { isValidImageUrl(it) }?.let { imageUrls.add(it) } } + return imageUrls +} + +private fun getImagesFromStyles(soup: Document): Set<String> { + val imageUrls = mutableSetOf<String>() soup.select("[style]").forEach { tag -> val style = tag.attr("style") val foundUrls = Pattern.compile("url\\(([^)]+)\\)").matcher(style) while (foundUrls.find()) { val cleanedUrl = foundUrls .group(1) .replace("\"", "") .replace("'", "") .trim() if (isValidImageUrl(cleanedUrl)) imageUrls.add(cleanedUrl) } } + return imageUrls +} + +private fun getImagesFromHtml(soup: Document): Set<String> { + val imageUrls = mutableSetOf<String>() val htmlStr = soup.toString() val regexPattern = "(https?://[^\\s'\"]+\\.(?:jpg|jpeg|png|gif|webp|svg))(?:\\?[^)\\s'\"]+)?" val regexMatches = Pattern.compile(regexPattern, Pattern.CASE_INSENSITIVE).matcher(htmlStr) while (regexMatches.find()) { regexMatches.group(1)?.takeIf { isValidImageUrl(it) }?.let { imageUrls.add(it) } } + return imageUrls - return imageUrls.toList() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
domain/generator/src/main/kotlin/com/few/generator/core/Scrapper.kt
(1 hunks)
🧰 Additional context used
🪛 detekt (1.23.7)
domain/generator/src/main/kotlin/com/few/generator/core/Scrapper.kt
[warning] 68-68: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (3)
domain/generator/src/main/kotlin/com/few/generator/core/Scrapper.kt (3)
10-16
: 데이터 구조가 명확하게 정의되어 있습니다
ScrappedResult
데이터 클래스는 스크래핑 결과를 저장하기 위한 모든 필요한 필드를 포함하고 있으며, 기본값 설정이 잘 되어 있습니다.
18-25
: 클래스 구성 및 의존성 주입이 잘 구현되어 있습니다스프링
@Component
어노테이션을 통해 의존성 주입이 잘 설정되어 있으며, 재시도 횟수와 대기 시간을 파라미터로 받아 구성 가능하게 만든 점이 좋습니다. 로깅 설정도 적절합니다.
64-71
: 예외 처리 시 예외를 단순 무시하지 말고 로깅 필요Exception을 잡기만 하고 넘어가면 원인 파악이 어렵습니다. 예외 정보를 로깅하여 디버깅에 유용하도록 개선해 주세요.
fun isValidImageUrl(url: String): Boolean = try { val uri = URI(url) - imageExtensions.any { uri.path.lowercase().endsWith(it) } + return imageExtensions.any { uri.path.lowercase().endsWith(it) } } catch (e: Exception) { + log.warn { "유효하지 않은 이미지 URL: $url / 예외: ${e.message}" } - false + return false }🧰 Tools
🪛 detekt (1.23.7)
[warning] 68-68: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
fun scrape(url: String): ScrappedResult? { | ||
val soup = get(url) ?: return null | ||
|
||
soup.select("script, style, nav, footer, header").forEach { it.remove() } | ||
|
||
val mainContent = soup.selectFirst("main, article, div.content") ?: soup | ||
val title = soup.selectFirst("title")?.text()?.trim() | ||
val description = soup.selectFirst("meta[name=description]")?.attr("content")?.trim() | ||
val thumbnailImageUrl = soup.selectFirst("meta[property=og:image]")?.attr("content")?.trim() ?: "" | ||
|
||
val textElements = mainContent.select("p, h1, h2, h3, h4, h5, h6, div") | ||
val allTexts = mutableListOf<String>() | ||
textElements.forEach { element -> | ||
element.text()?.let { allTexts.add(it) } | ||
element.wholeText()?.let { allTexts.add(it) } | ||
} | ||
|
||
val rawTexts = getTexts(allTexts.joinToString("\n")) | ||
val images = getImages(soup) | ||
|
||
if (title == null || description == null) throw RuntimeException("스크래핑 실패") | ||
|
||
return ScrappedResult(title, description, thumbnailImageUrl, rawTexts, images) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
예외 처리 개선 필요
현재 title
이나 description
이 null인 경우 단순히 RuntimeException을 발생시키는데, 이는 디버깅이 어렵습니다. 더 구체적인 예외 클래스를 사용하고 상황에 대한 정보를 추가하는 것이 좋습니다.
fun scrape(url: String): ScrappedResult? {
val soup = get(url) ?: return null
soup.select("script, style, nav, footer, header").forEach { it.remove() }
val mainContent = soup.selectFirst("main, article, div.content") ?: soup
val title = soup.selectFirst("title")?.text()?.trim()
val description = soup.selectFirst("meta[name=description]")?.attr("content")?.trim()
val thumbnailImageUrl = soup.selectFirst("meta[property=og:image]")?.attr("content")?.trim() ?: ""
val textElements = mainContent.select("p, h1, h2, h3, h4, h5, h6, div")
val allTexts = mutableListOf<String>()
textElements.forEach { element ->
element.text()?.let { allTexts.add(it) }
element.wholeText()?.let { allTexts.add(it) }
}
val rawTexts = getTexts(allTexts.joinToString("\n"))
val images = getImages(soup)
- if (title == null || description == null) throw RuntimeException("스크래핑 실패")
+ if (title == null || description == null) {
+ log.error { "URL '$url'에서 필수 콘텐츠(제목 또는 설명) 스크래핑 실패" }
+ throw IllegalStateException("URL '$url'에서 스크래핑 실패: 제목 또는 설명을 찾을 수 없습니다.")
+ }
return ScrappedResult(title, description, thumbnailImageUrl, rawTexts, images)
}
📝 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.
fun scrape(url: String): ScrappedResult? { | |
val soup = get(url) ?: return null | |
soup.select("script, style, nav, footer, header").forEach { it.remove() } | |
val mainContent = soup.selectFirst("main, article, div.content") ?: soup | |
val title = soup.selectFirst("title")?.text()?.trim() | |
val description = soup.selectFirst("meta[name=description]")?.attr("content")?.trim() | |
val thumbnailImageUrl = soup.selectFirst("meta[property=og:image]")?.attr("content")?.trim() ?: "" | |
val textElements = mainContent.select("p, h1, h2, h3, h4, h5, h6, div") | |
val allTexts = mutableListOf<String>() | |
textElements.forEach { element -> | |
element.text()?.let { allTexts.add(it) } | |
element.wholeText()?.let { allTexts.add(it) } | |
} | |
val rawTexts = getTexts(allTexts.joinToString("\n")) | |
val images = getImages(soup) | |
if (title == null || description == null) throw RuntimeException("스크래핑 실패") | |
return ScrappedResult(title, description, thumbnailImageUrl, rawTexts, images) | |
} | |
fun scrape(url: String): ScrappedResult? { | |
val soup = get(url) ?: return null | |
soup.select("script, style, nav, footer, header").forEach { it.remove() } | |
val mainContent = soup.selectFirst("main, article, div.content") ?: soup | |
val title = soup.selectFirst("title")?.text()?.trim() | |
val description = soup.selectFirst("meta[name=description]")?.attr("content")?.trim() | |
val thumbnailImageUrl = soup.selectFirst("meta[property=og:image]")?.attr("content")?.trim() ?: "" | |
val textElements = mainContent.select("p, h1, h2, h3, h4, h5, h6, div") | |
val allTexts = mutableListOf<String>() | |
textElements.forEach { element -> | |
element.text()?.let { allTexts.add(it) } | |
element.wholeText()?.let { allTexts.add(it) } | |
} | |
val rawTexts = getTexts(allTexts.joinToString("\n")) | |
val images = getImages(soup) | |
if (title == null || description == null) { | |
log.error { "URL '$url'에서 필수 콘텐츠(제목 또는 설명) 스크래핑 실패" } | |
throw IllegalStateException("URL '$url'에서 스크래핑 실패: 제목 또는 설명을 찾을 수 없습니다.") | |
} | |
return ScrappedResult(title, description, thumbnailImageUrl, rawTexts, images) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiErrorDecoder.kt (2)
20-35
: 🛠️ Refactor suggestion상세한 에러 처리로 개선하세요.
현재 구현은 상태 코드만 로깅하고 응답 본문의 상세 정보를 활용하지 않고 있습니다. OpenAI API는 자세한 에러 메시지를 제공하므로 이를 파싱하고 적절하게 처리해야 합니다.
override fun decode( methodKey: String, response: Response, ): Exception = runCatching { - log.error { "Open AI API Status Code: ${response.status()}" } + log.error { "OpenAI API 에러 - 상태 코드: ${response.status()}" } ResponseClassThreadLocal.clear() val responseBody = response .body() ?.asInputStream() ?.reader() ?.readText() ?: throw RuntimeException("Empty response body")
51-57
: 🛠️ Refactor suggestion상태 코드에 따른 예외 처리 개선
OpenAI API의 상태 코드에 따라 더 구체적인 예외 처리가 필요합니다. 현재는 상태 코드를 로깅만 하고 실제 처리는 defaultDecoder에 위임하고 있습니다.
- defaultDecoder.decode(methodKey, response) + when (response.status()) { + 429 -> RateLimitExceededException("요청 한도 초과: $responseBody") + 401 -> InvalidApiKeyException("유효하지 않은 API 키: $responseBody") + 400 -> InvalidRequestException("잘못된 요청: $responseBody") + else -> defaultDecoder.decode(methodKey, response) + } }.getOrElse { log.error(it) { "Error while decoding response" } defaultDecoder.decode(methodKey, response) }.also { ResponseClassThreadLocal.clear() }
🧹 Nitpick comments (1)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiErrorDecoder.kt (1)
43-45
: 중복 조건 확인 수정 필요44번 라인에서 중복 조건 확인이 있습니다.
refusal
이 null이 아니라고 이미 확인했는데 에러 메시지에서 다시 null 체크를 하고 있습니다.if (refusal != null) { - throw RuntimeException("ChatGpt response choices count is 0. Refusal: ${refusal ?: "NotFound"}") + throw RuntimeException("ChatGpt response choices count is 0. Refusal: $refusal") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiEncoder.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiErrorDecoder.kt
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiEncoder.kt
🔇 Additional comments (3)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiErrorDecoder.kt (3)
1-11
: 패키지 및 임포트 구조가 적절합니다.패키지 구조와 필요한 클래스들의 임포트가 잘 정의되어 있습니다.
12-18
: 컴포넌트 구성 및 의존성 주입이 잘 되어 있습니다.Spring의
@Component
어노테이션과@Qualifier
를 사용한 의존성 주입이 적절하게 구현되어 있습니다. 기본 디코더를 참조로 유지하는 것은 폴백 처리를 위한 좋은 접근법입니다.
24-55
:❓ Verification inconclusive
ResponseClassThreadLocal 사용에 대한 문서화 필요
ResponseClassThreadLocal.clear()
를 호출하고 있지만, 이 클래스가 무엇인지, 어떤 역할을 하는지 문서화되어 있지 않습니다. 코드 가독성과 유지보수성을 위해 이에 대한 설명이나 주석을 추가하는 것이 좋습니다.
🌐 Web query:
ThreadLocal in Feign client for response type handling
💡 Result:
To handle
ThreadLocal
variables in a Feign client for response type handling, you must address threading constraints introduced by libraries like Hystrix or Resilience4j. Here's how to ensureThreadLocal
context propagation:
1. Thread Isolation Configuration
Feign integrates with Hystrix (or Resilience4j) for circuit breaking, which uses thread pools by default. This disrupts
ThreadLocal
context. To resolve this:Option A: Disable Hystrix
feign: hystrix: enabled: falseOption B: Use Semaphore Isolation
hystrix: command: default: execution: isolation: strategy: SEMAPHORE # Use semaphore instead of thread poolsThis ensures Feign executes on the same thread, preserving
ThreadLocal
values[1][3][6].
2. Propagating
ThreadLocal
to Child ThreadsIf asynchronous processing is required (e.g., with
CompletableFuture
), useInheritableThreadLocal
to pass context to child threads:private static final InheritableThreadLocal<String> context = new InheritableThreadLocal<>();Note: This only copies the parent thread’s context once. For thread pools, manual propagation is needed[2][8].
3. Custom
RequestInterceptor
withThreadLocal
Access
ThreadLocal
in Feign requests via interceptors:public class ThreadLocalInterceptor implements RequestInterceptor { @Override public void apply(RequestTemplate template) { String value = MyThreadLocalContext.get(); // Retrieve ThreadLocal value template.header("X-Context", value); } }Register the interceptor in Feign configuration:
feign: client: config: default: requestInterceptors: - com.example.ThreadLocalInterceptor
4. Error Handling with
ThreadLocal
Custom error decoders or fallbacks may lose
ThreadLocal
context if executed on separate threads. Ensure error handling respects the original thread:@Component public class CustomErrorDecoder implements ErrorDecoder { @Override public Exception decode(String methodKey, Response response) { String contextValue = MyThreadLocalContext.get(); // Access ThreadLocal return new CustomException("Error with context: " + contextValue); } }
5. Pitfalls and Solutions
Issue Solution Thread pool reuse causing stale ThreadLocal
valuesExplicitly clear ThreadLocal
after processing:MyThreadLocalContext.remove()
[8]Asynchronous Feign clients (e.g., CompletableFuture
)Use InheritableThreadLocal
or manually propagate context[14]Third-party library conflicts (e.g., NoSuchMethodError
)Ensure consistent dependency versions and clean rebuilds[10]
Example Workflow
- Set
ThreadLocal
in the main thread:MyThreadLocalContext.set("user-auth-token");- Feign Client Call propagates the header via
RequestInterceptor
.- Error Handling retains context using semaphore isolation or disabled Hystrix.
- Cleanup:
finally { MyThreadLocalContext.remove(); // Prevent memory leaks }By configuring thread isolation and propagating
ThreadLocal
values through interceptors, you ensure consistent context availability across Feign client requests and error handling.Citations:
- 1: https://docs.spring.io/spring-cloud-openfeign/docs/2.2.8.RELEASE/reference/html/
- 2: https://www.north-47.com/microservice-architecture-using-java-thread-locals-and-tomcat-spring-capabilities-for-automated-information-propagation/
- 3: https://cloud.spring.io/spring-cloud-openfeign/reference/html/
- 4: https://arnoldgalovics.com/feign-error-handling/
- 5: https://github.com/OpenFeign/feign/blob/master/README.md
- 6: https://cloud.spring.io/spring-cloud-netflix/multi/multi_spring-cloud-feign.html
- 7: https://cloud.spring.io/spring-cloud-openfeign/2.1.x/multi/multi_spring-cloud-feign.html
- 8: https://blog.ycrash.io/threadlocal-easy-introduction/
- 9: https://cloud.spring.io/spring-cloud-netflix/2.0.x/single/spring-cloud-netflix.html
- 10: https://rollbar.com/blog/how-to-fix-java-lang-nosuchmethoderror-in-java/
- 11: Loss of ThreadLocal Information in OpenFeign with Resilience4j Circuit Breaker spring-cloud/spring-cloud-openfeign#949
- 12: https://www.baeldung.com/java-feign-client-exception-handling
- 13: https://gitee.com/ZsonQ/feign
- 14: https://www.baeldung.com/feign-client-completablefuture-spring-boot
- 15: https://resilience4j.readme.io/docs/bulkhead
ResponseClassThreadLocal 문서화 보완 필요
현재 코드에서는
ResponseClassThreadLocal.clear()
를 호출하고 있으나, 해당 클래스의 역할과clear()
메서드 호출의 의도가 명시적으로 문서화되어 있지 않습니다.
보다 나은 코드 가독성과 유지보수를 위해 아래 사항을 고려하여 관련 주석 또는 문서를 추가해 주시기 바랍니다.
ResponseClassThreadLocal
클래스가 수행하는 역할 및 책임 명시- 왜 특정 시점에
clear()
호출이 필요한지, 특히 Feign 클라이언트 환경이나 Hystrix/Resilience4j 등의 스레드 관리와의 연관성을 간략하게 설명- 필요 시,
ThreadLocal
사용으로 인한 스레드 컨텍스트 전파 관련 고려사항(예, semaphore isolation 또는 부모 스레드 컨텍스트 복사 등)에 대한 참고 자료 링크 추가
val completion = gson.fromJson(responseBody, ChatCompletion::class.java) | ||
val refusal = | ||
completion.choices | ||
?.get(0) | ||
?.message | ||
?.refusal | ||
|
||
if (refusal != null) { | ||
throw RuntimeException("ChatGpt response choices count is 0. Refusal: ${refusal ?: "NotFound"}") | ||
} | ||
|
||
if (completion.error?.message != null) { | ||
throw RuntimeException("ChatGpt response error message: ${completion.error.message}") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
사용자 정의 예외 클래스로 개선하세요.
현재 모든 에러 상황에서 일반적인 RuntimeException
을 사용하고 있습니다. 다양한 OpenAI API 오류 유형에 맞는 사용자 정의 예외 클래스를 생성하여 호출자가 더 세밀하게 오류를 처리할 수 있도록 해야 합니다.
- if (refusal != null) {
- throw RuntimeException("ChatGpt response choices count is 0. Refusal: ${refusal ?: "NotFound"}")
- }
-
- if (completion.error?.message != null) {
- throw RuntimeException("ChatGpt response error message: ${completion.error.message}")
- }
+ if (refusal != null) {
+ throw GptRefusalException("ChatGpt 응답 거부: $refusal")
+ }
+
+ if (completion.error?.message != null) {
+ throw GptApiException("ChatGpt 에러 메시지: ${completion.error.message}")
+ }
다음과 같은 사용자 정의 예외 클래스를 생성하세요:
class GptRefusalException(message: String) : RuntimeException(message)
class GptApiException(message: String) : RuntimeException(message)
class RateLimitExceededException(message: String) : RuntimeException(message)
class InvalidApiKeyException(message: String) : RuntimeException(message)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiDecoder.kt (3)
21-41
: 예외 처리를 더 구체적으로 개선할 필요가 있습니다.현재 모든 오류 상황에서 일반적인
RuntimeException
을 사용하고 있습니다. 더 명확한 예외 클래스를 정의하거나 사용하면 오류 처리가 더 효과적일 것입니다.- ?: throw RuntimeException("Empty response body") + ?: throw OpenAiDecodingException("Empty response body") - throw RuntimeException("Failed to decode response body", it) + throw OpenAiDecodingException("Failed to decode response body: ${it.message}", it)또한
ResponseClassThreadLocal.clear()
가 성공/실패 여부와 상관없이 항상 호출되는 것은 좋습니다만, 이 ThreadLocal 변수에 대한 설명이나 문서화가 있으면 더 좋을 것 같습니다.
58-71
: 잠재적 NPE 위험과 로깅 개선 필요58-71줄의
validateResponse
메서드에서 발견된 문제점들:
- 62-64줄:
choices
가 비어있는 경우 NPE 가능성이 있습니다.- 66줄: 로그 메시지에 어떤 요청에 대한 응답인지 컨텍스트 정보가 부족합니다.
- 69줄: 예외 메시지에 더 상세한 정보를 포함하면 디버깅에 도움이 될 것입니다.
- log.info { "Asking ChatGpt response choices count: $choicesCount" } + log.info { "ChatGPT API response for completion ${completion.id}: $choicesCount choices received" } if (choicesCount == 0 || refusal != null) { - throw RuntimeException("ChatGpt response choices count is 0. Refusal: ${refusal ?: "NotFound"}") + throw RuntimeException("Invalid ChatGPT response for completion ${completion.id}. Choices: $choicesCount, Refusal: ${refusal ?: "NotFound"}") }또한 62-64줄의 NPE 방지를 위해 안전한 호출 연산자(?.)를 일관되게 사용하세요.
1-72
: 문서화 (KDoc) 부족이 클래스와 주요 메서드들에 KDoc을 추가하여 사용 목적, 매개변수, 반환 값, 예외 등을 문서화하는 것이 좋습니다. 이는 코드의 가독성과 유지보수성을 크게 향상시킵니다.
예시:
/** * OpenAI API 응답을 디코딩하는 Feign 디코더 구현체. * ChatGPT API로부터 받은 JSON 응답을 GptResponse 객체로 변환합니다. */ @Component class OpenAiDecoder( @Qualifier(GeneratorGsonConfig.GSON_BEAN_NAME) private val gson: Gson, ) : Decoder { // ... /** * Feign 응답을 디코딩하여 GptResponse 객체로 변환합니다. * * @param response 디코딩할 Feign 응답 * @param type 대상 타입 (무시됨) * @return 디코딩된 GptResponse 객체 * @throws RuntimeException 응답 본문이 비어있거나, 디코딩에 실패한 경우 */ override fun decode( // ... ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiDecoder.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/core/gpt/ChatGpt.kt
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- domain/generator/src/main/kotlin/com/few/generator/core/gpt/ChatGpt.kt
🔇 Additional comments (3)
domain/generator/src/main/kotlin/com/few/generator/config/feign/OpenAiDecoder.kt (3)
1-13
: 패키지 구조와 임포트가 적절합니다.패키지 네이밍과 관련 임포트들이 잘 구성되어 있습니다.
14-19
: 의존성 주입 및 로거 설정이 잘되어 있습니다.Spring의 의존성 주입을 위한 어노테이션과 Gson 빈 지정을 위한 Qualifier가 적절하게 사용되었습니다. 코틀린 로깅 설정도 잘 되어 있습니다.
43-56
:❓ Verification inconclusive
ThreadLocal 사용 방식 검토 필요
ResponseClassThreadLocal
의 사용이 이 클래스와 강하게 결합되어 있습니다. 이는 단위 테스트를 어렵게 만들 수 있습니다. 또한 모든GptResponse
구현체가completionId
속성을 가진다는 것을 전제로 하고 있습니다.또한 아래와 같이 확인이 필요합니다:
GptResponse
인터페이스 또는 추상 클래스가completionId
속성을 정의하는지ResponseClassThreadLocal
이 항상 적절하게 초기화되고 정리되는지
🌐 Web query:
Are there alternatives to ThreadLocal for passing context in Spring applications?
💡 Result:
Yes, Spring applications offer several alternatives to
ThreadLocal
for propagating context across threads, each addressing different use cases and challenges:1. Spring
TaskDecorator
for Async Propagation
- Use Case: Propagating context (e.g., tenant IDs, security context) to asynchronous tasks using
ThreadPoolTaskExecutor
.- Implementation:
- Decorate tasks to copy context from the parent thread to the child thread.
- Example from [1]:
@Bean public TaskDecorator tenantStoreTaskDecorator() { return task -> { String tenantId = tenantStore.getTenantId(); return () -> { try { tenantStore.setTenantId(tenantId); task.run(); } finally { tenantStore.clear(); } }; }; }- Configure the executor with the decorator:
@Bean public ThreadPoolTaskExecutor taskExecutor() { ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); executor.setTaskDecorator(tenantStoreTaskDecorator()); return executor; }
2. Spring Security Context Propagation
- Use Case: Propagating security context (e.g.,
Authentication
) to child threads.- Options:
- Inheritable Mode: Set
SecurityContextHolder
strategy toMODE_INHERITABLETHREADLOCAL
[3]:SecurityContextHolder.setStrategyName(SecurityContextHolder.MODE_INHERITABLETHREADLOCAL);- Wrapped Executors: Use
DelegatingSecurityContextAsyncTaskExecutor
to wrap tasks with the current security context [3]:@Bean public AsyncTaskExecutor taskExecutor(ThreadPoolTaskExecutor delegate) { return new DelegatingSecurityContextAsyncTaskExecutor(delegate); }
3. Scoped Values (Java 21+)
- Use Case: Lightweight, structured context propagation compatible with virtual threads.
- Implementation:
ScopedValue
provides immutable, thread-specific data that is inherited by child threads and automatically cleared when the scope exits [12][17]:static final ScopedValue<String> PRINCIPAL = ScopedValue.newInstance(); void handleRequest() { ScopedValue.where(PRINCIPAL, "user123") .run(() -> processRequest()); }
4. Spring-Managed Scoped Beans
- Use Case: Request/session-scoped data without direct
ThreadLocal
usage.- Implementation:
- Define beans with
@RequestScope
or@SessionScope
, which Spring manages via proxies:@Component @RequestScope public class UserContext { private String userId; // Getters/setters }- Inject the bean where needed:
@Autowired private UserContext userContext;
5. TransmittableThreadLocal (Library)
- Use Case: Propagating context across thread pools.
- Implementation:
- This library enhances
InheritableThreadLocal
to work with pooled threads [4][10]:private static final TransmittableThreadLocal<String> context = new TransmittableThreadLocal<>();- Requires wrapping thread pools or using a Java agent for automatic propagation.
6. ThreadLocalTargetSource (Spring AOP)
- Use Case: Delegating
ThreadLocal
management to Spring.- Implementation:
- Configure a
ThreadLocalTargetSource
bean to create per-thread instances [1][7]:<bean id="tenantStore" class="org.springframework.aop.target.ThreadLocalTargetSource"> <property name="targetBeanName" value="tenantStoreTarget"/> </bean>- Inject the proxied bean into components.
Comparison Table
Approach Use Case Thread Pool Safe? Spring Integration TaskDecorator
Async tasks (e.g., tenant ID) ✅ Native support Security Wrappers Security context propagation ✅ Native support Scoped Beans Request/session data ✅ Native support ScopedValue
Virtual threads, structured tasks ✅ Requires Java 21+ TransmittableThreadLocal
Legacy thread pools ✅ Library dependency By leveraging these alternatives, developers can avoid
ThreadLocal
pitfalls (e.g., memory leaks) while ensuring context propagates correctly across threads and frameworks.Citations:
- 1: https://tech.asimio.net/2024/09/12/Propagating-data-to-Async-Threads-with-ThreadLocalTargetSource-TaskDecorator-Spring-Boot.html
- 2: https://docs.spring.io/spring-framework/reference/web/webflux-webclient/client-context.html
- 3: https://howtodoinjava.com/spring-security/spring-security-context-propagation/
- 4: https://anyroad.dev/posts/2022/spring-request-context-java-thread-local-inreritance/
- 5: https://docs.spring.io/spring-framework/reference/core/beans/context-introduction.html
- 6: https://community.temporal.io/t/injected-workflowcontext-vs-threadlocal/549
- 7: https://dzone.com/articles/an-alternative-approach-to-threadlocal-using-sprin-1
- 8: https://dzone.com/articles/what-is-a-spring-context
- 9: Getting rid of ThreadLocals open-telemetry/opentelemetry-java#6584
- 10: https://www.kapresoft.com/java/2023/12/22/java-threadlocal-alternatives.html
- 11: https://www.youtube.com/watch?v=dST46t0zWGk
- 12: https://dzone.com/articles/preserving-context-across-threads
- 13: https://realjenius.com/2023/11/10/scoped-values/
- 14: https://www.baeldung.com/spring-web-contexts
- 15: https://github.com/cristianprofile/spring-boot-thread-local-and-bouncy-castle
- 16: https://www.kapresoft.com/java/2023/12/21/java-threadlocal-vs-thread.html
- 17: https://www.infoworld.com/article/2336255/understanding-javas-new-scopedvalue.html
- 18: https://blog.ycrash.io/pitfalls-to-avoid-when-switching-to-virtual-threads/
- 19: https://dzone.com/articles/mastering-thread-local-variables-in-java-a-compreh
- 20: https://www.baeldung.com/java-threadlocal
ResponseClassThreadLocal 사용 방식 및 GptResponse 인터페이스 검토 필요
ResponseClassThreadLocal
가 해당 클래스와 강하게 결합되어 단위 테스트 작성에 어려움을 줄 수 있으므로, Spring에서 제공하는 TaskDecorator, RequestScope와 같은 대안을 고려해보면 좋겠습니다.- 또한 모든
GptResponse
구현체가completionId
속성을 보유하고 있다는 전제가 확실한지 확인할 필요가 있습니다. 만약 그렇지 않다면, 공통 인터페이스나 추상 클래스로 해당 속성을 보장하는 구조 개선을 검토해 주세요.- 추가로,
ResponseClassThreadLocal
의 초기화 및 정리(clean-up) 로직이 안전하게 관리되고 있는지 면밀히 점검해 주세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
domain/generator/src/main/kotlin/com/few/generator/domain/ProvisioningContents.kt (1)
8-8
: TODO 주석에 대한 구체적인 계획 필요
// TODO: DB컬럼 타입 변경 필요
가 명시되어 있으므로, 컬럼 타입을 실제로 변경할 예정인지 또는 마이그레이션 계획이 있는지 구체적으로 정리해 주세요.필요하시다면, DB 마이그레이션 스크립트 작성을 도와드릴 수 있습니다. 요청하시면 새 이슈를 생성하거나, 여기에 추가 제안 드리겠습니다.
domain/generator/src/main/kotlin/com/few/generator/service/GenService.kt (1)
20-64
: 중복된 코드 블록 개선 제안
genBasic
,genKorean
,genKoreanQuestion
,genKoreanLongQuestion
를 생성하기 위한Material
인스턴스가 반복적으로 정의되고 있습니다. 이를 간결화하여 유지 보수성과 가독성을 높일 수 있도록 리팩터링을 제안드립니다.예시 리팩터링:
private fun createMaterial( provisioningContents: ProvisioningContents, rawContents: RawContents, headline: String? = null, summary: String? = null ): Material { return Material( provisioningContentsId = provisioningContents.id!!, title = rawContents.title, description = rawContents.description, coreTextsJson = provisioningContents.coreTextsJson, headline = headline, summary = summary, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
domain/generator/src/main/kotlin/com/few/generator/domain/Gen.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/domain/ProvisioningContents.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/service/GenService.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/service/ProvisioningService.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/service/strategy/BasicGenGenerationStrategy.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/service/strategy/KoreanGenGenerationStrategy.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/service/strategy/KoreanLongQuestionGenGenerationStrategy.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/service/strategy/KoreanQuestionGenGenerationStrategy.kt
(1 hunks)domain/generator/src/main/kotlin/com/few/generator/service/strategy/Material.kt
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- domain/generator/src/main/kotlin/com/few/generator/service/strategy/KoreanGenGenerationStrategy.kt
- domain/generator/src/main/kotlin/com/few/generator/domain/Gen.kt
- domain/generator/src/main/kotlin/com/few/generator/service/strategy/KoreanLongQuestionGenGenerationStrategy.kt
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: api-test
- GitHub Check: docs
- GitHub Check: lint
🔇 Additional comments (9)
domain/generator/src/main/kotlin/com/few/generator/service/strategy/Material.kt (1)
3-10
: 자료 클래스 선언 적절해당
Material
클래스는 필요한 필드를 잘 구조화하여 선언하였습니다. 다만,coreTextsJson
등 JSON 형태로 관리되는 필드는 차후 타입 안정성이나 검증 로직을 고려해볼 수 있습니다.domain/generator/src/main/kotlin/com/few/generator/domain/ProvisioningContents.kt (1)
16-21
: JSON 컬럼에 대한 이전 리뷰 코멘트 재확인
TEXT 컬럼에 JSON 문자열을 저장하는 것은 타입 안정성을 해칠 수 있습니다.
과거에도 동일한 개선 사항이 제시되었으므로, 여전히 유효한 제안입니다.- @Column(columnDefinition = "TEXT", nullable = false) - val bodyTextsJson: String = "[]", - @Column(columnDefinition = "TEXT", nullable = false) - val coreTextsJson: String = "[]", + @Column(columnDefinition = "JSONB", nullable = false) + val bodyTexts: JsonNode = JsonNodeFactory.instance.arrayNode() + @Column(columnDefinition = "JSONB", nullable = false) + val coreTexts: JsonNode = JsonNodeFactory.instance.arrayNode()domain/generator/src/main/kotlin/com/few/generator/service/strategy/BasicGenGenerationStrategy.kt (2)
17-21
: 안전하지 않은 null 단언 및 캐스팅 사용
material.title!!
,material.description!!
,material.coreTextsJson!!
와 같은!!
사용은 null이 들어올 경우 예외를 야기할 수 있으며,chatGpt.ask(...) as Headline
와 같은 직접 캐스팅은 타입 불일치 시ClassCastException
이 발생할 수 있습니다.아래 예시처럼 null 검증과 안전한 캐스팅을 적용해 보시는 것을 권장합니다:
val title = material.title ?: throw IllegalArgumentException("Title is required") val description = material.description ?: throw IllegalArgumentException("Description is required") val coreTextsJson = material.coreTextsJson ?: "[]" val headlineData = chatGpt.ask(headlinePrompt) val headline = headlineData as? Headline ?: throw IllegalStateException("Expected Headline but got ${headlineData::class.simpleName}")
17-21
: 예외 상황 처리 로직 부족
ChatGpt API 호출 실패나 응답 데이터 형식 오류에 대한 예외 처리가 없습니다.아래 예시처럼
try-catch
를 통해 API 호출 예외를 처리하는 방안을 고려해 보세요:val summaryResponse = try { chatGpt.ask(summaryPrompt) } catch (e: Exception) { throw IllegalStateException("Failed to generate summary: ${e.message}", e) }domain/generator/src/main/kotlin/com/few/generator/service/strategy/KoreanQuestionGenGenerationStrategy.kt (2)
25-39
: 안전하지 않은 null 단언 사용
material.title!!
,material.description!!
,material.headline!!
,material.summary!!
,material.coreTextsJson!!
사용 시 NPE가 발생할 수 있습니다.필요하다면 null 체크 후 적절한 예외를 던지도록 수정해 주세요.
30-40
: ChatGpt 응답에 대한 예외 처리 필요
ChatGpt 호출 실패, 응답 형식 오류 등에 대한 에러 처리가 고려되지 않았습니다.
try-catch
구문을 활용하여 호출 과정에서 발생할 수 있는 예외를 처리하고, 적절한 예외나 메시지를 반환하도록 개선하시길 권장드립니다.domain/generator/src/main/kotlin/com/few/generator/service/GenService.kt (2)
21-55
: 이미 언급된 위험:!!
연산자 사용
genGenerationStrategies
에서 전략 오브젝트를 가져올 때!!
연산자를 사용하면 해당 키가 존재하지 않을 경우 예외가 발생할 수 있습니다.
66-69
: 이미 언급된 저장 결과 검증 누락
saveAll()
결과를 즉시 반환하기 전에 예외 처리나 저장된 데이터의 유효성 검증이 필요합니다.domain/generator/src/main/kotlin/com/few/generator/service/ProvisioningService.kt (1)
45-46
: 이전 리뷰와 동일한 외부 서비스 예외 처리 미흡
chatGpt.ask(prompt)
자체가 실패할 경우 적절히 예외 처리가 되어 있지 않아 장애 대응이 어렵습니다.Also applies to: 55-56
rawContentsId = rawContents.id!!, | ||
completionIds = mutableListOf(bodyTexts.completionId!!, coreTexts.completionId!!), | ||
bodyTextsJson = gson.toJson(bodyTexts.texts), // TODO: DB 저장 타입 등 정의, 수정 필요 | ||
coreTextsJson = gson.toJson(bodyTexts.texts), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코어 텍스트 저장 로직 오류
bodyTexts.texts
가 coreTextsJson
에도 그대로 사용되어, coreTexts
내용이 반영되지 않습니다. 아래와 같은 수정이 필요해 보입니다.
- coreTextsJson = gson.toJson(bodyTexts.texts),
+ coreTextsJson = gson.toJson(coreTexts.texts),
📝 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.
coreTextsJson = gson.toJson(bodyTexts.texts), | |
coreTextsJson = gson.toJson(coreTexts.texts), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
domain/generator/src/main/kotlin/com/few/generator/service/ProvisioningService.kt (2)
33-33
: TODO 처리 계획이 필요합니다.
bodyTextsJson
와coreTextsJson
에 JSON 문자열을 저장하고 있으나, 추후 DB 컬럼 타입이나 매핑 로직(엔티티 구조)이 정돈되지 않으면 유지보수 시 혼동이 생길 수 있습니다. 할 일(TODO)을 조속히 해결하거나, 미리 구체적인 스펙을 확정하는 것이 좋겠습니다.
49-57
: 중복 로직을 통합하여 간소화할 수 있습니다.
makeBodyTexts
와makeCoreTexts
가 거의 동일한 패턴을 반복하고 있으므로, 공통 로직을 일반화하는 메서드로 추출하면 코드 중복을 줄이고 유지보수성을 높일 수 있습니다.예시 리팩터링 아이디어:
private fun makeBodyTexts(title: String, description: String, rawTexts: String): Texts { - val prompt = promptGenerator.toBodyTexts(title, description, rawTexts) - val texts: Texts = chatGpt.ask(prompt) as Texts - return texts + return askGpt { promptGenerator.toBodyTexts(title, description, rawTexts) } } private fun makeCoreTexts(title: String, description: String, bodyTexts: Texts): Texts { - val prompt = promptGenerator.toCoreTexts(title, description, bodyTexts) - val texts = chatGpt.ask(prompt) as Texts - return texts + return askGpt { promptGenerator.toCoreTexts(title, description, bodyTexts) } } +private fun askGpt(promptBuilder: () -> String): Texts { + val prompt = promptBuilder.invoke() + return chatGpt.ask(prompt) as Texts +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
domain/generator/src/main/kotlin/com/few/generator/service/ProvisioningService.kt
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: api-test
- GitHub Check: docs
- GitHub Check: lint
🔇 Additional comments (1)
domain/generator/src/main/kotlin/com/few/generator/service/ProvisioningService.kt (1)
25-37
: 외부 API 예외 처리가 필요합니다.
이 메서드 내부에서chatGpt.ask(...)
를 호출하면서, 예외 발생 시 적절한 복구나 사용자에게 의미 있는 오류를 반환하는 로직이 없습니다. 기존 리뷰에서도 같은 지적이 있었던 것으로 보이므로, 재발 방지를 위해 일정 범위의 예외를 처리하고 로깅 및 사용자 피드백 혹은 재시도를 고려해 보시기 바랍니다.
private fun makeBodyTexts( | ||
title: String, | ||
description: String, | ||
rawTexts: String, | ||
): Texts { | ||
val prompt = promptGenerator.toBodyTexts(title, description, rawTexts) | ||
val texts: Texts = chatGpt.ask(prompt) as Texts | ||
return texts | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
ChatGpt 결과를 Texts
로 다운캐스팅하는 부분을 점검하세요.
chatGpt.ask(...)
의 반환 타입이 항상 Texts
와 호환된다는 보장이 없다면, ClassCastException이 발생할 수 있습니다. 반환 타입이 동적으로 변경될 가능성, 또는 API 스펙 변동 시의 오류 가능성을 고려하여, 타입 안전성 검사나 에러 처리가 필요합니다.
🏁 Script executed:
#!/bin/bash
# 캐스팅이 안전한지 점검하기 위해 ProvisioningService 외부에서 chatGpt.ask 사용 예를 찾아봅니다.
rg -A 5 $'chatGpt.ask\\('
Length of output: 8848
주제: chatGpt.ask
의 다운캐스팅 안전성 강화 필요
현재 ProvisioningService.kt
의 makeBodyTexts
함수에서 chatGpt.ask(prompt) as Texts
와 같이 직접 다운캐스팅하고 있습니다.
- 반환된 객체가 항상
Texts
타입임이 보장되지 않을 경우, 런타임에ClassCastException
이 발생할 수 있습니다. - API 스펙 변경 또는 동적 타입 반환 가능성을 고려하여, 안전한 캐스팅(
as?
)과 함께 적절한 에러 처리 로직 또는 타입 검증 절차를 도입하는 것을 권장합니다. - 이와 동일한 다운캐스팅 패턴이 다른 파일(예:
KoreanQuestionGenGenerationStrategy.kt
,BasicGenGenerationStrategy.kt
등)에도 사용되고 있으므로, 전반적으로 검토 후 개선하는 것이 좋겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함께 리뷰도 했고 추가적으로 확인하면서 코멘트 남겼습니다.
현재 구현해주신 것도 좋은데 시간이 더 있으면 고려해보면 좋지 않을까? 하는 맘으로 작성해두었습니다!
고생하셨슴다!!
@Service | ||
class GenService( | ||
private val genRepository: GenRepository, | ||
private val genGenerationStrategies: Map<String, GenGenerationStrategy>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예전에 이렇게 인터페이스로 선언되고 구현체가 여러개인 빈을 구현한 경우를 본적 있는데
해당 프로젝트에서는 XXXManager과 같은 네이밍으로 해 구현체를 관리했는데 괜찮았던 것 같아요!
기억나서 코멘트로 남깁니다!
domain/generator/src/main/kotlin/com/few/generator/service/GenService.kt
Show resolved
Hide resolved
val rawContents = rawContentsService.create(sourceUrl) | ||
|
||
// 2. raw 데이터 기반 provisioning 생성 | ||
val provisioningContents = provisioningService.create(rawContents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프롬프트를 provisioningService
안에서 만들고 있는데 해당 클래스는 provisioning을 생성하는 역할만 할당하고 프롬포트를 UC에서 만들어서 주입시켜 주는 것은 어떨까요?
추가로 저는 프롬포트라는 것이 자주 바뀔 수 있는 것이라 생각이 들어 해당 UC에서도 요청으로 추가될 가능성도 있을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음.. 프롬프트가 여러개 사용되어서 어떻게 서비스로 전달해야할 지 고민해봐야겠습니다
val provisioningContents = provisioningService.create(rawContents) | ||
|
||
// 3. gen 생성 | ||
val gens = genService.create(rawContents, provisioningContents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서도 전략을 UC에서 선택하고 해당 UC에 맞는 Material을 생성하여 전달 할 것 같아여!
안에 구현을 보니 genBasic을 먼저 생성하고 그것을 기반으로 이후 과정을 진행하는 것으로 파악하였는데 UC에서 genBasic을 생성하고 나머지 gen들은 Material에 담아서 넘겨 순서대로 실행하도록 하고요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전 gen에서 생성된걸 다음 gen 생성시 사용할 경우 Material로 넘기고 있습니다
headline = genKoreanQuestion.headline, | ||
summary = genBasic.summary, | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STRATEGY_NAME_KOREAN_QUESTION와 STRATEGY_NAME_KOREAN_LONG_QUESTION는 이렇게 직접적으로 연관이 있으면 하나의 STRATEGY로 합치는 것도 고려해보면 좋을 것 같아요!
STRATEGY_NAME_KOREAN_QUESTION_AND_LONG_QUESTION과 같이요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
근데 엄밀히 다른 젠이라 로직이 같아도 이름은 분리되어야 함
Summary by CodeRabbitPR 본문에 위의 멘트가 중복되어서 삭제하였슴다! |
🎫 연관 이슈
resolved #453
💁♂️ PR 내용
🙏 작업
플로우
참고
🙈 PR 참고 사항
OPENAI_API_TOKEN
📸 스크린샷
저장된 데이터 세로가 너무 길어서 SQL로 첨부합니다
🚩 추가된 SQL 운영계 실행계획
🤖 테스트 체크리스트
Summary by CodeRabbit
Summary by CodeRabbit