fotinakis / jsonapi-serializers

Pure Ruby readonly serializers for the JSON:API spec.
MIT License
414 stars 91 forks source link

top level errors #66

Closed koriroys closed 8 years ago

koriroys commented 8 years ago

per #63, and the json-api spec,

The members data and errors MUST NOT coexist in the same document.

So I'm opening this issue to discuss a way forward.

@fotinakis I read that you don't want to bind this library to ActiveRecord, which I applaud. It is likely the most common case to be using AR though, so some kind of support, while leaving other options open seems good to me.

A first stab:

config/initializers/jsonapi_initializer.rb

JSONAPI.setup do |config|
  config.error_adapter = :active_record
end

option 1:

JSONAPI::ErrorSerializer.serialize(errors)

option 2:

JSONAPI::Serializer.serialize_errors(errors)

option 3:

JSONAPI::Serializer.serialize(obj) # detect errors

Of the three, I lean towards option 1, letting the user pass in the errors, and letting the configured error_adapter munge the errors into a form acceptable to JSON API.

A user might use it like this: some_controller.rb

def create
  book = Book.create(underscore(book_params))
  if book.errors
    render json: JSONAPI::ErrorSerializer.serialize(book.errors), status: 422
  else
    render json: JSONAPI::Serializer.serialize(book)
  end
end

def book_params
  params.require(:data).require(:attributes).permit(:title, :author_id, :page_count)
end

Of course, you might be in the unenviable position of migrating an app to a new api, and so you should be able to override the configured error serializer when called. e.g.

  JSONAPI::ErrorSerializer.serialize(book.errors, error_adapter: :some_custom_adapter)

I'm not sure where some_custom_adapter.rb would live though.

fotinakis commented 8 years ago

Thanks for thinking this through and getting things started!

I think step one is we should change the current API (or add a new one and deprecate the old one) to be Option 2 above, JSONAPI::Serializer.serialize_errors(errors). This would get rid of the first big problem of including the top-level data key or needing to pass an object. We can do this without needing to build an adapter layer at this stage, it can just pass through the JSON into the errors key like it currently does.

Then, stage 2 we should either add the adapter layer or a duck-typing check and then be able to handle ActiveModel::Errors objects and automatically mash them into a valid JSON-API format. I've tried to avoid adding a configuration layer to this gem until absolutely required, and I think we can probably still avoid it for a while.

So, as a simple start, we should:

  1. Deprecate the serialize(nil, errors: errors) method and remove the documentation.
  2. Add serialize_errors(errors) which basically just returns {errors: errors} for now.
  3. Update the documentation.

Then stage two, separate PR:

  1. Add a simple duck-typing check for respond_to?(:messages) (or some other more specific part of ActiveModel::Errors) and have logic for transforming the object into JSON-API valid error objects. This logic could exist as an ActiveModelErrorAdapter < JSONAPI::Serializer::ErrorAdapter or something so that it can later be extended for other framework error objects.

@koriroys If you can take on Step 1 that'd be great! Otherwise I can get to it in the next week or so, it's a quick change.

fotinakis commented 8 years ago

Another thought for later: we should probably just have a JSONAPI::Error or JSONAPI::Serializer::Error class, and the error adapter's job should just be to transform arbitrary objects into JSONAPI::Error objects, which we then know how to serialize. This is how Ember's JSON-API error handling works internally.

fotinakis commented 8 years ago

Fixed by https://github.com/fotinakis/jsonapi-serializers/pull/67

fotinakis commented 8 years ago

Actually, leaving open for stage 2 support later on.

fotinakis commented 8 years ago

Fixed in https://github.com/fotinakis/jsonapi-serializers/pull/68