Skip to content

Conversation

@young970
Copy link

@young970 young970 commented Jul 22, 2023

📌 과제 설명

  • (GET) /api/vouchers : 모든 바우처들 조회
  • (POST) /api/vouchers : 바우처 생성
  • (GET) /api/vouchers/{id} : id로 바우처 조회
  • (GET) /api/vouchers/type/{type} : 바우처 타입으로 바우처 조회
  • (DELETE) /api/vouchers/{id} : id로 바우처 삭제

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

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

  • Spring MVC를 적용해서 thymeleaf 템플릿을 설정해보세요.
    • 조회페이지
    • 상세페이지
    • 입력페이지
    • 삭제기능

(기본) 바우처 서비스의 API 개발하기

  • Spring MVC를 적용해서 REST API를 개발
    • 전체 조회기능
    • 조건별 조회기능
    • 바우처 추가기능
    • 바우처 삭제기능
    • 바우처 아이디로 조회 기능

✅ 궁금한 점

  • 이번 팀 미팅 때 멘토님께서 말씀하신 것을 3주차 미션에 반영해 보았습니다.
    controller와 service의 request DTO를 따로 만들어 의존 방향을 맞추고자 하였습니다. response DTO는 원래 controller가 service를 의존하는 것이 맞다고 생각하여 따로 분리하지 않았습니다.
    제가 제대로 이해하고 적용한게 맞는지 궁금합니다. 혹시 response DTO도 따로 분리를 해야 할까요??

  • 저번 흑구멘토님의 코멘트를 통해 Nested Loop join 알고리즘에 대해 알게 되었습니다. 해당 알고리즘에 대해 알게 되며 들었던 생각은 join을 사용하는 것 보다 트랜잭션으로 묶어 쿼리를 여러번 날리는 것이 성능이 더 좋을 것 같다는 생각이 들었습니다. 혹시 이 생각이 맞을까요??

Copy link

@devYSK devYSK left a comment

Choose a reason for hiding this comment

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

안녕하세요 영운님 과제하느냐 고생 많으셨습니다

  • controller와 service의 request DTO를 따로 만들어 의존 방향을 맞추고자 하였습니다.
    response DTO는 원래 controller가 service를 의존하는 것이 맞다고 생각하여 따로 분리하지 않았습니다.
    제가 제대로 이해하고 적용한게 맞는지 궁금합니다. 혹시 response DTO도 따로 분리를 해야 할까요??

-> 그렇게 생각할수도 있습니다만, 응답 스펙이 바뀌었을때도 생각해보면 좋을거같아요
결국 컨트롤러와 서비스의 dto를 다르게 운용한다는것은 의존과 관련이 있는데 그 둘도 같이 사용한다면 서로 변경에 취약해지지 않을까 생각합니다.
현재는 일단 이대로 사용하다가 분리가 필요하다면 그때 가서 분리해도 된다고 생각합니다

  • 저번 흑구멘토님의 코멘트를 통해 Nested Loop join 알고리즘에 대해 알게 되었습니다. 해당 알고리즘에 대해 알게 되며 들었던 생각은 join을 사용하는 것 보다 트랜잭션으로 묶어 쿼리를 여러번 날리는 것이 성능이 더 좋을 것 같다는 생각이 들었습니다. 혹시 이 생각이 맞을까요??

일단 DBMS마다 조인 알고리즘을 지원하는 방식이 달라요.
그리고 쿼리를 여러번 날리게 된다면 비용에 대해 고려해봐야합니다.
네트워크 오버헤드, 메모리, 쿼리 파싱 및 최적화 후 결과 전송 후 다시 반복하게 되겠죠

반면에 조인은 잘 사용한다면 데이터베이스 자체에서 테이블간에 효율적으로 처리할 수 있어요

때문에 일반적으로는 간단한 조인이라면 데이터베이스에 조인을 맡기는게 더 효율적일 수 있지요
매우 큰 테이블 또는 여러 테이블들을 join해야 하는경우에는 쿼리를 쪼개 여러번 날리는것이 더 효율적일때도 있어요
항상 트레이드 오프라고 생각해요

그러니 여러 요인을 고려해서 상황에 맞게 최적의 방법을 고려해보세요


@ExceptionHandler(EntityNotFoundException.class)
public ResponseEntity<ErrorResponse> handleEntityNotFoundException(EntityNotFoundException e){
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(getErrorResponse(400, e));
Copy link

Choose a reason for hiding this comment

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

HttpStatus.BAD_REQUEST.value() 라는 상수를 사용할 수 있어요

Comment on lines 36 to 39
private static ErrorResponse getErrorResponse(int status, Exception e) {
ErrorResponse errorResponse = new ErrorResponse(status, e.getMessage());
return errorResponse;
}
Copy link

Choose a reason for hiding this comment

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

private static ErrorResponse getErrorResponse(int status, Exception e) {
    return new ErrorResponse(status, e.getMessage());
}


@ExceptionHandler(NotUpdateException.class)
public ResponseEntity<ErrorResponse> handleNotUpdateException(NotUpdateException e){
return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(getErrorResponse(500, e));
Copy link

Choose a reason for hiding this comment

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

HttpStatus.INTERNAL_SERVER_ERROR.value()

response status로 500응답을 내려주시는 의도가 궁금해요.
500은 어떤것을 의미할까요?

Copy link
Author

Choose a reason for hiding this comment

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

NotUpdateException은 클라이언트의 요청은 올바르나 데이터 베이스의 update가 제대로 수행되지 않을 경우 발생하는 커스텀 예외로 서버쪽 오류라고 판단되어 500으로 처리하였습니다.

Copy link

Choose a reason for hiding this comment

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

@young970

한번 더 고민해볼 포인트가 생겼네요.
500에러를 사용자에게 보여줘야할지? 감춰야할지? 어떤것들을 더 고려해볼 수 있을까요?

import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;

@RestControllerAdvice
public class ApiExceptionHandler extends ResponseEntityExceptionHandler {
Copy link

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.

전반적으로 RuntimeException을 상속한 커스텀 클래스만 선언되어있는데요. 실행중에 발생할 수 있는 Unchecked 와 Checked모두 기재해주는게 좋아요.
Exception과 RuntimeException까지 설계해봅시다. 그거 터지면 나 자바 스프링으로 개발했다고 티내는 에러 납니다. 어쩌면 디비 뭐쓰는지까지 노출됩니다.

@@ -0,0 +1,4 @@
package org.prgrms.kdt.global;

public record ErrorResponse(int statusCode, String message) {
Copy link

Choose a reason for hiding this comment

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

에러에 대해 응답을 알려주는 스펙도 있어요 한번 참고해보세요

클라이언트가 에러에 대해 잘 알 수 있으면 해결하기 더 쉽지 않을까?

Comment on lines 24 to 27

@GetMapping("/new")
public String save() {
return "voucher/voucher_create";
Copy link

Choose a reason for hiding this comment

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

create랑 save랑 둘중 하나로 용어를 맞춰보면 어떨까요?

Comment on lines 49 to 54

@PostMapping("/{id}")
public String deleteById(@PathVariable UUID id) {
voucherService.deleteById(id);
return "redirect:/view/vouchers";
}
Copy link

Choose a reason for hiding this comment

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

/delete로 행위를 표현하기도 해요

Comment on lines 14 to 18
public interface ServiceVoucherMapper {
@Mapping(target = "voucherId", expression = "java(createUUID())")
@Mapping(target = "discountPolicy", expression = "java(createDiscountPolicy(request.voucherType(), request.discountAmount()))")
@Mapping(target = "createdAt", expression = "java(createLocalDateTime())")
Voucher serviceDtoToVoucher(ServiceCreateVoucherRequest request);
Copy link

Choose a reason for hiding this comment

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

java코드를 String으로 집어넣으면 어떤 단점이 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

createDiscountPolicy 등의 메서드 이름을 rename 시 인텔리제이가 String 부분은 인식하지 못해
변경 사항이 제대로 반영되지 않아 문제가 될 것 같습니다.


public record CreateWalletControllerRequest(UUID walletId, UUID memberId, UUID voucherId) {
public CreateWalletControllerRequest(UUID memberId, UUID voucherId) {
this(UUID.randomUUID(), memberId, voucherId);
Copy link

Choose a reason for hiding this comment

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

여기서 왜 UUID를 생성하는지 궁금합니다!

this.walletRepository = walletRepository;
}

@Transactional
Copy link

Choose a reason for hiding this comment

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

@Transactional(readonly=true)

라는 옵션에 대해서도 알아보세요

Copy link

@WooSungHwan WooSungHwan left a comment

Choose a reason for hiding this comment

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

영운님 코드가 많이 좋아졌군요 ㅎㅎ
하지만 3주차는 오프라인으로 만나기전까진 approve를 드릴순 없을 것 같습니다 ㅎㅎ
그때 좀더 이야기할 수 있는 부분이 있을 것 같습니다. 우선 영수님과 제가 드린 리뷰내용 토대로 수정해보고 계시고 수요일에 뵈어요 ㅎㅎ

return ResponseEntity.ok(response);
}

@GetMapping("/type/{type}")

Choose a reason for hiding this comment

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

(첨언)
API로 만든순간부터 이 조건을 다른 api의 기능으로 포함할 수 있는지에 대한 검토가 필요합니다.

Comment on lines 44 to 48
@GetMapping
public ResponseEntity<VoucherResponses> findAll() {
VoucherResponses response = voucherService.findAll();
return ResponseEntity.ok(response);
}

Choose a reason for hiding this comment

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

api 이렇게 열어두면 rest controller라서 endpoint에 요청 도착하면 repository findAll까지 바로 실행될겁니다.
해당 이슈를 어떻게 제한할 수 있을지 고민해보시기 바랍니다. (hint: 검색필터)

hint를 토대로 저 위에 있는 이슈도 함께 해결해보세요.

Comment on lines 26 to 30
@PostMapping
public ResponseEntity<VoucherResponse> create(@RequestBody CreateVoucherControllerRequest request) {
VoucherResponse response = voucherService.createVoucher(mapper.controllerDtoToServiceDto(request));
return ResponseEntity.ok(response);
}

Choose a reason for hiding this comment

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

보통 이렇게 만들면 ok(200)을 내려주기도 하지만 created(201)을 내려다주기도 합니다.
여기서 200과 201에 어떤 차이가 있는지 확인해보시고
ReponseEntity.created() 안에 들어가는 파라미터에 대해서 한번 보시면 좋을 듯 합니다.

Choose a reason for hiding this comment

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

CreateVoucherControllerRequest 해당 클래스에 대한 유효성 검사가 전혀 없습니다. API 엔드포인트로 기능을 노출했을 경우에는 최대한 많은 예외케이스에 대해서 고민해봐야합니다. 어떤 요청이 무작위로 날라올지도 모르거든요. 최악의 요청은 HTTP method, uri가 일치하지만 유효성 검증이 전혀 없어 null과 double의 기본값인 0.0이 디비로 들어가버릴 수 있겠네요.

return ResponseEntity.ok(response);
}

@GetMapping("/type/{type}")

Choose a reason for hiding this comment

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

참고로 pathVariable에 들어갈 수 있는 변수는 식별자로 구성되어야 좋습니다.

return ResponseEntity.ok(response);
}

@GetMapping("/type/{type}")

Choose a reason for hiding this comment

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

fixed로 어떤 바우처인지 특정할 수 있을까요?

import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;

@RestControllerAdvice
public class ApiExceptionHandler extends ResponseEntityExceptionHandler {

Choose a reason for hiding this comment

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

전반적으로 RuntimeException을 상속한 커스텀 클래스만 선언되어있는데요. 실행중에 발생할 수 있는 Unchecked 와 Checked모두 기재해주는게 좋아요.
Exception과 RuntimeException까지 설계해봅시다. 그거 터지면 나 자바 스프링으로 개발했다고 티내는 에러 납니다. 어쩌면 디비 뭐쓰는지까지 노출됩니다.

@young970
Copy link
Author

young970 commented Aug 1, 2023

피드백 반영사항

  • 에러 핸들링 케이스 추가
  • ErrorResponse 상세화
  • 서버 에러 발생시 로그 남기기
  • DTO 이름변경
  • Mapper에 존재하던 String으로 표시하던 자바 코드들 제거
  • 전체 조회 API 페이지네이션으로 변경 (findAllBy)
  • 바우처 타입 조회 API를 findAllBy에 적용 (검색 필터)
  • createVoucher 응답 상태코드 201로 변경
  • PK값 순차적으로 생성되는 UUID로 변경

@young970 young970 requested a review from devYSK August 1, 2023 14:11
@young970 young970 requested a review from WooSungHwan August 1, 2023 14:11
//web
implementation 'org.springframework.boot:spring-boot-starter-web'

//타입리프
Copy link

Choose a reason for hiding this comment

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

오타나신거같아용

Comment on lines +30 to +36

for (FieldError fieldError : bindingResult.getFieldErrors()) {
stringBuilder.append(fieldError.getField()).append(":");
stringBuilder.append(fieldError.getDefaultMessage());
stringBuilder.append(", ");
}
int statusCode = HttpStatus.BAD_REQUEST.value();
Copy link

Choose a reason for hiding this comment

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

메서드로 뺄 수 있을것 같습니다

return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(getErrorResponse(statusCode, stringBuilder.toString(), request.getRequestURI()));
}

@ExceptionHandler({NullPointerException.class, InvalidInputException.class, InvalidDiscountException.class, MissingRequestHeaderException.class, HttpMessageNotReadableException.class,
Copy link

Choose a reason for hiding this comment

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

NPE는 서버측 잘못일수도 있다고 생각해요


import java.time.LocalDateTime;

public record ErrorResponse(int statusCode, String detail, String instance, String time) {
Copy link

Choose a reason for hiding this comment

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

필드가 많아지니 개행할 위치가 애매해지죠.

가독성을 위해

public record ErrorResponse(
    int statusCode, 
    String detail, 
    String instance, 
    String time
) { }

로 둬도 좋을것 같아요.

instance라는 필드는 애매모호해보입니다. 명확하게 바꾸는건 어떨까요?

import java.util.UUID;

@Component
public class GeneratorImp implements Generator{
Copy link

Choose a reason for hiding this comment

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

Imp는 무엇인가용

public SearchRequest(long page, long recordSize, VoucherType voucherType) {
this.page = page;
this.recordSize = recordSize;
this.offset = (page - 1) * recordSize;
Copy link

Choose a reason for hiding this comment

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

new SearchRequest(-999999, -10, ~,~)


import org.prgrms.kdt.voucher.domain.VoucherType;

public class SearchRequest {
Copy link

Choose a reason for hiding this comment

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

Pageable 객체에 대해 알아보세용

Comment on lines +19 to +20
VoucherType voucherType = request.voucherType();
return new Voucher(generator.generateId(),
Copy link

Choose a reason for hiding this comment

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

한칸 개행해주면 더 읽기 좋을것같아요

import java.util.UUID;

public interface WalletCommandRepository {
Wallet insert(Wallet wallet);
Copy link

Choose a reason for hiding this comment

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

Wallet을 집어넣는다?

Voucher랑 target을 집어넣는것도 어색하진 않을거같은데용

Comment on lines +83 to +84
Voucher savedVoucher1 = new Voucher(UUID.randomUUID(), VoucherType.FIXED, VoucherType.FIXED.createPolicy(30.0), LocalDateTime.now());
Voucher savedVoucher2 = new Voucher(UUID.randomUUID(), VoucherType.FIXED, VoucherType.FIXED.createPolicy(30.0), LocalDateTime.now());
Copy link

Choose a reason for hiding this comment

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

voucherSource를 이용할 수 있을것 가아용

Copy link

@WooSungHwan WooSungHwan left a comment

Choose a reason for hiding this comment

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

영운님 과제하시느라 수고 많으셨습니다. 해당 과제는 merge 해주시고 리뷰가 좀더 필요하다고 생각되는 부분이 있으시면 짚어주세요. 근데 개인적으로는 voucher 과제는 여기서 닫고 이후 과제에서의 포커스를 좀더 집중하는게 좋다고 생각합니다. 이 부분은 이해하셨으리라 생각합니다. voucher 과제하시느라 수고 많으셨어요 ㅎㅎ 이제 좀더 쉽게 쉽게 결정하고 코드를 작성하실 수 있을 거라고 생각이 됩니다. 많이 성장하셨네요. 바우처 과제는 처음 내가 스스로의 지식으로 짰던 첫번째 코드라고 인식하고 가져가시고 이후 많이 돌아보세요. 시공간을 초월한 코드리뷰를 해보면서 스스로의 성장을 느껴보셨으면 좋겠습니다.

수고 많으셨습니다.

@young970 young970 merged commit 0b58e6e into prgrms-be-devcourse:young970/p1 Aug 10, 2023
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.

3 participants