hapijs / joi

The most powerful data validation library for JS
Other
20.93k stars 1.51k forks source link

Schema assert in Joi extension args are not using updated value from validation #2470

Open Nargonath opened 4 years ago

Nargonath commented 4 years ago

Support plan

Context

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

In Confidence when using Joi@15 we used to validate extension rules arguments using the assert property with a Joi schema. In that schema we were using a .default and it worked. The rule argument was initialized with the default when value was not given.

We recently updated to the latest v17 and we noticed that this was no longer the case when switching to the new extension API. I believe this is a breaking change that was not documented in Joi@16 release notes but I may be wrong. At least I didn't find it there FWIW. I think the problem comes from here since the value resulting from the validation is not used. Compared to v15 where it seems that the value from the arguments validation is used afterwards, i'm not entirely sure that's the right line though.

What was the result you got?

The .default was not applied hence the subsequent conditions in the validate function based on the assumption that it was applied did not work anymore.

What result did you expect?

I expected the .default to be applied.

My point is to bring awareness on this issue but I don't know if it's an intended breaking change that just got missed during the changelog write-up or whether it's an unintended breaking change. Depending on the situation we could look into either add a mention in the v16 release note issue about it or see whether that's a behavior you'd want to bring back or not. Either way I'd be happy to help.

AdriVanHoudt commented 4 years ago

I encountered the same issue on joi-phone-number with a .single. We solved it with an alternatives().try and manual cast to array. See https://github.com/Salesflare/joi-phone-number/blob/1876840415347663bd652077941621cc03d86644/lib/index.js#L37 and https://github.com/Salesflare/joi-phone-number/blob/1876840415347663bd652077941621cc03d86644/lib/index.js#L53 I definitely expected the .single to work/convert.

noinkling commented 2 years ago

This doesn't happen to affect .default() or .single(), but in addition to not using the result, Joi also explicitly disables conversion/casting (which could be helpful for similar reasons) by adding .strict() to the supplied schema: https://github.com/sideway/joi/blob/2cde8a3656498753b42ec22d1bf7f871959488a3/lib/extend.js#L120-L122

We do have the ability to supply a normalize function as a workaround: https://github.com/sideway/joi/blob/2cde8a3656498753b42ec22d1bf7f871959488a3/lib/base.js#L606-L609

But separating the conversion from the validation seems super clunky given how it worked in v15 / how the library provides the feature to do both at once.

noinkling commented 2 years ago

Another issue: if your method has a required argument, you can't enforce that using assert (whether using a schema or a function) because $_addRule() simply bails out before running it when the arg is undefined:

https://github.com/sideway/joi/blob/2cde8a3656498753b42ec22d1bf7f871959488a3/lib/base.js#L591-L594

If you don't account for this, and the argument is missing, you won't get an error at schema construction time, only when you use it to validate something. Actually some of the built-in methods with a required arg have this issue too, e.g: number.greater/less/max/min/multiple() or string.length/max/min() but not others like number.precision/sign() or string.case().

In v15 you could just add .required() to the arg schema and have it work like you would expect.