[hoony-lab-issue12] User 도메인 구현 #19

hoony-lab · 2021-08-07T11:41:23Z

Issue: #12

작업 내용

Project Setting
User : Entity, Repository, Service, Controller, DTO
Authentication : POST /api/users/login (No authentication)
- token 제외
Registration : POST /api/users (No authentication)
- token 제외
Test
- 악

생성/변경 로직

개인 코멘트

처음부터 만들기는 참 힘들다
JUnit5 의 hello world는 참 힘들다
~~What is TDD and DDD~~

user_builder user_equals_and_hashCode

findAll, findById, findByEmail, save, deleteById

save_and_findAll save_and_findByEmail save_and_findById save_and_deleteById save_and_deleteById_exception

`@JsonTypeName`, `@JsonTypeInfo` : Mapping Object static `toUser` : convert DTO to Entity(User) for communicate within layers

`@JsonTypeName`, `@JsonTypeInfo` : Mapping Object static `of`: convert Entity(User) to DTO for communicate within layers

`@JsonTypeName`, `@JsonTypeInfo` : Mapping Object

joinUser : `POST /api/users/` loginUser : `POST /api/users/login`

http authorizeRequest - `/api/users/`, `/api/users/login/`

PasswordEncoder >> BCryptPasswordEncoder

정리 필요

POST_api_users POST_api_users_login 테스트에 대한 추가 이해와 유닛/단위 테스트 필요

- line coverage : 0.7 > 0.5 - method coverage : 1.0 > delete

`@SpringBootTest` >> `@ExtendWith(MockitoExtension.class)`

`@SpringBootTest` >> `@WebMvcTest(UserController.class)`

jinyoungchoi95

엄살 부리셨던거에 비해서 잘 구현하신 것 같아요 💯

validation에 대한 테스트들도 추가해보시면 좋은 경험이 될 것 같아요 :)
몇가지 코멘트 남겨두었으니 확인부탁드려요~

jinyoungchoi95 · 2021-08-07T19:57:57Z

src/main/java/com/study/realworld/config/SecurityConfig.java

+import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
+import org.springframework.security.crypto.password.PasswordEncoder;
+
+@RequiredArgsConstructor


Suggested change

@RequiredArgsConstructor

Constructor가 필요한 클래스일까요?

보안에 ! 필요한걸 ! 생성한다 !

확인하겠습니다 ~

jinyoungchoi95 · 2021-08-07T20:01:41Z

src/main/java/com/study/realworld/global/common/BaseTimeEntity.java

+    @CreatedDate
+    @Column
+    private LocalDateTime createdAt;
+
+    @LastModifiedDate
+    @Column
+    private LocalDateTime updatedAt;
+
+    @Column
+    private LocalDateTime deletedAt;


Suggested change

@CreatedDate

@Column

private LocalDateTime createdAt;

@LastModifiedDate

@Column

private LocalDateTime updatedAt;

@Column

private LocalDateTime deletedAt;

@CreatedDate

@Column(name = "created_at", updatable = false)

private LocalDateTime createdAt;

@LastModifiedDate

@Column(name = "updated_at")

private LocalDateTime updatedAt;

@Column(name = "deleted_at")

private LocalDateTime deletedAt;

테이블 컬럼명을 맞추어야할 것 같아요 :)

앱 실행하고 h2-console에서 확인하면 이렇게 나오는데 혹시 이렇게 저 처럼 알아서 넣어지길 바라는건 틀린걸까요 ?

위 같은 상황에서 name은 안 넣어도 상관없을 것 같아요
updatable=false는 필요할수도 있겠네요

저는 생각이 조금 다른 것 같아요
물론, 명명 규칙에 의해서 자동으로 컬럼 명을 만들어주지만
명명 규칙은 스프링 설정 파일에서도 변경 가능하고, 무엇이든 명시적으로 기술하는게 낫다고 보거든요

저도 동일한 의견입니다.
자동으로 만들어둔 상황이 어떻게 될지 모르기때문에 과도하게 보일지라도 네이밍 규칙은 항상 명시하는게 좋다고 생각합니다.

명시적... 누구에겐 취향일 수도 있겠지만 확실하게 있는 것도 좋을 것 같네요 !

jinyoungchoi95 · 2021-08-07T20:03:01Z

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

+    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();
+    }


혹시 어디에 사용되는 메소드일까용?

