Skip to content
Merged
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.debatetimer.dto.customize.request;

public record BellRequest(
int time,
int count
) {
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.debatetimer.entity.customize.CustomizeTimeBox;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;
import java.util.List;
import org.springframework.lang.Nullable;

public record CustomizeTimeBoxCreateRequest(
Expand All @@ -22,6 +23,9 @@ public record CustomizeTimeBoxCreateRequest(
@Nullable
Integer time,

@Nullable
List<BellRequest> bell,
Comment on lines +26 to +27
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

새로운 bell 필드가 추가되었지만 변환 로직에서 처리되지 않습니다.

bell 필드가 요청 DTO에 추가되었지만, toTimeBox 메서드에서 이 데이터를 처리하지 않고 있습니다. 이로 인해 클라이언트가 전송한 벨 설정 정보가 손실될 수 있습니다.

🤖 Prompt for AI Agents
In
src/main/java/com/debatetimer/dto/customize/request/CustomizeTimeBoxCreateRequest.java
around lines 26 to 27, the new 'bell' field is added but not handled in the
'toTimeBox' method. Update the 'toTimeBox' method to include logic that converts
and assigns the 'bell' list from the request DTO to the corresponding field in
the TimeBox entity, ensuring the bell settings from the client are preserved.


@Nullable
Integer timePerTeam,

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.debatetimer.dto.customize.response;

public record BellResponse(
int time,
int count
) {
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ public CustomizeTableResponse(
toTimeBoxResponses(customizeTimeBoxes));
}

public CustomizeTableResponse(
CustomizeTable customizeTable,
List<CustomizeTimeBoxResponse> timeBoxResponses
) {
this(customizeTable.getId(), new CustomizeTableInfoResponse(customizeTable), timeBoxResponses);
}

private static List<CustomizeTimeBoxResponse> toTimeBoxResponses(CustomizeTimeBoxes timeBoxes) {
List<CustomizeTimeBox> customizeTimeBoxes = timeBoxes.getTimeBoxes();
return customizeTimeBoxes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@
import com.debatetimer.domain.customize.CustomizeBoxType;
import com.debatetimer.domain.customize.Stance;
import com.debatetimer.entity.customize.CustomizeTimeBox;
import java.util.List;

public record CustomizeTimeBoxResponse(
Stance stance,
String speechType,
CustomizeBoxType boxType,
Integer time,
List<BellResponse> bell,
Integer timePerTeam,
Integer timePerSpeaking,
String speaker
Expand All @@ -20,6 +22,20 @@ public CustomizeTimeBoxResponse(CustomizeTimeBox customizeTimeBox) {
customizeTimeBox.getSpeechType(),
customizeTimeBox.getBoxType(),
convertTime(customizeTimeBox),
null,
Copy link
Contributor

Choose a reason for hiding this comment

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

[추후 논의 부탁]

  • 개인적으로 논리의 비일관성이 생기고 분기문이 발생하는 이유가 이 nullable 함 때문이라고 생각해요
    ex) 일반 타임박스에는 벨이 담길 수 있으나, 커스텀은 null이어야 함 등등

Request에서 Dto to Domain을 할때 빈 리스트를 반환하거나 초기화 방식으로 논리 일관성을 가져가면 어떨까요? 그럼 비토가 쓴 서비스 코드도 null을 의식하지 않고 분기문없이 초기화가능할 것 같아서요

(아... 코틀린 배우는 중이라 그런게 코틀린 마렵네 😄 )

customizeTimeBox.getTimePerTeam(),
customizeTimeBox.getTimePerSpeaking(),
customizeTimeBox.getSpeaker()
);
}

public CustomizeTimeBoxResponse(CustomizeTimeBox customizeTimeBox, List<BellResponse> bell) {
this(
customizeTimeBox.getStance(),
customizeTimeBox.getSpeechType(),
customizeTimeBox.getBoxType(),
convertTime(customizeTimeBox),
bell,
customizeTimeBox.getTimePerTeam(),
customizeTimeBox.getTimePerSpeaking(),
customizeTimeBox.getSpeaker()
Expand Down
60 changes: 60 additions & 0 deletions src/main/java/com/debatetimer/entity/customize/BellEntity.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package com.debatetimer.entity.customize;

import com.debatetimer.exception.custom.DTClientErrorException;
import com.debatetimer.exception.errorcode.ClientErrorCode;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.FetchType;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.Table;
import jakarta.validation.constraints.NotNull;
import lombok.AccessLevel;
import lombok.Getter;
import lombok.NoArgsConstructor;

@Table(name = "bell")
@Entity
@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class BellEntity {

public static final int MAX_BELL_COUNT = 3;

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@NotNull
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "customize_time_box_id")
private CustomizeTimeBox customizeTimeBox;

@Column(name = "bell_time")
private int time;
private int count;

public BellEntity(CustomizeTimeBox customizeTimeBox, int time, int count) {
validateTime(time);
validateCount(count);

this.customizeTimeBox = customizeTimeBox;
this.time = time;
this.count = count;
}

private void validateTime(int time) {
if (time < 0) {
throw new DTClientErrorException(ClientErrorCode.INVALID_BELL_TIME);
}
}

private void validateCount(int count) {
if (count <= 0 || count > MAX_BELL_COUNT) {
throw new DTClientErrorException(ClientErrorCode.INVALID_BELL_COUNT);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.debatetimer.domain.customize.Agenda;
import com.debatetimer.domain.customize.TableName;
import com.debatetimer.domain.customize.TeamName;
import com.debatetimer.entity.customize.BellEntity;
import com.debatetimer.entity.customize.CustomizeTimeBox;
import lombok.Getter;
import org.springframework.http.HttpStatus;
Expand Down Expand Up @@ -65,6 +66,9 @@ public enum ClientErrorCode implements ResponseErrorCode {
ALREADY_DISCONNECTED(HttpStatus.BAD_REQUEST, "이미 클라이언트에서 요청이 종료되었습니다."),
NO_COOKIE_FOUND(HttpStatus.BAD_REQUEST, "필수 쿠키 값이 존재하지 않습니다."),
FILE_UPLOAD_ERROR(HttpStatus.BAD_REQUEST, "파일 업로드에 실패했습니다."),

INVALID_BELL_TIME(HttpStatus.BAD_REQUEST, "벨 시간은 0 이상의 정수여야 합니다."),
INVALID_BELL_COUNT(HttpStatus.BAD_REQUEST, "벨 카운트는 1 이상 %d 이하의 정수여야 합니다.".formatted(BellEntity.MAX_BELL_COUNT)),
;

private final HttpStatus status;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package com.debatetimer.repository.customize;

import com.debatetimer.entity.customize.BellEntity;
import com.debatetimer.entity.customize.CustomizeTimeBox;
import java.util.List;
import org.springframework.data.repository.Repository;

public interface BellRepository extends Repository<BellEntity, Long> {

BellEntity save(BellEntity bell);

List<BellEntity> findByCustomizeTimeBox(CustomizeTimeBox customizeTimeBox);

void deleteAllByCustomizeTimeBoxIn(List<CustomizeTimeBox> customizeTimeBoxes);
Copy link
Contributor

Choose a reason for hiding this comment

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

[질문]

오... 이거 in절로 해서 한번에 찾아와지는 건가요? 신기..🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

처음 보는 쿼리 메소드라 궁금해서 그런데 이거 @Modifying, clearAutomatically, flush 설정 없이 영속성 컨텍스트 1차 캐시를 활용해 동작하는 것인가요? 아니면 deleteAllInBatch 처럼 영속성 컨텍스트 반영 없이 바로 DB에 쏘아지게 되는건가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

전자입니다. 다만 그로인해 delete 쿼리가 customizeTimeBoxes의 수만큼 나가게 되긴하지만 악의적 유저의 요청을 제외하곤 잘 처리될 것 같습니다.

악의적 유저가 많은 것 같긴한데... 일단 좀 더 고민해보죠

Copy link
Contributor

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.

낙낙 패거리에 비토가 포함되어 있는 거잖아요?
그렇다면 비토 패거리 아닐까요?


List<BellEntity> findAllByCustomizeTimeBoxIn(List<CustomizeTimeBox> timeBoxes);
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
import com.debatetimer.dto.customize.request.CustomizeTableCreateRequest;
import com.debatetimer.dto.customize.response.CustomizeTableResponse;
import com.debatetimer.entity.customize.CustomizeTableEntity;
import com.debatetimer.exception.custom.DTClientErrorException;
import com.debatetimer.exception.errorcode.ClientErrorCode;
import com.debatetimer.repository.customize.CustomizeTableRepository;
import com.debatetimer.repository.customize.CustomizeTimeBoxRepository;
import java.util.List;
Expand All @@ -33,8 +31,7 @@ public CustomizeTableResponse save(CustomizeTableCreateRequest tableCreateReques

@Transactional(readOnly = true)
public CustomizeTableResponse findTable(long tableId, Member member) {
CustomizeTableEntity tableEntity = tableRepository.findByIdAndMember(tableId, member)
.orElseThrow(() -> new DTClientErrorException(ClientErrorCode.TABLE_NOT_FOUND));
CustomizeTableEntity tableEntity = tableRepository.getByIdAndMember(tableId, member);
CustomizeTimeBoxes timeBoxes = timeBoxRepository.findTableTimeBoxes(tableEntity);
return new CustomizeTableResponse(tableEntity.toDomain(), timeBoxes);
}
Expand All @@ -56,8 +53,7 @@ public CustomizeTableResponse updateTable(

@Transactional
public CustomizeTableResponse updateUsedAt(long tableId, Member member) {
CustomizeTableEntity tableEntity = tableRepository.findByIdAndMember(tableId, member)
.orElseThrow(() -> new DTClientErrorException(ClientErrorCode.TABLE_NOT_FOUND));
CustomizeTableEntity tableEntity = tableRepository.getByIdAndMember(tableId, member);
CustomizeTimeBoxes timeBoxes = timeBoxRepository.findTableTimeBoxes(tableEntity);
tableEntity.updateUsedAt();

Expand All @@ -66,8 +62,7 @@ public CustomizeTableResponse updateUsedAt(long tableId, Member member) {

@Transactional
public void deleteTable(long tableId, Member member) {
CustomizeTableEntity table = tableRepository.findByIdAndMember(tableId, member)
.orElseThrow(() -> new DTClientErrorException(ClientErrorCode.TABLE_NOT_FOUND));
CustomizeTableEntity table = tableRepository.getByIdAndMember(tableId, member);
timeBoxRepository.deleteAllByTable(table);
tableRepository.delete(table);
}
Expand Down
Copy link
Member

Choose a reason for hiding this comment

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

서비스 로직이 길어지는 건 어쩔 수 없는 것 같아요. 일단 넘어가고, 월요일에 더 이야기해보죠

Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
package com.debatetimer.service.customize;

import com.debatetimer.domain.customize.CustomizeTable;
import com.debatetimer.domain.customize.CustomizeTimeBoxes;
import com.debatetimer.domain.member.Member;
import com.debatetimer.dto.customize.request.BellRequest;
import com.debatetimer.dto.customize.request.CustomizeTableCreateRequest;
import com.debatetimer.dto.customize.request.CustomizeTimeBoxCreateRequest;
import com.debatetimer.dto.customize.response.BellResponse;
import com.debatetimer.dto.customize.response.CustomizeTableResponse;
import com.debatetimer.dto.customize.response.CustomizeTimeBoxResponse;
import com.debatetimer.entity.customize.BellEntity;
import com.debatetimer.entity.customize.CustomizeTableEntity;
import com.debatetimer.entity.customize.CustomizeTimeBox;
import com.debatetimer.repository.customize.BellRepository;
import com.debatetimer.repository.customize.CustomizeTableRepository;
import com.debatetimer.repository.customize.CustomizeTimeBoxRepository;
import java.util.List;
import java.util.stream.IntStream;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@Service
@RequiredArgsConstructor
public class CustomizeServiceV2 {

private final CustomizeTableRepository tableRepository;
private final CustomizeTimeBoxRepository timeBoxRepository;
private final BellRepository bellRepository;

@Transactional
public CustomizeTableResponse save(CustomizeTableCreateRequest tableCreateRequest, Member member) {
CustomizeTable table = tableCreateRequest.toTable(member);
CustomizeTableEntity savedTable = tableRepository.save(new CustomizeTableEntity(table));

return saveTimeBoxesAndBells(tableCreateRequest, savedTable.toDomain());
}

@Transactional(readOnly = true)
public CustomizeTableResponse findTable(long tableId, Member member) {
CustomizeTableEntity tableEntity = tableRepository.getByIdAndMember(tableId, member);
CustomizeTimeBoxes timeBoxes = timeBoxRepository.findTableTimeBoxes(tableEntity);
List<CustomizeTimeBoxResponse> timeBoxResponses = timeBoxes.getTimeBoxes()
.stream()
.map(this::getTimeBoxResponse)
Copy link
Contributor

Choose a reason for hiding this comment

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

[추후 반영 제안]
getTimeBoxResponse가 무언가 했는데

  1. DTO를 조회한다? -> 이거 아니었네? -> DTO를 만드는 거엿네
  2. 조회용 메서드인가 보다 -> Bell도 저장하고 있잖아?

=> 내외적으로 TimeBox와 Bell을 함께 다루는 객체를 커찬이 이야기꺼냈던 것 같은데 현재 타임박스 안에 bell이 완전 종속관계로 따라다니다보니까 두 객체를 함께 다루어야 하고 + 반환형은 하나이므로 도메인 객체나 엔티티가 아닌 DTO를 반환하는 private 메서드를 만들 수 밖에 없다는 게 아쉬운 것 같아요.

  • 역할적으로는 잘 이해되었으나 커찬이 말한 (타임박스 + 벨 리스트)를 묶는 단위 도메인 객체를 고려해봐도 좋을 듯 싶습니다.

.toList();
return new CustomizeTableResponse(tableEntity.toDomain(), timeBoxResponses);
}

@Transactional
public CustomizeTableResponse updateTable(
CustomizeTableCreateRequest tableCreateRequest,
long tableId,
Member member
) {
CustomizeTableEntity existingTable = tableRepository.getByIdAndMember(tableId, member);
CustomizeTable renewedTable = tableCreateRequest.toTable(member);
existingTable.updateTable(renewedTable);

deleteBell(timeBoxRepository.findTableTimeBoxes(existingTable));
timeBoxRepository.deleteAllByTable(existingTable);
return saveTimeBoxesAndBells(tableCreateRequest, existingTable.toDomain());
}

@Transactional
public CustomizeTableResponse updateUsedAt(long tableId, Member member) {
CustomizeTableEntity tableEntity = tableRepository.getByIdAndMember(tableId, member);
CustomizeTimeBoxes timeBoxes = timeBoxRepository.findTableTimeBoxes(tableEntity);
tableEntity.updateUsedAt();
List<CustomizeTimeBoxResponse> timeBoxResponses = timeBoxes.getTimeBoxes()
.stream()
.map(this::getTimeBoxResponse)
.toList();
return new CustomizeTableResponse(tableEntity.toDomain(), timeBoxResponses);
}

@Transactional
public void deleteTable(long tableId, Member member) {
CustomizeTableEntity table = tableRepository.getByIdAndMember(tableId, member);

deleteBell(timeBoxRepository.findTableTimeBoxes(table));
timeBoxRepository.deleteAllByTable(table);
tableRepository.delete(table);
}

private CustomizeTableResponse saveTimeBoxesAndBells(
CustomizeTableCreateRequest tableCreateRequest,
CustomizeTable table
) {
List<CustomizeTimeBoxCreateRequest> timeBoxCreateRequests = tableCreateRequest.table();
List<CustomizeTimeBoxResponse> timeBoxResponses = IntStream.range(0, timeBoxCreateRequests.size())
Copy link
Contributor

Choose a reason for hiding this comment

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

이 코드 마음에 안드는데 익숙해서 어디서 봤나 했더니 내가 쓴 코드였네 ㅋㅋ

.mapToObj(i -> createTimeBoxResponse(timeBoxCreateRequests.get(i), table, i + 1))
.toList();
return new CustomizeTableResponse(table, timeBoxResponses);
}

private CustomizeTimeBoxResponse createTimeBoxResponse(
CustomizeTimeBoxCreateRequest request,
CustomizeTable table,
int sequence
) {
CustomizeTimeBox savedTimeBox = timeBoxRepository.save(request.toTimeBox(table, sequence));
return createTimeBoxResponse(request.bell(), savedTimeBox);
}

private CustomizeTimeBoxResponse createTimeBoxResponse(List<BellRequest> bellRequests, CustomizeTimeBox timeBox) {
if (timeBox.getBoxType().isTimeBased()) {
return new CustomizeTimeBoxResponse(timeBox, null);
}

List<BellResponse> bellResponses = bellRequests
.stream()
.map(bellRequest -> new BellEntity(timeBox, bellRequest.time(), bellRequest.count()))
.map(bellRepository::save)
.map(bell -> new BellResponse(bell.getTime(), bell.getCount()))
.toList();
return new CustomizeTimeBoxResponse(timeBox, bellResponses);
}

private CustomizeTimeBoxResponse getTimeBoxResponse(CustomizeTimeBox timeBox) {
if (timeBox.getBoxType().isTimeBased()) {
return new CustomizeTimeBoxResponse(timeBox, null);
}

List<BellResponse> bellResponses = bellRepository.findByCustomizeTimeBox(timeBox)
.stream()
.map(bell -> new BellResponse(bell.getTime(), bell.getCount()))
.toList();
return new CustomizeTimeBoxResponse(timeBox, bellResponses);
}

private void deleteBell(CustomizeTimeBoxes savedCustomizeTimeBoxes) {
bellRepository.deleteAllByCustomizeTimeBoxIn(savedCustomizeTimeBoxes.getTimeBoxes());
}
}
13 changes: 13 additions & 0 deletions src/main/resources/db/migration/V10__add_bell_table.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
create table bell
(
id bigint auto_increment,
bell_time bigint not null,
count bigint not null,
customize_time_box_id bigint not null,
primary key (id)
);

alter table bell
add constraint bell_to_customize_time_box
foreign key (customize_time_box_id)
references customize_time_box(id);
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.debatetimer.client.oauth.OAuthClient;
import com.debatetimer.domain.customize.CustomizeBoxType;
import com.debatetimer.domain.customize.Stance;
import com.debatetimer.dto.customize.request.BellRequest;
import com.debatetimer.dto.customize.request.CustomizeTableCreateRequest;
import com.debatetimer.dto.customize.request.CustomizeTableInfoCreateRequest;
import com.debatetimer.dto.customize.request.CustomizeTimeBoxCreateRequest;
Expand Down Expand Up @@ -97,8 +98,15 @@ private ArbitraryBuilder<CustomizeTimeBoxCreateRequest> getCustomizeTimeBoxCreat
.set("speechType", "입론1")
.set("boxType", CustomizeBoxType.NORMAL)
.set("time", 120)
.set("bell", getBellRequestBuilder().sampleList(2))
Copy link
Contributor

Choose a reason for hiding this comment

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

어우 픽스쳐 몽키 사용하길 잘했다 ㅋㅋ

.set("timePerTeam", 60)
.set("timePerSpeaking", null)
.set("speaker", "발언자");
}

private ArbitraryBuilder<BellRequest> getBellRequestBuilder() {
return fixtureMonkey.giveMeBuilder(BellRequest.class)
.set("time", 30)
.set("count", 1);
}
}
Loading