Refactor/file delete - #180
Merged
Merged
Conversation
fix : docker-compose.prod.yml admin-bff에 FASTAPI_SERVICE_URL 추가(#175)
- DocumentRepository: findByStatusAndCreatedAtBefore 쿼리 추가 - DocumentService: getPendingDocumentsOlderThan(int minutes) 메서드 추가
- GuideonCoreApplication: @EnableScheduling 추가 - DocumentGlobalInternalController: GET /internal/v1/documents/pending 엔드포인트 추가 - DocumentTimeoutScheduler: PENDING 30분 초과 문서 FAILED 자동 전환 (5분 주기)
- GuideonBackendApplication: @EnableScheduling 추가 - CoreDocumentClient: getPendingDocuments Feign 메서드 추가 - DocumentRetryScheduler: PENDING 5분 초과 문서 FastAPI 자동 재처리 요청 (5분 주기)
|
Warning Review limit reached
More reviews will be available in 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
개요이 PR은 대기 중인 문서에 대한 자동 재처리 및 타임아웃 스케줄링 기능을 추가합니다. Core 서비스는 대기 문서 조회 API와 타임아웃 스케줄러를 제공하며, Admin-BFF는 이 API를 호출하는 재시도 스케줄러를 구현합니다. 변경사항문서 재시도 및 타임아웃 스케줄링 프레임워크
예상 코드 리뷰 난이도🎯 3 (중간 난이도) | ⏱️ ~25분 관련 이슈
축하 시
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
guideon-admin-bff/src/main/java/com/guideon/guideonbackend/GuideonBackendApplication.java (1)
14-18:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
@EnableScheduling+com.guideon.core.domain스캔으로 core 스케줄러가 여기서도 실행됩니다.
scanBasePackages에com.guideon.core.domain이 포함되어 있어, 해당 패키지 하위의DocumentTimeoutScheduler(com.guideon.core.domain.document.scheduler)가 admin-bff 컨텍스트에도 빈으로 등록됩니다. 여기에@EnableScheduling까지 더해지면 타임아웃 스케줄러가 core와 admin-bff 양쪽에서 중복 실행됩니다. 자세한 내용은DocumentTimeoutScheduler코멘트를 참고하세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@guideon-admin-bff/src/main/java/com/guideon/guideonbackend/GuideonBackendApplication.java` around lines 14 - 18, The application scans com.guideon.core.domain while also enabling scheduling, which causes DocumentTimeoutScheduler (com.guideon.core.domain.document.scheduler.DocumentTimeoutScheduler) to be registered and run twice; to fix, stop scanning that package or explicitly exclude the scheduler: in GuideonBackendApplication remove "com.guideon.core.domain" from the scanBasePackages array or add a ComponentScan excludeFilters entry (or use `@ComponentScan`(basePackages = {...}, excludeFilters = `@Filter`(type = FilterType.ASSIGNABLE_TYPE, classes = DocumentTimeoutScheduler.class))) so the DocumentTimeoutScheduler is not created in this context while keeping `@EnableScheduling` for the admin-bff only.
🧹 Nitpick comments (1)
guideon-core/src/main/java/com/guideon/core/service/DocumentService.java (1)
115-121: ⚡ Quick win
DocumentDto.from에서SiteLAZY 로딩으로 N+1 쿼리 가능성이 있습니다.
DocumentDto.from이doc.getSite().getSiteId()를 호출하고,Document.site가@ManyToOne(fetch = FetchType.LAZY)라서findByStatusAndCreatedAtBefore로 가져온 문서 개수만큼 추가 SELECT가 발생할 수 있습니다.JOIN FETCH/@EntityGraph로Site를 함께 가져오는 쪽으로 개선 권장./internal/v1/documents/pending의olderThanMinutes는defaultValue="5"만 있고 음수 입력 검증이 없어(예:@Min없음) 음수 전달 시 cutoff이 미래로 계산되어 조회 범위가 과도해질 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@guideon-core/src/main/java/com/guideon/core/service/DocumentService.java` around lines 115 - 121, The current getPendingDocumentsOlderThan uses documentRepository.findByStatusAndCreatedAtBefore and then DocumentDto.from which calls doc.getSite().getSiteId(), causing N+1 due to Document.site being LAZY; change the repository call to fetch Site eagerly (e.g., add a repository method annotated with `@EntityGraph`(attributePaths = "site") or a JPQL query with JOIN FETCH, e.g., findWithSiteByStatusAndCreatedAtBefore) and call that from DocumentService.getPendingDocumentsOlderThan so DocumentDto.from won't trigger extra selects; additionally validate the olderThanMinutes input on the /internal/v1/documents/pending endpoint (add `@Min`(0) or explicit negative check on the controller method parameter) to prevent a negative value producing a future cutoff.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose.yml`:
- Line 79: The docker-compose sets FILE_BASE_URL to an internal container host
(http://admin-bff:8081) which LocalFileStorageService uses as the public file
URL (via file.base-url in application.yml), causing external clients to receive
unreachable links; update the local docker-compose.yml to set FILE_BASE_URL to
an externally reachable address (e.g., http://localhost:8081) or add an env
branching mechanism so application.yml’s file.base-url resolves to a public host
for local runs, ensuring FileDownloadController-served paths remain accessible
and SecurityConfig’s permitAll endpoints produce usable URLs.
In
`@guideon-admin-bff/src/main/java/com/guideon/guideonbackend/domain/document/scheduler/DocumentRetryScheduler.java`:
- Around line 28-50: The scheduler currently fetches PENDING docs and calls
reprocessDocument but because Document.reprocess resets fields without changing
createdAt, the same doc is repeatedly picked up; modify retryPendingDocuments to
atomically mark a document as PROCESSING (or RETRYING) before calling
coreDocumentClient.reprocessDocument (e.g., via a new
coreDocumentClient.markProcessing(siteId, docId) or a single
coreDocumentClient.reserveAndReprocess(siteId, docId) endpoint) and ensure the
core side updates createdAt/lastRetryAt or attemptCount so
getPendingDocuments(retryAfterMinutes) excludes recently retried items;
alternatively add/consume lastRetryAt/attemptCount fields in getPendingDocuments
filtering to throttle retries. Ensure changes touch
DocumentRetryScheduler.retryPendingDocuments, coreDocumentClient.* (new
mark/reserve API or reprocess semantics), and server-side Document.reprocess /
FastApiDocumentService.processDocument to perform atomic state transition to
PROCESSING/RETRYING.
In
`@guideon-core/src/main/java/com/guideon/core/api/DocumentGlobalInternalController.java`:
- Around line 22-26: Add parameter validation to prevent non-positive minutes:
annotate the controller class DocumentGlobalInternalController with `@Validated`
and add a javax.validation constraint on the getPendingDocuments method
parameter (e.g., `@Min`(1) or `@Positive` on the olderThanMinutes `@RequestParam`) so
invalid values are rejected before calling
documentService.getPendingDocumentsOlderThan; add the required import for the
chosen constraint and ensure javax validation is on the class via `@Validated`.
In
`@guideon-core/src/main/java/com/guideon/core/domain/document/scheduler/DocumentTimeoutScheduler.java`:
- Around line 1-44: The DocumentTimeoutScheduler is being registered in both
core and admin-bff causing duplicate scheduling; restrict its activation by
annotating the DocumentTimeoutScheduler class with a conditional (e.g., Spring's
`@ConditionalOnProperty` or `@Profile`) so it only runs when a property like
document.enable-timeout-scheduler=true is set (choose matchIfMissing=true or
false per desired default), and update the deployments: enable it in
GuideonCoreApplication and disable it in GuideonBackendApplication (or set the
appropriate profile) so only one app executes failTimedOutDocuments(); refer to
the class DocumentTimeoutScheduler, its failTimedOutDocuments() method, and the
new property document.enable-timeout-scheduler when applying the change and
documenting the required configuration.
---
Outside diff comments:
In
`@guideon-admin-bff/src/main/java/com/guideon/guideonbackend/GuideonBackendApplication.java`:
- Around line 14-18: The application scans com.guideon.core.domain while also
enabling scheduling, which causes DocumentTimeoutScheduler
(com.guideon.core.domain.document.scheduler.DocumentTimeoutScheduler) to be
registered and run twice; to fix, stop scanning that package or explicitly
exclude the scheduler: in GuideonBackendApplication remove
"com.guideon.core.domain" from the scanBasePackages array or add a ComponentScan
excludeFilters entry (or use `@ComponentScan`(basePackages = {...}, excludeFilters
= `@Filter`(type = FilterType.ASSIGNABLE_TYPE, classes =
DocumentTimeoutScheduler.class))) so the DocumentTimeoutScheduler is not created
in this context while keeping `@EnableScheduling` for the admin-bff only.
---
Nitpick comments:
In `@guideon-core/src/main/java/com/guideon/core/service/DocumentService.java`:
- Around line 115-121: The current getPendingDocumentsOlderThan uses
documentRepository.findByStatusAndCreatedAtBefore and then DocumentDto.from
which calls doc.getSite().getSiteId(), causing N+1 due to Document.site being
LAZY; change the repository call to fetch Site eagerly (e.g., add a repository
method annotated with `@EntityGraph`(attributePaths = "site") or a JPQL query with
JOIN FETCH, e.g., findWithSiteByStatusAndCreatedAtBefore) and call that from
DocumentService.getPendingDocumentsOlderThan so DocumentDto.from won't trigger
extra selects; additionally validate the olderThanMinutes input on the
/internal/v1/documents/pending endpoint (add `@Min`(0) or explicit negative check
on the controller method parameter) to prevent a negative value producing a
future cutoff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e9199d8-baf4-49e2-be4e-5ed2f755e3bf
📒 Files selected for processing (10)
docker-compose.ymlguideon-admin-bff/src/main/java/com/guideon/guideonbackend/GuideonBackendApplication.javaguideon-admin-bff/src/main/java/com/guideon/guideonbackend/client/CoreDocumentClient.javaguideon-admin-bff/src/main/java/com/guideon/guideonbackend/domain/document/scheduler/DocumentRetryScheduler.javaguideon-core/src/main/java/com/guideon/core/GuideonCoreApplication.javaguideon-core/src/main/java/com/guideon/core/api/DocumentGlobalInternalController.javaguideon-core/src/main/java/com/guideon/core/api/DocumentInternalController.javaguideon-core/src/main/java/com/guideon/core/domain/document/repository/DocumentRepository.javaguideon-core/src/main/java/com/guideon/core/domain/document/scheduler/DocumentTimeoutScheduler.javaguideon-core/src/main/java/com/guideon/core/service/DocumentService.java
| SPRING_DATA_REDIS_PORT: 6379 | ||
| CORE_SERVICE_URL: http://core:8080 | ||
| FASTAPI_SERVICE_URL: http://fastapi:8000 | ||
| FILE_BASE_URL: http://admin-bff:8081 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: FILE_BASE_URL 환경변수 사용처 확인
# admin-bff 코드베이스에서 FILE_BASE_URL 참조 검색
rg -n --type=java -C3 'FILE_BASE_URL' guideon-admin-bff/
# application.yml 또는 properties 파일에서 설정 확인
fd -e yml -e yaml -e properties . guideon-admin-bff/ --exec rg -n 'file.*base.*url' {} \;Repository: Project-Guideon/guideon-backend
Length of output: 57
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show docker-compose.yml around the referenced line
echo "== docker-compose.yml: around line 70-90 =="
nl -ba docker-compose.yml | sed -n '70,95p'
# 2) Repo-wide search for exact key
echo
echo "== rg: FILE_BASE_URL (repo-wide) =="
rg -n --hidden --no-ignore -S 'FILE_BASE_URL' . || true
# 3) Repo-wide search for likely config/property names (case-insensitive)
echo
echo "== rg: file.*base.*url (repo-wide, case-insensitive) =="
rg -n --hidden --no-ignore -i -S 'file.*base.*url' . || true
echo
echo "== rg: file-base-url (repo-wide, case-insensitive) =="
rg -n --hidden --no-ignore -i -S 'file[-_]?base[-_]?url' . || true
echo
echo "== rg: fileBaseUrl (repo-wide) =="
rg -n --hidden --no-ignore -S 'fileBaseUrl' . || trueRepository: Project-Guideon/guideon-backend
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-compose.yml: around line 70-90 =="
# show with line numbers using awk (avoid nl)
awk 'NR>=70 && NR<=95 {printf "%4d:%s\n", NR, $0}' docker-compose.yml
echo
echo "== rg: FILE_BASE_URL (repo-wide, exact key) =="
rg -n --hidden --no-ignore -S 'FILE_BASE_URL' . || true
echo
echo "== rg: file.*base.*url (repo-wide, case-insensitive) =="
rg -n --hidden --no-ignore -i -S 'file.*base.*url' . || true
echo
echo "== rg: file-base-url (repo-wide, case-insensitive) =="
rg -n --hidden --no-ignore -i -S 'file[-_]?base[-_]?url' . || true
echo
echo "== rg: fileBaseUrl (repo-wide, exact) =="
rg -n --hidden --no-ignore -S 'fileBaseUrl' . || trueRepository: Project-Guideon/guideon-backend
Length of output: 4639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== application.yml (guideon-admin-bff) around line 80-120 =="
sed -n '80,120p' guideon-admin-bff/src/main/resources/application.yml
echo
echo "== LocalFileStorageService.java around lines 1-120 =="
sed -n '1,120p' guideon-admin-bff/src/main/java/com/guideon/guideonbackend/global/storage/LocalFileStorageService.javaRepository: Project-Guideon/guideon-backend
Length of output: 6012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore -S 'internal/files' guideon-admin-bff/src/main/java || true
rg -n --hidden --no-ignore -S '"/internal/files' guideon-admin-bff/src/main/java || true
rg -n --hidden --no-ignore -S 'internalFiles' guideon-admin-bff/src/main/java || trueRepository: Project-Guideon/guideon-backend
Length of output: 1918
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== FileDownloadController.java (around mapping) =="
sed -n '1,140p' guideon-admin-bff/src/main/java/com/guideon/guideonbackend/global/storage/FileDownloadController.java
echo
echo "== SecurityConfig.java (around /internal/files/** allowlist) =="
sed -n '1,120p' guideon-admin-bff/src/main/java/com/guideon/guideonbackend/global/config/SecurityConfig.java
echo
echo "== deploy/docker-compose.prod.yml (around FILE_BASE_URL) =="
sed -n '60,110p' deploy/docker-compose.prod.ymlRepository: Project-Guideon/guideon-backend
Length of output: 11625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rg: internal/files/ (repo-wide) =="
rg -n --hidden --no-ignore -S 'internal/files/' . || true
echo
echo "== rg: storageUrl (repo-wide) =="
rg -n --hidden --no-ignore -S 'storageUrl' . || trueRepository: Project-Guideon/guideon-backend
Length of output: 6769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DocumentService.java (around where it sets storageUrl) =="
sed -n '1,220p' guideon-admin-bff/src/main/java/com/guideon/guideonbackend/domain/document/service/DocumentService.java
echo
echo "== MascotService.java (around where it sets imageUrl/storageUrl) =="
sed -n '1,180p' guideon-admin-bff/src/main/java/com/guideon/guideonbackend/domain/mascot/service/MascotService.java
echo
echo "== PlaceService.java (around where it sets imageUrl/storageUrl) =="
sed -n '110,220p' guideon-admin-bff/src/main/java/com/guideon/guideonbackend/domain/place/service/PlaceService.java
echo
echo "== rg FASTAPI_SERVICE_URL usage (admin-bff) =="
rg -n --hidden --no-ignore -S 'FASTAPI_SERVICE_URL' guideon-admin-bff/src/main/java || true
echo
echo "== rg calls to FastAPI base URL or /pdf /generate (admin-bff) =="
rg -n --hidden --no-ignore -S 'fastapi' guideon-admin-bff/src/main/java || trueRepository: Project-Guideon/guideon-backend
Length of output: 20994
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-compose.yml: admin-bff service (ports + environment) =="
# show admin-bff block (best-effort by printing a range around the first occurrence)
rg -n --hidden --no-ignore -S '^ admin-bff:' docker-compose.yml
START=$(rg -n --hidden --no-ignore -S '^ admin-bff:' docker-compose.yml | head -n1 | cut -d: -f1)
# show 80 lines starting from admin-bff start
sed -n "$((START)),$((START+120))p" docker-compose.yml
echo
echo "== deploy/nginx.conf.template (location /internal/files) =="
sed -n '1,120p' deploy/nginx.conf.template
sed -n '35,80p' deploy/nginx.conf.templateRepository: Project-Guideon/guideon-backend
Length of output: 7014
FILE_BASE_URL가 실제 파일 다운로드 URL 생성에 사용되므로 로컬 값(admin-bff)이 외부 클라이언트에선 깨질 수 있습니다.
guideon-admin-bff/src/main/resources/application.yml의file.base-url: ${FILE_BASE_URL:...}값이LocalFileStorageService에서/internal/files/{siteId}/{filename}URL 호스트로 그대로 사용됩니다./internal/files/**는FileDownloadController로 제공되고SecurityConfig에서permitAll이라 생성된 URL이 그대로 노출/소비됩니다.- 프로덕션은
deploy/docker-compose.prod.yml에서FILE_BASE_URL=https://${DOMAIN}로 외부 접근 가능한 값이라 문제 소지가 낮습니다. - 하지만 로컬
docker-compose.yml(79행)FILE_BASE_URL: http://admin-bff:8081는 컨테이너 내부 DNS라 브라우저/외부 클라이언트에서 해석되지 않으면 이미지/다운로드가 실패할 수 있습니다.
로컬 실행 시 FILE_BASE_URL을 http://localhost:8081처럼 외부에서 접근 가능한 주소로 변경(또는 env 분기) 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose.yml` at line 79, The docker-compose sets FILE_BASE_URL to an
internal container host (http://admin-bff:8081) which LocalFileStorageService
uses as the public file URL (via file.base-url in application.yml), causing
external clients to receive unreachable links; update the local
docker-compose.yml to set FILE_BASE_URL to an externally reachable address
(e.g., http://localhost:8081) or add an env branching mechanism so
application.yml’s file.base-url resolves to a public host for local runs,
ensuring FileDownloadController-served paths remain accessible and
SecurityConfig’s permitAll endpoints produce usable URLs.
Contributor
Author
There was a problem hiding this comment.
현재 docker-compose.yml은 이미 https://${DOMAIN} 으로 되어 있어 pull 후 해결된 상태. application.yml 기본값 http://localhost:8081은 로컬 개발용이라 문제없을 것 같습니다.
There was a problem hiding this comment.
@hwangtae123, 설명 감사합니다. 정리하면:
- 프로덕션 (
deploy/docker-compose.prod.yml):FILE_BASE_URL=https://${DOMAIN}— 외부 접근 가능, 문제없음 ✅ application.yml기본값 (http://localhost:8081): 순수 로컬 개발(IDE에서 직접 실행)용으로 적절 ✅
한 가지만 참고로 남기면, 로컬 Docker Compose 환경 (현재 docker-compose.yml의 http://admin-bff:8081)에서는 컨테이너 내부 DNS를 사용하므로, 브라우저/외부 클라이언트에서 생성된 파일 URL이 깨질 수 있습니다. 로컬 Compose 실행 시에는 http://localhost:8081로 변경하거나 .env로 분기하는 것을 추후 고려해 보시면 좋을 것 같습니다. 현재 요구사항 범위에서는 프로덕션이 이미 올바르게 설정되어 있으므로 큰 문제는 없어 보입니다.
(∪・ω・∪)
- reprocess() 상태를 PENDING → PROCESSING으로 변경하여 재처리 스케줄러의 중복 요청 방지 - DocumentTimeoutScheduler에 @ConditionalOnProperty 추가로 admin-bff 중복 실행 방지 - admin-bff application.yml에 timeout-scheduler 비활성화 설정 추가 - DocumentGlobalInternalController olderThanMinutes 파라미터에 @positive 검증 추가
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tasks
To Reviewer
Screenshot
Summary by CodeRabbit
주요 변경사항