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

[4 - 6단계 방탈출 사용자 예약] 구름(김민수) 미션 제출합니다. #87

Merged
merged 67 commits into from
May 11, 2024

Conversation

alstn113
Copy link
Member

@alstn113 alstn113 commented May 9, 2024

안녕하세요. 영이! 구름 ⛅️ 입니다.

이번 미션은 인증과 관련된 문제들이었습니다.
다른 언어의 프레임워크를 사용했어서 인증에 큰 어려움은 없었습니다.

테스트 없이 쌩구현으로 하고 이후에 테스트만 따로 작성했습니다.
테스트 정말 힘든 것 같아요. 영이는 어떤 순서와 방식으로 테스트를 작성하시나요?
테스트가 생산성이 떨어진다고 생각하는건 제 부족한 경험때문이겠죠?,,,

⛅️ 주요 변경 사항은 다음과 같습니다.

  1. jbcrypt로 회원가입 시 비밀번호를 암호화하고 jjwt로 jwt token이 담긴 cookie를 통해 인증을 구현했습니다.
  2. 인증 처리를 위해 HandlerMethodArgumentResolver와 HandlerInterceptor를 사용했습니다.
  3. Request에서 Date와 Time을 검증하는 Validator는 코드 복잡도가 너무 올라가서 제거했습니다.
  4. service에서 mock을 사용했으나 피드백을 받고 실제 테스트로 변경했습니다.

⛅️ 미션을 하면서 했던 고민들입니다.

  1. 인증과 관련된 부분, 예를 들어, 예약을 하는 경우가 있을 때 token cookie가 필요해요.
    이 경우 실제로 로그인을 해서 쿠키에 토큰을 넣어서 처리하는게 실제와 제일 비슷하다고 생각은 했어요.
    하지만 복잡도가 올라가서 jwtTokenProvider를 SpyBean으로 모킹해서 사용했어요. 이게 적합한 방법일까요?

  2. controller를 테스트할 때, BaseControllerTest에서 truncate로 데이터를 다 밀어버리는 방식을 사용했어요.
    테스트용 데이터는 필요한 부분에 대해서 Sql 어노테이션으로 넣어줬어요.
    현업에서는 테스트용 데이터가 아주 많기 때문에 truncate는 잘 사용하지 않고, 썼던 데이터만 부분적으로 지운다는데 어떠한가요??

  3. JwtTokenProvider에서 적절하게 예외 처리를 했는가 궁금합니다.

초기 데이터 정보

어드민

유저

Copy link

@choijy1705 choijy1705 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 1 to 6
SET referential_integrity FALSE;
TRUNCATE TABLE member RESTART IDENTITY;
TRUNCATE TABLE theme RESTART IDENTITY;
TRUNCATE TABLE reservation_time RESTART IDENTITY;
TRUNCATE TABLE reservation RESTART IDENTITY;
SET referential_integrity TRUE;

Choose a reason for hiding this comment

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

controller를 테스트할 때, BaseControllerTest에서 truncate로 데이터를 다 밀어버리는 방식을 사용했어요.
테스트용 데이터는 필요한 부분에 대해서 Sql 어노테이션으로 넣어줬어요.
현업에서는 테스트용 데이터가 아주 많기 때문에 truncate는 잘 사용하지 않고, 썼던 데이터만 부분적으로 지운다는데 어떠한가요??

처음 듣는 얘기네요😭
애초에 test data를 sql 파일로 넣는것은 테스트의 가독성을 현저히 떨어뜨린다고 생각합니다. 뿐만 아니라 다른 테스트에도 영향을 줄 수 있고요. 정말 기본적으로 필요한 값들이 아닌이상 sql 파일로 테스트 데이터를 만드는건 좋은방법이 아니라고 생각합니다

given-when-then 방법으로 통해서 given에서는 적절히 해당상황에 맞는 테스트 데이터를 세팅하고 처리한 후 지워지도록 설계하는것이 좋을것 같네요

Copy link
Member Author

Choose a reason for hiding this comment

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

초기 데이터가 굉장히 많을 수 있지 않나요?
그런 경우에 자바 코드로 데이터를 집어넣을려면 오히려 가독성이 떨어질 것 같은데 @Sql을 사용하지 않나요??

나머지 부분에 대해서는 given-when-then에서 데이터를 세팅하는게 맞다고 생각합니다.

Choose a reason for hiding this comment

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

