SCRUM-116 이전 pr 리뷰 반영 #3
Conversation
Walkthrough의존성 업데이트(bcprov-jdk18on 1.78→1.80), 패키지 오타 수정(infrastucture→infrastructure), 구독 비교 메서드명 정정 및 부분 업데이트 로직 추가, 동일 엔드포인트 구독 시 자격(p256dh/auth) 갱신 처리 도입, 기본 애플리케이션 이름 설정 제거, 테스트에 test 프로필 적용. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebPushService
participant Repo as PushSubscriptionRepository
participant Sub as PushSubscription
Client->>WebPushService: subscribe(request, userId)
WebPushService->>Repo: findByUserAndEndpoint(userId, request.endpoint)
alt 구독 존재
Repo-->>WebPushService: existing Sub
WebPushService->>Sub: update(request.toDomain(userId)) (p256dh, auth)
WebPushService-->>Client: existing Sub (updated)
else 구독 미존재
WebPushService->>Repo: save(new Sub from request)
Repo-->>WebPushService: saved Sub
WebPushService-->>Client: saved Sub
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
src/main/java/com/wellmeet/webpush/infrastructure/WebPushSender.java (2)
30-30: ObjectMapper 직접 생성 대신 빈 주입 권장Spring의 ObjectMapper 커스터마이징(모듈, 날짜 포맷, Kotlin/Jackson 모듈 등)을 적용하려면 new ObjectMapper() 대신 빈 주입을 사용하세요. Lombok의 @requiredargsconstructor를 이미 사용 중이므로 final 필드 주입으로 전환이 간단합니다.
아래 변경을 제안합니다:
- private final ObjectMapper objectMapper = new ObjectMapper(); + private final ObjectMapper objectMapper;
70-77: InterruptedException 처리 시 인터럽트 플래그 복원멀티캐치에서 InterruptedException을 함께 처리하면 인터럽트 플래그가 소실됩니다. 별도 catch로 분리해 플래그를 복구하세요.
- } catch (JoseException | GeneralSecurityException | IOException | ExecutionException | InterruptedException e) { - throw new WellMeetNotificationException(ErrorCode.WEB_PUSH_SEND_FAILED); - } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new WellMeetNotificationException(ErrorCode.WEB_PUSH_SEND_FAILED); + } catch (JoseException | GeneralSecurityException | IOException | ExecutionException e) { + throw new WellMeetNotificationException(ErrorCode.WEB_PUSH_SEND_FAILED); + }src/main/java/com/wellmeet/webpush/domain/PushSubscription.java (1)
13-16: (선택) 중복 구독 방지를 위한 유니크 제약 고려경쟁 상황에서 동일 (userId, endpoint) 행이 중복 생성될 가능성이 있습니다. DB 레벨 유니크 제약으로 데이터 무결성을 보장하는 것을 권장합니다.
다음과 같이 JPA 매핑과 DB 마이그레이션을 추가해 주세요.
- 엔티티 어노테이션(파일 외부 변경 코드 예시):
// import jakarta.persistence.Table; // import jakarta.persistence.UniqueConstraint; @Table(uniqueConstraints = @UniqueConstraint(columnNames = {"userId", "endpoint"})) public class PushSubscription extends BaseEntity { ... }
- DDL 마이그레이션 예시(예: Flyway)
ALTER TABLE push_subscription ADD CONSTRAINT uk_user_endpoint UNIQUE (user_id, endpoint);원하시면 레포 구조에 맞게 마이그레이션 스크립트까지 생성해 드리겠습니다.
🧹 Nitpick comments (5)
src/test/java/com/wellmeet/WellmeetNotificationApplicationTests.java (1)
5-9: 테스트 프로필 지정 추가는 적절합니다@activeprofiles("test") 추가로 테스트 격리가 명확해졌습니다. 이 클래스는 단순 컨텍스트 로딩 테스트이므로, 부트 시간 최적화를 위해 필요 시 webEnvironment를 NONE으로 지정하는 것도 고려해 보세요.
아래와 같이 변경 가능합니다:
-@SpringBootTest +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE) @ActiveProfiles("test") class WellmeetNotificationApplicationTests {src/main/java/com/wellmeet/webpush/infrastructure/WebPushSender.java (1)
35-44: BouncyCastle Provider 중복 추가 방지테스트/부트 재기동 시 Provider가 중복 추가될 수 있으므로 존재 여부 확인 후 등록하는 것이 안전합니다.
- Security.addProvider(new BouncyCastleProvider()); + if (Security.getProvider(BouncyCastleProvider.PROVIDER_NAME) == null) { + Security.addProvider(new BouncyCastleProvider()); + }src/main/java/com/wellmeet/webpush/domain/PushSubscription.java (1)
44-47: 재구독 시 자격(p256dh/auth) 갱신 로직 도입 확인update(...)로 p256dh, auth만 갱신하도록 한 방향성은 PR 요지와 일치합니다. 필요 시 재활성화를 의도한다면 active도 true로 재설정할지 논의 여지가 있습니다(현재는 unsubscribe 시 삭제를 택하므로 무관).
원하실 경우 아래처럼 active도 함께 복구할 수 있습니다:
public void update(PushSubscription updatedSubscription) { this.p256dh = updatedSubscription.p256dh; this.auth = updatedSubscription.auth; + this.active = true; }src/main/java/com/wellmeet/webpush/WebPushService.java (2)
23-24: 필드 네이밍 일관성타입은 WebPushSender인데 필드명이 pushService로 되어 있어 오해 소지가 있습니다. webPushSender 혹은 pushSender로 변경해 가독성을 높이세요.
- private final WebPushSender pushService; + private final WebPushSender webPushSender; ... - subscriptions.forEach(subscription -> pushService.send(subscription, request)); + subscriptions.forEach(subscription -> webPushSender.send(subscription, request));Also applies to: 47-47
41-48: 대량 전송 실패 처리 전략 검토sendTestPush에서 개별 전송 실패 시 전체를 중단하지 않고 예외를 퍼뜨립니다. 실무에서는 410 Gone 등으로 무효화된 구독을 정리하거나, 일부 실패를 수집/로깅하고 계속 전송하는 전략이 필요할 수 있습니다.
예: 실패 건만 모아 로깅하고, 410/404 등 특정 오류 시 해당 구독을 정리하는 후처리를 도입.
원하시면 web-push 예외 타입/상태코드에 따른 세부 처리 분기 예시를 제안드리겠습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
build.gradle(1 hunks)src/main/java/com/wellmeet/webpush/WebPushService.java(2 hunks)src/main/java/com/wellmeet/webpush/domain/PushSubscription.java(1 hunks)src/main/java/com/wellmeet/webpush/infrastructure/WebPushSender.java(1 hunks)src/main/resources/application.properties(0 hunks)src/test/java/com/wellmeet/WellmeetNotificationApplicationTests.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/resources/application.properties
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/main/java/com/wellmeet/webpush/infrastructure/WebPushSender.java (1)
src/main/java/com/wellmeet/webpush/infrastucture/WebPushSender.java (3)
send(46-51)Service(25-78)webPushSend(69-77)
src/main/java/com/wellmeet/webpush/domain/PushSubscription.java (3)
src/test/java/com/wellmeet/webpush/WebPushServiceTest.java (3)
Test(44-61)Test(28-42)Nested(25-62)src/main/java/com/wellmeet/webpush/dto/SubscribeRequest.java (2)
SubscribeRequest(15-17)SubscribeRequest(7-44)src/main/java/com/wellmeet/webpush/dto/SubscribeResponse.java (1)
SubscribeResponse(5-22)
src/test/java/com/wellmeet/WellmeetNotificationApplicationTests.java (2)
src/test/java/com/wellmeet/BaseServiceTest.java (1)
ExtendWith(9-16)src/test/java/com/wellmeet/BaseControllerTest.java (1)
ExtendWith(16-41)
src/main/java/com/wellmeet/webpush/WebPushService.java (3)
src/test/java/com/wellmeet/webpush/WebPushServiceTest.java (4)
Test(44-61)WebPushServiceTest(20-90)Nested(25-62)Test(28-42)src/test/java/com/wellmeet/webpush/WebPushControllerTest.java (2)
WebPushControllerTest(20-164)Test(25-44)src/main/java/com/wellmeet/webpush/infrastucture/WebPushSender.java (1)
Service(25-78)
🔇 Additional comments (5)
build.gradle (1)
34-34: 보안 및 호환성 검토 요약
- CVE 상태: CVE-2024-29857, CVE-2024-30172 모두 Bouncy Castle 1.78 이상(→1.80)에서 이미 해결됨.
- 의존성 현황:
build.gradle에org.bouncycastle:bcprov-jdk18on:1.80이 고정되어 있으며nl.martijndwars:web-push:5.1.1에서async-http-client를 제외(exclude)한 뒤 별도org.asynchttpclient:async-http-client:3.0.1을 사용 중 → 버전 충돌 위험은 낮으나 런타임 테스트 권장.- 잠재적 동작 변경:
- BC 1.80부터 AES-CBC IV 길이(16바이트) 검증이 강화됨. Jasypt의 NoIvGenerator 등 IV 자동생성 로직을 쓰는 경우 예외가 발생할 수 있으니, 관련 암호화 모듈이 있다면 설정(IV 생성기) 확인 및 테스트 필요.
점검 항목
- build.gradle (라인 30–34)에서 의존성 선언이 의도한 대로 동작하는지 로컬/CI 환경에서 빌드 및 런타임 검증
- 프로젝트 내 AES-CBC 사용 코드(Jasypt 설정 등)에서 16바이트 IV 요구 사항 미준수 여부 확인
- web-push 연계 기능(푸시 발송 등) 실행 시 BC 프로바이더 충돌 또는 예외 발생 여부 테스트
src/main/java/com/wellmeet/webpush/infrastructure/WebPushSender.java (1)
1-1: 패키지명 오타 수정 LGTM패키지 오타(infrastucture→infrastructure) 정정으로 import 경로 일관성이 맞춰졌습니다.
src/main/java/com/wellmeet/webpush/domain/PushSubscription.java (1)
49-51: 메서드명 정정 LGTMisSameEnpPoint → isSameEndpoint 정정으로 오탈자 해소 및 가독성 개선되었습니다.
src/main/java/com/wellmeet/webpush/WebPushService.java (2)
10-10: import 경로 수정 LGTM패키지명 정정에 맞춘 import 수정이 일관되게 반영되었습니다.
28-35: 테스트가 이미 업데이트된 p256dh/auth 값을 검증하고 있습니다.현재
WebPushServiceTest의 39–40라인에서assertThat(subscription.getP256dh()).isEqualTo(request.p256dh()); assertThat(subscription.getAuth()).isEqualTo(request.auth());로, 재구독 시 새로운
p256dh/auth값이 적용되었는지를 점검하고 있어 의도한 로직과 완전히 일치합니다.
추가로, 레포 전반에 제시된 오타(isSameEnpPoint,infrastucture)도 존재하지 않으므로 별도 수정이 필요 없습니다.Likely an incorrect or invalid review comment.
| List<PushSubscription> existingSubscriptions = pushSubscriptionRepository.findByUserId(userId); | ||
| Optional<PushSubscription> pushSubscription = existingSubscriptions.stream() | ||
| .filter(subscription -> subscription.isSameEnpPoint(request.endpoint())) | ||
| .filter(subscription -> subscription.isSameEndpoint(request.endpoint())) | ||
| .findAny(); | ||
| if (pushSubscription.isPresent()) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
쿼리 최적화: 전체 조회 후 필터링 대신 쿼리 메서드 사용
findByUserId(...)로 전체를 가져온 뒤 스트림 필터링은 불필요한 로딩입니다. Repository에 findByUserIdAndEndpoint(...)를 정의해 단건 조회하세요.
- List<PushSubscription> existingSubscriptions = pushSubscriptionRepository.findByUserId(userId);
- Optional<PushSubscription> pushSubscription = existingSubscriptions.stream()
- .filter(subscription -> subscription.isSameEndpoint(request.endpoint()))
- .findAny();
+ Optional<PushSubscription> pushSubscription =
+ pushSubscriptionRepository.findByUserIdAndEndpoint(userId, request.endpoint());Repository 인터페이스(파일 외부 변경 코드 예시):
public interface PushSubscriptionRepository extends JpaRepository<PushSubscription, Long> {
List<PushSubscription> findByUserId(String userId);
boolean existsByUserIdAndEndpoint(String userId, String endpoint);
Optional<PushSubscription> findByUserIdAndEndpoint(String userId, String endpoint); // 추가
void deleteByUserIdAndEndpoint(String userId, String endpoint);
}🤖 Prompt for AI Agents
In src/main/java/com/wellmeet/webpush/WebPushService.java around lines 27 to 31,
the code loads all subscriptions with findByUserId(...) then filters in-memory
for a matching endpoint; replace this with a repository query method to avoid
unnecessary loads: add and use
PushSubscriptionRepository.findByUserIdAndEndpoint(userId, request.endpoint())
(or existsByUserIdAndEndpoint/deleteByUserIdAndEndpoint where appropriate),
update the service to call that repository method and handle the returned
Optional (or boolean) instead of streaming the full list.
2 participants
SCRUM-116
#2
ai리뷰가 머지된 뒤에 달려서 해당 리뷰 중 반영이 필요한 내용들 반영했습니다.
Summary by CodeRabbit