Possible regression of handling of empty strings

[dbussink]

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:

 {"code":"invalid_params","message":"Invalid parameter 'notes' value nil: Must be a String"}

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 as nil. What we post looks like:

params: { notes: "" }

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.

[themilkman]

FWIW, I also had to add allow_blank: true to my required: false params. Not sure which behaviour was originally planned.

[colinbruce]

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 }

[mathieujobin]

@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]

@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?

[mathieujobin]

@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.

#751

cc: @ofedoren @themilkman

[dbussink]

@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]

@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.

[dbussink]

That also makes me wonder, should allow_blank by default match the value for required?

[mathieujobin]

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

[dbussink]

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.

[mathieujobin]

@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

[mathieujobin]

please reopen if I missed something