로그인 트랜잭션 생성 및 반환 타입 생성

[gwgw123]

🔗 관련 이슈

#65

📝작업 내용

• 로그인 시 DB 가 수정되는 작업은 트랜잭션으로 묶어주었습니다.
  유저정보 생성 → 유저 hp 생성 → 유저 part 생성 → 소셜 토큰 저장 과 같은 일련의 작업들이 포함됩니다.

📝작업 내용

• 로그인 시 DB 가 수정되는 작업은 트랜잭션으로 묶어주었습니다.
  유저정보 생성 → 유저 hp 생성 → 유저 part 생성 → 소셜 토큰 저장 과 같은 일련의 작업들을 트랜잭션으로 만들었습니다.
• 로그인 과정중 반환타입이 any 인 것들에 대해 타입을 지정해주었습니다.

🔍 변경 사항

• 로그인 트랜잭션 생성
• any 타입을 수정

💬리뷰 요구사항 (선택사항)

• 이번에 트랜잭션을 처음 구현해보면서 여러가지 방법이 있다는걸 알게되었고
  트랜잭션 클라이언트 인자를 사용하는 방법을 택했습니다.
  이유는 여러 단계의 작업이 겹쳐져 더 적합하다고 판단했습니다.
  추가로 확장성을 위해 트랜잭션에 사용된 기존 메서드들은 tx 인자가 들어오지 않는다면 default로 prisma 인자를 사용하도록 설계했습니다
  이외 적합한 방법이나 의견이 있으시면 리뷰 부탁드립니다 ~
• 로그인시 사용되는 많은 타입이 any 타입이었는데 새로 지정해주었습니다.
  소셜 서비스에서 바로 제공해주는 값이면 interface만 사용해주었고,
  Layer 간 데이터 이동이 있었다면 DTO 나 객체클래스를 만들어 타입으로 지정해주었습니다.
  이 부분도 적합한 방법이나 의견이 있으시면 리뷰 부탁드립니다 ~

[dg1418]

프리스마 객체 타입이 와야 하는 부분이네요. 같이 any 타입 바꿔보죵

[gwgw123]

네 알겠습니다 추후 변경 후 댓글 달겠습니다 ~

[hobiJeong]

GoogleUserInfo는 Guard에서 return type 정의를 위한 interface이고 해당 클래스는 User 데코레이터에서 가져오는 프로퍼티의 타입을 정의하기 위함인가요?
근데 별 다른 validate 로직을 하지 않는다면 굳이 class-validator 데코레이터를 사용할 필요가 없을 것 같습니다.
아니면 굳이 interface를 따로 만들지 않고 guard에서 해당 dto의 인스턴스로 만들어서 validate를 한번 해주고 return 하는 게 더 효율적일 것 같네요

[gwgw123]

GoogleUserInfo는 Guard에서 return type 정의를 위한 interface이고 해당 클래스는 User 데코레이터에서 가져오는 프로퍼티의 타입을 정의하기 위함인가요?
ㄴ 네 맞습니다

근데 별 다른 validate 로직을 하지 않는다면 굳이 class-validator 데코레이터를 사용할 필요가 없을 것 같습니다.
ㄴ validate 로직을 하지 않는다는게 어떤 말씀이실까요? dto를 validate 해주기 위한 용도로 사용했습니다.

아니면 굳이 interface를 따로 만들지 않고 guard에서 해당 dto의 인스턴스로 만들어서 validate를 한번 해주고 return 하는 게 더 효율적일 것 같네요
ㄴ 가드에서 리턴할때 인터페이스를 사용하지 않고 만들어놓은 dto를 사용하는게 효율적으로 좋겠다는 말씀이신가요 ?
이부분에서 인터페이스를 따로 만들어야할까 고민했었는데요, 소셜 서비스에서 검증되어 제공되는 값을 한번 더 검증하지 않아도 되겠다 생각해서 인터페이스만 넣어줬습니다. ( dto 사용시 불필요한 인스턴스를 만드는 느낌이었습니다 )
그래서 두 개중 인터페이스를 만드는 선택을 했는데 어떻게 생각하시나요 ?

[hobiJeong]

따로 Dto의 인스턴스를 만들어서 valdiate를 호출하는 로직이 안보여서� 코멘트를 작성했는데 만약 해당 로직이 있다면 굳이 코멘트를 안봐도 될 것 같네요.

만약 그게 아니라면, class-validator의 검증 로직은 타입에 클래스 지정만으로 validation 로직이 진행되는 게 아닙니다.
Body나 Query같은 decorator는 nestjs의 built-in decorator고 �type으로 설정한 Dto 클래스의 인스턴스 생성 및 class-validator의 validate 함수를 내부적으로 실행하는겁니다.
실제로 class-validator의 decorator를 통해 유효성 검사를 하려면 아래와 같이 class instance 생성 및 validate함수 호출을 해야 합니다.

import {
  validate,
  validateOrReject,
  Contains,
  IsInt,
  Length,
  IsEmail,
  IsFQDN,
  IsDate,
  Min,
  Max,
} from 'class-validator';

export class Post {
  @Length(10, 20)
  title: string;

  @Contains('hello')
  text: string;

  @IsInt()
  @Min(0)
  @Max(10)
  rating: number;

  @IsEmail()
  email: string;

  @IsFQDN()
  site: string;

  @IsDate()
  createDate: Date;
}

let post = new Post();
post.title = 'Hello'; // should not pass
post.text = 'this is a great post about hell world'; // should not pass
post.rating = 11; // should not pass
post.email = 'google.com'; // should not pass
post.site = 'googlecom'; // should not pass

validate(post).then(errors => {
  // errors is an array of validation errors
  if (errors.length > 0) {
    console.log('validation failed. errors: ', errors);
  } else {
    console.log('validation succeed');
  }
});

validateOrReject(post).catch(errors => {
  console.log('Promise rejected (validation failed). Errors: ', errors);
});
// or
async function validateOrRejectExample(input) {
  try {
    await validateOrReject(input);
  } catch (errors) {
    console.log('Caught promise rejection (validation failed). Errors: ', errors);
  }
}

class-validator cv에서 제공하는 여러 데코레이터가 생각보다 많기도 하고 공식 문서를 한번� 보시는 것도 괜찮을 것 같아요.

검증을 안해도 될 것 같아서 인터페이스를 만들었다고 하셨는데 그 값을 받는 라우터의 파라미터에서는 검증을 위해 Dto를 넣으셨다고 하셔서 이게 어떤 의도인지 여쭤봐도 될까요?

마지막으로 dto 사용 시 불필요한 인스턴스를 만드는 느낌이었다고 하셨는데 어차피 인스턴스라는건 class를 통해 만들어진 object일 뿐이기 때문에 별 다른 데코레이터가 붙어 있지 않은 이상 똑같은 object입니다

[hobiJeong]

따로 Dto의 인스턴스를 만들어서 valdiate를 호출하는 로직이 안보여서� 코멘트를 작성했는데 만약 해당 로직이 있다면 굳이 코멘트를 안봐도 될 것 같네요.

만약 그게 아니라면, class-validator의 검증 로직은 타입에 클래스 지정만으로 validation 로직이 진행되는 게 아닙니다. Body나 Query같은 decorator는 nestjs의 built-in decorator고 �type으로 설정한 Dto 클래스의 인스턴스 생성 및 class-validator의 validate 함수를 내부적으로 실행하는겁니다. 실제로 class-validator의 decorator를 통해 유효성 검사를 하려면 아래와 같이 class instance 생성 및 validate함수 호출을 해야 합니다.

import {
  validate,
  validateOrReject,
  Contains,
  IsInt,
  Length,
  IsEmail,
  IsFQDN,
  IsDate,
  Min,
  Max,
} from 'class-validator';

export class Post {
  @Length(10, 20)
  title: string;

  @Contains('hello')
  text: string;

  @IsInt()
  @Min(0)
  @Max(10)
  rating: number;

  @IsEmail()
  email: string;

  @IsFQDN()
  site: string;

  @IsDate()
  createDate: Date;
}

let post = new Post();
post.title = 'Hello'; // should not pass
post.text = 'this is a great post about hell world'; // should not pass
post.rating = 11; // should not pass
post.email = 'google.com'; // should not pass
post.site = 'googlecom'; // should not pass

