-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
fix(NODE-3071): Ignore error message if error code is defined #2770
Conversation
eb21117
to
171ca01
Compare
171ca01
to
95348e3
Compare
@@ -30,7 +30,7 @@ export type WriteProtocolMessageType = Query | Msg | GetMore | KillCursor; | |||
|
|||
/** @internal */ | |||
export interface OpQueryOptions extends CommandOptions { | |||
socketTimeout?: number; | |||
socketTimeoutMS?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -7,26 +7,55 @@ export type AnyError = MongoError | Error; | |||
|
|||
const kErrorLabels = Symbol('errorLabels'); | |||
|
|||
/** @internal MongoDB Error Codes */ | |||
export const MONGODB_ERROR_CODES = Object.freeze({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const SDAM_NOTMASTER_CODES = new Set<number>([ | ||
MONGODB_ERROR_CODES.NotMaster, | ||
MONGODB_ERROR_CODES.NotMasterNoSlaveOk, | ||
MONGODB_ERROR_CODES.LegacyNotPrimary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This additional error code is part of the spec change
} | ||
|
||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was to make the return type boolean
instead of RegExpMatch
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
if (isStaleServerDescription(this.s.description, serverDescription)) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
function isStaleServerDescription( | ||
topologyDescription: TopologyDescription, | ||
incomingServerDescription: ServerDescription | ||
) { | ||
const currentServerDescription = topologyDescription.servers.get( | ||
incomingServerDescription.address | ||
); | ||
const currentTopologyVersion = currentServerDescription?.topologyVersion; | ||
return ( | ||
compareTopologyVersion(currentTopologyVersion, incomingServerDescription.topologyVersion) > 0 | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also NODE-2559
test/functional/spec-runner/index.js
Outdated
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(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging this comment because it seems I didn't leave it as a review last time.
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(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Hey @emadum if you can take a look when you get the chance that would be much appreciated 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
test/functional/spec-runner/index.js
Outdated
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(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging this comment because it seems I didn't leave it as a review last time.
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