Handle from activerecord error(s) while saving the record

[kuldeepaggarwal]

SCIMitar gem relies on the fact that Rails throw ActiveRecord::RecordInvalid only if the record is invalid however, Rails can also raise RecordNotSaved error in case of the object is not stored.

In case of RecordNotSaved, we should return ResourceInvalidError error(status_code=400) from SCIMitar instead of 500 status code.

ErrorResponse.new(status: 500, detail: exception.message)

activerecord-rescue_from_duplicate is a gem that we can use to rescue from DB level unique errors in case of race condition.

Adding DB level unique constraint guarantees uniqueness else we could have duplicate records.

And activerecord-rescue_from_duplicate raises RecordNotSaved error in case we receive unique validation error from MYSQL.
Note: SCIM server should respond with 409 in this case.

Proposal

So, I am proposing a slight change in the implementation:

def rescuable_exceptions
  [ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved]
end

def save!(record)
  record.save!

rescue *rescuable_exceptions # here is the slight change
  joined_errors = record.errors.full_messages.join('; ')

  # https://tools.ietf.org/html/rfc7644#page-12
  #
  #   If the service provider determines that the creation of the requested
  #   resource conflicts with existing resources (e.g., a "User" resource
  #   with a duplicate "userName"), the service provider MUST return HTTP
  #   status code 409 (Conflict) with a "scimType" error code of
  #   "uniqueness"
  #
  if record.errors.any? { | e | e.type == :taken }
    raise Scimitar::ErrorResponse.new(
      status:   409,
      scimType: 'uniqueness',
      detail:   joined_errors
    )
  else
    raise Scimitar::ResourceInvalidError.new(joined_errors)
  end
end

This way, this will be extendible in the future for the consumer(like us) to add more custom exceptions in the list if we want to return Scimitar::ResourceInvalidError for particular exceptions.

[pond]

No objections in principle! Are you thinking of creating a PR or wanting us to do this from our side based on the suggestion in this GitHub issue?

[kuldeepaggarwal]

I can raise the PR if we are okay with the approach.

[pond]

@kuldeepaggarwal (Belatedly, sorry) Yes, something like that would work. Please note some minor changes to the code in that area in the current branch, with the bulk of the actual exception handling moved into a new method but otherwise, the idea of rescuing a configurable list of exceptions would be applied in the same way as you suggest.

[pond]

Since I've allocated myself a couple of days to work on Scimitar backlog stuff, I'm implementing this now, so no need for a new PR at this point.

[gsar]

@pond please also include ActiveRecord::RecordNotUnique in rescuable_exceptions. thanks!

[pond]

This is now in v2.7.0.

[pond]

...and v1.8.0.