Basic deserialization. #1248
Basic deserialization. #1248
Conversation
| def self.parse(document) | ||
| hash = {} | ||
|
|
||
| hash[:id] = document['data']['id'] if document['data']['id'] |
There was a problem hiding this comment.
should there be a check for the existence of 'data'? could be handy to have detailed parse errors
There was a problem hiding this comment.
Sure. C.f. the description of the PR, there is currently no error handling, but it is planned.
There was a problem hiding this comment.
There was a problem hiding this comment.
Rails' from_json and this method do different stuff. Here we are not building an instance of a model, we are just translating a json payload into a hash that can be passed to an AR model's constructor.
beauby
force-pushed
the
jsonapi-parse
branch
2 times, most recently
from
3d18d7e
to
42addfb
Compare
|
Updated with handling of ill-formed documents, whitelisting, and aliased relationships. |
| # specifying the attribute name on the model. | ||
| # @return [Hash] ActiveRecord-ready hash | ||
| # | ||
| def parse(document, options = {}) |
There was a problem hiding this comment.
The issue with from_json is that it should be called on a model. Here, we make no assumption about the model. I guess we could override ActiveModel::Serializers::JSON so that it calls the parse method and updates the model. But it would be JSON API specific. Thoughts?
There was a problem hiding this comment.
There was a problem hiding this comment.
My question still holds though. As a consistent interface of what? to what?
There was a problem hiding this comment.
I think calling from_json on json-api data should generate a model / associations / whatever.
so, imo, from_json could call this method, parse and return an instance of a model.
|
Posting something like Is this on my end? |
|
@sachse Your payload looks ok. Could you give me your ruby version, rails version, platform and a full stacktrace? |
|
@beauby I'm running Ruby 2.2.3 and Rails 5 from master on OS X. The test suite passes without any problems. Full trace: |
|
I'm getting the same issue. I'm running Ruby 2.2.3, Rails 5 (from master), and AMS from your fork as of 10 minutes ago. With the following code:
I get this stack trace: |
|
Thanks @sachse. I believe ActionController::Parameters changed a bit in Rails5. I'll find a workaround tonight. |
|
Couldn't resist to see what's going on. Since this commit |
|
|
||
| def test_parameters | ||
| parameters = ActionController::Parameters.new(@hash) | ||
| parsed_hash = ActiveModel::Serializer::Adapter::JsonApi::Deserialization.parse(parameters) |
There was a problem hiding this comment.
One solution to fixing this would be to send parameters.to_unsafe_h instead of only parameters. Or to try to call that method before calling to_h inside the #parse method.
There was a problem hiding this comment.
@jmonteiro The purpose of this test is to ensure the parse method can handle ActionController::Parameters in order to reduce friction for the user.
There was a problem hiding this comment.
The idea is that deserialization should handle parameters transparently. The preferred way for whitelisting would be through the parse method, but in case a user wants to do whitelisting on the ActionController::Parameters instance, we should not override its will, that's why we're using to_h instead of to_unsafe_h inside parse.
There was a problem hiding this comment.
Oh, I may have made a false assumption here. I thought a pristine instance of ActionController::Parameters would have all its parameters whitelisted by default.
There was a problem hiding this comment.
Ok the catch here is that when initializing an instance of ActionController::Parameters like here, it will automatically whitelist all attributes, hence the test wouldn't catch the problem you're mentioning. However, the to_unsafe_h method was only added in rails 4.2, so I'm not sure what the best solution is here.
There was a problem hiding this comment.
For now, I'll remove direct support for ActionController::Parameters.
|
It works great. It should be merged! Thanks for your work. |
beauby
force-pushed
the
jsonapi-parse
branch
from
c0dd8d6
to
d5b3e25
Compare
|
Rebased. Any feedback on this? I believe it is pretty useful. |
|
Any updates since last discussion? Also, ref: #1098 |
|
my only feedback was for inline documentation, which I know @beauby despises... so... :-) |
|
but other than that, I think it should be merged -- just to get things moving. 👍 |
|
|
||
| def test_illformed_payload | ||
| parsed_hash = ActiveModel::Serializer::Adapter::JsonApi::Deserialization.parse({}) | ||
| assert_equal({}, parsed_hash) |
There was a problem hiding this comment.
I don't understand what this is testing. What would a failure look like? (also, malformed or invalid?)
There was a problem hiding this comment.
This test just makes sure AMS does not go crazy when the hash does not comply to the JSON API spec. I guess ideally there should be a bunch more tests for invalid/missing keys.
|
|
||
| document = document.dup.permit!.to_h if document.is_a?(ActionController::Parameters) | ||
|
|
||
| fail ArgumentError, 'Invalid payload' unless payload_valid?(document) |
There was a problem hiding this comment.
I'd rather not use the exception for flow control. I'd rather have all the code in parse and have it do, maybe
if payload_invalid?(document)
if block_given?
yield document
else
return {} # I guess this is supposed to be a benign value?
end
endand then
InvalidDocument = Class.new(ArgumentError)
def parse!(document, options = {})
parse(document, options) do |invalid_document)
fail InvalidDocument, "Invalid Document: #{reason}" # reason would be good, right?
end
endThere was a problem hiding this comment.
I am personally less comfortable with yielding stuff around for flow-control, but that's probably just me, so it's not really relevant. The way I thought of it: I defined an "unsafe" method (which would throw an exception when encountering an ill-formed document – I intended to make a custom exception with the reason and such, but haven't got there yet), and a "safe" version which would just catch the exception. I don't know whether this is common practice or specifically avoided. I'd have to have a look at how rails does stuff. In the meantime, would you mind elaborating on the reasons why using exceptions for (highly-local) flow-control does not suit you?
There was a problem hiding this comment.
oh, sorry... there's a number of reasons not to use exceptions for flow control, in no particular order
- they're slower than the alternative
- flow control isn't "exceptional"
- flow control isn't a good reason to possibly crash a program
- it's better to use throw/catch or blocks
- throw/catch for unwinding the stack
- blocks for allowing user-defined error-handling strategies
which to me I would summarize as "don't do it unless there's a really good reason"
If I had to choose over 'fail' vs. 'throw/catch', without any refactor, I'd pick throw catch, but really, I think the convention that parse! raises an exception is fine (if it is a good API for us), as long as the exceptional method doesn't raise an exception!
There was a problem hiding this comment.
I can dig up sources if you're curious. they're used in Nokogiri::SAX, Sinatra, and in Rails 5 callbacks rails/rails#17227 .
There was a problem hiding this comment.
Pong
B mobile phone
On Jan 3, 2016, at 9:08 PM, Lucas Hosseini notifications@github.com wrote:
In lib/active_model/serializer/adapter/json_api/deserialization.rb:
# # }## parse(document, fields: [:title, :author, date: :published_at]) #=># # {# # title: 'Title 1',# # published_at: '2015-12-20',# # author_id: 2,# # }## rubocop:disable Metrics/CyclomaticComplexitydef parse!(document, options = {})fields = parse_fields_option(options[:fields])
document = document.dup.permit!.to_h if document.is_a?(ActionController::Parameters)
fail ArgumentError, 'Invalid payload' unless payload_valid?(document)—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
could I intervene? Why not to use pros of both approaches?
DEFAULT_FALLBACK = ->(error) { fail ArgumentError, 'Invalid payload' }
SAFE_FALLBACK = ->(error) { {} }
def parse!(document, options = {}, &fallback)
fallback ||= DEFAULT_FALLBACK
fields = parse_fields_option(options[:fields])
document = document.dup.permit!.to_h if document.is_a?(ActionController::Parameters)
return fallback.call(document) unless payload_valid?(document)
# ...
def parse(document, options = {})
parse!(document, options, &SAFE_FALLBACK)
end
There was a problem hiding this comment.
Skipping back to the first post in this thread, and now that I'm starting to experiment with this PR a bit more; the current Argument Error: Invalid Payload format is a little too inflexible to return detailed error messages to the client. In terms of technical approach I couldn't comment but I agree with @bf4 on the following line:
fail InvalidDocument, "Invalid Document: #{reason}" # reason would be good, right?A custom error class with a reason would allow us to rescue_from InvalidDocument, with: ... so the default param is missing or the value is empty: #{reason_field} error message can be replicated, at least functionally.
I'd feel less comfortable rescuing an ArgumentError in a base controller since this is a bit of a catch all and without a reason, a specific error can't be passed on to the client.
| # @example | ||
| # parse_attributes({ 'a' => 'b', 'c' => 'd'}, nil) | ||
| # # => { :a => 'b', :c => 'd' } | ||
| # parse_attributes({ 'a' => 'b', 'c' => 'd', 'e' => 'f' }, { :a, :c => :g }) |
There was a problem hiding this comment.
You're right, it was meant to be [ :a, :c => :g ]. Thanks for spotting this.
|
Last commit:
|
beauby
force-pushed
the
jsonapi-parse
branch
2 times, most recently
from
4abc3a3
to
fa70440
Compare
|
Recap on the current situation:
There should be a bit more tests, but other than that, I believe this PR is ready. Any thoughts? |
|
Will merge once green. |
|
Ladies and gentlemen: merging! |
|
Woohoo! Well done @beauby ! |
| # # } | ||
| # | ||
| # parse(document, only: [:title, :date, :author], | ||
| # keys: { date: :published_at }, |
|
Should this support transforming the key back? Typically an application will store keys in underscore format. AMS should then convert them to dashed for JSON API. But, upon deserialization, they should be converted back to underscores. Seems like that should happen here. |
|
@maletor Can you make a new issue? thanks |
Latest update:
Recap on the current situation:
ActiveModelSerializers::Deserialization.jsonapi_parseandActiveModelSerializers::Deserialization.jsonapi_parse!)document(either a Hash or an instance ofActionController::Parametersrepresenting a JSON API payload)optionshash:onlyorexcept: array of white/black-listed fields (recall that in the context of JSON API, "fields" denotes both attributes and relationships)polymorphic: array of polymorphic associations (those for which the#{relationship}_typemust be set forActiveRecordto handle it properly)keys: hash of translated keys (for instance when the payload contains anauthorrelationship which refers to anuserassociation on the model)!) throws anInvalidDocumentexception when the payload does not meet basic criteria for JSON API deserialization.Example:
There should be a bit more tests, but other than that,I believe this PR is ready. Any thoughts?Update:
This PR offers basic JSON API deserialization through
ActiveModel::Serializer::Adapter::JsonApi::Deserialization.parse(hash, options = {})which takes a hashor an instance ofrepresenting a JSON API document (and possibly some options, more on that further down) and returns anActionController::ParametersActiveRecord-friendly hash, so that you can use it as follows:The only available option currently is
fields, which allows forkeyoption on the association in the serializer, by providing a Hash at the end of the array.Example:
returns
Note: It currently does not handle polymorphic associations, but could easily be extended to do so.
Initial post:
This PR offers basic JSON API deserialization through
ActiveModel::Serializer::Adapter::JsonApi.parse(params_or_hash)ActionController::Parametersrepresenting a JSON API document and turns it into anActiveRecord-friendly hash, so that you cas use it as follows:Note that this PR does not support aliased relationships(that is the name of the relationship in the client-issued JSON API document won't be changed).Moreover,
it currently does not support any kind of whitelisting directly, although you could simply dopost_params.slice!(:id, :name, :description)to whitelist orpost_params.except!(:is_admin)to blacklist.Finally,
it currently fails if the JSON API document is ill-formed.We could also make it so that the
parsemethod transforms whatever comes its way into a newActionController::Parametersinstance, so that people canrequire/permitstuff instead ofslice/except.