Adding JSONAPI::Utils to controller causes PATCH operations to malfunction

[delner]

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::RequestParseris 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?

[tiagopog]

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.

[delner]

Okay. From what I understand, dup does not deep copy references, so when JSONAPI::RequestHelper uses the duped 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!

[tiagopog]

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

[tiagopog]

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.

[tiagopog]

There's also a similar open issue here. So I think it's really relevant to bring up some "fix" now.

[delner]

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.

[tiagopog]

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.

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.

[tiagopog]

Fixed in #45