Merge pull request #40679 from ghiculescu/negative-enum-warning

Better handling of negative elements in enum

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*
+
 *   Allow the inverse of a `has_one` association that was previously autosaved to be loaded.
 
     Fixes #34255.

activerecord/lib/active_record/enum.rb

@@ -173,6 +173,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}_"
@@ -186,6 +187,7 @@ def enum(definitions)
             end
 
             value_method_name = "#{prefix}#{label}#{suffix}"
+            value_method_names << value_method_name
             enum_values[label] = value
             label = label.to_s
 
@@ -200,15 +202,14 @@ 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) }
 
               klass.send(:detect_enum_conflict!, name, "not_#{value_method_name}", true)
               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
@@ -264,10 +265,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

activerecord/test/cases/enum_test.rb

@@ -567,14 +567,18 @@ def self.name; "Book"; end
     assert_raises(NoMethodError) { klass.proposed }
   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
@@ -585,7 +589,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