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

アンケート回答者の一括取得api #169

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mumumu6
Copy link
Collaborator

@mumumu6 mumumu6 commented Feb 8, 2025

一括取得できるapiを作成しました。
子質問に対してリクエストをします。子質問のidと回答したユーザーのtraqidの配列を返します。

公開質問でない場合は合宿係じゃないとforbiddenを返しています。

openapi.yaml Outdated
schema:
type: array
items:
$ref: "#/components/schemas/GetQuestionAnswers"
Copy link
Member

Choose a reason for hiding this comment

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

これはAnswerの配列を返すのではダメですか?新しいschemaを定義するのに何か意図があれば教えてほしいです

Suggested change
$ref: "#/components/schemas/GetQuestionAnswers"
$ref: "#/components/schemas/Answer"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Answerの配列だとquestion_idが全部についていて余分かな、と思って作ったんですがakimo君的にないほうがいいならそうします。

openapi.yaml Outdated
@@ -588,6 +588,26 @@ paths:
$ref: "#/components/responses/NotFound"
"500":
$ref: "#/components/responses/InternalServerError"
/api/users/answers/{question_id}:
Copy link
Member

Choose a reason for hiding this comment

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

/api/usersときたらその後にはユーザーのIDが来てほしい感があるので、/api/answers/{question_id}とかで良いかもです(元々僕がやったエンドポイントの命名が全体的に下手なので、どこかで直したいな……)

Suggested change
/api/users/answers/{question_id}:
/api/answers/{question_id}:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

分かりました。修正します!

openapi.yaml Outdated
Comment on lines 1256 to 1257
user_traq_id:
type: string
Copy link
Member

Choose a reason for hiding this comment

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

もしAnswerを使わずにGetQuestionsAnswersを使う必要があるのであれば、ここのuser_traq_idは消したほうが良いです(user_traq_idのようなフィールド名を指定する必要があるのはtype: objectのときで、単純なstringの配列には必要ないです)。このままだと型が正確に解釈されず、[]stringではなく[]interfaceになってしまいます……

Suggested change
user_traq_id:
type: string
type: string

Copy link
Collaborator Author

@mumumu6 mumumu6 Feb 16, 2025

Choose a reason for hiding this comment

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

そうなんですね。いわれてみたら名前を二つ付けるのは無駄でした。ありがとう!!

Comment on lines 361 to 371
res.Answers = new([]interface{})

for _, answer := range answers {
answeredUesr, err := s.repo.GetUserTraqID(answer.UserID)
if err != nil {
e.Logger().Errorf("failed to get user: %v", err)

return echo.NewHTTPError(http.StatusInternalServerError, "Internal server error")
}

*res.Answers = append(*res.Answers, answeredUesr)
Copy link
Member

@akimon658 akimon658 Feb 11, 2025

Choose a reason for hiding this comment

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

スライスのサイズが分かっている場合はmakeで指定したほうが良いです
参考:https://zenn.dev/mkosakana/articles/2867cb1667bfa8

あと、各answerに対してs.repo.GetUserTraqIDを呼ぶことでN+1問題が起きています。最初からtraQ IDを外部キーにしていればこんなことは起きなかったので原因は僕にあるし、修正にはDBのマイグレーションが必要になりそうなので(別にマイグレーションは必要なさそう)とりあえず今は放置で大丈夫なんですが、一応知っておいてほしいです 🙏

Suggested change
res.Answers = new([]interface{})
for _, answer := range answers {
answeredUesr, err := s.repo.GetUserTraqID(answer.UserID)
if err != nil {
e.Logger().Errorf("failed to get user: %v", err)
return echo.NewHTTPError(http.StatusInternalServerError, "Internal server error")
}
*res.Answers = append(*res.Answers, answeredUesr)
res.Answers = make([]string, len(answers))
for i, answer := range answers {
answeredUser, err := s.repo.GetUserTraqID(answer.UserID)
if err != nil {
e.Logger().Errorf("failed to get user: %v", err)
return echo.NewHTTPError(http.StatusInternalServerError, "Internal server error")
}
res.Answers[i] = answeredUser

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ほんとですね、traqid毎回呼び出すこれは良くなさそう。初めて自分でn+1を作成してしまいました...

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.

2 participants