From 8d41688ff7cf38598531354268eb18209b1a8aa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Fran=C3=A7a?= Date: Wed, 9 Dec 2020 12:11:31 -0500 Subject: [PATCH] Merge pull request #40679 from ghiculescu/negative-enum-warning Better handling of negative elements in enum --- activerecord/CHANGELOG.md | 6 ++ activerecord/lib/active_record/enum.rb | 19 ++++-- activerecord/test/cases/enum_test.rb | 81 ++++++++++++++++++++++++-- 3 files changed, 96 insertions(+), 10 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 24de275d035a3..3519f60fb214a 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Only warn about negative enums if a positive form that would cause conflicts exists. + + Fixes #39065. + + *Alex Ghiculescu* + * Change `attribute_for_inspect` to take `filter_attributes` in consideration. *Rafael Mendonça França* diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index 87da3a756cff3..cba38faebd537 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -189,6 +189,7 @@ def enum(definitions) _enum_methods_module.module_eval do pairs = values.respond_to?(:each_pair) ? values.each_pair : values.each_with_index + value_method_names = [] pairs.each do |label, value| if enum_prefix == true prefix = "#{name}_" @@ -203,6 +204,7 @@ def enum(definitions) method_friendly_label = label.to_s.gsub(/\W+/, "_") value_method_name = "#{prefix}#{method_friendly_label}#{suffix}" + value_method_names << value_method_name enum_values[label] = value label = label.to_s @@ -217,8 +219,6 @@ def enum(definitions) # scope :active, -> { where(status: 0) } # scope :not_active, -> { where.not(status: 0) } if enum_scopes != false - klass.send(:detect_negative_condition!, value_method_name) - klass.send(:detect_enum_conflict!, name, value_method_name, true) klass.scope value_method_name, -> { where(attr => value) } @@ -226,6 +226,7 @@ def enum(definitions) klass.scope "not_#{value_method_name}", -> { where.not(attr => value) } end end + klass.send(:detect_negative_enum_conditions!, value_method_names) if enum_scopes != false end enum_values.freeze end @@ -281,10 +282,16 @@ def raise_conflict_error(enum_name, method_name, type: "instance", source: "Acti } end - def detect_negative_condition!(method_name) - if method_name.start_with?("not_") && logger - logger.warn "An enum element in #{self.name} uses the prefix 'not_'." \ - " This will cause a conflict with auto generated negative scopes." + def detect_negative_enum_conditions!(method_names) + return unless logger + + 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 diff --git a/activerecord/test/cases/enum_test.rb b/activerecord/test/cases/enum_test.rb index b1686bfd348e5..9270dfb4153aa 100644 --- a/activerecord/test/cases/enum_test.rb +++ b/activerecord/test/cases/enum_test.rb @@ -646,14 +646,18 @@ def self.name; "Book"; end assert_not_predicate computer, :extendedGold? 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 logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new ActiveRecord::Base.logger = logger - expected_message = "An enum element in Book uses the prefix 'not_'."\ - " This will cause a conflict with auto generated negative scopes." + 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 @@ -664,7 +668,76 @@ def self.name 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 ActiveRecord::Base.logger = old_logger end