jsonapi-rb / jsonapi-rails

Rails gem for fast jsonapi-compliant APIs.
http://jsonapi-rb.org
MIT License
319 stars 63 forks source link

Return source pointer '/data/attributes' if attribute was not supplied in payload #94

Open JoeWoodward opened 6 years ago

JoeWoodward commented 6 years ago

This PR fixes the source pointer when the payload is missing a validated attribute.

e.g.

class User
  validates :name, presence: true
end

class UsersController
  deserializable_resource :user

  def create
    user = User.new(user_params)
    if user.save
      render jsonapi: user
    else
      render jsonapi_errors: user.errors
    end 
  end

  private

  def user_params
    params.require(:user).permit(:name)
  end
end

if we post

{
  data: {
    attributes: {}
  }
}

We get back an error with an empty pointer

{
  "errors": [
    {
      "title": "Invalid name",
      "detail": "Name can't be blank",
      "source": {}
    }
  ],
  "jsonapi": {
    "version": "1.0"
  }
}

With this change we return the node where the attribute is missing from

{
  "errors": [
    {
      "title": "Invalid name",
      "detail": "Name can't be blank",
      "source": {
        "pointer": "/data/attributes"
      }
    }
  ],
  "jsonapi": {
    "version": "1.0"
  }
}
JoeWoodward commented 6 years ago

Should we override the error description if the pointer is missing? http://jsonapi.org/examples/#error-objects-source-usage the example here returns a description

"detail": "Missing data Member at document's top level." which maybe makes more sense. Then supply the original error in the meta

i.e.

{
  "errors": [
    {
      "detail": "Missing `name` Member at document's /data/attributes level",
      "source": {
        "pointer": "/data/attributes"
      },
      "meta": {
        "detailed_error_message": {
          "title": "Invalid name",
          "detail": "Name cannot be blank"
        }
      }
    }
  ],
  "jsonapi": {
    "version": "1.0"
  }
}

Maybe the meta is overkill. Probably enough to just override the detail and remote the title

beauby commented 5 years ago

IIRC, the /data pointer should always be valid, unless the payload itself is invalid (to be double checked), in which case we could add some logic to craft the right pointer/error depending on the payload (/data vs /data/attributes).

JoeWoodward commented 5 years ago

I think that's the issue though. The following is a valid payload

{
  data: {
    attributes: {}
  }
}

Even if the logic was changed to consider this invalid we can still get null pointers with missing attributes. e.g.

{
  data: {
    type: 'users',
    attributes: {
      email: 'valid@mail.com'
      # name: 'expected but not passed at all, not just null'
    }
  }
}
{
  "errors": [
    {
      "title": "Invalid name",
      "detail": "Name can't be blank",
      "source": {}
    }
  ],
  "jsonapi": {
    "version": "1.0"
  }
}

There is an issue with this PR though; The error isn't necessarily going to be from a member on /data/attributes. I actually looked back at this repo because I was researching sideposting, which I believe isn't through the attributes node so returning source: { pointer: '/data/attributes' } would be invalid.

Maybe there's a way to decipher the pointer from the AM::Errors hash. Need to look into it further