[FEAT] 분석 요청 API#35
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthrough사용자가 GitHub 레포지토리의 보안 분석을 요청하는 REST API 엔드포인트를 구현하고, 분석 이력을 데이터베이스에 저장한 뒤 이벤트 기반으로 FastAPI 분석 서버에 비동기 요청을 전달하는 완전한 파이프라인입니다. 에러 코드 패키지 재구성, GitHub API 클라이언트 확장, 서비스 계층 구현, 비동기 처리 인프라를 포함합니다. Changes분석 요청 API 및 비동기 처리
Sequence DiagramsequenceDiagram
participant User
participant AnalysisRequestController
participant AnalysisRequestService
participant AnalysisRequestPersistenceService
participant ApplicationEventPublisher
participant AnalysisRequestedEventListener
participant AnalysisDispatchService
participant FastApiAnalysisClient
participant Database
User->>AnalysisRequestController: POST /api/analysis/request
AnalysisRequestController->>AnalysisRequestService: createAnalysisRequest
AnalysisRequestService->>AnalysisRequestService: validateRepository & validateBranch
AnalysisRequestService->>AnalysisRequestPersistenceService: saveAnalysisRequest
AnalysisRequestPersistenceService->>Database: save ProjectRepository
AnalysisRequestPersistenceService->>Database: save Analysis (PENDING)
AnalysisRequestPersistenceService->>ApplicationEventPublisher: publishEvent(AnalysisRequestedEvent)
AnalysisRequestPersistenceService-->>AnalysisRequestService: return AnalysisRequestCreateResponse
AnalysisRequestService-->>AnalysisRequestController: return ApiResponse
AnalysisRequestController-->>User: 201 Created
ApplicationEventPublisher->>AnalysisRequestedEventListener: async notify (AFTER_COMMIT)
AnalysisRequestedEventListener->>AnalysisDispatchService: markInProgress
AnalysisDispatchService->>Database: update Analysis status=IN_PROGRESS
AnalysisRequestedEventListener->>AnalysisDispatchService: dispatch
AnalysisDispatchService->>Database: findWithRepository
AnalysisDispatchService->>FastApiAnalysisClient: requestAnalysis(FastApiAnalysisRequest)
FastApiAnalysisClient->>FastApiAnalysisClient: POST /api/internal/analyze
alt Success
FastApiAnalysisClient-->>AnalysisDispatchService: response
else Failure
AnalysisDispatchService->>AnalysisDispatchService: markFailed(failureReason)
AnalysisDispatchService->>Database: update Analysis status=FAILED
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/main/java/SeCause/SeCause_be/domain/analysis/event/AnalysisRequestedEventListener.java (1)
20-36: 비동기 이벤트 처리 구조가 적절합니다.
@Async와@TransactionalEventListener(AFTER_COMMIT)조합으로 트랜잭션 커밋 이후 비동기 처리를 구현한 점이 좋습니다. 예외 처리도AnalysisException과 일반 예외를 구분하여 적절히 로깅하고 있습니다.참고:
markInProgress이후 애플리케이션이 중단되면 분석이IN_PROGRESS상태로 남을 수 있습니다. PR 설명에서 언급된 것처럼 향후 메시지 큐나 outbox 패턴 도입 시 이러한 edge case를 처리할 수 있습니다. 운영 환경에서는 장기간IN_PROGRESS상태인 분석을 모니터링하는 것을 권장합니다.🤖 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 `@src/main/java/SeCause/SeCause_be/domain/analysis/event/AnalysisRequestedEventListener.java` around lines 20 - 36, The current handle method in AnalysisRequestedEventListener calls analysisDispatchService.markInProgress(event.analysisId()) before dispatch and can leave analyses stuck in IN_PROGRESS if the app crashes; replace this fragile in-memory transition with a durable handoff: implement a transactional outbox or publish the dispatch intent to a message queue within the same transaction that created the analysis, and have the consumer perform analysisDispatchService.dispatch(event) (or call markInProgress at consumer start) so the state transition and dispatch are atomic; alternatively add a background reconciler that scans Analysis entities stuck in IN_PROGRESS beyond a threshold and calls analysisDispatchService.markFailed or retries dispatch, referencing handle, analysisDispatchService.markInProgress, analysisDispatchService.dispatch, and analysisDispatchService.markFailed to locate the affected logic.src/main/java/SeCause/SeCause_be/domain/analysis/service/AnalysisRequestService.java (1)
131-136: 💤 Low value브랜치 검증 결과가 사용되지 않습니다.
getRepositoryBranch호출로 브랜치 존재 여부를 검증하고 있지만, 반환된GithubBranchResponse객체를 사용하지 않습니다. 현재는 검증 목적으로만 사용되고 있어 문제는 없으나, 주석을 추가하여 의도를 명확히 하면 좋겠습니다.🔍 명확성 개선 제안
- githubRepositoryClient.getRepositoryBranch( + // 브랜치 존재 여부만 검증 (404 발생 시 AnalysisException throw) + githubRepositoryClient.getRepositoryBranch( githubToken, request.owner(), request.repositoryName(), request.branch() );🤖 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 `@src/main/java/SeCause/SeCause_be/domain/analysis/service/AnalysisRequestService.java` around lines 131 - 136, The call to githubRepositoryClient.getRepositoryBranch(...) in AnalysisRequestService verifies branch existence but the returned GithubBranchResponse is ignored; to make intent explicit either capture the return (e.g., GithubBranchResponse branch = githubRepositoryClient.getRepositoryBranch(...)) or, if you only need validation, add a clear comment above the call stating that the method is invoked solely for validation and any exception signals a missing/invalid branch—refer to getRepositoryBranch and GithubBranchResponse in your note so future readers understand the intentional discard of the result.src/main/java/SeCause/SeCause_be/domain/analysis/controller/AnalysisRequestApi.java (1)
374-456: ⚡ Quick win파라미터 명 변경은 호환성에 영향을 줄 수 있습니다.
owner/repository에서ownerName/repositoryName으로 경로 파라미터 명을 변경했습니다. 이는 기존 클라이언트에 영향을 주지 않는 변경이지만(경로 파라미터는 위치 기반), API 문서와 클라이언트 코드 가독성을 개선합니다.🤖 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 `@src/main/java/SeCause/SeCause_be/domain/analysis/controller/AnalysisRequestApi.java` around lines 374 - 456, You changed path parameter names from owner/repository to ownerName/repositoryName which can break consistency; update the controller and OpenAPI annotations so the path template, method signature and `@PathVariable` bindings all use the new names consistently: in the AnalysisRequestApi method getLinkableRepositoryBranches (and any other affected methods) ensure the `@RequestMapping/`@GetMapping path uses {ownerName} and {repositoryName}, the method parameters are annotated as `@PathVariable`("ownerName") and `@PathVariable`("repositoryName") (or `@PathVariable` with matching parameter names), and update the `@Parameter/`@Schema/@Example annotations to reflect ownerName/repositoryName so Swagger/OpenAPI and runtime binding remain aligned. Ensure any callers or generated clients are regenerated if needed.
🤖 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
`@src/main/java/SeCause/SeCause_be/domain/analysis/client/FastApiAnalysisClient.java`:
- Around line 25-31: FastApiAnalysisClient#requestAnalysis currently calls
webClient.post()...retrieve().toBodilessEntity().block(), which blocks without
explicit connect/overall request timeouts; update the call to either (a) remove
block() and return a reactive Mono/Response type so the caller can handle
timeouts/reactive scheduling, or (b) if synchronous behavior is required, add
explicit connection and overall timeouts by configuring the underlying
reactor-netty HttpClient (in WebClientConfig where
HttpClient.create().responseTimeout(Duration.ofSeconds(10)) is set) to include
.option(ChannelOption.CONNECT_TIMEOUT_MILLIS, ...) and a per-request timeout via
.doOnSubscribe/timeout(...) on the Mono before blocking; reference
FastApiAnalysisClient#requestAnalysis, WebClientConfig, and the existing
HttpClient.create().responseTimeout(...) when applying the change.
In
`@src/main/java/SeCause/SeCause_be/domain/analysis/controller/AnalysisRequestController.java`:
- Line 27: The class AnalysisRequestController has its base mapping changed to
"/api/analysis/request", which is a breaking change for clients; either revert
the `@RequestMapping` value in AnalysisRequestController back to the original
"/analysis/request" to preserve compatibility, or if the API path must change,
coordinate with the frontend and update all client calls and API docs
accordingly (ensure any controllers/routers that reference
AnalysisRequestController are updated to the new "/api/analysis/request" path
and communicate the change to the frontend team).
In
`@src/main/java/SeCause/SeCause_be/domain/analysis/dto/FastApiAnalysisRequest.java`:
- Around line 3-9: The FastApiAnalysisRequest record exposes a sensitive
githubToken field; remove the githubToken component from the
FastApiAnalysisRequest record signature and from all code that
constructs/serializes/deserializes it (consumers, mappers, tests, HTTP clients)
and update request producers to follow the agreed FastAPI contract fields
(analysis_id, repository_id, repository_link/repository_url, branch); implement
server-to-server authentication via internal auth headers or a separate auth
flow instead of embedding a GitHub token in the request body so no code paths
depend on FastApiAnalysisRequest.githubToken anymore.
In
`@src/main/java/SeCause/SeCause_be/domain/analysis/service/AnalysisRequestService.java`:
- Around line 149-155: The event currently embeds a sensitive githubToken when
publishing AnalysisRequestedEvent from AnalysisRequestService (the publishEvent
call passing analysis.getAnalysisId(), repository.getRepositoryId(),
githubRepository.cloneUrl(), request.branch(), githubToken); remove githubToken
from the event payload and update AnalysisRequestedEvent to carry only
non-sensitive identifiers (e.g., analysisId and userId or repositoryId). Update
the event publisher call in AnalysisRequestService to omit githubToken and
include the chosen user identifier (e.g., analysis.getAnalysisId() and userId).
Adjust the listener (e.g., handleAnalysisRequested) to, after commit, fetch the
GitHub token via UserRepository (or via analysisId -> User lookup) and proceed
using the retrieved token. Ensure constructors/signatures for
AnalysisRequestedEvent and any consumers are updated accordingly and that no
other code paths continue to pass or log the token.
- Around line 117-158: The createAnalysisRequest method lacks a
duplicate-request check so multiple IN_PROGRESS analyses can be created for the
same repository; add a lookup on AnalysisRepository (e.g.
existsByRepository_CloneUrlAndStatusAndDeletedFalse) using
githubRepository.cloneUrl and AnalysisStatus.IN_PROGRESS before saving the
ProjectRepository/Analysis and if it returns true, fail the request (throw an
appropriate domain exception or return a validation error) to prevent creating a
new analysis; place this check at the start of createAnalysisRequest right after
obtaining githubRepository (use
AnalysisRepository.existsByRepository_CloneUrlAndStatusAndDeletedFalse and
AnalysisStatus.IN_PROGRESS) so the eventPublisher and saves never run for
duplicates.
In
`@src/main/java/SeCause/SeCause_be/global/apiPayload/code/GlobalSuccessCode.java`:
- Line 11: The change to the GlobalSuccessCode enum altered the OK constant to
use "COMMON200", which will change the `code` returned by all
ApiResponse.onSuccess(...) calls and break existing API contract (previously
"COMMON2000"); restore the original value for the OK enum constant in
GlobalSuccessCode (revert the literal back to "COMMON2000") or, if the new code
is intentional, update all related artifacts (Swagger examples, frontend
parsers, and any tests) to expect "COMMON200"—locate the OK enum in
GlobalSuccessCode and revert or coordinate the contract updates accordingly.
In `@src/main/java/SeCause/SeCause_be/global/config/AsyncConfig.java`:
- Around line 6-9: AsyncConfig currently only enables async processing and
relies on defaults; explicitly define and register a ThreadPoolTaskExecutor bean
(e.g., bean name analysisTaskExecutor) or implement AsyncConfigurer in
AsyncConfig to provide a customized Executor, then update
AnalysisRequestedEventListener#handle to reference that executor via
`@Async`("analysisTaskExecutor") (or ensure AsyncConfigurer returns the desired
executor) so thread pool size, queue capacity, and rejection policy are
explicitly controlled.
In `@src/main/resources/application-dev.yml`:
- Line 39: The placeholder key used in application-dev.yml
(fast-api.analysis-request-url: ${FASTAPI_ANALYSIS_REQUEST}) is inconsistent
with the expected environment variable name referenced in docs/deploy
(FASTAPI_ANALYSIS_REQUEST_URL) and may cause unresolved placeholders at startup;
update either the YAML to use ${FASTAPI_ANALYSIS_REQUEST_URL} or harmonize the
deployment/docs to use FASTAPI_ANALYSIS_REQUEST, and ensure the injected Spring
property in FastApiAnalysisClient (`@Value`("${fast-api.analysis-request-url}"))
remains aligned with that environment variable name so the value is resolved at
runtime.
---
Nitpick comments:
In
`@src/main/java/SeCause/SeCause_be/domain/analysis/controller/AnalysisRequestApi.java`:
- Around line 374-456: You changed path parameter names from owner/repository to
ownerName/repositoryName which can break consistency; update the controller and
OpenAPI annotations so the path template, method signature and `@PathVariable`
bindings all use the new names consistently: in the AnalysisRequestApi method
getLinkableRepositoryBranches (and any other affected methods) ensure the
`@RequestMapping/`@GetMapping path uses {ownerName} and {repositoryName}, the
method parameters are annotated as `@PathVariable`("ownerName") and
`@PathVariable`("repositoryName") (or `@PathVariable` with matching parameter
names), and update the `@Parameter/`@Schema/@Example annotations to reflect
ownerName/repositoryName so Swagger/OpenAPI and runtime binding remain aligned.
Ensure any callers or generated clients are regenerated if needed.
In
`@src/main/java/SeCause/SeCause_be/domain/analysis/event/AnalysisRequestedEventListener.java`:
- Around line 20-36: The current handle method in AnalysisRequestedEventListener
calls analysisDispatchService.markInProgress(event.analysisId()) before dispatch
and can leave analyses stuck in IN_PROGRESS if the app crashes; replace this
fragile in-memory transition with a durable handoff: implement a transactional
outbox or publish the dispatch intent to a message queue within the same
transaction that created the analysis, and have the consumer perform
analysisDispatchService.dispatch(event) (or call markInProgress at consumer
start) so the state transition and dispatch are atomic; alternatively add a
background reconciler that scans Analysis entities stuck in IN_PROGRESS beyond a
threshold and calls analysisDispatchService.markFailed or retries dispatch,
referencing handle, analysisDispatchService.markInProgress,
analysisDispatchService.dispatch, and analysisDispatchService.markFailed to
locate the affected logic.
In
`@src/main/java/SeCause/SeCause_be/domain/analysis/service/AnalysisRequestService.java`:
- Around line 131-136: The call to
githubRepositoryClient.getRepositoryBranch(...) in AnalysisRequestService
verifies branch existence but the returned GithubBranchResponse is ignored; to
make intent explicit either capture the return (e.g., GithubBranchResponse
branch = githubRepositoryClient.getRepositoryBranch(...)) or, if you only need
validation, add a clear comment above the call stating that the method is
invoked solely for validation and any exception signals a missing/invalid
branch—refer to getRepositoryBranch and GithubBranchResponse in your note so
future readers understand the intentional discard of the result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 690e24d6-059c-412d-9ca1-59f05045817e
📒 Files selected for processing (25)
src/main/java/SeCause/SeCause_be/domain/analysis/client/FastApiAnalysisClient.javasrc/main/java/SeCause/SeCause_be/domain/analysis/client/GithubRepositoryClient.javasrc/main/java/SeCause/SeCause_be/domain/analysis/controller/AnalysisRequestApi.javasrc/main/java/SeCause/SeCause_be/domain/analysis/controller/AnalysisRequestController.javasrc/main/java/SeCause/SeCause_be/domain/analysis/dto/AnalysisRequestCreateRequest.javasrc/main/java/SeCause/SeCause_be/domain/analysis/dto/AnalysisRequestCreateResponse.javasrc/main/java/SeCause/SeCause_be/domain/analysis/dto/FastApiAnalysisRequest.javasrc/main/java/SeCause/SeCause_be/domain/analysis/dto/GithubRepositoryResponse.javasrc/main/java/SeCause/SeCause_be/domain/analysis/dto/LinkableRepositoryResponse.javasrc/main/java/SeCause/SeCause_be/domain/analysis/event/AnalysisRequestedEvent.javasrc/main/java/SeCause/SeCause_be/domain/analysis/event/AnalysisRequestedEventListener.javasrc/main/java/SeCause/SeCause_be/domain/analysis/exception/AnalysisException.javasrc/main/java/SeCause/SeCause_be/domain/analysis/exception/code/AnalysisErrorCode.javasrc/main/java/SeCause/SeCause_be/domain/analysis/repository/AnalysisRepository.javasrc/main/java/SeCause/SeCause_be/domain/analysis/service/AnalysisDispatchService.javasrc/main/java/SeCause/SeCause_be/domain/analysis/service/AnalysisRequestService.javasrc/main/java/SeCause/SeCause_be/domain/analysis/validator/AnalysisRequestValidator.javasrc/main/java/SeCause/SeCause_be/domain/projectRepository/exception/ProjectRepositoryException.javasrc/main/java/SeCause/SeCause_be/domain/projectRepository/exception/code/ProjectRepositoryErrorCode.javasrc/main/java/SeCause/SeCause_be/domain/projectRepository/service/RepositoryIssueService.javasrc/main/java/SeCause/SeCause_be/domain/projectRepository/validator/ProjectRepositoryValidator.javasrc/main/java/SeCause/SeCause_be/global/apiPayload/code/GlobalSuccessCode.javasrc/main/java/SeCause/SeCause_be/global/config/AsyncConfig.javasrc/main/resources/application-dev.ymlsrc/main/resources/application-local.yml
| ); | ||
| } | ||
|
|
||
| public GithubBranchResponse getRepositoryBranch( |
There was a problem hiding this comment.
현재 명시된 반환 타입을 호출부에서는 사용하고 있지 않네요. 브랜치 존재 여부 검증이 목적이라면 validateRepositoryBranchExists처럼 의도가 드러나는 메서드명으로 변경하면 좋을 것 같습니다!
| } catch (WebClientResponseException exception) { | ||
| throw new AnalysisException(AnalysisErrorCode.GITHUB_API_REQUEST_FAILED); | ||
| } catch (WebClientException exception) { | ||
| throw new AnalysisException(AnalysisErrorCode.GITHUB_API_REQUEST_FAILED); | ||
| } |
There was a problem hiding this comment.
WebClientResponseException은 WebClientException의 하위 타입이므로 앞에서 처리한 401, 403, 404를 제외한 나머지는 마지막 하나로 합칠 수 있을 것 같아요!
| //비동기 처리 (추후 대기 큐 도입 고려중) | ||
| eventPublisher.publishEvent(new AnalysisRequestedEvent( |
There was a problem hiding this comment.
현재 Spring 내부 이벤트는 메모리에만 존재하므로, DB 커밋 후 리스너 실행 전에 서버가 종료된다면 이벤트가 유실되어 분석이 PENDING 상태로 남을 수 있습니다. 따라서 서버 재시작 후 사용자에게는 계속 대기 중인 요청으로 노출될 수 있을 것 같네요!
적어두신 것 처럼 만약 메시지 큐를 도입한다면 DB 저장과 메시지 발행 사이의 불일치를 방지 할 수 있도록 하고, 그전에는 과거 PENDING 요청을 재처리하는 보완 로직을 고려하면 좋겠습니다!
There was a problem hiding this comment.
해당 부분 관련해서는, 큐 로직이 추가됨에 따라 변동될 거 같아서 우선 추후 이슈로 작성해두겠습니다!
| @Transactional | ||
| public AnalysisRequestCreateResponse createAnalysisRequest( |
There was a problem hiding this comment.
현재 트랜잭션 내부에서 GitHub API를 호출하고 있어 외부 API 응답이 지연되면 트랜잭션도 함께 길어집니다. GitHub 검증을 먼저 수행하고, Repository와 Analysis 저장 구간만 별도 트랜잭션으로 묶는 방식을 고려하면 좋겠습니다!
| String githubToken = analysisRequestValidator.validateGithubToken(user.getGithubToken()); | ||
|
|
||
| //분석 요청을 위한 github repository, branch 정보 가져오기 | ||
| GithubRepositoryResponse githubRepository = githubRepositoryClient.getRepository( |
There was a problem hiding this comment.
저장소 목록 API에서는 본인 또는 소속 조직의 저장소만 제공하지만, 분석 생성 API에서는 조회 가능한 공개 저장소도 요청할 수 있는 것으로 보입니다.
만약 요구사항이 공개된 저장소 전체를 분석 대상으로 허용 것이라면 괜찮지만, 아니라면 검토 후 사용자 소유 저장소로 제한하기 위해 owner 검증이 추가로 필요할 것 같습니다!
| } | ||
| } | ||
|
|
||
| private <T> List<T> getList( |
There was a problem hiding this comment.
현재 GitHub 목록 API를 per_page=100으로 한 번만 호출하고 있어 100개를 초과하는 데이터는 누락될 수 있습니다. Link 헤더를 이용해 다음 페이지를 조회하거나 페이지네이션을 제공하는 방식이 필요해 보입니다!
넵~ fork방식이라 fork뜬 레포의 develop 브랜치에서 작업했습니당 |
close #31
🔎 개요
프론트에서 선택한 GitHub 레포지토리와 브랜치를 기반으로 분석 요청을 생성하고, FastAPI 분석 서버에 비동기로 요청을 전달하는 API를 구현했습니다.
📝 작업 내용
POST /analysis/request분석 요청 생성 API 구현ownerrepositoryNamebranchrepositories테이블에 분석 대상 레포지토리 정보 저장analyses테이블에 분석 요청 상태PENDING으로 저장IN_PROGRESS로 변경FAILED로 변경githubUrl추가👀 변경 사항
repositoryId를 받지 않습니다.{ "owner": "SeCause", "repositoryName": "SeCause-BE", "branch": "develop" }📸 스크린샷 (Optional)
✅ 체크리스트
💬 고민사항 및 리뷰 요구사항 (Optional)
Summary by CodeRabbit
릴리스 노트
New Features
/api/analysis/request).Improvements