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

Request-level bail #1214

Merged
merged 6 commits into from
Mar 11, 2023
Merged

Request-level bail #1214

merged 6 commits into from
Mar 11, 2023

Conversation

gustavohenke
Copy link
Member

@gustavohenke gustavohenke commented Feb 18, 2023

Description

Closes #1100

Implements an option to .bail() so that no further contexts will run if there are any errors.

Supports:

  • oneOf
  • checkSchema
  • checkExact

⚠️ This is a perf-loss for those using any of the above functions, as the chains now need to be awaited on, otherwise results will be unreliable.

Suppose that you have a request with 1k fields, one validator on each field, each taking 10ms to complete.
Also suppose that no fields present errors, so every validator runs.

On my machine, the time it takes for running all validation chains looks like this:

  • With request-level bail on every field: 10s
  • With request-level bail on half of all fields: 10s (no difference)
  • With request-level bail on a third of all fields: ~7s (roughly 1/3 better than the above scenarios)
  • Without request-level bail: ~100 ms.

To-do list

  • I have added tests for what I changed.
  • This pull request is ready to merge.

@coveralls
Copy link

coveralls commented Feb 18, 2023

Coverage Status

Coverage: 100.0%. Remained the same when pulling 38f61a9 on v7-request-bail into d3d1549 on v7-features.

@@ -62,12 +69,25 @@ export function oneOf(
const middleware = async (req: InternalRequest, _res: any, next: (err?: any) => void) => {
const surrogateContext = new ContextBuilder().addItem(dummyItem).build();

// Run each group of chains in parallel, and within each group, run each chain in parallel too.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a perf loss, but I don't see any other way of implementing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Latest version is better, but still a perf loss. I've added a summary of changes to the PR description.

@gustavohenke gustavohenke linked an issue Feb 18, 2023 that may be closed by this pull request
@gustavohenke gustavohenke marked this pull request as ready for review March 11, 2023 02:21
@gustavohenke gustavohenke added this to the v7.0.0 milestone Mar 11, 2023
@gustavohenke gustavohenke merged commit f50a1b4 into v7-features Mar 11, 2023
@gustavohenke gustavohenke deleted the v7-request-bail branch March 11, 2023 06:32
gustavohenke added a commit that referenced this pull request Mar 11, 2023
gustavohenke added a commit that referenced this pull request Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Middleware fail fast
2 participants