[Review] 스프링 카페 4단계 - 로그인 구현 (2022/03/15)

[Shine] 스프링 카페 4단계 - 로그인 구현 by zbqmgldjfh · Pull Request #73 · codesquad-members-2022/java-spring-cafe

우선 이번 리뷰를 해주신 우아한 형제들의 Roach에게 감사 인사를 전합니다!

 

1. 질문

질문 1) LoginControllerTest

LoginControllerTest 를 작성하는데 많은 어려움이 있었습니다. ㅠ,ㅠ....
LoginControllerTest 중심으로 리뷰해주시면 감사하겠습니다
Spring의 테스트 코드 작성이 아직은 미숙한것 같아 어렵네요 ㅠ,ㅠ

세션에 정상적으로 담겨있는지 테스트 하는 과정이 어렵다 생각됩니다.

@BeforeEach
public void setUp() {
    user = new User("Shine", "1234", "Shine", "Shine@gmail.com");
    user.setId(1L);

    mySession = new MockHttpSession();
}

@Test
@DisplayName("정상 로그인 테스트")
void loginSuccessTest() throws Exception {
    // given
    given(loginService.login(any(), any())).willReturn(user);

    // when
    ResultActions requestThenResult = mockMvc.perform(post("/login")
            .param("userId", "Shine")
            .param("password", "1234")
            .accept(MediaType.TEXT_HTML_VALUE)
            .session(mySession)
    );

    // then
    then(loginService).should(only()).login(any(), any());
    requestThenResult.andExpect(status().is3xxRedirection())
            .andExpect(redirectedUrl("/"))
            .andExpect(request().sessionAttribute("SESSIONED_USER", user));

    // 세션 반환 부분 꼭 질문하기! 첫 요청으로부터 세션이 정상적으로 생성됨을 어떻게 확인해야 하는가??
}

대표적으로 위와 같은 로그인 테스트를 작성해 보기는 했는데...

1. when 절에서 요청을 보낼때 session을 담아서 보내는것이 올바른 방식 인가요??
   이게 사실은 when에서 요청을 하면 스스로 session이 생겨야 하는데... 테스트 상에서 이걸 어떻게 해줘야 할지 모르겠습니다...
2. then에서 login메서드가 호출됬는지 확인하는것이 좋을까요?
3. andExpect에서 session에 회원정보가 잘 저장되는지 확인하고 싶어 다음과 같이 사용했는데 올바른 코드 일까요?

andExpect(request().sessionAttribute("SESSIONED_USER", user));

로그아웃 쪽 테스트 코드도 확인해주시면 감사하겠습니다!!

 

질문 2) Exception 처리하기

음.. 이부분에 서 확신이 안서 질문해 봅니다 ㅎㅎ!

1. catch에서는 예외를 외부로 다시 던지도록 하는것이 맞을까요?
2. 아니면 rollback을 실행하여 트랜잭션 처리를 하는것이 맞을까요?
3. 아니면 기본 값을 catch에서 반환하도록 하는것 어떨까요?
   raoch님이 말씀해주신 catch 문에서의 예외처리는 어떤 내용이셨을까요?
   확실하지 않아 재질문 해 봅니다!!

우선 저는 3번으로 기본값을 반환해주도록 변경하였습니다.

 

답변

예외 처리하는 방법에 대하여 복습좀 해야겠다... 예외처리 방식을 다 까먹어서 적용 못하고 있다... ExceptionHandler 뭐시기가 있었던거 같은데...

 

2. 코드 리뷰

1. 참신하다!

이거는 수정 사항은 아니고, 내가 생각해도 참신하게 해결한 부분이라. 남겨본다.

SQL을 yml파일로 관리하면서 외부로 부터 읽어오게 만들었다. 이때 사용하는 것 이 queryLoader이다!

 

- query.yml

# user
SAVE_USER: >
  INSERT INTO user_info (user_id, password, name, email) 
  VALUES (?, ?, ?, ?)

SELECT_USER: >
  SELECT id, user_id, password, name, email 
  FROM user_info 
  WHERE user_id = (?)

SELECT_USERS: >
  SELECT id, user_id, password, name, email 
  FROM user_info

UPDATE_USER: >
  UPDATE user_info SET user_id = (?), password = (?), name = (?), email = (?) 
  WHERE user_id = (?)

DELETE_USER: >
  DELETE FROM user_info
  WHERE user_id = (?)

# article
SAVE_ARTICLE: >
  INSERT INTO article (user_id, title, contents, local_date_time) 
  VALUES (?, ?, ?, ?)
SELECT_ARTICLE: >
  SELECT id, user_id, title, contents, local_date_time 
  FROM article 
  WHERE id = (?)

SELECT_ARTICLES: >
  SELECT id, user_id, title, contents, local_date_time 
  FROM article

UPDATE_ARTICLES: >
  UPDATE article SET user_id = (?), title = (?), contents = (?), local_date_time = (?) 
  WHERE id = (?)

DELETE_ARTICLE: >
  DELETE FROM article 
  WHERE id = (?)

이를 읽어오는 QueryLoader는 다음과 같다.

@Component
public class QueryLoader {

    private Map<String, String> loader = new HashMap<>();

    public QueryLoader() {
        try {
            ClassPathResource resource = new ClassPathResource("query.yml");
            loader = new Yaml().load(new InputStreamReader(resource.getInputStream()));
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public String get(Query query) {
        return loader.get(query.name());
    }
}

 

2. 이상한 null의 반환

음 이건 내가 생각해도 좀 웃겼닼ㅋㅋㅋㅋㅋ 나는 무슨 생각으로 짠 코드일까?ㅋㅋㅋㅋㅋ

아닠ㅋㅋ if에서 null이면 null을 반환한다닠ㅋㅋㅋ 그냥 바로 return하면 되는것 을 굳이 if문을 추가한 것 이다.

따라서 바로 return 하도록 수정하였다.

 

3. 어떤 Login을 사용해야 하는가?

우선 2개의 login 코드를 보면 다음과 같다.

public User login(String userId, String password) {
    return userRepository.findByUserId(userId)
            .filter(m -> m.isSamePassword(password))
            .orElse(null);
}

public User login(LoginDto dto) {
    return login(dto.getUserId(), dto.getPassword());
}

사실 원래의 의도는 API의 인자를 좀 더 다양하게 열어두고 싶어 둘다 public으로 해둔 것 이였다.
다만, 나의 나머지 코드에서는 dto를 통해서 받도록 하는 login만 사용하기 때문에 이것만 public으로 열어두는것이 적합하다 생각한다.

 

4. Validation 오류

사용자가 정보를 기입하지 않거나, 타입이 부적합 할 경우 BindingResult를 통해서 예외를 보여준다.

이때 해당 메서드가 사용자 폼에 접근하는 부분이라 403 에러를 보여주게 했었다.

 

하지만 사실은 Client가 애당초 올바르지 못한 요청을 한 것 이니?(예를 들어 비밀번호를 입력하지 않았다 거나??)
400 에러가 적합한 것 이였다. 따라서 400으로 변경하게 되었다!

 

5. Session 확인하기

사용자가 로그인 할때 서버 측 에서는 Session이 만들어지고 여기에 정보가 저장된다.

이때 나는 test로 Session 자체가 생성되는 것을 확인해야 된다고 생각했다.

 

하지만 Roach의 말을 듣고 나니, 어쩌면 불필요한 요소일수도 있겠구나라는 생각이 들었다.

 

우리가 핵심적으로 Test할 부분은 다음과 같다.

사용자가 로그인 성공 -> 세션에 저장되고 키값 반환

사용자가 로그인 실패 -> 세션에 저장되지 않는다.

 

즉 세션 생성 자체를 검증할 필요는 없는 부분인 것 이다.

 

비유를 좀 해보자면? 복권에 당첨되면 뭘 할지 생각해 보라 했는데, 복권 당첨시 당첨금 수령을 어떻게 할지를 고민하고 있는 것 이다.

당첨금을 어떻게 사용할지를 고민하랬더니, 돈 받을 수단을 생각했던 것 이다.

당첨시 돈은 자동적으로 다 받아온다고 당연시 생각 한 후, 당첨금을 어디에 사용할지 생각해야 한다.

 

- Roach

일단 SESSION 을 넣어야 하는가에 대한 테스트는 답변을 달기도 달았지만 세션은 정상적으로 생긴다고 테스트 하는게 좋아보여요. 그리고 로그 아웃의 경우 반드시 세션에 로그인 정보가 남아 있어야 하는데 만약 테스트 순서가 로그인이 먼져 되지 않는다면, 세션에 로그인 정보가 없어서 실패하겠죠? 테스트는 해당 테스트 안에서만 의존성을 가져야 하므로 넣는 방식으로 하는게 맞다고 생각해요.

 

6. @DisplayName 에 결과 포함하기

Roach 가 말씀해주신것 처럼 결과에 대한 내용도 포함되도록 작성해야겠다!