Adding JSONAPI::Utils to controller causes PATCH operations to malfunction
delner opened this issue · 8 comments
Affects jsonapi-utils 0.5.0.beta4
, jsonapi-resources 0.8.1
and Rails 5.0.0.1
.
Given a model SampleModel
:
class SampleModel < ApplicationRecord
# name: :string
end
And a resource SampleModelResource
:
class SampleModelResource < JSONAPI::Resource
attributes :name
end
With controllers:
class JsonApiController < JSONAPI::ResourceController
include JSONAPI::Utils
end
class SampleModelsController < JsonApiController
end
And routes:
Rails.application.routes.draw do
jsonapi_resources :sample_models
end
And a controller spec that performs a patch operation:
require 'rails_helper'
describe SampleModelsController, type: :controller do
before do
request.headers['Content-Type'] = "application/vnd.api+json"
end
describe 'PATCH #update' do
subject { patch :update, params: params, body: body.to_json }
let(:params) { { id: id } }
let(:body) { { data: { id: id, type: 'sample_models', attributes: { name: 'bar' } } }
let(:sample_model) { SampleModel.new(name: 'foo').tap { |m| m.save } }
let(:id) { sample_model.id }
it { expect(subject.status).to eq(200) }
end
end
Running PATCH raises a 400 Bad Request
, which manifests as:
{"errors":[{"title":"A key is required","detail":"The resource object does not contain a key.","code":"109","status":"400"}]}
I dug deeper, and what I found was that JSONAPI::Utils
adds a Rails callback that creates a JSONAPI::RequestParser
prior to the request being processed:
def setup_request
@request ||=
JSONAPI::RequestParser.new(
params.dup,
context: context,
key_formatter: key_formatter,
server_error_callbacks: (self.class.server_error_callbacks || [])
)
end
This ends up running some code in jsonapi-resources which parses the data parameters for the PATCH operation. In the process of parsing this data, it performs a delete
on the data hash.
def parse_single_replace_operation(data, keys, id_key_presence_check_required: true)
fail JSONAPI::Exceptions::MissingKey.new if data[:id].nil?
key = data[:id].to_s
if id_key_presence_check_required && !keys.include?(key)
fail JSONAPI::Exceptions::KeyNotIncludedInURL.new(key)
end
data.delete(:id) unless keys.include?(:id)
# ...
end
Now the JSONAPI::Utils
callback finishes without issue. However, when JSONAPI::RequestParser
is invoked again as Rails runs the actual action, the data
hash in params
no long has id
, which is required. It raises a MissingKey
error, and the request fails.
In summary...
It seems like calling JSONAPI::RequestHelper
twice with the same parameters is not safe. Despite JSONAPI::Util
's effort to params.dup
prior to invoking it, the JSONAPI::RequestHelper
code still modifies the underlying data
hash in a pretty bad way.
Frankly, this seems to be more of an issue with jsonapi-resources
than jsonapi-utils
. But I thought I'd bring it to your attention, because as it stands, it means one cannot compose your package into a JSON API controller.
Are there any feasible workarounds? Or should an issue be opened with jsonapi-resources
?
Thanks for reporting, @delner.
As you pointed, since it works with a duplicated params
object when instantiating JSONAPI::RequestHelper
, the described error was not supposed to be happening. Anyway, I may try to figure out the buggy code late today and get back with more clarification.
Okay. From what I understand, dup
does not deep copy references, so when JSONAPI::RequestHelper
uses the dup
ed ActionController::Parameters
object, and deletes the ID, it modifies the original reference too. (At least, that's the theory right now.)
e.g. Something akin to...
irb(main):001:0> h = { c: 1 }
=> {:c=>1}
irb(main):002:0> a = { h: h }
=> {:h=>{:c=>1}}
irb(main):003:0> b = a.dup
=> {:h=>{:c=>1}}
irb(main):004:0> h.delete(:c)
=> 1
irb(main):005:0> h
=> {}
irb(main):006:0> a
=> {:h=>{}}
irb(main):007:0> b
=> {:h=>{}}
Let me know what you find, or if I can be of any help!
Hey, @delner! Sorry for the delay, I had a pretty busy friday.
Well, taking a quick look now I just realized that the sample controller code is missing its actions so that JR applies its default update operation. That's why the request is being processed twice.
JU kind of enforces developers not to define operations within resource classes (JR's default) but in controllers rather.
Maybe for default actions – like the one from your example – we should consider a workaround, but I need to think a bit more since it's contrary to one of the gem's main goals: keep operations explicit in controllers.
To fix the sample code try this:
class SampleModelsController < JsonApiController
# PATCH /sample_models/:id
def update
sample_model = SampleModel.find(params[:id])
if sample_model.update(resource_params)
jsonapi_render json: sample_model
else
jsonapi_render_errors json: sample_model, status: :unprocessable_entity
end
end
end
Well, instead of enforcing developers to write their operations in controllers we may just recommend it on README
and let it optional. I'll really give it a thought.
There's also a similar open issue here. So I think it's really relevant to bring up some "fix" now.
Hmmm, I'm not really a fan of the having to write my own update
action. The whole point of using JSON API Resources was to use default, standard endpoints without writing a lot of repetitive code. Having to manually write this in the controller, although it being a work around for the reported issue, does add the risk of introducing bugs, too.
I think the best way to fix it would to be to have the jsonapi-resources
folks not modify the params
object by reference. But short of that, I think deep copying the params
object instead of a dup
, or some other direct approach, such that implementing json-utils
for the default case remains intuitive and easy.
Yeah, @delner. I had a thought on it yesterday and, yes, it's a better idea not to enforce developers to write their own definition of default actions. This way the gem will also keep as transparent as possible avoiding any additional learning curve in order to make things work.
Yesterday, while working on #45, I considered three ways to fix that buggy behavior with params
:
1. Make a deep copy of params
via marshalling/unmarshalling:
- Pros:
- Easy to implement;
- Cons:
- Every request will apply a deep copy of
params
– which eventually may contain a large payload, thus a bit expensive copy will happen – just to make sure that it will not affect/be affected by subsequent instances using this object; - A duplicated
@request
object will still be instantiated.
- Every request will apply a deep copy of
2. Override JSONAPI::ActsAsResourceController#process_request
removing the buggy code:
- Pros:
- More performant than the option 1;
- Fix the
@request
duplication.
- Cons:
- It's tightly coupled to the way JR deals with this method. Anyway, this may not be such a big deal since JU already consumes some APIs from JR.
3. Make a PR to JR applying a memoization where the @request
object is assigned:
- Pros:
- No further implementations on JU;
- It's probably the best way to fix this issue.
- Cons:
- Depends on the JR's approval.
For now, I started implementing the option 2 (#45) in order to pack a minor release with the fix. In parallel, I will work on a PR suggesting the memoization of @resource
on JR's JSONAPI::ActsAsResourceController
.