지금은 안쓰이네요 ㅠ
테스트할 때도 커버리지 안걸려서 나중에 쓰겠지하고 일단 냅뒀는데...
User 쪽 다만들고도 안쓰면 지우겠습니다 !

@jinyoungchoi95

테이블 컬럼명을 맞추어야할 것 같아요 :) from @jinyoungchoi95 물론, 명명 규칙에 의해서 자동으로 컬럼 명을 만들어주지만, 명명 규칙은 스프링 설정 파일에서도 변경 가능하고, 무엇이든 명시적으로 기술 from @kwj1270 Co-authored-by: 최진영 <jinyoungchoi95@gmail.com>

Constructor가 필요한 클래스일까요? from jinyoungchoi95 Co-authored-by: 최진영 <jinyoungchoi95@gmail.com>

chance0523

깔끔하네용 👍

chance0523 · 2021-08-09T15:26:56Z

src/main/java/com/study/realworld/user/web/UserController.java

+
+    @PostMapping("/users")
+    public ResponseEntity<UserResponse> joinUser(@RequestBody UserJoinRequest request) {
+        User user = userService.save(request);


이거 save 말고 join이 좋지 않을까요? 컨트롤러랑 리퀘스트 네이밍 가져가는게 좋은 것 같아요

chance0523 · 2021-08-09T15:27:33Z

src/main/java/com/study/realworld/user/web/UserController.java

+        String token = null;
+
+        return ResponseEntity.ok().body(UserResponse.of(user, token));


이거 null로 해서 넣어주는거 별로 보기 안 좋은 것 같아요...
혹시 꼭 이렇게 넣어야하나요?

아직 토큰 구현 안하셔서이지 않을까 싶어요

추후 response에 담겨질 token을 미리 만들어보았습니다 !

chance0523 · 2021-08-09T15:32:00Z

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

+        when(userRepository.findById(anyLong()))
+                .thenReturn(Optional.ofNullable(user));
+
+        //doNothing().when(userRepository).delete(any());


얘는 지워도 되겠죠?

지워도 되는 부분입니다 !
테스트 코드 부분이라서 개인적으로 고생했던 부분을 따뜻하게 담아보았습니다 😢

chance0523 · 2021-08-09T15:32:22Z

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

+        /**
+         * This exception may occur if matchers are combined with raw values:
+         *     //incorrect:
+         *     someMethod(anyObject(), "raw String");
+         * When using matchers, all arguments have to be provided by matchers.
+         * For example:
+         *     //correct:
+         *     someMethod(anyObject(), eq("String by matcher"));
+         *
+         * For more info see javadoc for Matchers class.
+         */


이거 필요한 주석인가요??

지워도 되는 부분입니다 !
테스트 코드 부분이라서 개인적으로 고생했던 부분을 따뜻하게 담아보았습니다 😢

chance0523 · 2021-08-09T15:32:48Z

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

+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;
+
+@WebMvcTest(UserController.class)
+class UserControllerTest {


이 테스트에 뭔가 주석들이 많네요
아직 미완성 테스트인가요??

미완성 테스트는 아닌 것 같습니다 😢

제가 테스트 코드를 처음 써봐서 @SpringBootTest @WebMvcTest @DataJpaTest @ExtentionWith(SpringExtention.class) @ExtentionWith(MockitoExtention.class) @Mock @InjectMocks @MockBean 등 여러가지를 써보고 의미 있는 부분을 코드에 직접 기록하는 느낌으로 주석을 남겼습니다. 같이 쓰는 공간(?)에 이런걸 남겨도 되나 싶었지만 테스트라는 것을 처음 해봤고, 시간도 굉장히 많이 할애했어서 의미 있다고 생각되어 남겨봤습니다.

리뷰 달아주셔서 감사합니다 찬스님 💘

`@DataJpaTest` 에 `JpaConfig` 추가해서 `BasetimeEntity` 테스트

codecov-commenter · 2021-08-11T02:55:42Z

Codecov Report

Merging #19 (51e51e6) into hoony-lab (f7b58eb) will decrease coverage by 9.09%.
The diff coverage is 90.67%.

@@               Coverage Diff               @@
##             hoony-lab      #19      +/-   ##
===============================================
- Coverage       100.00%   90.90%   -9.10%     
- Complexity           2       54      +52     
===============================================
  Files                1       10       +9     
  Lines                3      121     +118     
  Branches             0        1       +1     
===============================================
+ Hits                 3      110     +107     
- Misses               0        8       +8     
- Partials             0        3       +3

Impacted Files	Coverage Δ
...ain/java/com/study/realworld/user/domain/User.java	`70.00% <70.00%> (ø)`
.../com/study/realworld/user/service/UserService.java	`89.47% <89.47%> (ø)`
...ain/java/com/study/realworld/config/JpaConfig.java	`100.00% <100.00%> (ø)`
...ava/com/study/realworld/config/SecurityConfig.java	`100.00% <100.00%> (ø)`
.../study/realworld/global/common/BaseTimeEntity.java	`100.00% <100.00%> (ø)`
.../com/study/realworld/user/dto/UserJoinRequest.java	`100.00% <100.00%> (ø)`
...com/study/realworld/user/dto/UserLoginRequest.java	`100.00% <100.00%> (ø)`
...ava/com/study/realworld/user/dto/UserResponse.java	`100.00% <100.00%> (ø)`
...a/com/study/realworld/user/web/UserController.java	`100.00% <100.00%> (ø)`
... and 8 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 f7b58eb...51e51e6. Read the comment docs.

com8599

코드 잘봤습니다!!! 👍 이쁜코드가 많아서 편하게 봤습니다!

com8599 · 2021-08-11T09:13:01Z

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

+
+    public User save(UserJoinRequest request) {
+        userRepository.findByEmail(request.getEmail())
+                .ifPresent(o -> new DuplicateKeyException(o.getEmail() + " already exist"));


이런식으로 익셉션을 사용하는거군요..! 너무 좋은 코드같아요 많이 배웁니다!!!... 👍 😄

com8599 · 2021-08-11T09:18:23Z

src/test/java/com/study/realworld/user/domain/UserTest.java

+                () -> assertEquals(PASSWORD, user.getPassword()),
+                () -> assertEquals(BIO, user.getBio()),
+                () -> assertEquals(IMAGE, user.getImage())
+        );


궁금한 점이 있는데용 객체를 빌드하는 단계까지 테스트하는 의의를 여쭤볼 수 있을까요?
제가 객체빌드 또는 생성까지 테스트하는건 처음봐서요! ➰

User 가 Builder로 잘 만들어지나 한 번 테스트해봤어요

DolphaGo

깔끔하네요.

hoony-lab · 2021-08-11T10:18:44Z

깔끔하네요.

LGTM 👍

kwj1270

깔끔한 코드네요 💯
몇 가지 코멘트 남겼으니 확인 부탁드려요 :)

kwj1270 · 2021-08-10T04:39:06Z

src/main/java/com/study/realworld/user/web/UserController.java

+        String token = null;
+
+        return ResponseEntity.ok().body(UserResponse.of(user, token));


아직 토큰 구현 안하셔서이지 않을까 싶어요

kwj1270 · 2021-08-11T10:57:07Z

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

+
+    @Builder
+    public User(Long id, String email, String username, String password, String bio, String image) {
+        this.id = id;


GenerationType.IDENTITY의 경우 id를 제외한 빌더를 사용해도 상관없으니 참고 부탁해요 :)

아직 토큰 구현 안하셔서이지 않을까 싶어요

넵 맞습니다

GenerationType.IDENTITY의 경우 id를 제외한 빌더를 사용해도 상관없으니 참고 부탁해요 :)

DB쪽(?) 에서 케어하는 부분이라 상관없나보군요 🆗

위에꺼는 왜 코멘트를 달 수 있는 부분이 안보일까요 ㅠㅠ

이전에 코멘트 달았는데 outdate 되어서 그런것 같아요 ㅎㅎ

kwj1270 · 2021-08-11T10:58:39Z

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

+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        User user = (User) o;
+        return Objects.equals(email, user.email);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(email);
+    }


프록시 객체 활용시 값이 null 이 나올 수 있어서 email()로 사용해주면 좋을 것 같아요

프록시 객체 라는 걸 확인해보도록 하겠습니다 !

kwj1270 · 2021-08-11T11:00:44Z

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

+        User user = userRepository.findByEmail(request.getEmail())
+                .orElseThrow(() -> new NoSuchElementException(request.getEmail() + " not found"));


findByEmail이 중복되는 것 같은데 메서드로 분리해서 재사용도 좋을 것 같네요!

kwj1270 · 2021-08-11T11:01:36Z

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

+        if(!user.matchesPassword(request.getPassword(), passwordEncoder)) {
+            throw new NoSuchElementException(request.getPassword() + " wrong wrong wrong triple wrong" + user.getPassword());
+        }


에러 검증 로직이 login()이라는 비즈니스 로직과는 조금 거리가 있다고 생각해요
이럴 경우 메서드로 분리해서 내부가 어떻게 돌아가는지 감추는 것도 좋을 것 같습니다

