jsonapi-suite / jsonapi_compliable

MIT License
20 stars 35 forks source link

Disassociating a required belongs_to should raise error #95

Open richmolj opened 6 years ago

richmolj commented 6 years ago

If Post has many Comments, and you try to sidepost disassociate a comment, it will silently fail. Instead, we should raise an expressive error message.

The issue is that Rails 5 defaults belongs_to to be required and will fail validation if the foreign key is nil. So we attempt to disassociate these objects, but no DB update is made. This error should be a 422 validation error but since these objects are already disassociate in memory we never know to do so.

richmolj commented 6 years ago

Thanks to @jkeen for reporting in Slack.

jkeen commented 6 years ago

I don't have a PR or a fix I feel good about for this, but I have an idea of what needs to happen based on how far I went down the rabbit hole of debugging yesterday.

I think the _persist hook needs to be modified to do something like this:

def _persist
  validation_response = nil
  jsonapi_resource.transaction do

    # Get handles on the comments we're intending to disassociate before params 
    # are modified in the yield 
    disassociations = fetch_pending_disassociations

    object = yield # disassociation attempt happens here

    # the `comment` model's foreign_key has now been nullified, and 
    # will fail validation since `optional` wasn't specified on the `belongs_to` 
    # declaration in the comment model 

    # the validator below will fetch the `post` model's `comment`
    # association, but the comment we wanted to disassociate won't be fetched 
    # because its already been nullified and removed from the Active Record
    # associations array. So pass in that extra model to add to the validation?

    validation_response = Util::ValidationResponse.new \
      object, deserialized_params, disassociations
    raise Errors::ValidationError unless validation_response.to_a[1]
  end
  validation_response
end

Hope that helps someone out in fixing this in a more elegant way.