초기 데이터가 어떤 경우에 굉장히 많을 수 있나요?
회사 마다 다를 수 있겠지만 제가 프로젝트하거나 회사에서 코드를 짜면서 sql을 통해 테스트 환경을 세팅하는 경우는 없었습니다
물론 핵심적인 로직들을 수행하기 위해 회원같은 정보들을 미리 만들어두긴 하지만 이 또한 쿼리로 데이터를 넣도록 하지 않고
로직을 수행하여 테스트 db에 저장되도록 합니다

테스트 코드긴 하지만 직접적으로 쿼리를 입력하는건 굉장히 안좋은것 같아요😭
정합성이 안 맞을 여지가 있을것 같고
테스트가 잘 못 동작하더라도 쉽게 원인을 파악하지 못할 가능성도 충분히 있어보입니다

관리자 페이지를 만드는 이유중 하나도
직접 쿼리를 날리는건 위험하기 때문이라고 생각합니다

Copy link
Member Author

Choose a reason for hiding this comment

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

100개의 예약을 보여줘야하는 예시가 있어요. 그러면 최소 100개의 예약과 그것에 연결된 예약 시간과, 테마와, 멤버가 필요해요.
그런 경우 너무 복잡해지지 않나요?

물론 비즈니스 로직을 다 제끼고 DB에 들어가다보니 문제가 있을 것 같다는 생각이 드네요.
변경에도 취약하고, 원인 파악도 쉽지 않을 것 같아요.

Choose a reason for hiding this comment

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

100개의 예약을 보여주는 것은 반복인데 반복을 테스트 할 필요가 있나요?🤔
한개의 예약을 원하는데로 잘 보여주면 99개는 테스트할 필요가 없을것 같아요 모든 상황에 대해서 테스트 하는게 테스트 코드의 작성 목적은 아니니까요!
설령 있다하더라도 sql로 데이터 셋을 만드는것보다
코드 반복문을 통해서 생성하는게 더 쉽고 안전할것 같아요


