[hoony-lab-issue44] 사용자 업데이트 #45

hoony-lab · 2021-09-05T06:26:44Z

Issue: #44

작업 내용

API PUT /api/user

생성/변경 로직

PUT /api/user
SecurityContextHolder >> @AuthenticationPrincipal

개인 코멘트

뭔가 쓸대없고? 잦은? 코드수정을 하고싶은데 해도 되는지에 대한 의문이 든다
다음 PR 들어가기전 확실히 마무리 해야겠다

check all params and update fields which is nonNul

IDK

use `@AuthenticationPrincipal` String principal to retrieve current login user info

update update_validateExistUser_exception some refactoring

`SecurityContextHolder.getContext().setAuthentication(new Authentication());` some refactoring

umm

codecov-commenter · 2021-09-05T06:28:06Z

Codecov Report

Merging #45 (202b634) into hoony-lab (66cb514) will increase coverage by 2.55%.
The diff coverage is 84.78%.

@@               Coverage Diff               @@
##             hoony-lab      #45      +/-   ##
===============================================
+ Coverage        92.57%   95.12%   +2.55%     
- Complexity          67       81      +14     
===============================================
  Files               12       13       +1     
  Lines              175      205      +30     
  Branches             3        8       +5     
===============================================
+ Hits               162      195      +33     
+ Misses               9        0       -9     
- Partials             4       10       +6

Impacted Files	Coverage Δ
...ain/java/com/study/realworld/user/domain/User.java	`75.86% <16.66%> (+5.86%)`	⬆️
...realworld/config/auth/JwtAuthenticationFilter.java	`84.61% <80.00%> (ø)`
.../com/study/realworld/user/service/UserService.java	`96.29% <87.50%> (-3.71%)`	⬇️
...om/study/realworld/user/dto/UserUpdateRequest.java	`100.00% <100.00%> (ø)`
...a/com/study/realworld/user/web/UserController.java	`100.00% <100.00%> (ø)`
...a/com/study/realworld/config/auth/JwtProvider.java	`100.00% <0.00%> (+7.69%)`	⬆️

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 66cb514...202b634. Read the comment docs.

jinyoungchoi95

점점 작업 속도가 빨라지시는 것 같아 좋은 것 같습니다 👍

잦은 코드 수정같은 경우에는 refactoring 후 pr 주셔도 좋을 것 같아요.
file changed가 몇개 없어도 괜찮다고 생각하는 편이에요.

관련해서 몇가지 코멘트 드렸으니 확인 부탁드려용 :)

jinyoungchoi95 · 2021-09-09T02:11:41Z

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

+        if(Objects.nonNull(user.getEmail())) this.email = user.getEmail();
+        if(Objects.nonNull(user.getUsername())) this.username = user.getUsername();
+        if(Objects.nonNull(user.getPassword())) this.password = user.getPassword();
+        if(Objects.nonNull(user.getBio())) this.bio = user.getBio();
+        if(Objects.nonNull(user.getImage())) this.image = user.getImage();


각각 5개의 메소드로 분리할 수 있지 않을까요?
하나의 updateUser 메소드가 담당하는 역할이 많아보여요 🤔

저도 동감합니다!. 각각의 메서드를 분리하고 호출하는 형태도 좋을 것 같아요

한방에 한방에는 용납하기 힘든가보네요 !
각각 분리해서 한번에 콜 해보도록 하겠습니다 ~

오우 이건 후에 저도 해야할 고민이겠네요

jinyoungchoi95 · 2021-09-09T02:12:46Z

src/main/java/com/study/realworld/user/service/UserService.java


