Skip to content
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.

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

[유안] API 테스트 / 문서자동화 미션 제출합니다. #54

Merged
merged 65 commits into from
Jun 26, 2020

Conversation

KimSeongGyu1
Copy link

@KimSeongGyu1 KimSeongGyu1 commented May 29, 2020

잘 부탁드립니다

1 멤버 관리 / 즐겨찾기에 관한 코드에만 수정을 했습니다

2 학습을 위해 세션과 토큰을 동시에 사용하는 연습을 해보았습니다
- Resolver에서 이 둘이 같은 값인지에 대한 검증을 해주었습니다

3 한 리소스에 대해서 한 개의 uri만을 가지게 하고 싶어서 "/me" 를 "/members/id"로 변경했습니다
- uri의 id값과 세션, 토큰의 정보가 일치하는지 Resolver에서 검증해줍니다

WebMvcConfig: Basic 방식 제거
LoginMemberController: 사용자 정보 조회 Basic URI 제거
AuthAcceptanceTest: Basic 방식 테스트 제거
/snackbar: alert 대신 사용할 snackbar 생성
Join.js: 유효성 검사 메서드 생성
MemberRequest: 생성자 추가
index.js: member 관련 라우트 추가
join.js: 멤버 생성 API 요청
PageController: "/"를 사용자 메인 페이지로 수정
LoginMemberController: JWT 토큰과 Session을 함께 발행
index.js: find를 POST로 변경
Login.js: 로그인 fetch api 연동, 사용자 메인 페이지로 리다이렉트 추가
Join.js: 사용자 메인 페이지로 리다이렉트 추가
LoginMemberController: 내 정보 조회 기능 라우팅 통합
LoginMemberMethodArgumentResolver: 세션과 JWT 회원정보 일치 여부 비교
SessionInterceptor, BearerAuthInterceptor: 회원정보 일치 여부 비교를 위한 요청 키 값 수정
Login.js: 로그인시 JWT 값을 localStorage에 저장
index.js: 헤더에 Authorization을 담아 요청
PageController: 내 정보 조회 라우팅
index.js: 헤더에 Authorization을 담아 요청
mypage-edit.html: email 수정 못하도록 p 태그로 변경, data-id 속성 추가
index.js: 헤더에 Authorization을 담아 요청
MyPageEdit.js: 버튼에 삭제 이벤트 연결
ValidateLogin.js: 로그인 확인 로직 구현
index.js: 회원 정보 삭제 시 localStorage 삭제
myPage.html, myPage-edit.html: 로그인 validation js 삽입
templates.js: 로그인 여부에 따른 동적 템플릿 구성
index.js: 로그인 여부에 따라 적절한 템플릿 호출
Index.js: 로그아웃 버튼 클릭시 localStorage 초기화 및 리로딩
AuthAcceptanceTest: session과 bearer을 동시에 검증하도록 수정
PageAcceptanceTest: MediaType 수정
MemberControllerTest: readMember 테스트
MemberDocumentation, api-guide.adoc: 생성할 API 문서 설정
MemberControllerTest: updateMember 테스트
MemberDocumentation: updateMember 메서드 수정
MemberControllerTest: deleteMember 테스트
MemberDocumentation: deleteMember 메서드 구현
AOP를 활용해서 Interceptor에서 설정해준 attribute를 이용하여 유효성 검사
어노테이션을 활용해 미적용할 메서드 지정
테스트도 수정사항에 맞추어 변경
커스텀 예외를 사용하도록 코드 수정
PathService: NoPathExistsException를 던지는 예외처리 추가
관련 예외에 대한 문서화 진행
AOP 사용하던 부분을 MethodArgumentResolver로 변경
Favorite: 즐겨찾기 도메인 클래스 생성
Favorites: 일급 컬렉션 생성
Member: Favorites 필드 추가
MemberService: 즐겨찾기 추가, 제거 메서드 구현
Favorite: 정적 생성자 메서드 추가
Favorites, Member: 사용하지 않는 메서드 제거
FavoriteController: 즐겨찾기 추가 라우팅
AddFavoriteRequest, FavoriteResponse: dto 생성
WebMvcConfig: 즐겨찾기 uri를 인터셉터에 추가
FavoriteService: addFavorite시 추가된 즐겨찾기를 반환하도록 수정
FavoriteController: 즐겨찾기 조회 라우팅
FavoritesResponse: dto 생성
FavoriteService: readFavorites 메서드 구현
Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

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

유안 안녕하세요! 리뷰가 늦었네요 ㅠㅠ
몇가지 피드백 남겼으니 확인 후 반영해보세요 :)

@ExceptionHandler(Exception.class)
public ResponseEntity<ExceptionResponse> handleUnexpectedException(Exception e) {
LOGGER.error(e);
return new ResponseEntity<>(ExceptionResponse.of("서버 오류가 발생했어요."), HttpStatus.INTERNAL_SERVER_ERROR);
Copy link

Choose a reason for hiding this comment

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

👍


@ExceptionHandler(Exception.class)
public ResponseEntity<ExceptionResponse> handleUnexpectedException(Exception e) {
LOGGER.error(e);
Copy link

Choose a reason for hiding this comment

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

log를 남길때 해당하는 log를 찾기 쉽도록
logger.error("error message >>> {}", error.getMessage(), e);
이런 식으로 앞에 error message >>> 같은 문구를 넣어주거나 에러에 관한 더 자세한 정보를 함께 남길 수 있어요 :)

아래 글을 참고 해 보세요 :)
https://blog.leocat.kr/notes/2018/12/30/logger-last-throwable-param-on-logging-formatted-message

Copy link
Author

Choose a reason for hiding this comment

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

log를 잘보이도록 하는 김에 더 잘보이도록 줄바꿈과 특수문자를 섞어서

LOGGER.error("\n@@@@@@@@@@ error message @@@@@@@@@@\n >>> {}", e.getMessage(), e);

이렇게 바꿔 봤어요.
저는 이게 더 잘 보여서서 좋은데 이런식으로 해도 문제가 없을까요?

Copy link

Choose a reason for hiding this comment

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

문제는 없지만 보통 로그를 서버에 파일로 남기고, 이는 서버의 리소스를 사용하는 것이기 때문에, 왠만하면 필요한 문자만 사용하는 것이 좋을 것 같아요 :)
생각보다 서버에서 disk full이 자주 일어나는 것 같아요.

