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のレスポンスにタグ名を追加 #115

Merged
merged 8 commits into from
Oct 19, 2024

Conversation

kuri0616
Copy link
Collaborator

@kuri0616 kuri0616 commented Oct 17, 2024

対応するissue

対応内容

  • メモ一覧API
    • Queryモジュールのcallメソッドにメモに紐づくタグを取得し、レスポンスに含める処理を追加
  • メモ詳細API
    • includesでメモに紐づくタグ名を取得し、j builderでタグ名をレスポンスに含める処理を追加
  • テスト、仕様書
    • Callメソッドのテストの期待値にタグの名前を追加
    • Openapi仕様書にタグ名の仕様を追加、不要なレスポンスの定義を削除

確認したいこと、聞きたいこと

  • タグの取得方法について、includesを使用して取得するようにしましたが、outerjoin を使って1回のクエリで取得する方が良いのか等、パフォーマンス面また処理の記載箇所、分け方など気になる点などあれば指摘いただきたいです💦
  • Rspecの期待値にタグの名前を追加するようにしたのですが、冗長な定義になってしまったこと、callメソッド内でレコードを取得するためにlet!で即時評価にしたのですが、そのことにより、memoテーブルに3件レコードが追加されてしまい、後のpage nationのテストで23件となり、total_pageが3件になってしまう副作用が出てしまいました。
    context毎に期待値を定義する方法も検討したのですが、全てのcontextブロックに記載するとコード量も多くなってしまうかつ他の解決策が思いつかなかったため、とりあえず3件にしています。

@kuri0616 kuri0616 added the backend バックエンドのissues label Oct 17, 2024
@kuri0616 kuri0616 requested a review from kakeru-one October 17, 2024 07:46
@kuri0616 kuri0616 self-assigned this Oct 17, 2024
Copy link
Contributor

@kakeru-one kakeru-one left a comment

Choose a reason for hiding this comment

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

@kuri0616
コメントしました! 🙏

@@ -10,7 +10,7 @@ def index

# GET /memos/:id
def show
@memo = Memo.find(params[:id])
@memo = Memo.includes(:tags).find(params[:id])
Copy link
Contributor

@kakeru-one kakeru-one Oct 17, 2024

Choose a reason for hiding this comment

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

Nits

includesはeager_loadにもpreloadの挙動にもなり得るので、この場合はpreloadを使用するようにしましょう!

Suggested change
@memo = Memo.includes(:tags).find(params[:id])
@memo = Memo.preload(:tags).find(params[:id])

Ref: https://tech.stmn.co.jp/entry/2020/11/30/145159#:~:text=%E3%81%AB%E3%81%AA%E3%82%8A%E3%81%BE%E3%81%99%E3%80%82-,preload%E3%80%81eager_load%E3%81%AE%E4%BD%BF%E3%81%84%E5%88%86%E3%81%91,-%E4%B8%8A%E8%BF%B0%E3%81%AE%E5%86%85%E5%AE%B9


へえ、直接関連していなくても、has_many throughの記述があれば先読みできるんですね!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

includesはクエリによって挙動がかわるのですね!
知らなかったです。
結合で一括取得か、テーブル毎にin句で取得か、今回は、関連先の要素で絞り込みを行ったりしないし、preloadのが良いということですかね!
めちゃくちゃ勉強なりました!

Copy link
Contributor

Choose a reason for hiding this comment

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

結合で一括取得か、テーブル毎にin句で取得か、今回は、関連先の要素で絞り込みを行ったりしないし、preloadのが良いということですかね!

確かに基本的にLEFT JOINすると重複が生まれるためにpreloadの方が速そうに思えますが、
必ずしもそうでないわけではないので、毎回preloadがいいわけではないとは思います!
また、あまりにもIN句が大きすぎるとSQL文のbyte数が大きくなっちゃってのlimitに引っかかることもあります。
Ref: https://moroto1122.hatenadiary.org/entry/20100805/1280992251

Copy link
Contributor

Choose a reason for hiding this comment

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

preloadとeager_loadの速度を比較したいなら、benchmarkとってみるといいと思います。
(「推測するな、計測せよ」の精神です。)
https://qiita.com/scivola/items/c5b2aeaf7d67a9ef310a

@@ -37,7 +37,11 @@ def call(filter_collection:, params:)
params: params
)
memo_count = memo_relation.count
{ memos: PageFilter.resolve(scope: memo_relation, params: params),
paginated_memos = PageFilter.resolve(scope: memo_relation, params: params)
memo_with_tags = paginated_memos.includes(:tags).map do |memo|
Copy link
Contributor

Choose a reason for hiding this comment

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

Nits/IMO

controller側でMemo.allする代わりにMemo.preload(memo_tags: :tags)するようにしましょうか。
また、jbuilderを使用するようにしましょう。

# GET /memos
def index
  @memos = Memo::Query.call(filter_collection: preload(memo_tags: :tags), params: params), status: :ok
rescue TypeError
  render json: { error: 'ページパラメータが無効です' }, status: :bad_request
end

(ちなみにhashに変換するのは完全な悪手というわけではなくて、場合によってはメモリ効率的に良かったりすることもあります。)


なので、ここではmapを実行する必要はないかなと思っていまして、
以下のようにMemoTagクラスにdelegateの記述をするといいと思っています。

MemoTagの記述

class MemoTag
  delegate :name, to: :tag, prefix: true
end

jbuilderの記述

json.tag_names @memo.tags.map(&:tag_name)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

jbuilderの存在、完全に忘れておりました・・・
モデルのメソッド側でフォーマットしなくて良いのも利点ですね!

 delegate :name, to: :tag, prefix: true

delegateも恥ずかしながら初めて知りました・・・
指定したモデルのメソッドを譲渡できるんですね、prefixもオプション指定で自動でつけてくれるのも便利ですね

@@ -8,6 +8,8 @@ json.memo do
json.created_at @memo.created_at
json.updated_at @memo.updated_at

json.tag_names @memo.tags.map(&:name)
Copy link
Contributor

Choose a reason for hiding this comment

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

delegate使うと以下のような記述になるかなと。

Suggested change
json.tag_names @memo.tags.map(&:name)
json.tag_names @memo.tags.map(&:tag_name)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

こちら

delegate :name, to: :tag, prefix: true

の対応なんですが
memoTagモデルで上記を定義すれば
下記のようにmemoTagモデルからtagモデルのnameメソッドをtag_nameというメソッドで呼べるようになるという感じですかね?

memo.memo_tags.map(&:tag_name)

今回はおそらくhas_many throughの定義によってmemoのインスタンスから直接tagsにアクセスかつnameメソッドを呼べるからいらないという認識であっていますかね?

Copy link
Contributor

Choose a reason for hiding this comment

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

ごめんなさい、コメント雑にしちゃってました!
今回はこのままで良いですね!

Comment on lines 119 to 141
let(:firts_expected_memo) do
{
'id' => memos[0].id,
'title' => memos[0].title,
'content' => memos[0].content,
'poster' => memos[0].poster,
'created_at' => memos[0].created_at,
'updated_at' => memos[0].updated_at,
:tag_names => memos[0].tags.map(&:name)
}
end
let(:second_expected_memo) do
{
'id' => memos[1].id,
'title' => memos[1].title,
'content' => memos[1].content,
'poster' => memos[1].poster,
'created_at' => memos[1].created_at,
'updated_at' => memos[1].updated_at,
:tag_names => memos[1].tags.map(&:name)
}
end
let!(:third_expected_memo) do
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO/Nits

わざわざletで定義するほどでもないかなとおもいました。
また、letはどちらかというと、テストに必要な初期データを用意するイメージで、期待する結果をletで用意するのはあまり見ないかなと思ってます。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

letはどちらかというと、テストに必要な初期データを用意するイメージで、期待する結果をletで用意するのはあまり見ないかなと思ってます。
たしかに、今までも期待値はcontext内で作っていましたね💦
tag_nameの配列もcallメソッドではなくjbuilderでするようになったので、モデルのテストではなくコントローラーのテストで検証するようにしました!

Comment on lines 20 to 22
after :create do |memo|
create_list(:memo_tag, 3, memo: memo)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

traitで定義してあげたほうがいいと思いました。
(optionalでmemo_tagを作成するようにした方がパフォーマンスがいい。)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

たしかに全てのテストケースでtagのレコード作成する必要ないですもんね
trait 使えば指定したシンボルを記載したときだけ、作成するようにできるんですね!

Copy link
Contributor

@kakeru-one kakeru-one left a comment

Choose a reason for hiding this comment

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

@kuri0616
コメントしましたが、LGTMです!


context 'ログイン中かつメモが存在し、パラメータが指定されていない場合' do
before { sign_in(user) }

it '降順で、1ページ目に10件のメモが返ること' do
aggregate_failures do
get '/memos'
get '/memos', headers: { Accept: 'application/json' }
Copy link
Contributor

Choose a reason for hiding this comment

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

質問

このリクエストヘッダー、必要ですかね?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

このヘッダー僕も不要だと思っていたのですが、Rspecのテスト&Postmanで動作確認したときにPostmanでは、正常に成功、Rspecでは、500が返ってきまして💦

binding.pryでデバックしたところ
get '/memos'
でエンドポイント叩いたとき
headers明示的に指定しなかった場合
indexアクションの
render 'index'
のところでjbuilderのファイルを探すことができずなっていたんですよね
その時のログ

1.  MemosController GET /memos ログイン中かつメモが存在し、パラメータが指定されていない場合 降順で、1 ページ目に 10 件のメモが返ること
    Failure/Error: assert_response_schema_confirm(200)

        Committee::InvalidResponse:
          Expected `200` status code, but it was `500`.
        # /usr/local/bundle/gems/committee-5.1.0/lib/committee/test/methods.rb:32:in `assert_response_schema_confirm'
        # ./spec/requests/memos_spec.rb:16:in `block (5 levels) in <top (required)>'
        # ./spec/requests/memos_spec.rb:13:in `block (4 levels) in <top (required)>'

render 部分のログ

[1] pry(#<MemosController>)> render 'index'
ActionView::MissingTemplate: Missing template memos/index, application/index with {:locale=>[:ja], :formats=>[:html], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby, :jbuilder]}.

Searched in:

- "/usr/src/app/app/views"
- "/usr/local/bundle/gems/actiontext-7.2.1/app/views"
- "/usr/local/bundle/gems/actionmailbox-7.2.1/app/views"

ここの
formats=>[:html]
がhtmlになっているので
index.html.[handlersの配列の中にいずれか]
のパス内を探しているため、index.json.jbuilderが見つからず500になっていると判断して、明示的に指定するようにしました💦

Comment on lines +161 to +173
before do
memos_data = Array.new(20) do
{
title: Faker::Lorem.sentence(word_count: 3),
content: Faker::Lorem.paragraph(sentence_count: 5),
poster: Faker::Name.name,
created_at: Time.current,
updated_at: Time.current
}
end

described_class.bulk_import!(memos_data)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

ついでにパフォーマンス向上させてます。

Comment on lines +7 to +42
# そこそこ重いSQLを発行するので、一回だけ呼ばれるようにしたい。
# ref: https://github.com/test-prof/test-prof/blob/master/docs/recipes/let_it_be.md
let_it_be(:memos) do
memos_data = Array.new(20) do
{
title: Faker::Lorem.sentence(word_count: 3),
content: Faker::Lorem.paragraph(sentence_count: 5),
poster: Faker::Name.name,
created_at: Time.current,
updated_at: Time.current
}
end

Memo.bulk_import!(memos_data)
Memo.order(:id).last(20)
end

# そこそこ重いSQLを発行するので、一回だけ呼ばれるようにしたい。
# ref: https://github.com/test-prof/test-prof/blob/master/docs/recipes/before_all.md
before_all do
# Tagの生成
tag_data = Array.new(3) do |n|
{
name: "tag-#{n + 1}",
priority: n + 1,
created_at: Time.current,
updated_at: Time.current
}
end
Tag.bulk_import!(tag_data)
tags = Tag.pluck(:id)

# MemoTagの生成
memo_tags_data = memos.flat_map do |memo|
tags.map do |tag_id|
{
Copy link
Contributor

Choose a reason for hiding this comment

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

ここもついでにパフォーマンス向上させてます。
全体のテスト実行時間は大体半分くらいになってるはずです!

@kakeru-one
Copy link
Contributor

kakeru-one commented Oct 19, 2024

@kuri0616
マージしますね〜
ついでにテストのパフォーマンス向上しといたので良かったら後で見てみてください〜
ちょっと速くなった! 🎉

テスト実行速度の比較

Before

5 sec

After

3.67 sec

やったこと

  • create_list(:memo, 20)にしてる部分をbulk insertにした。
    • As Isでは、memos/memo_tags/tagsテーブルへ1レコードごとにINSERTが走ってたので、一括で挿入するようにした。
  • letブロックとbeforeブロックを一回しか実行されないようにした。
    • As Isでは、毎itブロックでletとbeforeブロックが実行されちゃってた。

参考にした記事: https://blog.nightonly.com/2022/08/17/let_it_be%E3%81%A8before_all%E3%81%A7rspec%E3%81%AE%E5%AE%9F%E8%A1%8C%E6%99%82%E9%96%93%E3%82%92%E7%9F%AD%E3%81%8F%E3%81%99%E3%82%8B/

@kakeru-one kakeru-one merged commit fc7da68 into main Oct 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend バックエンドのissues
Projects
Status: 完了
Development

Successfully merging this pull request may close these issues.

メモに紐づくタグのデータをメモ一覧、メモ詳細取得時のレスポンスに含める
2 participants