Skip to content

Fix bug displaying nil for relationship link href #1516

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

Merged
merged 1 commit into from
Feb 16, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ Features:
- [#1340](https://github.com/rails-api/active_model_serializers/pull/1340) Add support for resource-level meta. (@beauby)

Fixes:
- [#1516](https://github.com/rails-api/active_model_serializers/pull/1501) No longer return a nil href when only
adding meta to a relationship link. (@groyoh)
- [#1458](https://github.com/rails-api/active_model_serializers/pull/1458) Preserve the serializer
type when fragment caching. (@bdmac)
- [#1477](https://github.com/rails-api/active_model_serializers/pull/1477) Fix `fragment_cached?`
Expand Down
5 changes: 3 additions & 2 deletions lib/active_model/serializer/adapter/json_api/link.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ def meta(value)
def as_json
return @value if @value

hash = { href: @href }
hash.merge!(meta: @meta) if @meta
hash = {}
hash[:href] = @href if @href
hash[:meta] = @meta if @meta

hash
end
Expand Down
61 changes: 40 additions & 21 deletions test/adapter/json_api/relationship_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,25 @@ class RelationshipAuthorSerializer < ActiveModel::Serializer

has_many :locations do
link :related do
ids = object.locations.map!(&:id).join(',')
ids = object.locations.map(&:id).join(',')
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 was using map! instead of map with actually change the array and might cause some issues.

href "//example.com/locations/#{ids}"
end
end

has_many :posts do
link :related do
ids = object.posts.map!(&:id).join(',')
ids = object.posts.map(&:id).join(',')
href "//example.com/posts/#{ids}"
Copy link
Member

Choose a reason for hiding this comment

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

Could you comment on this change as an assertion for the bug fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

In #1504, I originally wrote the test to test the meta call and there was no call to href. But then found out that we had this bug and added a call to href to prevent having a nil href. This change simply remove the href to test the behavior I wanted to test from the beginning. Thinking about it again, I could also had another test instead.

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 added another spec instead to make it more clear.

meta ids: ids
end
end

has_many :comments do
Copy link
Member Author

Choose a reason for hiding this comment

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

This actually does a call to meta without calling href.

link :self do
meta ids: [1]
end
end

has_many :roles do
meta count: object.posts.count
end
Expand All @@ -48,7 +54,7 @@ class RelationshipAuthorSerializer < ActiveModel::Serializer

has_many :likes do
link :related do
ids = object.likes.map!(&:id).join(',')
ids = object.likes.map(&:id).join(',')
href "//example.com/likes/#{ids}"
meta ids: ids
end
Expand All @@ -65,6 +71,7 @@ def setup
@profile = Profile.new(id: 1337)
@location = Location.new(id: 1337)
@reviewer = Author.new(id: 1337)
@comment = Comment.new(id: 1337)
@author = RelationshipAuthor.new(
id: 1337,
posts: [@post],
Expand All @@ -74,12 +81,12 @@ def setup
likes: [@like],
roles: [@role],
locations: [@location],
profile: @profile
profile: @profile,
comments: [@comment]
)
end

def test_relationship_simple_link
hash = serializable(@author, adapter: :json_api).serializable_hash
expected = {
data: {
id: '1337',
Expand All @@ -89,31 +96,28 @@ def test_relationship_simple_link
self: '//example.com/link_author/relationships/bio'
}
}
assert_equal(expected, hash[:data][:relationships][:bio])
assert_relationship(:bio, expected)
end

def test_relationship_block_link
hash = serializable(@author, adapter: :json_api).serializable_hash
expected = {
data: { id: '1337', type: 'profiles' },
links: { related: '//example.com/profiles/1337' }
}
assert_equal(expected, hash[:data][:relationships][:profile])
assert_relationship(:profile, expected)
end

def test_relationship_block_link_href
hash = serializable(@author, adapter: :json_api).serializable_hash
expected = {
data: [{ id: '1337', type: 'locations' }],
links: {
related: { href: '//example.com/locations/1337' }
}
}
assert_equal(expected, hash[:data][:relationships][:locations])
assert_relationship(:locations, expected)
end

def test_relationship_block_link_meta
hash = serializable(@author, adapter: :json_api).serializable_hash
def test_relationship_block_link_href_and_meta
expected = {
data: [{ id: '1337', type: 'posts' }],
links: {
Expand All @@ -123,37 +127,45 @@ def test_relationship_block_link_meta
}
}
}
assert_equal(expected, hash[:data][:relationships][:posts])
assert_relationship(:posts, expected)
end

def test_relationship_block_link_meta
expected = {
data: [{ id: '1337', type: 'comments' }],
links: {
self: {
meta: { ids: [1] }
}
}
}
assert_relationship(:comments, expected)
end

def test_relationship_meta
hash = serializable(@author, adapter: :json_api).serializable_hash
expected = {
data: [{ id: '1337', type: 'roles' }],
meta: { count: 1 }
}
assert_equal(expected, hash[:data][:relationships][:roles])
assert_relationship(:roles, expected)
end

def test_relationship_not_including_data
hash = serializable(@author, adapter: :json_api).serializable_hash
expected = {
links: { self: '//example.com/link_author/relationships/blog' }
}
assert_equal(expected, hash[:data][:relationships][:blog])
assert_relationship(:blog, expected)
end

def test_relationship_including_data_explicit
hash = serializable(@author, adapter: :json_api).serializable_hash
expected = {
data: { id: '1337', type: 'authors' },
meta: { name: 'Dan Brown' }
}
assert_equal(expected, hash[:data][:relationships][:reviewer])
assert_relationship(:reviewer, expected)
end

def test_relationship_with_everything
hash = serializable(@author, adapter: :json_api).serializable_hash
expected = {
data: [{ id: '1337', type: 'likes' }],
links: {
Expand All @@ -164,7 +176,14 @@ def test_relationship_with_everything
},
meta: { liked: true }
}
assert_equal(expected, hash[:data][:relationships][:likes])
assert_relationship(:likes, expected)
end

private

def assert_relationship(relationship_name, expected)
Copy link
Member Author

Choose a reason for hiding this comment

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

Created this helper method to DRY the code.

Copy link
Member

Choose a reason for hiding this comment

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

nice.

hash = serializable(@author, adapter: :json_api).serializable_hash
assert_equal(expected, hash[:data][:relationships][relationship_name])
end
end
end
Expand Down