[Review] 2022/06/04 4차 PR (Aribnb 서비스)

이번 리뷰는 Dion 께서 해주셨다! 리뷰해주셔서 감사합니다 !!

https://github.com/codesquad-members-2022/airbnb/pull/203#discussion_r891396646

[Team - 06][BE] 에어비앤비 프로젝트 2주차 2회차 PR by zbqmgldjfh · Pull Request #203 · codesquad-members-2022/air

1. 질문

1-1) URL의 설계

개발을 진행하던 중 URL설계에 대해 고민이 있었습니다.

바로 방을 예약하는 controller들을 호출하는 URL 부분인데요,

/api/users/reservation

vs

/api/house/reservation

위 2개의 URL 중 고민이 많았습니다.

해당 방을 예약하는 것 이니,(1) /api/houser/{houseId}/reservation 과 같이 되야할 것 같고. Resource 에 대하야 표현되어 있으니 이방식이 더 적합할것 같다 생각하는데

아니면 유저가 방을 등록하는 것 이니 (2) /api/users/reservation 으로 진행되야 할것 같기도 하고...

Dion 께서는 어느 URL 이 더 적합하다 생각하시나요?

 

1-2) oauth URL

다음 두개의 URL 에서 고민이 있었습니다.
/api/login/oauth/callback/{vendor} 와 /api/login/oauth/{vendor}/callback 중 후자가 더 적합하다 생각하기는 하는데,
왜냐하면 해당 vendor에 종속되는 callback 이라는 resource로 인식하였기 때문입니다!
혹시 디온이라면 어느쪽을 택하실까요?

 

답변

질문 순서도 조심하자! 읽는 사람이 읽기 편헤야 한다!

 

2. 코드리뷰

2-1) Optional이 필요한 상황인가?

위와 같은 리뷰를 받게 되었다.

해당 resolveToken의 코드는 다음과 같다.

private Optional<String> resolveToken(HttpServletRequest request) {

    String authorizationInfo = request.getHeader("Authorization");
    
    if (authorizationInfo == null) {
        return Optional.empty();
    }
    
    String[] parts = authorizationInfo.split(" ");
    
    if (parts.length == 2 && parts[0].equals("Bearer")) {
        return Optional.ofNullable(parts[1]);
    }
    return Optional.empty();
}

잘 보면 길이가 2인지 검증을 한후, 1번 part를 반환한다.

만약 길이가 2가 아니라면, Optonal.empty()를 보내게 된다.

 

원래 Optional은 null을 반환할만한 곳에서 null safe함을 보장하기 위해 한번 wrapping하여 반환하는 것 이다.

여기서 스스로의 의문은, 굳이 Null을 감싸서 반환할 필요가 있는가? 그냥 예외를 발생시키면 되지 않을까? 였습니다.

 

따라서 다음과 같이 변경하게 되었습니다.

private String resolveToken(HttpServletRequest request) {
    String authorizationInfo = request.getHeader("Authorization");
    if (authorizationInfo == null) {
        throw new IllegalStateException("토큰이 없습니다. 로그인 먼저 해주세요.");
    }
    String[] parts = authorizationInfo.split(" ");
    if (isInvalidToken(parts)) {
        throw new IllegalStateException("정상적인 형태로 토큰을 전달해주세요.");
    }
    return parts[1];
}

private boolean isInvalidToken(String[] parts) {
    return parts.length != 2 || !parts[0].equals("Bearer");
}

만약 parts[1]이 없다면 그냥 예외를 던지게 될것 입니다.

 

2-2) 로그를 좀더 명확하게 남기기

 

우리가 남긴 로그 정보가 부족했던것 같다.

대표적으로 log level을 info 로 하였는데, 이를 debug 용으로 만들었으면 더 좋지 않을까 싶다?

또한애당초 저 부분의 log가 단순 Controller 호출 확인용인데, 해당 로그가 필요한지부터 의문이다?

 

2-3) 상수를 상수답게

 

전혀 의식 못한 부분인데... static 키워드가 빠져 있었다... 

따라서 static 을 추가해줄까 하다가, 그냥 yml 자체로 설정 부분을 추출하도록 변경하였다!

 

2-4) 생성자 주입

다음 리뷰를 보면 "당연히 생성자 주입해야 하는거 아냐?" 라고 생각하실수도 있지만, 나름 그럴만한 이유가 있었다...

우선 RestTemplate()를 빈으로 만들지 않는 이유는 해당 Class에서만 딱 사용하고 소멸시키고 싶었기 때문이다.

 

해당 Class 말고는 사용하지 않는 객체를 굳이 Bean으로 등록시켜야 할까?

 

더 나아가 큰 문제가 있다. 우선 다음 코드를 살펴보자.

@Override
public UserProfileDto getUserProfile(OauthToken oAuthToken) {
    HttpHeaders headers = createHeaders(oAuthToken.getAccessTokenHeader()); // 헤더 만들기

    HttpEntity<MultiValueMap<String, String>> accessProfileRequest = new HttpEntity<>(headers);
    ResponseEntity<String> response = restTemplate.exchange( // 사용자 정보 반환받기
            KAKAO_OAUTH_SERVER_URI,
            HttpMethod.POST,
            accessProfileRequest,
            String.class
    );

    objectMapper.setPropertyNamingStrategy(PropertyNamingStrategy.SNAKE_CASE);// 이곳!!!
    KakaoProfile kakaoProfile;
    try {
        kakaoProfile = objectMapper.readValue(response.getBody(), KakaoProfile.class);
    } catch (JsonProcessingException e) {
        throw new IllegalStateException("변환할 수 없는 User 입니다.");
    }

    Long id = kakaoProfile.getId();
    String email = kakaoProfile.kakaoAccount.getEmail();
    String username = kakaoProfile.kakaoAccount.profile.getNickname();
    return new UserProfileDto(id, email, username);
}