Object handler,
ModelAndView modelAndView) throws Exception {
if (StringUtils.isEmpty(token)) {
throw new InvalidAuthenticationException("토큰이 없어요");
Copy link

Choose a reason for hiding this comment

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

토큰이 없을때를 테스트 하는 코드가 없는것 같아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

컨트롤러에서만 테스트를 하고 인지 테스트를 하지 않았네요ㅜ
추가했습니다

Comment on lines 64 to 66
String inputJson = "{\"email\":\"" + TEST_USER_EMAIL + "\"," +
"\"name\":\"" + TEST_USER_NAME + "\"," +
"\"password\":\"" + TEST_USER_PASSWORD + "\"}";
Copy link

Choose a reason for hiding this comment

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

이 부분이 중복되어 보여요!

Copy link
Author

@KimSeongGyu1 KimSeongGyu1 Jun 1, 2020

Choose a reason for hiding this comment

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

String.format()으로 처리했어요

Comment on lines 108 to 112
this.mockMvc.perform(get("/members?email=" + TEST_USER_EMAIL)
.session(session)
.header("authorization", "Bearer " + TEST_USER_TOKEN)
.accept(MediaType.APPLICATION_JSON)
.contentType(MediaType.APPLICATION_JSON))
Copy link

Choose a reason for hiding this comment

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

이부분도 중복 제거 가능하지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

상속 형태로 FavoriteControllerTest까지 중복제거 해봤어요


public class Member {
@Id
private Long id;
private String email;
private String name;
private String password;
@Embedded.Empty
private Favorites favorites = Favorites.empty();
Copy link

Choose a reason for hiding this comment

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

Member가 favorites를 갖고있는 것이 맞는가에 대해 의문이 드는데, 유안은 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

Member가 favorites를 가지지 않는다면 따로 id값을 가지는 엔티티가 될텐데,
이 때 가장 큰 차이점은 Member가 삭제될 때 Favorites이 같이 삭제가 되는가 라고 생각해요.

계속 누적되는 즐겨찾기 데이터를 가지고 유의미한 정보를 가공하고 싶다면 Favorites 데이터가 삭제되지 않도록 막아야 한다고 생각해요.
사용자가 탈퇴했을 때 개인 정보를 괜히 계속 들고 있을 필요가 있는가라고 생각했을 때, 지금은 필요 없다고 생각했어요.

현재 Spring data jdbc를 어설프게 사용하고 있어서 생기는 문제가 있어요
단순히 Member의 password 하나만 수정해도 Member의 모든 값에 대한 쿼리가 날아가기 때문에 Member가 favorites를 가지면 성능 이슈가 있어요
커스텀 쿼리를 만들어서 해결할 수 있다고 들었는데 이 부분은 한 번 공부해서 적용해볼게요

Copy link

Choose a reason for hiding this comment

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

Member가 삭제될 때 Favorites이 같이 삭제하는 것은 Member가 꼭 Favorites를 필드로 갖고있지 않아도 로직적으로 처리하거나, DB기능으로 처리 가능해 보여요.

Member를 조회할때 Favorites를 함께 조회하는가에 대해 생각해보면 어떨까요?
그리고, Favorites같은 테이블이 많아졌을때 지금처럼 Member가 필드로 들고있으면 어떤 문제가 있을지 생각해 보면 좋을 것 같아요 :)


public void validateId(Long id) {
if (!this.id.equals(id)) {
throw new InvalidAuthenticationException("니 아이디 아님");
Copy link

Choose a reason for hiding this comment

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

니 아이디 아님 인지 테스트코드가 없는 것 같아요 😁
전체적으로 Exception에관한 테스트가 없어보이는 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

전반적으로 Exception에 관한 테스트를 추가했어요
리뷰를 받고 우연히 바로 책에서 Exception 테스트의 중요성에 대해 읽었는데, 중요성이 더 와닿아서 좋았어요

Comment on lines +28 to +29
registry.addInterceptor(sessionInterceptor).addPathPatterns("/members", "/members/**");
registry.addInterceptor(bearerAuthInterceptor).addPathPatterns("/members", "/members/**");
Copy link

Choose a reason for hiding this comment

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

지금은 members에 관해서만 인터셉터가 걸렸지만, 나중에 api가 많아졌을땐 전체 Path를 걸고 인증처리 하지 않을 api만 exclude 해주는 것이 인증 누락될 확률이 낮을 것 같아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

실수가 일어났을 때 인증처리를 할 곳에 안하는 상황보다는, 인증처리를 안 할 곳에 하는 것이 더 안전해 보이네요 :)

그런데 궁금한 점이 있어요
api가 많아졌을 때 path별로 다른 인증방식을 사용하는 경우가 있을까요?
그런 상황에서는 path별로 인증을 걸어주는 방향으로 하고 싶을 것 같아요

Copy link

Choose a reason for hiding this comment

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

api가 많아졌을 때 path별로 다른 인증방식을 사용하는 경우

이런 경우는 거의 없는 것 같아요. 있더라도 기본적인 인증방식을 먼저 전체에 걸고 필요에 따라 몇몇 Path에 다른 인증방식을 걸지 않을까싶어요.

전체 path에 걸고 exclude하는 방식은 제안일 뿐이라서 필요에 따라 맞게 사용하시면 될 것 같아요 :)

return ResponseEntity.ok().body(MemberResponse.of(member));
}

@PutMapping("/members/{id}")
public ResponseEntity<MemberResponse> updateMember(@PathVariable Long id, @RequestBody UpdateMemberRequest param) {
@GetMapping("/{id}")
Copy link

Choose a reason for hiding this comment

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

코멘트 남겨주신 /me -> /members/{id} 부분이 이 부분일까요?
제 생각엔 다른 사람의 정보를 조회 할 수 있도록 Api가 열린 것 같아서 보안 취약점이 될 수도 있을 것 같다는 생각이 드네요 ㅎㅎ
물론 인증 처리로 토큰이나 세션과 해당 {id}가 같은가? 에 대한 체크를 해줘도 될 것 같긴 하지만 불필요하게 한번 더 하는것이 아닌가? 라는 생각이 들어요

Copy link
Author

Choose a reason for hiding this comment

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

네 지금은 토큰, 세션, id가 전부 일치하는지 체크하고 있어요
말씀하신대로 불필요할 수도 있다는 생각이 들어요

그런데 REST에 대해 신경쓰기 시작하면서 "/me"는 리소스를 제대로 표현하지 못한다고 생각해서 "members/id"로 했어요
"/me"는 자주 쓰이는 uri 패턴이라고 듣기는 했는데 이게 rest하지는 않지만 효율적이라서 사용하는 것인지, rest한지 잘 모르겠어요.
이 부분은 어떻게 검색해야 답을 얻을 수 있는지도 잘 모르겠어요ㅜ

Copy link

Choose a reason for hiding this comment

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

- MemberController, FavoriteController에 대해 개선
공통되는 MockMvc에 대한 부분 묶어서 상속하도록 변경

- 그 외 String format 이용 중복 개선
MemberControllerTest안에 있던 인가에 대한 테스트 상위 클래스로 위치 변경
테스트를 세분화 한 뒤 DynamicTest를 이용해 각자 검증
인가에 대해 부족한 테스트를 DynamicTest에 추가
인가에 대한 부분은 따로 검증
인가 인지 테스트에 자기 아이디가 아닐 때 검증 추가
Authentication 관련 예외 세분화
NoMemberExistException
NoFavoriteExistException
DuplicatedEmailException
DuplicatedFavoriteException
Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

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

유안! 테스트를 꼼꼼히 추가해주신 듯 하네요 👍
기존 남겼던 코멘트만 읽어보셔도 될 것 같아요.
추가적으로 질문이 있으시면 편하게 코멘트나 DM주세요 :)

lineStation은 line과 함께 eager로 로딩되는 벨류로 취급함
Favorite는 Member(member_id), Station(source_id, target_id)을 ManyToOne으로 참조
인수테스트 제외 domain, service, controller까지 테스트 통과 확인
FavoriteService 적절하게 수정
통과하지 못하는 인수테스트 일단 주석처리
1. @transactional 문제(jdbc 이용할 때 작성했던 불필요한 save문 제거도 진행)
2. Long과 long으로 인한 문제: (더티 체킹 후 delete 쿼리가 의도하지 않은 값에 대해서만 비교하는 문제)
Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

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

유안 안녕하세요!
미션 하느라 고생 많으셨습니다 👍
여기서 머지 해도 될 것 같아요. 코멘트 하나 남겼으니 확인해보세요~!

Comment on lines 29 to 30
@LastModifiedDate
private LocalDateTime updatedAt;
Copy link

Choose a reason for hiding this comment

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

JPA의 Audit기능에 대해 한번 읽어보셔도 좋을 것 같아요 :)
https://tramyu.github.io/java/spring/jpa-auditing/

@dave915 dave915 merged commit 4b7a283 into woowacourse:kimseonggyu1 Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants