-
Notifications
You must be signed in to change notification settings - Fork 90
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
[범블비] 3단계 - HTTP 웹 서버 구현 미션 제출합니다. #199
base: ddaaac
Are you sure you want to change the base?
Conversation
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.
범블비 안녕하세요!
로그인 여부 체크를 AuthFilter로 구현 한 부분이 인상 깊네요 :)
간단한 피드백 몇개 남겼어요. 확인 후 반영 해 주세요!
import java.util.stream.Stream; | ||
|
||
public class HttpSessionStorage { | ||
private static Map<UUID, HttpSession> sessions = new HashMap<>(); |
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.
sessions 에는 여러 Request Thread가 동시에 접근 가능하기 때문에
HashMap
보단 ConcurrentHashMap
이 Thread Safe 할 것 같아요 :)
public void invalidate() { | ||
attributes.clear(); | ||
} |
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.
session의 invalidate 메소드가 호출된 후에는 해당 세션은 사용하지 못하는 것으로 알고 있어요.
아래 링크 한번 읽어보시고, 한번 검색해보시면 좋을 것 같아요 :)
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.
그 부분까지는 미처 고려하지 못했네요 👍
감사합니다 :D
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
public class AuthFilter implements Filter { |
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.
filter로 처리한 점 👏👏👏👏👏
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 HttpSession { | ||
private UUID id = UUID.randomUUID(); | ||
private boolean isValid = true; | ||
private Map<String, Object> attributes = new HashMap<>(); |
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.
유저가 로그인 된 상태에서 클라이언트가 api를 병렬로 요청한다면 attributes에도 동시성 이슈가 발생 할 가능성이 있을 것 같아요 :)
public void setAttribute(String name, Object value) { | ||
attributes.put(name, value); | ||
} | ||
|
||
public Object getAttribute(String name) { | ||
return attributes.get(name); | ||
} | ||
|
||
public void removeAttribute(String name) { | ||
attributes.remove(name); | ||
} |
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.
isValid 메소드가 있긴 하지만 isValid메소드로 확인하지 않는 경로로 해당 Session을 얻었다면 기존대로 사용 가능 할 것 같아요.
각 메소드 마다 valid 체크를 한 후 오류를 던져주도록 하면 어떨까요?
너무 오랜만에 보내네요 ㅠㅠ 잘부탁드립니다!