@SpringBootTest(
classes = TestConfig.class,
webEnvironment = WebEnvironment.RANDOM_PORT
)
@DirtiesContext(classMode = ClassMode.BEFORE_EACH_TEST_METHOD)
abstract class BaseControllerTest {
public abstract class BaseControllerTest {

Choose a reason for hiding this comment

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

테스트 없이 쌩구현으로 하고 이후에 테스트만 따로 작성했습니다.
테스트 정말 힘든 것 같아요. 영이는 어떤 순서와 방식으로 테스트를 작성하시나요?
테스트가 생산성이 떨어진다고 생각하는건 제 부족한 경험때문이겠죠?,,,

ㅎㅎ 테스트는 생산성이 떨어지는게 맞습니다. 같은 동작을 두번 작성해야하니 당연히 떨어지지요
하지만 유지보수의 관점에서는 테스트가 있고 없고의 차이는 엄청난 차이입니다
테스트가 없는 코드를 리팩토링 한다면 아예 다시 짜버리는게 더 안전한 경우도 많을거에요
특히나 한사람이 작업하는 것이 아닌 여러 사람이 작업하는 코드에서는 더더욱 테스트 코드가 필요합니다

Copy link
Member Author

Choose a reason for hiding this comment

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

오늘 브라운 코치님이 테스트에 관한 강의를 해주셨는데 다양한 인사이트를 얻었어요. :)
사실 개발자가 개발만 하는건 아니잖아요?
테스트를 통해서 프로그램의 품질을 높일 수도 있고, 인수인계할 수 있는 문서의 역할을 한다는 점에서 테스트의 가치는 충분히 있다는 것을 깨달았어요.

Choose a reason for hiding this comment

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

좋은 강의를 들으셨군요!👍👍

@@ -1,152 +1,152 @@
document.addEventListener('DOMContentLoaded', function () {
updateUIBasedOnLogin();
updateUIBasedOnLogin();

Choose a reason for hiding this comment

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

javascript와 java의 컨벤션은 기본적으로 다릅니다
이 부분은 javscript 코드들은 수정되지 않도록 코드 포맷팅 하면 좋을거 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

prettier이라도 집어넣고 싶네요. 수정했습니다 :)

}

@PostMapping("/login")
public ResponseEntity<Void> login(

Choose a reason for hiding this comment

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

login에도 memberResponse가 반환되면 좋을것 같네요

Copy link
Member Author

Choose a reason for hiding this comment

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

서비스에서 LoginedMemberResponse라는 dto에 token과 MemberResponse를 담아서 넘기도록 처리했습니다.

.status(HttpStatus.FORBIDDEN)
.body(new ErrorResponse(e.getMessage()));
}

Choose a reason for hiding this comment

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

Suggested change

Copy link
Member Author

Choose a reason for hiding this comment

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

하하,,, 지웠습니다

Comment on lines +49 to +61
private Claims toClaims(String token) {
try {
Jws<Claims> claimsJws = getClaimsJws(token);

return claimsJws.getPayload();
} catch (ExpiredJwtException e) {
throw new IllegalArgumentException("만료된 토큰입니다.");
} catch (UnsupportedJwtException e) {
throw new IllegalArgumentException("지원하지 않는 토큰입니다.");
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("유효하지 않은 토큰입니다.");
}
}

Choose a reason for hiding this comment

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

JwtTokenProvider에서 적절하게 예외 처리를 했는가 궁금합니다.

어색하거나 이상한 부분 없는것 같아요!

Comment on lines +30 to +31
@SpyBean
private JwtTokenProvider jwtTokenProvider;

Choose a reason for hiding this comment

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

인증과 관련된 부분, 예를 들어, 예약을 하는 경우가 있을 때 token cookie가 필요해요.
이 경우 실제로 로그인을 해서 쿠키에 토큰을 넣어서 처리하는게 실제와 제일 비슷하다고 생각은 했어요.
하지만 복잡도가 올라가서 jwtTokenProvider를 SpyBean으로 모킹해서 사용했어요. 이게 적합한 방법일까요?

얼마나 복잡도가 올라갔나요?
개인적으로 어쩔 수 없는 경우가 아니라면 mocking을 하는 것은 의미없는 테스트라고 생각합니다.
(어쩔 수 없는 경우는 대부분 날짜와 관련된 부분에이였어요)
지금 테스트도 어쩔 수 없었던걸까요? 아니면 복잡해서 mocking한걸까요?
실제 production 코드가 변경되었을때 테스트가 잘 실패할지에 대해서 체크해보면 좋을것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

현재있는 인증에 대한 모든 부분과 앞으로 생길 모든 인증관련 코드에 토큰을 받고, cookie에 넣어주는 로직이 필요하잖아요.
그 정도면 복잡도가 올라갔다고 생각합니다.

/login, /login/check에서 잘 작동하는 것을 테스트했으면 다른 부분들에 대해서는 안해도 괜찮지 않을까요?

그리고 지금은 아니지만 oauth가 들어간다면 실제 요청을 많이 보낼 수는 없으니 mocking이 필요하지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

생각해보니 현재 프로젝트 수준에서는 충분히 테스트할 것 같네요 :)
조금 복잡하기는 하지만 BaseControllerTest에서 protected로 어드민 로그인과 유저 로그인 메서드를 만들고, 필드에 token을 두었어요.
인증이 필요한 테스트에서 token과 로그인 메서드를 사용하도록 변경했습니다!

Copy link

@choijy1705 choijy1705 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 7 to 10
@SpringBootTest(classes = TestConfig.class)
@Transactional
public abstract class BaseServiceTest {
}

Choose a reason for hiding this comment

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

추상 클래스를 만들어 상속하기보다는
test각각의 클래스에서 선언해주는게 좀 더 가독성이 좋지않을까 싶네요!

공통적인 로직같은 부분이 없이 단순히 transactional, springBootTest를 공통적으로 사용하기 위한 목적이라면 각각의 클래스에서 표현하는게 좋지 않을까요?

Copy link
Member Author

@alstn113 alstn113 May 11, 2024

Choose a reason for hiding this comment

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

사실 Controller, Service, Repository 등등 계층마다 추상 클래스를 만들고 붙이고 싶었어요.
확실하게 구분할 수도 있고, 찾으러 가기도 편해서요. 가독성 오히려 좋지 않나요?

현재는 불필요하니 제거하고 각각 클래스에 넣었습니다

ReservationTimeResponse timeResponse = reservationResponse.time();
ThemeResponse themeResponse = reservationResponse.theme();

