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.

Sign up for GitHub

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

Jump to bottom

feat(bulk)!: add collation to FindOperators #2679

Merged
merged 5 commits into from
Dec 21, 2020
Merged

feat(bulk)!: add collation to FindOperators #2679

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ceb2c58
refactor: add collation to FindOperators
emadum Dec 16, 2020
aeab23c
Merge remote-tracking branch 'origin/master' into NODE-2757/add-colla…
emadum Dec 17, 2020
85b8911
review feedback
emadum Dec 18, 2020
085fb5f
add validation and test for raw ops in bulkWrite
emadum Dec 21, 2020
2716d07
Merge remote-tracking branch 'origin/master' into NODE-2757/add-colla…
emadum Dec 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 62 additions & 87 deletions src/bulk/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,8 +725,8 @@ export class FindOperators {
this.bulkOperation = bulkOperation;
}

/** Add a multiple update operation to the bulk operation */
update(updateDocument: Document): BulkOperationBase {
/** @internal */
makeUpdateDocument(u: Document, multi: boolean): Document {
Copy link
Member

Choose a reason for hiding this comment

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

A few nits/questions:

  • is this making an UpdateStatement? Can we reuse this internal helper I introduced last week?
  • at very least I don't think this should be mounted on FindOperators since you're now effectively making it public API (despite the loose contract of @internal)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we reuse this internal helper I introduced last week?

Nice, I refactored to use the makeUpdateStatement and makeDeleteStatement helpers. Note that there were already helpers by that name in this file, which do a very similar thing but with slightly different behavior.

It would be nice to consolidate them, but for now I've renamed them to makeBulkUpdateStatement and makeBulkDeleteStatement. There are a few TODOs in there so I want to be careful about replacing them:

// NOTE: legacy support for a raw statement, consider removing
// TODO: this check should be done at command construction against a connection, not a topology

Regarding removing the legacy support for raw statements, would you have any concerns about that? 4.0 would probably be a good time for it.

Copy link
Contributor Author

@emadum emadum Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I went ahead with that consolidation, removed the legacy support for raw statements and fixed the tests we had using the legacy format, and updated the validation checks marked with TODOs to happen against a server rather than a topology.

Copy link
Member

Choose a reason for hiding this comment

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

Heck'n YES, I love it! I'd say make sure you take another trip through the API docs and tests to ensure that you're properly documenting the shape of what's accepted in bulk. Also, now that the "raw" form of the operations are no longer accepted I think you will want to add some validation to let users know that things have changed. Right now I think you'll be silently breaking their applications potentially, so good to add some tests that prove their apps will not break. Otherwise, I think this is a huge improvement for bulk, great job.

if (!this.bulkOperation.s.currentOp) {
this.bulkOperation.s.currentOp = {};
}
Expand All @@ -738,86 +738,93 @@ export class FindOperators {
: false;

// Establish the update command
const document: Document = {
q: this.bulkOperation.s.currentOp.selector,
u: updateDocument,
multi: true,
upsert: upsert
};
const q = this.bulkOperation.s.currentOp.selector;
const result: Document = { q, u, multi, upsert };

if (u.hint) {
result.hint = u.hint;
}

if (updateDocument.hint) {
document.hint = updateDocument.hint;
if (this.bulkOperation.s.currentOp.collation) {
result.collation = this.bulkOperation.s.currentOp.collation;
}

// Clear out current Op
this.bulkOperation.s.currentOp = undefined;
return this.bulkOperation.addToOperationsList(BatchType.UPDATE, document);

return result;
}

/** Add a single update operation to the bulk operation */
updateOne(updateDocument: Document): BulkOperationBase {
/** @internal */
makeDeleteDocument(limit: number): Document {
mbroadst marked this conversation as resolved.
Show resolved Hide resolved
if (!this.bulkOperation.s.currentOp) {
this.bulkOperation.s.currentOp = {};
}

// Perform upsert
const upsert =
typeof this.bulkOperation.s.currentOp.upsert === 'boolean'
? this.bulkOperation.s.currentOp.upsert
: false;

// Establish the update command
const document: Document = {
const document: DeleteStatement = {
q: this.bulkOperation.s.currentOp.selector,
u: updateDocument,
multi: false,
upsert: upsert
limit
};

if (updateDocument.hint) {
document.hint = updateDocument.hint;
if (this.bulkOperation.s.currentOp.collation) {
document.collation = this.bulkOperation.s.currentOp.collation;
}

// Clear out current Op
this.bulkOperation.s.currentOp = undefined;

return document;
}

/** Add a multiple update operation to the bulk operation */
update(updateDocument: Document): BulkOperationBase {
return this.bulkOperation.addToOperationsList(
BatchType.UPDATE,
this.makeUpdateDocument(updateDocument, true)
);
}

/** Add a single update operation to the bulk operation */
updateOne(updateDocument: Document): BulkOperationBase {
if (!hasAtomicOperators(updateDocument)) {
throw new TypeError('Update document requires atomic operators');
}

// Clear out current Op
this.bulkOperation.s.currentOp = undefined;
return this.bulkOperation.addToOperationsList(BatchType.UPDATE, document);
return this.bulkOperation.addToOperationsList(
BatchType.UPDATE,
this.makeUpdateDocument(updateDocument, false)
);
}

/** Add a replace one operation to the bulk operation */
replaceOne(replacement: Document): BulkOperationBase {
if (!this.bulkOperation.s.currentOp) {
this.bulkOperation.s.currentOp = {};
if (hasAtomicOperators(replacement)) {
throw new TypeError('Replacement document must not use atomic operators');
}

// Perform upsert
const upsert =
typeof this.bulkOperation.s.currentOp.upsert === 'boolean'
? this.bulkOperation.s.currentOp.upsert
: false;
return this.bulkOperation.addToOperationsList(
BatchType.UPDATE,
this.makeUpdateDocument(replacement, false)
);
}

// Establish the update command
const document: Document = {
q: this.bulkOperation.s.currentOp.selector,
u: replacement,
multi: false,
upsert: upsert
};
/** Add a delete one operation to the bulk operation */
deleteOne(): BulkOperationBase {
return this.bulkOperation.addToOperationsList(BatchType.DELETE, this.makeDeleteDocument(1));
}

if (replacement.hint) {
document.hint = replacement.hint;
}
/** Add a delete many operation to the bulk operation */
delete(): BulkOperationBase {
return this.bulkOperation.addToOperationsList(BatchType.DELETE, this.makeDeleteDocument(0));
}

if (hasAtomicOperators(replacement)) {
throw new TypeError('Replacement document must not use atomic operators');
}
removeOne(): BulkOperationBase {
Copy link
Member

Choose a reason for hiding this comment

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

remind me, is there a ticket to remove these duplicated methods for v4?

Copy link
Contributor Author

@emadum emadum Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't able to find one, it's not in the main ticket tracking deprecation (NODE-2317). Do you think we should get rid of them?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think now's the time to do 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.

I added NODE-2978 so as to not scope creep more on this PR 👍

return this.deleteOne();
}

// Clear out current Op
this.bulkOperation.s.currentOp = undefined;
return this.bulkOperation.addToOperationsList(BatchType.UPDATE, document);
remove(): BulkOperationBase {
return this.delete();
}

/** Upsert modifier for update bulk operation, noting that this operation is an upsert. */
Expand All @@ -830,46 +837,14 @@ export class FindOperators {
return this;
}

/** Add a delete one operation to the bulk operation */
deleteOne(): BulkOperationBase {
/** Specifies the collation for the query condition. */
collation(collation: CollationOptions): this {
if (!this.bulkOperation.s.currentOp) {
this.bulkOperation.s.currentOp = {};
}

// Establish the update command
const document = {
q: this.bulkOperation.s.currentOp.selector,
limit: 1
};

// Clear out current Op
this.bulkOperation.s.currentOp = undefined;
return this.bulkOperation.addToOperationsList(BatchType.DELETE, document);
}

/** Add a delete many operation to the bulk operation */
delete(): BulkOperationBase {
if (!this.bulkOperation.s.currentOp) {
this.bulkOperation.s.currentOp = {};
}

// Establish the update command
const document = {
q: this.bulkOperation.s.currentOp.selector,
limit: 0
};

// Clear out current Op
this.bulkOperation.s.currentOp = undefined;
return this.bulkOperation.addToOperationsList(BatchType.DELETE, document);
}

removeOne(): BulkOperationBase {
return this.deleteOne();
}

remove(): BulkOperationBase {
return this.delete();
this.bulkOperation.s.currentOp.collation = collation;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should expand the scope of this PR, but I just wanted to comment on something so you can keep it in mind. We have "builders" for a number of things in the driver, specifically for bulk and cursors. I think it would be really great for readability/maintainability if you were doing them all the same way. The bulk builders need a lot of work, and are confusingly coupled, but I think you could use a [kBuiltOptions] approach here (or better yet, wrap [kBuiltOptions] in some well-typed methods like addToCurrentOperation that you can reuse as a toolkit across all "builders" in the codebase).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that it'd be nice to have a single approach to built options. There's already a new ticket for adding a builder method for arrayFilters (NODE-2751), so perhaps we should prioritize this work as part of NODE-2734.

return this;
}
}

Expand Down
49 changes: 48 additions & 1 deletion test/functional/bulk.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
'use strict';
const { withClient, withClientV2, setupDatabase, ignoreNsNotFound } = require('./shared');
const {
withClient,
withClientV2,
withMonitoredClient,
setupDatabase,
ignoreNsNotFound
} = require('./shared');
const test = require('./shared').assert;
const { MongoError } = require('../../src/error');
const { Long } = require('../../src');
Expand Down Expand Up @@ -1841,4 +1847,45 @@ describe('Bulk', function () {
});
})
);

it('should apply collation via FindOperators', {
metadata: { requires: { mongodb: '>= 3.4' } },
test: withMonitoredClient(['update', 'delete'], function (client, events, done) {
const locales = ['fr', 'de', 'es'];
const bulk = client.db().collection('coll').initializeOrderedBulkOp();

// updates
bulk
.find({ b: 1 })
.collation({ locale: locales[0] })
.updateOne({ $set: { b: 2 } });
bulk
.find({ b: 2 })
.collation({ locale: locales[1] })
.update({ $set: { b: 3 } });
bulk.find({ b: 3 }).collation({ locale: locales[2] }).replaceOne({ b: 2 });

// deletes
bulk.find({ b: 2 }).collation({ locale: locales[0] }).removeOne();
bulk.find({ b: 1 }).collation({ locale: locales[1] }).remove();

bulk.execute(err => {
expect(err).to.not.exist;
expect(events).to.be.an('array').with.length.at.least(1);
expect(events[0]).property('commandName').to.equal('update');
const updateCommand = events[0].command;
expect(updateCommand).property('updates').to.be.an('array').with.length.at.least(1);
updateCommand.updates.forEach((statement, idx) => {
expect(statement).property('collation').to.eql({ locale: locales[idx] });
});
expect(events[1]).property('commandName').to.equal('delete');
const deleteCommand = events[1].command;
expect(deleteCommand).property('deletes').to.be.an('array').with.length.at.least(1);
deleteCommand.deletes.forEach((statement, idx) => {
expect(statement).property('collation').to.eql({ locale: locales[idx] });
});
client.close(done);
});
})
});
});