mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Better handling of negative words in enum
Fixes https://github.com/rails/rails/issues/39065. Adds a more descriptive warning, and logs it less often.
This commit is contained in:
parent
ef61c9c8a3
commit
6f9c639825
3 changed files with 96 additions and 10 deletions
|
@ -1,3 +1,9 @@
|
||||||
|
* Only warn about negative enums if a positive form that would cause conflicts exists.
|
||||||
|
|
||||||
|
Fixes #39065.
|
||||||
|
|
||||||
|
*Alex Ghiculescu*
|
||||||
|
|
||||||
* Add option to run `default_scope` on all queries.
|
* Add option to run `default_scope` on all queries.
|
||||||
|
|
||||||
Previously, a `default_scope` would only run on select or insert queries. In some cases, like non-Rails tenant sharding solutions, it may be desirable to run `default_scope` on all queries in order to ensure queries are including a foreign key for the shard (ie `blog_id`).
|
Previously, a `default_scope` would only run on select or insert queries. In some cases, like non-Rails tenant sharding solutions, it may be desirable to run `default_scope` on all queries in order to ensure queries are including a foreign key for the shard (ie `blog_id`).
|
||||||
|
|
|
@ -189,6 +189,7 @@ module ActiveRecord
|
||||||
|
|
||||||
_enum_methods_module.module_eval do
|
_enum_methods_module.module_eval do
|
||||||
pairs = values.respond_to?(:each_pair) ? values.each_pair : values.each_with_index
|
pairs = values.respond_to?(:each_pair) ? values.each_pair : values.each_with_index
|
||||||
|
value_method_names = []
|
||||||
pairs.each do |label, value|
|
pairs.each do |label, value|
|
||||||
if enum_prefix == true
|
if enum_prefix == true
|
||||||
prefix = "#{name}_"
|
prefix = "#{name}_"
|
||||||
|
@ -203,6 +204,7 @@ module ActiveRecord
|
||||||
|
|
||||||
method_friendly_label = label.to_s.gsub(/\W+/, "_")
|
method_friendly_label = label.to_s.gsub(/\W+/, "_")
|
||||||
value_method_name = "#{prefix}#{method_friendly_label}#{suffix}"
|
value_method_name = "#{prefix}#{method_friendly_label}#{suffix}"
|
||||||
|
value_method_names << value_method_name
|
||||||
enum_values[label] = value
|
enum_values[label] = value
|
||||||
label = label.to_s
|
label = label.to_s
|
||||||
|
|
||||||
|
@ -217,8 +219,6 @@ module ActiveRecord
|
||||||
# scope :active, -> { where(status: 0) }
|
# scope :active, -> { where(status: 0) }
|
||||||
# scope :not_active, -> { where.not(status: 0) }
|
# scope :not_active, -> { where.not(status: 0) }
|
||||||
if enum_scopes != false
|
if enum_scopes != false
|
||||||
klass.send(:detect_negative_condition!, value_method_name)
|
|
||||||
|
|
||||||
klass.send(:detect_enum_conflict!, name, value_method_name, true)
|
klass.send(:detect_enum_conflict!, name, value_method_name, true)
|
||||||
klass.scope value_method_name, -> { where(attr => value) }
|
klass.scope value_method_name, -> { where(attr => value) }
|
||||||
|
|
||||||
|
@ -226,6 +226,7 @@ module ActiveRecord
|
||||||
klass.scope "not_#{value_method_name}", -> { where.not(attr => value) }
|
klass.scope "not_#{value_method_name}", -> { where.not(attr => value) }
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
klass.send(:detect_negative_enum_conditions!, value_method_names) if enum_scopes != false
|
||||||
end
|
end
|
||||||
enum_values.freeze
|
enum_values.freeze
|
||||||
end
|
end
|
||||||
|
@ -281,10 +282,16 @@ module ActiveRecord
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
def detect_negative_condition!(method_name)
|
def detect_negative_enum_conditions!(method_names)
|
||||||
if method_name.start_with?("not_") && logger
|
return unless logger
|
||||||
logger.warn "An enum element in #{self.name} uses the prefix 'not_'." \
|
|
||||||
" This will cause a conflict with auto generated negative scopes."
|
method_names.select { |m| m.start_with?("not_") }.each do |potential_not|
|
||||||
|
inverted_form = potential_not.sub("not_", "")
|
||||||
|
if method_names.include?(inverted_form)
|
||||||
|
logger.warn "Enum element '#{potential_not}' in #{self.name} uses the prefix 'not_'." \
|
||||||
|
" This has caused a conflict with auto generated negative scopes." \
|
||||||
|
" Avoid using enum elements starting with 'not' where the positive form is also an element."
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -646,14 +646,18 @@ class EnumTest < ActiveRecord::TestCase
|
||||||
assert_not_predicate computer, :extendedGold?
|
assert_not_predicate computer, :extendedGold?
|
||||||
end
|
end
|
||||||
|
|
||||||
test "enums with a negative condition log a warning" do
|
test "enum logs a warning if auto-generated negative scopes would clash with other enum names" do
|
||||||
old_logger = ActiveRecord::Base.logger
|
old_logger = ActiveRecord::Base.logger
|
||||||
logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
|
logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
|
||||||
|
|
||||||
ActiveRecord::Base.logger = logger
|
ActiveRecord::Base.logger = logger
|
||||||
|
|
||||||
expected_message = "An enum element in Book uses the prefix 'not_'."\
|
expected_message_1 = "Enum element 'not_sent' in Book uses the prefix 'not_'."\
|
||||||
" This will cause a conflict with auto generated negative scopes."
|
" This has caused a conflict with auto generated negative scopes."\
|
||||||
|
" Avoid using enum elements starting with 'not' where the positive form is also an element."
|
||||||
|
|
||||||
|
# this message comes from ActiveRecord::Scoping::Named, but it's worth noting that both occur in this case
|
||||||
|
expected_message_2 = "Creating scope :not_sent. Overwriting existing method Book.not_sent."
|
||||||
|
|
||||||
Class.new(ActiveRecord::Base) do
|
Class.new(ActiveRecord::Base) do
|
||||||
def self.name
|
def self.name
|
||||||
|
@ -664,7 +668,76 @@ class EnumTest < ActiveRecord::TestCase
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
assert_match(expected_message, logger.logged(:warn).first)
|
assert_includes(logger.logged(:warn), expected_message_1)
|
||||||
|
assert_includes(logger.logged(:warn), expected_message_2)
|
||||||
|
ensure
|
||||||
|
ActiveRecord::Base.logger = old_logger
|
||||||
|
end
|
||||||
|
|
||||||
|
test "enum logs a warning if auto-generated negative scopes would clash with other enum names regardless of order" do
|
||||||
|
old_logger = ActiveRecord::Base.logger
|
||||||
|
logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
|
||||||
|
|
||||||
|
ActiveRecord::Base.logger = logger
|
||||||
|
|
||||||
|
expected_message_1 = "Enum element 'not_sent' in Book uses the prefix 'not_'."\
|
||||||
|
" This has caused a conflict with auto generated negative scopes."\
|
||||||
|
" Avoid using enum elements starting with 'not' where the positive form is also an element."
|
||||||
|
|
||||||
|
# this message comes from ActiveRecord::Scoping::Named, but it's worth noting that both occur in this case
|
||||||
|
expected_message_2 = "Creating scope :not_sent. Overwriting existing method Book.not_sent."
|
||||||
|
|
||||||
|
Class.new(ActiveRecord::Base) do
|
||||||
|
def self.name
|
||||||
|
"Book"
|
||||||
|
end
|
||||||
|
silence_warnings do
|
||||||
|
enum status: [:not_sent, :sent]
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
assert_includes(logger.logged(:warn), expected_message_1)
|
||||||
|
assert_includes(logger.logged(:warn), expected_message_2)
|
||||||
|
ensure
|
||||||
|
ActiveRecord::Base.logger = old_logger
|
||||||
|
end
|
||||||
|
|
||||||
|
test "enum doesn't log a warning if no clashes detected" do
|
||||||
|
old_logger = ActiveRecord::Base.logger
|
||||||
|
logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
|
||||||
|
|
||||||
|
ActiveRecord::Base.logger = logger
|
||||||
|
|
||||||
|
Class.new(ActiveRecord::Base) do
|
||||||
|
def self.name
|
||||||
|
"Book"
|
||||||
|
end
|
||||||
|
silence_warnings do
|
||||||
|
enum status: [:not_sent]
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
assert_empty(logger.logged(:warn))
|
||||||
|
ensure
|
||||||
|
ActiveRecord::Base.logger = old_logger
|
||||||
|
end
|
||||||
|
|
||||||
|
test "enum doesn't log a warning if opting out of scopes" do
|
||||||
|
old_logger = ActiveRecord::Base.logger
|
||||||
|
logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
|
||||||
|
|
||||||
|
ActiveRecord::Base.logger = logger
|
||||||
|
|
||||||
|
Class.new(ActiveRecord::Base) do
|
||||||
|
def self.name
|
||||||
|
"Book"
|
||||||
|
end
|
||||||
|
silence_warnings do
|
||||||
|
enum status: [:not_sent, :sent], _scopes: false
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
assert_empty(logger.logged(:warn))
|
||||||
ensure
|
ensure
|
||||||
ActiveRecord::Base.logger = old_logger
|
ActiveRecord::Base.logger = old_logger
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in a new issue