From 33ac1de787d5ccb2e15f81be4860d387cbad0f07 Mon Sep 17 00:00:00 2001 From: Keith Pitty Date: Fri, 3 Apr 2020 17:36:21 +1100 Subject: [PATCH 1/4] Add scenario to test coercion of grape entity This is a first attempt at exposing a regression in Grape 1.3.1 compared with 1.2.4 with respect to specifying subclasses of Grape::Entity as parameters. --- .../validations/validators/coerce_spec.rb | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index f0feff176..3e7ed8194 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/spec/grape/validations/validators/coerce_spec.rb @@ -2,6 +2,10 @@ require 'spec_helper' +class AnEntity < Grape::Entity + expose :name, documentation: { required: true, type: 'String', desc: 'a name' } +end + describe Grape::Validations::CoerceValidator do subject do Class.new(Grape::API) @@ -984,5 +988,21 @@ def self.parsed?(value) 10.times { get '/' } end end + + context 'grape entity' do + it 'handles a subclass of Grape::Entity' do + subject.params do + requires :grape_entity, type: AnEntity + end + subject.get '/single' do + 'grape entity works' + end + + get '/single', grape_entity: AnEntity.new(name: 'an entity') + + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('grape entity works') + end + end end end From 789959e956be2ddef8bd336d13802317e05ee84c Mon Sep 17 00:00:00 2001 From: Keith Pitty Date: Mon, 6 Apr 2020 14:38:42 +1000 Subject: [PATCH 2/4] Coerce subclasses of Grape::Entity The addition of a GrapeEntityCoercer to handle subclasses of Grape::Entity enables grape entity parameters to be specified without resulting in the coercing logic to mark them as invalid. --- lib/grape/validations/types/build_coercer.rb | 3 ++ .../validations/types/grape_entity_coercer.rb | 32 +++++++++++++++++++ .../validations/validators/coerce_spec.rb | 11 ++++--- 3 files changed, 41 insertions(+), 5 deletions(-) create mode 100644 lib/grape/validations/types/grape_entity_coercer.rb diff --git a/lib/grape/validations/types/build_coercer.rb b/lib/grape/validations/types/build_coercer.rb index b867485c1..df12a83f2 100644 --- a/lib/grape/validations/types/build_coercer.rb +++ b/lib/grape/validations/types/build_coercer.rb @@ -3,6 +3,7 @@ require_relative 'array_coercer' require_relative 'set_coercer' require_relative 'primitive_coercer' +require_relative 'grape_entity_coercer' module Grape module Validations @@ -64,6 +65,8 @@ def self.create_coercer_instance(type, method, strict) ArrayCoercer.new type, strict elsif type.is_a?(Set) SetCoercer.new type, strict + elsif type.superclass == Grape::Entity + GrapeEntityCoercer.new type else PrimitiveCoercer.new type, strict end diff --git a/lib/grape/validations/types/grape_entity_coercer.rb b/lib/grape/validations/types/grape_entity_coercer.rb new file mode 100644 index 000000000..72b2663a0 --- /dev/null +++ b/lib/grape/validations/types/grape_entity_coercer.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Grape + module Validations + module Types + # Handles coercion and type checking for parameters that are subclasses + # of Grape::Entity. + class GrapeEntityCoercer + def initialize(type) + @type = type + end + + def call(val) + return if val.nil? + + return InvalidValue.new unless coerced?(val) + end + + private + + attr_reader :type + + def coerced?(val) + val.each_key do |k| + exposure = type.find_exposure(k.to_sym) + return false unless exposure + end + end + end + end + end +end diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index 3e7ed8194..7c63dcd6b 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/spec/grape/validations/validators/coerce_spec.rb @@ -1,10 +1,7 @@ # frozen_string_literal: true require 'spec_helper' - -class AnEntity < Grape::Entity - expose :name, documentation: { required: true, type: 'String', desc: 'a name' } -end +require 'grape_entity' describe Grape::Validations::CoerceValidator do subject do @@ -990,6 +987,10 @@ def self.parsed?(value) end context 'grape entity' do + class AnEntity < Grape::Entity + expose :name, documentation: { required: true, type: 'String', desc: 'a name' } + end + it 'handles a subclass of Grape::Entity' do subject.params do requires :grape_entity, type: AnEntity @@ -998,7 +999,7 @@ def self.parsed?(value) 'grape entity works' end - get '/single', grape_entity: AnEntity.new(name: 'an entity') + get '/single', grape_entity: { name: 'an entity' } expect(last_response.status).to eq(200) expect(last_response.body).to eq('grape entity works') From 7d64431e1109ce427af365d9fb3e87ef07508a79 Mon Sep 17 00:00:00 2001 From: Keith Pitty Date: Mon, 6 Apr 2020 16:24:09 +1000 Subject: [PATCH 3/4] Handle grape entities with as: A grape entity can expose an attribute with a different key. So we need to handle that case as well. --- lib/grape/validations/types/grape_entity_coercer.rb | 9 +++++++-- spec/grape/validations/validators/coerce_spec.rb | 5 +++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/grape/validations/types/grape_entity_coercer.rb b/lib/grape/validations/types/grape_entity_coercer.rb index 72b2663a0..143e50ff5 100644 --- a/lib/grape/validations/types/grape_entity_coercer.rb +++ b/lib/grape/validations/types/grape_entity_coercer.rb @@ -22,9 +22,14 @@ def call(val) def coerced?(val) val.each_key do |k| - exposure = type.find_exposure(k.to_sym) - return false unless exposure + return false unless exposure_keys.include?(k.to_sym) end + + true + end + + def exposure_keys + type.root_exposures.map(&:key) end end end diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index 7c63dcd6b..c1320281d 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/spec/grape/validations/validators/coerce_spec.rb @@ -988,7 +988,8 @@ def self.parsed?(value) context 'grape entity' do class AnEntity < Grape::Entity - expose :name, documentation: { required: true, type: 'String', desc: 'a name' } + expose :first_name, documentation: { required: true, type: 'String', desc: 'first name' } + expose :surname, as: 'lastName', documentation: { required: true, type: 'String', desc: 'last name' } end it 'handles a subclass of Grape::Entity' do @@ -999,7 +1000,7 @@ class AnEntity < Grape::Entity 'grape entity works' end - get '/single', grape_entity: { name: 'an entity' } + get '/single', grape_entity: { first_name: 'John', 'lastName' => 'Smith' } expect(last_response.status).to eq(200) expect(last_response.body).to eq('grape entity works') From 3d530b7263baa67dec229907472cb5269f2a0e8b Mon Sep 17 00:00:00 2001 From: Keith Pitty Date: Mon, 6 Apr 2020 17:57:04 +1000 Subject: [PATCH 4/4] Handle an array of grape entities Validation of grape entities now handles an array. However, we still need to ensure that the coerced values are returned. --- .../validations/types/grape_entity_coercer.rb | 8 ++- .../validations/validators/coerce_spec.rb | 50 ++++++++++++++++--- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/lib/grape/validations/types/grape_entity_coercer.rb b/lib/grape/validations/types/grape_entity_coercer.rb index 143e50ff5..34399413f 100644 --- a/lib/grape/validations/types/grape_entity_coercer.rb +++ b/lib/grape/validations/types/grape_entity_coercer.rb @@ -13,7 +13,13 @@ def initialize(type) def call(val) return if val.nil? - return InvalidValue.new unless coerced?(val) + if val.is_a?(Array) + val.each do |i| + return InvalidValue.new unless coerced?(i) + end + else + return InvalidValue.new unless coerced?(val) + end end private diff --git a/spec/grape/validations/validators/coerce_spec.rb b/spec/grape/validations/validators/coerce_spec.rb index c1320281d..dda074894 100644 --- a/spec/grape/validations/validators/coerce_spec.rb +++ b/spec/grape/validations/validators/coerce_spec.rb @@ -987,23 +987,61 @@ def self.parsed?(value) end context 'grape entity' do - class AnEntity < Grape::Entity + class Name < Grape::Entity expose :first_name, documentation: { required: true, type: 'String', desc: 'first name' } expose :surname, as: 'lastName', documentation: { required: true, type: 'String', desc: 'last name' } end + class Person < Grape::Entity + expose :email, documentation: { required: true, type: 'String', desc: 'email address' } + expose :name, using: Name, documentation: { required: true, type: 'Name', desc: 'full name' } + end + + class Question < Grape::Entity + expose :order, documentation: { required: true, type: 'Integer', desc: 'order of question' } + expose :text, documentation: { required: true, type: 'String', desc: 'text of question' } + end + it 'handles a subclass of Grape::Entity' do subject.params do - requires :grape_entity, type: AnEntity + requires :name, type: Name end - subject.get '/single' do - 'grape entity works' + subject.get '/' do + 'simple grape entity works' + end + + get '/', name: { first_name: 'John', 'lastName' => 'Smith' } + + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('simple grape entity works') + end + + it 'handles a subclass of Grape::Entity which exposes another Grape::Entity' do + subject.params do + requires :person, type: Person + end + subject.get '/' do + 'complex grape entity works' + end + + get '/', person: { email: 'john@example.com', name: { first_name: 'John', 'lastName' => 'Smith' } } + + expect(last_response.status).to eq(200) + expect(last_response.body).to eq('complex grape entity works') + end + + it 'handles an array of elements of a subclass of Grape::Entity' do + subject.params do + requires :question, type: Question + end + subject.get '/' do + 'an array of grape entities works' end - get '/single', grape_entity: { first_name: 'John', 'lastName' => 'Smith' } + get '/', question: [{ order: 1, text: 'Why?' }, { order: 2, text: 'How?' }] expect(last_response.status).to eq(200) - expect(last_response.body).to eq('grape entity works') + expect(last_response.body).to eq('an array of grape entities works') end end end