enum now raises on "dangerous" name conflicts

Dangerous name conflicts includes instance or class method conflicts
with methods defined within `ActiveRecord::Base` but not its ancestors,
as well as conflicts with methods generated by other enums on the same
class.

Fixes #13389.

activerecord/CHANGELOG.md

@@ -1,3 +1,14 @@
+*   `enum` now raises on "dangerous" name conflicts
+
+    Dangerous name conflicts includes instance or class method conflicts
+    with methods defined within `ActiveRecord::Base` but not its ancestors,
+    as well as conflicts with methods generated by other enums on the same
+    class.
+
+    Fixes #13389.
+
+    *Godfrey Chan*
+
 *   `scope` now raises on "dangerous" name conflicts
 
     Similar to dangerous attribute methods, a scope name conflict is

activerecord/lib/active_record/enum.rb

@@ -77,10 +77,12 @@ module ActiveRecord
         name        = name.to_sym
 
         # def self.statuses statuses end
+        detect_enum_conflict!(name, name.to_s.pluralize, true)
         klass.singleton_class.send(:define_method, name.to_s.pluralize) { enum_values }
 
         _enum_methods_module.module_eval do
           # def status=(value) self[:status] = statuses[value] end
+          klass.send(:detect_enum_conflict!, name, "#{name}=")
           define_method("#{name}=") { |value|
             if enum_values.has_key?(value) || value.blank?
               self[name] = enum_values[value]
@@ -95,23 +97,28 @@ module ActiveRecord
           }
 
           # def status() statuses.key self[:status] end
+          klass.send(:detect_enum_conflict!, name, name)
           define_method(name) { enum_values.key self[name] }
 
           # def status_before_type_cast() statuses.key self[:status] end
+          klass.send(:detect_enum_conflict!, name, "#{name}_before_type_cast")
           define_method("#{name}_before_type_cast") { enum_values.key self[name] }
 
           pairs = values.respond_to?(:each_pair) ? values.each_pair : values.each_with_index
           pairs.each do |value, i|
             enum_values[value] = i
 
-            # scope :active, -> { where status: 0 }
-            klass.scope value, -> { klass.where name => i }
-
             # def active?() status == 0 end
+            klass.send(:detect_enum_conflict!, name, "#{value}?")
             define_method("#{value}?") { self[name] == i }
 
             # def active!() update! status: :active end
+            klass.send(:detect_enum_conflict!, name, "#{value}!")
             define_method("#{value}!") { update! name => value }
+
+            # scope :active, -> { where status: 0 }
+            klass.send(:detect_enum_conflict!, name, value, true)
+            klass.scope value, -> { klass.where name => i }
           end
 
           DEFINED_ENUMS[name.to_s] = enum_values
@@ -148,5 +155,38 @@ module ActiveRecord
           mod
         end
       end
+
+      ENUM_CONFLICT_MESSAGE = \
+        "You tried to define an enum named \"%{enum}\" on the model \"%{klass}\", but " \
+        "this will generate a %{type} method \"%{method}\", which is already defined " \
+        "by %{source}."
+
+      def detect_enum_conflict!(enum_name, method_name, klass_method = false)
+        if klass_method && dangerous_class_method?(method_name)
+          raise ArgumentError, ENUM_CONFLICT_MESSAGE % {
+            enum: enum_name,
+            klass: self.name,
+            type: 'class',
+            method: method_name,
+            source: 'Active Record'
+          }
+        elsif !klass_method && dangerous_attribute_method?(method_name)
+          raise ArgumentError, ENUM_CONFLICT_MESSAGE % {
+            enum: enum_name,
+            klass: self.name,
+            type: 'instance',
+            method: method_name,
+            source: 'Active Record'
+          }
+        elsif !klass_method && method_defined_within?(method_name, _enum_methods_module, Module)
+          raise ArgumentError, ENUM_CONFLICT_MESSAGE % {
+            enum: enum_name,
+            klass: self.name,
+            type: 'instance',
+            method: method_name,
+            source: 'another enum'
+          }
+        end
+      end
   end
 end

activerecord/test/cases/enum_test.rb

@@ -163,4 +163,63 @@ class EnumTest < ActiveRecord::TestCase
   test "_before_type_cast returns the enum label (required for form fields)" do
     assert_equal "proposed", @book.status_before_type_cast
   end
+
+  test "reserved enum names" do
+    klass = Class.new(ActiveRecord::Base) do
+      self.table_name = "books"
+      enum status: [:proposed, :written, :published]
+    end
+
+    conflicts = [
+      :column,     # generates class method .columns, which conflicts with an AR method
+      :logger,     # generates #logger, which conflicts with an AR method
+      :attributes, # generates #attributes=, which conflicts with an AR method
+    ]
+
+    conflicts.each_with_index do |name, i|
+      assert_raises(ArgumentError, "enum name `#{name}` should not be allowed") do
+        klass.class_eval { enum name => ["value_#{i}"] }
+      end
+    end
+  end
+
+  test "reserved enum values" do
+    klass = Class.new(ActiveRecord::Base) do
+      self.table_name = "books"
+      enum status: [:proposed, :written, :published]
+    end
+
+    conflicts = [
+      :new,      # generates a scope that conflicts with an AR class method
+      :valid,    # generates #valid?, which conflicts with an AR method
+      :save,     # generates #save!, which conflicts with an AR method
+      :proposed, # same value as an existing enum
+    ]
+
+    conflicts.each_with_index do |value, i|
+      assert_raises(ArgumentError, "enum value `#{value}` should not be allowed") do
+        klass.class_eval { enum "status_#{i}" => [value] }
+      end
+    end
+  end
+
+  test "overriding enum method should not raise" do
+    assert_nothing_raised do
+      klass = Class.new(ActiveRecord::Base) do
+        self.table_name = "books"
+
+        def published!
+          super
+          "do publish work..."
+        end
+
+        enum status: [:proposed, :written, :published]
+
+        def written!
+          super
+          "do written work..."
+        end
+      end
+    end
+  end
 end