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

Don't require multi_json and multi_xml. #1623

Merged
merged 3 commits into from Jun 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions .rubocop_todo.yml
@@ -1,19 +1,19 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2017-06-12 13:25:24 -0600 using RuboCop version 0.47.0.
# on 2017-06-13 12:09:49 -0400 using RuboCop version 0.47.0.
# 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: 44
# Offense count: 45
Metrics/AbcSize:
Max: 44

# Offense count: 277
# Offense count: 278
# Configuration parameters: CountComments, ExcludedMethods.
Metrics/BlockLength:
Max: 3104
Max: 3117

# Offense count: 8
# Configuration parameters: CountComments.
Expand All @@ -24,7 +24,7 @@ Metrics/ClassLength:
Metrics/CyclomaticComplexity:
Max: 14

# Offense count: 1098
# Offense count: 1108
# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
# URISchemes: http, https
Metrics/LineLength:
Expand Down
10 changes: 10 additions & 0 deletions .travis.yml
Expand Up @@ -17,6 +17,16 @@ matrix:
gemfile: gemfiles/rails_edge.gemfile
- rvm: 2.4.1
gemfile: gemfiles/rails_5.gemfile
- rvm: 2.4.1
gemfile: gemfiles/multi_json.gemfile
script:
- bundle exec rake
- bundle exec rspec spec/integration/multi_json
- rvm: 2.4.1
gemfile: gemfiles/multi_xml.gemfile
script:
- bundle exec rake
- bundle exec rspec spec/integration/multi_xml
- rvm: 2.3.4
gemfile: Gemfile
- rvm: 2.3.4
Expand Down
8 changes: 8 additions & 0 deletions Appraisals
Expand Up @@ -22,3 +22,11 @@ end
appraise 'rack-edge' do
gem 'rack', github: 'rack/rack'
end

appraise 'multi_json' do
gem 'multi_json', require: 'multi_json'
end

appraise 'multi_xml' do
gem 'multi_xml', require: 'multi_xml'
end
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -6,6 +6,7 @@
* [#1622](https://github.com/ruby-grape/grape/pull/1622): Add `except_values` validator to replace `except` option of `values` validator - [@jlfaber](https://github.com/jlfaber).
* [#1635](https://github.com/ruby-grape/grape/pull/1635): Instrument validators with ActiveSupport::Notifications - [@ktimothy](https://github.com/ktimothy).
* [#1646](https://github.com/ruby-grape/grape/pull/1646): Add ability to include an array of modules as helpers - [@pablonahuelgomez](https://github.com/pablonahuelgomez).
* [#1623](https://github.com/ruby-grape/grape/pull/1623): Removed `multi_json` and `multi_xml` dependencies - [@dblock](https://github.com/dblock).
* Your contribution here.

#### Fixes
Expand Down
7 changes: 6 additions & 1 deletion README.md
Expand Up @@ -69,6 +69,7 @@
- [CORS](#cors)
- [Content-type](#content-type)
- [API Data Formats](#api-data-formats)
- [JSON and XML Processors](#json-and-xml-processors)
- [RESTful Model Representations](#restful-model-representations)
- [Grape Entities](#grape-entities)
- [Hypermedia and Roar](#hypermedia-and-roar)
Expand Down Expand Up @@ -1146,7 +1147,7 @@ end
```

The `:values` option can also be supplied with a `Proc`, evaluated lazily with each request.
If the Proc has arity zero (i.e. it takes no arguments) it is expected to return either a list
If the Proc has arity zero (i.e. it takes no arguments) it is expected to return either a list
or a range which will then be used to validate the parameter.

For example, given a status model you may want to restrict by hashtags that you have
Expand Down Expand Up @@ -2564,6 +2565,10 @@ curl -X PUT -d 'data' 'http://localhost:9292/value' -H Content-Type:text/custom

You can disable parsing for a content-type with `nil`. For example, `parser :json, nil` will disable JSON parsing altogether. The request data is then available as-is in `env['api.request.body']`.

## JSON and XML Processors

Grape uses `JSON` and `ActiveSupport::XmlMini` for JSON and XML parsing by default. It also detects and supports [multi_json](https://github.com/intridea/multi_json) and [multi_xml](https://github.com/sferik/multi_xml). Adding those gems to your Gemfile and requiring them will enable them and allow you to swap the JSON and XML back-ends.

## RESTful Model Representations

Grape supports a range of ways to present your data with some help from a generic `present` method,
Expand Down
1 change: 1 addition & 0 deletions Rakefile
Expand Up @@ -7,6 +7,7 @@ Bundler::GemHelper.install_tasks
require 'rspec/core/rake_task'
RSpec::Core::RakeTask.new(:spec) do |spec|
spec.pattern = 'spec/**/*_spec.rb'
spec.exclude_pattern = 'spec/integration/**/*_spec.rb'
end

RSpec::Core::RakeTask.new(:rcov) do |spec|
Expand Down
10 changes: 10 additions & 0 deletions UPGRADING.md
Expand Up @@ -3,6 +3,16 @@ Upgrading Grape

### Upgrading to >= 1.0.0

#### Changes in XML and JSON Parsers

Grape no longer uses `multi_json` or `multi_xml` by default and uses `JSON` and `ActiveSupport::XmlMini` instead. This has no visible impact on JSON processing, but the default behavior of the XML parser has changed. For example, an XML POST containing `<user>Bobby T.</user>` was parsed as `Bobby T.` with `multi_xml`, and as now parsed as `{"__content__"=>"Bobby T."}` with `XmlMini`.

If you were using `MultiJson.load`, `MultiJson.dump` or `MultiXml.parse`, you can substitute those with `Grape::Json.load`, `Grape::Json.dump`, `::Grape::Xml.parse`, or directly with `JSON.load`, `JSON.dump`, `XmlMini.parse`, etc.

To restore previous behavior, add `multi_json` or `multi_xml` to your `Gemfile` and `require` it.

See [#1623](https://github.com/ruby-grape/grape/pull/1623) for more information.

#### Changes in Parameter Class

The default class for `params` has changed from `Hashie::Mash` to `ActiveSupport::HashWithIndifferentAccess` and the `hashie` dependency has been removed. This means that by default you can no longer access parameters by method name.
Expand Down
36 changes: 36 additions & 0 deletions gemfiles/multi_json.gemfile
@@ -0,0 +1,36 @@
# This file was generated by Appraisal

source 'https://rubygems.org'

gem 'multi_json', require: 'multi_json'

group :development, :test do
gem 'bundler'
gem 'rake'
gem 'rubocop', '0.47.0'
gem 'hashie'
end

group :development do
gem 'guard'
gem 'guard-rspec'
gem 'guard-rubocop'
gem 'yard'
gem 'appraisal'
gem 'benchmark-ips'
gem 'redcarpet'
end

group :test do
gem 'grape-entity', '~> 0.6'
gem 'maruku'
gem 'rack-test'
gem 'rspec', '~> 3.0'
gem 'cookiejar'
gem 'rack-jsonp', require: 'rack/jsonp'
gem 'mime-types'
gem 'ruby-grape-danger', '~> 0.1.0', require: false
gem 'coveralls', '~> 0.8.17', require: false
end

gemspec path: '../'
36 changes: 36 additions & 0 deletions gemfiles/multi_xml.gemfile
@@ -0,0 +1,36 @@
# This file was generated by Appraisal

source 'https://rubygems.org'

gem 'multi_xml', require: 'multi_xml'

group :development, :test do
gem 'bundler'
gem 'rake'
gem 'rubocop', '0.47.0'
gem 'hashie'
end

group :development do
gem 'guard'
gem 'guard-rspec'
gem 'guard-rubocop'
gem 'yard'
gem 'appraisal'
gem 'benchmark-ips'
gem 'redcarpet'
end

group :test do
gem 'grape-entity', '~> 0.6'
gem 'maruku'
gem 'rack-test'
gem 'rspec', '~> 3.0'
gem 'cookiejar'
gem 'rack-jsonp', require: 'rack/jsonp'
gem 'mime-types'
gem 'ruby-grape-danger', '~> 0.1.0', require: false
gem 'coveralls', '~> 0.8.17', require: false
end

gemspec path: '../'
2 changes: 0 additions & 2 deletions grape.gemspec
Expand Up @@ -16,8 +16,6 @@ Gem::Specification.new do |s|
s.add_runtime_dependency 'mustermann-grape', '~> 1.0.0'
s.add_runtime_dependency 'rack-accept'
s.add_runtime_dependency 'activesupport'
s.add_runtime_dependency 'multi_json', '>= 1.3.2'
s.add_runtime_dependency 'multi_xml', '>= 0.5.2'
s.add_runtime_dependency 'virtus', '>= 1.0.0'
s.add_runtime_dependency 'builder'

Expand Down
4 changes: 2 additions & 2 deletions lib/grape.rb
Expand Up @@ -16,8 +16,6 @@
require 'active_support/core_ext/hash/conversions'
require 'active_support/dependencies/autoload'
require 'active_support/notifications'
require 'multi_json'
require 'multi_xml'
require 'i18n'
require 'thread'

Expand All @@ -44,6 +42,8 @@ module Grape
autoload :Parser
autoload :Request
autoload :Env, 'grape/util/env'
autoload :Json, 'grape/util/json'
autoload :Xml, 'grape/util/xml'
end

module Http
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/error_formatter/json.rb
Expand Up @@ -10,7 +10,7 @@ def call(message, backtrace, options = {}, env = nil)
if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty?
result = result.merge(backtrace: backtrace)
end
MultiJson.dump(result)
::Grape::Json.dump(result)
end

private
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/error_formatter/txt.rb
Expand Up @@ -7,7 +7,7 @@ class << self
def call(message, backtrace, options = {}, env = nil)
message = present(message, env)

result = message.is_a?(Hash) ? MultiJson.dump(message) : message
result = message.is_a?(Hash) ? ::Grape::Json.dump(message) : message
if (options[:rescue_options] || {})[:backtrace] && backtrace && !backtrace.empty?
result += "\r\n "
result += backtrace.join("\r\n ")
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/formatter/json.rb
Expand Up @@ -4,7 +4,7 @@ module Json
class << self
def call(object, _env)
return object.to_json if object.respond_to?(:to_json)
MultiJson.dump(object)
::Grape::Json.dump(object)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/grape/formatter/serializable_hash.rb
Expand Up @@ -4,9 +4,9 @@ module SerializableHash
class << self
def call(object, _env)
return object if object.is_a?(String)
return MultiJson.dump(serialize(object)) if serializable?(object)
return ::Grape::Json.dump(serialize(object)) if serializable?(object)
return object.to_json if object.respond_to?(:to_json)
MultiJson.dump(object)
::Grape::Json.dump(object)
end

private
Expand Down
4 changes: 2 additions & 2 deletions lib/grape/parser/json.rb
Expand Up @@ -3,8 +3,8 @@ module Parser
module Json
class << self
def call(object, _env)
MultiJson.load(object)
rescue MultiJson::ParseError
::Grape::Json.load(object)
rescue ::Grape::Json::ParseError
# handle JSON parsing errors via the rescue handlers or provide error message
raise Grape::Exceptions::InvalidMessageBody, 'application/json'
end
Expand Down
4 changes: 2 additions & 2 deletions lib/grape/parser/xml.rb
Expand Up @@ -3,8 +3,8 @@ module Parser
module Xml
class << self
def call(object, _env)
MultiXml.parse(object)
rescue MultiXml::ParseError
::Grape::Xml.parse(object)
rescue ::Grape::Xml::ParseError
# handle XML parsing errors via the rescue handlers or provide error message
raise Grape::Exceptions::InvalidMessageBody, 'application/xml'
end
Expand Down
8 changes: 8 additions & 0 deletions lib/grape/util/json.rb
@@ -0,0 +1,8 @@
module Grape
if Object.const_defined? :MultiJson
Json = ::MultiJson
else
Json = ::JSON
Json::ParseError = Json::ParserError
end
end
Copy link
Contributor

@namusyaka namusyaka Jun 9, 2017

Choose a reason for hiding this comment

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

I'd like to declare require 'json' explicitly instead of trying to require it here.

Copy link
Member Author

@dblock dblock Jun 12, 2017

Choose a reason for hiding this comment

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

What do you mean exactly? That the user has to require 'multi_json' explicitly instead of trying to require and catch LoadError here? That's a bit tricky because that forces you to include multi_json before Grape, doesn't it?

8 changes: 8 additions & 0 deletions lib/grape/util/xml.rb
@@ -0,0 +1,8 @@
module Grape
if Object.const_defined? :MultiXml
Xml = ::MultiXml
else
Xml = ::ActiveSupport::XmlMini
Xml::ParseError = StandardError
end
Copy link
Contributor

@namusyaka namusyaka Jun 9, 2017

Choose a reason for hiding this comment

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

Likewise, I'd like to declare require 'active_support/xml_mini' explicitly.
At the moment we don't declare require 'active_support' itself, so autoload won't be worked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or are you saying that I should add require 'active_support/xml_mini' here after else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Do I understand things correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, take a look at the PR as now, any issues? Thank you.

end
6 changes: 3 additions & 3 deletions spec/grape/api/invalid_format_spec.rb
Expand Up @@ -27,17 +27,17 @@ def app
it 'no format' do
get '/foo'
expect(last_response.status).to eq 200
expect(last_response.body).to eq(MultiJson.dump(id: 'foo', format: nil))
expect(last_response.body).to eq(::Grape::Json.dump(id: 'foo', format: nil))
end
it 'json format' do
get '/foo.json'
expect(last_response.status).to eq 200
expect(last_response.body).to eq(MultiJson.dump(id: 'foo', format: 'json'))
expect(last_response.body).to eq(::Grape::Json.dump(id: 'foo', format: 'json'))
end
it 'invalid format' do
get '/foo.invalid'
expect(last_response.status).to eq 200
expect(last_response.body).to eq(MultiJson.dump(id: 'foo', format: 'invalid'))
expect(last_response.body).to eq(::Grape::Json.dump(id: 'foo', format: 'invalid'))
end
end
end