#39 헌혈인증 내역 조회 API 개발 #40

slolee · 2022-09-11T16:15:30Z

📌 Linked Issues

헌혈인증 내역 조회 API 개발 #39

🔎 Change Details

BloodDonationCertService (Presentation Layer 의 Service) 추가
CertificationService (Domain Layer 의 Service) 추가
인증 관련 Entity 구조 변경
FunctionalUtils 클래스 추가 → Stream 으로 바꿔서 처리하는 로직 추상화
ChatMessage 관련 Repository 추가 → CertificationService 에서 사용목적
CertificationService 테스트 코드 작성

💬 Comment

BloodDonationCertController → BloodDonationCertService → CertificationService 순으로 읽으면 편합니다.

📑 References

✅ Check Lists

추가한 기능에 대한 테스트는 모두 완료하셨나요?
코드 정렬(Ctrl + Alt + L), 불필요한 코드나 오타는 없는지 확인하셨나요?
merge할 base branch가 올바른지 확인하셨나요?

slolee · 2022-09-11T16:15:48Z

Repository 테스트 코드 추가 예정입니다.

seonpilKim

찬준님 추석 연휴 잘 보내셨나요?😊
리뷰 남겼으니 시간 되실 때 확인 부탁드립니다!

seonpilKim · 2022-09-12T15:44:40Z

module-common/src/main/java/com/zoopi/util/FunctionalUtils.java

+	public static <T, K, V> Map<K, V> associateFrom(List<T> list, Function<T, K> key, Function<T, V> value) {
+		return list.stream().collect(HashMap::new, (map, t) -> map.put(key.apply(t), value.apply(t)), HashMap::putAll);
+	}


혹시 list를 map으로 변환할 때, key가 중복되는 경우도 허용하려는 의도로 위와 같이 구현하신 걸까요??

mapFrom, filter 메소드에서는 Collectors.toList() 메소드를 사용하셨는데, associateFrom 메소드에서는 Collectors.toMap() 메소드를 사용하지 않으신 이유도 궁금합니다..!

public static <T, K, V> Map<K, V> associateFrom(List<T> list, Function<T, K> key, Function<T, V> value) { return list.stream().collect(Collectors.toMap(key, value)); }

제가 원하는 결과는 Value 가 있으면 (Key, Value) 가 나오고 없으면 (Key, null) 이 나오도록 하길 원했는데, toMap 같은 경우에 Key 에 대한 Value 가 없을 경우 NPE 가 발생하더라구요. 그래서 직접 HashMap 을 만들도록 구현했습니다 :)

Ref) https://stackoverflow.com/questions/24630963/nullpointerexception-in-collectors-tomap-with-null-entry-values

아하 그런 의도였군요 ㅎㅎ 알겠습니다. 주석으로 부연설명도 추가해 주시면 나중에 까먹더라도 기억할 수 있을테니 좋을 것 같아요!

seonpilKim · 2022-09-12T15:51:58Z

module-model/src/main/java/com/zoopi/ResultCode.java

+	// Common
+	OK(200, "-200", ""),
+


혹시 이 코드는 아직 확정되지 않아서 이렇게 작성하신 걸까요?

음.. 성공시 데이터외에 별도로 내릴만한 메세지나 상태코드가 없을것 같아서 두었습니다.

ResponseEntity<ResultResponse> 를 핸들러 반환값으로 사용하고 있는데 이렇게 쓰는 구조가 특정 상황에서 좀더 디테일한 정보를 내릴 수 있긴한것 같은데,,, 코드상에서 봤을 때 어떤 응답객체가 내려가는지 확인하기가 어려운 것 같아요.

예를 들어서 ResponseEntity<List<CertificationResponse>> 를 내릴경우 해당 핸들러에서 반환하는값이 명확히 보이는 반면 ResultResponse 만봐서는 알수가 없습니다.

제네릭으로 풀어내는 방법이 있을것 같긴하지만,, 우선 구조 잡아주신대로 진행했고 상태값은 공통으로 사용할 임의의 값을 만들었어요.

HTTP 요청이 성공해서 상태 코드가 200이더라도, 비즈니스 로직 결과는 다양하게 존재할 수 있으니, 각 비즈니스 로직의 결과에 대한 ResultCode를 유니크하게 설정해주는 것이 좋을 것 같습니다!

