Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hoony-lab wants to merge 7 commits into
base: hoony-lab
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e62bf5e
[#44] feat: add UserUpdateRequest
hoony-lab Sep 5, 2021
de002a3
[#44] feat: add updateUser in User
hoony-lab Sep 5, 2021
29f820e
[#44] feat: add update in UserService
hoony-lab Sep 5, 2021
ab73728
[#44] feat: add updateUser in UserController
hoony-lab Sep 5, 2021
02f7f73
[#44] test: add update, update-exception in UserServiceTest
hoony-lab Sep 5, 2021
6bdaa5d
[#44] test: add PUT_api_user_updateUser in UserControllerTest
hoony-lab Sep 5, 2021
202b634
[#44] fix
hoony-lab Sep 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/main/java/com/study/realworld/config/auth/JwtAuthenticationFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import lombok.extern.slf4j.Slf4j;
import org.springframework.http.HttpHeaders;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.stereotype.Component;
import org.springframework.web.filter.OncePerRequestFilter;
Expand All @@ -27,15 +28,15 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter {
@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException {
log.info(DOFILTERMESSAGE, request.getMethod(), request.getRequestURI(), request.getHeader(HttpHeaders.AUTHORIZATION));
String header = request.getHeader(HttpHeaders.AUTHORIZATION);
String tokenHeader = request.getHeader(HttpHeaders.AUTHORIZATION);

if(header != null) {
String tokenPrefix = header.split(" ")[0];
String accessToken = header.split(" ")[1];
if(tokenHeader != null && tokenHeader.split(" ").length == 2) {
String tokenPrefix = tokenHeader.split(" ")[0];
String accessToken = tokenHeader.split(" ")[1];

if("Token".equals(tokenPrefix)) {
String email = jwtProvider.getSubjectFromToken(accessToken);
final UsernamePasswordAuthenticationToken authentication = new UsernamePasswordAuthenticationToken(email, accessToken, Collections.emptyList());
final Authentication authentication = new UsernamePasswordAuthenticationToken(email, accessToken, Collections.emptyList());

SecurityContextHolder.getContext().setAuthentication(authentication);
}
Expand Down
15 changes: 7 additions & 8 deletions src/main/java/com/study/realworld/user/domain/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,13 @@ public User(String email, String username, String password, String bio, String i
this.image = image;
}

public static User of(User user) {
return User.builder()
.email(user.getEmail())
.username(user.getUsername())
.password(user.getPassword())
.bio(user.getBio())
.image(user.getImage())
.build();
public User updateUser(User user) {
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();
Comment on lines +48 to +52

@jinyoungchoi95 jinyoungchoi95 Sep 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kwj1270 kwj1270 Sep 12, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hoony-lab hoony-lab Sep 13, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@chance0523 chance0523 Sep 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return this;
}

public void encodePassword(PasswordEncoder passwordEncoder) {
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/study/realworld/user/domain/UserRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@

public interface UserRepository extends JpaRepository<User, Long> {
Optional<User> findByEmail(String email);
boolean existsByEmail(String email);

}
44 changes: 44 additions & 0 deletions src/main/java/com/study/realworld/user/dto/UserUpdateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package com.study.realworld.user.dto;


import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeName;
import com.study.realworld.user.domain.User;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;

import static com.fasterxml.jackson.annotation.JsonTypeInfo.As.WRAPPER_OBJECT;
import static com.fasterxml.jackson.annotation.JsonTypeInfo.Id.NAME;

@JsonTypeName("user")
@JsonTypeInfo(include = WRAPPER_OBJECT, use = NAME)
@Getter
@NoArgsConstructor
public class UserUpdateRequest {
private String email;
private String username;
private String password;
private String bio;
private String image;

@Builder
public UserUpdateRequest(String email, String username, String password, String bio, String image) {
this.email = email;
this.username = username;
this.password = password;
this.bio = bio;
this.image = image;
}

public static User toUser(UserUpdateRequest request) {
return User.builder()
.email(request.getEmail())
.username(request.getUsername())
.password(request.getPassword())
.bio(request.getBio())
.image(request.getImage())
.build();
}

}
25 changes: 22 additions & 3 deletions src/main/java/com/study/realworld/user/service/UserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,18 @@
import com.study.realworld.user.domain.UserRepository;
import com.study.realworld.user.dto.UserJoinRequest;
import com.study.realworld.user.dto.UserLoginRequest;
import com.study.realworld.user.dto.UserUpdateRequest;
import lombok.RequiredArgsConstructor;
import org.springframework.dao.DuplicateKeyException;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Propagation;
import org.springframework.transaction.annotation.Transactional;

import java.util.List;
import java.util.NoSuchElementException;

import static java.util.Objects.nonNull;

@RequiredArgsConstructor
@Transactional(readOnly = true)
@Service
Expand All @@ -36,7 +38,7 @@ public User findByEmail(String email) {
.orElseThrow(() -> new NoSuchElementException(email + " not found"));
}

@Transactional(propagation = Propagation.NEVER)
@Transactional
public User save(UserJoinRequest request) {
User user = UserJoinRequest.toUser(request);
user.encodePassword(passwordEncoder);
Expand All @@ -45,7 +47,7 @@ public User save(UserJoinRequest request) {

return userRepository.save(user);
}

public User login(UserLoginRequest request) {
User user = this.findByEmail(request.getEmail());

Expand All @@ -54,6 +56,17 @@ public User login(UserLoginRequest request) {
return user;
}

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

@jinyoungchoi95 jinyoungchoi95 Sep 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validation 👍


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

@jinyoungchoi95 jinyoungchoi95 Sep 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

@hoony-lab hoony-lab Sep 13, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jinyoungchoi95 jinyoungchoi95 Sep 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

완전히 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);

@jinyoungchoi95 jinyoungchoi95 Sep 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


User user = this.findByEmail(principal);
return user.updateUser(requestUser);
}

private void validateDuplicateUser(String email) {
if(userRepository.findByEmail(email).isPresent()) {
throw new DuplicateKeyException(email + " duplicated email");
Expand All @@ -66,4 +79,10 @@ private void validateMatchesPassword(User user, String rawPassword) {
}
}

private void validateExistUser(String email) {
if(userRepository.existsByEmail(email)) {
throw new DuplicateKeyException(email);
}
}
Comment on lines +82 to +86

@jinyoungchoi95 jinyoungchoi95 Sep 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@chance0523 chance0523 Sep 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

흠 뭐가 정답이라고 하기는 뭐하지만
저라면 따로 메소드를 만들지 않고
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을 써도 될 것 같긴하겠네요


}
25 changes: 15 additions & 10 deletions src/main/java/com/study/realworld/user/web/UserController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,14 @@
import com.study.realworld.user.dto.UserJoinRequest;
import com.study.realworld.user.dto.UserLoginRequest;
import com.study.realworld.user.dto.UserResponse;
import com.study.realworld.user.dto.UserUpdateRequest;
import com.study.realworld.user.service.UserService;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.http.HttpHeaders;
import org.springframework.http.ResponseEntity;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.web.bind.annotation.*;

import javax.servlet.http.HttpServletRequest;

@Slf4j
@RequiredArgsConstructor
@RequestMapping("/api")
Expand All @@ -40,15 +37,23 @@ public ResponseEntity<UserResponse> loginUser(@RequestBody UserLoginRequest requ
String token = jwtProvider.generateJwtToken(user);

return ResponseEntity.ok()
.header(HttpHeaders.AUTHORIZATION, "Token " + token)
.body(UserResponse.of(user, token));
}

@GetMapping("/user")
public ResponseEntity<UserResponse> getUser(HttpServletRequest servletRequest) {
String email = (String) SecurityContextHolder.getContext().getAuthentication().getPrincipal();
User user = userService.findByEmail(email);
String token = servletRequest.getHeader(HttpHeaders.AUTHORIZATION).split(" ")[1];
public ResponseEntity<UserResponse> getUser(@AuthenticationPrincipal String principal) {
User user = userService.findByEmail(principal);
String token = jwtProvider.generateJwtToken(user);

return ResponseEntity.ok()
.body(UserResponse.of(user, token));
}

@PutMapping("/user")
public ResponseEntity<UserResponse> updateUser(@AuthenticationPrincipal String principal,
@RequestBody UserUpdateRequest request) {
User user = userService.update(request, principal);
String token = jwtProvider.generateJwtToken(user);

return ResponseEntity.ok()
.body(UserResponse.of(user, token));
Expand Down
13 changes: 13 additions & 0 deletions src/test/java/com/study/realworld/config/auth/JwtProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
package com.study.realworld.config.auth;

import com.study.realworld.user.domain.User;
import io.jsonwebtoken.JwtException;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertThrows;

@ExtendWith(MockitoExtension.class)
class JwtProviderTest {

private static final Long ID = 1L;
private static final String EMAIL = "email";
private static final String TOKEN = "token";
private static final String WRONG_TOKEN = "wrong-token";
private static final String USERNAME = "username";
private static final String PASSWORD = "password";
private static final String BIO = "bio";
Expand Down Expand Up @@ -52,4 +55,14 @@ void JwtProviderTest() {
);
}

@Test
void getClaimsFromToken_exception() {
// given

// when

// then
assertThrows(JwtException.class, () -> jwtProvider.getClaimsFromToken(WRONG_TOKEN));
}
Comment on lines +59 to +66

@kwj1270 kwj1270 Sep 12, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


}
Loading