SoftAssertions.assertSoftly(softly -> {

Choose a reason for hiding this comment

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

assertSoftly는 저는 처음보는데요
검색해보니 assertAll과 같은 기능을 하는것 같네요!
junit의 assertAll이 아닌 다른 라이브러리의 assertSoftly를 사용하신 이유가 궁금합니다!
(잘못되었다는게 아닌 진짜 궁금증입니다🙏)

Copy link
Member Author

Choose a reason for hiding this comment

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

첫 순서의 테스트가 실패하면 멈추는 assertThat과 달리 assertSoftly는 모든 테스트를 다 진행해줍니다.
asserj의 assertThat을 주로 사용하니 assertj의 assertSoftly로 맞추는 것도 좋다는 리뷰가 있더라구요.
실패해도 모든 테스트 결과를 보고 싶기도 하구요.

Choose a reason for hiding this comment

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

넵 참고로 assertAll도 모든 결과를 테스트 해볼수 있습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 assertSoftly에 대해 잘못 기억하고 있었네요.
assertAll과 달리 실패한 코드의 라인 위치까지 보여주는 것이었네요

https://zzang9ha.tistory.com/418


@TestFactory
@DisplayName("로그인, 로그인 상태 확인, 로그아웃을 한다.")
@Sql("/member.sql")

Choose a reason for hiding this comment

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

앞서 설명 드렸지만 sql로는 안전한 데이터가 저장된다고 볼 수 없을것 같아요!
DB에 제약조건을 건다고는 하지만 어디까지나 한계가 있고 (예를들면 숫자의 최대 최소값이 있는데 쿼리로 넣으면 그냥 들어가버리는) 이를 막기 위해서 실제 로직과 동일한 방법으로 데이터들을 초기화 해주는게 좋을것 같습니다

저의 회사의 경우는 coreTest라는 테스트 클래스가 있어서 모든 테스트 전에 초기화하는 작업들을 진행한뒤 테스트 코드를 수행하고 있습니다.

Choose a reason for hiding this comment

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

이 부분은 참고만 해주세요
반영을 하려면 꽤 많은 작업이 필요할것 같아요!

public class CookieUtil {

private static final String TOKEN = "token";
private static final int TOKEN_MAX_AGE = 60 * 60; // 1 hour

Choose a reason for hiding this comment

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

상수명이 maxAge보다는 oneHour에 초점이 맞춰져있으면 더 가독성 좋을것 같네요!

Copy link
Member Author

Choose a reason for hiding this comment

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

그렇게요. 수정했습니다!

Comment on lines 30 to 41
public LoginedMemberResponse createToken(LoginRequest loginRequest) {
Member member = memberRepository.getByEmail(loginRequest.email());

if (!passwordEncoder.matches(loginRequest.password(), member.getPassword())) {
throw new IllegalArgumentException("비밀번호가 일치하지 않습니다.");
}

String token = jwtTokenProvider.createToken(member.getId().toString());
MemberResponse memberResponse = MemberResponse.from(member);

return new LoginedMemberResponse(memberResponse, token);
}

Choose a reason for hiding this comment

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

하나의 메서드에서 여러개의 로직(조회와 토큰생성)을 수행하고 있네요
email로 조회하는 로직은 memberService를 통해 처리하는게 더 좋지않을까? 라는 생각이 드네요
createToken은 member를 파라미터로 받아서 토큰만 생성하고요

Copy link
Member Author

Choose a reason for hiding this comment

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

이메일로 찾고, 비밀번호 검증까지를 validatePassword로 묶고, 토큰 만드는 것을 따로 분리했습니다!
역할 분리에 대해서 신경써야겠다는 느낌이 드네요. 감사합니다!

Comment on lines 81 to 86
private ReservationResponse createReservation(
LocalDate date,
Long timeId,
Long themeId,
Long memberId
) {

Choose a reason for hiding this comment

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

이 메서드를 public으로 열고 호출하는 곳에서 얘를 호출하도록 하면 service의 가독성을 더 높일 수 있을것 같네요
admin 혹은 일반 Reservation이라는걸 reservationService가 알고 있을 필요도 없을것 같아요!

지금 방식대로 하신다면 add혹은 create 중 하나로 통일하는게 좋아보입니다

Copy link
Member Author

Choose a reason for hiding this comment

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

제가 원래 있던 파라미터를 고정시켜서 생각했던 것 같네요.

좋은 방법인 것 같아요!

@alstn113
Copy link
Member Author

원래 커스텀 예외를 만들어서 던지는 편이었습니다.
미션을 진행하면서 도메인에 표준 예외를 적용하다보니 서비스나 컨트롤러에서도 표준 예외가 많습니다.
GlobalExceptionHandler에서 IllegalArgumentException이나 NoSuchElementException 같은 표준 예외를 받아서 처리하고 있습니다.
근데 제가 모르는 부분에서 이런 에러들이 터지고, 예외 메세지가 그대로 노출될 수도 있을 것 같은데 어떻게 생각하시나요?
커스텀 예외를 만들어서 아는 부분에 대해서만 예외 처리를 하는 것이 좋을까요??

Copy link

@choijy1705 choijy1705 left a comment

Choose a reason for hiding this comment

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

구름 이번 미션도 고생많으셨습니다👍
질문에 대한 답변은 코멘트로 남겨뒀습니다!
다음 미션도 화이팅입니다!
머지할께요

ReservationTimeResponse timeResponse = reservationResponse.time();
ThemeResponse themeResponse = reservationResponse.theme();

SoftAssertions.assertSoftly(softly -> {

Choose a reason for hiding this comment

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

넵 참고로 assertAll도 모든 결과를 테스트 해볼수 있습니다

import org.springframework.web.bind.MethodArgumentNotValidException;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.RestControllerAdvice;
import org.springframework.web.context.request.WebRequest;
import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException;
import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;

@RestControllerAdvice

Choose a reason for hiding this comment

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

원래 커스텀 예외를 만들어서 던지는 편이었습니다.
미션을 진행하면서 도메인에 표준 예외를 적용하다보니 서비스나 컨트롤러에서도 표준 예외가 많습니다.
GlobalExceptionHandler에서 IllegalArgumentException이나 NoSuchElementException 같은 표준 예외를 받아서 처리하고 있습니다.
근데 제가 모르는 부분에서 이런 에러들이 터지고, 예외 메세지가 그대로 노출될 수도 있을 것 같은데 어떻게 생각하시나요?
커스텀 예외를 만들어서 아는 부분에 대해서만 예외 처리를 하는 것이 좋을까요??

저 같은 경우라면 모든 상황에서이 Exception을 정의하지 않을것 같습니다. 클라이언트에서 에러를 통해서 특별하게 무엇인가 처리하는 동작이 있는게 아니라면 범용적인 Exception하나만 만들고 모두 해당 Exception으로 처리하고 있습니다.

지금 ErrorResponse 클래스를 보면 단순히 message만 내려주고 있기 때문에 사실 CustomException을 만드는건 아무런 기능이 없을것 같습니다. custom exception을 클라이언트에게 전달하려면 적어도 ErrorResponse 클래스에 throwalbe을 필드를 같이 가지고 있어서 어떤 exception이 던져졌는지를 내려줄 수 있어야 할 것 같아요!

public class ErrorResponse {

    private String message;
    private Throwable exception;

    public ErrorResponse(String message) {
        this.message = message;
    }

    public ErrorResponse(String message, Throwable exception) {
        this.message = message;
        this.exception = exception;
    }
}

이렇게 내려줘야 클라이언트에서

{
    "message": "error message",
    "exception": "CustomException"
}

위 값을 받아서 특정 exception의 경우 핸들링할 수 있을것 같네요

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 참고로 assertAll도 모든 결과를 테스트 해볼수 있습니다

앗 assertSoftly에 대해 잘못 기억하고 있었네요.
assertAll과 달리 실패한 코드의 라인 위치까지 보여주는 것이었네요
https://zzang9ha.tistory.com/418

Comment on lines 1 to 6
SET referential_integrity FALSE;
TRUNCATE TABLE member RESTART IDENTITY;
TRUNCATE TABLE theme RESTART IDENTITY;
TRUNCATE TABLE reservation_time RESTART IDENTITY;
TRUNCATE TABLE reservation RESTART IDENTITY;
SET referential_integrity TRUE;

Choose a reason for hiding this comment

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

100개의 예약을 보여주는 것은 반복인데 반복을 테스트 할 필요가 있나요?🤔
한개의 예약을 원하는데로 잘 보여주면 99개는 테스트할 필요가 없을것 같아요 모든 상황에 대해서 테스트 하는게 테스트 코드의 작성 목적은 아니니까요!
설령 있다하더라도 sql로 데이터 셋을 만드는것보다
코드 반복문을 통해서 생성하는게 더 쉽고 안전할것 같아요

@choijy1705 choijy1705 merged commit 9a1d42b into woowacourse:alstn113 May 11, 2024
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

2 participants