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

fix(Bulk): change BulkWriteError message to first item from writeErrors #2013

Merged
merged 4 commits into from
Jun 11, 2019

Conversation

spal1
Copy link
Contributor

@spal1 spal1 commented Jun 4, 2019

Fixes NODE-1965

Description

An unspecific BulkWriteError: write operation failed message was changed to the first item from writeErrors.

What changed?
A user requested to make the error more specific and suggested to use the first item from writeErrors. A change was made to the MongoError message field to reflect this error message.

@spal1 spal1 changed the base branch from master to next June 4, 2019 20:49
Copy link
Contributor

@kvwalker kvwalker left a comment

Choose a reason for hiding this comment

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

Great job! I left a couple of comments about the test to see if we can clean it up.

@@ -937,6 +937,69 @@ describe('Bulk', function() {
}
});

it(
'should provide descriptive error message for Unordered Batch with duplicate key errors on updates',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can change this from Unordered Batch to unordered batch (no caps).

it(
'should provide descriptive error message for Unordered Batch with duplicate key errors on updates',
{
metadata: { requires: { topology: ['single', 'replicaset', 'ssl', 'heap', 'wiredtiger'] } },
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're allowing every topology here, we can change this test from

it('title', {
  metadata: { requires: { topology: ['single', 'replicaset', 'ssl', 'heap', 'wiredtiger'] } },
  test: function(done) {
});

to

it('title', function(done) {
});

and it will run on all topology types.


test: function(done) {
const configuration = this.configuration;
var client = configuration.newClient(configuration.writeConcernMax(), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you switch all of these vars to const? const is more modern and we are trying to slowly modernize our tests by making any new tests modern.

writeConcern.sparse = false;

// Add unique index on b field causing all updates to fail
col.ensureIndex({ b: 1 }, writeConcern, function(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since ensureIndex is deprecated in favor of createIndexes, can we use col.createIndexes instead? Here are the docs for how to use createIndexes.

expect(result.nInserted).to.equal(2);
expect(result.hasWriteErrors()).to.equal(true);
expect(
result.getWriteErrorCount() === 4 || result.getWriteErrorCount() === 3
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can change this test to ensure our error count is always 3. Maybe we can get rid of some of the batch.inserts.

Copy link
Contributor

@kvwalker kvwalker left a comment

Choose a reason for hiding this comment

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

This looks great! Just a couple of small changes.

client.close();
done();
});
client.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this to client.close(done); since we're here. This helps us modernize our tests.

// Add unique index on b field causing all updates to fail
col.ensureIndex({ b: 1 }, writeConcern, function(err) {
expect(err).to.not.exist;
client.connect(function(err, client) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For all callbacks in this test, let's use (err, client) => { notation instead of function(err, client) {.

@@ -1119,7 +1119,7 @@ class BulkOperationBase {
callback,
new BulkWriteError(
toError({
message: 'write operation failed',
message: this.s.bulkResult.writeErrors[0].errmsg,
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this.s.bulkResult.writeErrors[0] or its errmsg doesn't exist? Let's check that the errmsg exists and use the default write operation failed message if not.

Copy link
Contributor

@kvwalker kvwalker left a comment

Choose a reason for hiding this comment

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

looks good to me!

@spal1 spal1 merged commit 5432fbd into mongodb:next Jun 11, 2019
@spal1 spal1 deleted the NODE-1965 branch June 11, 2019 14:11
daprahamian pushed a commit that referenced this pull request Aug 13, 2019
…rs (#2013)

* fix(Bulk): change BulkWriteError message to first item from writeErrors

Fixes NODE-1965
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants