-
-
Notifications
You must be signed in to change notification settings - Fork 616
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
checkSchema support for multiple custom validators/sanitisers #1180
Conversation
Reviewing this later today! |
@@ -12,7 +12,7 @@ Its syntax looks like this: | |||
```js | |||
const { checkSchema, validationResult } = require('express-validator'); | |||
app.put( | |||
'/user/:id/password', | |||
'/user/:id', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so that it fits better with the validated schema below.
@@ -69,7 +64,7 @@ app.put( | |||
}, | |||
}, | |||
// Support if functionality in schemas | |||
someField: { | |||
age: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation wise looks really good!
My only comments are on the types part but if it works things are awesome
: K extends keyof SanitizersSchema | ||
? SanitizersSchema[K] | ||
: CustomSanitizerSchemaOptions | CustomValidatorSchemaOptions; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would format this if else segment type. Something like this:
export type ParamSchema<T extends string = DefaultSchemaKeys> =
BaseParamSchema &
ValidatorsSchema &
SanitizersSchema & {
[K in T]?: K extends keyof BaseParamSchema
? BaseParamSchema[K]
: K extends keyof ValidatorsSchema
? ValidatorsSchema[K]
: K extends keyof SanitizersSchema
? SanitizersSchema[K]
: CustomSanitizerSchemaOptions | CustomValidatorSchemaOptions;
};
also I would put the options you think will be used first, as it will generally make the typescript checker quicker.
Another option to this, if it makes sense, could be making things more narrow:
export type ParamSchema<T extends string = DefaultSchemaKeys> =
BaseParamSchema &
ValidatorsSchema &
SanitizersSchema & {
[K in T]?: K extends keyof BaseParamSchema
? BaseParamSchema[K]
: K extends keyof ValidatorsSchema
? ValidatorsSchema[K]
: K extends keyof SanitizersSchema
? SanitizersSchema[K]
: K extends keyof CustomSanitizerSchemaOptions ?
CustomSanitizerSchemaOptions[K]
: K extends keyof CustomValidatorSchemaOptions ?
CustomValidatorSchemaOptions[K]
: never;
};
Note that I've made having
CustomSanitizerSchemaOptions
orCustomValidatorSchemaOptions
a must otherwise, it will throw a never.
Maybe to work as the first example it maybe needing to add CustomSanitizerSchemaOptions
& CustomValidatorSchemaOptions
to the begin of the type, like this:
export type ParamSchema<T extends string = DefaultSchemaKeys> =
BaseParamSchema &
ValidatorsSchema &
SanitizersSchema &
CustomSanitizerSchemaOptions &
CustomValidatorSchemaOptions &
{
[K in T]?: K extends keyof BaseParamSchema
? BaseParamSchema[K]
: K extends keyof ValidatorsSchema
? ValidatorsSchema[K]
: K extends keyof SanitizersSchema
? SanitizersSchema[K]
: K extends keyof CustomSanitizerSchemaOptions ?
CustomSanitizerSchemaOptions[K]
: K extends keyof CustomValidatorSchemaOptions ?
CustomValidatorSchemaOptions[K]
: never;
};
if for some reason CustomSanitizerSchemaOptions
& CustomValidatorSchemaOptions
are nullable and you want to prevent that you can use this utilitary type like this:
type IfNullThen<IF, THEN> = IF extends null ? THEN : IF;
export type ParamSchema<T extends string = DefaultSchemaKeys> =
BaseParamSchema &
ValidatorsSchema &
IfNullThen<CustomValidatorSchemaOptions, ValidatorsSchema> & // if CustomValidatorSchemaOptions is null it returns ValidatorsSchema. ValidatorsSchema & ValidatorsSchema is equal to just ValidatorsSchema
SanitizersSchema & {
[K in T]?: K extends keyof BaseParamSchema
? BaseParamSchema[K]
: K extends keyof ValidatorsSchema
? ValidatorsSchema[K]
: K extends keyof SanitizersSchema
? SanitizersSchema[K]
: K extends keyof CustomSanitizerSchemaOptions ?
CustomSanitizerSchemaOptions[K]
: K extends keyof CustomValidatorSchemaOptions ?
CustomValidatorSchemaOptions[K]
: never;
};
if this is too much sorry about this :/ as you asked me on twitter but I only got the chance to review it closely now.
This was how I understood the problem that you wanted to solve. If all worlds but it seems too complex maybe making it simpler is okay or making a doc to this pr where we explain the decisions is a good call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ideas!
Can't do much formatting because of prettier 😛
Don't know if the performance wins will be obvious if I change things around. Maybe it's coincidentally at its optimal already 😄
The other suggestions are probably not going to work.
CustomSanitizerSchemaOptions | CustomValidatorSchemaOptions
represents the value really, not a dictionary of possible keys/values that K
might be in, so I have to dismiss your suggestion :(
But thanks again!!!
@fedeci do you have any thoughts? |
Description
This PR addresses #552 by adding support for multiple custom validators/sanitisers to
checkSchema()
.These names aren't used for anything but to allow placing the validators/sanitisers in different places in the resulting chain.
This is an advantage over using an array with
custom
/customSanitizer
, which wouldn't be too flexible.For example, the following would run the custom validators in sequence, which may not be desired:
To-do list
@ts-expect-error
for mixing custom validator/sanitiser syntax with standard syntax