[✨ feat] FCM 환경설정 및 JSON Key 관리 환경 구축 #41
Conversation
There was a problem hiding this comment.
FCM 환경설정하시느라 정말 고생 많으셨습니다!
코드를 작성하는 것뿐만 아니라, 설정 자체도 쉽지 않았을 텐데 꼼꼼하게 정리해주셔서 인상 깊었어요 🙌
몇 가지 궁금했던 부분을 중심으로 코멘트 남겨두었는데,
특히 이번 PR에서 FCM 초기화 설정이 알림 서버에 포함된 구조로 보이더라고요!
다만 저희는 클라이언트에서 FCM 토큰을 발급받고, 운영 서버에서 토큰 유효성 검사를 한 뒤 알림 서버로 전달하는 흐름으로 이해하고 있어서
이러한 구조에서 FCM SDK 초기화는 알림 서버에만 있어도 충분하다고 판단하신 걸지 궁금했습니다 🤔
혹시 운영 서버에서도 메시지를 직접 보낼 가능성을 고려하셨는지도 함께 여쭤보고 싶어요!
정말 수고 많으셨습니다 😊
편하게 확인해 주세요!
| // 환경변수에서 주입된 JSON 가져오기 | ||
| String serviceKeyJson = fcmProperties.getServiceKey(); | ||
|
|
||
| // Json 문자열 > InputStream 으로 변환 |
There was a problem hiding this comment.
serviceKeyJson이라는 변수명이 명확해서 주석 없이도 코드의 의미가 충분히 드러나는 것 같아요!
그래서 이 부분은 주석 없이도 읽는 데 무리가 없어 보이고, 오히려 중복 정보처럼 느껴질 수도 있을 것 같아요.
주석을 살짝 정리해보면 코드가 더 깔끔해질 것 같은데, 어떠신가요?
지금처럼 변수명을 잘 짓는 방향을 계속 유지해주시면, 주석 없이도 자연스럽게 읽히는 코드가 될 것 같아요! 🙌
There was a problem hiding this comment.
팀원들의 이해를 돕기 위한 의도로 주석을 작성했는데요!
흐름을 파악하는데 큰 이상이 없다면 주석은 삭제하도록 할께요 ㅎㅎ
| try (ByteArrayInputStream serviceAccountStream = | ||
| new ByteArrayInputStream(serviceKeyJson.getBytes(StandardCharsets.UTF_8))) { |
There was a problem hiding this comment.
ByteArrayInputStream으로 변환하는 과정에서도 변수명이 잘 지어져 있어서 어떤 역할을 하는지 바로 이해돼서 좋았어요! 🙌
혹시 getBytes(StandardCharsets.UTF_8) 부분은 어떤 기준으로 작성하신 걸까요?
제 생각엔 이 부분이 다른 곳에서도 반복적으로 사용될 수 있는 로직이라,
공통 인코딩 상수로 관리하거나 유틸 메서드로 추출해두면 어떨까 하는 생각이 들었어요 🤔
예를 들어 Encoding.UTF_8 같은 상수 클래스로 정리해두면,
추후 인코딩 정책이 변경될 때 한 곳에서 일괄적으로 관리할 수 있어 유지보수에 도움이 될 것 같아요!
혹시 이런 방향은 어떻게 생각하시나요?
작은 부분이지만 이런 것도 하나씩 정리되면 전체적인 코드 품질에 꽤 긍정적인 영향을 줄 수 있을 것 같아요 💪😊
There was a problem hiding this comment.
Java에서는 기본 인코딩이 운영체제에 따라 달라질 수 있어서 getBytes()를 인자 없이 호출하면 예측 불가능한 결과가 나올 수 있어요! 그레서 getBytes(Charset charset) 메서드로 문자 인코딩 방식을 명시적으로 지정해 플랫폼 종속성 없이 일관된 결과를 보장하고자 했습니다!
인코딩 방식을 상수 클래스로 변경해두는 방식도 좋은 것 같네요!! 한번 반영해볼께요!
좋은의견 감사합니다ㅎㅎ
| if (FirebaseApp.getApps().isEmpty()) { | ||
| FirebaseApp.initializeApp(options); | ||
| log.info("✅ Firebase 초기화 성공"); |
There was a problem hiding this comment.
Firebase 초기화 성공 로그 메시지 부분 잘 읽었습니다!
혹시 로그 메시지를 이렇게 하드코딩하신 이유가 있으실까요? ✨
제 개인적인 생각으로는, 이런 메시지는 상수나 Enum, 혹은 메시지 핸들러 클래스로 따로 관리하면 어떨까 싶었어요.
예를 들어 추후 로그 메시지를 통일된 포맷으로 관리하거나, 영어 처리가 필요해질 때에도 유연하게 대응할 수 있을 것 같더라고요.
또한 테스트 코드에서 메시지를 검증하거나, 운영 중 로그를 분석할 때도
일관된 메시지 관리는 꽤 유용하게 작용할 수 있다고 생각해요! 😊
예를 들어 아래처럼 사용할 수 있을 것 같아요:
log.info(FcmLogMessages.INIT_SUCCESS);혹시 이런 구조로 관리해보는 건 어떻게 생각하시나요?
지금처럼 로그도 친절하게 남겨주시는 흐름을 잘 살리면서, 유지보수성까지 챙길 수 있는 방법일 것 같아요 🙌
There was a problem hiding this comment.
그렇네요! 앞으로 로그를 띄울떄도 공통응답처리와 동일하게 처리하는 것이 좋겠네요..!
좋은 의견감사합니다:)
| } catch (IOException e) { | ||
| throw new RuntimeException("❌ Firebase 초기화 실패: " + e.getMessage(), e); |
There was a problem hiding this comment.
RuntimeException으로 바로 예외를 던지신 부분 잘 봤습니다!
혹시 이 부분은 빠르게 처리하기 위해 간단히 작성하신 걸까요?
제 생각에는 이 상황에 딱 맞는 의미 있는 커스텀 예외 클래스를 만들어 사용하는 건 어떨까 하는 생각이 들었어요 🤔
예를 들어 FcmException처럼 이름만 봐도 어떤 문제가 발생했는지 알 수 있는 예외를 만들어두면
코드 가독성은 물론이고, 예외 상황을 구체적으로 다루기에도 더 좋을 것 같아요!
또 메시지도 예외 클래스 내부에 상수로 정리해두면 아래와 같은 장점이 있을 것 같아요:
- 예외별 메시지 통일성 유지
- 예외 상황별 로깅/분석/필터링이 용이
- 테스트나 운영 대응 시 문제 파악이 빠름
예를 들어 아래처럼 작성해볼 수 있을 것 같아요:
throw new FcmException(FcmErrorCode.FAIL_INIT);이렇게 하면 예외의 의미도 분명해지고, 전체적인 예외 처리 흐름도 좀 더 견고해질 것 같아요 😊
혹시 이런 방향은 어떻게 생각하시나요? 🙌
There was a problem hiding this comment.
사실 앞서 공통응답처리와 마찬가지로 빠른 테스트를 위해 작성한 부분이라 이후 커스텀 예외처리로 추가해줘도 좋을 것 같아요!!
말씀해주신 부분으로 반영해보도록 할께요:)
| @@ -0,0 +1,44 @@ | |||
| package org.terning.fcm.config; | |||
There was a problem hiding this comment.
혹시 fcm/config 패키지를 config/fcm으로 옮기는 건 어떨까요?
지금 저희가 엔티티 구조를 user 하위에 fcm을 두고 있어서, fcm 패키지가 terning 바로 아래에 있으면 구조적으로 조금 헷갈릴 수 있을 것 같아서요.
예를 들어 fcm 관련 설정 파일은 config/fcm처럼 "설정"이라는 목적이 드러나는 위치에 두고, fcm 비즈니스 로직은 user/fcm 쪽에서 다루면 더 자연스럽게 역할이 구분될 것 같아요.
또 fcm 관련 API를 구현해야 할 때, fcm이라는 패키지가 이미 존재하면 "API도 거기다 둬야 하나?" 하고 헷갈릴 수 있을 것 같더라고요. 실제로는 user 하위에서 다뤄야 하는 로직인데, 네이밍 때문에 오해가 생길 여지가 있을 것 같아요.
그래서 FcmConfig, FcmProperties 같은 설정 관련 클래스들은 config/fcm으로 옮겨서 역할과 관심사를 조금 더 명확하게 분리해보면 어떨까 싶습니다! 🙌
There was a problem hiding this comment.
말씀해주신 부분 명확하게 이해했습니다! 저는 처음에 fcm은 하나의 주된 기능이라 생각이 들어서 이를 기준으로 fcm 관련된 로직들만 한번에 모아보고자 하는 목적에서 통합관리 하고자 했어요! fcm 관련된 로직을 user에서 찾고 구성파일은 config에서 찾는 이러한 구조가 아직 익숙하지가 않네요 ㅎㅎ..
이번에 한번 말씀해주신 방향대로 반영해보겠습니다 :)
|
@jsoonworld |
빠르게 방향 공유해주셔서 감사합니다! 🙏 운영서버에서 FCM 설정파일을 관리하지 않아도 되는 점도 확실히 유지보수 측면에서 이점이 있겠네요 :) |
⚙️ ISSUE
📄 Work Description
build.gradle에 Firebase Admin SDK 의존성을 추가했습니다.FcmProperties클래스를 생성했습니다.Firebase Admin SDK를 사용하려면 반드시 이전에FirebaseApp이 초기화되어 있어야 하는데요,@PostConstruct를 통해 해당 빈이 생성된 후, 어플리케이션이 실제로 동작하기 전에 초기화코드를 실행해 Firebase와의 서비스 통신을 위한 인증 정보 및 설정을 구성하도록FcmConfig클래스를 추가했습니다.📷 Screenshot
서버 실행 시 @PostConstruct로 FirebaseApp 초기화
✅ PR check list