Skip to content

refactor(NODE-3402): Implement MongoAPIError and its children #2883

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

refactor(NODE-3402): Implement MongoAPIError and its children #2883

wants to merge 9 commits into from

Conversation

W-A-James
Copy link
Contributor

@W-A-James W-A-James commented Jul 6, 2021 •
edited
Loading

Description

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

What changed?
Replaced some MongoDriverError instances with the following errors where appropriate

  • MongoInvalidArgumentError
  • MongoMissingDependencyError
  • MongoMissingCredentialsError
  • MongoCompatibilityError

@W-A-James W-A-James requested a review from andymina July 6, 2021 21:13
andymina
andymina reviewed Jul 7, 2021
View reviewed changes
@andymina
Copy link
Contributor

andymina commented Jul 7, 2021

I can't leave comments on things you didn't change so unfortunately this will have to do for now ;(

src/bulk/ordered.ts:29: I think this error should also be changed to MongoInvalidArgumentError since the size of the doc the user supplied is too large.

@andymina
Copy link
Contributor

andymina commented Jul 7, 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 MongoCompatabilityError since it was decided that no drivers would make use of payload type 1.

@andymina
Copy link
Contributor

andymina commented Jul 7, 2021 •
edited
Loading

src/cursor/abstract_cursor.ts:549 and 568: I think these should also be MongoInvalidArgumentError.

@andymina
Copy link
Contributor

andymina commented Jul 7, 2021 •
edited
Loading

src/operations/execute_operations.ts:130 and 191: I think this should be MongoInvalidArgumentError.

andymina
andymina suggested changes Jul 7, 2021
View reviewed changes
@andymina
Copy link
Contributor

andymina commented Jul 7, 2021

src/operations/update.ts:278 and 282: I also think these should be MongoInvalidArgumentError.

@W-A-James W-A-James requested a review from andymina July 7, 2021 19:19
@andymina
Copy link
Contributor

andymina commented Jul 7, 2021

src/mongo_client.ts:559: This should be MongoInvalidArgumentError I think.

@andymina
Copy link
Contributor

andymina commented Jul 7, 2021

src/sessions.ts:272: I think MongoCompatabilityError might fit better here.

andymina
andymina suggested changes Jul 7, 2021
View reviewed changes
src/change_stream.ts
@@ -355,7 +361,7 @@ export class ChangeStream<TSchema extends Document> extends TypedEventEmitter<Ch
*/
stream(options?: CursorStreamOptions): Readable {
this.streamOptions = options;
if (!this.cursor) throw new MongoDriverError(NO_CURSOR_ERROR);
if (!this.cursor) throw new MongoInvalidArgumentError(NO_CURSOR_ERROR);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this should be under MongoInvalidArgumentError since the user isn't providing the cursor in this function, nor in the constructor. I feel like it might fall under MongoChangeStreamError which is covered in NODE-3404.

src/error.ts Outdated
export class MongoAPIError extends MongoDriverError {
constructor(message: string) {
super(message);
this.name = 'MongoAPIError';
Copy link
Contributor

Choose a reason for hiding this comment

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

NODE-3368 just got merged which made the name prop of error classes getter functions. Moving forward names should implemented as:

get name(): string {
  return 'MongoAPIError';
}

Can you add these changes so we can stay consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

src/utils.ts
@@ -248,7 +254,7 @@ export function executeLegacyOperation(
const optionsIndex = args.length - 2;
args[optionsIndex] = Object.assign({}, args[optionsIndex], { session: session });
} else if (opOptions.session && opOptions.session.hasEnded) {
throw new MongoDriverError('Use of expired sessions is not permitted');
throw new MongoInvalidArgumentError('Use of expired sessions is not permitted');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little torn about this one. The same error message appears in src/operations/execute_operation.ts:88 as well so I feel like the verdict has to be consistent for the two cases. That being said I'm not sure whether MongoInvalidArgumentError or MongoResourceClosedError would fit better here. Definitely open to other perspectives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had left the error in src/operations/execute_operation.ts:88 because of that ambiguity. Was intending to leave that until we added the MongoResourceClosedError and its children, so yeah, I think you're right about changing this to MongoResourceClosedError as well

@W-A-James W-A-James marked this pull request as ready for review July 7, 2021 20:12
@W-A-James W-A-James force-pushed the NODE-3402/4.0/Implement-MongoAPIError-and-children branch from 98fbf25 to 34389d1 Compare July 7, 2021 20:17
@W-A-James
Copy link
Contributor Author

Closing for replacement with #2891

@W-A-James W-A-James closed this Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andymina andymina andymina requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@W-A-James @andymina