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

9주차 미션 / 서버 1조 김상준 #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

drbug2000
Copy link

@drbug2000 drbug2000 commented May 30, 2024

진행도

/users 하위 API 대상으로 인증/인가 resolver 방식과 interceptor 방식으로 구현해서 API에 적용했습니다.
에 적용했습니다.
/shop/list-page 에 조건을 받으면 non-offset 방식으로 paging 구현하였습니다.

userId

7,8주차까지의 API 설계에서는 인증/인가 기능이 없었기 때문에 user id를 pathvariable이나 request body에서 직접 받게 끔 구현하였습니다.
이번 주 미션부터는 token(혹은 session)에 사용자 id 혹은 정보가 직접 전달 되게 됩니다. 기존 API를 유지하면서 추가적으로 parameter에 user id와 authorize 된 user id가 같은지 비교하는 controller를 구현해보긴 하였으나, 불필요하다고 생각하여, Dto와 API를 수정하였습니다.

paging

paging을 non offset(cursor) 방식으로 구현했습니다.
이때, 사용자가 전달하는 lastId를 +1 하는 코드와, 다음 page가 존재하는지 판단하여. hasNextPage 값을 response에 담아야 했습니다.
이런 값들을 조작하고, 읽어온 값을 판단해서 Dto에 담아주는 코드 부분들이 어느 레이어(Dao, service, controller)에 있어야 할지 고민하다가, data layer에는 최대한 단순히 입력값을 SQL로 치환하고, 결과 값을 객체로 변환하는 역할을 맡는 것이 좋고, controller에는 유효성 검사, service 호출을 하고 어떤 비즈니스 로직이 들어가면 좋지 않다고 생각해서 service에 해당 코드들을 작성했습니다.

Anotation

argument resolver를 구현하는 과정에서 annotation을 직접 만들어 보고, 어떻게 argument resolver들이 작동하는지 알 수 있었습니다.
인가 처리와 같이 반복되는 코드들을 어노테이션으로 묶어 작성할 때 유용한 것 같습니다.

pull API and file from 8-week repository
apply aythorize in /user API
apply pagenation in /shop/list API
@drbug2000 drbug2000 self-assigned this May 30, 2024
Copy link
Contributor

@jaeuk520 jaeuk520 left a comment

Choose a reason for hiding this comment

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

이번주차 미션을 잘 이해하고 수행해주신 것 같아 코멘트를 조금 남겨드렸습니다!!
스터디 때도 말씀 드렸지만 ArgumentResolverInterceptor의 역할을 잘 이해해보시면 좋을 것 같아요!
고생많으셨습니다:))

jwtTokenUtil.validateAccessToken(accessToken);
String email = jwtTokenUtil.getEmail(accessToken);
jwtTokenUtil.validatePayload(email);
long userId = authService.getUserIdByEmail(email);
Copy link
Contributor

@jaeuk520 jaeuk520 May 31, 2024

Choose a reason for hiding this comment

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

토큰을 생성할 때부터 payload에 userId를 담아 생성한다면 ArgumentResolver에서 Service를 호출하는 행위를 방지할 수 있을 것 같아요:) 레이어드 아키텍처의 의존 방향을 생각해보시면 좋을 것 같습니다! (사실 9주차 미션 제공 코드에는 이미 payload에 userId를 담아둡니다..!! 서비스 단으로 갈 필요 없이 바로 추출해서 사용하면 돼요)

@RequiredArgsConstructor
public class GetShopListResponse {
private List<GetShopResponseEntity> shopList;
private boolean hasNextPage;
Copy link
Contributor

Choose a reason for hiding this comment

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

lastId를 반환하지 않은 이유는 GetShopResponseEntity에 shopId가 있기 때문일까요??
프론트에서는 모든 shopId를 가지고 있을 필요가 없을 것 같아 lastId만 반환해줘서 가지고 있다가 다음 요청에 lastId+1을 요청받을 수 있도록 하는게 좋을 것 같습니다!

import java.util.Date;

import static kuit3.backend.common.response.status.BaseExceptionResponseStatus.*;

@Slf4j
@Component
public class JwtProvider {

@Value("${secret.jwt-expired-in}")
Copy link
Contributor

Choose a reason for hiding this comment

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

스프링에서 환경 변수를 주입해주는 방법은 @Value 이외에도 @ConfigurationProperties와 같은 방법이 있어요. 두 가지 방법의 차이와 장단점을 비교해보셔도 좋을 것 같습니다 👍

}

public void validateAccessToken(String accessToken) {
if (jwtProvider.isExpiredToken(accessToken)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

JwtUtil을 생성하는 것은 너무 좋은 아이디어인 것 같아요:)
이왕 책임을 분리한 만큼 JwtProvider는 클래스 명과 같이 Jwt를 "생성"해주는 역할만 하는게 어떨까요?

private final AuthService authService;

@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {

String accessToken = resolveAccessToken(request);
validateAccessToken(accessToken);
String accessToken = jwtUtil.resolveAccessToken(request);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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