RegularEvent#organizersがhas_manyの関連メソッドを上書きしている

[yokomaru]

概要

• RegularEventにてhas_many :organizersを定義しているが、同名のdef organizersが定義されているため、ActiveRecordの関連メソッドを上書きしている状態になっている
  • 意図的に上書きをしているかどうかは当時のコミットメッセージから読み取れず
• 現状、regular_event.organizersはOrganizerではなくUserを返している

class RegularEvent < ApplicationRecord # rubocop:disable Metrics/ClassLength
  has_many :organizers, dependent: :destroy
  has_many :users, through: :organizers


  def organizers
    users.preload(avatar_attachment: :blob).order('organizers.created_at')
  end

end

発行されるSQLの違い

# メソッドの上書きあり(`organizers`と書くと`User`が引っ張られる)
bootcamp(dev):002> RegularEvent.last.organizers.to_sql
  RegularEvent Load (1.0ms)  SELECT "regular_events".* FROM "regular_events" 
ORDER BY "regular_events"."id" DESC LIMIT 1 /*application='Bootcamp'*/
=> "SELECT "users".* FROM "users" INNER JOIN "organizers" ON "users"."id" = \"organizers\".\"user_id\" 
WHERE \"organizers\".\"regular_event_id\" = 1004488906 ORDER BY organizers.created_at"

# メソッドの上書きなし(`organizers`と書くと`Organizer`が引っ張られる)
bootcamp(dev):004> RegularEvent.last.organizers.to_sql
  RegularEvent Load (0.5ms)  SELECT "regular_events".* FROM "regular_events" 
ORDER BY "regular_events"."id" DESC LIMIT 1 /*application='Bootcamp'*/
=> "SELECT \"organizers\".* FROM \"organizers\" WHERE \"organizers\".\"regular_event_id\" = 1004488906"

問題点

• メソッド名と実体が一致しておらず読み手が混乱しやすい（organizersなのにUserが返る）
• regular_event.organizers.destroy_allなどを書いた場合、意図せずUserが削除されてしまう可能性がある
• また、現状regular_event.usersでorganizersになっているUserを持って来れるが、別のUserとの関連モデルであるParticipation(参加者)もあるため、usersだとどちらを参照しているのかわかりづらい

期待される振る舞い

• regular_event.organizersはOrganizerを返すようにしたい
• 主催者ユーザー一覧が必要なら別名にする（例：organizer_users）
  • 別名のhas_manyアソシエーションを作り必要な情報を引っ張って来れるようにする
  • scopeで定義でも良い？
• 不要になったコードは削除する

対応内容

• OrganizerモデルをRegularEventOrganizerにリネーム
• organizersテーブルをregular_event_organizersに変更
• 関連(has_many / through)を修正
• scopeやorder句などクエリ内のテーブル参照を修正
• organizersメソッド内の参照テーブルを修正(互換性のためメソッドは維持)
• Fixture / seed の名称変更
• 複合インデックスを使用できるため、不要なインデックス(user_id単体)を削除

[torinoko]

当時の経緯がわからないので何ともですが、これは雑事……というか……何というか……
おっしゃっている通り混乱を招く記述なので、Ruby on Rails の実装として普通によくないですね。
既存の処理にどのような影響があるか確認した上で対応したいですね。

[ryufuta]

e3ffcde で上書きされていますね。
定期イベント機能が実装された当初のPRなので初めからこの上書きしたメソッドの方が使用されてしまっています。
コミットの流れを見ると当時の実装者が混乱しただけで意図はないようです。
regular_event.organizersで主催者であるUserが返ってくると思ったら違うものが返ってきたのでUserが返ってくるようにメソッドを追加したのだと推測しています。
（定期イベントの一覧ページと詳細ページに主催者をアバター画像付きで表示したかったのでそれも実現できるようなメソッドにした。がおそらくアバター画像付きはコントローラで対処すれば良いので不要）

この混乱の主要因は中間モデルOrganizerの命名のわかりにくさにあるのかなと思います。
Organizerは主催者（ユーザーの一種）という意味なのでregular_event.organizersで主催者Userが返ってくると思うのは私には英語的に自然に感じました。
参加者との関連付けのための中間モデルはParticipant（参加者）ではなくRegularEventParticipation（定期イベントへの参加）となっていて、regular_event.participantsで参加者Userが返ってくるようになっているため、直感的にわかりやすいです。

1. 中間モデルOrganizerの命名変更（それに合わせて色々変更）
2. has_many :throughを使ってregular_event.organizersで主催者であるUserが返ってくるようにする
3. defで定義した不要なorganizersメソッドを削除

というのが個人的には今後の混乱を防ぐためにも有効かなと思います。（あくまで個人の感覚に基づく提案です）
中間モデルの命名については 交差テーブルには関連の意味を表す名前をつけよう - Qiita が参考になりそうです。

[torinoko]

@ryufuta
あーなるほど、確かに organizers を参照したときに users が返ってくる方が確かに自然ですね。
中間モデルの命名もそれらしくないですしね。
Organizer のモデル名のみ変更であれば、既存コードへの影響も少ない気がします。
（本当はテーブル名も直せたらいいんだろうけどそれはちょっと話が大きくなりそうなので）

