Skip to content

Commit e0e6869

Browse files
emadumemadum
committed
review feedback
1 parent aeab23c commit e0e6869

File tree

8 files changed

+137
-183
lines changed

8 files changed

+137
-183
lines changed

‎src/bulk/common.ts

+46-159
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,13 @@ import {
77
hasAtomicOperators,
88
Callback,
99
MongoDBNamespace,
10-
maxWireVersion,
1110
getTopology,
1211
resolveOptions
1312
} from '../utils';
1413
import { executeOperation } from '../operations/execute_operation';
1514
import { InsertOperation } from '../operations/insert';
16-
import { UpdateOperation, UpdateStatement } from '../operations/update';
17-
import { DeleteOperation, DeleteStatement } from '../operations/delete';
15+
import { UpdateOperation, UpdateStatement, makeUpdateStatement } from '../operations/update';
16+
import { DeleteOperation, DeleteStatement, makeDeleteStatement } from '../operations/delete';
1817
import { WriteConcern } from '../write_concern';
1918
import type { Collection } from '../collection';
2019
import type { Topology } from '../sdam/topology';
@@ -725,63 +724,15 @@ export class FindOperators {
725724
this.bulkOperation = bulkOperation;
726725
}
727726

728-
/** @internal */
729-
makeUpdateDocument(u: Document, multi: boolean): Document {
730-
if (!this.bulkOperation.s.currentOp) {
731-
this.bulkOperation.s.currentOp = {};
732-
}
733-
734-
// Perform upsert
735-
const upsert =
736-
typeof this.bulkOperation.s.currentOp.upsert === 'boolean'
737-
? this.bulkOperation.s.currentOp.upsert
738-
: false;
739-
740-
// Establish the update command
741-
const q = this.bulkOperation.s.currentOp.selector;
742-
const result: Document = { q, u, multi, upsert };
743-
744-
if (u.hint) {
745-
result.hint = u.hint;
746-
}
747-
748-
if (this.bulkOperation.s.currentOp.collation) {
749-
result.collation = this.bulkOperation.s.currentOp.collation;
750-
}
751-
752-
// Clear out current Op
753-
this.bulkOperation.s.currentOp = undefined;
754-
755-
return result;
756-
}
757-
758-
/** @internal */
759-
makeDeleteDocument(limit: number): Document {
760-
if (!this.bulkOperation.s.currentOp) {
761-
this.bulkOperation.s.currentOp = {};
762-
}
763-
764-
// Establish the update command
765-
const document: DeleteStatement = {
766-
q: this.bulkOperation.s.currentOp.selector,
767-
limit
768-
};
769-
770-
if (this.bulkOperation.s.currentOp.collation) {
771-
document.collation = this.bulkOperation.s.currentOp.collation;
772-
}
773-
774-
// Clear out current Op
775-
this.bulkOperation.s.currentOp = undefined;
776-
777-
return document;
778-
}
779-
780727
/** Add a multiple update operation to the bulk operation */
781728
update(updateDocument: Document): BulkOperationBase {
729+
const currentOp = buildCurrentOp(this.bulkOperation);
782730
return this.bulkOperation.addToOperationsList(
783731
BatchType.UPDATE,
784-
this.makeUpdateDocument(updateDocument, true)
732+
makeUpdateStatement(currentOp.selector, updateDocument, {
733+
...currentOp,
734+
multi: true
735+
})
785736
);
786737
}
787738

@@ -791,9 +742,10 @@ export class FindOperators {
791742
throw new TypeError('Update document requires atomic operators');
792743
}
793744

745+
const currentOp = buildCurrentOp(this.bulkOperation);
794746
return this.bulkOperation.addToOperationsList(
795747
BatchType.UPDATE,
796-
this.makeUpdateDocument(updateDocument, false)
748+
makeUpdateStatement(currentOp.selector, updateDocument, { ...currentOp, multi: false })
797749
);
798750
}
799751

@@ -803,20 +755,29 @@ export class FindOperators {
803755
throw new TypeError('Replacement document must not use atomic operators');
804756
}
805757

758+
const currentOp = buildCurrentOp(this.bulkOperation);
806759
return this.bulkOperation.addToOperationsList(
807760
BatchType.UPDATE,
808-
this.makeUpdateDocument(replacement, false)
761+
makeUpdateStatement(currentOp.selector, replacement, { ...currentOp, multi: false })
809762
);
810763
}
811764

812765
/** Add a delete one operation to the bulk operation */
813766
deleteOne(): BulkOperationBase {
814-
return this.bulkOperation.addToOperationsList(BatchType.DELETE, this.makeDeleteDocument(1));
767+
const currentOp = buildCurrentOp(this.bulkOperation);
768+
return this.bulkOperation.addToOperationsList(
769+
BatchType.DELETE,
770+
makeDeleteStatement(currentOp.selector, { ...currentOp, limit: 1 })
771+
);
815772
}
816773

817774
/** Add a delete many operation to the bulk operation */
818775
delete(): BulkOperationBase {
819-
return this.bulkOperation.addToOperationsList(BatchType.DELETE, this.makeDeleteDocument(0));
776+
const currentOp = buildCurrentOp(this.bulkOperation);
777+
return this.bulkOperation.addToOperationsList(
778+
BatchType.DELETE,
779+
makeDeleteStatement(currentOp.selector, { ...currentOp, limit: 0 })
780+
);
820781
}
821782

