Skip to content

[그리디] 서현진 Spring JPA (2차) 4, 5, 6 단계 미션 제출합니다.#209

Open
nonactress wants to merge 49 commits intonext-step:nonactressfrom
nonactress:hyeonjin3
Open

[그리디] 서현진 Spring JPA (2차) 4, 5, 6 단계 미션 제출합니다.#209
nonactress wants to merge 49 commits intonext-step:nonactressfrom
nonactress:hyeonjin3

Conversation

@nonactress
Copy link
Copy Markdown

@nonactress nonactress commented Jan 8, 2026

해윤님 안녕하세요!
마지막 미션에서 리뷰어님으로 뵙게 되어 정말 반갑습니다 😊
잘 부탁드립니다!!

이번 미션은 기존 JPA 구현을 Spring Data JPA로 전환하는 것이었습니다.
따라서 이전 미션과 비교해 비즈니스 로직의 변경은 없으며,
주로 구현 방식에 초점을 맞추어 진행했습니다.

아래는 이번 미션에서 수행한 주요 변경 사항입니다.

수정 사항


  • 엔티티 Soft Delete 처리 로직을 @Where 조건으로 적용
  • 기존 JPA 기반 Repository를 Spring Data JPA 기반으로 리팩토링

미션을 진행하는 과정에서 이전 미션들의 리뷰가 아직 모두 완료되지 않아 브랜치가 다소 분리되어 있습니다.
남아 있는 피드백들은 빠른 시일 내에 정리하여 모두 반영 후 푸시하겠습니다. 🙇

1) 모든 도메인 엔티티로 변경
2) TimeRepository 퀴리문 변경
3) DataInitializer로 초기값 설정
Copy link
Copy Markdown

@haeyoon1 haeyoon1 left a comment

Choose a reason for hiding this comment

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

안녕하세요 현진님!! 또 만나뵙게 됐네요😃 저도 잘부탁드립니다

먼저 전반적으로 마지막 줄 개행 및 엔터와 같은 코드정렬이 일정하지 않은 것 같아요! 수정이 필요해보입니다🥲
또한 사용하지 않은 메서드들은 삭제해주시는게 좋아요! 사용하지 않는 getter와 같은 메서드들이 일부 남아있는 것 같습니다.

그리고 도메인별로 코드의 형식이 비슷해서 현재는 time 패키지에 대한 리뷰만 중점적으로 남겼지만 다른 도메인 코드에도 적용되는 부분이 있어보이는데, 확인 후 수정이 필요해보이는 부분이라면 다른 패키지(도메인)에서도 수정해주시면 중복되는 리뷰를 줄일 수 있을 것 같아요!

놓치신 부분이 있다면 다음 리뷰에서 언급드릴테니 수정 또는 편하게 의견 말씀해주세요~

Comment on lines +3 to +4
import jakarta.persistence.Entity;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

사용하지 않는 import문은 자동 정렬을 통해 삭제해주세요~
또한 사용하지 않는 메서드들 (getTimeId, isBooked)는 삭제가 필요해보입니다!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

넵 수정해보겠습니다!!

반영 커밋 : db04811

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

@Column(name = "time_value")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

해당 칼럼만 이름을 따로 지정해주신 이유가 무엇인가요??

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

아 이게 원래 미션에서 주어진 테이블에서
image
위 방식으로 되어있어서 이름 규칙을 유지해보았습니다!

@Column(name = "time_value")
private String value;

private boolean deleted = false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

삭제 방식으로 soft deleted를 사용하셨는데 이유가 궁금합니다~

Copy link
Copy Markdown
Author

@nonactress nonactress Jan 14, 2026

Choose a reason for hiding this comment

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

soft delete 적용 이유


만약 10:00 타임을 삭제하고 11:00 타임을 새로 만든다고 가정했을 때:

  • 하드 딜리트 시: 10:00라는 데이터 자체가 DB에서 증발합니다. 이때 이 시간을 참조하던 기존 예약들은 '가리킬 대상(FK)'이 사라져 버립니다.

  • 소프트 딜리트 시: 10:00 데이터의 deleted 상태만 true로 바뀝니다. 데이터는 그대로 존재하므로, 기존 예약들은 여전히 10:00라는 정보를 안전하게 붙잡고 있을 수 있습니다.

라고 생각합니다!!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

soft delete 적용의 장점을 잘파악하셨군요!

사용자가 특정 시간대를 이미 예약한 상태에서 해당 시간대가 soft delete 되면, 사용자는 존재하지 않는 시간대를 예약한 이상한 사람이 될 것 같아요😔

따라서 time을 soft delete 할 때,
"해당 시간대를 사용하는 예약(또는 대기)이 존재하면 삭제를 막고 에러를 반환하도록" 하는 로직을 추가하는 것은 어떨까요?
이렇게 하면 기존 예약의 의미를 유지하면서도 데이터 일관성을 지킬 수 있을 것 같습니다!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

네넵 시간이 먼저 사라지면 정말 말씀 하신 내용처럼 로직이 이상해질 것 같습니다! 적용 해보겠습니다!

반영 커밋 : 137c138

Comment on lines +41 to +43
public void setDeleted(boolean deleted) {
this.deleted = deleted;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

public void delete() {
        this.isDeleted = true;
    }

해당 메서드는 삭제를 할 때 호출하는 메서드이므로, 보통 boolean 상태를 받기보다는 요런식으로 많이 사용합니다!

Copy link
Copy Markdown
Author

@nonactress nonactress Jan 13, 2026

Choose a reason for hiding this comment

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

앞으로 위 방식을 사용해보겠습니다!!!

반영 커밋 : 1ca9fa1

import java.util.List;

@Service
@Transactional
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

service 상단에 @Transactional 어노테이션을 붙여주셨는데 아래 save와 deleteById 메서드에도 붙어있네요! 중복된 어노테이션 같습니다.
또한 @Transactional@Transactional(readOnly = true)의 차이를 아시나요?! 각 메서드들은 어떤 어노테이션이 어울릴 지 고민해보시면 좋을 것 같습니다~

Copy link
Copy Markdown
Author

@nonactress nonactress Jan 14, 2026

Choose a reason for hiding this comment

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

차이점


구분 @Transactional (기본) @Transactional(readOnly = true)
주 목적 데이터 수정(CUD) 및 읽기 포함 데이터 조회(R) 전용
더티 체킹 활성화. 변경 감지를 위해 스냅샷 보관 비활성화. 스냅샷 생략으로 메모리 절약
Flush 모드 AUTO (트랜잭션 커밋 시 실행) NEVER/MANUAL (Flush 건너뜀)
성능 보통 높음 (조회 시 오버헤드 최적화)

timeService 에서
transactional : save() , deleteById()
(readOnly = true) : findAll() , getAvailableTime()
하면 좋은 것 같습니다!

반영 커밋 : 494fffe

Comment on lines +13 to +14
private TimeRepository timeRepository;
private ReservationRepository reservationRepository;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

필드에 final을 붙이면 좋을 것 같아요!(이유도 오랜만에 한번 복습해보시면 좋을 것 같아요)

Copy link
Copy Markdown
Author

@nonactress nonactress Jan 14, 2026

Choose a reason for hiding this comment

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

필드 값에 final 을 붙여야 하는 이유

  1. 런타임에 불변성 보장
  • 한번 스프링 에서 주입해준 값을 런타임 시점에 다른 값으로 초기화 할 수 없습니다!
  1. 생성자에서 필드 초기화 강제
  • 만약 생성자를 통한 DI를 하는 것에 실수가 있다면 컴파일 시점에 알려줍니다!

추가) Lombok과의 시너지 : @RequriedArgsConstructor 를 사용하면 final 이 붙은 필드 값만 가지고 생성자를 만들어 줘서 DI 하기 편리합니다!

반영 커밋 : 0f655d2

Comment on lines +17 to +30
@Transactional
public Theme save(Theme theme) {
return themeRepository.save(theme);
}

@Transactional
public List<Theme> findAll() {
return themeRepository.findAll();
}

