Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reify_has_many_through doesn't consider :source option #777

Closed
Mask opened this issue Apr 17, 2016 · 2 comments
Closed

reify_has_many_through doesn't consider :source option #777

Mask opened this issue Apr 17, 2016 · 2 comments
Milestone

Comments

@Mask
Copy link
Contributor

Mask commented Apr 17, 2016

If a has_many association uses both the :through and the :source option, reify restores completely arbitrary associations.

I isolated this problem here: https://gist.github.com/Mask/29e2e3d286183277d2f391aafc997025#file-bug_report_template-rb-L71

Notice that Alice starts with one friend (Bob), then gains another friend (so we now have Bob and Charlie in the friends collection). After reify, the friends collection isn't reverted back to only Bob, but suddenly contains Alice.
After tracing through the code, I think I have found the culprit:

https://github.com/airblade/paper_trail/blob/7733894b0d13c2a2a172ff0a8a6b1509eb02011c/lib/paper_trail/reifier.rb#L258

It calls association_foreign_key to find out how to follow the association. If we look at the implementation of association_foreign_key, we find:

https://github.com/rails/rails/blob/ac027338e4a165273607dccee49a3d38bc836794/activerecord/lib/active_record/reflection.rb#L388-L390

def association_foreign_key
  @association_foreign_key ||= options[:association_foreign_key] || class_name.foreign_key
end

Well, options[:association_foreign_key] will always we nil, since it can only be specified for HABTM associations, not has_many :through. So this method always falls back to class_name.foreign_key. The result is that paper_trail's reify_has_many_through only follows the correct association if its name matches the class it's pointing to. If my example, it returns user_id(since to points to the User model) instead of to_id (as specified in the :source option).

I'll research this a bit more and see if I can come up with a PR to fix this.

@Mask
Copy link
Contributor Author

Mask commented Apr 17, 2016

think I've got it.

at https://github.com/airblade/paper_trail/blob/7733894b0d13c2a2a172ff0a8a6b1509eb02011c/lib/paper_trail/reifier.rb#L258

replacing assoc.association_foreign_key with assoc.source_reflection.foreign_key fixes this.
I can get a PR ready if you prefer.

Mask added a commit to Mask/paper_trail that referenced this issue Apr 17, 2016
…son to authorship.author so that source is not the same as the class name) to verify this case.
@jaredbeck
Copy link
Member

If a has_many association uses both the :through and the :source option, reify restores completely arbitrary associations.

Yikes.

I can get a PR ready if you prefer.

Yes please! Let us know if you have trouble adapting your current test to fit in the existing test suite.

@jaredbeck jaredbeck added this to the 5.0.0 milestone Apr 19, 2016
Mask added a commit to Mask/paper_trail that referenced this issue Apr 20, 2016
…son to authorship.author so that source is not the same as the class name) to verify this case.
jaredbeck pushed a commit that referenced this issue Apr 20, 2016
Also modified test (changed authorship.person to authorship.author so that source is not the same as the class name) to verify this case.

[Fixes #777]
jaredbeck pushed a commit that referenced this issue Apr 20, 2016
Also modified test (changed authorship.person to authorship.author so that source is not the same as the class name) to verify this case.

[Fixes #777]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants