Adds an :unknown column type. #7954

Closed
wants to merge 1 commit into
from

5 participants

@elthariel

Trying to fix an "issue" where an unnoticed typo in the type parameter
of a migration could lead to data loss.

Using this example :

class PeopleWriteBuggyMigrations < ActiveRecord::Migration
  def self.up
    add_column "people", "type_typos", :bool
  end

  def self.down
    remove_column "people", "type_typos"
  end
end

This migration will run fine and without warning(s), but if you try to
remove this column later, active record would fail :

  • SQLite cannot directly drop a column, so a temp table is used.
  • AR then tries to create a temp table with the same structure.
  • the #simplified_type method of the Column class is called at some point
  • since the type doesn't exists it returns nil
  • migration fails in a very ugly fashion.
  • user has to fix it manually (dangerous and time comsuming) but will be tempted to fix the faulty migration, drop the db and migrate again.

This commit add tests for this issue and runs rake test_sqlite3 without error.

This is my first rails pull request, so i hope althought i read the guidelines i didn't make too big mistakes. I would really enjoy getting comments on this PR.

Thanks lovely community.

@elthariel elthariel Adds an :unknown column type.
Trying to fix an "issue" where an unnoticed typo in the type parameter
of a migration could lead to data loss.

Using this example :

    class PeopleWriteBuggyMigrations < ActiveRecord::Migration
      def self.up
        add_column "people", "type_typos", :bool
      end

      def self.down
        remove_column "people", "type_typos"
      end
    end

This migration will run fine and without warning(s), but if you try to
remove this column later, active record would fail :
- SQLite cannot directly drop a column, so a temp table is used.
- AR then tries to create a temp table with the same structure.
- the #simplified_type method of the Column class is called at some point
- since the type doesn't exists it returns nil
- migration fails in a very ugly fashion.
- user has to fix it manually (dangerous and time comsuming) but will
  be tempted to fix the faulty migration, drop the db and migrate
  again.

This commit add tests for this issue and runs rake test_sqlite3 without error.
6d76b70
@carlosantoniodasilva
Ruby on Rails member

I believe that instead of having a default unknown column type, Rails should simply raise saying: I don't know this ":bool" column type you're trying to use or something similar. Just my 2 cents :)

@elthariel

Thank you for your comment

I think this is a great idea, i think we should do both.
Because without the :unkown column type (or something similar) your are unable to remove the column in a later migration, which is pretty annoying (and dangerous if there are fools out there who use SQLite3 in production)

@mhuggins

With carlosantioniodasilva's suggestion, there is no need for an :unknown type column. Also, Rails column types attempts to mimic the most common column types across databases, and there is no such thing as an "unknown" column type in databases.

👎 on the unknown column type idea
👍 on the raise on invalid column type idea

@steveklabnik
Ruby on Rails member

Agree with both @carlosantoniodasilva and @mhuggins.

@tenderlove
Ruby on Rails member

I'm worried about raising an exception on unknown data types. SQLite3 has no datatypes, so it can actually support all data types (it just doesn't know how to cast some of them). We use this feature, developing on SQLite3, then pushing to Oracle in production.

I think the correct way to fix this is for SQLite3, we accept any data type, then fix the output of schema.rb.

Also, I think this is a dup of #2708

@elthariel

@tenderlove Not mentioning PostgreSQL extensions like PostGIS and maybe other which can define new data types. Also, what do you mean by "fix the output of schema.rb" ?

This is clearly a duplicate of #2708.

@tenderlove
Ruby on Rails member

@elthariel we need to come up with a way to serialize the unknown types to schema.rb so that unknown column types will result in a t.column "column_name", "column_type". I suspect (but please verify) that even after your patch, we would not be able to reconstruct the database given the schema.rb.

@elthariel

@tenderlove, Yeah ... most likely :(

Actually, a rather simple solution would be to just return the type who was provided, like field_type.to_s (which will avoid the nil error) and to emit a warning.

@tenderlove
Ruby on Rails member

I think even doing the field_type.to_s solution will break the schema.rb, since I don't think (but again, please verify) that a migration like this will break if the type isn't supported:

create_table("foos") do |t|
  t.bool 'omg'
end

Which is the format of schema.rb. The main problem with schema.rb is that SchemaDumper knows too much about columns. Also, the logic for determining what is and isn't a valid column type is in places outside of the connection.

IMO, we should be asking the connection if the type is valid, e.g.:

unless connection.valid_type?(type)
  raise "omg not valid"
end

Then we need to create a ColumnSpec object, and ask it to format itself. e.g.:

columns.each do |column|
  puts connection.column_spec(column)
end

SQLite3 connection could return an "unknown" spec type for datatypes that we don't handle in AR, where other adapters could blow up (or whatever).

The tricky part is that we have tests which ensure that the output is lined up. We need to figure out a way to format the output without accessing too much of the guts of the column specs.

Anyway, "fun" stuff. ;-)

@elthariel

@tenderlove :

Anyway, "fun" stuff. ;-)

Indeed. I was kinda hoping that if the tests were running, it would be ok :)

Thank you very much for the insights. I'll submit another fix asap (might be next week) and test on Postgre and MySQL too.

@tenderlove
Ruby on Rails member

OK! No rush! :-)

@steveklabnik
Ruby on Rails member

Great discussion guys, 👍. Can we close this PR then?

@elthariel

Yeah, close it, i'll reopen it when i push better implementation

@elthariel elthariel closed this Oct 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment