fix issue in #reify_has_many_through

[theRealNG]

Fix for #590

The assoc.foreign_key holds true only when the association is as follows

Book
has_many :authorships
has_many :authors, :through => :authorships

Authorships
belongs_to :book
belongs_to :author

Author
has_many :authorships
has_many :books, :through => :authorships

In this case the assoc.foreign_key and assoc.association_foreign_key are the same

But in the case of the following associations

Document
has_many :sections
has_many :paragraphs, :through => :sections

Section
belongs_to :document
has_many :paragraphs

Paragraph
belongs_to :section

the assoc.foreign_key and assoc.association_foreign_key are different and assoc.association_foreign_key gets the correct list of associated foreign_keys for the model.

For Ref: http://www.rubydoc.info/docs/rails/3.0.0/ActiveRecord%2FReflection%2FAssociationReflection%3Aassociation_foreign_key

EDIT:
When a nested has_many relation is present, using the "reify_has_manys" method to reify the has_many through associations.

[jaredbeck]

Thanks for the PR! Can you add a test that demonstrates the issue?

[jaredbeck]

Can you add a test that demonstrates the issue?

Just to clarify, we know that #590 is a confirmed bug, but it'd still be nice to have a test. Thanks.

[theRealNG]

Ya sure. Will work on it.

[jaredbeck]

You should be able to use the failing test I already wrote (https://github.com/jaredbeck/pt_issue_590/blob/de54892426966d146a13c8c2382735826aa76660/spec/model/document_spec.rb) as a starting point.

[jaredbeck]

The method is called reify, not reified. Please update the context string here.

[jaredbeck]

@theRealNG this is looking good. I appreciate how thorough you were with the tests. My comments are mostly about style. I'm trying my best to understand the meat of the change, but I'd really like to get Ben's opinion as he is more familiar with the reify functions.

CC: @batter

[theRealNG]

Bug:
We are not handling nested has_many associations.

Fix:
For a nested has_many associations we can call "has_manys" method for each of the nested associations so that we handle any levels of nesting.

Explanation with example :-

• A chapter has many sections and a section has many paragraphs.
• So when we call "reify_has_many_directly" on the chapter object, we get a list of sections that are associated with the chapter.
• Since the association between section and paragraphs is also a has_many association, we can call reify_has_many_directly on each of the sections we got from the above result to get all the paragraphs associated to that section and in turn associated to the chapter.

But i'm calling "reify_has_manys" on each section so that we can handle any levels of nesting.
(ex:- A chapter has_many sections, A section has_many paragraphs and a paragraph belongs_to a source)

[jaredbeck]

If I revert the change to this one line, by calling assoc.foreign_key instead of assoc.association_foreign_key, all the tests still pass. So, is it necessary to change this line?

[theRealNG]

In the example of Document, Sections and paragraphs:
The through_model is a object of Section.

What we are trying to do is get the list of all the paragraphs associated to the section by calling:
section.paragraph_ids

If we do Section.reflect_on_association(:paragraphs).foreign_key, it returns 'section_id' and if we do Section.reflect_on_association(:paragraphs).association_foreign_key, it returns 'paragraph_id'. So we should be using 'association_foreign_key' instead of 'foreign_key'.

This was not an issue so far because in a 'belongs_to' association assoc.association_foreign_key and assoc.foreign_key are the same.

Even if you replace assoc.association_foreign_key with assoc.foreign_key it works because there is no chance of 'has_many' associations to hit the line because we are calling 'return' on line:424

[jaredbeck]

That all makes sense, thanks.

[jaredbeck]

Ben, I think this is ready to merge, now that it includes theRealNG#1

What do you think?

[theRealNG]

Do you want me to squash the commits into a single commit?

[jaredbeck]

Do you want me to squash the commits into a single commit?

Sure, thanks.

[batter]

👍 Nice work, thanks to both of you.

My take is it's fine to leave it as a split commit as long as the merge has no fast forward but I don't care

[batter]

@jaredbeck Did the test we wrote ever end up working out of curiosity?

[jaredbeck]

@jaredbeck Did the test we wrote ever end up working out of curiosity?

Yes, see theRealNG#1

[batter]

So what is our merge process now for these types of things @jaredbeck?

Merge to master, then cherry-pick / backport to 4.1-stable, 4.1-stable?

[jaredbeck]

So what is our merge process now for these types of things?
Merge to master, then cherry-pick / backport to [4.0-stable], 4.1-stable?

Yes, that's how I picture it working. I think it's a common strategy; it's what rails seems to do. One thing I'm not sure of is how to run CI for a backport. I guess we could do a PR for each backport..

[batter]

ok that sounds good, just wanted to be clear about how you were envisioning it