Conversation
Walkthrough이번 변경 사항에서는 동아리(Club) 관련 데이터 구조에서 기존의 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Club Entity
Client->>Controller: ClubInfoRequest (socialLinks 포함)
Controller->>Service: ClubInfoRequest 전달
Service->>Club Entity: update(ClubInfoRequest)
Club Entity-->>Service: socialLinks 필드 업데이트
Service-->>Controller: 처리 결과 반환
Controller-->>Client: ClubDetailedResult (socialLinks 포함)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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 (
|
Test Results1 tests 1 ✅ 0s ⏱️ Results for commit 430dfa5. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/main/java/moadong/club/entity/Club.java(3 hunks)backend/src/main/java/moadong/club/entity/ClubRecruitmentInformation.java(3 hunks)backend/src/main/java/moadong/club/payload/dto/ClubDetailedResult.java(3 hunks)backend/src/main/java/moadong/club/payload/request/ClubInfoRequest.java(2 hunks)
🔇 Additional comments (8)
backend/src/main/java/moadong/club/payload/request/ClubInfoRequest.java (1)
5-5: Map 타입 추가 확인Map 타입이 정상적으로 import 되었습니다.
backend/src/main/java/moadong/club/entity/Club.java (1)
9-9: Map 타입 추가 확인Map 타입이 정상적으로 import 되었습니다.
backend/src/main/java/moadong/club/entity/ClubRecruitmentInformation.java (4)
18-18: import 순서 변경import 순서가 변경되었지만 코드 기능에는 영향을 미치지 않습니다.
91-93: 메서드 포맷팅 변경메서드 포맷팅이 변경되었지만 로직은 동일하게 유지됩니다.
95-97: 메서드 포맷팅 변경메서드 포맷팅이 변경되었지만 로직은 동일하게 유지됩니다.
99-104:Details
❓ Verification inconclusive
recruitmentForm 필드 관련 로직 제거
update 메서드에서 recruitmentForm 관련 로직이 제거되었습니다. 기존 데이터에 대한 마이그레이션 계획이 필요합니다.
기존 데이터베이스에 recruitmentForm 값이 있는 레코드가 있을 경우, 이 값들을 새로운 socialLinks 필드로 마이그레이션하는 계획을 수립하세요. 데이터 손실을 방지하기 위해 마이그레이션 스크립트를 작성하는 것이 좋습니다.
🏁 Script executed:
#!/bin/bash # 기존 recruitmentForm 데이터가 있는지 확인 # MongoDB 컬렉션을 조회하여 recruitmentForm 필드를 사용하고 있는 문서가 있는지 확인 echo "기존 recruitmentForm 데이터를 확인하고 있습니다..." mongo --eval "db.clubs.find({\"clubRecruitmentInformation.recruitmentForm\": {\$exists: true}}).count()"Length of output: 282
recruitmentForm 필드 마이그레이션 검토 필요
update() 메서드에서
recruitmentForm관련 로직이 제거되었습니다. 기존 DB에 해당 필드 값이 남아있는지 확인하고, 새로운socialLinks필드로 안전하게 마이그레이션할 계획을 수립하세요.
- 위치:
backend/src/main/java/moadong/club/entity/ClubRecruitmentInformation.java
public void update(ClubInfoRequest request)메서드- 검증 예시 (운영 DB에서 수동 실행):
mongo --eval 'db.clubs.find({"clubRecruitmentInformation.recruitmentForm": {$exists: true}}).count()'- 확인 후 데이터 손실 방지를 위해 마이그레이션 스크립트 작성 권장
실제 운영 환경에서 수동으로 남아있는
recruitmentForm데이터를 확인하고, 필요한 경우 이를socialLinks로 이전하는 스크립트를 작성해 주세요.backend/src/main/java/moadong/club/payload/dto/ClubDetailedResult.java (2)
5-5: Map 타입 추가 확인Map 타입이 정상적으로 import 되었습니다.
25-25: socialLinks 필드 추가socialLinks 필드가 올바르게 추가되었습니다.
| String presidentName, | ||
| String presidentPhoneNumber, | ||
| String recruitmentForm | ||
| Map<String, String> socialLinks |
There was a problem hiding this comment.
🛠️ Refactor suggestion
socialLinks 필드에 유효성 검증 추가 필요
socialLinks 필드에 유효성 검증 로직이 없습니다. 지정된 소셜 미디어 키와 URL 형식을 검증하는 로직이 필요합니다.
다음과 같이 Jakarta Validation API의 @Valid와 커스텀 검증기를 사용하는 방법을 고려해 보세요:
import jakarta.validation.constraints.NotBlank;
import java.util.List;
import java.util.Map;
+import jakarta.validation.Valid;
public record ClubInfoRequest(
@NotBlank
String id,
@NotBlank
String name,
@NotBlank
String category,
@NotBlank
String division,
List<String> tags,
String introduction,
String presidentName,
String presidentPhoneNumber,
+ @Valid
Map<String, String> socialLinks
) {
}또한 socialLinks 필드에 대한 JavaDoc 또는 주석을 추가하여 허용되는 키 형식(예: "instagram", "facebook")과 URL 형식을 명시하는 것이 좋습니다.
📝 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.
| Map<String, String> socialLinks | |
| import jakarta.validation.constraints.NotBlank; | |
| import java.util.List; | |
| import java.util.Map; | |
| +import jakarta.validation.Valid; | |
| public record ClubInfoRequest( | |
| @NotBlank | |
| String id, | |
| @NotBlank | |
| String name, | |
| @NotBlank | |
| String category, | |
| @NotBlank | |
| String division, | |
| List<String> tags, | |
| String introduction, | |
| String presidentName, | |
| String presidentPhoneNumber, | |
| + @Valid | |
| Map<String, String> socialLinks | |
| ) { | |
| } |
| this.category = request.category(); | ||
| this.division = request.division(); | ||
| this.state = ClubState.AVAILABLE; | ||
| this.socialLinks = request.socialLinks(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
null 체크 없이 socialLinks 업데이트
request.socialLinks()가 null일 경우 NPE가 발생할 수 있습니다.
다음과 같이 방어적 코딩을 추가하세요:
public void update(ClubInfoRequest request) {
this.name = request.name();
this.category = request.category();
this.division = request.division();
this.state = ClubState.AVAILABLE;
- this.socialLinks = request.socialLinks();
+ this.socialLinks = request.socialLinks() != null ? request.socialLinks() : new HashMap<>();
this.clubRecruitmentInformation.update(request);
}또는 Optional을 사용하는 방법도 고려할 수 있습니다.
📝 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.
| this.socialLinks = request.socialLinks(); | |
| public void update(ClubInfoRequest request) { | |
| this.name = request.name(); | |
| this.category = request.category(); | |
| this.division = request.division(); | |
| this.state = ClubState.AVAILABLE; | |
| this.socialLinks = request.socialLinks() != null | |
| ? request.socialLinks() | |
| : new HashMap<>(); | |
| this.clubRecruitmentInformation.update(request); | |
| } |
|
|
||
| private String userId; | ||
|
|
||
| private Map<String, String> socialLinks; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
socialLinks 필드 추가
socialLinks 필드가 올바르게 추가되었습니다. 그러나 생성자에서 초기화되지 않아 NPE 위험이 있습니다.
생성자에서 빈 맵으로 초기화하는 것이 좋습니다:
public Club() {
this.name = "";
this.category = "";
this.division = "";
this.state = ClubState.UNAVAILABLE;
this.clubRecruitmentInformation = ClubRecruitmentInformation.builder().build();
+ this.socialLinks = new HashMap<>();
}
public Club(String userId) {
this.name = "";
this.category = "";
this.division = "";
this.state = ClubState.UNAVAILABLE;
this.clubRecruitmentInformation = ClubRecruitmentInformation.builder().build();
this.userId = userId;
+ this.socialLinks = new HashMap<>();
}또한 Builder 사용 시에도 초기화를 고려하세요.
📝 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.
| private Map<String, String> socialLinks; | |
| public Club() { | |
| this.name = ""; | |
| this.category = ""; | |
| this.division = ""; | |
| this.state = ClubState.UNAVAILABLE; | |
| this.clubRecruitmentInformation = ClubRecruitmentInformation.builder().build(); | |
| this.socialLinks = new HashMap<>(); | |
| } | |
| public Club(String userId) { | |
| this.name = ""; | |
| this.category = ""; | |
| this.division = ""; | |
| this.state = ClubState.UNAVAILABLE; | |
| this.clubRecruitmentInformation = ClubRecruitmentInformation.builder().build(); | |
| this.userId = userId; | |
| this.socialLinks = new HashMap<>(); | |
| } |
|
|
||
| private String userId; | ||
|
|
||
| private Map<String, String> socialLinks; |
There was a problem hiding this comment.
객체를 쓰게 되면 SNS의 확장성을 제한한다고 생각하였습니다. 대신 Map을 사용한다면 사용자들이 원하는 SNS에 대한 정보를 보다 자유롭게 제공할 수 있을것 같아요.
There was a problem hiding this comment.
만약 Map을 사용한다면 링크 검증같은 것이 불가능해질 것 같습니다. 클래스로 바꾸는 것이 확장성을 오히려 고려하는 건 아닐까요?
There was a problem hiding this comment.
클래스를 꼭 만들지 않고도 링크 검증은 가능할것 같습니다만, 검증이 추가될거라면 자가검증 방식을 사용하는게 좋아보이긴 합니다. 다만 링크검증의 필요성에 대해 의문이 생기네요. 어떤 이유로 링크 검증이 필요하다고 생각하시나요?
There was a problem hiding this comment.
인스타 아이콘을 눌렀는데 유튜브나 블로그가 나오는 건 사용자 경험 측면에서 안좋을 것 같아요. 그래서 검증이 필요하다고 생각했습니다.
There was a problem hiding this comment.
추가로 클래스가 좋다고 생각한 이유는 검증 로직이 엔티티안에 있으면 깔끔할 거 같아서 그랬습니다.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
|
||
| private String userId; | ||
|
|
||
| private Map<String, String> socialLinks; |
There was a problem hiding this comment.
주로 사용되는 SNS의 종류가 몇 종류 없다는 점과 키값 일관성과 검증도 된다는 점에서 키를 enum으로 구성해보는건 어떨까요? e.g. 키값이 "Youtube"인 곳도 있고 "yotube" 인 곳도 생길 것 같습니다
SNS의 확장성을 고려한다면 비주류 SNS들은 사용자가 선택지 추가를 요청하게 해서 확장성을 확보하는 방식으로 구성하는건 어떨까요?
There was a problem hiding this comment.
key를 enum으로 규제하는건 일관성을 지키기에 좋은 방법인것 같네요.
다만, 저는 사용자가 요청후 확장같은 번거로운 절차 없이 자유롭게 sns를 지정하는게 좋지 않을까 싶었습니다. 만약 사전에 정의된 SNS내에서 사용한다고 해도 사용자가 api요청을 직접하지 않는 이상 클라이언트 선에서 일관성을 유지하는게 가능하다고도 생각하는데 어떻게 생각하시나요?
There was a problem hiding this comment.
말씀 들어보니 자유로운 확장성을 위해 클라이언트측에서 일관성을 유지하도록하고 저희는 가능성을 열어두는게 더 좋아보이네요!
3 participants
#️⃣연관된 이슈
#362
📝 작업 내용
동아리 관리자는 동아리 관련 SNS 관련 링크를 제공할 수 있습니다.
Summary by CodeRabbit
신규 기능
변경 사항