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

fix: reject promise if deserializeMessage throws #413

Merged
merged 3 commits into from
May 19, 2024

Conversation

williamhorning
Copy link
Contributor

if deserializeMessage throws for any reason (such as a BSONError), the pendingMessage promise will never resolve. this PR changes this behavior so that the promise will reject if there's an error thrown

@lucsoft
Copy link
Collaborator

lucsoft commented May 16, 2024

Is this testable? Would be nice to have one bad path test where it gets rejected 😅

Thanks anyway and if you don't find the time i can merge it

@williamhorning
Copy link
Contributor Author

Is this testable? Would be nice to have one bad path test where it gets rejected 😅

Thanks anyway and if you don't find the time i can merge it

I could try to take a look at this later today but in theory it shouldn't be too difficult (get some bad BSON data to cause an issue)

@williamhorning
Copy link
Contributor Author

@lucsoft it doesn't seem like there's an easy way to force MongoDB to return invalid bson so unless you know of a way to do that, I don't really know how I'd write a test for this

@lucsoft
Copy link
Collaborator

lucsoft commented May 18, 2024

@williamhorning well the only way would be to edit the raw bson like flipping a bit or something

@williamhorning
Copy link
Contributor Author

@lucsoft i don't really see a way of reliably doing that with the current CI setup. additionally, most people will probably never run into bson errors unless they're doing something weird or if there's a bug with mongodb

@lucsoft
Copy link
Collaborator

lucsoft commented May 18, 2024

@williamhorning okay then I just merge it

@lucsoft
Copy link
Collaborator

lucsoft commented May 18, 2024

Just fix CI and then I merge it

@williamhorning
Copy link
Contributor Author

@lucsoft i fixed CI

@lucsoft lucsoft merged commit 1ade65f into denodrivers:main May 19, 2024
5 checks passed
@williamhorning williamhorning deleted the reject-on-bson-error branch May 19, 2024 14:49
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.

2 participants