Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Using strings in association definition methods breaks eager loading #7418

Closed
wants to merge 1 commit into from

8 participants

@oruen

ActiveRecord::Associations::Preloader#records_by_reflection uses symbolized association name to find a reflection. If association was defined using string as it's name, model reflections hash would contain that string as a key — so Preloader fails to find that reflection. I think storing symbolized association names in reflections hash is a good solution.

Failing example:

class Post < ActiveRecord::Base
  belongs_to "author"
end

class Author < ActiveRecord::Base
  has_many :posts
end
rails> Post.includes(:author).where(:id => 1)
ActiveRecord::ConfigurationError: Association named 'author' was not found; perhaps you misspelled it?
@carlosantoniodasilva

I'm fine with the code change, the question is: why would one use a string? I think the API always shows/requires a symbol, so there'd be no need to support both I guess.

/cc @jonleighton

@oruen

There is no warning or error when using a string instead of symbol. So I guess it's better to support both argument types.

@vad4msiu

:+1:
I also encountered this problem. In API is not indicated that it is necessary to use symbol and you have to see the source code to understand what will happen if there will get a string. It would be nice not to think about what to write symbol or string.

@oruen

@carlosantoniodasilva Should this issues be assigned to someone to give it a go?

@steveklabnik
Collaborator

In most places we use both strings and symboles, so while I'm not sure why you'd ever use a string, I don't see this as being bad either.

@oruen

I've encountered this bug in one of the gems I use. String was used because it could be used. If rails wouldn't like to support strings in association names it should raise an exception.

@schneems
Collaborator

I'm fine with this, the only potential reason I could think we wouldn't want to accept multiple types would be for speed, but it seems pretty negligible to coherce a string to a symbol and would be done only once on initilization

Benchmark.measure {1000000.times {"foo".to_sym}}.real
 => 0.5129928588867188 

I say let's merge it.

@rafaelfranca

I'm not sure about this but I think we should not encourage strings as argument to associations. It is right now raising a exception and it is easy to fix it changing to a symbol.

@schneems
Collaborator

@rafaelfranca the thing I don't like about this currently is you have to wait till you use the association to get the error, and the error doesn't make it obvious what the issue is. If we want to discourage this I would say let's raise an error in the method, but if we did that likely people would ask if we knew what the intent was why not just fix it from the beginning. At the very least we should add some docs to the association methods to say they must use a symbol

@jonleighton
Collaborator

Declaring an association using a string rather than symbol as the name is seriously weird. We should just raise an ArgumentError at that point.

@rafaelfranca

So lets raise the exception. @oruen could you change this pull request?

@frodsan

@oruen Hi! Any update on this?

@steveklabnik steveklabnik referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@steveklabnik steveklabnik referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 22, 2012
  1. @oruen
This page is out of date. Refresh to see the latest.
View
2  activerecord/lib/active_record/associations/builder/association.rb
@@ -14,7 +14,7 @@ def self.build(*args, &block)
def initialize(model, name, scope, options)
@model = model
- @name = name
+ @name = name.to_sym
if scope.is_a?(Hash)
@scope = nil
View
3  activerecord/test/models/post.rb
@@ -10,7 +10,8 @@ def author
scope :limit_by, lambda {|l| limit(l) }
- belongs_to :author do
+ # to test string as association name
+ belongs_to "author" do
def greeting
"hello"
end
Something went wrong with that request. Please try again.