diff --git a/CHANGELOG.md b/CHANGELOG.md index 22cdf8f606..84860df4b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ #### Fixes * Your contribution here. +* [#2031](https://github.com/ruby-grape/grape/pull/2031): Fix a regression with an array of a custom type - [@dnesteryuk](https://github.com/dnesteryuk). * [#2026](https://github.com/ruby-grape/grape/pull/2026): Fix a regression in `coerce_with` when coercion returns `nil` - [@misdoro](https://github.com/misdoro). * [#2025](https://github.com/ruby-grape/grape/pull/2025): Fix Decimal type category - [@kdoya](https://github.com/kdoya). * [#2019](https://github.com/ruby-grape/grape/pull/2019): Avoid coercing parameter with multiple types to an empty Array - [@stanhu](https://github.com/stanhu). diff --git a/lib/grape/validations/types.rb b/lib/grape/validations/types.rb index ee60949d80..a682286a56 100644 --- a/lib/grape/validations/types.rb +++ b/lib/grape/validations/types.rb @@ -42,7 +42,6 @@ class InvalidValue; end Grape::API::Boolean, String, Symbol, - Rack::Multipart::UploadedFile, TrueClass, FalseClass ].freeze @@ -54,8 +53,7 @@ class InvalidValue; end Set ].freeze - # Types for which Grape provides special coercion - # and type-checking logic. + # Special custom types provided by Grape. SPECIAL = { JSON => Json, Array[JSON] => JsonArray, @@ -130,7 +128,6 @@ def self.custom?(type) !primitive?(type) && !structure?(type) && !multiple?(type) && - !special?(type) && type.respond_to?(:parse) && type.method(:parse).arity == 1 end @@ -143,7 +140,11 @@ def self.custom?(type) def self.collection_of_custom?(type) (type.is_a?(Array) || type.is_a?(Set)) && type.length == 1 && - custom?(type.first) + (custom?(type.first) || special?(type.first)) + end + + def self.map_special(type) + SPECIAL.fetch(type, type) end end end diff --git a/lib/grape/validations/types/build_coercer.rb b/lib/grape/validations/types/build_coercer.rb index 90b1dbf01f..b867485c16 100644 --- a/lib/grape/validations/types/build_coercer.rb +++ b/lib/grape/validations/types/build_coercer.rb @@ -42,6 +42,9 @@ def self.build_coercer(type, method: nil, strict: false) 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) @@ -55,10 +58,8 @@ def self.create_coercer_instance(type, method, strict) # method is supplied. elsif Types.collection_of_custom?(type) Types::CustomTypeCollectionCoercer.new( - type.first, type.is_a?(Set) + Types.map_special(type.first), type.is_a?(Set) ) - elsif Types.special?(type) - Types::SPECIAL[type].new elsif type.is_a?(Array) ArrayCoercer.new type, strict elsif type.is_a?(Set) diff --git a/lib/grape/validations/types/file.rb b/lib/grape/validations/types/file.rb index 6b79389de4..8c2f6d9242 100644 --- a/lib/grape/validations/types/file.rb +++ b/lib/grape/validations/types/file.rb @@ -7,21 +7,23 @@ module Types # 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 if input.nil? - return InvalidValue.new unless coerced?(input) + class << self + def parse(input) + return if input.nil? + return InvalidValue.new unless parsed?(input) - # Processing of multipart file objects - # is already taken care of by Rack::Request. - # Nothing to do here. - input - end + # Processing of multipart file objects + # is already taken care of by Rack::Request. + # Nothing to do here. + input + end - def coerced?(value) - # Rack::Request creates a Hash with filename, - # content type and an IO object. Do a bit of basic - # duck-typing. - value.is_a?(::Hash) && value.key?(:tempfile) && value[:tempfile].is_a?(Tempfile) + def parsed?(value) + # Rack::Request creates a Hash with filename, + # content type and an IO object. Do a bit of basic + # duck-typing. + value.is_a?(::Hash) && value.key?(:tempfile) && value[:tempfile].is_a?(Tempfile) + end end end end diff --git a/lib/grape/validations/types/json.rb b/lib/grape/validations/types/json.rb index 472f73a17f..25dded6f04 100644 --- a/lib/grape/validations/types/json.rb +++ b/lib/grape/validations/types/json.rb @@ -12,35 +12,37 @@ module Types # validation system will apply nested validation rules to # all returned objects. class Json - # Coerce the input into a JSON-like data structure. - # - # @param input [String] a JSON-encoded parameter value - # @return [Hash,Array,nil] - def call(input) - return input if coerced?(input) + class << self + # Coerce the input into a JSON-like data structure. + # + # @param input [String] a JSON-encoded parameter value + # @return [Hash,Array,nil] + def parse(input) + return input if parsed?(input) - # Allow nulls and blank strings - return if input.nil? || input.match?(/^\s*$/) - JSON.parse(input, symbolize_names: true) - end + # Allow nulls and blank strings + return if input.nil? || input.match?(/^\s*$/) + JSON.parse(input, symbolize_names: true) + end - # Checks that the input was parsed successfully - # and isn't something odd such as an array of primitives. - # - # @param value [Object] result of {#coerce} - # @return [true,false] - def coerced?(value) - value.is_a?(::Hash) || coerced_collection?(value) - end + # Checks that the input was parsed successfully + # and isn't something odd such as an array of primitives. + # + # @param value [Object] result of {#parse} + # @return [true,false] + def parsed?(value) + value.is_a?(::Hash) || coerced_collection?(value) + end - protected + protected - # Is the value an array of JSON-like objects? - # - # @param value [Object] result of {#coerce} - # @return [true,false] - def coerced_collection?(value) - value.is_a?(::Array) && value.all? { |i| i.is_a? ::Hash } + # Is the value an array of JSON-like objects? + # + # @param value [Object] result of {#parse} + # @return [true,false] + def coerced_collection?(value) + value.is_a?(::Array) && value.all? { |i| i.is_a? ::Hash } + end end end @@ -49,18 +51,20 @@ def coerced_collection?(value) # objects and arrays of objects, but wraps single objects # in an Array. class JsonArray < Json - # See {Json#coerce}. Wraps single objects in an array. - # - # @param input [String] JSON-encoded parameter value - # @return [Array] - def call(input) - json = super - Array.wrap(json) unless json.nil? - end + class << self + # See {Json#parse}. Wraps single objects in an array. + # + # @param input [String] JSON-encoded parameter value + # @return [Array] + def parse(input) + json = super + Array.wrap(json) unless json.nil? + end - # See {Json#coerced_collection?} - def coerced?(value) - coerced_collection? value + # See {Json#coerced_collection?} + def parsed?(value) + coerced_collection? value + end end end end diff --git a/spec/grape/validations/types_spec.rb b/spec/grape/validations/types_spec.rb index 133cba1302..1bee89e771 100644 --- a/spec/grape/validations/types_spec.rb +++ b/spec/grape/validations/types_spec.rb @@ -17,7 +17,7 @@ def self.parse; end [ Integer, Float, Numeric, BigDecimal, Grape::API::Boolean, String, Symbol, - Date, DateTime, Time, Rack::Multipart::UploadedFile + Date, DateTime, Time ].each do |type| it "recognizes #{type} as a primitive" do expect(described_class.primitive?(type)).to be_truthy diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index 2badb4c4ce..e77d839349 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/spec/grape/validations/validators/coerce_spec.rb @@ -354,42 +354,60 @@ def self.parsed?(value) expect(last_response.body).to eq('TrueClass') end - it 'Rack::Multipart::UploadedFile' do - subject.params do - requires :file, type: Rack::Multipart::UploadedFile - end - subject.post '/upload' do - params[:file][:filename] - end + context 'File' do + let(:file) { Rack::Test::UploadedFile.new(__FILE__) } + let(:filename) { File.basename(__FILE__).to_s } - post '/upload', file: Rack::Test::UploadedFile.new(__FILE__) - expect(last_response.status).to eq(201) - expect(last_response.body).to eq(File.basename(__FILE__).to_s) + it 'Rack::Multipart::UploadedFile' do + subject.params do + requires :file, type: Rack::Multipart::UploadedFile + end + subject.post '/upload' do + params[:file][:filename] + end - post '/upload', file: 'not a file' - expect(last_response.status).to eq(400) - expect(last_response.body).to eq('file is invalid') - end + post '/upload', file: file + expect(last_response.status).to eq(201) + expect(last_response.body).to eq(filename) - it 'File' do - subject.params do - requires :file, coerce: File - end - subject.post '/upload' do - params[:file][:filename] + post '/upload', file: 'not a file' + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('file is invalid') end - post '/upload', file: Rack::Test::UploadedFile.new(__FILE__) - expect(last_response.status).to eq(201) - expect(last_response.body).to eq(File.basename(__FILE__).to_s) + it 'File' do + subject.params do + requires :file, coerce: File + end + subject.post '/upload' do + params[:file][:filename] + end - post '/upload', file: 'not a file' - expect(last_response.status).to eq(400) - expect(last_response.body).to eq('file is invalid') + post '/upload', file: file + expect(last_response.status).to eq(201) + expect(last_response.body).to eq(filename) - post '/upload', file: { filename: 'fake file', tempfile: '/etc/passwd' } - expect(last_response.status).to eq(400) - expect(last_response.body).to eq('file is invalid') + post '/upload', file: 'not a file' + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('file is invalid') + + post '/upload', file: { filename: 'fake file', tempfile: '/etc/passwd' } + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('file is invalid') + end + + it 'collection' do + subject.params do + requires :files, type: Array[File] + end + subject.post '/upload' do + params[:files].first[:filename] + end + + post '/upload', files: [file] + expect(last_response.status).to eq(201) + expect(last_response.body).to eq(filename) + end end it 'Nests integers' do