Level2. atdd-subway-fare 정리

2021-06-16글

요금 정책

• 로그인 사용자와 비로그인 사용자를 구별하여 요금 정책을 적용
• PathController에서는 토큰이 없어도 예외가 아니었고
• MemberController에서는 토큰이 없으면 예외였기에 Resolver에서는 각각에 맞는 처리를 해주어야 했음
• 맨 처음 페어랑 기능만 돌아가게 구현했을 때는 기존에 있는 로그인된 사용자에 대한 인가 과정을 거치는 Resolver를 재사용하도록 구현
• 이러기 위해서는 이 각각에 맞는 처리를 일간 Controller로 옮겨야 했음

AuthenticationPrincipalArgumentResolver

@Override
public Object resolveArgument(MethodParameter parameter, ModelAndViewContainer mavContainer, NativeWebRequest webRequest, WebDataBinderFactory binderFactory) {
    String credentials = AuthorizationExtractor.extract(webRequest.getNativeRequest(HttpServletRequest.class));
    LoginMember member = authService.findMemberByToken(credentials);
    if (member.getId() == null) {
        throw new AuthorizationException();
    }
    return member;
}

MemberController에서 LoginMember에 대한 처리

private void unAuthorizedThrowException(@AuthenticationPrincipal LoginMember loginMember) {
    if (loginMember.getId() == null) {
        throw new AuthorizationException();
    }
}

• id가 없으면 (존재하지 않는 사용자이면) 예외

PathController에서 LoginMember에 대한 처리

private LoginMember unLoginMemberWrapper(@AuthenticationPrincipal LoginMember loginMember) {
  if (loginMember.getId() == null) {
    return new LoginMember(null, null, -1);
  }
  return loginMember;
}

• id가 존재하지 않으면 age를 -1 로 wrap

그런데 이렇게 구현하고 생각한 문제점이...

• Controller에서 이미 리졸빙된 LoginMember에 대한 처리를 또 건드리고 있다는 느낌
• Controller에서 LoginMember에 대한 비즈니스 로직을 알고 있는 느낌이라 개선이 필요하다고 느낌
• age를 -1로 감싸서 이 경우 비로그인자로 가정하는 로직이 괜찮은지 의문을 품고 있었음

페어의 리뷰어에게 온 피드백

• 일단 AuthenticationPrincipalArgumentResolver 와 AgeAuthenticationPrincipalArgumentResolver 두개로 분리하였음
• AgeAuthenticationPrincipalArgumentResolver 에서는 요금 정책에 필요한 값인 age 만 반환하도록 리팩토링

AgeAuthenticationPrincipalArgumentResolver

public class AgeAuthenticationPrincipalArgumentResolver implements HandlerMethodArgumentResolver {
    private AuthService authService;

    public AgeAuthenticationPrincipalArgumentResolver(AuthService authService) {
        this.authService = authService;
    }

    @Override
    public boolean supportsParameter(MethodParameter parameter) {
        return parameter.hasParameterAnnotation(AgeAuthenticationPrincipal.class);
    }

    @Override
    public Object resolveArgument(MethodParameter parameter, ModelAndViewContainer mavContainer, NativeWebRequest webRequest, WebDataBinderFactory binderFactory) {
        String credentials = AuthorizationExtractor.extract(Objects.requireNonNull(webRequest.getNativeRequest(HttpServletRequest.class)));
        LoginMember memberByToken = authService.findMemberByToken(credentials);
        return memberByToken.getAge();
    }
}

• Integer 형인 age를 반환하도록....
  • 반환타입을 명시적으로 적어줄걸 그랬네

PathService의 findPath()

public PathResponse findPath(Integer age, Long source, Long target) {
    try {
        List<Line> lines = lineService.findLines();
        Station sourceStation = stationService.findStationById(source);
        Station targetStation = stationService.findStationById(target);
        SubwayPath subwayPath = pathFinder.findPath(lines, sourceStation, targetStation);
        int distance = subwayPath.calculateDistance();
        int lineFare = subwayPath.getMaxLineFare();
        SubwayPathFare subwayPathFare = new SubwayPathFare(age, distance, lineFare);
        return PathResponseAssembler.assemble(subwayPath, subwayPathFare);
    } catch (Exception e) {
        throw new InvalidPathException();
    }
}

SubwayPathFare

public class SubwayPathFare {
    private final Integer age;
    private final int distance;
    private final int lineFare;

    public SubwayPathFare(Integer age, int distance, int lineFare) {
        this.age = wrapAge(age);
        this.distance = distance;
        this.lineFare = lineFare;
    }

    private int wrapAge(Integer age) {
        if (age == null) {
            return -1;
        }
        return age;
    }

    public int getFare() {
        int fareByDistance = FareCalculatorByDistance.from(distance);
        return FareAdjusterByAge.of(age, fareByDistance, lineFare);
    }
}

• 요금 정책을 해당 도메인에서 Enum인 FareCalculatorByDistance 와 FareAdjusterByAge 를 통해 계산하고 있었음
• wrapAge() 를 통해 null 값으로 age가 들어오면 FareAdjusterByAge.of()에서 스트림을 돌리는데, 이에 예외가 발생하지 않도록 -1로 포장

참고 - FareAdjusterByAge.of()

public static int of(int age, int fare, int lineFare) {
    return Arrays.stream(values())
            .filter(ageRange -> ageRange.minInclusive <= age && age < ageRange.maxExclusive)
            .map(calculator -> calculator.adjuster.apply(fare, lineFare))
            .findAny()
            .orElse(fare + lineFare);
}

FareCalculatorByDistance - 거리별 요금 정책

public enum FareCalculatorByDistance {
    BASE(0, Constants.BASE_MAX_BOUNDARY, distance -> Constants.BASE_FARE),
    FIRST_ADDITIONAL(Constants.BASE_MAX_BOUNDARY, Constants.FIRST_ADDITIONAL_MAX_BOUNDARY,
            (distance) -> Constants.BASE_FARE + (int) ((Math.ceil((distance - Constants.BASE_MAX_BOUNDARY - 1) / 5)) + 1) * Constants.EXTRA_FARE),
    SECOND_ADDITIONAL(Constants.FIRST_ADDITIONAL_MAX_BOUNDARY, Integer.MAX_VALUE,
            (distance) -> Constants.BASE_FARE + 800 + (int) ((Math.ceil((distance - Constants.FIRST_ADDITIONAL_MAX_BOUNDARY - 1) / 8)) + 1) * Constants.EXTRA_FARE);


    private final int minExclusive;
    private final int maxInclusive;
    private final UnaryOperator<Integer> calculator;

