refactor(NODE-3405): implement MongoRuntimeError children #2913

andymina · 2021-07-20T03:03:42Z

Description

Replace instances of MongoDriverError with the appropriate children of MongoRuntimeError, listed below, as described in docs/errors.md.

Children of MongoRuntimeError used:

MongoTransactionError
MongoExpiredSessionError
MongoKerberosError
MongoResourceClosedError
MongoServerClosedError
MongoStreamClosedError
MongoTopologyClosedError

src/change_stream.ts

src/utils.ts

W-A-James · 2021-07-21T15:43:41Z

src/cmap/connection_pool.ts

        }
        if (!waitQueueMember[kCancelled]) {
-          waitQueueMember.callback(new MongoDriverError('connection pool closed'));
+          waitQueueMember.callback(new MongoResourceClosedError('Connection pool closed'));


Should we just go ahead and create a MongoConnectionPoolClosedError? It feels somewhat out of place having the more generic form of this error directly instantiated.

Sounds good! Leaving a TODO comment with a new ticket for this.

In general, I think it might be worth to make specific ClosedErrors for things that users will commonly run into (servers, streams, etc) and leaving MongoResourceClosedError as the general use case.

In that case, this might be worth discussing with the team to get a better idea of the more common cases

src/gridfs/upload.ts

W-A-James · 2021-07-21T15:50:47Z

src/sdam/server.ts

  [STATE_CLOSING]: [STATE_CLOSING, STATE_CLOSED]
 });

+const SERVER_CLOSED_ERROR = 'Server is closed';


In general, I think that when we have a resource that has been closed, the message should be of the form

'<resource type> has been closed'

That's a personal preference, but I think having it be more consistent would be better for error message readability. I'll flag other instances of this.

I'm a little hesitant to use the past tense in this error message because it might imply that the server has been closed as a result of this operation rather than conveying the current state of the server. Happy to make this change for now and see what everyone else thinks; maybe I'm being a little too semantic.

src/sdam/topology.ts

src/change_stream.ts

W-A-James · 2021-07-26T17:00:07Z

