New records should load has_many relationships with custom primary keys

[samsonasu]

If a has_many relationship is defined with a custom primary key, a new, unsaved record with that key set will not load the association. This is because AR checks new_record? and assumes (correctly in normal situations) that an unsaved record couldn't possibly have any related records, so it skips the round trip to the database.

I believe this is an error, and the association should be loaded from the database.

class Author < ActiveRecord::Base 
  has_many :essays, :primary_key => :name, :as => :writer
end

Author.new(:name => "David").essays # => []
                                    # => Should return [#<Essay ...>]

This pull request fixes this issue. I tested the 4 big connection adapters and added 2 tests.

I overrode the foreign_key_defined? method in has_many_association to handle this since that seemed to be written almost specifically for this purpose in has_many_through, but if that isn't the best way to handle this I'm open to discussion.

Thanks

[jonleighton]

Please could you add some tests to check the actual list of records returned, as well as checking the size? Also please use assert_equal, e.g.

assert_equal 1, foo.size

Not:

assert foo.size == 1

[samsonasu]

Thanks for the tip on assert_equal.

I added a check to that test to ensure the proper list of essays is returned.

I assumed custom primary_key on a has_many was already under test coverage which wasn't true so I added a basic test for that case.

If there is anything else you think should be added, please let me know.

[vijaydev]

You should not commit the config.yml

[samsonasu]

Sorry about that, good catch.

[arunagw]

Squashing extra commits is also a good practice :-)

[jonleighton]

This is not actually testing the bug, as it's not a new record?!

[samsonasu]

line 1404 author is never saved and is a new record. Should I move line 1404 to a separate test?

On line 1409, I added this test because the custom primary key functionality of a has_many was not under test coverage previously, even for the standard case of an existing record.

[jonleighton]

Made a couple more comments. Please also squash the commits as suggested above. Thanks.

[samsonasu]

I squashed the commits and cleaned up the comments. Is there anything else needed?