굳이 응답 데이터만 응답하는 게 아니라, ResultResponse로 한 번 감싸서 응답하는 이유도 위와 같습니다.
이 부분은 프론트/안드 팀이랑 이야기가 된 부분이라 ㅠㅠ.. 비즈니스 코드를 통해서 후처리를 하시기로 하셔서 유니크하게 설정해 주시면 좋을 것 같습니다.

"인증정보 조회 성공", "채팅방 목록 조회 성공" 등과 같이 세세하게 나누는게 프론트와 백에서 어떤 이점이 있는지 잘 와닿는것 같아요. 하나의 API 에 대한 성공케이스가 여러 종류가 있다면 고려해볼 수 있겠지만,,,

프론트쪽과 컴하기도 오히려 더 신경써야 할 부분이 하나 더 늘어나는 것 같고, 프론트에서 백엔드에 있는 결과 코드에 의존적인 개발을 하게되는것 같아 장점이 없는것 같아 보입니다.

위에 작성했듯이 Controller 상에서 응답 객체의 타입이 명확히 노출되지 않는 문제가 더 크게 느껴지는것 같아요..ㅠㅠ

seonpilKim · 2022-09-12T16:12:22Z

module-model/src/main/java/com/zoopi/client/certification/model/CertificationResponse.java

+	@JsonInclude(JsonInclude.Include.NON_NULL)
+	private BloodDonationDetailResponse detail;


detail이 null인 경우 json 응답이 아래와 같이 나오는게 맞을까요?

"bloodDonationHistoryId" : "...", "type" : "...", "imageUrl" : "...", "hospital" : { ... }, "bloodDonationAt" : "..."

어떤 경우에 detail을 제외한 응답이 발생하는지에 대해서 API 명세서(BloodDonationCertApi.java)에 ResultCode로 구분하여 작성해 주시면 좋을 것 같습니다🙂

detail 이 NULL 일 경우에 제외된 형태가 되는것이 맞습니다. 일반헌혈의 경우가 되겠네요. 이후에 필요하다면 일반헌혈, 지정헌혈을 따로 묶어서 내릴 생각도 하고있지만,, 이 부분은 프론트와 이야기가 되어야할듯 합니다.

Api 쪽 명세는 도메인별로 기능이 정리되는 시점에 일괄작성할게요 :)

모든 헌혈인증정보가 내려가기때문에 하나의 응답에 일반허혈, 지정헌혈 정보가 모두 들어갑니다. 따라서 특정 응답에 detail 이 있고 없고 결정되는게 아니라고 생각해주시면 될듯합니다!

seonpilKim · 2022-09-12T16:43:44Z

module-domain/src/main/java/com/zoopi/domain/chat/repository/ChatMessageRepository.java

+public interface ChatMessageRepository extends CrudRepository<ChatMessage, Long> {
+
+	@Query("select c from ChatMessage c where c.chatRoom.id = :chatRoomId and c.type = :type")
+	ChatMessage findByChatRoomIdAndType(Long chatRoomId, MessageType type);
+}


JpaRepository가 아닌 CrudRepository를 상속하신 이유가 궁금합니다!

JpaRepository를 상속받으면 아래와 같이 쿼리 메소드로 간단히 추가할 수 있지 않나요..?

Optional<ChatMessage> findByChatRoomIdAndType(Long chatRoomId, Messagetype type);

그리고 @Query에서 파라미터 변수를 사용하려면, 해당 파라미터 타입 앞에 @Param("chatRoomId")와 같이 어노테이션을 추가해야 하는 것으로 알고 있습니다!

음.. 습관적으로 JPQL 을 사용했네요..! 이 부분은 수정해볼게요!

@Param("chatRoomId") 의 경우에는 필수적인 부분인가요..? 저는 프로젝트에서 생략하는 형태로 가져왔던것 같아요. JAVA8 이후에 파라미터로 자동 바인딩하도록 구성할 수 있는듯합니다.

엇 그런가요? 최근에 개발할 때 @Param 빼먹어서 오류가 났었던 기억이 있어서 말씀드렸던건데, 다시 한 번 확인해봐야겠네요!

제가 개발했던 환경이 아니라 했다가 에러나면 반영하도록할게요! 늘 디테일있는 부분까지 신경 써주셔서 감사합니다 ㅎㅎㅎ 👍 👍 👍

seonpilKim · 2022-09-12T16:44:34Z

