Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add custom error formatter #76

Merged
merged 15 commits into from
Oct 20, 2017
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -1,20 +1,48 @@
# This configuration was generated by `rubocop --auto-gen-config`
# on 2015-01-13 18:47:14 -0500 using RuboCop version 0.28.0.
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2017-10-09 10:22:52 -0500 using RuboCop version 0.41.2.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
# versions of RuboCop, may require this file to be generated again.

# Offense count: 25
# Configuration parameters: AllowURI, URISchemes.
# Offense count: 1
Metrics/AbcSize:
Max: 20
Max: 18

# Offense count: 7
# Offense count: 1
Metrics/CyclomaticComplexity:
Max: 9

# Offense count: 1
# Configuration parameters: AllowHeredoc, AllowURI, URISchemes.
# URISchemes: http, https
Metrics/LineLength:
Max: 87

# Offense count: 1
# Configuration parameters: CountComments.
Metrics/MethodLength:
Max: 18

# Offense count: 1
Metrics/PerceivedComplexity:
Max: 11

# Offense count: 5
Style/Documentation:
Enabled: false
Exclude:
- 'spec/**/*'
- 'test/**/*'
- 'lib/grape-active_model_serializers/endpoint_extension.rb'
- 'lib/grape-active_model_serializers/error_formatter.rb'
- 'lib/grape-active_model_serializers/formatter.rb'
- 'lib/grape-active_model_serializers/options_builder.rb'
- 'lib/grape-active_model_serializers/serializer_resolver.rb'

# Offense count: 2
# Configuration parameters: Exclude.
# Configuration parameters: ExpectMatchingDefinition, Regex, IgnoreExecutableScripts.
Style/FileName:
Enabled: false
Exclude:
- 'lib/grape-active_model_serializers.rb'
- 'spec/grape-active_model_serializers_spec.rb'
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

### 1.5.2 (Next)

* [#76](https://github.com/ruby-grape/grape-active_model_serializers/pull/76): Add custom error formatter - [@xn](https://github.com/xn).
* Your contribution here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this back.


### 1.5.1 (April 25, 2017)
Expand Down
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ source 'https://rubygems.org'

gemspec

case version = ENV['GRAPE_VERSION'] || '~> 0.10.0'
case version = ENV['GRAPE_VERSION'] || '~> 1.0.0'
when 'HEAD'
gem 'grape', github: 'intridea/grape'
else
Expand Down
1 change: 1 addition & 0 deletions grape-active_model_serializers.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ Gem::Specification.new do |gem|

gem.add_dependency 'grape', '>= 0.8.0'
gem.add_dependency 'active_model_serializers', '>= 0.10.0'
gem.add_dependency 'multi_json', '~> 1.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want this. Grape has a way to format JSON regardless of the library used, you can call it with ::Grape::Json.dump.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uninitialized constant Grape::Json

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then do you want to change the version of grape depended on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we implement the same thing?

Copy link
Member

@dblock dblock Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gem depends on grape, so you should be able to use it, I am not sure what the problem is, but maybe you need to make sure to depend on Grape >= 1.0 where this was added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this one should still be removed, it's unnecessary in all Grape versions since it's either coming via Grape or is not used..


gem.add_development_dependency 'listen', '~> 3.0.7'
gem.add_development_dependency 'rspec'
Expand Down
1 change: 1 addition & 0 deletions lib/grape-active_model_serializers.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require 'active_model_serializers'
require 'grape'
require 'grape-active_model_serializers/endpoint_extension'
require 'grape-active_model_serializers/error_formatter'
require 'grape-active_model_serializers/formatter'
require 'grape-active_model_serializers/serializer_resolver'
require 'grape-active_model_serializers/options_builder'
Expand Down
56 changes: 56 additions & 0 deletions lib/grape-active_model_serializers/error_formatter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
module Grape
module ErrorFormatter
module ActiveModelSerializers
extend Base

class << self
def call(message, backtrace, options = {}, env = nil, original_exception = nil)
result = if respond_to? :present
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, might be shorter and more concise, but not a must have.

message = present(message) if respond_to?(:present)
message = wrap_message(message)
message = ...

wrap_message(present(message, env))
else
wrap_message(message)
end

rescue_options = options[:rescue_options] || {}
if rescue_options[:backtrace] && backtrace && !backtrace.empty?
result = result.merge(backtrace: backtrace)
end
if rescue_options[:original_exception] && original_exception
result = result
.merge(original_exception: original_exception.inspect)
end
if ::Grape.const_defined? :Json
::Grape::Json.dump(result)
else
::MultiJson.dump(result)
end
end

private

def wrap_message(message)
if active_model?(message)
::ActiveModelSerializers::SerializableResource.new(
message,
serializer: ActiveModel::Serializer::ErrorSerializer
).as_json
elsif ok_to_pass_through?(message)
message
else
{ error: message }
end
end

def active_model?(message)
message.respond_to?(:errors) &&
message.errors.is_a?(ActiveModel::Errors)
end

def ok_to_pass_through?(message)
message.is_a?(Exceptions::ValidationErrors) ||
message.is_a?(Hash)
end
end
end
end
end
81 changes: 81 additions & 0 deletions spec/grape-active_model_serializers/error_formatter_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
require 'spec_helper'
require 'grape-active_model_serializers/error_formatter'

describe Grape::ErrorFormatter::ActiveModelSerializers do
subject { Grape::ErrorFormatter::ActiveModelSerializers }
let(:backtrace) { ['Line:1'] }
let(:options) { Hash.new }
let(:env) { { 'api.endpoint' => app.endpoints.first } }
let(:original_exception) { StandardError.new('oh noes!') }

let(:app) { Class.new(Grape::API) }

before do
ActiveModel::Serializer.config.adapter = :json_api
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pollutes future tests, so should either be set for all tests or unset in after.

app.format :json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this into the let, so

let(:app) {
   Class.new(Grape::API) do
      formatter ...
      ...
   end
}

app.formatter :jsonapi, Grape::Formatter::ActiveModelSerializers
app.error_formatter :jsonapi, Grape::ErrorFormatter::ActiveModelSerializers

app.namespace('space') do |ns|
ns.get('/', root: false) do
error!(message)
end
end
end

describe '#call' do
context 'message is an activemodel' do
let(:message) {
class Foo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This globally defines a class Foo. It should be a Class.new as well.

include ActiveModel::Model
attr_accessor :name
def initialize(attributes = {})
super
errors.add(:name, 'We don\'t like bears')
end
end
Foo.new(name: 'bar')
}
it 'formats the error' do
result = subject
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shared with the example below and should be moved to a let as well.

.call(message, backtrace, options, env, original_exception)
json_hash = JSON.parse(result)

expected_result = {
'errors' => [
{
'source' => {
'pointer' => '/data/attributes/name'
},
'detail' => 'We don\'t like bears'
}
]
}

expect(json_hash == expected_result).to eq(true)
end
end

context 'message is hash like' do
let(:message) { { 'errors' => ['error'] } }
it 'passes the message through' do
result = subject
.call(message, backtrace, options, env, original_exception)
json_hash = JSON.parse(result)

expect(json_hash == message).to eq(true)
end
end

context 'message is text' do
let(:message) { 'error' }
it 'wraps the error' do
result = subject
.call(message, backtrace, options, env, original_exception)
json_hash = JSON.parse(result)

expect(json_hash == { 'error' => message }).to eq(true)
end
end
end
end