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

Add test for validateSync to test ignore async functions but still run error hooks #12372

Closed
wants to merge 1 commit into from

Conversation

hasezoey
Copy link
Collaborator

@hasezoey hasezoey commented Sep 1, 2022

Summary

This PR adds tests that test validateSync to execute sync functions and error hooks but ignore async function() and ignores returned Promises

fixes #8703

Blocked currently because validateSync does not seem to execute any hooks

Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez left a comment

Choose a reason for hiding this comment

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

If async validators don't work, I think we should throw an error instead of silently ignoring them. Similar to how we handle transform option, see #9176.
What do you think @hasezoey

@hasezoey
Copy link
Collaborator Author

hasezoey commented Sep 1, 2022

If async validators don't work, I think we should throw an error instead of silently ignoring them. Similar to how we handle transform option, see #9176.

i dont know what the convention for this is here, though currently in #12371 doValidateSync silently ignores the async function (as stated in the documentation)

this PR here (#12372) focuses on hooks, which do not seem to be executed in validateSync currently

@AbdelrahmanHafez
Copy link
Collaborator

Interesting, I didn't know that.

So async validators would run with Document#validate(), but not with Document#validateSync(), nor implicitly with await user.save();. Is that correct? @hasezoey

@hasezoey
Copy link
Collaborator Author

hasezoey commented Sep 1, 2022

Interesting, I didn't know that.

So async validators would run with Document#validate(), but not with Document#validateSync(), nor implicitly with await user.save();. Is that correct? @hasezoey

(already answered in #12371 (comment) with similar question)

from what i know, in both validate and save async validators are run (and promises "awaited") (from what i can tell, save calls validate)

@AbdelrahmanHafez
Copy link
Collaborator

Thanks @hasezoey
I just realized they are two different PRs, thought there was an issue with GitHub and my comments didn't go through, apparently I just duplicated them. 😄

@hasezoey
Copy link
Collaborator Author

rebased on latest master to resolve conflicts

@vkarpov15
Copy link
Collaborator

@hasezoey is this PR still valid? It's still in draft state and has been for a while, so I wanted to check if we can close this PR.

@hasezoey
Copy link
Collaborator Author

@hasezoey is this PR still valid? It's still in draft state and has been for a while, so I wanted to check if we can close this PR.

i have been waiting for feedback about the pr and the referenced issue, simply said this PR adds a test for a issue, but the functionality is not quite what this test is meant to test and there has not been feedback about what to do (is this wanted behavior or should be changed)

if i remember correctly, the current issue is about that validateSync states that it does not call any async hooks but should still call (sync) error handling hooks, which it seemingly does not

i am not sure what the exact problem is / was because the logs have expired and i am currently not on my dev machine

@vkarpov15
Copy link
Collaborator

Ok, I'll take a closer look tomorrow

@hasezoey
Copy link
Collaborator Author

hasezoey commented Jul 13, 2023

rebased onto latest master to resolve conflicts

current issue is that both tests fail because none of the 3 validate hooks are called, and i dont know if this is expected or unexpected behavior (meaning if the code should be changed to run validate hooks, or the tests to be changed to test that none are executed)

@vkarpov15
Copy link
Collaborator

So I think the confusion is due to the fact that I mixed up async hooks and async validators when I wrote up #8703. Right now it is expected behavior that validateSync() skips all middleware, and I'm not convinced it is worthwhile to make validateSync() execute sync middleware.

Right now we only have one function that supports sync middleware, init(), and that code feels a bit hacky. There's also the concern about validateSync() firing pre('validate') and post('validate') hooks as opposed to pre('validateSync') and post('validateSync'), which may be a better API.

Thanks for the work you put in on this PR @hasezoey , and I'm very sorry it took me so long to take a more thorough look. However, I'm going to close this PR and #8703 because I'm not convinced that executing validation hooks on validateSync() is worth doing ATM.

@vkarpov15 vkarpov15 closed this Jun 4, 2024
@hasezoey hasezoey deleted the validateSyncNoHooks branch June 5, 2024 11:36
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.

How to run error handling middleware on validateSync()?
3 participants