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

Ticket #3735 (closed defect: fixed)

Opened 4 years ago

Last modified 3 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 mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang mushang

01/12/07 18:46:43 changed by jramirez

  • attachment active_schema_test_mysql.rb added.

test to try the error before/after applying the patch (adding test_add_column_with_primary_key_type to existing unit test)

01/12/07 18:49:57 changed by jramirez

  • attachment schema_statements.rb added.

patch to fix the problem on edge version (2007/01/12)

01/12/07 18:51:08 changed by jramirez

  • attachment migrations_add_primary_key_column.diff added.

patch to fix the problem on edge version (2007/01/12)

01/12/07 19:05:55 changed by jramirez

  • cc set to jramirez@aspgems.com.
  • keywords set to tiny.
  • status changed from new to closed.
  • resolution set to fixed.

Patch submitted and unit test provided

With this patch, we are returning directly the native type in case it's a String and not a hash, so it would work for :primary_key type provided the native adapter is defining this type. If the type is undefined, then we'll get the same error we are getting for unsupported types, which I would say is the expected behaviour.

In the diff file I'm providing the fix to the patch and also adding the unit test for mysql testing (for different adapters the assert string should be changed accordingly, but the point of the test is trying the defined string is returned unchanged and not the concrete sql syntax, so one adapter is enough)

note: accidentally I submitted the whole file instead of the diff (unfortunately i have no privileges to remove it from the page).I submitted the diff immediately after that