Skip to content
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

feat(NODE-5988): Provide access to raw results doc after MongoServerError #4016

Merged
merged 7 commits into from
Mar 7, 2024
Merged
12 changes: 11 additions & 1 deletion src/error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@ export class MongoError extends Error {
* @category Error
*/
export class MongoServerError extends MongoError {
/** Raw error result document returned by server. */
errorResponse: ErrorDescription;
codeName?: string;
writeConcernError?: Document;
errInfo?: Document;
Expand All @@ -223,9 +225,17 @@ export class MongoServerError extends MongoError {
this[kErrorLabels] = new Set(message.errorLabels);
}

this.errorResponse = message;

for (const name in message) {
if (name !== 'errorLabels' && name !== 'errmsg' && name !== 'message')
if (
name !== 'errorLabels' &&
name !== 'errmsg' &&
name !== 'message' &&
name !== 'errorResponse'
) {
this[name] = message[name];
}
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/sdam/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ export class Server extends TypedEventEmitter<ServerEvents> {
this.monitor.on(event, (e: any) => this.emit(event, e));
}

this.monitor.on('resetServer', (error: MongoError) => markServerUnknown(this, error));
this.monitor.on('resetServer', (error: MongoServerError) => markServerUnknown(this, error));
this.monitor.on(Server.SERVER_HEARTBEAT_SUCCEEDED, (event: ServerHeartbeatSucceededEvent) => {
this.emit(
Server.DESCRIPTION_RECEIVED,
Expand Down Expand Up @@ -369,7 +369,7 @@ export class Server extends TypedEventEmitter<ServerEvents> {
// clear for the specific service id.
if (!this.loadBalanced) {
error.addErrorLabel(MongoErrorLabel.ResetPool);
markServerUnknown(this, error);
markServerUnknown(this, error as MongoServerError);
} else if (connection) {
this.pool.clear({ serviceId: connection.serviceId });
}
Expand All @@ -385,7 +385,7 @@ export class Server extends TypedEventEmitter<ServerEvents> {
if (shouldClearPool) {
error.addErrorLabel(MongoErrorLabel.ResetPool);
}
markServerUnknown(this, error);
markServerUnknown(this, error as MongoServerError);
process.nextTick(() => this.requestCheck());
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/sdam/topology_description.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ export class TopologyDescription {
);

if (descriptionsWithError.length > 0) {
return descriptionsWithError[0].error;
return descriptionsWithError[0].error as MongoServerError;
}

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ const { loadSpecTests } = require('../../spec/index');
const { runUnifiedSuite } = require('../../tools/unified-spec-runner/runner');

// The Node driver does not have a Collection.modifyCollection helper.
const SKIPPED_TESTS = ['modifyCollection to changeStreamPreAndPostImages enabled'];
const SKIPPED_TESTS = [
'modifyCollection to changeStreamPreAndPostImages enabled',
'modifyCollection prepareUnique violations are accessible'
];

describe('Collection management unified spec tests', function () {
runUnifiedSuite(loadSpecTests('collection-management'), ({ description }) =>
Expand Down
6 changes: 5 additions & 1 deletion test/integration/crud/crud.spec.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,10 @@ describe('CRUD spec v1', function () {
}
});

// TODO(NODE-5998) - The Node driver UTR does not have a Collection.modifyCollection helper.
const SKIPPED_TESTS = ['findOneAndUpdate document validation errInfo is accessible'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are different from the collection management specs. These tests use the modifyCollection operation to set up the collection for the test, but they're not testing the driver's modifyCollection helper.

I think it might make sense to implement a unified test format helper for modifyCollection that uses runCommandto send a collMod to the server (i.e., a new operation in operations.ts). Then we could unskip this test. We'd still leave the modifyCollection tests alone though because we don't have a driver helper.

what do you think?

(also see https://github.com/mongodb/specifications/pull/1316/files#r994171436)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could merge these changes and make a separate subtask or even ticket for adding in the modifyCollection changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me - if we file a separate ticket, can we add a TODO(NODE-XXXX) comment here so we don't forget about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Just filed the ticket NODE-5998.

describe('CRUD unified', function () {
runUnifiedSuite(loadSpecTests(path.join('crud', 'unified')));
runUnifiedSuite(loadSpecTests(path.join('crud', 'unified')), ({ description }) =>
SKIPPED_TESTS.includes(description) ? `the Node driver does not have a collMod helper.` : false
);
});
118 changes: 118 additions & 0 deletions test/spec/collection-management/modifyCollection-errorResponse.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
{
"description": "modifyCollection-errorResponse",
"schemaVersion": "1.12",
"createEntities": [
{
"client": {
"id": "client0",
"observeEvents": [
"commandStartedEvent"
]
}
},
{
"database": {
"id": "database0",
"client": "client0",
"databaseName": "collMod-tests"
}
},
{
"collection": {
"id": "collection0",
"database": "database0",
"collectionName": "test"
}
}
],
"initialData": [
{
"collectionName": "test",
"databaseName": "collMod-tests",
"documents": [
{
"_id": 1,
"x": 1
},
{
"_id": 2,
"x": 1
}
]
}
],
"tests": [
{
"description": "modifyCollection prepareUnique violations are accessible",
"runOnRequirements": [
{
"minServerVersion": "5.2"
}
],
"operations": [
{
"name": "createIndex",
"object": "collection0",
"arguments": {
"keys": {
"x": 1
}
}
},
{
"name": "modifyCollection",
"object": "database0",
"arguments": {
"collection": "test",
"index": {
"keyPattern": {
"x": 1
},
"prepareUnique": true
}
}
},
{
"name": "insertOne",
"object": "collection0",
"arguments": {
"document": {
"_id": 3,
"x": 1
}
},
"expectError": {
"errorCode": 11000
}
},
{
"name": "modifyCollection",
"object": "database0",
"arguments": {
"collection": "test",
"index": {
"keyPattern": {
"x": 1
},
"unique": true
}
},
"expectError": {
"isClientError": false,
"errorCode": 359,
"errorResponse": {
"violations": [
{
"ids": [
1,
2
]
}
]
}
}
}
]
}
]
}
59 changes: 59 additions & 0 deletions test/spec/collection-management/modifyCollection-errorResponse.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
description: "modifyCollection-errorResponse"

schemaVersion: "1.12"

createEntities:
- client:
id: &client0 client0
observeEvents: [ commandStartedEvent ]
- database:
id: &database0 database0
client: *client0
databaseName: &database0Name collMod-tests
- collection:
id: &collection0 collection0
database: *database0
collectionName: &collection0Name test

initialData: &initialData
- collectionName: *collection0Name
databaseName: *database0Name
documents:
- { _id: 1, x: 1 }
- { _id: 2, x: 1 }

tests:
- description: "modifyCollection prepareUnique violations are accessible"
runOnRequirements:
- minServerVersion: "5.2" # SERVER-61158
operations:
- name: createIndex
object: *collection0
arguments:
keys: { x: 1 }
- name: modifyCollection
object: *database0
arguments:
collection: *collection0Name
index:
keyPattern: { x: 1 }
prepareUnique: true
- name: insertOne
object: *collection0
arguments:
document: { _id: 3, x: 1 }
expectError:
errorCode: 11000 # DuplicateKey
- name: modifyCollection
object: *database0
arguments:
collection: *collection0Name
index:
keyPattern: { x: 1 }
unique: true
expectError:
isClientError: false
errorCode: 359 # CannotConvertIndexToUnique
errorResponse:
violations:
- { ids: [ 1, 2 ] }
90 changes: 90 additions & 0 deletions test/spec/crud/unified/aggregate-merge-errorResponse.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
{
"description": "aggregate-merge-errorResponse",
"schemaVersion": "1.12",
"createEntities": [
{
"client": {
"id": "client0"
}
},
{
"database": {
"id": "database0",
"client": "client0",
"databaseName": "crud-tests"
}
},
{
"collection": {
"id": "collection0",
"database": "database0",
"collectionName": "test"
}
}
],
"initialData": [
{
"collectionName": "test",
"databaseName": "crud-tests",
"documents": [
{
"_id": 1,
"x": 1
},
{
"_id": 2,
"x": 1
}
]
}
],
"tests": [
{
"description": "aggregate $merge DuplicateKey error is accessible",
"runOnRequirements": [
{
"minServerVersion": "5.1",
"topologies": [
"single",
"replicaset"
]
}
],
"operations": [
{
"name": "aggregate",
"object": "database0",
"arguments": {
"pipeline": [
{
"$documents": [
{
"_id": 2,
"x": 1
}
]
},
{
"$merge": {
"into": "test",
"whenMatched": "fail"
}
}
]
},
"expectError": {
"errorCode": 11000,
"errorResponse": {
"keyPattern": {
"_id": 1
},
"keyValue": {
"_id": 2
}
}
}
}
]
}
]
}