Conversation
chounjae
left a comment
chounjae
left a comment
There was a problem hiding this comment.
고생하셨습니다.
이제부터는, 기능 상 동작하나 더 공부했으면 하는 부분은 [Study], 코드적 결함 의심은 [Warning] 으로 나눠서 리뷰 할게요
| return ApiResponse.success(SuccessCode.PRODUCT_OK, response); | ||
| } | ||
|
|
||
| @GetMapping("/brand") |
There was a problem hiding this comment.
[Study]
브랜드별 조회같은 필터링하는 로직들의 경우, 파라미터 형식으로만 받는 경우가 많습니다.
RESTful API를 무조건 지켜야 하는 것은 아니지만, 대체로 pathvariable에는 해당 자원 / 지금 같은 경우는 products, {product_id} 정도가 있겠네요
이미 requestParam으로 brand를 받는 상황에서 /brand 추가로 분기하는 것보다는 기존 API 통합이나, 새로운 방식이 좋을 듯 싶습니다
There was a problem hiding this comment.
이미 GetMapping으로 getAllProducts가 있어서 /brand로 분기한건데 생각해보니.. URI구성도 좋지 않게 되겠네요. 그래서 메서드를 통합할지, GetMapping(params="brand")로 분리해둘지 두 가지 케이스인 것 같습니다. 어느쪽이 더 좋다고 보시는지 의견 여쭙겠습니다!!
There was a problem hiding this comment.
There was a problem hiding this comment.
[정리]
하나로 가져올 수 있는 방식이 더 나은것 같습니다.
이유는 굳이 거의 동일한 동작의 API를 여러개 만들거나 할 필요는 굳이 없을 것 같아서요.
적당히 세가지 방법이 있습니다.
- 그냥 QueryDSL 쓰자
- 이미 QueryDSL을 쓰고 있는 환경이거나, 혹은 이러한 필터값이 너무 많을때 유효해보이는 전략입니다.
- 제가 생각하기에는 가장 괜찮은 접근인데, 초기 세팅이나 기타 컨벤션 추가, 아키텍처 가이드라인 수정 등의 이슈가 있을 수 있어요.
- 번외로 QueryDSL 비슷한 다른 동적 쿼리 특화 도구들이 좀 있어요.
- kotlin jdsl 같은게 있는데 세팅이 훨씬 쉬운것에서 이게 좀 더 나은 대안같기도 합니다. 라인에서 만든거라 개발자 지원이나 이런게 훨씬 접근성이 좋아요.(디스코드 운영)
- 단 우리 플젝은 java를 쓰기에 유효한 선택지는 아닌것 같습니다. 나중에 Kotlin + Spring하실때 한번 찾아보세요.
- Specification
ref: https://velog.io/@jun-k0/%EC%8A%A4%ED%94%84%EB%A7%81%EB%B6%80%ED%8A%B8-JPA-Specification
한계: https://blaxsior-repository.tistory.com/288
- 이건 저희가 chatgpt한테 질문해서 나온 답변입니다.
- 일단 한눈에봐도 귀찮고, 여러가지 한계점이 있어서 권장되지는 않는 것 같습니다.
- QBE(Query by Example)
- Example을 이용하는 방식입니다.
- 눈으로 봤을 땐 제일 간단합니다.
- 다만 얘는 특히 한계가 명확합니다. Equal 만 가능해요. LIKE나 기타 복잡한 필터로 동작을 기대한다면 사용할 수 없습니다.
저의 결론은?
- 이미 QueryDSL 관련 세팅이 있는걸로 압니다. 그걸로 동적 쿼리로 작성하는게 나아보여요.
- 로직은 똑같은데, 필터 존재 여부에 따라서 결과가 달라질 수 있도록 하는게 제일 깔끔하고 합리적으로 생각됩니다.
There was a problem hiding this comment.
[todo]
- 운재님 말대로 브랜드는 쿼리 파라미터로 받는게 좀 더 restful하니 수정 부탁드립니다.
- 하나의 로직으로 유연한 쿼리 대응을 하는건 위 리뷰에서 제안된 3가지 방법 중 하나 사용하시면 됩니다.
- 개인적인 권장은 QueryDSL 세팅이 있으니 해당 부분을 활용하는 것인데, 판단에 맡기겠습니다.
- 수고 많으셨습니다 !
| @RequestParam String brand, | ||
| @PageableDefault(size = 10, sort = "releasedAt", direction = Sort.Direction.ASC) Pageable pageable | ||
| ) { |
There was a problem hiding this comment.
[Warning]
현재 sort 방식이 단순 releasedAt이 오래된 것부터 정렬될 것 같습니다.
그러면 이미 발매가 지났던 상품들도 모두 필터링 되어, 원활한 정렬이 안될 것 같네요. 확인 부탁드려요
There was a problem hiding this comment.
좋은 지적 감사합니다. 서비스 계층에서 getAllProuducts 메서드도 같은 방식으로 정렬하는데, 이 부분도 그럼 수정이 필요하지 않을까 싶은데 어떻게 생각하시나요?
There was a problem hiding this comment.
getAllProducts는 제가 만들 당시 사용자가 사용하는 API가 아닌 개발할 때 단순 모든 products 확인용으로 만든 임시 API 였습니다! 당연히 getAllProducts도 과거의 물품부터 정렬되겠네요.
다만, 앞에서 말한 것처럼, 테스트용이므로 정렬기준을 바꾸기보다는 admin 전용 API로 전환하거나 삭제하는게 더 적절할 듯 싶습니다
| where upper(p.brand) = upper(:brand) | ||
| and p.status = :status | ||
| """) | ||
| Page<Product> findByBrandIgnoreCase(@Param("brand") String brand, @Param("status") ProductStatus status, Pageable pageable); |
There was a problem hiding this comment.
그냥 간단한 퀴즈 하나 드리겠습니다.
위 @query 없이 쿼리 메소드만으로 동일한 기능을 구현하려면 메서드 이름을 어떻게 구성 해야할까요?
There was a problem hiding this comment.
ㅎㅎ findByBrandIgnoreCaseAndStatus 가 되겠네요 쿼리문 짜는거 연습도 해볼겸 이렇게 구성했습니다!
hefour
force-pushed
the
develop
branch
2 times, most recently
from
a41a000 to
a9112d5
Compare
4 participants
📌 Issue
✍️ Summary
📢 Notify
최대한 기존 로직에서 벗어나지 않는 방향으로 구현했습니다.