mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
518b9a301f
Background --- I recently noticed we had a number of associations in GitHub that would benefit from having `inverse_of` set, and so I began adding them. I ended up adding them to virtually every association with a scope, at which point I wondered whether Rails might be able to automatically find these inverses for us. For GitHub, the changes in this commit end up automatically adding `inverse_of` to 171 of associations that were missing it. My understanding is that we do not automatically detect `inverse_of` for associations with scopes because the scopes could exclude the records we are trying to inverse from. But I think that should only matter if there is a scope on the inverse side, not on the association itself. For example: Scope on has_many ---- ```rb class Post < ActiveRecord::Base has_many :comments, -> { visible } end class Comment < ActiveRecord::Base belongs_to :post scope :visible, -> { where(visible: true) } scope :hidden, -> { where(visible: false) } end post = Post.create! comment = post.comments.hidden.create! assert comment.post ``` This code leaves `post.comments` in sort of a weird state, since it includes a comment that the association would filter out. But that's true regardless of the changes in this commit. Regardless of whether the comments association has an inverse, `comment.post` will return the post. The difference is that when `inverse_of` is set we use the existing post we have in memory, rather than loading it again. If there is a downside to having the `inverse_of` automatically set here I'm not seeing it. Scope on belongs_to ---- ```rb class Post < ActiveRecord::Base has_many :comments scope :visible, -> { where(visible: true) } scope :hidden, -> { where(visible: false) } end class Comment < ActiveRecord::Base belongs_to :post, -> { visible } end post = Post.hidden.create! comment = post.comments.create! assert_nil comment.post ``` This example is a different story. We don't want to automatically infer the inverse here because that would change the behavior of `comment.post`. It should return `nil`, since it's scoped to visible posts while this one is hidden. This behavior was not well tested, so this commit adds a test to ensure we haven't changed it. Changes --- This commit changes `can_find_inverse_of_automatically` to allow us to automatically detect `inverse_of` when there is a scope on the association, but not when there is a scope on the potential inverse association. (`can_find_inverse_of_automatically` gets called first with the association's reflection, then if it returns true we attempt to find the inverse reflection, and finally we call the method again with the inverse reflection to ensure we can really use it.) Since this is a breaking change—specifically in places where code may have relied on a missing `inverse_of` causing fresh copies of a record to be loaded—we've placed it behind the `automatic_scope_inversing` flag (whose name was inspired by `has_many_inversing`). It is set to true for new applications via framework defaults. Testing --- In addition to the inverse association tests, this commit also adds some cases to a few tests related to preloading. They are basically duplicates of existing tests, but with lower query counts. Testing this change with GitHub, the bulk of the failing tests were related to lower query counts. There were additionally 3 places (2 in tests and one in application code) where we relied on missing `inverse_of` causing fresh copies of a record to be loaded. There's still one Rails test that wouldn't pass if we ran the whole suite with `automatic_scope_inversing = true`. It's related to `TaggedPost`, which changes the polymorphic type from the base class `Post` to the subclass `TaggedPost`. ```rb class TaggedPost < Post has_many :taggings, -> { rewhere(taggable_type: "TaggedPost") }, as: :taggable end ``` Setting the inverse doesn't work because it ends up changing the type back to `Post`, something like this: ```rb post = TaggedPost.new tagging = post.taggings.new puts tagging.taggable_type => TaggedPost tagging.taggable = post puts tagging.taggable_type => Post ``` I think this is an acceptable change, given that it's a fairly specific scenario, and is sort of at odds with the way polymorphic associations are meant to work (they are meant to contain the base class, not the subclass). If someone is relying on this specific behavior they can still either keep `automatic_scope_inversing` set to false, or they can add `inverse_of: false` to the association. I haven't found any other cases where having the `inverse_of` would cause problems like this. Co-authored-by: Chris Bloom <chrisbloom7@gmail.com>
175 lines
5.8 KiB
Ruby
175 lines
5.8 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
require "active_support"
|
|
require "active_support/testing/autorun"
|
|
require "active_support/testing/method_call_assertions"
|
|
require "active_support/testing/stream"
|
|
require "active_record/fixtures"
|
|
|
|
require "cases/validations_repair_helper"
|
|
|
|
module ActiveRecord
|
|
# = Active Record Test Case
|
|
#
|
|
# Defines some test assertions to test against SQL queries.
|
|
class TestCase < ActiveSupport::TestCase # :nodoc:
|
|
include ActiveSupport::Testing::MethodCallAssertions
|
|
include ActiveSupport::Testing::Stream
|
|
include ActiveRecord::TestFixtures
|
|
include ActiveRecord::ValidationsRepairHelper
|
|
|
|
self.fixture_path = FIXTURES_ROOT
|
|
self.use_instantiated_fixtures = false
|
|
self.use_transactional_tests = true
|
|
|
|
def create_fixtures(*fixture_set_names, &block)
|
|
ActiveRecord::FixtureSet.create_fixtures(ActiveRecord::TestCase.fixture_path, fixture_set_names, fixture_class_names, &block)
|
|
end
|
|
|
|
def teardown
|
|
SQLCounter.clear_log
|
|
end
|
|
|
|
def capture_sql
|
|
ActiveRecord::Base.connection.materialize_transactions
|
|
SQLCounter.clear_log
|
|
yield
|
|
SQLCounter.log.dup
|
|
end
|
|
|
|
def assert_sql(*patterns_to_match, &block)
|
|
capture_sql(&block)
|
|
ensure
|
|
failed_patterns = []
|
|
patterns_to_match.each do |pattern|
|
|
failed_patterns << pattern unless SQLCounter.log_all.any? { |sql| pattern === sql }
|
|
end
|
|
assert failed_patterns.empty?, "Query pattern(s) #{failed_patterns.map(&:inspect).join(', ')} not found.#{SQLCounter.log.size == 0 ? '' : "\nQueries:\n#{SQLCounter.log.join("\n")}"}"
|
|
end
|
|
|
|
def assert_queries(num = 1, options = {})
|
|
ignore_none = options.fetch(:ignore_none) { num == :any }
|
|
ActiveRecord::Base.connection.materialize_transactions
|
|
SQLCounter.clear_log
|
|
x = yield
|
|
the_log = ignore_none ? SQLCounter.log_all : SQLCounter.log
|
|
if num == :any
|
|
assert_operator the_log.size, :>=, 1, "1 or more queries expected, but none were executed."
|
|
else
|
|
mesg = "#{the_log.size} instead of #{num} queries were executed.#{the_log.size == 0 ? '' : "\nQueries:\n#{the_log.join("\n")}"}"
|
|
assert_equal num, the_log.size, mesg
|
|
end
|
|
x
|
|
end
|
|
|
|
def assert_no_queries(options = {}, &block)
|
|
options.reverse_merge! ignore_none: true
|
|
assert_queries(0, options, &block)
|
|
end
|
|
|
|
def assert_column(model, column_name, msg = nil)
|
|
assert has_column?(model, column_name), msg
|
|
end
|
|
|
|
def assert_no_column(model, column_name, msg = nil)
|
|
assert_not has_column?(model, column_name), msg
|
|
end
|
|
|
|
def has_column?(model, column_name)
|
|
model.reset_column_information
|
|
model.column_names.include?(column_name.to_s)
|
|
end
|
|
|
|
def with_has_many_inversing(model = ActiveRecord::Base)
|
|
old = model.has_many_inversing
|
|
model.has_many_inversing = true
|
|
yield
|
|
ensure
|
|
model.has_many_inversing = old
|
|
if model != ActiveRecord::Base && !old
|
|
model.singleton_class.remove_method(:has_many_inversing) # reset the class_attribute
|
|
end
|
|
end
|
|
|
|
def with_automatic_scope_inversing(*reflections)
|
|
old = reflections.map { |reflection| reflection.klass.automatic_scope_inversing }
|
|
|
|
reflections.each do |reflection|
|
|
reflection.klass.automatic_scope_inversing = true
|
|
reflection.remove_instance_variable(:@inverse_name) if reflection.instance_variable_defined?(:@inverse_name)
|
|
reflection.remove_instance_variable(:@inverse_of) if reflection.instance_variable_defined?(:@inverse_of)
|
|
end
|
|
|
|
yield
|
|
ensure
|
|
reflections.each_with_index do |reflection, i|
|
|
reflection.klass.automatic_scope_inversing = old[i]
|
|
reflection.remove_instance_variable(:@inverse_name) if reflection.instance_variable_defined?(:@inverse_name)
|
|
reflection.remove_instance_variable(:@inverse_of) if reflection.instance_variable_defined?(:@inverse_of)
|
|
end
|
|
end
|
|
|
|
def reset_callbacks(klass, kind)
|
|
old_callbacks = {}
|
|
old_callbacks[klass] = klass.send("_#{kind}_callbacks").dup
|
|
klass.subclasses.each do |subclass|
|
|
old_callbacks[subclass] = subclass.send("_#{kind}_callbacks").dup
|
|
end
|
|
yield
|
|
ensure
|
|
klass.send("_#{kind}_callbacks=", old_callbacks[klass])
|
|
klass.subclasses.each do |subclass|
|
|
subclass.send("_#{kind}_callbacks=", old_callbacks[subclass])
|
|
end
|
|
end
|
|
|
|
def with_postgresql_datetime_type(type)
|
|
adapter = ActiveRecord::ConnectionAdapters::PostgreSQLAdapter
|
|
adapter.remove_instance_variable(:@native_database_types) if adapter.instance_variable_defined?(:@native_database_types)
|
|
datetime_type_was = adapter.datetime_type
|
|
adapter.datetime_type = type
|
|
yield
|
|
ensure
|
|
adapter = ActiveRecord::ConnectionAdapters::PostgreSQLAdapter
|
|
adapter.datetime_type = datetime_type_was
|
|
adapter.remove_instance_variable(:@native_database_types) if adapter.instance_variable_defined?(:@native_database_types)
|
|
end
|
|
end
|
|
|
|
class PostgreSQLTestCase < TestCase
|
|
def self.run(*args)
|
|
super if current_adapter?(:PostgreSQLAdapter)
|
|
end
|
|
end
|
|
|
|
class Mysql2TestCase < TestCase
|
|
def self.run(*args)
|
|
super if current_adapter?(:Mysql2Adapter)
|
|
end
|
|
end
|
|
|
|
class SQLite3TestCase < TestCase
|
|
def self.run(*args)
|
|
super if current_adapter?(:SQLite3Adapter)
|
|
end
|
|
end
|
|
|
|
class SQLCounter
|
|
class << self
|
|
attr_accessor :ignored_sql, :log, :log_all
|
|
def clear_log; self.log = []; self.log_all = []; end
|
|
end
|
|
|
|
clear_log
|
|
|
|
def call(name, start, finish, message_id, values)
|
|
return if values[:cached]
|
|
|
|
sql = values[:sql]
|
|
self.class.log_all << sql
|
|
self.class.log << sql unless ["SCHEMA", "TRANSACTION"].include? values[:name]
|
|
end
|
|
end
|
|
|
|
ActiveSupport::Notifications.subscribe("sql.active_record", SQLCounter.new)
|
|
end
|