Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

has_many association build/create method is overwrite forign_key #5069

Closed
suzukis opened this Issue Feb 17, 2012 · 7 comments

Comments

Projects
None yet
5 participants

suzukis commented Feb 17, 2012

Behavior of build / create method created has_many association differs by rails 2 and rails 3.0 and after rails3.1

class Blog < AR:Base
   has_many :entries
end
class Entry < AR:Base; end

b = Blog.create
b.id # => 1

#rails 2.3
e = b.entries.build(:blog_id => 999) 
e.blog_id # => 1
e = b.entries.create(:blog_id => 999) 
e.blog_id # => 1

#rails 3.0.11
e = b.entries.build(:blog_id => 999) 
e.blog_id # => 1
e = b.entries.create(:blog_id => 999) 
e.blog_id # => 999

#rails 3.1.3 / 3.2.1
e = b.entries.build(:blog_id => 999) 
e.blog_id # => 999
e = b.entries.create(:blog_id => 999) 
e.blog_id # => 1
Member

drogus commented Feb 18, 2012

I think that using collection.build or collection.create along with overwriting foreign_key is a bit weird. Apart from that, it would be nice to have consistent and tested behavior here and I think that it should not allow to overwrite foreign key. If you want to overwrite it, don't use collection. This may be very misleading, as you're creating a record on a collection which inf fact should not land into that collection (which may also complicate code on rails side).

I would vote for fixing it to behave like in 2.3 and throw error or show warning on try to overwrite that key.

Contributor

rohit commented Mar 4, 2012

If you use attr_accessible in the Entry model then you should not be able to override :blog_id using either build or create, correct?

I don't think throwing an error is a good idea. A warning would be nice. But definitely the Rails 2.3 behavior is the preferred way imho.

byroot added a commit to byroot/rails that referenced this issue Mar 4, 2012

Member

byroot commented Mar 4, 2012

Here's a fix for that issue with test cases

Contributor

rohit commented Mar 4, 2012

Nice work! Tests and code looks good. I'm not familiar with the internals of AR but does has_many :through require tests too?

Member

byroot commented Mar 4, 2012

Good question, I'll check that soon.

Member

drogus commented Mar 4, 2012

Member

byroot commented Mar 4, 2012

@rohit: in fact has_many :through associations are readonly unless they delegate to a belongs_to association.

So while I'm not absolutely sure, I think they are not affected by this change.

@tenderlove tenderlove closed this in c97a166 Mar 5, 2012

sikachu pushed a commit to sikachu/rails that referenced this issue Mar 6, 2012

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