diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e7b60c49..d4f97ff65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ * [#2582](https://github.com/ruby-grape/grape/pull/2582): Fix leaky slash when normalizing - [@ericproulx](https://github.com/ericproulx). * [#2583](https://github.com/ruby-grape/grape/pull/2583): Optimize api parameter documentation and memory usage - [@ericproulx](https://github.com/ericproulx). * [#2589](https://github.com/ruby-grape/grape/pull/2589): Replace `send` by `__send__` in codebase - [@ericproulx](https://github.com/ericproulx). +* [#2598](https://github.com/ruby-grape/grape/pull/2598): Refactor settings DSL to use explicit methods instead of dynamic generation - [@ericproulx](https://github.com/ericproulx). +* [#2599](https://github.com/ruby-grape/grape/pull/2599): Simplify settings DSL get_or_set method and optimize logger implementation - [@ericproulx](https://github.com/ericproulx). +* [#2600](https://github.com/ruby-grape/grape/pull/2600): Refactor versioner middleware: simplify base class and improve consistency - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 3c77ae398..241100a88 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -232,22 +232,19 @@ def collect_route_config_per_pattern(all_routes) end end + ROOT_PREFIX_VERSIONING_KEY = %i[version version_options root_prefix].freeze + private_constant :ROOT_PREFIX_VERSIONING_KEY + # Allows definition of endpoints that ignore the versioning configuration # used by the rest of your API. def without_root_prefix_and_versioning - old_version = self.class.namespace_inheritable(:version) - old_version_options = self.class.namespace_inheritable(:version_options) - old_root_prefix = self.class.namespace_inheritable(:root_prefix) - - self.class.namespace_inheritable_to_nil(:version) - self.class.namespace_inheritable_to_nil(:version_options) - self.class.namespace_inheritable_to_nil(:root_prefix) - + inheritable_setting = self.class.inheritable_setting + deleted_values = inheritable_setting.namespace_inheritable.delete(*ROOT_PREFIX_VERSIONING_KEY) yield - - self.class.namespace_inheritable(:version, old_version) - self.class.namespace_inheritable(:version_options, old_version_options) - self.class.namespace_inheritable(:root_prefix, old_root_prefix) + ensure + ROOT_PREFIX_VERSIONING_KEY.each_with_index do |key, index| + inheritable_setting.namespace_inheritable[key] = deleted_values[index] + end end end end diff --git a/lib/grape/dsl/logger.rb b/lib/grape/dsl/logger.rb index c0b38b6f1..5962f0fed 100644 --- a/lib/grape/dsl/logger.rb +++ b/lib/grape/dsl/logger.rb @@ -7,10 +7,11 @@ module Logger # method will create a new one, logging to stdout. # @param logger [Object] the new logger to use def logger(logger = nil) + global_settings = inheritable_setting.global if logger - global_setting(:logger, logger) + global_settings[:logger] = logger else - global_setting(:logger) || global_setting(:logger, ::Logger.new($stdout)) + global_settings[:logger] || global_settings[:logger] = ::Logger.new($stdout) end end end diff --git a/lib/grape/dsl/settings.rb b/lib/grape/dsl/settings.rb index 415e4237f..81a9c98a6 100644 --- a/lib/grape/dsl/settings.rb +++ b/lib/grape/dsl/settings.rb @@ -26,64 +26,32 @@ def inheritable_setting @inheritable_setting ||= Grape::Util::InheritableSetting.new.tap { |new_settings| new_settings.inherit_from top_level_setting } end - # @param type [Symbol] - # @param key [Symbol] - def unset(type, key) - setting = inheritable_setting.__send__(type) - setting.delete key + def namespace_inheritable(key, value = nil) + get_or_set(inheritable_setting.namespace_inheritable, key, value) end - # @param type [Symbol] - # @param key [Symbol] - # @param value [Object] will be stored if the value is currently empty - # @return either the old value, if it wasn't nil, or the given value - def get_or_set(type, key, value) - setting = inheritable_setting.__send__(type) - if value.nil? - setting[key] - else - setting[key] = value - end + def namespace_stackable(key, value = nil) + get_or_set(inheritable_setting.namespace_stackable, key, value) end - # defines the following methods: - # - namespace_inheritable - # - namespace_stackable - - %i[namespace_inheritable namespace_stackable].each do |method_name| - define_method method_name do |key, value = nil| - get_or_set method_name, key, value - end + def global_setting(key, value = nil) + get_or_set(inheritable_setting.global, key, value) end - def unset_namespace_stackable(*keys) - keys.each do |key| - unset :namespace_stackable, key - end + def route_setting(key, value = nil) + get_or_set(inheritable_setting.route, key, value) end - # defines the following methods: - # - global_setting - # - route_setting - # - namespace_setting - - %i[global route namespace].each do |method_name| - define_method :"#{method_name}_setting" do |key, value = nil| - get_or_set method_name, key, value - end - end - - # @param key [Symbol] - def namespace_inheritable_to_nil(key) - inheritable_setting.namespace_inheritable[key] = nil + def namespace_setting(key, value = nil) + get_or_set(inheritable_setting.namespace, key, value) end def namespace_reverse_stackable(key, value = nil) - get_or_set :namespace_reverse_stackable, key, value + get_or_set(inheritable_setting.namespace_reverse_stackable, key, value) end def namespace_stackable_with_hash(key) - settings = get_or_set :namespace_stackable, key, nil + settings = namespace_stackable(key) return if settings.blank? settings.each_with_object({}) { |value, result| result.deep_merge!(value) } @@ -107,6 +75,12 @@ def within_namespace result end + + def get_or_set(setting, key, value) + return setting[key] if value.nil? + + setting[key] = value + end end end end diff --git a/lib/grape/dsl/validations.rb b/lib/grape/dsl/validations.rb index 6beb8f37a..e90f5af0b 100644 --- a/lib/grape/dsl/validations.rb +++ b/lib/grape/dsl/validations.rb @@ -22,7 +22,7 @@ module Validations # # whatever # end def reset_validations! - unset_namespace_stackable :declared_params, :params, :validations + inheritable_setting.namespace_stackable.delete(:declared_params, :params, :validations) end # Opens a root-level ParamsScope, defining parameter coercions and diff --git a/lib/grape/middleware/versioner/accept_version_header.rb b/lib/grape/middleware/versioner/accept_version_header.rb index 1d24388f1..7d3f72167 100644 --- a/lib/grape/middleware/versioner/accept_version_header.rb +++ b/lib/grape/middleware/versioner/accept_version_header.rb @@ -19,7 +19,7 @@ module Versioner class AcceptVersionHeader < Base def before potential_version = env['HTTP_ACCEPT_VERSION'].try(:scrub) - not_acceptable!('Accept-Version header must be set.') if strict? && potential_version.blank? + not_acceptable!('Accept-Version header must be set.') if strict && potential_version.blank? return if potential_version.blank? diff --git a/lib/grape/middleware/versioner/base.rb b/lib/grape/middleware/versioner/base.rb index 8662d2a88..d56d8c890 100644 --- a/lib/grape/middleware/versioner/base.rb +++ b/lib/grape/middleware/versioner/base.rb @@ -6,67 +6,49 @@ module Versioner class Base < Grape::Middleware::Base DEFAULT_OPTIONS = { pattern: /.*/i, + prefix: nil, + mount_path: nil, version_options: { strict: false, cascade: true, - parameter: 'apiver' + parameter: 'apiver', + vendor: nil }.freeze }.freeze - def self.inherited(klass) - super - Versioner.register(klass) - end - - def versions - options[:versions] - end - - def prefix - options[:prefix] - end - - def mount_path - options[:mount_path] - end + CASCADE_PASS_HEADER = { 'X-Cascade' => 'pass' }.freeze - def pattern - options[:pattern] + DEFAULT_OPTIONS.each_key do |key| + define_method key do + options[key] + end end - def version_options - options[:version_options] + DEFAULT_OPTIONS[:version_options].each_key do |key| + define_method key do + options[:version_options][key] + end end - def strict? - version_options[:strict] - end - - # By default those errors contain an `X-Cascade` header set to `pass`, which allows nesting and stacking - # of routes (see Grape::Router) for more information). To prevent - # this behavior, and not add the `X-Cascade` header, one can set the `:cascade` option to `false`. - def cascade? - version_options[:cascade] - end - - def parameter_key - version_options[:parameter] + def self.inherited(klass) + super + Versioner.register(klass) end - def vendor - version_options[:vendor] - end + attr_reader :error_headers, :versions - def error_headers - cascade? ? { 'X-Cascade' => 'pass' } : {} + def initialize(app, **options) + super + @error_headers = cascade ? CASCADE_PASS_HEADER : {} + @versions = options[:versions]&.map(&:to_s) # making sure versions are strings to ease potential match end def potential_version_match?(potential_version) - versions.blank? || versions.any? { |v| v.to_s == potential_version } + versions.blank? || versions.include?(potential_version) end def version_not_found! - throw :error, status: 404, message: '404 API Version Not Found', headers: { 'X-Cascade' => 'pass' } + throw :error, status: 404, message: '404 API Version Not Found', headers: CASCADE_PASS_HEADER end end end diff --git a/lib/grape/middleware/versioner/header.rb b/lib/grape/middleware/versioner/header.rb index 5470fe170..ecaf72ed7 100644 --- a/lib/grape/middleware/versioner/header.rb +++ b/lib/grape/middleware/versioner/header.rb @@ -53,7 +53,7 @@ def accept_header end def strict_header_checks! - return unless strict? + return unless strict accept_header_check! version_and_vendor_check! diff --git a/lib/grape/middleware/versioner/param.rb b/lib/grape/middleware/versioner/param.rb index 102d30c55..6fd6cc90e 100644 --- a/lib/grape/middleware/versioner/param.rb +++ b/lib/grape/middleware/versioner/param.rb @@ -20,11 +20,11 @@ module Versioner # env['api.version'] => 'v1' class Param < Base def before - potential_version = query_params[parameter_key] + potential_version = query_params[parameter] return if potential_version.blank? version_not_found! unless potential_version_match?(potential_version) - env[Grape::Env::API_VERSION] = env[Rack::RACK_REQUEST_QUERY_HASH].delete(parameter_key) + env[Grape::Env::API_VERSION] = env[Rack::RACK_REQUEST_QUERY_HASH].delete(parameter) end end end diff --git a/lib/grape/util/base_inheritable.rb b/lib/grape/util/base_inheritable.rb index cc68ababf..c70ad20f1 100644 --- a/lib/grape/util/base_inheritable.rb +++ b/lib/grape/util/base_inheritable.rb @@ -14,8 +14,11 @@ def initialize(inherited_values = nil) @new_values = {} end - def delete(key) - new_values.delete key + def delete(*keys) + keys.map do |key| + # since delete returns the deleted value, seems natural to `map` the result + new_values.delete key + end end def initialize_copy(other) diff --git a/spec/grape/dsl/logger_spec.rb b/spec/grape/dsl/logger_spec.rb index 3b135c48e..ead059ac5 100644 --- a/spec/grape/dsl/logger_spec.rb +++ b/spec/grape/dsl/logger_spec.rb @@ -4,17 +4,7 @@ let(:dummy_logger) do Class.new do extend Grape::DSL::Logger - def self.global_setting(key, value = nil) - if value - global_setting_hash[key] = value - else - global_setting_hash[key] - end - end - - def self.global_setting_hash - @global_setting_hash ||= {} - end + extend Grape::DSL::Settings end end diff --git a/spec/grape/dsl/settings_spec.rb b/spec/grape/dsl/settings_spec.rb index 1a475a764..ecdf5a85b 100644 --- a/spec/grape/dsl/settings_spec.rb +++ b/spec/grape/dsl/settings_spec.rb @@ -15,57 +15,29 @@ def reset_validations!; end end end - describe '#unset' do - it 'deletes a key from settings' do - subject.namespace_setting :dummy, 1 - expect(subject.inheritable_setting.namespace.keys).to include(:dummy) - - subject.unset :namespace, :dummy - expect(subject.inheritable_setting.namespace.keys).not_to include(:dummy) - end - end - - describe '#get_or_set' do - it 'sets a values' do - subject.get_or_set :namespace, :dummy, 1 - expect(subject.namespace_setting(:dummy)).to eq 1 - end - - it 'returns a value when nil is new value is provided' do - subject.get_or_set :namespace, :dummy, 1 - expect(subject.get_or_set(:namespace, :dummy, nil)).to eq 1 - end - end - describe '#global_setting' do - it 'delegates to get_or_set' do - expect(subject).to receive(:get_or_set).with(:global, :dummy, 1) - subject.global_setting(:dummy, 1) + it 'sets a value globally' do + subject.global_setting :some_thing, :foo_bar + expect(subject.global_setting(:some_thing)).to eq :foo_bar + subject.with_namespace do + subject.global_setting :some_thing, :foo_bar_baz + expect(subject.global_setting(:some_thing)).to eq :foo_bar_baz + end + expect(subject.global_setting(:some_thing)).to eq(:foo_bar_baz) end end describe '#route_setting' do - it 'delegates to get_or_set' do - expect(subject).to receive(:get_or_set).with(:route, :dummy, 1) - subject.route_setting(:dummy, 1) - end - - it 'sets a value until the next route' do - subject.route_setting :some_thing, :foo_bar - expect(subject.route_setting(:some_thing)).to eq :foo_bar - - subject.inheritable_setting.route_end - + it 'sets a value until the end of a namespace' do + subject.with_namespace do + subject.route_setting :some_thing, :foo_bar + expect(subject.route_setting(:some_thing)).to eq :foo_bar + end expect(subject.route_setting(:some_thing)).to be_nil end end describe '#namespace_setting' do - it 'delegates to get_or_set' do - expect(subject).to receive(:get_or_set).with(:namespace, :dummy, 1) - subject.namespace_setting(:dummy, 1) - end - it 'sets a value until the end of a namespace' do subject.with_namespace do subject.namespace_setting :some_thing, :foo_bar @@ -88,11 +60,6 @@ def reset_validations!; end end describe '#namespace_inheritable' do - it 'delegates to get_or_set' do - expect(subject).to receive(:get_or_set).with(:namespace_inheritable, :dummy, 1) - subject.namespace_inheritable(:dummy, 1) - end - it 'inherits values from surrounding namespace' do subject.with_namespace do subject.namespace_inheritable(:some_thing, :foo_bar) @@ -108,11 +75,6 @@ def reset_validations!; end end describe '#namespace_stackable' do - it 'delegates to get_or_set' do - expect(subject).to receive(:get_or_set).with(:namespace_stackable, :dummy, 1) - subject.namespace_stackable(:dummy, 1) - end - it 'stacks values from surrounding namespace' do subject.with_namespace do subject.namespace_stackable(:some_thing, :foo_bar) @@ -126,13 +88,6 @@ def reset_validations!; end end end - describe '#unset_namespace_stackable' do - it 'delegates to unset' do - expect(subject).to receive(:unset).with(:namespace_stackable, :dummy) - subject.unset_namespace_stackable(:dummy) - end - end - describe 'complex scenario' do it 'plays well' do obj1 = dummy_class.new diff --git a/spec/grape/dsl/validations_spec.rb b/spec/grape/dsl/validations_spec.rb index 93a55191d..301ac287c 100644 --- a/spec/grape/dsl/validations_spec.rb +++ b/spec/grape/dsl/validations_spec.rb @@ -5,17 +5,23 @@ let(:dummy_class) do Class.new do + extend Grape::DSL::Settings extend Grape::DSL::Validations - def self.unset_namespace_stackable(*_keys); end end end describe '.reset_validations' do subject { dummy_class.reset_validations! } + before do + %i[declared_params params validations other].each do |key| + dummy_class.inheritable_setting.namespace_stackable[key] = key + end + end + it 'calls unset_namespace_stackable properly' do - expect(dummy_class).to receive(:unset_namespace_stackable).with(:declared_params, :params, :validations) subject + expect(dummy_class.inheritable_setting.namespace_stackable.to_hash).to eq(other: [:other]) end end