Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update boolean handling of false #749

Merged
merged 3 commits into from
Apr 5, 2022
Merged

Update boolean handling of false #749

merged 3 commits into from
Apr 5, 2022

Conversation

colinbruce
Copy link
Contributor

@colinbruce colinbruce commented Apr 1, 2022

Expand the logic around blank value checking.

If @allow_blank is false, make sure it's not a BooleanValidator before looking for value.blank? as false is a valid boolean response, but

false.blank? # => true

Create a test to replicate what we are seeing in use, when passing the value of false to a :boolean parameter (as opposed to "false" which is parsed as a string)

Create a test to relicate what we are seeing in use,
When passing the value of `false` to a `:boolean` parameter
(as opposed to `"false"` which is parsed as a string)
it was ignoring the value and falling foul of the value.blank?
check that is always true for a FalseClass value
Remove some debugging steps and add a test for the
correct raising of an error when a String parameter
is sent an empty value
@colinbruce
Copy link
Contributor Author

I couldn't find a collaboration guide, so I hope this is okay?
If there are changes needed for CI, release notes, etc feel free to shout

Ensure an empty string passed to boolean
still fails, this is correctly failed
by `!@validator.valid?(value)` returning
an invalid response when expecting
`true` or `false`
@dbussink
Copy link

dbussink commented Apr 4, 2022

Given the discussion in #750 I think there's probably a deeper underlying issue here and not something purely affecting only the boolean case. So I think a fix / change also would need to address things more generically and not only for boolean values.

@colinbruce
Copy link
Contributor Author

Agreed - I was only fixing the issue I saw! 👍
Some analysis of #733 and regression testing of that may be useful @mathieujobin ?

@mathieujobin
Copy link
Collaborator

@colinbruce Thank you so much for this quick fix and added tests.
I didn't think of this case when reviewing the other PR.
After I get another 👍 I will merge and release as 0.7.1

@dbussink
Copy link

dbussink commented Apr 5, 2022

@colinbruce Thank you so much for this quick fix and added tests.

@mathieujobin I don't think this fix is complete then though? See also the comment earlier in #749 (comment). Would more changes be necessary here?

@mathieujobin
Copy link
Collaborator

@dbussink this still appears to be a good fix to add, the remaining issue can be fixed as a separate PR.
we need a test that reproduce it.

@mathieujobin mathieujobin merged commit 071d90a into Apipie:master Apr 5, 2022
@dbussink
Copy link

dbussink commented Apr 5, 2022

@dbussink this still appears to be a good fix to add, the remaining issue can be fixed as a separate PR.
we need a test that reproduce it.

What I was trying to say is that the issue extends to anything where .blank? returns true, not only for booleans then. But I'll look at a separate change if that's required then. I'm mostly not sure what the intended behavior is here and if it really needs a code change or a change to the release notes to indicate the change.

@mathieujobin
Copy link
Collaborator

I am thinking we should actually question ActiveSupport as to why false.blank? Is true?

I don't know of any other objects that would have such a problem.

The allow_blank option was defaulting to false but wasn't enforced. So thing #733 and this PR is good.

I think I will add more tests with more object types.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants