Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove value inspection from param validation #2915

Merged
merged 2 commits into from
Sep 25, 2023
Merged

Remove value inspection from param validation #2915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62534d0
Remove value inspection from param validation
mullermp Sep 25, 2023
8d27f3e
Fix tests and error message
mullermp Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions gems/aws-sdk-core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
Unreleased Changes
------------------

* Issue - Remove value inspection from param validation errors.

3.183.0 (2023-09-20)
------------------

Expand Down
4 changes: 2 additions & 2 deletions gems/aws-sdk-core/lib/aws-sdk-core/param_validator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class ParamValidator

include Seahorse::Model::Shapes

EXPECTED_GOT = "expected %s to be %s, got value %s (class: %s) instead."
EXPECTED_GOT = 'expected %s to be %s, got class %s instead.'

# @param [Seahorse::Model::Shapes::ShapeRef] rules
# @param [Hash] params
Expand Down Expand Up @@ -230,7 +230,7 @@ def error_messages(errors)
end

def expected_got(context, expected, got)
EXPECTED_GOT % [context, expected, got.inspect, got.class.name]
EXPECTED_GOT % [context, expected, got.class.name]
end

end
Expand Down
121 changes: 48 additions & 73 deletions gems/aws-sdk-core/spec/aws/param_validator_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ module Aws
class TestClass; end

describe ParamValidator do

let(:shapes) { ApiHelper.sample_shapes }

let(:api) { ApiHelper.sample_api(shapes: shapes) }
Expand All @@ -16,9 +15,9 @@ def validate(params, expected_errors = [])
if expected_errors.empty?
ParamValidator.new(rules).validate!(params)
else
expect {
expect do
ParamValidator.new(rules).validate!(params)
}.to raise_error(ArgumentError) do |error|
end.to raise_error(ArgumentError) do |error|
match_errors(error, expected_errors)
end
end
Expand All @@ -36,59 +35,56 @@ def match_errors(error, expected_errors)
end

describe 'empty rules' do

it 'accepts an empty hash of params when rules are empty' do
expect(validate({}))
end

end

describe 'structures' do

it 'validates nested structures' do
validate('abc', 'expected params to be a hash, got value "abc" (class: String) instead.')
validate('abc',
'expected params to be a hash, got class String instead.')
validate({ nested: 'abc' },
'expected params[:nested] to be a hash, got value "abc" (class: String) instead.')
'expected params[:nested] to be a hash, got class String instead.')
validate({ nested: { nested: 'abc' } },
'expected params[:nested][:nested] to be a hash, got value "abc" (class: String) instead.')
'expected params[:nested][:nested] to be a hash, got class String instead.')
end

it 'accepts hashes' do
validate({})
end

it 'raises an error when a required parameter is missing' do
shapes['StructureShape']['required'] = %w(String)
shapes['StructureShape']['required'] = %w[String]
validate({}, 'missing required parameter params[:string]')
end

it 'raises an error when a given parameter is unexpected' do
validate({foo: 'bar'}, 'unexpected value at params[:foo]')
validate({ foo: 'bar' }, 'unexpected value at params[:foo]')
end

it 'accepts members that pass validation' do
shapes['StructureShape']['required'] = %w(String)
shapes['StructureShape']['required'] = %w[String]
validate(string: 'abc')
end

it 'aggregates errors for members' do
shapes['StructureShape']['required'] = %w(String)
validate({nested: { foo: 'bar' }}, [
'missing required parameter params[:string]',
'missing required parameter params[:nested][:string]',
'unexpected value at params[:nested][:foo]'
])
shapes['StructureShape']['required'] = %w[String]
validate({ nested: { foo: 'bar' } }, [
'missing required parameter params[:string]',
'missing required parameter params[:nested][:string]',
'unexpected value at params[:nested][:foo]'
])
end

it 'raises an error when providing eventstream at input' do
validate({event_stream: [].each }, 'instead of providing value directly for eventstreams at input, expected to use #signal events per stream')
validate({ event_stream: [].each }, 'instead of providing value directly for eventstreams at input, expected to use #signal events per stream')
end

