Do not try to coerce "" for numeric & date params

[jlw]

Description

When using an HTML form for searching or filtering and a field is left blank, Rails puts an empty string in the params hash, and this raised an unexpected InvalidParameterError when we defined the param as a non-required Integer, Float, Date, etc.

Now we will treat that empty string as an un-supplied param and only reject it if the param is flagged as required.

Additional Notes

• added shared examples for Float tests
• nested shared examples in the appropriate scope

[ifellinaholeonce]

Thanks for putting this up and for the clear explanation of the issue. It is an interesting and kind of unique case. I want to offer an alternative and see what you think about it.

One solution that the gem provides that could be helpful here is the transform operation. For the GET endpoint where empty parameters are causing issues you could:

param! :foo, Integer, transform: :presence

I haven't actually tested this but I suspect it should work because

''.presence -> nil
' '.presence -> nil
'foo'.presence -> 'foo'

This isn't a problem for parameters provided in the body, this issue is caused by the fact that all query parameter values are strings. My inclination is not to let the limitations for query parameters change the behaviour for body parameters. Interested in thoughts on this since it is opinionated. @jlw @iMacTia does anything stand out to you that I might be missing?

[iMacTia]

Yeah I agree with @ifellinaholeonce, this change might make sense under some circumstances, but it is affectively a breaking change as it changes existing behaviour and could cause unwanted consequences in other use-cases.

If there was a clear rule written somewhere where this behaviour is documented as the correct one, then we could consider it. But my understanding on this kind of issues is that there isn't such a rule, only common sense.
In that case, sticking with existing behaviour is our preferred stance, unless we can demonstrate the vast majority of cases would benefit from the change.

Interested to know if the transform: :presence workaround suggested above does help?

[jlw]

@ifellinaholeonce and @iMacTia - the suggested workaround does not work since the coerce code - which runs prior to transforms - rescues TypeError and then raises InvalidParameterError:

rails_param/lib/rails_param/param_evaluator.rb

Line 83 in 15d3cc8

raise InvalidParameterError.new("'#{param}' is not a valid #{type}", param: param_name)

so that is why this logic would need to be inside the coercion.

The only workaround that I've found for this with rails_param v1.3.1 is the horrid param! :user_id, Integer unless params[:user_id].blank?. That is such an ugly and counterintuitive pattern (especially when it is required multiple times in the same controller) that I carefully comment it to help the future maintainers of my app.

I have put together a sample app (https://github.com/jlw/rails_param_test_app) that shows the current problem I am trying to solve. You can try it out yourself:

• with rails_param v1.3.1: https://rails-param-test.onrender.com/
• adding transform: :presence: https://rails-param-test-transform.onrender.com/
• and with my change: https://rails-param-test-change.onrender.com/

It seems to me like the current gem release only works for JSON APIs and HTML forms where all integer, float, date, and/or time values are all required, which seems like an odd set of restrictions.

I completely understand having an initial resistance to anything that might introduce a breaking change, but I also don't understand the scenario that you think this change would break. Any required params attempted with an empty string will still be rejected. Have you really seen people write JSON API client-side handlers that provide empty strings for all undefined values? As a user of your gem, who has an endpoint that allows an optional integer param, why would I even care if the request was crafted with { user_id: null } vs.{ user_id: '' } as long as it comes through as params[:user_id].nil??

Am I missing something?

[jlw]

Note: after adding the test app last week to show that this affects both GET and POST forms, I have force-pushed my change with improved comments and commit message.

[iMacTia]

@jlw you make a very compelling argument and even provided detailed examples, so I can totally see why this is so annoying for folks doing normal Rails forms.

On the other hand, APIs are more expressive so we need to be careful about what is possible, what is not, and avoid breaking behaviour folks might be relying on.

The scary bit here is interpreting '' as nil, as that might not be desired behaviour in some circumstances.
For strings, this would obviously be unacceptable, but your change only affects numbers and dates, so that makes it more reasonable.

After giving it some thought, I'm not sure I can find a case where a number or date that were passed as blank values and interpreted as nil would cause issues.
In the end, we're changing the coercion of a string into a number/date by effectively saying that '' goes from being "not a valid number/date" to "a no-value number/date". It is indeed a change compared to the current behaviour, but it doesn't feel too stretched anymore after all this discussion.

@ifellinaholeonce what do you think?

[ifellinaholeonce]

I don't want to hold this up anymore than I already have. I have my reservations but I don't feel strongly enough to block this. @iMacTia if you're comfortable with it then I say we go for it.

we're changing the coercion of a string into a number/date by effectively saying that '' goes from being "not a valid number/date" to "a no-value number/date".

I really like this framing of the change. Thanks for that. I think part of my hesitation for this change comes because the "no-value" doesn't exist for the other type (excluding nil). Special cases always give me pause. I liked that this gem was opinionated and the coercion paths were the same for all the types.

the current gem release only works for JSON APIs and HTML forms where all integer, float, date, and/or time values are all required, which seems like an odd set of restrictions.

I am certainly guilty of approaching this codebase with a JSON API-first mindset. Thanks for reminding me that that isn't the only usecase.

Have you really seen people write JSON API client-side handlers that provide empty strings for all undefined values?

I've seen consumers of JSON APIs do all sorts of things. I'm not sure this is what we should be making our decision based on.

[iMacTia]

I am certainly guilty of approaching this codebase with a JSON API-first mindset.

Same here, I'm really out of the game when it comes to building forms with Rails, and I appreciate this must be annoying for those folks.

I have only one reservation though: this is, in practice, a backwards-incompatible change.
So I want to thread carefully and I have 3 proposals on how to avoid surprises:

1. We merge this as-is, but ship it as v2.0 – a bit heavy handed, people might have ~> 1.x pinning in their Gemfile and miss on the update
2. We add a config setting to enable this and ship it with a default to false to preserve previous behaviour – people won't know about this unless they stumble on the issue first
3. We make the gem smart enough to behave differently based on the use-case: the new behaviour for form submissions, the old one for JSON APIs – this is the most intriguing, but I'm unsure if there's a clear way to make the distinction. Maybe we can leverage the format from the request?

[ifellinaholeonce]

Option 3 seems very interesting but likely more complex. Option 2 seems really appealing to me - there have been a couple of threads in this repo where having configs would be helpful. This might be a good opportunity to introduce that pattern to the gem.

[jlw]

I would advise against option 3 - pushing any kind of request-related metadata down into the coercion logic strikes me as a nasty code smell. If you want to pursue option 2, I'll wait until you have a config pattern in place before refactoring this change.

I'm still curious about the assertion that this is a backwards-incompatible change. What examples have you seen in your careers where a JSON client was sending empty strings instead of null values? I've mainly worked with JSON APIs written by large companies and I've never seen anything like that.