Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

Cannot assign values to non numeric primary key attributes in Model#new and #create

#3473

This is a model that defines a string field as primary key:

class Game < ActiveRecord::Base
set_primary_key "name" end

and this is the migration that creates the table and a couple of records.

class CreateGames < ActiveRecord::Migration
def self.up

create_table (:games, :id => false, :primary_key => "name")  do |t|
  t.string :name
end

Game.create(:name => "go")
Game.create(:name => "chess")

end ... end

Unfortunately the name attribute never gets assigned and is NULL inside the database.

Apparently there is no way to assign primary keys values into the new and create methods of the models. This is ok and desirable for the standard id auto incremented primary key but is bad when using non numeric primary keys. The workaround exists and is obvious but doesn't look nice:

x = Model.new(attributes_hash)
x.non_numeric_key = "value"
x.save

It is less DRY than a Model.create single liner.

I looked inside the ActiveRecord sources and found the culprit. It's the unless check inside the loop in this method:

  def attributes_from_column_definition
    self.class.columns.inject({}) do |attributes, column|
      attributes[column.name] = column.default unless column.name == self.class.primary_key
      attributes
    end
  end

I'm sure there are good reasons to have the check there. One I can think about is that AR is assuming that the primary key is managed by the database itself as an auto incremented field. On the other side rails gives us a way to define non numeric primary keys which cannot be auto incremented in a straightforward way so this looks like a sort of bug to me.

I implemented a workaround that doesn't remove the check because there might be a lot of code that assumes it's there. I want to be able to tell AR that I'm taking the responsibility to manage the values of the primary key. I added a new method to AR to do so and the model now is:

class Game < ActiveRecord::Base
set_primary_key "name" automatic_primary_key false # the new method end

Here's the patch to active_record/base.rb.
I had to define the automatic_primary_key method and modify the code of the initialize method, which is also called by create.

--- /usr/lib/ruby/gems/1.8/gems/activerecord-2.3.4/lib/active_record/base.rb 2009-11-09 12:22:12.000000000 +0100 +++ base.rb 2009-11-09 12:21:37.000000000 +0100 @@ -1219,6 +1219,18 @@

   end
   alias :primary_key= :set_primary_key
  • # PATCH
  • # Accept a value for the primary key in the Model#new and Model#create
  • # methods
  • def set_automatic_primary_key(value = false)
  •  define_attr_method :automatic_primary_key, value
    
  • end
  • alias :automatic_primary_key= :set_automatic_primary_key
  • def automatic_primary_key(value)
  •  @automatic_primary_key = value
    
  • end
  • # END OF PATCH + # Sets the name of the inheritance column to use to the given value, # or (if the value # is nil or false) to the value returned by the # given block. @@ -2432,6 +2444,12 @@ # hence you can't have attributes that aren't part of the table columns. def initialize(attributes = nil)
     @attributes = attributes_from_column_definition
    
  •  # PATCH
    
  •  unless @automatic_primary_key
    
  •    pk = self.class.primary_key
    
  •    @attributes.merge!({ pk => attributes[pk.to_sym] })
    
  •  end
    
  •  # END OF PATCH
     @attributes_cache = {}
     @new_record = true
     ensure_proper_type
    

I'm not familiar with the inner workings of AR so I really don't know if I'm breaking some other parts of rails but it seems to work. I'm also sure that there are better ways to do it and I'll be more than glad to be advised and learn.

Paolo

Reported by pmontrasio · November 9th, 2009 @ 12:15 PM

State: stale
Milestone: none
Assigned to: nobody
Importance: none

Activity

  1. pmontrasio
    pmontrasio

    I attach a better patch. The formatting of the one in the original post got garbled and it contained a but of its own anyway.

    November 15th, 2009 @ 09:16 AM

  2. Rohit Arondekar
    Rohit Arondekar
    • State changed from new to stale
    • Importance changed from to

    Marking ticket as stale. If this is still an issue please leave a comment with suggested changes, creating a patch with tests, rebasing an existing patch or just confirming the issue on a latest release or master/branches.

    October 6th, 2010 @ 06:36 AM

  3. Ryan Bigg
    Ryan Bigg
    • Tag cleared.

    Automatic cleanup of spam.

    October 11th, 2010 @ 10:57 AM