Skip to content

Add support for jsonapi top level member #1147

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

Closed
wants to merge 1 commit into from

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Sep 14, 2015

C.f. http://jsonapi.org/format/#document-top-level

Supports :

  • a global boolean config option jsonapi_toplevel_member, that decides whether a top level jsonapi member will be included in each serialized resource, which defaults to false,
  • a global option jsonapi_version, which defaults to 1.0,
  • an adapter option jsonapi_toplevel_meta.

This PR is based on #1121 for convenience.

@beauby beauby force-pushed the add-top-level-jsonapi branch 2 times, most recently from 2f40b25 to 336972d Compare September 14, 2015 04:47
@beauby beauby force-pushed the add-top-level-jsonapi branch 5 times, most recently from 47509d8 to c3c992c Compare September 15, 2015 08:20
@@ -9,3 +9,5 @@ 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_toplevel_member`: Whether to include a [top level JSON API member](http://jsonapi.org/format/#document-jsonapi-object) in the response document. Default: `false`.
- `jsonapi_version`: The latest version of the spec the API conforms to. Used when `jsonapi_toplevel_member` is `true`. Default: `'1.0'`.
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

@joaomdmoura
Copy link
Member

This is cool, I'm not sure about the adapter options, feels like edge cases for me that might add complexity to maintain.
It also makes me thing we indeed need to add namespace to the config options 😁 It will get mess quickly if we keep it this way and adding options.

@beauby
Copy link
Contributor Author

beauby commented Sep 15, 2015

Hmm namespacing config options... I remember someone very wise mentioning that in #1097 :p
I don't mind removing the option for now.

@joaomdmoura
Copy link
Member

So lets remove the adapter options? So can add it if someone supports/needs it 😁
We can handle the namespace config on other PR 😄

@beauby beauby force-pushed the add-top-level-jsonapi branch 2 times, most recently from 9b8716b to 1a167f8 Compare September 15, 2015 10:17
@beauby beauby force-pushed the add-top-level-jsonapi branch from 1a167f8 to 8ab4c4a Compare September 15, 2015 10:18
@beauby
Copy link
Contributor Author

beauby commented Sep 15, 2015

Done 👍

@NullVoxPopuli
Copy link
Contributor

I'm good with this. 👍

end

def test_toplevel_jsonapi_meta
with_adapter :json_api do
Copy link
Member

Choose a reason for hiding this comment

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

no need for with_adapter. Just pass in adapter: :json_api

@bf4
Copy link
Member

bf4 commented Sep 16, 2015

@@ -8,6 +8,8 @@ module Configuration
base.config.array_serializer = ActiveModel::Serializer::ArraySerializer
base.config.adapter = :flatten_json
base.config.jsonapi_resource_type = :plural
base.config.jsonapi_toplevel_member = false
base.config.jsonapi_version = '1.0'
Copy link
Member

Choose a reason for hiding this comment

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

should be in the JsonApi adapter IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense.

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 16, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 16, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 16, 2015
@bf4
Copy link
Member

bf4 commented Sep 16, 2015

Alternative implementation at #1050 which was rebased to include this work

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 16, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 18, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 18, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 18, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 18, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 18, 2015
@NullVoxPopuli
Copy link
Contributor

needs rebase :-(

@beauby
Copy link
Contributor Author

beauby commented Sep 21, 2015

Closing in favor of #1050.

@beauby beauby closed this Sep 21, 2015
bf4 added a commit to beauby/active_model_serializers that referenced this pull request Sep 21, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 21, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 21, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 21, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 21, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 21, 2015
bf4 added a commit to bf4/active_model_serializers that referenced this pull request Sep 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants