CSERVER-48 s3 file url 개선 - Phase 2 : Concert 도메인#629
CSERVER-48 s3 file url 개선 - Phase 2 : Concert 도메인#629
Conversation
|
Claude finished @ch1hyun's task in 1m 27s —— View job 코드 리뷰 완료
주요 변경 사항 요약S3 파일 URL 결합 로직을 Presentation 계층(Response DTO)에서 Domain Service 계층( 검토 의견1. Redis 캐싱과 URL 결합 시점 문제 (중요)파일: ConcertDetailDTO concertDetail = ConcertDetailDTO.from(concert)
.withFileUrls(concertFileService.getFileUrls(concert));
redisHandler.set(RedisKey.PERFORMANCE_CONCERTS.createKeyInfo(concertId), concertDetail);
권장 방안:
2. 캐시 조회 시 URL 결합 누락파일: Optional<ConcertDetailDTO> cachedConcert = redisHandler.get(
RedisKey.PERFORMANCE_CONCERTS.createKeyInfo(concertId));
if (cachedConcert.isPresent()) {
return cachedConcert.get();
}캐시에서 조회된 데이터는 이미 URL이 결합된 상태로 저장되어 있어 현재 로직상 문제는 없습니다. 다만 위 1번 이슈와 연관하여, 만약 3. 미사용 필드 잔존파일:
// AdminConcertDetailResponse.java:76
info.posterUrl(), // posterPath는 사용되지 않음권장 방안:
4.
|
| return ConcertPreviewInfo.builder() | ||
| .concertId(concert.getId()) | ||
| .title(concert.getTitle()) | ||
| .startAt(concert.getStartAt()) | ||
| .endAt(concert.getEndAt()) | ||
| .area(concert.getArea()) | ||
| .posterPath(concert.getPosterPath()) | ||
| .ageRating(concert.getAgeRating()) | ||
| .time(concert.getTime()) | ||
| .price(concert.getPrice()) | ||
| .address(concert.getAddress()) | ||
| .createdAt(concert.getCreatedAt()) | ||
| .updatedAt(concert.getUpdatedAt()) | ||
| .build(); | ||
| } | ||
|
|
||
| public ConcertPreviewInfo withFileUrls(ConcertFileInfo fileUrls) { | ||
| return this.toBuilder() | ||
| .posterUrl(fileUrls.posterUrl()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
위의 정적 팩토리 메서드에서 한번에 처리하지 않고 이렇게 체이닝으로 빼서 처리하신 이유가 궁금해요~
만약 클라이언트의 이미지 띄워주는 방식이 posterUrl를 바라보고 있는데, 이 메서드가 호출되지 않아서 null이 들어가면 문제가 발생할 수 있지 않을까요?
public static ConcertPreviewInfo of(Concert concert, ConcertFileInfo fileUrls) {
요런 형태는 별로라고 생각하시나요?
There was a problem hiding this comment.
@jher235 엔티티 -> 도메인 객체 변환 사이에서 외부 값 의존도에 의한 강결합을 풀고 싶었어요
엔티티 -> 도메인 = 엔티티 내부 값에 의한 도메인 객체 변환
도메인 -> withFileUrls = 외부 서비스에서 계산된 (클라이언트 종속적인 비즈니스 로직) 값을 추가
추가로 메서드 체이닝 방식으로 서순을 나열하는게 더 가독성이 좋다고 생각했습니다!
There was a problem hiding this comment.
되게 이쁘고 가독성 자체는 좋다고 생각하는데 posterUrl을 강제하지 못해서 관리 포인트가 좀 생길 것 같다는 점이 걸리네요..
데이터 전달을 위한 DTO를 생성하기 위해서 이미지가 있는 객체인지 확인하고 withFileUrls를 체킹해야 함 -> 관리포인트 증가로 이어질까봐요! 근데 이것도 좀 과한 걱정인지,,, 판단이 잘 안서네요
There was a problem hiding this comment.
이건 단순 참고만 하셔도 좋을 것 같아요!
좀 고민돼서 찾아보니까 이런 방식도 있는 것 같더라구요
@JsonSerialize(using = CdnUrlSerializer.class)
private final String posterPath;@Component
public class CdnUrlSerializer extends JsonSerializer<String> {
private final CdnFileDomainResolveService cdnResolver;
public CdnUrlSerializer(CdnFileDomainResolveService cdnResolver) {
this.cdnResolver = cdnResolver;
}
@Override
public void serialize(String value, JsonGenerator gen, SerializerProvider serializers) throws IOException {
if (value == null) {
gen.writeNull();
return;
}
gen.writeString(cdnResolver.resolve(value));
}There was a problem hiding this comment.
@jher235 그렇네요.. 생각해보니 항상 필수이어야 하는 값이 옵셔널로 제공되면서 누락될 수 있는 케이스가 걱정이긴 하네요.
제안해주신 시리얼라이저 방법도 괜찮은 것 같은데, 클로드한테 질문해보니 캐싱이 불가하고, 도메인 객체에서 외부 서비스에 의존하고 있고, 테스트가 어렵다고 하네요..
정적 팩토리 메서드를 사용하면 또 외부 데이터가 변환에 사용되기도 하고요.
아래 패턴은 어떠신가요?
class ConcertDraft {
String posterPath;
public static ConcertDraft from(Concert concert) {
return ConcertDraft.builder()
...
.build();
}
public ConcertDTO withFileUrls(ConcertFileInfo fileUrls) {
return ConcertDTO.builder()
...
.posterUrl(fileUrls.posterUrl())
.build();
}
}
class ConcertDTO {
String posterUrl;
}
There was a problem hiding this comment.
코멘트를 놓쳐서 답변이 너무 늦었네요...!
좋은 것 같아요! 살짝 복잡도가 올라가겠지만, 필수값 누락을 방지할 수 있는 좋은 타협점인 것 같습니다!!
2 participants
🔊 Summaries
✨ Notification