refactor(NODE-3337): unwrap WriteConcernError #2932

andymina · 2021-08-06T19:06:09Z

Description

Unwrapped the nested errors seen in handleMongoWriteConcernError to be clearer and more readable.

dariakp · 2021-08-06T19:27:21Z

@andymina we should be careful about removing the server error behavior of assigning all input object props to the error object; the function of the server error class is to wrap errors returned from the server, and I believe the original design was based around the idea that we might not always know what those properties are, so we just want to pass them on to the user so that they have that information from the server; removing this behavior is a potentially breaking change. The goal of the ticket was just to remove the explicit rewrapping of the write concern error, not necessarily change the actual implementation of the MongoServerError class.

andymina · 2021-08-06T20:38:58Z

@andymina we should be careful about removing the server error behavior of assigning all input object props to the error object; the function of the server error class is to wrap errors returned from the server, and I believe the original design was based around the idea that we might not always know what those properties are, so we just want to pass them on to the user so that they have that information from the server; removing this behavior is a potentially breaking change. The goal of the ticket was just to remove the explicit rewrapping of the write concern error, not necessarily change the actual implementation of the MongoServerError class.

@dariakp Ahh, didn't realize that there were potentially more/new props coming in from the server. Initially I figured that the shape of MongoServerErrors that the driver needs would remain the same so I just explicitly retrieved them rather than relying on the loop. So sorry for any confusion! Will revert this change, ty :)

dariakp · 2021-08-09T14:06:58Z

src/error.ts

+    this.ok = message.ok;
+    this.topologyVersion = message.topologyVersion;
+
+    const currentProps = Object.getOwnPropertyNames(this);


Is all this really necessary to simplify the multiple constructor calls in bulk/common? It looks like all this filtering is doing the same thing as the original for..in, but it's less efficient because of the multiple array function calls. Ideally, the server error class should not need to be touched at all to complete this ticket, and we definitely should not be adding new functionality to it unless absolutely necessary (referring to the kErrorLabels line above), so any logic that's specific to particular errors, and not all server errors, should live in those specific classes.

Neal and I talked things over and I removed the tons of array calls that were there. I opted to put prop names into a set for better efficiency and it'll likely be easier to read/maintain than loads of if conditions.

nbbeeken · 2021-08-09T17:08:15Z

src/bulk/common.ts

 */
 export class WriteConcernError {
-  err: MongoServerError;
+  err: { code: number; errmsg: string; errInfo?: Document };


We technically have a breakage here, this is no longer instanceof MongoServerError but this was just relying on the key mapping logic of the MongoServerError constructor.
I think its fair to make this change, all the properties are still accessible. Of course, up for discussion:

I mean, technically, based on the spec, there shouldn't be an err property here at all, so we could consider saving err as a symbol (and perhaps replacing err with a deprecated getter); any thoughts on this? This class never extended Error, so losing the stack trace from a property that should never have even been exposed wouldn't really be an issue

You want to map these props on to this class? I think that sounds good removes a layer of indirection (and we keep a getter to prevent breakage)

something along the lines of defining a kErr symbol (maybe more descriptive) set equal to the { code, errmsg, errInfo } and then a public getter marked deprecated: get err() { return this[kErr] } so that we hopefully don't break any existing functionality in case someone is trying to read directly from the current err prop

src/bulk/common.ts

nbbeeken · 2021-08-09T17:13:20Z

src/bulk/common.ts

  if (result.writeConcernError) {
-    bulkResult.writeConcernErrors.push(new WriteConcernError(result.writeConcernError));
+    const { code, errmsg, errInfo } = result.writeConcernError;
+    bulkResult.writeConcernErrors.push(new WriteConcernError({ code, errmsg, errInfo }));


I think we have to change this back to result.writeConcernError, we might be losing properties in this destructuring.
We can make the constructor accept a more flexible type, it's still good that we have a better typed WriteConcernError class, but incase there are more properties in certain server versions we should have them automatically pass through here.

nbbeeken · 2021-08-09T17:13:44Z

src/bulk/common.ts

 ) {
  mergeBatchResults(batch, bulkResult, undefined, err.result);

  // TODO: Remove multiple levels of wrapping (NODE-3337)


We can delete the TODO

nbbeeken · 2021-08-09T17:16:56Z

src/bulk/common.ts

-    super(error as Error);
-    Object.assign(this, error);
+  constructor(
+    x: { message: string; code: number; writeErrors?: WriteError[] } | WriteConcernError | AnyError,


We can name this something a little more descriptive than x now.

nbbeeken · 2021-08-09T17:20:36Z

src/bulk/common.ts

+    result: BulkWriteResult
+  ) {
+    super(x);
+    if (x instanceof Error) Object.assign(this, x);


I believe we put the assign behind the gate of instanceof Error since that was filtering for MongoServerErrors while testing.
We should move the assign to be the last call of the constructor ungated. (Or remove it all together and see if tests fail, but I defer to the team on seeing if that's acceptable).

nbbeeken · 2021-08-09T17:23:30Z

src/operations/insert.ts

      if (err || res == null) return callback(err);
      if (res.code) return callback(new MongoServerError(res));
-      if (res.writeErrors) return callback(new MongoServerError(res.writeErrors[0]));
+      if (res.writeErrors) return callback(new MongoWriteConcernError(res.writeErrors[0]));


Is this always correct? I don't think a writeError is always in the category of a WriteConcernError, but I could be wrong, lets sync to make sure this is correct

Capturing what spoke about in person: This has been reverted to MongoServerError since not every write error is a write concern error. The ideal solution would be to make this a WriteError class, but that would call for some error hierarchy refactoring since WriteError isn't a child of anything and that would be out of scope.

src/error.ts

andymina · 2021-08-10T21:56:16Z

Neal requested reviews from my laptop lol

dariakp · 2021-08-12T18:47:16Z

src/bulk/common.ts

 */
 export class WriteConcernError {
-  err: MongoServerError;
+  err: { code: number; errmsg: string; errInfo?: Document };


I mean, technically, based on the spec, there shouldn't be an err property here at all, so we could consider saving err as a symbol (and perhaps replacing err with a deprecated getter); any thoughts on this? This class never extended Error, so losing the stack trace from a property that should never have even been exposed wouldn't really be an issue

src/error.ts

…ServerError

src/bulk/common.ts

emadum

One small request, and there is a lint error to fix, otherwise LGTM though! 👍

src/bulk/common.ts

dariakp · 2021-08-16T18:12:10Z

src/bulk/common.ts

+  }
+
+  /**
+   * @internal


does it make sense to mark this internal if it's explicitly intended as a backcompat option for users?

I suppose my thinking was it avoids breaking runtime code but intentionally makes it inaccessible to TS but I realize that could be nearly equal in breakage.

lets go with making this interface public and not introduce all these specialized exceptions. Sorry @andymina my advice steered you here!!

No worries!

dariakp · 2021-08-16T18:13:06Z

src/bulk/common.ts

+    errmsg: string;
+    errInfo?: Document;
+  } {
+    // WriteConcernErrorData has been inlined as the return to avoid making it public


what's the reasoning against making it public?

I figured it would be yet another type clouding the namespace relating to an unclearly layered WriteConcernError/WriteError/BulkError space. But it's an 🧅 that can be argued otherwise

emadum

LGTM 👍

dariakp

LGTM :)

nbbeeken

LGTM

andymina added 17 commits August 3, 2021 18:25

refactor(NODE-3337): unwrap-WriteConcernError

6712fbe

refactor(NODE-3337): potential unwrapping solution

992c3c7

refactor(NODE-3337): remove WriteConcernError wrapping

2269004

refactor(NODE-3337): update more instances of WriteConcernError

d43977c

refactor(NODE-3337): unwrap MongoBulkWriteError constructor

01fc75b

refactor(NODE-3337): unwrap WriteConcernError constructor

4f38794

refactor(NODE-3337): update MongoBulkWriteError constructor

0fc0bbf

test(NODE-3337): fix ts definitions test

ce1ab0a

refactor(NODE-3337): unwrap-WriteConcernError

fa88f86

refactor(NODE-3337): potential unwrapping solution

2f4e16e

refactor(NODE-3337): remove WriteConcernError wrapping

e1ce12f

refactor(NODE-3337): update more instances of WriteConcernError

d6a7430

refactor(NODE-3337): unwrap MongoBulkWriteError constructor

3df81f1

refactor(NODE-3337): unwrap WriteConcernError constructor

d177f29

refactor(NODE-3337): update MongoBulkWriteError constructor

d6cb94f

test(NODE-3337): fix ts definitions test

759e6c6

refactor(NODE-3337): add topologyVersion to MongoServerError

687e4a9

andymina closed this Aug 6, 2021

andymina reopened this Aug 6, 2021

andymina marked this pull request as draft August 6, 2021 19:06

andymina self-assigned this Aug 6, 2021

andymina added the wip label Aug 6, 2021

andymina added 2 commits August 6, 2021 16:41

refactor(NODE-3337): reintroduce loop in MongoServerError constructor

b5616f5

refactor(NODE-3404): prevent double work in MongoServerError constructor

7914e29

dariakp requested changes Aug 9, 2021

View reviewed changes

andymina added 2 commits August 9, 2021 11:55

refactor(NODE-3337): revert MongoServerError constructor

43e1acd

fix(NODE-3337): fix duplicate declaration typo

487c8b7

andymina removed the wip label Aug 9, 2021

andymina added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Aug 9, 2021

nbbeeken suggested changes Aug 9, 2021

View reviewed changes

andymina added 2 commits August 9, 2021 16:18

refactor(NODE-3337): revert incorrect usage of MongoWriteConcernError

eb5e0c1

docs(NODE-3337): add comment explaining MongoServerError usage

d701150

andymina requested a review from nbbeeken August 10, 2021 18:51

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 Aug 10, 2021

andymina requested review from dariakp and emadum August 10, 2021 21:56

dariakp requested changes Aug 12, 2021

View reviewed changes

refactor(NODE-3337): add symbol to WriteConcernError and revert Mongo…

430e8ec

…ServerError

dariakp requested changes Aug 12, 2021

View reviewed changes

src/bulk/common.ts Outdated Show resolved Hide resolved

andymina added 2 commits August 12, 2021 17:20

refactor(NODE-3337): remove explicit props in WriteConcernError

524ee39

refactor(NODE-3337): add WriteConcernErrorData interface

3015451

emadum suggested changes Aug 12, 2021

View reviewed changes

src/bulk/common.ts Outdated Show resolved Hide resolved

andymina added 2 commits August 12, 2021 20:05

style(NODE-3337): mark kServerError internal

8b6ea51

refactor(NODE-3337): inline return typeof toJSON

f5a1a7d

andymina requested review from dariakp and emadum August 16, 2021 16:13

dariakp reviewed Aug 16, 2021

View reviewed changes

refactor(NODE-3337): make WriteConcernErrorData public

3b6f491

andymina requested a review from dariakp August 16, 2021 19:22

emadum approved these changes Aug 17, 2021

View reviewed changes

dariakp approved these changes Aug 17, 2021

View reviewed changes

nbbeeken approved these changes Aug 17, 2021

View reviewed changes

nbbeeken changed the title ~~refactor(NODE-3337): unwrap-WriteConcernError~~ refactor(NODE-3337): unwrap WriteConcernError Aug 17, 2021

nbbeeken merged commit 2cdbc08 into 4.1 Aug 17, 2021

nbbeeken deleted the NODE-3337/unwrap-WriteConcernError branch August 17, 2021 15:18

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

refactor(NODE-3337): unwrap WriteConcernError (#2932)

065045d

refactor(NODE-3337): unwrap WriteConcernError #2932

refactor(NODE-3337): unwrap WriteConcernError #2932

Uh oh!

Conversation

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

Uh oh!

Description

Uh oh!

dariakp commented Aug 6, 2021

Uh oh!

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

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!

dariakp Aug 12, 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!

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!

Uh oh!

andymina commented Aug 10, 2021

Uh oh!

dariakp Aug 12, 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!

emadum left a comment

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!

dariakp Aug 16, 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!

emadum 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!

nbbeeken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

andymina commented Aug 6, 2021 •

edited

Loading

andymina commented Aug 6, 2021 •

edited

Loading

dariakp Aug 12, 2021 •

edited

Loading

dariakp Aug 12, 2021 •

edited

Loading

dariakp Aug 16, 2021 •

edited

Loading