Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 48 additions & 16 deletions docs/conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,37 +121,69 @@ public class ClubBookReviewCommandService {
private final ClubManagementAPI clubManagementAPI;
private final ClubMeetingQueryService clubMeetingQueryService;
private final ClubBookReviewQueryService clubBookReviewQueryService;
private final BookReviewRepository bookReviewRepository;

@Retryable(
retryFor = OptimisticLockingFailureException.class,
maxAttempts = 5,
backoff = @Backoff(delay = 300)
)
public Long updateBookReview(Long meetingId, Long reviewId, String memberId, BookReviewCreate request) {
Meeting meeting = clubMeetingQueryService.validateMeeting(meetingId);
Long clubMemberId = clubManagementAPI.fetchActiveClubMemberId(meeting.getClubId(), memberId);

BookReview bookReview = clubBookReviewQueryService.validateBookReview(reviewId, meeting.getId());
if (!bookReview.getClubMemberId().equals(clubMemberId)) {
throw new ClubMeetingException(ClubMeetingErrorStatus.BOOK_REVIEW_FORBIDDEN);
public void updateBookReview(
Long clubId,
Long meetingId,
Long reviewId,
Long memberId,
BookReviewCreate request
) {
clubManagementAPI.validateClub(clubId);
ClubManagementExternalDTO.MembershipInfo membership =
clubManagementAPI.fetchMembershipInfo(clubId, memberId);
if (!membership.isActive()) {
throw new ClubMeetingException(ClubMeetingErrorStatus.CLUB_MEMBER_INACTIVE);
}
ClubMeetingActor actor = new ClubMeetingActor(
membership.getClubMemberId(),
membership.isStaff()
);
Meeting meeting = clubMeetingQueryService.validateMeeting(clubId, meetingId);

double oldRate = bookReview.getRate();
double newRate = request.getRate();

bookReview.updateBookReview(
BookReview bookReview = clubBookReviewQueryService.validateBookReview(reviewId, meeting.getId());
meeting.reviseBookReviewBy(
actor,
bookReview,
request.getDescription(),
request.getRate()
);
}
}
```

서비스는 조회와 외부 모듈 협력을 조율하고, 한줄평 수정 권한과 별점 합계 변경 순서는 `Meeting`이 책임집니다.

```java

public class Meeting {
public void reviseBookReviewBy(
ClubMeetingActor actor,
BookReview review,
String description,
double newRate
) {
validateBookReviewAuthorOrStaff(actor, review);
reviseBookReview(review, description, newRate);
}

private void reviseBookReview(BookReview review, String description, double newRate) {
double oldRate = review.getRate();

// 별점이 변경된 경우에만 미팅의 별점 합산
if (oldRate != newRate) {
meeting.subtractSumRate(oldRate);
meeting.addSumRate(newRate);
subtractSumRate(oldRate);
}

return bookReview.getId();
review.updateBookReview(description, newRate);

if (oldRate != newRate) {
addSumRate(newRate);
}
}
}
```
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public static BookShelfResponseDTO.TopicDetail toTopicDetailDTO(
.topicId(topic.getId())
.content(topic.getDescription())
.authorInfo(authorInfo)
.author(topic.isOwnedBy(memberId))
.author(topic.isAuthoredBy(memberId))
.build();
}

Expand Down Expand Up @@ -244,4 +244,4 @@ public static DetailInfo toMeetingInfoExternalDTO(
.bookInfo(bookInfo)
.build();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public class BookReview extends BaseEntity {
@JoinColumn(name = "meeting_id")
private Meeting meeting;

public void updateBookReview(String description, double rate) {
void updateBookReview(String description, double rate) {
this.description = description;
this.rate = rate;
}
Expand All @@ -52,7 +52,7 @@ public boolean isOwnedBy(Long clubMemberId) {
return this.clubMemberId.equals(clubMemberId);
}

public void setMeeting(Meeting meeting) {
void setMeeting(Meeting meeting) {
if (this.meeting == meeting) {
return;
}
Expand All @@ -68,7 +68,7 @@ public void setMeeting(Meeting meeting) {
}
}

public void removeMeeting() {
void removeMeeting() {
if (this.meeting != null) {
this.meeting.getBookReviews().remove(this);
this.meeting = null;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package checkmo.clubMeeting.internal.entity;

public record ClubMeetingActor(Long clubMemberId, boolean staff) {
}
100 changes: 92 additions & 8 deletions src/main/java/checkmo/clubMeeting/internal/entity/Meeting.java
Original file line number Diff line number Diff line change
@@ -1,12 +1,27 @@
package checkmo.clubMeeting.internal.entity;

import checkmo.clubMeeting.internal.exception.ClubMeetingErrorStatus;
import checkmo.clubMeeting.internal.exception.ClubMeetingException;
import checkmo.common.BaseEntity;
import jakarta.persistence.*;
import lombok.*;

import jakarta.persistence.CascadeType;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.OneToMany;
import jakarta.persistence.Version;
import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.function.Function;
import java.util.stream.Collectors;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;


@Getter
Expand Down Expand Up @@ -44,14 +59,17 @@ public class Meeting extends BaseEntity {
@Column(name = "book_id", nullable = false)
private String bookId;

@Getter(AccessLevel.PACKAGE)
@Builder.Default
@OneToMany(mappedBy = "meeting", cascade = CascadeType.ALL, orphanRemoval = true)
private List<Team> teams = new ArrayList<>();

@Getter(AccessLevel.PACKAGE)
@Builder.Default
@OneToMany(mappedBy = "meeting", cascade = CascadeType.REMOVE, orphanRemoval = true)
private List<Topic> topics = new ArrayList<>();

@Getter(AccessLevel.PACKAGE)
@Builder.Default
@OneToMany(mappedBy = "meeting", cascade = CascadeType.REMOVE, orphanRemoval = true)
private List<BookReview> bookReviews = new ArrayList<>();
Expand All @@ -71,11 +89,56 @@ public void updateMeeting(
this.tag = tag;
}

public void addSumRate(double rate) {
public void addBookReview(BookReview review) {
review.setMeeting(this);
addSumRate(review.getRate());
}

public void reviseBookReviewBy(
ClubMeetingActor actor,
BookReview review,
String description,
double newRate
) {
validateBookReviewAuthorOrStaff(actor, review);
reviseBookReview(review, description, newRate);
}

public void removeBookReviewBy(ClubMeetingActor actor, BookReview review) {
validateBookReviewAuthorOrStaff(actor, review);
removeBookReview(review);
}

private void validateBookReviewAuthorOrStaff(ClubMeetingActor actor, BookReview review) {
if (!review.isOwnedBy(actor.clubMemberId()) && !actor.staff()) {
throw new ClubMeetingException(ClubMeetingErrorStatus.BOOK_REVIEW_FORBIDDEN);
}
}

private void reviseBookReview(BookReview review, String description, double newRate) {
double oldRate = review.getRate();

if (oldRate != newRate) {
subtractSumRate(oldRate);
}

review.updateBookReview(description, newRate);

if (oldRate != newRate) {
addSumRate(newRate);
}
}
Comment on lines +118 to +130

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

🐛 버그 리포트: 별점 수정 시 합계 재계산 로직의 왜곡 발생

Meeting.reviseBookReview 메서드에서 review.updateBookReview(description, newRate)를 호출하여 BookReview 객체의 별점(rate)을 먼저 변경한 뒤, subtractSumRate(oldRate)를 호출하고 있습니다.

이때, 만약 subtractSumRate 내부에서 this.sumRate < rate 조건이 참이 되어 별점 합계 재계산(this.bookReviews.stream().mapToDouble(BookReview::getRate).sum())이 수행되면 심각한 버그가 발생합니다.

🔍 원인 분석

  1. review.updateBookReview가 이미 호출되었으므로, this.bookReviews 목록에 있는 해당 리뷰의 별점은 이미 **새로운 별점(newRate)**으로 반영되어 있습니다.
  2. 따라서 재계산된 sumRate는 이미 newRate를 포함하고 있으며, oldRate는 포함하고 있지 않습니다.
  3. 하지만 subtractSumRate는 재계산된 값에서 다시 oldRate를 차감하고, 이후 reviseBookReview에서 addSumRate(newRate)를 호출하여 newRate를 한 번 더 더하게 됩니다.
  4. 결과적으로 재계산이 발생할 경우 sumRate(다른 리뷰들의 합 + newRate) - oldRate + newRate가 되어, 실제 별점 합계보다 newRate - oldRate 만큼 왜곡된 잘못된 값이 저장됩니다.

🛠️ 해결 방안

subtractSumRate(oldRate)를 호출하여 기존 별점을 먼저 차감한 이후에 review.updateBookReview를 호출하고, 마지막으로 addSumRate(newRate)를 호출하도록 순서를 변경해야 합니다.

Suggested change
private void reviseBookReview(BookReview review, String description, double newRate) {
double oldRate = review.getRate();
review.updateBookReview(description, newRate);
if (oldRate != newRate) {
subtractSumRate(oldRate);
addSumRate(newRate);
}
}
private void reviseBookReview(BookReview review, String description, double newRate) {
double oldRate = review.getRate();
if (oldRate != newRate) {
subtractSumRate(oldRate);
review.updateBookReview(description, newRate);
addSumRate(newRate);
} else {
review.updateBookReview(description, newRate);
}
}


private void removeBookReview(BookReview review) {
subtractSumRate(review.getRate());
review.removeMeeting();
}

private void addSumRate(double rate) {
this.sumRate += rate;
}

public void subtractSumRate(double rate) {
private void subtractSumRate(double rate) {
if (this.sumRate < rate) {
this.sumRate = this.bookReviews.stream()
.mapToDouble(BookReview::getRate)
Expand All @@ -92,8 +155,30 @@ public LocalDateTime getChatDeadline() {
return this.getMeetingTime().plusDays(CHAT_AVAILABLE_DAYS_AFTER_MEETING);
}

public void organizeTeams(Map<Integer, List<Long>> requestedMembersByTeamNumber) {
Map<Integer, Team> existingTeamsByTeamNumber = this.teams.stream()
.collect(Collectors.toMap(Team::getTeamNumber, Function.identity()));

for (Map.Entry<Integer, List<Long>> entry : requestedMembersByTeamNumber.entrySet()) {
Team team = existingTeamsByTeamNumber.get(entry.getKey());
if (team == null) {
team = Team.builder()
.teamNumber(entry.getKey())
.build();
addTeam(team);
}
team.replaceMembers(entry.getValue());
}

for (Team team : new ArrayList<>(this.teams)) {
if (!requestedMembersByTeamNumber.containsKey(team.getTeamNumber())) {
removeTeam(team);
}
}
}

// ========= 연관관계 메서드 =========
public void addTeam(Team team) {
private void addTeam(Team team) {
if (team == null) {
return;
}
Expand All @@ -106,11 +191,10 @@ public void removeAllTeams() {
}
}

public void removeTeam(Team team) {
private void removeTeam(Team team) {
if (team == null) {
return;
}
team.removeMeeting();
}

}
33 changes: 24 additions & 9 deletions src/main/java/checkmo/clubMeeting/internal/entity/Team.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;
import org.hibernate.annotations.BatchSize;

@Getter
@Builder
Expand All @@ -44,10 +45,12 @@ public class Team extends BaseEntity {
@JoinColumn(name = "meeting_id", nullable = false)
private Meeting meeting;

@BatchSize(size = 12)
@OneToMany(mappedBy = "team", cascade = CascadeType.ALL, orphanRemoval = true)
@Builder.Default
private List<TeamTopic> teamTopics = new ArrayList<>();

@BatchSize(size = 12)
@OneToMany(mappedBy = "team", cascade = CascadeType.ALL, orphanRemoval = true)
@Builder.Default
private List<ClubMemberTeam> clubMemberTeams = new ArrayList<>();
Expand Down Expand Up @@ -76,23 +79,35 @@ public void removeMeeting() {
}
}

public void addClubMemberTeam(ClubMemberTeam clubMemberTeam) {
if (clubMemberTeam == null) {
public void replaceMembers(List<Long> clubMemberIds) {
if (hasSameMembers(clubMemberIds)) {
return;
}
clubMemberTeam.setTeam(this);
this.clubMemberTeams.clear();
for (Long clubMemberId : clubMemberIds) {
addClubMemberTeam(ClubMemberTeam.builder()
.clubMemberId(clubMemberId)
.build());
}
}

public void removeAllClubMemberTeams() {
for (ClubMemberTeam cmt : new ArrayList<>(this.clubMemberTeams)) {
removeClubMemberTeam(cmt);
private boolean hasSameMembers(List<Long> clubMemberIds) {
if (clubMemberTeams.size() != clubMemberIds.size()) {
return false;
}
List<Long> unmatchedClubMemberIds = new ArrayList<>(clubMemberIds);
for (ClubMemberTeam clubMemberTeam : clubMemberTeams) {
if (!unmatchedClubMemberIds.remove(clubMemberTeam.getClubMemberId())) {
return false;
}
}
return unmatchedClubMemberIds.isEmpty();
}

private void removeClubMemberTeam(ClubMemberTeam clubMemberTeam) {
if (clubMemberTeam == null || !clubMemberTeams.contains(clubMemberTeam)) {
private void addClubMemberTeam(ClubMemberTeam clubMemberTeam) {
if (clubMemberTeam == null) {
return;
}
this.clubMemberTeams.remove(clubMemberTeam);
clubMemberTeam.setTeam(this);
}
}
Loading