複合キーがnilの場合にPreloaderが余分なクエリを発行するバグを修正
ActiveRecord::Associations::Preloaderを直接呼び出す際、複合キーの値がnilの場合でも余分なクエリが発行されていたバグが修正されました。owners_by_keyメソッドにおけるnilチェックが複合キー(Array)を考慮していなかったことが原因です。
背景
ActiveRecord::Associations::Preloaderを直接呼び出すコードで、複合キーの一部またはすべての値がnilのレコードが存在する場合、不要なクエリが1件余分に発行されるという問題がありました。通常の単一キーではnilチェックが正しく機能していましたが、複合キーの場合は値がすべてnilであってもArrayオブジェクト自体はtruthy扱いとなるため、チェックを通過してしまっていました。
この問題は、ActiveRecord::Associations::BelongsToAssociationがすでにArray(reflection.foreign_key).all?というパターンでnilを正しく処理していたことからも、意図しない挙動であることがわかります。今回の修正はその設計方針と一致させるバグ修正と位置づけられています。
技術的な変更
変更はactiverecord/lib/active_record/associations/preloader/association.rb内のowners_by_keyメソッド1行のみです。
変更前:
def owners_by_key
@owners_by_key ||= owners.each_with_object({}) do |owner, result|
key = derive_key(owner, owner_key_name)
(result[key] ||= []) << owner if key
end
end
変更後:
def owners_by_key
@owners_by_key ||= owners.each_with_object({}) do |owner, result|
key = derive_key(owner, owner_key_name)
(result[key] ||= []) << owner if key.is_a?(Array) ? key.all? : key
end
end
キーがArray(複合キー)である場合はkey.all?で全要素がtruthy(nil以外)かを確認し、単一キーの場合は従来通りそのままtruthy評価を行います。Array(key).all?という統一した書き方も検討されましたが、PR作者によるベンチマーク(1000イテレーション)の結果、Array()へのキャストが単一キーの通常パスで約50%のオーバーヘッドを生じさせることが確認されたため、is_a?(Array)による分岐が採用されています。
テストはPreloaderTestとEagerAssociationTestの両クラスに追加されました。具体的には以下のケースがカバーされています:
- 複合外部キーを持つ未永続化レコード(
Cpk::Book.new)に対するプリロードがクエリを発行しないこと - 複合キーのbelongs_to関連がnilの場合、プリロードが余分なクエリを発行しないこと
- 複合キーのhas_many関連で、ビルド済みレコードがプリロード後も保持されること
設計判断
パフォーマンスを考慮した条件分岐が意図的に選択されています。
最もシンプルな実装はArray(key).all?で単一キーと複合キーを統一的に扱うことですが、ベンチマーク結果によると通常パス(非複合キー)で0.185ms→0.280msと約51%遅くなります。一方、is_a?(Array)による分岐では通常パスのオーバーヘッドはほぼゼロ(0.185ms→0.190ms)に抑えられています。owners_by_keyはプリロード処理の中核にあり、多数のレコードを処理する際に繰り返し呼ばれるため、共通パスのパフォーマンスを優先する判断は妥当です。
変更が1行にとどまっている点も重要で、既存の単一キーパスには一切手を加えず、複合キーの場合のみ追加チェックを適用する最小限の修正になっています。
まとめ
本PRは、owners_by_keyのnilチェックが複合キー(Array)のtruthy性を見落としていたという1行のバグを修正したものです。変更そのものは小さいながら、既存のBelongsToAssociationの設計方針との一貫性を取り戻しつつ、通常パスのパフォーマンスを犠牲にしない丁寧な実装判断が光る修正です。