refactor: 메시지 서비스에서 Querydsl 로직 분리 #678

hyewoncc · 2022-10-26T08:53:00Z

요약

메시지 서비스에서 Querydsl 로직 분리

작업 내용

Querydsl이 사용되는 모든 로직을 별도의 QMessageRepository 클래스로 분리
QMessageRepository의 메서드를 JPA 레포지토리 메서드 명명 규칙과 비슷하게 개선
MessageRequest가 레포지토리 레이어까지 넘어가지 않도록 파라미터 분리

Unit Test Results

235 tests ±0 235 ✔️ ±0 18s ⏱️ -2s
  89 suites +2     0 💤 ±0
  89 files +2     0 ❌ ±0

Results for commit ddb7c83. ± Comparison against base commit 77afb87.

This pull request removes 9 and adds 9 tests. Note that renamed tests count towards both.

com.pickpick.acceptance.auth.AuthAcceptanceTest ‑ 워크스페이스 등록 후 워크스페이스 재등록시 예외처리
com.pickpick.acceptance.auth.AuthAcceptanceTest ‑ 워크스페이스 초기화 후 다시 로그인
com.pickpick.auth.application.AuthServiceTest ‑ 워크스페이스 초기화 시 워크스페이스를 저장한다.
com.pickpick.auth.application.AuthServiceTest ‑ 워크스페이스 초기화 시 해당 워크스페이스의 채널과 멤버를 저장한다.
com.pickpick.auth.application.AuthServiceTest ‑ 이미 등록된 워크스페이스를 재등록시 예외가 발생한다.
com.pickpick.slackevent.SlackEventTest ‑ [2] request={event={subtype=message_changed, type=message}}, expected=MESSAGE_CHANGED
com.pickpick.slackevent.SlackEventTest ‑ [3] request={event={subtype=message_deleted, type=message}}, expected=MESSAGE_DELETED
com.pickpick.slackevent.SlackEventTest ‑ [4] request={event={subtype=thread_broadcast, type=message}}, expected=MESSAGE_THREAD_BROADCAST
com.pickpick.slackevent.SlackEventTest ‑ [5] request={event={subtype=file_share, type=message}}, expected=MESSAGE_FILE_SHARE

com.pickpick.acceptance.workspace.WorkspaceAcceptanceTest ‑ 워크스페이스 등록 후 워크스페이스 재등록시 예외처리
com.pickpick.acceptance.workspace.WorkspaceAcceptanceTest ‑ 정상 워크 스페이스 등록 후 로그인
com.pickpick.slackevent.SlackEventTest ‑ [2] request={event={type=message, subtype=message_changed}}, expected=MESSAGE_CHANGED
com.pickpick.slackevent.SlackEventTest ‑ [3] request={event={type=message, subtype=message_deleted}}, expected=MESSAGE_DELETED
com.pickpick.slackevent.SlackEventTest ‑ [4] request={event={type=message, subtype=thread_broadcast}}, expected=MESSAGE_THREAD_BROADCAST
com.pickpick.slackevent.SlackEventTest ‑ [5] request={event={type=message, subtype=file_share}}, expected=MESSAGE_FILE_SHARE
com.pickpick.workspace.application.WorkspaceServiceTest ‑ 워크스페이스 초기화 시 워크스페이스를 저장한다.
com.pickpick.workspace.application.WorkspaceServiceTest ‑ 워크스페이스 초기화 시 해당 워크스페이스의 채널과 멤버를 저장한다.
com.pickpick.workspace.application.WorkspaceServiceTest ‑ 이미 등록된 워크스페이스를 재등록시 예외가 발생한다.

♻️ This comment has been updated with latest results.

hyewoncc · 2022-10-26T09:11:51Z

backend/src/main/java/com/pickpick/message/application/MessageService.java

+        List<MessageResponse> messageResponses = messages.findMessages(
+                memberId,
+                channelIds,
+                messageRequest.getKeyword(),
+                messageRequest.getMessageId(),
+                messageRequest.getDate(),
+                messageRequest.isNeedPastMessage(),
+                messageRequest.getMessageCount());


MessageRequest라는 DTO가 레포지토리까지 넘어가면 레포지토리가 클라이언트에서 넘어오는 값을 알고 있는 상황이 되는 것 같아서, 파라미터에 값을 꺼내서 넣도록 했습니다
조금 너저분한 감이 없지 않지만 jpaQueryFactory 로직보다는 짧으니까 이게 낫지 않을까요...👀

