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

[WIP] Add parsing for jsonapi. #25050

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@beauby

beauby commented May 17, 2016

Summary

  • Add the :jsonapi MIME type (application/vnd.api+json).
  • Add a default parser for the :jsonapi MIME type using the jsonapi gem.

The following JSON API payload:

{ 
  "data": { 
    "type": "posts", 
    "attributes" : {
      "title": "Hello",
      "date": "today"
    }, 
    "relationships": { 
      "author": { "data": { "id": "2", "type": "users" } }, 
      "comments": { 
        "data": [
          { "type": "comments", "id": "3" }, 
          { "type": "comments", "id": "4" }
        ]
      }, 
      "journal": { "data": null }
    }
  }
}

will be parsed into:

{ 
  "_type" => "posts", 
  "title" => "Hello", 
  "date" => "today",
  "author_id" => "2",
  "author_type" => "User",
  "comment_ids" => ["3", "4"],
  "comment_types" => ["Comment", "Comment"],
  "journal_id" => nil 
}

Other Information

Related to #23712.
Based on @bf4's blog post.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot May 17, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rails-bot commented May 17, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

" }, \"journal\": { \"data\": null } } } }"
expected = { "_type" => "posts", "title" => "Hello", "date" => "today",
"author_id" => "2", "author_type" => "User",
"comment_ids" => ["3", "4"], "comment_types" => ["Comment", "Comment"],

This comment has been minimized.

@bf4

bf4 May 17, 2016

Contributor

Is this compatible with activemodel/lib/active_model/serializers/json.rb ?

@bf4

bf4 May 17, 2016

Contributor

Is this compatible with activemodel/lib/active_model/serializers/json.rb ?

This comment has been minimized.

@beauby

beauby May 17, 2016

If you are referring to ActiveModel::Serializers#from_json then no, the format is not totally compatible (the associations are referred by id and type, rather than being nested – the attributes are in a compatible format). It is, however, compatible with ActiveRecord#new.
The format outlined in https://github.com/rails/rails/blob/master/activemodel/lib/active_model/serializers/json.rb#L137 seems a bit unclear about polymorphic relationships. It may make sense to add a ActiveModel::Serializers#from_jsonapi method.

@beauby

beauby May 17, 2016

If you are referring to ActiveModel::Serializers#from_json then no, the format is not totally compatible (the associations are referred by id and type, rather than being nested – the attributes are in a compatible format). It is, however, compatible with ActiveRecord#new.
The format outlined in https://github.com/rails/rails/blob/master/activemodel/lib/active_model/serializers/json.rb#L137 seems a bit unclear about polymorphic relationships. It may make sense to add a ActiveModel::Serializers#from_jsonapi method.

This comment has been minimized.

@bf4

bf4 May 17, 2016

Contributor

Interesting, since

def serializable_hash(options = nil)
is should be basically the same. I wonder how used these APIs are

@bf4

bf4 May 17, 2016

Contributor

Interesting, since

def serializable_hash(options = nil)
is should be basically the same. I wonder how used these APIs are

This comment has been minimized.

@beauby

beauby May 17, 2016

ActiveRecord#new has a different API though. You can feed it a hash with a bunch of attributes (including "relationship attributes", like author_id for belongs_to and comment_ids for has_many).

@beauby

beauby May 17, 2016

ActiveRecord#new has a different API though. You can feed it a hash with a bunch of attributes (including "relationship attributes", like author_id for belongs_to and comment_ids for has_many).

"author_id" => "2", "author_type" => "User",
"comment_ids" => ["3", "4"], "comment_types" => ["Comment", "Comment"],
"journal_id" => nil }
assert_parses(expected, payload, { 'CONTENT_TYPE' => 'application/vnd.api+json' })

This comment has been minimized.

@bf4

bf4 May 17, 2016

Contributor

If I recall, the content_type only affects the params parsing. the Accept header is used to choose the Renderer. So, this implementation, to be complete, still requires a JSON API renderer, right? (Which can be added in a subsequent PR, of course)

header-related files I've found

  • actionpack/lib/action_dispatch/http/mime_negotiation.rb
  • actionpack/lib/action_dispatch/http/mime_type.rb
  • actionpack/lib/action_dispatch/http/request.rb
  • actionpack/lib/action_dispatch/middleware/public_exceptions.rb
  • actionpack/lib/action_dispatch/middleware/show_exceptions.rb
@bf4

bf4 May 17, 2016

Contributor

If I recall, the content_type only affects the params parsing. the Accept header is used to choose the Renderer. So, this implementation, to be complete, still requires a JSON API renderer, right? (Which can be added in a subsequent PR, of course)

