From 3d986377b66f708e25a663bc85a295545da1c63f Mon Sep 17 00:00:00 2001 From: Benjamin Fleischer Date: Thu, 11 Feb 2016 00:14:54 -0600 Subject: [PATCH] Collapse JSON API success/failure documents in one adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Idea per remear (Ben Mills) in the slack: https://amserializers.slack.com/archives/general/p1455140474000171 remear: just so i understand, the adapter in `render json: resource, status: 422, adapter: 'json_api/error', serializer: ActiveModel::Serializer::ErrorSerializer` is a different one than, say what i’ve specified in a base serializer with `ActiveModel::Serializer.config.adapter = :json_api`. correct? and a followup question of, why not same adapter but different serializer? me: With the way the code is written now, it might be possible to not require a special jsonapi adapter. However, the behavior is pretty different from the jsonapi adapter. this first draft of the PR had it automatically set the adapter if there were errors. since that requires more discussion, I took a step back and made it explicit for this PR If I were to re-use the json api adapter and remove the error one, it think the serializable hash method would look like ``` def serializable_hash(options = nil) return { errors: JsonApi::Error.collection_errors } if serializer.is_a?(ErrorsSerializer) return { errors: JsonApi::Error.resource_errors(serializer) } if serializer.is_a?(ErrorSerializer) options ||= {} ``` I suppose it could be something more duckish like ``` def serializable_hash(options = nil) if serializer.errors? # object.errors.any? || object.any? {|o| o.errors.any? } JsonApi::Error.new(serializer).serializable_hash else # etc ``` --- docs/jsonapi/errors.md | 7 ++-- lib/active_model/serializer.rb | 4 +++ .../serializer/collection_serializer.rb | 4 +++ .../serializer/error_serializer.rb | 4 +++ .../serializer/errors_serializer.rb | 6 +++- .../adapter/json_api.rb | 29 +++++++++++++++ .../adapter/json_api/error.rb | 36 +++---------------- .../action_controller/json_api/errors_test.rb | 2 +- .../adapter_for_test.rb | 4 +-- test/adapter/json_api/errors_test.rb | 4 +-- test/serializable_resource_test.rb | 4 +-- 11 files changed, 60 insertions(+), 44 deletions(-) diff --git a/docs/jsonapi/errors.md b/docs/jsonapi/errors.md index f180d117..74f3b166 100644 --- a/docs/jsonapi/errors.md +++ b/docs/jsonapi/errors.md @@ -2,9 +2,8 @@ # JSON API Errors -Rendering error documents requires specifying the serializer and the adapter: +Rendering error documents requires specifying the error serializer(s): -- `adapter: :'json_api/error'` - Serializer: - For a single resource: `serializer: ActiveModel::Serializer::ErrorSerializer`. - For a collection: `serializer: ActiveModel::Serializer::ErrorsSerializer`, `each_serializer: ActiveModel::Serializer::ErrorSerializer`. @@ -23,7 +22,7 @@ resource.errors.add(:name, 'cannot be nil') resource.errors.add(:name, 'must be longer') resource.errors.add(:id, 'must be a uuid') -render json: resource, status: 422, adapter: 'json_api/error', serializer: ActiveModel::Serializer::ErrorSerializer +render json: resource, status: 422, adapter: :json_api, serializer: ActiveModel::Serializer::ErrorSerializer # #=> # { :errors => # [ @@ -44,7 +43,7 @@ resource.errors.add(:name, 'must be awesome') serializable_resource = ActiveModel::SerializableResource.new( resource, { serializer: ActiveModel::Serializer::ErrorSerializer, - adapter: 'json_api/error' + adapter: :json_api }) serializable_resource.as_json(options) # #=> diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb index 1471876f..b0ca3fa6 100644 --- a/lib/active_model/serializer.rb +++ b/lib/active_model/serializer.rb @@ -118,6 +118,10 @@ module ActiveModel end end + def success? + true + end + # Used by adapter as resource root. def json_key root || object.class.model_name.to_s.underscore diff --git a/lib/active_model/serializer/collection_serializer.rb b/lib/active_model/serializer/collection_serializer.rb index c1edfeaf..02258838 100644 --- a/lib/active_model/serializer/collection_serializer.rb +++ b/lib/active_model/serializer/collection_serializer.rb @@ -22,6 +22,10 @@ module ActiveModel end end + def success? + true + end + def json_key root || derived_root end diff --git a/lib/active_model/serializer/error_serializer.rb b/lib/active_model/serializer/error_serializer.rb index bfbd1ec0..c12dfd37 100644 --- a/lib/active_model/serializer/error_serializer.rb +++ b/lib/active_model/serializer/error_serializer.rb @@ -3,4 +3,8 @@ class ActiveModel::Serializer::ErrorSerializer < ActiveModel::Serializer def as_json object.errors.messages end + + def success? + false + end end diff --git a/lib/active_model/serializer/errors_serializer.rb b/lib/active_model/serializer/errors_serializer.rb index eaab0a14..4b67bae8 100644 --- a/lib/active_model/serializer/errors_serializer.rb +++ b/lib/active_model/serializer/errors_serializer.rb @@ -1,5 +1,5 @@ require 'active_model/serializer/error_serializer' -class ActiveModel::Serializer::ErrorsSerializer < ActiveModel::Serializer +class ActiveModel::Serializer::ErrorsSerializer include Enumerable delegate :each, to: :@serializers attr_reader :object, :root @@ -13,6 +13,10 @@ class ActiveModel::Serializer::ErrorsSerializer < ActiveModel::Serializer end end + def success? + false + end + def json_key nil end diff --git a/lib/active_model_serializers/adapter/json_api.rb b/lib/active_model_serializers/adapter/json_api.rb index 57179a04..4f82835f 100644 --- a/lib/active_model_serializers/adapter/json_api.rb +++ b/lib/active_model_serializers/adapter/json_api.rb @@ -53,7 +53,14 @@ module ActiveModelSerializers def serializable_hash(options = nil) options ||= {} + if serializer.success? + success_document(options) + else + failure_document + end + end + def success_document(options) is_collection = serializer.respond_to?(:each) serializers = is_collection ? serializer : [serializer] primary_data, included = resource_objects_for(serializers) @@ -77,6 +84,28 @@ module ActiveModelSerializers hash end + # TODO: look into caching + # rubocop:disable Style/AsciiComments + # definition: + # ☑ toplevel_errors array (required) + # ☐ toplevel_meta + # ☐ toplevel_jsonapi + # rubocop:enable Style/AsciiComments + def failure_document + hash = {} + # PR Please :) + # ApiObjects::Jsonapi.add!(hash) + + if serializer.respond_to?(:each) + hash[:errors] = serializer.flat_map do |error_serializer| + Error.resource_errors(error_serializer) + end + else + hash[:errors] = Error.resource_errors(serializer) + end + hash + end + def fragment_cache(cached_hash, non_cached_hash) root = false if instance_options.include?(:include) ActiveModelSerializers::Adapter::JsonApi::FragmentCache.new.fragment_cache(root, cached_hash, non_cached_hash) diff --git a/lib/active_model_serializers/adapter/json_api/error.rb b/lib/active_model_serializers/adapter/json_api/error.rb index fbe2654d..c8383d21 100644 --- a/lib/active_model_serializers/adapter/json_api/error.rb +++ b/lib/active_model_serializers/adapter/json_api/error.rb @@ -1,29 +1,13 @@ module ActiveModelSerializers module Adapter class JsonApi < Base - class Error < Base - UnknownSourceTypeError = Class.new(ArgumentError) + module Error # rubocop:disable Style/AsciiComments - # TODO: look into caching - - # definition: - # ☐ toplevel_errors array (required) - # ☑ toplevel_meta - # ☑ toplevel_jsonapi - def serializable_hash(*) - hash = {} - # PR Please :) - # Jsonapi.add!(hash) - - # Checking object since we're not using an ArraySerializer - if serializer.object.respond_to?(:each) - hash[:errors] = collection_errors - else - hash[:errors] = Error.resource_errors(serializer) - end - hash - end + UnknownSourceTypeError = Class.new(ArgumentError) + # Builds a JSON API Errors Object + # {http://jsonapi.org/format/#errors JSON API Errors} + # # @param [ActiveModel::Serializer::ErrorSerializer] # @return [Array] i.e. attribute_name, [attribute_errors] def self.resource_errors(error_serializer) @@ -86,16 +70,6 @@ module ActiveModelSerializers fail UnknownSourceTypeError, "Unknown source type '#{source_type}' for attribute_name '#{attribute_name}'" end end - - private - - # @return [Array<#object_errors>] - def collection_errors - serializer.flat_map do |error_serializer| - Error.resource_errors(error_serializer) - end - end - # rubocop:enable Style/AsciiComments end end diff --git a/test/action_controller/json_api/errors_test.rb b/test/action_controller/json_api/errors_test.rb index 09fd3099..d5890110 100644 --- a/test/action_controller/json_api/errors_test.rb +++ b/test/action_controller/json_api/errors_test.rb @@ -30,7 +30,7 @@ module ActionController resource.errors.add(:name, 'cannot be nil') resource.errors.add(:name, 'must be longer') resource.errors.add(:id, 'must be a uuid') - render json: resource, adapter: 'json_api/error', serializer: ActiveModel::Serializer::ErrorSerializer + render json: resource, adapter: :json_api, serializer: ActiveModel::Serializer::ErrorSerializer end end diff --git a/test/active_model_serializers/adapter_for_test.rb b/test/active_model_serializers/adapter_for_test.rb index 4fe870be..8dfbc9f3 100644 --- a/test/active_model_serializers/adapter_for_test.rb +++ b/test/active_model_serializers/adapter_for_test.rb @@ -102,8 +102,7 @@ module ActiveModelSerializers 'null'.freeze => ActiveModelSerializers::Adapter::Null, 'json'.freeze => ActiveModelSerializers::Adapter::Json, 'attributes'.freeze => ActiveModelSerializers::Adapter::Attributes, - 'json_api'.freeze => ActiveModelSerializers::Adapter::JsonApi, - 'json_api/error'.freeze => ActiveModelSerializers::Adapter::JsonApi::Error + 'json_api'.freeze => ActiveModelSerializers::Adapter::JsonApi } actual = ActiveModelSerializers::Adapter.adapter_map assert_equal actual, expected_adapter_map @@ -114,7 +113,6 @@ module ActiveModelSerializers 'attributes'.freeze, 'json'.freeze, 'json_api'.freeze, - 'json_api/error'.freeze, 'null'.freeze ] end diff --git a/test/adapter/json_api/errors_test.rb b/test/adapter/json_api/errors_test.rb index 0ab596f9..da7eff9b 100644 --- a/test/adapter/json_api/errors_test.rb +++ b/test/adapter/json_api/errors_test.rb @@ -13,7 +13,7 @@ module ActiveModelSerializers def test_active_model_with_error options = { serializer: ActiveModel::Serializer::ErrorSerializer, - adapter: :'json_api/error' + adapter: :json_api } @resource.errors.add(:name, 'cannot be nil') @@ -37,7 +37,7 @@ module ActiveModelSerializers def test_active_model_with_multiple_errors options = { serializer: ActiveModel::Serializer::ErrorSerializer, - adapter: :'json_api/error' + adapter: :json_api } @resource.errors.add(:name, 'cannot be nil') diff --git a/test/serializable_resource_test.rb b/test/serializable_resource_test.rb index 14ac73b6..4c683f9b 100644 --- a/test/serializable_resource_test.rb +++ b/test/serializable_resource_test.rb @@ -40,7 +40,7 @@ module ActiveModel serializable_resource = ActiveModel::SerializableResource.new( resource, { serializer: ActiveModel::Serializer::ErrorSerializer, - adapter: 'json_api/error' + adapter: :json_api }) expected_response_document = { :errors => @@ -61,7 +61,7 @@ module ActiveModel resources, { serializer: ActiveModel::Serializer::ErrorsSerializer, each_serializer: ActiveModel::Serializer::ErrorSerializer, - adapter: 'json_api/error' + adapter: :json_api }) expected_response_document = { :errors =>