Conversation
WalkthroughRepositoryFacadeService.save(...) now fetches repo info first, checks for an existing repository by externalId per user, and only registers a webhook when creating a new repository via a new registerNewRepo(...) method. Tests updated to allow cross-user duplicate saves and to stop asserting webhook on re-tracking. Added @OverRide to getCharge in ClaudeAiModel and OpenAiModel. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant F as RepositoryFacadeService
participant G as GitHubService
participant R as RepositoryService
participant W as WebHookService
U->>F: save(repoUrl, user)
F->>G: getRepositoryInfo(repoUrl, user.githubToken)
G-->>F: GithubRepoInfoResponse
F->>R: findByUserAndExternalId(user, repoInfo.id)
alt repository exists
F->>R: reTrackingOrThrowException(user, existingRepo)
R-->>F: Repository (tracking ensured or exception)
F-->>U: Repository
else repository not found
F->>R: save(RepositoryCreateCommand(...))
R-->>F: Repository (saved)
F->>W: registerWebhook(user, savedRepository.id)
W-->>F: void
F-->>U: Repository
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
📝 Test Coverage Report
|
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 84745bd. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
gss-domain/src/main/java/com/devoops/domain/entity/analysis/ClaudeAiModel.java (1)
18-23: @OverRide 추가는 적절합니다. 다만 통화 계산에 double 사용은 장기적으로 오차 리스크가 있습니다.금액 계산은 BigDecimal로 전환을 고려해 주세요. 현재 PR 범위를 넘는다면 추후 작업으로 분리해도 됩니다.
gss-api-app/src/main/java/com/devoops/service/facade/RepositoryFacadeService.java (4)
50-66: 동시성 폴백 처리 추가 제안(유니크 충돌 시 재연결).
save시 유니크 제약 충돌을 캐치해 기존 레코드로 재연결하면 레이스를 안전하게 흡수할 수 있습니다.private GithubRepository registerNewRepo( GithubRepoInfoResponse repositoryInfo, GithubRepoUrl url, User user ) { RepositoryCreateCommand createCommand = new RepositoryCreateCommand( user.getId(), repositoryInfo.name(), url.getUrl(), repositoryInfo.getOwnerName(), INITIAL_PULL_REQUEST_COUNT, repositoryInfo.id() ); - GithubRepository savedRepository = repositoryService.save(createCommand); - webHookService.registerWebhook(user, savedRepository.getId()); - return savedRepository; + try { + GithubRepository savedRepository = repositoryService.save(createCommand); + // 웹훅 등록은 커밋 이후로 지연(별도 코멘트 참고) + org.springframework.transaction.support.TransactionSynchronizationManager + .registerSynchronization(new org.springframework.transaction.support.TransactionSynchronization() { + @Override public void afterCommit() { + webHookService.registerWebhook(user, savedRepository.getId()); + } + }); + return savedRepository; + } catch (org.springframework.dao.DataIntegrityViolationException ex) { + // 이미 다른 트랜잭션이 선점해 저장한 경우: 기존 레코드로 재연결 + return repositoryService.findByUserAndExternalId(user, repositoryInfo.id()) + .map(existing -> reTrackingOrThrowException(user, existing)) + .orElseThrow(ex); + } }
87-92: 다른 유저가 같은 외부 레포를 추적 중인 상황에서의 삭제 정책 확정 필요.현재 TODO 주석과 같이, 여러 유저가 동일 레포를 추적하는 구조라면 한 유저가 삭제 시 공용 리소스(웹훅)를 제거해 다른 유저에 영향이 갈 수 있습니다. 웹훅을 유저별로 분리/중복 생성하는지, 공유하는지에 따라 분기 로직이 달라집니다. 정책 명세와 구현 정합성 확인을 권장합니다.
정책 예:
- 유저별 개별 웹훅: 해당 유저 소유 웹훅 ID만 제거
- 공유 단일 웹훅: 마지막 구독 해지 시에만 제거(참조 카운트)
필요 시 설계/구현 초안 제안 가능합니다.
36-48: 외부 API 호출을 트랜잭션 경계 밖으로 이동 고려.
@Transactional내부에서gitHubService.getRepositoryInfo를 호출하면 트랜잭션 유지 시간이 길어집니다. 네트워크 지연/재시도로 인한 잠금 장기화 가능성이 있어, 조회를 트랜잭션 시작 전으로 분리하는 것을 권장합니다.
36-48: 예외 매핑 범위를 재점검해 주세요.현재
GithubNotFoundException만REGISTRY_GITHUB_REPOSITORY_NOT_FOUND로 매핑합니다. 웹훅 등록 실패가 NotFound 외 사유(권한, 네트워크 등)일 때 다른 에러코드가 더 적절할 수 있습니다. 에러코드 세분화 또는 공통 상위 예외 매핑을 검토해 주세요.gss-api-app/src/test/java/com/devoops/service/facade/RepositoryFacadeServiceTest.java (4)
72-83: 동일 유저 중복 저장 테스트에 웹훅 중복 호출 검증을 추가해 주세요.예외가 던져진 후에도
createWebhook가 추가 호출되지 않았음을 보장하면 회귀를 막을 수 있습니다.assertThatThrownBy(() -> repositoryFacadeService.save(request, user)) .isInstanceOf(GssException.class) .hasMessage(ErrorCode.ALREADY_SAVED_REPOSITORY.getMessage()); + // 웹훅은 최초 1회만 호출 + Mockito.verify(gitHubClient, times(1)).createWebhook(any(), any(), any(), any());
85-97: 서로 다른 유저의 중복 저장 시 웹훅 호출 정책을 명시적으로 검증하세요.의도에 따라 다음 중 하나를 선택해 검증 추가를 권장합니다.
- 정책 A(유저별 웹훅 분리):
createWebhook가 유저 수만큼 호출되어야 함}).doesNotThrowAnyException(); + Mockito.verify(gitHubClient, times(2)).createWebhook(any(), any(), any(), any());
- 정책 B(공유 웹훅 단일화): 두 번째 유저 저장 시 훅 미생성
}).doesNotThrowAnyException(); + Mockito.verify(gitHubClient, times(1)).createWebhook(any(), any(), any(), any());정책 확정 후 해당 검증으로 일치시켜 주세요.
105-112: 재연결 시에는 웹훅 미생성을 명시적으로 검증해 주세요.현재 로직상 재연결(re-tracking)에는 훅을 심지 않으므로 이를 검증하면 의도가 분명해집니다.
repositoryFacadeService.save(request, user); +// 재연결 시 웹훅 미생성 확인 +Mockito.verify(gitHubClient, times(0)).createWebhook(any(), any(), any(), any()); ... assertThat(actual.isTracking()).isTrue();
124-132: 불필요한 지역 변수 정리 제안.
mockingErrorWhenCreateWebHook의WebHookCreateResponse mockWebHookCreateResponse는 사용되지 않습니다. 제거해도 됩니다.- WebHookCreateResponse mockWebHookCreateResponse = new WebHookCreateResponse(123);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
gss-api-app/src/main/java/com/devoops/service/facade/RepositoryFacadeService.java(2 hunks)gss-api-app/src/test/java/com/devoops/service/facade/RepositoryFacadeServiceTest.java(3 hunks)gss-domain/src/main/java/com/devoops/domain/entity/analysis/ClaudeAiModel.java(1 hunks)gss-domain/src/main/java/com/devoops/domain/entity/analysis/OpenAiModel.java(1 hunks)
🔇 Additional comments (3)
gss-domain/src/main/java/com/devoops/domain/entity/analysis/OpenAiModel.java (1)
39-44: 일관성 있는 @OverRide 추가, 좋습니다.기능 변화 없고 가독성 향상에 도움이 됩니다.
gss-api-app/src/test/java/com/devoops/service/facade/RepositoryFacadeServiceTest.java (2)
44-59: 성공 케이스에서 외부 호출 횟수 검증이 명확하고 좋습니다.신규 저장 시 훅 1회, repo 정보 조회 1회를 보장합니다. 테스트 의도와 구현이 일치합니다.
61-71: 웹훅 등록 실패 시 예외 매핑 테스트도 적절합니다.현재 구현과 테스트가 정합적입니다.
| GithubRepoInfoResponse repositoryInfo = gitHubService.getRepositoryInfo(repoUrl, user.getGithubToken()); | ||
| GithubRepository savedRepository = repositoryService.findByUserAndExternalId(user, repositoryInfo.id()) | ||
| .map(alreadyRegisteredRepo -> reTrackingOrThrowException(user, alreadyRegisteredRepo)) | ||
| .orElseGet(() -> registerNewRepo(repositoryInfo, repoUrl, user)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
신규/기존 분기 로직은 명확합니다. 다만 동시성 레이스를 방지하는 보강이 필요합니다.
동일 유저가 같은 repo를 거의 동시에 저장하면 둘 다 findByUserAndExternalId에서 미존재로 판단 후 save로 진입할 수 있습니다. DB 유니크 제약이 있다면 DataIntegrityViolationException으로 끝나고, 없다면 중복 데이터가 생깁니다. registerNewRepo 내부에서 저장 시 예외를 캐치해 재조회/재연결로 폴백하는 방식을 권장합니다.
가능한 보강 예시(핵심 아이디어만 제시):
- GithubRepository savedRepository = repositoryService.findByUserAndExternalId(user, repositoryInfo.id())
+ GithubRepository savedRepository = repositoryService.findByUserAndExternalId(user, repositoryInfo.id())
.map(alreadyRegisteredRepo -> reTrackingOrThrowException(user, alreadyRegisteredRepo))
.orElseGet(() -> registerNewRepo(repositoryInfo, repoUrl, user));registerNewRepo에서 save를 try/catch로 감싸 폴백 처리(아래 코멘트 참고).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
gss-api-app/src/main/java/com/devoops/service/facade/RepositoryFacadeService.java
around lines 38 to 42, the branch correctly chooses registerNewRepo for missing
repos but is vulnerable to a race where two concurrent requests both decide to
insert; update registerNewRepo to wrap the repositoryService.save(...) call in a
try/catch that catches DataIntegrityViolationException (and other relevant
persistence exceptions), and in the catch re-query
repositoryService.findByUserAndExternalId(user, repositoryInfo.id()) and return
that existing entity if found, otherwise rethrow or convert to a clear
application exception; ensure the save remains in a transactional context (or
uses REQUIRES_NEW if appropriate) so the fallback query sees committed state.
| GithubRepository savedRepository = repositoryService.save(createCommand); | ||
| webHookService.registerWebhook(user, savedRepository.getId()); | ||
| return savedRepository; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
웹훅 등록은 트랜잭션 커밋 후로 지연시켜야 외부 부작용과 롤백 불일치를 막을 수 있습니다.
현재는 DB 저장 → 웹훅 호출 순이며, 이후 예외로 롤백되면 GitHub에는 훅이 남고 DB에는 레코드가 없는 불일치가 생길 수 있습니다. 커밋 이후에 호출하도록 전환해 주세요(예: TransactionSynchronizationManager 또는 @TransactionalEventListener 사용).
아주 국소 수정 예시:
- GithubRepository savedRepository = repositoryService.save(createCommand);
- webHookService.registerWebhook(user, savedRepository.getId());
- return savedRepository;
+ GithubRepository savedRepository = repositoryService.save(createCommand);
+ // 커밋 이후 실행
+ org.springframework.transaction.support.TransactionSynchronizationManager
+ .registerSynchronization(new org.springframework.transaction.support.TransactionSynchronization() {
+ @Override
+ public void afterCommit() {
+ webHookService.registerWebhook(user, savedRepository.getId());
+ }
+ });
+ return savedRepository;또는 도메인 이벤트 발행 → @TransactionalEventListener(phase = AFTER_COMMIT) 리스너에서 registerWebhook 수행도 좋습니다.
📝 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.
| GithubRepository savedRepository = repositoryService.save(createCommand); | |
| webHookService.registerWebhook(user, savedRepository.getId()); | |
| return savedRepository; | |
| } | |
| GithubRepository savedRepository = repositoryService.save(createCommand); | |
| // 커밋 이후 실행 | |
| org.springframework.transaction.support.TransactionSynchronizationManager | |
| .registerSynchronization(new org.springframework.transaction.support.TransactionSynchronization() { | |
| @Override | |
| public void afterCommit() { | |
| webHookService.registerWebhook(user, savedRepository.getId()); | |
| } | |
| }); | |
| return savedRepository; | |
| } |
🤖 Prompt for AI Agents
In
gss-api-app/src/main/java/com/devoops/service/facade/RepositoryFacadeService.java
around lines 63-66, the code calls webHookService.registerWebhook immediately
after saving the repository which can leave a webhook registered if the
surrounding transaction later rolls back; change this so webhook registration
runs after the transaction commits — either publish a domain event (e.g.
RepositoryCreatedEvent) and handle it in a @TransactionalEventListener(phase =
AFTER_COMMIT) that calls webHookService.registerWebhook(user,
savedRepository.getId()), or register a TransactionSynchronization (via
TransactionSynchronizationManager.registerSynchronization) that invokes
registerWebhook in afterCommit; ensure the save still returns the saved entity
and that exception handling/logging remains in the async/after-commit path.
coli-geonwoo
deleted the
fix/#82-skip-creatwebhook-when-duplicate-repository
branch
1 participant
🚩 연관 이슈
🔂 변경 내역
Summary by CodeRabbit
버그 수정
테스트
기타