[ryufuta]

@torinoko
大規模なサービスだとテーブル名を変更するために徐々にデータを移行するなど面倒な運用が必要だと認識していますが、bootcampアプリの場合はそこまで意識するべきなのかちょっと気になっています。
マイグレーションファイルを見ると過去にテーブル名を変更している例もあります。
モデル名とテーブル名が異なってしまうことによるわかりにくさが生まれることが気がかりです。

[torinoko]

@ryufuta
おっしゃるとおり、regular_events テーブルであれば問題はなさそうです。
モデル名とテーブル名が不一致であることも混乱を招くため、避けたい気持ちは私もあります。
ただ気軽に「えいっ」と変更するものでもないので、段階を踏んで進める必要はあるのかな〜と思っています。

1. モデルの名称変更（モデル内で self.table_name を用いてテーブル名を指定）
2. リリース
3. テーブル名の変更（ migration ファイル作成）
4. リリース

なのでチーム開発メンバー内でその認識を合わせて対応していただけると嬉しいです！

[torinoko]

結論

• Organizer モデルの名前を変更する
• organizers テーブルの名前を変更する

手順

1. モデルの名称変更
   • RegularEvent モデルの修正（self.table_name を用いてテーブル名を指定）
   • Organizer のテストの修正
2. リリース
3. テーブル名の変更（migration ファイル作成）
4. リリース

[yokomaru]

修正手順

• RegularEvent#organizersがhas_manyの関連メソッドを上書きしている #9640 (comment) の手順に則り以下の順番で対応する
  1. Organizerモデルの名前を変更する
  2. organizersテーブルの名前を変更する

やること

Organizer モデルの名前を変更

• OrganizerをRegularEventOrganizerにリネーム
  • self.table_name = 'organizers'で参照テーブルを変更する
• 使用されている箇所を変更
• 定期イベントの作成＋主催者作成されるかを手動で確認
• CIが通ることを確認

対応者

• @yokomaru

PR

• OrganizerをRegularEventOrganizerにリネームした #9715

テーブル名の変更(migration ファイル作成)でやること

• テーブル名を変更するmigrationファイルを作成
• TODOの該当の行を削除
  • app/models/regular_event_organizer.rb
  • db/fixtures/organizers.yml
  • test/fixtures/organizers.yml
• 以下ファイルをregular_event_organizersにrenameする
  • db/fixtures/organizers.yml
  • test/fixtures/organizers.yml
• db/seeds.rbのorganizersをregular_event_organizersに変更
• organizersメソッド内のorderのテーブル名を変更
• 定期イベントの作成＋主催者作成されるかを手動で確認
• CIが通ることを確認

対応者

• @yokomaru

PR

• これから

organizersメソッドはどうするか

• organizersメソッドは依存箇所が多いので一旦そのままにしておき、必要があれば対応

[yokomaru]

質問内容

• Organizersテーブルの名称をRegularEventOrganizersに変えたいが問題ないか

  • 問題ないが、 機能の実装は止まらないようにする(3/4質問タイムにてkomagataさんに確認済み)
  • 以下の順番で取り組む
    • リレーション名を一時的に変更して退会関連の処理を実装してリリースする
    • 実際のリレーションを変更してからリリースする

• モデル名→テーブル名の順番で変えていきたいが問題ないか

  • モデル名とテーブル名は一緒に変更する(3/4質問タイムにてkomagataさんに確認済み)

以下は質問のために整理した内容

前提

• OrganizerはRegularEventとUserの中間モデル(has_many: organizers)
• 今のリレーションだとRegularEvent.organizersとすると通常だと中間モデルOrganizerを返す
• RegularEventモデルに上書くようにorganizersメソッドが定義されているため、Userを返している
  • RegularEventモデルにはorganizersを経由してユーザー情報を返すリレーションusersが存在している

問題点

• 主催者が1人しかいなかったらイベントを終了する、複数人いたら主催者を削除する処理で以下みたいな処理が書きたいが、RegularEvent#organizersがhas_manyの関連メソッドを上書きしている RegularEvent#organizersがhas_manyの関連メソッドを上書きしている #9640 問題が起きていて、今だと↑の処理を書こうとするとuserが削除されてしまう

RegularEvent.organizers.destroy(user: user)

• organizersメソッド経由でUser情報を取得したいという需要はあるので、organizersでusersを返すようにしたい
• RegularEventモデルにはorganizersを経由してユーザー情報を返すリレーションusersが存在しているものの、同じユーザー情報とリレーションがある参加者(paticipation)があり、usersが何を指すのか分かりづらい

  has_many :users, through: :organizers

• Organizerという命名が中間モデルの命名としても少しわかりづらい

  • event、regular_eventがあるのでどっちの？とのもなる

• 参加者テーブルだと以下のような感じ
  - 同じユーザーを返す中間モデルの名称は揃えた方がコードとしてわかりやすい

  has_many :regular_event_participations, dependent: :destroy
  has_many :participants,
           through: :regular_event_participations,
           source: :user

進め方

• 以下のように段階を踏んで進めたい

1. モデルの名称変更（モデル内で self.table_name を用いてテーブル名を指定）
2. リリース
3. テーブル名の変更（ migration ファイル作成）
4. リリース