Ensure complete declared params structure is present #2103

tlconnor · 2020-09-17T05:25:36Z

Back in April 2020 a fix was merged that ensured that missing Hash / Array params would be initialized as [] / {} during declared(params, include_missing: true).

#2043

This fix introduced a bug that would result in deeply nested param structures being replaced with an empty Hash.
#2101

This PR fixes the issue by ensuring that missing hashes are only initialized as {} if they don't have any declared nested params.

Fixes #2101

dblock

This is great. Thank you.

Let's beef up the specs a little bit: make one without deep nesting, make a separate spec for deep nested hashes, deep nested arrays, multiple levels of deep nesting.

And let's go an extra mile in README to explain these behaviors?

UPGRADING needs a paragraph, too.

tlconnor · 2020-09-18T04:06:20Z

@dblock I have improved the specs and updated the README / CHANGELOG / and UPGRADING guide.

It is not clear to me what we should do with missing Set params. Do you think a missing Set / Set[Anything] should be resolved as Set.new, or do you think they should remain as nil?

dblock

This LGTM with minor comments for UPGRADING.

For Set, I think it should be Set.new because [] and {} are just shortcuts for Array.new and Hash.new.

2.6.6 :001 > Hash.new
 => {} 
2.6.6 :002 > {}.class
 => Hash 
2.6.6 :003 > Hash.new == {}
 => true 
2.6.6 :004 > Array.new
 => [] 
2.6.6 :005 > [].class
 => Array 
2.6.6 :006 > Array.new == []
 => true

dblock · 2020-09-18T12:34:24Z

UPGRADING.md

@@ -1,6 +1,19 @@
 Upgrading Grape
 ===============

+### Upgrading to >= 1.4.1
+
+Previous to 1.3.3, the `declared` helper would always return the complete params structure if `include_missing=true` was set. In 1.3.3 the behavior was changed, so a missing Hash with or without nested parameters would always resolve as `{}`.


Previous -> prior

dblock · 2020-09-18T12:36:02Z

UPGRADING.md

+* Hash params with childern will resolve as a hash with keys for each declared child.
+* Hash params with no children will resolve as `{}`.
+* Array params will resolve as `[]`.
+* All other params will resolve as `null`.


resolve "to", not "as" I think is more correct

Add a simple before/after example.

Add a link to the PR, For more information see [#2103](https://github.com/ruby-grape/grape/pull/2103).. We've been forgetting those, would be nice to re-add where missing below.

dblock · 2020-09-18T12:43:50Z

If you don't mind feeding my OCD, squash your last two commits (amend the second one that makes the actual code change with the CHANGELOG, etc. update), and keep the move of the spec separate, so we can continue seeing the diff in the specs. NBD otherwise.

tlconnor · 2020-09-27T23:48:51Z

@dblock all comments have now been addressed. I have also added support for Set, and have included test cases for Array[Something] and Set[Something].

dblock

I'm good with this with a 🍏 build and a 1.5 version bump.

dblock · 2020-09-28T16:22:56Z

UPGRADING.md

@@ -1,6 +1,45 @@
 Upgrading Grape
 ===============

+### Upgrading to >= 1.4.1
+
+Prior to 1.3.3, the `declared` helper would always return the complete params structure if `include_missing=true` was set. In 1.3.3 the behavior was changed, so a missing Hash with or without nested parameters would always resolve to `{}`.


was changed -> a regression was introduced that ...

dblock · 2020-09-28T16:23:17Z

UPGRADING.md

+
+Prior to 1.3.3, the `declared` helper would always return the complete params structure if `include_missing=true` was set. In 1.3.3 the behavior was changed, so a missing Hash with or without nested parameters would always resolve to `{}`.
+
+In 1.4.1 this behavior is reverted, so the whole params structure will always be available via `declared`, regardless of whether any params are passed.


Let's make this 1.5, and please bump the version in version.rb.

dblock · 2020-09-29T15:13:44Z

Change other references to 1.4.1 (in CHANGELOG, etc.) to 1.5.

dblock · 2020-09-29T15:15:49Z

lib/grape/dsl/inside_route.rb

            {}
-          elsif should_be_empty_array?(passed_children_params, params_nested_path)
+          elsif type == 'Array' || type&.start_with?('[')


This seems iffy but I'll let it slide. I wish we were comparing a non-String type...

I agree, it is a bit messy. It was also pre-existing, so not entirely my fault 😄

tlconnor · 2020-09-29T21:21:18Z

Change other references to 1.4.1 (in CHANGELOG, etc.) to 1.5.

Not sure why I missed those. Now fixed.

dblock · 2020-09-30T12:21:57Z

Merged, thanks for your excellent work here, @tlconnor! You might want to email the mailing list asking people to test this out before we release v. next.

tlconnor · 2020-10-01T04:58:00Z

@dblock please hold off from releasing the new version, through testing against my app I believe I have found a newly introduced bug. I will work on a fix ASAP.

Prior to Grape v1.5.0 and ruby-grape#2103, the following would return `nil`: ``` params do optional :status_code, types: [Integer, String] end get '/' do declared_params end ``` However, now it turns an empty `Array`. We restore the previous behavior by not returning an empty `Array` if multiple types are used. Closes ruby-grape#2115

dblock requested changes Sep 17, 2020

View reviewed changes

tlconnor force-pushed the deepDeclared branch from e219776 to ec3bd8d Compare September 18, 2020 04:04

dblock approved these changes Sep 18, 2020

View reviewed changes

tlconnor force-pushed the deepDeclared branch from ec3bd8d to 5dc4e5b Compare September 27, 2020 23:47

dblock approved these changes Sep 28, 2020

View reviewed changes

tlconnor force-pushed the deepDeclared branch from 5dc4e5b to 0a08f04 Compare September 28, 2020 22:35

dblock reviewed Sep 29, 2020

View reviewed changes

Tim Connor added 2 commits September 30, 2020 10:19

Move Grape::Endpoint#declared specs into own spec file

4913534

Ensure complete declared params structure is present

678cd13

tlconnor force-pushed the deepDeclared branch from 0a08f04 to 678cd13 Compare September 29, 2020 21:19

dblock merged commit 7e43215 into ruby-grape:master Sep 30, 2020

dblock mentioned this pull request Oct 5, 2020

field using types turned into array when declared(params) #2112

Open

stanhu mentioned this pull request Oct 9, 2020

Fix declared_params regression with multiple allowed types #2116

Merged

johnk87 mentioned this pull request Oct 18, 2021

Declared broken for hash params with overlapping names #2195

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure complete declared params structure is present #2103

Ensure complete declared params structure is present #2103

tlconnor commented Sep 17, 2020

dblock left a comment •

edited

Loading

tlconnor commented Sep 18, 2020

dblock left a comment

dblock Sep 18, 2020

dblock Sep 18, 2020 •

edited

Loading

dblock commented Sep 18, 2020 •

edited

Loading

tlconnor commented Sep 27, 2020

dblock left a comment

dblock Sep 28, 2020

dblock Sep 28, 2020

dblock commented Sep 29, 2020

dblock Sep 29, 2020

tlconnor Sep 29, 2020

tlconnor commented Sep 29, 2020

dblock commented Sep 30, 2020

tlconnor commented Oct 1, 2020


		Prior to 1.3.3, the `declared` helper would always return the complete params structure if `include_missing=true` was set. In 1.3.3 the behavior was changed, so a missing Hash with or without nested parameters would always resolve to `{}`.

		In 1.4.1 this behavior is reverted, so the whole params structure will always be available via `declared`, regardless of whether any params are passed.

Ensure complete declared params structure is present #2103

Ensure complete declared params structure is present #2103

Conversation

tlconnor commented Sep 17, 2020

dblock left a comment • edited Loading

Choose a reason for hiding this comment

tlconnor commented Sep 18, 2020

dblock left a comment

Choose a reason for hiding this comment

dblock Sep 18, 2020

Choose a reason for hiding this comment

dblock Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

dblock commented Sep 18, 2020 • edited Loading

tlconnor commented Sep 27, 2020

dblock left a comment

Choose a reason for hiding this comment

dblock Sep 28, 2020

Choose a reason for hiding this comment

dblock Sep 28, 2020

Choose a reason for hiding this comment

dblock commented Sep 29, 2020

dblock Sep 29, 2020

Choose a reason for hiding this comment

tlconnor Sep 29, 2020

Choose a reason for hiding this comment

tlconnor commented Sep 29, 2020

dblock commented Sep 30, 2020

tlconnor commented Oct 1, 2020

dblock left a comment •

edited

Loading

dblock Sep 18, 2020 •

edited

Loading

dblock commented Sep 18, 2020 •

edited

Loading