Utilize JSONAPI family of gems for deserialization. #1928
Conversation
|
@NullVoxPopuli, thanks for your PR! By analyzing the annotation information on this pull request, we identified @domitian, @beauby and @lawitschka to be potential reviewers |
|
thanks, @mention-bot ... |
| @@ -43,6 +43,7 @@ Gem::Specification.new do |spec| | |||
| # 'thread_safe' | |||
|
|
|||
| spec.add_runtime_dependency 'jsonapi', '~> 0.1.1.beta2' | |||
| spec.add_runtime_dependency 'json_key_transform', '>= 0.1' | |||
There was a problem hiding this comment.
not sure on the name of this?
There was a problem hiding this comment.
it's since been renamed to case_transform :-)
| @@ -1,12 +1,12 @@ | |||
| require 'jsonapi' | |||
|
|
|||
| module ActiveModelSerializers | |||
| module Adapter | |||
| class JsonApi | |||
| # NOTE(Experimental): | |||
| # This is an experimental feature. Both the interface and internals could be subject | |||
| # to changes. | |||
| module Deserialization | |||
There was a problem hiding this comment.
At the end of this PR, this should be removed / deprecated
/cc @richmolj - rumor has it you want to tackle moving deserialization out?
|
nil check PR pending beauby/jsonapi#37 |
|
Some early benchmark comparisons: With rustified case_transform gem (so for just the string inflectors have been rustified. The whole case_transform module should probably be written in rust) I think this is proof enough that all of case_transform needs to be rustified, because there is probably a lot of overhead going back and forth between rust and ruby objects every iteration. Overall, it's a performance loss. :-( Today (pure ruby) For reference for myself, both of these bin/bench runs were on my ASUS laptop. |
|
@NullVoxPopuli this one seems a little off to me: That's quite the decrease from 495 :) And not in-line with the rest of the benches. Maybe something is off with the script? |
|
yeah, it's super disappointing. There are two things for me to look at:
|
|
@richmolj for what it's worth, the current implementation https://github.com/NullVoxPopuli/case_transform/blob/ruru-rust-speed-boost/lib/case_transform.rb#L45 It goes from Ruby -> Rust -> Ruby -> Rust -> Ruby. Just to do And this is the weird loading thing I was talking about: https://github.com/NullVoxPopuli/case_transform/blob/ruru-rust-speed-boost/lib/case_transform.rb#L12 |
|
Latest bin/bench re: key transform: |
|
This is a little easier to digest |
|
dashed is at least much faster now |
|
and again with AMS: |
|
Pure Ruby case_transform: |
|
Btw, the code for deserialization in the |
|
yup, I discovered that while investigating, and have been changing the tests to expect the exception thrown from your gem |
|
So, all that's remaining here is to extract the jsonapi -> rails json format stuff this could be merged as is, and just represent the extraction of a couple other things. |
NullVoxPopuli
force-pushed
the
1927-change-deserialization-to-json-api-gem
branch
from
711abc6 to
502b691
Compare
| @@ -1,12 +1,12 @@ | |||
| require 'jsonapi' | |||
There was a problem hiding this comment.
Isn't this gem already required elsewhere? I know we use it for the include directives...
There was a problem hiding this comment.
i'll take a look
There was a problem hiding this comment.
this is the first place it's used.
| { 'data' => { 'attributes' => [] } }, | ||
| { 'data' => { 'relationships' => [] } }, | ||
| { 'data' => { 'relationships' => { 'rel' => nil } } }, | ||
| { 'data' => { 'relationships' => { 'rel' => {} } } }] |
| @@ -76,9 +65,14 @@ def test_illformed_payloads_safe | |||
| end | |||
| end | |||
|
|
|||
| test 'null data is allow per the JSON API Schema' do | |||
There was a problem hiding this comment.
Just curious, should we also be adding a test for []?
| @@ -43,6 +43,7 @@ Gem::Specification.new do |spec| | |||
| # 'thread_safe' | |||
|
|
|||
| spec.add_runtime_dependency 'jsonapi', '~> 0.1.1.beta2' | |||
| spec.add_runtime_dependency 'case_transform', '>= 0.2' | |||
There was a problem hiding this comment.
I always prefer ~> here or at least < 1. Who knows if you'll give up on AMS and break something with a 1.0 release some day :)
| @@ -7,6 +7,9 @@ eval_gemfile local_gemfile if File.readable?(local_gemfile) | |||
| # Specify your gem's dependencies in active_model_serializers.gemspec | |||
| gemspec | |||
|
|
|||
| # TODO: remove when @beauby re-publishes this gem | |||
| gem 'jsonapi', github: 'beauby/jsonapi' | |||
There was a problem hiding this comment.
I think this would prevent a merge, right? Or at the very least we'd have to tell anyone running off of master (like me) to add a similar entry to their app's Gemfile.
There was a problem hiding this comment.
yup. gotta see if beauby cut a new gem with his recent merges
|
@NullVoxPopuli I know this came up in slack at some point - does this bring in additional dependencies due to rust, or does installation stay the same? |
|
Also heads up it looks like the parsing logic may be changing beauby/jsonapi#43 |
|
the case transform gem is pure ruby. i'll probably make a doc later with performance improvement tips, and mention the rust-extensions version. i've gotten the rust build to be effortless. (just gem install), but it's still opt-in. |
| return {} | ||
| end | ||
| document = JSONAPI.parse(document, options) | ||
| document = document.to_hash |
There was a problem hiding this comment.
The API of jsonapi-parser is about to change. You probably want to do JSONAPI.parse_resource!(document) now (which does only the validation).
There was a problem hiding this comment.
what specifically is changing / could AMS utilize any of those changes for the better?
There was a problem hiding this comment.
The parser now only validates a document, it does not build an object-like structure for traversing it anymore (which was a perf issue). AMS could possibly make use of jsonapi-validator which does domain-specific validations of payloads, or jsonapi-deserializable which does general-purpose deserialization of JSONAPI resource/relationship payloads.
There was a problem hiding this comment.
I think the validator could be interesting as long as it was quick. In my project, I had to disable validation, cause I am doing embedded records in relationships -- so.. we'll want to document how to override this for some people.
I was looking at jsonapi-deserializable the other day. It errored, so I didn't add it to this PR.
There was a problem hiding this comment.
The validator is as quick as it could possibly be (given it is written in ruby, but then again so is rails so I doubt it is a bottleneck).
Would you mind opening an issue on jsonapi-deserializable?
There was a problem hiding this comment.
Note that jsonapi-deserializable is not expected to work right now, as it relies on an unpublished gem (jsonapi-validator), which in turn relies on an unpublished version of jsonapi-parser. All this should be fixed tonight though.
|
So, I'll just wait till it's all published before pokin' around :-) On Sun, Oct 2, 2016 at 12:57 PM Lucas Hosseini [email protected]
|
|
Just released jsonapi v0.1.1.beta3 (containing jsonapi-parser v0.1.1.beta1). |
|
@beauby this is what I was talking about earlier: where parse is def parse!(document, options = {})
parse(document, options) do |invalid_payload, reason|
fail JSONAPI::Parser::InvalidDocument, "Invalid payload (#{reason}): #{invalid_payload}"
end
end
def parse(document, options = {})
JSONAPI.parse_response!(document)
document = document.to_h
puts document
return JSONAPI::Deserializable::Resource.new(document)
rescue JSONAPI::Parser::InvalidDocument => e
puts e.message
return {} unless block_given?
yield
end |
|
ok, that's fair. So, for the deserialization implementation in AMS, should a parameter be passed for differentiate between create / update / etc? since different validations exist for the different scenarios? |
|
The only difference between create and update resources is whether the |
|
ok, cool. and does deserializable have a way to split apart polymorphic relationships, like how AMS currently does? |
|
Sure, basically how it works is:
|
|
I'm making some good progress on this beauby/jsonapi-rails#1 some things I wanted to ask clarification on: the
They have the ability to infer a relationship (author means both author_id and author_type). I remember there being talk of using only |
NullVoxPopuli
force-pushed
the
1927-change-deserialization-to-json-api-gem
branch
from
db20605 to
f3332f5
Compare
|
@beauby I think all that's left for this is to publish merge by jsonapi-rails branch and cut a release. |
extract key_transfor chaneg to case_transform gem fix tests pass add empty array test upgrade to jsonapi beta5, improve tests to be valid resource requests use github links in gemfile for now, due to unreleased code use github links in gemfile for now, due to unreleased code some clenaup deserialization would work now if it didn't depend on the existance of certain objects progress on implementing new deserializer -- just have some behavior differences with relationships tests pass address rubocop issue -- update dependencies... wonder if we need to move @beauby's jsonapi gems to rails-api, in order to faster turnaround time reduce to minimum dependencies. Now we need beauby to merge my jsonapi-rails branch :-)
NullVoxPopuli
force-pushed
the
1927-change-deserialization-to-json-api-gem
branch
from
871ad86 to
10b4850
Compare
|
This should also close this PR: #1986 |
NullVoxPopuli
changed the title
|
Ready for re-review. |
|
I don't know if you guys want the tests gone, since the functionality is handled by an external gem? |
|
@NullVoxPopuli can this still be done? |
|
I believe so :) |
|
@NullVoxPopuli probably best to just reset against current master then swap in jsonapi-deserializable and update code/docs/tests :) |
|
I tried https://github.com/bf4/active_model_serializers/tree/replace_jsonapi_deserialization but I don't think it's worth it bf4@59f8627 |
|
Why's that? At the very least, it removes the need of us to maintain a
jsonapi validater
…On Mon, May 1, 2017, 3:20 AM Benjamin Fleischer ***@***.***> wrote:
I tried
https://github.com/bf4/active_model_serializers/tree/replace_jsonapi_deserialization
but I don't think it's worth it ***@***.***
<bf4@59f8627>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1928 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMJakvCeSIHhlofS4eICuo_z3F6TuX-ks5r1YfNgaJpZM4J-KPp>
.
|
|
Cleaning up my PRs. If this work is still desired, feel free to cherry-pick |
Purpose
To further break apart AMS into functional sections that can be easily swapped with native extension gems to allow for much faster serialization/deserialization.
This PR cleans up a lot of Deserialization functionality, and offloads it to jsonapi-deserialization.
Changes
Caveats
Related GitHub issues
#1927 - use JSON API gem for deserialization
#1925 - data can be an array
Additional helpful information
/cc @remear @richmolj @beauby