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

[Feature] 내 정보 조회 API #109

Merged
merged 24 commits into from
Nov 29, 2022
Merged

Conversation

pythonstrup
Copy link
Member

@pythonstrup pythonstrup commented Nov 28, 2022

체크 리스트

  • 적절한 제목으로 수정했나요?
  • 관련된 이슈와 연결 시켰나요?
  • Target Branch를 올바르게 설정했나요?
  • Label을 알맞게 설정했나요?

작업 내역

  • MyInfo API 제작
  • 토큰을 유저 정보로 치환하기 위한 커스텀 데코레이터 제작

고민

  • 먼저 ID를 조회해 해당 유저를 찾고, 찾아온 User의 Username을 서로 비교해 해당 유저가 맞는지 확인합니다
  • 이중으로 확인하는 작업인데 다들 어떻게 생각하시나요? 필요하다고 생각하시나요?

image

비고

Copy link
Member

@username1103 username1103 left a comment

Choose a reason for hiding this comment

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

고생하셨어요! 이제 그래도 Nest에 어느정도 익숙해져 가시는 것 같네요

@@ -18,7 +19,7 @@ export const setSwagger = (app: INestApplication) => {
.build();

const document = SwaggerModule.createDocument(app, config, {
include: [AuthModule, ImageModule, GroupArticleModule],
include: [AuthModule, ImageModule, GroupArticleModule, MyInfoModule],
Copy link
Member

Choose a reason for hiding this comment

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

빼먹지 않고 잘 추가해주셨네요!

Comment on lines +24 to +27
@Put()
modifyMyInfo() {
return '';
}
Copy link
Member

Choose a reason for hiding this comment

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

아직 완성하지 않는 API인데 열어둔 이유가 있으신가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

merge되면 바로 이어서 만들려고 열어놨는데 좋지 않은 방법일까요..???

Copy link
Member

Choose a reason for hiding this comment

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

음 크게 문제되지는 않을 것 같아요. 그치만 굳이 안쓰는 API를 열어둘 이유도 없어보여요!

Copy link
Member Author

Choose a reason for hiding this comment

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

일단 만들어놓은 게 있어서.. 다음부터는 안 열어두는 게 좋겠네요!

Comment on lines 4 to 8
export const CurrentUser = createParamDecorator(
(data: unknown, context: ExecutionContext) => {
const ctx = GqlExecutionContext.create(context);
return ctx.getContext().req.user;
},
Copy link
Member

Choose a reason for hiding this comment

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

현재 유저정보를 가져오기 위해 데코레이터 만든거 좋은 것 같아요. 근데 저희는 graphql을 사용하고 있지 않은데 gql context를 사용한 이유가 있을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

왠지 꺼림칙해서 명일님 피드백을 반영해 http context로 처리했습니다.

example:
'https://kr.object.ncloudstorage.com/uploads/images/1669276833875-64adca9c-94cd-4162-a53f-f75e951e39db',
description: '프로필 이미지',
required: false,
Copy link
Member

Choose a reason for hiding this comment

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

혹시 해당 부분만 required: false인 이유가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

그러게요 굳이 false일 필요가 없겠네요..?
깃허브는 프로필이미지가 없는 경우에도 무조건 이미지가 있군요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

반영했습니다.

@@ -29,6 +29,7 @@
"@nestjs/common": "^9.0.0",
"@nestjs/config": "^2.2.0",
"@nestjs/core": "^9.0.0",
"@nestjs/graphql": "^10.1.6",
Copy link
Member

Choose a reason for hiding this comment

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

안쓰는 의존성은 제거해주세요!

Copy link
Member Author

Choose a reason for hiding this comment

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

반영했습니다.

import { MyInfoService } from '@app/myinfo/myinfo.service';
import { ApiTags } from '@nestjs/swagger';
import { ApiSuccessResponse } from '@src/common/decorator/api-success-resposne.decorator';
import { MyInfoGetResponse } from './dto/myinfo-get-response.dto';
Copy link
Member

Choose a reason for hiding this comment

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

절대경로 사용하시는김에 일관되게 사용하시면 좋을 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

앜.. 제가 놓쳤군요 감사합니다!

Comment on lines +1 to +8
import { createParamDecorator, ExecutionContext } from '@nestjs/common';

export const CurrentUser = createParamDecorator(
(data: unknown, context: ExecutionContext) => {
const request = context.switchToHttp().getRequest();
return request.user;
},
);
Copy link
Member

Choose a reason for hiding this comment

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

이 데코레이터는 어떤역할을 하는 것일까요? 조회하는 유저의 정보를 가져오는 것 같은데 어떤식으로 가져오는 건가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

Once again, we're applying the AuthGuard that the @nestjs/passport module has automatically provisioned for us when we configured the passport-jwt module. This Guard is referenced by its default name, jwt. When our GET /profile route is hit, the Guard will automatically invoke our passport-jwt custom configured logic, validating the JWT, and assigning the user property to the Request object.

image

  • 결론부터 말씀드리자면, Passport-JWT가 유저의 정보를 가져오는 로직을 제공해줍니다!
  • 우리 코드의 @JwtGuard()를 사용하는 경로에 도착하게되면 User를 자동으로 만드는 로직을 호출하게 되고 jwt를 검증한 뒤 requeset.user에 user의 정보를 넣어주게 됩니다!! 놀랍죠?
  • 커스텀 데코레이터는 그냥 해당 로직을 조금 응용한 내용입니다! request object를 콘텍스트를 이용해 가져오고 request.user를 반환해 바로 User 정보를 받아볼 수 있게 도와주는 역할만 수행합니다. 이 내용도 공식문서에서 확인해볼 수 있는데 아래와 같습니다.

image

Copy link
Member

@username1103 username1103 left a comment

Choose a reason for hiding this comment

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

@pythonstrup 고생하셨어요!!

@@ -1,6 +1,6 @@
import { NotFoundException } from '@nestjs/common';

export class GroupCategoryNotFound extends NotFoundException {
export class GroupCategoryNotFoundException extends NotFoundException {
Copy link
Member

Choose a reason for hiding this comment

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

바꾸신거 너무 편안하네요 굳굳

Comment on lines 11 to 20
async checkUserInfo(user: User) {
const userItem = await this.userRepository.findById(user.id);
if (!userItem) {
throw new UserIdNotFoundException();
}

if (userItem.username !== user.username) {
throw new UsernameNotFoundException();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

혹시 유저 정보를 다시한번 체크한 이유가 있을까요? JwtAuthGuard를 통해, 쿠키 토큰을 확인해서 디비에 있는 유저정보를 가져와 request.user 에 넣어주게 된다고 알고 있어서요.

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 저도 그게 고민이라 PR에 적어놓긴 했엇는데 굳이 할 필요가 없을 것 같긴 하네요!
그러면 UserIdNotFoundException 정도만 있어도 괜찮을까요? 아니면 다 없앨까요??

Copy link
Member

Choose a reason for hiding this comment

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

이미 JwtAuthGuard에서 유저 존재여부를 한번 체크하기 때문에 굳이 검증할 필요가 없을 것 같아요.

Comment on lines +30 to 32
describe('GET /v1/test/:id', () => {
const url = (id) => `/v1/test/${id}`;

Copy link
Member

Choose a reason for hiding this comment

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

이제 테스트까지 손댈줄 아시는군요

- 불필요한 유효성 검증 제거
Copy link
Collaborator

@kong430 kong430 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~!

@pythonstrup pythonstrup merged commit 69277cc into develop Nov 29, 2022
@sxungchxn sxungchxn deleted the feature/103-myInfo-get-api branch December 16, 2022 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] 내 정보 조회 API
4 participants