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

Painful defaults #1559

Open
Marsup opened this Issue Aug 3, 2018 · 15 comments

Comments

Projects
None yet
7 participants
@Marsup
Member

Marsup commented Aug 3, 2018

Hi people!

I'm trying to create a list of the painful defaults that you would like to see disappear in the next breaking release, so please help me if you see something I don't.

So far I have:

  • empty strings not being valid
  • stripUnknown stripping arrays (does anyone even use that ?) Done in v14.
  • most people expect minDomainAtoms: 2 in string().email() apparently, should it change?

Anything else ?

@Marsup Marsup added this to the 14.0.0 milestone Aug 3, 2018

@Marsup Marsup self-assigned this Aug 3, 2018

@sholladay

This comment has been minimized.

sholladay commented Aug 3, 2018

empty strings not being valid

IMO this is good default behavior, but it's made confusing by the fact that .required() is not the default. As a result, the current behavior feels inconsistent...

joi.attempt(undefined, joi.string());  // no error
joi.attempt('', joi.string());         // ValidationError: "value" is not allowed to be empty

My preference would be this:

joi.attempt(undefined, joi.string());  ValidationError: "value" is required
joi.attempt('', joi.string());         // ValidationError: "value" is not allowed to be empty
joi.attempt('', joi.string().min(0));      // no error
joi.attempt(undefined, joi.string().min(0));  ValidationError: "value" is required
@devinivy

This comment has been minimized.

Member

devinivy commented Aug 3, 2018

Hmmm, not sure how I feel about required() being the default—would you want it that way only for the string type? If we go that route, I would at least request that it be consistent across all types.

The way I often handle this behavior (naturally, depends on the situation!) is with empty('').

@Marsup

This comment has been minimized.

Member

Marsup commented Aug 3, 2018

When is it a good default apart from query strings ? (which can easily be solved by Joi.string().min(1))

@sholladay

This comment has been minimized.

sholladay commented Aug 3, 2018

To be clear, I would want required on all types. That would be very strange if it were only for strings.

As for .empty(''), I find it almost laughable that I have to tell joi that '' is empty. IMO, it should know that already and on my side, I should specify if I want to allow empty things, or allow specific empty things. But I shouldn't have to specify what is empty, at least for such a general case as this.

@WesTyler

This comment has been minimized.

Contributor

WesTyler commented Aug 3, 2018

@WesTyler

This comment has been minimized.

Contributor

WesTyler commented Aug 3, 2018

I'm also not a big fan of .required() as the default. I like it being explicit.

@sholladay

This comment has been minimized.

sholladay commented Aug 3, 2018

When is it a good default apart from query strings ?

The vast majority of forms that I've wrriten contain more required fields, such as username, than optional fields. For them, an empty string and undefined are approximately the same thing. The user didn't fill it out.

I prefer for strictness to be the default and force people to be explicit about optionality. It's important for me to know when something is optional because that implies extra complexity - there usually needs to be double the code paths to handle whether the field was given or not.

@Marsup

This comment has been minimized.

Member

Marsup commented Aug 3, 2018

To be clear I was asking about empty strings being denied, not required becoming default, for which I haven't seen evidence in the last few years that people were confused about it.

The default presence seems very subjective to me, personally I prefer seeing mandatory properties being mentioned, pretty much following the standard of putting red stars on form fields for the same purpose. Considering the migration hell it would create, I'd rather have really compelling advantages before changing it.

@DavidTPate

This comment has been minimized.

Contributor

DavidTPate commented Aug 7, 2018

  • string.uri([options]) the option allowRelative being false by default has been a little weird as it is a very common thing to have relative URLs. I haven't really seen much complaints about it though so it might just be a non-issue.
@rokoroku

This comment has been minimized.

Contributor

rokoroku commented Aug 7, 2018

👍 for disable stripUnknown stripping arrays.
its behavior (stripping failed value rather than returning a validation error) is mostly not an expected result, and also a thing that somewhat hard to inspect

Marsup added a commit that referenced this issue Aug 11, 2018

@skeggse

This comment has been minimized.

Member

skeggse commented Aug 11, 2018

I'm just gonna toss in my vote that we leave the current email behavior until there are no more gTLDs with MX records. I do appreciate the desire to reject common false-positives, but IMO we use isemail to be super accurate and using minDomainAtoms begins to reject "valid" (though discouraged) email addresses. As of today, there are 23 gTLDs with MX records

@Marsup

This comment has been minimized.

Member

Marsup commented Aug 12, 2018

@skeggse How often does it really happen though? I'm trying to go for the most common case here, but of course if that doesn't make sense people are free to override.

@skeggse

This comment has been minimized.

Member

skeggse commented Aug 12, 2018

To be sure, that happens super rarely. My inclination is to steer towards the more conservative approach, but you're more familiar with joi's users and use-cases, so I'm deferring to your judgement.

@Marsup

This comment has been minimized.

Member

Marsup commented Aug 12, 2018

In my experience, people expect email validation to be user@domain.tld at least, they don't want to validate user@localhost, that's not something you see very often in a web context, which represents most of the joi usage. I just have to understand the little issue I posted on isemail repo ;p

@skeggse

This comment has been minimized.

Member

skeggse commented Aug 12, 2018

Fwiw I would usually add localhost to the blacklist. Alternatively it would also fail the DNS resolution test (provided you make the call to a public DNS server like 1.1.1.1 or 8.8.8.8). I think minDomainAtoms=2 is reasonable but I'd still never leave it as default :p

@Marsup Marsup removed this from the 14.0.0 milestone Oct 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment