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

Fix: post response 응답코드 swagger 명세 수정 #164

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

Yaminyam
Copy link
Member

@Yaminyam Yaminyam commented Apr 6, 2022

바뀐점

ApiOkResponse로 명세된 POST 응답들의 명세를 ApiCreatedResponse로 수정

바꾼이유

#162 /intra-auth 뿐만아닌 post 요청에서 201 응답이 날아오는 부분에 관한 명세를 알맞게 수정

설명

POST /intra-auth의 경우에는 상의 후 201로 명세를 표기 할 지 응답코드를 수정할지 결정

Copy link
Member

@Skyrich2000 Skyrich2000 left a comment

Choose a reason for hiding this comment

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

오홍 수정되어야할부분 콕 찝어주셨군요 좋습니다

이미지는 url만 만드는거라서 201으로 하기엔 조금 애매한거같아요 200으로 충분한거같습니다
리액션도 요청할때마다 생성/삭제 가 반복되는 구조라서 200이 맞는거같습니다

코드 수정이 되어야할부분은 한번 알아보겠습니다

@Yaminyam
Copy link
Member Author

Yaminyam commented Apr 6, 2022

image는 image가 s3에 업로드가 되는게 서버입장에서 새로운 content가 created 된다고 생각해서 201로 작성하였습니다
그런데 이는 api서버가 아닌 s3라는 분리된 서버로 작동하고 api서버에서는 url만 만들어진다고 봐야할까요?
reaction도 그냥 값이 +-1씩 증감하는방식인줄 알았는데 새로운 객체가 생성돼서 들어가는거로 보여서 201로 작성하였습니다
그런데 -일때 삭제되는 경우도 있어서 애매하긴 하군요

@Skyrich2000
Copy link
Member

네 이미지 api는 url만 만들어지는게 맞습니다
리액션도 삭제되는 경우때문에 애매해서 200이라고 생각했습니다

@Skyrich2000
Copy link
Member

생각해보면 리액션은 추가랑 삭제가 동시에 있으니까 PUT 이 좀더 어울릴지도모르겠네요
다른분은 어떻게 생각하시나요? @rockpell @blingblin-g @mimseong @raejun92

@Yaminyam
Copy link
Member Author

Yaminyam commented Apr 6, 2022

Restful에 대해 얕은 지식이지만 restful하려면 post/delete로 분리되어야하는게 아닌가? 싶습니다

@Skyrich2000
Copy link
Member

post/delete으로 하면, 프론트에서 post/delete를 번갈아서 보내줘야하는데 네트워크 지연이 생기면 중복해서 날아올수있어서 합친거같습니다.
이론 : post delete post delete
실제: post delete delete post

@mimseong
Copy link
Contributor

mimseong commented Apr 7, 2022

그러게요 저도 리엑션은 추가/삭제가 동시에 있어서 PUT이 맞다고 생각해요.
-> 아니다 좀 더 찾아볼게요
url, 리엑션은 멘토님 의견도 궁금한데 한번 물어 보는 것도 좋을듯요ㅋㅋ

@mimseong
Copy link
Contributor

mimseong commented Apr 7, 2022

post / put 사용방식 조금 더 찾아보니 이런 차이가 있었네요

POST

  • 리소스를 새로 생성할 때 사용한다
  • 멱등하지 않다
  • N번 반복한다면 N개의 다른 리소스가 만들어진다
  • request url에 정의된 리소스 하위에 생성하고 싶을 때 사용한다 EX /alticles
  • 따라서 request url는 리소스의 Entity를 나타내는 Collection URL이어야 한다

PUT

  • 업데이트에 사용한다
  • 만약 이미 있는 데이터라면 업데이트를 하고 아니라면 생성을 한다
  • 기존 Collection URL /alticles 에 더해 해당 리소스의 Resource Identifier를 나타내줘야 한다 EX alticles/id
  • 멱등해야 한다
  • 여러번 반복하더라도 한번 보낸 결과와 같아야 한다

정리하고 보니 좋아요 누르는 건

  • 좋아요 Collection 하위에 리소스들이 만들어지고 (누가 누구의 게시글에 좋아요를 했는지)
  • 좋아요 id의 값을 업데이트 하는 것도 아니라

POST가 맞는 것 같아요. 그렇담 저도 post delete 나누는 게 좋을 것 같긴 한데 순서 섞이는게 문제니... 정말 멘토님께 조언을...

@Yaminyam
Copy link
Member Author

Yaminyam commented Apr 7, 2022

넵 맞습니다 저도 어제 찾아봤는데 post delete로 분리되는게 가장 확실하긴 한것같습니다 서비스상의 문제가 생길수있으니 그 문제를 해결하고 분리하면 좋을것 같습니다.

@Skyrich2000
Copy link
Member

프론트에서 잘 처리하면 괜찮을거같기도한데... 지금 프론트가 PC 개발하시는것도 벅찰거같은데 어떻게 하는게 좋을지 고민되네요...

@mimseong
Copy link
Contributor

mimseong commented Apr 7, 2022

당장 급한 건 아니라서 이슈 남기고 다음에 변경해도 될듯요

@Skyrich2000
Copy link
Member

금요일에 어떻게 할지 정하고 머지하시죠

Copy link
Member

@rockpell rockpell left a comment

Choose a reason for hiding this comment

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

수고하셨습니다

@rockpell
Copy link
Member

rockpell commented Apr 7, 2022

근데 post / delete로 나눈다고 하더라도 완전 restful 하게 만들기는 어려울거 같네요
post가 여러개 날아오면 첫번째 api 요청 이후의 요청들은 아무 동작도 안하도록 만들어야할거 같은데 그러면 post 요청 하나당 1개의 자원이 생긴다는 규칙을 어기지 않을까 싶습니다

@Yaminyam
Copy link
Member Author

Yaminyam commented Apr 8, 2022

그러면 put / delete의 형태가 되어야겠군요

Copy link
Member

@blingblin-g blingblin-g left a comment

Choose a reason for hiding this comment

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

좋아요 👍

@rockpell
Copy link
Member

rockpell commented Apr 8, 2022

일단 코드는 그대로 두고, 프론트도 수정해야하니까
명세만 맞추는게 맞다고 판단했습니다

@rockpell rockpell merged commit fb68a2a into develop Apr 8, 2022
@rockpell rockpell deleted the fix/post-response branch April 8, 2022 13:25
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.

5 participants