-
Notifications
You must be signed in to change notification settings - Fork 24
[김동욱] sprint7 #201
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
base: 김동욱
Are you sure you want to change the base?
[김동욱] sprint7 #201
Conversation
bladnoch
commented
Jun 29, 2025
+ 로그백 설정 파일 추가 + 로그백 요구사항에 맞게 설정 + aop로 user, message, channel CUD사용시 진입점에서 info 로그 추가되게 업데이트 + 로컬 스토리지 로그 추가
+ 커스텀 에러코드 추가 + 커스텀 예외 4개 추가 + 비즈니스 로직에 커스텀 예외 적용 + 예외 dto 추가
+ 모든 request dto에 유효성검사를 위한 어노테이션 추가 + 전역 익셉션에 valid 에러를 위한 응답 추가
+ message를 위한 익셉션, 에러 코드 추가
+ user, channel, message repository slice 테스트 추가 + sql 파일 추가 + slice 테스트용 yaml 추가 +
+ test channel 추가 + test message 추가 + test user 작성중 + @EnableJpaAuditing test환경에서 분리
+ 쿼리 SQL 추가 + controller 테스트 코드 추가
+ user, message, channel 통합테스트 추가 + 단위테스트 문법 수정 verify() -> then(), should() + 일부 엔드포인트에 파라미터 확인 어노테이션 추가 + MDC추가 logback 수정
String className = joinPoint.getTarget().getClass().getName(); | ||
String methodName = joinPoint.getSignature().getName(); | ||
Object[] args = joinPoint.getArgs(); | ||
log.info("Service method started " + className + ".\n" + methodName + "(" + Arrays.toString(args) + ")"); |
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.
private static final Logger log 로 로거를 따로 정의하는 방법대신 @slf4j 애노테이션을 사용해서 logging을 적용해보면 어떨까요?
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.
AOP를 이용해서 중복되는 로깅 로직들을 잘 분리해주셨습니다!
다만 서비스별로 디버깅에 필요한 필드들이 달라질 수 있으니 이점을 참고해서 추가로 필요한 필드들이 있는 메소드들에는 로깅을 추가로 남겨보면 어떨까요?
super(errorCode.getMessage()); | ||
this.timestamp = Instant.now(); | ||
this.errorCode = errorCode; | ||
this.details = details; |
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.
details 객체를 그대로 할당하면 어떤 문제가 생길 수 있을까요?
문제점
1. 불변성(Immutable) 보장 실패
details
가 외부에서 전달된Map<String, Object>
타입 그대로 할당되고 있습니다.- 만약 외부에서 전달된
details
맵이 이후에 변경된다면,DiscodeitException
객체 내부의details
도 함께 변경됩니다. - 이는 예외 객체의 상태가 예기치 않게 변할 수 있음을 의미하며, 예외 객체는 생성 이후 상태가 변하지 않는 것이 바람직합니다.
2. 캡슐화 위반
- 외부에서 전달된 객체를 그대로 참조하므로, 예외 객체의 내부 상태가 외부에 노출됩니다.
개선 방법
1. 방어적 복사(Defensive Copy)
- 생성자에서 전달받은 맵을 새로운 맵으로 복사하여 할당하면, 외부 변경으로부터 내부 상태를 보호할 수 있습니다.
public DiscodeitException(ErrorCode errorCode, Map<String, Object> details) {
super(errorCode.getMessage());
this.errorCode = errorCode;
this.details = (details == null) ? null : new HashMap<>(details);
}
2. 불변 맵 사용
- Java 9 이상이라면
Map.copyOf()
를 사용할 수도 있습니다.
this.details = (details == null) ? null : Map.copyOf(details);
결론
- 예외 객체의 내부 상태가 외부에 의해 변경될 수 있으므로, 방어적 복사를 통해 불변성을 보장하는 것이 좋습니다.
- 특히 예외 객체는 로그나 디버깅 용도로 많이 사용되므로, 상태가 변하지 않도록 주의해야 합니다.
src/main/java/com/sprint/mission/discodeit/exception/ChannelException.java
Show resolved
Hide resolved
List<ReadStatus> readStatuses = new ArrayList<>(); | ||
|
||
//0테스트코드 작성 끝나면 리팩토링: readStatusRepository.saveAll(readStatuses); |
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.
향후 작업을 위해 주석을 남기신 거라면 TODO
주석을 남겨보셔도 좋을 것 같습니다~!
List<Channel> privateChannels = readStatuses.stream().map(readStatus -> readStatus.getChannel()).toList(); | ||
|
||
// 모든 방 순회 | ||
for (Channel channel : readStatuses.stream().map(readStatus -> readStatus.getChannel()).toList()) { | ||
if (readStatuses.stream().anyMatch(status -> status.getChannel().getId().equals(channel.getId()))) { | ||
if(!channelIds.contains(channel.getId())) { | ||
responses.add(channelMapper.toDto(channel)); | ||
} | ||
for (Channel channel : privateChannels) { | ||
if(!channelIds.contains(channel.getId())) { | ||
responses.add(channelMapper.toDto(channel)); |
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.
이부분을 stream 연산들로 모두 합쳐보면 어떨까요?