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

refactor(NODE-3402): implement MongoAPIError and its children #2891

Merged
merged 56 commits into from Jul 23, 2021

Conversation

W-A-James
Copy link
Contributor

@W-A-James W-A-James commented Jul 7, 2021

Description

Implement the errors detailed in docs/errors.md from NODE-3363 that fall under MongoAPIError

What changed?
Replacing MongoDriverError instances with the following errors where appropriate

  • MongoInvalidArgumentError
  • MongoMissingDependencyError
  • MongoMissingCredentialsError
  • MongoCompatibilityError

@W-A-James W-A-James changed the title chore: Replace relevant instances of MongoDriverError with MongoInvalidArgumentError refactor(NODE-3402): Implement MongoAPIError and its children Jul 7, 2021
@W-A-James W-A-James marked this pull request as ready for review July 7, 2021 20:56
@W-A-James W-A-James force-pushed the NODE-3402/4.0/Implement-MongoAPIError-and-children branch from c283fea to 63eb2be Compare July 7, 2021 21:27
@W-A-James W-A-James added the wip label Jul 7, 2021
@W-A-James
Copy link
Contributor Author

Currently going back through the driver to locate anywhere I may have missed that would benefit from a child instance of MongoAPIError in place of MongoDriverError

src/db.ts Outdated Show resolved Hide resolved
@W-A-James W-A-James requested a review from andymina July 8, 2021 15:49
@andymina andymina added Primary Review In Review with primary reviewer, not yet ready for team's eyes and removed wip labels Jul 8, 2021
@andymina
Copy link
Contributor

andymina commented Jul 8, 2021

src/bulk/common.ts:1150: I think it might be worth to make this MongoInvalidArgumentError.

@andymina
Copy link
Contributor

andymina commented Jul 8, 2021

src/cmap/auth/scram.ts:31: This should be MongoMissingCredentialsError.

@andymina
Copy link
Contributor

andymina commented Jul 8, 2021

src/cmap/commands.ts:85: I'm not quite sure how I feel about this being MongoInvalidArgumentError. I feel like MongoURIError might be a little more fitting here since the string to be parsed has an invalid char. Happy to defer to the rest of team.

@andymina
Copy link
Contributor

andymina commented Jul 8, 2021

src/cmap/commands.ts:855: Although realistically we shouldn't run into this error, I think it'd be good practice to replace this with MongoCompatibilityError since it was decided that no drivers would make use of payload type 1.

Update: Originally, I thought MongoCompatibilityError would fit here, but because of Eric's reasoning I think MongoDeprecationError is better here.

@andymina
Copy link
Contributor

andymina commented Jul 8, 2021

src/cmap/connection.ts:661: This should be MongoCompatabilityError.

@W-A-James
Copy link
Contributor Author

src/cmap/commands.ts:85: I'm not quite sure how I feel about this being MongoInvalidArgumentError. I feel like MongoURIError might be a little more fitting here since the string to be parsed has an invalid char. Happy to defer to the rest of them.

@andymina
I think it might be worth keeping MongoParseError since in instances like this, we're not doing anything with the URI string itself, but rather just parsing a namespace string (<database>.<collection>)

@andymina
Copy link
Contributor

andymina commented Jul 8, 2021

src/cmap/commands.ts:85: I'm not quite sure how I feel about this being MongoInvalidArgumentError. I feel like MongoURIError might be a little more fitting here since the string to be parsed has an invalid char. Happy to defer to the rest of team.

@andymina
I think it might be worth keeping MongoParseError since in instances like this, we're not doing anything with the URI string itself, but rather just parsing a namespace string (<database>.<collection>)

@W-A-James
From the description of MongoURIError that we've got in docs/errors.md, I think this use case should be covered by it. Although if you think it's worth to split the two and reintroduce MongoParseError, I'm down.

@andymina
Copy link
Contributor

andymina commented Jul 8, 2021

src/cmap/connection.ts:651: This should MongoMissingDependencyError per NODE-3363.

@andymina
Copy link
Contributor

andymina commented Jul 8, 2021

src/encrypter.ts:29: Should this also be MongoInvalidArgumentError?

Copy link
Contributor

@andymina andymina left a comment

Choose a reason for hiding this comment

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

I think once the changes requested in the comments are resolved, LGTM.

src/change_stream.ts Outdated Show resolved Hide resolved
src/db.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
@W-A-James W-A-James changed the base branch from NODE-3399/Implement-New-Error-hierarchy to 4.0 July 8, 2021 19:59
@W-A-James
Copy link
Contributor Author

src/cmap/commands.ts:85: I'm not quite sure how I feel about this being MongoInvalidArgumentError. I feel like MongoURIError might be a little more fitting here since the string to be parsed has an invalid char. Happy to defer to the rest of team.

@andymina
I think it might be worth keeping MongoParseError since in instances like this, we're not doing anything with the URI string itself, but rather just parsing a namespace string (<database>.<collection>)

@W-A-James
From the description of MongoURIError that we've got in docs/errors.md, I think this use case should be covered by it. Although if you think it's worth to split the two and reintroduce MongoParseError, I'm down.

@andymina
From the description we have for MongoURIError, namespaces, outside the context of connecting to the server itself, seem like they come up enough to justify having MongoParseError continue to be a separate error. There are other cases (src/cmap/message_stream.ts: 138, 144 & src/sdam/topology.ts: 279) where MongoParseError fits better than MongoURIError in my opinion, so my vote is to keep MongoParseError and add it to our hierarchy alongside MongoURIError

@andymina
Copy link
Contributor

andymina commented Jul 8, 2021

src/cmap/commands.ts:85: I'm not quite sure how I feel about this being MongoInvalidArgumentError. I feel like MongoURIError might be a little more fitting here since the string to be parsed has an invalid char. Happy to defer to the rest of team.

@andymina
I think it might be worth keeping MongoParseError since in instances like this, we're not doing anything with the URI string itself, but rather just parsing a namespace string (<database>.<collection>)

@W-A-James
From the description of MongoURIError that we've got in docs/errors.md, I think this use case should be covered by it. Although if you think it's worth to split the two and reintroduce MongoParseError, I'm down.

@andymina
From the description we have for MongoURIError, namespaces, outside the context of connecting to the server itself, seem like they come up enough to justify having MongoParseError continue to be a separate error. There are other cases (src/cmap/message_stream.ts: 138, 144 & src/sdam/topology.ts: 279) where MongoParseError fits better than MongoURIError in my opinion, so my vote is to keep MongoParseError and add it to our hierarchy alongside MongoURIError

@W-A-James SGTM, lets see what the rest of the team thinks because this would likely cause another PR to 4.0 to update the doc

@W-A-James W-A-James requested a review from andymina July 8, 2021 20:42
Copy link
Contributor

@andymina andymina left a comment

Choose a reason for hiding this comment

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

LGTM, nice job

@andymina andymina added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jul 8, 2021
@W-A-James W-A-James force-pushed the NODE-3402/4.0/Implement-MongoAPIError-and-children branch from eed35ba to 2a1515f Compare July 23, 2021 20:17
@W-A-James W-A-James requested a review from dariakp July 23, 2021 20:20
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

LGTM, keep up the great work!

@andymina andymina changed the title refactor(NODE-3402): Implement MongoAPIError and its children refactor(NODE-3402): implement MongoAPIError and its children Jul 23, 2021
@andymina andymina merged commit 967c193 into 4.0 Jul 23, 2021
@andymina andymina deleted the NODE-3402/4.0/Implement-MongoAPIError-and-children branch July 23, 2021 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
5 participants