+    @Transactional
+    public User update(UserUpdateRequest request, String principal) {
+        validateExistUser(request.getEmail());


validation 👍

jinyoungchoi95 · 2021-09-09T02:16:21Z

src/main/java/com/study/realworld/user/service/UserService.java

+        validateExistUser(request.getEmail());
+
+        User requestUser = UserUpdateRequest.toUser(request);
+        if(nonNull(requestUser.getPassword())) requestUser.encodePassword(passwordEncoder);


Suggested change

if(nonNull(requestUser.getPassword())) requestUser.encodePassword(passwordEncoder);

password를 service에서 encoding하셨는데 그냥 이 작업 자체를 user에서 할 수는 없을까요?
어찌보면 password를 encoding하는것도 user의 역할이고 updateUser하는 것도 user의 역할인데, 그렇게 되면 encoding 하는 작업 자체를 user에 할당해도 문제가 없을것 같아요.

그리고 그렇게 넘기게 된다면 nonNull checking을 updateUser에서 진행하니까 두개의 nonNull이라는 작업을 최소화할 수도 있구요 :)

이런 생각이 DDD의 개념(?) 이라고 볼 수 있을까요 ?
짜면서 생각은 들었었는데, 조금 더 신중히 고민해봐야겠습니다.

완전히 DDD와 직결적으로 동일한 이야기를 하기보다는 객체가 가져야할 역할을 그에 맞게 할당하는 이야기가 더 맞을 것 같아요.

물론 null체크를 service에서도 할 수 있지만 지금은 저희가 User객체를 새로 생성했고 User도메인의 로직을 잘 사용하고 있잖아요?
그럼 null체크하는 것또한 user가 자신의 필드 상태를 체크하는 비즈니스 로직이 될수 있지않을까요?
만약 domain에서 이걸 진행하게 된다면 다음과 같이 변경할 수 있을 것 같아요.

public void encodePassword(PasswordEncoder passwordEncoder) { if(nonNull(this.password) { this.password = passwordEncoder.encode(this.password); } }

그렇게 된다면 지금 Service는 입력된 값에 대한 null체크를 해야한다는 무거운 비즈니스 로직에 대한 책임을 지지 않고 User를 호출할 수 있게 되겠죠.

requestUser.encodePassword(passwordEncoder);

첫 리뷰를 제가 조금 잘못한 것 같은데 null checking에 대한 이야기를 하려고했던 거 였어요 🙇‍♂️

jinyoungchoi95 · 2021-09-09T02:17:53Z

src/main/java/com/study/realworld/user/service/UserService.java

+    private void validateExistUser(String email) {
+        if(userRepository.existsByEmail(email)) {
+            throw new DuplicateKeyException(email);
+        }
+    }


validateDuplicateUser와 이 둘의 기능차이가 없어보이네요

굳이 validateDuplicateUser메소드 대신 이 메소드를 정의한 이유가 있을까용?

흠 뭐가 정답이라고 하기는 뭐하지만
저라면 따로 메소드를 만들지 않고
save() 메소드와 update() 메소드에 직접 써줄 것 같아요

@Transactional public User save(UserJoinRequest request) { if (userRepository.findByEmail(email).isPresent()) { throw new DuplicateKeyException(email + " duplicated email"); } // do someting } @Transactional public User update(UserUpdateRequest request, String principal) { if (userRepository.existsByEmail(email)) { throw new DuplicateKeyException(email); } // do something }

뭐 이런식으로요...
둘 다 existsByEmail을 써도 될 것 같긴하겠네요

jinyoungchoi95 · 2021-09-09T02:21:04Z

src/test/java/com/study/realworld/user/service/UserServiceTest.java

                .thenReturn(List.of(user));

-        // when
+        // then


이전에 하신게 맞습니다 :)

given > 주어진 상황
when > 주어진 상황을 통해 발생된 결과
then > 결과에 대한 검증

준비 - 실행 - 검증으로 when mocking 절은 대부분 setup & given에 속하고 result는 when 에 속합니다.
이번에 다른 모든 테스트를 지금처럼 하셨는데 한번 다시 고려해보시면 좋을 것 같습니다 :)

when이라는 단어에 잠깐 홀렸었나보네요

롤백 ON

jinyoungchoi95 · 2021-09-09T02:22:12Z

src/test/java/com/study/realworld/user/service/UserServiceTest.java

+        // when
+        when(userRepository.findByEmail(anyString()))
+                .thenReturn(Optional.ofNullable(user));
+
+        // then
+        assertThrows(DuplicateKeyException.class, () -> userService.save(request));


