[1단계 - 상품 관리 기능] 허브(방대의) 미션 제출합니다. #244

greeng00se · 2023-04-27T07:22:43Z

안녕하세요! 웨지!

코드를 작성할 때 신경 쓴 점

테스트를 꼼꼼히 작성하려고 노력했습니다.
TDD를 바텀업 방식으로 진행했습니다.

질문

update 나 delete 시 select 을 이용해서 데이터가 먼저 있는지 검증해도 괜찮을까요?

현재 update 나 delete 요청 시 요청한 값이 없는지 Dao 클래스의 findById() 를 통해 먼저 찾은 뒤 예외를 던지고 있습니다.

public void delete(final Long id) {
        productDao.findById(id)
                .orElseThrow(() -> new NoSuchElementException("상품을 찾을 수 없습니다."));
        productDao.delete(id);
    }

조회의 경우에 예외를 던지는 게 괜찮다고 생각이 드는데, 수정과 삭제에도 위와 같이 예외를 던지는 것이 좋을까요?

이번 미션에서 적절하게 예외처리를 했는지 궁금합니다.

2단계에서 추가적으로 궁금한 점이 많이 생긴다면 페어와 함께 고민해서 PR에 남겨보도록 하겠습니다!

감사합니다 웨지 👍

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

sihyung92

허브신님 안녕하시옵니까, 리뷰어 웨지이옵니다.
허브신님의 리뷰를 할 수 있게 되어 영광으로 생각하고 있습니다.

질문해주신 부분에 대한 답변은 리뷰 내용에 포함이 되어있는 것 같아 송구하옵게도 따로 답변하진 않겠사옵니다.
리뷰 반영해서 재요청 주시면 확인해보겠사옵니다.
감사하옵니다.

sihyung92 · 2023-04-28T17:00:11Z

http-request.http

@@ -0,0 +1,23 @@
+### 상품 추가


http를 통해 실제 요청을 테스트할 수 있어 좋네요 ㅎㅎ

감사합니다 😄

sihyung92 · 2023-04-28T17:02:49Z

src/main/java/cart/domain/Product.java

+ validatePrice(price);
+ this.id = id;
+ this.name = name;
+ this.image = image;


이미지에 대해선 검증하지 않나요? ㅎㅎ

이미지에 대한 검증을 하지 않았군요. 😢
페어랑 할 때 확장자로 검사할 지 고민했었는데, 이미지 링크의 경우 이미지에 대한 확장자로 끝나지 않는 경우도 있었기 때문에 일단 빈 값이 아닌지만 검증을 해보도록 하겠습니다!

넵 ㅎㅎ 이미지 URI 같은 경우엔 URI 객체를 비롯해 이미 선배님들이 다양한 검증 객체를 만들어 두었으니 직접 구현하기보단 기구현 라이브러리를 사용하는 것도 방법입니다

sihyung92 · 2023-04-28T17:03:15Z

src/main/java/cart/dto/ProductSaveRequestDto.java

+
+public class ProductSaveRequestDto {
+
+ @NotBlank(message = "이름은 공백일 수 없습니다.")


bean validation 활용 👍

sihyung92 · 2023-04-28T17:03:55Z

src/main/java/cart/dto/ProductSaveRequestDto.java

+import javax.validation.constraints.Size;
+import org.hibernate.validator.constraints.Range;
+
+public class ProductSaveRequestDto {


Request라는 객체가 없는데 Dto라고 하니까 어색하네요. Request까지만 있었어도 의미가 전달될거 같아요.

ProductDto는 뒤에 Dto를 붙여서 일관성 있게 붙여야하지 않을까? 고민했었는데, 코멘트 남겨주신대로 Request만 있어도 충분히 의미를 전달할 수 있을 것 같습니다 👍

sihyung92 · 2023-04-28T17:04:53Z

src/main/java/cart/exception/GlobalExceptionHandler.java

+ @ExceptionHandler(Exception.class)
+ public ResponseEntity<ExceptionResponse> handleException(final Exception e) {
+ return ResponseEntity.internalServerError()
+ .body(new ExceptionResponse("서버가 응답할 수 없습니다."));


예측할 수 없는 500상황에서 메시지를 감추는 것 좋습니다 ㅎㅎ

sihyung92 · 2023-04-28T17:10:09Z

src/test/java/cart/controller/PageControllerTest.java

+ "products",
+ hasItem(generatePropertiesMatcher(id2, "고양이", "cat.jpg", 1000000L))
+ ))
+ .andDo(print());


로그까지 👍

sihyung92 · 2023-04-28T17:15:29Z

src/main/java/cart/dao/ProductDao.java

+ .usingGeneratedKeyColumns("id");
+ }
+
+ public Optional<Long> saveAndGetId(final Product product) {


왜 Optional 인가요? save에서 id가 생성 안 되었으면 오류 상황인데, Optional 처리는 오류 제어를 지연하는 셈이 됩니다. nullable한 데이터도 아니고요.

사용하는 쪽에서 안전하게 처리할 수 있도록 Optional을 적용했었는데, 생각해보니 불필요한 Optional인 것 같습니다! 👍 👍 👍

sihyung92 · 2023-04-28T17:16:26Z

src/main/java/cart/dao/ProductDao.java

+ public void update(final Product product) {
+ final String sql = "update product set name = ?, image = ?, price = ? where id = ?";
+ jdbcTemplate.update(sql, product.getName(), product.getImage(), product.getPrice(), product.getId());
+ }
+
+ public void delete(final Long id) {
+ final String sql = "delete from product where id = ?";
+ jdbcTemplate.update(sql, id);
+ }


update와 delete 모두 affected row가 나오는데, 단건 수정이므로 해당값이 1인지 아닌지 를 통해 수정 / 삭제 여부를 검증할 수 있습니다 :)

이러면 exist에 대한 검증을 대신할 수 있으리라 봅니다

오 조회를 추가적으로 했었는데 이런 좋은 방법이 있었군요!
추가적으로 Service에서 affected row에 대한 검증을 수행하도록 했습니다!

sihyung92 · 2023-04-28T17:25:29Z

src/main/java/cart/service/ProductService.java

+ public void update(final Long id, final ProductUpdateRequestDto request) {
+ productDao.findById(id)
+ .orElseThrow(() -> new NoSuchElementException("상품을 찾을 수 없습니다."));
+ final Product savedProduct = new Product(id, request.getName(), request.getImage(), request.getPrice());
+ productDao.update(savedProduct);
+ }
+
+ public void delete(final Long id) {
+ productDao.findById(id)
+ .orElseThrow(() -> new NoSuchElementException("상품을 찾을 수 없습니다."));
+ productDao.delete(id);
+ }


질문 주신 부분이네요, delete는 구성상 멱등한 결과로 이어지므로 응답결과는 200으로 내리거나 400으로 내리거나 취향이라고 생각합니다.
-> 어차피 서버에 최종 형태는 동일하다는 의미입니다. 1 id에 대한 삭제 요청을 했으면 서버에선 1 id를 가진 상품이 항상 삭제되므로 멱등합니다. (존재했든 비존재했든 간에) 그래서 200, 성공 응답을 해도 어색하진 않지만 클라이언트에게 더 나은 정보를 제공하기 위해 404 등을 응답하는 것도 좋은 방법입니다. (500대는 오류로 인해 클라이언트가 재처리 요청 등을 할 수 있으므로 안 됩니다)

update는 해당 id가 없을 경우 동작이 달라져 완전히 멱등하다고 생각되진 않으므로 대상이 없을 때 400대 코드를 내려주는 게 더 낫다고 보구용.

선 조회를 할 필요는 없지만 (제 리뷰를 반영하면 조회없이 판단이 가능합니다) 또 선 조회를 해서 처리하는 거 자체는 괜찮다고 봅니다. id 단건 조회는 db 부하를 거의 주지 않으므로 비효율을 초래한다고 보지 않습니다.

꼼꼼하게 답변해주셔서 감사합니다!
delete의 멱등성 때문에 고민을 잠시 했었는데요! 이번에는 클라이언트에게 조금 더 많은 정보를 제공하는 방향으로 코드를 작성해보겠습니다!

sihyung92 · 2023-04-28T17:27:01Z

src/test/java/cart/service/ProductServiceTest.java

+ @Test
+ void 없는_상품을_수정하는_경우_NoSuchElementException_을_던진다() {
+ // given
+ final Long id = 999999999999L;


기왕 하는거 Long.MAX_VALUE도 있습니다 ㅎㅎ

👍 👍 👍

greeng00se · 2023-04-28T19:24:23Z

늦은 시간에 리뷰 남겨주셔서 감사하옵니다.
행복한 주말 보내시고, 시간 되실 때 웨지신님이 의도한 코멘트가 맞는지 확인 부탁드리옵니다.

sihyung92

허브신님 또 웨지이옵니다. 하루 두 번 영접할 수 있다니 영광이옵니다.
제가 의도한대로 리뷰를 반영해주셔서 레벨 1은 머지하오니, 레벨 2 미션을 진행해 주시면 될 것 같사옵니다.
허멘

sihyung92 · 2023-04-29T07:45:51Z

src/main/java/cart/domain/Product.java

+ validatePrice(price);
+ this.id = id;
+ this.name = name;
+ this.image = image;


넵 ㅎㅎ 이미지 URI 같은 경우엔 URI 객체를 비롯해 이미 선배님들이 다양한 검증 객체를 만들어 두었으니 직접 구현하기보단 기구현 라이브러리를 사용하는 것도 방법입니다

sihyung92 · 2023-04-29T07:48:05Z

src/main/java/cart/service/ProductService.java


+@Transactional


ㅎㅎ 빼먹을 수 있는 트랜잭쇼놀 디폴트 처리 좋습니다

감사합니다 👍 처음 PR 할 때 넣어야지 하다가 빼먹었더라고용 😢

sihyung92 · 2023-04-29T07:54:35Z

src/main/java/cart/dto/ProductSaveRequest.java

+import javax.validation.constraints.Size;
+import org.hibernate.validator.constraints.Range;
+
+public class ProductSaveRequest {


첫 리뷰 때 작성한거 같은데 무언가~? 모종의 이유로~? 날아가버린 리뷰가 있네요.
ProductSaveRequest와 ProductUpdateRequest가 완전히 동일한데, 재사용할 수 없을까? 라는 리뷰를 남겼었어요.
사실 생성과 수정은 서로 달라질 개연성이 높아서 미리 분리해놓는 게 더 좋은 방법이긴 한데, 그래도 중복은 싫어서 저도 요즘 이런저런 방법들을 시도해보는 중 입니다. 허브는 이 부분에 대해 어떤 생각을 가지고 있을지 궁금하네요 ㅎㅎ

저장과 수정할 때 필요한 필드값이 동일하여 현재 구조에서는 하나로 사용해도 된다고 생각을 하지만,
말씀해주신대로 요구사항이 변경된다면 달라질 가능성이 높다고 판단하였습니다!
웨지가 시도해보신 여러가지 방법들이 궁금하네요 😄

Songusika and others added 22 commits April 25, 2023 15:03

docs: 요구사항 정의

b56670d

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 상품 도메인 추가

71934eb

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: db 테이블 추가

273275a

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: Dao 상품 저장 및 전체 조회 기능 추가

0dab4f7

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 서비스 상품 저장 기능 추가

264c9b3

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 페이지 접근 기능 추가

ee3f7b2

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 데이터베이스 설정 추가

79371f7

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 상품 저장 기능 추가

052ec84

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

docs: 요구사항 체크

0893198

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 서비스 상품 조회 기능 추가

89983ba

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 상품 조회 기능 추가

4439085

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 상품 수정 기능 추가

4d41187

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 상품 삭제 기능 추가

20bf537

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

refactor: RestController, Controller 분리

c334322

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: Thymeleaf 단일 상품 조회 페이지 추가

c9b051f

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 단일 조회 기능 구현

915f69e

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

test: 테스트 중복 제거 및 위치 변경

ee06f64

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 저장시 location header 반환기능 추가

15f68e2

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

feat: 예외처리 기능 추가

78edd9f

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

refactor: 오타 수정

f19c7fe

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

refactor: 사용하지 않는 DTO 클래스 삭제

fb5916b

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

refactor: 예외메시지 수정

69b8fef

Co-authored-by: greeng00se <thegreengoose7@gmail.com>

sihyung92 requested changes Apr 28, 2023

View reviewed changes

greeng00se added 7 commits April 29, 2023 02:36

feat: 이미지 검증 기능 추가

a0444c7

refactor: 모든 RequestDto -> Request 클래스명 변경

3c5df38

refactor: 필요없는 Optional 제거

62ed7ae

feat: 커스텀 예외 적용

2a64763

feat: 엔티티에 audit 기능 추가

86a86d0

refactor: 컨트롤러 메서드명 변경

9cdd6dd

feat: Service에 Transactional 추가

0f263e3

greeng00se added 5 commits April 29, 2023 04:11

test: Long.MAX_VALUE 적용!

148b296

test: expect 빠진 부분 수정

1140fc5

refactor: 예약어 대문자로 변경

d11b36f

test: test 설정 파일 수정

9b7ff3d

test: Controller 테스트에서 id를 이용하여 검증하는 방향으로 수정

a462217

sihyung92 approved these changes Apr 29, 2023

View reviewed changes

sihyung92 merged commit f282f04 into woowacourse:greeng00se Apr 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1단계 - 상품 관리 기능] 허브(방대의) 미션 제출합니다. #244

[1단계 - 상품 관리 기능] 허브(방대의) 미션 제출합니다. #244

greeng00se commented Apr 27, 2023

sihyung92 left a comment

sihyung92 Apr 28, 2023

greeng00se Apr 28, 2023

sihyung92 Apr 28, 2023

greeng00se Apr 28, 2023

sihyung92 Apr 29, 2023

sihyung92 Apr 28, 2023

sihyung92 Apr 28, 2023

greeng00se Apr 28, 2023

sihyung92 Apr 28, 2023

sihyung92 Apr 28, 2023

sihyung92 Apr 28, 2023

greeng00se Apr 28, 2023

sihyung92 Apr 28, 2023

greeng00se Apr 28, 2023

sihyung92 Apr 28, 2023

greeng00se Apr 28, 2023

sihyung92 Apr 28, 2023

greeng00se Apr 28, 2023

greeng00se commented Apr 28, 2023

sihyung92 left a comment

sihyung92 Apr 29, 2023

sihyung92 Apr 29, 2023

greeng00se Apr 29, 2023

sihyung92 Apr 29, 2023

greeng00se Apr 29, 2023


		public class ProductSaveRequestDto {

		@NotBlank(message = "이름은 공백일 수 없습니다.")


		@Transactional

[1단계 - 상품 관리 기능] 허브(방대의) 미션 제출합니다. #244

[1단계 - 상품 관리 기능] 허브(방대의) 미션 제출합니다. #244

Conversation

greeng00se commented Apr 27, 2023

코드를 작성할 때 신경 쓴 점

질문

sihyung92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greeng00se commented Apr 28, 2023

sihyung92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment