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(pkFactory): use pkFactory for operations with insert as side effect #2193

Closed
wants to merge 1 commit into from
Closed

fix(pkFactory): use pkFactory for operations with insert as side effect #2193

wants to merge 1 commit into from

Conversation

smeijer
Copy link

@smeijer smeijer commented Oct 26, 2019

Description

As described in issue NODE-2275, the provided pkFactory is not being used for all _id generating methods. Or better said, all methods that can insert a document in some way.

It's being used for col.insertOne and col.insertMany, but not for findOneAndUpdate, bulk.insert or bulk.find.upsert.

This is my attempt to fix that.

Small reproduction to experience the issue yourself:

const Client = require('mongodb');
 
function CustomPKFactory() {}
CustomPKFactory.prototype = new Object();
CustomPKFactory.count = 0;
CustomPKFactory.createPk = function() {
return ++CustomPKFactory.count;
};
 
const host = 'mongodb://localhost:27017';
const options = { pkFactory: CustomPKFactory };
 
Client.connect(host, options, async (err, client) => {
  const db = client.db('pkFactory');
  const col = db.collection('test_custom_key');
  await col.deleteMany({});
 
  // ✓ expected _id: 1 | received _id: 1
  await col.insertOne({ a: 1 });
 
  // ⨉ expected _id: 2 | received _id: ObjectId(…)
  await col.findOneAndUpdate({ a: 2 }, { $set: { b: 2 } }, { upsert: true });
 
  const bulk = col.initializeUnorderedBulkOp();
 
  // ⨉ expected _id: 3 | received _id: ObjectId(…)
  bulk.insert({ a: 3 });
 
  // ⨉ expected _id: 4 | received _id: ObjectId(…)
  bulk
    .find({ a: 4 })
    .upsert()
    .update({ $set: { b: 4 } });
 
  await bulk.execute();
});

What changed?
Instead of simply using ObjectId to generate the id, the pkFactory is now being used.

Are there any files to ignore?
Not really, I only changed two files. Existing of 22 additions and 4 deletions. The change isn't that big.


fixes https://jira.mongodb.org/projects/NODE/issues/NODE-2275

Copy link
Contributor

@daprahamian daprahamian left a comment

Choose a reason for hiding this comment

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

We can probably add the pkFactory support for bulk inserts. However, I do not think that it is possible at this time to add client-side id creation for upserts:

  1. As of MongoDB 4.2, updates can be pipelines
  2. It would deviate from other drivers that do not have this behavior
  3. This mutates a user-passed document in a way it previously didn't, which can lead to unexpected behavior

If you'd like to put up a PR for just the change to bulk inserts, we could review that separately.

@daprahamian
Copy link
Contributor

Closing due to lack of response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants