-
Notifications
You must be signed in to change notification settings - Fork 30
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
[Jinny] 스프링 카페 미션 - 3단계 구현 #90
Conversation
* 목적: 의도를 명확히 하기 위함 * 변경 내용: index -> sequence
* 이유: 해당 인터페이스 사용법에 대해 완전히 이해하지 못함
* 기존 쿼리는 테이블이 계속 초기화가 되는 이슈가 있음
* DB 연동하면서 개념이 헷갈려 Optional 삭제한 것을 되돌림
* 이렇게 하는 것이 아닌 것 같음.. 수정 필요
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.
Jinny 안녕하세요~ 리뷰어 Jane입니다.
리뷰가 늦어져 죄송합니다 ㅠㅠ 많이 기다리셨죠 😢
PR 메시지 읽어봈는데요ㅎㅎ AWS 배포 url도 Jinny의 학습속도에 맞게 천천히 공유주셔도 되고, 예외처리에 대해서도 이미 충분히 많은 고민 해주시고 있는 것 같습니다.
그리고 이번 cafe 프로젝트에서 다 소화하지 못하셔도 다음 프로젝트들을 하시며 자연스레 익히게 될 것이니 너무 걱정하지 않으셔도 됩니다 😄
그래도 열심히 하시는 욕심있는 스타일이신 것 같아..! 이번 코멘트는 조금 더 자세하고 쉽게 이해할 수 있도록 예외 처리관련 피드백을 남겨드렸어요. 확인해보시고 모르는 것 있으시면 편하게 질문주셔요ㅎㅎ
- [ ] 절대 경로, 상대 경로 | ||
- [ ] View Resolver 동작 원리 | ||
- [ ] redirect | ||
- [ ] redirect & forward |
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.
👍🏻
- Template Engine 및 매핑 URI "/" 처리 방식 | ||
- H2 DB 3가지 모드 | ||
- gradle 의존성 옵션 | ||
- 커넥션 풀 |
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.
아직까지는 엄청 깊게 알지는 못하고 이런 개념이 있다~정도로만 아는 것 같습니다. 😞
- 데이터베이스에 연결하기 위해서는 커넥션 풀이 필요하다.
- 커넥션풀은 데이터베이스와 연결된 커넥션을 미리 만들어 관리를 해주어 필요할 때마다 가져다 쓰고 반납하면 된다.
- 커넥션풀을 사용하지 않을 경우 데이터베이스에 연결하기 위한 매우 복잡하고 긴 코드를 작성해야 한다.
- 사용할 때마다 매번 커넥션을 생성해야 하기 때문에 시간과 비용이 많이 든다.
- JdbcTemplate을 쓰면 이런 코드를 작성하지 않고 커넥션풀을 생성해 사용할 수 있다.
userService.join(userDTO); | ||
return "redirect:/users"; | ||
} catch (InvalidUserIdException e) { | ||
System.out.println(e.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.
@ControllerAdvice
를 이용하여 InvalidUserIdException
을 잡아주시면 다소 지저분해보이는 try catch문을 제거할 수 있답니다ㅎㅎ
너무 어렵게 생각하지 마시고 아래 아티클을 보시고 간단하게 적용해보세요~
https://tecoble.techcourse.co.kr/post/2021-05-10-controller_advice_exception_handler/
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.
그리고 System.out.println 보다는 logger를 사용하여 로깅해주세요ㅎㅎ
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.
앗 잠깐 테스트하면서 추가했던 System.out.println
을 삭제하지 않았네요,,
@ControllerAdvice
를 적용해서 예외처리하는 것으로 수정했습니다. 다만 의도대로 수정되지는 않았습니다. 이렇게 하는 것이 맞는지 모르겠습니다 😞
의도대로 수정하지 못한 이유는 리다이렉트하면 에러 메시지가 출력되지 않기 때문입니다. RedirectAttribute도 사용해봤지만... 의도대로 구현이 안되어 리다이렉트 시 데이터를 어떻게 전달해야 하는지 더 공부를 해봐야 할 것 같습니다.
- 수정 커밋
- 기존 의도:
- 에러 발생 시,
GlobalExceptionHandler
클래스에서@ExceptionHandler
을 통해 에러 메시지를 담아 /error URI를 리다이렉트 ErrorController
에서 /error URI을 error 페이지와 매핑, 에러 메시지를 받아와 뷰에 전달
- 에러 발생 시,
- 수정 방향
- i 단계에서 URI를 리턴하는 대신 error 페이지에 에러 메시지를 담아 뷰를 리턴
|
||
@Override | ||
public Article save(Article article) { | ||
String sql = "insert into articles(writer, title, contents) values(?, ?, ?)"; |
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.
NamedParameterJdbcTemplate
을 사용하도록 변경해보셔도 좋을 것 같네요ㅎㅎ
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.
NamedParameterJdbcTemplate
을 사용하도록 수정했습니다.
컬럼이 많아질 수록 파라미터의 바인딩 순서가 헷갈리는데 어떻게 개선할 수 있을지 고민했는데, 해당 방법을 사용하면 파라미터가 순서대로 바인딩되지 않을 경우 에러가 발생할 위험을 방지할 수 있을 것 같습니다.
|
||
@Override | ||
public Optional<Article> findBySequence(long sequence) { | ||
String sql = "select * from articles where sequence = ?"; |
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.
쿼리에 *
을 사용하지 않는 이유에 대해서 알아보았는데,
Join 쿼리 사용할 때, 컬럼 삭제가 있을 때 등 문제가 발생할 수 있다고 합니다.
아직까지 쿼리를 많이 사용하지 않아서 잘 몰랐는데 좀 더 공부해야 할 것 같습니다.!
그래도 현재 상황에서 와일드카드보다 명시하는 것이 좋다고 생각합니다.
|
||
@Override | ||
public List<Article> findAll() { | ||
String sql = "select * from articles"; |
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.
해당 커밋에 수정해두었습니다.
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.
UserRepositoryImpl 클래스 쿼리문도 같이 수정했습니다.
public User save(User user) { | ||
String sql = "insert into users(userId, password, name, email) values(?, ?, ?, ?)"; // TODO: 컬럼이 많아질 수록 순서가 헷갈리는데 개선할 수 있는 방법이 있을까 | ||
template.update( | ||
sql, user.getUserId(), user.getPassword(), user.getName(), user.getEmail() |
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.
~ParameterSource
클래스들을 이용할 수도 있을 것 같아요~
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.
해당 커밋에 같이 수정해두었습니다!
} catch (EmptyResultDataAccessException e) { // TODO: 해당 처리를 안하면 DB에 해당 데이터가 없을 때 예외가 발생한다. 이렇게 하는것이 맞는지 모르겠다. | ||
return Optional.ofNullable(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.
JdbcTemplate뿐만 아니라 JPA 등을 사용하실 때도 비슷한 상황에서 비슷한 예외를 보실 수 있을 것 같습니다.
그 때마다 어떻게 처리할지가 관건인데요.
지금처럼 구현해주시는 것도 흔한 처리방법입니다만, 이렇게되면 받는쪽에서 바로 Optional.get()
을 호출하시면 안됩니다.
제 개인적인 의견으로는 해당 예외를 받아서 해당 예외를 Throwable cause로 받는 적절한 커스텀 예외를 또 던진 후 ControllerAdvice에서 처리해줄 것 같습니다.
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.
그리고 Optional.empty()와 orElseThrow()에 대해서도 공부해보세요ㅎㅎ
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.
흠 ㅠㅠ 아직 어떻게 처리를 해야할지 감이 안잡히네요 ㅠㅠ 조금 더 공부해보고 다음 PR에 반영해보겠습니다!
} | ||
|
||
@GetMapping("/users") | ||
public String listPage(final Model model) { | ||
List<User> users = userService.findUsers(); | ||
model.addAttribute("users", users); | ||
return "/user/list"; | ||
return "user/list"; | ||
} | ||
|
||
@GetMapping("/users/{userId}") | ||
public String viewUserProfile(@PathVariable final String userId, final Model model) { | ||
User findUser = userService.findOne(userId).get(); |
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.
구현을 보면 이렇게 바로 get하시면 안될 것 같습니다.ㅎㅎ
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.
Optional 및 예외처리 관련해서 좀 더 공부해보겠습니다!
@Size(min = 1, max = 10) | ||
private String userId; | ||
|
||
@Size(min = 8, max = 10) | ||
private String password; | ||
|
||
@Size(min = 1, max = 10) |
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.
아직 스프링 예외처리 흐름에 대해 이해가 되지않아서,
일단 디테일한 검증보다 전체적인 예외처리를 경험해보기 위해 사이즈는 깊게 신경쓰지는 않았습니다. 😞
* 수정 이유: JdbcTemplate을 사용할 경우, 파라미터가 순서대로 바인딩되지 않을 경우 에러가 발생될 위험이 있음 * 참고: 학습을 위해 두가지 방법을 사용함 - MapSqlParameterSource - BeanPropertySqlParameterSource
* 참고: RowMapper에서 sequence를 갖고 오지 않아 페이지 매핑이 안되던 문제 같이 수정
적어주신 고민들에 대한 답변 드립니다. 😄
Jinny 말씀이 맞습니다ㅎㅎ
현재 다만
DTO, 도메인 객체에 대하여 어떤 이해를 하시고 계신지 먼저 적어주시면 질문 답변에 도움이 될 것 같기는 한데요ㅎㅎ 주로 DTO에서는 클라이언트에서 검증하지 못하고 온 부분에 대한 validation check를 수행합니다. 꼭 요청으로 들어와야 하는 필드인데 null이 들어오면 예외를 던진다든가 하는 부분이 이 부분에 해당할 것 같아요. jsr 380 등으로 수행될 수 있구요. 도메인에서는 비즈니스 로직을 수행하며 필요한 여러 검증들이 수행된다고 보시면 되는데요. 현재 Jinny의 구현을 보면 핵심 비즈니스 로직이 엔티티가 아닌 서비스에 위치해 있고, 엔티티는 데이터를 전달하는 역할만 하고 있는데요. 핵심 비즈니스 로직과 그에 필요한 검증이 entity에 위치해야 하는가 도메인 service에 위치해야 하는가는 상황에 따라 설계 철학에 따라 달라서 정답은 없습니다ㅎㅎ
controller에서 view로 넘어갈 때 현재처럼 엔티티를 직접 넘겨주는 건 안 좋은 방식입니다.
|
@GetMapping("/error") | ||
public String errorPage(Model model) { | ||
model.addAttribute("error", model.getAttribute("error")); | ||
return "error"; | ||
} | ||
} |
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.
구글링하지 않고, 그냥 스스로 생각으로 이렇게 하면
model에 담긴 에러 메시지를 갖고 올 수 있지 않을까?라고 생각되어 구현한 부분입니다.
뭔가 잘못된 것 같다는 생각이 듭니다.
public String create(@Valid final UserDTO userDTO, BindingResult bindingResult, Model model) { | ||
if (bindingResult.hasErrors()) { | ||
// TODO: 에러 내용이 출력되게끔 로직 추가 | ||
return "redirect:/error"; | ||
} | ||
try { | ||
userService.join(userDTO); | ||
return "redirect:/users"; | ||
} catch (InvalidUserIdException e) { | ||
System.out.println(e.getMessage()); | ||
model.addAttribute("error", e.getMessage()); | ||
return "/error"; // TODO: 리다이렉트하도록 수정 필요 | ||
} | ||
} |
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.
@Override | ||
public Optional<User> findById(String userId) { | ||
String sql = "select * from users where userId = ?"; | ||
try { | ||
return Optional.ofNullable(template.queryForObject(sql, userRowMapper(), userId)); | ||
} catch (EmptyResultDataAccessException e) { // TODO: 해당 처리를 안하면 DB에 해당 데이터가 없을 때 예외가 발생한다. 이렇게 하는것이 맞는지 모르겠다. | ||
return Optional.ofNullable(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.
Optional.ofNullable
로 감싸면 쿼리 결과가 없을 때 null
이 넘어 올 것이라고 생각했는데,
queryForObject
했을 때 쿼리 결과가 없으면 Exception이 터졌습니다.
임시로 try-catch 처리를 해줬는데, 이렇게 하는 것이 맞는지 모르겠습니다.
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.
jdbc template의 queryForObject 메서드와 DataAccessException에 대해 더 학습해보시구요ㅎㅎ
SQLErrorCodeSQLExceptionTranslator
가 내부에서 어떤 처리를 하는지도 추가적으로 학습해보시기를 바랍니다ㅎㅎ
userService.join(userDTO); | ||
return "redirect:/users"; | ||
} catch (InvalidUserIdException e) { | ||
System.out.println(e.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.
앗 잠깐 테스트하면서 추가했던 System.out.println
을 삭제하지 않았네요,,
@ControllerAdvice
를 적용해서 예외처리하는 것으로 수정했습니다. 다만 의도대로 수정되지는 않았습니다. 이렇게 하는 것이 맞는지 모르겠습니다 😞
의도대로 수정하지 못한 이유는 리다이렉트하면 에러 메시지가 출력되지 않기 때문입니다. RedirectAttribute도 사용해봤지만... 의도대로 구현이 안되어 리다이렉트 시 데이터를 어떻게 전달해야 하는지 더 공부를 해봐야 할 것 같습니다.
- 수정 커밋
- 기존 의도:
- 에러 발생 시,
GlobalExceptionHandler
클래스에서@ExceptionHandler
을 통해 에러 메시지를 담아 /error URI를 리다이렉트 ErrorController
에서 /error URI을 error 페이지와 매핑, 에러 메시지를 받아와 뷰에 전달
- 에러 발생 시,
- 수정 방향
- i 단계에서 URI를 리턴하는 대신 error 페이지에 에러 메시지를 담아 뷰를 리턴
|
||
@Override | ||
public Article save(Article article) { | ||
String sql = "insert into articles(writer, title, contents) values(?, ?, ?)"; |
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.
NamedParameterJdbcTemplate
을 사용하도록 수정했습니다.
컬럼이 많아질 수록 파라미터의 바인딩 순서가 헷갈리는데 어떻게 개선할 수 있을지 고민했는데, 해당 방법을 사용하면 파라미터가 순서대로 바인딩되지 않을 경우 에러가 발생할 위험을 방지할 수 있을 것 같습니다.
public User save(User user) { | ||
String sql = "insert into users(userId, password, name, email) values(?, ?, ?, ?)"; // TODO: 컬럼이 많아질 수록 순서가 헷갈리는데 개선할 수 있는 방법이 있을까 | ||
template.update( | ||
sql, user.getUserId(), user.getPassword(), user.getName(), user.getEmail() |
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.
해당 커밋에 같이 수정해두었습니다!
|
||
@Override | ||
public List<Article> findAll() { | ||
String sql = "select * from articles"; |
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.
해당 커밋에 수정해두었습니다.
|
||
@Override | ||
public List<Article> findAll() { | ||
String sql = "select * from articles"; |
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.
UserRepositoryImpl 클래스 쿼리문도 같이 수정했습니다.
} catch (EmptyResultDataAccessException e) { // TODO: 해당 처리를 안하면 DB에 해당 데이터가 없을 때 예외가 발생한다. 이렇게 하는것이 맞는지 모르겠다. | ||
return Optional.ofNullable(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.
흠 ㅠㅠ 아직 어떻게 처리를 해야할지 감이 안잡히네요 ㅠㅠ 조금 더 공부해보고 다음 PR에 반영해보겠습니다!
} | ||
|
||
@GetMapping("/users") | ||
public String listPage(final Model model) { | ||
List<User> users = userService.findUsers(); | ||
model.addAttribute("users", users); | ||
return "/user/list"; | ||
return "user/list"; | ||
} | ||
|
||
@GetMapping("/users/{userId}") | ||
public String viewUserProfile(@PathVariable final String userId, final Model model) { | ||
User findUser = userService.findOne(userId).get(); |
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.
Optional 및 예외처리 관련해서 좀 더 공부해보겠습니다!
Jane, 안녕하세요.
Jinny입니다.
스프링 카페 미션 3단계 기능 구현하여 PR드립니다.
이번 주는 뭔가 공부는 많이 한 것 같은데, 커밋을 보니 별 내용이 없는 것 같아 아쉬웠습니다.
저번주에 이제 Spring에 대해 조금은 알 것 같다는 생각을 했는데,
이번주에 Spring
예외처리
를 공부하면서 를 하면서 다시 “스프링에 대해서 아무것도 모르는구나”라는 생각이 들었습니다.Spring 예외처리에 대해 전체적인 흐름을 모르는 상태에서
다른 멤버들의 코드를 보며 조각조각 퍼즐을 맞춰서 구현하려다보니 원리에 대해 이해하기 힘들었습니다.
수많은 학습 키워드 속에서 갈피는 못잡고,
특히
@Valid, BindingResult
로 예외처리하는 것과@ControllerAdvice, ExceptionHandler
로 예외처리하는 개념이 많이 헷갈렸습니다.PR 마감을 하루 앞두고 예외처리까지 구현해서 피드백받고 싶어서 더욱 허둥대다 보니 조급해져서 더욱 갈피를 못잡았던 것 같습니다.
그래서 이번주는 아쉽지만 예외처리 구현까지는 못하고, 예외처리 관련 기초 공부를 제대로 차근차근해서 다음주에 꼭 코드에 반영해서 PR보내겠습니다.
+) AWS 배포는 완료했는데, 몇번 더 해보고 싶어서 다음주나 다다음주 중으로 배포 URL 공유드리겠습니다.
이번주에 어떤 내용을 공부했고, 어떤 고민과 과정으로 미션을 구현했는지는 하단에 기재해두었습니다.
긴 글 읽어주셔서 감사합니다. 🙂
✔️ 1~2단계 피드백 관련
DTO
->도메인
변환은 기존 컨트롤러에서 서비스 계층에서 하도록 수정하였습니다.@Autowired
어노테이션 삭제했습니다.빌더 패턴
을 사용해보았습니다.✨ 3단계 기능 목록
H2 DB 연동 기능
AWS 배포
📌 3단계 URL 및 메서드 컨벤션
GET
🔥 고민
WebMvcConfigurer
클래스를 삭제했습니다.WebMvcConfigurer
에 대해 제대로 알지 못하기 때문입니다.WebMvcConfigurer
를 사용할 경우, 똑같은 URL에서 Http 메서드만 달라졌을 때 다른 URI를 부여해야 했습니다. 그렇지 않으면 기존 컨트롤러 있는 Mapping 메서드와WebMvcConfigurer
의 매핑 메서드에서 충돌이 생겼습니다.Controller -> View로 넘어갈 때는 도메인으로 데이터가 전달되고 있어서 어떤 것이 DTO로 전달하는 것이 맞을지 고민이 됩니다.
✏️ 이번 주 학습 내용
월요일
화요일
수요일
목요일