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

[HTTP 서버 구현하기 - 2, 3단계] 배럴(김나은) 미션 제출합니다. #84

Merged
merged 19 commits into from
Sep 5, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions app/src/main/java/nextstep/jwp/HttpServer.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ public class HttpServer {
private static final Logger log = LoggerFactory.getLogger(HttpServer.class);

private final BufferedReader reader;
private final RequestLine requestLine;
private final RequestHeader headers;
private final RequestBody requestBody;
private final RequestHeader requestHeader;
private final MappingHandler mappingHandler;

public HttpServer(InputStream inputStream) throws IOException {
this.reader = new BufferedReader(new InputStreamReader(inputStream));
this.requestLine = new RequestLine(extractRequestLine());
this.headers = new RequestHeader(extractHeaders());
this.requestBody = new RequestBody(extractRequestBody());
final RequestLine requestLine = new RequestLine(extractRequestLine());
this.requestHeader = new RequestHeader(extractHeaders());
final RequestBody requestBody = new RequestBody(extractRequestBody());
this.mappingHandler = new MappingHandler(requestLine, requestHeader, requestBody);

Choose a reason for hiding this comment

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

단순히 생성자의 인자로 들어가기때문에, 이전의 방식이 더 좋아보여요. 변수 인스턴스화를 하게된 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

사용되지 않아서 불필요하다고 생각했던 것 같아요!ㅎㅎㅎ 지금은 HttpRequest 라는 객체에 모아두었는데, 프로퍼티로 설정하는 것과 변수 인스턴스로 만드는 검프만의 기준은 어떤 것인지 궁금하네요!ㅎㅎㅎ

Copy link

@Livenow14 Livenow14 Sep 5, 2021

Choose a reason for hiding this comment

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

저는 인스턴스화 된 변수가 다른곳에서 쓰이고, 할당된다면 인스턴스화를 진행하는 거 같아요!
저기서는 생성자의 인자로만 들어가고 있어 남긴 피드백이었습니다. 😊

}

private String extractRequestLine() throws IOException {
Expand All @@ -54,8 +54,8 @@ private String extractHeaders() throws IOException {
}

private String extractRequestBody() throws IOException {
if (headers.contains(Header.CONTENT_LENGTH.getKey())) {
int contentLength = Integer.parseInt(headers.get(Header.CONTENT_LENGTH.getKey()));
if (requestHeader.contains(Header.CONTENT_LENGTH.getKey())) {
int contentLength = Integer.parseInt(requestHeader.get(Header.CONTENT_LENGTH.getKey()));
char[] buffer = new char[contentLength];
reader.read(buffer, 0, contentLength);
return new String(buffer);
Expand All @@ -64,11 +64,10 @@ private String extractRequestBody() throws IOException {
}

public String getResponse() throws IOException {
final MappingHandler mappingHandler = new MappingHandler(this.requestLine, this.requestBody);
try {
return mappingHandler.response();
} catch (InvocationTargetException e) {
log.info(e.getMessage());
log.info("Reflection related controller Error {}", e.toString());
return handleReflectionException(e);
} catch (PageNotFoundException e) {
log.info(e.getMessage());
Expand Down
3 changes: 2 additions & 1 deletion app/src/main/java/nextstep/jwp/constants/ContentType.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ public enum ContentType {
HTML("html", "text/html"),
CSS("css", "text/css"),
JS("js", "text/js"),
SVG("svg", "image/svg+xml");
SVG("svg", "image/svg+xml"),
ICO("ico", "image/svg+xml");

private final String fileType;
private final String contentType;
Expand Down
4 changes: 3 additions & 1 deletion app/src/main/java/nextstep/jwp/constants/Header.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
public enum Header {
CONTENT_LENGTH("Content-Length"),
LOCATION("Location"),
CONTENT_TYPE("Content-Type");
CONTENT_TYPE("Content-Type"),
SET_COOKIE("Set-Cookie"),
COOKIE("Cookie");

private final String key;

Choose a reason for hiding this comment

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

Suggested change
public enum Header {
CONTENT_LENGTH("Content-Length"),
LOCATION("Location"),
CONTENT_TYPE("Content-Type");
CONTENT_TYPE("Content-Type"),
SET_COOKIE("Set-Cookie"),
COOKIE("Cookie");
private final String key;
public enum HeaderType {
CONTENT_LENGTH("Content-Length"),
LOCATION("Location"),
CONTENT_TYPE("Content-Type");
CONTENT_TYPE("Content-Type"),
SET_COOKIE("Set-Cookie"),
COOKIE("Cookie");
private final String value;

위와 같은 네이밍은 어떤가요?!

Copy link
Author

@knae11 knae11 Sep 4, 2021

Choose a reason for hiding this comment

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

HeaderType, value 가 더 명확해 보이네요:)


Expand Down
2 changes: 2 additions & 0 deletions app/src/main/java/nextstep/jwp/constants/Http.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ public class Http {
public static final String AND_PERCENT_SEPARATOR = "&";
public static final String EQUAL_SEPARATOR = "=";
public static final String COLUMN_SEPARATOR = ":";
public static final String SEMI_COLUMN_SEPARATOR = ";";
public static final String JSESSIONID = "JSESSIONID";

private Http() {
}

Choose a reason for hiding this comment

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

Suggested change
private Http() {
}
private HttpOperation() {
}

Http라는 네이밍은 조금 Http 자체를 뜻하는 거 같아요. 위와같은 네이밍은 어떨까요?!

Copy link
Author

Choose a reason for hiding this comment

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

HttpTerms 라고 하였어요 :)

Expand Down
59 changes: 53 additions & 6 deletions app/src/main/java/nextstep/jwp/controller/Controller.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,45 +2,57 @@

import java.io.IOException;
import java.util.Map;
import java.util.Objects;
import java.util.UUID;
import nextstep.jwp.constants.Header;
import nextstep.jwp.constants.Http;
import nextstep.jwp.constants.StatusCode;
import nextstep.jwp.constants.UserParams;
import nextstep.jwp.exception.UnauthorizedException;
import nextstep.jwp.model.User;
import nextstep.jwp.request.HttpCookie;
import nextstep.jwp.request.HttpSession;
import nextstep.jwp.request.HttpSessions;
import nextstep.jwp.request.RequestBody;
import nextstep.jwp.request.RequestHeader;
import nextstep.jwp.response.ResponseEntity;
import nextstep.jwp.service.HttpService;

public class Controller {

@GetMapping(path = "/")
public String basic() {
public String basic(RequestHeader header) {
return ResponseEntity
.responseBody("Hello world!")
.build();
}

@GetMapping(path = "/login")
public String login() throws IOException {
public String login(RequestHeader header) throws IOException {
if (header.contains(Header.COOKIE.getKey())) {
return redirectToIndex(header);
}
return ResponseEntity
.responseResource("/login.html")
.build();
}

@GetMapping(path = "/register")
public String register() throws IOException {
public String register(RequestHeader header) throws IOException {
return ResponseEntity
.responseResource("/register.html")
.build();
}

@GetMapping(path = "/index")
public String index() throws IOException {
public String index(RequestHeader header) throws IOException {
return ResponseEntity
.responseResource("/index.html")
.build();
}

@PostMapping(path = "/register")
public String register(RequestBody body) throws IOException {
public String register(RequestHeader header, RequestBody body) throws IOException {

Choose a reason for hiding this comment

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

HttpRequest를 만들어 RequestHeader, RequestBody를 넣어주면, 전체 컨트롤러에 공통 적용 가능하고, 역할을 분리 할 수 있을 거 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

좋은 생각입니다! 해당 내용으로 변경할게요 :)

Map<String, String> params = body.getParams();
HttpService.register(params);
return ResponseEntity
Expand All @@ -51,11 +63,46 @@ public String register(RequestBody body) throws IOException {
}

@PostMapping(path = "/login")
public String login(RequestBody body) throws IOException {
public String login(RequestHeader header, RequestBody body) throws IOException {
Map<String, String> params = body.getParams();
if (header.contains(Header.COOKIE.getKey())) {
return redirectToIndex(header);
}
if (!HttpService.isAuthorized(params)) {
throw new UnauthorizedException("인증되지 않은 사용자 입니다.");
}
final UUID sessionId = UUID.randomUUID();
setSession(params, sessionId);
return ResponseEntity
.statusCode(StatusCode.FOUND)
.addHeaders(Header.LOCATION, "/index.html")
.addHeaders(Header.SET_COOKIE, Http.JSESSIONID + Http.EQUAL_SEPARATOR + sessionId)

Choose a reason for hiding this comment

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

Suggested change
.addHeaders(Header.SET_COOKIE, Http.JSESSIONID + Http.EQUAL_SEPARATOR + sessionId)
.addHeaders(Header.SET_COOKIE, httpSession.valuesToString())

HttpRequest등에 HttpSession을 넣으면, 위와 같이 활용할 수 있을거라 생각해요!

Choose a reason for hiding this comment

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

저는 HttpCookie에 이와 같은 로직을 넣었어요

Copy link
Author

Choose a reason for hiding this comment

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

해당 로직 이동해 보았습니다 :)

.responseResource("/index.html")
.build();
}

private void setSession(Map<String, String> params, UUID sessionId) {
HttpSessions.addSession(sessionId.toString());
HttpSession session = HttpSessions.getSession(sessionId.toString());
User user = HttpService.findUser(params.get(UserParams.ACCOUNT));
session.setAttribute("user", user);
}

private void checkCookieSessionId(RequestHeader header) {
HttpCookie cookie = header.getCookie();
if (!cookie.contains(Http.JSESSIONID)) {
throw new UnauthorizedException("인증되지 않은 사용자 입니다.");
}
String sessionId = cookie.get(Http.JSESSIONID);
HttpSession session = HttpSessions.getSession(sessionId);
Object user = session.getAttribute("user");
if (Objects.isNull(user)) {
throw new UnauthorizedException("인증되지 않은 사용자 입니다.");
}
}

Choose a reason for hiding this comment

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

이부분도 HttpRequest를 만들면 자연스레 사라질 것이라 생각해요.
우선 HttpRequest, HttpResponse를 만들어 현재 로직에서의 책임을 분리하고(ResponseEntity가 HttpResponse의 역할을 한다고 생각하는데, 이 부분을 리팩터링 해보는건 어떨까요?? 😊),
정상 작동을 한다면, HttpSession, HttpCookie에 책임을 할당 해주는 식으로 좋을 것 같아요!


private String redirectToIndex(RequestHeader header) throws IOException {
checkCookieSessionId(header);
return ResponseEntity
.statusCode(StatusCode.FOUND)
.addHeaders(Header.LOCATION, "/index.html")

Choose a reason for hiding this comment

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

네이밍에서의 Index라는 의미가 어떤 의민지 알 수 있을까요~~?

Copy link
Author

Choose a reason for hiding this comment

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

index 페이지를 의미했었는데 해당 내용도 인자로 받을 수 있도록 변경하였어요 :-D

Expand Down
7 changes: 5 additions & 2 deletions app/src/main/java/nextstep/jwp/controller/GetHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.util.stream.Collectors;
import nextstep.jwp.constants.HttpMethod;
import nextstep.jwp.exception.HttpException;
import nextstep.jwp.request.RequestHeader;
import nextstep.jwp.response.ResponseEntity;

public class GetHandler implements Handler {
Expand All @@ -14,14 +15,16 @@ public class GetHandler implements Handler {
public GetHandler() {
}

@Override
public boolean matchHttpMethod(HttpMethod httpMethod) {
return this.httpMethod == httpMethod;
}

Choose a reason for hiding this comment

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

Suggested change
@Override
public boolean matchHttpMethod(HttpMethod httpMethod) {
return this.httpMethod == httpMethod;
}
@Override
public boolean matchHttpMethod(HttpMethod httpMethod) {
return HttpMethod.GET == httpMethod;
}

이렇게 하면, 한군데서만 쓰이는 인스턴스 생성을 막을 수 있을거라 생각해요!

Copy link
Author

Choose a reason for hiding this comment

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

👍 좋은 생각입니다!!


public String runController(String uri, Controller controller) throws Exception {
@Override
public String runController(String uri, RequestHeader requestHeader, Controller controller) throws Exception {

Choose a reason for hiding this comment

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

Suggested change
@Override
public String runController(String uri, RequestHeader requestHeader, Controller controller) throws Exception {
@Override
public String handle(String uri, RequestHeader requestHeader, Controller controller) throws Exception {

Handler 인터페이스의 메소드니까 handle이라 둬도 의미상 변함없을 거같아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

👍 동의합니다!ㅎㅎㅎㅎㅎ

for (Method method : Controller.class.getDeclaredMethods()) {
if (matchGetMapping(method, uri)) {
return (String) method.invoke(controller);
return (String) method.invoke(controller, requestHeader);
}
}

Expand Down
3 changes: 2 additions & 1 deletion app/src/main/java/nextstep/jwp/controller/Handler.java
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package nextstep.jwp.controller;

import nextstep.jwp.constants.HttpMethod;
import nextstep.jwp.request.RequestHeader;

public interface Handler {

boolean matchHttpMethod(HttpMethod httpMethod);

String runController(String uri, Controller controller) throws Exception;
String runController(String uri, RequestHeader requestHeader, Controller controller) throws Exception;
}
7 changes: 5 additions & 2 deletions app/src/main/java/nextstep/jwp/controller/MappingHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,18 @@
import nextstep.jwp.constants.HttpMethod;
import nextstep.jwp.exception.HttpException;
import nextstep.jwp.request.RequestBody;
import nextstep.jwp.request.RequestHeader;
import nextstep.jwp.request.RequestLine;

public class MappingHandler {
private final RequestLine requestLine;
private final RequestHeader requestHeader;
private final RequestBody requestBody;
private final Controller controller;

public MappingHandler(RequestLine requestLine, RequestBody requestBody) {
public MappingHandler(RequestLine requestLine, RequestHeader requestHeader, RequestBody requestBody) {
this.requestLine = requestLine;
this.requestHeader = requestHeader;
this.requestBody = requestBody;
this.controller = new Controller();
}
Expand All @@ -22,7 +25,7 @@ public String response() throws Exception {
final HttpMethod httpMethod = requestLine.getHttpMethod();
final String uri = requestLine.getUri();
final Handler handler = findHandler(httpMethod);
return handler.runController(uri, controller);
return handler.runController(uri, requestHeader, controller);
}

private Handler findHandler(HttpMethod httpMethod) {
Expand Down
7 changes: 5 additions & 2 deletions app/src/main/java/nextstep/jwp/controller/PostHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import nextstep.jwp.constants.HttpMethod;
import nextstep.jwp.exception.HttpException;
import nextstep.jwp.request.RequestBody;
import nextstep.jwp.request.RequestHeader;

public class PostHandler implements Handler {
private final HttpMethod httpMethod = HttpMethod.POST;

Choose a reason for hiding this comment

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

GetHandler와 같은 피드백입니다!

Copy link
Author

Choose a reason for hiding this comment

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

👍 반영했어요:)

Expand All @@ -13,14 +14,16 @@ public PostHandler(RequestBody requestBody) {
this.requestBody = requestBody;
}

@Override
public boolean matchHttpMethod(HttpMethod httpMethod) {
return this.httpMethod == httpMethod;
}

public String runController(String uri, Controller controller) throws Exception {
@Override
public String runController(String uri, RequestHeader requestHeader, Controller controller) throws Exception {
for (Method method : Controller.class.getDeclaredMethods()) {
if (matchPostMapping(method, uri)) {
return (String) method.invoke(controller, requestBody);
return (String) method.invoke(controller, requestHeader, requestBody);
}
}
throw new HttpException("잘못된 http post 요청입니다.");
Expand Down
20 changes: 20 additions & 0 deletions app/src/main/java/nextstep/jwp/model/User.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nextstep.jwp.model;

import java.util.Objects;

public class User {
private final long id;
private final String account;
Expand All @@ -21,6 +23,24 @@ public String getAccount() {
return account;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
User user = (User) o;
return id == user.id && Objects.equals(account, user.account) && Objects.equals(password,
user.password) && Objects.equals(email, user.email);
}

@Override
public int hashCode() {
return Objects.hash(id, account, password, email);
}

Choose a reason for hiding this comment

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

  1. equals를 사용하는 곳을 찾을 수가 없어요. 혹시 어디서 사용되는지 알 수 있을까요?

2 만약. equal가 사용되지 않는다면, 재정의를 해야할 필요성은 무엇이 있을까요?
이펙티브 자바 규칙 8 - equals 재정의 / 오버라이드 방법 참고해봐도 좋을 거같아요 :)

Copy link
Author

Choose a reason for hiding this comment

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

구현 중간에 같은 유저가 맞는지 확인하는 로직을 짜느라 넣었는데 해당 로직을 삭제하면서 해당 내용까지 삭제하지 못한 것 같아요! 삭제 처리 하였습니다! :) 예리검프! 👍

@Override
public String toString() {
return "User{" +
Expand Down
28 changes: 28 additions & 0 deletions app/src/main/java/nextstep/jwp/request/HttpCookie.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package nextstep.jwp.request;

import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import nextstep.jwp.constants.Http;

public class HttpCookie {
private final Map<String, String> cookies;

public HttpCookie(String cookie) {
this.cookies = parseCookie(cookie);
}
Comment on lines +8 to +13

Choose a reason for hiding this comment

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

Map 자료구조에 값들이 생성될 때만 저장되고 있어요.
Cookie에 값을 넣어주고, Response시에 HttpResponse(코드에서 ResponseEntity.class)에 넣어주는 방식은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

해당부분은 Request 에서 사용되는 것 같은데.. 혹시 어떤 부분을 의도한 건지 좀 더 자세히 설명해주실 수 있을까요? 🙏

Choose a reason for hiding this comment

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

HttpCookie가 HttpRequest에서만 있어야 할까? 라는 의문점에서 나온 피드백이었어요!
HttpResponse에도 Cookie를 가지고 있으면 더 자연스러워 보일 거 같았어요 😊


private Map<String, String> parseCookie(String cookie) {
return Stream.of(cookie.split(Http.SEMI_COLUMN_SEPARATOR))
.map(item -> item.trim().split(Http.EQUAL_SEPARATOR))
.collect(Collectors.toMap(x -> x[Http.KEY].trim(), x -> x[Http.VALUE].trim()));
}

public boolean contains(String key) {
return cookies.containsKey(key);
}

public String get(String key) {
return cookies.get(key);
}
}
25 changes: 25 additions & 0 deletions app/src/main/java/nextstep/jwp/request/HttpSession.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package nextstep.jwp.request;

import java.util.HashMap;
import java.util.Map;

public class HttpSession {
private final String id;
private final Map<String, Object> values = new HashMap<>();

HttpSession(String id) {
this.id = id;
}

public String getId() {
return id;
}

public void setAttribute(String name, Object value) {
values.put(name, value);
}

public Object getAttribute(String name) {
return values.get(name);
}
}
19 changes: 19 additions & 0 deletions app/src/main/java/nextstep/jwp/request/HttpSessions.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package nextstep.jwp.request;

import java.util.HashMap;
import java.util.Map;

public class HttpSessions {
private static final Map<String, HttpSession> SESSIONS = new HashMap<>();

public static void addSession(String id) {
SESSIONS.put(id, new HttpSession(id));
}

Choose a reason for hiding this comment

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

여러 상황을 고려해서 인자로 HttpSession을 넘겨 주는 건 어떨까요?
HttpSession.setAttribute 이후 HttpSessions에 넣어줄 상황도 생길 수 있다고 생각이 들어요

Copy link
Author

Choose a reason for hiding this comment

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

오! 좋은 생각입니다! :)


public static HttpSession getSession(String id) {
return SESSIONS.get(id);
}

private HttpSessions() {
}
}
Loading