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

andymina · 2021-07-16T01:28:55Z

Description

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

Children of MongoRuntimeError used

MongoURIError
MongoStreamError
MongoChangeStreamError
MongoGridFSStreamError
MongoGridFSChunkError
MongoCompressionError
MongoDecompressionError

src/connection_string.ts

test/functional/gridfs_stream.test.js

W-A-James · 2021-07-20T21:33:21Z

Apart from requested changes, LGTM

src/change_stream.ts

test/functional/gridfs_stream.test.js

src/change_stream.ts

src/connection_string.ts

src/db.ts

src/operations/execute_operation.ts

src/utils.ts

test/functional/gridfs_stream.test.js

test/functional/unified-spec-runner/entities.ts

test/functional/unified-spec-runner/operations.ts

test/manual/ldap.test.js

nbbeeken · 2021-07-26T20:49:48Z

src/cmap/wire_protocol/compression.ts

  if (compressorID < 0 || compressorID > Math.max(2)) {
-    throw new MongoDriverError(
+    throw new MongoDecompressionError(
      `Server sent message compressed using an unsupported compressor.` +


Lets make this one template string?

nbbeeken · 2021-07-26T21:00:37Z

src/cmap/message_stream.ts

      callback(
-        new MongoDriverError(
+        new MongoDecompressionError(
          'Decompressing a compressed message from the server failed. The message is corrupt.'


Seems like we could improve the message a bit here, a MongoDecompressError implies the first sentence maybe its worth being specific, we can workshop but something like: 'Message header size does not match message body size'

How does 'Message body and message header must be the same length' sound?

nbbeeken · 2021-07-26T21:02:35Z

src/gridfs/download.ts

-        expectedLength;
-      return __handleError(stream, new MongoDriverError(errmsg));
+      errmsg = `ChunkIsWrongSize: Got unexpected length: ${buf.byteLength}, expected: ${expectedLength}`;
+      return __handleError(stream, new MongoGridFSChunkError(errmsg));


__handleError is a oneliner (maybe it contained more at some point) can we just inline it everywhere?

return stream.emit( GridFSBucketReadStream.ERROR, new MongoGridFSChunkError( `ChunkIsWrongSize: Got unexpected length: ${buf.byteLength}, expected: ${expectedLength}` ) );

nbbeeken · 2021-07-26T21:05:17Z

src/gridfs/upload.ts

  abort(callback?: Callback<void>): Promise<void> | void {
    const Promise = PromiseProvider.get();
-    let error: MongoDriverError;
+    let error: MongoGridFSStreamError;


Can we refactor this to use maybePromise?

In hindsight, I think this whole thing is out of scope for this ticket. I think MongoStreamClosedError, covered in NODE-3405, better covers the errors thrown here. I'll leave a TODO comment for that PR and revert this change.

src/operations/execute_operation.ts

src/sdam/server.ts

nbbeeken · 2021-07-26T21:19:23Z

src/gridfs/download.ts

+  MongoGridFSChunkError,
+  MongoGridFSStreamError
+} from '../error';
 import type { FindOptions } from '../operations/find';


There's import changes here that aren't needed, I think you import sorting turned on in vscode? This is something we want to do as a lint fix down the line if you don't mind reverting these.

nbbeeken · 2021-07-26T21:19:50Z

src/gridfs/download.ts

@@ -1,14 +1,20 @@
+import type { ObjectId } from 'bson';


This should be '../bson' must've slipped through I'll look into linting for this.

src/change_stream.ts

nbbeeken · 2021-08-06T17:46:10Z

src/connection_string.ts

      for (const compVal of values as string[]) {
        for (const c of compVal.split(',')) {
-          if (['none', 'snappy', 'zlib'].includes(String(c))) {
+          if (Object.values(VALID_COMPRESSION_MECHS).includes(String(c))) {


Suggested change

if (Object.values(VALID_COMPRESSION_MECHS).includes(String(c))) {

if (Object.keys(Compressor).includes(String(c))) {

We already have an enum Compressor lets use that here

Oops, didn't realize there was an existing enum already 😅

nbbeeken · 2021-08-06T17:46:52Z

src/connection_string.ts

          } else {
            throw new MongoInvalidArgumentError(
-              `${c} is not a valid compression mechanism. Must be 'none', 'snappy', or 'zlib'. `
+              `${c} is not a valid compression mechanism. Must be one of: ${enumToString(


I guess that would just make this ${Object.keys(Compressor)}

src/change_stream.ts

src/cmap/connect.ts

test/functional/unified-spec-runner/operations.ts

test/manual/ldap.test.js

src/connection_string.ts

src/db.ts

dariakp

Other than this small but important detail, LGTM

dariakp · 2021-08-10T18:06:51Z

test/functional/change_stream.test.js

            changeStream.close(close);
          } else if (counter >= 3) {
-            close(new Error('should not have received more than 2 events'));
+            expect.fail('Should not have received more than 2 events');


The original close function here served an important purpose: to prevent done from being called more than once; an expect.fail would throw an error, which would call done in whatever context is currently running, potentially causing unrelated tests to fail. tl;dr: passing the error explicitly to a wrapped done function (the close function in this context), the way it was written originally, is the safest way to handle event listener errors in tests

Ahh, I didn't realize that. This was helpful in understanding the tests a bit better. Thank you!

nbbeeken

once the fix daria mentions is in then this LGTM

dariakp

LGTM 🎉

andymina added the wip label Jul 16, 2021

andymina force-pushed the NODE-3404/Implement-MongoRuntimeError-children branch 2 times, most recently from 1bfa5fc to 990c92c Compare July 19, 2021 17:30

andymina self-assigned this Jul 19, 2021

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

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

andymina removed the wip label Jul 20, 2021

W-A-James suggested changes Jul 20, 2021

View reviewed changes

src/connection_string.ts Outdated Show resolved Hide resolved

test/functional/gridfs_stream.test.js Outdated Show resolved Hide resolved

W-A-James added the Team Review Needs review from team label Jul 20, 2021

W-A-James requested review from dariakp, emadum and nbbeeken July 20, 2021 21:33

W-A-James reviewed Jul 21, 2021

View reviewed changes

src/change_stream.ts Outdated Show resolved Hide resolved

W-A-James reviewed Jul 21, 2021

View reviewed changes

src/change_stream.ts Outdated Show resolved Hide resolved

W-A-James added Primary Review In Review with primary reviewer, not yet ready for team's eyes and removed Team Review Needs review from team labels Jul 21, 2021

andymina requested a review from W-A-James July 21, 2021 18:38

andymina force-pushed the NODE-3404/Implement-MongoRuntimeError-children branch from bc6fe74 to 0e77b80 Compare July 26, 2021 19:38

W-A-James suggested changes Jul 26, 2021

View reviewed changes

src/change_stream.ts Outdated Show resolved Hide resolved

test/functional/gridfs_stream.test.js Show resolved Hide resolved

src/change_stream.ts Outdated Show resolved Hide resolved

W-A-James removed the request for review from emadum July 26, 2021 20:04

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

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

dariakp requested changes Jul 26, 2021

View reviewed changes

nbbeeken suggested changes Jul 26, 2021

View reviewed changes

andymina requested review from dariakp and nbbeeken July 27, 2021 20:46

W-A-James suggested changes Jul 28, 2021

View reviewed changes

src/change_stream.ts Show resolved Hide resolved

andymina requested a review from nbbeeken August 6, 2021 15:00

nbbeeken suggested changes Aug 6, 2021

View reviewed changes

andymina requested a review from nbbeeken August 6, 2021 19:09

dariakp requested changes Aug 9, 2021

View reviewed changes

dariakp requested changes Aug 10, 2021

View reviewed changes

nbbeeken approved these changes Aug 10, 2021

View reviewed changes

dariakp approved these changes Aug 10, 2021

View reviewed changes

andymina added 17 commits August 10, 2021 14:57

refactor(NODE-3404): implement MongoRuntimeError children

319fcd0

refactor(NODE-3404): pruned more instances of MongoDriverError

2629a60

refactor(NODE-3404): modifying errors in tests

143f44a

test(NODE-3404): correct gridfs tests to wait for the right errors

409afa2

refactor(NODE-3404): exported errors from src/

eeea005

refactor(NODE-3404): export MongoRuntimeError from src/

3a13cb6

test(NODE-3404): revert use of MongoChangeStreamError

8a17031

refactor(NODE-3404): revert select usage of MongoGridFSStreamError

9cc26c8

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

68d440e

refactor(NODE-3404): use MongoRuntime children where appropriate

04207b8

test(NODE-3404): remove errors throws and replace with expect.fail

6dab25a

style(NODE-3404): remove unused import

34ec480

refactor(NODE-3404): use existing compressor enum

7ddb4aa

refactor(NODE-3404): remove MongoURIError

3b5c9e1

refactor(NODE-3405): revert use of MongoRuntimeError

e175181

style(NODE-3404): remove ununsed import

1b0ae05

test(NODE-3404): revert expect.fail use in ChangeStream test

5659654

andymina force-pushed the NODE-3404/Implement-MongoRuntimeError-children branch from d2f57e8 to 5659654 Compare August 10, 2021 18:57

andymina added 2 commits August 10, 2021 15:44

fix(NODE-3404): export missing errors from src/

57420ce

refactor(NODE-3404): remove unused MongoCompressionError

84e92ca

W-A-James merged commit e69d992 into 4.1 Aug 10, 2021

W-A-James deleted the NODE-3404/Implement-MongoRuntimeError-children branch August 10, 2021 20:27

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

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

5c287fa

	if (Object.values(VALID_COMPRESSION_MECHS).includes(String(c))) {
	if (Object.keys(Compressor).includes(String(c))) {

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

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

Uh oh!

Conversation

andymina commented Jul 16, 2021

Description

Uh oh!

Uh oh!

Uh oh!

W-A-James commented Jul 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

andymina Jul 27, 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!

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!

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!

Uh oh!

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!

nbbeeken left a comment

Choose a reason for hiding this comment

Uh oh!

dariakp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andymina Jul 27, 2021 •

edited

Loading