Service > login 에는 비즈니스 로직만...(?)
메서드로 분리 라는게 클린 코드 에 해당 하는 부분이라고 생각하면 될까요 ?

참고할수있는 키워드 하나만 주시면 더 공부해보겠습니다 !

사람마다 기준점은 다르지만 저 같은 경우
login()은 순수하게 로그인 기능만을 표현해야 된다고 생각해요
저 같은 경우 이런 예외를 던지는 부가 기능 부분에 있어서 최대한 메서드로 분리시키려고 해요

에러 발생을 도메인단으로 보내도 될 것 같은데 이 부분도 고려해볼만 하겠네요
제 리뷰는 단순히 저의 견해이고 아직 저도 좋은 방법은 못찾아서 그냥 참고만 부탁해요

감사합니다 뇌섹 우지님 💘

User Builder 관련 수정 User, UserTest, UserRepositoryTest, UserServiceTest, UserControllerTest 수정 idea-by: kwj1270 Co-Authored-By: 최진영 <jinyoungchoi95@gmail.com>

`findByEmail이 중복되는 것 같은데 메서드로 분리해서 재사용도 좋을 것 같네요!` Co-Authored-By: KimWooJae <50267433+kwj1270@users.noreply.github.com>

> `메서드로 분리해서 내부가 어떻게 돌아가는지 감추는 것` `validateMatchesPassword` 메소드 추가하여 비밀번호 확인 로직 이동 Co-Authored-By: KimWooJae <50267433+kwj1270@users.noreply.github.com>

povia

의견만 하나 드렸습니다~ LGTM 👍

povia · 2021-08-13T17:41:44Z

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

+import java.util.NoSuchElementException;
+
+@RequiredArgsConstructor
+@Transactional


@Transactional은 보통 Class단에 어노테이션으로 먹일 때에는 readOnly = true, CUD 메소드에 먹일 때 readOnly = false로 먹이니 참고만 해주셔용

hoony-lab and others added 25 commits August 1, 2021 22:49