.../src/main/java/com/zoopi/domain/certification/repository/BloodDonationHistoryRepository.java

+@Repository
+public interface BloodDonationHistoryRepository extends CrudRepository<BloodDonationHistory, Long> {
+
+	@Query("select h from BloodDonationHistory h join fetch h.hospital join fetch h.pet where h.pet.id = :petId")
+	List<BloodDonationHistory> findAllByPetId(Long petId);
+}


위와 동일한 내용입니다!

추가)
fetch join을 이용하는 경우 메소드 네이밍에 차별점을 주는 건 어떨까요?
fetch join 없이 조회하는 메소드와 공존하는 경우도 고려하면 좋을 것 같습니다!

ex) findAllFetchPetByPetId

Fetch Join 이 발생한다는게 해당 메소드를 호출하는쪽에서 관심 가질만한 부분인지 잘 모르겠습니다.. 🤔 사용하는쪽의 입장을 생각하고 메소드이름을 짓는편이 비즈니스 로직의 가독성이 더 좋아진다고 생각해서.. 고민스러운 부분이네요!

fetch join이 필요 없는 곳에서도 해당 메소드를 사용하면 아무래도 불필요한 데이터까지 db에서 fetch 하는게 비효율적이라.. 저는 구분하는 걸 선호하는편입니다 ㅠ_ㅠ

음....🤔 이 부분은 말씀해주신대로 findAllFetchPetBy(Long petId) 정도로 정리해볼게요~

seonpilKim · 2022-09-12T17:40:54Z

module-domain/src/main/java/com/zoopi/domain/certification/service/CertificationService.java

+	@Transactional
+	public Map<BloodDonationHistory, CertDetailDto> mapHistoryAndDetail(List<BloodDonationHistory> histories) {
+		final List<Long> ids = mapFrom(histories, BloodDonationHistory::getId);
+		final List<BloodDonationDetail> details = detailRepository.findByHistoryIdsIn(ids);
+		final Map<Long, CertDetailDto> certDtoMap =
+			associateFrom(details, detail -> detail.getHistory().getId(), this::makeCertDetailDto);
+
+		return associateFrom(histories, history -> history, history -> certDtoMap.get(history.getId()));
+	}


해당 메소드에 @Transactional을 추가하신 이유가 궁금합니다!
변경 감지가 필요 없이 read만 하는 코드로 보여지는데, 혹시 해당 어노테이션이 다른 이유로도 필요한 걸까요?

readOnly 옵션이 필요하겠네요!

seonpilKim · 2022-09-12T17:47:40Z

module-domain/src/main/java/com/zoopi/domain/certification/entity/BloodDonationDetail.java

 @Entity
 @NoArgsConstructor(access = AccessLevel.PROTECTED)
+@AllArgsConstructor
+@Getter
 public class BloodDonationDetail extends BaseEntity {



테스트 코드 작성 편의 목적으로 @AllArgsConstructor을 추가하신 것으로 생각되는데 맞을까요..?

개인적으로 Dto의 경우 @AllArgsConstructor나 @Setter가 있어도 큰 문제가 없다고 생각하지만,
Entity의 경우 Id 주입 시점이 DB에 row를 저장한 시점(IDENTITY)만 가능하기 때문에, @AllArgsConstructor을 열어두는 것은 좋은 코드라는 생각이 들지 않네요 ㅠ_ㅠ

@AllArgsConstructor을 열어두는 방법 대신, 테스트 코드에서 ReflectionTestUtils을 이용하여 Id를 주입해 주는 방식은 어떠실까요..?

말씀해주신 내용에 대해서는 충분히 이해가 가는 내용입니다! 엄격하게 따졌을 때 굉장히 중요한 내용인것 같고, 잘못 개발 되었을 때 발생할 수 있는 문제를 잘 잡아주신것 같아요!

개인적으로는 이러한 부분을 엄격하게 챙기는 것과 어느정도 개발 편의성을 가져가는 것에 트레이드 오프가 필요하다고 생각하는데요,, JPA 에 대한 이해도가 있는 상태라면 도메인 Entity 객체를 생성할 때 ID 를 넣지는 않도록 관리가 되어질수는 있을것 같다는 생각입니다!

실제로 코틀린에서는 일반적으로 data class 를 이용해 Entity 클래스를 만드는데, data class 에는 기본적으로 AllArgsArgument 생성자를 가집니다..! 그럼에도 data class 를 이용해 Entity 클래스를 만드는 이유는 이로인해 따라오는 개발 편의성이 매우 크기 때문입니다. 추가로 Reflection 은 개인적으로 별로 선호하지 않는 방법입니다!!

이 부분에 대해서는 약간의 타협점이 필요해보이네요! 🤔

음.. 저희끼리만(혹은 JPA를 잘 아는 사람들끼리) 개발 및 유지보수 한다는 가정 하에는 개발 편의성을 선택하는 것도 방법이겠군요..
그럼 이 부분은 제가 작성한 코드에도 적용하도록 하겠습니다!

그리고 혹시 Reflection을 선호하지 않으시는 이유도 여쭤봐도 될까요?

굳이 맞출필요는 없는 부분이고 선필님이 작업하시는 범위는 하시는대로 진행해주셔도 될것 같아요 :) 크게 충돌이 날만한 부분은 아닌것 같아서요!

제가 Reflection 을 선호하지 않는 이유는 컴파일시점에 타입에 대한 체크가 어려워서입니다! 물론 id 같은 경우엔 Long 타입이 거의 고정시되어있어서 크게 의미가 있는 부분은 아닌것 같지만요. 특히 테스트코드가 아니라 비즈니스 로직에서는 더 안좋은 영향을 많이 주는것 같아요.

module-domain/src/main/java/com/zoopi/domain/certification/entity/BloodDonationHistory.java

module-domain/src/main/java/com/zoopi/domain/chat/entity/ChatMessage.java

module-domain/src/main/java/com/zoopi/domain/chat/entity/ChatRoom.java

slolee added 5 commits September 3, 2022 20:40

Refactor: 인증 & 채팅 관련 Entity 최신화

7ef5ba7

Refactor: 불필요 파일 삭제

30ae33d

Refactor: referencedColumnName 오타 수정

9532e69

Feat: 헌혈인증 내역 조회 API 개발

7381a5d

Refactor: 헌혈인증 내역 조회 API 응답 상태코드 수정

43afeef

slolee added the ✏ Review request 코드 리뷰 요청 label Sep 11, 2022

slolee requested a review from seonpilKim September 11, 2022 16:15

slolee self-assigned this Sep 11, 2022

slolee added 3 commits September 12, 2022 01:39

Refactor: CertificationService 로직 리팩토링

435eb00

Refactor: CertificationService 로직 리팩토링

e2109df

Refactor: CertificationService 로직 리팩토링

1a2670e

seonpilKim reviewed Sep 12, 2022

View reviewed changes

slolee added 2 commits September 14, 2022 02:13

Refactor: 코드리뷰 내용 반영

2af9916

Refactor: 코드리뷰 반영

10b7864

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#39 헌혈인증 내역 조회 API 개발 #40

#39 헌혈인증 내역 조회 API 개발 #40

slolee commented Sep 11, 2022

slolee commented Sep 11, 2022

seonpilKim left a comment

seonpilKim Sep 12, 2022

slolee Sep 13, 2022

slolee Sep 13, 2022

seonpilKim Sep 16, 2022

seonpilKim Sep 12, 2022

slolee Sep 13, 2022

seonpilKim Sep 16, 2022

slolee Sep 17, 2022

seonpilKim Sep 12, 2022

slolee Sep 13, 2022

slolee Sep 13, 2022

seonpilKim Sep 12, 2022

slolee Sep 13, 2022

seonpilKim Sep 16, 2022

slolee Sep 17, 2022

seonpilKim Sep 12, 2022

slolee Sep 13, 2022

seonpilKim Sep 16, 2022

slolee Sep 17, 2022

seonpilKim Sep 12, 2022

slolee Sep 13, 2022

seonpilKim Sep 12, 2022

slolee Sep 13, 2022

seonpilKim Sep 16, 2022

slolee Sep 17, 2022

		@JsonInclude(JsonInclude.Include.NON_NULL)
		private BloodDonationDetailResponse detail;

#39 헌혈인증 내역 조회 API 개발 #40

Are you sure you want to change the base?

#39 헌혈인증 내역 조회 API 개발 #40

Conversation

slolee commented Sep 11, 2022

📌 Linked Issues

🔎 Change Details

💬 Comment

📑 References

✅ Check Lists

slolee commented Sep 11, 2022

seonpilKim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment