[RFC] JSON API Errors (Initial implementation and roadmap for full feature-set) #1004

bf4 · 2015-07-14T05:29:57Z

Usage

Requires explicit passing of Error serializer

# POST /api/{plural_resource_name}
def create  
  set_resource(resource_class.new(resource_params))

  if get_resource.save
    render :show, status: :created, adapter: :json_api
  else
    render json: get_resource.errors, status: :unprocessable_entity, adapter: :json_api, serializer: ActiveModel::Serializer::ErrorSerializer
  end
end

PR TODOS:

DOCS
- Usage and requirements jsonapi/errors.md
- data and errors cannot co-exist. http://jsonapi.org/format/#document-top-level ref
  - requests are transactional, so any part that succeeds needs to be rolled back if any part fails. http://jsonapi.org/format/#crud ref
- examples http://jsonapi.org/examples/#error-objects ref
changelog

Error object:

Serializing errors is different from resources.

Errors may be an instance of ActiveModel::Error, which we'd want to call model.errors.messages on.
- Resolution: true
Errors may be given as a hash.
- Resolution: undecided
Errors may be given by the end-user as a special 'error' object.
- Resolution undecided
ActiveModel::Error is necessarily a collection of error objects, each with it's own 'source' and 'detail'
- Resolution: that's how we'll use it.
Errors may be automatically generated from the controller.
- Resolution: deferred
- 422 for errors..
- but see [RFC] JSON API Errors (Initial implementation and roadmap for full feature-set) #1004 (comment) for concerns and alternative implementations

Full Error API Implementation:

Implementation tests:

multiple error objects
serializable resource?
Controller
- test/action_controller/json_api/errors_test.rb
- test/action_controller/rescue_from_test.rb
- rescue_with_handler, handle_exceptions

Other implementations

ref:

bf4 · 2015-07-14T06:19:01Z

Rubinius!!!! 😡 https://travis-ci.org/rails-api/active_model_serializers/jobs/70853922

  1) Error:
ActiveModel::Serializer::Adapter::JsonApi::ErrorsTest#test_active_model_errors:
NoMethodError: undefined method `first' on name:Symbol.
    kernel/delta/kernel.rb:78:in `first (method_missing)'
    /home/travis/build/rails-api/active_model_serializers/test/adapter/json_api/errors_test.rb:49:in `source'
    /home/travis/build/rails-api/active_model_serializers/lib/active_model/serializer.rb:195:in `attributes'
    kernel/common/enumerable.rb:83:in `each_with_object'
    kernel/bootstrap/array.rb:76:in `each'
    kernel/common/enumerable.rb:81:in `each_with_object'
    /home/travis/build/rails-api/active_model_serializers/lib/active_model/serializer.rb:193:in `attributes'
    /home/travis/build/rails-api/active_model_serializers/test/adapter/json_api/errors_test.rb:84:in `test_active_model_errors'
154 tests, 237 assertions, 0 failures, 1 errors, 0 skips
rake aborted!
Command failed with status (1): [ruby -I"lib:test" -r./test/test_helper.rb -I"/home/travis/.rvm/gems/rbx-2.5.7@global/gems/rake-10.4.2/lib" "/home/travis/.rvm/gems/rbx-2.5.7@global/gems/rake-10.4.2/lib/rake/rake_test_loader.rb" "test/action_controller/adapter_selector_test.rb" "test/action_controller/explicit_serializer_test.rb" "test/action_controller/json_api_linked_test.rb" "test/action_controller/rescue_from_test.rb" "test/action_controller/serialization_scope_name_test.rb" "test/action_controller/serialization_test.rb" "test/adapter/fragment_cache_test.rb" "test/adapter/json/belongs_to_test.rb" "test/adapter/json/collection_test.rb" "test/adapter/json/has_many_test.rb" "test/adapter/json_api/belongs_to_test.rb" "test/adapter/json_api/collection_test.rb" "test/adapter/json_api/errors_test.rb" "test/adapter/json_api/has_many_embed_ids_test.rb" "test/adapter/json_api/has_many_explicit_serializer_test.rb" "test/adapter/json_api/has_many_test.rb" "test/adapter/json_api/has_one_test.rb" "test/adapter/json_api/json_api_test.rb" "test/adapter/json_api/linked_test.rb" "test/adapter/json_test.rb" "test/adapter/null_test.rb" "test/adapter_test.rb" "test/array_serializer_test.rb" "test/generators/scaffold_controller_generator_test.rb" "test/generators/serializer_generator_test.rb" "test/lint_test.rb" "test/serializers/adapter_for_test.rb" "test/serializers/associations_test.rb" "test/serializers/attribute_test.rb" "test/serializers/attributes_test.rb" "test/serializers/cache_test.rb" "test/serializers/configuration_test.rb" "test/serializers/fieldset_test.rb" "test/serializers/meta_test.rb" "test/serializers/options_test.rb" "test/serializers/serializer_for_test.rb" "test/serializers/urls_test.rb" ]
kernel/bootstrap/proc.rb:20:in `call'
kernel/bootstrap/proc.rb:20:in `call'
kernel/bootstrap/array.rb:76:in `each'
/home/travis/.rvm/rubies/rbx-2.5.7/gems/gems/rubysl-monitor-2.0.0/lib/rubysl/monitor/monitor.rb:211:in `synchronize (mon_synchronize)'
kernel/bootstrap/array.rb:76:in `each'
/home/travis/.rvm/rubies/rbx-2.5.7/gems/gems/rubysl-monitor-2.0.0/lib/rubysl/monitor/monitor.rb:211:in `synchronize (mon_synchronize)'
kernel/bootstrap/array.rb:76:in `each'
kernel/common/kernel.rb:498:in `load'
kernel/common/block_environment.rb:53:in `call_on_instance'
kernel/common/eval.rb:176:in `eval'
/home/travis/.rvm/gems/rbx-2.5.7/bin/ruby_executable_hooks:15:in `__script__'
kernel/delta/code_loader.rb:66:in `load_script'
kernel/delta/code_loader.rb:152:in `load_script'
kernel/loader.rb:656:in `script'
kernel/loader.rb:842:in `main'
Tasks: TOP => default => test
(See full trace by running task with --trace)

bbuchalter · 2015-07-20T19:29:08Z

I have a question about the note you made regarding " The members data and errors MUST NOT coexist in the same document.". I don't see that in the spec. What's driving that?

bf4 · 2015-07-20T20:44:25Z

Good eyes. That's paraphrased from where the spec discusses the three roots

bbuchalter · 2015-07-20T21:15:18Z

@bf4, I see it now here: http://jsonapi.org/format/#document-top-level

bf4 · 2015-07-23T16:00:14Z

@joaomdmoura @kurko Have you had a chance to look at this?

joaomdmoura · 2015-07-31T05:58:48Z

Hey @bf4, have just check this one.
Really interested on this! Indeed we should cover all the edges on our json-api support. 👍 keep up the good work, let me know if you need some help 😄

bf4 · 2015-07-31T16:11:15Z

@joaomdmoura Thoughts about the adapters and serializers approach? And the bullet points at the top of the pr description?

For discussion of how serializing errors is different from resources

may often be an instance of ActiveModel::Error

which we'd want to call 'messages' on

if not, should we expect a hash? or give the end-user an object they must use?

is necessarily a collection of error objects, each with it's own 'source' and 'detail'

it's not quite right to use an ArraySerializer

bbuchalter · 2015-08-05T15:02:14Z

test/adapter/json_api/errors_test.rb

+
+            def detail
+              detail = object.last
+              if detail.respond_to?(:join)


Forgive me if this is obvious, but I don't see this path of execution under test and I'm not sure why it's required. Could you share your thinking?

Basically, I'm just coding to think through what the implementation should look like, so there's cases like here where I'm not sure how I should be generating an error and details. If 'detail' should always be a string, then I need to join a collection of details, but I can't easily convert to an array due to how ActiveModel::Error behaves etc. to the join just points out that I'm not sure what I'm dealing with :)

This PR is partly about generating discussion about how the implementation should look (in addition to actually writing it ;).

So, that's why it's not tested :)

Oh, and @bbuchalter thanks for taking a look at this and I look forward to hearing your thoughts!

Ember, a driver of the JSON api spec, notes that whereas before we might have

errors: { first_name: ['is invalid'] }

now we'd have

errors: { source: { pointer: 'data/attributes/first_name' }, detail: 'is invalid' }

And you see that the field 'first_name' can have multiple errors on it

See https://github.com/rails-api/active_model_serializers/pull/1004/files#r36335739 where I think I figured out what it is supposed to be

joaomdmoura · 2015-08-12T01:14:32Z

Hey everyone, so I just read through the thread.
So @bf4 it seems we have defined a convention already.
I would treat the ActiveModel::Error the same way we will treat the same way we are planning to treat the AR erros giving an object they could use.
Also agree that use ArraySerializer might be even hard than not use it, and would probably involve some hacks.
Awesome work on json-api as well 👍

bf4 · 2015-08-12T02:30:49Z

@joaomdmoura Still working on that PR json-api/json-api#828 and then will come back here

bf4 · 2015-08-21T08:47:16Z

The json-api PR is almost done. Will continue with this then.

bf4 · 2015-08-26T19:37:12Z

The JSON API errors example is merged json-api/json-api#828 (comment) and it's live! http://jsonapi.org/examples/

bf4 · 2015-08-27T16:56:40Z

Updated with some lessons learned with an MVP of just attribute errors, though I might want to check if they're attributes or relationships.. I just haven't thought about it yet

bf4 · 2015-08-27T16:57:38Z

lib/active_model/serializer/error_serializer.rb

@@ -0,0 +1,2 @@
+class ErrorSerializer < ActiveModel::Serializer
+end


I'm not sure at this time if I want to assign any attributes, or just use it as a 'pass through' for the adapter to make use of the 'object' (see def object_errors in the adapter).

bf4 · 2015-08-28T03:06:40Z

lib/active_model/serializer/adapter/json_api/error.rb

+class ActiveModel::Serializer::Adapter::JsonApi::Error < ActiveModel::Serializer::Adapter
+  def serializable_hash(options = nil)
+    options ||= {}
+


Why two blank lines?

NullVoxPopuli · 2016-03-06T16:37:14Z

Appveyor, why? :-(

NullVoxPopuli · 2016-03-06T16:38:27Z

@bf4 looks like this needs squash rebase - I can handle that if you want / need

- ActiveModelSerializers::JsonPointer - ActiveModel::Serializer::Adapter::JsonApi::Error - ActiveModel::Serializer::Adapter::JsonApi::Error.attributes - Fix rubocop config

- Separate collection errors from resource errors in adapter - Refactor to ErrorsSerializer; first-class json error methods - DOCS - Rails 4.0 requires assert exact exception class, boo

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 ```

bf4 · 2016-03-06T18:04:38Z

@NullVoxPopuli rebased from ba8ee45

NullVoxPopuli · 2016-03-07T00:38:07Z

Hey, the checks passed! do we want to merge a partial feature, or wait? there are a few parts of the json api errors spec that I don't know how active record errors would translate (@bf4, you have notes on those in your PR Summary.

At the very least, I think it would be nice to clean up all the discussion on this.

bf4 · 2016-03-07T01:10:29Z

@NullVoxPopuli

At the very least, I think it would be nice to clean up all the discussion on this.

You mean what I wrote in the pr issue? The checkboxes describe what's not done and what can be done. (No one said we need to implement everything, or that it's even a good idea to.)

bf4 · 2016-03-07T01:11:21Z

@NullVoxPopuli

do we want to merge a partial feature,

This PR is done. It will be the basis of any future error feature prs.

…esn't break

bf4 · 2016-03-07T02:05:48Z

Updated docs and changelog

bf4 · 2016-03-07T02:06:07Z

Ready to merge if tests pass and docs look good

NullVoxPopuli · 2016-03-07T02:19:09Z

lib/active_model/serializer.rb

@@ -116,6 +118,10 @@ def initialize(object, options = {})
      end
    end

+    def success?


what's the purpose of this?

Sentinel for rendering success vs failurr doc. Search diff for usage

NullVoxPopuli · 2016-03-07T02:23:05Z

I think the docs look good.

NullVoxPopuli · 2016-03-07T02:28:06Z

Just needs squashing

bf4 · 2016-03-07T03:07:01Z

@NullVoxPopuli I wasn't planning on squashing conmits more than currently as I wanted to tell the story of how we got here. How would you squash them?

NullVoxPopuli · 2016-03-07T03:08:57Z

@bf4, fine by me. As long as the reason makes sense, I'm good with it.

JSON API Errors (Initial implementation and roadmap for full feature-set)

musaffa · 2016-03-07T03:47:16Z

❤️

NullVoxPopuli · 2016-03-07T19:20:02Z

@bf4 information here on the string format thing: jruby/jruby#3712

I guess it's just a runtime flag that was causing the issue. why do we have debug turned on in the jruby environments? it's not a huge deal regardless, I don't think, but I think it's good to log this issue for historical reasons.

bf4 · 2016-03-07T20:14:28Z

@NullVoxPopuli Thanks for the link. Those options are to make code coverage work. see https://github.com/colszowka/simplecov/blob/058dbba074d665c5ab1354582d5946fafd1c00e6/lib/simplecov.rb#L6-L15

NullVoxPopuli · 2016-03-07T20:16:43Z

gotchya -- also good to know. :-)

bf4 force-pushed the json_api_errors branch from 790a5c8 to 178dd56 Compare July 14, 2015 05:44

bf4 mentioned this pull request Jul 14, 2015

Return errors in jsonapi format RestPack/restpack_serializer#27

Open

bbuchalter reviewed Aug 5, 2015
View reviewed changes

This was referenced Aug 5, 2015

Example Error Objects json-api/json-api#828

Merged

Pagination Links #1041

Merged

bf4 force-pushed the json_api_errors branch from 178dd56 to f921acd Compare August 27, 2015 16:54

bf4 reviewed Aug 27, 2015
View reviewed changes

bf4 force-pushed the json_api_errors branch from f921acd to d048f20 Compare August 27, 2015 17:11

bf4 mentioned this pull request Aug 27, 2015

Arity not consistent on rbx vs. CRuby, JRuby rubinius/rubinius#3491

Closed

bf4 force-pushed the json_api_errors branch 2 times, most recently from 7db1c3a to 85f5c7c Compare August 27, 2015 18:16

bf4 reviewed Aug 28, 2015
View reviewed changes

NullVoxPopuli mentioned this pull request Mar 5, 2016

add an extra format token to the primary data string so that JRuby do… bf4/active_model_serializers#3

Merged

bf4 added 4 commits March 6, 2016 12:03

RFC: Json Api Errors (WIP)

0ba944d

- ActiveModelSerializers::JsonPointer - ActiveModel::Serializer::Adapter::JsonApi::Error - ActiveModel::Serializer::Adapter::JsonApi::Error.attributes - Fix rubocop config

Generalize detection of serializable resource with errors

dfe1626

Require explicit adapter/serializer to render JSON API errors

96107c5

- Separate collection errors from resource errors in adapter - Refactor to ErrorsSerializer; first-class json error methods - DOCS - Rails 4.0 requires assert exact exception class, boo

bf4 force-pushed the json_api_errors branch from ba8ee45 to 62a1b7e Compare March 6, 2016 18:03

bf4 force-pushed the json_api_errors branch from 62a1b7e to a210e99 Compare March 7, 2016 01:37

bf4 and others added 2 commits March 6, 2016 20:05

Update documentation with Yard links

e6ae34b

add an extra format token to the primary data string so that JRuby do…

d03db81

…esn't break

bf4 force-pushed the json_api_errors branch from a210e99 to d03db81 Compare March 7, 2016 02:05

NullVoxPopuli reviewed Mar 7, 2016
View reviewed changes

NullVoxPopuli added a commit that referenced this pull request Mar 7, 2016

Merge pull request #1004 from bf4/json_api_errors

79e6acb

JSON API Errors (Initial implementation and roadmap for full feature-set)

NullVoxPopuli merged commit 79e6acb into rails-api:master Mar 7, 2016

bf4 deleted the json_api_errors branch March 7, 2016 03:30

		@@ -0,0 +1,2 @@
		class ErrorSerializer < ActiveModel::Serializer
		end

[RFC] JSON API Errors (Initial implementation and roadmap for full feature-set) #1004

[RFC] JSON API Errors (Initial implementation and roadmap for full feature-set) #1004

Conversation

bf4 commented Jul 14, 2015

Usage

Error object:

Full Error API Implementation:

Implementation tests:

Other implementations

bf4 commented Jul 14, 2015

bbuchalter commented Jul 20, 2015

bf4 commented Jul 20, 2015

bbuchalter commented Jul 20, 2015

bf4 commented Jul 23, 2015

joaomdmoura commented Jul 31, 2015

bf4 commented Jul 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaomdmoura commented Aug 12, 2015

bf4 commented Aug 12, 2015

bf4 commented Aug 21, 2015

bf4 commented Aug 26, 2015

bf4 commented Aug 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Mar 6, 2016

NullVoxPopuli commented Mar 6, 2016

bf4 commented Mar 6, 2016

NullVoxPopuli commented Mar 7, 2016

bf4 commented Mar 7, 2016

bf4 commented Mar 7, 2016

bf4 commented Mar 7, 2016

bf4 commented Mar 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Mar 7, 2016

NullVoxPopuli commented Mar 7, 2016

bf4 commented Mar 7, 2016

NullVoxPopuli commented Mar 7, 2016

musaffa commented Mar 7, 2016

NullVoxPopuli commented Mar 7, 2016

bf4 commented Mar 7, 2016

NullVoxPopuli commented Mar 7, 2016