Skip to content
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

[Spring JDBC] 황승준 미션 제출합니다. #378

Open
wants to merge 61 commits into
base: davidolleh
Choose a base branch
from

Conversation

davidolleh
Copy link

@davidolleh davidolleh commented Nov 13, 2024

안녕하세요! 잘 부탁드립니다~
미션 하면서 생겨났던 고민, 질문사항 README에 담아두었습니다!

@davidolleh davidolleh closed this Nov 13, 2024
@davidolleh davidolleh reopened this Nov 13, 2024
@davidolleh davidolleh changed the title 안녕하세요! 잘 부탁드립니다~ [Spring JDBC] 신지훈 미션 제출합니다. Nov 13, 2024
@davidolleh davidolleh changed the title [Spring JDBC] 신지훈 미션 제출합니다. [Spring JDBC] 황승준 미션 제출합니다. Nov 13, 2024
Copy link

@be-student be-student left a comment

Choose a reason for hiding this comment

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

일단 어프루브를 누르기는 했지만, 더 학습해보시고 싶거나 한 부분이 있다면 언제든 고쳐서 와주시면 계속 봐드리겠습니다 🙇
수정해주시고 dm 으로 요청해주세요!

- [x] 데이터 삭제 잘못된 요청시 예외처리

### 7 단계 고민
- Entity id에 setter 함수를 두면 위험성이 크다는 생각이 들어 새로운 객체를 생성해서 return 해야 되겠다 라는 생각을 가지게 되었습니다

Choose a reason for hiding this comment

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

jpa 를 기준으로 보면, save 시점에 알아서 값을 추가해주게 됩니다!
find 를 했을 때는 자동으로 id 를 채워진 상태로 오기 때문에, id 를 직접 set 하는 경우는 거의 없다고 보셔도 될 것 같아요!

Comment on lines +20 to +24
### 질문사항
- 어플리케이션 실행중 어디서든 exception이 발생하면 ControllerAdvice를 exception과 관련된 응답을 전달해줍니다.
대부분의 exception을 최종적으로 ControllerAdvice에서 처리를 하다 보니 Service, Repository, Entity 등등 아무데서나 exception을
던질 수 있겠다라는 착각을 하게 되는것 같습니다. 혹시 exception은 보통 어느 layer에서 처리를 하는지 궁금하며 예외처리는 어떻게 관리
되는지 궁금합니다!

Choose a reason for hiding this comment

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

이 부분은 현실적으로 말하면 프로젝트 바이 프로젝트가 너무 심하고, 팀 바이 팀이 너무 심해요
대부분의 경우에 throw 를 했다 -> 잘못되었다는 응답을 준다
이 공식이 작동하다보니 그냥 공통 레이어에서 handler 를 추가하는 경우가 많은데요

다르게 말하자면 throw 를 했다 -> 잘못된 응답을 줄 필요가 없다 (자체적으로 롤백을 할 수 있다)
정도로 생각된다면 이는 직접 catch 를 서비스 레이어에서 하는 편인 것 같아요

Comment on lines 22 to 29
@GetMapping("/reservation")
public String reservationPage() {
return "reservation";
}

@GetMapping("/reservations")
@ResponseBody
public ResponseEntity<List<ReservationResponseDto>> readReservations() {

Choose a reason for hiding this comment

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

요 2가지는 서로 분리하면 어떨까요?
view 를 반환하는 컨트롤러와 json 을 반환하는 메소드가 한 클래스에 같이 있을 이유는 딱히 없을 것 같아요

Comment on lines 30 to 32
return ResponseEntity
.status(HttpStatus.OK)
.body(

Choose a reason for hiding this comment

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

ResponseEntity.ok(객체)
이런 형태로 단순화 가능할 것 같아요!

Comment on lines +33 to +35
reservationService.readReservations().stream().
map(ReservationResponseDto::fromEntity)
.toList()

Choose a reason for hiding this comment

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

controller 레이어에서 dto 를 만드는 것과, service 레이어에서 dto 를 만드는 것중 전자를 선택한 이유가 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

개인적인 생각으로 dto로 형변환하는 것은 핵심 비지니스 로직으로 생각하지 않는거 같습니다.
service에서는 entity로만(Spring에서는 jpa기술을 사용함으로써 entity를 domain class로 사용하기에) 작업해야지 라는 생각이 있습니다!

Comment on lines 78 to 80
if (count == 0) {
throw new EntityNotFoundException("해당 예약은 존재하지 않스빈다");
}

Choose a reason for hiding this comment

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

이 부분은 구현의 취향 차이인데요
혹시 예외를 던지게 된 이유에 대해서 알 수 있을까요?
결과적으로 db 에는 예약이 없다는 상태가 되는 것은 동일하잖아요?

db 에 id 1 있음 -> 1 삭제 -> db 에 id 1 없음
db 에 id 1 없음 -> 1 삭제 -> db 에 id 1 없음

이런 것들을 보통 멱등성이라고 표현하는데요
1이 없는 상황에서도 1을 삭제하려고 했을 때 예외를 던지는 것은 멱등하지 않은 코드를 짠 것이죠
어느 것이 무조건 맞다는 정답은 없으니 어떤 것을 선택했을 때 그 이유가 있으면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

db 에 id 1 있음 -> 1 삭제 -> db 에 id 1 없음
db 에 id 1 없음 -> 1 삭제 -> db 에 id 1 없음

혹시 이부분에서

db 에 id 1 있음 -> 1 삭제 -> db 에 id 1 없어짐
db 에 id 1 없음 -> 1 삭제 -> db 에 id 1 없음
이지 않나요?

변경된 즉 delete된 행이 있다면
int count = jdbcTemplate.update(query, id);
이것은 0이 아닌 다른 값을 반환할 것이고
변경된 행이 없다면 0을 반환하는 것으로 알고 있는데
삭제하려는 행이 없다는 것 즉 entity가 db에 없다고 알릴 수 있는 방법 아닌가요?

조금 헷갈려서 질문드립니다!

Comment on lines 39 to 47
jdbcTemplate.update(connection -> {
PreparedStatement ps = connection.prepareStatement(
query,
new String[]{"id"});
ps.setString(1, reservation.getPerson().getName());
ps.setString(2, reservation.getDate().format(CustomDateTimeFormat.dateFormatter));
ps.setString(3, reservation.getTime().format(CustomDateTimeFormat.timeFormatter));
return ps;
}, keyHolder);

Choose a reason for hiding this comment

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

이미 너무 잘 해주셔서 뭐라 코멘트 할 내용이 없네요

Choose a reason for hiding this comment

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

오 schema.sql 까지 두시다니 진짜 많이 학습하신 티가 나네요

그러면 한 단계 더 가서 data.sql 을 통해서 초기화 하는 방법도 학습을 해보셨을까요?
혹은 다른 방식으로 초기 데이터를 넣어주는 과정도 학습을 해보셨을까요?
ex) ApplicationRunner, ConsoleRunner, data.sql 로 inmemoryReservationRepository 처럼 초기화를 진행해주셔도 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

요것은 다음 미션에 적용해 보았습니다!

import static org.assertj.core.api.Assertions.assertThat;

@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT)
@DirtiesContext(classMode = DirtiesContext.ClassMode.BEFORE_EACH_TEST_METHOD)

Choose a reason for hiding this comment

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

https://velog.io/@tjddus0302/Spring-DirtiesContext
dirtiest context 는 성능상 단점이 있다보니 성능상 단점이 있는데요
다른 방식으로 테스트간 격리를 진행해주실 수 있으실까요?

Copy link
Author

Choose a reason for hiding this comment

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

이것은 따로 공부가 길게 필요할거 같아 공부를 조금 뒤로 미뤄놨습니다! 꼭 공부해 보겠습니다~


@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT)
@DirtiesContext(classMode = DirtiesContext.ClassMode.BEFORE_EACH_TEST_METHOD)
@SuppressWarnings("NonAsciiCharacters")

Choose a reason for hiding this comment

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

이거 보기 좋네요 👍

@davidolleh
Copy link
Author

안녕하세요 피드백 반영하면서 생겨났던 질문들 댓글로 남겨보았습니다!

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