The JSON API media type should only work with a JSON API handler #23712

bf4 · 2016-02-16T16:25:35Z

Revises behavior introduced in #20329 #21251

Since the media type 'application/vnd.api+json' is a spec,
it is inappropriate to handle it with the JSON renderer.

Updated Description per discussion

Let's remove it and introduce back only when we have defined the proper API for the handlers.

This PR removes support for the JSON API media type.

We recommend the media type be registered on its own as jsonapi when a jsonapi Renderer and deserializer (Http::Parameters::DEFAULT_PARSERS) are added.

Original Description

This PR adds support for a JSON API media type.

It does not currently include a JSON API renderer, but I could add that
here.
The JSON being parsed in the tests is invalid JSON API, but I
retained the example for test continuity. I'd like to change that as
well.

I've chosen to register JSON API as api_json per
JSONAPI-resources,
one of the more mature Rails+JSON API gems out there. An alternate name
could be jsonapi. In both cases, I would recommend the renderer be
named after the registered mime type.

Is related to work in #21496

cc @dgeb

rails-bot · 2016-02-16T16:25:38Z

r? @senny

(@rails-bot has picked a reviewer for you, use r? to override)

dgeb · 2016-02-16T19:34:05Z

Thanks for this, @bf4. It's a step in the right direction.

Since the media type 'application/vnd.api+json' is a spec, it is inappropriate to handle it with the JSON renderer.

I agree. To go further, I don't think the mime type should be registered at all without an appropriate corresponding renderer.

I've chosen to register JSON API as api_json per JSONAPI-resources, one of the more mature Rails+JSON API gems out there. An alternate name could be jsonapi.

We are actually looking to switch to jsonapi in our gem. The current name is based directly on the media type and feels a bit clunky as an alias.

I've been talking with @lgebhardt about proposing a more pluggable rendering solution for mime types, that could cover the base mime types as well as custom registrations. We'll review with @bf4 and others here soon.

lgebhardt · 2016-02-16T19:58:41Z

If we're looking for pluggable renderers it might be best to remove the json-api mime type and DEFAULT_PARSERS entries. These could be registered as part of the specific library supporting json-api.

bf4 · 2016-02-16T23:44:43Z

Perhaps the pr should remove the jsonapi code and maybe update the guides with how to add to custom json media type

B mobile phone

On Feb 16, 2016, at 2:00 PM, Larry Gebhardt notifications@github.com wrote:

If we're looking for pluggable renderers it might be best to remove the json-api mime type and DEFAULT_PARSERS entries. These could be registered as part of the specific library supporting json-api.

—
Reply to this email directly or view it on GitHub.

rafaelfranca · 2016-02-17T02:12:47Z

Perhaps the pr should remove the jsonapi code and maybe update the guides with how to add to custom json media type

👍. Let's remove it and introduce back only when we have defined the proper API for the handlers.

dgeb · 2016-02-17T02:31:03Z

Let's remove it and introduce back only when we have defined the proper API for the handlers.

sounds good to me 👍

Since the media type 'application/vnd.api+json' is a spec, it is inappropriate to handle it with the JSON renderer. This PR removes support for a JSON API media type. I would recommend the media type be registered on its own as `jsonapi` when a jsonapi Renderer and deserializer (Http::Parameters::DEFAULT_PARSERS) are added. Is related to work in rails#21496

bf4 · 2016-02-17T03:49:26Z

actionpack/test/dispatch/request/json_params_parsing_test.rb

    assert_parses(
-      {"person" => {"name" => "David"}},
+      {},


I kept this test around both in case anyone is looking for json api support, and because I didn't see the behavior tested elsewhere.

However, I removed the test for rootless JSON as it duplicates what I left here and also isn't relevant to JSON API.

Yeah. Make sense 👍

ha, that was quick! you merged it right before I revised the commit message from 'I' to 'we' and fixed a typo. :) Thanks!

…ot_render_spec The JSON API media type should only work wih a JSON API handler

bf4 · 2016-02-17T04:00:37Z

actionpack/test/dispatch/request/json_params_parsing_test.rb

@@ -37,9 +37,9 @@ def teardown
    )
  end

-  test "parses json params for application/vnd.api+json" do
+  test "does not parses unregistered media types such as application/vnd.api+json" do


@rafaelfranca typo does not parses should be does not parse. Just leave it for someone else?

I can change it quickly, but if you want to make a PR go ahead.

I think it's cleaner if you just fix it, right?

ianks · 2016-02-17T05:23:59Z

@bf4 Can we add some documentation about how to register the application/vnd.api+json, last time I tried in Rails 5 the MIME type registration did not work as it did in previous versions of Rails.

bf4 · 2016-02-17T05:50:00Z

@ianks I spiked a first step in 6f73b19#diff-6a80fa5836403509e86a81c2117af391R12 or did you mean in the guides?

ianks · 2016-02-17T05:53:09Z

Nevermind my comment :)

rthbound · 2016-02-18T16:58:15Z

I've been developing on a greenfield API project using edge, and have found that the commit in this PR has broken application/vnd.api+json params parsing. Bisecting using 5.0.0.beta2 as good and the head of master as bad, confirmed c4d90b7 was the breaking change.

initializers:

# active_model_initializer.rb
ActiveModelSerializers.config.adapter = :json_api

# wrap_parameters.rb
ActiveSupport.on_load(:action_controller) do
  wrap_parameters format: [:json, :api_json]
end

Processing by DashboardsController#update as */*
  Parameters: {"{\"data\":{\"id\":\"1\",\"attributes\": ... ... ... ... ...

I'm not sure (@bf4, @rafaelfranca), should this be broken right now?... is it pending some development, or should I submit an app or tests for reproduction for the failure to build a good parameters object?

rthbound · 2016-02-18T17:26:46Z

I went ahead and generated an app for demoing the problem: Repo is here

/cc @bf4, @rafaelfranca

rafaelfranca · 2016-02-18T17:41:34Z

Yes it is a "breaking change" by design. That feature was never released officially do we just removed it. If you need it in your application you can register the mime type.

rthbound · 2016-02-18T18:01:04Z

@rafaelfranca I attempted that last night to no avail. Confirmed in the demo app that it still does not work, even having registered the mime type: link to change

rafaelfranca · 2016-02-18T18:02:48Z

As you can see in c4d90b7 the only thing made was to remove application/vnd.api+json from the registered mime type.

rthbound · 2016-02-18T18:47:50Z

@rafaelfranca something must be awry... On rails master, the following does not work as expected:

# custom_mime_type_initializer.rb
Mime::Type.unregister :json
Mime::Type.register "application/json", :json, %w( text/x-json application/jsonrequest application/vnd.api+json )

# Confirmed successful re-registration of :json mime type:
Mime::Type.lookup_by_extension("json")
=> #<Mime::Type:0x000000033173f8 @hash=4269350542389124482, @string="application/json", @symbol=:json, @synonyms=["text/x-json", "application/jsonrequest", "application/vnd.api+json"]>

Reverting c4d90b7 resolves the issue (assuming I also remove the above initializer). I appreciate your time, and I'll continue looking into this. Thanks again @rafaelfranca

rafaelfranca · 2016-02-18T18:52:19Z

Could you open a new issue with a way to reproduce it?

bf4 · 2016-02-23T14:02:19Z

@rthbound would you mind opening an issue in ams, as well, so we can add a test there? Also to confirm docs are up to date.

rthbound · 2016-02-23T16:26:17Z

@bf4 : Here it is, #1534

The tests introduced in https://github.com/rails/rails/pull/23816/files#diff-384a5a15d8d53de799fb6541688ea5f9R153 register the JSON API media type `application/vnd.api+json` with `Mime[:json]`. The JSON API media type should not be registered with `Mime[:json]`, as discussed in rails#23712. Moreover, since the actual mime type used in the test is incidental, I've changed this to a valid, but fictional `applcation/vnd.rails+json`. These tests were causing failures in rails#25050 (comment) where `Mime[:jsonapi]` is being added, so that JSON API request params are parsed with the JSONAPI gem.

We will wait until 5.1 to make a decision

rails-bot assigned senny Feb 16, 2016

maclover7 added the actionpack label Feb 16, 2016

bf4 force-pushed the incorrect_to_accept_json_api_and_not_render_spec branch from 6f73b19 to c4d90b7 Compare February 17, 2016 03:44

bf4 reviewed Feb 17, 2016
View reviewed changes

rafaelfranca added a commit that referenced this pull request Feb 17, 2016

Merge pull request #23712 from bf4/incorrect_to_accept_json_api_and_n…

2572584

…ot_render_spec The JSON API media type should only work wih a JSON API handler

rafaelfranca merged commit 2572584 into rails:master Feb 17, 2016

bf4 reviewed Feb 17, 2016
View reviewed changes

rthbound added a commit to rthbound/ams-json-api-broken-23712 that referenced this pull request Feb 18, 2016

Demonstrated problem noted here: rails/rails#23712 (comment)

aec05e0

This was referenced Feb 24, 2016

Add tests around application/vnd.api+json MIME type, review/update documentation rails-api/active_model_serializers#1534

Closed

Deserializing Empty HasMany Relationship Array Params in JSONAPI Adapter rails-api/active_model_serializers#1532

Closed

bf4 changed the title ~~The JSON API media type should only work wih a JSON API handler~~ The JSON API media type should only work with a JSON API handler Mar 4, 2016

bf4 mentioned this pull request Mar 4, 2016

Add configurable serializers in each renderer #21496

Closed

14 tasks

beauby mentioned this pull request May 17, 2016

[WIP] Add parsing for jsonapi. #25050

Closed

bf4 mentioned this pull request May 23, 2016

Modifies mime-registration test not to interfere with real mime types #25123

Merged

bf4 mentioned this pull request Jun 27, 2016

update integration documentation for ember and JSONAPI to include… rails-api/active_model_serializers#1802

Merged

bf4 mentioned this pull request Aug 29, 2016

ActionDispatch does not understand :jsonapi mime type #26309

Open

bf4 referenced this pull request Aug 31, 2017

Do not default to any Serializer

f8edd20

We will wait until 5.1 to make a decision

MGatner mentioned this pull request Mar 3, 2022

Mime Support for "problem detail" #44608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The JSON API media type should only work with a JSON API handler #23712

The JSON API media type should only work with a JSON API handler #23712

bf4 commented Feb 16, 2016

rails-bot commented Feb 16, 2016

dgeb commented Feb 16, 2016

lgebhardt commented Feb 16, 2016

bf4 commented Feb 16, 2016

rafaelfranca commented Feb 17, 2016

dgeb commented Feb 17, 2016

bf4 Feb 17, 2016

rafaelfranca Feb 17, 2016

bf4 Feb 17, 2016

bf4 Feb 17, 2016

rafaelfranca Feb 17, 2016

bf4 Feb 17, 2016

rafaelfranca Feb 17, 2016

ianks commented Feb 17, 2016

bf4 commented Feb 17, 2016

ianks commented Feb 17, 2016

rthbound commented Feb 18, 2016

rthbound commented Feb 18, 2016

rafaelfranca commented Feb 18, 2016

rthbound commented Feb 18, 2016

rafaelfranca commented Feb 18, 2016

rthbound commented Feb 18, 2016

rafaelfranca commented Feb 18, 2016

bf4 commented Feb 23, 2016

rthbound commented Feb 23, 2016

The JSON API media type should only work with a JSON API handler #23712

The JSON API media type should only work with a JSON API handler #23712

Conversation

bf4 commented Feb 16, 2016

Updated Description per discussion

Original Description

rails-bot commented Feb 16, 2016

dgeb commented Feb 16, 2016

lgebhardt commented Feb 16, 2016

bf4 commented Feb 16, 2016

rafaelfranca commented Feb 17, 2016

dgeb commented Feb 17, 2016

bf4 Feb 17, 2016

Choose a reason for hiding this comment

rafaelfranca Feb 17, 2016

Choose a reason for hiding this comment

bf4 Feb 17, 2016

Choose a reason for hiding this comment

bf4 Feb 17, 2016

Choose a reason for hiding this comment

rafaelfranca Feb 17, 2016

Choose a reason for hiding this comment

bf4 Feb 17, 2016

Choose a reason for hiding this comment

rafaelfranca Feb 17, 2016

Choose a reason for hiding this comment

ianks commented Feb 17, 2016

bf4 commented Feb 17, 2016

ianks commented Feb 17, 2016

rthbound commented Feb 18, 2016

rthbound commented Feb 18, 2016

rafaelfranca commented Feb 18, 2016

rthbound commented Feb 18, 2016

rafaelfranca commented Feb 18, 2016

rthbound commented Feb 18, 2016

rafaelfranca commented Feb 18, 2016

bf4 commented Feb 23, 2016

rthbound commented Feb 23, 2016