[#12] chore: jpa, security, lombok dependency 추가

7905b5a

[#12] chore: lombok dependency 추가

9ebe365

[#12] feat: User Entity 추가

ea16e7b

[#12] feat: User에 BaseTimeEntity 추가

6713f70

[#12] test: RealworldApplicationTest 수정

f8fda88

[#12] feat: UserRepository 추가

6c474b6

[#12] test: UserTest 추가

214377d

user_builder user_equals_and_hashCode

[#12] feat: UserService 추가

d958304

findAll, findById, findByEmail, save, deleteById

[#12] test: UserServiceTest 추가

d2817b8

save_and_findAll save_and_findByEmail save_and_findById save_and_deleteById save_and_deleteById_exception

[#12] feat: UserJoinRequest 추가

48e6cf0

`@JsonTypeName`, `@JsonTypeInfo` : Mapping Object static `toUser` : convert DTO to Entity(User) for communicate within layers

[#12] feat: UserResponse 추가

0623bb6

`@JsonTypeName`, `@JsonTypeInfo` : Mapping Object static `of`: convert Entity(User) to DTO for communicate within layers

[#12] feat: UserLoginRequest 추가

e43fb1f

`@JsonTypeName`, `@JsonTypeInfo` : Mapping Object

[#12] feat: UserContoller 추가

e60bde9

joinUser : `POST /api/users/` loginUser : `POST /api/users/login`

[#12] feat: SecurityConfig 추가

f77ffb7

http authorizeRequest - `/api/users/`, `/api/users/login/`

[#12] feat: PasswordEncoder Bean 추가

6ce2d98

PasswordEncoder >> BCryptPasswordEncoder

[#12] feat: 비밀번호 암호화 및 확인 로직 추가

3612e99

정리 필요

[#12] test: UserControllerTest (@SpringBootTest) 추가

74abc86

POST_api_users POST_api_users_login 테스트에 대한 추가 이해와 유닛/단위 테스트 필요

chore: test.gradle coverage change

9a6f361

- line coverage : 0.7 > 0.5 - method coverage : 1.0 > delete

chore: build.yml push, pr branch retouch

91cddf0

[#12] feat: passwordEncoder를 controller에서 service로 이동

496fc01

[#12] chore

02df674

[#12] test: userServiceTest 수정

fa4bffc

`@SpringBootTest` >> `@ExtendWith(MockitoExtension.class)`

[#12] test: userControllerTest 수정

e96f72a

`@SpringBootTest` >> `@WebMvcTest(UserController.class)`

Merge branch 'main' into hoony-lab-issue12

61940b0

[#12] feat: UserNotFoundException 삭제

35d86b6

hoony-lab requested review from chance0523, com8599, DolphaGo, kwj1270 and povia August 7, 2021 11:41

hoony-lab requested a review from sujl95 August 7, 2021 11:41

hoony-lab self-assigned this Aug 7, 2021

hoony-lab added the hoony-lab 후니의 이슈 label Aug 7, 2021

jinyoungchoi95 reviewed Aug 7, 2021

View reviewed changes

hoony-lab changed the title ~~[hoony-lab #12] User 도메인 구현~~ [hoony-lab-issue12] User 도메인 구현 Aug 8, 2021

jinyoungchoi95 approved these changes Aug 8, 2021

View reviewed changes

hoony-lab and others added 2 commits August 9, 2021 01:05

[#12] proposal

812e27f

Constructor가 필요한 클래스일까요? from jinyoungchoi95 Co-authored-by: 최진영 <jinyoungchoi95@gmail.com>

chance0523 reviewed Aug 9, 2021

View reviewed changes

[#12] feat: UserRepositoryTest 추가

cec6f69

`@DataJpaTest` 에 `JpaConfig` 추가해서 `BasetimeEntity` 테스트

com8599 reviewed Aug 11, 2021

View reviewed changes

DolphaGo approved these changes Aug 11, 2021

View reviewed changes

kwj1270 approved these changes Aug 11, 2021

View reviewed changes

hoony-lab and others added 3 commits August 12, 2021 18:22

fix: User Builder에 id 삭제

c68c196

User Builder 관련 수정 User, UserTest, UserRepositoryTest, UserServiceTest, UserControllerTest 수정 idea-by: kwj1270 Co-Authored-By: 최진영 <jinyoungchoi95@gmail.com>

[#12] fix: UserService 중복되는 함수 삭제

86bf551

`findByEmail이 중복되는 것 같은데 메서드로 분리해서 재사용도 좋을 것 같네요!` Co-Authored-By: KimWooJae <50267433+kwj1270@users.noreply.github.com>

[#12] fix: UserService 부가 기능 수정

51e51e6

> `메서드로 분리해서 내부가 어떻게 돌아가는지 감추는 것` `validateMatchesPassword` 메소드 추가하여 비밀번호 확인 로직 이동 Co-Authored-By: KimWooJae <50267433+kwj1270@users.noreply.github.com>

hoony-lab merged commit b5adfca into hoony-lab Aug 13, 2021

povia reviewed Aug 13, 2021

View reviewed changes

		String token = null;

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

		User user = userRepository.findByEmail(request.getEmail())
		.orElseThrow(() -> new NoSuchElementException(request.getEmail() + " not found"));

[hoony-lab-issue12] User 도메인 구현 #19

[hoony-lab-issue12] User 도메인 구현 #19

Conversation

hoony-lab commented Aug 7, 2021

Issue: #12

작업 내용

생성/변경 로직

개인 코멘트

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

kwj1270 Aug 8, 2021 • edited

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

chance0523 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

codecov-commenter commented Aug 11, 2021 • edited

Codecov Report

com8599 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

DolphaGo left a comment

Choose a reason for hiding this comment

hoony-lab commented Aug 11, 2021

kwj1270 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

kwj1270 Aug 12, 2021 • edited

Choose a reason for hiding this comment

kwj1270 Aug 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

povia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwj1270 Aug 8, 2021 •

edited

codecov-commenter commented Aug 11, 2021 •

edited

kwj1270 Aug 12, 2021 •

edited

kwj1270 Aug 12, 2021 •

edited