hyewoncc · 2022-10-26T09:12:34Z

backend/src/main/java/com/pickpick/message/domain/QMessageRepository.java

+        return jpaQueryFactory
+                .select(getMessageResponseConstructor())
+                .from(message)
+                .leftJoin(message.member)
+                .leftJoin(bookmark)
+                .on(bookmarksFindByMemberId(memberId))
+                .leftJoin(reminder)
+                .on(remindersFindByMemberIdWhereRemindDateAfterNow(memberId))
+                .where(inChannelsFilterByTextAndPostedDate(channelIds, keyword, messageId, date, needPastMessage))
+                .orderBy(postedDateDescOrAsc(needPastMessage))
+                .limit(messageCount)
+                .fetch();


가독성 개선을 위해 JPA 메서드 명명 규칙을 최대한 모방해봤어요

크 좋아요 👍 👍 ❤️

yeon-06

안녕하세요 써머~!
분리 정말 고생 많으셨습니다
MessageService가 깔끔해진 것을 보니까 감탄이 나오네요👍
메시지 조회 쪽은 언제봐도 조건이 복잡하군요😔

yeon-06 · 2022-10-27T02:12:39Z

backend/src/main/java/com/pickpick/message/application/MessageService.java

-    private static boolean isNonNullNorEmpty(final List<Long> channelIds) {
-        return Objects.nonNull(channelIds) && !channelIds.isEmpty();
+    private boolean isNonNullNorEmpty(final List<Long> channelIds) {
+        return channelIds != null && !channelIds.isEmpty();


꼼꼼한 개선👍

!= 굿! 👍 👍 👍 ❤️
근데 이거 원래 예전 메서드명부터 좀 이상했던 것 같아요

null이 아니거나 비어있음이 아니라 null이 아니거나 비어있지 않음 이어야할 것 같아서
isNonNullAndNotEmpty()가 어떨까요?

이렇게 적는 게 확실하겠네요! 반영하겠습니다

yeon-06 · 2022-10-27T02:13:40Z

backend/src/main/java/com/pickpick/message/domain/QMessageRepository.java

+
+

JangBomi

써머 👍
MessageService에서 QueryDsl이 빠지니까 정말 깔끔해요 👍 👍 👍 👍 👍 최고의 개선
리뷰 몇개 남겼고, 답을 보고싶어서 우선 RC할게요!!

JangBomi · 2022-10-27T08:03:00Z

backend/src/main/java/com/pickpick/message/application/MessageService.java

+    public MessageService(final MemberRepository members,
+                          final QMessageRepository messages,
                          final ChannelSubscriptionRepository channelSubscriptions,


진짜 너무너무 사소한거긴한데 순서를
MemberRepo
ChannelSubscriptionRepo
QMessageRepo
로 쓰는거 어떨까요? 이 PR이 Q레포 도입의 첫 PR이라 순서 컨벤션 맞추면 좋을 것 같아서 의견 남겨요!

제 알록달록한 인텔리에서는 인터페이스랑 클래스랑 색이 달라서 컨벤션 만들면 좋을 것 같아서 의견 남깁니다:)

그리고 하나 더 하자면 QMessageRepository 인스턴스 이름이 messages 인데, MessageService에는 MessageRepository가 없어서 괜찮은데 Bookmark랑 Reminder는 이미 Repository가 있어서 변수 이름 생각해보면 좋을 것 같아요 ! 👍

거기서 둘 다 사용한다면 QRepository 인터페이스 추출 후 상속하는 식으로 하나의 레포만 쓸 수 있게 해야겠네요
설명이 어려운데 또 블로그 홍보같아졌지만!!.. 제 블로그 글 초반에 해당 방식이 설명되어있어요

선언 순서는 변경할게요!

아하 이해했습니다! 👍

JangBomi · 2022-10-27T08:09:18Z

backend/src/main/java/com/pickpick/message/application/MessageService.java

-    private static boolean isNonNullNorEmpty(final List<Long> channelIds) {
-        return Objects.nonNull(channelIds) && !channelIds.isEmpty();
+    private boolean isNonNullNorEmpty(final List<Long> channelIds) {
+        return channelIds != null && !channelIds.isEmpty();


!= 굿! 👍 👍 👍 ❤️
근데 이거 원래 예전 메서드명부터 좀 이상했던 것 같아요

null이 아니거나 비어있음이 아니라 null이 아니거나 비어있지 않음 이어야할 것 같아서
isNonNullAndNotEmpty()가 어떨까요?

JangBomi · 2022-10-27T08:21:28Z

backend/src/main/java/com/pickpick/message/domain/QMessageRepository.java

+
+    public List<MessageResponse> findMessages(final Long memberId,
+                                              final List<Long> channelIds,
+                                              final String keyword,


MessageRequest에서 코멘트 달아주신거 여기서 답변 남깁니다!
써머가 해주신대로 MessageRequest를 풀어서 보내준거 저는 훨씬 좋은 것 같아요! 파라미터가 많아진다는 단점이 있긴 하지만, 그래도 꽤 직관적이라고 생각해서 저는 너무 동의합니다!
여기서 MessageResponse도 생각해봤는데, MessageResponse도 레포지토리가 클라이언트에서 넘어가는 값을 알고 있는 상황이 되는 것 같다고 느껴져요.
MessageRequest, MessageResponse 둘 다 ui 패키지 내부의 controller에서 외부와의 소통 때 사용되는 dto라(실제 저희 패키지 분리에서도 ui 패키지에 있으니) 저는 둘 다 Repository단까지는 내려오지 않았으면 좋겠어요.
코드의 복잡도가 좀 올라가는 것 같긴하지만, repo-service 사이에서의 객체를 새로 정의하는건 어떨까요? 이렇게 만들어진 객체를 도메인이라 봐야할지, dto라 봐야할지까지는 잘 모르겠지만, MessageResponse는 ui와 관련한 dto니 repository 단까지는 내려오지 않았으면 좋겠습니다!

Response 까지는 생각 못했는데 정말 좋은 접근이네요!!
그런데 별도의 dto를 만들어도 Response랑 내부가 동일하게 될 것 같은데 좀 더 효율적인 방안이 있을까요?

음...그러게요 어ㄷ쨋든 반환하는 그대로니 내용은 똑같을 것 같네요 ㅠㅠ
도메인과 request가 동일하더라도 도메인을 view에 노출시키지 않기위해 controller에서 dto를 사용하는것처럼 그냥 내용이 같은 새로운 객체 만들어서 ResponseDto 자체를 repository단까지 들어가게 하지 않는다는 것에 대한 의의만 있을 것 같네요!!
이 부분은 다른 Service도 옮기면서 차차 같이 생각해봐용!!!

JangBomi · 2022-10-27T08:22:55Z

backend/src/main/java/com/pickpick/message/domain/QMessageRepository.java

+        return jpaQueryFactory
+                .select(getMessageResponseConstructor())
+                .from(message)
+                .leftJoin(message.member)
+                .leftJoin(bookmark)
+                .on(bookmarksFindByMemberId(memberId))
+                .leftJoin(reminder)
+                .on(remindersFindByMemberIdWhereRemindDateAfterNow(memberId))
+                .where(inChannelsFilterByTextAndPostedDate(channelIds, keyword, messageId, date, needPastMessage))
+                .orderBy(postedDateDescOrAsc(needPastMessage))
+                .limit(messageCount)
+                .fetch();


크 좋아요 👍 👍 ❤️

JangBomi · 2022-10-27T08:23:51Z

backend/src/main/java/com/pickpick/message/domain/QMessageRepository.java

+import org.springframework.util.StringUtils;
+
+@Repository
+public class QMessageRepository {
+
+    private final QMessage message = QMessage.message;


이건 제가 확실하게 안살펴봤는데, 저희 컨벤션에서
public -> 사용하는 private들 -> 또 다른 public -> 또 다른 public 에서 사용하는 private들
이 순서로 하기로 했던 것 같아요...!!

사용되는 순서대로 재정렬 했습니다~

JangBomi · 2022-10-27T08:25:43Z

backend/src/main/java/com/pickpick/message/domain/QMessageRepository.java

+    private BooleanExpression textIsNotNullNorEmpty() {
+        return message.text.isNotNull()
+                .and(message.text.isNotEmpty());
+    }


여기도 아까 isNotNullNorEmpty랑 같은 리뷰 남길게요!

JangBomi · 2022-10-27T08:28:58Z

backend/src/main/java/com/pickpick/message/domain/QMessageRepository.java

+        Message target = Optional.ofNullable(jpaQueryFactory
+                        .select(message)
+                        .from(message)
+                        .where(message.id.eq(messageId))
+                        .fetchOne())


Suggested change

Message target = Optional.ofNullable(jpaQueryFactory

.select(message)

.from(message)

.where(message.id.eq(messageId))

.fetchOne())

Message target = Optional.ofNullable(selectMessage())

.fetchOne())

private Message selectMessage() {

returnjpaQueryFactory

.select(message)

.from(message)

.where(message.id.eq(messageId)

.fetchOne()

}

혹시 이부분은 따로 추출 안하신 이유가 있는걸까요? ofNullable의 파라미터 안에 로직이 있는 느낌이라 살짝 와닿지 않아서 리뷰 남겨요!

😁 이유 없어요! 확실히 추출하는게 낫네요

pickpick-sonarqube · 2022-10-28T00:35:47Z

Analysis Details

3 Issues

0 Bugs
0 Vulnerabilities
3 Code Smells

Coverage and Duplications

91.89% Coverage (88.30% Estimated after merge)
0.00% Duplicated Code (0.00% Estimated after merge)

Project ID: woowacourse-teams_2022-pickpick_AYKprLeNXDQxKhlck1fc

View in SonarQube

JangBomi

댓글 달아신거 확인했어요! approve 하겠습니당 👍

* refactor: 잘못된 파일명 수정 , 불필요한 import 문 제거 ,사용하지 않은 패키지 제거 (#668) * fix: 파일명 앞 공백 제거 * refactor: 불필요한 import 문 제거 * chore: jest 설정 파일 제거 * chore: 사용하지 않는 패키지 제거 - jest 관련 패키지 - babel-loader - react-refresh * refactor: package.json 내 name,version,description 명시 * chore: 더 이상 사용되지 않는 목데이터 제거 * hotfix: 슬랙 권한 문제 해결 (#671) * fix: 슬랙 권한 문제 해결 * refactor: 사용하지 않은 파일 삭제 * refactor: 사용하지 않는 메서드 삭제 및 순서 변경 * feat: 검색엔진 최적화를 위한 meta tag 설정 (#682) * refactor: 슬랙 관련 URL을 환경변수로 관리 (#683) undefined * refactor: @EnableJpaAuditing 설정 클래스 분리 (#680) * refactor: jpa auditing 설정 클래스 분리 * refactor: 필요없는 MockBean 제거 * feat: 워크스페이스 등록 후 바로 로그인 (#677) * feat: callWorkspaceInfo의 반환값에 사용자의 slack id 값 추가 * feat: workspace 등록 시 로그인까지 함께 처리 * test: 워크스페이스 등록 후 별도로 로그인 호출하는 과정 제거 * test: 한글 메서드명으로 인한 경고 무시 * refactor: auth와 workspace 분리 * test: 테스트에서 Auth와 Workspace 분리 * style: Slack에서 Workspace 정보 불러오는 경우에 사용되는 DTO 및 메서드명 변경 - callWorkspaceInfo -> callOAuthAccessInfo - WorkspaceInfoDto -> OAuthAccessInfoDto * style: 검증 메서드명 변경 validateExistWorkspace -> validateUnregisteredWorkspace * style: 반복되는 메서드명 변경 registerWorkspace -> register * style: 테스트 잘못된 DisplayName 변경 * test: Optional 값이 존재하는 경우 isPresent()로 검증 * style: toEntity -> toWorkspace 이름 변경 * style: 워크스페이스_초기화 -> 워크스페이스_초기화_및_로그인 이름 변경 * refactor: AuthService login 로직 분리 * refactor: 슬랙 워크스페이스 등록 플로우 수정 (#676) * refactor: 랜딩페이지 이용 문구 수정 (#684) * feat: 랜딩페이지 이용 문구 수정 * fix: 맞춤법 수정 * refactor: 스낵바 메시지 어투 수정 (#685) * refactor: Dropdown 컴포넌트에 스크린리더 설명 추가 (#686) * fix: msw 구동을 위해 기존에 삭제되었던 mockServiceWorker 파일 추가 * refactor: 드랍다운 열림 여부 알려주는 스크린리더설명을 공용 Dropdown 컴포넌트로 이동 * refactor: 메시지 서비스에서 Querydsl 로직 분리 (#678) * refactor: Querydsl 로직을 별도 클래스로 분리 * refactor: 빌더 패턴 안 메서드 이름 개선 * refactor: 메서드 명 개선 및 서비스로 비즈니스 로직 이동으로 파라미터 변경 * refactor: DTO가 레포지토리로 넘어가지 않도록 파라미터 분리 * refactor: 메서드에 필요없는 static 선언 제거 * refactor: 코드리뷰 반영 | 메서드 명, 선언 순서 개선 및 메서드 추출 Co-authored-by: 봄 <[email protected]> Co-authored-by: Jaejeung Ko <[email protected]> Co-authored-by: 써머(최혜원) <[email protected]> Co-authored-by: yeonLog <[email protected]>

hyewoncc added 4 commits October 26, 2022 14:45

refactor: Querydsl 로직을 별도 클래스로 분리

0f28258

refactor: 빌더 패턴 안 메서드 이름 개선

9b8db30

refactor: 메서드 명 개선 및 서비스로 비즈니스 로직 이동으로 파라미터 변경

47f431a

refactor: DTO가 레포지토리로 넘어가지 않도록 파라미터 분리

976ccd3

hyewoncc requested review from yeon-06 and JangBomi October 26, 2022 08:53

refactor: 메서드에 필요없는 static 선언 제거

9538a28

hyewoncc force-pushed the feature/refactor-querydsl-repository branch from b9e2fdd to 9538a28 Compare October 26, 2022 08:54

This comment has been minimized.

Sign in to view

hyewoncc self-assigned this Oct 26, 2022

hyewoncc added 🎉 BE 백엔드 관련 ⚙️ REFACTOR labels Oct 26, 2022

hyewoncc marked this pull request as ready for review October 26, 2022 09:10

hyewoncc commented Oct 26, 2022

View reviewed changes

yeon-06 approved these changes Oct 27, 2022

View reviewed changes

JangBomi requested changes Oct 27, 2022

View reviewed changes

refactor: 코드리뷰 반영 | 메서드 명, 선언 순서 개선 및 메서드 추출

ddb7c83

JangBomi approved these changes Oct 28, 2022

View reviewed changes

hyewoncc merged commit fb6db92 into develop Oct 28, 2022

hyewoncc deleted the feature/refactor-querydsl-repository branch October 28, 2022 05:59

yeon-06 linked an issue Oct 29, 2022 that may be closed by this pull request

서비스 레이어에서 Querydsl 로직 분리 #679

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: 메시지 서비스에서 Querydsl 로직 분리 #678

refactor: 메시지 서비스에서 Querydsl 로직 분리 #678

hyewoncc commented Oct 26, 2022 •

edited

Loading

github-actions bot commented Oct 26, 2022 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

hyewoncc Oct 26, 2022

hyewoncc Oct 26, 2022

JangBomi Oct 27, 2022

yeon-06 left a comment

yeon-06 Oct 27, 2022

JangBomi Oct 27, 2022

hyewoncc Oct 28, 2022

yeon-06 Oct 27, 2022

JangBomi left a comment

JangBomi Oct 27, 2022

JangBomi Oct 27, 2022

hyewoncc Oct 28, 2022

hyewoncc Oct 28, 2022

JangBomi Oct 28, 2022

JangBomi Oct 27, 2022

JangBomi Oct 27, 2022

hyewoncc Oct 28, 2022

JangBomi Oct 28, 2022

JangBomi Oct 27, 2022

JangBomi Oct 27, 2022

hyewoncc Oct 28, 2022

JangBomi Oct 27, 2022

JangBomi Oct 27, 2022

hyewoncc Oct 28, 2022

pickpick-sonarqube bot commented Oct 28, 2022

JangBomi left a comment

refactor: 메시지 서비스에서 Querydsl 로직 분리 #678

refactor: 메시지 서비스에서 Querydsl 로직 분리 #678

Conversation

hyewoncc commented Oct 26, 2022 • edited Loading

요약

작업 내용

관련 이슈

github-actions bot commented Oct 26, 2022 • edited Loading

Unit Test Results

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeon-06 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

JangBomi 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

pickpick-sonarqube bot commented Oct 28, 2022

Analysis Details

3 Issues

Coverage and Duplications

JangBomi left a comment

Choose a reason for hiding this comment

hyewoncc commented Oct 26, 2022 •

edited

Loading

github-actions bot commented Oct 26, 2022 •

edited

Loading