railsにコントリビュートする勉強会

rails.md

第1回の題材はこちら rails/rails#35204

# 勉強会を開催した時のrails/railsのhash値はこちらです
$ git reset --hard 3397924e6697631970bb50c6475d2922656311aa

1. セットアップ

まずは、ActiveRecordにコントリビュートするために、ローカルで全てのテストが通るようにします。

DBのユーザーを作成する

# mysql
GRANT ALL PRIVILEGES ON *.* TO `rails`@`localhost` IDENTIFIED BY '';

# postgresql
$ brew install postgresql
$ pg_ctl -D /usr/local/var/postgres start

# sqlite
$ brew install sqlite

ActiveRecordのセットアップ手順を試す

ローカル環境にrailsをクローンして、テストが通るところまでを行う。

$ git clone https://github.com/rails/rails
$ cd rails
$ bundle install
$ cd activerecord

# DBのセットアップをする
$ bundle exec rake db:mysql:build
$ bundle exec rake db:postgresql:build

# 全てのテストを実行する(これが通ればOK)
$ bundle exec rake

# DBの種類ごとに実行する場合はこちら
$ bundle exec rake test:sqlite3
$ bundle exec rake test:mysql2
$ bundle exec rake test:postgresql

2. コントリビュートの準備をする

今回はbug fixなので、バグを再現するテストを書き、続いて修正する処理を書きます。

失敗するテストを書いて実行する

diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb
index b8b8858733..ccdef6e448 100644
--- a/activerecord/test/cases/associations/inverse_associations_test.rb
+++ b/activerecord/test/cases/associations/inverse_associations_test.rb
@@ -85,6 +85,15 @@ def test_has_many_and_belongs_to_should_find_inverse_automatically_for_sti
     assert_equal post_reflection, author_child_reflection.inverse_of, "The Author reflection's inverse should be the Post reflection"
   end

+  def test_has_one_and_belongs_to_with_non_default_foreign_key_should_not_find_inverse_automatically
+    author = Author.create!(name: 'name')
+    post = Post.create!(writer: author, title: 'hello', body: 'world')
+
+    assert_nil author.post
+    assert_nil post.author_id
+    assert_empty author.post_ids
+  end
+
   def test_has_one_and_belongs_to_automatic_inverse_shares_objects
     car = Car.first
     bulb = Bulb.create!(car: car)
diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb
index a19890a0a5..50b34fdefc 100644
--- a/activerecord/test/models/author.rb
+++ b/activerecord/test/models/author.rb
@@ -8,6 +8,7 @@ class Author < ActiveRecord::Base
   has_many :posts_with_comments, -> { includes(:comments) }, class_name: "Post"
   has_many :popular_grouped_posts, -> { includes(:comments).group("type").having("SUM(legacy_comments_count) > 1").select("type") }, class_name: "Post"
   has_many :posts_with_comments_sorted_by_comment_id, -> { includes(:comments).order("comments.id") }, class_name: "Post"
+  has_many :wrote_posts, class_name: 'Post', foreign_key: 'writer_id'
   has_many :posts_sorted_by_id, -> { order(:id) }, class_name: "Post"
   has_many :posts_sorted_by_id_limited, -> { order("posts.id").limit(1) }, class_name: "Post"
   has_many :posts_with_categories, -> { includes(:categories) }, class_name: "Post"
diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb
index 41a5ea74cf..c569ce577a 100644
--- a/activerecord/test/models/post.rb
+++ b/activerecord/test/models/post.rb
@@ -34,6 +34,7 @@ def greeting
   scope :locked, -> { lock }

   belongs_to :author
+  belongs_to :writer, class_name: 'Author'
   belongs_to :readonly_author, -> { readonly }, class_name: "Author", foreign_key: :author_id

   belongs_to :author_with_posts, -> { includes(:posts) }, class_name: "Author", foreign_key: :author_id
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 23377911d9..ee817f51ce 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -783,6 +783,7 @@

   create_table :posts, force: true do |t|
     t.references :author
+    t.references :writer
     t.string :title, null: false
     # use VARCHAR2(4000) instead of CLOB datatype as CLOB data type has many limitations in
     # Oracle SELECT WHERE clause which causes many unit test failures

テストが失敗することを確認する

$ cd activerecord
$ bundle exec ruby -Itest test/cases/associations/inverse_associations_test.rb
Using sqlite3
Run options: --seed 38924

# Running:

..................................
F

Failure:
AutomaticInverseFindingTests#test_has_one_and_belongs_to_with_non_default_foreign_key_should_not_find_inverse_automatically [test/cases/associations/inverse_associations_test.rb:95]:
Expected #<Post id: 12, author_id: nil, writer_id: 4, title: "hello", body: "world", type: nil, legacy_comments_count: 0, taggings_with_delete_all_count: 0, taggings_with_destroy_count: 0, tags_count: 0, indestructible_tags_count: 0, tags_with_destroy_count: 0, tags_with_nullify_count: 0> to be nil.


rails test test/cases/associations/inverse_associations_test.rb:88

............................................

Finished in 0.458077s, 172.4601 runs/s, 574.1393 assertions/s.
79 runs, 263 assertions, 1 failures, 0 errors, 0 skips

コードを修正する

今回は、kamipoさんのヒントをもとに調査して、コードを修正し、テストが通るようになったことを確認した。

このissueを参照してるここ rails/rails#36708 (comment) でも言及してるんですが、automatic_inverse_ofは改善可能で改善すれば自ずとこの問題も解決されます。具体的には :foreign_key オプションをinverse_of判定に含めることでこれまで判定不能だったinverse_of判定も可能になります。 このissue自体はforeign_key未指定の同一クラス参照してるassociationがある場合にforeign_keyの一致を判定せずにクラスが一致するかどうかだけでinverse_of判定してることが問題なのだと思います。 このへんの話ですね https://github.com/rails/rails/blob/b60838b0ac7a9ba8ef26ef7a6abce70fbcf84f15/activerecord/lib/active_record/reflection.rb#L626-L627

自身で挑戦する場合は↑のヒントだけみて修正してみましょう。 ちなみに答えは次のステップのPRを見れば分かります。

3. PRを作成する

今回 @ktmouk が送ったPRがこちら 🎉 rails/rails#40643

会の最中で話した小さいアドバイスメモ

• 修正内容とテストは同一commitにするのが良い
• (難しいけど)出来る限り他のコードと似た実装に寄せる方が小言を言われない

---

次回、ネタがあればまたやりましょう！