Skip to content
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: message entity, repository 구현 #424

Open
wants to merge 3 commits into
base: BE/develop
Choose a base branch
from

Conversation

kevstevie
Copy link
Collaborator

😋 작업한 내용

쪽지(dm)기능 의견 구합니다

🙏 PR Point

1:1 쪽지 기록이 남는 곳을 dm방이라고 이야기하겠습니다

dm방을 의미하는 엔티티를 따로 두지 않고 쪽지 엔티티에 보낸사람, 받은 사람, 내용이 있습니다.

조금 복잡한 형태의 쿼리와 dto projection을 통해 구현했는데

  1. 쪽지함 조회를 했을 때 groupby와 having 쿼리로 최소한의 row만 가져와 인스타 dm 방 형태의 화면을 띄울 생각입니다
  2. 특정 dm방을 조회하는 요청이 오면 해당 방의 대화상대, 유저의 쪽지를 페이징해서 띄워줍니다 이때 상대의 정보는 없고 내가 보낸 메시지인가 아닌가만을 가진 dto를 사용해 projection합니다

service controller는 금방 구현될 것 같아서 가장 중요한 부분인 조회쿼리에 의견 구합니다

👍 관련 이슈

@github-actions
Copy link

github-actions bot commented Sep 18, 2023

Test Results

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0

Results for commit a6f94b7.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@poi1649 poi1649 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주드 고생많으셨습니다!
테스트가 아주아주 꼼꼼하네요 몇 가지 치명적인(?) 사항이 있는 것 같아 오랜만에 RC 날리겠습니다. 확인 부탁드려요!!

import org.springframework.data.domain.Slice;

@Import(JpaConfig.class)
@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 안쓰면 어떻게 되나요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분과 아래 Application.yml을 변경한 부분과 연관관계가 있는 것 같은데 저도 궁금하네요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

DataJpaTest 내부에 있는 AutoConfigureTestDatabase 어노테이션의 기본 설정입니다
replace.any로 DataJpaTest가 테스트 시에 임의의 인메모리 데이터베이스를 사용합니다
Replace.None으로 두지 않으면 로컬의 설정과 맞지 않아 ddl이 뜨지 않습니다

Comment on lines 15 to 18
select m.sender as sender, m.receiver as receiver
from Message m
group by m.sender, m.receiver
having m.sender = :member or m.receiver = :member
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금 쿼리는 다음과 같이 message 테이블을 풀스캔하네요.(type = all)

image

where 절을 사용해 수정하면 type = index merge 가 되어 필요한 컬럼만 효율적으로 스캔할 수 있습니다! 👍

image

추가로 이 정보도 페이징 할 것일면 order 절을 넣어주면 좋겠네요! (메서드 이름에 있는데 쿼리엔 없어요)

Suggested change
select m.sender as sender, m.receiver as receiver
from Message m
group by m.sender, m.receiver
having m.sender = :member or m.receiver = :member
SELECT m.sender AS sender, m.receiver AS receiver
FROM Message m
WHERE m.sender = :member OR m.receiver = :member
GROUP BY m.sender, m.receiver

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은데요? 그리고 unique 값을 뽑아내기 위해 group by를 쓰셨다면 distinct를 쓰시는 걸 추천드립니다.
성능도 더 좋고 의미적으로도 더 잘 전달될 것 같아요

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 인덱스 머지 최적화에 대해 가볍게 정리해봤는데 혹시 도움이 될까 싶어 링크 추가합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다


@Query("""
select m.content as content, m.createdAt as createdAt,
case when m.sender.id = :memberId then true else false end as isMine
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

별게 다있네요ㅋㅋㅋ 배워갑니다

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우리가 예전에 우려했던 대로 Pageable을 사용하면 새로운 dm을 보낼 때 과거의 메시지가 중복돼서 보이는 문제가 있을 수 있겠네요..
하지만 이런 앱의 dm에서 중복된 메시지가 등장하는 것이 유저에게 얼마나 불편함을 불러일으킬지도 미지수긴 합니다ㅎㅎ
이건 다른 분들의 의견도 들어보고 싶네요!

Copy link
Member

@CFalws CFalws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

쿼리에 대한 제안 가볍게 넣어봤습니다
고생하셨습니다~

import org.springframework.data.domain.Slice;

@Import(JpaConfig.class)
@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분과 아래 Application.yml을 변경한 부분과 연관관계가 있는 것 같은데 저도 궁금하네요?

Comment on lines 15 to 18
select m.sender as sender, m.receiver as receiver
from Message m
group by m.sender, m.receiver
having m.sender = :member or m.receiver = :member
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은데요? 그리고 unique 값을 뽑아내기 위해 group by를 쓰셨다면 distinct를 쓰시는 걸 추천드립니다.
성능도 더 좋고 의미적으로도 더 잘 전달될 것 같아요


@Query("""
select m.content as content, m.createdAt as createdAt,
case when m.sender.id = :memberId then true else false end as isMine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우리가 예전에 우려했던 대로 Pageable을 사용하면 새로운 dm을 보낼 때 과거의 메시지가 중복돼서 보이는 문제가 있을 수 있겠네요..
하지만 이런 앱의 dm에서 중복된 메시지가 등장하는 것이 유저에게 얼마나 불편함을 불러일으킬지도 미지수긴 합니다ㅎㅎ
이건 다른 분들의 의견도 들어보고 싶네요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend🤎 백엔드 feat 기능 추가 netflix-george🎅🏻 주드
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants