Fixes #1211, include_tree is null when using the key: options #1214
Conversation
| @@ -92,7 +92,7 @@ def associations(include_tree = DEFAULT_INCLUDE_TREE) | |||
|
|
|||
| Enumerator.new do |y| | |||
| self.class._reflections.each do |reflection| | |||
| next unless include_tree.key?(reflection.name) | |||
| next unless include_tree.try(:key?, reflection.name) | |||
There was a problem hiding this comment.
This fix is not right, because then you can never explicitly include an association with a custom key.
There was a problem hiding this comment.
Clearly, the test should be on reflection.key instead of reflection.name in order to handle custom keys (reflection.key falls back to reflection.name anyways if one was not provided). I'm still trying to understand what caused include_tree to be nil though.
|
ok, hopefully the build passes. any other out standing concerns? |
| @@ -92,7 +92,8 @@ def associations(include_tree = DEFAULT_INCLUDE_TREE) | |||
|
|
|||
| Enumerator.new do |y| | |||
| self.class._reflections.each do |reflection| | |||
| next unless include_tree.key?(reflection.name) | |||
| key = reflection.options.fetch(:key, reflection.name) | |||
| next unless include_tree.key?(key) | |||
|
yay it passed! |
NullVoxPopuli
force-pushed
the
issue/1211-failing-test
branch
from
ff0ea6e
to
fc20908
Compare
|
rebased |
|
LGTM, let's squash and merge. |
…hen building associations
NullVoxPopuli
force-pushed
the
issue/1211-failing-test
branch
from
fc20908
to
f8323fc
Compare
|
done |
|
the author says hut8, should that be changed? he did point out the issue and gave us a failing test :-) |
|
He did point out the issue so that seems fair. |
|
I'm fine with @NullVoxPopuli being the author. I wrote the failing test but he fixed it which is really what counts. |
|
@hut8, I'm leaving you as the author, to show how thankful we are that you found the bug, and gave us the test. :-) |
Fix #1211, include_tree is null when using the key: options
Fixes #1211, and Closes #1212
Thanks @hut8 for the failing test!