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

User Proto 업데이트 #1

Merged
merged 2 commits into from
Jan 27, 2024
Merged

User Proto 업데이트 #1

merged 2 commits into from
Jan 27, 2024

Conversation

jyoo0515
Copy link
Member

@jyoo0515 jyoo0515 commented Jan 27, 2024

  • Lint 룰 적용
  • User DTO 생성
  • id 로 string 사용하기
  • optional 한 필드는 optional 처리하기

proto/user.proto Outdated
Comment on lines 17 to 25
// message CreateUserRequest {
// string name = 1;
// int age = 2;
// string disorderType = 3;
// }

// message CreateUserResponse {
// int id = 1;
// }
Copy link
Member Author

Choose a reason for hiding this comment

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

@namooplus @C-limlim Firebase Authentication 에 구글 로그인 사용하면 따로 CreateUser API 는 필요 없을거 같은데 필요한가욤??

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.

Copy link
Member

Choose a reason for hiding this comment

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

만약 클라이언트 상에서 firebase authentication을 직접 참조하지 않고 서버 거쳐서 작업하면 필요할지도?

Copy link
Member Author

Choose a reason for hiding this comment

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

보통은 클라에서 firebase auth 진행 -> 나오는 id token 을 서버로 올려줌 -> 서버에서는 firebase admin sdk (모든 firebase 서비스에 접근 가능한 sdk) 로 올라온 token 의 validity 체크 순으로 진행되는 것 같던데

이런 방법도 있긴 하네요.

message GetUserRequest {
int id = 1;
string id = 1;
Copy link
Member

Choose a reason for hiding this comment

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

@jyoo0515 id 다 string으로 할건가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

그 firebase authentication 을 쓰면 거기서 주는 id 가 string 입니당

string id = 1;
optional string name = 2;
optional int32 age = 3;
DisorderType disorder_type = 4;
Copy link
Member

Choose a reason for hiding this comment

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

DisorderType도 optional 일 것 같은데 어떻게 생각하시나요

Copy link
Member Author

Choose a reason for hiding this comment

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

따로 message 로 정의한 애들은 자동으로 optional 입니다! (primitive 가 아니고 object 느낌으로다가) 근데 명시적인게 더 좋으면 optional prefix 붙여도 좋을듯

Copy link
Member

Choose a reason for hiding this comment

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

아하 배우고 갑니다~ b 안붙이는걸로 하겠습니당

Copy link
Member

Choose a reason for hiding this comment

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

그럼 id에는 required 붙여야하지 않나여

Copy link
Member

Choose a reason for hiding this comment

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

put의 역할을 할까요 patch의 역할을 할까요

Copy link
Member Author

@jyoo0515 jyoo0515 Jan 27, 2024

Choose a reason for hiding this comment

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

그럼 id에는 required 붙여야하지 않나여

이게 역사가 있는데 처음에는 requiredoptional 키워드가 있다가 -> 둘다 없어졌다가 -> 그중에 optional 만 돌아옴 ㅋㅋㅋㅋ

따라서 기본적으로 optional 안붙어있는 애들은 required 라고 보면 됨! optional 키워드 안쓰고 아까 말한거처럼 따로 message 를 정의한 애들은 기본적으로 nullbale 이라는 점을 이용한 wrapper라는 애들이 있긴 한데 사용성이 안좋아서 (StringValue user_id = 1 이면 userId.value 로 꺼내써야 함) optional 을 붙였습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

put의 역할을 할까요 patch의 역할을 할까요

patch 로 생각하긴 했는데 put 이 편하면 put 으로 가도 됩니다~

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

Choose a reason for hiding this comment

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

patch 좋슴돠

Copy link
Member

@C-limlim C-limlim left a comment

Choose a reason for hiding this comment

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

lgtm 멋져요

Copy link
Member

@C-limlim C-limlim left a comment

Choose a reason for hiding this comment

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

lgtm 멋져요

Copy link
Member

@namooplus namooplus left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

string id = 1;
optional string name = 2;
optional int32 age = 3;
DisorderType disorder_type = 4;
Copy link
Member

Choose a reason for hiding this comment

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

그럼 id에는 required 붙여야하지 않나여

string id = 1;
optional string name = 2;
optional int32 age = 3;
DisorderType disorder_type = 4;
Copy link
Member

Choose a reason for hiding this comment

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

put의 역할을 할까요 patch의 역할을 할까요

@jyoo0515 jyoo0515 merged commit e8658ea into main Jan 27, 2024
1 check passed
@jyoo0515 jyoo0515 deleted the jason/20240127-update-user-proto branch January 27, 2024 10:09
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.

3 participants