Handle json array

[mohammednasser-32]

Fix #2526

There is an issue in handling json arrays,
The problem is here we mark a Scope with type Array as meeting dependency if at least 1 element meets the dependency,
And therefore in the next iteration (json items), @dependant_on is nil, @parent meets dependency, and so we validate all json items, even the ones that should be skipped since they are not meeting the dependency

So I added @params_meeting_dependency param (name might be changed) which filters only the json items that should be validated so we can skip the rest

[dblock]

@ericproulx take a look?

[dblock]

@mohammednasser-32 Thank you!

Run rubocop -a ; rubocop --auto-gen-config to make RuboCop happy in the meantime.

[mohammednasser-32]

@dblock Fixed 🙏

[ericproulx]

@ericproulx take a look?

I'll take a look

[ericproulx]

@mohammednasser-32 there's still an issue with the returned message

it 'fails' do
  params = {
    array: [
      {
        a: 'a',
        json: { b: 'b' }
      },
      {
        a: 'a'
      },
      {
        a: 'a',
        json: { b: 'b' }
      }
    ]
  }
  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

^ this test will fail.

[mohammednasser-32]

@ericproulx ah I see, the @scope.index is not being reset for nested json arrays, honestly with the way nested json arrays are currently handled I am not sure how to do it...maybe it requires more refactoring :/

[ericproulx]

@ericproulx ah I see, the @scope.index is not being reset for nested json arrays, honestly with the way nested json arrays are currently handled I am not sure how to do it...maybe it requires more refactoring :/

Yeah I know, It's a tough one.

[ericproulx]

@mohammednasser-32 I'm not sure your solution works. I've simplified the issue regarding params with a given and without.

Both endpoints array_without_given and array_with_given are expecting an array of hashes { a: Integer, b: Integer }. The array_with_given is just forcing the requires :b, type: Integer only if a is odd?

Sending the following payload array: [ { a: 1, b: 2 }, { a: 3 }, { a: 5 } ] where all a are odd? to both endpoints should return a body containing array[1][b] is missing, array[2][b] is missing. Unfortunately, only the array_without_given works.

Here's are the specs

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: ->(v) { v.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

[mohammednasser-32]

@ericproulx Indeed the solution returns the correct status code but not the proper indexing, alright feel free to close the PR then..thanks for checking!

[ericproulx]

@ericproulx Indeed the solution returns the correct status code but not the proper indexing, alright feel free to close the PR then..thanks for checking!

I think I found a solution.

[ericproulx]

@mohammednasser-32 could you add the tests from this comment in your branch and simply switch the @index || index in the full_name function here ? We should read "#{@parent.full_name(@element)}#{brackets(index || @index)}#{brackets(name)}". It felt really strange to not prioritize the index param.

[mohammednasser-32]

@ericproulx Yes that was it! working perfectly now..thank you! pushed here

[dblock]

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) }

[mohammednasser-32]

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?

[dblock]

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

[mohammednasser-32]

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?

[dblock]

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.

[dblock]

@ericproulx good with you? merge?

[ericproulx]

@mohammednasser-32 could you add the test in from that comment. It all started from that and I just want to make sure it passes

[mohammednasser-32]

@ericproulx done, it needed one extra small fix to handle json array within json array

[ericproulx]

@mohammednasser-32 just missing a changelog entry and we're good to go.

[mohammednasser-32]

@ericproulx done 🙏

[ericproulx]

@dblock sounds good to me.