-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add support for resource-level JSON API links. #1246
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,9 @@ def self.digest_caller_file(caller_line) | |
self._attributes ||= [] | ||
class_attribute :_attributes_keys # @api private : maps attribute value to explict key name, @see Serializer#attribute | ||
self._attributes_keys ||= {} | ||
class_attribute :_links # @api private : links definitions, @see Serializer#link | ||
self._links ||= {} | ||
|
||
serializer.class_attribute :_cache # @api private : the cache object | ||
serializer.class_attribute :_fragmented # @api private : @see ::fragmented | ||
serializer.class_attribute :_cache_key # @api private : when present, is first item in cache_key | ||
|
@@ -72,6 +75,7 @@ def self.inherited(base) | |
caller_line = caller.first | ||
base._attributes = _attributes.dup | ||
base._attributes_keys = _attributes_keys.dup | ||
base._links = _links.dup | ||
base._cache_digest = digest_caller_file(caller_line) | ||
super | ||
end | ||
|
@@ -83,6 +87,10 @@ def self.type(type) | |
self._type = type | ||
end | ||
|
||
def self.link(name, value = nil, &block) | ||
_links[name] = block || value | ||
end | ||
|
||
# @example | ||
# class AdminAuthorSerializer < ActiveModel::Serializer | ||
# attributes :id, :name, :recent_edits | ||
|
@@ -249,6 +257,12 @@ def attributes(requested_attrs = nil) | |
end | ||
end | ||
|
||
# @api private | ||
# Used by JsonApi adapter to build resource links. | ||
def links | ||
self.class._links | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding an instance method is still kind of dangerous, no? |
||
|
||
protected | ||
|
||
attr_accessor :instance_options | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ class JsonApi < Base | |
extend ActiveSupport::Autoload | ||
autoload :PaginationLinks | ||
autoload :FragmentCache | ||
autoload :Link | ||
|
||
# TODO: if we like this abstraction and other API objects to it, | ||
# then extract to its own file and require it. | ||
|
@@ -94,16 +95,15 @@ def serializable_hash_for_collection(options) | |
|
||
if serializer.paginated? | ||
hash[:links] ||= {} | ||
hash[:links].update(links_for(serializer, options)) | ||
hash[:links].update(pagination_links_for(serializer, options)) | ||
end | ||
|
||
hash | ||
end | ||
|
||
def serializable_hash_for_single_resource | ||
primary_data = primary_data_for(serializer) | ||
relationships = relationships_for(serializer) | ||
primary_data[:relationships] = relationships if relationships.any? | ||
primary_data = resource_object_for(serializer) | ||
|
||
hash = { data: primary_data } | ||
|
||
included = included_resources(@include_tree, [primary_data]) | ||
|
@@ -136,22 +136,27 @@ def resource_identifier_for(serializer) | |
{ id: id.to_s, type: type } | ||
end | ||
|
||
def attributes_for(serializer, fields) | ||
serializer.attributes(fields).except(:id) | ||
end | ||
|
||
def resource_object_for(serializer) | ||
cache_check(serializer) do | ||
resource_object = cache_check(serializer) do | ||
resource_object = resource_identifier_for(serializer) | ||
|
||
requested_fields = fieldset && fieldset.fields_for(resource_object[:type]) | ||
attributes = serializer.attributes(requested_fields).except(:id) | ||
attributes = attributes_for(serializer, requested_fields) | ||
resource_object[:attributes] = attributes if attributes.any? | ||
resource_object | ||
end | ||
end | ||
|
||
def primary_data_for(serializer) | ||
if serializer.respond_to?(:each) | ||
serializer.map { |s| resource_object_for(s) } | ||
else | ||
resource_object_for(serializer) | ||
end | ||
relationships = relationships_for(serializer) | ||
resource_object[:relationships] = relationships if relationships.any? | ||
|
||
links = links_for(serializer) | ||
resource_object[:links] = links if links.any? | ||
|
||
resource_object | ||
end | ||
|
||
def relationship_value_for(serializer, options = {}) | ||
|
@@ -191,9 +196,7 @@ def add_included_resources_for(serializer, include_tree, primary_data, included) | |
else | ||
return unless serializer && serializer.object | ||
|
||
resource_object = primary_data_for(serializer) | ||
relationships = relationships_for(serializer) | ||
resource_object[:relationships] = relationships if relationships.any? | ||
resource_object = resource_object_for(serializer) | ||
|
||
return if included.include?(resource_object) || primary_data.include?(resource_object) | ||
included.push(resource_object) | ||
|
@@ -204,7 +207,21 @@ def add_included_resources_for(serializer, include_tree, primary_data, included) | |
end | ||
end | ||
|
||
def links_for(serializer, options) | ||
def links_for(serializer) | ||
serializer.links.each_with_object({}) do |(name, value), hash| | ||
hash[name] = | ||
if value.respond_to?(:call) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why check respond_to? call? I think cause, what if I set value to an instance of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basic duck-typing for a callable object (mainly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the if should use is_a? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really. Rails uses this kind of duck-typing as well: https://github.com/rails/rails/blob/7f18ea14c893cb5c9f04d4fda9661126758332b5/activemodel/lib/active_model/validations/clusivity.rb#L18. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then can you put a note in here about what sort of objects are expected, because if I have a Phone for my value, it's gonna error...... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah but common sense kinda plays its role here: a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair enough. I guess this comment doesn't really matter here. I'll bring it up when I see other duck-typing usages |
||
link = Link.new(serializer) | ||
link.instance_eval(&value) | ||
|
||
link.to_hash | ||
else | ||
value | ||
end | ||
end | ||
end | ||
|
||
def pagination_links_for(serializer, options) | ||
JsonApi::PaginationLinks.new(serializer.object, options[:context]).serializable_hash(options) | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
module ActiveModel | ||
class Serializer | ||
module Adapter | ||
class JsonApi | ||
class Link | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should other pieces of the JSON API spec have their own classes as well? (not that it should be part of this PR, but just wondering what the overall plan is) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly, I don't have a definitive answer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so maybe, but that's what the ApiObjects namespace was for, and also the two RFCs |
||
def initialize(serializer) | ||
@object = serializer.object | ||
@scope = serializer.scope | ||
end | ||
|
||
def href(value) | ||
self._href = value | ||
end | ||
|
||
def meta(value) | ||
self._meta = value | ||
end | ||
|
||
def to_hash | ||
hash = { href: _href } | ||
hash.merge!(meta: _meta) if _meta | ||
|
||
hash | ||
end | ||
|
||
protected | ||
|
||
attr_accessor :_href, :_meta | ||
attr_reader :object, :scope | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,8 +5,19 @@ class Serializer | |
module Adapter | ||
class JsonApi | ||
class LinksTest < Minitest::Test | ||
LinkAuthor = Class.new(::Model) | ||
class LinkAuthorSerializer < ActiveModel::Serializer | ||
link :self do | ||
href "//example.com/link_author/#{object.id}" | ||
meta stuff: 'value' | ||
end | ||
|
||
link :other, '//example.com/resource' | ||
end | ||
|
||
def setup | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was this an invalid test before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's arguably valid, but not recommended by the JSON API team |
||
@post = Post.new(id: 1337, comments: [], author: nil) | ||
@author = LinkAuthor.new(id: 1337) | ||
end | ||
|
||
def test_toplevel_links | ||
|
@@ -15,16 +26,36 @@ def test_toplevel_links | |
adapter: :json_api, | ||
links: { | ||
self: { | ||
href: '//posts' | ||
href: '//example.com/posts', | ||
meta: { | ||
stuff: 'value' | ||
} | ||
} | ||
}).serializable_hash | ||
expected = { | ||
self: { | ||
href: '//posts' | ||
href: '//example.com/posts', | ||
meta: { | ||
stuff: 'value' | ||
} | ||
} | ||
} | ||
assert_equal(expected, hash[:links]) | ||
end | ||
|
||
def test_resource_links | ||
hash = serializable(@author, adapter: :json_api).serializable_hash | ||
expected = { | ||
self: { | ||
href: '//example.com/link_author/1337', | ||
meta: { | ||
stuff: 'value' | ||
} | ||
}, | ||
other: '//example.com/resource' | ||
} | ||
assert_equal(expected, hash[:data][:links]) | ||
end | ||
end | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut is to avoid long-lived blocks, as there may be a lot of these, and the memory would never be released
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative? You actually need this block every time to instantiate a serializer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only if we design it this way. simple solution is just pass in
links
to the adapter until we need moreThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't understand your last comment. Could you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note also that there would be at most 1 block per serializer class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An other option would be to declare each link as a lambda, but that would make much more stored procs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't think we need this code at all until someone needs it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. Are you saying we should not support JSON API resource-level links?