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

Chaining string.insensitive() will not convert values with lowercase/uppercase modifiers #1191

Closed
rooftopsparrow opened this issue May 18, 2017 · 21 comments
Assignees
Labels
breaking changes Change that can breaking existing code bug Bug or defect
Milestone

Comments

@rooftopsparrow
Copy link

Context

  • node version: v6.10.2
  • joi version: v10.4.2
  • environment (node, browser): node
  • used with (hapi, standalone, ...): standalone
  • any other relevant information: macOS 10.12.14

What are you trying to achieve or the steps to reproduce ?

I thought it was fairly confusing that these options do not work together

const schema = Joi.string().valid(['A', 'B', 'C']).insensitive().lowercase()
schema.validate('A')
// { error: null, value: 'A' }
// or for uppercase
const anotherSchema = Joi.string().valid(['a', 'b', 'c']).insensitive().uppercase()
anotherSchema.validate('b')
// { error: null, value: 'b' }

Which result you had ?

After validation with schema, the value was 'A'
After validation with anotherSchema, the value was 'b'

What did you expect ?

After validation with schema, the value should be 'a'
After validation with anotherSchema, the value should be 'B'

Now its fine that there is a workaround to just convert the array to the correct casing and use:

Joi.string().valid(['a', 'b', 'c']).lowercase();
schema.validate('A')
// { error: null, value: 'a' }

but seems like odd behavior for this to not work with both. I can submit a PR if you agree.

Thanks for Joi!

@WesTyler
Copy link
Contributor

So essentially you want the validation portion to be insensitive() but you want to leverage .lowercase() as a data transformation step?

@rooftopsparrow
Copy link
Author

Yes, I would expect them both to work together. The fact that insensitive() and lowercase() do produce the same validation, but different transformations when used together is weird and awkward.

@WesTyler
Copy link
Contributor

So, from looking at Joi primarily as a validator it seems weird to me to expect them both to be respected equally. When validating a string, you're simultaneously saying "I don't care what case it is" and "it must be lowercase".

That said, I think there is room for improvement on the coercion step. If the validation rule is insensitive I don't see any reason for either lowercase or uppercase to be applied to the value as well. I imagine a PR along those lines would be up for review :)

@rooftopsparrow
Copy link
Author

I agree with you that its a little strange from a validation perspective. But the statement

"I don't care what case it is" and "it must be lowercase".

are not mutually exclusive. The lowercase() is a direct subset of the insensitive(), and I would expect that the most specific rule wins out if you chain them ( like an override ).

But like you said, this is really just about the conversion, and perhaps we need to look at what happens if the conversion doesn't happen? Does this make any sense if you have convert: false?

schema = Joi.string().valid(['a', 'b', 'c']).insensitive().lowercase();
schema.validate('A', { convert: false })
/// ???

This does seem really strange...because I'm not sure what the behavior should be.

@WesTyler
Copy link
Contributor

Like you said

I would expect that the most specific rule wins out if you chain them

so the current validation behavior matches my personal expectation:

Joi.string().insensitive().lowercase().options({convert: false}).validate('Ay No');
// ValidationError: "value" must only contain lowercase characters

That's primarily why I could get behind a PR to apply the same idea to the data coercion/conversion. :)

@WesTyler
Copy link
Contributor

Hey wait...

Joi.string().insensitive().lowercase().validate('Ay No')
// { error: null, value: 'ay no' }

It looks like it's already doing the thing.

@rooftopsparrow
Copy link
Author

Ah, yeah this doesn't work when you specify a whitelist 😄

@rooftopsparrow
Copy link
Author

And more to the point, the validation against the whitelist is what is the concern. If you have a whitelist in mixed case ( perhaps dynamically generated ) you may want many things:

  • to validate against that whitelist insensitively
  • but convert the result to upper/lowercase

@WesTyler
Copy link
Contributor

I just went back and read your original issue, and I apologize for not catching the .valid specifically.

I think that's actually a different question altogether on whether or not the values provided to valid should be coerced by the additional schema options. And I actually think probably not at this point.

@rooftopsparrow
Copy link
Author

I think this is actually just a documentation problem.

If the validation convert option is on (enabled by default), a string will be converted using the specified modifiers for string.lowercase(), string.uppercase(), string.trim(), and each replacement specified with string.replace()

and paired with

string.insensitive()
Allows the value to match any whitelist of blacklist item in a case insensitive comparison.

The documentation suggests that lowercase(), uppercase(), trim() and replace() are convert options only, and not validation directives.

@WesTyler
Copy link
Contributor

Documentation PRs are always welcome ;)

@WesTyler WesTyler added the documentation Non-code related changes label May 19, 2017
@rooftopsparrow
Copy link
Author

rooftopsparrow commented May 19, 2017

Yeah, basically does removing that whole paragraph make it correct? Because each one of those is separately documented.

I'll gladly send a PR 👍

@WesTyler
Copy link
Contributor

I think so, yes, since there is a note on the convert behavior on the individual methods.

@rooftopsparrow
Copy link
Author

rooftopsparrow commented May 19, 2017

I think this is still a bug at the valid() whitelist level because these two statements behave differently

Joi.string().valid(['c', 'b', 'a']).insensitive().lowercase().validate('A', { convert: false })
// { error: null, value: 'A' }
Joi.string().insensitive().lowercase().validate('A', { convert: false })
// { error: ValidationError: "value" must only contain lowercase characters ...

@WesTyler
Copy link
Contributor

I'll let others with more experience in managing Joi make the final call, but my first instinct is to say that valid values should not be coerced. I'm honestly a bit surprised that valid(['A']).insensitive() is a thing, haha. To me, valid is a strict equality and adding coerced values breaks hurts my confidence in cases where I really do want it to be a strict list of valid values.

That being said, I don't have strong feelings about it and could be convinced otherwise :)

@DavidTPate
Copy link
Contributor

Yeah, I agree with you there @WesTyler. That said, I'd definitely be willing to take a look at a PR to address your concerns @rooftopsparrow.

@Marsup Marsup added bug Bug or defect and removed documentation Non-code related changes labels Jun 2, 2017
@Marsup
Copy link
Collaborator

Marsup commented Jun 2, 2017

Re-qualifying as a bug. valid should win (as-is), always, and uppercase or lowercase are indeed omitted, it would be different for allow. The missing part here is the "as-is".

@rooftopsparrow
Copy link
Author

Just so I'm 100% clear here (sorry this issue kind of went all over). This behavior is wrong:

Joi.string().valid(['c', 'b', 'a']).insensitive().lowercase().validate('A', { convert: false })
// { error: null, value: 'A' }

and should instead return the validation message from the .valid() chain?

@Marsup
Copy link
Collaborator

Marsup commented Jun 2, 2017

From my perspective yes, it should return 'a' not matter the casing, lowercase being superfluous.

Remember valid is a strict whitelisting, you could also have Joi.string().uppercase().valid(true, false, 42), insensitive only influencing the matching on strings, but otherwise I consider it should act as an enumeration and return the list you provided if a match is found, consistency being your problem.

@Marsup
Copy link
Collaborator

Marsup commented Jun 2, 2017

And to answer about the convert (which I totally missed), good question, I'd say probably not, when convert is false the input shouldn't be transformed.

@Marsup Marsup added the breaking changes Change that can breaking existing code label Aug 14, 2018
@Marsup Marsup self-assigned this Aug 14, 2018
@Marsup Marsup added this to the 14.0.0 milestone Aug 14, 2018
@Marsup Marsup closed this as completed in 67ae968 Oct 14, 2018
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking changes Change that can breaking existing code bug Bug or defect
Projects
None yet
Development

No branches or pull requests

4 participants