Closed dbussink closed 2 years ago
FWIW, I also had to add allow_blank: true
to my required: false
params. Not sure which behaviour was originally planned.
See my comments around booleans #748
We were seeing the same thing, I think it's the change at #733 that is affecting it
We also saw nil
warnings when submitting
params: { boolean: false }
@dbussink would you be able to add a test to reproduce the issue?
if this is a blocker to you or anyone, feel free to use v0.6.0
which does not have the involved change
@dbussink would you be able to add a test to reproduce the issue?
Looks like https://github.com/Apipie/apipie-rails/pull/749/files#diff-d4fcdc9897861b246c2b7c55a697f50ffa978ffda2d1abb45b0879c5a86a6221R129 explicitly added a test / the behavior that an empty string should raise an error. So in that sense, it looks like the current behavior would be what is intended?
But if this behavior change is intended, I think it would be good to add it to the release notes that this is explicit breaking changing behavior?
@dbussink you are correct. I have added a note to the Changelog, and a few more tests to be extra explicit. please let me know if that solves your concerns.
https://github.com/Apipie/apipie-rails/pull/751
cc: @ofedoren @themilkman
@dbussink you are correct. I have added a note to the Changelog, and a few more tests to be extra explicit. please let me know if that solves your concerns.
Yeah, if this is now explicit behavior that is desired, we'll look at updating the checks. I suspect we'll have to add a lot of allow_blank: false
on all our properties to not break the API for existing users.
@dbussink you are correct. I have added a note to the Changelog, and a few more tests to be extra explicit. please let me know if that solves your concerns.
Btw, I do have another concern I realize. The error message doesn't really describe well what's wrong. It complains that the value is nil
, not that it is the empty string. Especially for a non required field it seems strange since nil
/ not specifying the field is allowed and what should be done.
That also makes me wonder, should allow_blank
by default match the value for required
?
Good points, not to be taken likely. I wonder if it would not make sense to have a global option to revert back to ignoring allow_blank, I still consider it a bug, but for backward compatibility. it makes sense
Good points, not to be taken likely. I wonder if it would not make sense to have a global option to revert back to ignoring allow_blank, I still consider it a bug, but for backward compatibility. it makes sense
For now we've added allow_blank
where needed in a change to update to 0.7.0, but going to wait for 0.7.1 with the boolean fix then.
@dbussink I have also added a configuration variable, so if you encounter any problem you should be able to revert to 0.6.0 behavior promptly.
please see https://github.com/Apipie/apipie-rails/releases/tag/v0.7.1 RELEASE NOTES and merged PR #751
please reopen if I missed something
There seems to be a change / regression in updating from 0.5.20 to 0.7.0 around the behavior of how empty strings are treated. We're seeing an error like this:
The
notes
parameter here is a string, but it's optional. But when an explicit empty string value is passed in, we get the above error that the value is seen asnil
. What we post looks like:I've been looking through the release notes and I can't really find anything that indicates this is a deliberate change or not. Also that it treats it as
nil
instead of the empty string seems a bit confusing in the error message here.