feat: 채팅 관련 엔티티 및 DDL 작성 #395
Conversation
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7분
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
src/main/java/com/example/solidconnection/chat/domain/ChatParticipant.java (2)
25-26: 외래 키 컬럼명을 명시적으로 지정해주세요.
@JoinColumn어노테이션을 추가하여 외래 키 컬럼명을 명확히 해주시면 좋겠습니다. 이렇게 하면 데이터베이스 스키마와의 일관성을 보장할 수 있어요.@ManyToOne(fetch = FetchType.LAZY) +@JoinColumn(name = "chat_room_id") private ChatRoom chatRoom;
23-23: siteUserId 필드에 대한 검증이 필요할 수 있습니다.primitive
long타입을 사용하신 점은 null 안전성 측면에서 좋습니다. 하지만 비즈니스 로직에 따라 양수 값만 허용하는 등의 추가 검증이 필요할지 검토해보시기 바랍니다.예시:
@Positive어노테이션 추가 고려+@Positive private long siteUserId;src/main/java/com/example/solidconnection/chat/domain/ChatAttachment.java (3)
24-25: 첨부파일 타입을 나타내는 더 확장 가능한 방식을 고려해보세요.현재
Boolean isImage를 사용하고 계시는데, 향후 다른 파일 타입(비디오, 문서 등)을 지원할 가능성을 고려하면 enum을 사용하는 것이 더 확장성 있는 설계일 수 있습니다.+public enum AttachmentType { + IMAGE, VIDEO, DOCUMENT, OTHER +} + -@Column(nullable = false) -private Boolean isImage; +@Enumerated(EnumType.STRING) +@Column(nullable = false) +private AttachmentType attachmentType;
27-31: URL 필드에 대한 개선 제안
- URL 길이 제한: 500자 제한이 일부 클라우드 스토리지 서비스의 긴 URL을 수용하지 못할 수 있습니다.
- URL 형식 검증: URL 형식의 유효성을 검증하는 어노테이션을 추가하면 좋겠습니다.
+import jakarta.validation.constraints.NotBlank; +import org.hibernate.validator.constraints.URL; + -@Column(nullable = false, length = 500) +@Column(nullable = false, length = 1000) +@NotBlank +@URL private String url; -@Column(length = 500) +@Column(length = 1000) +@URL private String thumbnailUrl;
33-34: 외래 키 컬럼명을 명시해주세요.
@JoinColumn어노테이션을 추가하여 데이터베이스 스키마와의 일관성을 보장해주시기 바랍니다.@ManyToOne(fetch = FetchType.LAZY) +@JoinColumn(name = "chat_message_id") private ChatMessage chatMessage;src/main/java/com/example/solidconnection/chat/domain/ChatRoom.java (1)
26-27: 채팅방 타입을 더 명확하게 표현할 수 있습니다.
Boolean isGroup대신 enum을 사용하면 향후 다른 채팅방 타입(예: 공지방, 일대일 채팅, 그룹 채팅 등)을 추가할 때 더 유연하게 대응할 수 있습니다.+public enum ChatRoomType { + DIRECT, GROUP +} + -@Column(nullable = false) -private Boolean isGroup; +@Enumerated(EnumType.STRING) +@Column(nullable = false) +private ChatRoomType roomType;src/main/java/com/example/solidconnection/chat/domain/ChatMessage.java (2)
34-34: 발신자 ID 검증을 추가해주세요.
senderId가 유효한 사용자 ID인지 검증하는 로직을 추가하면 좋겠습니다.+import jakarta.validation.constraints.Positive; + +@Positive private long senderId;
36-37: 외래 키 컬럼명 명시가 필요합니다.다른 엔티티들과의 일관성을 위해
@JoinColumn어노테이션을 추가해주시기 바랍니다.@ManyToOne(fetch = FetchType.LAZY) +@JoinColumn(name = "chat_room_id") private ChatRoom chatRoom;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/example/solidconnection/chat/domain/ChatAttachment.java(1 hunks)src/main/java/com/example/solidconnection/chat/domain/ChatMessage.java(1 hunks)src/main/java/com/example/solidconnection/chat/domain/ChatParticipant.java(1 hunks)src/main/java/com/example/solidconnection/chat/domain/ChatReadStatus.java(1 hunks)src/main/java/com/example/solidconnection/chat/domain/ChatRoom.java(1 hunks)src/main/resources/db/migration/V23__add_chat_related_tables.sql(1 hunks)
🔇 Additional comments (6)
src/main/java/com/example/solidconnection/chat/domain/ChatReadStatus.java (1)
21-23: 직접적인 엔티티 관계 대신 ID를 사용한 설계에 대한 확인
ChatRoom이나ChatParticipant엔티티와 직접적인 관계를 설정하지 않고 ID만 저장하신 설계 선택을 확인했습니다. 이는 성능상 이점이 있지만, 다음 사항들을 고려해보시기 바랍니다:
- 조회 시 추가 쿼리가 필요할 수 있음
- 참조 무결성 검증이 어려울 수 있음
- 객체 그래프 탐색이 제한됨
현재 설계가 의도된 것인지, 향후 성능 요구사항과 일치하는지 검토해주시기 바랍니다.
src/main/java/com/example/solidconnection/chat/domain/ChatRoom.java (2)
29-30: 참가자 관계에서 CASCADE.ALL 사용에 대한 주의사항
CascadeType.ALL을 사용하면 ChatRoom 삭제 시 모든 참가자가 함께 삭제됩니다. 이는 의도된 동작인지 확인이 필요합니다. 일반적으로 채팅방 참가자는 독립적인 엔티티로 관리되는 경우가 많습니다.다음 시나리오들을 검토해주시기 바랍니다:
- 채팅방 삭제 시 참가자 정보도 함께 삭제되어야 하는가?
- 참가자가 여러 채팅방에 참여할 수 있는가?
필요에 따라
CascadeType.PERSIST, CascadeType.MERGE등 특정 연산만 cascade하는 것을 고려해보세요.-@OneToMany(mappedBy = "chatRoom", cascade = CascadeType.ALL) +@OneToMany(mappedBy = "chatRoom", cascade = {CascadeType.PERSIST, CascadeType.MERGE}) private List<ChatParticipant> chatParticipants = new ArrayList<>();
32-33: 메시지 관계의 CASCADE 설정이 적절합니다.메시지에 대한
CascadeType.ALL사용은 적절합니다. 채팅방이 삭제되면 해당 방의 모든 메시지도 함께 삭제되는 것이 일반적인 비즈니스 로직이기 때문입니다.src/main/java/com/example/solidconnection/chat/domain/ChatMessage.java (2)
31-32: 첨부파일 존재 여부 필드가 좋은 설계입니다.
hasAttachment필드를 통한 비정규화는 성능 최적화에 도움이 되는 좋은 설계 선택입니다. 첨부파일 존재 여부를 별도 쿼리 없이 확인할 수 있어요.
39-40: 첨부파일 관계의 CASCADE 설정이 적절합니다.첨부파일에 대한
CascadeType.ALL사용은 적절합니다. 메시지가 삭제되면 해당 메시지의 첨부파일도 함께 삭제되어야 하는 것이 자연스러운 비즈니스 로직입니다.src/main/resources/db/migration/V23__add_chat_related_tables.sql (1)
3-6: 타임스탬프 필드에 기본값을 지정해 주세요
created_at,updated_at컬럼이 NOT NULL 인데 기본값이 없어 INSERT 시 값을 일일이 넣어야 합니다.
DEFAULT CURRENT_TIMESTAMP(6)과ON UPDATE CURRENT_TIMESTAMP(6)를 설정하면 편리하고 오류를 줄일 수 있습니다.
[ suggest_optional_refactor ]Also applies to: 14-15, 27-28, 40-41, 50-51
| @Column(nullable = false, length = 500) | ||
| private String content; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
메시지 내용에 대한 검증을 추가해주세요.
빈 메시지나 공백만 있는 메시지를 방지하기 위한 검증을 추가하는 것이 좋겠습니다.
+import jakarta.validation.constraints.NotBlank;
+
-@Column(nullable = false, length = 500)
+@Column(nullable = false, length = 500)
+@NotBlank
private String content;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Column(nullable = false, length = 500) | |
| private String content; | |
| import jakarta.validation.constraints.NotBlank; | |
| @Column(nullable = false, length = 500) | |
| @NotBlank | |
| private String content; |
🤖 Prompt for AI Agents
In src/main/java/com/example/solidconnection/chat/domain/ChatMessage.java at
lines 28-29, the content field lacks validation to prevent empty or
whitespace-only messages. Add validation logic, such as using annotations like
@NotBlank or implementing a custom validator, to ensure the content is neither
null, empty, nor only whitespace before persisting or processing the message.
src/main/java/com/example/solidconnection/chat/domain/ChatReadStatus.java
Show resolved
Hide resolved
src/main/resources/db/migration/V23__add_chat_related_tables.sql
Outdated
Show resolved
Hide resolved
src/main/resources/db/migration/V23__add_chat_related_tables.sql
Outdated
Show resolved
Hide resolved
src/main/resources/db/migration/V23__add_chat_related_tables.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
간단하게 코드 관련해서만 코멘트 남겼습니다 ㅎㅎ
추가적으로 궁금한 부분은,
길이 제한은 임의로 500으로 설정했는데, 필요 시 변경하겠습니다.
한국말 한 글자가 VARCHAR 3~4자를 차지한다고 알고 있어요.
그래서 500으로 설정한다면 실질적으로 한국어 150자 정도를 작성할 수 있을 것 같아요.
적당하다고 생각합니다!
한가지 주의해야 할 것 같은 부분은, 한국어는 글자수로 VARCHAR 예측이 어려우니
데이터베이스에서 설정한 size >> 입력할 수 있는 크기 제한으로 둬야 할 것 같아요.
예를 들어 입력할 수 있는 글자를 100자, VARCHAR은 500으로 하는식으로요!
만약 간당간당하게 200자까지 허용한다면, 복잡한 자음모음으로 이루어진 글자들만 있다면 저장에 실패할 수 있으니깐요.
혹시 제가 잘못 알고 있는게 있다면 알려주세요!
| @Column(nullable = false) | ||
| private Boolean hasAttachment; |
There was a problem hiding this comment.
null 을 코드단에서 막아주는건 어떤가요?
Boolean -> boolean 을 사용하고, 선언과 동시에 초기화해주는 방법도 있을 것 같아요
e.g. private boolean hasAttachment = false;
그리고 만약 이 코멘트를 반영하신다면,
ddl 단에도 default를 설정해주는게 어떨까요?
There was a problem hiding this comment.
그리고 내부적으로 chatAttachments를 가지고 있음에도 불구하고,
어떤 배경에서 hasAttachment 컬럼이 나왔는지도 궁금합니다! 🧐
There was a problem hiding this comment.
저희도 그거에 대해 얘기를 오래 했었는데요 ㅎㅎ..
처음엔 저도 굳이 반정규화를 할 필요 없다 생각했었는데 매번 chatAttachment가 있는지 확인하는 게 리소스 낭비라는 의견에 설득되어 hasAttachment를 사용하게 되었습니다!
그런데 지금와서 생각해보니 어차피 양방향으로 연관관계가 되어있으니 chatAttachments를 같이 가져온다면 크게 성능차이도 없지 않을까란 생각이 드는 거 같네요! 성혁님과 수연님은 어떻게 생각하시나요?
There was a problem hiding this comment.
hasAttachment필드에 기본값을 세팅하는 것은 좋은 방법 같습니다.- 저희 코드를 보니 기본값을 설정하는 경우 널 방지를 primitive type을 사용하는 거 같아, 따라가는 게 좋을 거 같습니다.
이건 주제와 다소 벗어난 이야기이긴 한데, primitive type을 통해 널 방지를 하는 경우 단독으로 사용된 @Column 어노테이션은 제거해야 할 듯 합니다. 둘을 같이 사용하면 JPA가 DDL 작성 시 NOT NULL 을 적용하지 않습니다. [참고]
- 분명
hasAttachment필드가 있다면 불필요한 쿼리는 줄일 수 있겠지만, 추가로 고려해야 하는 부분도 생기고(데이터 정합성 등), 결정적으로 큰 성능 개선이 있을지는 모르겠네요 .. 실제 서비스를 굴려봐야 알 것 같습니다. 그렇다면 일단 해당 필드 제거하고, 필요 시 작성하는 것이 좋을 것 같습니다.
There was a problem hiding this comment.
primitive type을 통해 널 방지를 하는 경우 단독으로 사용된 @column 어노테이션은 제거해야 할 듯 합니다. 둘을 같이 사용하면 JPA가 DDL 작성 시 NOT NULL 을 적용하지 않습니다.
헉.,! 아이고 이건 처음 알았네요🥲 공유해주셔서 감사합니다!
그럼 원시값 필드에 @Column을 생략하기보다,
@Column(nullable=false)라고 명시하는게 좋을 것 같은데 어떠신가요?
원시값을 사용하면 자바 코드상에서 null 을 막아서 원천 차단할 수도 있기도 하고,
어차피 저희 모든 컬럼에 @Column(name=xxx)을 명시하기로 했었어서요!
There was a problem hiding this comment.
음... 저는 hasAttachment를 가지고 있는 게 좋은 것 같긴 한데 크게 상관은 없습니당 다수 의견에 따르겠습니다
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @Table(uniqueConstraints = { | ||
| @UniqueConstraint( | ||
| name = "uk_chat_participant_chat_room_site_user", |
There was a problem hiding this comment.
저희가 지금 uk_테이블명_컬럼이름들 이라는 규칙으로 이름을 짓고 있어요.
컨벤션으로 논의된 것은 아니지만..
기존 이름과 동일한 규칙으로 가는건 어떤가요?
여기에서는 uk_chat_participant_chat_room_id_site_user_id가 되겠네요!
There was a problem hiding this comment.
변경했습니다 ! 지금처럼 UK, FK 모두 XX_<테이블명>_<컬럼명> 형식으로 가도 괜찮을 것 같습니다. 일반적으로 정해진 컨벤션은 없는데 보통 이렇게 사용하는 경우가 많은 거 같아요
| @UniqueConstraint( | ||
| name = "uk_chat_read_status_chat_room_participant", | ||
| columnNames = {"chat_room_id", "chat_participant_id"} | ||
| ) |
| @Column(nullable = false) | ||
| private Boolean isGroup; |
There was a problem hiding this comment.
Ditto!
ChatRoom 에서는 특히나, 아직 그룹 채팅 기능이 활성화되어있지 않으니 default를 명시하는게 좋을 것 같아요.
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
제가 작업했어야하는데 정신이 너무 없어서 죄송하고 감사합니다...ㅠㅠ
고생하셨습니다! 제가 그때 회의내용이 조금 헷갈리는데 ChatMessage에서 senderId를 long으로 관리하기로 했었나요?
연관관계 매핑하기로 했었던 거 같기도 한데 헷갈리네요!
보기엔 senderId가 확실히 좋아보이긴하지만 이제 해당 메시지의 닉네임이나 프로필 등을 가져오려면 senderId를 통해 ChatParticipant를 가져오고 거기에 있는 siteUserId를 통해 SiteUser를 가져올 수 있을 거 같은데 어떤 게 좋은 거 같으신가요?
- 데이터 타입을 primitive type으로 지정 - Column 어노테이션 제거 (App level과 DB level 간 일관성 보장) - DDL에 기본값 설정
엇 제가 알기론 |
ERD 상엔 연관관계를 설정하지 않아서 그렇게 구현했습니다 ! 말씀하신 거 생각해보니 연관관계가 이점이 있을 것 같으면서도, 영서 님께서 말씀하신 순환 참조가 발생해서 좀 고민이네요 ... 일단 깊게 고민해보겠습니다 |
성혁님이 맞습니다! 제가 오라클이랑 헷갈렸던 것 같아요🥲 |
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
음.. 순환참조도 고려를 해야하니 우선은 그대로 진행하고 나중에 비교를 한 번 해보죠!!
고생하셨습니다!!
4 participants
관련 이슈
작업 내용
작성한 ERD를 기반으로 채팅 관련 엔티티를 작성하였고, flyway 마이그레이션 파일을 작성하였습니다.
특이 사항
nullable은 임의로 판단하여 결정하였습니다.thumbnailUrl은 이미지 파일인 경우만 존재하므로 널 허용으로 설정하였습니다.orphanRemoval은 관련 정책을 확인하지 못해 일단 설정하지 않았습니다. 필요 시 추가 구현하겠습니다.chat_read_status테이블에서(chat_room_id, chat_participant_id)쌍은 고유해야 합니다. 이에 고유 키를 설정해주었습니다.chat_message의content필드의 타입은TEXT로 설정되어 있으나, 성능 상VARCHAR가 유리하기에VARCHAR로 설정하였습니다. 길이 제한은 임의로 500으로 설정했는데, 필요 시 변경하겠습니다.chat_read_status테이블에서chat_room과chat_participant테이블 간 연관관계를 맺지 않는 방향으로 정해진 것 같아 그렇게 구현하였습니다. [관련 논의]hasAttachment컬럼을 제거합니다.리뷰 요구사항 (선택)