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

[5기 김용상] SpringBoot Part3 Weekly Mission 제출합니다. #953

Open
wants to merge 110 commits into
base: yongnyeo/3
Choose a base branch
from

Conversation

YongNyeo
Copy link

@YongNyeo YongNyeo commented Nov 2, 2023

📌 과제 설명

Api Voucher Controller : api 를 통해 서비스 동작을 돕는 Controller입니다.
Web Voucher Controller : web thymeleaf를 통해 서비스 동작을 돕는 Controller입니다.

API컨트롤러에서는 @ExceptionHandler를 통해 예외처리를 진행했고,
Web 컨트롤러에서는 BindingResult로 예외처리를 도왔습니다.

👩‍💻 요구 사항과 구현 내용

SpringBoot Part3 Weekly Mission

(기본) 바우처 서비스 관리페이지 개발하기

  • Spring MVC를 적용해서 thymeleaf 템플릿을 설정해보세요.
  • 커맨드로 지원했던 기능을 thymeleaf를 이용해서 관리페이지를 만들고 다음 기능을 지원가능하게 해보세요
    • 조회페이지
    • 상세페이지
    • 입력페이지
    • 삭제기능

(기본) 바우처 서비스의 API 개발하기 (뷰는 따로 X)

  • Spring MVC를 적용해서 JSON과 XML을 지원하는 REST API를 개발해보세요
    • 전체 조회기능
    • 조건별 조회기능 (바우처 생성기간 및 특정 할인타입별)
    • 바우처 추가기능
    • 바우처 삭제기능
    • 바우처 아이디로 조회 기능

(보너스) 바우처 지갑용 관리페이지를 만들어보세요.

  • Create (지갑 생성기능)
  • deleteByCustomerId (고객 아이디 해당하는 지갑 삭제)
  • findByCustomerId(고객 아이디로 지갑 검색)
  • findByVoucherId(바우처 아이디로 지갑 검색)

✅ PR 포인트 & 궁금한 점

  1. 처음 예외처리할때 @ExceptionHandler에서 status변경 +메시지 변경을 했습니다.
    그런데 mockMvc 테스트를 짜다보니, 실패했어도 무조건 200 (ok) 처리가 돼서 찾아보니(웹과 postman은 정상작동하지만 mockMvc만 )
    MockMvc Test always return status code 200 라는 키워드가 있었고, 해당 문제를 처리하려면 컨트롤러 코드를 다 바꿔야하는데,
    과연 테스트를 위해 도메인쪽 코드를 바꾸는게 옳은가? 라는 생각이 들었습니다. 그래서 일단은 테스트코드를 보류했는데..
    @ExceptionHandler에서 모두 status를 ok 로 관리하면 문제없지만.. 실무에선 상태코드를 어떻게 관리하는지? 궁금합니다.

  2. Api controller에서 현재로서는 response dto가 따로 없이 ResponseEntity<>로 반환을 하고있습니다. response dto는 request dto와 다르게 검증이 따로 필요없기에.. 복잡도 올라간다 생각해서 구현을 안했는데, 엔티티의 변경 가능성 때문이라도 반드시 구현해야 하는 것일까요?

  3. 어떤 예외는 controller-service-repository레이어에서 처리하고, 어떤 예외는 디스패쳐서블릿 밖으로 던져서 ExceptionHandler로 처리하는지... 궁금합니다! 예외를 특정시점에서 정상흐름으로 처리해야할지/말지 정도로 이해하면될까요?

고민할거리 많이 던져주셔서 항상 감사합니다 😄 👍

@YongNyeo YongNyeo changed the title Yongnyeo/3 [5기 김용상] SpringBoot Part3 Weekly Mission 제출합니다. Nov 2, 2023
@YongNyeo YongNyeo self-assigned this Nov 2, 2023
Copy link

@zxcv9203 zxcv9203 left a comment

Choose a reason for hiding this comment

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

안녕하세요 용상님, 스프링 3주차 과제 고생하셨습니다. 🙇

궁금한 부분 코멘트 남겨드렸으니 확인부탁드립니다. 🙇

Comment on lines +36 to +52
@GetMapping("/date")
ResponseEntity<List<Voucher>> findByDate(@RequestParam String startTime, @RequestParam String endTime) {
LocalDateTime startDate = LocalDateTime.parse(startTime);
LocalDateTime endDate = LocalDateTime.parse(endTime);
return ResponseEntity.ok(voucherService.findByDate(startDate, endDate));
}

@GetMapping("/type")
ResponseEntity<List<Voucher>> findByType(@RequestParam String type) {
VoucherType voucherType = VoucherType.valueOf(type);
return ResponseEntity.ok(voucherService.findByType(voucherType));
}

@GetMapping("")
ResponseEntity<List<Voucher>> findAll() {
return ResponseEntity.ok(voucherService.findAll());
}
Copy link

Choose a reason for hiding this comment

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

해당 API 들은 findAll()로 하나로 합칠 수 있을 것 같습니다.

추가로 Voucher의 데이터의 개수가 매우 많을수도 있으므로 Pagination 적용을 고려해보셔도 좋을 것 같습니다 🤔

Comment on lines +54 to +62
@PostMapping("/create")
ResponseEntity<Voucher> create(@RequestBody CreateVoucherDto dto) {
if (dto.getVoucherType() == 2 && dto.getValue() > 100) {
throw new InputMismatchException(INVALID_VOUCHER_PERCENT.getMessage());
}
return ResponseEntity
.status(HttpStatus.CREATED)
.body(voucherService.create(dto));
}
Copy link

Choose a reason for hiding this comment

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

CREATED는 HTTP 스펙상 URI Location 정보를 추가하는 것이 좋을 것 같습니다.

추가로 설계한 API에 동작이 들어가 있는데 Http Method로 동작을 어느정도 표현가능할 것 같습니다 🤔

Rest API 디자인 가이드를 한번 읽어보셔도 좋을 것 같습니다.
https://learn.microsoft.com/ko-kr/azure/architecture/best-practices/api-design



@PutMapping("/{id}/edit")
ResponseEntity<?> update(@PathVariable String id, @RequestBody UpdateVoucherDto dto) {
Copy link

Choose a reason for hiding this comment

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

아무것도 반환하지 않는다면 와일드카드를 반환하는 것보다는 ResponseEntity<Void>를 반환하는게 적절할 것 같습니다 🤔

Comment on lines +77 to +95
@ExceptionHandler
String ArgumentTypeMistMatch(HttpMessageNotReadableException e) {
return INVALID_VOUCHER_INFO.getMessage();
}

@ExceptionHandler
String DuplicatedId(DuplicateKeyException e) {
return e.getMessage();
}

@ExceptionHandler
String ExceedVoucherPercent(InputMismatchException e) {
return e.getMessage();
}

@ExceptionHandler
String EmptyResult(EmptyResultDataAccessException e) {
return e.getMessage();
}
Copy link

Choose a reason for hiding this comment

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

예외에 대한 핸들링은 @RestControllerAdvice를 통해 한곳에서 관리하면 컨트롤러 클래스의 코드가 간결해질 것 같습니다.

Comment on lines +50 to +57
// @Test
// @DisplayName("findById()로 존재하지 않는 바우처 읽기에 실패한다.")
// void findByIdFail() throws Exception {
// mockMvc.perform(get("/api/vouchers/{id}", NotExistId)
// .contentType(MediaType.APPLICATION_JSON))
// .andExpect(status().isNotFound())
// .andDo(print());
// } --> 웹이나 postman에서는 notFound 처리되지만, mockMvc 테스트만 항상 200처리됩니다.
Copy link

Choose a reason for hiding this comment

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

실제로 VoucherService에 대한 처리가 진행되지 않아 결과로 null이 반환되어 200이 발생한 상황입니다.

image

이미지를 보면 200임에도 아무것도 반환되지 않는 것을 볼 수 있는데 원래 Service는 Exception이 발생하는데 @MockBean 으로 선언된 VoucherService의 findById(...)는 어떤 동작을 기대하는지 지정하지 않아 null이 반환된 것이고 이로인해 테스트에서는 200을 반환하는 상황입니다.

이를 위해 어떤 동작을 기대하는지 명시하거나, 직접 서비스를 등록하여 주입받는 방법이 있을 것 같습니다 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants