Skip to content
This repository

belongs_to not very DRY #1661

Closed
bitaxis opened this Issue · 5 comments

2 participants

Nathan Brazil Andrew White
Nathan Brazil

With the :foreign_key option commented out in the code snippet below, AR will try to put nil into the user_id column on save. But if I uncomment it, it works fine.

class User < ActiveRecord::Base
    has_many :posts, :dependent => :destroy
end

class Post < ActiveRecord::Base
    belongs_to :author, :class_name => "User" #, :foreign_key => :user_id
end

user = User.new(...)
post = Post.new(:author => user)
user.save # ActiveRecord::StatementInvalid is raised if :foreign_key is commented out

Seems to me in the above example, the foreign key column can, by default, be inferred from either the class name (I prefer this one) or the association name instead of requiring me to explicitly specify it, which doesn't seem very DRY.

Andrew White
Owner

But what about the situation where there are multiple belongs_to, e.g:

class Post < ActiveRecord::Base
  belongs_to :created_by, :class_name => 'User'
  belongs_to :updated_by, :class_name => 'User'
end

This is a long-standing convention and isn't going to be changed as it would impact a huge number of developers.

Andrew White pixeltrix closed this
Nathan Brazil
Andrew White
Owner

What do you get at the moment? Isn't it a NoMethodError for author_id - that's seems enough for me. You could alter derive_foreign_key to check if the target for respond_to?(foreign_key) but it could have unintended consequences by loading models earlier than they would've been or triggering false positives if methods are defined in modules that are included after the association is defined.

Nathan Brazil

Well, author_id yields NoMethodError whether I specify the :foreign_key option or not.

In the code snippet I originally provided, ActiveRecord would happily write a null to the user_id column. My code errored out only because I had set :null => false on the user_id column in the migration that creates the posts table.

So long-standing convention or not, this seems like a bug to me.

Andrew White
Owner

I'm not sure what you're asking for - an error being raised if there exists a user_id column? You might have a belongs_to :user and a belongs_to :author in the same model, so you'd have to check to see if it's used in another association and even then it might just be a legacy column in a database so that isn't going to be okay.

The other option that I thought that you wanted to do was to raise an error if the model doesn't respond to the derived foreign key name, but even that isn't going to be acceptable, e.g:

class Post < ActiveRecord::Base
  belongs_to :author

  def author_id
    #some custom method
  end

  def author_id=(value)
    #some custom method
  end
end

This is perfectly acceptable at the moment but would raise an error if we check respond_to?.

Jake Varghese jake3030 referenced this issue from a commit in jake3030/rails
Joshua Peek josh Build query string and POST params parser on top of Rack::Request. Al…
…so switch our multipart parser to use Racks. Moved XML, JSON, and YAML parsers into ActionController::ParamsParser middleware [#1661 state:resolved]
ff0a267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.