Permalink
Browse files

Merge pull request #9204 from ranjaykrishna/col-prob

schema dumper tests now conducted by ActiveRecord::Base.Connection
  • Loading branch information...
tenderlove committed Feb 12, 2013
2 parents 1fc6876 + c321b30 commit f8c8ad56c8a1cb48c6535fd8e248dcb9049e0aa3
@@ -326,6 +326,10 @@ def translate_exception(exception, message)
# override in derived class
ActiveRecord::StatementInvalid.new(message)
end
+
+ def valid_types?(type)
+ true
+ end
end
end
end
@@ -748,6 +748,9 @@ def configure_connection
execute("SET #{encoding} #{variable_assignments}", :skip_logging)
end
+ def valid_type?(type)
+ !native_database_types[type].nil?
+ end
end
end
end
@@ -887,6 +887,10 @@ def extract_table_ref_from_insert_sql(sql)
def table_definition
TableDefinition.new(self)
end
+
+ def valid_type?(type)
+ !native_database_types[type].nil?
+ end
end
end
end
@@ -603,6 +603,10 @@ def translate_exception(exception, message)
end
end
+ def valid_type?(type)
+ true
+ end

This comment has been minimized.

Show comment Hide comment
@rubys

rubys Feb 13, 2013

Contributor

db/schema.rb before this change:

ActiveRecord::Schema.define(version: 20130213000001) do

  create_table "products", force: true do |t|
    t.string   "title"
    t.text     "description"
    t.datetime "created_at"
    t.datetime "updated_at"
  end

end

After this change:

ActiveRecord::Schema.define(version: 20130213000001) do

# Could not dump table "products" because of following NoMethodError
#   protected method `valid_type?' called for #<ActiveRecord::ConnectionAdapters::SQLite3Adapter:0x000000042cd6d0>

end

Reproduction scenario:

rails generate scaffold Product title:string description:text
rake db:migrate
@rubys

rubys Feb 13, 2013

Contributor

db/schema.rb before this change:

ActiveRecord::Schema.define(version: 20130213000001) do

  create_table "products", force: true do |t|
    t.string   "title"
    t.text     "description"
    t.datetime "created_at"
    t.datetime "updated_at"
  end

end

After this change:

ActiveRecord::Schema.define(version: 20130213000001) do

# Could not dump table "products" because of following NoMethodError
#   protected method `valid_type?' called for #<ActiveRecord::ConnectionAdapters::SQLite3Adapter:0x000000042cd6d0>

end

Reproduction scenario:

rails generate scaffold Product title:string description:text
rake db:migrate
+
end
end
end
@@ -118,7 +118,7 @@ def table(table, stream)
# then dump all non-primary key columns
column_specs = columns.map do |column|
- raise StandardError, "Unknown type '#{column.sql_type}' for column '#{column.name}'" if @types[column.type].nil?
+ raise StandardError, "Unknown type '#{column.sql_type}' for column '#{column.name}'" unless @connection.valid_type?(column.type)

This comment has been minimized.

Show comment Hide comment
@kenips

kenips Feb 13, 2013

@rubys meant this line

@kenips

kenips Feb 13, 2013

@rubys meant this line

This comment has been minimized.

Show comment Hide comment
@rubys

rubys Feb 13, 2013

Contributor

If you add public before the valid_type? method I indicated, the db/schema.rb will be again produced as expected.

A better solution would be to move the method declaration to earlier in the class definition.

Even better may be deleting the method itself from the sqlite3_adapter and making the definition in the abstract adaptor public, making it suitable as the default behavior for all other adapters. If it is felt that it is better to keep the abstract adapter abstract, I would recommend having that method raise a more precise error.

In any case, the placement of the definitions for the mysql and postgre methods also needs to be examined.

@rubys

rubys Feb 13, 2013

Contributor

If you add public before the valid_type? method I indicated, the db/schema.rb will be again produced as expected.

A better solution would be to move the method declaration to earlier in the class definition.

Even better may be deleting the method itself from the sqlite3_adapter and making the definition in the abstract adaptor public, making it suitable as the default behavior for all other adapters. If it is felt that it is better to keep the abstract adapter abstract, I would recommend having that method raise a more precise error.

In any case, the placement of the definitions for the mysql and postgre methods also needs to be examined.

next if column.name == pk
@connection.column_spec(column, @types)
end.compact
@@ -16,6 +16,15 @@ def setup
eosql
end
+ def test_valid_column
+ column = @conn.column('ex').find { |col| col.name == 'id' }
+ assert @conn.valid_type?(column.type)
+ end
+
+ def test_invalid_column
+ assert !@conn.valid_type?(:foobar)
+ end
+
def test_client_encoding
assert_equal Encoding::UTF_8, @conn.client_encoding
end
@@ -10,6 +10,15 @@ def setup
@connection.exec_query('create table ex(id serial primary key, number integer, data character varying(255))')
end
+ def test_valid_column
+ column = @connection.column('ex').find { |col| col.name == 'id' }
+ assert @connection.valid_type?(column.type)
+ end
+
+ def test_invalid_column
+ assert !@connection.valid_type?(:foobar)
+ end
+
def test_primary_key
assert_equal 'id', @connection.primary_key('ex')
end
@@ -25,6 +25,15 @@ def setup
@conn.intercepted = true
end
+ def test_valid_column
+ column = @conn.column('items').find { |col| col.name == 'id' }
+ assert @conn.valid_type?(column.type)
+ end
+
+ def test_invalid_column
+ assert @conn.valid_type?(:foobar)
+ end
+
def teardown
@conn.intercepted = false
@conn.logged = []

0 comments on commit f8c8ad5

Please sign in to comment.