Skip to content
Draft
Show file tree
Hide file tree
Changes from 10 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
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) {
}
96 changes: 88 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,52 @@ 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();
review.updateBookReview(description, newRate);

if (oldRate != newRate) {
subtractSumRate(oldRate);
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 +151,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 +187,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);
}
}
26 changes: 17 additions & 9 deletions src/main/java/checkmo/clubMeeting/internal/entity/Topic.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,33 @@ public class Topic extends BaseEntity {
@OneToMany(mappedBy = "topic", cascade = CascadeType.REMOVE, orphanRemoval = true)
private List<TeamTopic> teamTopics = new ArrayList<>();

public boolean isOwnedBy(String anotherMemberId) {
public boolean isAuthoredBy(Long anotherMemberId) {
return this.memberId.equals(anotherMemberId);
}

public boolean isOwnedBy(Long anotherClubMemberId) {
return this.clubMemberId.equals(anotherClubMemberId);
}

public void updateTopic(String description) {
public void updateBy(ClubMeetingActor actor, String description) {
validateAuthorOrStaff(actor);
this.description = description;
}

public void removeBy(ClubMeetingActor actor) {
validateAuthorOrStaff(actor);
if (this.meeting != null) {
this.meeting.getTopics().remove(this);
this.meeting = null;
}
}

private void validateAuthorOrStaff(ClubMeetingActor actor) {
if (!isOwnedBy(actor.clubMemberId()) && !actor.staff()) {
throw new ClubMeetingException(ClubMeetingErrorStatus.TOPIC_FORBIDDEN);
}
}

// == 연관관계 메서드 == //
public void setMeeting(Meeting meeting) {
if (meeting == null) {
Expand All @@ -79,11 +94,4 @@ public void setMeeting(Meeting meeting) {
meeting.getTopics().add(this);
}
}

public void removeMeeting() {
if (this.meeting != null) {
this.meeting.getTopics().remove(this);
this.meeting = null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import checkmo.clubManagement.ClubManagementExternalDTO;
import checkmo.clubMeeting.internal.converter.ClubMeetingConverter;
import checkmo.clubMeeting.internal.entity.BookReview;
import checkmo.clubMeeting.internal.entity.ClubMeetingActor;
import checkmo.clubMeeting.internal.entity.Meeting;
import checkmo.clubMeeting.internal.exception.ClubMeetingErrorStatus;
import checkmo.clubMeeting.internal.exception.ClubMeetingException;
Expand Down Expand Up @@ -43,9 +44,7 @@ public void createBookReview(Long clubId, Long meetingId, Long memberId, BookRev
Meeting meeting = clubMeetingQueryService.validateMeeting(clubId, meetingId);

BookReview bookReview = ClubMeetingConverter.toBookReview(request, clubMemberId, memberId);
bookReview.setMeeting(meeting);

meeting.addSumRate(bookReview.getRate());
meeting.addBookReview(bookReview);

bookReviewRepository.save(bookReview);
}
Expand All @@ -61,26 +60,19 @@ public void updateBookReview(Long clubId, Long meetingId, Long reviewId, Long me
if (!clubMembership.isActive()) {
throw new ClubMeetingException(ClubMeetingErrorStatus.CLUB_MEMBER_INACTIVE);
}
ClubMeetingActor actor = new ClubMeetingActor(
clubMembership.getClubMemberId(),
clubMembership.isStaff()
);
Meeting meeting = clubMeetingQueryService.validateMeeting(clubId, meetingId);

BookReview bookReview = clubBookReviewQueryService.validateBookReview(reviewId, meeting.getId());
if (!bookReview.isOwnedBy(clubMembership.getClubMemberId()) && !clubMembership.isStaff()) {
throw new ClubMeetingException(ClubMeetingErrorStatus.BOOK_REVIEW_FORBIDDEN);
}

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

bookReview.updateBookReview(
meeting.reviseBookReviewBy(
actor,
bookReview,
request.getDescription(),
request.getRate()
);

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

@Retryable(
Expand All @@ -94,16 +86,14 @@ public void deleteBookReview(Long clubId, Long meetingId, Long reviewId, Long me
if (!clubMembership.isActive()) {
throw new ClubMeetingException(ClubMeetingErrorStatus.CLUB_MEMBER_INACTIVE);
}
ClubMeetingActor actor = new ClubMeetingActor(
clubMembership.getClubMemberId(),
clubMembership.isStaff()
);
Meeting meeting = clubMeetingQueryService.validateMeeting(clubId, meetingId);

BookReview bookReview = clubBookReviewQueryService.validateBookReview(reviewId, meeting.getId());
if (!bookReview.isOwnedBy(clubMembership.getClubMemberId()) && !clubMembership.isStaff()) {
throw new ClubMeetingException(ClubMeetingErrorStatus.BOOK_REVIEW_FORBIDDEN);
}

meeting.subtractSumRate(bookReview.getRate());

bookReview.removeMeeting();
meeting.removeBookReviewBy(actor, bookReview);
}

}
Loading