Typos in column type silently corrupt table using SQLITE3 #2708

Closed
stephenprater opened this Issue Aug 26, 2011 · 23 comments
@stephenprater

Running a migration like this:

class ScrewUpTable < ActiveRecord::Migration
   def self.up
       add_column :table, :flag, :bool
   end

   def self.down
      remove_column :table, :flag
   end
end

rake db:migrate will execute this migration without complain, but in your schema.rb will have the following error message in it.

# Could not dump table "table" because of following StandardError
#   Unknown type 'bool' for column 'flag'

Will prevent you from running any future migrations on that table. You will not be able to rollback this migration, or further address the table in any way short of dropping it.

This is Rails 3.0.7 with SQLITE3

@dmathieu

Does it also occurs in 3.0.10 ?

@vijaydev
Ruby on Rails member

Can confirm on edge. Haven't checked on 3.0.10.

@agrussellknives

Yes, it also occurs on 3.0.10

@vijaydev
Ruby on Rails member
@tenderlove tenderlove was assigned Oct 24, 2011
@kennyj

It also occuers on 3.2.rc1.

It seems that type_to_sql method (in active_record/connection_adapters/abstract/schema_statements.rb) returns a type when it cannot be looked up from native_database_types hash.
But I think that we should allow type_to_sql method to specify an unknown type...

Therefore, should we have a warning message ?
For example kennyj@aeee4d5

@tenderlove
Ruby on Rails member

@kennyj I think the correct way to fix this issue is to emit all types to the schema.rb for sqlite. SQLite doesn't care what the column types are, so essentially it handles all types.

@stephenprater

You can fix it by manually changing the sqlite_master table to a valid type ( which is what I did. ) but woe betide you if you mess THAT up.

@isaacsanders

Is this still an issue?

@cykhoo

I can confirm that this is still an issue with 3.2.8

@schneems
Ruby on Rails member

5 month ping, would be great to fix this issue, raise an error, etc. As far as I know this is still an issue on master.

@JonRowe

3 months later... 8 month ping!

Did this get fixed? Or is it still an issue?

@tomprats

Didn't @tenderlove fix this issue in the pull request above? It was merged into master

@JonRowe

The PR mentioned above #7954 was closed without merging into master... So I'd wager this isn't fixed...

@henrikhodne

Reproduced this on master, although the error message has changed slightly:

› ~/dev/rails/railties/bin/rails new rails2708 --dev
[…]
› cd rails2708
› rails --version
Rails 4.1.0.beta
› rails generate migration create_users username:string
      invoke  active_record
      create    db/migrate/20130803201104_create_users.rb
› bundle exec rake db:migrate
==  CreateUsers: migrating ====================================================
-- create_table(:users)
   -> 0.0013s
==  CreateUsers: migrated (0.0013s) ===========================================

› rails generate migration add_flag_to_users flag:bool
      invoke  active_record
      create    db/migrate/20130803201335_add_flag_to_users.rb
› bundle exec rake db:migrate
==  AddFlagToUsers: migrating =================================================
-- add_column(:users, :flag, :bool)
   -> 0.0050s
==  AddFlagToUsers: migrated (0.0050s) ========================================

› cat db/schema.rb | tail -n6
ActiveRecord::Schema.define(version: 20130803201335) do

# Could not dump table "users" because of following NoMethodError
#   undefined method `[]' for nil:NilClass

end
@wilfoa

We've looked at this as part of the http://bugmash.herokuapp.com/ Bug Mash day in TLV and we saw that this is a problem in performing the migration by the SQLite3 adapter which happens separately from the schema dumper. The issue is that the invalid column type is checked in the schema dumper but not when performing the migration itself.

We think that there is a simple fix to be put in https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L451-L459:

if not native_database_types.include?( type )
  raise StandardError, "Unknown type '#{type}' for column '#{column_name}'" 
end

There is also a test to perform in activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb:

def test_add_column_with_invalid_type
        assert_raises (StandardError) {
          @conn.add_column("items", "invalid_col", "invalid_type")
        }
end

But unfortunately we didn't have the time for a pull request.

@vipulnsward
Ruby on Rails member

Thanks @wilfoa I l try working on a fix based on above.

@vipulnsward
Ruby on Rails member

@wilfoa this fix won't work since as per discussion on #7954 and above , raising an exception can't be a solution for sqlite3, which doesn't have fixed datatypes.

@DaniTheLion

@vipulnsward If I understand you correctly - since SQLite uses dynamic typing we don't want the migration to fail. So it seems to me that the only thing we can do is try and fix the schema dumper so it handles unknown data types in case a SQLite3 db is used and outputs a valid schema.rb file even if it contains unknown data types.
Am I missing something?

@stephenprater stephenprater added the stale label Apr 23, 2014
@rafaelfranca
Ruby on Rails member

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@JuanitoFatas

Confirmed this error does not occur on master. Gist.

@laurocaetano laurocaetano removed the stale label May 11, 2014
@laurocaetano

@JuanitoFatas actually it does occur on master. The problem happens when running the rake task, so it will not be reproduced with the gist you have provided.

About the issue per se, I don't think there is something to be fixed. See #7954 (comment), but lets wait for feedback.

Btw, I have removed the stale tag.

@JuanitoFatas

@laurocaetano OK. Sorry about that. Thank you for explanation.

@rafaelfranca
Ruby on Rails member

Yes, I had confirmed this is wontfix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment