rails-api / active_model_serializers

ActiveModel::Serializer implementation and Rails hooks
MIT License
5.32k stars 1.39k forks source link

ErrorSerializer not pointing to JSON:API relationship errors appropriately? #1774

Open chrisdpeters opened 8 years ago

chrisdpeters commented 8 years ago

Stripped-down model logic:

class ThemeAsset < ActiveRecord::Base
  belongs_to :asset
  validates :asset_id, presence: true
end

And its matching serializer:

class ThemeAssetSerializer < ActiveModel::Serializer
  attributes :id, :path, :created_at, :updated_at
  belongs_to :asset
end

If I submit a blank value for the asset relationship, I get this error payload:

{
  "errors": [
    {
      "source": {
        "pointer": "/data/attributes/asset-id"
      },
      "detail": "can't be blank"
    }
  ]
}

Because of the JSON:API payload that the server must accept, shouldn't the error look more like this?

{
  "errors": [
    {
      "source": {
        "pointer": "/data/relationships/asset/data/id"
      },
      "detail": "can't be blank"
    }
  ]
}

This is the payload that the server expects to receive to further illustrate how the pointer should work:

{
  "data": {
    "type": "theme-assets",
    "attributes": {
      "path": "images/logo.png"
    },
    "relationships": {
      "asset": {
        "data": {
          "type": "assets",
          "id": "f4b297ce-1b88-4773-8dc7-cc546cad5655"
        }
      }
    }
  }
}

This one seems like it would be tricky to implement (would need to involve association reflections and such), but I do think that it would be against the spec until it's resolved.

Environment

ActiveModelSerializers Version (commit ref if not on tag): 0.10.0

NullVoxPopuli commented 8 years ago

Well, your validation is on the belongs to relationship, so that is a field on your theme asset. I think to get the behavior you want, it may require a change to Active Record.

chrisdpeters commented 8 years ago

@NullVoxPopuli Do you think it would be possible for ErrorSerializer to check if an error is set on an attribute involved with an AR belongs_to association and then instead map the error in the manner that I've outlined above if it is?

I could see how it would get complicated pretty fast though, especially if it is a polymorphic association.

chrisdpeters commented 8 years ago

Plus it would need to reflect the association to see what the key's name is vs. if the error is on the association itself. asset and asset_id are both attributes that are involved with the association that could have errors, plus the belongs_to call could also map a different foreign_key as well.

I'm considering if this is something that I could submit a pull request for. I've tried poking around in the AMS source before and have done some work with ActiveRecord::Reflection as well. Hmm.

bf4 commented 8 years ago

@chrisdpeters thanks for finding a JSON Pointer generation bug. Shouldn't be too hard to fix...

"pointer": "/data/relationships/asset/data/id"

It doesn't do included ( e.g. "/included/0/attributes/#{attribute}" ) properly, either :(

bf4 commented 8 years ago

Also, the ErrorSerializer is kind of an MVP. There's a lot of useful things it doesn't do :)

chrisdpeters commented 8 years ago

@bf4 I didn't know that it was appropriate/allowed to submit included data on a POST/PATCH. I've never seen that in any examples at least.

I figured that ErrorSerializer was relatively fresh. :) I've been watching you guys develop the JSON:API adapter with eager anticipation. Thanks to you @rails-api for everything that you've been doing to make this a reality.

bf4 commented 8 years ago

@chrisbranson sorry, that if query request has include and there is an error on an included attribute in the response...

chrisdpeters commented 8 years ago

@bf4 Gotcha. Just making sure that you didn't shed the light on something that I was unaware of. I've stared at that spec for many, many hours. 😄