[Feat/#7] ApiGatewayHandler 구현 #23
Conversation
- 관련 ErrorMessage 추가
- sns 구독, endpoint ARN 등록 등
ApiGatewayHandler 클래스명과 함수명 수정
Summary by CodeRabbit릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAPI Gateway용 새 Lambda 핸들러와 EndpointFacade를 도입해 디바이스 토큰 등록/취소/푸시 전송 흐름을 통합하고, InvalidEndpointCleaner를 제거하며 관련 서비스·유틸·환경 설정을 확장했습니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant ApiGW as ApiGatewayHandler
participant Endpoint as EndpointFacade
participant DeviceSvc as DeviceTokenService
participant Notif as NotificationService
participant UserSvc as UserService
participant History as HistoryService
rect rgb(200,220,255)
Note over Client,ApiGW: REGISTER 요청 흐름
Client->>ApiGW: APIGatewayProxyRequestEvent
ApiGW->>ApiGW: 요청 파싱/검증
ApiGW->>Endpoint: register(deviceToken, platform, userId)
Endpoint->>DeviceSvc: findByDeviceToken(deviceToken)
DeviceSvc-->>Endpoint: Optional(existing)
Endpoint->>Notif: registerEndpoint(deviceToken, platform, userId)
Notif-->>Endpoint: endpointArn
Endpoint->>Notif: subscribe(endpointArn)
Notif-->>Endpoint: subscriptionArn
Endpoint->>DeviceSvc: createToken(...)
Endpoint->>UserSvc: registerUser(...)
Endpoint-->>ApiGW: 완료
ApiGW->>History: save register history
ApiGW-->>Client: APIGatewayProxyResponseEvent (200)
end
rect rgb(255,220,200)
Note over Client,ApiGW: CANCEL 요청 흐름
Client->>ApiGW: APIGatewayProxyRequestEvent
ApiGW->>DeviceSvc: findTokenByDeviceTokenAndUserId(token,userId)
DeviceSvc-->>ApiGW: DeviceTokenEntity
ApiGW->>Endpoint: clean(userTokenInfo)
Endpoint->>Notif: deleteEndpoint(endpointArn)
Endpoint->>Notif: unsubscribe(subscriptionArn)
Endpoint->>UserSvc: deleteUser(...)
Endpoint->>DeviceSvc: deleteToken(...)
ApiGW->>History: save cancel history
ApiGW-->>Client: APIGatewayProxyResponseEvent (200)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 8
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/com/sopt/push/config/AppFactory.java (1)
49-72: 서비스 이중 인스턴스화로 인한 리소스 낭비 및 잠재적 불일치서비스들이 두 번 생성되고 있습니다:
- 로컬 변수 (lines 49-55)
- 인스턴스 필드 (lines 56-62)
특히
SendPushFacade생성 시(line 72) 로컬invalidEndpointCleaner를 사용하지만,invalidEndpointCleaner()접근자는this.invalidEndpointCleaner를 반환하여 서로 다른 객체 그래프를 참조하게 됩니다.🔎 중복 제거 수정안
- HistoryService historyService = new HistoryService(historyRepository); - NotificationService notificationService = new NotificationService(snsClient, envConfig); - SnsFactory snsFactory = new SnsFactory(snsClient, envConfig); - DeviceTokenService deviceTokenService = new DeviceTokenService(tokenRepository, userRepository, snsFactory); - UserService userService = new UserService(userRepository); - InvalidEndpointCleaner invalidEndpointCleaner = - new InvalidEndpointCleaner(userService, deviceTokenService, notificationService); - this.userService = new UserService(userRepository); - this.historyService = new HistoryService(historyRepository); - this.deviceTokenService = new DeviceTokenService(tokenRepository, userRepository, snsFactory); - this.notificationService = new NotificationService(snsClient, envConfig); - this.invalidEndpointCleaner = - new InvalidEndpointCleaner( - this.userService, this.deviceTokenService, this.notificationService); + SnsFactory snsFactory = new SnsFactory(snsClient, envConfig); + this.userService = new UserService(userRepository); + this.historyService = new HistoryService(historyRepository); + this.deviceTokenService = new DeviceTokenService(tokenRepository, userRepository, snsFactory); + this.notificationService = new NotificationService(snsClient, envConfig); + this.invalidEndpointCleaner = + new InvalidEndpointCleaner( + this.userService, this.deviceTokenService, this.notificationService); this.webHookService = new WebHookService(httpClient, envConfig); this.sendPushFacade = new SendPushFacade( this.notificationService, this.webHookService, this.historyService, this.userService, this.deviceTokenService, - invalidEndpointCleaner); + this.invalidEndpointCleaner);
🧹 Nitpick comments (9)
src/main/java/com/sopt/push/dto/RegisterUserDto.java (1)
5-7:Set불변성 보장을 위한 방어적 복사 고려Java record는 필드에 대해 얕은 복사만 수행합니다. 외부에서 전달된
Set이 수정될 경우 DTO의 불변성이 깨질 수 있습니다.🔎 방어적 복사 적용 예시
public record RegisterUserDto( String deviceToken, - Set<String> userIds) {} + Set<String> userIds) { + public RegisterUserDto { + userIds = userIds != null ? Set.copyOf(userIds) : Set.of(); + } +}src/main/java/com/sopt/push/dto/RegisterHeaderDto.java (1)
13-17: 중복 유효성 검증 제거 고려
ApiGatewayHandler.extractRequest()(lines 106-109)에서 동일한 platform 필수 조건을 이미 검사하고 있어 생성자 내 유효성 검증이 중복됩니다. 핸들러에서BusinessException으로 처리하는 반면 여기서는IllegalArgumentException을 던지므로 일관성도 떨어집니다.하나의 위치에서만 검증하도록 통일하는 것을 권장합니다.
src/main/java/com/sopt/push/config/AppFactory.java (1)
24-30: 일관되지 않은 들여쓰기필드 선언(lines 24-30)과 접근자 메서드(lines 79-105)에서 4칸 들여쓰기를 사용하지만, 생성자 본문에서는 2칸 들여쓰기를 사용하고 있습니다. 코드 전체에서 일관된 들여쓰기 스타일을 적용하는 것을 권장합니다.
Also applies to: 79-105
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (1)
290-302: DeviceTokenService와 코드 중복
extractUserIdFromSk와extractDeviceTokenFromPk메서드가DeviceTokenService에 이미 존재합니다 (relevant code snippets 참조). 중복 코드를 제거하고 서비스 메서드를 재사용하거나, 공통 유틸리티 클래스로 추출하는 것을 권장합니다.src/main/java/com/sopt/push/common/ErrorMessage.java (1)
10-14: TOKEN_NOT_EXIST와 TOKEN_NOT_FOUND가 중복될 수 있습니다.Line 10의
TOKEN_NOT_EXIST와 Line 12의TOKEN_NOT_FOUND가 의미적으로 유사합니다. 두 에러 메시지의 사용 목적이 다르다면 명확하게 구분할 수 있도록 네이밍이나 메시지를 조정하는 것을 권장합니다.src/main/java/com/sopt/push/config/SnsFactory.java (3)
3-15: 사용되지 않는 import와 중복 import를 제거하세요.
- Line 5:
com.sopt.push.config.EnvConfig는 동일 패키지이므로 import가 불필요합니다.- Line 11:
DeleteEndpointRequest가 import되었으나 사용되지 않습니다.- Line 12:
SnsException이 import되었으나 사용되지 않습니다.🔎 수정 제안
import com.sopt.push.common.DeviceTokenException; import com.sopt.push.common.ErrorMessage; -import com.sopt.push.config.EnvConfig; import com.sopt.push.enums.Platform; import lombok.extern.slf4j.Slf4j; import software.amazon.awssdk.services.sns.SnsClient; import software.amazon.awssdk.services.sns.model.CreatePlatformEndpointRequest; import software.amazon.awssdk.services.sns.model.CreatePlatformEndpointResponse; -import software.amazon.awssdk.services.sns.model.DeleteEndpointRequest; -import software.amazon.awssdk.services.sns.model.SnsException; import software.amazon.awssdk.services.sns.model.SubscribeRequest; import software.amazon.awssdk.services.sns.model.SubscribeResponse; -import software.amazon.awssdk.services.sns.model.UnsubscribeRequest;
58-67: 환경 변수 접근 방식이 일관되지 않습니다.
allTopicArn은EnvConfig를 통해 가져오지만, 플랫폼 ARN은System.getenv()를 직접 호출합니다. 일관성을 위해EnvConfig에 플랫폼 ARN 관련 메서드를 추가하고 이를 활용하는 것을 권장합니다.🔎 EnvConfig를 통한 접근 제안
EnvConfig에 다음 메서드를 추가:
public String getPlatformApplicationArn(Platform platform) { String envVar = platform == Platform.IOS ? "PLATFORM_APPLICATION_iOS" : "PLATFORM_APPLICATION_ANDROID"; return System.getenv(envVar); }그리고 SnsFactory에서 활용:
+ private final EnvConfig envConfig; + public SnsFactory(SnsClient snsClient, EnvConfig envConfig) { this.snsClient = snsClient; this.allTopicArn = envConfig.getAllTopicArn(); + this.envConfig = envConfig; } private String getPlatformApplicationArn(Platform platform) { - String envVar = - platform == Platform.IOS ? PLATFORM_APPLICATION_IOS_ENV : PLATFORM_APPLICATION_ANDROID_ENV; - String value = System.getenv(envVar); + String value = envConfig.getPlatformApplicationArn(platform); if (value == null || value.isBlank()) { throw new DeviceTokenException( - ErrorMessage.PLATFORM_APP_ARN_NOT_SET, "Platform application ARN not set: " + envVar); + ErrorMessage.PLATFORM_APP_ARN_NOT_SET, "Platform application ARN not set for: " + platform); } return value; }
30-39: SNS API 호출에 대한 예외 처리가 누락되었습니다.
snsClient.subscribe()및snsClient.createPlatformEndpoint()호출 시SnsException이 발생할 수 있습니다. 네트워크 오류나 권한 문제 등 AWS SDK 예외를 적절히 처리하여 의미 있는 에러 메시지를 반환하는 것을 권장합니다.🔎 예외 처리 추가 예시
+import software.amazon.awssdk.services.sns.model.SnsException; + public SubscribeResponse subscribe(String endpointArn) { + try { SubscribeRequest request = SubscribeRequest.builder() .protocol("application") .endpoint(endpointArn) .topicArn(allTopicArn) .build(); return snsClient.subscribe(request); + } catch (SnsException e) { + log.error("SNS subscribe failed for endpoint: {}", endpointArn, e); + throw new DeviceTokenException(ErrorMessage.SNS_PUBLISH_FAILED, e.getMessage()); + } }Also applies to: 41-56
src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
65-69: Optional 대신 null 반환을 고려하세요.
findByPkAndSk가Optional을 반환하지만, 이 메서드는.orElse(null)로 null을 반환합니다. 호출자가 null 체크를 누락할 수 있으므로Optional<DeviceTokenEntity>를 반환하거나, 명확한 사유가 있다면@Nullable어노테이션을 추가하는 것을 권장합니다.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/main/java/com/sopt/push/common/DeviceTokenException.javasrc/main/java/com/sopt/push/common/ErrorMessage.javasrc/main/java/com/sopt/push/common/InvalidEndpointException.javasrc/main/java/com/sopt/push/common/PushFailException.javasrc/main/java/com/sopt/push/config/AppFactory.javasrc/main/java/com/sopt/push/config/EnvConfig.javasrc/main/java/com/sopt/push/config/SnsFactory.javasrc/main/java/com/sopt/push/dto/ApiGatewayRequestDto.javasrc/main/java/com/sopt/push/dto/DeleteTokenDto.javasrc/main/java/com/sopt/push/dto/LogCreateRequestDto.javasrc/main/java/com/sopt/push/dto/RegisterHeaderDto.javasrc/main/java/com/sopt/push/dto/RegisterUserDto.javasrc/main/java/com/sopt/push/dto/SendAllPushDto.javasrc/main/java/com/sopt/push/dto/SendPushDto.javasrc/main/java/com/sopt/push/lambda/ApiGatewayHandler.javasrc/main/java/com/sopt/push/repository/DeviceTokenRepository.javasrc/main/java/com/sopt/push/service/DeviceTokenService.javasrc/main/java/com/sopt/push/service/InvalidEndpointCleaner.javasrc/main/java/com/sopt/push/service/NotificationService.javatemplate.yaml
💤 Files with no reviewable changes (1)
- src/main/java/com/sopt/push/config/EnvConfig.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/com/sopt/push/service/NotificationService.java (1)
src/main/java/com/sopt/push/common/PushFailException.java (1)
PushFailException(3-8)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (3)
src/main/java/com/sopt/push/config/AppFactory.java (1)
AppFactory(20-106)src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
DeviceTokenService(21-175)src/main/java/com/sopt/push/service/HistoryService.java (1)
HistoryService(14-56)
src/main/java/com/sopt/push/service/DeviceTokenService.java (2)
src/main/java/com/sopt/push/repository/DeviceTokenRepository.java (1)
DeviceTokenRepository(14-48)src/main/java/com/sopt/push/repository/UserRepository.java (1)
UserRepository(11-32)
🔇 Additional comments (16)
src/main/java/com/sopt/push/common/InvalidEndpointException.java (1)
1-11: LGTM!예외 클래스 구조가 적절합니다.
endpointArn을 저장하고 원인과 함께 메시지를 전달하는 방식이 올바릅니다.src/main/java/com/sopt/push/dto/DeleteTokenDto.java (1)
5-7: LGTM!
RegisterUserDto와 동일한 구조의 DTO입니다. 동일하게Set불변성 보장을 위한 방어적 복사를 고려할 수 있습니다.src/main/java/com/sopt/push/dto/SendAllPushDto.java (1)
5-10: LGTM!푸시 알림 전송을 위한 DTO 구조가 적절합니다.
Categoryenum을 사용하여 타입 안전성을 확보했습니다.src/main/java/com/sopt/push/repository/DeviceTokenRepository.java (1)
36-40: LGTM!
queryByPk메서드 구현이 올바릅니다. 기존findByDeviceToken메서드와 일관된 패턴을 사용하고 있습니다.대량의 데이터가 예상되는 경우, DynamoDB 페이지네이션 처리를 고려해볼 수 있습니다.
src/main/java/com/sopt/push/dto/ApiGatewayRequestDto.java (1)
5-7: LGTM!API Gateway 요청을 캡슐화하는 간결한 DTO입니다.
Map<String, Object>를 사용하여 동적 JSON body를 유연하게 처리할 수 있습니다.필요시
Map.copyOf(body)를 통해 불변성을 보장할 수 있습니다.src/main/java/com/sopt/push/service/InvalidEndpointCleaner.java (1)
22-34: LGTM!
deleteToken메서드 시그니처 변경에 맞춰 인자 순서가 올바르게 수정되었습니다. SNS 엔드포인트 삭제 실패 시 로깅 후 예외를 삼키는 방식도 cleanup 로직에 적합합니다.src/main/java/com/sopt/push/service/NotificationService.java (1)
68-72: LGTM!
PushFailException으로의 일관된 예외 타입 변경이 적용되었습니다. 원인(cause)과 메시지가 적절히 래핑됩니다.src/main/java/com/sopt/push/dto/SendPushDto.java (1)
6-12: LGTM!푸시 알림 전송을 위한 DTO가 잘 정의되었습니다.
src/main/java/com/sopt/push/common/PushFailException.java (1)
3-7: LGTM!SNS 푸시 실패를 위한 예외 클래스가 깔끔하게 구현되었습니다.
src/main/java/com/sopt/push/common/DeviceTokenException.java (1)
5-24: LGTM!다양한 생성자 오버로드와
ErrorMessage연동이 잘 설계되었습니다. 예외 처리 시 에러 코드 접근이 용이합니다.src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (1)
55-86: LGTM!핸들러 메인 로직이 잘 구조화되었습니다. switch 표현식을 통한 액션 분기,
BusinessException과 일반 예외에 대한 적절한 에러 응답 처리가 좋습니다.src/main/java/com/sopt/push/common/ErrorMessage.java (2)
15-17: ARN 관련 오류에 적절한 상태 코드가 적용되었습니다.
ENDPOINT_ARN_UNDEFINED,SUBSCRIPTION_ARN_UNDEFINED,PLATFORM_APP_ARN_NOT_SET모두 서버 측 설정 문제이므로INTERNAL_SERVER_ERROR사용이 적절합니다.
21-25: 새로운 오류 메시지가 적절하게 추가되었습니다.SNS 발행 실패 및 토큰 관련 오류 처리를 위한 상수들이 올바르게 정의되었습니다.
src/main/java/com/sopt/push/service/DeviceTokenService.java (3)
27-34: 생성자가 적절하게 확장되었습니다.
UserRepository와SnsFactory의존성이 추가되어 토큰 등록 및 사용자 관리 기능을 지원합니다.
103-118: UserEntity 저장 로직이 올바르게 구현되었습니다.
UserEntity를 생성하고 저장하는 로직이DeviceTokenEntity와 대칭적으로 구현되어 있어 데이터 일관성이 보장됩니다.
143-148: 이 메서드는 실제로 사용 중입니다.
extractDeviceTokenFromPk메서드는 ApiGatewayHandler.java의 201번 줄에서 호출되고 있습니다. 제거할 필요가 없습니다.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
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/com/sopt/push/config/AppFactory.java (1)
49-73: 서비스 인스턴스가 중복 생성되고 있습니다Lines 49-56에서 로컬 변수로 서비스 인스턴스를 생성하고, Lines 57-63에서 인스턴스 필드에 새로운 인스턴스를 다시 생성하고 있습니다. 이로 인해:
- 메모리 낭비 (동일 서비스가 2번씩 생성됨)
- Line 73에서
sendPushFacade가 로컬invalidEndpointCleaner를 사용하지만,this.invalidEndpointCleaner는 다른 인스턴스를 참조 → 일관성 문제🔎 수정안: 로컬 변수 제거 및 인스턴스 필드 직접 사용
- HistoryService historyService = new HistoryService(historyRepository); - NotificationService notificationService = new NotificationService(snsClient, envConfig); - SnsFactory snsFactory = new SnsFactory(snsClient, envConfig); - DeviceTokenService deviceTokenService = - new DeviceTokenService(tokenRepository, userRepository, snsFactory); - UserService userService = new UserService(userRepository); - InvalidEndpointCleaner invalidEndpointCleaner = - new InvalidEndpointCleaner(userService, deviceTokenService, notificationService); + SnsFactory snsFactory = new SnsFactory(snsClient, envConfig); + this.userService = new UserService(userRepository); this.historyService = new HistoryService(historyRepository); this.deviceTokenService = new DeviceTokenService(tokenRepository, userRepository, snsFactory); this.notificationService = new NotificationService(snsClient, envConfig); this.invalidEndpointCleaner = new InvalidEndpointCleaner( this.userService, this.deviceTokenService, this.notificationService); this.webHookService = new WebHookService(httpClient, envConfig); this.sendPushFacade = new SendPushFacade( this.notificationService, this.webHookService, this.historyService, this.userService, this.deviceTokenService, - invalidEndpointCleaner); + this.invalidEndpointCleaner);
♻️ Duplicate comments (5)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (3)
143-145:DeviceTokenException이중 래핑 문제
DeviceTokenException이 발생할 경우 다시DeviceTokenException으로 래핑되어 원래 에러 정보가 손실됩니다.🔎 수정안
- } catch (Exception e) { - throw new DeviceTokenException(ErrorMessage.REGISTER_USER_ERROR, e.getMessage(), e); + } catch (DeviceTokenException e) { + throw e; + } catch (Exception e) { + throw new DeviceTokenException(ErrorMessage.REGISTER_USER_ERROR, e.getMessage(), e); }
155-155: 하드코딩된 "NULL" 값으로 실제 사용자 ID가 로깅되지 않음
logUserIds가Set.of("NULL")로 하드코딩되어 있어 실제 감사 로그에 의미 있는 사용자 정보가 기록되지 않습니다. Line 191의createCancelLog에서 실제userIds를 사용해야 합니다.🔎 수정안
- Set<String> logUserIds = Set.of("NULL"); // ... createCancelLog( transactionId, - logUserIds, + userIds, deviceToken,
192-194:handleCancel에서도 동일한 예외 이중 래핑 문제
handleRegister와 동일하게DeviceTokenException이 이중 래핑됩니다.🔎 수정안
- } catch (Exception e) { - throw new DeviceTokenException(ErrorMessage.DELETE_TOKEN_ERROR, e.getMessage(), e); + } catch (DeviceTokenException e) { + throw e; + } catch (Exception e) { + throw new DeviceTokenException(ErrorMessage.DELETE_TOKEN_ERROR, e.getMessage(), e); }src/main/java/com/sopt/push/service/DeviceTokenService.java (2)
76-82: 사용자 변경 시 기존 토큰/엔드포인트 정리 누락
changedUserPayload가true를 반환할 때 (사용자 변경), 새 엔드포인트 등록 전에 기존DeviceTokenEntity,UserEntity, 그리고 SNS 엔드포인트/구독이 삭제되지 않습니다. 이로 인해 DynamoDB에 고아 레코드와 SNS에 비활성 엔드포인트가 남을 수 있습니다.🔎 수정 제안
if (!existingTokens.isEmpty()) { DeviceTokenEntity existing = existingTokens.get(0); String existingUserId = extractUserIdFromSk(existing.getSk()); if (!changedUserPayload(existingUserId, actualUserId)) { return; } + // 기존 토큰과 사용자 엔트리 삭제 (SNS 정리는 InvalidEndpointCleaner 활용 권장) + deleteToken(existingUserId, deviceToken); } var endpoint = snsFactory.registerEndPoint(deviceToken, platform, userId);
129-137:tokenUserId가 null일 경우 NPE 발생 가능Line 130에서
tokenUserId.equals("unknown")호출 시tokenUserId가 null이면 NPE가 발생합니다.🔎 수정 제안
private boolean changedUserPayload(String tokenUserId, String inputUserId) { - if (inputUserId == null && tokenUserId.equals("unknown")) { + if (inputUserId == null && "unknown".equals(tokenUserId)) { return false; } - if (inputUserId != null && inputUserId.equals(tokenUserId)) { + if (Objects.equals(inputUserId, tokenUserId)) { return false; } return true; }
🧹 Nitpick comments (4)
src/main/java/com/sopt/push/dto/RegisterHeaderDto.java (1)
10-14:action이 null일 경우 NPE 발생 가능
action == Actions.REGISTER비교 시action이 null이면 정상 동작하지만, 명시적인 null 체크를 추가하면 더 안전합니다. 또한transactionId나service에 대한 유효성 검증도 고려해 보세요.🔎 선택적 개선안
public RegisterHeaderDto { + if (action == null) { + throw new IllegalArgumentException("Action is required"); + } if ((action == Actions.REGISTER || action == Actions.CANCEL) && platform == null) { throw new IllegalArgumentException("Platform is required for REGISTER and CANCEL actions"); } }src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (1)
260-272:DeviceTokenService와 중복된 유틸리티 메서드
extractUserIdFromSk와extractDeviceTokenFromPk메서드가DeviceTokenService.java에도 동일하게 존재합니다. 공통 유틸리티 클래스로 추출하거나,DeviceTokenService의 메서드를public으로 변경하여 재사용하는 것을 권장합니다.src/main/java/com/sopt/push/config/SnsFactory.java (1)
54-63: 플랫폼 기본값 처리 개선 필요
platform == Platform.IOS가 아닌 경우 무조건 ANDROID로 처리됩니다. 향후 새로운 플랫폼이 추가되면 잘못된 ARN이 반환될 수 있습니다.🔎 명시적 플랫폼 처리 제안
private String getPlatformApplicationArn(Platform platform) { - String envVar = - platform == Platform.IOS ? PLATFORM_APPLICATION_IOS_ENV : PLATFORM_APPLICATION_ANDROID_ENV; + String envVar; + switch (platform) { + case IOS -> envVar = PLATFORM_APPLICATION_IOS_ENV; + case ANDROID -> envVar = PLATFORM_APPLICATION_ANDROID_ENV; + default -> throw new DeviceTokenException( + ErrorMessage.PLATFORM_APP_ARN_NOT_SET, "Unsupported platform: " + platform); + } String value = System.getenv(envVar); if (value == null || value.isBlank()) { throw new DeviceTokenException( ErrorMessage.PLATFORM_APP_ARN_NOT_SET, "Platform application ARN not set: " + envVar); } return value; }src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
146-151: 불필요한 중복 메서드 제거 검토
extractDeviceTokenFromPk메서드가DeviceTokenService에서 호출되지 않습니다.ApiGatewayHandler에 동일한 구현이 존재하므로, 이를 공유 유틸리티로 추출하거나DeviceTokenService에서 메서드를 제거하는 것을 고려하세요.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/com/sopt/push/common/DeviceTokenException.javasrc/main/java/com/sopt/push/common/PushFailException.javasrc/main/java/com/sopt/push/config/AppFactory.javasrc/main/java/com/sopt/push/config/SnsFactory.javasrc/main/java/com/sopt/push/dto/ApiGatewayRequestDto.javasrc/main/java/com/sopt/push/dto/DeleteTokenDto.javasrc/main/java/com/sopt/push/dto/LogCreateRequestDto.javasrc/main/java/com/sopt/push/dto/RegisterHeaderDto.javasrc/main/java/com/sopt/push/dto/RegisterUserDto.javasrc/main/java/com/sopt/push/dto/SendAllPushDto.javasrc/main/java/com/sopt/push/dto/SendPushDto.javasrc/main/java/com/sopt/push/lambda/ApiGatewayHandler.javasrc/main/java/com/sopt/push/repository/DeviceTokenRepository.javasrc/main/java/com/sopt/push/service/DeviceTokenService.java
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/java/com/sopt/push/dto/ApiGatewayRequestDto.java
- src/main/java/com/sopt/push/dto/SendPushDto.java
- src/main/java/com/sopt/push/common/PushFailException.java
- src/main/java/com/sopt/push/dto/LogCreateRequestDto.java
- src/main/java/com/sopt/push/repository/DeviceTokenRepository.java
- src/main/java/com/sopt/push/dto/DeleteTokenDto.java
- src/main/java/com/sopt/push/common/DeviceTokenException.java
- src/main/java/com/sopt/push/dto/RegisterUserDto.java
- src/main/java/com/sopt/push/dto/SendAllPushDto.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/sopt/push/service/DeviceTokenService.java (2)
src/main/java/com/sopt/push/repository/DeviceTokenRepository.java (1)
DeviceTokenRepository(14-48)src/main/java/com/sopt/push/repository/UserRepository.java (1)
UserRepository(11-32)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (3)
src/main/java/com/sopt/push/config/AppFactory.java (1)
AppFactory(20-107)src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
DeviceTokenService(21-178)src/main/java/com/sopt/push/service/HistoryService.java (1)
HistoryService(14-56)
🔇 Additional comments (3)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (1)
55-87: 전체 핸들러 구조 LGTM
handleRequest메서드의 action별 라우팅 로직과 에러 처리 구조가 명확하게 구현되어 있습니다.BusinessException과 일반Exception을 구분하여 적절한 HTTP 상태 코드를 반환합니다.src/main/java/com/sopt/push/config/SnsFactory.java (1)
13-24: 구조 및 DI 설계 LGTM
SnsClient와EnvConfig를 생성자 주입받아 테스트 용이성을 확보했습니다. SNS 관련 로직을 캡슐화하여DeviceTokenService의 책임을 분리한 점이 좋습니다.src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
71-100: registerToken 핵심 로직 LGTMSNS 엔드포인트 생성 → 구독 → DynamoDB 저장 순서가 올바르게 구현되었습니다. ARN 유효성 검증도 적절합니다. 단, 위에서 언급한 기존 토큰 정리 문제만 해결되면 됩니다.
| public SubscribeResponse subscribe(String endpointArn) { | ||
| SubscribeRequest request = | ||
| SubscribeRequest.builder() | ||
| .protocol("application") | ||
| .endpoint(endpointArn) | ||
| .topicArn(allTopicArn) | ||
| .build(); | ||
|
|
||
| return snsClient.subscribe(request); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to snsFactory methods
rg -n -A5 "snsFactory\.(subscribe|registerEndPoint)" --type=javaRepository: sopt-makers/sopt-push-notification
Length of output: 1353
🏁 Script executed:
#!/bin/bash
# Also search for direct snsClient calls in SnsFactory
rg -n -B2 -A8 "snsClient\.(subscribe|createPlatformEndpoint)" --type=javaRepository: sopt-makers/sopt-push-notification
Length of output: 2074
🏁 Script executed:
#!/bin/bash
# Find the SnsFactory class and examine its methods
fd -e java -x grep -l "class SnsFactory"Repository: sopt-makers/sopt-push-notification
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Search for exception handling around SNS calls
rg -n "catch.*SnsException|try.*snsClient" --type=javaRepository: sopt-makers/sopt-push-notification
Length of output: 270
🏁 Script executed:
#!/bin/bash
# Get full context of the method containing the SNS calls in DeviceTokenService
sed -n '70,110p' src/main/java/com/sopt/push/service/DeviceTokenService.javaRepository: sopt-makers/sopt-push-notification
Length of output: 1588
🏁 Script executed:
#!/bin/bash
# Check if DeviceTokenService method has try-catch block
sed -n '70,120p' src/main/java/com/sopt/push/service/DeviceTokenService.javaRepository: sopt-makers/sopt-push-notification
Length of output: 1961
🏁 Script executed:
#!/bin/bash
# Check the full registerToken method signature and surrounding context
sed -n '70,130p' src/main/java/com/sopt/push/service/DeviceTokenService.java | head -60Repository: sopt-makers/sopt-push-notification
Length of output: 2247
🏁 Script executed:
#!/bin/bash
# Check what exceptions are declared in the registerToken method signature
grep -n "public void registerToken" src/main/java/com/sopt/push/service/DeviceTokenService.java -A 1Repository: sopt-makers/sopt-push-notification
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Verify NotificationService exception handling for context
sed -n '60,75p' src/main/java/com/sopt/push/service/NotificationService.java
sed -n '95,110p' src/main/java/com/sopt/push/service/NotificationService.javaRepository: sopt-makers/sopt-push-notification
Length of output: 1205
DeviceTokenService.registerToken()의 SNS API 예외 처리 누락
snsFactory.registerEndPoint() (84행) 및 snsFactory.subscribe() (91행) 호출이 예외 처리 없이 진행되고 있습니다. AWS SDK의 SNS API는 SnsException, EndpointDisabledException, InvalidParameterException 등을 발생시킬 수 있으며, 현재 코드는 이들 예외를 처리하지 않아 예외가 상위로 전파됩니다. NotificationService의 try-catch 예제를 참고하여 적절한 예외 처리를 추가하세요.
🤖 Prompt for AI Agents
In src/main/java/com/sopt/push/config/SnsFactory.java around lines 26 to 35, the
subscribe(...) method calls the AWS SNS client without handling SDK exceptions
(e.g. SnsException, EndpointDisabledException, InvalidParameterException); add a
try-catch around snsClient.subscribe(request) that catches SnsException (and
specific AWS exceptions if desired), logs the error with context, and either
rethrows a wrapped runtime exception or returns a controlled failure value
consistent with how NotificationService handles SNS errors; ensure the log
includes the endpointArn and exception message and preserves the original
exception as the cause when rethrowing.
seoyeonjin
left a comment
seoyeonjin
left a comment
There was a problem hiding this comment.
수고하셨습니다!
이전 작업 내용이랑 충돌나서 덮어써진 부분이 조금 있는 것 같아요 !! 그 부분 신경써서 확인해주시면 감사하겠습니당
src/main/java/com/sopt/push/repository/DeviceTokenRepository.java
Outdated
Show resolved
Hide resolved
sung-silver
left a comment
sung-silver
left a comment
There was a problem hiding this comment.
boolean expression 분리 및 상수 분리 반영 부탁드릴게요!!
- 기존 ApiGatewayHandler에서 토큰 등록하는 부분을 파사드 클래스로 분리
- 토큰 등록시 aws 엔드포인트 등록을 위한 env를 추가합니다.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (3)
170-171: 변수명이 실제 로직과 반대입니다.
isInvalidUserId변수는 실제로 userId가 유효한지(존재하는지) 확인하는 로직입니다. 변수명이 의미를 역전시키고 있어 코드 가독성을 해칩니다.🔎 수정 제안
- boolean isInvalidUserId = userIds != null && !userIds.isEmpty(); - String userId = isInvalidUserId ? userIds.iterator().next() : null; + boolean hasValidUserId = userIds != null && !userIds.isEmpty(); + String userId = hasValidUserId ? userIds.iterator().next() : null;
25-25: 와일드카드 import를 명시적 import로 변경해주세요.과거 리뷰에서 지적되었듯이, 와일드카드 import(
com.sopt.push.dto.*)는spotlessJavaCheck를 실패시킵니다. 사용하는 DTO 클래스들을 명시적으로 import해주세요.🔎 수정 제안
-import com.sopt.push.dto.*; +import com.sopt.push.dto.ApiGatewayRequestDto; +import com.sopt.push.dto.CreateHistoryDto; +import com.sopt.push.dto.RegisterHeaderDto; +import com.sopt.push.dto.RequestDeleteTokenDto; +import com.sopt.push.dto.RequestRegisterUserDto; +import com.sopt.push.dto.RequestSendAllPushMessageDto; +import com.sopt.push.dto.RequestSendPushMessageDto; +import com.sopt.push.dto.UserTokenInfoDto;
169-204: 예외 발생 시에도 성공 응답이 반환되는 치명적 버그
handleRegister와 동일한 문제입니다.catch블록(lines 202-204)이 예외를 로그만 남기고 삼키고 있어, 토큰 취소 실패 시에도 성공 응답이 반환됩니다.🔎 수정 제안
} catch (Exception e) { log.error("Failed to cancel token: {}", e.getMessage(), e); + createHistoryLog( + transactionId, + userIds, + deviceToken, + platform, + service, + NotificationStatus.FAILURE, + Actions.CANCEL); + throw new DeviceTokenException(ErrorMessage.DELETE_TOKEN_ERROR, e.getMessage(), e); }
🧹 Nitpick comments (1)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (1)
242-248: 변수명이 실제 검증 로직과 반대입니다.
isValidPlatform변수는 platform이 null이거나 blank인지 확인합니다(즉, invalid 여부를 확인). 변수명과 로직이 반대로 되어 있어 혼란을 줍니다.🔎 수정 제안
private void checkPlatform(String platformStr) { - boolean isValidPlatform = platformStr == null || platformStr.isBlank(); - if (isValidPlatform) { + boolean isInvalidPlatform = platformStr == null || platformStr.isBlank(); + if (isInvalidPlatform) { throw new BusinessException( ErrorMessage.INVALID_REQUEST, "Platform is required for REGISTER and CANCEL actions"); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (3)
src/main/java/com/sopt/push/config/AppFactory.java (1)
AppFactory(21-104)src/main/java/com/sopt/push/service/HistoryService.java (1)
HistoryService(14-56)src/main/java/com/sopt/push/service/InvalidEndpointCleaner.java (1)
Slf4j(6-35)
🔇 Additional comments (4)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (4)
64-96: 메인 핸들러 로직 구조가 적절합니다.요청 라우팅, 에러 핸들링, 응답 변환 로직이 명확하게 구성되어 있습니다.
98-127: 요청 추출 및 검증 로직이 잘 구성되어 있습니다.헤더 검증, 플랫폼 체크 메서드 분리 등 과거 리뷰 피드백이 잘 반영되었습니다.
207-240: SEND 및 SEND_ALL 핸들러가 적절합니다.예외 처리를 상위
handleRequest에 위임하는 구조가 올바르게 구현되어 있습니다.
250-308: 헬퍼 메서드들이 명확하고 적절하게 구현되어 있습니다.요청 바디 파싱, 응답 변환, 히스토리 로깅 로직이 잘 구조화되어 있습니다.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (2)
25-25: 와일드카드 import를 명시적 import로 변경해주세요.과거 리뷰에서 지적된 사항입니다. 코드 가독성과 의존성 명확화를 위해
com.sopt.push.dto.*를 개별 클래스로 명시해주세요.🔎 수정 제안
-import com.sopt.push.dto.*; +import com.sopt.push.dto.ApiGatewayRequestDto; +import com.sopt.push.dto.CreateHistoryDto; +import com.sopt.push.dto.RegisterHeaderDto; +import com.sopt.push.dto.RequestDeleteTokenDto; +import com.sopt.push.dto.RequestRegisterUserDto; +import com.sopt.push.dto.RequestSendAllPushMessageDto; +import com.sopt.push.dto.RequestSendPushMessageDto; +import com.sopt.push.dto.UserTokenInfoDto;
170-206: 변수명 혼동 및 예외 이중 래핑 문제
Line 171:
isInvalidUserId가 실제로는 "유효한 userId가 있는지"를 확인합니다. 이름과 로직이 반대입니다.Line 203-206:
catch (Exception e)블록에서DeviceTokenException을 포함한 모든 예외를 다시DeviceTokenException으로 래핑합니다. Lines 174, 180, 187에서 발생한DeviceTokenException이 이중 래핑되어 원래 에러 코드가 손실됩니다.🔎 수정 제안
try { - boolean isInvalidUserId = userIds != null && !userIds.isEmpty(); - String userId = isInvalidUserId ? userIds.iterator().next() : null; + boolean hasValidUserId = userIds != null && !userIds.isEmpty(); + String userId = hasValidUserId ? userIds.iterator().next() : null; if (userId == null) { throw new DeviceTokenException(ErrorMessage.USER_ID_REQUIRED); } // ... existing code ... + } catch (DeviceTokenException e) { + log.error("Failed to cancel token: {}", e.getMessage(), e); + throw e; } catch (Exception e) { log.error("Failed to cancel token: {}", e.getMessage(), e); throw new DeviceTokenException(ErrorMessage.DELETE_TOKEN_ERROR, e.getMessage(), e); }
🧹 Nitpick comments (4)
src/main/java/com/sopt/push/service/TokenRegisterFacade.java (2)
16-23:DeviceTokenRepository직접 의존성 검토 필요
TokenRegisterFacade가DeviceTokenService와DeviceTokenRepository를 모두 의존하고 있습니다. Line 29의deviceTokenRepository.findAllByDeviceToken()은DeviceTokenService를 통해 호출하는 것이 계층 구조상 더 적절합니다. Repository 직접 접근은 Service 레이어의 캡슐화를 깨뜨립니다.🔎 제안하는 수정안
@RequiredArgsConstructor public class TokenRegisterFacade { private final DeviceTokenService deviceTokenService; private final UserService userService; private final NotificationService notificationService; - private final DeviceTokenRepository deviceTokenRepository;
DeviceTokenService에findAllByDeviceToken메서드를 추가하여 호출하도록 변경하세요.
71-79: 잠재적 NullPointerException 위험
tokenUserId가 null일 경우 Line 72에서tokenUserId.equals("unknown")호출 시 NPE가 발생합니다. 방어적 코딩을 권장합니다.🔎 수정 제안
private boolean changedUserPayload(String tokenUserId, String inputUserId) { - if (inputUserId == null && tokenUserId.equals("unknown")) { + if (inputUserId == null && "unknown".equals(tokenUserId)) { return false; } - if (inputUserId != null && inputUserId.equals(tokenUserId)) { + if (Objects.equals(inputUserId, tokenUserId)) { return false; } return true; }src/main/java/com/sopt/push/service/DeviceTokenService.java (2)
49-53: Optional 대신 null 반환 패턴 불일치Repository의
findByPkAndSk는Optional을 반환하지만, 이 메서드는.orElse(null)로 null을 반환합니다. 호출부(ApiGatewayHandlerline 179)에서 null 체크를 해야 하므로Optional패턴이 깨집니다.Optional<DeviceTokenEntity>를 반환하여 호출부에서 명시적으로 처리하도록 하는 것을 권장합니다.🔎 수정 제안
- public DeviceTokenEntity findTokenByDeviceTokenAndUserId(String deviceToken, String userId) { + public Optional<DeviceTokenEntity> findTokenByDeviceTokenAndUserId(String deviceToken, String userId) { String tokenPk = TOKEN_PREFIX + deviceToken; String userSk = USER_PREFIX + userId; - return deviceTokenRepository.findByPkAndSk(tokenPk, userSk).orElse(null); + return deviceTokenRepository.findByPkAndSk(tokenPk, userSk); }
55-62: 단순 위임 메서드 - 의도적 설계인지 확인 필요
registerToken이createToken을 단순히 위임합니다. 퍼사드 패턴에서 명확한 API를 제공하기 위한 의도라면 괜찮지만, 향후 중복으로 인한 혼란을 방지하기 위해 둘 중 하나만 public으로 유지하는 것을 고려하세요.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.javasrc/main/java/com/sopt/push/service/DeviceTokenService.javasrc/main/java/com/sopt/push/service/TokenRegisterFacade.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (3)
src/main/java/com/sopt/push/config/AppFactory.java (1)
AppFactory(21-104)src/main/java/com/sopt/push/service/HistoryService.java (1)
HistoryService(14-56)src/main/java/com/sopt/push/service/InvalidEndpointCleaner.java (1)
Slf4j(6-35)
src/main/java/com/sopt/push/service/TokenRegisterFacade.java (1)
src/main/java/com/sopt/push/repository/DeviceTokenRepository.java (1)
DeviceTokenRepository(14-48)
src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
src/main/java/com/sopt/push/service/UserService.java (1)
RequiredArgsConstructor(17-78)
🔇 Additional comments (4)
src/main/java/com/sopt/push/service/TokenRegisterFacade.java (1)
25-65: 등록 흐름 구현이 잘 되어 있습니다.기존 토큰 체크, SNS 엔드포인트 생성, 구독, 저장의 흐름이 명확하게 구현되어 있습니다.
actualUserId를 일관되게 사용하고 있으며, ARN 검증도 적절히 수행하고 있습니다.src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
14-16: Lombok 적용으로 코드 간소화
@RequiredArgsConstructor적용으로 보일러플레이트 코드가 제거되었습니다.src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (2)
64-96: 핸들러 구조가 잘 설계되어 있습니다.액션별 분기 처리, 예외 핸들링, 응답 변환이 명확하게 분리되어 있습니다.
switch표현식을 사용한 액션 라우팅이 깔끔합니다.
280-310: 히스토리 로그 생성 구현이 적절합니다.
CreateHistoryDto의 많은 필드가 null로 설정되어 있지만, REGISTER/CANCEL 액션에 필요한 필수 필드는 올바르게 채워져 있습니다.userIds에USER_PREFIX를 추가하는 처리도 적절합니다.
| } catch (Exception e) { | ||
| log.error("Failed to register token: {}", e.getMessage(), e); | ||
| throw new DeviceTokenException(ErrorMessage.REGISTER_USER_ERROR, e.getMessage(), e); | ||
| } |
There was a problem hiding this comment.
예외 이중 래핑 문제
tokenRegisterFacade.register()가 DeviceTokenException을 던질 수 있는데, 현재 catch (Exception e) 블록이 모든 예외를 다시 DeviceTokenException으로 래핑합니다.
🔎 수정 제안
+ } catch (DeviceTokenException e) {
+ log.error("Failed to register token: {}", e.getMessage(), e);
+ throw e;
} catch (Exception e) {
log.error("Failed to register token: {}", e.getMessage(), e);
throw new DeviceTokenException(ErrorMessage.REGISTER_USER_ERROR, e.getMessage(), e);
}📝 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.
| } catch (Exception e) { | |
| log.error("Failed to register token: {}", e.getMessage(), e); | |
| throw new DeviceTokenException(ErrorMessage.REGISTER_USER_ERROR, e.getMessage(), e); | |
| } | |
| } catch (DeviceTokenException e) { | |
| log.error("Failed to register token: {}", e.getMessage(), e); | |
| throw e; | |
| } catch (Exception e) { | |
| log.error("Failed to register token: {}", e.getMessage(), e); | |
| throw new DeviceTokenException(ErrorMessage.REGISTER_USER_ERROR, e.getMessage(), e); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java around lines 152 to
155, the generic catch(Exception e) block indiscriminately wraps all exceptions
into a new DeviceTokenException causing double-wrapping when
tokenRegisterFacade.register() already throws DeviceTokenException; update the
exception handling to first catch DeviceTokenException and rethrow it as-is
(preserving original type and stacktrace), then have a subsequent
catch(Exception e) to log and wrap only other exception types into a new
DeviceTokenException with the error message and cause.
sung-silver
left a comment
sung-silver
left a comment
There was a problem hiding this comment.
정윤아 한번만 더 확인부탁해용.. ..!!! 고생많았습니다
|
|
||
| import java.util.Map; | ||
|
|
||
| public record ApiGatewayRequestDto(RegisterHeaderDto header, Map<String, Object> body) {} |
There was a problem hiding this comment.
요기 Body는 Map으로 꼭 써야하나요? 요부분 제가 잘 몰라서..ㅎㅎ
There was a problem hiding this comment.
extractRequest에서 아래 액션을 확인하기 전에 body를 파싱하는데, 어떤 타입으로 변환할지 알 수가 없어서 Map<String, Object>로 받아서, 각 핸들러(handleRegister, handleCancel 등)에서 필요한 타입으로 mapper.convertValue()를 사용해서 변환하고 있습니다 !!
switch (action) {
case REGISTER -> handleRegister(request);
case CANCEL -> handleCancel(request);
case SEND -> handleSend(request);
case SEND_ALL -> handleSendAll(request);
default -> throw new BusinessException(ErrorMessage.INVALID_REQUEST);
}
seoyeonjin
left a comment
seoyeonjin
left a comment
There was a problem hiding this comment.
정윤싀 수고하셨습니다 👍
코드 양이 많아서 리뷰도 조금 길어지는 것 같아요
단순히 제안하는 것도 많아서 모두 다 반영하지 않더라도 리소스나 우선순위 고려해서 반영해주시면 좋을 것 같습니다!
- 두 클래스를 하나의 EndpointFacade로 통합
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (3)
169-172: 변수명isInvalidUserId가 실제 로직과 반대입니다이전 리뷰에서도 지적된 사항입니다. 조건
userIds != null && !userIds.isEmpty()는 userId가 유효한지를 확인하는데, 변수명은isInvalidUserId입니다.hasValidUserId로 변경하는 것이 코드 가독성에 도움이 됩니다.🔎 변수명 수정 제안
- boolean isInvalidUserId = userIds != null && !userIds.isEmpty(); - String userId = isInvalidUserId ? userIds.iterator().next() : null; + boolean hasValidUserId = userIds != null && !userIds.isEmpty(); + String userId = hasValidUserId ? userIds.iterator().next() : null;
190-193: handleCancel에서도 동일한 예외 이중 래핑 문제
handleRegister와 마찬가지로,endpointFacade.clean()이나deviceTokenService에서DeviceTokenException이 발생하면 이중 래핑됩니다.🔎 수정 제안
+ } catch (DeviceTokenException e) { + log.error("Failed to cancel token: {}", e.getMessage(), e); + throw e; } catch (Exception e) { log.error("Failed to cancel token: {}", e.getMessage(), e); throw new DeviceTokenException(ErrorMessage.DELETE_TOKEN_ERROR, e.getMessage(), e); }
151-154: 예외 이중 래핑 문제 확인 필요이전 리뷰에서
DeviceTokenException이 발생했을 때 다시DeviceTokenException으로 래핑되는 문제가 지적되었습니다. 현재 코드도 동일한 패턴으로,endpointFacade.register()가DeviceTokenException을 던지면 이중 래핑됩니다.🔎 수정 제안
+ } catch (DeviceTokenException e) { + log.error("Failed to register token: {}", e.getMessage(), e); + throw e; } catch (Exception e) { log.error("Failed to register token: {}", e.getMessage(), e); throw new DeviceTokenException(ErrorMessage.REGISTER_USER_ERROR, e.getMessage(), e); }
🧹 Nitpick comments (2)
src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
56-63: 래퍼 메서드의 필요성을 재검토하세요.
registerToken메서드가createToken을 단순히 호출하기만 하여 추가적인 가치를 제공하지 않습니다. 호출자가createToken을 직접 사용하는 것을 고려하거나, 향후 등록 로직이 확장될 예정이라면 주석으로 의도를 명시하세요.🔎 제안하는 수정안
옵션 1: 래퍼 제거 및 createToken 직접 사용
- public void registerToken( - String userId, - String deviceToken, - String platform, - String endpointArn, - String subscriptionArn) { - createToken(userId, deviceToken, platform, endpointArn, subscriptionArn); - }옵션 2: 의도 명시 (향후 확장 예정인 경우)
+ // Facade method for token registration. Will include additional validation/logging in future. public void registerToken( String userId, String deviceToken, String platform, String endpointArn, String subscriptionArn) { createToken(userId, deviceToken, platform, endpointArn, subscriptionArn); }src/main/java/com/sopt/push/service/NotificationService.java (1)
127-142: registerEndpoint: 예외 처리 보강 권장
snsClient.createPlatformEndpoint()호출 시 발생할 수 있는SnsException에 대한 예외 처리가 없습니다.platformPush나allTopicPush메서드처럼SnsException을ExternalException으로 래핑하는 것이 일관성 있는 에러 핸들링에 도움이 될 수 있습니다.🔎 예외 처리 추가 제안
public CreatePlatformEndpointResponse registerEndpoint( String deviceToken, Platform platform, String userId) { + try { String platformApplicationArn = getPlatformApplicationArn(platform); CreatePlatformEndpointRequest.Builder requestBuilder = CreatePlatformEndpointRequest.builder() .platformApplicationArn(platformApplicationArn) .token(deviceToken); boolean isValidUserId = userId != null && !userId.isBlank(); if (isValidUserId) { requestBuilder.customUserData(userId); } CreatePlatformEndpointRequest request = requestBuilder.build(); return snsClient.createPlatformEndpoint(request); + } catch (SnsException ex) { + throw new ExternalException("SNS endpoint creation failed: " + ex.getMessage(), ex); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/sopt/push/common/Constants.javasrc/main/java/com/sopt/push/config/AppFactory.javasrc/main/java/com/sopt/push/lambda/ApiGatewayHandler.javasrc/main/java/com/sopt/push/lambda/EventBridgeHandler.javasrc/main/java/com/sopt/push/lambda/SnsHandler.javasrc/main/java/com/sopt/push/service/DeviceTokenService.javasrc/main/java/com/sopt/push/service/EndpointFacade.javasrc/main/java/com/sopt/push/service/InvalidEndpointCleaner.javasrc/main/java/com/sopt/push/service/NotificationService.javasrc/main/java/com/sopt/push/service/SendPushFacade.javasrc/main/java/com/sopt/push/util/ResponseUtil.javasrc/main/java/com/sopt/push/util/ValidationUtil.java
💤 Files with no reviewable changes (1)
- src/main/java/com/sopt/push/service/InvalidEndpointCleaner.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/sopt/push/lambda/SnsHandler.java (1)
src/main/java/com/sopt/push/config/AppFactory.java (1)
AppFactory(20-94)
src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
src/main/java/com/sopt/push/service/UserService.java (1)
RequiredArgsConstructor(17-78)
🔇 Additional comments (19)
src/main/java/com/sopt/push/util/ValidationUtil.java (1)
17-23: 메서드명 변경이 명확성을 개선합니다.
validate에서validateDto로의 메서드명 변경이 메서드의 목적을 더 명확하게 전달하며, 다른 핸들러 클래스에서 일관되게 사용되고 있습니다.src/main/java/com/sopt/push/lambda/SnsHandler.java (1)
18-18: InvalidEndpointCleaner에서 EndpointFacade로의 리팩토링이 적절합니다.엔드포인트 정리 로직이 새로운
EndpointFacade로 일관되게 마이그레이션되었으며, 기존의 에러 핸들링과 제어 흐름이 그대로 유지되고 있습니다.Also applies to: 36-36, 44-44, 130-130
src/main/java/com/sopt/push/lambda/EventBridgeHandler.java (1)
4-4: 검증 메서드 사용이 일관되게 업데이트되었습니다.
ValidationUtil.validateDto로의 변경사항이 올바르게 반영되어 있으며, 정적 임포트와 호출이 일관성 있게 처리되었습니다.Also applies to: 83-83, 104-104
src/main/java/com/sopt/push/service/DeviceTokenService.java (2)
15-15: Lombok 통합이 보일러플레이트를 제거했습니다.
@RequiredArgsConstructor적용으로 명시적 생성자 코드가 제거되어 코드가 간결해졌습니다.Also applies to: 17-17
50-54: 새로운 조회 메서드가 적절하게 추가되었습니다.토큰 조회를 위한
findTokenByDeviceTokenAndUserId와findByDeviceToken메서드가EndpointFacade의 요구사항을 지원하기 위해 적절하게 추가되었습니다.Also applies to: 73-75
src/main/java/com/sopt/push/config/AppFactory.java (1)
9-9: EndpointFacade 통합이 깔끔하게 완료되었습니다.
InvalidEndpointCleaner에서EndpointFacade로의 마이그레이션이 일관되게 수행되었으며, 의존성이 올바르게 주입되고 있습니다. 이전 리뷰 코멘트에서 지적된 중복 인스턴스 생성 문제도 해결되었습니다.Also applies to: 25-25, 53-54, 64-64, 91-93
src/main/java/com/sopt/push/common/Constants.java (1)
46-50: 새로운 상수가 API Gateway 통합을 적절히 지원합니다.헤더 키와 SNS 프로토콜을 위한 상수가 일관된 네이밍 규칙으로 추가되어, ApiGatewayHandler와 관련 DTO에서 하드코딩된 문자열 사용을 방지합니다.
Also applies to: 61-61
src/main/java/com/sopt/push/service/SendPushFacade.java (1)
29-29: EndpointFacade로의 전환이 일관되게 적용되었습니다.
InvalidEndpointCleaner에서EndpointFacade로의 교체가 올바르게 수행되었으며,InvalidEndpointException발생 시 엔드포인트 정리 로직이 그대로 유지되고 있습니다.Also applies to: 37-37, 43-43, 128-128
src/main/java/com/sopt/push/service/NotificationService.java (2)
35-44: LGTM! 필요한 ARN 값만 필드로 초기화하는 패턴 적용이전 리뷰에서 제안된 대로
EnvConfig에서 필요한 값만 내부 필드로 설정하여 사용하는 패턴이 잘 적용되었습니다.
155-157: Null platform 처리 확인 필요
platform이null인 경우androidArn이 반환됩니다. 이것이 의도된 동작인지 확인해 주세요. 명시적으로 null 체크를 추가하거나, null일 경우 예외를 던지는 것이 더 안전할 수 있습니다.src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (5)
58-65: LGTM! AppFactory를 통한 의존성 주입 패턴싱글톤
AppFactory에서 필요한 서비스들을 주입받는 구조가 깔끔합니다.
67-100: LGTM! 예외 처리 구조가 적절합니다
BusinessException과 일반Exception을 분리하여 처리하고, 적절한 HTTP 상태 코드와 에러 메시지를 반환합니다.ResponseUtil.convertToApiGatewayResponse를 사용하여 응답 생성을 통일한 것도 좋습니다.
196-229: LGTM! SEND/SEND_ALL 핸들러 구현DTO 변환, 유효성 검증, 그리고
SendPushFacade로의 위임이 깔끔하게 구현되었습니다.
243-249: switch 표현식에서 default 케이스 누락
getSuccessMessage의 switch 표현식에서default케이스가 없습니다.Actionsenum에 새로운 값이 추가되면 컴파일 에러가 발생하지만, 명시적인 처리를 위해default케이스를 추가하거나, 이미handleRequest에서 처리되므로 현재 상태로 유지해도 됩니다.
251-281: createHistoryLog: USER_PREFIX 추가 로직 확인
userIds에USER_PREFIX를 추가하여 저장하고 있습니다 (Lines 273-275). 이전 리뷰에서 prefix 관련 논의가 있었는데,EndpointFacade.clean()에서도 prefix가 포함된 userId를 사용하는지 확인이 필요합니다. 일관성 있게 처리되고 있는지 검토해 주세요.src/main/java/com/sopt/push/service/EndpointFacade.java (4)
24-35: LGTM! register 메서드의 흐름이 명확합니다기존 토큰 확인 → 정리 → SNS 엔드포인트 생성 → 구독 → 저장의 흐름이 잘 구조화되어 있습니다.
37-47: LGTM! Best-effort 클린업 패턴DB 레코드 삭제 후 SNS 리소스 정리를 try-catch로 감싸 실패해도 로깅만 하는 패턴이 적절합니다. 외부 서비스 장애가 전체 플로우를 중단시키지 않습니다.
69-79: LGTM! 엔드포인트 ARN 유효성 검증SNS 엔드포인트 생성 후 ARN이 유효한지 확인하고, 무효한 경우 명확한 에러를 던지는 패턴이 좋습니다.
92-97: LGTM! USER_PREFIX 추출 로직sort key에서 USER_PREFIX를 제거하여 순수 userId를 추출하는 헬퍼 메서드가 깔끔합니다.
| private void checkAndCleanExistingToken( | ||
| Optional<DeviceTokenEntity> existingTokenOpt, String actualUserId, String deviceToken) { | ||
| if (existingTokenOpt.isEmpty()) return; | ||
|
|
||
| DeviceTokenEntity deviceTokenEntity = existingTokenOpt.get(); | ||
| String existingUserId = extractUserId(deviceTokenEntity.getSk()); | ||
| boolean isSameUserId = actualUserId.equals(existingUserId); | ||
| if (isSameUserId) return; | ||
|
|
||
| UserTokenInfoDto existingToken = | ||
| new UserTokenInfoDto( | ||
| existingUserId, | ||
| deviceToken, | ||
| deviceTokenEntity.getEndpointArn(), | ||
| Platform.fromValue(deviceTokenEntity.getPlatform()), | ||
| deviceTokenEntity.getSubscriptionArn()); | ||
|
|
||
| clean(existingToken); | ||
| } |
There was a problem hiding this comment.
Null userId 처리 시 NPE 위험
Line 55에서 actualUserId.equals(existingUserId)를 호출할 때, actualUserId가 null이면 NullPointerException이 발생합니다. ApiGatewayHandler.handleRegister()에서 userId가 null일 수 있으므로 (Line 135), 방어적인 null 체크가 필요합니다.
🔎 수정 제안
private void checkAndCleanExistingToken(
Optional<DeviceTokenEntity> existingTokenOpt, String actualUserId, String deviceToken) {
if (existingTokenOpt.isEmpty()) return;
+ if (actualUserId == null) return;
DeviceTokenEntity deviceTokenEntity = existingTokenOpt.get();
String existingUserId = extractUserId(deviceTokenEntity.getSk());
- boolean isSameUserId = actualUserId.equals(existingUserId);
+ boolean isSameUserId = Objects.equals(actualUserId, existingUserId);
if (isSameUserId) return;Objects.equals()를 사용하려면 import 추가가 필요합니다:
import java.util.Objects;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/sopt/push/service/EndpointFacade.java around lines 49 to
67, the code uses actualUserId.equals(existingUserId) which can throw NPE if
actualUserId is null; change the comparison to a null-safe check (e.g., use
java.util.Objects.equals(actualUserId, existingUserId)) and add the import
java.util.Objects; ensure the logic remains the same (return when IDs are equal)
and run tests to verify behavior.
| public static APIGatewayProxyResponseEvent convertToApiGatewayResponse( | ||
| Map<String, Object> responseMap) { | ||
| APIGatewayProxyResponseEvent response = new APIGatewayProxyResponseEvent(); | ||
| response.setStatusCode((Integer) responseMap.get(KEY_STATUS_CODE)); | ||
| response.setBody((String) responseMap.get(KEY_BODY)); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
타입 안전성을 위한 방어 코드 추가를 권장합니다.
responseMap에서 값을 가져올 때 명시적 캐스팅이 사용되고 있어, 잘못된 타입이 전달될 경우 ClassCastException이 발생할 수 있습니다. 현재는 내부적으로 생성된 맵만 사용되지만, 향후 안전성을 위해 방어 코드 추가를 고려하세요.
🔎 제안하는 수정안
public static APIGatewayProxyResponseEvent convertToApiGatewayResponse(
Map<String, Object> responseMap) {
+ Object statusCodeObj = responseMap.get(KEY_STATUS_CODE);
+ Object bodyObj = responseMap.get(KEY_BODY);
+
+ if (!(statusCodeObj instanceof Integer) || !(bodyObj instanceof String)) {
+ throw new IllegalArgumentException("Invalid response map structure");
+ }
+
APIGatewayProxyResponseEvent response = new APIGatewayProxyResponseEvent();
- response.setStatusCode((Integer) responseMap.get(KEY_STATUS_CODE));
- response.setBody((String) responseMap.get(KEY_BODY));
+ response.setStatusCode((Integer) statusCodeObj);
+ response.setBody((String) bodyObj);
return response;
}📝 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.
| public static APIGatewayProxyResponseEvent convertToApiGatewayResponse( | |
| Map<String, Object> responseMap) { | |
| APIGatewayProxyResponseEvent response = new APIGatewayProxyResponseEvent(); | |
| response.setStatusCode((Integer) responseMap.get(KEY_STATUS_CODE)); | |
| response.setBody((String) responseMap.get(KEY_BODY)); | |
| return response; | |
| } | |
| public static APIGatewayProxyResponseEvent convertToApiGatewayResponse( | |
| Map<String, Object> responseMap) { | |
| Object statusCodeObj = responseMap.get(KEY_STATUS_CODE); | |
| Object bodyObj = responseMap.get(KEY_BODY); | |
| if (!(statusCodeObj instanceof Integer) || !(bodyObj instanceof String)) { | |
| throw new IllegalArgumentException("Invalid response map structure"); | |
| } | |
| APIGatewayProxyResponseEvent response = new APIGatewayProxyResponseEvent(); | |
| response.setStatusCode((Integer) statusCodeObj); | |
| response.setBody((String) bodyObj); | |
| return response; | |
| } |
- 동일한 메서드가 중복되어 registerToken 메서드 삭제
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/sopt/push/repository/DeviceTokenRepository.javasrc/main/java/com/sopt/push/service/DeviceTokenService.javasrc/main/java/com/sopt/push/service/EndpointFacade.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/sopt/push/repository/DeviceTokenRepository.java
- src/main/java/com/sopt/push/service/EndpointFacade.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/sopt/push/service/DeviceTokenService.java (1)
src/main/java/com/sopt/push/service/UserService.java (1)
RequiredArgsConstructor(17-78)
🔇 Additional comments (3)
src/main/java/com/sopt/push/service/DeviceTokenService.java (3)
14-17: LGTM! Lombok을 활용한 좋은 리팩토링입니다.명시적 생성자를
@RequiredArgsConstructor로 대체하여 보일러플레이트 코드를 제거했습니다.Optionalimport 추가도 적절합니다.
55-61: LGTM! Optional 패턴을 올바르게 사용했습니다.
ifPresent를 사용한 구현이 간결하고 null-safe합니다.
63-65: LGTM! 올바른 Optional 사용입니다.저장소로 단순 위임하며
Optional을 반환하는 것은 현대 Java 모범 사례에 부합합니다.
| public DeviceTokenEntity findTokenByDeviceTokenAndUserId(String deviceToken, String userId) { | ||
| String tokenPk = TOKEN_PREFIX + deviceToken; | ||
| String userSk = USER_PREFIX + userId; | ||
| return deviceTokenRepository.findByPkAndSk(tokenPk, userSk).orElse(null); | ||
| } |
There was a problem hiding this comment.
일관성을 위해 Optional<DeviceTokenEntity>를 반환하도록 변경하세요.
현재 이 메서드는 null을 반환하지만, 같은 클래스의 findByDeviceToken 메서드(Lines 63-65)는 Optional을 반환합니다. 이러한 불일치는 호출자에게 혼란을 주고 NPE 위험을 높입니다.
🔎 제안하는 수정안
- public DeviceTokenEntity findTokenByDeviceTokenAndUserId(String deviceToken, String userId) {
+ public Optional<DeviceTokenEntity> findTokenByDeviceTokenAndUserId(String deviceToken, String userId) {
String tokenPk = TOKEN_PREFIX + deviceToken;
String userSk = USER_PREFIX + userId;
- return deviceTokenRepository.findByPkAndSk(tokenPk, userSk).orElse(null);
+ return deviceTokenRepository.findByPkAndSk(tokenPk, userSk);
}📝 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.
| public DeviceTokenEntity findTokenByDeviceTokenAndUserId(String deviceToken, String userId) { | |
| String tokenPk = TOKEN_PREFIX + deviceToken; | |
| String userSk = USER_PREFIX + userId; | |
| return deviceTokenRepository.findByPkAndSk(tokenPk, userSk).orElse(null); | |
| } | |
| public Optional<DeviceTokenEntity> findTokenByDeviceTokenAndUserId(String deviceToken, String userId) { | |
| String tokenPk = TOKEN_PREFIX + deviceToken; | |
| String userSk = USER_PREFIX + userId; | |
| return deviceTokenRepository.findByPkAndSk(tokenPk, userSk); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/sopt/push/service/DeviceTokenService.java around lines 49
to 53, the method currently returns a nullable DeviceTokenEntity which is
inconsistent with the other method returning Optional; change the signature to
return Optional<DeviceTokenEntity>, return the repository result directly
(remove orElse(null)), add/import java.util.Optional, and update any callers to
handle Optional (orElse, isPresent, or map) so there are no NPEs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/sopt/push/service/NotificationService.java (1)
155-157: 플랫폼 선택 로직을 더 명확하게 개선할 수 있습니다.현재 삼항 연산자는 iOS와 Android 두 플랫폼만을 가정하고 있습니다. 향후
Platformenum에 다른 플랫폼(예: WEB, HUAWEI)이 추가될 경우, 모든 iOS가 아닌 플랫폼이androidArn을 반환하게 되어 예상치 못한 동작이 발생할 수 있습니다.🔎 switch 문을 사용한 명시적 처리 제안
private String getPlatformApplicationArn(Platform platform) { - return platform == Platform.IOS ? iosArn : androidArn; + switch (platform) { + case IOS: + return iosArn; + case ANDROID: + return androidArn; + default: + throw new IllegalArgumentException("Unsupported platform: " + platform); + } }이렇게 하면 지원하지 않는 플랫폼에 대해 명시적으로 예외가 발생하여 더 안전합니다.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/sopt/push/service/NotificationService.java
🔇 Additional comments (5)
src/main/java/com/sopt/push/service/NotificationService.java (5)
4-5: 새로운 상수 및 유틸리티 import가 적절합니다.하드코딩된 값을 상수화하고 검증 로직을 유틸리티로 분리한 것은 이전 리뷰 피드백을 잘 반영한 개선사항입니다.
18-19: 새로운 SNS 모델 import가 적절합니다.엔드포인트 등록 및 구독 기능을 위한 필수 AWS SDK 모델 import입니다.
Also applies to: 26-27
35-36: 플랫폼별 ARN 필드 추가가 적절합니다.이전 리뷰 피드백을 반영하여 필요한 환경 변수를 내부 필드로 저장하는 방식을 사용했습니다. 기존
allTopicArn패턴과 일관성 있게 구현되었습니다.Also applies to: 42-43
56-62: 변수명 통일 및 검증 유틸리티 사용이 적절합니다.이전 리뷰 피드백을 반영하여
messageFactoryDto로 변수명을 명시적으로 통일하고validateDto유틸리티를 사용한 것은 좋은 개선입니다.
96-96: 검증 메서드 업데이트가 일관성 있게 적용되었습니다.
platformPush메서드와 동일하게validateDto유틸리티를 사용하여 일관성을 유지했습니다.
| public CreatePlatformEndpointResponse registerEndpoint( | ||
| String deviceToken, Platform platform, String userId) { | ||
| String platformApplicationArn = getPlatformApplicationArn(platform); | ||
| CreatePlatformEndpointRequest.Builder requestBuilder = | ||
| CreatePlatformEndpointRequest.builder() | ||
| .platformApplicationArn(platformApplicationArn) | ||
| .token(deviceToken); | ||
| boolean isValidUserId = userId != null && !userId.isBlank(); | ||
|
|
||
| if (isValidUserId) { | ||
| requestBuilder.customUserData(userId); | ||
| } | ||
|
|
||
| CreatePlatformEndpointRequest request = requestBuilder.build(); | ||
| return snsClient.createPlatformEndpoint(request); | ||
| } |
There was a problem hiding this comment.
예외 처리 및 입력 검증이 누락되었습니다.
이 메서드는 다음과 같은 심각한 문제가 있습니다:
- 예외 처리 부재:
platformPush와allTopicPush메서드는 포괄적인 try-catch 블록으로SnsException,InvalidParameterException등을 처리하지만, 이 메서드는 예외 처리가 전혀 없습니다. - 입력 검증 부재:
deviceToken파라미터에 대한 null/empty 검증이 없습니다. - 일관성 부족: 같은 클래스 내 다른 SNS 호출 메서드들과 에러 처리 패턴이 일관되지 않습니다.
🔎 예외 처리 추가 제안
public CreatePlatformEndpointResponse registerEndpoint(
String deviceToken, Platform platform, String userId) {
+ try {
+ if (deviceToken == null || deviceToken.isBlank()) {
+ throw new IllegalArgumentException("Device token cannot be null or empty");
+ }
+
String platformApplicationArn = getPlatformApplicationArn(platform);
CreatePlatformEndpointRequest.Builder requestBuilder =
CreatePlatformEndpointRequest.builder()
.platformApplicationArn(platformApplicationArn)
.token(deviceToken);
boolean isValidUserId = userId != null && !userId.isBlank();
if (isValidUserId) {
requestBuilder.customUserData(userId);
}
CreatePlatformEndpointRequest request = requestBuilder.build();
return snsClient.createPlatformEndpoint(request);
+ } catch (InvalidParameterException ex) {
+ throw new ExternalException("Invalid parameter for endpoint registration: " + ex.getMessage(), ex);
+ } catch (SnsException ex) {
+ throw new ExternalException("SNS endpoint registration failed: " + ex.getMessage(), ex);
+ } catch (Exception ex) {
+ throw new ExternalException("Unknown error while registering endpoint: " + ex.getMessage(), ex);
+ }
}🤖 Prompt for AI Agents
In src/main/java/com/sopt/push/service/NotificationService.java around lines
127-142, the registerEndpoint method lacks input validation and exception
handling: first validate deviceToken (non-null and not blank) and fail fast
(throw IllegalArgumentException or a domain-specific exception) before building
the request; then wrap the call to snsClient.createPlatformEndpoint(...) in a
try-catch that mirrors the class pattern (catch SnsException and
InvalidParameterException), log the error with context (deviceToken, platform,
userId) and rethrow or translate to your service's domain exception to maintain
consistency with platformPush/allTopicPush error handling; ensure you only set
customUserData when userId is valid.
| public SubscribeResponse subscribe(String endpointArn) { | ||
| SubscribeRequest request = | ||
| SubscribeRequest.builder() | ||
| .protocol(SNS_PROTOCOL_APPLICATION) | ||
| .endpoint(endpointArn) | ||
| .topicArn(allTopicArn) | ||
| .build(); | ||
|
|
||
| return snsClient.subscribe(request); | ||
| } |
There was a problem hiding this comment.
예외 처리 및 입력 검증이 누락되었습니다.
registerEndpoint와 동일한 문제가 있습니다:
- 예외 처리 부재: SNS 호출에 대한 try-catch 블록이 없어
SnsException등이 처리되지 않습니다. - 입력 검증 부재:
endpointArn파라미터에 대한 null/empty 검증이 없습니다. - 일관성 부족: 클래스 내 다른 메서드들과 에러 처리 패턴이 일관되지 않습니다.
🔎 예외 처리 추가 제안
public SubscribeResponse subscribe(String endpointArn) {
+ try {
+ if (endpointArn == null || endpointArn.isBlank()) {
+ throw new IllegalArgumentException("Endpoint ARN cannot be null or empty");
+ }
+
SubscribeRequest request =
SubscribeRequest.builder()
.protocol(SNS_PROTOCOL_APPLICATION)
.endpoint(endpointArn)
.topicArn(allTopicArn)
.build();
return snsClient.subscribe(request);
+ } catch (InvalidParameterException ex) {
+ throw new ExternalException("Invalid parameter for subscription: " + ex.getMessage(), ex);
+ } catch (SnsException ex) {
+ throw new ExternalException("SNS subscription failed: " + ex.getMessage(), ex);
+ } catch (Exception ex) {
+ throw new ExternalException("Unknown error while subscribing: " + ex.getMessage(), ex);
+ }
}📝 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.
| public SubscribeResponse subscribe(String endpointArn) { | |
| SubscribeRequest request = | |
| SubscribeRequest.builder() | |
| .protocol(SNS_PROTOCOL_APPLICATION) | |
| .endpoint(endpointArn) | |
| .topicArn(allTopicArn) | |
| .build(); | |
| return snsClient.subscribe(request); | |
| } | |
| public SubscribeResponse subscribe(String endpointArn) { | |
| try { | |
| if (endpointArn == null || endpointArn.isBlank()) { | |
| throw new IllegalArgumentException("Endpoint ARN cannot be null or empty"); | |
| } | |
| SubscribeRequest request = | |
| SubscribeRequest.builder() | |
| .protocol(SNS_PROTOCOL_APPLICATION) | |
| .endpoint(endpointArn) | |
| .topicArn(allTopicArn) | |
| .build(); | |
| return snsClient.subscribe(request); | |
| } catch (InvalidParameterException ex) { | |
| throw new ExternalException("Invalid parameter for subscription: " + ex.getMessage(), ex); | |
| } catch (SnsException ex) { | |
| throw new ExternalException("SNS subscription failed: " + ex.getMessage(), ex); | |
| } catch (Exception ex) { | |
| throw new ExternalException("Unknown error while subscribing: " + ex.getMessage(), ex); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/sopt/push/service/NotificationService.java around lines 144
to 153, the subscribe method lacks input validation and exception handling; add
a null/empty check for endpointArn and throw an IllegalArgumentException (or the
same input-validation exception used elsewhere in this class) if invalid, then
wrap the snsClient.subscribe(...) call in a try-catch that catches SnsException
(and other runtime exceptions as per the class pattern), log the error using the
same logger and message format used in registerEndpoint, and rethrow or
translate the exception into the same application-level exception used by other
methods to keep error-handling consistent.
Related Issue 🚀
Work Description ✏️
ApiGatewayHandler 구현 요약
🔆 액션별 처리 내용
PR Point 📸