-
Notifications
You must be signed in to change notification settings - Fork 0
[Step2] 로그인 기능을 구현한다. #3
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
Conversation
- 설정 값을 컨트롤러에서 변경하고, 이후 프로세스에서는 그 상태 값을 사용하는 것에 집중할 수 있도록 하기 위해 리팩토링
- 하나의 메서드가 너무 길기 때문에 파일의 양식을 알 수 있도록 파싱하는 메서드 분리 - URI Get 메서드를 Uri로 변경 - Url Get 메서드를 Uri로 변경
- 분기문 개행
- 문자열을 더하는 +를 가장 앞으로 보냄
- 분기분 개행
- Header에 쿠키를 넣기 위한 메서드 추가
- 분기문 개행
- Session, SessionManager 클래스 생성 - 로그인 시 쿠키를 내려주도록 응답 설정
- 테스트 메서드 접근 제어자 제거
- 메서드 네이밍 변경
- 값 객체가 아닌 문자열을 반환하도록 변경
- SocketChannel과 비동기를 활용하기 위해 파싱 부분 변경 - Request 생성자 변경 - Response 응답 양식 변경
- Application 생성자 조합으로 구동방식 변경
- 30분 -> 15분
- Session 체크 - 권한이 없을 경우 예외 처리 - 권한 관련 유틸 클래스 추가
- 쿠키가 제대로 파싱되지 않는 이슈 처리
- 응답 방식 변경 - 예외 발생 시 정적 리소스를 활용하도록 리팩토링
- 불필요한 로직 제거
- 세션이 있는 경우, 없는 경우 통합 테스트 작성
- equals 단위 테스트 추가
- 테스트 재활용성을 위해 Fixture 클래스 추가
- 인자를 받아서 검증
- 정적 코드 분석을 위한 Sonarqube 설정 추가/적용
| log.info("username: {}, password: {}", username, password); | ||
|
|
||
| validator.validateSignUp(username, password); | ||
| validator.validateLoginInfo(username, password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateSignUp에서 validateLoginInfo로 변경하신 이유가 뭔가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 제 실수 같아요. 원래대로 돌리겠습니다. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다시보니 둘 다 있네요 ㅎㅎ 이대로 두겠습니다~!
@Component
public class UserValidator {
public void validateSignUpInfo(
String username,
String password
) {
if (username == null || password == null) {
throw new InvalidParameterException(username, password);
}
if (username.isBlank() || password.isBlank()) {
throw new InvalidParameterException(username, password);
}
}
public void validateLoginInfo(
String username,
String password
) {
if (username == null || password == null) {
throw new InvalidParameterException(username, password);
}
if (username.isBlank() || password.isBlank()) {
throw new InvalidParameterException(username, password);
}
}
public void validateSessionId(Long userId) {
if (userId == null) {
throw new UnAuthorizedException();
}
}
}| public Map<String, String> getCookiesMap() { | ||
| return cookiesMap; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L43: this.cookiesMap
this키워드가 가독성에 도움을 준다고 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가독성은 잘 모르겠어요. 저도 이 부분이 일관성이 없을 때가 조금 있는데, 앞으로는 한 방식으로 통일하도록 해볼게요.
| private final List<String> cookies; | ||
| private final Map<String, String> cookiesMap; | ||
| private final Map<String, Cookie> cookiesMap; | ||
| public static final Cookies emptyCookies = new Cookies(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필드로 List cookies를 선언해놓으신 이유가 무엇인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 필요없는 것 같아요. 삭제하겠습니다. 👍
| void cookieGetValueTest() { | ||
| Cookies cookies = new Cookies(List.of("name=value")); | ||
| assertEquals("value", cookies.getValue("name")); | ||
| assertEquals("value", cookies.getValue("name").value()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 디미터의 법칙에 위반되는 구현 아닌가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 맞아요. 다만 디미터의 법칙을 깬다고 문제되진 않을 것 같아 사용했는데, cookies.getValue("name")을 하면 Cookie가 나오고, 해당 쿠키의 값을 반환하해요. 즉, 연관된 클래스의 연관된 값이 나오기 때문에 상관없다고 판단했습니다. 그래도 Cookie를 선언하고, 별도로 value를 하는 게 조금 더 좋아 보이긴 하네요. 반영하겠습니다. 👍
제가 이전에 MU님 코드에 디미터 법칙을 말했던 건, getDeclaringClass 메서드는 Clazz 타입을 반환하고, newInstance 메서드는 T 타입을 반환했기 때문이었어요. newInstance가 Exception을 선언해야 하기도 하고. 즉, 서로 연관된 문맥이 다르면 디미터의 법칙을 지키는 게 좋지만, 비슷한 문맥이라면 같이 쓰더라도 상관없다고 생각합니다. 이건 개인적 판단이라 MU가 판단해 주세요.
Class<?> clazz = User.class.getDeclaringClass( );
Object object = clazz.newInstance( );There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
답변 정정합니다. 디미터의 법칙을 어긴 것 같네요. 객체 내부의 구현이 노출된 것 같아, 다음과 같이 수정하겠습니다.
public class Cookies {
......
private final Map<String, Cookie> cookiesMap;
public static final Cookies emptyCookies = new Cookies();
......
public String getValue(String name) {
Cookie findCookie = this.cookiesMap.get(name);
if(findCookie != null) {
return findCookie.getValue( );
}
return null;
}
......
}| @DisplayName("사용자 이름이 Null 또는 빈 값이면 InvalidParameterException이 발생한다.") | ||
| void usernameNullOrBlank(String parameter) { | ||
| assertThatThrownBy(() -> validator.validateSignUp(parameter, "helloworld")) | ||
| assertThatThrownBy(() -> validator.validateSignUpInfo(parameter, "helloworld")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 명을 변경하신 이유가 뭔가요?
SignUpInfo가 기존 메서드보다 특별히 하는 일을 더 잘 나타낸다고 생각하시나요?이유가 뭔가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 제 실수 같아요. 반영하겠습니다. 👍
| assertAll( | ||
| () -> assertEquals(GET, requestLine.getHttpMethod()), | ||
| () -> assertEquals("/index.html", requestLine.getRequestUrl()), | ||
| () -> assertEquals("/index.html", requestLine.getRequestUri()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url -> uri로 변경하신 이유가 뭔가요?
url과 uri의 차이를 설명해주세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요청 라인에서 실제 위치를 반환할 수도, 식별자가 필요할 수도 있기 때문에 URI가 더 적절하다고 판단했습니다. URL은 실제 자원이 위치하는 경로를 말하고, URI는 자원의 위치 뿐 아니라 식별자도 나타냅니다. URI가 더 넓은 개념이고요!
|
|
||
| public class LoginUser { | ||
|
|
||
| private final Long userId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
userId가 Wrapper타입이여야 했던 특별한 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용자 아이디가 long이면 값이 없다면 0으로 초기화 될거에요. Long이면 값이 없다면 null 값으로 들어오며, 값이 없다는 것을 명시적으로 나타낼 수 있기 때문에 사용했습니다.
| String password | ||
| ) { | ||
| return new LoginUser(1L, password, ""); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
username자리에 password를 넣으시는 이유가 뭐죠? 의도하신게 맞나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 수정했는데 커밋에는 이렇게 남아있네요. 제 실수고, 변경됐습니다.
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(userId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순하게 userId를 리턴하는 거에 비해서 Objects.hash를 이용하는 것이 이점이 있을까요?
속도면에서도 hash collision을 줄인다는 면에서도 아무런 이점이 없다고 느껴져요.
어차피 userId는 유니크한 값이니까요. 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Objects.hash 메서드는 사실 IntelliJ가 정의해주는대로 사용하고 있었는데, 학습해보니 이는 아래 두 가지 장점이 있네요. 이를 사용한다고 해서 성능이 좋아지거나, 혹은 이미 유니크한 값을 더 좋은 방법으로 식별/판별할 것 같지는 않구요, Null-Safe, 다중 인자 지원 정도 때문에 사용하는 것 같습니다.
- Null-Safe
- 다중 인자 지원
kochungcheon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번 미션 고생 많으셨습니다!
|
|
||
| @Override | ||
| public Session createSession(Long userId) { | ||
| String uuid = randomUUID().toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
randomUUID에 대해서 설명해주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
또한 UUID 충돌에 대해 어떻게 생각하시나요
|
|
||
| public final class HeaderUtils { | ||
|
|
||
| private static final String SESSION_ID = "sessionId"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 좋네요!
|
|
||
| @Slf4j | ||
| @Service | ||
| public class UserLoginService implements UserLoginUseCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 해당 인터페이스에 대한 구현체는 한개이네요! 구현체가 한개일 것으로 예상될 때도 인터페이스 사용하는 게 좋을 지 의견을 듣고 싶습니다!
|
|
||
| private boolean isStaticResource(String[] startLineArray) { | ||
| return startLineArray.length >= 2; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static final 선언 기준이 궁금합니다! 2의 경우 왜 사용안하셨나요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요청라인을 파싱할 때, >=2 조건을 상수로 뽑아낼 때, 2를 어떻게 상수로 표현할지 안떠올랐습니다. 적절한 표현이 있을까요?
|
|
||
| public ModelMap addAttribute( | ||
| private final Map<String, Object> map = new LinkedHashMap(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LinkedHashMap을 사용하신 이유가 궁금합니다!
kochungcheon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
| ) { | ||
|
|
||
| public boolean isValid(LocalDateTime expiredAt) { | ||
| return this.expiredAt.isAfter(expiredAt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
세션 키가 만료되면 Map에 남아있게 되나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 구현에서는 세션이 만료되더라도 키가 Map에 남아있습니다. 간단하게 스케줄러를 사용해서 불필요한 세션을 주기적으로 제거해 주는 작업이 추가되면 좋을 것 같네요. 다음 PR 때 반영하겠습니다.
| this.loginIp = loginIp; | ||
| } | ||
|
|
||
| public LoginUser(Session session) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
세션으로 로그인을 구현한 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요구사항에 세션으로 로그인을 구현한다 는 조건이 있었습니다. 👀
| import java.time.LocalDateTime; | ||
| import java.util.Objects; | ||
|
|
||
| public record Session( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
세션과 쿠키의 차이점에 대해 설명해주세요!
📝 구현 내용
구현한 내용은 다음과 같습니다. 회원정보 상세보기는 아직 HTML 페이지가 완성되지 않아, 조금 더 다듬고 올리도록 하겠습니다.
✨ 로그인
세션은 애플리케이션 내부에 저장했으며, 이는 SessionManager를 통해 관리됩니다.
세션 만료 시간을 관리하는 요구사항이 있었기 때문에, 이는 세션 클래스 내부에 만료 시간을 두어 세션을 처리할 수 있도록 했습니다.
🔎 Nio2EndPoint
스프링에서 사용자 요청이 서블릿 컨테이너에 도달하기 전, 다음과 같은 과정을 거칩니다. 이는 비동기로 이루어지는데, 이전에 구현한 내용은 동기적으로 모든 것을 처리했습니다.
따라서 이를 개선하기 위해 다음과 같이 Channel, Selector 등 NIO의 구성요소를 이용해, 사용자 요청이 비동기적으로 처리되도록 바꾸고 있습니다. 이 부분은 어느 선까지 구현해야 할지, 아직 결정하지 못한 상태로, 미션을 진행하면서 조금씩 리팩토링할 예정입니다.
쓰레드 풀을 만들어 쓰레드를 재사용 했습니다. 다만, 이 부분에서 read/write 부분의 구현이 조금 아쉬운데,
read와 write가 한 메서드에서 이루어지기 때문입니다. 이 부분은 추후 시간이 된다면 수정할 예정입니다.