Adding JSONAPI::Utils to controller causes PATCH operations to malfunction about jsonapi-utils HOT 8 CLOSED

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.

from jsonapi-utils.

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!

from jsonapi-utils.

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

from jsonapi-utils.

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.

from jsonapi-utils.

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

from jsonapi-utils.

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.

from jsonapi-utils.

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.

from jsonapi-utils.

Fixed in #45

from jsonapi-utils.