From d2915f52a40cac7b35cb30f2d45e7bd85820d43b Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Fri, 4 Oct 2019 10:07:53 +0300 Subject: [PATCH 01/12] replace Virtus with dry-types As we know Virtus isn't maintained anymore, so it makes sense to use something different for coercing. There is a dry-types lib which was created as a replacement for Virtus (at least a part of it). Although, it is only about coercion. This change gets rid of Virtus and adds dry-types to the stack. Almost, all tests pass. However, there are a few tests which were modified, actually, they didn't make any sense. Yeah, Virtus was providing some "magic" to make them green, but I think they were unreal. The structure inside didn't change match. I tried to integrate dry-types without huge refactoring. There might be more improvements after this change. If people want to use Virtus for custom types, it is possible after implementing a parse method which initializes a model. class User include Virtus.model attribute :id, Integer attribute :name, String def self.parse(*args) new(*args) end end --- Gemfile | 1 + grape.gemspec | 2 +- lib/grape.rb | 2 - lib/grape/dsl/helpers.rb | 2 +- lib/grape/validations/params_scope.rb | 4 +- lib/grape/validations/types.rb | 35 +------ lib/grape/validations/types/array_coercer.rb | 54 +++++++++++ lib/grape/validations/types/build_coercer.rb | 91 +++++++++---------- .../validations/types/custom_type_coercer.rb | 67 ++++---------- .../types/custom_type_collection_coercer.rb | 33 ++----- .../validations/types/dry_type_coercer.rb | 36 ++++++++ lib/grape/validations/types/file.rb | 17 ++-- lib/grape/validations/types/json.rb | 17 ++-- .../types/multiple_type_coercer.rb | 45 +++------ .../validations/types/primitive_coercer.rb | 56 ++++++++++++ lib/grape/validations/types/set_coercer.rb | 36 ++++++++ .../types/variant_collection_coercer.rb | 14 +-- .../types/virtus_collection_patch.rb | 16 ---- lib/grape/validations/validators/coerce.rb | 59 ++++++------ .../api/defines_boolean_in_params_spec.rb | 6 +- spec/grape/dsl/helpers_spec.rb | 4 +- spec/grape/validations/params_scope_spec.rb | 2 +- spec/grape/validations/types_spec.rb | 41 ++------- .../validations/validators/coerce_spec.rb | 29 ++++-- .../validators/except_values_spec.rb | 2 +- 25 files changed, 356 insertions(+), 315 deletions(-) create mode 100644 lib/grape/validations/types/array_coercer.rb create mode 100644 lib/grape/validations/types/dry_type_coercer.rb create mode 100644 lib/grape/validations/types/primitive_coercer.rb create mode 100644 lib/grape/validations/types/set_coercer.rb delete mode 100644 lib/grape/validations/types/virtus_collection_patch.rb diff --git a/Gemfile b/Gemfile index b088fc793..28b9fe058 100644 --- a/Gemfile +++ b/Gemfile @@ -30,4 +30,5 @@ group :test do gem 'rack-test', '~> 1.1.0' gem 'rspec', '~> 3.0' gem 'ruby-grape-danger', '~> 0.1.0', require: false + gem 'virtus' end diff --git a/grape.gemspec b/grape.gemspec index a483dd8b7..7cce5f3d1 100644 --- a/grape.gemspec +++ b/grape.gemspec @@ -17,7 +17,7 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'mustermann-grape', '~> 1.0.0' s.add_runtime_dependency 'rack', '>= 1.3.0' s.add_runtime_dependency 'rack-accept' - s.add_runtime_dependency 'virtus', '>= 1.0.0' + s.add_runtime_dependency 'dry-types', '~> 1.1.1' s.files = Dir['**/*'].keep_if { |file| File.file?(file) } s.test_files = Dir['spec/**/*'] diff --git a/lib/grape.rb b/lib/grape.rb index 79818a1ab..d86bc4750 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -20,8 +20,6 @@ require 'i18n' require 'thread' -require 'virtus' - I18n.load_path << File.expand_path('../grape/locale/en.yml', __FILE__) module Grape diff --git a/lib/grape/dsl/helpers.rb b/lib/grape/dsl/helpers.rb index d58e2cf02..718511799 100644 --- a/lib/grape/dsl/helpers.rb +++ b/lib/grape/dsl/helpers.rb @@ -65,7 +65,7 @@ def include_all_in_scope def define_boolean_in_mod(mod) return if defined? mod::Boolean - mod.const_set('Boolean', Virtus::Attribute::Boolean) + mod.const_set('Boolean', Grape::API::Boolean) end def inject_api_helpers_to_mod(mod, &_block) diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index 75a287c7f..c1192e580 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -427,8 +427,8 @@ def validate_value_coercion(coerce_type, *values_list) values_list.each do |values| next if !values || values.is_a?(Proc) value_types = values.is_a?(Range) ? [values.begin, values.end] : values - if coerce_type == Virtus::Attribute::Boolean - value_types = value_types.map { |type| Virtus::Attribute.build(type) } + if coerce_type == Grape::API::Boolean + value_types = value_types.map { |type| Grape::API::Boolean.build(type) } end unless value_types.all? { |v| v.is_a? coerce_type } raise Grape::Exceptions::IncompatibleOptionValues.new(:type, coerce_type, :values, values) diff --git a/lib/grape/validations/types.rb b/lib/grape/validations/types.rb index 9320668f0..ee2ab22ed 100644 --- a/lib/grape/validations/types.rb +++ b/lib/grape/validations/types.rb @@ -6,10 +6,6 @@ require_relative 'types/json' require_relative 'types/file' -# Patch for Virtus::Attribute::Collection -# See the file for more details -require_relative 'types/virtus_collection_patch' - module Grape module Validations # Module for code related to grape's system for @@ -27,8 +23,7 @@ module Types # a parameter value could not be coerced. class InvalidValue; end - # Types representing a single value, which are coerced through Virtus - # or special logic in Grape. + # Types representing a single value, which are coerced. PRIMITIVES = [ # Numerical Integer, @@ -42,10 +37,12 @@ class InvalidValue; end Time, # Misc - Virtus::Attribute::Boolean, + Grape::API::Boolean, String, Symbol, - Rack::Multipart::UploadedFile + Rack::Multipart::UploadedFile, + TrueClass, + FalseClass ].freeze # Types representing data structures. @@ -86,8 +83,6 @@ def self.primitive?(type) # @param type [Class] type to check # @return [Boolean] whether or not the type is known by Grape as a valid # data structure type - # @note This method does not yet consider 'complex types', which inherit - # Virtus.model. def self.structure?(type) STRUCTURES.include?(type) end @@ -104,25 +99,6 @@ def self.multiple?(type) (type.is_a?(Array) || type.is_a?(Set)) && type.size > 1 end - # Does the given class implement a type system that Grape - # (i.e. the underlying virtus attribute system) supports - # out-of-the-box? Currently supported are +axiom-types+ - # and +virtus+. - # - # The type will be passed to +Virtus::Attribute.build+, - # and the resulting attribute object will be expected to - # respond correctly to +coerce+ and +value_coerced?+. - # - # @param type [Class] type to check - # @return [Boolean] +true+ where the type is recognized - def self.recognized?(type) - return false if type.is_a?(Array) || type.is_a?(Set) - - type.is_a?(Virtus::Attribute) || - type.ancestors.include?(Axiom::Types::Type) || - type.include?(Virtus::Model::Core) - end - # Does Grape provide special coercion and validation # routines for the given class? This does not include # automatic handling for primitives, structures and @@ -152,7 +128,6 @@ def self.custom?(type) !primitive?(type) && !structure?(type) && !multiple?(type) && - !recognized?(type) && !special?(type) && type.respond_to?(:parse) && type.method(:parse).arity == 1 diff --git a/lib/grape/validations/types/array_coercer.rb b/lib/grape/validations/types/array_coercer.rb new file mode 100644 index 000000000..01279cffe --- /dev/null +++ b/lib/grape/validations/types/array_coercer.rb @@ -0,0 +1,54 @@ +require_relative 'dry_type_coercer' + +module Grape + module Validations + module Types + # Coerces elements in an array. It might be an array of strings or integers or + # anything else. + # + # It could've been possible to use an +of+ + # method (https://dry-rb.org/gems/dry-types/1.2/array-with-member/) + # provided by dry-types. Unfortunately, it doesn't work for Grape because of + # behavior of Virtus which was used earlier, a `Grape::Validations::Types::PrimitiveCoercer` + # maintains Virtus behavior in coercing. + class ArrayCoercer < DryTypeCoercer + def initialize(type, strict = false) + super + + @coercer = scope::Array + @elem_coercer = PrimitiveCoercer.new(type.first, strict) + end + + def call(_val) + collection = super + + return collection if collection.is_a?(InvalidValue) + + coerce_elements collection + end + + protected + + def coerce_elements(collection) + collection.each_with_index do |elem, index| + return InvalidValue.new if would_virtus_reject?(elem) + + coerced_elem = @elem_coercer.call(elem) + + return coerced_elem if coerced_elem.is_a?(InvalidValue) + + collection[index] = coerced_elem + end + + collection + end + + # This method maintaine logic which was defined by Virtus for arrays. + # Virtus doesn't allow nil in arrays. + def would_virtus_reject?(val) + val.nil? + end + end + end + end +end diff --git a/lib/grape/validations/types/build_coercer.rb b/lib/grape/validations/types/build_coercer.rb index 2a8e9968c..9ed1e5d6f 100644 --- a/lib/grape/validations/types/build_coercer.rb +++ b/lib/grape/validations/types/build_coercer.rb @@ -1,78 +1,73 @@ +require_relative 'array_coercer' +require_relative 'set_coercer' +require_relative 'primitive_coercer' + module Grape module Validations module Types - # Work out the +Virtus::Attribute+ object to - # use for coercing strings to the given +type+. - # Coercion +method+ will be inferred if none is - # supplied. + # 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+. # - # If a +Virtus::Attribute+ object already built - # with +Virtus::Attribute.build+ is supplied as - # the +type+ it will be returned and +method+ - # will be ignored. + # +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. # - # See {CustomTypeCoercer} for further details - # about coercion and type-checking inference. + # 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 [Virtus::Attribute] object to be used + # @return [Object] object to be used # for coercion and type validation - def self.build_coercer(type, method = nil) - cache_instance(type, method) do - create_coercer_instance(type, method) + 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 = nil) - # Accept pre-rolled virtus attributes without interference - return type if type.is_a? Virtus::Attribute - - converter_options = { - nullify_blank: true - } - conversion_type = if method == JSON - Object - # because we want just parsed JSON content: - # if type is Array and data is `"{}"` - # result will be [] because Virtus converts hashes - # to arrays - else - type - end - + def self.create_coercer_instance(type, method, strict) # Use a special coercer for multiply-typed parameters. if Types.multiple?(type) - converter_options[:coercer] = Types::MultipleTypeCoercer.new(type, method) - conversion_type = Object + MultipleTypeCoercer.new(type, method) # Use a special coercer for custom types and coercion methods. elsif method || Types.custom?(type) - converter_options[:coercer] = Types::CustomTypeCoercer.new(type, method) + 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) - converter_options[:coercer] = Types::CustomTypeCollectionCoercer.new( + Types::CustomTypeCollectionCoercer.new( type.first, type.is_a?(Set) ) - - # Grape swaps in its own Virtus::Attribute implementations - # for certain special types that merit first-class support - # (but not if a custom coercion method has been supplied). elsif Types.special?(type) - conversion_type = Types::SPECIAL[type] + Types::SPECIAL[type].new + elsif type.is_a?(Array) + ArrayCoercer.new type, strict + elsif type.is_a?(Set) + SetCoercer.new type, strict + else + PrimitiveCoercer.new type, strict end - - # Virtus will infer coercion and validation rules - # for many common ruby types. - Virtus::Attribute.build(conversion_type, converter_options) end - def self.cache_instance(type, method, &_block) - key = cache_key(type, method) + def self.cache_instance(type, method, strict, &_block) + key = cache_key(type, method, strict) return @__cache[key] if @__cache.key?(key) @@ -85,8 +80,8 @@ def self.cache_instance(type, method, &_block) instance end - def self.cache_key(type, method) - [type, method].compact.map(&:to_s).join('_') + def self.cache_key(type, method, strict) + [type, method, strict].compact.map(&:to_s).join('_') end instance_variable_set(:@__cache, {}) diff --git a/lib/grape/validations/types/custom_type_coercer.rb b/lib/grape/validations/types/custom_type_coercer.rb index f6b26cf83..ffd4cbbd8 100644 --- a/lib/grape/validations/types/custom_type_coercer.rb +++ b/lib/grape/validations/types/custom_type_coercer.rb @@ -1,21 +1,6 @@ module Grape module Validations module Types - # Instances of this class may be passed to - # +Virtus::Attribute.build+ as the +:coercer+ - # option for custom types that do not otherwise - # satisfy the requirements for +Virtus::Attribute::coerce+ - # and +Virtus::Attribute::value_coerced?+ to work - # as expected. - # - # Subclasses of +Virtus::Attribute+ or +Axiom::Types::Type+ - # (or for which an axiom type can be inferred, i.e. the - # primitives, +Date+, +Time+, etc.) do not need any such - # coercer to be passed with them. - # - # Coercion - # -------- - # # This class will detect type classes that implement # a class-level +parse+ method. The method should accept one # +String+ argument and should return the value coerced to @@ -30,14 +15,14 @@ module Types # Type Checking # ------------- # - # Calls to +value_coerced?+ will consult this class to check + # Calls to +coerced?+ will consult this class to check # that the coerced value produced above is in fact of the # expected type. By default this class performs a basic check # against the type supplied, but this behaviour will be # overridden if the class implements a class-level # +coerced?+ or +parsed?+ method. This method # will receive a single parameter that is the coerced value - # and should return +true+ iff the value meets type expectations. + # and should return +true+ if the value meets type expectations. # Arbitrary assertions may be made here but the grape validation # system should be preferred. # @@ -46,15 +31,6 @@ module Types # contract as +coerced?+, and must be supplied with a coercion # +method+. class CustomTypeCoercer - # Uses +Virtus::Attribute.build+ to build a new - # attribute that makes use of this class for - # coercion and type validation logic. - # - # @return [Virtus::Attribute] - def self.build(type, method = nil) - Virtus::Attribute.build(type, coercer: new(type, method)) - end - # A new coercer for the given type specification # and coercion method. # @@ -64,37 +40,23 @@ def self.build(type, method = nil) # optional coercion method. See class docs. def initialize(type, method = nil) coercion_method = infer_coercion_method type, method - @method = enforce_symbolized_keys type, coercion_method - @type_check = infer_type_check(type) end - # This method is called from somewhere within - # +Virtus::Attribute::coerce+ in order to coerce - # the given value. + # Coerces the given value. # # @param value [String] value to be coerced, in grape # this should always be a string. # @return [Object] the coerced result - def call(value) - @method.call value + def call(val) + coerced_val = @method.call(val) + return InvalidValue.new unless coerced?(coerced_val) + coerced_val end - # This method is called from somewhere within - # +Virtus::Attribute::value_coerced?+ in order to - # assert that the value has been coerced successfully. - # - # @param _primitive [Axiom::Types::Type] primitive type - # for the coercion as detected by axiom-types' inference - # system. For custom types this is typically not much use - # (i.e. it is +Axiom::Types::Object+) unless special - # inference rules have been declared for the type. - # @param value [Object] a coerced result returned from {#call} - # @return [true,false] whether or not the coerced value - # satisfies type requirements. - def success?(_primitive, value) - @type_check.call value + def coerced?(val) + @type_check.call val end private @@ -160,10 +122,15 @@ def enforce_symbolized_keys(type, method) # Collections have all values processed individually if [Array, Set].include?(type) lambda do |val| - method.call(val).tap do |new_value| - new_value.map do |item| - item.is_a?(Hash) ? symbolize_keys(item) : item + # if the value cannot be parsed, return InvalidValue + begin + coerced_value = method.call(val).tap do |new_value| + new_value.map do |item| + item.is_a?(Hash) ? symbolize_keys(item) : item + end end + rescue => _e + InvalidValue.new end end diff --git a/lib/grape/validations/types/custom_type_collection_coercer.rb b/lib/grape/validations/types/custom_type_collection_coercer.rb index 7534420fe..ebdae4d40 100644 --- a/lib/grape/validations/types/custom_type_collection_coercer.rb +++ b/lib/grape/validations/types/custom_type_collection_coercer.rb @@ -1,12 +1,6 @@ module Grape module Validations module Types - # Instances of this class may be passed to - # +Virtus::Attribute.build+ as the +:coercer+ - # option, to handle collections of types that - # provide their own parsing (and optionally, - # type-checking) functionality. - # # See {CustomTypeCoercer} for details on types # that will be supported by this by this coercer. # This coercer works in the same way as +CustomTypeCoercer+ @@ -38,32 +32,21 @@ def initialize(type, set = false) @set = set end - # This method is called from somewhere within - # +Virtus::Attribute::coerce+ in order to coerce - # the given value. + # Coerces the given value. # # @param value [Array] an array of values to be coerced # @return [Array,Set] the coerced result. May be an +Array+ or a # +Set+ depending on the setting given to the constructor def call(value) - coerced = value.map { |item| super(item) } + coerced = value.map do |item| + coerced_item = super(item) - @set ? Set.new(coerced) : coerced - end + return coerced_item if coerced_item.is_a?(InvalidValue) - # This method is called from somewhere within - # +Virtus::Attribute::value_coerced?+ in order to assert - # that the all of the values in the array have been coerced - # successfully. - # - # @param primitive [Axiom::Types::Type] primitive type for - # the coercion as deteced by axiom-types' inference system. - # @param value [Enumerable] a coerced result returned from {#call} - # @return [true,false] whether or not all of the coerced values in - # the collection satisfy type requirements. - def success?(primitive, value) - value.is_a?(@set ? Set : Array) && - value.all? { |item| super(primitive, item) } + coerced_item + end + + @set ? Set.new(coerced) : coerced end end end diff --git a/lib/grape/validations/types/dry_type_coercer.rb b/lib/grape/validations/types/dry_type_coercer.rb new file mode 100644 index 000000000..34537a29b --- /dev/null +++ b/lib/grape/validations/types/dry_type_coercer.rb @@ -0,0 +1,36 @@ +require 'dry-types' + +module DryTypes + include Dry.Types() +end + +module Grape + module Validations + module Types + # A base class for classes which must identify a coercer to be used. + # If the +strict+ argument is true, it won't coerce the given value + # but check its type. More information there + # https://dry-rb.org/gems/dry-types/1.2/built-in-types/ + class DryTypeCoercer + def initialize(type, strict = false) + @type = type + @scope = strict ? DryTypes::Strict : DryTypes::Params + end + + # Coerces the given value to a type which was specified during + # initialization as a type argument. + # + # @param val [Object] + def call(val) + @coercer[val] + rescue Dry::Types::CoercionError => _e + InvalidValue.new + end + + protected + + attr_reader :scope, :type + end + end + end +end diff --git a/lib/grape/validations/types/file.rb b/lib/grape/validations/types/file.rb index 62aa3f694..0a47bc370 100644 --- a/lib/grape/validations/types/file.rb +++ b/lib/grape/validations/types/file.rb @@ -1,21 +1,20 @@ module Grape module Validations module Types - # +Virtus::Attribute+ implementation for parameters - # that are multipart file objects. Actual handling - # of these objects is provided by +Rack::Request+; - # this class is here only to assert that rack's - # handling has succeeded, and to prevent virtus - # from interfering. - class File < Virtus::Attribute - def coerce(input) + # Implementation for parameters that are multipart file objects. + # Actual handling of these objects is provided by +Rack::Request+; + # this class is here only to assert that rack's handling has succeeded. + class File + def call(input) + return InvalidValue.new unless coerced?(input) + # Processing of multipart file objects # is already taken care of by Rack::Request. # Nothing to do here. input end - def value_coerced?(value) + def coerced?(value) # Rack::Request creates a Hash with filename, # content type and an IO object. Do a bit of basic # duck-typing. diff --git a/lib/grape/validations/types/json.rb b/lib/grape/validations/types/json.rb index 220d1db6d..c464477b1 100644 --- a/lib/grape/validations/types/json.rb +++ b/lib/grape/validations/types/json.rb @@ -3,19 +3,20 @@ module Grape module Validations module Types - # +Virtus::Attribute+ implementation that handles coercion - # and type checking for parameters that are complex types - # given as JSON-encoded strings. It accepts both JSON objects + # Handles coercion and type checking for parameters that are complex + # types given as JSON-encoded strings. It accepts both JSON objects # and arrays of objects, and will coerce the input to a +Hash+ # or +Array+ object respectively. In either case the Grape # validation system will apply nested validation rules to # all returned objects. - class Json < Virtus::Attribute + class Json # Coerce the input into a JSON-like data structure. # # @param input [String] a JSON-encoded parameter value # @return [Hash,Array,nil] - def coerce(input) + def call(input) + return input if coerced?(input) + # Allow nulls and blank strings return if input.nil? || input =~ /^\s*$/ JSON.parse(input, symbolize_names: true) @@ -26,7 +27,7 @@ def coerce(input) # # @param value [Object] result of {#coerce} # @return [true,false] - def value_coerced?(value) + def coerced?(value) value.is_a?(::Hash) || coerced_collection?(value) end @@ -50,13 +51,13 @@ class JsonArray < Json # # @param input [String] JSON-encoded parameter value # @return [Array] - def coerce(input) + def call(input) json = super Array.wrap(json) unless json.nil? end # See {Json#coerced_collection?} - def value_coerced?(value) + def coerced?(value) coerced_collection? value end end diff --git a/lib/grape/validations/types/multiple_type_coercer.rb b/lib/grape/validations/types/multiple_type_coercer.rb index e78745382..215030022 100644 --- a/lib/grape/validations/types/multiple_type_coercer.rb +++ b/lib/grape/validations/types/multiple_type_coercer.rb @@ -22,53 +22,32 @@ def initialize(types, method = nil) @type_coercers = types.map do |type| if Types.multiple? type - VariantCollectionCoercer.new type + VariantCollectionCoercer.new type, @method else - Types.build_coercer type + Types.build_coercer type, strict: !@method.nil? end end end - # This method is called from somewhere within - # +Virtus::Attribute::coerce+ in order to coerce - # the given value. + # Coerces the given value. # - # @param value [String] value to be coerced, in grape + # @param val [String] value to be coerced, in grape # this should always be a string. # @return [Object,InvalidValue] the coerced result, or an instance # of {InvalidValue} if the value could not be coerced. - def call(value) - return @method.call(value) if @method + def call(val) + # once the value is coerced by the custom method, its type should be checked + val = @method.call(val) if @method + + coerced_val = InvalidValue.new @type_coercers.each do |coercer| - coerced = coercer.coerce(value) + coerced_val = coercer.call(val) - return coerced if coercer.value_coerced? coerced + return coerced_val unless coerced_val.is_a?(InvalidValue) end - # Declare that we couldn't coerce the value in such a way - # that Grape won't ask us again if the value is valid - InvalidValue.new - end - - # This method is called from somewhere within - # +Virtus::Attribute::value_coerced?+ in order to - # assert that the value has been coerced successfully. - # Due to Grape's design this will in fact only be called - # if a custom coercion method is being used, since {#call} - # returns an {InvalidValue} object if the value could not - # be coerced. - # - # @param _primitive [Axiom::Types::Type] primitive type - # for the coercion as detected by axiom-types' inference - # system. For custom types this is typically not much use - # (i.e. it is +Axiom::Types::Object+) unless special - # inference rules have been declared for the type. - # @param value [Object] a coerced result returned from {#call} - # @return [true,false] whether or not the coerced value - # satisfies type requirements. - def success?(_primitive, value) - @type_coercers.any? { |coercer| coercer.value_coerced? value } + coerced_val end end end diff --git a/lib/grape/validations/types/primitive_coercer.rb b/lib/grape/validations/types/primitive_coercer.rb new file mode 100644 index 000000000..e467c3e8e --- /dev/null +++ b/lib/grape/validations/types/primitive_coercer.rb @@ -0,0 +1,56 @@ +require_relative 'dry_type_coercer' + +module Grape + module Validations + module Types + # Coerces the given value to a type defined via a +type+ argument during + # initialization. + class PrimitiveCoercer < DryTypeCoercer + MAPPING = { + Grape::API::Boolean => DryTypes::Params::Bool, + + # unfortunatelly, a +Params+ scope doesn't contain String + String => DryTypes::Coercible::String + } + + STRICT_MAPPING = { + Grape::API::Boolean => DryTypes::Strict::Bool + } + + def initialize(type, strict = false) + super + + @type = type + + @coercer = if strict + STRICT_MAPPING.fetch(type) { scope.const_get(type.name) } + else + MAPPING.fetch(type) { scope.const_get(type.name) } + end + end + + def call(val) + return InvalidValue.new if would_virtus_reject?(val) + return nil if val.nil? + return '' if val == '' + + super + end + + protected + + attr_reader :type + + # This method maintaine logic which was defined by Virtus. For example, + # dry-types is ok to convert an array or a hash to a string, it is supported, + # but Virtus wouldn't accept it. So, this method only exists to not introduce + # breaking changes. + def would_virtus_reject?(val) + (val.is_a?(Array) && type == String) || + (val.is_a?(String) && type == Hash) || + (val.is_a?(Hash) && type == String) + end + end + end + end +end diff --git a/lib/grape/validations/types/set_coercer.rb b/lib/grape/validations/types/set_coercer.rb new file mode 100644 index 000000000..1f351bb09 --- /dev/null +++ b/lib/grape/validations/types/set_coercer.rb @@ -0,0 +1,36 @@ +require 'set' +require_relative 'dry_type_coercer' + +module Grape + module Validations + module Types + # Takes the given array and converts it to a set. Every element of the set + # is also coerced. + class SetCoercer < DryTypeCoercer + def initialize(type, strict = false) + super + + @elem_coercer = PrimitiveCoercer.new(type.first, strict) + end + + def call(value) + return InvalidValue.new unless value.is_a?(Array) + + coerce_elements(value) + end + + protected + + def coerce_elements(collection) + collection.each_with_object(Set.new) do |elem, memo| + coerced_elem = @elem_coercer.call(elem) + + return coerced_elem if coerced_elem.is_a?(InvalidValue) + + memo.add(coerced_elem) + end + end + end + end + end +end diff --git a/lib/grape/validations/types/variant_collection_coercer.rb b/lib/grape/validations/types/variant_collection_coercer.rb index f387457ee..d841b6e55 100644 --- a/lib/grape/validations/types/variant_collection_coercer.rb +++ b/lib/grape/validations/types/variant_collection_coercer.rb @@ -3,7 +3,7 @@ module Validations module Types # This class wraps {MultipleTypeCoercer}, for use with collections # that allow members of more than one type. - class VariantCollectionCoercer < Virtus::Attribute + class VariantCollectionCoercer # Construct a new coercer that will attempt to coerce # a list of values such that all members are of one of # the given types. The container may also optionally be @@ -30,7 +30,7 @@ def initialize(types, method = nil) # @return [Array,Set,InvalidValue] # the coerced result, or an instance # of {InvalidValue} if the value could not be coerced. - def coerce(value) + def call(value) return InvalidValue.new unless value.is_a? Array value = @@ -43,16 +43,6 @@ def coerce(value) value end - - # Assert that the value has been coerced successfully. - # - # @param value [Object] a coerced result returned from {#coerce} - # @return [true,false] whether or not the coerced value - # satisfies type requirements. - def value_coerced?(value) - value.is_a?(@types.class) && - value.all? { |v| @member_coercer.success?(@types, v) } - end end end end diff --git a/lib/grape/validations/types/virtus_collection_patch.rb b/lib/grape/validations/types/virtus_collection_patch.rb deleted file mode 100644 index eab5208b2..000000000 --- a/lib/grape/validations/types/virtus_collection_patch.rb +++ /dev/null @@ -1,16 +0,0 @@ -require 'virtus/attribute/collection' - -# See https://github.com/solnic/virtus/pull/343 -# This monkey-patch fixes type validation for collections, -# ensuring that type assertions are applied to collection -# members. -# -# This patch duplicates the code in the above pull request. -# Once the request, or equivalent functionality, has been -# published into the +virtus+ gem this file should be deleted. -Virtus::Attribute::Collection.class_eval do - # @api public - def value_coerced?(value) - super && value.all? { |item| member_type.value_coerced? item } - end -end diff --git a/lib/grape/validations/validators/coerce.rb b/lib/grape/validations/validators/coerce.rb index 4638d67f4..51015549c 100644 --- a/lib/grape/validations/validators/coerce.rb +++ b/lib/grape/validations/validators/coerce.rb @@ -1,9 +1,15 @@ module Grape class API - Boolean = Virtus::Attribute::Boolean + class Boolean + def self.build(val) + return nil if val != true && val != false + + new + end + end class Instance - Boolean = Virtus::Attribute::Boolean + Boolean = Grape::API::Boolean end end @@ -11,7 +17,12 @@ module Validations class CoerceValidator < Base def initialize(*_args) super - @converter = Types.build_coercer(type, @option[:method]) + + if type.is_a?(Grape::Validations::Types::VariantCollectionCoercer) + @converter = type + else + @converter = Types.build_coercer(type, method: @option[:method]) + end end def validate(request) @@ -19,11 +30,14 @@ def validate(request) end def validate_param!(attr_name, params) - raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:coerce) unless params.is_a? Hash - return unless requires_coercion?(params[attr_name]) + raise validation_exception(attr_name) unless params.is_a? Hash + new_value = coerce_value(params[attr_name]) - raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:coerce) unless valid_type?(new_value) - params[attr_name] = new_value + + raise validation_exception(attr_name) unless valid_type?(new_value) + + # fixes some problems with Hashie::Mash + params[attr_name] = new_value unless params[attr_name] == new_value end private @@ -33,31 +47,25 @@ def validate_param!(attr_name, params) # # See {Types.build_coercer} # - # @return [Virtus::Attribute] + # @return [Object] attr_reader :converter def valid_type?(val) - # Special value to denote coercion failure - return false if val.instance_of?(Types::InvalidValue) - - # Allow nil, to ignore when a parameter is absent - return true if val.nil? - - converter.value_coerced? val + !val.is_a?(Types::InvalidValue) end def coerce_value(val) - # Don't coerce things other than nil to Arrays or Hashes - unless (@option[:method] && !val.nil?) || type.is_a?(Virtus::Attribute) - return val || [] if type == Array - return val || Set.new if type == Set - return val || {} if type == Hash + # define default values for structures, the dry-types lib which is used + # for coercion doesn't accept nil as a value, so it would fail + if val.nil? + return [] if type == Array || type.is_a?(Array) + return Set.new if type == Set + return {} if type == Hash end - converter.coerce(val) + converter.call(val) - # not the prettiest but some invalid coercion can currently trigger - # errors in Virtus (see coerce_spec.rb:75) + # Some custom types might fail, so it should be treated as an invalid value rescue Types::InvalidValue.new end @@ -69,9 +77,8 @@ def type @option[:type].is_a?(Hash) ? @option[:type][:value] : @option[:type] end - def requires_coercion?(value) - # JSON types do not require coercion if value is valid - !valid_type?(value) || converter.coercer.respond_to?(:method) && !converter.is_a?(Grape::Validations::Types::Json) + def validation_exception(attr_name) + Grape::Exceptions::Validation.new(params: [@scope.full_name(attr_name)], message: message(:coerce)) end end end diff --git a/spec/grape/api/defines_boolean_in_params_spec.rb b/spec/grape/api/defines_boolean_in_params_spec.rb index 166b2d0a8..d057bbbce 100644 --- a/spec/grape/api/defines_boolean_in_params_spec.rb +++ b/spec/grape/api/defines_boolean_in_params_spec.rb @@ -21,7 +21,7 @@ def app { class: 'TrueClass', value: true }.to_s end - it 'sets Boolean as a Virtus::Attribute::Boolean' do + it 'sets Boolean as a type' do post '/echo?message=true' expect(last_response.status).to eq(201) expect(last_response.body).to eq expected_body @@ -29,8 +29,8 @@ def app context 'Params endpoint type' do subject { DefinesBooleanInstanceSpec::API.new.router.map['POST'].first.options[:params]['message'][:type] } - it 'params type is a Virtus::Attribute::Boolean' do - is_expected.to eq 'Virtus::Attribute::Boolean' + it 'params type is a boolean' do + is_expected.to eq 'Grape::API::Boolean' end end end diff --git a/spec/grape/dsl/helpers_spec.rb b/spec/grape/dsl/helpers_spec.rb index 6e24cfe00..c1ce8de70 100644 --- a/spec/grape/dsl/helpers_spec.rb +++ b/spec/grape/dsl/helpers_spec.rb @@ -75,9 +75,9 @@ def test end context 'with an external file' do - it 'sets Boolean as a Virtus::Attribute::Boolean' do + it 'sets Boolean as a Grape::API::Boolean' do subject.helpers BooleanParam - expect(subject.first_mod::Boolean).to eq Virtus::Attribute::Boolean + expect(subject.first_mod::Boolean).to eq Grape::API::Boolean end end diff --git a/spec/grape/validations/params_scope_spec.rb b/spec/grape/validations/params_scope_spec.rb index 3658efb87..6faf5680b 100644 --- a/spec/grape/validations/params_scope_spec.rb +++ b/spec/grape/validations/params_scope_spec.rb @@ -30,7 +30,7 @@ def app context 'when the default value is false' do before do subject.params do - optional :bool, type: Virtus::Attribute::Boolean, default: false + optional :bool, type: Grape::API::Boolean, default: false end subject.get end diff --git a/spec/grape/validations/types_spec.rb b/spec/grape/validations/types_spec.rb index 353d75fda..1380b1016 100644 --- a/spec/grape/validations/types_spec.rb +++ b/spec/grape/validations/types_spec.rb @@ -11,29 +11,10 @@ def self.parse; end end end - VirtusA = Virtus::Attribute.build(String) - - module VirtusModule - include Virtus.module - end - - class VirtusB - include VirtusModule - end - - class VirtusC - include Virtus.model - end - - MyAxiom = Axiom::Types::String.new do - minimum_length 1 - maximum_length 30 - end - describe '::primitive?' do [ Integer, Float, Numeric, BigDecimal, - Virtus::Attribute::Boolean, String, Symbol, + Grape::API::Boolean, String, Symbol, Date, DateTime, Time, Rack::Multipart::UploadedFile ].each do |type| it "recognizes #{type} as a primitive" do @@ -57,16 +38,6 @@ class VirtusC end end - describe '::recognized?' do - [ - VirtusA, VirtusB, VirtusC, MyAxiom - ].each do |type| - it "recognizes #{type}" do - expect(described_class.recognized?(type)).to be_truthy - end - end - end - describe '::special?' do [ JSON, Array[JSON], File, Rack::Multipart::UploadedFile @@ -97,14 +68,14 @@ class VirtusC expect(described_class.instance_variable_get(:@__cache_write_lock)).to be_a(Mutex) end - it 'caches the result of the Virtus::Attribute.build method' do + it 'caches the result of the build_coercer method' do original_cache = described_class.instance_variable_get(:@__cache) described_class.instance_variable_set(:@__cache, {}) - coercer = 'TestCoercer' - expect(Virtus::Attribute).to receive(:build).once.and_return(coercer) - expect(described_class.build_coercer(Array[String])).to eq(coercer) - expect(described_class.build_coercer(Array[String])).to eq(coercer) + a_coercer = described_class.build_coercer(Array[String]) + b_coercer = described_class.build_coercer(Array[String]) + + expect(a_coercer.object_id).to eq(b_coercer.object_id) described_class.instance_variable_set(:@__cache, original_cache) end diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index 94f66e873..6e65ea0d9 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/spec/grape/validations/validators/coerce_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' +require 'virtus' describe Grape::Validations::CoerceValidator do subject do @@ -15,6 +16,10 @@ class User include Virtus.model attribute :id, Integer attribute :name, String + + def self.parse(attrs) + new(attrs) + end end end @@ -96,6 +101,7 @@ class User it 'respects :coerce_with' do get '/', a: 'yup' + expect(last_response.status).to eq(200) expect(last_response.body).to eq('TrueClass') end @@ -197,7 +203,7 @@ class User it 'Array of Bools' do subject.params do - requires :arry, coerce: Array[Virtus::Attribute::Boolean] + requires :arry, coerce: Array[Grape::API::Boolean] end subject.get '/array' do params[:arry][0].class @@ -295,7 +301,7 @@ def self.parsed?(value) it 'Set of Bools' do subject.params do - requires :set, coerce: Set[Virtus::Attribute::Boolean] + requires :set, coerce: Set[Grape::API::Boolean] end subject.get '/set' do params[:set].first.class @@ -309,7 +315,7 @@ def self.parsed?(value) it 'Bool' do subject.params do - requires :bool, coerce: Virtus::Attribute::Boolean + requires :bool, coerce: Grape::API::Boolean end subject.get '/bool' do params[:bool].class @@ -443,19 +449,19 @@ def self.parsed?(value) it 'parses parameters with Array[String] type' do subject.params do - requires :values, type: Array[String], coerce_with: ->(val) { val.split(/\s+/).map(&:to_i) } + requires :values, type: Array[String], coerce_with: ->(val) { val.split(/\s+/) } end - subject.get '/ints' do + subject.get '/strings' do params[:values] end - get '/ints', values: '1 2 3 4' + get '/strings', values: '1 2 3 4' expect(last_response.status).to eq(200) expect(JSON.parse(last_response.body)).to eq(%w[1 2 3 4]) - get '/ints', values: 'a b c d' + get '/strings', values: 'a b c d' expect(last_response.status).to eq(200) - expect(JSON.parse(last_response.body)).to eq(%w[0 0 0 0]) + expect(JSON.parse(last_response.body)).to eq(%w[a b c d]) end it 'parses parameters with Array[Integer] type' do @@ -914,14 +920,17 @@ def self.parsed?(value) end context 'converter' do - it 'does not build Virtus::Attribute multiple times' do + it 'does not build a coercer multiple times' do subject.params do requires :something, type: Array[String] end subject.get do end - expect(Virtus::Attribute).to receive(:build).at_most(2).times.and_call_original + expect(Grape::Validations::Types::ArrayCoercer).to( + receive(:new).at_most(:once).and_call_original + ) + 10.times { get '/' } end end diff --git a/spec/grape/validations/validators/except_values_spec.rb b/spec/grape/validations/validators/except_values_spec.rb index 7030774cc..9e0787697 100644 --- a/spec/grape/validations/validators/except_values_spec.rb +++ b/spec/grape/validations/validators/except_values_spec.rb @@ -110,7 +110,7 @@ def excepts optional: { type: Array[Integer], except_values: [10, 11], default: 12 }, tests: [ { value: 'invalid-type1', rc: 400, body: { error: 'type is invalid' }.to_json }, - { value: 10, rc: 400, body: { error: 'type has a value not allowed' }.to_json }, + { value: 10, rc: 400, body: { error: 'type is invalid' }.to_json }, { value: [10], rc: 400, body: { error: 'type has a value not allowed' }.to_json }, { value: ['3'], rc: 200, body: { type: [3] }.to_json }, { value: [3], rc: 200, body: { type: [3] }.to_json }, From dda2cb38badadfb6567c1a6483a37a63387119d5 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Tue, 22 Oct 2019 09:13:18 +0300 Subject: [PATCH 02/12] explain the fix for Hashie::Mash --- lib/grape/validations/validators/coerce.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/grape/validations/validators/coerce.rb b/lib/grape/validations/validators/coerce.rb index 51015549c..83c996846 100644 --- a/lib/grape/validations/validators/coerce.rb +++ b/lib/grape/validations/validators/coerce.rb @@ -36,7 +36,15 @@ def validate_param!(attr_name, params) raise validation_exception(attr_name) unless valid_type?(new_value) - # fixes some problems with Hashie::Mash + # Don't assign a value if it is identical. It fixes a problem with Hashie::Mash + # which looses wrappers for hashes and arrays after reassigning values + # + # h = Hashie::Mash.new(list: [1, 2, 3, 4]) + # => #> + # list = h.list + # h[:list] = list + # h + # => # params[attr_name] = new_value unless params[attr_name] == new_value end From ad8f6a89b83a1b42a3cbb74cb5ea00e5cc72fb83 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Tue, 22 Oct 2019 09:16:07 +0300 Subject: [PATCH 03/12] a changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 504f4227c..c87e56560 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ #### Features * Your contribution here. +* [#1920](https://github.com/ruby-grape/grape/pull/1920): Replace Virtus with dry-types - [@dnesteryuk](https://github.com/dnesteryuk). * [#1918](https://github.com/ruby-grape/grape/pull/1918): Helper methods to access controller context from middleware - [@NikolayRys](https://github.com/NikolayRys). * [#1915](https://github.com/ruby-grape/grape/pull/1915): Micro optimizations in allocating hashes and arrays - [@dnesteryuk](https://github.com/dnesteryuk). * [#1904](https://github.com/ruby-grape/grape/pull/1904): Allows Grape to load files on startup rather than on the first call - [@myxoh](https://github.com/myxoh). From e0e60e7ba65f0ca6dace6ebfb3463896cda438f1 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Tue, 22 Oct 2019 09:31:51 +0300 Subject: [PATCH 04/12] upgrade instruction --- UPGRADING.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/UPGRADING.md b/UPGRADING.md index a220a468e..12f13b624 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,6 +1,39 @@ Upgrading Grape =============== +### Upgrading to >= 1.2.5 + +[Virtus](https://github.com/solnic/virtus) isn't used for coercing anymore. If your project depends on Virtus, you have to add it to your `Gemfile`. Also, if Virtus is used as a custom type + +```ruby +class User + include Virtus.model + + attribute :id, Integer + attribute :name, String +end + +# somewhere in your API +params do + requires :user, type: User +end +``` + +you need to add a class-level `parse` method to the model: + +```ruby +class User + include Virtus.model + + attribute :id, Integer + attribute :name, String + + def self.parse(attrs) + new(attrs) + end +end +``` + ### Upgrading to >= 1.2.4 #### Headers in `error!` call From cd0ba7055cdf3e832605068bac02dfcf74bdbf02 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Tue, 22 Oct 2019 09:36:02 +0300 Subject: [PATCH 05/12] happy rubocop --- grape.gemspec | 2 +- lib/grape/validations/types/custom_type_coercer.rb | 6 +++--- lib/grape/validations/types/primitive_coercer.rb | 8 ++++---- lib/grape/validations/types/set_coercer.rb | 4 ++-- lib/grape/validations/validators/coerce.rb | 10 +++++----- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/grape.gemspec b/grape.gemspec index 7cce5f3d1..13e05b333 100644 --- a/grape.gemspec +++ b/grape.gemspec @@ -14,10 +14,10 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'activesupport' s.add_runtime_dependency 'builder' + s.add_runtime_dependency 'dry-types', '~> 1.1.1' s.add_runtime_dependency 'mustermann-grape', '~> 1.0.0' s.add_runtime_dependency 'rack', '>= 1.3.0' s.add_runtime_dependency 'rack-accept' - s.add_runtime_dependency 'dry-types', '~> 1.1.1' s.files = Dir['**/*'].keep_if { |file| File.file?(file) } s.test_files = Dir['spec/**/*'] diff --git a/lib/grape/validations/types/custom_type_coercer.rb b/lib/grape/validations/types/custom_type_coercer.rb index ffd4cbbd8..cd1917b6f 100644 --- a/lib/grape/validations/types/custom_type_coercer.rb +++ b/lib/grape/validations/types/custom_type_coercer.rb @@ -124,12 +124,12 @@ def enforce_symbolized_keys(type, method) lambda do |val| # if the value cannot be parsed, return InvalidValue begin - coerced_value = method.call(val).tap do |new_value| - new_value.map do |item| + method.call(val).tap do |new_val| + new_val.map do |item| item.is_a?(Hash) ? symbolize_keys(item) : item end end - rescue => _e + rescue StandardError => _e InvalidValue.new end end diff --git a/lib/grape/validations/types/primitive_coercer.rb b/lib/grape/validations/types/primitive_coercer.rb index e467c3e8e..d8f6a821a 100644 --- a/lib/grape/validations/types/primitive_coercer.rb +++ b/lib/grape/validations/types/primitive_coercer.rb @@ -11,11 +11,11 @@ class PrimitiveCoercer < DryTypeCoercer # unfortunatelly, a +Params+ scope doesn't contain String String => DryTypes::Coercible::String - } + }.freeze STRICT_MAPPING = { Grape::API::Boolean => DryTypes::Strict::Bool - } + }.freeze def initialize(type, strict = false) super @@ -47,8 +47,8 @@ def call(val) # breaking changes. def would_virtus_reject?(val) (val.is_a?(Array) && type == String) || - (val.is_a?(String) && type == Hash) || - (val.is_a?(Hash) && type == String) + (val.is_a?(String) && type == Hash) || + (val.is_a?(Hash) && type == String) end end end diff --git a/lib/grape/validations/types/set_coercer.rb b/lib/grape/validations/types/set_coercer.rb index 1f351bb09..402dd60c1 100644 --- a/lib/grape/validations/types/set_coercer.rb +++ b/lib/grape/validations/types/set_coercer.rb @@ -14,9 +14,9 @@ def initialize(type, strict = false) end def call(value) - return InvalidValue.new unless value.is_a?(Array) + return InvalidValue.new unless value.is_a?(Array) - coerce_elements(value) + coerce_elements(value) end protected diff --git a/lib/grape/validations/validators/coerce.rb b/lib/grape/validations/validators/coerce.rb index 83c996846..f21bbaadf 100644 --- a/lib/grape/validations/validators/coerce.rb +++ b/lib/grape/validations/validators/coerce.rb @@ -18,11 +18,11 @@ class CoerceValidator < Base def initialize(*_args) super - if type.is_a?(Grape::Validations::Types::VariantCollectionCoercer) - @converter = type - else - @converter = Types.build_coercer(type, method: @option[:method]) - end + @converter = if type.is_a?(Grape::Validations::Types::VariantCollectionCoercer) + type + else + Types.build_coercer(type, method: @option[:method]) + end end def validate(request) From 73197ec7b645bc1dd1d028afc2141a0c11c3eb9c Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Tue, 22 Oct 2019 09:51:41 +0300 Subject: [PATCH 06/12] Virtus in dev --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 28b9fe058..4db059742 100644 --- a/Gemfile +++ b/Gemfile @@ -9,6 +9,7 @@ group :development, :test do gem 'hashie' gem 'rake' gem 'rubocop', '0.51.0' + gem 'virtus' end group :development do @@ -30,5 +31,4 @@ group :test do gem 'rack-test', '~> 1.1.0' gem 'rspec', '~> 3.0' gem 'ruby-grape-danger', '~> 0.1.0', require: false - gem 'virtus' end From d9789077c7ff368ccba2fbcfc7087bc55c710a84 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Wed, 23 Oct 2019 09:33:16 +0300 Subject: [PATCH 07/12] post-review updates --- UPGRADING.md | 2 +- lib/grape/validations/types/array_coercer.rb | 4 ++-- lib/grape/validations/types/dry_type_coercer.rb | 3 +++ lib/grape/validations/types/primitive_coercer.rb | 4 ++-- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 12f13b624..fdacc0de8 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,7 +3,7 @@ Upgrading Grape ### Upgrading to >= 1.2.5 -[Virtus](https://github.com/solnic/virtus) isn't used for coercing anymore. If your project depends on Virtus, you have to add it to your `Gemfile`. Also, if Virtus is used as a custom type +[Virtus](https://github.com/solnic/virtus) has been replaced by [dry-types](https://dry-rb.org/gems/dry-types/1.2/) for parameter coercion. If your project depends on Virtus, add it to your `Gemfile`. Also, if Virtus is used as a custom type ```ruby class User diff --git a/lib/grape/validations/types/array_coercer.rb b/lib/grape/validations/types/array_coercer.rb index 01279cffe..57174fa68 100644 --- a/lib/grape/validations/types/array_coercer.rb +++ b/lib/grape/validations/types/array_coercer.rb @@ -31,7 +31,7 @@ def call(_val) def coerce_elements(collection) collection.each_with_index do |elem, index| - return InvalidValue.new if would_virtus_reject?(elem) + return InvalidValue.new if reject?(elem) coerced_elem = @elem_coercer.call(elem) @@ -45,7 +45,7 @@ def coerce_elements(collection) # This method maintaine logic which was defined by Virtus for arrays. # Virtus doesn't allow nil in arrays. - def would_virtus_reject?(val) + def reject?(val) val.nil? end end diff --git a/lib/grape/validations/types/dry_type_coercer.rb b/lib/grape/validations/types/dry_type_coercer.rb index 34537a29b..a0a9841c1 100644 --- a/lib/grape/validations/types/dry_type_coercer.rb +++ b/lib/grape/validations/types/dry_type_coercer.rb @@ -1,6 +1,9 @@ require 'dry-types' module DryTypes + # Call +Dry.Types()+ to add all registered types to +DryTypes+ which is + # a container in this case. Check documentation for more information + # https://dry-rb.org/gems/dry-types/1.2/getting-started/ include Dry.Types() end diff --git a/lib/grape/validations/types/primitive_coercer.rb b/lib/grape/validations/types/primitive_coercer.rb index d8f6a821a..d46247bda 100644 --- a/lib/grape/validations/types/primitive_coercer.rb +++ b/lib/grape/validations/types/primitive_coercer.rb @@ -30,7 +30,7 @@ def initialize(type, strict = false) end def call(val) - return InvalidValue.new if would_virtus_reject?(val) + return InvalidValue.new if reject?(val) return nil if val.nil? return '' if val == '' @@ -45,7 +45,7 @@ def call(val) # dry-types is ok to convert an array or a hash to a string, it is supported, # but Virtus wouldn't accept it. So, this method only exists to not introduce # breaking changes. - def would_virtus_reject?(val) + def reject?(val) (val.is_a?(Array) && type == String) || (val.is_a?(String) && type == Hash) || (val.is_a?(Hash) && type == String) From e7b7a91d235a269290194d873c5652caec760a9e Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Wed, 23 Oct 2019 10:05:04 +0300 Subject: [PATCH 08/12] do not validate a custom type if it is missing --- .../validations/types/custom_type_coercer.rb | 2 ++ .../validations/validators/presence_spec.rb | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/lib/grape/validations/types/custom_type_coercer.rb b/lib/grape/validations/types/custom_type_coercer.rb index cd1917b6f..4be3dee0e 100644 --- a/lib/grape/validations/types/custom_type_coercer.rb +++ b/lib/grape/validations/types/custom_type_coercer.rb @@ -50,6 +50,8 @@ def initialize(type, method = nil) # this should always be a string. # @return [Object] the coerced result def call(val) + return if val.nil? + coerced_val = @method.call(val) return InvalidValue.new unless coerced?(coerced_val) coerced_val diff --git a/spec/grape/validations/validators/presence_spec.rb b/spec/grape/validations/validators/presence_spec.rb index d00163ead..7de97190a 100644 --- a/spec/grape/validations/validators/presence_spec.rb +++ b/spec/grape/validations/validators/presence_spec.rb @@ -269,4 +269,32 @@ def app expect(last_response.body).to eq('Hello optional'.to_json) end end + + context 'with a custom type' do + it 'does not validate their type when it is missing' do + class CustomType + def self.parse(value) + return if value.blank? + + new + end + end + + subject.params do + requires :custom, type: CustomType + end + subject.get '/custom' do + 'custom' + end + + get 'custom' + + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('{"error":"custom is missing"}') + + get 'custom', custom: 'filled' + + expect(last_response.status).to eq(200) + end + end end From a0d132807d12fcf7044b5803d871796900d07045 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Mon, 28 Oct 2019 09:19:28 +0200 Subject: [PATCH 09/12] do not depend on Virtus --- Gemfile | 1 - .../validations/validators/coerce_spec.rb | 86 ++++++------------- 2 files changed, 26 insertions(+), 61 deletions(-) diff --git a/Gemfile b/Gemfile index 4db059742..b088fc793 100644 --- a/Gemfile +++ b/Gemfile @@ -9,7 +9,6 @@ group :development, :test do gem 'hashie' gem 'rake' gem 'rubocop', '0.51.0' - gem 'virtus' end group :development do diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index 6e65ea0d9..1c956f6fd 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/spec/grape/validations/validators/coerce_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require 'virtus' describe Grape::Validations::CoerceValidator do subject do @@ -11,15 +10,13 @@ def app end describe 'coerce' do - module CoerceValidatorSpec - class User - include Virtus.model - attribute :id, Integer - attribute :name, String + class SecureURIOnly + def self.parse(value) + URI.parse(value) + end - def self.parse(attrs) - new(attrs) - end + def self.parsed?(value) + value.is_a? URI::HTTPS end end @@ -154,25 +151,6 @@ def self.parse(attrs) expect(last_response.body).to eq('array int works') end - context 'complex objects' do - it 'error on malformed input for complex objects' do - subject.params do - requires :user, type: CoerceValidatorSpec::User - end - subject.get '/user' do - 'complex works' - end - - get '/user', user: '32' - expect(last_response.status).to eq(400) - expect(last_response.body).to eq('user is invalid') - - get '/user', user: { id: 32, name: 'Bob' } - expect(last_response.status).to eq(200) - expect(last_response.body).to eq('complex works') - end - end - context 'coerces' do it 'Integer' do subject.params do @@ -187,6 +165,25 @@ def self.parse(attrs) expect(last_response.body).to eq(integer_class_name) end + it 'is a custom type' do + subject.params do + requires :uri, coerce: SecureURIOnly + end + subject.get '/secure_uri' do + params[:uri].class + end + + get 'secure_uri', uri: 'https://www.example.com' + + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('URI::HTTPS') + + get 'secure_uri', uri: 'http://www.example.com' + + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('uri is invalid') + end + context 'Array' do it 'Array of Integers' do subject.params do @@ -214,27 +211,6 @@ def self.parse(attrs) expect(last_response.body).to eq('TrueClass') end - it 'Array of Complex' do - subject.params do - requires :arry, coerce: Array[CoerceValidatorSpec::User] - end - subject.get '/array' do - params[:arry].size - end - - get 'array', arry: [31] - expect(last_response.status).to eq(400) - expect(last_response.body).to eq('arry is invalid') - - get 'array', arry: { id: 31, name: 'Alice' } - expect(last_response.status).to eq(400) - expect(last_response.body).to eq('arry is invalid') - - get 'array', arry: [{ id: 31, name: 'Alice' }] - expect(last_response.status).to eq(200) - expect(last_response.body).to eq('1') - end - it 'Array of type implementing parse' do subject.params do requires :uri, type: Array[URI] @@ -259,17 +235,7 @@ def self.parse(attrs) expect(last_response.body).to eq('Set,URI::HTTP,1') end - it 'Array of class implementing parse and parsed?' do - class SecureURIOnly - def self.parse(value) - URI.parse(value) - end - - def self.parsed?(value) - value.is_a? URI::HTTPS - end - end - + it 'Array of a custom type' do subject.params do requires :uri, type: Array[SecureURIOnly] end From 00538482993b23e804abe118048dfec8a443ee05 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Mon, 28 Oct 2019 09:46:10 +0200 Subject: [PATCH 10/12] more clear statement about custom types --- UPGRADING.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/UPGRADING.md b/UPGRADING.md index fdacc0de8..d238b7005 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,7 +3,7 @@ Upgrading Grape ### Upgrading to >= 1.2.5 -[Virtus](https://github.com/solnic/virtus) has been replaced by [dry-types](https://dry-rb.org/gems/dry-types/1.2/) for parameter coercion. If your project depends on Virtus, add it to your `Gemfile`. Also, if Virtus is used as a custom type +[Virtus](https://github.com/solnic/virtus) has been replaced by [dry-types](https://dry-rb.org/gems/dry-types/1.2/) for parameter coercion. If your project depends on Virtus, explicitly add it to your `Gemfile`. Also, if Virtus is used for defining custom types ```ruby class User @@ -34,6 +34,8 @@ class User end ``` +Custom types which don't depend on Virtus don't require any changes. + ### Upgrading to >= 1.2.4 #### Headers in `error!` call From f843edda111ec6b2920da3e65e353cdc55d97818 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Mon, 28 Oct 2019 09:49:20 +0200 Subject: [PATCH 11/12] do not catch exceptions comming from custom types --- lib/grape/validations/types/custom_type_coercer.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/grape/validations/types/custom_type_coercer.rb b/lib/grape/validations/types/custom_type_coercer.rb index 4be3dee0e..9a6b4da18 100644 --- a/lib/grape/validations/types/custom_type_coercer.rb +++ b/lib/grape/validations/types/custom_type_coercer.rb @@ -124,15 +124,10 @@ def enforce_symbolized_keys(type, method) # Collections have all values processed individually if [Array, Set].include?(type) lambda do |val| - # if the value cannot be parsed, return InvalidValue - begin - method.call(val).tap do |new_val| - new_val.map do |item| - item.is_a?(Hash) ? symbolize_keys(item) : item - end + method.call(val).tap do |new_val| + new_val.map do |item| + item.is_a?(Hash) ? symbolize_keys(item) : item end - rescue StandardError => _e - InvalidValue.new end end From e90bdc5ef6337b60661057f7e91002263704c1a8 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Fri, 1 Nov 2019 09:18:46 +0200 Subject: [PATCH 12/12] deprecate Ruby 2.3, run tests against latest major Ruby versions --- .travis.yml | 25 +++++++++++-------------- README.md | 2 ++ UPGRADING.md | 6 ++++++ grape.gemspec | 1 + 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/.travis.yml b/.travis.yml index 1626ab6f3..c591a70b5 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,45 +8,42 @@ gemfile: matrix: include: - - rvm: 2.5.3 + - rvm: 2.6.5 script: - bundle exec danger - - rvm: 2.5.3 + - rvm: 2.6.5 gemfile: Gemfile - - rvm: 2.5.3 + - rvm: 2.6.5 gemfile: gemfiles/rack_edge.gemfile - - rvm: 2.5.3 + - rvm: 2.6.5 gemfile: gemfiles/rails_edge.gemfile - - rvm: 2.5.3 + - rvm: 2.6.5 gemfile: gemfiles/rails_5.gemfile - - rvm: 2.5.3 + - rvm: 2.6.5 gemfile: gemfiles/multi_json.gemfile script: - bundle exec rake - bundle exec rspec spec/integration/multi_json - - rvm: 2.5.3 + - rvm: 2.6.5 gemfile: gemfiles/multi_xml.gemfile script: - bundle exec rake - bundle exec rspec spec/integration/multi_xml - - rvm: 2.4.5 + - rvm: 2.5.7 gemfile: Gemfile - - rvm: 2.4.5 + - rvm: 2.5.7 gemfile: gemfiles/rails_5.gemfile - - rvm: 2.3.8 + - rvm: 2.4.9 gemfile: Gemfile - - rvm: 2.3.8 + - rvm: 2.4.9 gemfile: gemfiles/rails_5.gemfile - - rvm: 2.2.10 - rvm: ruby-head - rvm: jruby-head - rvm: rbx-3 allow_failures: - - rvm: 2.2.10 - rvm: ruby-head - rvm: jruby-head - rvm: rbx-3 - - rvm: 2.5.3 gemfile: gemfiles/rack_edge.gemfile bundler_args: --without development diff --git a/README.md b/README.md index 1fa73bbd7..fa85cd0fd 100644 --- a/README.md +++ b/README.md @@ -167,6 +167,8 @@ The current stable release is [1.2.4](https://github.com/ruby-grape/grape/blob/v ## Installation +After **1.2.5**, Ruby 2.4.0 or newer is required. + Grape is available as a gem, to install it just install the gem: gem install grape diff --git a/UPGRADING.md b/UPGRADING.md index d238b7005..e8a758ef3 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,8 @@ Upgrading Grape ### Upgrading to >= 1.2.5 +#### Coercion + [Virtus](https://github.com/solnic/virtus) has been replaced by [dry-types](https://dry-rb.org/gems/dry-types/1.2/) for parameter coercion. If your project depends on Virtus, explicitly add it to your `Gemfile`. Also, if Virtus is used for defining custom types ```ruby @@ -36,6 +38,10 @@ end Custom types which don't depend on Virtus don't require any changes. +#### Ruby + +After adding dry-types, Ruby 2.4.0 or newer is required. + ### Upgrading to >= 1.2.4 #### Headers in `error!` call diff --git a/grape.gemspec b/grape.gemspec index 13e05b333..8ecc96f31 100644 --- a/grape.gemspec +++ b/grape.gemspec @@ -22,4 +22,5 @@ Gem::Specification.new do |s| s.files = Dir['**/*'].keep_if { |file| File.file?(file) } s.test_files = Dir['spec/**/*'] s.require_paths = ['lib'] + s.required_ruby_version = '>= 2.4.0' end