Don't crash obscurely on missing data payload for POST & PATCH requests

[aCandidMind]

Let #data and indirectly #attributes return an empty hash instead. Added corresponding specs.

Apps receiving no data param and the new 'when data is absent' spec for #attributes crash with an obscure ERROR: NoMethodError: undefined method '[]' for nil:NilClass without the Deserializer#data fix.

Tested with appraisal rspec and found no errors.

[richmolj]

@aCandidMind thanks! Just so I understand, what's the use case for this - writing a spec and mis-typing the payload?

Also - if we raised an unhelpful error before, what would the new experience be? I think we should still be returning an error payload and want to make sure this doesn't incorrectly return a 200.

[aCandidMind]

Currently if there's no data in the request payload for a POST or PATCH you simply get an error json with status 500 as the client response and on the server you get a very obscure and minimal trace of
ERROR: NoMethodError: undefined method '[]' for nil:NilClass because Deserializer#raw_attributes uses data[:attributes] and data just returns nil.

This pull request would result in the deserializer not crashing and the payload being forwarded to the resource. The response is then ultimately dependent on the models having validations (likely a 422 because of missing fields then) or not (might actually really create a record and return 200) this way. For an update I got a RecordNotFound exception (response code 500) with this new commit. So far this PR focusses only on not letting the deserializer crash obscurely. We could also generally raise a new error called BadPayload in the event of an update request with missing data or type subkey instead (and jsonapi_errorable might even return code 400 by default in that scenario).

[richmolj]

We could also generally raise a new error called BadPayload in the event of an update request with missing data or type subkey instead (and jsonapi_errorable might even return code 400 by default in that scenario).

This is the direction I would like to head ❤️ Since this is a malformed request, we should be responding with a 400. And to do that, the deserializer would identify what "malformed" means (in this context it's just missing the data key), and raise an new error class. That error class can be caught by jsonapi_errorable to ensure it returns a 400 instead of 500.

That last bit I think is just a bonus - if we got a better trace from returning a custom error class, but still returned a 500, we've at least made some really good progress.

[aCandidMind]

For requests other than GET or DELETE a new BadRequest error is raised now on missing data. This is also the new base class for BadFilter, UnsupportedPageSize and StatNotFound. Also made ValidationError inherit from RuntimeError instead of StandardError.

All tests are green. Not sure, whether I will add more tests in the near future out of my own motivation. Your nice persistence integration test covered actual requests (e.g. missing data on DELETE). It's your call, whether this needs more tests.

[richmolj]

Small change to move the logic then we are good to go. I like what you did with the errors! 👍

[richmolj]

All looks good, except I was hoping we could do something like validate_payload! within the constructor, to keep the logic out of the accessor. The intent is to eventually do more complex validations, possibly using jsonapi-rb's parser.