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

[MVC 구현하기 - 3단계] 다즐(최우창) 미션 제출합니다. #589

Merged
merged 5 commits into from
Sep 25, 2023

Conversation

woo-chang
Copy link
Member

안녕하세요 키아라 🥸

이번 미션은 기존 코드 리팩토링 위주로 빠르게 진행해보았습니다 !
빠르게 진행하다 보니 놓친 부분도 있을 것 같은데 놓친 부분이 보이신다면 리뷰해주시면 감사합니다 :)

DispatcherServlet은 org.springframework.web.servlet으로 옮겼으나 DispatcherServletInitializer는 옮기지 못했습니다. 원인은 WebApplicationInitializer 때문이라고 생각하는데 해당 인터페이스는 애플리케이션이 실행될 때 자동으로 탐지되고 실행되기 때문입니다.

https://github.com/spring-projects/spring-framework/blob/main/spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractAnnotationConfigDispatcherServletInitializer.java

스프링에서도 이를 추상화한 클래스를 제공하기에 개발자가 이를 구현한다면 DispatcherServlet이 등록되고 웹 애플리케이션 초기화 작업이 수행되도록 할 수 있습니다. 스프링 부트를 사용한다면 @SpringBootApplication 어노테이션이 이러한 설정을 자동으로 해주는 것으로 이해했습니다.

따라서 DispatcherServletInitializer는 결국 app 패키지에 위치해야 할 것 같은데 확실치는 않은 것 같아서 이 부분에 대해서 같이 얘기해보면서 알아가고 싶어요~!

이번 리뷰도 잘 부탁드립니다 🙇🏻‍♂️🙇🏻‍♂️

@woo-chang woo-chang self-assigned this Sep 25, 2023
Copy link

@kiarakim kiarakim left a comment

Choose a reason for hiding this comment

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

다즐~ 주말 잘 보내셨나요?
하루만에 빠른 구현 해주셨어요!👍👍

몇가지 저와 공통된 문제점이 보여서 코멘트로 남겨놨어요.

  1. 홈 디렉토리("/")에 접속하면 쿠키에 세션 id가 저장됩니다.
  2. 세션 id가 있는 상태로 로그인을 시도할 수 있습니다.
  3. 로그인을 gugu password로 하면 세션 id가 바뀌거나 하지 않아요. 아마 1번에서 저장된 세션id가 gugu password에 대한 세션 id였겠죠..?
  4. /logout과 /api/user 접속을 할 수 없습니다.
    -> 로그아웃의 경우 post메소드 대신 get을 쓰면 해결이 되는 것 같아요.

해결 방법을 찾게 되면 디엠 드릴게요! 화이팅

Comment on lines 29 to 32
} else {
return objectMapper.writeValueAsString(model);
}
}

Choose a reason for hiding this comment

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

else??👀 if문으로 걸러준 다음에 return 해도 좋을 것 같아요!

@Override
public String execute(final HttpServletRequest req, final HttpServletResponse res) throws Exception {
final var session = req.getSession();
@RequestMapping(value = "/logout", method = RequestMethod.POST)

Choose a reason for hiding this comment

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

localhost에서 로그인 후 로그아웃을 시도하면 요청에 알맞은 핸들러가 없다는 서버 에러가 뜨고 있어요.
스크린샷 2023-09-25 오후 2 51 10

혹시나 해서 이 메소드를 GET으로 바꿔봤는데 되더라구요.

그리고 이 부분은 다즐뿐만 아니라 저한테도 해당되는데 정확한 원인은 모르겠지만 홈 디렉토리로 가기만 해도 세션 아이디가 쿠키로 저장됩니다. 그리고 세션 아이디가 저장된 상태에서 로그인이 눌리구요.
스크린샷 2023-09-25 오후 2 52 55
스크린샷 2023-09-25 오후 2 53 18

해당 부분에 대해선 오늘함께 해결해보면 좋을 것같아요.

Copy link
Member Author

@woo-chang woo-chang Sep 25, 2023

Choose a reason for hiding this comment

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

해당 부분은 DM으로 얘기 나눈 것처럼 HTML 태그와 관련있는 것 같아요 😀

<li><a class="dropdown-item" href="/logout">로그아웃</a></li>

로그아웃 버튼에 a 태그를 사용하는데 href에서 명시되어 있는 링크로 단순히 이동하게 됩니다. 그렇기에 별다른 HTTP method를 설정할 수 없게 되고 GET으로 동작하게 되는 것 같아요. 다른 메서드로 수정하고 싶으면 별도의 자바스크립트 코드를 통해 해결할 수 있다고 하네요!

따라서 POST -> GET 메서드로 수정하여 위의 문제를 해결할 수 있도록 하였습니다 :)


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

@RequestMapping(value = "/api/user", method = RequestMethod.GET)

Choose a reason for hiding this comment

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

이 부분 정상 작동되고 있지 않습니다ㅠ
저도 같은 문제를 겪고 있어서 해결 방법 알게 되면 코멘트 남길게요!

스크린샷 2023-09-25 오후 3 06 20

Copy link
Member Author

Choose a reason for hiding this comment

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

슬랙을 통해 해당 부분도 해결되었다고 생각합니다 😀

Copy link

@kiarakim kiarakim left a comment

Choose a reason for hiding this comment

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

다즐! 넘넘 고생 많으셨어요
이번 미션 함께 하게 되어 너무 기뻤습니다ㅎㅎ

MVC 미션은 여기서 마무리 할게요 다음 미션도 열심히 해봅시당. 화이팅~

@kiarakim kiarakim merged commit ef25cbc into woowacourse:woo-chang Sep 25, 2023
1 check failed
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.

2 participants