Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Joi.string().optional() doesn't treat an empty string as unset #482

Closed
ThisIsMissEm opened this issue Nov 11, 2014 · 33 comments
Closed

Joi.string().optional() doesn't treat an empty string as unset #482

ThisIsMissEm opened this issue Nov 11, 2014 · 33 comments
Assignees
Labels
support Questions, discussions, and general support

Comments

@ThisIsMissEm
Copy link

I'm hitting into this when creating Joi schemas for forms, where I have a text input which can optionally be filled in, but if it's not, then no problems.

At the moment I have to use the work around of:

Joi.string().valid('').optional() // followed by any other validations

This is due to this line: https://github.com/hapijs/joi/blob/master/lib/string.js#L19

I'd be inclined to argue that if you've just said Joi.string() without any optional or required tags, then an empty string is actually a valid value. If you say it's required, then an empty string would be invalid.

Thoughts?

@ThisIsMissEm
Copy link
Author

Actually, that work around doesn't work if you've got an additional validation for regex, e.g.,

alias: Joi.string().regex(/^[a-zA-Z0-9][a-zA-Z0-9\-]+[a-zA-Z0-9]$/).optional()

This will never actually be allowed to be optional / empty. I really don't want to have to munge the object structure to remove keys that point to empty string values before I pass it into Joi to validate the form data.

@Marsup
Copy link
Collaborator

Marsup commented Nov 11, 2014

Aren't you looking for Joi.string().allow('').optional() ?

@Marsup Marsup self-assigned this Nov 11, 2014
@Marsup Marsup added the support Questions, discussions, and general support label Nov 11, 2014
@Marsup
Copy link
Collaborator

Marsup commented Nov 11, 2014

Not that it matters but it's optional by default, optional is if you want to transform back a joi object that has been .required().

@ThisIsMissEm
Copy link
Author

Oh, I always thought .optional() was a way of explicitly saying that that field in an object was optional.

Looks like .allow('') works.

@ThisIsMissEm
Copy link
Author

Also, I think the docs may be ambiguous here: https://github.com/hapijs/joi#anyoptional

@Marsup
Copy link
Collaborator

Marsup commented Nov 12, 2014

The wording seems fine to me, how would you explain it ?

@ThisIsMissEm
Copy link
Author

hmm.. actually, on a second reading, yeah, it does look fine. Would it be an idea to accept empty strings by default and have people explicitly require a length?

@Marsup
Copy link
Collaborator

Marsup commented Nov 12, 2014

I don't think that's the most common use case.

@ThisIsMissEm
Copy link
Author

Okay, but the other primitives don't put any constrain on values. e.g., array() doesn't force an array to have any values; nor does binary() force you to have data, just that the key is present. Same with object().

string() is the only one which sets a validation of not-empty by default.

@hueniverse
Copy link
Contributor

Because of how query strings work. q= will produce an empty string which is not a valid value in most cases. You can easily create your own type that allows empty strings. This was discussed a few times before.

@ThisIsMissEm
Copy link
Author

hmm, alright, that's a fair case. I guess I can indeed create a custom layer on top of Joi for this, as my use case isn't for an API, but rather a CRUD / Rails style admin panel.

@Marsup Marsup closed this as completed Nov 13, 2014
@hueniverse
Copy link
Contributor

It's as simple as export.myString = Joi.string().allow('');

@kevinsimper
Copy link

This quite baffled me that Joi.string() and Joi.string().optional() does not work.

@WesTyler
Copy link
Contributor

WesTyler commented Dec 2, 2016

What do you mean it does not work?

@kevinsimper
Copy link

@WesTyler It does work, but just not the way I would assume it would work. I would expect .required() to be needed if the string must not be empty.

@WesTyler
Copy link
Contributor

WesTyler commented Dec 4, 2016

Gotcha. FWIW, .optional() and .required() just set presence flags on the property, they don't affect the validity/constraints on the content of the property.

For example, you can require a property, but allow it to be an empty string; or you can allow an optional property, but place constraints on it in the case that it exists. That's why presence constraints are separate from content constraints in the schema definitions.

@stephen-last
Copy link

This confused me too. I thought setting Joi.string() would allow empty strings, because, well, it's still a string.

So Joi.string() effectively has a default constraint (non-empty) above and beyond the type check.

Joi.string().allow('') works.

Amazingly useful library BTW! 👍

@DavidTPate
Copy link
Contributor

@stephen-last If you'd like to take a stab at an update to the docs to clear up the possible confusion we're always glad to review and take action on it.

@stephen-last
Copy link

@DavidTPate The docs look fine, well, very good actually. This is a well maintained library.

I guess I'm just saying that the default behavior of Joi.string() confused me, reading the docs makes it clear though.

@Scott-MacD
Copy link

Personally I agree that if a string is set as optional, then it shouldn't force a string to be non-empty by default. A perfect example of this is form submission. In the joi schema, optional doesn't mean a form field is optional, as submitting a form sends empty strings for fields it doesn't have. So this forces extra extra explicitness that doesn't feel like it should be necessary to have to add .allow("") to every single form field that is actually optional.

@DavidTPate
Copy link
Contributor

DavidTPate commented Dec 14, 2017

The calls for any.optional() and any.required() are quite explicit on this. They define presence requirements, not what the values actually are namely required doesn't allow undefined while optional allows undefined.

Once the presence requirement is done, then you move on to validation. An empty string is a string. This isn't a truthy/falsey check, if it was then passing -1 or false for a string that is optional would be valid.

We're always glad to take a look at PRs and do our best to help them get merged, feel free to make a PR and see what the community thinks. Personally, I think it is an incorrect change as it removes explicitness that you should have within a validation library. You want to minimize side effects so you can control the input that you accept.

@Scott-MacD
Copy link

Scott-MacD commented Dec 15, 2017

Hi David, thanks for at least giving it a thought, happy to look into the repo and submit a PR once our holiday crunch is over. I agree with you on two major points you just stated, however I think we have a bit of a disagreement on what exactly that means.

You want to minimize side effects so you can control the input that you accept.

and

An empty string is a string.

To me, Joi.string() says "make sure this input is a string". I concur that an empty string is a string, and therefore "" returning an error on Joi.string() is an unexpected side effect. I would expect that if I wanted to disallow empty strings, I should have to be explicit about that, regardless of whether that's through .required() or .not(""), or whatever else is deemed most appropriate.

The main take away here is that "" definitely seems like it should pass Joi.string().optional(), regardless of whether .optional() is part of that equation or not. Joi.string().allow("").optional() seems redundant.

The more that I read over this, Joi.string().not("").optional() definitely sounds the most like how you'd actually describe what this validation does: "Optionally allow any string that is not """. Conversely, "Optionally allow any string, including """ sounds as silly to me personally (Isn't "" a string?).

@Marsup
Copy link
Collaborator

Marsup commented Jan 4, 2018

I'll admit the idea is starting to grow on me, if you're expecting a string it's easy enough to specify a min(1). This one seems to have broken the principle of least surprise more than it should. I might revisit someday but no promises on version or ETA.

aeftimia referenced this issue in aeftimia/paratii-lib Feb 13, 2018
Problem: according to its documentation all arguments in Joi are
considered optional. This is appearently incorrect, at least for
strings. It is currently unknown how to get joi to accept undefined
values as acceptable for strings which were meant to be optional
arguments.

to try this:

run `yarn test ./test/paratii.eth.vids.js` to try to get joi to handle
undefined strings for `id`, `ipfsHashOrig`, etc.

These seemed helpful, but did not fix the problem:
- [https://github.com/hapijs/joi/issues/482](https://github.com/hapijs/joi/issues/482)
- [https://github.com/hapijs/joi/issues/251](https://github.com/hapijs/joi/issues/251)
@Rohit221990
Copy link

joi@12.0.0 is allowing empty from (field.string().allow('').optional())

@NullSoldier
Copy link

string() not allowing empty strings is absolutely a bug that exists still in JOI 14.

  • It doesn't matter if you can work around the bug.
  • string() validates strings, "" is a string
  • requiring users to whitelist an already possible value is an insane solution

@WesTyler
Copy link
Contributor

You can always open a PR if you feel that strongly about it

@Marsup
Copy link
Collaborator

Marsup commented Feb 19, 2019

How can you describe an intentional behavior that's been there for years as a bug, let alone a regression? You may not understand or agree with the behavior, but stop depicting it as a bug when it clearly is not.

@Youssefares
Copy link

Any updates on this? I think this issue should be open. I disagree with @Marsup. I think the inconsistency with how joi.array() behaves alone makes this if not a bug, a much more pressing issue.

I am really surprised this hasn't been fixed given how old the issue is.

@hueniverse
Copy link
Contributor

This is never going to change. No one is ever going to change my mind on this. It's been like this from day one for a reason. It is not a bug just because it doesn't match your expectations.

I am going to add a big notice in the docs saying "empty strings are not valid strings, use allow('') if you want to allow them". That's the best I am willing to do here so at least no one is surprised by it.

@shonubijerry
Copy link

I thinks allow('') doesn't logically pass for an optional behavior. Spent quality time on figuring out that this is how to make fields optional.

@Rohit221990
Copy link

@shonubijery are you looking for optional pass...if yes, then try joi.string().optional()

@lostpebble
Copy link

I still find this all a bit silly. "" is a string ( typeof "" === "string" ). The default joi behavior is going beyond simple type validation here and into a validation on length on top of the type validation. If we wanted that, we'd use min(1).

This still trips me up every few months after not using Joi for a while.

@hueniverse
Copy link
Contributor

Still arguing about this is what's silly.

@hapijs hapijs locked and limited conversation to collaborators Feb 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
support Questions, discussions, and general support
Projects
None yet
Development

No branches or pull requests