822783
removeOne(): BulkOperationBase {
@@ -1111,19 +1072,23 @@ export abstract class BulkOperationBase {
11111072

11121073
if ('replaceOne' in op || 'updateOne' in op || 'updateMany' in op) {
11131074
if ('replaceOne' in op) {
1114-
const updateStatement = makeUpdateStatement(this.s.topology, op.replaceOne, false);
1075+
const updateStatement = makeUpdateStatement(
1076+
op.replaceOne.filter,
1077+
op.replaceOne.replacement,
1078+
{ ...op.replaceOne, multi: false }
1079+
);
11151080
if (hasAtomicOperators(updateStatement.u)) {
11161081
throw new TypeError('Replacement document must not use atomic operators');
11171082
}
11181083

1119-
return this.addToOperationsList(
1120-
BatchType.UPDATE,
1121-
makeUpdateStatement(this.s.topology, op.replaceOne, false)
1122-
);
1084+
return this.addToOperationsList(BatchType.UPDATE, updateStatement);
11231085
}
11241086

11251087
if ('updateOne' in op) {
1126-
const updateStatement = makeUpdateStatement(this.s.topology, op.updateOne, false);
1088+
const updateStatement = makeUpdateStatement(op.updateOne.filter, op.updateOne.update, {
1089+
...op.updateOne,
1090+
multi: false
1091+
});
11271092
if (!hasAtomicOperators(updateStatement.u)) {
11281093
throw new TypeError('Update document requires atomic operators');
11291094
}
@@ -1132,7 +1097,10 @@ export abstract class BulkOperationBase {
11321097
}
11331098

11341099
if ('updateMany' in op) {
1135-
const updateStatement = makeUpdateStatement(this.s.topology, op.updateMany, true);
1100+
const updateStatement = makeUpdateStatement(op.updateMany.filter, op.updateMany.update, {
1101+
...op.updateMany,
1102+
multi: true
1103+
});
11361104
if (!hasAtomicOperators(updateStatement.u)) {
11371105
throw new TypeError('Update document requires atomic operators');
11381106
}
@@ -1144,28 +1112,28 @@ export abstract class BulkOperationBase {
11441112
if ('removeOne' in op) {
11451113
return this.addToOperationsList(
11461114
BatchType.DELETE,
1147-
makeDeleteStatement(this.s.topology, op.removeOne, false)
1115+
makeDeleteStatement(op.removeOne.filter, { ...op.removeOne, limit: 1 })
11481116
);
11491117
}
11501118

11511119
if ('removeMany' in op) {
11521120
return this.addToOperationsList(
11531121
BatchType.DELETE,
1154-
makeDeleteStatement(this.s.topology, op.removeMany, true)
1122+
makeDeleteStatement(op.removeMany.filter, { ...op.removeMany, limit: 0 })
11551123
);
11561124
}
11571125

11581126
if ('deleteOne' in op) {
11591127
return this.addToOperationsList(
11601128
BatchType.DELETE,
1161-
makeDeleteStatement(this.s.topology, op.deleteOne, false)
1129+
makeDeleteStatement(op.deleteOne.filter, { ...op.deleteOne, limit: 1 })
11621130
);
11631131
}
11641132

11651133
if ('deleteMany' in op) {
11661134
return this.addToOperationsList(
11671135
BatchType.DELETE,
1168-
makeDeleteStatement(this.s.topology, op.deleteMany, true)
1136+
makeDeleteStatement(op.deleteMany.filter, { ...op.deleteMany, limit: 0 })
11691137
);
11701138
}
11711139

@@ -1303,94 +1271,6 @@ function shouldForceServerObjectId(bulkOperation: BulkOperationBase): boolean {
13031271
return false;
13041272
}
13051273

1306-
function makeUpdateStatement(
1307-
topology: Topology,
1308-
model: ReplaceOneModel | UpdateOneModel | UpdateManyModel,
1309-
multi: boolean
1310-
): UpdateStatement {
1311-
// NOTE: legacy support for a raw statement, consider removing
1312-
if (isUpdateStatement(model)) {
1313-
if ('collation' in model && maxWireVersion(topology) < 5) {
1314-
throw new TypeError('Topology does not support collation');
1315-
}
1316-
1317-
return model as UpdateStatement;
1318-
}
1319-
1320-
const statement: UpdateStatement = {
1321-
q: model.filter,
1322-
u: 'update' in model ? model.update : model.replacement,
1323-
multi,
1324-
upsert: 'upsert' in model ? model.upsert : false
1325-
};
1326-
1327-
if ('collation' in model) {
1328-
if (maxWireVersion(topology) < 5) {
1329-
throw new TypeError('Topology does not support collation');
1330-
}
1331-
1332-
statement.collation = model.collation;
1333-
}
1334-
1335-
if ('arrayFilters' in model) {
1336-
// TODO: this check should be done at command construction against a connection, not a topology
1337-
if (maxWireVersion(topology) < 6) {
1338-
throw new TypeError('arrayFilters are only supported on MongoDB 3.6+');
1339-
}
1340-
1341-
statement.arrayFilters = model.arrayFilters;
1342-
}
1343-
1344-
if ('hint' in model) {
1345-
statement.hint = model.hint;
1346-
}
1347-
1348-
return statement;
1349-
}
1350-
1351-
function isUpdateStatement(model: Document): model is UpdateStatement {
1352-
return 'q' in model;
1353-
}
1354-
1355-
function makeDeleteStatement(
1356-
topology: Topology,
1357-
model: DeleteOneModel | DeleteManyModel,
1358-
multi: boolean
1359-
): DeleteStatement {
1360-
// NOTE: legacy support for a raw statement, consider removing
1361-
if (isDeleteStatement(model)) {
1362-
if ('collation' in model && maxWireVersion(topology) < 5) {
1363-
throw new TypeError('Topology does not support collation');
1364-
}
1365-
1366-
model.limit = multi ? 0 : 1;
1367-
return model as DeleteStatement;
1368-
}
1369-
1370-
const statement: DeleteStatement = {
1371-
q: model.filter,
1372-
limit: multi ? 0 : 1
1373-
};
1374-
1375-
if ('collation' in model) {
1376-
if (maxWireVersion(topology) < 5) {
1377-
throw new TypeError('Topology does not support collation');
1378-
}
1379-
1380-
statement.collation = model.collation;
1381-
}
1382-
1383-
if ('hint' in model) {
1384-
statement.hint = model.hint;
1385-
}
1386-
1387-
return statement;
1388-
}
1389-
1390-
function isDeleteStatement(model: Document): model is DeleteStatement {
1391-
return 'q' in model;
1392-
}
1393-
13941274
function isInsertBatch(batch: Batch): boolean {
13951275
return batch.batchType === BatchType.INSERT;
13961276
}
@@ -1402,3 +1282,10 @@ function isUpdateBatch(batch: Batch): batch is Batch<UpdateStatement> {
14021282
function isDeleteBatch(batch: Batch): batch is Batch<DeleteStatement> {
14031283
return batch.batchType === BatchType.DELETE;
14041284
}
1285+
1286+
function buildCurrentOp(bulkOp: BulkOperationBase): Document {
1287+
let { currentOp } = bulkOp.s;
1288+
bulkOp.s.currentOp = undefined;
1289+
if (!currentOp) currentOp = {};
1290+
return currentOp;
1291+
}

‎src/collection.ts

+2
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,8 @@ export class Collection {
333333
* ```js
334334
* { insertOne: { document: { a: 1 } } }
335335
*
336+
* { insertMany: [{ g: 1 }, { g: 2 }]}
337+
*
336338
* { updateOne: { filter: {a:2}, update: {$set: {a:2}}, upsert:true } }
337339
*
338340
* { updateMany: { filter: {a:2}, update: {$set: {a:2}}, upsert:true } }

‎src/operations/delete.ts

+6-1
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,11 @@ export class DeleteOperation extends CommandOperation<Document> {
8888
}
8989
}
9090

91+
if (this.statements.some(statement => !!statement.collation) && maxWireVersion(server) < 5) {
92+
callback(new MongoError(`server ${server.name} does not support collation`));
93+
return;
94+
}
95+
9196
super.executeCommand(server, session, command, callback);
9297
}
9398
}
@@ -132,7 +137,7 @@ export class DeleteManyOperation extends DeleteOperation {
132137
}
133138
}
134139

135-
function makeDeleteStatement(
140+
export function makeDeleteStatement(
136141
filter: Document,
137142
options: DeleteOptions & { limit?: number }
138143
): DeleteStatement {

‎src/operations/update.ts

+11-1
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,16 @@ export class UpdateOperation extends CommandOperation<Document> {
115115
return;
116116
}
117117

118+
if (this.statements.some(statement => !!statement.arrayFilters) && maxWireVersion(server) < 6) {
119+
callback(new MongoError('arrayFilters are only supported on MongoDB 3.6+'));
120+
return;
121+
}
122+
123+
if (this.statements.some(statement => !!statement.collation) && maxWireVersion(server) < 5) {
124+
callback(new MongoError(`server ${server.name} does not support collation`));
125+
return;
126+
}
127+
118128
super.executeCommand(server, session, command, callback);
119129
}
120130
}
@@ -247,7 +257,7 @@ export class ReplaceOneOperation extends UpdateOperation {
247257
}
248258
}
249259

250-
function makeUpdateStatement(
260+
export function makeUpdateStatement(
251261
filter: Document,
252262
update: Document,
253263
options: UpdateOptions & { multi?: boolean }

‎test/functional/apm.test.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -483,9 +483,9 @@ describe('APM', function () {
483483
.collection('apm_test_3')
484484
.bulkWrite(
485485
[
486-
{ insertOne: { a: 1 } },
487-
{ updateOne: { q: { a: 2 }, u: { $set: { a: 2 } }, upsert: true } },
488-
{ deleteOne: { q: { c: 1 } } }
486+
{ insertOne: { document: { a: 1 } } },
487+
{ updateOne: { filter: { a: 2 }, update: { $set: { a: 2 } }, upsert: true } },
488+
{ deleteOne: { filter: { c: 1 } } }
489489
],
490490
{ ordered: true }
491491
)

‎test/functional/bulk.test.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -1874,13 +1874,13 @@ describe('Bulk', function () {
18741874
expect(events).to.be.an('array').with.length.at.least(1);
18751875
expect(events[0]).property('commandName').to.equal('update');
18761876
const updateCommand = events[0].command;
1877-
expect(updateCommand).property('updates').to.be.an('array').with.length.at.least(1);
1877+
expect(updateCommand).property('updates').to.be.an('array').with.length(3);
18781878
updateCommand.updates.forEach((statement, idx) => {
18791879
expect(statement).property('collation').to.eql({ locale: locales[idx] });
18801880
});
18811881
expect(events[1]).property('commandName').to.equal('delete');
18821882
const deleteCommand = events[1].command;
1883-
expect(deleteCommand).property('deletes').to.be.an('array').with.length.at.least(1);
1883+
expect(deleteCommand).property('deletes').to.be.an('array').with.length(2);
18841884
deleteCommand.deletes.forEach((statement, idx) => {
18851885
expect(statement).property('collation').to.eql({ locale: locales[idx] });
18861886
});

0 commit comments

Comments
 (0)