fix(NODE-3071): Ignore error message if error code is defined #2770

nbbeeken · 2021-03-26T21:27:03Z

Prior behavior was to check the error message if the code did not
correspond to the server state. With this change the driver will not
inspect the error message for error type. ServerDescriptions with
topology versions less than the current will be ignored.

NODE-3071, NODE-2559

Prior behavior was to check the error message if the code did not correspond to the server state. With this change the driver will not inspect the error message for error type. ServerDescriptions with topology versions less than the current will be ignored. NODE-3071, NODE-2559

nbbeeken · 2021-03-29T18:34:44Z

src/cmap/commands.ts

@@ -30,7 +30,7 @@ export type WriteProtocolMessageType = Query | Msg | GetMore | KillCursor;

 /** @internal */
 export interface OpQueryOptions extends CommandOptions {
-  socketTimeout?: number;
+  socketTimeoutMS?: number;


We had an inconsistency where socketTimeoutMS was being ignored because we only checked for socketTimeout. You'll see that update in a number of places.

nbbeeken · 2021-03-29T18:39:05Z

src/error.ts

@@ -7,26 +7,55 @@ export type AnyError = MongoError | Error;

 const kErrorLabels = Symbol('errorLabels');

+/** @internal MongoDB Error Codes */
+export const MONGODB_ERROR_CODES = Object.freeze({


Since we're using codes in a number of places it seemed to make sense to make an enum out of them all. Luckily the git diff shows the naming is the same as the comment that was placed next to a lot of the numbers.

nbbeeken · 2021-03-29T18:39:35Z

src/error.ts

+const SDAM_NOTMASTER_CODES = new Set<number>([
+  MONGODB_ERROR_CODES.NotMaster,
+  MONGODB_ERROR_CODES.NotMasterNoSlaveOk,
+  MONGODB_ERROR_CODES.LegacyNotPrimary


This additional error code is part of the spec change

nbbeeken · 2021-03-29T18:40:20Z

src/error.ts

  }

-  return err.message.match(/not master or secondary/) || err.message.match(/node is recovering/);
+  return /not master or secondary/.test(err.message) || /node is recovering/.test(err.message);


This change was to make the return type boolean instead of RegExpMatch

nbbeeken · 2021-03-29T18:47:23Z

src/sdam/server_description.ts

+    // tests mock counter as just number, but in a real situation counter should always be a Long
+    const lhsCounter = Long.isLong(lhs.counter) ? lhs.counter : Long.fromNumber(lhs.counter);
+    const rhsCounter = Long.isLong(rhs.counter) ? lhs.counter : Long.fromNumber(rhs.counter);
+    return lhsCounter.compare(rhsCounter);


When testing the counter property is a number so === works as expected. However the counter is supposed to be a Long and === as well as < will fail when this is the case. So converting here made sense. There might be a larger issue here of internal communications with mongod losing precision since we don't breakout our own set of bson options, thoughts?

nbbeeken · 2021-03-29T18:48:57Z

src/sdam/topology.ts

+    if (isStaleServerDescription(this.s.description, serverDescription)) {
+      return;
+    }


This is the changes from NODE-2559 that didn't get ported to 4.0 branch. It was necessary to do here to get spec tests to pass.

nbbeeken · 2021-03-29T18:49:09Z

src/sdam/topology.ts

+function isStaleServerDescription(
+  topologyDescription: TopologyDescription,
+  incomingServerDescription: ServerDescription
+) {
+  const currentServerDescription = topologyDescription.servers.get(
+    incomingServerDescription.address
+  );
+  const currentTopologyVersion = currentServerDescription?.topologyVersion;
+  return (
+    compareTopologyVersion(currentTopologyVersion, incomingServerDescription.topologyVersion) > 0
+  );
+}


Also NODE-2559

nbbeeken · 2021-03-29T18:50:52Z

test/functional/spec-runner/index.js

+              if (requires.authEnabled) {
+                // TODO: We do not have a way to determine if auth is enabled in our mocha metadata
+                // We need to do a admin.command({getCmdLineOpts: 1}) if it errors (code=13) auth is on
+                this.skip();
+              }


Syncing the tests added some tests that use authEnabled as a runOn requirement, I believe we don't currently have a way of filtering for this, correct me if I'm wrong and I'll make the change. If not I left a comment suggesting the method the python driver uses.

One option could be to check the $AUTH environment variable. At least when running on Evergreen, it'll be auth if the tests are running against a server with auth enabled (noauth otherwise). A bit more hassle when running tests locally since you need to set that env var when running your tests, but the simplicity of the implementation could make that worth it.

Flagging this comment because it seems I didn't leave it as a review last time.

nbbeeken · 2021-03-29T18:52:14Z

test/functional/spec-runner/index.js

+              if (
+                spec.operations.some(
+                  op => op.name === 'waitForEvent' && op.arguments.event === 'PoolReadyEvent'
+                )
+              ) {
+                // TODO(NODE-2994): Connection storms work will add new events to connection pool
+                this.skip();
+              }


We haven't implemented the paused or ready events on a connection pool yet which some tests block on a 'waitForEvent' operation, until mocha times them out.

nbbeeken · 2021-04-02T16:39:42Z

Hey @emadum if you can take a look when you get the chance that would be much appreciated 🙂

emadum

Had one comment a few days ago but it looks like it got lost in the shuffle, either way I don't think that needs to hold this up; LGTM 👍

emadum · 2021-04-02T17:37:01Z

test/functional/spec-runner/index.js

+              if (requires.authEnabled) {
+                // TODO: We do not have a way to determine if auth is enabled in our mocha metadata
+                // We need to do a admin.command({getCmdLineOpts: 1}) if it errors (code=13) auth is on
+                this.skip();
+              }


Flagging this comment because it seems I didn't leave it as a review last time.

nbbeeken · 2021-04-06T14:21:46Z

Hey @durran just a reminder to take a look at this when you get the chance, thanks!

Prior behavior was to check the error message if the code did not correspond to the server state. With this change the driver will not inspect the error message for error type. ServerDescriptions with topology versions less than the current will be ignored. NODE-3071, NODE-2559

nbbeeken force-pushed the NODE-3071/4.0/spec-do-not-inspect-error-message branch from eb21117 to 171ca01 Compare March 29, 2021 17:00

nbbeeken force-pushed the NODE-3071/4.0/spec-do-not-inspect-error-message branch from 171ca01 to 95348e3 Compare March 29, 2021 18:01

nbbeeken commented Mar 29, 2021

View reviewed changes

test: clean up erroneous test skip

0a4f78d

nbbeeken requested review from emadum and durran March 29, 2021 18:54

nbbeeken marked this pull request as ready for review March 29, 2021 18:54

emadum approved these changes Apr 2, 2021

View reviewed changes

fix: check AUTH env variable

88a61e4

nbbeeken merged commit d4cc936 into 4.0 Apr 6, 2021

nbbeeken deleted the NODE-3071/4.0/spec-do-not-inspect-error-message branch April 6, 2021 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(NODE-3071): Ignore error message if error code is defined #2770

fix(NODE-3071): Ignore error message if error code is defined #2770

nbbeeken commented Mar 26, 2021

nbbeeken Mar 29, 2021

nbbeeken Mar 29, 2021

nbbeeken Mar 29, 2021

nbbeeken Mar 29, 2021

nbbeeken Mar 29, 2021

nbbeeken Mar 29, 2021

nbbeeken Mar 29, 2021

nbbeeken Mar 29, 2021

emadum Mar 30, 2021 •

edited

Loading

emadum Apr 2, 2021

nbbeeken Mar 29, 2021

nbbeeken commented Apr 2, 2021

emadum left a comment •

edited

Loading

emadum Apr 2, 2021

nbbeeken commented Apr 6, 2021

fix(NODE-3071): Ignore error message if error code is defined #2770

fix(NODE-3071): Ignore error message if error code is defined #2770

Conversation

nbbeeken commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emadum Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken commented Apr 2, 2021

emadum left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken commented Apr 6, 2021

emadum Mar 30, 2021 •

edited

Loading

emadum left a comment •

edited

Loading