Skip to content
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

[Roach-Pyro] 웹 서버 미션 제출 #9

Merged
merged 12 commits into from
Apr 2, 2021

Conversation

tmdgusya
Copy link
Contributor

서론

사실 미션대로 하지 않았고, 스프링 구조가 궁금해서 urlHandler 나 returnValueHandler 를 만들어보았습니다.
미션 자체의 의의 보다는 스프링을 공부하면서 짝 프로그래밍을 해보자는 의의가 컸습니다.
리플렉션을 이용해서 기존의 스프링의 서블릿(저희껀 싸구려 서블릿) 과 유사하게 만들었습니다.

MappingUrlHandler

  • MappingUrlHandler 는 @GetMapping / @PostMapping 을 분석하고, 해당 어노테이션에 mapping 된 url 을 기반으로
    request url 과 매칭하여 @controller 어노테이션이 적혀있는 클래스에서 해당 요청을 처리할 수 있는 method 를 찾아 해당 Method 를
    invoke 시켜줍니다.

ReturnValueHandler

  • ReturnValueHandler 는 MappingUrlHandler 를 통해서 invoke 된 return 값을 view 로 resolve 해줍니다.
    사실 String 으로 제한하였는데.. 뭐 더 만든다면 Object 로 풀어서 여러 handler 를 거치는것도 좋다고 생각하나 기간상 만들수 없었습니다.

Controller

  • 기존에 if-else.. 로 떡칠되어 있던 부분을 Controller 로 분리하여 기존 스프링이 하는것처럼 컨트롤러에서 처리 할 수 있도록 하였습니다.

tmdgusya added 11 commits March 23, 2021 15:18
현재 프로젝트 진행방법 그리고 커밋 컨벤션에 대한 요구사항을 반영하여 README.md 를 수정하였다.
BufferedReader 를 통해서 Request 정보를 읽은 뒤에 URL 을 parsing 하여
해당 자원을 Response 에 넣어주었습니다.
url 의 queryString 을 분석해서 User 객체를 생성하였음
RequestMethod 로 비교하기 위해 POST 와 GET 메소드를 별도 Enum 으로 분리하였음. 기존의 GET 로직을 변경하여 POST 로도 회원가입이 될 수도 있도록 하였다. parseQueryString 은 body 값을 해석해준다.
CSS 파일을 다시로딩해서 나던 오류를 return 을 통해서 사전에 로딩하고 종료하는 방식으로 바꿈.
@ghojeong
Copy link

@ghojeong

알림 subscribe을 위한 Comment

실수로 삭제해서 다시 적었음.
@tmdgusya
Copy link
Contributor Author

아직 리팩토링은 진행하지 않았습니다.

두번째 PR 에서 올릴 예정입니다.

Copy link
Contributor

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리팩토링에 대한 의견을 신나게 달고 있었는데 리팩토링은 아직 진행하지 않으신 거군요.
리팩토링에 도움될만한 아이디어들을 남겨뒀다고 해석해 주시면 감사하겠습니다.

미션 진행하시는 방향이 상당히 좋네요. 단순히 웹서버라는 기능 구현에 집중하지 않고 그동안 배운 지식을 기반으로 새로운 것을 만들어보려는 시도가 참 신선한 것 같습니다.
기회가 된다면 JPA 구현체를 만들어보시는 것도 추천합니다. JPA spec은 공식 문서를 통해 확인하시고요.

리팩토링은 다음 PR에서 보여주신다 하니 일단 머지하고 다음 PR을 설레는 마음으로 기다려 보겠습니다. 수고 많으셨습니다. 💯

import handler.mapping.Controller;
import handler.mapping.GetMapping;

@Controller
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

가슴이 웅장해지네요

Comment on lines +12 to +21
public void saveCookie(String cookie) {
String[] cookieEntry = cookie.split("=");
for(int i = 0; i < cookieEntry.length; i++) {
cookies.put(cookieEntry[0], cookieEntry[1]);
}
}

public void addCookie(String key, String value) {
cookies.put(key, value);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saveadd 의 차이가 뭔가요? 잘은 이해가 안가네요.


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

public void saveHeaders(BufferedReader br, String line) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HeaderParser 와 같은 컴포넌트가 별도로 구성되면 좋을 것 같습니다.


@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface GetMapping {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetMapping 에 이의 있습니다.
나중에 HTTP spec이 확장돼서 현재 있는 메소드 이외의 다른 메소드가 추가돼야 한다면, 그것은 어떻게 대응해야 할까요?
RequestMapping 이라는 베이스 애노테이션이 존재하고, 해당 애노테이션이 메소드에 대응되는 상태값을 관리하면서,
GetMapping, PostMapping 등은 alias의 형태로 작성돼야 하는 건 아닐까요.
예를 들면 이런 겁니다.

public @interface RequestMapping {
    HttpMethod method;
    String path;
}

@RequestMapping(method = HttpMethod.GET)
public @interface GetMapping {

}

이 코드 도 참조해 보시면 좋겠네요.

}

public Object invokeMethod(HttpRequest httpRequest, HttpResponse httpResponse) throws InvocationTargetException, IllegalAccessException, InstantiationException, NoSuchMethodException, ClassNotFoundException {
final Method method = this.findMethod();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요청이 들어올때마다 모든 컨트롤러를 스캔하나요?


private static final Logger log = LoggerFactory.getLogger(MappingUrlHandler.class);

private final String method;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method 에 대한 추상화가 이뤄지면 더 유연한 대응이 가능할 것 같네요.

Comment on lines +45 to +65
private Method findController(ArrayList<Class<?>> controllerclasses) throws ClassNotFoundException {
for(Class<?> c : controllerclasses) {
log.info("Class Name : {}", c.getName());
this.targetClass = Class.forName(c.getName());
final Method[] declaredMethods = c.getDeclaredMethods();
log.info("Method : " + method);
if(method.equals("GET")) {
final Method controllerMethod = findGetController(declaredMethods);
if(controllerMethod != null) {
return controllerMethod;
}
}
if(method.equals("POST")) {
final Method controllerMethod = findPostController(declaredMethods);
if(controllerMethod != null) {
return controllerMethod;
}
}
}
throw new IllegalArgumentException("500 을 리턴해야한다.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 메소드도 별도의 컴포넌트로 분리돼야 할 것 같습니다.
제 생각에 분리된 컴포넌트가 미리 컴포넌트-스캔 된 컨트롤러 클래스들을 갖고 있으면서, 요청에 맞는 컨트롤러 클래스를 뿌려주면 될 것 같네요.

Comment on lines +26 to +29
if(returnValue.startsWith("redirect")) {
String url = returnValue.split(":")[1];
response302Header(dos, url);
}else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

각 리턴 밸류 패턴에 맞는 ReturnValueHandler 가 독립적으로 존재했으면 좋겠습니다.
각 handler를 찾아주거나 생성하는 로직은 전략 패턴을 응용하면 쉽게 구현 가능할 듯 보이네요.

@wheejuni wheejuni merged commit fb2822f into codesquad-members-2021:master Apr 2, 2021
@wheejuni
Copy link
Contributor

wheejuni commented Apr 2, 2021

@tmdgusya @ghojeong 마스터로 PR 여셨습니다. 확인 잘 해주세요.

@ghojeong
Copy link

ghojeong commented Apr 2, 2021

@wheejuni

리팩토링에 대한 의견을 신나게 달고 있었는데 리팩토링은 아직 진행하지 않으신 거군요.

리팩토링에 대한 피드백 감사합니다!!
부담 가지지 마시고 진행에 도움이 될 수 있도록,
혹시 아껴두신 피드백이 있다면, 부담갖지 마시고, 추가로 달아주시면 감사하겠습니다.!!

@wheejuni
Copy link
Contributor

wheejuni commented Apr 2, 2021

@ghojeong 네네 근데 지금 리팩토링이 문제가 아니구요, 브랜치 확인하셔서 PR open 다시 해주시면 바로 머지하겠습니다.

@ghojeong
Copy link

ghojeong commented Apr 2, 2021

헙! 죄송합니다.

wheejuni pushed a commit that referenced this pull request Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants