-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: 회원 정보 조회 기능을 구현한다 #27
Conversation
- EqualsAndHashCode 제거 - 임베디드 -> 엔티티로 타입변경 - 값타입 컬렉션 필드 일대다 관계로 변경(값타입 엔티티로 승격)
- EqualsAndHashCode 제거 - 임베디드 -> 엔티티로 타입변경 - 값타입 컬렉션 필드 일대다 관계로 변경(값타입 엔티티로 승격)
- MemberResponse - MemberProfileResponse - HobbiesResponse - StylesResponse
- MemberHobbies 필드 타입 변경(임베디드 -> 엔티티) - MemberStyles 필드 타입 변경(임베디드 -> 엔티티)
- Hobby(Enum), Style(Enum)을 엔티티로 승격시키기 위해 MemberHobby, MemberStyle 추가
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.
리뷰 완료했습니다! 몇 가지만 확인 부탁드려요 :)
@Autowired | ||
private MemberQueryRepository memberQueryRepository; | ||
|
||
|
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.
불필요한 공백 있습니다 :)
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.
수정하겠습니다
@SuppressWarnings("NonAsciiCharacters") | ||
public class MemberProfileDtoFixture { | ||
|
||
public static MemberProfileDto 회원_프로핑_DTO_요청() { |
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.
프로핑
오타가 있습니다
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.
수정하겠습니다
return MemberProfileResponse.createWith( | ||
member.getNickname(), | ||
member.getPhoneNumber(), | ||
getProfile(member).getJob(), |
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.
getProfile, getPhysicalProfile을 연속적으로 사용하셨는데 return 전에 getProfile, getPhysicalProfile 반환값을 변수로 미리 정의한 뒤 사용하시는 게 더 좋을 것 같습니다!
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.
좋은 지적 감사합니다 :)
@@ -10,6 +11,8 @@ public interface MemberRepository { | |||
|
|||
Optional<Member> findByNickname(String nickname); | |||
|
|||
MemberResponse findMemberWithProfile(Long id); |
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.
memberId로 조회하는 것으로 보이는데 그렇다면 WithProfile 보다 WithMemberId로 풀어서 사용하시면 어떨까 싶습니다 :)
class 닉네임_중복_확인 { | ||
|
||
@Test | ||
void 중복된_닉네임이_존재하는지_않으면_예외가_발생하지_않는다() { |
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.
존재하는지
-> 존재하지
로 바꾸셔야 할 것 같습니다
@Embeddable | ||
@AllArgsConstructor(access = AccessLevel.PRIVATE) | ||
@EqualsAndHashCode(of = "id") | ||
@AllArgsConstructor(access = AccessLevel.PUBLIC) |
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.
AllArgsConstructor 접근 레벨을 변경하신 이유가 있으실까요?
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.
@NoArgsConstructor를 수정하다가 저도 모르게 @AllArgsConstructor까지 수정했던 것 같습니다.
좋은 지적 감사합니다 :)
@DeleteMapping("/{memberId}") | ||
public ResponseEntity<Void> deleteMember(@PathVariable final Long memberId) { | ||
@DeleteMapping | ||
public ResponseEntity<Void> deleteMember(@AuthMember final Long memberId) { |
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.
API 경로가 이제 memberId가 들어가지 않게 되어 /api/members
로 되는데, 그렇다면 DELETE /api/members
가 됩니다. 자신을 삭제한다는 측면에서 /api/members/me
와 같은 방식으로 수정하면 좋을 것 같은데 어떻게 생각하시나요?
수정도 그렇고 조회, 초기화에 대해서도 동일한 측면으로 적용해보면 좋을 것 같습니다. (/api/members
로만 진행하기에는 members가 복수형이라 전체적인 느낌을 주는 것 같아 제안드립니다!)
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.
좋은 방법인 거 같네요! 수정하도록 하겠습니다
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.
몇 부분만 수정해주시면 될 것 같습니다!
수고하셨습니다~
public void checkMemberExists(final MemberNicknameRequest memberNicknameRequest) { | ||
validateNicknameIsUnique(memberNicknameRequest.nickname()); | ||
} | ||
|
||
private void validateNicknameIsUnique(final String nickname) { | ||
if (memberRepository.existsByNickname(nickname)) { | ||
throw new MemberNicknameAlreadyExistedException(); | ||
} | ||
} |
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.
하나로 묶어도 될 것 같습니다
} | ||
@JoinColumn(name = "member_hobbies_id") | ||
@OneToMany(cascade = {CascadeType.PERSIST, CascadeType.REMOVE}, orphanRemoval = true, fetch = FetchType.LAZY) | ||
private Set<MemberHobby> hobbies = new HashSet<>(); |
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.
👍
public boolean isSame(final MemberHobby memberHobby) { | ||
return this.hobby.equals(memberHobby.hobby); | ||
} |
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.
equals&hashcode가 없어도 되나요??
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.
Hobby는 enum 타입이라 인스턴스가 한개만 존재하기 때문에 equals와 hashcode를 재정의하지 않았습니다.
MemberHobby클래스의 equals와 hashcode를 재정이 하지 않은 이유는
private void changeHobbies(final List hobbyCodes) {
hobbies.removeIf(memberHobby -> !memberHobby.hasMatchingHobbyCodeOf(hobbyCodes));
hobbyCodes.stream()
.map(MemberHobby::createWith)
.filter(memberHobby -> !isAlreadyExist(memberHobby))
.forEach(memberHobby -> hobbies.add(memberHobby));
}
이 메서드는 MemberHobbies에 존재하는 메서드인데 여기서 stream에서 MemberHobby 객체를 생성합니다.
하지만 이 시점에 id값이 존재하지 않는데 JPA는 PK의 id값이 같으면 똑같은 객체로 인식하기 때문에
.filter(memberHobby -> !isAlreadyExist(memberHobby)) 이 로직에서 새로운 취미 코드가 들어와도 이미 존재한다고 인식해서 저장이 안되게 됩니다.
이런 문제가 있어서 MemberHobby에는 eqauls와 hashcode를 재정의하지 않았습니다.
|
||
private final JPAQueryFactory jpaQueryFactory; | ||
|
||
public MemberResponse findMemberWithProfile(final Long memberId) { |
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.
하나의 쿼리로는 할 수 없을까요?
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.
취미와 스타일은 프로필과 일대다 관계입니다.
한번의 쿼리로 조회할 시 1 x n x m으로 컬럼이 늘게 되어 일대다 관계인 취미와 스타일은 따로 조회하는 방법을 선택했습니다.
재윤님은 한번의 쿼리로 조회하는 것이 더 좋다고 생각하시나요?
@@ -24,32 +26,38 @@ | |||
public class MemberController { | |||
|
|||
private final MemberService memberService; | |||
private final MemberQueryService memberQueryService; | |||
|
|||
@GetMapping("/nickname/existence") |
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.
/nickname
이 부분도 자원에 해당하는 것을 나타내야하는데
/api/members/{nickname}/existences
이건 어떨까요?
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.
음 그러면 nickname을 PathVariable로 받아서 사용하는 방식을 사용하는 건가요?
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.
@eom-tae-in 넵! 자원을 표기해서 사용하는 방식입니다
MemberResponse memberResponse = memberQueryRepository.findMemberWithProfile(member.getId()); | ||
|
||
// then | ||
assertSoftly(softly -> { |
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.
👍
@@ -36,6 +39,8 @@ | |||
@WebMvcTest(MemberController.class) | |||
class MemberControllerWebMvcTest extends MockBeanInjection { | |||
|
|||
private static final String bearerToken = "Bearer token"; |
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.
상수면 대문자 컨벤션으로 하는게 좋을 것 같습니다
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.
네 알겠습니다
|
||
// when & then | ||
mockMvc.perform(get("/api/members") | ||
.param("memberId", String.valueOf(memberId)) |
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.
param 문서화도 해야할 것 같습니다. 해당되는 부분 수정 부탁드릴게요!
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.
네 알겠습니다!
PUBLIC -> PRIVATE
findMemberWithProfile -> findMemberWithId
MemberNicknameRequest
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.
수고 많으셨습니다!
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.
수고하셨습니다! 저도 얼른 PR 올리겠습니다 :)
📄 Summary
close #24