#106 [RESTRUCTURING] : 이벤트를 활용한 fcm push 알림 구현 #122
Conversation
| public class NotificationEventPublisher { | ||
| private final ApplicationEventPublisher eventPublisher; | ||
|
|
||
| public void publishNotificationSendEvent(NotificationSendVo notificationSendVo) { |
There was a problem hiding this comment.
P5: 해당 메서드의 인수에서 ~event 클래스를 받지 않는데 특별히 이렇게 구현하신 이유가 있을까요?
There was a problem hiding this comment.
어떤 것이 궁금하신가요??
NotificationSendVo의 이름을 NotificationSendEvent로 하지 않은 이유가 궁금하신지
아니면 NotificationEventPublisher의 구현이 궁금하신지 알려주시면 좋을 것 같습니다!
There was a problem hiding this comment.
eventPublisher는 event를 인수로 받는다고 알고 있는데 따로 event를 만들지 않고 Vo를 사용하신 이유 (재사용인지는 모르겠습니다)가 궁금합니다.
There was a problem hiding this comment.
내부 구현에 들어가면 오브젝트를 인자로 받을 수 있습니다! 원래 이름이 NotificationSendVo이였는데 변경할 생각을 못했습니다. NotificationSendEvent으로 변경해도 좋을 것 같네요!
void publishEvent(Object event);
There was a problem hiding this comment.
네 내부에서는 Object를 받지만 해당 클래스를 vo로 사용할 것인지 event로 사용할것인지 명확하게 하기 위해 클래스 이름을 변경하는 것이 좋아보입니다.
책에서 구현된 것처럼 (createdAt을 공통으로 가짐) 부모클래스를 만들 이유가 없다고 생각했습니다. 만약에 한 도메인에서 여러 이벤트가 생기는데 공통되는 필드가 필요하다면 그 도메인을 위한 부모클래스를 만들어서 사용하면 좋을 것 같습니다.
Events의 역할을 NotificationEventPublisher가 하고 있습니다!
package com.wakeUpTogetUp.togetUp.api.notification;
import com.wakeUpTogetUp.togetUp.api.notification.vo.NotificationSendVo;
import lombok.RequiredArgsConstructor;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.stereotype.Component;
@Component
@RequiredArgsConstructor
public class NotificationEventPublisher {
private final ApplicationEventPublisher eventPublisher;
public void publishNotificationSendEvent(NotificationSendVo notificationSendVo) {
eventPublisher.publishEvent(notificationSendVo);
}
}public class Events {
private static ApplicationEventPublisher publisher;
static void setPublisher(ApplicationEventPublisher publisher) {
Events.publisher = publisher;
}
public static void raise(Object event) {
if (publisher != null) {
publisher.publishEvent(event);//이벤트 발생
}
}
}fcmToken에 대해서는 FcmTokenEventPublisher 를 따로 만들이유가 없다고 생각하여 fcmService에 바로 |
제 개인적인 생각으로는 이벤트에 공통 필드로 현재 시간을 저장하는 것이 좋을 것이라고 생각이 드는데요, 그 이유는 이벤트는 비동기로 처리되기 때문에 성공하지 못해도 클라이언트에는 성공했다는 응답이 갈텐데, 그래서 이벤트가 실패할 경우에 발생시킬 custom exception을 만들고, 해당 exception에 맞는 조치를 취할 수 있도록 처리하는 것도 좋을 것 같습니다.
책에서는 ApplicationEventPublisher 를 인스턴스 변수로 가진 하나의 Events 클래스를 정의하고 따로 다른 publisher를 정의하고 있지 않은데요, 사실 Events 내부의 필드와 메서드들은 이벤트를 출판하는 공통의 코드고, 인수로 들어오는 event만 달라지기 때문에 (그래서 Object로 선언) 각 서비스마다 publisher와 handler를 따로 선언하는 것이 아니라, 책에서처럼 공통으로 처리하는 ApplicationEventPublisher를 주입받는 Events 클래스를 두고, handler만 각 상황에 맞게 정의하는 것이 좋아 보이는데 어떻게 생각하시나요? |
이벤트가 발생한 시간을 정확히 알아야한다는 것에 동의합니다. 그리고 custom exception을 만들어서 어떤 exception에 대응하면 좋을까요?? 어떤 조치를 취할 수 있는지 궁금합니다! log.warn("[PushException][{}]{} {}",
token,
errorCode,
errorMessage);
책에 있는 코드를 바로 가져오기 보다는 실제로 어떻게 적용되고 있나 다른 레포를 참고했습니다. 아니면 각 서비스마다 각 서비스마다 publiser를 두지 않고 이벤트를 발행하는 서비스에 ApplicationEventPublisher를 선언하는 것은 어떨까요??(Event패키지를 따로 만들지 않고 각 도메인에 핸들러를 두면 좋을 것 같습니다. ) |
Event 부모 클래스 구현에 대한 것은 그 필요성이 크지 않다고 생각하지만, 아래 메서드 구현 시 Object 대신에 부모 클래스를 인수로 넣어주도록 하여 event만 들어갈 수 있게 강제할 수 있을 것 같습니다. public static void raise(Object event) {
if (publisher != null) {
publisher.publishEvent(event);//이벤트 발생
}
}살펴보니 현재 로그가 발생 시간까지 찍히도록 세팅되어 있어 현재로써 생각하기에는 이벤트의 부모 클래스는 필수적이진 않은 것 같습니다!
현재는 이벤트에서 에러가 발생했을 때 위에서 코드 예제를 가져와주신 것처럼 log.warn으로 찍고 있는데, 클린 코드에서 예외 처리와 로직을 분리하라고 한 것처럼 두 코드를 분리할 필요성이 있어보입니다.
책의 코드를 바로 가져오지 않고 참고하신 것은 좋은 습관이라고 생각합니다. 그렇지만 책에서 구현한 방법도 그 나름의 이유도 있다고 생각하는데, 여기서 ApplicationEventPublisher를 이벤트를 사용하는 서비스마다 매번 선언하는 것은 코드 중복을 늘린다고 생각합니다. 공통의 클래스에 한번에 정의하고 static 메서드를 구현하여 이벤트 발생할 필요가 있는 곳마다 사용한다면 편리할 것 같습니다. 또한 개인적으로 책의 Events라는 클래스명에 공감이 잘 안가는데, EventPublisher 등의 이름으로 선언하면 좋을 것 같은데 이에 대해 어떻게 생각하시나요? |
인수 통일 만을 위해 안티패턴인 상속을 사용하는 것은 장점보다 단점이 많을 것 같습니다. 위에 리뷰로 남겨주신 Object로 반영하겠습니다!
클린코드는 소프트웨어 전반적인 이야기를 다루기 때문에 전체적인 상황을 고려하여 종합적으로 설계하면 좋을 것 같습니다. MessagingErrorCode errorCode = responses.get(idx).getException().getMessagingErrorCode();
ntStream.range(0, responses.size())
.filter(idx -> !responses.get(idx).isSuccessful())
.forEach(idx -> {
//로직 생략
//각각의 에러코드 로그 남기기 로직 등
});토큰 배열을 각각 조회해서 에러코드를 가져옵니다. 따라서 토큰마다 에러 이유가 다를 수 있습니다. public enum MessagingErrorCode {
/**
* APNs 인증서 또는 웹 푸시 인증 키가 잘못되었거나 누락되었습니다.
*/
THIRD_PARTY_AUTH_ERROR,
/**
* 요청에 지정된 하나 이상의 인수가 잘못되었습니다.
*/
INVALID_ARGUMENT,
/**
* 내부 서버 오류입니다.
*/
INTERNAL,
/**
* 메시지 대상에 대한 발송 제한을 초과했습니다.
*/
QUOTA_EXCEEDED,
/**
* 인증된 발신자 ID가 등록 토큰의 발신자 ID와 다릅니다.
*/
SENDER_ID_MISMATCH,
/**
* 클라우드 메시징 서비스가 일시적으로 사용 불가능합니다.
*/
UNAVAILABLE,
/**
* 앱 인스턴스가 FCM에서 등록 해제되었습니다. 이는 사용된 토큰이 더 이상 유효하지 않고 새로운 토큰을 사용해야 함을 의미합니다.
*/
UNREGISTERED,
}그리고 현재 로직을 보면 유효하지 않은 토큰을 모아서 삭제하는 이벤트를 발행하고 있습니다. push말고 에러처리를 해야하는 다른 중요한 이벤트가 있다면 찬미님이 말씀하신 방법을 이용하면 좋을 것 같습니다!
좋습니다! EventPublisher위치를 어디에 두면 좋을지 의견 부탁드립니다. 저는 common, event패키지가 생각나네요! |
|
리뷰 반영했습니다. |
아하 제가 로직 측면에서 이해도가 낮았던 것 같습니다. 사실 custom exception을 발생시켜 global exception handler에 에러 토큰 리스트와 함께 보내고 로그로 찍은 다음 invalid token을 삭제하려고 했는데 너무 비즈니스 로직이 섞이는 것 같아서 단념했습니다.
공통으로 사용하는 코드이기도 해서 common/event가 위치가 좋아보이는 것 같습니다! 이것저것 찾아본 결과 domain 수정 등의 로직과 관련있다면 domain, 응용 계층의 로직을 사용한다면 application, 인프라 기반의 외부 서비스 통신 등을 사용한다면 infra에 위치 시키는 등 유연한 대처가 가능할 것 같습니다. 추가) 아니면 common/event/handler 에 위치시키는 것도 괜찮을 것 같습니다! |
좋아요 event패키지에 넣었는데 common으로 옮기겠습니다!
후자입니다!
저도 이 배치가 좋은 것 같아요. domain의 행위로 일어나면 domain 안에 그 나머지는 infra에 위치하게 하는 것은 어떨까요? |
☀️ 작업 사항
☀️ 관련 이슈
resolved : #106
☀️ 참고 사항
fcm 전송실패에는 아래와 같은 이유들이 있습니다. (코드 참고)
UNREGISTERED 한 토큰만 삭제를 하게 구현하였습니다.
나머지 토큰들은 잘못된 토큰으로 인해 발생하는 것이 아닐 수도 있기 때문에 삭제 전에 조사 및 수정 시도가 필요하다고 생각하여 로그로 남기고 삭제하지 않았습니다.