diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d95cf9eec..54284527e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,11 +2,13 @@ #### Features +* [#2475](https://github.com/ruby-grape/grape/pull/2475): Remove Grape::Util::Registrable - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes * [#2471](https://github.com/ruby-grape/grape/pull/2471): Fix absence of original_exception and/or backtrace even if passed in error! - [@numbata](https://github.com/numbata). +* Your contribution here. ### 2.1.3 (2024-07-13) diff --git a/lib/grape/content_types.rb b/lib/grape/content_types.rb index cc0cc2cab1..336948b9bc 100644 --- a/lib/grape/content_types.rb +++ b/lib/grape/content_types.rb @@ -2,10 +2,10 @@ module Grape module ContentTypes - extend Util::Registrable + module_function # Content types are listed in order of preference. - CONTENT_TYPES = { + DEFAULTS = { xml: 'application/xml', serializable_hash: 'application/json', json: 'application/json', @@ -13,13 +13,18 @@ module ContentTypes txt: 'text/plain' }.freeze - class << self - def content_types_for_settings(settings) - settings&.inject(:merge!) - end + MIME_TYPES = Grape::ContentTypes::DEFAULTS.except(:serializable_hash).invert.freeze + + def content_types_for(from_settings) + from_settings.presence || DEFAULTS + end + + def mime_types_for(from_settings) + return MIME_TYPES if from_settings == Grape::ContentTypes::DEFAULTS - def content_types_for(from_settings) - from_settings.presence || Grape::ContentTypes::CONTENT_TYPES.merge(default_elements) + from_settings.each_with_object({}) do |(k, v), types_without_params| + # remove optional parameter + types_without_params[v.split(';', 2).first] = k end end end diff --git a/lib/grape/dsl/request_response.rb b/lib/grape/dsl/request_response.rb index 7e23c99560..7f0a0d27df 100644 --- a/lib/grape/dsl/request_response.rb +++ b/lib/grape/dsl/request_response.rb @@ -17,18 +17,16 @@ def default_format(new_format = nil) # Specify the format for the API's serializers. # May be `:json`, `:xml`, `:txt`, etc. def format(new_format = nil) - if new_format - namespace_inheritable(:format, new_format.to_sym) - # define the default error formatters - namespace_inheritable(:default_error_formatter, Grape::ErrorFormatter.formatter_for(new_format, **{})) - # define a single mime type - mime_type = content_types[new_format.to_sym] - raise Grape::Exceptions::MissingMimeType.new(new_format) unless mime_type - - namespace_stackable(:content_types, new_format.to_sym => mime_type) - else - namespace_inheritable(:format) - end + return namespace_inheritable(:format) unless new_format + + symbolic_new_format = new_format.to_sym + namespace_inheritable(:format, symbolic_new_format) + namespace_inheritable(:default_error_formatter, Grape::ErrorFormatter.formatter_for(symbolic_new_format)) + + content_type = content_types[symbolic_new_format] + raise Grape::Exceptions::MissingMimeType.new(new_format) unless content_type + + namespace_stackable(:content_types, symbolic_new_format => content_type) end # Specify a custom formatter for a content-type. @@ -43,12 +41,10 @@ def parser(content_type, new_parser) # Specify a default error formatter. def default_error_formatter(new_formatter_name = nil) - if new_formatter_name - new_formatter = Grape::ErrorFormatter.formatter_for(new_formatter_name, **{}) - namespace_inheritable(:default_error_formatter, new_formatter) - else - namespace_inheritable(:default_error_formatter) - end + return namespace_inheritable(:default_error_formatter) unless new_formatter_name + + new_formatter = Grape::ErrorFormatter.formatter_for(new_formatter_name) + namespace_inheritable(:default_error_formatter, new_formatter) end def error_formatter(format, options) diff --git a/lib/grape/error_formatter.rb b/lib/grape/error_formatter.rb index 4d76fe2962..7784055b6a 100644 --- a/lib/grape/error_formatter.rb +++ b/lib/grape/error_formatter.rb @@ -2,34 +2,22 @@ module Grape module ErrorFormatter - extend Util::Registrable + module_function - class << self - def builtin_formatters - @builtin_formatters ||= { - serializable_hash: Grape::ErrorFormatter::Json, - json: Grape::ErrorFormatter::Json, - jsonapi: Grape::ErrorFormatter::Json, - txt: Grape::ErrorFormatter::Txt, - xml: Grape::ErrorFormatter::Xml - } - end + DEFAULTS = { + serializable_hash: Grape::ErrorFormatter::Json, + json: Grape::ErrorFormatter::Json, + jsonapi: Grape::ErrorFormatter::Json, + txt: Grape::ErrorFormatter::Txt, + xml: Grape::ErrorFormatter::Xml + }.freeze - def formatters(**options) - builtin_formatters.merge(default_elements).merge!(options[:error_formatters] || {}) - end + def formatter_for(format, error_formatters = nil, default_error_formatter = nil) + select_formatter(error_formatters, format) || default_error_formatter || DEFAULTS[:txt] + end - def formatter_for(api_format, **options) - spec = formatters(**options)[api_format] - case spec - when nil - options[:default_error_formatter] || Grape::ErrorFormatter::Txt - when Symbol - method(spec) - else - spec - end - end + def select_formatter(error_formatters, format) + error_formatters&.key?(format) ? error_formatters[format] : DEFAULTS[format] end end end diff --git a/lib/grape/formatter.rb b/lib/grape/formatter.rb index 4a84f0e2be..d586b0bd6a 100644 --- a/lib/grape/formatter.rb +++ b/lib/grape/formatter.rb @@ -2,34 +2,24 @@ module Grape module Formatter - extend Util::Registrable + module_function - class << self - def builtin_formatters - @builtin_formatters ||= { - json: Grape::Formatter::Json, - jsonapi: Grape::Formatter::Json, - serializable_hash: Grape::Formatter::SerializableHash, - txt: Grape::Formatter::Txt, - xml: Grape::Formatter::Xml - } - end + DEFAULTS = { + json: Grape::Formatter::Json, + jsonapi: Grape::Formatter::Json, + serializable_hash: Grape::Formatter::SerializableHash, + txt: Grape::Formatter::Txt, + xml: Grape::Formatter::Xml + }.freeze - def formatters(**options) - builtin_formatters.merge(default_elements).merge!(options[:formatters] || {}) - end + DEFAULT_LAMBDA_FORMATTER = ->(obj, _env) { obj } - def formatter_for(api_format, **options) - spec = formatters(**options)[api_format] - case spec - when nil - ->(obj, _env) { obj } - when Symbol - method(spec) - else - spec - end - end + def formatter_for(api_format, formatters) + select_formatter(formatters, api_format) || DEFAULT_LAMBDA_FORMATTER + end + + def select_formatter(formatters, api_format) + formatters&.key?(api_format) ? formatters[api_format] : DEFAULTS[api_format] end end end diff --git a/lib/grape/middleware/base.rb b/lib/grape/middleware/base.rb index 2ee9cbeaea..6a54aa6d76 100644 --- a/lib/grape/middleware/base.rb +++ b/lib/grape/middleware/base.rb @@ -61,22 +61,20 @@ def response @app_response = Rack::Response.new(@app_response[2], @app_response[0], @app_response[1]) end - def content_type_for(format) - HashWithIndifferentAccess.new(content_types)[format] + def content_types + @content_types ||= Grape::ContentTypes.content_types_for(options[:content_types]) end - def content_types - ContentTypes.content_types_for(options[:content_types]) + def mime_types + @mime_types ||= Grape::ContentTypes.mime_types_for(content_types) end - def content_type - content_type_for(env[Grape::Env::API_FORMAT] || options[:format]) || TEXT_HTML + def content_type_for(format) + content_types_indifferent_access[format] end - def mime_types - @mime_types ||= content_types.each_pair.with_object({}) do |(k, v), types_without_params| - types_without_params[v.split(';').first] = k - end + def content_type + content_type_for(env[Grape::Env::API_FORMAT] || options[:format]) || TEXT_HTML end private @@ -89,6 +87,10 @@ def merge_headers(response) when Array then response[1].merge!(headers) end end + + def content_types_indifferent_access + @content_types_indifferent_access ||= content_types.with_indifferent_access + end end end end diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index 573db672cc..b798a19210 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -45,7 +45,7 @@ def rack_response(status, headers, message) def format_message(message, backtrace, original_exception = nil) format = env[Grape::Env::API_FORMAT] || options[:format] - formatter = Grape::ErrorFormatter.formatter_for(format, **options) + formatter = Grape::ErrorFormatter.formatter_for(format, options[:error_formatters], options[:default_error_formatter]) return formatter.call(message, backtrace, options, env, original_exception) if formatter throw :error, diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index 35a315001d..4de1af02ea 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -54,7 +54,7 @@ def build_formatted_response(status, headers, bodies) def fetch_formatter(headers, options) api_format = mime_types[headers[Rack::CONTENT_TYPE]] || env[Grape::Env::API_FORMAT] - Grape::Formatter.formatter_for(api_format, **options) + Grape::Formatter.formatter_for(api_format, options[:formatters]) end # Set the content type header for the API format if it is not already present. @@ -97,7 +97,7 @@ def read_rack_input(body) fmt = request.media_type ? mime_types[request.media_type] : options[:default_format] throw :error, status: 415, message: "The provided content-type '#{request.media_type}' is not supported." unless content_type_for(fmt) - parser = Grape::Parser.parser_for fmt, **options + parser = Grape::Parser.parser_for fmt, options[:parsers] if parser begin body = (env[Grape::Env::API_REQUEST_BODY] = parser.call(body, env)) diff --git a/lib/grape/parser.rb b/lib/grape/parser.rb index 3676a45a71..a446b4da20 100644 --- a/lib/grape/parser.rb +++ b/lib/grape/parser.rb @@ -2,32 +2,16 @@ module Grape module Parser - extend Util::Registrable + module_function - class << self - def builtin_parsers - @builtin_parsers ||= { - json: Grape::Parser::Json, - jsonapi: Grape::Parser::Json, - xml: Grape::Parser::Xml - } - end + DEFAULTS = { + json: Grape::Parser::Json, + jsonapi: Grape::Parser::Json, + xml: Grape::Parser::Xml + }.freeze - def parsers(**options) - builtin_parsers.merge(default_elements).merge!(options[:parsers] || {}) - end - - def parser_for(api_format, **options) - spec = parsers(**options)[api_format] - case spec - when nil - nil - when Symbol - method(spec) - else - spec - end - end + def parser_for(format, parsers = nil) + parsers&.key?(format) ? parsers[format] : DEFAULTS[format] end end end diff --git a/lib/grape/util/accept_header_handler.rb b/lib/grape/util/accept_header_handler.rb index c7fc7bee9a..3098f3a822 100644 --- a/lib/grape/util/accept_header_handler.rb +++ b/lib/grape/util/accept_header_handler.rb @@ -13,7 +13,7 @@ def initialize(accept_header:, versions:, **options) @cascade = options.fetch(:cascade, true) end - def match_best_quality_media_type!(content_types: Grape::ContentTypes::CONTENT_TYPES, allowed_methods: nil) + def match_best_quality_media_type!(content_types: Grape::ContentTypes::DEFAULTS, allowed_methods: nil) return unless vendor strict_header_checks! diff --git a/lib/grape/util/registrable.rb b/lib/grape/util/registrable.rb deleted file mode 100644 index e154456f8b..0000000000 --- a/lib/grape/util/registrable.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module Grape - module Util - module Registrable - def default_elements - @default_elements ||= {} - end - - def register(format, element) - default_elements[format] = element unless default_elements[format] - end - end - end -end diff --git a/spec/grape/content_types_spec.rb b/spec/grape/content_types_spec.rb new file mode 100644 index 0000000000..19040bbeb6 --- /dev/null +++ b/spec/grape/content_types_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +describe Grape::ContentTypes do + describe 'DEFAULTS' do + subject { described_class::DEFAULTS } + + let(:expected_value) do + { + xml: 'application/xml', + serializable_hash: 'application/json', + json: 'application/json', + binary: 'application/octet-stream', + txt: 'text/plain' + }.freeze + end + + it { is_expected.to eq(expected_value) } + end + + describe 'MIME_TYPES' do + subject { described_class::MIME_TYPES } + + let(:expected_value) do + { + 'application/xml' => :xml, + 'application/json' => :json, + 'application/octet-stream' => :binary, + 'text/plain' => :txt + }.freeze + end + + it { is_expected.to eq(expected_value) } + end + + describe '.content_types_for' do + subject { described_class.content_types_for(from_settings) } + + context 'when from_settings is present' do + let(:from_settings) { { a: :b } } + + it { is_expected.to eq(from_settings) } + end + + context 'when from_settings is not present' do + let(:from_settings) { nil } + + it { is_expected.to be(described_class::DEFAULTS) } + end + end + + describe '.mime_types_for' do + subject { described_class.mime_types_for(from_settings) } + + context 'when from_settings is equal to Grape::ContentTypes::DEFAULTS' do + let(:from_settings) do + { + xml: 'application/xml', + serializable_hash: 'application/json', + json: 'application/json', + binary: 'application/octet-stream', + txt: 'text/plain' + }.freeze + end + + it { is_expected.to be(described_class::MIME_TYPES) } + end + + context 'when from_settings is not equal to Grape::ContentTypes::DEFAULTS' do + let(:from_settings) do + { + xml: 'application/xml;charset=utf-8' + } + end + + it { is_expected.to eq('application/xml' => :xml) } + end + end +end diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb index 9b7dc9b56e..28ec6c9aa3 100644 --- a/spec/grape/middleware/formatter_spec.rb +++ b/spec/grape/middleware/formatter_spec.rb @@ -192,11 +192,7 @@ def to_xml end context 'with custom vendored content types' do - before do - subject.options[:content_types] = {}.tap do |ct| - ct[:custom] = 'application/vnd.test+json' - end - end + subject { described_class.new(app, content_types: { custom: 'application/vnd.test+json' }) } it 'uses the custom type' do subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test+json') @@ -227,18 +223,14 @@ def to_xml end it 'is set for custom' do - subject.options[:content_types] = {}.tap do |ct| - ct[:custom] = 'application/x-custom' - end - _, headers, = subject.call(Rack::PATH_INFO => '/info.custom') + s = described_class.new(app, content_types: { custom: 'application/x-custom' }) + _, headers, = s.call(Rack::PATH_INFO => '/info.custom') expect(headers[Rack::CONTENT_TYPE]).to eq('application/x-custom') end it 'is set for vendored with registered type' do - subject.options[:content_types] = {}.tap do |ct| - ct[:custom] = 'application/vnd.test+json' - end - _, headers, = subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test+json') + s = described_class.new(app, content_types: { custom: 'application/vnd.test+json' }) + _, headers, = s.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test+json') expect(headers[Rack::CONTENT_TYPE]).to eq('application/vnd.test+json') end @@ -250,10 +242,8 @@ def to_xml context 'format' do it 'uses custom formatter' do - subject.options[:content_types] = {} - subject.options[:content_types][:custom] = "don't care" - subject.options[:formatters][:custom] = ->(_obj, _env) { 'CUSTOM FORMAT' } - r = Rack::MockResponse[*subject.call(Rack::PATH_INFO => '/info.custom')] + s = described_class.new(app, content_types: { custom: "don't care" }, formatters: { custom: ->(_obj, _env) { 'CUSTOM FORMAT' } }) + r = Rack::MockResponse[*s.call(Rack::PATH_INFO => '/info.custom')] expect(r.body).to eq('CUSTOM FORMAT') end @@ -471,6 +461,8 @@ def to_xml end context 'inheritable formatters' do + subject { described_class.new(app, formatters: { invalid: invalid_formatter }, content_types: { invalid: 'application/x-invalid' }) } + let(:invalid_formatter) do Class.new do def self.call(_, _) @@ -481,22 +473,12 @@ def self.call(_, _) let(:app) { ->(_env) { [200, {}, ['']] } } let(:env) do - { Rack::PATH_INFO => '/hello.invalid', Grape::Http::Headers::HTTP_ACCEPT => 'application/x-invalid' } - end - - before do - Grape::Formatter.register :invalid, invalid_formatter - Grape::ContentTypes.register :invalid, 'application/x-invalid' - end - - after do - Grape::ContentTypes.default_elements.delete(:invalid) - Grape::Formatter.default_elements.delete(:invalid) + Rack::MockRequest.env_for('/hello.invalid', Grape::Http::Headers::HTTP_ACCEPT => 'application/x-invalid') end it 'returns response by invalid formatter' do r = Rack::MockResponse[*subject.call(env)] - expect(r.body).to eq(Grape::Json.dump('message' => 'invalid')) + expect(JSON.parse(r.body)).to eq('message' => 'invalid') end end diff --git a/spec/grape/parser_spec.rb b/spec/grape/parser_spec.rb index a3b43856f6..ecc5fdfa4d 100644 --- a/spec/grape/parser_spec.rb +++ b/spec/grape/parser_spec.rb @@ -3,77 +3,34 @@ describe Grape::Parser do subject { described_class } - describe '.builtin_parsers' do - it 'returns an instance of Hash' do - expect(subject.builtin_parsers).to be_an_instance_of(Hash) - end - - it 'includes json and xml parsers by default' do - expect(subject.builtin_parsers).to include(json: Grape::Parser::Json, xml: Grape::Parser::Xml) - end - end - - describe '.parsers' do - it 'returns an instance of Hash' do - expect(subject.parsers(**{})).to be_an_instance_of(Hash) - end + describe 'DEFAULTS' do + subject { described_class::DEFAULTS } - it 'includes built-in parsers' do - expect(subject.parsers(**{})).to include(subject.builtin_parsers) + let(:expected_defaults) do + { + json: Grape::Parser::Json, + jsonapi: Grape::Parser::Json, + xml: Grape::Parser::Xml + } end - context 'with :parsers option' do - let(:parsers) { { customized: Class.new } } - - it 'includes passed :parsers values' do - expect(subject.parsers(parsers: parsers)).to include(parsers) - end - end - - context 'with added parser by using `register` keyword' do - let(:added_parser) { Class.new } - - before { subject.register :added, added_parser } - - it 'includes added parser' do - expect(subject.parsers(**{})).to include(added: added_parser) - end - end + it { is_expected.to eq(expected_defaults) } end describe '.parser_for' do let(:options) { {} } - it 'calls .parsers' do - expect(subject).to receive(:parsers).with(any_args).and_return(subject.builtin_parsers) - subject.parser_for(:json, **options) - end - it 'returns parser correctly' do expect(subject.parser_for(:json)).to eq(Grape::Parser::Json) end context 'when parser is available' do - before { subject.register :customized_json, Grape::Parser::Json } - - it 'returns registered parser if available' do - expect(subject.parser_for(:customized_json)).to eq(Grape::Parser::Json) - end - end - - context 'when parser is an instance of Symbol' do - before do - allow(subject).to receive(:foo).and_return(:bar) - subject.register :foo, :foo + let(:parsers) do + { customized_json: Grape::Parser::Json } end - it 'returns an instance of Method' do - expect(subject.parser_for(:foo)).to be_an_instance_of(Method) - end - - it 'returns object which can be called' do - method = subject.parser_for(:foo) - expect(method.call).to eq(:bar) + it 'returns registered parser if available' do + expect(subject.parser_for(:customized_json, parsers)).to eq(Grape::Parser::Json) end end