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.

Sign up for GitHub

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

Prevent loading association when include_data is set to false #1710

Merged
merged 1 commit into from
May 25, 2016
Merged

Prevent loading association when include_data is set to false #1710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ Breaking changes:
Features:

Fixes:
- [#1710](https://github.com/rails-api/active_model_serializers/pull/1710) Prevent association loading when `include_data` option
is set to `false`. (@groyoh)

Misc:
- [#1734](https://github.com/rails-api/active_model_serializers/pull/1734) Adds documentation for conditional attribute (@lambda2)
Expand Down
6 changes: 3 additions & 3 deletions lib/active_model/serializer/reflection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@ def value(serializer)

if block
block_value = instance_exec(serializer, &block)
if block_value == :nil
serializer.read_attribute_for_serialization(name)
else
if block_value != :nil
block_value
elsif @_include_data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when is @_include_data present?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aight, I guess we'll document _include_data at a later date

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already documented but the behavior added in this PR is not though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. 😱 :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense.

serializer.read_attribute_for_serialization(name)
end
else
serializer.read_attribute_for_serialization(name)
Expand Down
13 changes: 9 additions & 4 deletions test/adapter/json_api/relationships_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ class Serializer
module Adapter
class JsonApi
class RelationshipTest < ActiveSupport::TestCase
RelationshipAuthor = Class.new(::Model)
class RelationshipAuthor < ::Model; end

class RelationshipAuthorSerializer < ActiveModel::Serializer
has_one :bio do
link :self, '//example.com/link_author/relationships/bio'
Expand Down Expand Up @@ -71,7 +72,6 @@ def cached_roles

def setup
@post = Post.new(id: 1337, comments: [], author: nil)
@blog = Blog.new(id: 1337, name: 'extra')
@bio = Bio.new(id: 1337)
@like = Like.new(id: 1337)
@role = Role.new(id: 'from-record')
Expand All @@ -82,7 +82,6 @@ def setup
@author = RelationshipAuthor.new(
id: 1337,
posts: [@post],
blog: @blog,
reviewer: @reviewer,
bio: @bio,
likes: [@like],
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

😦

Copy link
Member

@bf4 bf4 Aug 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't understand why this test was changed, why blog was removed from the setup

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, this test file is basically garbage. See #1857 would be better to do an 'integration' test

super(attr)
end
expected = {
links: { self: '//example.com/link_author/relationships/blog' }
}
assert_relationship(:blog, expected)
assert_nothing_raised do
assert_relationship(:blog, expected)
end
end

def test_relationship_including_data_explicit
Expand Down