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 jsonapi_include_toplevel_object adapter option #1991

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@nilsding

nilsding commented Dec 2, 2016

Purpose

Sometimes, I have the need to include the JSON-API top level object while it's disabled in the config. This PR adds the functionality to include it on the fly using an option to the serializer:

 ActiveModelSerializers::SerializableResource.new(
  UserSerializer.new(obj),
  adapter:                         :json_api,
  jsonapi_include_toplevel_object: true
).as_json

Changes

Pass instance options to the ActiveModelSerializers::Adapter::JsonApi::Jsonapi model, and modify the include_object? method to use the value of the instance option :jsonapi_include_toplevel_object instead of the global config, if given.

Caveats

Related GitHub issues

Additional helpful information

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Dec 2, 2016

@nilsding, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bf4, @domitian and @groyoh to be potential reviewers.

mention-bot commented Dec 2, 2016

@nilsding, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bf4, @domitian and @groyoh to be potential reviewers.

@NullVoxPopuli

This comment has been minimized.

Show comment
Hide comment
@NullVoxPopuli

NullVoxPopuli Dec 3, 2016

Contributor

@beauby do you have opinions on this?

Contributor

NullVoxPopuli commented Dec 3, 2016

@beauby do you have opinions on this?

@beauby

This comment has been minimized.

Show comment
Hide comment
@beauby

beauby Dec 4, 2016

Member

I agree this should be overridable locally (and although it is weird that jsonapi_include_toplevel_object would be an option on the resource, since it is a property of the document, not of the primary resource, I don't see an other way to do it currently).

Member

beauby commented Dec 4, 2016

I agree this should be overridable locally (and although it is weird that jsonapi_include_toplevel_object would be an option on the resource, since it is a property of the document, not of the primary resource, I don't see an other way to do it currently).

@bf4 bf4 changed the title from Add jsonapi_include_toplevel_object option to serializer to Add jsonapi_include_toplevel_object adapter option Dec 7, 2016

@bf4

Idea is good, needs discussion of option name and distinction between turning on default via an option and passing in the jsonapi object in an option.

@@ -2,7 +2,7 @@
module ActiveModelSerializers
class SerializableResource
ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter, :meta, :meta_key, :links, :serialization_context, :key_transform])
ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter, :meta, :meta_key, :links, :serialization_context, :key_transform, :jsonapi_include_toplevel_object])

This comment has been minimized.

@bf4

bf4 Dec 7, 2016

Member

Should just be jsonapi_object, I think. (jsonapi can't be the key since render uses that

@bf4

bf4 Dec 7, 2016

Member

Should just be jsonapi_object, I think. (jsonapi can't be the key since render uses that

This comment has been minimized.

@bf4

bf4 Dec 7, 2016

Member

A question I have is what should be different between wanting to pass in your own jsonapi object vs. telling the adapter to include the default one?

@bf4

bf4 Dec 7, 2016

Member

A question I have is what should be different between wanting to pass in your own jsonapi object vs. telling the adapter to include the default one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment