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.

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

Bugfix: Correctly handle given in Array params #1625

Merged
merged 3 commits into from
Jun 12, 2017

Conversation

rnubel
Copy link
Contributor

@rnubel rnubel commented May 5, 2017

Addresses #1600 and #1602.

Array parameters are handled as a parameter that opens a scope with type: Array; given opens up a new scope, but was always setting the type to Hash. This patch fixes that, as well as correctly labeling the error messages associated with array fields.

@dblock
Copy link
Member

dblock commented May 5, 2017

Build needs to be fixed please.

@rnubel rnubel force-pushed the fix/given-within-array-param branch from 3d125cb to 44913f7 Compare May 5, 2017 14:52
timothysu added a commit to salsify/grape that referenced this pull request May 8, 2017
@timothysu
Copy link
Contributor

timothysu commented May 8, 2017

@rnubel Including an extra test to check for each element being independently validated fails. Given your test, removing bar from the second element exemplifies the issue, unless this is intended behavior.

it 'applies the constraint within each element' do
  post '/test',
    { foos: [{ foo_type: 'a' }, { baz_type: 'c' }] }.to_json,
    'CONTENT_TYPE' => 'application/json'

  expect(last_response.status).to eq(400)
  expect(last_response.body).to eq('foos[0][bar] is missing')
end

results in

expected: "foos[0][bar] is missing"
got: "foos[0][bar] is missing, foos[1][bar] is missing"

foos[1] should not have bar required because it does not contain a foo_type

@dblock
Copy link
Member

dblock commented May 9, 2017

I'll wait for @rnubel to comment on the above before merging. Code looks OK to me otherwise.

Array parameters are handled as a parameter that opens a
scope with `type: Array`; `given` opens up a new scope, but
was always setting the type to Hash. This patch fixes that,
as well as correctly labeling the error messages associated
with array fields.
@rnubel rnubel force-pushed the fix/given-within-array-param branch from 44913f7 to f2c9b95 Compare May 10, 2017 05:08
@rnubel
Copy link
Contributor Author

rnubel commented May 10, 2017

Good catch, @timothysu. The problem was that an Array block defines a single scope, for which should_validate? is only called once; the AttributesIterator within applies the validations to each attribute. So, I've updated the PR to perform the dependency check within that iteration, but it needs some polish and further testing.

@avellable
Copy link
Contributor

Any update on this?

@dblock
Copy link
Member

dblock commented Jun 8, 2017

Please contribute @avellable.

@dblock
Copy link
Member

dblock commented Jun 8, 2017

Still needs CHANGELOG and stuff.

return true if parent.nil?
parent.should_validate?(parameters)
end

def meets_dependency?(params)
if @dependent_on
Copy link
Member

Choose a reason for hiding this comment

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

Make this a guard, return unless ...

@avellable
Copy link
Contributor

@dblock Done: #1645

@rnubel
Copy link
Contributor Author

rnubel commented Jun 9, 2017

@avellable appreciate the help. dependency is the correct spelling of the word, though, and your changelog format is a bit off, so I've pulled the changes into this PR instead and put your name on the changelog entry as well.

@timothysu would you be able to test this patch to verify it works in real-life usage? I can test it against contrived scenarios, but there's no substitute for a real app.

@avellable
Copy link
Contributor

avellable commented Jun 10, 2017

@rnubel Thanks! Sorry I forgot to add your name in the CHANGELOG of my changes. I'll close my PR now.

@timothysu
Copy link
Contributor

@rnubel It doesn't seem to solve real-life usage issues, during a quick test. I'll double check and follow up with more actionable info when I get a chance.

@dblock
Copy link
Member

dblock commented Jun 12, 2017

I am going to merge this since it fixes at least one problem (per spec), @timothysu add anything on top that's failing in a new issue/spec please.

@dblock dblock merged commit f34fe12 into ruby-grape:master Jun 12, 2017
thogg4 added a commit to thogg4/grape that referenced this pull request Jun 12, 2017
use correct params class in declared

add changelog entry

add tests for declared class

fix rubocop

make changelog entry better

remove puts in tests

one more puts

change test names

conform changelog entry

return dynamic class name

parse response instead

remove params

Instrument validators with ActiveSupport::Notifications

Suppress `warning: method redefined`

Bugfix: Correctly handle `given` in Array params (ruby-grape#1625)

* Bugfix: Correctly handle `given` in Array params

Array parameters are handled as a parameter that opens a
scope with `type: Array`; `given` opens up a new scope, but
was always setting the type to Hash. This patch fixes that,
as well as correctly labeling the error messages associated
with array fields.

* Code review fix

* Add CHANGELOG entry

Add ability to include an array of modules as helpers

Changing helpers DSL to allow the inclusion of many modules.
This attemps to bring a better readability, since it seems to be
more intuitive to send a list of modules when the message in
question is called helpers.

ensure we return a string from test

return json in tests

Update README according to new `helpers` macro interface

Silence warnings (ruby-grape#1632)

* silence warnings

initialize vars in initializers

subclass hashie mash to silence warnings

rubocop fixes

add changelog entry

Revert "use correct params class in declared"

This reverts commit 61f0c8e.

fix tests

* remove disable_warnings in hashie mash

* make rubocop happy

* fix hashie tests
rnubel added a commit to rnubel/grape that referenced this pull request Oct 6, 2017
Multi-param and default validators, all of which implemented their
own `validate!` method, were not respecting the `given` dependency
as expected. This change copies the check into all those validators.

The bug was likely introduced by ruby-grape#1625, which required that the
dependency check be moved out of the scope itself and into the
validators. Rolling back that change would require fundamentally
re-architecting how array parameters are validated (possibly with
a new kind of scope).
rnubel added a commit to rnubel/grape that referenced this pull request Oct 6, 2017
Multi-param and default validators, all of which implemented their
own `validate!` method, were not respecting the `given` dependency
as expected. This change copies the check into all those validators.

The bug was likely introduced by ruby-grape#1625, which required that the
dependency check be moved out of the scope itself and into the
validators. Rolling back that change would require fundamentally
re-architecting how array parameters are validated (possibly with
a new kind of scope).
rnubel added a commit to rnubel/grape that referenced this pull request Oct 10, 2017
Multi-param and default validators, all of which implemented their
own `validate!` method, were not respecting the `given` dependency
as expected. This change copies the check into all those validators.

The bug was likely introduced by ruby-grape#1625, which required that the
dependency check be moved out of the scope itself and into the
validators. Rolling back that change would require fundamentally
re-architecting how array parameters are validated (possibly with
a new kind of scope).
jdurand pushed a commit to jdurand/grape that referenced this pull request Jan 25, 2019
* Bugfix: Correctly handle `given` in Array params

Array parameters are handled as a parameter that opens a
scope with `type: Array`; `given` opens up a new scope, but
was always setting the type to Hash. This patch fixes that,
as well as correctly labeling the error messages associated
with array fields.

* Code review fix

* Add CHANGELOG entry
jdurand pushed a commit to jdurand/grape that referenced this pull request Jan 25, 2019
* Bugfix: Correctly handle `given` in Array params

Array parameters are handled as a parameter that opens a
scope with `type: Array`; `given` opens up a new scope, but
was always setting the type to Hash. This patch fixes that,
as well as correctly labeling the error messages associated
with array fields.

* Code review fix

* Add CHANGELOG entry
jdurand pushed a commit to jdurand/grape that referenced this pull request Jan 25, 2019
* Bugfix: Correctly handle `given` in Array params

Array parameters are handled as a parameter that opens a
scope with `type: Array`; `given` opens up a new scope, but
was always setting the type to Hash. This patch fixes that,
as well as correctly labeling the error messages associated
with array fields.

* Code review fix

* Add CHANGELOG entry
@braktar braktar mentioned this pull request Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants