-
Notifications
You must be signed in to change notification settings - Fork 1
fix: 구글 시트 preview API의 request 제거 #513
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
Changes from all commits
01ffcab
cbe5f6b
388bd39
5e737e5
fa0dd52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.transaction.annotation.Transactional; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.transaction.support.TransactionTemplate; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.fasterxml.jackson.core.type.TypeReference; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.api.services.sheets.v4.Sheets; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.api.services.sheets.v4.model.ValueRange; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -53,28 +55,55 @@ public class SheetImportService { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ChatRoomMembershipService chatRoomMembershipService; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ClubPermissionValidator clubPermissionValidator; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final PlatformTransactionManager transactionManager; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ObjectMapper objectMapper; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+65
to
77
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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; | |
| } | |
| } |
Copilot
AI
Apr 8, 2026
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.
resolveRegisteredMemberListMapping()의 JSON 파싱 로직이 SheetSyncExecutor.resolveRawMapping()와 거의 동일하게 중복되어 있습니다. 두 곳이 서로 다른 방식으로 진화하면(기본값/검증 규칙 등) 동작이 쉽게 어긋날 수 있으니, 공용 유틸(예: SheetColumnMapping의 static parse/fromJson)로 추출하거나 한쪽 로직을 재사용하도록 정리하는 편이 유지보수에 유리합니다.
Copilot
AI
Apr 8, 2026
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.
예외를 잡아 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 | |
| ); |
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.
🧹 Nitpick | 🔵 Trivial
[LEVEL: low] 넓은 범위의 예외 처리가 디버깅을 어렵게 할 수 있습니다.
문제: catch (Exception e)는 JsonProcessingException, ClassCastException 외에 예상치 못한 프로그래밍 오류(NullPointerException 등)도 동일하게 처리합니다. 영향: 실제 버그가 CLUB_SHEET_ANALYSIS_REQUIRED 응답으로 은폐될 수 있습니다. 제안: 특정 예외 타입만 처리하거나, 최소한 로그를 남겨 디버깅을 돕도록 개선하세요.
🔧 개선 예시
} catch (Exception e) {
+ log.warn("Failed to parse sheet column mapping for club. json={}", mappingJson, e);
throw CustomException.of(ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED);
}🤖 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 104 - 106, Replace the broad catch (Exception e) in
SheetImportService with targeted handling: catch the expected parsing/format
exceptions (e.g., JsonProcessingException, ClassCastException) and throw
CustomException.of(ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED) for those
cases, and for any other unexpected Throwable log the error using the class
logger and rethrow or wrap it (do not map all unexpected errors to
CLUB_SHEET_ANALYSIS_REQUIRED); reference the existing CustomException.of and
ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED so the specific-case behavior is
preserved while ensuring unexpected errors are logged and propagated for proper
debugging.
Copilot
AI
Apr 9, 2026
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.
resolveRegisteredMemberListMapping()에서 sheetColumnMapping JSON 파싱 실패(예: 잘못된 JSON/예상치 못한 타입) 시 바로 CLUB_SHEET_ANALYSIS_REQUIRED로 변환되는데, CustomException은 GlobalExceptionHandler에서 별도 로깅이 없어 원인(및 clubId)을 추적하기 어렵습니다. catch 블록에서 최소한 clubId와 함께 log.warn으로 예외 원인을 남기고, 필요하면 원본 예외를 포함해 로깅해 주세요.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package gg.agit.konect.domain.club.service; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.ArgumentMatchers.anyList; | ||
| import static org.mockito.ArgumentMatchers.anySet; | ||
|
|
@@ -15,9 +16,10 @@ | |
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.InjectMocks; | ||
| import org.mockito.Mock; | ||
| import org.mockito.Spy; | ||
| import org.springframework.transaction.PlatformTransactionManager; | ||
| import org.springframework.transaction.support.SimpleTransactionStatus; | ||
|
|
||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.google.api.services.sheets.v4.Sheets; | ||
| import com.google.api.services.sheets.v4.model.ValueRange; | ||
|
|
||
|
|
@@ -33,6 +35,8 @@ | |
| import gg.agit.konect.domain.club.repository.ClubRepository; | ||
| import gg.agit.konect.domain.user.model.User; | ||
| import gg.agit.konect.domain.user.repository.UserRepository; | ||
| import gg.agit.konect.global.code.ApiResponseCode; | ||
| import gg.agit.konect.global.exception.CustomException; | ||
| import gg.agit.konect.support.ServiceTestSupport; | ||
| import gg.agit.konect.support.fixture.ClubFixture; | ||
| import gg.agit.konect.support.fixture.UniversityFixture; | ||
|
|
@@ -43,8 +47,6 @@ class SheetImportServiceTest extends ServiceTestSupport { | |
| private static final Integer CLUB_ID = 1; | ||
| private static final Integer REQUESTER_ID = 2; | ||
| private static final String SPREADSHEET_ID = "sheet-id"; | ||
| private static final String SPREADSHEET_URL = | ||
| "https://docs.google.com/spreadsheets/d/" + SPREADSHEET_ID + "/edit"; | ||
|
|
||
| @Mock | ||
| private Sheets googleSheetsService; | ||
|
|
@@ -82,23 +84,26 @@ class SheetImportServiceTest extends ServiceTestSupport { | |
| @Mock | ||
| private PlatformTransactionManager transactionManager; | ||
|
|
||
| @Spy | ||
| private ObjectMapper objectMapper = new ObjectMapper(); | ||
|
|
||
| @InjectMocks | ||
| private SheetImportService sheetImportService; | ||
|
|
||
| @Test | ||
| void previewPreMembersFromSheetReturnsDirectAndPreMembers() throws IOException { | ||
| Club club = ClubFixture.create(UniversityFixture.create()); | ||
| club.updateGoogleSheetId(SPREADSHEET_ID); | ||
| club.updateSheetColumnMapping(objectMapper.writeValueAsString( | ||
| SheetColumnMapping.defaultMapping().toMap() | ||
| )); | ||
| User directUser = UserFixture.createUser(club.getUniversity(), "Alex Kim", "2021232948"); | ||
|
|
||
| given(clubRepository.getById(CLUB_ID)).willReturn(club); | ||
| given(sheetHeaderMapper.analyzeAllSheets(SPREADSHEET_ID)).willReturn( | ||
| new SheetHeaderMapper.SheetAnalysisResult(SheetColumnMapping.defaultMapping(), null, null) | ||
| ); | ||
| given(clubRepository.getByIdWithUniversity(CLUB_ID)).willReturn(club); | ||
| given(clubMemberRepository.findStudentNumbersByClubId(CLUB_ID)).willReturn(Set.of()); | ||
| given(clubPreMemberRepository.findStudentNumberAndNameByClubId(CLUB_ID)) | ||
| .willReturn(List.<ClubPreMemberRepository.PreMemberKey>of()); | ||
| given(clubMemberRepository.findUserIdsByClubId(CLUB_ID)).willReturn(List.of()); | ||
| given(transactionManager.getTransaction(any())).willReturn(new SimpleTransactionStatus()); | ||
| given(userRepository.findAllByUniversityIdAndStudentNumberIn( | ||
| eq(club.getUniversity().getId()), | ||
| anySet() | ||
|
|
@@ -115,8 +120,7 @@ void previewPreMembersFromSheetReturnsDirectAndPreMembers() throws IOException { | |
|
|
||
| SheetImportPreviewResponse response = sheetImportService.previewPreMembersFromSheet( | ||
| CLUB_ID, | ||
| REQUESTER_ID, | ||
| SPREADSHEET_URL | ||
| REQUESTER_ID | ||
| ); | ||
|
|
||
| assertThat(response.previewCount()).isEqualTo(2); | ||
|
|
@@ -133,6 +137,35 @@ void previewPreMembersFromSheetReturnsDirectAndPreMembers() throws IOException { | |
| .containsExactly(true, true); | ||
| } | ||
|
|
||
| @Test | ||
| void previewPreMembersFromSheetThrowsWhenSheetIsNotRegistered() { | ||
| Club club = ClubFixture.create(UniversityFixture.create()); | ||
|
|
||
| given(clubRepository.getByIdWithUniversity(CLUB_ID)).willReturn(club); | ||
|
|
||
| assertThatThrownBy(() -> sheetImportService.previewPreMembersFromSheet(CLUB_ID, REQUESTER_ID)) | ||
| .isInstanceOf(CustomException.class) | ||
| .extracting(exception -> ((CustomException)exception).getErrorCode()) | ||
| .isEqualTo(ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED); | ||
|
|
||
| verifyNoInteractions(googleSheetsService, sheetHeaderMapper); | ||
| } | ||
|
|
||
| @Test | ||
| void previewPreMembersFromSheetThrowsWhenSheetMappingIsMissing() { | ||
| Club club = ClubFixture.create(UniversityFixture.create()); | ||
| club.updateGoogleSheetId(SPREADSHEET_ID); | ||
|
|
||
| given(clubRepository.getByIdWithUniversity(CLUB_ID)).willReturn(club); | ||
|
|
||
| assertThatThrownBy(() -> sheetImportService.previewPreMembersFromSheet(CLUB_ID, REQUESTER_ID)) | ||
| .isInstanceOf(CustomException.class) | ||
|
Comment on lines
+140
to
+147
|
||
| .extracting(exception -> ((CustomException)exception).getErrorCode()) | ||
| .isEqualTo(ApiResponseCode.CLUB_SHEET_ANALYSIS_REQUIRED); | ||
|
|
||
| verifyNoInteractions(googleSheetsService, sheetHeaderMapper); | ||
| } | ||
|
|
||
| @Test | ||
| void confirmImportPreMembersImportsOnlyEnabledMembers() { | ||
| Club club = ClubFixture.create(UniversityFixture.create()); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.