-
Notifications
You must be signed in to change notification settings - Fork 170
[Spring JDBC] 김하늘 5, 6, 7단계 미션 제출합니다. #528
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: kimsky247-coder
Are you sure you want to change the base?
Conversation
dooboocookie
left a comment
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.
안녕하세요, 하늘.
리뷰어 루카입니다.
늘 Spring MVC 미션은 리뷰를 드리기가 쉽지 않은데요.
일단 서로 맥락의 차이를 좀 줄이고자 질문위주로 리뷰를 드렸습니다.
stream이나 메서드 참조를 사용하신 것이
JAVA 문법에 많이 익숙해보이시네요 굿입니다.
🤓 질문에 대한 답변
이번 미션에서 H2 데이터베이스의 AUTO_INCREMENT를 사용하여 ID 생성을 DB에 위임했습니다. AUTO_INCREMENT 에 대해 학습하는 과정에서 INSERT가 실패해도 이미 증가한 AUTO_INCREMENT 카운트는 줄어들지 않아 ID 값에 구멍이 생긴다는 것을 알게 되었습니다.예약 번호가 반드시 연속적이어야 하는지 아니면 동시성 성능을 위해 ID의 불연속성을 허용해도 되는 것인지 궁금합니다!
우려가 되는 상황이 있어서 주신 질문이실까요??
ID가 불연속적인 숫자일 때 어떤 '동시성 성능' 이슈가 있다고 판단하신 걸까요?
질문에 대한 답을 저 나름대로 해석해서 드리자면,
상관없을 것 같아요.
ID를 부여할 때 가장 중요한 것은 unique하다는 것입니다. 사실 이것만 지켜지면, 되죠.
중복되는 순간 그것은 아이디라고 할 수 없을테니까요.
ID를 autoincrement로 하는 것은 여러가지고 아이디 부여 전략중 하나입니다.
ID가 큰 row가 나중에 생긴 row라고 생각하면 안되겠죠.
그리고 추가적으로 ID는 어떤 도메인적인 의미가 없는 난수나 자동증가하는 숫자로 하는 것을 추천드립니다.
하늘 사실 많은 대화를 나누기 힘든 미션이지만 질문에 한번 답을 주시면서 한번더 핑퐁해보면 좋을것같습니다.
| date DATE NOT NULL, | ||
| time TIME NOT NULL, |
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.
👍
| public Reservation save(Reservation reservation) { | ||
| String sql = "INSERT INTO reservation (name, date, time) VALUES (?, ?, ?)"; | ||
| KeyHolder keyHolder = new GeneratedKeyHolder(); | ||
|
|
||
| jdbcTemplate.update(connection->{ | ||
| PreparedStatement ps = connection.prepareStatement(sql, new String[]{"id"}); | ||
| ps.setString(1, reservation.getName()); | ||
| ps.setObject(2, reservation.getDate()); | ||
| ps.setObject(3, reservation.getTime()); | ||
| return ps; | ||
| }, keyHolder); | ||
|
|
||
| long id = Objects.requireNonNull(keyHolder.getKey()).longValue(); | ||
|
|
||
| return new Reservation(id, reservation.getName(), reservation.getDate(), reservation.getTime()); | ||
| } |
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.
👍 Approve
전반적으로 JDBC Template을 잘 사용하셨네요.
요구사항에도 맞게 생성된 ID를 수집하기 위해서 Keyholder 굿굿👍
| public List<ReservationResponse> getAllReservations() { | ||
| return reservationList.stream() | ||
| return reservationRepository.findAll().stream() | ||
| .map(ReservationResponse::from) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public ReservationResponse createReservation(ReservationRequest request) { | ||
| try { | ||
| LocalDate date = LocalDate.parse(request.date()); | ||
| LocalTime time = LocalTime.parse(request.time()); | ||
|
|
||
| Long newId = index.incrementAndGet(); | ||
|
|
||
| Reservation newReservation = new Reservation( | ||
| newId, | ||
| Reservation reservation = new Reservation( | ||
| null, | ||
| request.name(), | ||
| date, | ||
| time | ||
| ); | ||
|
|
||
| reservationList.add(newReservation); | ||
| return ReservationResponse.from(newReservation); | ||
| Reservation savedReservation = reservationRepository.save(reservation); | ||
| return ReservationResponse.from(savedReservation); | ||
|
|
||
| } catch (DateTimeParseException e) { | ||
| throw new BadRequestReservationException(ErrorMessage.INVALID_DATE_TIME_FORMAT.getMessage()); | ||
| } | ||
| catch (DateTimeParseException e) { | ||
| throw new BadRequestReservationException(ErrorMessage.INVALID_DATE_TIME_FORMAT.getMessage()); } | ||
| } |
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.
👀 Comment
여기서 2가지 질문이 있습니다.
- DateTimeParseException 을 custom exception으로 변환한 이유
- 예외 처리 범위를 메서드 전체로 한 이유
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.
- DateTimeParseException 을 custom exception으로 변환한 이유
DateTimeParseException이 발생하면 기본적으로 500 에러가 반환됩니다. 하지만 날짜 형식이 잘못된 것은 서버 오류가 아닌 클라이언트의 잘못된 입력이라고 생각해 400 Bad Request로 명확히 응답하기 위해 커스텀 예외로 변환했습니다.
- 예외 처리 범위를 메서드 전체로 한 이유
try-catch 범위를 전체로 잡은 것은 실수입니다. 파싱 로직에서만 해당 예외가 발생하므로 범위가 너무 넓으면 다른 로직에서 발생하는 예상치 못한 예외까지 400 에러로 판단되어 오류를 놓칠 수 있을 것 같습니다. 파싱이 일어나는 부분만 감싸도록 범위를 좁히는 것이 옳다고 생각해 범위를 수정하였습니다.
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.
DateTimeParseException이 발생하면 기본적으로 500 에러가 반환됩니다. 하지만 날짜 형식이 잘못된 것은 서버 오류가 아닌 클라이언트의 잘못된 입력이라고 생각해 400 Bad Request로 명확히 응답하기 위해 커스텀 예외로 변환했습니다.
어차피 CustomException 예외 핸들링해서 응답을 지정해준것 아닌가요?
DateTimeParseException도 그냥 응답을 주면 될 것 같은데요.
CustomException도 트레이드 오프가 있습니다.
관리포인트를 늘리죠. Java 혹은 사용하는 프레임웤이나 라이브러리들은 대부분 예외들을 잘 정리해놨는데요.
그를 활용하는 것도 좋은 방법일 것 같습니다.
(저는 커스텀예외 사용할 것 같아요)
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.
GlobalExceptionHandler에서 표준 예외를 직접 잡으면 불필요한 클래스 추가를 막고 관리 포인트를 줄일 수 있을 것 같습니다. 하지만 커스텀 예외를 이용하면 서비스 계층의 의도를 명확히 드러낼 수 있고, 내부 로직이 바뀌더라도 컨트롤러 계층에는 영향이 없도록 결합도를 낮출 수 있는 것이 장점이 있어 커스텀 예외를 선택했습니다!
| public void deleteReservation(Long id) { | ||
| boolean removed = reservationList.removeIf(reservation -> reservation.getId().equals(id)); | ||
| int deletedCount = reservationRepository.deleteById(id); | ||
|
|
||
| if (!removed) { | ||
| if (deletedCount == 0) { | ||
| throw new NotFoundReservationException(ErrorMessage.NOT_FOUND_RESERVATION.getMessage()); | ||
| } |
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.
👀 Comment
삭제된 Reservation이 없을 때, 예외를 throw 하는 이유가 있을까요?
삭제된 내용이 애초에 없었다면 요청자는 원하는 결과 아니었을까요?
그리고 추가적으로 이 PR에는 포함되어있지 않지만, 해당 에러가 404가 아닌 400인 이유도 있을까요?
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.
이전의 4단계 미션 요구사항에 '삭제 할 예약이 없는 경우 400으로 응답하세요'라는 조건이 있어서 이를 따르기 위해 400을 반환하도록 구현했습니다. 하지만 DELET메서드는 멱등성을 만족해야 합니다. 따라서 이미 삭제된 리소스에 대해 다시 삭제 요청을 보내더라도, 서버의 상태는 '삭제됨'으로 동일하므로 성공으로 처리하는 것이 더 자연스러운 설계라고 생각합니다. 삭제할 데이터가 없더라도 예외 대신 204 No Content를 반환하도록 수정했습니다.
| @Service | ||
| public class ReservationService { |
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.
👍 Approve
DB에 접근하는 Service 로직을 처리할 때에는,
그 일련의 과정을 하나의 트랜잭션에 두기 위해 @Transactional을 많이 사용하는데요.
혹시 하늘은 Transcational어노테이션을 사용하지 않은 이유가 따로 있으실까요?
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.
현재 로직에서는 create나 delete가 각각 하나의 SQL만 실행하기 때문에 별도의 설정 없이도 동작한다고 생각했습니다. 하지만 @transactional을 통해 코드의 의도를 명확히 전달하는 것이 더 좋은 설계인 것 같습니다. 수정했습니다
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.
저도 update를 위한 SQL문을 1개만 실행할 떄는 딱히 Transactional이 없어도 된다고 생각합니다.
확신이 있으면 그렇게 가셔도됩니다.
해당 코드들에서 Transactional 감싸지 않았을 때 문제점은 저는 발견하지 못했어요.
혹시나 해서 Keyholder의 동작도 살짝 체크해봤는데 괜찮을 것 같습니다.
안녕하세여 경환님! 이번 Spring JDBC 미션에 매칭된 김하늘입니다. 잘 부탁드립니다🙇
프로젝트 구조
현재 프로젝트 구조입니다.
궁금한 점
이번 미션에서 H2 데이터베이스의
AUTO_INCREMENT를 사용하여 ID 생성을 DB에 위임했습니다.AUTO_INCREMENT에 대해 학습하는 과정에서INSERT가 실패해도 이미 증가한AUTO_INCREMENT카운트는 줄어들지 않아 ID 값에 구멍이 생긴다는 것을 알게 되었습니다.예약 번호가 반드시 연속적이어야 하는지 아니면 동시성 성능을 위해 ID의 불연속성을 허용해도 되는 것인지 궁금합니다!