-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add top-level jsonapi member #1050
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
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 |
---|---|---|
|
@@ -9,3 +9,11 @@ The following configuration options can be set on `ActiveModel::Serializer.confi | |
## JSON API | ||
|
||
- `jsonapi_resource_type`: Whether the `type` attributes of resources should be singular or plural. Possible values: `:singular, :plural`. Default: `:plural`. | ||
- `jsonapi_include_toplevel_object`: Whether to include a [top level JSON API member](http://jsonapi.org/format/#document-jsonapi-object) | ||
in the response document. | ||
Default: `false`. | ||
- Used when `jsonapi_include_toplevel_object` is `true`: | ||
- `jsonapi_version`: The latest version of the spec the API conforms to. | ||
Default: `'1.0'`. | ||
- `jsonapi_toplevel_meta`: Optional metadata. Not included if empty. | ||
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. update config to encapsulate in json_api 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. rename to 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. top level meta wouldn't be set in a global config.. but name could be confusing 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. Agreed. |
||
Default: `{}`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,41 @@ class JsonApi < Base | |
autoload :PaginationLinks | ||
autoload :FragmentCache | ||
|
||
# TODO: if we like this abstraction and other API objects to it, | ||
# then extract to its own file and require it. | ||
module ApiObjects | ||
module JsonApi | ||
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 the 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 the 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 it's already the 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. good call. my intention was that I was considering having a whole bunch 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. Probably is a latent jsonapi config object here, or at least 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. Agreed, as mentioned in #1097, we should somehow namespace the config options. 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. Maybe we should do that in this PR before its too late? 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, it would make sense. Any specific way you'd like it to be? |
||
ActiveModel::Serializer.config.jsonapi_version = '1.0' | ||
ActiveModel::Serializer.config.jsonapi_toplevel_meta = {} | ||
# Make JSON API top-level jsonapi member opt-in | ||
# ref: http://jsonapi.org/format/#document-top-level | ||
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. @beauby :) |
||
ActiveModel::Serializer.config.jsonapi_include_toplevel_object = false | ||
|
||
module_function | ||
|
||
def add!(hash) | ||
hash.merge!(object) if include_object? | ||
end | ||
|
||
def include_object? | ||
ActiveModel::Serializer.config.jsonapi_include_toplevel_object | ||
end | ||
|
||
# TODO: see if we can cache this | ||
def object | ||
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. How about something like: def object
return @object if @object
object = {
jsonapi: {
version: ActiveModel::Serializer.config.jsonapi_version,
meta: ActiveModel::Serializer.config.jsonapi_toplevel_meta
}
}
object[:jsonapi].reject! { |_, v| v.blank? }
@object = object
@object
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. since we add meta in some of the tests, I'd have to write a way to clear the cache, which I don't want to do right now, could be in a test helper I guess. 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. which is why I left it as a todo :) |
||
object = { | ||
jsonapi: { | ||
version: ActiveModel::Serializer.config.jsonapi_version, | ||
meta: ActiveModel::Serializer.config.jsonapi_toplevel_meta | ||
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. Meta is optional as per the spec, so it should't be included by default. |
||
} | ||
} | ||
object[:jsonapi].reject! { |_, v| v.blank? } | ||
|
||
object | ||
end | ||
end | ||
end | ||
|
||
def initialize(serializer, options = {}) | ||
super | ||
@include_tree = IncludeTree.from_include_args(options[:include]) | ||
|
@@ -21,11 +56,16 @@ def initialize(serializer, options = {}) | |
def serializable_hash(options = nil) | ||
options ||= {} | ||
|
||
if serializer.respond_to?(:each) | ||
serializable_hash_for_collection(options) | ||
else | ||
serializable_hash_for_single_resource(options) | ||
end | ||
hash = | ||
if serializer.respond_to?(:each) | ||
serializable_hash_for_collection(options) | ||
else | ||
serializable_hash_for_single_resource(options) | ||
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. someday we'll reify these two latent objects.. |
||
|
||
ApiObjects::JsonApi.add!(hash) | ||
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. do we prefer this after? |
||
|
||
hash | ||
end | ||
|
||
def fragment_cache(cached_hash, non_cached_hash) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
require 'test_helper' | ||
|
||
module ActiveModel | ||
class Serializer | ||
module Adapter | ||
class JsonApi | ||
class TopLevelJsonApiTest < Minitest::Test | ||
def setup | ||
@author = Author.new(id: 1, name: 'Steve K.') | ||
@author.bio = nil | ||
@author.roles = [] | ||
@blog = Blog.new(id: 23, name: 'AMS Blog') | ||
@post = Post.new(id: 42, title: 'New Post', body: 'Body') | ||
@anonymous_post = Post.new(id: 43, title: 'Hello!!', body: 'Hello, world!!') | ||
@comment = Comment.new(id: 1, body: 'ZOMG A COMMENT') | ||
@post.comments = [@comment] | ||
@post.blog = @blog | ||
@anonymous_post.comments = [] | ||
@anonymous_post.blog = nil | ||
@comment.post = @post | ||
@comment.author = nil | ||
@post.author = @author | ||
@anonymous_post.author = nil | ||
@blog = Blog.new(id: 1, name: 'My Blog!!') | ||
@blog.writer = @author | ||
@blog.articles = [@post, @anonymous_post] | ||
@author.posts = [] | ||
end | ||
|
||
def test_toplevel_jsonapi_defaults_to_false | ||
assert_equal config.fetch(:jsonapi_include_toplevel_object), false | ||
end | ||
|
||
def test_disable_toplevel_jsonapi | ||
with_config(jsonapi_include_toplevel_object: false) do | ||
hash = serialize(@post) | ||
assert_nil(hash[:jsonapi]) | ||
end | ||
end | ||
|
||
def test_enable_toplevel_jsonapi | ||
with_config(jsonapi_include_toplevel_object: true) do | ||
hash = serialize(@post) | ||
refute_nil(hash[:jsonapi]) | ||
end | ||
end | ||
|
||
def test_default_toplevel_jsonapi_version | ||
with_config(jsonapi_include_toplevel_object: true) do | ||
hash = serialize(@post) | ||
assert_equal('1.0', hash[:jsonapi][:version]) | ||
end | ||
end | ||
|
||
def test_toplevel_jsonapi_no_meta | ||
with_config(jsonapi_include_toplevel_object: true) do | ||
hash = serialize(@post) | ||
assert_nil(hash[:jsonapi][:meta]) | ||
end | ||
end | ||
|
||
def test_toplevel_jsonapi_meta | ||
new_config = { | ||
jsonapi_include_toplevel_object: true, | ||
jsonapi_toplevel_meta: { | ||
'copyright' => 'Copyright 2015 Example Corp.' | ||
} | ||
} | ||
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. The value of meta must be a hash |
||
with_config(new_config) do | ||
hash = serialize(@post) | ||
assert_equal(new_config[:jsonapi_toplevel_meta], hash.fetch(:jsonapi).fetch(:meta)) | ||
end | ||
end | ||
|
||
private | ||
|
||
def serialize(resource, options = {}) | ||
serializable(resource, { adapter: :json_api }.merge!(options)).serializable_hash | ||
end | ||
end | ||
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.
I do think there's a latent
json_api
config object this exposesThere 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.
Indeed there is.