Merge pull request rails#42694 from p8/railties/generators-raise-on-invalid-index

Generators should raise an error if an attribute has an invalid index

Author: kamipo

railties/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Raise an error in generators if an index type is invalid.
+
+    *Petrik de Heus*
+
 *   `package.json` now uses a strict version constraint for Rails JavaScript packages on new Rails apps.
 
     *Zachary Scott*, *Alex Ghiculescu*

railties/lib/rails/generators/generated_attribute.rb

@@ -33,12 +33,12 @@ class GeneratedAttribute # :nodoc:
 
       class << self
         def parse(column_definition)
-          name, type, has_index = column_definition.split(":")
+          name, type, index_type = column_definition.split(":")
 
           # if user provided "name:index" instead of "name:string:index"
           # type should be set blank so GeneratedAttribute's constructor
           # could set it to :string
-          has_index, type = type, nil if INDEX_OPTIONS.include?(type)
+          index_type, type = type, nil if valid_index_type?(type)
 
           type, attr_options = *parse_type_and_options(type)
           type = type.to_sym if type
@@ -47,20 +47,28 @@ def parse(column_definition)
             raise Error, "Could not generate field '#{name}' with unknown type '#{type}'."
           end
 
+          if index_type && !valid_index_type?(index_type)
+            raise Error, "Could not generate field '#{name}' with unknown index '#{index_type}'."
+          end
+
           if type && reference?(type)
-            if UNIQ_INDEX_OPTIONS.include?(has_index)
+            if UNIQ_INDEX_OPTIONS.include?(index_type)
               attr_options[:index] = { unique: true }
             end
           end
 
-          new(name, type, has_index, attr_options)
+          new(name, type, index_type, attr_options)
         end
 
         def valid_type?(type)
           DEFAULT_TYPES.include?(type.to_s) ||
             ActiveRecord::Base.connection.valid_type?(type)
         end
 
+        def valid_index_type?(index_type)
+          INDEX_OPTIONS.include?(index_type.to_s)
+        end
+
         def reference?(type)
           [:references, :belongs_to].include? type
         end

railties/test/generators/generated_attribute_test.rb

@@ -68,6 +68,15 @@ def test_field_type_with_unknown_type_raises_error
     assert_match message, e.message
   end
 
+  def test_field_type_with_unknown_index_type_raises_error
+    index_type = :unknown
+    e = assert_raise Rails::Generators::Error do
+      create_generated_attribute "string", "name", index_type
+    end
+    message = "Could not generate field 'name' with unknown index 'unknown'"
+    assert_match message, e.message
+  end
+
   def test_default_value_is_integer
     assert_field_default_value :integer, 1
   end

railties/test/generators/migration_generator_test.rb

@@ -159,21 +159,6 @@ def test_add_migration_with_attributes_and_indices
     end
   end
 
-  def test_add_migration_with_attributes_and_wrong_index_declaration
-    migration = "add_title_and_content_to_books"
-    run_generator [migration, "title:string:inex", "content:text", "user_id:integer:unik"]
-
-    assert_migration "db/migrate/#{migration}.rb" do |content|
-      assert_method :change, content do |change|
-        assert_match(/add_column :books, :title, :string/, change)
-        assert_match(/add_column :books, :content, :text/, change)
-        assert_match(/add_column :books, :user_id, :integer/, change)
-      end
-      assert_no_match(/add_index :books, :title/, content)
-      assert_no_match(/add_index :books, :user_id/, content)
-    end
-  end
-
   def test_add_migration_with_attributes_without_type_and_index
     migration = "add_title_with_index_and_body_to_posts"
     run_generator [migration, "title:index", "body:text", "user_uuid:uniq"]

railties/test/generators/model_generator_test.rb

@@ -223,23 +223,6 @@ def test_migration_with_attributes_and_with_index
     end
   end
 
-  def test_migration_with_attributes_and_with_wrong_index_declaration
-    run_generator ["product", "name:string", "supplier_id:integer:inex", "user_id:integer:unqu"]
-
-    assert_migration "db/migrate/create_products.rb" do |m|
-      assert_method :change, m do |up|
-        assert_match(/create_table :products/, up)
-        assert_match(/t\.string :name/, up)
-        assert_match(/t\.integer :supplier_id/, up)
-        assert_match(/t\.integer :user_id/, up)
-
-        assert_no_match(/add_index :products, :name/, up)
-        assert_no_match(/add_index :products, :supplier_id/, up)
-        assert_no_match(/add_index :products, :user_id/, up)
-      end
-    end
-  end
-
   def test_migration_with_missing_attribute_type_and_with_index
     run_generator ["product", "name:index", "supplier_id:integer:index", "year:integer"]