header-related files I've found

  • actionpack/lib/action_dispatch/http/mime_negotiation.rb
  • actionpack/lib/action_dispatch/http/mime_type.rb
  • actionpack/lib/action_dispatch/http/request.rb
  • actionpack/lib/action_dispatch/middleware/public_exceptions.rb
  • actionpack/lib/action_dispatch/middleware/show_exceptions.rb

This comment has been minimized.

@beauby

beauby May 17, 2016

You are absolutely right. This PR only handles the incoming payloads.

@beauby

beauby May 17, 2016

You are absolutely right. This PR only handles the incoming payloads.

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4
Contributor

bf4 commented May 17, 2016

@beauby

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/parameters.rb Outdated
"{\"person\": {\"name\": \"David\"}}", { 'CONTENT_TYPE' => 'application/vnd.api+json' }
)
end

This comment has been minimized.

@bf4

bf4 May 17, 2016

Contributor

IMO this should be fixed rather than removed

@bf4

bf4 May 17, 2016

Contributor

IMO this should be fixed rather than removed

This comment has been minimized.

@beauby

beauby May 18, 2016

Well the point of this PR is to actually register the application/vnd.api+json media type, so I'm not sure this test is consistent with this PR.

@beauby

beauby May 18, 2016

Well the point of this PR is to actually register the application/vnd.api+json media type, so I'm not sure this test is consistent with this PR.

@@ -0,0 +1 @@
require 'active_support/jsonapi/decoding'

This comment has been minimized.

@bf4

bf4 May 17, 2016

Contributor

stub encoding?

@bf4

bf4 May 17, 2016

Contributor

stub encoding?

This comment has been minimized.

@beauby

beauby May 19, 2016

Encoding is a bit complicated in this context because it would need assumptions on the object (for instance it has an id), and the case of object attributes would be unclear (should they appear as relationships or attributes within the JSON API document?).

@beauby

beauby May 19, 2016

Encoding is a bit complicated in this context because it would need assumptions on the object (for instance it has an id), and the case of object attributes would be unclear (should they appear as relationships or attributes within the JSON API document?).

This comment has been minimized.

@bf4

bf4 May 19, 2016

Contributor

agreed. just wondering what it means for it to be missing, and if there should maybe be a no-op version to assist in deciding if this is the right api

@bf4

bf4 May 19, 2016

Contributor

agreed. just wondering what it means for it to be missing, and if there should maybe be a no-op version to assist in deciding if this is the right api

@joshsmith

This comment has been minimized.

Show comment
Hide comment
@joshsmith

joshsmith May 20, 2016

For what it's worth, I would very much like to see this PR merged. Consuming JSON API payloads is probably the single most painful point of my development process right now with my API.

joshsmith commented May 20, 2016

For what it's worth, I would very much like to see this PR merged. Consuming JSON API payloads is probably the single most painful point of my development process right now with my API.

@beauby

This comment has been minimized.

Show comment
Hide comment
@beauby

beauby May 23, 2016

  • This test fails randomly in my VM, but only when running the whole test suite for actionpack. Running the test alone seems to succeed consistently.
  • After investigation, the parameters parser is not even called when the test fails.
  • Changing the MIME type name from application/vnd.api+json to application/vndapijson seems to fix the issue. I suspect a bug in the parsing of the Content-Type header or in the lookup of the MIME type.
  • Upon failure, the request.content_type is set to #<Mime::Type:0x007fb9f3300250 @synonyms=[], @symbol=nil, @string="application/vnd.api+json", @hash=1061287254102306067>, whereas it is set to #<Mime::Type:0x007fdbfda2aea0 @synonyms=[], @symbol=:jsonapi, @string="application/vnd.api+json", @hash=2205739525733621489> upon success (note the @symbol=nil in case of failure).
  • Upon failure, the results of Mime::Type.lookup('application/vnd.api+json') and Mime[:jsonapi] (which is really Mime::Type.lookup_by_extension(:jsonapi)) are different. The former has @symbol=nil.
    They should be the same, as Mime::Type.register "application/vnd.api+json", :jsonapi does LOOKUP[string] = new_mime and EXTENSION_LOOKUP[symbol.to_s] = new_mime (where string = 'application/vnd.api+json' and symbol = :jsonapi).
  • Conclusion: Tests such as this one mess up the MIME lookup for application/vnd.api+json. Which explains the non-deterministic failures. Yay.

