-
Notifications
You must be signed in to change notification settings - Fork 0
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
[feat] #278 - 티켓 조회 api 변경 #281
Conversation
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.
고생하셨습니다 혜린님!
코멘트 몇가지 남겼습니다.
String scheduleNumberValue = null; | ||
if (scheduleNumber != null) { | ||
scheduleNumberValue = scheduleNumber.name(); | ||
List<String> scheduleNumberStrings = schedules.stream() |
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.
String 자료형이 아닌, ScheduleNumber Enum 객체를 받도록 바꿔서 타입 안정성(type safety)을 높이는 건 어떨까요?
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.
그리고 scheduleNumberStrings
라는 네이밍 방식은 좋지 않은 것 같아요!
(우테코 피드백: 변수 이름에 자료형은 사용하지 않는다)
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.
native query 에서 list를 param으로 전달할 때 허용되는 값은 long과 string뿐이기에 변환이 필요합니다!
scheduleNumberList로 네이밍 변경하는 건 어떻게 생각하시나요?
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.
아하 이해했습니다! 그런데 scheduleNumberList
도 List를 드러내는 네이밍 이라서 적절하지 않을 것 같아요!
selectedSchedules
은 어떠신가요?
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.
selected 좋네요! 그런데 schedule과 scheduleNumber간에 혼동이 있을 수도 있을 것 같아 selectedScheduleNumbers로 변경하겠습니다~
.map(schedule -> schedule.getScheduleNumber().name()) | ||
.toList(); | ||
|
||
List<String> bookingStatusStrings = Arrays.asList( |
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.
해당 부분도 동일합니다!
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.
위와 동일합니다!
@Param("bookingStatus") String bookingStatus); | ||
@Param("scheduleNumberStrings") List<String> scheduleNumberStrings, | ||
@Param("bookingStatusStrings") List<String> bookingStatusStrings | ||
); |
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.
@Query(value = """
SELECT b.*
FROM booking b
JOIN schedule s ON b.schedule_id = s.id
WHERE s.performance_id = :performanceId
AND b.booking_status != 'BOOKING_DELETED'
AND (:scheduleNumberStrings IS NULL OR s.schedule_number IN (:scheduleNumberStrings))
AND (:bookingStatusStrings IS NULL OR b.booking_status IN (:bookingStatusStrings))
AND (:searchWord IS NULL OR MATCH(b.booker_name) AGAINST(:searchWord IN BOOLEAN MODE))
ORDER BY
FIELD(b.booking_status, 'REFUND_REQUESTED', 'CHECKING_PAYMENT', 'BOOKING_CONFIRMED', 'BOOKING_CANCELLED'),
b.created_at DESC
""", nativeQuery = true)
List<Booking> searchBookings(
@Param("performanceId") Long performanceId,
@Param("searchWord") String searchWord,
@Param("scheduleNumberStrings") List<String> scheduleNumberStrings,
@Param("bookingStatusStrings") List<String> bookingStatusStrings
);
해당 방식으로 바꾸면 좀 더 FIELD 함수를 사용해 동일한 정렬 우선순위를 더 간결하게 작성할 수 있을 것 같은데 어떻게 생각하시나요?
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.
우선 is null or 적용 시 여러개의 enum 값으로 같이 조회하는 것이 불가능해서 conversation에 적었던 것처럼 service에 default 값을 설정하고 is null or은 적용하지 않는 방식으로 해결해뒀습니다!
case 함수는 대부분의 dbms에서 사용 가능한 방면 field 함수는 mysql 전용함수이고, 성능상에서는 차이가 거의 없고 데이터셋이 클 때는 case가 조금 더 낫다고 하는데 동훈님은 가독성을 위해 FIELD 함수 사용을 추천하신걸까요?
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.
DB 변경 가능성이 없을 것 같아서 FIELD
로 추천드렸었는데, 추후 QueryDSL을 사용하려면 FIELD와 같은 MySQL 전용 함수는 지원이 제한적일 수도 있을 것 같네요!!
혜린님 방식으로 유지해도 좋을 것 같습니다!
String scheduleNumberValue = null; | ||
if (scheduleNumber != null) { | ||
scheduleNumberValue = scheduleNumber.name(); | ||
List<String> scheduleNumberStrings = schedules.stream() |
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.
아하 이해했습니다! 그런데 scheduleNumberList
도 List를 드러내는 네이밍 이라서 적절하지 않을 것 같아요!
selectedSchedules
은 어떠신가요?
Related issue 🛠
Work Description ✏️
Trouble Shooting ⚽️
TicketRepository.java
처음에는 위와 같은 방식으로 IS NULL OR로 null인 경우 조건을 무시하도록 한 뒤 native query에서 IN으로 list로 넘어온 모든 string에 해당하는 데이터들을 조회하는 쿼리문을 작성했습니다. 하지만 이 경우 다중 값으로 조회 시도 시(ex. schedulNumber SECOND, THIRD로 필터링) 500 에러를 반환했습니다. 관련 사례들을 많이 찾아보았으나 is null or과 in을 함께 사용한 native query 사례를 발견할 수 없었습니다.. 결국 null인 경우에는 아래와 같이 service에서 디폴트값을 미리 설정하고 query문에서는 null 처리 없이 조회하는 방식으로 기능을 구현해두었습니다.
TicketService.java
TicketRepository.java
Related ScreenShot 📷
예매자 목록 조회
scheduleNumber 조건 두개로 조회
bookingStatus 조건 두개로 조회
scheduleNumber 조건 두개, bookingStatus 조건 두개로 조회
예매자 목록 검색
scheduleNumber 조건 두개로 검색
bookingStatus 조건 두개로 검색
scheduleNumber 조건 두개, bookingStatus 조건 두개로 검색
Uncompleted Tasks 😅
To Reviewers 📢
우선 빠른 도입을 위해 jpql, native query를 이용한 기존 방식을 유지했으나 조만간 queryDSL을 도입해 리팩토링하겠습니다..!!