validate(post).then(errors => {
  // errors is an array of validation errors
  if (errors.length > 0) {
    console.log('validation failed. errors: ', errors);
  } else {
    console.log('validation succeed');
  }
});

validateOrReject(post).catch(errors => {
  console.log('Promise rejected (validation failed). Errors: ', errors);
});
// or
async function validateOrRejectExample(input) {
  try {
    await validateOrReject(input);
  } catch (errors) {
    console.log('Caught promise rejection (validation failed). Errors: ', errors);
  }
}

class-validator cv에서 제공하는 여러 데코레이터가 생각보다 많기도 하고 공식 문서를 한번� 보시는 것도 괜찮을 것 같아요.

검증을 안해도 될 것 같아서 인터페이스를 만들었다고 하셨는데 그 값을 받는 라우터의 파라미터에서는 검증을 위해 Dto를 넣으셨다고 하셔서 이게 어떤 의도인지 여쭤봐도 될까요?

마지막으로 dto 사용 시 불필요한 인스턴스를 만드는 느낌이었다고 하셨는데 어차피 인스턴스라는건 class를 통해 만들어진 object일 뿐이기 때문에 별 다른 데코레이터가 붙어 있지 않은 이상 똑같은 object입니다

아 이 부분은 제가 잘못 알고 있었습니다.
Body, Query같은 데코레이터가 아닌 지정된 타입을 통해서 ValidationPipe에서 validate 로직을 진행합니다.
�별도로 validate 로직을 작성하지 않아도 되겠네요

[hobiJeong]

아 어떤 의도인지 알았습니다 ㅋㅋㅋ
어치피 Validation Pipe에서 validation 겸 GoogleUserDto로 변환돼서 오니까 굳이 가드에서 dto를 만들어 줄 필요는 없겠네요.
지금이 잘하신 것 같아요

[gwgw123]

dto와 validate에 대해서 알아보게 됐습니다
처음에 말씀하신대로 nest에서 제공하는 데코레이터 ( Body, Query같은 데코레이터 ) 에서 dto를 타입으로 지정했을 때만
자동으로 dto 인스턴스 생성 및 class-validator 가 실행된다는 것을 알았습니다.

그리고 난 이후에 추가로 리뷰달아주신 내용에서
validate 로직을 작성하지 않아도 된다고 하셨는데 왜 그런건지 잘 모르겠습니다 ㅠ

처음 리뷰를 토대로
google 로그인시 들어오는 user 정보를
98718b9
이런식으로 google.strategy.ts 파일에서 validate 하도록 수정했습니다.

추가적으로

구글 로그인시 유저정보를 받아올때
@User() user: GoogleUserDto 와 같이 사용되는 건 validate 용도가 아닌 타입 지정의 용도로 사용했고,
기본 서비스내 api 요청시 토큰에서 userId를 뽑아 유저정보를 가져올때
@User() user: UserInfo 와 같이 사용되는것도 validate 용도로 사용하지 않았습니다.

[hobiJeong]

@user 데코레이터를 단 파라미터의 타입에 GoogleUserDto를 지정해 줬을 떄 validation이 실행되지 않았나요?
혹시 해서 제가 따로 테스트 했을 땐 컨트롤러의 라우터의 파라미터에 class-validator 데코레이터를 단 class를 타입으로 지정해주면 자동으로 Validation Pipe에서 validate를 해줘서 따로 로직을 작성하지 않아도 된다고 한거였어요

[gwgw123]

아하
제가 테스트 해보지 않았다가 방금 테스트 해봤는데요

이렇게 TestDto 를 달아주고 console.log 찍어보니 검증되지 않고 문자열로 그대로 나옵니다

처음에 말씀해주신대로 nest에서 제공하는 데코레이터가 아니면 적용이 안되는걸로 보이네요

providerId 가 원래 문자열로 넘어오는데 dto 에서 number 타입으로 명시해주고 적용해봤습니다.

[hobiJeong]

해당 에러 메세지는 지금 봤는데 클라이언트 측에 레디스를 사용한다는 정보를 노출시킬 필요는 없을 것 같습니다.

