[2단계 - 장바구니 기능] 허브(방대의) 미션 제출합니다. #300

greeng00se · 2023-05-04T08:50:56Z

안녕하세요 웨지 오랜만입니다!
잘 지내셨나요?

일찍 머지해주셔서 시간이 많았지만 사이드 프로젝트를 조금 한다고 제출이 조금 늦어졌습니다. 😢

2단계 미션을 진행하면서 신경 쓴 부분은 다음과 같습니다.

테스트 코드
인증과정

인증을 할 때 인터셉터를 사용했는데요. 인터셉터와 ArgumentResolver 둘 다 DB에서 정보를 가져오는게 중복이라고 생각하여 인증 정보를 전달하는 방법으로 구현했습니다.
인터셉터에서 인증을 한 뒤에 인증정보를 ArgumentResolver로 전달하기 위해 다양한 방법을 생각해보았는데요.
저는 ThreadLocal를 이용해서 전달하도록 했습니다.
다양한 방법이 있지만 ThreadLocal을 사용하는 방법이 명확하다고 느꼈습니다.
헤더에 넣는 방법도 있고 다른 크루는 RequestScope를 사용해서 전달하는 걸 봤는데요.
어떤 방법으로 하는게 괜찮을지 웨지의 의견을 듣고 싶습니다.

추가적인 궁금증이 생긴다면 DM이나 해당 PR에 남겨두도록 하겠습니다!

감사합니다 웨지 😄

sihyung92

허브신님 안녕하세요, 웨지입니다
새롭게 추가된 요구사항을 잘 반영해주셨네요 👍 auth 패키지를 분리해보고, ThreadLocal과 같은 시도를 해보신게 인상 깊었습니다.

질답

저라면 Dao 재조회로 구현할거 같아요 ㅎㅎ email 인덱스가 잡혀있다면 조회부담이 작기 때문에요. 향후 성능이 문제가 되면 말씀해주신 ThreadLocal 같은 캐싱을 고려해 볼거 같아요. (@requestScope도 방법이겠네요)
개념상 요청~응답까지의 생명주기를 가지는 RequestScope나 쓰레드당 하나씩 할당되는 ThreadLocal의 개념상 동작이 크게 차이날거 같진 않지만요, ThreadLocal이 가지는 위험성(전역변수와 크게 다르지 않음)을 고려해보면 ThreadPool이 최대로 활용되는 부하상황에서도 버그가 없을까? 걱정되긴 해서 좀 더 연구를 해보거나, 이미 검증된 RequestScope를 활용할거 같네요 ㅎㅎ
구조에 대한 리뷰를 하나 드렸으니 확인해보시고 다시 요청주세용~~ 연휴 잘 보내시구요~~

sihyung92 · 2023-05-05T02:48:04Z

src/main/java/cart/auth/CredentialThreadLocal.java

+@Component
+public class CredentialThreadLocal {
+
+    private final ThreadLocal<Credential> local = new ThreadLocal<>();


일급컬렉션으로 활용하는 부분 좋네요 :)

감사합니다 😄

sihyung92 · 2023-05-05T02:50:46Z

src/main/java/cart/auth/AuthInterceptor.java

+
+        final Credential credential = basicAuthorizationParser.parse(authorizationHeader);
+        final Member member = authMemberDao.findByEmail(credential.getEmail())
+                .orElseThrow(AuthenticationException::new);


"인증에 실패했습니다" 라는 오류메시지는 정보가 너무 적습니다. 향후 디버깅에 유용하게 활용하려면 오류 정보를 풍부화하고 (+ 로깅),
클라이언트에게 인증에 대한 오류 내용을 감추고 싶다면 AuthenticationException를 응답으로 변환하는 과정 (ExceptionHandler) 에서 응답객체의 메시지만 "인증에 실패했습니다"로 내려주면 될거같네요

예외 상황에서 아무것도 출력하지 않으니 예외가 터졌을 때 불편함이 많이 생기더라고용..

그래서 말씀해주신대로 다음과 같이 변경하였습니다! 👍 👍 👍

ExceptionHandler에 로깅 추가

인증과정에서 조금 더 자세한 정보를 메시지에 담도록 수정

아래와 같이 인증 실패시 간단한 정보를 담아서 반환하도록 수정

return ResponseEntity.status(HttpStatus.UNAUTHORIZED) .body(new ExceptionResponse("인증에 실패했습니다."));

sihyung92 · 2023-05-05T03:15:08Z

src/main/java/cart/domain/CartProduct.java

+    private final LocalDateTime createdAt;
+    private final LocalDateTime updatedAt;


비즈니스적으로 의미 없고 데이터 관리 측면에서 사용되는 값이니 애초에 엔티티에 포함하지 않는 것도 방법입니당

생각지도 못한 방법! 편안하게 다 지워버렸습니다~

sihyung92 · 2023-05-05T03:19:43Z

src/main/java/cart/auth/BasicAuthorizationParser.java

+    public Credential parse(final String authorizationHeader) {
+        final String[] credential = parseCredential(authorizationHeader);
+        return new Credential(credential[EMAIL_INDEX], credential[PASSWORD_INDEX]);
+    }


Authroization 헤더는 베이직 뿐만 아니라 다양한 방식의 인증 방식이 전달될 수 있습니다. Bearer 토큰이 전달되면 500이 발생할 코드로 보이네요.

현재 인터셉터에서 Parser에게 파싱 가능한지 여부를 물어보고 파싱을 진행하고 있습니다.
말씀해주신대로 바로 parse를 하게 되면 500이 발생할 것으로 보입니다.
남겨주신 코멘트를 보고 BasicAuthorizationParser 에 대한 응집도가 떨어진다고 생각했습니다.

따라서 다음과 같이 변경하였습니다.

parse를 하지 못하는 헤더를 입력받는 경우 InvalidBasicCredentialException 예외를 던지도록 변경

인터셉터에서 파싱 가능 여부를 확인하는 코드 제거

InvalidBasicCredentialException 예외에 대한 ExceptionHandler 추가

@ExceptionHandler({AuthenticationException.class, InvalidBasicCredentialException.class}) public ResponseEntity<ExceptionResponse> handleAuthenticationException(final RuntimeException e) { logger.warn(e.getMessage()); return ResponseEntity.status(HttpStatus.UNAUTHORIZED) .body(new ExceptionResponse("인증에 실패했습니다.")); }

하지만 예외 처리에 대한 부분에서 약간 찜찜함이 남습니다.
Interceptor에서 InvalidBasicCredentialException 예외를 잡아 인증 실패 예외로 전환해주는 것이 더 좋을까요?
또한 이 경우 명시적으로 예외를 잡아 처리할 수 있게 InvalidBasicCredentialException을 CheckedException으로 변경하는게 좋을까요?
웨지는 어떻게 생각하시나요?

제출하기 전 코드를 확인하다가 InvalidBasicCredentialException이 AuthenticationException을 상속 받도록 수정하면 괜찮을 것 같아서 적용해보았습니다~

public class InvalidBasicCredentialException extends AuthenticationException

@ExceptionHandler(AuthenticationException.class) public ResponseEntity<ExceptionResponse> handleAuthenticationException(final AuthenticationException e) { logger.warn(e.getMessage()); return ResponseEntity.status(HttpStatus.UNAUTHORIZED) .body(new ExceptionResponse("인증에 실패했습니다.")); }

sihyung92 · 2023-05-05T03:29:07Z

src/main/java/cart/auth/Credential.java

+
+public class Credential {
+
+    private final Long id;


활용을 보면 memberId 네요~~
id라고 해두면 Credential이라는 엔티티에 대한 ID라고 오인할 수 있으니 이런 경우는 분명히 해주셔요

확실히 말씀해주신대로 처음 코드를 본 사람이라면 엔티티에 대한 ID로 오해할 수 있을 것 같습니다!
바로 변경했습니다 👍 👍 👍

sihyung92 · 2023-05-05T03:47:54Z

README.md

+| 기능  | Method | URL            |
+|-----|--------|----------------|
+| 생성  | POST   | /products      |
+| 수정  | PATCH  | /products/{id} |
+| 삭제  | DELETE | /products/{id} |


API 명세서 좋네요 ㅎ
요청객체와 응답객체 형태, 그리고 발생할 수 있는 오류와 그 때의 오류 코드 및 응답객체 까지 추가한다면 현업에서도 활용할만한 API 문서가 됩니다

오.. 코멘트를 읽고 궁금한게 웨지는 Swagger, Restdocs 중 어떤걸 선호하시나요?
현업에서는 RestDocs에서 OpenAPI Spec을 추출해서 Swagger UI를 연동해서 많이 사용하나요?

예전엔 프로덕션 코드에 문서화 코드가 들어가는게 싫어서 restdocs 선호했는데 요샌 문서 옆에 띄워놓고 문서화 주석들을 바로 비교 / 수정이 가능한 Swagger가 더 괜찮다 싶긴해요.
회사마다 다르겠지만 저희 회사는 팀마다 문서화 방식이 달라서 각자 알아서하는데,
말씀하신 것처럼 Restdocs -> OpenApi Spec -> Swagger 같은 커스텀은 수요가 적을거 같아요 (그 작업 할 개발 리소스를 기능 개발 / 리팩토링에 투자할 거 같습니다)

sihyung92 · 2023-05-05T03:48:25Z

src/main/java/cart/auth/Auth.java

+
+@Target(ElementType.PARAMETER)
+@Retention(RetentionPolicy.RUNTIME)
+public @interface Auth {


커스텀 어노테이션 활용 👍

sihyung92 · 2023-05-05T03:53:08Z

src/main/java/cart/auth/Auth.java

@@ -0,0 +1,11 @@
+package cart.auth;


나머지는 레이어드 아키텍처 기준으로 패키지 규칙이 정해져있는데, 갑자기 도메인 기준의 패키지가 나와버리면 혼동이 올거 같아요 ㅎ auth 패키지 내에선 cart의 다른 도메인을 의존하지 않도록 잘 구성해주셨으니, 아예 cart/auth가 아니라 auth 패키지로 분리하는건 어떨까요? (이 경우 component scan 범위가 달라지는데, 해당 부분도 어떻게 해결할 수 있을지 학습해보면 좋을 거같아요)

좀 더 코드를 보다보니 Member에 대한 의존이 있었군요 ㅎ 저라면 Member를 똑같이 복사해 해당 패키지에서 다루던지, 아니면 Credential 객체를 Member 테이블로부터 직접 조회하는 방법을 쓸거같습니다
아래 두가지 방식 중 취사선택 해주세용

기존 레이어드 방식으로 코드 재구성

auth에서 cart 의존성 모두 제거하고 코드분리

현재의 패키지 규칙을 생각하지 못하고, 모두 auth 패키지 안에 모두 작성해 일관성이 깨졌네요.. 😢 생각하지 못했습니다..
auth 패키지로 분리한다면 다음과 같이 컴포넌트 스캔을 위한 설정파일을 추가로 작성하여 해결할 수 있을 것 같습니다!

@ComponentScan(basePackages = "auth") @Configuration public class ComponentScanConfiguration { }

현재는 레이어드 방식으로 코드를 재구성하는 것이 더욱 간단해 보여서 1번을 선택해보겠습니다 😄

sihyung92 · 2023-05-05T04:21:35Z

src/main/resources/data.sql

+INSERT INTO PRODUCT (name, image, price)
+VALUES ('피자스쿨 치즈피자', 'https://t1.daumcdn.net/cfile/tistory/2647BE3754B7E8B733', 8900);
+INSERT INTO PRODUCT (name, image, price)
+VALUES ('도미노 치즈피자', 'https://cdn.dominos.co.kr/admin/upload/goods/20200311_TI57KvOH.jpg', 23900);


저는 페페로니가 더 좋습니다

어디 브랜드를 선호하시나요?
브랜드를 안달아주셔서 일단 도미노로..

INSERT INTO PRODUCT (name, image, price) VALUES ('도미노 페페로니피자', 'https://cdn.dominos.co.kr/admin/upload/goods/20200311_x8StB1t3.jpg', 25900);

요즘은 잭슨피자만 패구 있습니다

sihyung92 · 2023-05-05T04:21:57Z

src/main/resources/static/js/cart.js

@@ -8,9 +8,13 @@ const addCartItem = (productId) => {

    // TODO: [2단계] 장바구니 CRUD API에 맞게 변경


완료된 주석은 삭제해주셔용 ㅎㅎ (js긴 하지만)

제거했습니다! 👍 👍

- AuthMemberDao -> CredentialDao - 추가로 Credential을 검증하는 과정에서 에러메시지를 조금 더 자세하게 출력하도록 변경

- 올바른 Basic 유형의 헤더(파싱 불가능한 경우) InvalidBasicCredentialException 예외 던지도록 변경

…속 받도록 수정

greeng00se · 2023-05-05T14:11:21Z

웨지신님 안녕하세요. 🙇
정성이 가득한 리뷰를 달아주셔서 제가 성장할 수 있었습니다.
email에 인덱스를 걸고 Dao 재조회를 하는건 생각을 못했습니다. 👍 👍 👍

코멘트를 확인하면서 추가적으로 궁금한 부분을 조금씩 남겨보았습니다!

아 그리고 좋아하시는 페페로니 피자 🍕 브랜드를 남겨주시면 반영하도록 하겠습니다!
웨지도 연휴 잘 보내시길 바랍니다 😄

sihyung92

안녕하시옵니까 허브신님, 리뷰어 웨지이옵니다 🙏
리뷰 반영 잘 해주신 부분 확인하였고 잭슨 피자도 추가된 걸 보고 싶었지만 요구사항이 만족된 것 같아 이만 머지하겠사옵니다.
앞으로도 건승하시고 많은 신도 거느리시길 기원하겠나이다 허멘 🙏

sihyung92 · 2023-05-06T12:19:16Z

src/test/java/cart/auth/BasicAuthorizationParserTest.java

+            "Basic, pizza@pizza.com.password"
+    })
+    @ParameterizedTest(name = "올바른 Basic 인증 유형이 아니라면 InvalidBasicCredential 예외를_던진다. 입력: {0} {1}")
+    void 올바른_Basic_인증_유형이_아니라면_InvalidBasicCredential_예외를_던진다(final String startWith, final String credential) {


Auth에 대한 테스트 좋아요~~ 👍

greeng00se added 17 commits May 2, 2023 22:38

docs: 2단계 요구사항 정리

2256b19

test: 잘못된 테스트명 수정

27e9089

feat: data.sql 수정 및 schema.sql 파일 추가

02be345

refactor: auditing sql의 default를 사용하도록 변경

1a0bd1b

refactor: 사용자 엔티티 추가

33355cf

feat: 사용자 설정 페이지 연동 기능 추가

bf96972

feat: 장바구니 페이지 연동 기능 추가

64925cc

feat: Basic Auth로 오는 값을 파싱하는 클래스 추가

2cb430b

feat: 사용자 정보에 대한 ArgumentResolver 추가

b5cf8e6

test: 잘못 작성된 테스트 수정

5d075c1

feat: 장바구니 등록 및 조회 기능 추가

2d16d45

feat: 장바구니 삭제 기능 추가

f13465b

refactor: Cart -> CartProduct로 수정

6e8d373

refactor: create URI 수정

6040e7e

feat: 인증을 인터셉터가 담당하도록 설정

bbf1884

docs: 요구사항 업데이트

3d81cf3

feat: argumentResolver에서 threadLocal clear하는 기능 추가

eebb50b

sihyung92 requested changes May 5, 2023

View reviewed changes

greeng00se added 12 commits May 5, 2023 21:29

feat: Credential에서 비밀번호가 다른지 확인하는 기능 추가

b273000

refactor: 기존에 인증과정에서 Member를 반환하던 Dao를 Credential을 반환하도록 수정

cbb8bc6

- AuthMemberDao -> CredentialDao - 추가로 Credential을 검증하는 과정에서 에러메시지를 조금 더 자세하게 출력하도록 변경

feat: ExceptionHandler에 로깅 적용

3d3799c

refactor: Auditing을 위한 필드 전부 제거

705361a

refactor: BasicAuthorizationParser의 isNotValid를 제거하고 조금 더 응집력 있는 클래스로 변경

ac83592

- 올바른 Basic 유형의 헤더(파싱 불가능한 경우) InvalidBasicCredentialException 예외 던지도록 변경

refactor: Credential의 id를 memberId로 변경

eb44589

feat: getMemberId 메서드에 Nullable 애너테이션 추가

1ac95f0

refactor: 패키지 구조 변경

7b8de76

feat: 도미노 페페로니 피자 추가

aa41153

remove: todo 주석 제거

43eece1

refactor: InvalidBasicCredentialException이 AuthenticationException을 상…

fb42fa4

…속 받도록 수정

test: BaiscAuthorizationParser 테스트 출력 수정

dd5dee5

sihyung92 approved these changes May 6, 2023

View reviewed changes

sihyung92 merged commit cbc8152 into woowacourse:greeng00se May 6, 2023

greeng00se changed the title ~~[2단계 - [장바구니 기능] 허브(방대의) 미션 제출합니다.~~ [2단계 - 장바구니 기능] 허브(방대의) 미션 제출합니다. May 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 장바구니 기능] 허브(방대의) 미션 제출합니다. #300

[2단계 - 장바구니 기능] 허브(방대의) 미션 제출합니다. #300

greeng00se commented May 4, 2023

sihyung92 left a comment

sihyung92 May 5, 2023

greeng00se May 5, 2023

sihyung92 May 5, 2023

greeng00se May 5, 2023

sihyung92 May 5, 2023

greeng00se May 5, 2023

sihyung92 May 5, 2023

greeng00se May 5, 2023

greeng00se May 5, 2023

sihyung92 May 5, 2023

greeng00se May 5, 2023

sihyung92 May 5, 2023

greeng00se May 5, 2023

sihyung92 May 6, 2023

sihyung92 May 5, 2023

sihyung92 May 5, 2023

greeng00se May 5, 2023

sihyung92 May 5, 2023

greeng00se May 5, 2023

sihyung92 May 6, 2023

sihyung92 May 5, 2023

greeng00se May 5, 2023

greeng00se commented May 5, 2023

sihyung92 left a comment

sihyung92 May 6, 2023

		private final LocalDateTime createdAt;
		private final LocalDateTime updatedAt;

		@@ -8,9 +8,13 @@ const addCartItem = (productId) => {

		// TODO: [2단계] 장바구니 CRUD API에 맞게 변경

[2단계 - 장바구니 기능] 허브(방대의) 미션 제출합니다. #300

[2단계 - 장바구니 기능] 허브(방대의) 미션 제출합니다. #300

Conversation

greeng00se commented May 4, 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

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 May 5, 2023

sihyung92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment