-
Notifications
You must be signed in to change notification settings - Fork 2
[fix] 예약/결제 동시성 취약점 수정 (레이스 컨디션 및 오버부킹) #158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 5 commits
3ced665
6e3c14a
d89fd22
bab83c3
8ab24a9
9117e44
ebaed6b
8a28e52
3442430
aa1cbed
d01060b
568d29e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,11 +5,21 @@ | |
| import jakarta.persistence.*; | ||
| import lombok.*; | ||
|
|
||
| import java.time.LocalDate; | ||
| import java.time.LocalTime; | ||
|
|
||
| @Entity | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @AllArgsConstructor | ||
| @Builder | ||
| @Getter | ||
| @Table( | ||
| name = "booking_table", | ||
| uniqueConstraints = @UniqueConstraint( | ||
| name = "uq_booking_table_slot", | ||
| columnNames = {"store_table_id", "booking_date", "booking_time", "is_active"} | ||
| ) | ||
| ) | ||
|
Comment on lines
+16
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial 기존 테이블에
권장 마이그레이션 순서:
또한 🤖 Prompt for AI Agents |
||
| public class BookingTable { | ||
|
|
||
| @Id | ||
|
|
@@ -23,4 +33,20 @@ public class BookingTable { | |
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "booking_id") | ||
| private Booking booking; | ||
|
|
||
| // 유니크 제약 적용을 위해 Booking의 날짜/시간을 비정규화하여 저장 | ||
| @Column(name = "booking_date", nullable = false) | ||
| private LocalDate bookingDate; | ||
|
|
||
| @Column(name = "booking_time", nullable = false) | ||
|
Comment on lines
+38
to
+41
|
||
| private LocalTime bookingTime; | ||
|
|
||
| // true = 활성 예약 슬롯 (유니크 제약 적용) | ||
| // null = 취소된 슬롯 — MySQL은 NULL을 유니크 인덱스에서 중복으로 보지 않으므로 동일 시간대 재예약 허용 | ||
| @Column(name = "is_active") | ||
| private Boolean isActive = true; | ||
|
Comment on lines
+46
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -nP --type=java -C3 'BookingTable\s*\.\s*builder\s*\(\s*\)'Repository: Eatsfine/BE Length of output: 751 🏁 Script executed: cat -n src/main/java/com/eatsfine/domain/booking/entity/mapping/BookingTable.java | head -70Repository: Eatsfine/BE Length of output: 1803 🏁 Script executed: cat -n src/main/java/com/eatsfine/domain/booking/entity/Booking.java | sed -n '85,100p'Repository: Eatsfine/BE Length of output: 629 🏁 Script executed: rg -nP --type=java "uq_booking_table_slot|unique.*booking.*table" -iRepository: Eatsfine/BE Length of output: 173 🏁 Script executed: # Check for `@Builder` and `@AllArgsConstructor` annotations in BookingTable
rg -nP --type=java "@Builder|@AllArgsConstructor" src/main/java/com/eatsfine/domain/booking/entity/mapping/BookingTable.javaRepository: Eatsfine/BE Length of output: 90 🏁 Script executed: # Search for all BookingTable.builder() usages to see the full scope
rg -nP --type=java 'BookingTable\.builder\(\)'Repository: Eatsfine/BE Length of output: 179
🛡️ 제안 수정 — `@Builder.Default` 추가 // true = 활성 예약 슬롯 (유니크 제약 적용)
// null = 취소된 슬롯 — MySQL은 NULL을 유니크 인덱스에서 중복으로 보지 않으므로 동일 시간대 재예약 허용
+ `@Builder.Default`
`@Column`(name = "is_active")
private Boolean isActive = true;또한 운영 DB에 이미 🤖 Prompt for AI Agents |
||
|
|
||
| public void deactivate() { | ||
| this.isActive = null; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,9 +8,12 @@ | |
| import org.springframework.data.domain.Pageable; | ||
| import org.springframework.data.repository.query.Param; | ||
| import org.springframework.data.jpa.repository.JpaRepository; | ||
| import org.springframework.data.jpa.repository.Lock; | ||
| import org.springframework.data.jpa.repository.Query; | ||
| import org.springframework.stereotype.Repository; | ||
|
|
||
| import jakarta.persistence.LockModeType; | ||
|
|
||
| import java.time.LocalDate; | ||
| import java.time.LocalDateTime; | ||
| import java.time.LocalTime; | ||
|
|
@@ -70,6 +73,10 @@ List<Booking> findActiveBookingsByTableAndDate( | |
| @Param("tableId") Long tableId, | ||
| @Param("date") LocalDate date); | ||
|
|
||
| @Lock(LockModeType.PESSIMISTIC_WRITE) | ||
| @Query("SELECT b FROM Booking b WHERE b.id = :id") | ||
| Optional<Booking> findByIdWithLock(@Param("id") Long id); | ||
|
Comment on lines
+76
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🛡️ 제안 수정+import org.springframework.data.jpa.repository.QueryHints;
+import jakarta.persistence.QueryHint;
...
`@Lock`(LockModeType.PESSIMISTIC_WRITE)
+@QueryHints(`@QueryHint`(name = "jakarta.persistence.lock.timeout", value = "3000"))
`@Query`("SELECT b FROM Booking b WHERE b.id = :id")
Optional<Booking> findByIdWithLock(`@Param`("id") Long id);🤖 Prompt for AI Agents |
||
|
|
||
| Optional<Booking> findByIdAndStatus(Long bookingId, BookingStatus status); | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package com.eatsfine.domain.booking.service; | ||
|
|
||
| import com.eatsfine.domain.booking.enums.BookingStatus; | ||
| import com.eatsfine.domain.booking.repository.BookingRepository; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.stereotype.Component; | ||
| import org.springframework.transaction.annotation.Propagation; | ||
| import org.springframework.transaction.annotation.Transactional; | ||
|
|
||
| import java.time.LocalDateTime; | ||
| import java.util.List; | ||
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| @Slf4j | ||
| public class BookingCancelExecutor { | ||
|
|
||
| private final BookingRepository bookingRepository; | ||
|
|
||
| @Transactional(readOnly = true) | ||
| public List<Long> findExpiredPendingIds(LocalDateTime threshold) { | ||
| return bookingRepository.findAllByStatusAndCreatedAtBefore(BookingStatus.PENDING, threshold) | ||
| .stream() | ||
| .map(booking -> booking.getId()) | ||
|
|
||
| .toList(); | ||
| } | ||
|
|
||
| // REQUIRES_NEW: 호출마다 독립 트랜잭션 — 하나 실패해도 다른 예약에 영향 없음 | ||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public void cancelIfPending(Long bookingId) { | ||
| bookingRepository.findByIdWithLock(bookingId).ifPresent(booking -> { | ||
| if (booking.getStatus() == BookingStatus.PENDING) { | ||
| booking.cancel("결제 시간 초과로 인한 자동 취소"); | ||
| log.info("예약 ID {} 자동 취소 완료", bookingId); | ||
| } | ||
| }); | ||
| } | ||
|
Comment on lines
+22
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win 락/REQUIRES_NEW 적용은 잘 되어 있으며, 매직 문자열 상수화 및 ID 전용 프로젝션 쿼리 권장 전체 구조(락 재조회 + 독립 트랜잭션 + PENDING 재검증)는 적절합니다. 두 가지 사소한 개선 의견입니다.
♻️ 제안 수정 — 상수 추출 및 메소드 레퍼런스+ private static final String CANCEL_REASON_PAYMENT_TIMEOUT = "결제 시간 초과로 인한 자동 취소";
+
`@Transactional`(readOnly = true)
public List<Long> findExpiredPendingIds(LocalDateTime threshold) {
return bookingRepository.findAllByStatusAndCreatedAtBefore(BookingStatus.PENDING, threshold)
.stream()
- .map(booking -> booking.getId())
+ .map(Booking::getId)
.toList();
}
// REQUIRES_NEW: 호출마다 독립 트랜잭션 — 하나 실패해도 다른 예약에 영향 없음
`@Transactional`(propagation = Propagation.REQUIRES_NEW)
public void cancelIfPending(Long bookingId) {
bookingRepository.findByIdWithLock(bookingId).ifPresent(booking -> {
if (booking.getStatus() == BookingStatus.PENDING) {
- booking.cancel("결제 시간 초과로 인한 자동 취소");
+ booking.cancel(CANCEL_REASON_PAYMENT_TIMEOUT);
log.info("예약 ID {} 자동 취소 완료", bookingId);
}
});
}위 코딩 가이드라인의 “3. 성능 및 효율성: 불필요한 DB 쿼리 호출…”과 “2. 코드 품질 & 가독성: 매직 넘버, 하드코딩된 값이 존재하는지 점검”에 따른 의견입니다. 🤖 Prompt for AI Agents |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uq_booking_table_slot유니크 제약 조건은 오버부킹을 방지하는 강력한 수단이지만, 현재 구현상 심각한 부작용이 있습니다. 예약이 취소(CANCELED)되어도BookingTable레코드가 삭제되지 않고 유지되기 때문에, 취소된 예약이 점유했던 동일한 시간대에 다른 사용자가 예약을 시도하면 DB 제약 조건 위반으로 인해 재예약이 불가능해집니다. 이를 해결하려면 예약 취소 시 연관된BookingTable데이터를 삭제(또는 Soft Delete 처리 후 유니크 인덱스에서 제외)하는 로직이 반드시 추가되어야 합니다.