[Review] ATDD 3주차 3차 PR

권승철 리뷰어 님께 감사의 말을 전하고 싶다!!

 

1. 질문

1-1) Bearer 인증방식에서 권한이 없을 경우

public abstract class AuthenticationChainingFilter implements HandlerInterceptor {

    @Override
    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {
        if(isAlreadyLoginUser()) {
            return true;
        }

        try {
            AuthenticationToken token = convert(request);
            UserDetails userDetails = findUserDetails(token);
            afterAuthentication(userDetails);
            return true;
        } catch (UnauthorizedException e) {
            response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
            return false;
        } catch (AuthenticationException e) {
            return true;
        }
    }
    //... 생략
}

BearerTokenAuthenticationFilter 는 위 AuthenticationChainingFilter 를 상속받고 있습니다.
BearerTokenAuthenticationFilter 에서 전달받은 token을 검증하여 정상적인 token이 아닐경우 UnauthorizedException 예외를 발생시킵니다.

 

이때 401을 던져야 하는데, AuthenticationChainingFilter에서 UnauthorizedException를 catch해서 response에 응답코드 지정후, false를 반환시켜 더이상 Handler쪽으로의 진행을 막았는데, 적절한 방식일까요?

 

답변:

 

아! 그렇다! 정확한 리뷰이시다!!

난 인증 필터에서 왜 인가 처리를 생각하고 있었을까!...

 

사실 구현에 너무 치중해있었던 것 같다...

생각해보니 해당 필터는 2가지 이유 때문에 변경되고 있었다... 여기서 SRP 또한 의심했어야 했는데...

의식하지 못한점에 반성한다.

 

2. 리뷰 정리

2-1) JDK 1.4 의 assert 사용하기

지난 리뷰에서 Private 메서드 같은 경우 assert문을 활용하여 전달받는 인자를 검증하도록 하였습니다.

 

전달받는 인자를 검증한다는 내용은 같지만, 리뷰어 께서 말씀하신 내용은 JDK 1.4부터 추가된 assert를 의미하셨다.

따라서 이를 알맞게 수정하였다.

 

assert는 개발하는 동안은 실행시 -ea 옵션을 줘야 활성화가 된다.

출처 - https://velog.io/@jamie/assert

하지만 배포할때는 -ea 옵션을 빼기 때문에 바이너리코드 상에는 포함이 되지 않는다는 장점이 있다!

 

https://offbyone.tistory.com/294

Java에서 assert 사용하기

 

2-2) Delete 의 멱등성

HTTP를 공부하면서 Delete가 멱등하다는 내용을 공부했던적이 있다.

다만 해당 내용을 구현에 녹여내지 못하고 있었다!

 

최근 받은 리뷰중 가장 많은 생각이 드는 리뷰 였다.

당연히 처리해줘야 하는 예외처리가, 메서드의 멱등성을 무시하는 행동이였다니...

예외 처리에 따른 부차적인 부분을 전혀 생각하지 못한것 같다.

 

삭제를 위해서는 우선 해당 favorite가 있는지 찾아오는데, 이때 ExceptionHandler에서 NoContent를 반환하도록 하였다.

@RestControllerAdvice(assignableTypes = {FavoriteController.class})
public class FavoriteExceptionHandler {

    @ExceptionHandler({NotFoundFavoriteException.class, NoSuchElementException.class})
    public ResponseEntity<ErrorResponse> notFoundExceptionHandler(Exception e) {
        return ResponseEntity.noContent().build();
    }
}

 

아직 의문이 한가지 든다.

 

나의 코드를 보면 findMemberByPrincipal()에서 member가 없다면 예외를 던지는데,
다른 도메인 쪽에서는 ExceptionHandler에서 ErrorResponse를 보내고 있습니다.

 

하지만 저는 findMemberByPrincipal() 에서 발생하는 NoSuchElementException예외는 특별히 noContent를 동일하게 반환하고 싶습니다.

 

멱등성을 위해서 이죠!

 

이를 위해서 (assignableTypes = {FavoriteController.class}) 로 controllerAdivce에 컨트롤러를 지정해 놨는데,
혹시 해당 NoSuchElementException 부분만 특정 잡을 더 좋은방법이 있을까요?

 

2-3) Member에서 favorite 관리하기

 

음 원래는 둘간에 연관관계를 만들지 않고 그냥 favorite에서 Member의 id만 가지고 있도록 했었다.

하지만 리뷰어의 말을 듣고 나니, 1:N 관계로 설정하고, Member가 에그리게이트 root 역할을 하면 될것같다는 생각이 들었다.

 

따라서 코드를 다음과 같이 수정하게 되었다.

@Entity
public class Favorite {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;

    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "member_id")
    private Member member;

    // 일부 생략
}

@Entity
public class Member {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private Long id;

    @OneToMany(mappedBy = "member", cascade = CascadeType.PERSIST, orphanRemoval = true)
    private List<Favorite> favorites = new ArrayList<>();
}