Skip to content

Commit

Permalink
Generators should raise an error if a field has an invalid type
Browse files Browse the repository at this point in the history
Generators can create invalid migrations when passing an invalid
field type. For example, when mixing up the name and type:

    bin/rails g model post string:title

This will generate a field for post with a column named `string`
of the type `title`, instead of a column named `title` of the type
`string`. Running the migration will result in an error as the type
`title` is not known to the database.

Instead of generating invalid files, the generator should raise an error
if the type is invalid. We validate the type by checking if it's a
default migration types like: string, integer, datetime, but also
references, and rich_text.

If the type isn't a default type, we can ask the
database connection if the type is valid. This uses the `valid_type?`
method defined on each database adapter, which returns true if the
adapter supports the column type. This method is also used by the
SchemaDumper.

Some gems like 'postgis' add custom types. The 'postgis' gem adds these
types by overriding the `native_database_types` method.
That method is used by `valid_type?` method on the database adapter,
making this change compatible with 'postgis'.
  • Loading branch information
p8 committed May 29, 2021
1 parent d5a89c2 commit d163fcd
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 7 deletions.
4 changes: 4 additions & 0 deletions railties/CHANGELOG.md
@@ -1,3 +1,7 @@
* Raise an error in generators if a field type is invalid.

*Petrik de Heus*

* `bin/rails tmp:clear` deletes also files and directories in `tmp/storage`.

*George Claghorn*
Expand Down
28 changes: 28 additions & 0 deletions railties/lib/rails/generators/generated_attribute.rb
Expand Up @@ -7,6 +7,25 @@ module Generators
class GeneratedAttribute # :nodoc:
INDEX_OPTIONS = %w(index uniq)
UNIQ_INDEX_OPTIONS = %w(uniq)
DEFAULT_TYPES = %w(
attachment
attachments
belongs_to
boolean
date
datetime
decimal
digest
float
integer
references
rich_text
string
text
time
timestamp
token
)

attr_accessor :name, :type
attr_reader :attr_options
Expand All @@ -24,6 +43,10 @@ def parse(column_definition)
type, attr_options = *parse_type_and_options(type)
type = type.to_sym if type

if type && !valid_type?(type)
raise Error, "Could not generate field '#{name}' with unknown type '#{type}'."
end

if type && reference?(type)
if UNIQ_INDEX_OPTIONS.include?(has_index)
attr_options[:index] = { unique: true }
Expand All @@ -33,6 +56,11 @@ def parse(column_definition)
new(name, type, has_index, attr_options)
end

def valid_type?(type)
DEFAULT_TYPES.include?(type.to_s) ||
ActiveRecord::Base.connection.valid_type?(type)
end

def reference?(type)
[:references, :belongs_to].include? type
end
Expand Down
25 changes: 18 additions & 7 deletions railties/test/generators/generated_attribute_test.rb
Expand Up @@ -2,13 +2,15 @@

require "generators/generators_test_helper"
require "rails/generators/generated_attribute"
require "rails/generators/base"

class GeneratedAttributeTest < Rails::Generators::TestCase
include GeneratorsTestHelper

def setup
@old_belongs_to_required_by_default = Rails.application.config.active_record.belongs_to_required_by_default
Rails.application.config.active_record.belongs_to_required_by_default = true
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
end

def teardown
Expand Down Expand Up @@ -57,10 +59,13 @@ def test_field_type_returns_file_field
end
end

def test_field_type_with_unknown_type_returns_text_field
%w(foo bar baz).each do |attribute_type|
assert_field_type attribute_type, :text_field
def test_field_type_with_unknown_type_raises_error
field_type = :unknown
e = assert_raise Rails::Generators::Error do
create_generated_attribute field_type
end
message = "Could not generate field 'test' with unknown type 'unknown'"
assert_match message, e.message
end

def test_default_value_is_integer
Expand Down Expand Up @@ -109,7 +114,7 @@ def test_default_value_is_nil
end

def test_default_value_is_empty_string
%w(foo bar baz).each do |attribute_type|
%w(digest token).each do |attribute_type|
assert_field_default_value attribute_type, ""
end
end
Expand All @@ -128,7 +133,7 @@ def test_reference_is_true
end

def test_reference_is_false
%w(foo bar baz).each do |attribute_type|
%w(string text float).each do |attribute_type|
assert_not_predicate create_generated_attribute(attribute_type), :reference?
end
end
Expand All @@ -140,8 +145,8 @@ def test_polymorphic_reference_is_true
end

def test_polymorphic_reference_is_false
%w(foo bar baz).each do |attribute_type|
assert_not_predicate create_generated_attribute("#{attribute_type}{polymorphic}"), :polymorphic?
%w(references belongs_to).each do |attribute_type|
assert_not_predicate create_generated_attribute(attribute_type), :polymorphic?
end
end

Expand All @@ -163,6 +168,12 @@ def test_handles_column_names_for_references
assert_equal "post_id", create_generated_attribute("belongs_to", "post").column_name
end

def test_parse_works_with_adapter_specific_types
att = Rails::Generators::GeneratedAttribute.parse("document:json")
assert_equal "document", att.name
assert_equal :json, att.type
end

def test_parse_required_attribute_with_index
att = Rails::Generators::GeneratedAttribute.parse("supplier:references:index")
assert_equal "supplier", att.name
Expand Down

0 comments on commit d163fcd

Please sign in to comment.