fix(shell-api): handle errors in cursor.map/.forEach MONGOSH-703 #816
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Handle exceptions from callbacks passed to `cursor.map()` and `cursor.forEach()`. Since `cursor.forEach()` returns a Promise, use that to propagate the error to the user. For `cursor.map()`, this is a bit trickier, since the method itself does not return any results to the user. Therefore, we add checks before and after accessing cursor methods that read data which forward the exception if one has occurred. Both of these feel like situations that the driver should take care of explicitly, so NODE tickets have been opened for both cases. (One alternative would have been to add async context/Node.js domain propagation to these methods. However, I decided against that, because that solution would be a) Node.js-specific, and b) because it would still lead to undesirable results, like the Promise returned by `.forEach()` never resolving.)
mcasimir
approved these changes
Apr 28, 2021
| // Ideally, this kind of code could be lifted into the driver (NODE-3231 and | ||
| // NODE-3232 are the tickets for that). | ||
| async _withCheckMapError<Ret>(fn: () => Ret): Promise<Ret> { | ||
| if (this._mapError) { |
There was a problem hiding this comment.
Nice trick! is also pretty clear, i thought it would have been more hacky
There was a problem hiding this comment.
Yeah … would still be nice if the driver had something here :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Includes #809 for now, until that is merged.)
Handle exceptions from callbacks passed to
cursor.map()andcursor.forEach().Since
cursor.forEach()returns a Promise, use that to propagatethe error to the user.
For
cursor.map(), this is a bit trickier, since the method itselfdoes not return any results to the user. Therefore, we add checks
before and after accessing cursor methods that read data which forward
the exception if one has occurred.
Both of these feel like situations that the driver should take care of
explicitly, so NODE tickets have been opened for both cases.
(One alternative would have been to add async context/Node.js domain
propagation to these methods. However, I decided against that, because
that solution would be a) Node.js-specific, and b) because it would
still lead to undesirable results, like the Promise returned by
.forEach()never resolving.)