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

Made customSanitizer await for async functions #919

Merged

Conversation

NathanPB
Copy link
Contributor

@NathanPB NathanPB commented Aug 25, 2020

I made this little tweak to make possible use customSanitize in async environments. I did not made a proper error handler because I don't know how to integrate that with the validators, so I don't think the work of this feature is done, that's just a draft. This is related to #784

An example use case for this would be: body('user').customSanitizer(async(id) => await users.findById(id))

Also made the tests for that

@coveralls
Copy link

coveralls commented Aug 25, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 3686317 on NathanPB:feature/await-sanitizer into ef31f67 on express-validator:master.

Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Draft is already looking good to me!
What do you suggest that we do with the errors?

src/context-items/sanitization.ts Outdated Show resolved Hide resolved
@NathanPB
Copy link
Contributor Author

NathanPB commented Sep 25, 2020

What do you suggest that we do with the errors?

I'd suggest automatically send a 500 http error code and abort the request at all.

Adding the error to the error list given by validationResult(req) seems wrong because usually people just send a 400 when there are errors present, and a 400 error code would not be the case for errors thrown in the sanitizers (5xx instead)

Co-authored-by: Gustavo Henke <guhenke@gmail.com>
Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I think that an error will already cascade up to the route definition, so this LGTM as is.

@gustavohenke gustavohenke merged commit 41bd5d9 into express-validator:master Sep 27, 2020
@gustavohenke
Copy link
Member

Out in v6.7.0!

@fedeci fedeci mentioned this pull request Dec 27, 2020
@wwei-flux
Copy link

Thanks for the feature. Should the document at https://express-validator.github.io/docs/custom-validators-sanitizers.html#custom-sanitizers be updated?

@fedeci
Copy link
Member

fedeci commented Feb 23, 2021

Sure @wwei-flux, you can raise a PR if you want!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants