Prevent loading association when include_data is set to false #1710
Conversation
|
gasp! those errors! |
|
Yeah, already rebased to latest master. Not sure what happens here, I can't reproduce it locally. |
|
what if you uninstall the sqlite3 adapter? - it's complaining that it's not installed. While it's not a dep of ams, it is for the ActiveRecord tests |
|
I don't have sqlite3 adapter installed locally. |
|
even weirder |
|
Actually, sqlite3 adapter comes out of the box with AR. So THAT's even weirder. |
| block_value | ||
| elsif @_include_data |
There was a problem hiding this comment.
when is @_include_data present?
There was a problem hiding this comment.
I'm not sure about the question but it is set to true by default so includes the relationship data by default. The point of the PR is to prevent calling the relationship if include_data is set to false.
There was a problem hiding this comment.
aight, I guess we'll document _include_data at a later date
There was a problem hiding this comment.
It's already documented but the behavior added in this PR is not though.
There was a problem hiding this comment.
@NullVoxPopuli please don't merge code with missing else clauses. I was going to say it was a bug and raise a ❗ but then I realized that it was the feature of the PR. 😱 :(
|
woo! LGTM 🍕 |
|
@groyoh do you want to add a changelog entry? |
|
@NullVoxPopuli will do. |
|
@NullVoxPopuli added the CHANGELOG. |
|
thanks! merging! |
|
@NullVoxPopuli Although small, this PR has a very big impact for the performance of our app. Would it be possible to bump the version so that the latest version includes this? Thanks! |
|
you can specify a commit hash in your gemfile. :-) we aren't ready to bump the version quite yet. |
|
I know that :-). I was just wondering if a patch update was possible so that everyone can benefit from this change now, but I will resort to specifying the commit. |
|
This is a great addition, but wouldn't it make more sense if this was an option: has_one :blog, :include_data => false do |serializer|
serializer.cached_blog
endInstead of being set from within the block? Or have I missed something? |
| @@ -158,10 +157,16 @@ def test_relationship_meta | |||
| end | |||
|
|
|||
| def test_relationship_not_including_data | |||
| @author.define_singleton_method(:read_attribute_for_serialization) do |attr| | |||
| fail 'should not be called' if attr == :blog | |||
There was a problem hiding this comment.
this test needs to change as well. It shouldn't be raising a runtime error and is hard to read and why do we even need the define_singleton_method?
😦
There was a problem hiding this comment.
I also don't understand why this test was changed, why blog was removed from the setup
There was a problem hiding this comment.
In any case, this test file is basically garbage. See #1857 would be better to do an 'integration' test
Purpose
Fixes #1707.
Changes
Do not call
read_attribute_for_serializationwheninclude_datais set tofalse.