src/gridfs/upload.ts

  if (stream.state.aborted) {
    if (typeof callback === 'function') {
-      callback(new MongoDriverError('this stream has been aborted'));
+      callback(new MongoStreamClosedError('Stream has been aborted'));


In the same vein as this comment on NODE-3421 we may want to consider adding a default message for MongoStreamClosedError with an optional message parameter for cases where more information is needed

src/cmap/commands.ts

W-A-James · 2021-07-26T17:20:56Z

@andymina There is a TODO that points to this ticket in src/db.ts: 752. Feel free to point that to NODE-3484 instead

andymina · 2021-07-26T17:56:01Z

@andymina There is a TODO that points to this ticket in src/db.ts: 752. Feel free to point that to NODE-3484 instead

I removed the TODOs here because I realized this is the validateDatabaseName() mentioned in NODE-3483. I left a TODO pointing to NODE-3483 for this entire function.

dariakp

I didn't leave a comment on every line regarding transaction errors, but see my comment regarding reconsidering how we subclass these so that we can differentiate between incorrect usage from the user's side vs unexpected driver state

dariakp · 2021-07-26T20:06:23Z

src/change_stream.ts

+  AnyError,
+  isResumableError,
+  MongoDriverError,
+  MongoStreamClosedError


As a general question to the team, do we want to conflate generic streams with change streams in our error reporting? Our change streams aren't really node streams in the sense that they don't extend from any of the base node streams. I could go either way, but my thought is that there could be some merit in differentiating "steam closed" vs "change stream closed" @nbbeeken @emadum Thoughts?

Seems worthy of differentiating, I would imagine we'd only need use of MongoChangeStreamError, but there may be instances of Streams related errors, unless they stop something from continuing in our code I would say node stream errors should be bubbled up as is.

The initial goal we had was to use MongoStreamClosedError between GridFSStream and ChangeStream to prevent creating two similar errors. If it's worth differentiating we could just split them down into GridFSStreamClosed and ChangeStreamClosed.

src/operations/execute_operation.ts

dariakp · 2021-07-26T20:13:29Z

src/sessions.ts

  startTransaction(options?: TransactionOptions): void {
    if (this[kSnapshotEnabled]) {
-      throw new MongoDriverError('Transactions are not allowed with snapshot sessions');
+      throw new MongoTransactionError('Transactions are not allowed with snapshot sessions');


this is more of an API error - probably the compatibility one, because in order to get here, the user needs to intentionally start a snapshot session and then try to start a transaction on it

src/sessions.ts

W-A-James · 2021-07-28T19:45:17Z

src/sessions.ts

    assertAlive(this);
    if (this.inTransaction()) {
-      throw new MongoDriverError('Transaction already in progress');
+      throw new MongoAPIError('Transaction already in progress');


Since we are moving MongoTransactionError to be child of MongoAPIError, should we change this to MongoTransactionError and just go ahead and move MongoTransactionError to be a child of MongoAPIError in this PR?

If not, we should also mark this for replacement in NODE-3485

nbbeeken · 2021-08-10T21:37:16Z

src/gridfs/upload.ts

 import type { GridFSBucket } from './index';
+import type { GridFSFile } from './download';
+import type { WriteConcernOptions } from '../write_concern';
+import type { Collection } from '../collection';


revert the import sorting here

nbbeeken · 2021-08-10T21:39:25Z

src/sdam/topology.ts

    drainWaitQueue(
      topology[kWaitQueue],
-      new MongoDriverError('Topology is closed, please connect')
+      new MongoTopologyClosedError(`${TOPOLOGY_CLOSED_ERROR}, please connect`)


I think we can leave this error to be without message like the usage above

nbbeeken · 2021-08-10T21:44:27Z

src/error.ts

 */
 export class MongoServerClosedError extends MongoAPIError {
-  constructor(message: string) {
+  constructor(message = 'Server has been closed') {


Are we all in agreement on tense? super bikeshedding but present/imperative seems more ideal

@dariakp @emadum @W-A-James? Personally, I'm in favor of present tense but happy to defer.

My vote would also be for using present/imperative.

src/sessions.ts

src/utils.ts

src/change_stream.ts

dariakp · 2021-08-11T14:54:46Z

src/cmap/connection_pool.ts

        }
        if (!waitQueueMember[kCancelled]) {
-          waitQueueMember.callback(new MongoDriverError('connection pool closed'));
+          waitQueueMember.callback(new MongoAPIError('Connection pool closed'));


I am not convinced this is an API method, the entire class is marked internal, so this is unlikely to be due to a user error

src/sessions.ts

emadum

LGTM 👍

emadum · 2021-08-11T15:08:51Z

src/error.ts

 */
 export class MongoServerClosedError extends MongoAPIError {
-  constructor(message: string) {
+  constructor(message = 'Server has been closed') {


My vote would also be for using present/imperative.

emadum · 2021-08-11T15:09:34Z

src/operations/common_functions.ts

  // Did the user destroy the topology
  if (getTopology(db).isDestroyed())
-    return callback(new MongoDriverError('topology was destroyed'));
+    return callback(new MongoTopologyClosedError('Topology was destroyed'));


Suggested change

return callback(new MongoTopologyClosedError('Topology was destroyed'));

return callback(new MongoTopologyClosedError('Topology is destroyed'));

If we're going to update messages to present tense, can do this one as well.

We can also rely on the default string here

andymina · 2021-08-11T17:49:14Z

@dariakp re: this comment You're totally right. I'm going to leave a TODO pointing toward NODE-3483 which introduces MongoConnectionPoolClosedError as a child of RuntimeError. Also going to revert this to MongoRuntimeError for the time being.

src/change_stream.ts

…goAPIError

andymina self-assigned this Jul 20, 2021

andymina added the wip label Jul 20, 2021

andymina changed the title ~~refactor(NODE-3405): replace MongoDriverError where appropriate~~ refactor(NODE-3405): implement MongoRuntime children Jul 20, 2021

andymina requested a review from W-A-James July 20, 2021 17:47

andymina removed the wip label Jul 20, 2021

andymina marked this pull request as ready for review July 20, 2021 17:47

W-A-James added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 21, 2021

W-A-James suggested changes Jul 21, 2021

View reviewed changes

andymina mentioned this pull request Jul 21, 2021

refactor(NODE-3404): implement MongoRuntimeError children #2912

Merged

andymina changed the title ~~refactor(NODE-3405): implement MongoRuntime children~~ refactor(NODE-3405): implement MongoRuntimeError children Jul 22, 2021

andymina requested a review from W-A-James July 22, 2021 18:07

andymina force-pushed the NODE-3405/implement-MongoRuntimeError-children branch from 7937f49 to 7f7575d Compare July 26, 2021 15:14

W-A-James reviewed Jul 26, 2021

View reviewed changes

W-A-James requested review from W-A-James and removed request for W-A-James July 26, 2021 18:31

andymina requested a review from W-A-James July 26, 2021 19:40

W-A-James approved these changes Jul 26, 2021

View reviewed changes

W-A-James 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 26, 2021

W-A-James requested review from dariakp and nbbeeken July 26, 2021 19:46

dariakp requested changes Jul 26, 2021

View reviewed changes

W-A-James suggested changes Jul 28, 2021

View reviewed changes

andymina added wip and removed Team Review Needs review from team labels Jul 29, 2021

andymina force-pushed the NODE-3405/implement-MongoRuntimeError-children branch 3 times, most recently from 9631286 to 0aa6910 Compare August 10, 2021 14:15

andymina added 5 commits August 10, 2021 16:53

refactor(NODE-3405): add default msg to error classes

673aa1b

refactor(NODE-3405): use MongoAPIError

5284124

refactor(NODE-3405): use maybePromise in GridFS abort

a638d0e

refactor(NODE-3405): use maybePromise in GridFS abort

dda537a

style(NODE-3405): add TODO comment to replace error

3264b92

andymina force-pushed the NODE-3405/implement-MongoRuntimeError-children branch from 4722cc8 to 3264b92 Compare August 10, 2021 20:58

andymina requested a review from dariakp August 10, 2021 21:17

nbbeeken reviewed Aug 10, 2021

View reviewed changes

nbbeeken suggested changes Aug 10, 2021

View reviewed changes

refactor(NODE-3405): add default message for MongoExpiredSessionError

3f0f768

andymina requested a review from nbbeeken August 10, 2021 22:11

nbbeeken requested a review from emadum August 11, 2021 14:46

dariakp requested changes Aug 11, 2021

View reviewed changes

emadum approved these changes Aug 11, 2021

View reviewed changes

refactor(NODE-3405): revert use MongoAPIError and tidy up errmsgs

79c4da1

andymina requested a review from dariakp August 11, 2021 17:52

andymina added 2 commits August 11, 2021 22:04

test(NODE-3405): fix test waiting for the wrong error

7c56f63

test(NODE-3405): fix Topology test waiting for an outdated error

47563f2

nbbeeken approved these changes Aug 12, 2021

View reviewed changes

dariakp requested changes Aug 12, 2021

View reviewed changes

src/change_stream.ts Outdated Show resolved Hide resolved

style(NODE-3405): fix spelling in TODO comments and update use of Mon…

719b3bf

…goAPIError

andymina requested a review from dariakp August 12, 2021 20:53

dariakp approved these changes Aug 12, 2021

View reviewed changes

style(NODE-3405): unify default param format in error constructors

5332a67

W-A-James approved these changes Aug 12, 2021

View reviewed changes

W-A-James merged commit fb0b27d into 4.1 Aug 12, 2021

W-A-James deleted the NODE-3405/implement-MongoRuntimeError-children branch August 12, 2021 21:47

ljhaywar pushed a commit that referenced this pull request Nov 9, 2021

refactor(NODE-3405): implement MongoRuntimeError children (#2913)

735adaa

	return callback(new MongoTopologyClosedError('Topology was destroyed'));
	return callback(new MongoTopologyClosedError('Topology is destroyed'));

refactor(NODE-3405): implement MongoRuntimeError children #2913

refactor(NODE-3405): implement MongoRuntimeError children #2913

Uh oh!

Conversation

andymina commented Jul 20, 2021

Description

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andymina Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andymina Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

W-A-James commented Jul 26, 2021

Uh oh!

andymina commented Jul 26, 2021

Uh oh!

dariakp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emadum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andymina commented Aug 11, 2021

Uh oh!

Uh oh!

andymina Jul 21, 2021 •

edited

Loading

andymina Jul 21, 2021 •

edited

Loading