    FareCalculatorByDistance(int minExclusive, int maxInclusive, UnaryOperator<Integer> calculator) {
        this.minExclusive = minExclusive;
        this.maxInclusive = maxInclusive;
        this.calculator = calculator;
    }

FareAdjusterByAge - 나이별 요금 정책

public enum FareAdjusterByAge {    PRE_SCHOOLED(0, Constants.PRE_SCHOOL_MAX_BOUNDARY, getIntegerBinaryOperator(0)),    SCHOOL_AGED(Constants.PRE_SCHOOL_MAX_BOUNDARY, Constants.SCHOOL_AGED_MAX_BOUNDARY, getIntegerBinaryOperator(Constants.SCHOOL_AGED_RATE)),    ADOLESCENT(Constants.SCHOOL_AGED_MAX_BOUNDARY, Constants.ADOLESCENT_MAX_BOUNDARY, getIntegerBinaryOperator(Constants.ADOLESCENT_RATE));    private final int minInclusive;    private final int maxExclusive;    private final BinaryOperator<Integer> adjuster;    FareAdjusterByAge(int minInclusive, int maxExclusive, BinaryOperator<Integer> adjuster) {        this.minInclusive = minInclusive;        this.maxExclusive = maxExclusive;        this.adjuster = adjuster;    }    private static BinaryOperator<Integer> getIntegerBinaryOperator(double rate) {        return (fare, lineFare) -> (int) (((fare + lineFare) - 350) * rate);    }

리뷰어에게 받은 피드백

• 리뷰어의 의도는 Member를 나타내는 도메인에서 age를 nullable하게 두고 이를 처리하는 것이 더 명확할 것 같다하심

아쉬운 점

나이별 요금정책을 적용할 때 Member 도메인을 사용해볼걸!

현재 경로 조회를 할 때는 member 도메인이 넘어오지 않고 age 값만 넘어오도록 구현했었다.
그런데 Age는 Member에 종속되는 필드인데 이를 빼서 독립적으로 처리하고 있는게 지금보니 마음에 들지 않는다.
만약 Member의 다른 필드들도 요금정책에 영행을 주게 된다면 유지보수가 어려운 코드가 된다.
크루들이랑 이야기를 해보니 한 리졸버에서 Member를 반환해주는데, 이 반환 도메인을 한 인터페이스(예를 들면 Member)를 만들고 이를 구현한 Guest, LoginMember 로 로그인, 비로그인 사용자를 구별하여 로직을 수행하는 방법도 있다고 한다.

이러면 리졸버를 나눌 필요도 없고 Member가 age를 가지고 나이에 관한 요금 정책을 수행할 때 해당 도메인에게 물어서 처리할 수 있을 것 같다.

Enum에 Predicate를 가직 할걸!

또한 각각 요금 정책을 Enum으로 구현하였는데, 여기서 상수를 쓰니 가독성이 매우 떨어지는 느낌이다.

상수를 쓴 이유는 위의 피드백으로 매직 넘버에 의미를 부여하고 싶었으나...
이렇게 하니 오히려 가독성을 헤치는 느낌이었고 마지막 리뷰대로 Enum 안에 범위를 판단하는 Predicate 를 가지도록 할걸 그랬다.

프론트엔드와의 협업

• 백엔드 4, 프론트엔드 2명 정도씩 팀을 이루어 협업하는 미션이었음
• 해당 문서 에 있는 프론트엔드 요구사항을 뼈대코드에 반영해 나감

API 문서화

• Spring Rest Docs를 통해 API 문서화 작업을 진행
• 기존에 있는 RestAssured 테스트를 이용해 API를 만들수도 있는데, 페어랑 같이 시도하다가 계속 오류나서....
• 결국 MockMvc로 테스트 코드 다시짜서 API 문서를 만들었음
• 다른 크루가 RestAssured로 문서를 만들었던데 그거 보고 시도해봐야지...
• 또 리뷰어가 제시한 방향으로 API 문서 만드는 테스트 코드를 조금 수정했더니 더 좋은 문서를 만들 수 있었음

ControllerTest

@ActiveProfiles("test")
@AutoConfigureRestDocs
public class ControllerTest {
    @Autowired
    public MockMvc mockMvc;

    @Autowired
    public ObjectMapper objectMapper;

    protected static OperationRequestPreprocessor getDocumentRequest() {
        return preprocessRequest(
                modifyUris()
                        .scheme("https")
                        .host("newwisdom-subway.p-e.kr")
                        .removePort(),
                prettyPrint());
    }

    protected static OperationResponsePreprocessor getDocumentResponse() {
        return preprocessResponse(prettyPrint());
    }
}

• API 문서 만드는데 필요한 MockMvc 테스트들은 위 클래스를 상속받고 있음
• 리뷰어의 제안을 통해 getDocumentRequest()와 getDocumentResponse() 을 추가

테스트 코드의 가독성

• 이번 리뷰어가 테스트 코드에서 가독성을 지키지 못한 부분들을 많이 피드백 해주었음
• 테스트 코드도 하나의 문서이며 유지보수 대상이기 때문에 가독성을 고려해야 한다는 것을 느꼈음
• 서로 다른 테스트에서 공통으로 쓰이는 메서드들은 상위 클래스인 AcceptanceTest로 끌어 올림
• 테스트에 쓰이는 변수명들도 맥락을 파악하기 쉽도록 변경
• 상수처리도 신경씀

예외 처리

• "일단 프론트의 요구사항 다 반영하도록 구현해보자!"로 잡고 했더니 무수한 커스텀 예외가 만들어졌다...^ㅠ^
• 이를 개선할 방법을 찾다가... 크루들한테 들은 Spring Guide - Exception 전략 을 적용해보기로~
• RuntimeException을 상속받는 BuisnessException을 만들고 필드에 상태코드와 ErrorMessage(DTO)를 가지도록 구현
• 이를 상속하는 상태코드 400인 BusinessException과 상태코드 401인 AuthorizationException을 만듦
• 또 이를 상속받는 각각의 클라이언트의 요구사항에 맞는 커스텀 예외 만듦
• ControllerAdvice에서는 다음과 같이 BuisnessException을 잡고 이 예외에서 상태코드와 예외 메시지를 꺼내도록 구현
• 이렇게 되면 어드바이스에서 수많은 커스텀 예외들을 BuisnessException 하나만 잡고 처리할 수 있음
• 상태코드에 따른 클래스를 두어 이가 변경되어도 유지보수가 쉬움

@ExceptionHandler(BusinessException.class)public ResponseEntity<ErrorMessage> handleRuntimeException(BusinessException e) {  return ResponseEntity.status(e.getHttpStatus()).body(e.getErrorMessage());}

BusinessException

public class BusinessException extends RuntimeException {
    private final HttpStatus httpStatus;
    private final ErrorMessage errorMessage;

    public BusinessException(HttpStatus httpStatus, String errorMessage) {
        this(httpStatus, new ErrorMessage(errorMessage));
    }

    public BusinessException(HttpStatus httpStatus, ErrorMessage errorMessage) {
        this.httpStatus = httpStatus;
        this.errorMessage = errorMessage;
    }

    public HttpStatus getHttpStatus() {
        return httpStatus;
    }

    public ErrorMessage getErrorMessage() {
        return errorMessage;
    }
}

AuthorizationException

public class AuthorizationException extends BusinessException {
    public AuthorizationException(String message) {
        super(HttpStatus.UNAUTHORIZED, message);
    }
}

• BusinessException을 상속
• 401 상태코드

BadRequestException

public class BadRequestException extends BusinessException {
    public BadRequestException(String message) {
        super(HttpStatus.BAD_REQUEST, message);
    }
}

• BusinessException을 상속
• 400 상태코드

배포 스크립트

• 실제 API 사용이 가능하도록 EC2에 배포하여야 했음
• 프로젝트가 업데이트되면 프로세스를 재시동해야했음
• 이러면 여러가지 명령어를 실행해야 함....
• 이를 스크립트로 만들어 한번에 처리하도록 구현!

deploy-prod.sh

#!/bin/bash
#!/bin/bash in the top of your scripts then you are telling your system to use bash as a default shell.

REPOSITORY=/home/ubuntu/ # 경로 설정
PROJECT_NAME=atdd-subway-fare

cd $REPOSITORY/$PROJECT_NAME/ # 경로로 접근

echo "> git reset --hard" 

git reset --hard # 깃허브 초기화

echo "> git pull origin step1"

git pull origin step1 # pull 땡겨오기

echo "> 프로젝트 Build 시작"

./gradlew clean build # 빌드

echo "> Build 파일 경로 복사"

JAR_LOCATION=$(find ./* -name "*jar" | grep atdd-subway-fare)

echo "> 현재 구동중인 애플리케이션 pid 확인"

CURRENT_PID=$(pgrep -f ${PROJECT_NAME}[-A-z0-9.]*.jar$) # 실행시켜져있는 jar pid 받기 


if [ -z "$CURRENT_PID" ]; then # -z 플래그는 null인것을 체크함, PID가 null인 경우 if절 안으로 들어감
    echo "> 현재 구동 중인 애플리케이션이 없으므로 종료하지 않습니다."
else
    echo "> 현재 구동중인 애플리케이션 종료(pid : $CURRENT_PID)"
    echo "> kill -15 $CURRENT_PID"
    kill -15 $CURRENT_PID
    sleep 5
fi

echo "> 새 애플리케이션 배포"
echo "> JAR Location: $JAR_LOCATION" 해당 jar파일 실행

#nohup java -jar 실행
nohup java -jar -Dspring.profiles.active=prod ${JAR_LOCATION} 1> log-prod.md 2>&1  &