여기도 이렇게하는 것 보다는 when mocking절을 given에 올리고,
when & then으로 묶어서 throw testing을 진행하는게 더 bdd적으로 가독성 있을것 같아요 :)

jinyoungchoi95 · 2021-09-09T02:25:27Z

src/test/java/com/study/realworld/user/service/UserServiceTest.java

+        UserUpdateRequest request = UserUpdateRequest.builder()
+                .email(EMAIL+EMAIL)
+                .password(PASSWORD+PASSWORD)
+                .bio(BIO+BIO)
+                .build();
+
        // when
+        when(userRepository.existsByEmail(anyString()))
+                .thenReturn(true);

        // then
-        assertThrows(NoSuchElementException.class, () -> userService.login(request));
+        assertThrows(DuplicateKeyException.class, () -> userService.update(request, EMAIL));


조금더 상세하게 모킹해보는건 어떨까요?
request에 들어온 Email인 EMAIL+EMAIL이 이미 존재한다고 생각하고 테스트하는 거라면

when(userRepository.existsByEmail(EMAIL+EMAIL).thenReturn(true);

를 고려할 수 있지 않을까요?
저도 항상 단위테스트를 좋아하고 그 과정에서 mocking을 진행하지만 mocking이 구체화가 가능하다면 최대한 구체화해서 처리하는게 좋다고 생각해요 🙆‍♂️

확실히 주어진 케이스를 구체화 해서 사용해야겠네요 !
아무거나 들어오는 건 아니니까...

jinyoungchoi95 · 2021-09-09T02:30:15Z

src/test/java/com/study/realworld/user/service/UserServiceTest.java

+        assertAll(
+                () -> assertEquals(request.getEmail(), result.getEmail()),
+                () -> assertEquals(user.getUsername(), result.getUsername()),
+                () -> assertEquals(request.getBio(), result.getBio()),
+                () -> assertEquals(user.getImage(), result.getImage())


passwordd도 update에 적용이 되는데 이 부분도 테스트해야하지 않을까요?
password encoder를 mocking하셨으니 stubbing한다면 이부분도 테스트할 수 있을 것 같네요 :)

when(passwordEncoder.encode(PASSWORD)).thenReturn("encode_password");

jinyoungchoi95 · 2021-09-09T02:31:09Z

src/test/java/com/study/realworld/user/web/UserControllerTest.java

+    @MockBean
+    private Authentication authentication;


이 부분을 mock bean으로 설정한 이유가 있을까요?

이전에 설정했었었었던 거네요 !
현재는 날려도 무방한 것 같습니다 👍

kwj1270

오리님 말씀처럼 코드가 눈에 띄게 좋아졌고 속도도 엄청 빨라지셨네요 👍
갓후니님. 저번 처럼 크게 드릴 부분은 없고 몇가지 의견만 드렸습니다!!

kwj1270 · 2021-09-12T11:01:51Z

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

+        if(Objects.nonNull(user.getEmail())) this.email = user.getEmail();
+        if(Objects.nonNull(user.getUsername())) this.username = user.getUsername();
+        if(Objects.nonNull(user.getPassword())) this.password = user.getPassword();
+        if(Objects.nonNull(user.getBio())) this.bio = user.getBio();
+        if(Objects.nonNull(user.getImage())) this.image = user.getImage();


저도 동감합니다!. 각각의 메서드를 분리하고 호출하는 형태도 좋을 것 같아요

kwj1270 · 2021-09-12T11:03:09Z

src/test/java/com/study/realworld/config/auth/JwtProviderTest.java

+    void getClaimsFromToken_exception() {
+        // given
+
+        // when
+
+        // then
+        assertThrows(JwtException.class, () -> jwtProvider.getClaimsFromToken(WRONG_TOKEN));
+    }


메서드가 비어있군요...! 😥

kwj1270 · 2021-09-12T11:05:04Z

src/test/java/com/study/realworld/user/web/UserControllerTest.java

+        verify(userService).update(any(), any());
+        verify(jwtProvider).generateJwtToken(any());


verify()에 times() 가 없는데 이러면 그냥 통과되지 않나요..?
아니면 최소 1번은 통과시키나요? 이 부분 검증이 필요할 것 같습니다!

times(1) 가 default 로 설정되어 있는 걸로 알고 있습니다 !

@CheckReturnValue public static <T> T verify(T mock) { return MOCKITO_CORE.verify(mock, times(1)); }

chance0523

저에겐 괜찮아보이네요 👍

chance0523 · 2021-09-28T14:08:41Z

src/main/java/com/study/realworld/user/service/UserService.java

+    private void validateExistUser(String email) {
+        if(userRepository.existsByEmail(email)) {
+            throw new DuplicateKeyException(email);
+        }
+    }


흠 뭐가 정답이라고 하기는 뭐하지만
저라면 따로 메소드를 만들지 않고
save() 메소드와 update() 메소드에 직접 써줄 것 같아요

@Transactional public User save(UserJoinRequest request) { if (userRepository.findByEmail(email).isPresent()) { throw new DuplicateKeyException(email + " duplicated email"); } // do someting } @Transactional public User update(UserUpdateRequest request, String principal) { if (userRepository.existsByEmail(email)) { throw new DuplicateKeyException(email); } // do something }

뭐 이런식으로요...
둘 다 existsByEmail을 써도 될 것 같긴하겠네요

chance0523 · 2021-09-28T14:09:38Z

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

+        if(Objects.nonNull(user.getEmail())) this.email = user.getEmail();
+        if(Objects.nonNull(user.getUsername())) this.username = user.getUsername();
+        if(Objects.nonNull(user.getPassword())) this.password = user.getPassword();
+        if(Objects.nonNull(user.getBio())) this.bio = user.getBio();
+        if(Objects.nonNull(user.getImage())) this.image = user.getImage();


오우 이건 후에 저도 해야할 고민이겠네요

hoony-lab added 7 commits September 5, 2021 15:14

[#44] feat: add UserUpdateRequest

e62bf5e

[#44] feat: add updateUser in User

de002a3

check all params and update fields which is nonNul

[#44] feat: add update in UserService

29f820e

IDK

[#44] feat: add updateUser in UserController

ab73728

use `@AuthenticationPrincipal` String principal to retrieve current login user info

[#44] test: add update, update-exception in UserServiceTest

02f7f73

update update_validateExistUser_exception some refactoring

[#44] test: add PUT_api_user_updateUser in UserControllerTest

6bdaa5d

`SecurityContextHolder.getContext().setAuthentication(new Authentication());` some refactoring

[#44] fix

202b634

umm

hoony-lab added the hoony-lab 후니의 이슈 label Sep 5, 2021

hoony-lab requested review from DolphaGo, chance0523, jinyoungchoi95, kwj1270 and povia September 5, 2021 06:26

hoony-lab self-assigned this Sep 5, 2021

jinyoungchoi95 reviewed Sep 9, 2021

View reviewed changes

kwj1270 reviewed Sep 12, 2021

View reviewed changes

chance0523 approved these changes Sep 28, 2021

View reviewed changes

		verify(userService).update(any(), any());
		verify(jwtProvider).generateJwtToken(any());

[hoony-lab-issue44] 사용자 업데이트 #45

Are you sure you want to change the base?

[hoony-lab-issue44] 사용자 업데이트 #45

Uh oh!

Conversation

hoony-lab commented Sep 5, 2021

Issue: #44

작업 내용

생성/변경 로직

개인 코멘트

Uh oh!

codecov-commenter commented Sep 5, 2021

Codecov Report

Uh oh!

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Uh oh!

jinyoungchoi95 Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinyoungchoi95 Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinyoungchoi95 Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwj1270 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoony-lab Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chance0523 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jinyoungchoi95 Sep 9, 2021 •

edited

Loading

jinyoungchoi95 Sep 9, 2021 •

edited

Loading

jinyoungchoi95 Sep 13, 2021 •

edited

Loading

hoony-lab Sep 13, 2021 •

edited

Loading