@Transactional
public void deleteById(Long id) {
themeRepository.deleteById(id);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

요런 경우에는 상단에 @Transactional을 붙이면 중복 코드를 줄일 수 있을 것 같습니다

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

😨 아아 바로 수정하겠습니다!!!

반영 커밋 : 5ce1144

Comment on lines +8 to +9
@SQLDelete(sql = "UPDATE time SET deleted = true WHERE id = ?")
@Where(clause = "deleted = false")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

이 어노테이션들은 softdelete를 사용하기 위함으로 보이는데요

여기에 soft delete를 적용하는 메서드가 아래와 같이 구현되어있고

public void setDeleted(boolean deleted) {
        this.deleted = deleted;
    }

삭제를 하는 로직은 TimeController의 delete 메서드 -> timeService.deleteById(id);를 호출 / TimeService의 deleteById -> time.setDeleted(true); 호출 하는 것 같아요. 그러면 이미 메서드만으로 삭제 처리가 되고있는 것 같은데 해당 어노테이션을 붙이신 이유가 궁금합니다!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

리뷰어님이 말씀 해주신 소프트 딜리트를 적용하는 과정의 코드를 다시 보니 중복된 삭제 과정이 있었던 것 같습니다!

현재 상황

  1. @SQLDelete(sql = "UPDATE time SET deleted = true WHERE id = ?") -> delete를 호출하면 알아서 softdelete 로 처리
  2. timeRepository.delete() -> deleted = true

결과 : 두 번의 삭제 로직이 발생!!

해결

수정 후 : @Transactional public void deleteById(Long id) { timeRepository.deleteById(id); }

@SQLDelete(sql = "UPDATE time SET deleted = true WHERE id = ?") 으로만 소프트 딜리트 처리했습니다!!

반영 커밋 : eecfe3a

implementation 'org.springframework.boot:spring-boot-starter-jdbc'
implementation 'org.springframework.boot:spring-boot-starter-data-jpa'

implementation 'dev.akkinoc.spring.boot:logback-access-spring-boot-starter:4.0.0'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

실행시켜서 몇개 api들 동작시켜보면 현재 로그가 매우매우매우 많이 뜨는 걸 확인할 수 있을텐데요
요 줄을 없애면 해결된답니다.
로그가 너무 많으면 확인하기 힘드니 적당히 보이는 것이 좋을 것 같아요!

Copy link
Copy Markdown
Author

@nonactress nonactress Jan 20, 2026

Choose a reason for hiding this comment

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

저도 콘솔 창에 너무 많은 로그가 있어 해윤님이 말씀하신 방향으로 정리해보겠습니다!!!

위 코드를 반영해보니 컴파일에러가 발생하여 찾아보니
implementation 'org.springframework.boot:spring-boot-starter-data-jpa' : SQL 대신 자바 객체(Entity)를 조작합니다. 인터페이스만 선언하면 Spring Data JPA가 실행 시점에 적절한 SQL을 자동으로 생성합니다.
와 같다고 하여
리뷰어님이 말씀하신 방향으로 수정하기 위해
#logging.level.org.hibernate.SQL=DEBUG
를 주석 처리해보았습니다!!
저도 덕분에 어떤 의존성이 어떤 역할을 하는지 확실히 알 수 있는 기회가 된 것 같습니다!

반영 커밋 :

@haeyoon1
Copy link
Copy Markdown

haeyoon1 commented Jan 18, 2026

아래는 수정이 필요해보이는 부분을 정리했습니다!
혹시 다른 의견이 있다면 편하게 말씀해주세요~

  1. 로그인 관련 에러 코드

현재는 모두 500에러가 나가고 있는 것 같은데(아래 사진들에 나와있어요!) 로그인 관련 에러 코드는 400 혹은 401등이 더 일반적이에요!
각 에러코드의 특징을 찾아보고 수정해주시면 디버깅에 좋을 것 같습니다!
또한 다른 요청들에서도 예외를 상황별로 구분하고, Global Exception Handler를 통해 다양하게 매핑하면 API의 의도가 더 잘 드러날 것 같습니다!

  • 없는 email, pw로 로그인 한 경우 / 혹은 잘못된 id 또는 pw로 로그인 한 경우 등 로그인 관련 에러
    image
  1. 방탈출 예약 요청 시 발생하는 에러
스크린샷 2026-01-19 오전 1 36 05 로그인 후 예약 요청을 보내보았는데, 500에러가 떠서 예약 생성이 안되는 것 같은데... 혹시 확인해주실 수 있을까요?! 저와 동일하게 에러가 난다면 수정부탁드립니다!

Comment on lines 36 to 40
if (member != null) {
response = reservationService.saveByMember(request, member);
} else {
response = reservationService.saveByAdmin(request);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

위 로직에 대해 설명해주실 수 있나요?
member == null 이면 saveByAdmin이 실행되고 있는데 제가 해당 시나리오를 잘 이해하지 못한 것 같아서요😂

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

아 제가 생각한 바로는 admin은 관리자 계정이므로 waiting을 해야할 필요가 없다고 생각해서 만들었던 로직인데 제 의도와는 다르게 현재는 역할의 유무로 나눠지는 것이 아닌 등록된 멤버인가로 나눠지고 있어 역할로 예약을 저장할 수 있도록 수정해보았습니다!

반영 커밋 : faf90f5

public ReservationResponse save(ReservationRequest reservationRequest) {
Reservation reservation = reservationDao.save(reservationRequest);
@Transactional
public ReservationResponse saveByMember(ReservationRequest request, Member member) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

해당 메서드에서 맡은 책임들이 조금 많아 길어지는 것 같아요!
메서드 역할 분리를 진행해보는 것은 어떨까요?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

최대한 하나의 역할을 하도록 분리해보겠습니다!

반영 커밋 : 0e1576d

@nonactress
Copy link
Copy Markdown
Author

image

로그인 관련해서 말씀해주신 내용인 예약 자체가 안되고 있는 것을 확인하였고 계속해서 원인을 찾아보고 있었는데 저번 미션 코드에서 로직상의 변화는 없는데 왜 안되는지 잘 이해가 되지 않습니다.... 위 사진을 보면 time 값이 제대로 넘어오지 않고 있어서 인 것 같은데 위 문제는 프론트엔드 코드에서의 처리 과정인 것 같아서 혹시 제가 잘 못 이해하고 있는것인지 아니면 어떤 식으로 해결 할 수 있을지 여쭤보고 싶습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants