[kwj1270-issue51] Exception 관련 리팩토링 #53

kwj1270 · 2021-09-19T17:34:13Z

Issue: #51

작업 내용

Exception 관련되어서 리팩토링 했습니다.
RestControllerAdvice 적용

생성/변경 로직

RestControllerAdvice 생성
커스텀 Exception 생성
익셉션 필터 및 관련 코드들 적용

개인 코멘트

ExceptionTranslationFilter 적용하려 했으나 Interceptor 단계에서 동작하는 필터로 동작을 안하네요
RestDocs / log4j2 적용은 내일 올릴 다음 pr에 하겠습니다.

… 처리하도록 리팩토링

codecov-commenter · 2021-09-19T17:36:04Z

Codecov Report

Merging #53 (05d538f) into kwj1270 (c86ab55) will increase coverage by 1.30%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             kwj1270      #53      +/-   ##
=============================================
+ Coverage      96.66%   97.96%   +1.30%     
- Complexity       120      188      +68     
=============================================
  Files             30       52      +22     
  Lines            300      492     +192     
  Branches           5        5              
=============================================
+ Hits             290      482     +192     
  Partials          10       10

Impacted Files	Coverage Δ
...m/study/realworld/domain/user/domain/Password.java	`100.00% <ø> (ø)`
...udy/realworld/domain/user/dto/UserJoinRequest.java	`100.00% <ø> (ø)`
.../realworld/global/jpa/config/JpaConfiguration.java	`100.00% <ø> (ø)`
.../exception/JwtProviderNotSupportTypeException.java	`100.00% <ø> (ø)`
...orld/domain/auth/application/AuthLoginService.java	`100.00% <100.00%> (ø)`
...orld/domain/auth/infrastructure/TokenProvider.java	`100.00% <100.00%> (ø)`
...dy/realworld/domain/auth/presentation/AuthApi.java	`100.00% <100.00%> (ø)`
...world/domain/user/application/UserFindService.java	`100.00% <100.00%> (ø)`
...world/domain/user/application/UserJoinService.java	`100.00% <100.00%> (ø)`
...rld/domain/user/application/UserUpdateService.java	`100.00% <100.00%> (ø)`
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c86ab55...05d538f. Read the comment docs.

jinyoungchoi95

상당히 많은 file change 😢 최대한 다 읽어보았습니다!

항상 우지님의 코드는 꼼꼼해서 보기 좋은것같아요 ㅎㅎ
개인적으로 코드 읽으면서 나라면 어떨까 싶은 부분에 대해서 코멘트 남겨두었으니 의견으로만 봐주세요 ㅎㅎ :)

jinyoungchoi95 · 2021-09-19T18:19:06Z

src/main/java/com/study/realworld/domain/auth/application/AuthLoginService.java

@@ -28,7 +29,7 @@ public User login(final Email email, final Password password) {

    private User findByEmail(final Email email) {
        return userRepository.findByEmail(email)
-                .orElseThrow(IllegalArgumentException::new);
+                .orElseThrow(() -> new EmailNotFoundException(email.email()));


람다식으로 다시 변경하게 된 이유가 있을까용?

커스텀 Exception 만들면서 매개변수로 email String 값을 받도록 해서 불가피하게 바꾸게 되었어요!
내부 String만 사용하면 메서드 참조 가능하지만, 매개변수 때문에 추가했습니다.

jinyoungchoi95 · 2021-09-19T18:23:01Z

src/main/java/com/study/realworld/domain/auth/presentation/AuthApi.java

@@ -14,13 +14,13 @@
 import javax.validation.Valid;

 @RestController
-public class AuthRestController {
+public class AuthApi {


물론 취향차이일 수도 있고, 해당 Api 클래스에서만 쓰기는 하는데 url을 분리하는건 어떻게 생각하세요?

제 생각에는 /api/users/login에서 api는 오히려 클래스단에 분리해서 매핑시켜두는것이 더 어울리지 않을까해요

ㅎㅎ 사실 이거 다른 분들 의견이 없으셔서 그냥 놔두었는데
Controller 단에서 공통 url 묶어주는게 좋을 것 같네요!

아 그리고 싸피하면서 ContextRoot 설정하는 방법도 있는데
api를 명시적으로 입력할지 ContextRoot로 api 넣을지 같이 고민해보면 좋을 것 같습니다,

따로 api문서나 주석으로 처리하지 않는이상 명시적인게 더 나을 수도 있을 것 같아요 :)

jinyoungchoi95 · 2021-09-19T18:26:25Z

src/main/java/com/study/realworld/domain/user/application/UserJoinService.java

+        final User user = requestUser.encode(passwordEncoder);
+        validateDuplicatedEmail(user.email());


Suggested change

final User user = requestUser.encode(passwordEncoder);

validateDuplicatedEmail(user.email());

validateDuplicatedEmail(user.email());

final User user = requestUser.encode(passwordEncoder);

제 생각이긴 한데 encoding하는 작업보다 email checking이 우선되어야 하지 않을까요?
email이 중복되었다면 굳이 encoding작업은 진행되지 않을 수도 있어 보여요

기존에 저장된 값이 인코딩되이 저장되어 있다고 생각해서 위와 같이 작성했습니다.
로직에 문제가 있는지 한번 봐야겠군요

jinyoungchoi95 · 2021-09-19T18:28:45Z

src/main/java/com/study/realworld/domain/user/domain/User.java

+
+    private void validateArgumentNull(final Object argument) {
+        if(isNull(argument)) {
+            throw new IllegalArgumentException();


연관된 에러 메시지를 담아 전달하는게 좋아보이네요 :)
혹은 custom하셨으니 Null에 대한 input들 또한 custom해서 묶어볼수도있을것 같아요

여기에는 CustomException을 안 넣었나 보네요
새로 추가하겠습니다. 👍

jinyoungchoi95 · 2021-09-19T18:29:21Z

src/main/java/com/study/realworld/domain/user/error/UserErrorCode.java

+    IDENTITY_NOT_FOUND(IdentityNotFoundException.class, HttpStatus.BAD_REQUEST, "Identity is not found"),
+    PASSWORD_MISS_MATCH(PasswordMissMatchException.class, HttpStatus.BAD_REQUEST, "Password is miss match");
+
+    private final Class exceptionClass;


exception class까지 담는것 좋은거같아요 👍

jinyoungchoi95 · 2021-09-19T18:29:57Z

src/main/java/com/study/realworld/domain/user/error/UserErrorCode.java

+    public static UserErrorCode values(final UserBusinessException exception) {
+        final Class<? extends UserBusinessException> exceptionClass = exception.getClass();
+        return Arrays.stream(values())
+                .filter(userErrorCode -> userErrorCode.exceptionClass.equals(exceptionClass))
+                .findFirst()
+                .orElseThrow(IllegalArgumentException::new);
+    }


어떤 역할을 하는지 한번만 설명해주실수있을까용?

Exception 만 넣으면 알아서 enum 상수 중 하나를 뱉어줍니다.
앞서 Exception 넣은 것을 이용해서 특정 Exception 값을 넣으면 알맞는 ErrorCode 상수를 반환해줘요!

jinyoungchoi95 · 2021-09-19T18:31:43Z

src/main/java/com/study/realworld/domain/user/presentation/UserApi.java

+                                               @AuthenticationPrincipal final Long principal) {
+        final User user = userUpdateService.update(userUpdateRequest.toEntity(), principal);
+        final ResponseToken responseToken = tokenProvider.createToken(user);
+        final UserResponse userResponse = UserResponse.fromUserWithToken(user, responseToken);


Suggested change

final UserResponse userResponse = UserResponse.fromUserWithToken(user, responseToken);

final UserResponse userResponse = UserResponse.fromUserAndToken(user, responseToken);

개인적으로는 문맥상 And 가 더 나아보일듯 하네요

오잉 왜 With 이 들어갔는지 제가 봐도 이해 안가네요

jinyoungchoi95 · 2021-09-19T18:33:20Z

src/main/java/com/study/realworld/global/jwt/error/JwtErrorCode.java

+    public static JwtErrorCode values(final AuthenticationException exception) {
+        final Class<? extends AuthenticationException> exceptionClass = exception.getClass();
+        return Arrays.stream(values())
+                .filter(userErrorCode -> userErrorCode.exceptionClass.equals(exceptionClass))
+                .findFirst()
+                .orElseThrow(IllegalArgumentException::new);
+    }
+
+    @Override
+    public HttpStatus httpStatus() {
+        return httpStatus;
+    }
+
+    @Override
+    public String message() {
+        return ERROR_CODE_MESSAGE;
+    }


오히려 모든 Error code가 재정의를 해야하는 상황이 오게 되었는데 조금더 좋은 방법은 없을까요?
관리의 편의성을 위한 부분이 뭔가 관리포인트의 증가로 온듯한?느낌이 들어서요

가능하다면 Abstract Class 로 만들고 싶지만
Enum 클래스는 기본적으로 SomeEnum extends Enum 구조여서 클래스를 상속 못받더라고요
그래서 interface로 확장시켰고 이 부분은 해결하기 어렵더라고요

jinyoungchoi95 · 2021-09-19T18:34:32Z

src/main/java/com/study/realworld/global/security/error/SecurityErrorCode.java

+
+import java.util.Arrays;
+
+public enum SecurityErrorCode implements ErrorCode {


이 부분과 jwt의 error code의 분리는 좋은것 같기는 한데 둘을 합쳐도 되지않을까요? (그냥 제 생각입니당)

JwtErrorCode 는 Jwt관련 Exception이 나오도록 했고
SecurityErrorCode는 UserDetails 와 같은 Security에서 general한 요소들로 분리했습니다.
시큐리티 적용을 기준으로 jwt -> session으로 변경도 가능하다고 판단해서 분리시켰습니다!

jinyoungchoi95 · 2021-09-19T18:35:03Z

src/main/java/com/study/realworld/global/security/error/SecurityErrorResponse.java

+
+@JsonTypeName("errors")
+@JsonTypeInfo(include = JsonTypeInfo.As.WRAPPER_OBJECT, use = JsonTypeInfo.Id.NAME)
+public class SecurityErrorResponse {


ErrorResponse를 상속받는 구조는 어떨까요?

ErrorCode와 같이 분리할까 생각하다 실현은 못했네요!!
사실 다른 ErrorResponse들도 구조가 아에 똑같아서 좋은 아이디어 같아요

kwj1270 · 2021-09-20T02:56:32Z

체인지 전부 보셨다니;;
이게 기존 PR도 열려있고 어떻게 PR보내려고 우회했더니 그렇게 되었나보네요 ㅠㅡㅠ 감사합니다. 오리님
오리님 코드도 세밀하게 신경쓰시는게 느껴져서 저도 덩달아 영향 받은 것 같네요.
저희 끝까지 마무리 열심히 해보도록 합시다.

hoony-lab

Exception 관리를 굉장히 철저히 관리하셨네요 ! 👍

hoony-lab · 2021-09-29T02:22:31Z

build.gradle

@@ -1,6 +1,7 @@
 plugins {
    id 'org.springframework.boot' version '2.5.3'
    id 'io.spring.dependency-management' version '1.0.11.RELEASE'
+    id "org.asciidoctor.convert" version "1.5.9.2"


아스키 의사선생님 ... !?

ㅋㅋㅋㅋㅋㅋㅋㅋ restdocs 추가하면서 추가했습니다 👍

hoony-lab · 2021-09-29T02:30:53Z

src/test/java/com/study/realworld/global/security/error/SecurityErrorCodeTest.java

+        assertAll(
+                () -> assertThat(SecurityErrorCode.values(new UserDetailsNullPointerException())).isNotNull()
+        );


테스트할게 하나인데 assertAll로 묶은건 통일성 있게 할려고 한건가요 ?

네네 그리고 시큐리티 에러는 도중에 추가될 가능성 염두해서 미리 처리했습니다.

kwj1270 added 30 commits August 28, 2021 10:30

[#40] test: UserUpdateService 생성자 테스트 작성

d62552c

[#40] feat: UserUpdateService 생성자 구현

811b199

[#40] test: UserUpdateRequest 기본 생성자 테스트 작성

2ef3105

[#40] test: UserUpdateRequest 생성자 테스트 작성

011be9c

[#40] feat: UserUpdateRequest 생성자 구현

827a1be

[#40] test: UserUpdateRequest 인스턴스 toEntity() 테스트 작성

5b08550

[#40] feat: UserUpdateRequest 인스턴스 toEntity() 구현

4f4b274

[#40] test: UserUpdateService 인스턴스 update() 테스트 작성

b83cc76

[#40] test: User 인스턴스 change() 테스트 작성

fb30375

[#40] feat: User 인스턴스 change() 기능 구현

c9a848f

[#40] test: UserUpdateService 인스턴스 update() 테스트 작성

e49332c

[#40] feat: UserUpdateService 인스턴스 update() 테스트 작성

d9d2d4a

[#40] test: UserRestController 인스턴스의 update() 단위 테스트 작성

81f98ad

[#40] feat: UserRestController 인스턴스의 update() 단위 테스트 구현

c6397ac

[#40] test: UserUpdateService 인스턴스 update() 실패 테스트 작성

c07d0f1

[#40] refactor: Authentication Principal Email 및 Password 타입으로 리팩토링

a75a644

[#40] refactor: userUpdateService.update() AlreadyExistEmailException…

10ff2ec

… 처리하도록 리팩토링

[#40] refactor: EmailNotFoundException 으로 예외처리 리팩토링

7db9b1b

refactor: 피드백 반영

ca7b696

feat: ErrorCode 인터페이스 생성

9433950

test: UserErrorCode 열거형 생성 테스트

e76b7c0

teat: UserErrorCode 열거형 httpStatus 테스트 작성

6f14510

test: UserErrorResponse 인스턴스 기본 생성자 테스트 작성

41407c5

test: UserErrorResponse 인스턴스 생성자 테스트 작성

868320b

feat: UserErrorResponse 인스턴스 생성자 구현

2f2bca0

test: UserErrorCode values() 테스트 작성

15b1316

refactor: UserService 이메일 중복 검증 로직 추가

fcf6b07

test: UserRestController 의 join() 호출시 이메일 중복으로 인한 실패 테스트 작성

d54af98

test: UserController에 대한 Advice 적용 테스트 작성

33911b9

test: TokenProvider 인스턴스 mapToUsername() 잘못된 토큰값으로 인한 실패 테스트 작성

0445ab4

kwj1270 added 8 commits September 20, 2021 00:41

test: JwtExceptionHandlerFilter 인스턴스 doFilter() 예외처리 테스트 작성

692467c

test: JwtAuthenticationEntryPoint 생성 테스트 작성

9beb631

test: JwtErrorResponse 인스턴스 기본 생성자 테스트 작성

1bbdc9f

test: SecurityExceptionHandlerFilter 인스턴스 생성자 테스트 작성

d77e900

test: SecurityExceptionHandlerFilter 인스턴스 doFilter() 예외처리 테스트 작성

1d4b882

test: SecurityExceptionHandlerFilter 인스턴스 doFilter() 테스트 작성

bbb7688

test: SecurityErrorResponse 인스턴스 생성자 테스트 작성

0de3e9f

style: 코드 컨벤션 적용

3920906

kwj1270 added the woozi 우지의 이슈 label Sep 19, 2021

kwj1270 self-assigned this Sep 19, 2021

jinyoungchoi95 approved these changes Sep 19, 2021

View reviewed changes

kwj1270 changed the title ~~[kwj1270-issue50] Exception 관련 리팩토링~~ [kwj1270-issue51] Exception 관련 리팩토링 Sep 20, 2021

kwj1270 added 2 commits September 20, 2021 20:50

feat: RestDocs 추가

05d538f

fix: EntryPointer가 기존 Exception 처리 못하는거 해결

a968655

hoony-lab reviewed Sep 29, 2021

View reviewed changes

kwj1270 merged commit 0b1eef6 into kwj1270 Oct 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kwj1270-issue51] Exception 관련 리팩토링 #53

[kwj1270-issue51] Exception 관련 리팩토링 #53

kwj1270 commented Sep 19, 2021

codecov-commenter commented Sep 19, 2021 •

edited

jinyoungchoi95 left a comment

jinyoungchoi95 Sep 19, 2021

kwj1270 Sep 20, 2021

jinyoungchoi95 Sep 19, 2021

kwj1270 Sep 20, 2021

kwj1270 Sep 20, 2021

jinyoungchoi95 Sep 27, 2021

jinyoungchoi95 Sep 19, 2021

kwj1270 Sep 20, 2021

jinyoungchoi95 Sep 19, 2021

kwj1270 Sep 20, 2021

jinyoungchoi95 Sep 19, 2021

jinyoungchoi95 Sep 19, 2021

kwj1270 Sep 20, 2021

jinyoungchoi95 Sep 19, 2021

kwj1270 Sep 20, 2021

jinyoungchoi95 Sep 19, 2021

kwj1270 Sep 20, 2021

jinyoungchoi95 Sep 19, 2021

kwj1270 Sep 20, 2021

jinyoungchoi95 Sep 19, 2021

kwj1270 Sep 20, 2021

kwj1270 commented Sep 20, 2021

hoony-lab left a comment

hoony-lab Sep 29, 2021

kwj1270 Oct 2, 2021

hoony-lab Sep 29, 2021

kwj1270 Oct 2, 2021

		final User user = requestUser.encode(passwordEncoder);
		validateDuplicatedEmail(user.email());

	final UserResponse userResponse = UserResponse.fromUserWithToken(user, responseToken);
	final UserResponse userResponse = UserResponse.fromUserAndToken(user, responseToken);


		import java.util.Arrays;

		public enum SecurityErrorCode implements ErrorCode {

[kwj1270-issue51] Exception 관련 리팩토링 #53

[kwj1270-issue51] Exception 관련 리팩토링 #53

Conversation

kwj1270 commented Sep 19, 2021

Issue: #51

작업 내용

생성/변경 로직

개인 코멘트

codecov-commenter commented Sep 19, 2021 • edited

Codecov Report

jinyoungchoi95 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

kwj1270 commented Sep 20, 2021

hoony-lab 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

codecov-commenter commented Sep 19, 2021 •

edited