Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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: 1 addition & 1 deletion .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2025-02-08 13:42:40 UTC using RuboCop version 1.71.2.
# on 2025-02-22 18:54:30 UTC using RuboCop version 1.71.2.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/dsl/parameters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ def map_params(params, element, is_array = false)
# @return hash of parameters relevant for the current scope
# @api private
def params(params)
params = @parent.params(params) if instance_variable_defined?(:@parent) && @parent
params = @parent.params_meeting_dependency.presence || @parent.params(params) if instance_variable_defined?(:@parent) && @parent
params = map_params(params, @element) if instance_variable_defined?(:@element) && @element
params
end
Expand Down
11 changes: 8 additions & 3 deletions lib/grape/validations/params_scope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module Grape
module Validations
class ParamsScope
attr_accessor :element, :parent, :index
attr_reader :type
attr_reader :type, :params_meeting_dependency

include Grape::DSL::Parameters

Expand Down Expand Up @@ -67,6 +67,7 @@ def initialize(opts, &block)
@type = opts[:type]
@group = opts[:group]
@dependent_on = opts[:dependent_on]
@params_meeting_dependency = []
@declared_params = []
@index = nil

Expand Down Expand Up @@ -94,7 +95,11 @@ def should_validate?(parameters)
def meets_dependency?(params, request_params)
return true unless @dependent_on
return false if @parent.present? && [email protected]_dependency?(@parent.params(request_params), request_params)
return params.any? { |param| meets_dependency?(param, request_params) } if params.is_a?(Array)

if params.is_a?(Array)
@params_meeting_dependency = params.filter { |param| meets_dependency?(param, request_params) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is memoized, shouldn't it be

Suggested change
@params_meeting_dependency = params.filter { |param| meets_dependency?(param, request_params) }
@params_meeting_dependency ||= params.filter { |param| meets_dependency?(param, request_params) }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not really memoized, @params_meeting_dependency is initiated as an empty array and we can not memoize it because:

[] || [1,2,3] # gives [] because it is a true value

so we will not be able to change the value, I guess we don't need to memoize it anyways we only set the value once and read it with attr_reader, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. Let’s get rid of the unneeded memorization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I think it is already not memorized, you mean this line right?

@params_meeting_dependency = params.filter { |param| meets_dependency?(param, request_params) }

I am already using = here, so which memorization you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, this is fine. I see we need @params_meeting_dependency later. I dislike that we have a @ in here but it can be called multiple times.

return @params_meeting_dependency.present?
end

meets_hash_dependency?(params)
end
Expand Down Expand Up @@ -127,7 +132,7 @@ def meets_hash_dependency?(params)
def full_name(name, index: nil)
if nested?
# Find our containing element's name, and append ours.
"#{@parent.full_name(@element)}#{brackets(@index || index)}#{brackets(name)}"
"#{@parent.full_name(@element)}#{brackets(index || @index)}#{brackets(name)}"
elsif lateral?
# Find the name of the element as if it was at the same nesting level
# as our parent. We need to forward our index upward to achieve this.
Expand Down
2 changes: 1 addition & 1 deletion spec/grape/dsl/parameters_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ def extract_message_option(attrs)
it 'inherits params from parent' do
parent_params = { foo: 'bar' }
subject.parent = Object.new
allow(subject.parent).to receive(:params).and_return(parent_params)
allow(subject.parent).to receive_messages(params: parent_params, params_meeting_dependency: nil)
expect(subject.params({})).to eq parent_params
end

Expand Down
122 changes: 122 additions & 0 deletions spec/grape/validations/params_scope_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,128 @@ def initialize(value)
expect(last_response.body).to eq 'inner3[0][baz][0][baz_category] is missing'
end

context 'detect when json array' do
before do
subject.params do
requires :array, type: Array do
requires :a, type: String
given a: ->(val) { val == 'a' } do
requires :json, type: Hash do
requires :b, type: String
end
end
end
end

subject.post '/nested_array' do
'nested array works!'
end
end

it 'succeeds' do
params = {
array: [
{
a: 'a',
json: { b: 'b' }
},
{
a: 'b'
}
]
}
post '/nested_array', params.to_json, 'CONTENT_TYPE' => 'application/json'

expect(last_response.status).to eq(201)
end

it 'fails' do
params = {
array: [
{
a: 'a',
json: { b: 'b' }
},
{
a: 'a'
}
]
}
post '/nested_array', params.to_json, 'CONTENT_TYPE' => 'application/json'

expect(last_response.status).to eq(400)
expect(last_response.body).to eq('array[1][json] is missing, array[1][json][b] is missing')
end
end

context 'array without given' do
before do
subject.params do
requires :array, type: Array do
requires :a, type: Integer
requires :b, type: Integer
end
end

subject.post '/array_without_given'
end

it 'fails' do
params = {
array: [
{
a: 1,
b: 2
},
{
a: 3
},
{
a: 5
}
]
}
post '/array_without_given', params.to_json, 'CONTENT_TYPE' => 'application/json'
expect(last_response.body).to eq('array[1][b] is missing, array[2][b] is missing')
expect(last_response.status).to eq(400)
end
end

context 'array with given' do
before do
subject.params do
requires :array, type: Array do
requires :a, type: Integer
given a: lambda(&:odd?) do
requires :b, type: Integer
end
end
end

subject.post '/array_with_given'
end

it 'fails' do
params = {
array: [
{
a: 1,
b: 2
},
{
a: 3
},
{
a: 5
}
]
}
post '/array_with_given', params.to_json, 'CONTENT_TYPE' => 'application/json'
expect(last_response.body).to eq('array[1][b] is missing, array[2][b] is missing')
expect(last_response.status).to eq(400)
end
end

it 'includes the parameter within #declared(params)' do
get '/test', a: true, b: true

Expand Down
Loading