it 'accepts no eventstream input even when marked required' do
shapes['StructureShape']['required'] = %w(EventStream)
shapes['StructureShape']['required'] = %w[EventStream]
validate({})
end

end

describe 'unions' do
Expand All @@ -99,17 +95,16 @@ def match_errors(error, expected_errors)

it 'raises an error when multiple values are set' do
shapes['StructureShape']['union'] = true
validate({string: 's', boolean: true}, 'multiple values provided to union')
validate({ string: 's', boolean: true }, 'multiple values provided to union')
end

it 'access a struct with exactly one value set' do
shapes['StructureShape']['union'] = true
validate({string: 's'})
validate({ string: 's' })
end
end

describe 'lists' do

it 'accepts arrays' do
validate(number_list: [])
validate(nested_list: [{}, {}])
Expand All @@ -118,110 +113,94 @@ def match_errors(error, expected_errors)
it 'expects the value to be an Array' do
validate({ nested_list: [] })
validate({ nested_list: 'abc' },
'expected params[:nested_list] to be an Array, got value "abc" (class: String) instead.')
'expected params[:nested_list] to be an Array, got class String instead.')
end

it 'validates each member of the list' do
validate({ nested_list: [{}] })
validate({ number_list: ['abc'] },
'expected params[:number_list][0] to be an Integer, got value "abc" (class: String) instead.')
'expected params[:number_list][0] to be an Integer, got class String instead.')
validate({ nested_list: [{}, 'abc'] },
'expected params[:nested_list][1] to be a hash, got value "abc" (class: String) instead.')
'expected params[:nested_list][1] to be a hash, got class String instead.')
validate({ nested_list: [{ number_list: ['abc'] }] },
'expected params[:nested_list][0][:number_list][0] to be an Integer, got value "abc" (class: String) instead.')
'expected params[:nested_list][0][:number_list][0] to be an Integer, got class String instead.')
validate({ nested_list: [{ foo: 'abc' }] },
'unexpected value at params[:nested_list][0][:foo]')
'unexpected value at params[:nested_list][0][:foo]')
end

end

describe 'maps' do

it 'accepts hashes' do
validate({ string_map: {}})
validate({ string_map: {} })
validate({ string_map: 'abc' },
'expected params[:string_map] to be a hash, got value "abc" (class: String) instead.')
'expected params[:string_map] to be a hash, got class String instead.')
end

it 'validates map keys' do
validate({ string_map: { 'abc' => 'mno' }})
validate({ string_map: { 123 => 'xyz' }},
[/expected params\[:string_map\] 123 key to be a String, got value 123 \(class: (Fixnum|Integer)\) instead./])
validate({ string_map: { 'abc' => 'mno' } })
validate({ string_map: { 123 => 'xyz' } },
[/expected params\[:string_map\] 123 key to be a String, got class (Fixnum|Integer) instead./])
end

it 'validates map values' do
validate({ string_map: { 'foo' => 'bar' }})
validate({ string_map: { 'foo' => 123 }},
[/expected params\[:string_map\]\["foo"\] to be a String, got value 123 \(class: (Fixnum|Integer)\) instead./])
validate({ string_map: { 'foo' => 'bar' } })
validate({ string_map: { 'foo' => 123 } },
[/expected params\[:string_map\]\["foo"\] to be a String, got class (Fixnum|Integer) instead./])
end

end

describe 'integers' do

it 'accepts integers' do
validate(integer: 123)
validate({ integer: '123' },
'expected params[:integer] to be an Integer, got value "123" (class: String) instead.')
'expected params[:integer] to be an Integer, got class String instead.')
end

end

describe 'floats' do

it 'accepts integers' do
validate(float: 123.0)
validate({ float: 123 },
[/expected params\[:float\] to be a Float, got value 123 \(class: (Fixnum|Integer)\) instead./])
[/expected params\[:float\] to be a Float, got class (Fixnum|Integer) instead./])
end

end

describe 'timestamps' do

it 'accepts time objects' do
validate(timestamp: Time.now)
validate({timestamp: Date.new},
['expected params[:timestamp] to be a Time object, got value', '(class: Date) instead.'])
validate({ timestamp: Date.new },
['expected params[:timestamp] to be a Time object, got ', 'class Date instead.'])
end

end

describe 'booleans' do

it 'accepts TrueClass and FalseClass' do
validate(boolean: true)
validate(boolean: false)
validate({ boolean: 'true' },
'expected params[:boolean] to be true or false, got value "true" (class: String) instead.')
'expected params[:boolean] to be true or false, got class String instead.')
end

end

describe 'blobs' do

it 'accepts strings and io objects for payload members' do
validate(blob: StringIO.new('abc'))
validate(blob: double('d', :read => 'abc', :size => 3, :rewind => 0))
validate(blob: double('d', read: 'abc', size: 3, rewind: 0))
validate({ blob: 'abc' })
validate({ blob: 123 },
[/expected params\[:blob\] to be a String or IO like object that supports read, rewind, and size, got value 123 \(class: (Fixnum|Integer)\) instead./])
[/expected params\[:blob\] to be a String or IO like object that supports read, rewind, and size, got class (Fixnum|Integer) instead./])
end

end

describe 'strings' do

it 'accepts string objects' do
validate(string: 'john doe')
validate({ string: 123 },
[/expected params\[:string\] to be a String, got value 123 \(class: (Fixnum|Integer)\) instead./])
[/expected params\[:string\] to be a String, got class (Fixnum|Integer) instead./])
end

end

describe 'document types' do

it 'accepts numeric objects' do
validate(document_type: 123)
validate(document_type: 3.14159)
Expand All @@ -242,38 +221,34 @@ def match_errors(error, expected_errors)

it 'rejects Objects' do
validate({ document_type: TestClass.new },
[/expected params\[:document_type\] to be one of Hash, Array, Numeric, String, TrueClass, FalseClass, NilClass, got value #<Aws::TestClass:[0-9a-z]+> \(class: Aws::TestClass\) instead/]
)
[/expected params\[:document_type\] to be one of Hash, Array, Numeric, String, TrueClass, FalseClass, NilClass, got class Aws::TestClass instead/])
end

describe 'Hash' do
it 'accepts a flat Hash' do
validate(document_type: {a: 1, b: 2})
validate(document_type: { a: 1, b: 2 })
end

it 'accepts a nested Hash' do
validate(document_type: {a: 1, b: {c: 'nested'} })
validate(document_type: { a: 1, b: { c: 'nested' } })
end

it 'recursively validates and rejects a nested arbitrary Object' do
validate({ document_type: {a: 1, b: { c: TestClass.new }} },
[/expected params\[:document_type\]\[b\]\[c\] to be one of Hash, Array, Numeric, String, TrueClass, FalseClass, NilClass, got value #<Aws::TestClass:[0-9a-z]+> \(class: Aws::TestClass\) instead/]
)
validate({ document_type: { a: 1, b: { c: TestClass.new } } },
[/expected params\[:document_type\]\[b\]\[c\] to be one of Hash, Array, Numeric, String, TrueClass, FalseClass, NilClass, got class Aws::TestClass instead/])
end
end

describe 'Array' do
it 'accepts an array' do
validate(document_type: [1,2,3])
validate(document_type: [1, 2, 3])
end

it 'rejects an array with arbitrary Objects' do
validate({ document_type: [1,2,TestClass.new] },
[/expected params\[:document_type\] to be one of Hash, Array, Numeric, String, TrueClass, FalseClass, NilClass, got value #<Aws::TestClass:[0-9a-z]+> \(class: Aws::TestClass\) instead/]
)
validate({ document_type: [1, 2, TestClass.new] },
[/expected params\[:document_type\] to be one of Hash, Array, Numeric, String, TrueClass, FalseClass, NilClass, got class Aws::TestClass instead/])
end
end

end
end
end
Loading