From 64235ad58ce5240a4f26e11b5608f23464c8912a Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 15:51:55 +0100 Subject: [PATCH 01/19] use `to_enum` instead of including `Enumerable` to attributes_iterator.rb and validation_errors.rb --- lib/grape/exceptions/validation_errors.rb | 23 ++++++++++---------- lib/grape/validations/attributes_iterator.rb | 4 ++-- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/lib/grape/exceptions/validation_errors.rb b/lib/grape/exceptions/validation_errors.rb index 8859d579b1..695650e6d9 100644 --- a/lib/grape/exceptions/validation_errors.rb +++ b/lib/grape/exceptions/validation_errors.rb @@ -6,8 +6,6 @@ class ValidationErrors < Base ERRORS_FORMAT_KEY = 'grape.errors.format' DEFAULT_ERRORS_FORMAT = '%s %s' - include Enumerable - attr_reader :errors def initialize(errors: [], headers: {}) @@ -16,6 +14,8 @@ def initialize(errors: [], headers: {}) end def each + return to_enum(:each) unless block_given? + errors.each_pair do |attribute, errors| errors.each do |error| yield attribute, error @@ -37,16 +37,17 @@ def to_json(*_opts) end def full_messages - messages = map do |attributes, error| - I18n.t( - ERRORS_FORMAT_KEY, - default: DEFAULT_ERRORS_FORMAT, - attributes: translate_attributes(attributes), - message: error.message - ) + [].tap do |messages| + each do |attributes, error| + messages << + I18n.t(ERRORS_FORMAT_KEY, + default: DEFAULT_ERRORS_FORMAT, + attributes: translate_attributes(attributes), + message: error.message + ) + end + messages.uniq! end - messages.uniq! - messages end end end diff --git a/lib/grape/validations/attributes_iterator.rb b/lib/grape/validations/attributes_iterator.rb index 97d7f0280d..a184aafdf7 100644 --- a/lib/grape/validations/attributes_iterator.rb +++ b/lib/grape/validations/attributes_iterator.rb @@ -3,8 +3,6 @@ module Grape module Validations class AttributesIterator - include Enumerable - attr_reader :scope def initialize(validator, scope, params) @@ -15,6 +13,8 @@ def initialize(validator, scope, params) end def each(&block) + return to_enum(:each) unless block + do_each(@params, &block) # because we need recursion for nested arrays end From d58082cde5229674fc49586af19122ba937b0614 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 15:52:09 +0100 Subject: [PATCH 02/19] Remove unused build_coercer.rb --- lib/grape/validations/types/build_coercer.rb | 92 -------------------- 1 file changed, 92 deletions(-) delete mode 100644 lib/grape/validations/types/build_coercer.rb diff --git a/lib/grape/validations/types/build_coercer.rb b/lib/grape/validations/types/build_coercer.rb deleted file mode 100644 index 5f15a1a0cb..0000000000 --- a/lib/grape/validations/types/build_coercer.rb +++ /dev/null @@ -1,92 +0,0 @@ -# frozen_string_literal: true - -module Grape - module Validations - module Types - module BuildCoercer - # Chooses the best coercer for the given type. For example, if the type - # is Integer, it will return a coercer which will be able to coerce a value - # to the integer. - # - # There are a few very special coercers which might be returned. - # - # +Grape::Types::MultipleTypeCoercer+ is a coercer which is returned when - # the given type implies values in an array with different types. - # For example, +[Integer, String]+ allows integer and string values in - # an array. - # - # +Grape::Types::CustomTypeCoercer+ is a coercer which is returned when - # a method is specified by a user with +coerce_with+ option or the user - # specifies a custom type which implements requirments of - # +Grape::Types::CustomTypeCoercer+. - # - # +Grape::Types::CustomTypeCollectionCoercer+ is a very similar to the - # previous one, but it expects an array or set of values having a custom - # type implemented by the user. - # - # There is also a group of custom types implemented by Grape, check - # +Grape::Validations::Types::SPECIAL+ to get the full list. - # - # @param type [Class] the type to which input strings - # should be coerced - # @param method [Class,#call] the coercion method to use - # @return [Object] object to be used - # for coercion and type validation - def self.build_coercer(type, method: nil, strict: false) - cache_instance(type, method, strict) do - create_coercer_instance(type, method, strict) - end - end - - def self.create_coercer_instance(type, method, strict) - # Maps a custom type provided by Grape, it doesn't map types wrapped by collections!!! - type = Types.map_special(type) - - # Use a special coercer for multiply-typed parameters. - if Types.multiple?(type) - MultipleTypeCoercer.new(type, method) - - # Use a special coercer for custom types and coercion methods. - elsif method || Types.custom?(type) - CustomTypeCoercer.new(type, method) - - # Special coercer for collections of types that implement a parse method. - # CustomTypeCoercer (above) already handles such types when an explicit coercion - # method is supplied. - elsif Types.collection_of_custom?(type) - Types::CustomTypeCollectionCoercer.new( - Types.map_special(type.first), type.is_a?(Set) - ) - else - DryTypeCoercer.coercer_instance_for(type, strict) - end - end - - def self.cache_instance(type, method, strict, &_block) - key = cache_key(type, method, strict) - - return @__cache[key] if @__cache.key?(key) - - instance = yield - - @__cache_write_lock.synchronize do - @__cache[key] = instance - end - - instance - end - - def self.cache_key(type, method, strict) - [type, method, strict].each_with_object(+'_') do |val, memo| - next if val.nil? - - memo << '_' << val.to_s - end - end - - instance_variable_set(:@__cache, {}) - instance_variable_set(:@__cache_write_lock, Mutex.new) - end - end - end -end From 5bc642ed5596058a514472ed76d18c6fc1049bef Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 15:52:34 +0100 Subject: [PATCH 03/19] Remove const_missing in api.rb --- lib/grape/api.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lib/grape/api.rb b/lib/grape/api.rb index 2a3435aa38..8ce789c25b 100644 --- a/lib/grape/api.rb +++ b/lib/grape/api.rb @@ -78,15 +78,6 @@ def call(...) instance_for_rack.call(...) end - # Alleviates problems with autoloading by tring to search for the constant - def const_missing(*args) - if base_instance.const_defined?(*args) - base_instance.const_get(*args) - else - super - end - end - # The remountable class can have a configuration hash to provide some dynamic class-level variables. # For instance, a description could be done using: `desc configuration[:description]` if it may vary # depending on where the endpoint is mounted. Use with care, if you find yourself using configuration From 1300722ecea112deed762eed8affcc96745ff6c1 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 15:53:35 +0100 Subject: [PATCH 04/19] Add Grape::Util::Registry Add deregister module in spec --- lib/grape/util/registry.rb | 18 ++++++++++++++++++ spec/spec_helper.rb | 2 ++ spec/support/deregister.rb | 7 +++++++ 3 files changed, 27 insertions(+) create mode 100644 lib/grape/util/registry.rb create mode 100644 spec/support/deregister.rb diff --git a/lib/grape/util/registry.rb b/lib/grape/util/registry.rb new file mode 100644 index 0000000000..38f2b00c12 --- /dev/null +++ b/lib/grape/util/registry.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Grape + module Util + module Registry + def register(short_name, klass) + warn "#{short_name} is already registered with class #{klass}" if registry.key?(short_name) + registry[short_name] = klass + end + + private + + def registry + @registry ||= {}.with_indifferent_access + end + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 1589a0881d..26d3d4aa58 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -17,6 +17,8 @@ # so it should be set to true here as well to reflect that. I18n.enforce_available_locales = true +Grape::Util::Registry.include(Deregister) + RSpec.configure do |config| config.include Rack::Test::Methods config.include Spec::Support::Helpers diff --git a/spec/support/deregister.rb b/spec/support/deregister.rb new file mode 100644 index 0000000000..f5dc7e5b23 --- /dev/null +++ b/spec/support/deregister.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module Deregister + def deregister(key) + registry.delete(key) + end +end From 59a841987b29e7641d836b8bf587f27c404a862e Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 15:54:45 +0100 Subject: [PATCH 05/19] Add Grape::Parser::Base and use Grape::Util::Registry --- lib/grape/parser.rb | 12 +++++------- lib/grape/parser/base.rb | 19 +++++++++++++++++++ lib/grape/parser/json.rb | 14 ++++++-------- lib/grape/parser/jsonapi.rb | 7 +++++++ lib/grape/parser/xml.rb | 14 ++++++-------- spec/grape/parser_spec.rb | 14 -------------- 6 files changed, 43 insertions(+), 37 deletions(-) create mode 100644 lib/grape/parser/base.rb create mode 100644 lib/grape/parser/jsonapi.rb diff --git a/lib/grape/parser.rb b/lib/grape/parser.rb index a446b4da20..9dcb81ef3d 100644 --- a/lib/grape/parser.rb +++ b/lib/grape/parser.rb @@ -2,16 +2,14 @@ module Grape module Parser - module_function + extend Grape::Util::Registry - DEFAULTS = { - json: Grape::Parser::Json, - jsonapi: Grape::Parser::Json, - xml: Grape::Parser::Xml - }.freeze + module_function def parser_for(format, parsers = nil) - parsers&.key?(format) ? parsers[format] : DEFAULTS[format] + return parsers[format] if parsers&.key?(format) + + registry[format] end end end diff --git a/lib/grape/parser/base.rb b/lib/grape/parser/base.rb new file mode 100644 index 0000000000..73a31e6623 --- /dev/null +++ b/lib/grape/parser/base.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Grape + module Parser + class Base + def self.call(_object, _env) + raise NotImplementedError + end + + def self.inherited(klass) + super + return if klass.name.blank? + + short_name = klass.name.demodulize.underscore + Parser.register(short_name, klass) + end + end + end +end diff --git a/lib/grape/parser/json.rb b/lib/grape/parser/json.rb index 4e665a1ec2..a808999cce 100644 --- a/lib/grape/parser/json.rb +++ b/lib/grape/parser/json.rb @@ -2,14 +2,12 @@ module Grape module Parser - module Json - class << self - def call(object, _env) - ::Grape::Json.load(object) - rescue ::Grape::Json::ParseError - # handle JSON parsing errors via the rescue handlers or provide error message - raise Grape::Exceptions::InvalidMessageBody.new('application/json') - end + class Json < Base + def self.call(object, _env) + ::Grape::Json.load(object) + rescue ::Grape::Json::ParseError + # handle JSON parsing errors via the rescue handlers or provide error message + raise Grape::Exceptions::InvalidMessageBody.new('application/json') end end end diff --git a/lib/grape/parser/jsonapi.rb b/lib/grape/parser/jsonapi.rb new file mode 100644 index 0000000000..58e16571ba --- /dev/null +++ b/lib/grape/parser/jsonapi.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module Grape + module Parser + class Jsonapi < Json; end + end +end diff --git a/lib/grape/parser/xml.rb b/lib/grape/parser/xml.rb index 930c57f13e..bdb2a485fc 100644 --- a/lib/grape/parser/xml.rb +++ b/lib/grape/parser/xml.rb @@ -2,14 +2,12 @@ module Grape module Parser - module Xml - class << self - def call(object, _env) - ::Grape::Xml.parse(object) - rescue ::Grape::Xml::ParseError - # handle XML parsing errors via the rescue handlers or provide error message - raise Grape::Exceptions::InvalidMessageBody.new('application/xml') - end + class Xml < Base + def self.call(object, _env) + ::Grape::Xml.parse(object) + rescue ::Grape::Xml::ParseError + # handle XML parsing errors via the rescue handlers or provide error message + raise Grape::Exceptions::InvalidMessageBody.new('application/xml') end end end diff --git a/spec/grape/parser_spec.rb b/spec/grape/parser_spec.rb index ecc5fdfa4d..a349d61e73 100644 --- a/spec/grape/parser_spec.rb +++ b/spec/grape/parser_spec.rb @@ -3,20 +3,6 @@ describe Grape::Parser do subject { described_class } - describe 'DEFAULTS' do - subject { described_class::DEFAULTS } - - let(:expected_defaults) do - { - json: Grape::Parser::Json, - jsonapi: Grape::Parser::Json, - xml: Grape::Parser::Xml - } - end - - it { is_expected.to eq(expected_defaults) } - end - describe '.parser_for' do let(:options) { {} } From f3a735aa420a2e1e759fa04f81668126f9a9bc03 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 15:55:33 +0100 Subject: [PATCH 06/19] Add Grape::Formatter::Base and use Grape::Util::Registry --- lib/grape/formatter.rb | 16 ++++------------ lib/grape/formatter/base.rb | 18 ++++++++++++++++++ lib/grape/formatter/json.rb | 10 ++++------ lib/grape/formatter/serializable_hash.rb | 2 +- lib/grape/formatter/txt.rb | 8 +++----- lib/grape/formatter/xml.rb | 10 ++++------ 6 files changed, 34 insertions(+), 30 deletions(-) create mode 100644 lib/grape/formatter/base.rb diff --git a/lib/grape/formatter.rb b/lib/grape/formatter.rb index d586b0bd6a..6d5affb34f 100644 --- a/lib/grape/formatter.rb +++ b/lib/grape/formatter.rb @@ -2,24 +2,16 @@ module Grape module Formatter - module_function + extend Grape::Util::Registry - DEFAULTS = { - json: Grape::Formatter::Json, - jsonapi: Grape::Formatter::Json, - serializable_hash: Grape::Formatter::SerializableHash, - txt: Grape::Formatter::Txt, - xml: Grape::Formatter::Xml - }.freeze + module_function DEFAULT_LAMBDA_FORMATTER = ->(obj, _env) { obj } def formatter_for(api_format, formatters) - select_formatter(formatters, api_format) || DEFAULT_LAMBDA_FORMATTER - end + return formatters[api_format] if formatters&.key?(api_format) - def select_formatter(formatters, api_format) - formatters&.key?(api_format) ? formatters[api_format] : DEFAULTS[api_format] + registry[api_format] || DEFAULT_LAMBDA_FORMATTER end end end diff --git a/lib/grape/formatter/base.rb b/lib/grape/formatter/base.rb new file mode 100644 index 0000000000..2aeb1d8745 --- /dev/null +++ b/lib/grape/formatter/base.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Grape + module Formatter + class Base + def self.call(_object, _env) + raise NotImplementedError + end + + def self.inherited(klass) + super + return if klass.name.blank? + + Formatter.register(klass.name.demodulize.underscore, klass) + end + end + end +end diff --git a/lib/grape/formatter/json.rb b/lib/grape/formatter/json.rb index 0f0152f6c4..bfdd2ca286 100644 --- a/lib/grape/formatter/json.rb +++ b/lib/grape/formatter/json.rb @@ -2,13 +2,11 @@ module Grape module Formatter - module Json - class << self - def call(object, _env) - return object.to_json if object.respond_to?(:to_json) + class Json < Base + def self.call(object, _env) + return object.to_json if object.respond_to?(:to_json) - ::Grape::Json.dump(object) - end + ::Grape::Json.dump(object) end end end diff --git a/lib/grape/formatter/serializable_hash.rb b/lib/grape/formatter/serializable_hash.rb index cb9bfb7c14..5b29ece151 100644 --- a/lib/grape/formatter/serializable_hash.rb +++ b/lib/grape/formatter/serializable_hash.rb @@ -2,7 +2,7 @@ module Grape module Formatter - module SerializableHash + class SerializableHash < Base class << self def call(object, _env) return object if object.is_a?(String) diff --git a/lib/grape/formatter/txt.rb b/lib/grape/formatter/txt.rb index 1f1f9ef2f5..cb77e4f07f 100644 --- a/lib/grape/formatter/txt.rb +++ b/lib/grape/formatter/txt.rb @@ -2,11 +2,9 @@ module Grape module Formatter - module Txt - class << self - def call(object, _env) - object.respond_to?(:to_txt) ? object.to_txt : object.to_s - end + class Txt < Base + def self.call(object, _env) + object.respond_to?(:to_txt) ? object.to_txt : object.to_s end end end diff --git a/lib/grape/formatter/xml.rb b/lib/grape/formatter/xml.rb index c170d1843e..de0531758a 100644 --- a/lib/grape/formatter/xml.rb +++ b/lib/grape/formatter/xml.rb @@ -2,13 +2,11 @@ module Grape module Formatter - module Xml - class << self - def call(object, _env) - return object.to_xml if object.respond_to?(:to_xml) + class Xml < Base + def self.call(object, _env) + return object.to_xml if object.respond_to?(:to_xml) - raise Grape::Exceptions::InvalidFormatter.new(object.class, 'xml') - end + raise Grape::Exceptions::InvalidFormatter.new(object.class, 'xml') end end end From 93fcb57320e1a13bfbbed22343d832fc95cee93e Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 15:56:25 +0100 Subject: [PATCH 07/19] Add Grape::Middleware::Versioner::Base and use Grape::Util::Registry --- lib/grape/middleware/versioner.rb | 8 +- .../versioner/accept_version_header.rb | 2 - lib/grape/middleware/versioner/base.rb | 85 +++++++++++++++++++ lib/grape/middleware/versioner/header.rb | 2 - lib/grape/middleware/versioner/param.rb | 2 - lib/grape/middleware/versioner/path.rb | 2 - lib/grape/middleware/versioner_helpers.rb | 75 ---------------- 7 files changed, 90 insertions(+), 86 deletions(-) create mode 100644 lib/grape/middleware/versioner/base.rb delete mode 100644 lib/grape/middleware/versioner_helpers.rb diff --git a/lib/grape/middleware/versioner.rb b/lib/grape/middleware/versioner.rb index 1589f9b76c..bcca9fe596 100644 --- a/lib/grape/middleware/versioner.rb +++ b/lib/grape/middleware/versioner.rb @@ -11,14 +11,16 @@ module Grape module Middleware module Versioner + extend Grape::Util::Registry + module_function # @param strategy [Symbol] :path, :header, :accept_version_header or :param # @return a middleware class based on strategy def using(strategy) - Grape::Middleware::Versioner.const_get(:"#{strategy.to_s.camelize}") - rescue NameError - raise Grape::Exceptions::InvalidVersionerOption, strategy + raise Grape::Exceptions::InvalidVersionerOption, strategy unless registry.key?(strategy) + + registry[strategy] end end end diff --git a/lib/grape/middleware/versioner/accept_version_header.rb b/lib/grape/middleware/versioner/accept_version_header.rb index 1cf2bb6748..ff76a73723 100644 --- a/lib/grape/middleware/versioner/accept_version_header.rb +++ b/lib/grape/middleware/versioner/accept_version_header.rb @@ -17,8 +17,6 @@ module Versioner # X-Cascade header to alert Grape::Router to attempt the next matched # route. class AcceptVersionHeader < Base - include VersionerHelpers - def before potential_version = env[Grape::Http::Headers::HTTP_ACCEPT_VERSION]&.strip not_acceptable!('Accept-Version header must be set.') if strict? && potential_version.blank? diff --git a/lib/grape/middleware/versioner/base.rb b/lib/grape/middleware/versioner/base.rb new file mode 100644 index 0000000000..2f5b07d45b --- /dev/null +++ b/lib/grape/middleware/versioner/base.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +module Grape + module Middleware + module Versioner + class Base < Grape::Middleware::Base + DEFAULT_PATTERN = /.*/i.freeze + DEFAULT_PARAMETER = 'apiver' + + def self.inherited(klass) + super + return if klass.name.blank? + + short_name = klass.name.demodulize.underscore + Versioner.register(short_name, klass) + end + + def default_options + { + versions: nil, + prefix: nil, + mount_path: nil, + pattern: DEFAULT_PATTERN, + version_options: { + strict: false, + cascade: true, + parameter: DEFAULT_PARAMETER + } + } + end + + def versions + options[:versions] + end + + def prefix + options[:prefix] + end + + def mount_path + options[:mount_path] + end + + def pattern + options[:pattern] + end + + def version_options + options[:version_options] + 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] + end + + def vendor + version_options[:vendor] + end + + def error_headers + cascade? ? { Grape::Http::Headers::X_CASCADE => 'pass' } : {} + end + + def potential_version_match?(potential_version) + versions.blank? || versions.any? { |v| v.to_s == potential_version } + end + + def version_not_found! + throw :error, status: 404, message: '404 API Version Not Found', headers: { Grape::Http::Headers::X_CASCADE => 'pass' } + end + end + end + end +end diff --git a/lib/grape/middleware/versioner/header.rb b/lib/grape/middleware/versioner/header.rb index 11cbfc7bcb..a34a80fc7f 100644 --- a/lib/grape/middleware/versioner/header.rb +++ b/lib/grape/middleware/versioner/header.rb @@ -22,8 +22,6 @@ module Versioner # X-Cascade header to alert Grape::Router to attempt the next matched # route. class Header < Base - include VersionerHelpers - def before match_best_quality_media_type! do |media_type| env.update( diff --git a/lib/grape/middleware/versioner/param.rb b/lib/grape/middleware/versioner/param.rb index 0c8f88a47c..771faf616b 100644 --- a/lib/grape/middleware/versioner/param.rb +++ b/lib/grape/middleware/versioner/param.rb @@ -19,8 +19,6 @@ module Versioner # # env['api.version'] => 'v1' class Param < Base - include VersionerHelpers - def before potential_version = Rack::Utils.parse_nested_query(env[Rack::QUERY_STRING])[parameter_key] return if potential_version.blank? diff --git a/lib/grape/middleware/versioner/path.rb b/lib/grape/middleware/versioner/path.rb index c824f2df81..dd4379767e 100644 --- a/lib/grape/middleware/versioner/path.rb +++ b/lib/grape/middleware/versioner/path.rb @@ -17,8 +17,6 @@ module Versioner # env['api.version'] => 'v1' # class Path < Base - include VersionerHelpers - def before path_info = Grape::Router.normalize_path(env[Rack::PATH_INFO]) return if path_info == '/' diff --git a/lib/grape/middleware/versioner_helpers.rb b/lib/grape/middleware/versioner_helpers.rb deleted file mode 100644 index 0cce2055cb..0000000000 --- a/lib/grape/middleware/versioner_helpers.rb +++ /dev/null @@ -1,75 +0,0 @@ -# frozen_string_literal: true - -module Grape - module Middleware - module VersionerHelpers - DEFAULT_PATTERN = /.*/i.freeze - DEFAULT_PARAMETER = 'apiver' - - def default_options - { - versions: nil, - prefix: nil, - mount_path: nil, - pattern: DEFAULT_PATTERN, - version_options: { - strict: false, - cascade: true, - parameter: DEFAULT_PARAMETER - } - } - end - - def versions - options[:versions] - end - - def prefix - options[:prefix] - end - - def mount_path - options[:mount_path] - end - - def pattern - options[:pattern] - end - - def version_options - options[:version_options] - 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] - end - - def vendor - version_options[:vendor] - end - - def error_headers - cascade? ? { Grape::Http::Headers::X_CASCADE => 'pass' } : {} - end - - def potential_version_match?(potential_version) - versions.blank? || versions.any? { |v| v.to_s == potential_version } - end - - def version_not_found! - throw :error, status: 404, message: '404 API Version Not Found', headers: { Grape::Http::Headers::X_CASCADE => 'pass' } - end - end - end -end From 81e68f1b6bf9274a047423dfb6e61bbaa18d35eb Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 15:57:06 +0100 Subject: [PATCH 08/19] Add Grape::ErrorFormatter::Base and use Grape::Util::Registry --- lib/grape/error_formatter.rb | 16 +--- lib/grape/error_formatter/base.rb | 74 +++++++++++++------ lib/grape/error_formatter/json.rb | 31 ++------ lib/grape/error_formatter/jsonapi.rb | 7 ++ .../error_formatter/serializable_hash.rb | 7 ++ lib/grape/error_formatter/txt.rb | 33 ++++----- lib/grape/error_formatter/xml.rb | 16 +--- 7 files changed, 94 insertions(+), 90 deletions(-) create mode 100644 lib/grape/error_formatter/jsonapi.rb create mode 100644 lib/grape/error_formatter/serializable_hash.rb diff --git a/lib/grape/error_formatter.rb b/lib/grape/error_formatter.rb index 7784055b6a..7d9ace8a88 100644 --- a/lib/grape/error_formatter.rb +++ b/lib/grape/error_formatter.rb @@ -2,22 +2,14 @@ module Grape module ErrorFormatter - module_function + extend Grape::Util::Registry - DEFAULTS = { - serializable_hash: Grape::ErrorFormatter::Json, - json: Grape::ErrorFormatter::Json, - jsonapi: Grape::ErrorFormatter::Json, - txt: Grape::ErrorFormatter::Txt, - xml: Grape::ErrorFormatter::Xml - }.freeze + module_function def formatter_for(format, error_formatters = nil, default_error_formatter = nil) - select_formatter(error_formatters, format) || default_error_formatter || DEFAULTS[:txt] - end + return error_formatters[format] if error_formatters&.key?(format) - def select_formatter(error_formatters, format) - error_formatters&.key?(format) ? error_formatters[format] : DEFAULTS[format] + registry[format] || default_error_formatter || Grape::ErrorFormatter::Txt end end end diff --git a/lib/grape/error_formatter/base.rb b/lib/grape/error_formatter/base.rb index 2a1e758eba..3c33a95f8e 100644 --- a/lib/grape/error_formatter/base.rb +++ b/lib/grape/error_formatter/base.rb @@ -2,36 +2,68 @@ module Grape module ErrorFormatter - module Base - def present(message, env) - present_options = {} - presented_message = message - if presented_message.is_a?(Hash) - presented_message = presented_message.dup - present_options[:with] = presented_message.delete(:with) + class Base + class << self + def call(message, backtrace, options = {}, env = nil, original_exception = nil) + merge_backtrace = backtrace.present? && options.dig(:rescue_options, :backtrace) + merge_original_exception = original_exception && options.dig(:rescue_options, :original_exception) + + wrapped_message = wrap_message(present(message, env)) + if wrapped_message.is_a?(Hash) + wrapped_message[:backtrace] = backtrace if merge_backtrace + wrapped_message[:original_exception] = original_exception.inspect if merge_original_exception + end + + format_structured_message(wrapped_message) end - presenter = env[Grape::Env::API_ENDPOINT].entity_class_for_obj(presented_message, present_options) + def present(message, env) + present_options = {} + presented_message = message + if presented_message.is_a?(Hash) + presented_message = presented_message.dup + present_options[:with] = presented_message.delete(:with) + end + + presenter = env[Grape::Env::API_ENDPOINT].entity_class_for_obj(presented_message, present_options) + + unless presenter || env[Grape::Env::GRAPE_ROUTING_ARGS].nil? + # env['api.endpoint'].route does not work when the error occurs within a middleware + # the Endpoint does not have a valid env at this moment + http_codes = env[Grape::Env::GRAPE_ROUTING_ARGS][:route_info].http_codes || [] + + found_code = http_codes.find do |http_code| + (http_code[0].to_i == env[Grape::Env::API_ENDPOINT].status) && http_code[2].respond_to?(:represent) + end if env[Grape::Env::API_ENDPOINT].request - unless presenter || env[Grape::Env::GRAPE_ROUTING_ARGS].nil? - # env['api.endpoint'].route does not work when the error occurs within a middleware - # the Endpoint does not have a valid env at this moment - http_codes = env[Grape::Env::GRAPE_ROUTING_ARGS][:route_info].http_codes || [] + presenter = found_code[2] if found_code + end - found_code = http_codes.find do |http_code| - (http_code[0].to_i == env[Grape::Env::API_ENDPOINT].status) && http_code[2].respond_to?(:represent) - end if env[Grape::Env::API_ENDPOINT].request + if presenter + embeds = { env: env } + embeds[:version] = env[Grape::Env::API_VERSION] if env.key?(Grape::Env::API_VERSION) + presented_message = presenter.represent(presented_message, embeds).serializable_hash + end - presenter = found_code[2] if found_code + presented_message end - if presenter - embeds = { env: env } - embeds[:version] = env[Grape::Env::API_VERSION] if env.key?(Grape::Env::API_VERSION) - presented_message = presenter.represent(presented_message, embeds).serializable_hash + def wrap_message(message) + return message if message.is_a?(Hash) + + { message: message } + end + + def format_structured_message(_structured_message) + raise NotImplementedError end - presented_message + def inherited(klass) + super + return if klass.name.blank? + + ErrorFormatter.register(klass.name.demodulize.underscore, klass) + end end end end diff --git a/lib/grape/error_formatter/json.rb b/lib/grape/error_formatter/json.rb index f4df46e849..bed5bd39df 100644 --- a/lib/grape/error_formatter/json.rb +++ b/lib/grape/error_formatter/json.rb @@ -2,28 +2,19 @@ module Grape module ErrorFormatter - module Json - extend Base - + class Json < Base class << self - def call(message, backtrace, options = {}, env = nil, original_exception = nil) - result = wrap_message(present(message, env)) - - result = merge_rescue_options(result, backtrace, options, original_exception) if result.is_a?(Hash) - - ::Grape::Json.dump(result) + def format_structured_message(structured_message) + ::Grape::Json.dump(structured_message) end private def wrap_message(message) - if message.is_a?(Hash) - message - elsif message.is_a?(Exceptions::ValidationErrors) - message.as_json - else - { error: ensure_utf8(message) } - end + return message if message.is_a?(Hash) + return message.as_json if message.is_a?(Exceptions::ValidationErrors) + + { error: ensure_utf8(message) } end def ensure_utf8(message) @@ -31,14 +22,6 @@ def ensure_utf8(message) message.encode('UTF-8', invalid: :replace, undef: :replace) end - - def merge_rescue_options(result, backtrace, options, original_exception) - rescue_options = options[:rescue_options] || {} - result = result.merge(backtrace: backtrace) if rescue_options[:backtrace] && backtrace && !backtrace.empty? - result = result.merge(original_exception: original_exception.inspect) if rescue_options[:original_exception] && original_exception - - result - end end end end diff --git a/lib/grape/error_formatter/jsonapi.rb b/lib/grape/error_formatter/jsonapi.rb new file mode 100644 index 0000000000..ed1d2d30f7 --- /dev/null +++ b/lib/grape/error_formatter/jsonapi.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module Grape + module ErrorFormatter + class Jsonapi < Json; end + end +end diff --git a/lib/grape/error_formatter/serializable_hash.rb b/lib/grape/error_formatter/serializable_hash.rb new file mode 100644 index 0000000000..14b9ed5972 --- /dev/null +++ b/lib/grape/error_formatter/serializable_hash.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module Grape + module ErrorFormatter + class SerializableHash < Json; end + end +end diff --git a/lib/grape/error_formatter/txt.rb b/lib/grape/error_formatter/txt.rb index 22fc5c5385..7c0c759184 100644 --- a/lib/grape/error_formatter/txt.rb +++ b/lib/grape/error_formatter/txt.rb @@ -2,26 +2,19 @@ module Grape module ErrorFormatter - module Txt - extend Base - - class << self - def call(message, backtrace, options = {}, env = nil, original_exception = nil) - message = present(message, env) - - result = message.is_a?(Hash) ? ::Grape::Json.dump(message) : message - Array.wrap(result).tap do |final_result| - rescue_options = options[:rescue_options] || {} - if rescue_options[:backtrace] && backtrace.present? - final_result << 'backtrace:' - final_result.concat(backtrace) - end - if rescue_options[:original_exception] && original_exception - final_result << 'original exception:' - final_result << original_exception.inspect - end - end.join("\r\n ") - end + class Txt < Base + def self.format_structured_message(structured_message) + message = structured_message[:message] || Grape::Json.dump(structured_message) + Array.wrap(message).tap do |final_message| + if structured_message.key?(:backtrace) + final_message << 'backtrace:' + final_message.concat(structured_message[:backtrace]) + end + if structured_message.key?(:original_exception) + final_message << 'original exception:' + final_message << structured_message[:original_exception] + end + end.join("\r\n ") end end end diff --git a/lib/grape/error_formatter/xml.rb b/lib/grape/error_formatter/xml.rb index e423c2fd9d..c78f6d650c 100644 --- a/lib/grape/error_formatter/xml.rb +++ b/lib/grape/error_formatter/xml.rb @@ -2,19 +2,9 @@ module Grape module ErrorFormatter - module Xml - extend Base - - class << self - def call(message, backtrace, options = {}, env = nil, original_exception = nil) - message = present(message, env) - - result = message.is_a?(Hash) ? message : { message: message } - rescue_options = options[:rescue_options] || {} - result = result.merge(backtrace: backtrace) if rescue_options[:backtrace] && backtrace && !backtrace.empty? - result = result.merge(original_exception: original_exception.inspect) if rescue_options[:original_exception] && original_exception - result.respond_to?(:to_xml) ? result.to_xml(root: :error) : result.to_s - end + class Xml < Base + def self.format_structured_message(structured_message) + structured_message.respond_to?(:to_xml) ? structured_message.to_xml(root: :error) : structured_message.to_s end end end From 8aee8f913543df945d097151f33c3a8515342cd0 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 15:58:28 +0100 Subject: [PATCH 09/19] Add Grape::Util::Registry to Grape::Validations ContractScope validator has been moved to validations/validators and renamed properly --- lib/grape/validations.rb | 25 +++-------- lib/grape/validations/contract_scope.rb | 36 +-------------- lib/grape/validations/validators/base.rb | 2 +- .../validators/contract_scope_validator.rb | 41 +++++++++++++++++ spec/grape/api/custom_validations_spec.rb | 45 ++++++++++++++++--- .../contract_scope_validator_spec.rb | 9 ++++ spec/grape/validations_spec.rb | 24 ++++++++-- .../dry_validation/dry_validation_spec.rb | 8 ---- 8 files changed, 117 insertions(+), 73 deletions(-) create mode 100644 lib/grape/validations/validators/contract_scope_validator.rb create mode 100644 spec/grape/validations/validators/contract_scope_validator_spec.rb diff --git a/lib/grape/validations.rb b/lib/grape/validations.rb index 9ae22ae6a1..76235bc2fa 100644 --- a/lib/grape/validations.rb +++ b/lib/grape/validations.rb @@ -2,29 +2,14 @@ module Grape module Validations - module_function - - def validators - @validators ||= {} - end - - # Register a new validator, so it can be used to validate parameters. - # @param short_name [String] all lower-case, no spaces - # @param klass [Class] the validator class. Should inherit from - # Grape::Validations::Validators::Base. - def register_validator(short_name, klass) - validators[short_name] = klass - end + extend Grape::Util::Registry - def deregister_validator(short_name) - validators.delete(short_name) - end + module_function def require_validator(short_name) - str_name = short_name.to_s - validators.fetch(str_name) { Grape::Validations::Validators.const_get(:"#{str_name.camelize}Validator") } - rescue NameError - raise Grape::Exceptions::UnknownValidator, short_name + raise Grape::Exceptions::UnknownValidator, short_name unless registry.key?(short_name) + + registry[short_name] end end end diff --git a/lib/grape/validations/contract_scope.rb b/lib/grape/validations/contract_scope.rb index 3b66df5722..218f47eec5 100644 --- a/lib/grape/validations/contract_scope.rb +++ b/lib/grape/validations/contract_scope.rb @@ -23,46 +23,12 @@ def initialize(api, contract = nil, &block) api.namespace_stackable(:contract_key_map, key_map) validator_options = { - validator_class: Validator, + validator_class: Grape::Validations.require_validator(:contract_scope), opts: { schema: contract, fail_fast: false } } api.namespace_stackable(:validations, validator_options) end - - class Validator < Grape::Validations::Validators::Base - attr_reader :schema - - def initialize(_attrs, _options, _required, _scope, opts) - super - @schema = opts.fetch(:schema) - end - - # Validates a given request. - # @param request [Grape::Request] the request currently being handled - # @raise [Grape::Exceptions::ValidationArrayErrors] if validation failed - # @return [void] - def validate(request) - res = schema.call(request.params) - - if res.success? - request.params.deep_merge!(res.to_h) - return - end - - raise Grape::Exceptions::ValidationArrayErrors.new(build_errors_from_messages(res.errors.messages)) - end - - private - - def build_errors_from_messages(messages) - messages.map do |message| - full_name = message.path.first.to_s - full_name << "[#{message.path[1..].join('][')}]" if message.path.size > 1 - Grape::Exceptions::Validation.new(params: [full_name], message: message.text) - end - end - end end end end diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index ee2dc4a87e..9b65573324 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -62,7 +62,7 @@ def self.inherited(klass) return if klass.name.blank? short_validator_name = klass.name.demodulize.underscore.delete_suffix('_validator') - Validations.register_validator(short_validator_name, klass) + Validations.register(short_validator_name, klass) end def message(default_key = nil) diff --git a/lib/grape/validations/validators/contract_scope_validator.rb b/lib/grape/validations/validators/contract_scope_validator.rb new file mode 100644 index 0000000000..b8a3365c1b --- /dev/null +++ b/lib/grape/validations/validators/contract_scope_validator.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Grape + module Validations + module Validators + class ContractScopeValidator < Base + attr_reader :schema + + def initialize(_attrs, _options, _required, _scope, opts) + super + @schema = opts.fetch(:schema) + end + + # Validates a given request. + # @param request [Grape::Request] the request currently being handled + # @raise [Grape::Exceptions::ValidationArrayErrors] if validation failed + # @return [void] + def validate(request) + res = schema.call(request.params) + + if res.success? + request.params.deep_merge!(res.to_h) + return + end + + raise Grape::Exceptions::ValidationArrayErrors.new(build_errors_from_messages(res.errors.messages)) + end + + private + + def build_errors_from_messages(messages) + messages.map do |message| + full_name = message.path.first.to_s + full_name << "[#{message.path[1..].join('][')}]" if message.path.size > 1 + Grape::Exceptions::Validation.new(params: [full_name], message: message.text) + end + end + end + end + end +end diff --git a/spec/grape/api/custom_validations_spec.rb b/spec/grape/api/custom_validations_spec.rb index d16c307fe2..dc6ae27811 100644 --- a/spec/grape/api/custom_validations_spec.rb +++ b/spec/grape/api/custom_validations_spec.rb @@ -35,7 +35,14 @@ def validate_param!(attr_name, params) end let(:app) { subject } - before { stub_const('Grape::Validations::Validators::DefaultLengthValidator', default_length_validator) } + before do + stub_const('DefaultLengthValidator', default_length_validator) + described_class.register(:default_length, DefaultLengthValidator) + end + + after do + described_class.deregister(:default_length) + end it 'under 140 characters' do get '/', text: 'abc' @@ -77,7 +84,14 @@ def validate(request) end let(:app) { subject } - before { stub_const('Grape::Validations::Validators::InBodyValidator', in_body_validator) } + before do + stub_const('InBodyValidator', in_body_validator) + described_class.register(:in_body, InBodyValidator) + end + + after do + described_class.deregister(:in_body) + end it 'allows field in body' do get '/', text: 'abc' @@ -113,7 +127,14 @@ def validate_param!(attr_name, _params) end let(:app) { subject } - before { stub_const('Grape::Validations::Validators::WithMessageKeyValidator', message_key_validator) } + before do + stub_const('WithMessageKeyValidator', message_key_validator) + described_class.register(:with_message_key, WithMessageKeyValidator) + end + + after do + described_class.deregister(:with_message_key) + end it 'fails with message' do get '/', text: 'foobar' @@ -159,7 +180,14 @@ def access_header let(:app) { subject } let(:x_access_token_header) { 'x-access-token' } - before { stub_const('Grape::Validations::Validators::AdminValidator', admin_validator) } + before do + stub_const('AdminValidator', admin_validator) + described_class.register(:admin, AdminValidator) + end + + after do + described_class.deregister(:admin) + end it 'fail when non-admin user sets an admin field' do get '/', admin_field: 'tester', non_admin_field: 'toaster' @@ -218,7 +246,14 @@ def validate_param!(_attr_name, _params) end end - before { stub_const('Grape::Validations::Validators::InstanceValidatorValidator', validator_type) } + before do + stub_const('InstanceValidatorValidator', validator_type) + described_class.register(:instance_validator, InstanceValidatorValidator) + end + + after do + described_class.deregister(:instance_validator) + end it 'passes validation every time' do expect(validator_type).to receive(:new).twice.and_call_original diff --git a/spec/grape/validations/validators/contract_scope_validator_spec.rb b/spec/grape/validations/validators/contract_scope_validator_spec.rb new file mode 100644 index 0000000000..b8d462c313 --- /dev/null +++ b/spec/grape/validations/validators/contract_scope_validator_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +describe Grape::Validations::Validators::ContractScopeValidator do + describe '.inherits' do + subject { described_class } + + it { is_expected.to be < Grape::Validations::Validators::Base } + end +end diff --git a/spec/grape/validations_spec.rb b/spec/grape/validations_spec.rb index 19dd130f03..e0499ea442 100644 --- a/spec/grape/validations_spec.rb +++ b/spec/grape/validations_spec.rb @@ -494,7 +494,8 @@ def validate_param!(attr_name, params) end before do - stub_const('Grape::Validations::Validators::DateRangeValidator', date_range_validator) + stub_const('DateRangeValidator', date_range_validator) + described_class.register(:date_range, DateRangeValidator) subject.params do optional :date_range, date_range: true, type: Hash do requires :from, type: Integer @@ -515,6 +516,10 @@ def validate_param!(attr_name, params) end end + after do + described_class.deregister(:date_range) + end + context 'which is optional' do it "doesn't throw an error if the validation passes" do get '/optional', date_range: { from: 1, to: 2 } @@ -1186,7 +1191,14 @@ def validate_param!(attr_name, params) end end - before { stub_const('Grape::Validations::Validators::CustomvalidatorValidator', custom_validator) } + before do + stub_const('CustomvalidatorValidator', custom_validator) + described_class.register(:customvalidator, CustomvalidatorValidator) + end + + after do + described_class.deregister(:customvalidator) + end context 'when using optional with a custom validator' do before do @@ -1338,8 +1350,8 @@ def validate_param!(attr_name, params) end before do - stub_const('Grape::Validations::Validators::CustomvalidatorWithOptionsValidator', custom_validator_with_options) - + stub_const('CustomvalidatorWithOptionsValidator', custom_validator_with_options) + described_class.register(:customvalidator_with_options, CustomvalidatorWithOptionsValidator) subject.params do optional :custom, customvalidator_with_options: { text: 'im custom with options', message: 'is not custom with options!' } end @@ -1348,6 +1360,10 @@ def validate_param!(attr_name, params) end end + after do + described_class.deregister(:customvalidator_with_options) + end + it 'validates param with custom validator with options' do get '/optional_custom', custom: 'im custom with options' expect(last_response.status).to eq(200) diff --git a/spec/integration/dry_validation/dry_validation_spec.rb b/spec/integration/dry_validation/dry_validation_spec.rb index 6333bdacdd..d7b2f8efab 100644 --- a/spec/integration/dry_validation/dry_validation_spec.rb +++ b/spec/integration/dry_validation/dry_validation_spec.rb @@ -236,12 +236,4 @@ end end end - - describe Grape::Validations::ContractScope::Validator do - describe '.inherits' do - subject { described_class } - - it { is_expected.to be < Grape::Validations::Validators::Base } - end - end end From 6790e4dd2acc797c727d9c1787bc6756b02a5cd9 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 16:03:04 +0100 Subject: [PATCH 10/19] Add `deregister in `before(:all)`` --- spec/spec_helper.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 26d3d4aa58..9340f0d83f 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -17,7 +17,7 @@ # so it should be set to true here as well to reflect that. I18n.enforce_available_locales = true -Grape::Util::Registry.include(Deregister) + RSpec.configure do |config| config.include Rack::Test::Methods @@ -25,7 +25,10 @@ config.raise_errors_for_deprecations! config.filter_run_when_matching :focus - config.before(:all) { Grape::Util::InheritableSetting.reset_global! } + config.before(:all) do + Grape::Util::InheritableSetting.reset_global! + Grape::Util::Registry.include(Deregister) + end config.before { Grape::Util::InheritableSetting.reset_global! } # Enable flags like --only-failures and --next-failure From 257a90c835a41cf57bd99e2066298ce2192c44af Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 16:13:46 +0100 Subject: [PATCH 11/19] Add `deregister` to Grape::Validations only --- spec/spec_helper.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 9340f0d83f..f8095dd863 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -17,8 +17,6 @@ # so it should be set to true here as well to reflect that. I18n.enforce_available_locales = true - - RSpec.configure do |config| config.include Rack::Test::Methods config.include Spec::Support::Helpers @@ -27,7 +25,7 @@ config.before(:all) do Grape::Util::InheritableSetting.reset_global! - Grape::Util::Registry.include(Deregister) + Grape::Validations.include(Deregister) end config.before { Grape::Util::InheritableSetting.reset_global! } From 710aa2e7b03846b90dc8e1b0523cda22e636a674 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 16:16:11 +0100 Subject: [PATCH 12/19] Use `prepend` --- spec/spec_helper.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index f8095dd863..020638af37 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -16,6 +16,7 @@ # The default value for this setting is true in a standard Rails app, # so it should be set to true here as well to reflect that. I18n.enforce_available_locales = true +Grape::Util::Registry.prepend(Deregister) RSpec.configure do |config| config.include Rack::Test::Methods @@ -23,10 +24,7 @@ config.raise_errors_for_deprecations! config.filter_run_when_matching :focus - config.before(:all) do - Grape::Util::InheritableSetting.reset_global! - Grape::Validations.include(Deregister) - end + config.before(:all) { Grape::Util::InheritableSetting.reset_global! } config.before { Grape::Util::InheritableSetting.reset_global! } # Enable flags like --only-failures and --next-failure From bda6e5913e66dba4a9927a31b032013074b34dd1 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 15 Dec 2024 16:32:14 +0100 Subject: [PATCH 13/19] Fix Ruby 2.7 Fix rubocop --- lib/grape/exceptions/validation_errors.rb | 7 +++---- spec/spec_helper.rb | 5 ++++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/grape/exceptions/validation_errors.rb b/lib/grape/exceptions/validation_errors.rb index 695650e6d9..6f9bae4cf1 100644 --- a/lib/grape/exceptions/validation_errors.rb +++ b/lib/grape/exceptions/validation_errors.rb @@ -41,10 +41,9 @@ def full_messages each do |attributes, error| messages << I18n.t(ERRORS_FORMAT_KEY, - default: DEFAULT_ERRORS_FORMAT, - attributes: translate_attributes(attributes), - message: error.message - ) + default: DEFAULT_ERRORS_FORMAT, + attributes: translate_attributes(attributes), + message: error.message) end messages.uniq! end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 020638af37..06cb282a00 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -13,10 +13,13 @@ end end +Grape::Util::Registry.include(Deregister) +# issue with ruby 2.7 with ^. We need to extend it again +Grape::Validations.extend(Grape::Util::Registry) if Gem::Version.new(RUBY_VERSION).release < Gem::Version.new('3.0') + # The default value for this setting is true in a standard Rails app, # so it should be set to true here as well to reflect that. I18n.enforce_available_locales = true -Grape::Util::Registry.prepend(Deregister) RSpec.configure do |config| config.include Rack::Test::Methods From 881fc9c4a31dfde65757fdbaf282810e998cdc38 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Thu, 19 Dec 2024 18:30:31 +0100 Subject: [PATCH 14/19] Refactor collection_coercer_for Refactor Grape::Validations::Types cache_key --- lib/grape/validations/types.rb | 23 +++++++------------ .../validations/types/dry_type_coercer.rb | 16 ++++++++----- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/lib/grape/validations/types.rb b/lib/grape/validations/types.rb index 86f9c9b601..90c36e0f21 100644 --- a/lib/grape/validations/types.rb +++ b/lib/grape/validations/types.rb @@ -176,9 +176,7 @@ def create_coercer_instance(type, method, strict) # CustomTypeCoercer (above) already handles such types when an explicit coercion # method is supplied. elsif Types.collection_of_custom?(type) - Types::CustomTypeCollectionCoercer.new( - Types.map_special(type.first), type.is_a?(Set) - ) + Types::CustomTypeCollectionCoercer.new(Types.map_special(type.first), type.is_a?(Set)) else DryTypeCoercer.coercer_instance_for(type, strict) end @@ -186,24 +184,19 @@ def create_coercer_instance(type, method, strict) def cache_instance(type, method, strict, &_block) key = cache_key(type, method, strict) - return @__cache[key] if @__cache.key?(key) - instance = yield - - @__cache_write_lock.synchronize do - @__cache[key] = instance + yield.tap do |instance| + @__cache_write_lock.synchronize do + @__cache[key] = instance + end end - - instance end def cache_key(type, method, strict) - [type, method, strict].each_with_object(+'_') do |val, memo| - next if val.nil? - - memo << '_' << val.to_s - end + key = [type.to_s, strict] + key.insert(1, method) if method + key.join('_') end instance_variable_set(:@__cache, {}) diff --git a/lib/grape/validations/types/dry_type_coercer.rb b/lib/grape/validations/types/dry_type_coercer.rb index 1067eaf3a8..f9672198ee 100644 --- a/lib/grape/validations/types/dry_type_coercer.rb +++ b/lib/grape/validations/types/dry_type_coercer.rb @@ -22,16 +22,20 @@ class << self # collection_coercer_for(Array) # #=> Grape::Validations::Types::ArrayCoercer def collection_coercer_for(type) - Grape::Validations::Types.const_get(:"#{type.name.camelize}Coercer") + case type + when Array + ArrayCoercer + when Set + SetCoercer + else + raise ArgumentError, "Unknown type: #{type}" + end end # Returns an instance of a coercer for a given type def coercer_instance_for(type, strict = false) - return PrimitiveCoercer.new(type, strict) if type.instance_of?(Class) - - # in case of a collection (Array[Integer]) the type is an instance of a collection, - # so we need to figure out the actual type - collection_coercer_for(type.class).new(type, strict) + klass = type.instance_of?(Class) ? PrimitiveCoercer : collection_coercer_for(type) + klass.new(type, strict) end end From 3a7fafd64322f33e324bda2525398b2b68929e93 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Thu, 19 Dec 2024 18:37:10 +0100 Subject: [PATCH 15/19] Add CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d274b058ea..86ec369fbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * [#2512](https://github.com/ruby-grape/grape/pull/2512): Optimize hash alloc - [@ericproulx](https://github.com/ericproulx). * [#2513](https://github.com/ruby-grape/grape/pull/2513): Optimize Grape::Path - [@ericproulx](https://github.com/ericproulx). * [#2514](https://github.com/ruby-grape/grape/pull/2514): Add rails 8.0 to CI - [@ericproulx](https://github.com/ericproulx). +* [#7](https://github.com/ericproulx/grape/pull/7): Dynamic registration for parsers, formatters, versioners - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes From ed08c8b396de01591c8d27c30cac6a26d6656208 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Thu, 19 Dec 2024 22:56:37 +0100 Subject: [PATCH 16/19] Revert coercer_cache changes. Will do it another time --- lib/grape/validations/types.rb | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/grape/validations/types.rb b/lib/grape/validations/types.rb index 90c36e0f21..86f9c9b601 100644 --- a/lib/grape/validations/types.rb +++ b/lib/grape/validations/types.rb @@ -176,7 +176,9 @@ def create_coercer_instance(type, method, strict) # CustomTypeCoercer (above) already handles such types when an explicit coercion # method is supplied. elsif Types.collection_of_custom?(type) - Types::CustomTypeCollectionCoercer.new(Types.map_special(type.first), type.is_a?(Set)) + Types::CustomTypeCollectionCoercer.new( + Types.map_special(type.first), type.is_a?(Set) + ) else DryTypeCoercer.coercer_instance_for(type, strict) end @@ -184,19 +186,24 @@ def create_coercer_instance(type, method, strict) def cache_instance(type, method, strict, &_block) key = cache_key(type, method, strict) + return @__cache[key] if @__cache.key?(key) - yield.tap do |instance| - @__cache_write_lock.synchronize do - @__cache[key] = instance - end + instance = yield + + @__cache_write_lock.synchronize do + @__cache[key] = instance end + + instance end def cache_key(type, method, strict) - key = [type.to_s, strict] - key.insert(1, method) if method - key.join('_') + [type, method, strict].each_with_object(+'_') do |val, memo| + next if val.nil? + + memo << '_' << val.to_s + end end instance_variable_set(:@__cache, {}) From 44784638b9a861701ab8f920217a3e0b253c785c Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Fri, 20 Dec 2024 16:21:30 +0100 Subject: [PATCH 17/19] Revert enumerable change --- lib/grape/exceptions/validation_errors.rb | 22 ++++++++++---------- lib/grape/validations/attributes_iterator.rb | 4 ++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/grape/exceptions/validation_errors.rb b/lib/grape/exceptions/validation_errors.rb index 6f9bae4cf1..8859d579b1 100644 --- a/lib/grape/exceptions/validation_errors.rb +++ b/lib/grape/exceptions/validation_errors.rb @@ -6,6 +6,8 @@ class ValidationErrors < Base ERRORS_FORMAT_KEY = 'grape.errors.format' DEFAULT_ERRORS_FORMAT = '%s %s' + include Enumerable + attr_reader :errors def initialize(errors: [], headers: {}) @@ -14,8 +16,6 @@ def initialize(errors: [], headers: {}) end def each - return to_enum(:each) unless block_given? - errors.each_pair do |attribute, errors| errors.each do |error| yield attribute, error @@ -37,16 +37,16 @@ def to_json(*_opts) end def full_messages - [].tap do |messages| - each do |attributes, error| - messages << - I18n.t(ERRORS_FORMAT_KEY, - default: DEFAULT_ERRORS_FORMAT, - attributes: translate_attributes(attributes), - message: error.message) - end - messages.uniq! + messages = map do |attributes, error| + I18n.t( + ERRORS_FORMAT_KEY, + default: DEFAULT_ERRORS_FORMAT, + attributes: translate_attributes(attributes), + message: error.message + ) end + messages.uniq! + messages end end end diff --git a/lib/grape/validations/attributes_iterator.rb b/lib/grape/validations/attributes_iterator.rb index a184aafdf7..97d7f0280d 100644 --- a/lib/grape/validations/attributes_iterator.rb +++ b/lib/grape/validations/attributes_iterator.rb @@ -3,6 +3,8 @@ module Grape module Validations class AttributesIterator + include Enumerable + attr_reader :scope def initialize(validator, scope, params) @@ -13,8 +15,6 @@ def initialize(validator, scope, params) end def each(&block) - return to_enum(:each) unless block - do_each(@params, &block) # because we need recursion for nested arrays end From 7d0f366c827f3e7bb447fe25486bceb146a17a9b Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Tue, 24 Dec 2024 00:56:07 +0100 Subject: [PATCH 18/19] Refactor registry --- lib/grape/error_formatter/base.rb | 4 +--- lib/grape/formatter/base.rb | 4 +--- lib/grape/middleware/versioner/base.rb | 5 +---- lib/grape/parser/base.rb | 5 +---- lib/grape/util/registry.rb | 11 ++++++++++- lib/grape/validations.rb | 6 ++++++ lib/grape/validations/params_scope.rb | 2 +- lib/grape/validations/validators/base.rb | 5 +---- spec/grape/api/custom_validations_spec.rb | 10 +++++----- spec/grape/validations_spec.rb | 6 +++--- 10 files changed, 30 insertions(+), 28 deletions(-) diff --git a/lib/grape/error_formatter/base.rb b/lib/grape/error_formatter/base.rb index 3c33a95f8e..f2cf223c67 100644 --- a/lib/grape/error_formatter/base.rb +++ b/lib/grape/error_formatter/base.rb @@ -60,9 +60,7 @@ def format_structured_message(_structured_message) def inherited(klass) super - return if klass.name.blank? - - ErrorFormatter.register(klass.name.demodulize.underscore, klass) + ErrorFormatter.register(klass) end end end diff --git a/lib/grape/formatter/base.rb b/lib/grape/formatter/base.rb index 2aeb1d8745..dfd56d65d4 100644 --- a/lib/grape/formatter/base.rb +++ b/lib/grape/formatter/base.rb @@ -9,9 +9,7 @@ def self.call(_object, _env) def self.inherited(klass) super - return if klass.name.blank? - - Formatter.register(klass.name.demodulize.underscore, klass) + Formatter.register(klass) end end end diff --git a/lib/grape/middleware/versioner/base.rb b/lib/grape/middleware/versioner/base.rb index 2f5b07d45b..68604f14e6 100644 --- a/lib/grape/middleware/versioner/base.rb +++ b/lib/grape/middleware/versioner/base.rb @@ -9,10 +9,7 @@ class Base < Grape::Middleware::Base def self.inherited(klass) super - return if klass.name.blank? - - short_name = klass.name.demodulize.underscore - Versioner.register(short_name, klass) + Versioner.register(klass) end def default_options diff --git a/lib/grape/parser/base.rb b/lib/grape/parser/base.rb index 73a31e6623..56640d2e58 100644 --- a/lib/grape/parser/base.rb +++ b/lib/grape/parser/base.rb @@ -9,10 +9,7 @@ def self.call(_object, _env) def self.inherited(klass) super - return if klass.name.blank? - - short_name = klass.name.demodulize.underscore - Parser.register(short_name, klass) + Parser.register(klass) end end end diff --git a/lib/grape/util/registry.rb b/lib/grape/util/registry.rb index 38f2b00c12..6980445e68 100644 --- a/lib/grape/util/registry.rb +++ b/lib/grape/util/registry.rb @@ -3,13 +3,22 @@ module Grape module Util module Registry - def register(short_name, klass) + def register(klass) + short_name = build_short_name(klass) + return if short_name.nil? + warn "#{short_name} is already registered with class #{klass}" if registry.key?(short_name) registry[short_name] = klass end private + def build_short_name(klass) + return if klass.name.blank? + + klass.name.demodulize.underscore + end + def registry @registry ||= {}.with_indifferent_access end diff --git a/lib/grape/validations.rb b/lib/grape/validations.rb index 76235bc2fa..fd33071d0f 100644 --- a/lib/grape/validations.rb +++ b/lib/grape/validations.rb @@ -11,5 +11,11 @@ def require_validator(short_name) registry[short_name] end + + def build_short_name(klass) + return if klass.name.blank? + + klass.name.demodulize.underscore.delete_suffix('_validator') + end end end diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index 36c6ed4d3c..cb9b3f43e3 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -525,7 +525,7 @@ def derive_validator_options(validations) def validates_presence(validations, attrs, doc, opts) return unless validations.key?(:presence) && validations[:presence] - validate(:presence, validations.delete(:presence), attrs, doc, opts) + validate('presence', validations.delete(:presence), attrs, doc, opts) validations.delete(:message) if validations.key?(:message) end end diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index 9b65573324..890963d9b7 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -59,10 +59,7 @@ def validate!(params) def self.inherited(klass) super - return if klass.name.blank? - - short_validator_name = klass.name.demodulize.underscore.delete_suffix('_validator') - Validations.register(short_validator_name, klass) + Validations.register(klass) end def message(default_key = nil) diff --git a/spec/grape/api/custom_validations_spec.rb b/spec/grape/api/custom_validations_spec.rb index dc6ae27811..6ed10527ac 100644 --- a/spec/grape/api/custom_validations_spec.rb +++ b/spec/grape/api/custom_validations_spec.rb @@ -37,7 +37,7 @@ def validate_param!(attr_name, params) before do stub_const('DefaultLengthValidator', default_length_validator) - described_class.register(:default_length, DefaultLengthValidator) + described_class.register(DefaultLengthValidator) end after do @@ -86,7 +86,7 @@ def validate(request) before do stub_const('InBodyValidator', in_body_validator) - described_class.register(:in_body, InBodyValidator) + described_class.register(InBodyValidator) end after do @@ -129,7 +129,7 @@ def validate_param!(attr_name, _params) before do stub_const('WithMessageKeyValidator', message_key_validator) - described_class.register(:with_message_key, WithMessageKeyValidator) + described_class.register(WithMessageKeyValidator) end after do @@ -182,7 +182,7 @@ def access_header before do stub_const('AdminValidator', admin_validator) - described_class.register(:admin, AdminValidator) + described_class.register(AdminValidator) end after do @@ -248,7 +248,7 @@ def validate_param!(_attr_name, _params) before do stub_const('InstanceValidatorValidator', validator_type) - described_class.register(:instance_validator, InstanceValidatorValidator) + described_class.register(InstanceValidatorValidator) end after do diff --git a/spec/grape/validations_spec.rb b/spec/grape/validations_spec.rb index e0499ea442..5f983d2aa0 100644 --- a/spec/grape/validations_spec.rb +++ b/spec/grape/validations_spec.rb @@ -495,7 +495,7 @@ def validate_param!(attr_name, params) before do stub_const('DateRangeValidator', date_range_validator) - described_class.register(:date_range, DateRangeValidator) + described_class.register(DateRangeValidator) subject.params do optional :date_range, date_range: true, type: Hash do requires :from, type: Integer @@ -1193,7 +1193,7 @@ def validate_param!(attr_name, params) before do stub_const('CustomvalidatorValidator', custom_validator) - described_class.register(:customvalidator, CustomvalidatorValidator) + described_class.register(CustomvalidatorValidator) end after do @@ -1351,7 +1351,7 @@ def validate_param!(attr_name, params) before do stub_const('CustomvalidatorWithOptionsValidator', custom_validator_with_options) - described_class.register(:customvalidator_with_options, CustomvalidatorWithOptionsValidator) + described_class.register(CustomvalidatorWithOptionsValidator) subject.params do optional :custom, customvalidator_with_options: { text: 'im custom with options', message: 'is not custom with options!' } end From 6212c4c560e32abc7fa7c07404e4c76ff39c1e20 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Tue, 24 Dec 2024 13:39:28 +0100 Subject: [PATCH 19/19] Update CHANGELOG.md Co-authored-by: Daniel (dB.) Doubrovkine --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86ec369fbe..6ec8b6d643 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ * [#2512](https://github.com/ruby-grape/grape/pull/2512): Optimize hash alloc - [@ericproulx](https://github.com/ericproulx). * [#2513](https://github.com/ruby-grape/grape/pull/2513): Optimize Grape::Path - [@ericproulx](https://github.com/ericproulx). * [#2514](https://github.com/ruby-grape/grape/pull/2514): Add rails 8.0 to CI - [@ericproulx](https://github.com/ericproulx). -* [#7](https://github.com/ericproulx/grape/pull/7): Dynamic registration for parsers, formatters, versioners - [@ericproulx](https://github.com/ericproulx). +* [#2516](https://github.com/ruby-grape/grape/pull/2516): Dynamic registration for parsers, formatters, versioners - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes