Skip to content

Commit d9d99ea

Browse files
authored
Merge pull request #2128 from dwhenry/fix-nested-validations
Fix validation error for nested array when `requires` => `optional` => `requires`
2 parents 338663d + 2647e2c commit d9d99ea

File tree

6 files changed

+221
-11
lines changed

6 files changed

+221
-11
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#### Fixes
88

99
* Your contribution here.
10+
* [#2128](https://github.com/ruby-grape/grape/pull/2128): Fix validation error when Required Array nested inside an optional array - [@dwhenry](https://github.com/dwhenry).
1011
* [#2127](https://github.com/ruby-grape/grape/pull/2127): Fix a performance issue with dependent params - [@dnesteryuk](https://github.com/dnesteryuk).
1112
* [#2126](https://github.com/ruby-grape/grape/pull/2126): Fix warnings about redefined attribute accessors in `AttributeTranslator` - [@samsonjs](https://github.com/samsonjs).
1213
* [#2121](https://github.com/ruby-grape/grape/pull/2121): Fix 2.7 deprecation warning in validator_factory - [@Legogris](https://github.com/Legogris).

lib/grape/dsl/parameters.rb

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -227,13 +227,17 @@ def declared_param?(param)
227227

228228
alias group requires
229229

230-
def map_params(params, element)
230+
class EmptyOptionalValue; end
231+
232+
def map_params(params, element, is_array = false)
231233
if params.is_a?(Array)
232234
params.map do |el|
233-
map_params(el, element)
235+
map_params(el, element, true)
234236
end
235237
elsif params.is_a?(Hash)
236-
params[element] || {}
238+
params[element] || (@optional && is_array ? EmptyOptionalValue : {})
239+
elsif params == EmptyOptionalValue
240+
EmptyOptionalValue
237241
else
238242
{}
239243
end

lib/grape/validations/single_attribute_iterator.rb

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,19 @@ class SingleAttributeIterator < AttributesIterator
77

88
def yield_attributes(val, attrs)
99
attrs.each do |attr_name|
10-
yield val, attr_name, empty?(val)
10+
yield val, attr_name, empty?(val), skip?(val)
1111
end
1212
end
1313

14+
15+
# This is a special case so that we can ignore tree's where option
16+
# values are missing lower down. Unfortunately we can remove this
17+
# are the parameter parsing stage as they are required to ensure
18+
# the correct indexing is maintained
19+
def skip?(val)
20+
val == Grape::DSL::Parameters::EmptyOptionalValue
21+
end
22+
1423
# Primitives like Integers and Booleans don't respond to +empty?+.
1524
# It could be possible to use +blank?+ instead, but
1625
#

lib/grape/validations/validators/base.rb

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,8 @@ def validate!(params)
4343
# there may be more than one error per field
4444
array_errors = []
4545

46-
attributes.each do |val, attr_name, empty_val|
46+
attributes.each do |val, attr_name, empty_val, skip_value|
47+
next if skip_value
4748
next if !@scope.required? && empty_val
4849
next unless @scope.meets_dependency?(val, params)
4950
begin

spec/grape/validations/single_attribute_iterator_spec.rb

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515

1616
it 'yields params and every single attribute from the list' do
1717
expect { |b| iterator.each(&b) }
18-
.to yield_successive_args([params, :first, false], [params, :second, false])
18+
.to yield_successive_args([params, :first, false, false], [params, :second, false, false])
1919
end
2020
end
2121

@@ -26,8 +26,8 @@
2626

2727
it 'yields every single attribute from the list for each of the array elements' do
2828
expect { |b| iterator.each(&b) }.to yield_successive_args(
29-
[params[0], :first, false], [params[0], :second, false],
30-
[params[1], :first, false], [params[1], :second, false]
29+
[params[0], :first, false, false], [params[0], :second, false, false],
30+
[params[1], :first, false, false], [params[1], :second, false, false]
3131
)
3232
end
3333

@@ -36,9 +36,20 @@
3636

3737
it 'marks params with empty values' do
3838
expect { |b| iterator.each(&b) }.to yield_successive_args(
39-
[params[0], :first, true], [params[0], :second, true],
40-
[params[1], :first, true], [params[1], :second, true],
41-
[params[2], :first, false], [params[2], :second, false]
39+
[params[0], :first, true, false], [params[0], :second, true, false],
40+
[params[1], :first, true, false], [params[1], :second, true, false],
41+
[params[2], :first, false, false], [params[2], :second, false, false]
42+
)
43+
end
44+
end
45+
46+
context 'when missing optional value' do
47+
let(:params) { [Grape::DSL::Parameters::EmptyOptionalValue, 10] }
48+
49+
it 'marks params with skipped values' do
50+
expect { |b| iterator.each(&b) }.to yield_successive_args(
51+
[params[0], :first, false, true], [params[0], :second, false, true],
52+
[params[1], :first, false, false], [params[1], :second, false, false],
4253
)
4354
end
4455
end

spec/grape/validations_spec.rb

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -883,6 +883,190 @@ def validate_param!(attr_name, params)
883883
end
884884
expect(declared_params).to eq([items: [:key, { optional_subitems: [:value] }, { required_subitems: [:value] }]])
885885
end
886+
887+
context <<~DESC do
888+
Issue occurs whenever:
889+
* param structure with at least three levels
890+
* 1st level item is a required Array that has >1 entry with an optional item present and >1 entry with an optional item missing
891+
* 2nd level is an optional Array or Hash
892+
* 3rd level is a required item (can be any type)
893+
* additional levels do not effect the issue from occuring
894+
DESC
895+
896+
it "example based off actual real world use case" do
897+
subject.params do
898+
requires :orders, type: Array do
899+
requires :id, type: Integer
900+
optional :drugs, type: Array do
901+
requires :batches, type: Array do
902+
requires :batch_no, type: String
903+
end
904+
end
905+
end
906+
end
907+
908+
subject.get '/validate_required_arrays_under_optional_arrays' do
909+
'validate_required_arrays_under_optional_arrays works!'
910+
end
911+
912+
data = {
913+
orders: [
914+
{ id: 77, drugs: [{batches: [{batch_no: "A1234567"}]}]},
915+
{ id: 70 }
916+
]
917+
}
918+
919+
get '/validate_required_arrays_under_optional_arrays', data
920+
expect(last_response.body).to eq("validate_required_arrays_under_optional_arrays works!")
921+
expect(last_response.status).to eq(200)
922+
end
923+
924+
it "simplest example using Arry -> Array -> Hash -> String" do
925+
subject.params do
926+
requires :orders, type: Array do
927+
requires :id, type: Integer
928+
optional :drugs, type: Array do
929+
requires :batch_no, type: String
930+
end
931+
end
932+
end
933+
934+
subject.get '/validate_required_arrays_under_optional_arrays' do
935+
'validate_required_arrays_under_optional_arrays works!'
936+
end
937+
938+
data = {
939+
orders: [
940+
{ id: 77, drugs: [{batch_no: "A1234567"}]},
941+
{ id: 70 }
942+
]
943+
}
944+
945+
get '/validate_required_arrays_under_optional_arrays', data
946+
expect(last_response.body).to eq("validate_required_arrays_under_optional_arrays works!")
947+
expect(last_response.status).to eq(200)
948+
end
949+
950+
it "simplest example using Arry -> Hash -> String" do
951+
subject.params do
952+
requires :orders, type: Array do
953+
requires :id, type: Integer
954+
optional :drugs, type: Hash do
955+
requires :batch_no, type: String
956+
end
957+
end
958+
end
959+
960+
subject.get '/validate_required_arrays_under_optional_arrays' do
961+
'validate_required_arrays_under_optional_arrays works!'
962+
end
963+
964+
data = {
965+
orders: [
966+
{ id: 77, drugs: {batch_no: "A1234567"}},
967+
{ id: 70 }
968+
]
969+
}
970+
971+
get '/validate_required_arrays_under_optional_arrays', data
972+
expect(last_response.body).to eq("validate_required_arrays_under_optional_arrays works!")
973+
expect(last_response.status).to eq(200)
974+
end
975+
976+
it "correctly indexes invalida data" do
977+
subject.params do
978+
requires :orders, type: Array do
979+
requires :id, type: Integer
980+
optional :drugs, type: Array do
981+
requires :batch_no, type: String
982+
requires :quantity, type: Integer
983+
end
984+
end
985+
end
986+
987+
subject.get '/correctly_indexes' do
988+
'correctly_indexes works!'
989+
end
990+
991+
data = {
992+
orders: [
993+
{ id: 70 },
994+
{ id: 77, drugs: [{batch_no: "A1234567", quantity: 12}, {batch_no: "B222222"}]}
995+
]
996+
}
997+
998+
get '/correctly_indexes', data
999+
expect(last_response.body).to eq("orders[1][drugs][1][quantity] is missing")
1000+
expect(last_response.status).to eq(400)
1001+
end
1002+
1003+
context "multiple levels of optional and requires settings" do
1004+
before do
1005+
subject.params do
1006+
requires :top, type: Array do
1007+
requires :top_id, type: Integer, allow_blank: false
1008+
optional :middle_1, type: Array do
1009+
requires :middle_1_id, type: Integer, allow_blank: false
1010+
optional :middle_2, type: Array do
1011+
requires :middle_2_id, type: String, allow_blank: false
1012+
optional :bottom, type: Array do
1013+
requires :bottom_id, type: Integer, allow_blank: false
1014+
end
1015+
end
1016+
end
1017+
end
1018+
end
1019+
1020+
subject.get '/multi_level' do
1021+
'multi_level works!'
1022+
end
1023+
end
1024+
1025+
it "with valid data" do
1026+
data_without_errors = {
1027+
top: [
1028+
{ top_id: 1, middle_1: [
1029+
{middle_1_id: 11}, {middle_1_id: 12, middle_2: [
1030+
{middle_2_id: 121}, {middle_2_id: 122, bottom: [{bottom_id: 1221}]}]}]},
1031+
{ top_id: 2, middle_1: [
1032+
{middle_1_id: 21}, {middle_1_id: 22, middle_2: [
1033+
{middle_2_id: 221}]}]},
1034+
{ top_id: 3, middle_1: [
1035+
{middle_1_id: 31}, {middle_1_id: 32}]},
1036+
{ top_id: 4 }
1037+
]
1038+
}
1039+
1040+
get '/multi_level', data_without_errors
1041+
expect(last_response.body).to eq("multi_level works!")
1042+
expect(last_response.status).to eq(200)
1043+
end
1044+
1045+
it "with invalid data" do
1046+
data = {
1047+
top: [
1048+
{ top_id: 1, middle_1: [
1049+
{middle_1_id: 11}, {middle_1_id: 12, middle_2: [
1050+
{middle_2_id: 121}, {middle_2_id: 122, bottom: [{bottom_id: nil}]}]}]},
1051+
{ top_id: 2, middle_1: [
1052+
{middle_1_id: 21}, {middle_1_id: 22, middle_2: [{middle_2_id: nil}]}]},
1053+
{ top_id: 3, middle_1: [
1054+
{middle_1_id: nil}, {middle_1_id: 32}]},
1055+
{ top_id: nil, missing_top_id: 4 }
1056+
]
1057+
}
1058+
# debugger
1059+
get '/multi_level', data
1060+
expect(last_response.body.split(", ")).to match_array([
1061+
"top[3][top_id] is empty",
1062+
"top[2][middle_1][0][middle_1_id] is empty",
1063+
"top[1][middle_1][1][middle_2][0][middle_2_id] is empty",
1064+
"top[0][middle_1][1][middle_2][1][bottom][0][bottom_id] is empty"
1065+
])
1066+
expect(last_response.status).to eq(400)
1067+
end
1068+
end
1069+
end
8861070
end
8871071

8881072
context 'multiple validation errors' do

0 commit comments

Comments
 (0)