user put update api #117

hobiJeong · 2024-01-05T04:40:03Z

Description

유저 put update api입니다

To Reviewer

범용성있게 쓸 수 있는게 어떤 느낌일까 고민을 좀 많이 해보고 짜보긴 했는데 지적 부탁드림니다 ㅠ

Reference Link

API

Method	Path	설명	작업(추가, 수정, 삭제)
PUT	/api/users	유저 PUT 업데이트	추가

rrgks6221 · 2024-01-05T10:19:37Z

src/apis/users/dto/put-update-user.dto.ts

@@ -0,0 +1,66 @@
+import { ApiProperty } from '@nestjs/swagger';


업데이트 가능한 필드와 그렇지 않은 필드 어떤 차이점으로 구분했는지 알 수 있을까요??

일단 성별이랑 loginType은 변경될 이유가 없을 것 같고 role이나� password는 별도의 api가 필요할 것 같아서 뺐습니다.
근데 다시 생각해보니까 이메일은 oauth로만 로그인하면 어차피 거기서 이메일을 받아 오니까 이메일도 뺄까 생각중인데 어떻게 생각 하시나여??
핸드폰 번호도 변경할 때 별도의 인증을 거쳐서 하지 않을까 같아서 마찬가지로 뺄까 고민중입니다!

네 우선 이메일과, 핸드폰번호는 인증이 들어가서 빼면 좋을거같고 성별같은 경우는 우선 열어두는게 좋을듯합니다(아래 참고)
nullable field도 고려되면 좋을거같습니다. 현재는 다른 필드를 수정하고싶어도 기존에 null로 유지되던 필드들을 다 입력해야 작동하는 api spec 인거같아요
ex) 핸드폰번호가 null인데 이름을 바꾸기 위해 핸드폰번호가 필요함

null이던 필드들을 그대로 null로 유지시키기 위해 null을 허용하게 되면 그건 PATCH 메서드랑 엄연히 다르긴 하겠지만 뭔가 경계가 허물어지는 느낌이 드는데 아닐까요?

우선 클라이언트측 사용처를 아직 모르는게 가장 큰 점이긴한데
PUT과 PATCH 메서드의 주요 차이점은
PUT: 업데이트 기능을 제공할 수 있는 모든 필드를 한번에 수정할 수 있다. (있으면 수정 없으면 생성은 다른 얘기)
PATCH: 업데이트 기능을 제공할 수 있는 필드를 선택적으로 수정할 수 있다.

interface User { // 수정불가 필드 id: number; // 수정 가능 필드 name: string | null; // 수정가능 필드 nickname: string; }

이런 인터페이스를 가진 객체가 있다고 가정했을 때

request 필드는 아래처럼 정의돼야한다고 생각하긴 해요.

// 들어온 값 그대로 유효성 검사 후 DB에 밀어넣음 interface PutUpdateUser { name: string | null; nickname: string; } // name이나 nickname이 들어오지 않았다면 업데이트 대상이 아님 interface PatchUpdateUser { name?: string | null; nickname?: string; }

반영했습니다

src/apis/users/dto/put-update-user.dto.ts

rrgks6221 · 2024-01-05T10:22:04Z

src/apis/users/dto/put-update-user.dto.ts

+  grade: number;
+
+  @ApiProperty({
+    description: 'url 이 아닌 profile path',


설명이 약간 모호한거같은데 보강될 수 있다면 보강되면 좋을듯합니다

사실 profile path가 정확히 뭔지 이해를 못했습니다 ㅠㅠ
image.jpg처럼 파일명과 확장자를 의미하는 걸까요?

ex) https://dongurami.s3.com/user/1/profile_image.jpeg => profile_image.jpeg
이렇게 받는 이유는 아래와 같아요.

클라이언트측에서 저장소에 대한 주소를 알 필요가 없어요

그리고 어떤 path로 보내는것도 알 필요가 없어요. s3 기준으로 버킷관리는 백엔드의 범위

이거는 presigned url처럼 클라이언트측에서 저장소에 바로 업로드한 뒤 이미지 파일 자체가 아닌 path를 보내주는 경우에 해당합니다.

그러면 여기서 클라이언트측에서 파일에 접근할 수 있는 url(s3 주소 포함)을 알 수 있다는 전제가 생기는데 파일(사진)에 접근할 때 s3를 통해 접근한다는 가정이 필요합니다. cloudFront같은 cdn을 통해 접근할 수 있겠죠.

보통 url주소를 받고 DB에 바로 insert할텐데 이렇게되면 변경에 취약해집니다.

저장소를 바꿀 때 전체 마이그레이션을 진행해야겠죠

이해됐습니다!

rrgks6221 · 2024-01-05T10:24:48Z

src/apis/users/services/users.service.ts

+    try {
+      const entityManager = queryRunner.manager;
+
+      const major = await this.majorRepository.findOne({


Service가 아닌 repository를 사용한건 아직 다른 pr이 머지되지 않아서죠??

넵 맞습니다!

rrgks6221 · 2024-01-05T10:25:03Z

src/apis/users/services/users.service.ts

+      const entityManager = queryRunner.manager;
+
+      const major = await this.majorRepository.findOne({
+        select: ['id'],


오 이런 방법도 있었군요

rrgks6221 · 2024-01-05T10:26:10Z

src/apis/users/services/users.service.ts

+        .withRepository(this.userRepository)
+        .update(
+          { id: userId, status: UserStatus.Active },
+          { ...putUpdateUserDto },


객체를 바로 안넘겨주고 스프레드로 새로운 객체릉 만들어 넘겨주는 이유가 있나요??

정확한 답변이 될 지는 모르겠지만 update 메서드의 partialEntity 파라미터는 _QueryDeepPartialEntity로 대충 User 엔티티의 프로퍼티를 optional하게 받는데
putUpdateUserDto가 PutUpdateUserDto로 타입이 달라서 에러가 나고
{ email: putUpdateUserDto.email }
이런 식으로 하나하나 프로퍼티를 지정해줘야 하는데 스프레드 연산자를 사용하면
{ email: asdf@asdf, name: 'asdf' }
이렇게 작성하는 것처럼 키-값 쌍을 개별적으로 전달하게 돼서 타입 에러가 발생하지 않게 됩니다.
제가 지식이 부족해서 머리속에 있는 것을 뭔가 정확하게 용어를 쓰면서 설명을 하기가 힘드네요 ㅠㅠ

타입에러가 발생하는지는 모르겠는데 다른 함수에 객체를 넘겨줄 때 보통 스프레드 연산자를 사용합니다.
호출하는 함수 내부에서 객체를 변경할 경우 기존의 객체가 변경이 일어날 수 있기 때문이죠.
이런 함수의 특성을 함수의 순수성이라고 부르는데 한번 참고하시면 좋을거같아요.

src/apis/users/services/users.service.ts

rrgks6221 · 2024-01-05T10:29:25Z

src/apis/users/services/users.service.ts

+
+        throw new HttpInternalServerErrorException({
+          code: COMMON_ERROR_CODE.SERVER_ERROR,
+          ctx: '유저 생성 중 알 수 없는 에러',


복붙 검거

이런 젠장

rrgks6221 · 2024-01-08T08:08:44Z

src/apis/users/dto/put-update-user.dto.ts

+  grade: number;
+
+  @ApiProperty({
+    description: 'url 이 아닌 profile path',


ex) https://dongurami.s3.com/user/1/profile_image.jpeg => profile_image.jpeg
이렇게 받는 이유는 아래와 같아요.

클라이언트측에서 저장소에 대한 주소를 알 필요가 없어요

그리고 어떤 path로 보내는것도 알 필요가 없어요. s3 기준으로 버킷관리는 백엔드의 범위

이거는 presigned url처럼 클라이언트측에서 저장소에 바로 업로드한 뒤 이미지 파일 자체가 아닌 path를 보내주는 경우에 해당합니다.

그러면 여기서 클라이언트측에서 파일에 접근할 수 있는 url(s3 주소 포함)을 알 수 있다는 전제가 생기는데 파일(사진)에 접근할 때 s3를 통해 접근한다는 가정이 필요합니다. cloudFront같은 cdn을 통해 접근할 수 있겠죠.

보통 url주소를 받고 DB에 바로 insert할텐데 이렇게되면 변경에 취약해집니다.

저장소를 바꿀 때 전체 마이그레이션을 진행해야겠죠

src/apis/users/dto/put-update-user.dto.ts

rrgks6221 · 2024-01-08T08:11:50Z

src/apis/users/services/users.service.ts

+        .withRepository(this.userRepository)
+        .update(
+          { id: userId, status: UserStatus.Active },
+          { ...putUpdateUserDto },


타입에러가 발생하는지는 모르겠는데 다른 함수에 객체를 넘겨줄 때 보통 스프레드 연산자를 사용합니다.
호출하는 함수 내부에서 객체를 변경할 경우 기존의 객체가 변경이 일어날 수 있기 때문이죠.
이런 함수의 특성을 함수의 순수성이라고 부르는데 한번 참고하시면 좋을거같아요.

rrgks6221 · 2024-01-08T08:22:03Z

src/apis/users/controllers/users.controller.ts

+  @ApiUsers.PutUpdate({ summary: '유저 정보 put 업데이트' })
+  @SetResponse({ type: ResponseType.Detail, key: 'user' })
+  @UseGuards(JwtAuthGuard)
+  @Put()


이전 리뷰때 말씀 못드렸는데 :userId path가 추가되야할거같아요.
path를 추가하지 않는다면 auth로 들어가는게 맞을듯합니다.
path랑 method만 보면 아래를 의미하게될듯합니다.

PUT /auth => 현재 로그인한 계정의 어떠한 값을 바꾼다.

path에 리소스가 특정됐지만 auth라는 path자체가 인증(인증된 사람)이기때문

PUT /users => 존재하는 모든 user의 정보를 바꾼다.

path에 어떤 리소스에 접근하는지 특정되지 않았기 때문

PUT /users/:userId => userId에 해당하는 user의 정보를 바꾼다.

로그인한 사람이 본인인지에 대한 로직도 추가돼야할듯합니다.

summary: '유저 정보 put 업데이트'

put을 안넣어도 될 것 같습니다

반영하겠습니다.

ohchanghoon · 2024-01-09T07:02:42Z

src/apis/users/controllers/users.controller.ts

+  @ApiUsers.PutUpdate({ summary: '유저 정보 put 업데이트' })
+  @SetResponse({ type: ResponseType.Detail, key: 'user' })
+  @UseGuards(JwtAuthGuard)
+  @Put()


summary: '유저 정보 put 업데이트'

put을 안넣어도 될 것 같습니다

ohchanghoon · 2024-01-09T07:08:27Z

src/apis/users/services/users.service.ts

+          { id: userId, status: UserStatus.Active },
+          { ...putUpdateUserDto },


Suggested change

{ id: userId, status: UserStatus.Active },

{ ...putUpdateUserDto },

{ id: userId, status: UserStatus.Active, ...putUpdateUserDto },

이렇게 되나요 ?

typeorm update method 첫번째 파라미터가 condition이고 두번째 파라미터가 updateData여서 통합은 안될거예요

hobiJeong · 2024-01-17T06:05:31Z

src/apis/users/controllers/users.controller.ts

+  @ApiUsers.PutUpdate({ summary: '유저 정보 put 업데이트' })
+  @SetResponse({ type: ResponseType.Detail, key: 'user' })
+  @UseGuards(JwtAuthGuard)
+  @Put()


반영하겠습니다.

hobiJeong · 2024-01-17T08:28:48Z

src/apis/users/dto/put-update-user.dto.ts

@@ -0,0 +1,66 @@
+import { ApiProperty } from '@nestjs/swagger';


null이던 필드들을 그대로 null로 유지시키기 위해 null을 허용하게 되면 그건 PATCH 메서드랑 엄연히 다르긴 하겠지만 뭔가 경계가 허물어지는 느낌이 드는데 아닐까요?

hobiJeong · 2024-01-17T08:32:08Z

src/apis/users/dto/put-update-user.dto.ts

+  grade: number;
+
+  @ApiProperty({
+    description: 'url 이 아닌 profile path',


이해됐습니다!

hobiJeong · 2024-01-17T11:44:42Z

src/apis/users/services/users.service.ts

+    const myId = user.id;
+
+    if (!(myId === userId)) {
+      throw new HttpForbiddenException({
+        code: COMMON_ERROR_CODE.PERMISSION_DENIED,
+      });
+    }


어차피 가드를 통해서 로그인한 유저를 검사하고, 요청한 userId랑 로그인한 user의 id값이 다르면 에러를 뱉으니까 기존에 user에 대해서 검사하던 로직이 불필요한 것 같아서 수정해봤는데 어떤지 봐주시면 감사하겠습니다.

컨트롤러 영역에서 가지고있는 변수에 의존하는거보다는 DB조회를 한번 더 하더라도 user를 조회한 뒤 예외처리를 하는게 나을듯합니다.

AuthStretegy 에서 req.user 에 user정보를 넣어줘서 이 로직이 가능한데 AuthStretegy가 변경되면 ctrl, service 까지 변경돼야합니다.

param으로 들어온 userId에 대한 정보가 암시적으로만 있다고 판단되게 됩니다.

아래 두 전제가 통과돼야 있다고 판단 가능

myid === userId

guard에서 myId에 대한 유저가 있는지 검사

resource not found에 대한 에러 케이스가 사라지게 됩니다.

물론 기능상 문제는 없고 성능과 코드 퀄리티의 트레이드오프긴한데 이 경우는 보통 한번 더 조회를 합니다.

반영하겠습니다!

rrgks6221 · 2024-01-18T02:12:55Z

src/apis/users/dto/put-update-user.dto.ts

@@ -0,0 +1,66 @@
+import { ApiProperty } from '@nestjs/swagger';


우선 클라이언트측 사용처를 아직 모르는게 가장 큰 점이긴한데
PUT과 PATCH 메서드의 주요 차이점은
PUT: 업데이트 기능을 제공할 수 있는 모든 필드를 한번에 수정할 수 있다. (있으면 수정 없으면 생성은 다른 얘기)
PATCH: 업데이트 기능을 제공할 수 있는 필드를 선택적으로 수정할 수 있다.

interface User { // 수정불가 필드 id: number; // 수정 가능 필드 name: string | null; // 수정가능 필드 nickname: string; }

이런 인터페이스를 가진 객체가 있다고 가정했을 때

request 필드는 아래처럼 정의돼야한다고 생각하긴 해요.

// 들어온 값 그대로 유효성 검사 후 DB에 밀어넣음 interface PutUpdateUser { name: string | null; nickname: string; } // name이나 nickname이 들어오지 않았다면 업데이트 대상이 아님 interface PatchUpdateUser { name?: string | null; nickname?: string; }

rrgks6221 · 2024-01-18T02:27:18Z

src/apis/users/dto/put-update-user.dto.ts

+
+  @ApiProperty({
+    description:
+      'url 이 아닌 profile path. ex) https://dongurami.s3.com/user/1/profile_image.jpeg => profile_image.jpeg',


생각해보니 이 부분은 클라이언트측이랑도 얘기해봐야하는 부분이라 넘어가고 주석으로 수정될 수 있다는 표시만 해주면 좋을거같스빈다.

넵 반영하겠습니다!

rrgks6221 · 2024-01-18T02:29:25Z

src/apis/users/controllers/users.swagger.ts

+        ...apiOperationOptions,
+      }),
+      ApiBearerAuth(),
+      ApiParam({


@param 데코레이터가 붙은 아이들은 nest에서 내부적으로 swagger에 등록해줘서 굳이 안써줘도 되긴 합니다.

실제로 스웨거 들어가봤는데 Param 등록이 자동으로 안되있길래 직접 등록해줬습니다.

아 그게 아니라 제가 param 데코레이터에 토큰을 안넣어줘서 자동으로 등록이 안됐었나 봅니다. 지금은 잘 등록되는거 확인하고 지웠습니다!!

rrgks6221 · 2024-01-18T02:34:58Z

src/apis/users/services/users.service.ts

+    const myId = user.id;
+
+    if (!(myId === userId)) {
+      throw new HttpForbiddenException({
+        code: COMMON_ERROR_CODE.PERMISSION_DENIED,
+      });
+    }


컨트롤러 영역에서 가지고있는 변수에 의존하는거보다는 DB조회를 한번 더 하더라도 user를 조회한 뒤 예외처리를 하는게 나을듯합니다.

AuthStretegy 에서 req.user 에 user정보를 넣어줘서 이 로직이 가능한데 AuthStretegy가 변경되면 ctrl, service 까지 변경돼야합니다.

param으로 들어온 userId에 대한 정보가 암시적으로만 있다고 판단되게 됩니다.

아래 두 전제가 통과돼야 있다고 판단 가능

myid === userId

guard에서 myId에 대한 유저가 있는지 검사

resource not found에 대한 에러 케이스가 사라지게 됩니다.

rrgks6221 · 2024-01-18T02:35:24Z

src/apis/users/services/users.service.ts

+  ) {
+    const myId = user.id;
+
+    if (!(myId === userId)) {


Suggested change

if (!(myId === userId)) {

if (myId !== userId) {

보통 이렇게 많이 처리하는데 저렇게 짠 이유가 있나요?

아.. 그러네요 원래 저렇게 안짜는데 너무 복잡하게 생각하면서 짜서 저렇게 된 것 같습니다.

rrgks6221 · 2024-01-18T02:37:12Z

src/apis/users/services/users.service.ts

+  async putUpdate(
+    user: UserDto,
+    userId: number,
+    putUpdateUserDto: PutUpdateUserDto,
+  ) {


UsersService.putUpdate 라는 메서드만 봤을 때 updateDto가 있는데 user라는 프로퍼티가 있는게 어떤 의미인지 와닿지가 않네요

rrgks6221 · 2024-01-18T02:38:42Z

src/apis/users/services/users.service.ts

+    const myId = user.id;
+
+    if (!(myId === userId)) {
+      throw new HttpForbiddenException({
+        code: COMMON_ERROR_CODE.PERMISSION_DENIED,
+      });
+    }


물론 기능상 문제는 없고 성능과 코드 퀄리티의 트레이드오프긴한데 이 경우는 보통 한번 더 조회를 합니다.

rrgks6221 · 2024-01-18T02:39:26Z

src/apis/users/controllers/users.swagger.ts

@@ -64,4 +64,41 @@ export const ApiUsers: ApiOperator<keyof UsersController> = {
      ]),
    );
  },
+  PutUpdate: (


403에 대한 정의가 빠졌습니다.
구현체에는 404케이스가 없는데 스웨거에는 있네요

반영하겠습니다.

hobiJeong added 10 commits January 4, 2024 16:04

feat/#110: create put update api

2fadf98

conflict/#110: solve conflict

5ff5727

feat/#110: add parameter

f37cccc

feat/#110: create dto and service code

246689d

feat/#110: modify dto property

fa68161

modify/#110: modify dto

b326c83

feat/#110: add validator in dto

910022e

feat/#110: create user put update api

0954ac4

feat/#110: set response and create swagger api docs

71d3855

modify/#110: fix error

79ddc9f

hobiJeong added the Feat 새로운 기능 추가 label Jan 5, 2024

hobiJeong requested a review from ohchanghoon January 5, 2024 04:40

hobiJeong self-assigned this Jan 5, 2024

hobiJeong requested a review from rrgks6221 as a code owner January 5, 2024 04:40

hobiJeong linked an issue Jan 5, 2024 that may be closed by this pull request

user put update #110

Open

1 task

rrgks6221 reviewed Jan 5, 2024

View reviewed changes

hobiJeong added 3 commits January 7, 2024 00:06

modify/#110: remove db find logic

8645792

conflict/#110: solve conflict

eaeb905

refactor/#110: modifiy error message

7dd7365

hobiJeong requested a review from rrgks6221 January 8, 2024 06:21

rrgks6221 requested changes Jan 8, 2024

View reviewed changes

ohchanghoon reviewed Jan 9, 2024

View reviewed changes

hobiJeong added 3 commits January 13, 2024 16:37

conflict/#110: solve conflict

ef8c618

feat/#110: add path variable

8dae047

modify(#110): modify logic

d6fd287

hobiJeong commented Jan 17, 2024

View reviewed changes

hobiJeong requested review from ohchanghoon and rrgks6221 January 17, 2024 11:45

rrgks6221 reviewed Jan 18, 2024

View reviewed changes

modify/#110: modify api logic, swagger and update dto

746ecee

refactor/#110: remove unused import

4af61cf

hobiJeong requested a review from rrgks6221 January 18, 2024 03:53

rrgks6221 approved these changes Jan 18, 2024

View reviewed changes

ohchanghoon approved these changes Jan 18, 2024

View reviewed changes

hobiJeong merged commit b0c6844 into develop Jan 19, 2024

hobiJeong deleted the feat/#110/user_put_update branch January 19, 2024 01:14

		@@ -0,0 +1,66 @@
		import { ApiProperty } from '@nestjs/swagger';

		{ id: userId, status: UserStatus.Active },
		{ ...putUpdateUserDto },

	{ id: userId, status: UserStatus.Active },
	{ ...putUpdateUserDto },
	{ id: userId, status: UserStatus.Active, ...putUpdateUserDto },

user put update api #117

user put update api #117

Conversation

hobiJeong commented Jan 5, 2024

Description

To Reviewer

Reference Link

Related Issue Link

API

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrgks6221 Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hobiJeong Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrgks6221 Jan 8, 2024 •

edited

Loading

hobiJeong Jan 6, 2024 •

edited

Loading