beauby commented May 23, 2016

  • This test fails randomly in my VM, but only when running the whole test suite for actionpack. Running the test alone seems to succeed consistently.
  • After investigation, the parameters parser is not even called when the test fails.
  • Changing the MIME type name from application/vnd.api+json to application/vndapijson seems to fix the issue. I suspect a bug in the parsing of the Content-Type header or in the lookup of the MIME type.
  • Upon failure, the request.content_type is set to #<Mime::Type:0x007fb9f3300250 @synonyms=[], @symbol=nil, @string="application/vnd.api+json", @hash=1061287254102306067>, whereas it is set to #<Mime::Type:0x007fdbfda2aea0 @synonyms=[], @symbol=:jsonapi, @string="application/vnd.api+json", @hash=2205739525733621489> upon success (note the @symbol=nil in case of failure).
  • Upon failure, the results of Mime::Type.lookup('application/vnd.api+json') and Mime[:jsonapi] (which is really Mime::Type.lookup_by_extension(:jsonapi)) are different. The former has @symbol=nil.
    They should be the same, as Mime::Type.register "application/vnd.api+json", :jsonapi does LOOKUP[string] = new_mime and EXTENSION_LOOKUP[symbol.to_s] = new_mime (where string = 'application/vnd.api+json' and symbol = :jsonapi).
  • Conclusion: Tests such as this one mess up the MIME lookup for application/vnd.api+json. Which explains the non-deterministic failures. Yay.

bf4 added a commit to bf4/rails that referenced this pull request May 23, 2016

Modifies mime-registration test not to interfere with real mime types
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.
@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 May 23, 2016

Contributor

Fix to the tests made in #25123

Contributor

bf4 commented May 23, 2016

Fix to the tests made in #25123

@beauby

This comment has been minimized.

Show comment
Hide comment
@beauby

beauby May 23, 2016

Awesome, last remaining failing test for this PR will be easily fixed 👍.

beauby commented May 23, 2016

Awesome, last remaining failing test for this PR will be easily fixed 👍.

@beauby

This comment has been minimized.

Show comment
Hide comment
@beauby

beauby May 23, 2016

The last failing test is not due to this PR. See this comment.

beauby commented May 23, 2016

The last failing test is not due to this PR. See this comment.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb May 24, 2016

Contributor

I should say that, as one of the authors of the JSON API spec, I'm excited to see adoption of the spec within Rails libraries such as AMS and JSONAPI::Resources.

However, I don't think that providing a generic parsing approach is appropriate within Rails itself. First of all, the format that is output here is a lossy representation of the spec that does not capture the full possibilities for data. Secondly, the output format imposes its own strong opinions on what's useful to extract from a payload. Last but not least, deserialization is only one side of the coin - an inverse approach to serialization is also important.

I would much prefer to collaborate on a non-lossy serialization / deserialization approach that can be easily registered and used within Rails projects.

And I really would like to collaborate - @beauby, @lgebhardt, @bf4 - I think we should talk to see if we can reach a consensus.

Contributor

dgeb commented May 24, 2016

I should say that, as one of the authors of the JSON API spec, I'm excited to see adoption of the spec within Rails libraries such as AMS and JSONAPI::Resources.

However, I don't think that providing a generic parsing approach is appropriate within Rails itself. First of all, the format that is output here is a lossy representation of the spec that does not capture the full possibilities for data. Secondly, the output format imposes its own strong opinions on what's useful to extract from a payload. Last but not least, deserialization is only one side of the coin - an inverse approach to serialization is also important.

I would much prefer to collaborate on a non-lossy serialization / deserialization approach that can be easily registered and used within Rails projects.

And I really would like to collaborate - @beauby, @lgebhardt, @bf4 - I think we should talk to see if we can reach a consensus.

@beauby

This comment has been minimized.

Show comment
Hide comment
@beauby

beauby May 24, 2016

@dgeb This was more intended as a WIP/proof of concept rather than a proposition for a definitive deserialization format. From discussions and personal use, it appeared to me that deserialization was a major pain point in the community of JSON API implementers. The idea here was to lay the ground for direct consuming by ActiveRecord.

All up for collaborating as well – I'm often up on the AMS slack.

beauby commented May 24, 2016

@dgeb This was more intended as a WIP/proof of concept rather than a proposition for a definitive deserialization format. From discussions and personal use, it appeared to me that deserialization was a major pain point in the community of JSON API implementers. The idea here was to lay the ground for direct consuming by ActiveRecord.

All up for collaborating as well – I'm often up on the AMS slack.

@beauby beauby changed the title from Add parsing for jsonapi. to [WIP] Add parsing for jsonapi. May 24, 2016

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb May 24, 2016

Contributor

@beauby ok great - let's sync up in the AMS slack soon 👍

Contributor

dgeb commented May 24, 2016

@beauby ok great - let's sync up in the AMS slack soon 👍

@beauby beauby closed this Mar 19, 2017

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