PropertyNamingStrategy.SNAKE_CASE 부분이 있는데,

Bean으로 만들어서 SNAKE_CASE를 적용시켜 버리면 예상치 못한곳에서 Object Mapping 되지 않아 문제가 생긴다.

 

두번째 방식으로는 yml 설정파일에서 따로 전략을 설정해 줬지만, 이또한 예상치 못한곳 까지 문제가 전파되었다.

 

전역으로 설정이 걸리는것이 문제였다!

 

아직 이부분을 해결하지 못하고 있다.... Bean Scope를 이용하면 될려나?... 리뷰어님께 물어봐야 겠다...

 

2-5) 사용자 Exception

사실 저같은 경우... 이미 있는 Exception을 잘 활용하는 편이 더 좋다는 개인적인 생각을 가지고 있습니다.
따라서 보통 프로젝트를 만들때 따로 예외 부분을 만들지 않고 개발해 왔습니다.

 

하지만 팀원들의 코드를 살펴볼때 Exception을 따로 정의해주는 팀원들이 많았습니다.

예로 Miller 라는 친구의 코드를 보면 상태 코드도 따로 정의 하여 사용하더군요. 다음과 같이 말이죠!

@Getter
public enum ErrorCode {
    MEMBER_NOT_FOUND(HttpStatus.NOT_FOUND, "멤버 정보가 존재하지 않습니다."),
    ROOM_NOT_FOUND(HttpStatus.NOT_FOUND, "숙소 정보가 존재하지 않습니다."),
    RESERVATION_NOT_FOUND(HttpStatus.NOT_FOUND, "예약 정보가 존재하지 않습니다."),
    WISH_NOT_FOUND(HttpStatus.NOT_FOUND, "위시리스트 정보가 존재하지 않습니다."),
    TOKEN_NOT_FOUND(HttpStatus.NOT_FOUND, "토큰 정보가 존재하지 않습니다."),
    GUEST_NOT_AVAILABLE(HttpStatus.NOT_ACCEPTABLE, "해당 인원으로 예약을 할 수 없습니다."),
    DATE_NOT_AVAILABLE(HttpStatus.NOT_ACCEPTABLE, "해당 날짜에 예약을 할 수 없습니다."),
    MEMBER_NOT_IDENTIFIED(HttpStatus.NOT_ACCEPTABLE, "멤버 정보가 일치하지 않습니다."),
    RESERVATION_NOT_CANCELED(HttpStatus.NOT_ACCEPTABLE, "예약된 상태가 아니므로 취소할 수 없습니다."),
    INTERNAL_ERROR(HttpStatus.INTERNAL_SERVER_ERROR, "서버 내부 오류가 발생했습니다.");

    private final HttpStatus status;
    private final String message;

    ErrorCode(HttpStatus status, String message) {
        this.status = status;
        this.message = message;
    }

}

이런 부분이 좋은지는 사실 아직 주니어인 저의 입장에서 판단이 잘 되지 않습니다.

다만 사용해본적 없던 방식이라 위와 같은 부분들이 매력적으로 느껴지더군요!

 

이후 Exception에서 다음과 같이 사용하더군요!

@Getter
public class BusinessException extends RuntimeException {

    private final ErrorCode errorCode;

    public BusinessException(ErrorCode errorCode) {
        super(errorCode.getMessage());
        this.errorCode = errorCode;
    }

    public BusinessException(ErrorCode errorCode, Exception e) {
        super(errorCode.getMessage(), e);
        this.errorCode = errorCode;
    }
}

다음 프로젝트에서는 저도 이러한 방식을 적용해보면서 장단점을 직접 경험해봐야 할 것 같습니다~~

 

2-6) 이름 설정 전략?

이는 맨 위에서도 언급했었던 부분이다.

yml 설정 방식, ObjectMapper를 생성하고 설정하는 방식 모두 Controller에서 @RequestParam으로 인자를 받을 때 문제가 발생하였다....

 

2-7) 동시성 이슈

아마 HashMap 부분에서 동시성 문제가 발생할 수 있을것 같습니다!

 

일단 LoginService 자체가 @Service가 달려있으니, 메모리 상에 로딩되는 Singleton bean입니다.

즉, 동일한 Singleton bean 에 여러 사용자가 접근할수 있으며 -> 그 안에있는 oAuthServerMap 또한 여러 사용자가 동시에 접근할 수 있습니다.

 

문제는 map.put("key", "Shine")을 하는 동안 HashMap의 내부에서는 많은 일이 발생한다는 점 일것입니다!

예를 들어서 map의 노드를 생성하고, 저장 위치를 찾고, 기존 데이터를 최적화 하는 등등 복잡한 알고리즘을 과정이 진행됩니다.

이런 과정을 진행중인데 누군가 map.get("key")을 동시에 호출하게 되면 아직 알고리즘이 완성되지 않은 상태에서 get()을 호출하게 될 수 있고, 이 경우 메모리 누수나 엉뚱한 데이터가 조회되는 문제가 발생할 수 있습니다.

 

즉, put()이 아직 완성되지 않은 상태에서 get()이 호출되버리는, 일종의 race condition이 생겨버리는 것 이죠

이러한 부분 때문에 ConcurrentHashMap 으로 변경해야 할것 같습니다.

 

이렇게 리뷰에게 질문을 다시 남겼는데, 답변을 다음과 같이 주셨다!

우리는 Map에서 해당 원소를 읽어오기만 할것 이다? 

읽기 전용이라면 HashMap 또한 문제가 없을것이다!