Conversation
📝 WalkthroughWalkthrough클럽 시트 미리보기 API가 요청 본문으로 스프레드시트 URL을 받지 않고, 클럽에 등록된 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant ClubRepo as ClubRepository
participant Mapper as ObjectMapper
participant GoogleSheets as GoogleSheetsService
Client->>Controller: POST /clubs/{clubId}/sheet/import/preview (path clubId, user)
Controller->>Service: previewPreMembersFromSheet(clubId, requesterId)
Service->>ClubRepo: getById(clubId)
alt club.googleSheetId & club.sheetColumnMapping present
Service->>Mapper: deserialize(club.sheetColumnMapping)
Service->>GoogleSheets: readSpreadsheet(spreadsheetId, mapping)
GoogleSheets-->>Service: sheet data
Service-->>Controller: SheetImportPreviewResponse
Controller-->>Client: 200 OK + body
else missing googleSheetId or mapping
Service-->>Controller: throw CustomException(CLUB_SHEET_ANALYSIS_REQUIRED)
Controller-->>Client: 400 Bad Request (code/message)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
시인(詩人)의 시
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java (1)
57-77:⚠️ Potential issue | 🟠 Major[LEVEL: medium] 중복 DB 조회 발생
resolveRegisteredSpreadsheetId(clubId)(line 63 → 80)에서clubRepository.getById(clubId)를 호출한 후,executeReadOnlyTransaction내부 (line 68)에서 동일한 조회가 반복됩니다. 매 preview 요청마다 불필요한 DB 쿼리가 발생합니다.resolveRegisteredSpreadsheetId에서Club엔티티를 반환하거나, 튜플로 반환하여 재사용하는 것을 권장합니다.♻️ 수정 제안
public SheetImportPreviewResponse previewPreMembersFromSheet( Integer clubId, Integer requesterId ) { clubPermissionValidator.validateManagerAccess(clubId, requesterId); - String spreadsheetId = resolveRegisteredSpreadsheetId(clubId); + Club club = clubRepository.getById(clubId); + String spreadsheetId = club.getGoogleSheetId(); + if (spreadsheetId == null || spreadsheetId.isBlank()) { + throw CustomException.of(ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED); + } SheetHeaderMapper.SheetAnalysisResult analysis = sheetHeaderMapper.analyzeAllSheets(spreadsheetId); SheetImportSource source = loadSheetImportSource(spreadsheetId, analysis.memberListMapping()); return executeReadOnlyTransaction(() -> { - Club club = clubRepository.getById(clubId); SheetImportPlan plan = buildImportPlan( clubId, club, source.members(), source.warnings() ); return SheetImportPreviewResponse.of(plan.previewMembers(), plan.warnings()); }); } - -private String resolveRegisteredSpreadsheetId(Integer clubId) { - Club club = clubRepository.getById(clubId); - String spreadsheetId = club.getGoogleSheetId(); - if (spreadsheetId == null || spreadsheetId.isBlank()) { - throw CustomException.of(ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED); - } - return spreadsheetId; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java` around lines 57 - 77, The previewPreMembersFromSheet method currently calls clubRepository.getById twice (once inside resolveRegisteredSpreadsheetId and again inside executeReadOnlyTransaction); modify resolveRegisteredSpreadsheetId to return the Club (or a small tuple/object containing both the Club and the resolved spreadsheetId) so previewPreMembersFromSheet can reuse that Club and avoid the second DB fetch, then update previewPreMembersFromSheet to pass the reused Club into buildImportPlan instead of calling clubRepository.getById again within executeReadOnlyTransaction; adjust caller usage of resolveRegisteredSpreadsheetId accordingly and keep functions named resolveRegisteredSpreadsheetId, previewPreMembersFromSheet, executeReadOnlyTransaction, buildImportPlan, and clubRepository.getById as reference points.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java`:
- Around line 57-77: The previewPreMembersFromSheet method currently calls
clubRepository.getById twice (once inside resolveRegisteredSpreadsheetId and
again inside executeReadOnlyTransaction); modify resolveRegisteredSpreadsheetId
to return the Club (or a small tuple/object containing both the Club and the
resolved spreadsheetId) so previewPreMembersFromSheet can reuse that Club and
avoid the second DB fetch, then update previewPreMembersFromSheet to pass the
reused Club into buildImportPlan instead of calling clubRepository.getById again
within executeReadOnlyTransaction; adjust caller usage of
resolveRegisteredSpreadsheetId accordingly and keep functions named
resolveRegisteredSpreadsheetId, previewPreMembersFromSheet,
executeReadOnlyTransaction, buildImportPlan, and clubRepository.getById as
reference points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4677c885-e228-4162-bff4-25145d89aab7
📒 Files selected for processing (6)
src/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationApi.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationController.javasrc/main/java/gg/agit/konect/domain/club/service/SheetImportService.javasrc/main/java/gg/agit/konect/global/code/ApiResponseCode.javasrc/test/java/gg/agit/konect/domain/club/service/SheetImportServiceTest.javasrc/test/java/gg/agit/konect/integration/domain/club/ClubSheetMigrationApiTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
src/main/java/**/*.java: 아래 원칙으로 리뷰 코멘트를 작성한다.
- 코멘트는 반드시 한국어로 작성한다.
- 반드시 수정이 필요한 항목만 코멘트로 남기고, 단순 취향 차이는 지적하지 않는다.
- 각 코멘트 첫 줄에 심각도를
[LEVEL: high|medium|low]형식으로 반드시 표기한다.- 심각도 기준: high=운영 장애 가능, medium=품질 저하, low=개선 권고.
- 각 코멘트는 "문제 -> 영향 -> 제안" 순서로 3문장 이내로 간결하게 작성한다.
- 가능하면 재현 조건 및 실패 시나리오도 포함한다.
- 제안은 현재 코드베이스(Spring Boot + JPA + Flyway) 패턴과 일치해야 한다.
- 보안, 트랜잭션 경계, 예외 처리, N+1, 성능 회귀 가능성을 우선 점검한다.
- 가독성: 변수/메서드 이름이 의도를 바로 드러내는지, 중첩과 메서드 길이가 과도하지 않은지 점검한다.
- 단순화: 불필요한 추상화, 중복 로직, 과한 방어 코드가 있으면 더 단순한 대안을 제시한다.
- 확장성: 새 요구사항 추가 시 변경 범위가 최소화되는 구조인지(하드코딩 분기/값 여부 포함) 점검한다.
Files:
src/main/java/gg/agit/konect/global/code/ApiResponseCode.javasrc/main/java/gg/agit/konect/domain/club/service/SheetImportService.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationController.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationApi.java
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/main/java/gg/agit/konect/global/code/ApiResponseCode.javasrc/main/java/gg/agit/konect/domain/club/service/SheetImportService.javasrc/test/java/gg/agit/konect/domain/club/service/SheetImportServiceTest.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationController.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationApi.javasrc/test/java/gg/agit/konect/integration/domain/club/ClubSheetMigrationApiTest.java
🔇 Additional comments (6)
src/main/java/gg/agit/konect/global/code/ApiResponseCode.java (1)
52-53: [LEVEL: low] LGTM!새로운
CLUB_SHEET_ANALYSIS_REQUIRED응답 코드가 기존 패턴에 맞게 잘 추가되었습니다.HttpStatus.BAD_REQUEST가 사전 조건 실패에 적절하며, 메시지가 사용자에게 명확한 안내를 제공합니다.src/test/java/gg/agit/konect/integration/domain/club/ClubSheetMigrationApiTest.java (1)
107-121: LGTM!새로운
previewPreMembersRequiresRegisteredSheet테스트가CLUB_SHEET_ANALYSIS_REQUIRED예외 시나리오를 적절히 검증합니다. HTTP 400 응답과code,messageJSON 필드 검증이 포함되어 있습니다.src/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationController.java (1)
44-52: [LEVEL: low] LGTM!컨트롤러 변경이 API 계약 업데이트와 일치합니다. 요청 본문 제거 후에도 컨트롤러가 서비스에 적절히 위임하고 있습니다.
src/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationApi.java (1)
38-50: [LEVEL: low] LGTM!API 문서와 메서드 시그니처가 새로운 동작을 정확히 반영합니다.
PUT /clubs/{clubId}/sheet로 등록된 시트를 사용한다는 설명이 명확합니다.src/test/java/gg/agit/konect/domain/club/service/SheetImportServiceTest.java (2)
137-149: LGTM!
previewPreMembersFromSheetThrowsWhenSheetIsNotRegistered테스트가 시트 미등록 시나리오를 적절히 검증합니다.verifyNoInteractions를 통해 예외 발생 후 Google Sheets API 호출이 없음을 확인하는 것이 좋습니다.
90-92: LGTM!성공 케이스 테스트에서
club.updateGoogleSheetId(SPREADSHEET_ID)설정이 추가되어 새로운 서비스 동작과 일치합니다.
JanooGwan
changed the title
JanooGwan
added
리팩토링
JanooGwan
force-pushed
the
fix/sheet-preview-registered-only
branch
from
550a2dd to
cbe5f6b
Compare
There was a problem hiding this comment.
Pull request overview
/clubs/{clubId}/sheet/import/preview API가 더 이상 요청 body의 스프레드시트 URL을 받지 않고, 사전에 PUT /clubs/{clubId}/sheet로 등록된 시트(club.googleSheetId)만을 기준으로 미리보기를 수행하도록 정리한 PR입니다.
Changes:
- Preview API에서 request body(
SheetImportRequest) 의존을 제거하고 등록된 시트 기반으로 동작하도록 컨트롤러/서비스 시그니처를 변경 - 등록되지 않은 시트에 대해 preview 호출 시
CLUB_SHEET_ANALYSIS_REQUIRED(400) 예외 코드 및 메시지 추가 - 변경된 API 동작에 맞춰 서비스 단위 테스트 및 API 통합 테스트를 갱신하고, “미등록 시트” 시나리오 테스트를 추가
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/gg/agit/konect/integration/domain/club/ClubSheetMigrationApiTest.java | preview endpoint가 body 없이 호출되도록 수정하고, 미등록 시트 400 케이스를 추가 |
| src/test/java/gg/agit/konect/domain/club/service/SheetImportServiceTest.java | preview가 등록된 sheetId를 사용하도록 갱신하고, 미등록 시 예외 발생 테스트를 추가 |
| src/main/java/gg/agit/konect/global/code/ApiResponseCode.java | 미등록/미분석 시트 preview 호출에 대한 신규 응답 코드 추가 |
| src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java | preview 로직을 club.googleSheetId 기반으로 변경하고, 미등록 시 예외 처리 추가 |
| src/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationController.java | preview API의 request body 제거 및 서비스 호출 시그니처 변경 |
| src/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationApi.java | Swagger 설명 및 API 시그니처에서 request body 제거 |
| private String resolveRegisteredSpreadsheetId(Integer clubId) { | ||
| Club club = clubRepository.getById(clubId); | ||
| String spreadsheetId = club.getGoogleSheetId(); | ||
| if (spreadsheetId == null || spreadsheetId.isBlank()) { | ||
| throw CustomException.of(ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED); |
There was a problem hiding this comment.
resolveRegisteredSpreadsheetId()에서 clubRepository.getById()로 클럽을 조회한 뒤, previewPreMembersFromSheet() 내부의 read-only 트랜잭션에서도 다시 clubRepository.getById()를 호출하고 있어 동일 요청에서 클럽 조회가 2번 발생합니다. 클럽을 한 번만 조회하도록(예: 클럽을 먼저 조회해 sheetId 검증 후 동일 객체 재사용, 또는 이 메서드가 Club까지 반환) 중복 조회를 제거해 주세요.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java (1)
57-76:⚠️ Potential issue | 🟡 Minor[LEVEL: medium]
clubRepository.getById(clubId)중복 호출로 불필요한 DB 쿼리 발생
resolveRegisteredSpreadsheetId(clubId)(Line 63 → 80)에서 Club을 조회한 후,executeReadOnlyTransaction내부 (Line 68)에서 동일한 Club을 다시 조회합니다. 미리보기 호출마다 DB 쿼리가 2회 발생하여 성능 저하 가능성이 있습니다.resolveRegisteredSpreadsheetId가 Club 객체를 반환하도록 수정하거나, 검증 로직을 트랜잭션 내부로 이동하는 것을 권장합니다.♻️ 중복 쿼리 제거 예시
public SheetImportPreviewResponse previewPreMembersFromSheet( Integer clubId, Integer requesterId ) { clubPermissionValidator.validateManagerAccess(clubId, requesterId); - String spreadsheetId = resolveRegisteredSpreadsheetId(clubId); + Club club = resolveRegisteredClub(clubId); + String spreadsheetId = club.getGoogleSheetId(); SheetHeaderMapper.SheetAnalysisResult analysis = sheetHeaderMapper.analyzeAllSheets(spreadsheetId); SheetImportSource source = loadSheetImportSource(spreadsheetId, analysis.memberListMapping()); return executeReadOnlyTransaction(() -> { - Club club = clubRepository.getById(clubId); SheetImportPlan plan = buildImportPlan( clubId, club, source.members(), source.warnings() ); return SheetImportPreviewResponse.of(plan.previewMembers(), plan.warnings()); }); } -private String resolveRegisteredSpreadsheetId(Integer clubId) { +private Club resolveRegisteredClub(Integer clubId) { Club club = clubRepository.getById(clubId); String spreadsheetId = club.getGoogleSheetId(); if (spreadsheetId == null || spreadsheetId.isBlank()) { throw CustomException.of(ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED); } - return spreadsheetId; + return club; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java` around lines 57 - 76, The previewPreMembersFromSheet method currently fetches the Club twice (once inside resolveRegisteredSpreadsheetId and again via clubRepository.getById in the executeReadOnlyTransaction), causing duplicate DB queries; fix by changing resolveRegisteredSpreadsheetId to return the resolved Club (or add an overload that accepts/returns Club) so you can reuse that Club instance in the transaction, or alternatively move the spreadsheetId resolution and manager validation into the executeReadOnlyTransaction block; update references in previewPreMembersFromSheet to pass the resolved Club to buildImportPlan and remove the second clubRepository.getById call to eliminate the redundant query (refer to previewPreMembersFromSheet, resolveRegisteredSpreadsheetId, executeReadOnlyTransaction, and clubRepository.getById).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java`:
- Around line 57-76: The previewPreMembersFromSheet method currently fetches the
Club twice (once inside resolveRegisteredSpreadsheetId and again via
clubRepository.getById in the executeReadOnlyTransaction), causing duplicate DB
queries; fix by changing resolveRegisteredSpreadsheetId to return the resolved
Club (or add an overload that accepts/returns Club) so you can reuse that Club
instance in the transaction, or alternatively move the spreadsheetId resolution
and manager validation into the executeReadOnlyTransaction block; update
references in previewPreMembersFromSheet to pass the resolved Club to
buildImportPlan and remove the second clubRepository.getById call to eliminate
the redundant query (refer to previewPreMembersFromSheet,
resolveRegisteredSpreadsheetId, executeReadOnlyTransaction, and
clubRepository.getById).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 267e833b-c738-4cb0-9ca2-43ed8d07f6f5
📒 Files selected for processing (6)
src/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationApi.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationController.javasrc/main/java/gg/agit/konect/domain/club/service/SheetImportService.javasrc/main/java/gg/agit/konect/global/code/ApiResponseCode.javasrc/test/java/gg/agit/konect/domain/club/service/SheetImportServiceTest.javasrc/test/java/gg/agit/konect/integration/domain/club/ClubSheetMigrationApiTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
src/main/java/**/*.java: 아래 원칙으로 리뷰 코멘트를 작성한다.
- 코멘트는 반드시 한국어로 작성한다.
- 반드시 수정이 필요한 항목만 코멘트로 남기고, 단순 취향 차이는 지적하지 않는다.
- 각 코멘트 첫 줄에 심각도를
[LEVEL: high|medium|low]형식으로 반드시 표기한다.- 심각도 기준: high=운영 장애 가능, medium=품질 저하, low=개선 권고.
- 각 코멘트는 "문제 -> 영향 -> 제안" 순서로 3문장 이내로 간결하게 작성한다.
- 가능하면 재현 조건 및 실패 시나리오도 포함한다.
- 제안은 현재 코드베이스(Spring Boot + JPA + Flyway) 패턴과 일치해야 한다.
- 보안, 트랜잭션 경계, 예외 처리, N+1, 성능 회귀 가능성을 우선 점검한다.
- 가독성: 변수/메서드 이름이 의도를 바로 드러내는지, 중첩과 메서드 길이가 과도하지 않은지 점검한다.
- 단순화: 불필요한 추상화, 중복 로직, 과한 방어 코드가 있으면 더 단순한 대안을 제시한다.
- 확장성: 새 요구사항 추가 시 변경 범위가 최소화되는 구조인지(하드코딩 분기/값 여부 포함) 점검한다.
Files:
src/main/java/gg/agit/konect/global/code/ApiResponseCode.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationController.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationApi.javasrc/main/java/gg/agit/konect/domain/club/service/SheetImportService.java
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/main/java/gg/agit/konect/global/code/ApiResponseCode.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationController.javasrc/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationApi.javasrc/main/java/gg/agit/konect/domain/club/service/SheetImportService.javasrc/test/java/gg/agit/konect/domain/club/service/SheetImportServiceTest.javasrc/test/java/gg/agit/konect/integration/domain/club/ClubSheetMigrationApiTest.java
🔇 Additional comments (5)
src/main/java/gg/agit/konect/global/code/ApiResponseCode.java (1)
52-53: LGTM!새 에러 코드가 기존 패턴을 잘 따르고 있으며, HTTP 400 상태와 메시지가 적절합니다.
src/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationController.java (1)
43-52: LGTM!컨트롤러가 요청 바디 파라미터를 제거하고 서비스 레이어에 올바르게 위임합니다.
src/main/java/gg/agit/konect/domain/club/controller/ClubSheetMigrationApi.java (1)
38-50: LGTM!API 인터페이스와 Swagger 문서가 변경된 요구사항을 명확하게 반영하고 있습니다.
src/test/java/gg/agit/konect/integration/domain/club/ClubSheetMigrationApiTest.java (1)
107-121: LGTM!시트 미등록 시나리오에 대한 테스트가 적절하게 추가되었습니다. HTTP 상태 코드와 응답 바디 검증이 모두 포함되어 있습니다.
src/test/java/gg/agit/konect/domain/club/service/SheetImportServiceTest.java (1)
137-149: LGTM!시트 미등록 시 예외 발생 테스트가 잘 작성되었습니다.
verifyNoInteractions로 불필요한 외부 호출이 없음을 검증하는 점이 좋습니다.
| @Test | ||
| void previewPreMembersFromSheetThrowsWhenSheetIsNotRegistered() { | ||
| Club club = ClubFixture.create(UniversityFixture.create()); | ||
|
|
||
| given(clubRepository.getById(CLUB_ID)).willReturn(club); | ||
|
|
||
| assertThatThrownBy(() -> sheetImportService.previewPreMembersFromSheet(CLUB_ID, REQUESTER_ID)) | ||
| .isInstanceOf(CustomException.class) |
There was a problem hiding this comment.
새로 추가된 테스트에서는 previewPreMembersFromSheet()가 내부적으로 TransactionTemplate을 실행하므로 transactionManager.getTransaction(...)가 반드시 TransactionStatus를 반환해야 합니다. 현재 이 테스트에서는 transactionManager 스텁이 없어 Mockito 기본값(null)로 인해 NPE로 테스트가 실패할 수 있으니, 다른 테스트처럼 given(transactionManager.getTransaction(any())).willReturn(new SimpleTransactionStatus()) 등을 추가해 주세요.
| return executeReadOnlyTransaction(() -> { | ||
| Club club = clubRepository.getById(clubId); | ||
| Club club = resolveRegisteredSpreadsheetId(clubId); | ||
| String spreadsheetId = club.getGoogleSheetId(); | ||
| SheetHeaderMapper.SheetAnalysisResult analysis = | ||
| sheetHeaderMapper.analyzeAllSheets(spreadsheetId); | ||
| SheetImportSource source = loadSheetImportSource(spreadsheetId, analysis.memberListMapping()); |
There was a problem hiding this comment.
executeReadOnlyTransaction 블록 안에서 sheetHeaderMapper.analyzeAllSheets(...)(Claude API 호출 포함)와 loadSheetImportSource(...)(Google Sheets I/O)가 수행되고 있어, DB 트랜잭션/커넥션을 잡은 채 외부 네트워크 호출을 하게 됩니다. 커넥션 풀 고갈/지연의 원인이 될 수 있으니 외부 API 호출은 트랜잭션 밖으로 빼고, DB 조회(멤버/프리멤버/유저 조회 등)만 read-only 트랜잭션으로 감싸도록 구조를 조정해 주세요.
| Club club = resolveRegisteredSpreadsheetId(clubId); | ||
| String spreadsheetId = club.getGoogleSheetId(); | ||
| SheetHeaderMapper.SheetAnalysisResult analysis = | ||
| sheetHeaderMapper.analyzeAllSheets(spreadsheetId); | ||
| SheetImportSource source = loadSheetImportSource(spreadsheetId, analysis.memberListMapping()); |
There was a problem hiding this comment.
PR 설명상 preview는 PUT /clubs/{clubId}/sheet에서 "AI 분석 및 등록"이 완료된 시트를 사용하도록 변경된 것으로 보이는데, 현재는 preview 호출 때마다 sheetHeaderMapper.analyzeAllSheets(spreadsheetId)로 재분석(Claude 호출)을 수행합니다. 이미 club.sheetColumnMapping을 저장하는 흐름이 있으니, 가능하면 저장된 매핑을 사용해 preview를 수행하고(없을 때만 분석), 분석 완료 여부 판단도 매핑 존재 여부까지 포함하도록 맞춰 주세요.
| private Club resolveRegisteredSpreadsheetId(Integer clubId) { | ||
| Club club = clubRepository.getById(clubId); | ||
| String spreadsheetId = club.getGoogleSheetId(); | ||
| if (spreadsheetId == null || spreadsheetId.isBlank()) { | ||
| throw CustomException.of(ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED); | ||
| } | ||
| return club; | ||
| } |
There was a problem hiding this comment.
메서드명 resolveRegisteredSpreadsheetId는 반환 타입이 Club인데도 "SpreadsheetId"를 resolve하는 것처럼 읽혀 오해 소지가 있습니다. 실제 역할(등록된 시트 ID가 있는 클럽을 조회/검증)을 드러내도록 resolveClubWithRegisteredSheetId 같은 형태로 이름을 조정해 주세요.
| Club club = resolveClubWithRegisteredSheet(clubId); | ||
| String spreadsheetId = club.getGoogleSheetId(); | ||
| SheetColumnMapping mapping = resolveRegisteredMemberListMapping(club); | ||
| SheetImportSource source = loadSheetImportSource(spreadsheetId, mapping); | ||
| SheetImportPlan plan = buildImportPlan( | ||
| clubId, | ||
| club, | ||
| source.members(), | ||
| source.warnings() | ||
| ); | ||
| return SheetImportPreviewResponse.of(plan.previewMembers(), plan.warnings()); | ||
| } | ||
|
|
There was a problem hiding this comment.
previewPreMembersFromSheet()가 @Transactional/executeReadOnlyTransaction 없이 Club 엔티티를 조회한 뒤 buildImportPlan()에서 club.getUniversity().getId()를 접근합니다. Club.university는 LAZY라서(그리고 ClubRepository.getById는 university를 fetch join 하지 않음) 트랜잭션 밖에서는 LazyInitializationException이 발생할 수 있습니다. 이 메서드 전체를 @transactional(readOnly = true)로 감싸거나, 최소한 club 조회 및 buildImportPlan 구간을 executeReadOnlyTransaction으로 감싸도록 수정해주세요(구글 시트 호출은 트랜잭션 밖에 두는 편이 안전합니다).
| Club club = resolveClubWithRegisteredSheet(clubId); | |
| String spreadsheetId = club.getGoogleSheetId(); | |
| SheetColumnMapping mapping = resolveRegisteredMemberListMapping(club); | |
| SheetImportSource source = loadSheetImportSource(spreadsheetId, mapping); | |
| SheetImportPlan plan = buildImportPlan( | |
| clubId, | |
| club, | |
| source.members(), | |
| source.warnings() | |
| ); | |
| return SheetImportPreviewResponse.of(plan.previewMembers(), plan.warnings()); | |
| } | |
| RegisteredSheetContext context = executeReadOnlyTransaction(() -> { | |
| Club club = resolveClubWithRegisteredSheet(clubId); | |
| return new RegisteredSheetContext( | |
| club.getGoogleSheetId(), | |
| resolveRegisteredMemberListMapping(club) | |
| ); | |
| }); | |
| SheetImportSource source = loadSheetImportSource(context.spreadsheetId(), context.mapping()); | |
| SheetImportPlan plan = executeReadOnlyTransaction(() -> { | |
| Club club = resolveClubWithRegisteredSheet(clubId); | |
| return buildImportPlan( | |
| clubId, | |
| club, | |
| source.members(), | |
| source.warnings() | |
| ); | |
| }); | |
| return SheetImportPreviewResponse.of(plan.previewMembers(), plan.warnings()); | |
| } | |
| private <T> T executeReadOnlyTransaction(Supplier<T> supplier) { | |
| TransactionTemplate transactionTemplate = new TransactionTemplate(transactionManager); | |
| transactionTemplate.setReadOnly(true); | |
| return transactionTemplate.execute(status -> supplier.get()); | |
| } | |
| private static final class RegisteredSheetContext { | |
| private final String spreadsheetId; | |
| private final SheetColumnMapping mapping; | |
| private RegisteredSheetContext(String spreadsheetId, SheetColumnMapping mapping) { | |
| this.spreadsheetId = spreadsheetId; | |
| this.mapping = mapping; | |
| } | |
| private String spreadsheetId() { | |
| return spreadsheetId; | |
| } | |
| private SheetColumnMapping mapping() { | |
| return mapping; | |
| } | |
| } |
| try { | ||
| Map<String, Object> raw = objectMapper.readValue(mappingJson, new TypeReference<>() {}); | ||
| int dataStartRow = raw.containsKey("dataStartRow") | ||
| ? ((Number)raw.get("dataStartRow")).intValue() : 2; | ||
| Map<String, Integer> fieldMap = new HashMap<>(); |
There was a problem hiding this comment.
resolveRegisteredMemberListMapping()의 JSON 파싱 로직이 SheetSyncExecutor.resolveRawMapping()와 거의 동일하게 중복되어 있습니다. 두 곳이 서로 다른 방식으로 진화하면(기본값/검증 규칙 등) 동작이 쉽게 어긋날 수 있으니, 공용 유틸(예: SheetColumnMapping의 static parse/fromJson)로 추출하거나 한쪽 로직을 재사용하도록 정리하는 편이 유지보수에 유리합니다.
| } | ||
| }); | ||
| return new SheetColumnMapping(fieldMap, dataStartRow); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
예외를 잡아 CLUB_SHEET_ANALYSIS_REQUIRED로 변환하는 것은 의도일 수 있지만, 현재 catch 블록에서 원인 예외를 완전히 삼켜 운영/디버깅 시 원인 파악이 어렵습니다. 최소한 log.warn으로 cause를 남기거나(민감정보가 없다면 mappingJson 길이/일부 정보 포함) detail에 원인 요약을 넣는 방식으로 추적 가능성을 확보해주세요.
| } catch (Exception e) { | |
| } catch (Exception e) { | |
| log.warn( | |
| "Failed to parse sheet column mapping. mappingJsonLength={}, causeType={}", | |
| mappingJson.length(), | |
| e.getClass().getSimpleName(), | |
| e | |
| ); |
| import org.mockito.Mock; | ||
| import org.mockito.Spy; | ||
| import org.springframework.transaction.PlatformTransactionManager; | ||
| import org.springframework.transaction.support.SimpleTransactionStatus; |
There was a problem hiding this comment.
현재 SimpleTransactionStatus import가 사용되지 않습니다. 자바는 unused import가 컴파일 에러라서 테스트/빌드가 깨질 수 있으니 해당 import를 제거해주세요(또는 실제로 필요하다면 사용 코드가 남아있어야 합니다).
| import org.springframework.transaction.support.SimpleTransactionStatus; |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java`:
- Around line 60-76: The previewPreMembersFromSheet method can trigger
LazyInitializationException because resolveClubWithRegisteredSheet returns a
Club JPA proxy and buildImportPlan calls club.getUniversity().getId() outside a
transaction; fix by making previewPreMembersFromSheet transactional (add
`@Transactional`(readOnly = true) to the method) or ensure
resolveClubWithRegisteredSheet eagerly loads the university (use a fetch-join
query or repository method that fetches University) so buildImportPlan can
access club.getUniversity().getId() safely.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 08f8919b-3be9-42d2-bfe6-b574b7de03cd
📒 Files selected for processing (2)
src/main/java/gg/agit/konect/domain/club/service/SheetImportService.javasrc/test/java/gg/agit/konect/domain/club/service/SheetImportServiceTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
src/main/java/**/*.java: 아래 원칙으로 리뷰 코멘트를 작성한다.
- 코멘트는 반드시 한국어로 작성한다.
- 반드시 수정이 필요한 항목만 코멘트로 남기고, 단순 취향 차이는 지적하지 않는다.
- 각 코멘트 첫 줄에 심각도를
[LEVEL: high|medium|low]형식으로 반드시 표기한다.- 심각도 기준: high=운영 장애 가능, medium=품질 저하, low=개선 권고.
- 각 코멘트는 "문제 -> 영향 -> 제안" 순서로 3문장 이내로 간결하게 작성한다.
- 가능하면 재현 조건 및 실패 시나리오도 포함한다.
- 제안은 현재 코드베이스(Spring Boot + JPA + Flyway) 패턴과 일치해야 한다.
- 보안, 트랜잭션 경계, 예외 처리, N+1, 성능 회귀 가능성을 우선 점검한다.
- 가독성: 변수/메서드 이름이 의도를 바로 드러내는지, 중첩과 메서드 길이가 과도하지 않은지 점검한다.
- 단순화: 불필요한 추상화, 중복 로직, 과한 방어 코드가 있으면 더 단순한 대안을 제시한다.
- 확장성: 새 요구사항 추가 시 변경 범위가 최소화되는 구조인지(하드코딩 분기/값 여부 포함) 점검한다.
Files:
src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/main/java/gg/agit/konect/domain/club/service/SheetImportService.javasrc/test/java/gg/agit/konect/domain/club/service/SheetImportServiceTest.java
| public SheetImportPreviewResponse previewPreMembersFromSheet( | ||
| Integer clubId, | ||
| Integer requesterId, | ||
| String spreadsheetUrl | ||
| Integer requesterId | ||
| ) { | ||
| clubPermissionValidator.validateManagerAccess(clubId, requesterId); | ||
| Club club = resolveClubWithRegisteredSheet(clubId); | ||
| String spreadsheetId = club.getGoogleSheetId(); | ||
| SheetColumnMapping mapping = resolveRegisteredMemberListMapping(club); | ||
| SheetImportSource source = loadSheetImportSource(spreadsheetId, mapping); | ||
| SheetImportPlan plan = buildImportPlan( | ||
| clubId, | ||
| club, | ||
| source.members(), | ||
| source.warnings() | ||
| ); | ||
| return SheetImportPreviewResponse.of(plan.previewMembers(), plan.warnings()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== preview method transaction boundary =="
sed -n '60,110p' src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java
echo
echo "== Club entity fetch-related declarations =="
fd '^Club\.java$' src/main/java | while read -r file; do
echo "-- $file --"
rg -n -C2 'University|fetch\s*=|@ManyToOne|@OneToOne' "$file"
done
echo
echo "== OSIV configuration =="
fd 'application.*' . | xargs -r rg -n 'open-in-view'Repository: BCSDLab/KONECT_BACK_END
Length of output: 3548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check buildImportPlan method
echo "== buildImportPlan method implementation =="
sed -n '1,150p' src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java | tail -n +110
# Check if buildImportPlan accesses club.getUniversity()
echo
echo "== Search for getUniversity access in SheetImportService =="
rg -n 'getUniversity|university' src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java
# Check Club entity to confirm getGoogleSheetId is simple field
echo
echo "== Club entity fields =="
rg -n 'getGoogleSheetId|googleSheetId' src/main/java/gg/agit/konect/domain/club/model/Club.javaRepository: BCSDLab/KONECT_BACK_END
Length of output: 1924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and display buildImportPlan method
echo "== buildImportPlan method full implementation =="
rg -n -A 30 'SheetImportPlan buildImportPlan' src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java
# Verify line 225 is inside buildImportPlan
echo
echo "== Context around line 225 =="
sed -n '200,240p' src/main/java/gg/agit/konect/domain/club/service/SheetImportService.javaRepository: BCSDLab/KONECT_BACK_END
Length of output: 3280
[LEVEL: high] preview 경로의 트랜잭션 경계가 없어서 지연 로딩 실패 위험이 있습니다.
문제: previewPreMembersFromSheet는 @Transactional 없이 resolveClubWithRegisteredSheet를 통해 clubRepository.getById(clubId)로 JPA 프록시를 받고, 이를 buildImportPlan으로 전달합니다. buildImportPlan 메서드의 225번 줄에서 club.getUniversity().getId()를 호출하는데, University는 @ManyToOne(fetch = LAZY)로 선언되어 있으므로 트랜잭션 없이 접근하면 지연 로딩 실패합니다.
영향: OSIV가 비활성화된 현재 환경(application-db.yml: open-in-view: false)에서 이 엔드포인트는 LazyInitializationException으로 500 실패합니다.
제안: 다른 import 경로처럼 @Transactional(readOnly = true) 어노테이션을 메서드에 추가하거나, 최소한 fetch join으로 필요한 관계를 즉시 로드하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/gg/agit/konect/domain/club/service/SheetImportService.java`
around lines 60 - 76, The previewPreMembersFromSheet method can trigger
LazyInitializationException because resolveClubWithRegisteredSheet returns a
Club JPA proxy and buildImportPlan calls club.getUniversity().getId() outside a
transaction; fix by making previewPreMembersFromSheet transactional (add
`@Transactional`(readOnly = true) to the method) or ensure
resolveClubWithRegisteredSheet eagerly loads the university (use a fetch-join
query or repository method that fetches University) so buildImportPlan can
access club.getUniversity().getId() safely.
📌 개요
/clubs/{clubId}/sheet/import/preview가 더 이상 요청 body의 URL을 직접 읽지 않고,PUT /clubs/{clubId}/sheet로 분석 및 등록이 완료된 시트만 사용하도록 정리했습니다.✅ 주요 변경 내용
/sheet/import/preview컨트롤러에서 request body를 제거하고 등록된 시트 기준으로 서비스 메서드를 호출하도록 변경SheetImportService에서club.googleSheetId를 기준으로 preview를 수행하고, 미등록 시CLUB_SHEET_ANALYSIS_REQUIRED예외를 반환하도록 추가ApiResponseCode에 "구글 시트 파일에서 동아리 부원을 가져오기 전에 먼저 AI 분석 및 등록이 완료되어야 합니다." 메시지의 예외 코드 추가🙏 참고 사항
✅ Checklist (완료 조건)