Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #3735 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

[PATCH] Migrations fail adding a primary key column

Reported by: rrwhite@gmail.com Assigned to: michael@koziarski.com
Priority: normal Milestone: 1.2
Component: ActiveRecord Version: 1.1.0 RC1
Severity: normal Keywords: tiny
Cc: jramirez@aspgems.com

Description

I create a migration script that included the following line: 

add_column :appt_service_logins, :id, :primary_key

This threw an error when running 'rake migrate' against my PostgreSQL DB that essentially boiled down to migrations trying to run the following SQL 

ALTER TABLE appt_service_logins ADD id

Obviously missing the datatype for the column. I tracked this down to type_to_sql returning nil. This was because native_database_types define a Hash where primary key does not map to another array with a :name key. Therefore the following in def type_to_sql would return nil since it specifically looks for the value using the :name key. 

def type_to_sql(type, limit = nil) #:nodoc:
  native = native_database_types[type]
  limit ||= native[:limit]
  column_type_sql = native[:name]
  column_type_sql << "(#{limit})" if limit
  column_type_sql
end

To get around this I modified def type_to_sql to use the native value if column_type_sql was nil. 

def type_to_sql(type, limit = nil) #:nodoc:
  native = native_database_types[type]
  limit ||= native[:limit]
  column_type_sql = native[:name]
  column_type_sql << "(#{limit})" if limit
  if column_type_sql
    column_type_sql
  else
    native
  end
end

I have no idea what the implications of such a change would be which is why I am submitting this as bug first and not a patch.

In the meantime I am just adding the column as unique and non null and creating an index for it, which is equivalent.

Attachments

ar-primary-key-test.txt (1.0 kB) - added by nathaniel@talbott.ws on 03/23/06 00:27:35.
Test showing the bug.
add_primary_key.diff (1.0 kB) - added by nzkoz on 03/26/06 01:30:51.
Patch to implement the functionality. Tests to come once other adapters can confirm the approach is feasible
active_schema_test_mysql.rb (1.1 kB) - added by jramirez on 01/12/07 18:46:43.
test to try the error before/after applying the patch (adding test_add_column_with_primary_key_type to existing unit test)
schema_statements.rb (11.8 kB) - added by jramirez on 01/12/07 18:49:57.
patch to fix the problem on edge version (2007/01/12)
migrations_add_primary_key_column.diff (1.6 kB) - added by jramirez on 01/12/07 18:51:08.
patch to fix the problem on edge version (2007/01/12)

Change History

02/04/06 20:23:50 changed by rrwhite@gmail.com

"In the meantime I am just adding the column as unique and non null and creating an index for it, which is equivalent."

I should also add that I have put a postgre_extensions.rb in my lib directory and required in my environment.rb that changes the native type mappgins to: 

  def native_database_types
    {
      :primary_key => "bigserial primary key",
      :string      => { :name => "character varying", :limit => 255 },
      :text        => { :name => "text" },
      :integer     => { :name => "integer" },
      :float       => { :name => "float" },
      :datetime    => { :name => "timestamp" },
      :timestamp   => { :name => "timestamp" },
      :time        => { :name => "time" },
      :date        => { :name => "date" },
      :binary      => { :name => "bytea" },
      :boolean     => { :name => "boolean" },
      :bigint      => { :name => "int8" },
      :bigserial   => { :name => "bigserial" }
    }
  end

Therefore I am adding the column in my migrations file using 

    add_column :appt_service_logins, :id, :bigserial, :null => false
    add_index :appt_service_logins, :id, :unique

Just wanted to clarify that part.

03/23/06 00:26:43 changed by nathaniel@talbott.ws

  • version changed from 1.0.0 to 1.1.0 RC1.
  • summary changed from Migrations fail adding a primary key column to [PATCH] Migrations fail adding a primary key column.

I've just run in to this, and am attaching a patch that adds a test (for MySQL) that shows the bug. I was sure of the best way to fix the problem - I'll leave that up to someone a bit more experienced with the full set of adapters.

Note: the attached test isn't included in the default suite.

03/23/06 00:27:35 changed by nathaniel@talbott.ws

  • attachment ar-primary-key-test.txt added.

Test showing the bug.

03/23/06 01:37:02 changed by nathaniel@talbott.ws

  • keywords changed from migrations to migrations needs_review.

OK, I've decided to tag this with needs_review because I really think it's going to bite folks moving to has_many :through.

03/25/06 23:33:06 changed by david

  • owner changed from David to michael@koziarski.com.

03/26/06 01:27:54 changed by anonymous

  • milestone set to 1.1.

03/26/06 01:29:57 changed by nzkoz

  • keywords changed from migrations needs_review to fd.

Block release

03/26/06 01:30:51 changed by nzkoz

  • attachment add_primary_key.diff added.

Patch to implement the functionality. Tests to come once other adapters can confirm the approach is feasible

03/27/06 19:13:43 changed by anonymous

For postgresql, it seems invalid SQL results when :text has any limit specified.

Solution can be:

(1) ignore any :limit for :text in postgresql, or

(2) change :text to :string (varchar) in postgresql

04/04/06 04:06:07 changed by anonymous

  • keywords deleted.
  • milestone changed from 1.1 to 1.2.

07/25/06 03:42:19 changed by anonymous

mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang