This project is archived and is in readonly mode.

#6337 ✓invalid
mat

Regression: mass assignment interaction with a non-standard primary key (2.3.5 -> 2.3.10)

Reported by mat | January 26th, 2011 @ 08:04 PM

We had to patch a year old app today that was running happily on 2.3.5. As part of the work, we did a routine upgrade to 2.3.10 and found a weird bug when using non-standard primary keys.

You can replicate this way. Create a new Rails app with 2.3.10 and then add these two files:

# app/models/country.rb
class Country < ActiveRecord::Base
  set_primary_key :iso
end

# db/migrations/20110126_create_countries.db
class CreateCountries < ActiveRecord::Migration
  def self.up
    create_table :countries, :id => false, :primary_key => :iso do |t|
      t.string :iso, :limit => 2, :null => false
      t.string :name
      t.integer :region_id

      t.timestamps
    end 

    execute "ALTER TABLE countries ADD PRIMARY KEY (iso);"
  end

  def self.down
    drop_table :countries
  end
end

Then create your database (you'll need to use MySQL or Postgres, not SQLite) and migrate it.

The following operations fail to set ID correctly:

# Create
Country.create(:iso => "AU", :name => "Australia")

# New & Save
c = Country.new(:iso => "AU", :name => "Australia")
c.save

This is the only way to set the iso column:

# Working #1
c = Country.new(:name => "Australia")
c.iso = "AU"
c.save

If you don't do this your primary key columns will be populated with the default value. Rails itself appears to be simply omitting the :iso column from the query:

INSERT INTO `countries` (`name`, `created_at`, `updated_at`, `region_id`)
VALUES('Australia', '2011-01-26 19:47:57', '2011-01-26 19:47:57', NULL)

These migrations worked under 2.3.5.

I don't know enough about ActiveRecord to find the location in the code but it appears it's making an assumption that the primary key cannot be mass assigned, and this change was introduced at some point between 2.3.5 and 2.3.10.

Comments and changes to this ticket

  • Cesario

    Cesario January 30th, 2011 @ 10:38 AM

    I'm afraid this is not a bug, and it can be reproduced on 2.3.5, 2.3.10 or 3.0.3.
    Primary keys are marked as protected, this test case will not fail:

    class Subscriber < AR::Base
  set_primary_key :nick
  attr_accessible :nick # Mark it available for mass-assignment
end

def test_customized_string_primary_key_set_with_create
  subscriber = Subscriber.create :nick => 'webster123'
  assert_equal 'webster123', subscriber.id
  assert_equal 'webster123', subscriber.nick
end

    Mass-assigning the key is just prevented unless you set it explicitly accessible.

    Even on 2.3.5, if I remove the attr_accessible line, i'm getting this result for the same test:

    1) Error:
test_customized_string_primary_key_set_with_create(PrimaryKeysTest):
ActiveRecord::StatementInvalid: SQLite3::ConstraintException: subscribers.nick may not be NULL: INSERT INTO "subscribers" ("name") VALUES(NULL)
  • mat

    mat January 30th, 2011 @ 09:56 PM

    I get that primary keys are protected, I don't get how the migration ever worked a year ago. Something's changed somewhere.

    I'll add the attr_accessible and we can all move on!

    Thanks

    M.

  • Cesario

    Cesario January 30th, 2011 @ 10:06 PM

    I won't jump into the code right now, but you could check in 2.3.5, at runtime, if the iso column has been set automatically attr_protected when you ran set_primary_key.

    In any case, I think we could close this ticket. I hope it helped :)

  • mat

    mat January 30th, 2011 @ 10:11 PM

    Yes, let's close it. Your explanation about why primary_keys are by default protected was very helpful and makes sense.

    I wish there was somewhere we could document that you need to open up the primary key for mass assignment if the purpose is to set it yourself. All the examples I found were to use things like sequences in Postgres, etc.

    Thanks

  • Rohit Arondekar

    Rohit Arondekar February 1st, 2011 @ 05:44 AM

    • State changed from “new” to “invalid”
    • Importance changed from “” to “Low”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Tags

Pages