[gwgw123]

아 그렇군요 알겠습니다 ~

[hobiJeong]

마찬가지로 굳이 레디스 사용한다는 정보라던가, '리프레시 토큰 저장을 실패했다'는 것까지 알려 줄 필요가 없어요.
결론은 클라이언트 측에는 서버 내부 인프라나 구현에 대한 정보를 아예 알리지 않는 게 좋습니다.
그냥 서버에선 '로그인 중 내부에서 에러가 발생' 정도만 알리고 클라이언트 측에서 '알 수 없는 에러로 로그인에 실패했습니다. 다시 시도해주세요' 이런 식으로 대충 띄워주면 됩니다.

[gwgw123]

디버깅에 용이하도록 자세히 적은 의도였는데
보안적인 생각을 하지 못했네요 알겠습니다 !

[hobiJeong]

any보단 optional로 만들고 tx 객체 타입을 넣어주는 게 나을 것 같습니다.
그리고 메서드 로직 내에서 tx 파라미터가 넘어 왔는지 여부에 따라 어떤 것을 통해 쿼리를 날릴지 코드를 작성하면 될 것 같습니다.

[gwgw123]

타입 수정이 필요한 부분이네요
지금 생각하기론 저희 서비스는 어떤 parameter 인지에 따라 바뀌지 않아도 될 거 같은 생각인데
이 부분 생각해보고 참고하겠습니다 ~

[gwgw123]

b2fd953

타입 만들어서 수정했습니다 ~

[hobiJeong]

여기저기 필요 없는 try catch가 많아서 좀 무분별한 try catch로 보이긴 합니다.
어떤 로직에서 에러가 발생했을 때 재시도 로직이라던가 후처리가 필요한 경우에는 try catch를 사용하면 좋겠죠
예로는 트랜잭션을 수동으로 제어한다고 했을 때 try에선 로우 생성 및 commit, catch에선 rollback, finally에선 connection 반납 로직을 짜는 등의 예가 있습니다.

[gwgw123]

여기도 디버깅시 어느 부분이 잘 못 되었는지 바로 확인하려고 세분화하니 필요없는 try catch가 많아졌나봅니다.
필요없는 try catch 부분은 정리해보겠습니다 ~

[gwgw123]

4f6b9c6, 334ebd2

불필요한 try catch 부분 삭제하고 에러메시지도 일부 수정했습니다 ~
에러 발생시 후속조치가 필요하지 않는데도 try catch를 사용했더라구요.

트랜잭션 처럼 로직의 결과에 따라 다르게 동작해야 하는 곳에 try catch를 적용해봐야겠습니다 ~

[hobiJeong]

저도 이렇게 tx 인자를 메서드의 인수로 넘기는 로직을 사용했었습니다.
tx 인자를 넘기고 받는 로직이 불편하다면 @nestjs-cls/transactional-adapter-prisma같은 라이브러리를 이용해봐도 좋을 것 같습니다.
문서

[gwgw123]

오 트랜잭션을 데코레이터로 만들어 사용할 수 있는 라이브러리네요 !

문서를 봤는데 tx 인자를 따로 넘기지 않고 repository 에서만 tx 객체를 주입 받아서 사용하네요 ?
로직이 단순화 되고 좋은 것 같네요 나중에 적용해보겠습니다

트랜잭션을 구현하면서 로직의 복잡성이 올라가는것 같습니다.. 정리가 잘 안되는 느낌인데
이 문제에서 비호님의 팁이 있을까요?
예를 들어 아예 트랜잭션용 repository를 만들고 사용하는건 어떨까요?

[hobiJeong]

맞습니다
아무래도 서비스에 기능이 많아지면 처음엔 단순하게 생각했던 코드들도 쌓이고 쌓여서 복잡도가 올라갑니다.
저는 transaction용 repository라기 보다는
그냥 prisma같은 orm을 사용해서 db에 접근하는 로직들은 repository에서 따로 관리하고 해당 라이브러리 같은 기능들을 이용해서 트랜잭션 관리를 맡기는 식으로 코드를 구현합니다.
현재 아키텍처 패턴에선 위 방법이 최선이지 않을까 싶네요