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

Wrong document updating after pulling embedded object with _id field alias #9319

Closed
Kamikadze4GAME opened this issue Aug 12, 2020 · 3 comments
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@Kamikadze4GAME
Copy link
Contributor

Kamikadze4GAME commented Aug 12, 2020

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Inconsistent object saving after .pull(id) if you disable _id in the scheme of the subfield (MongooseArray) and use another field that has an alias _id.

new Schema({
 children: [
   new Schema({
     field: {
       type: String,
       alias: '_id'
     }
   }, { _id: false })
 ]
});

If the current behavior is a bug, please provide the steps to reproduce.

'use strict';

const mongoose = require('mongoose');
mongoose.set('debug', true);

const GITHUB_ISSUE = `gh9319`;
const connectionString = `mongodb://localhost:27017/${GITHUB_ISSUE}`;
const { Schema } = mongoose;

run()
  .then(() => console.log('done'))
  .catch(error => console.error(error.stack))
  .then(() => process.exit(0));

async function run() {
  await mongoose.connect(connectionString, { useNewUrlParser: true, useUnifiedTopology: true });

  await mongoose.connection.dropDatabase();

  const childSchema = new Schema({
    field: {
      type: String,
      alias: '_id'        // <--------┐
    }                     //          ├------ important
  }, {                    //          |
    _id: false            // <--------┘
  });

  const parentSchema = new Schema({
    children: [childSchema]
  });

  const Parent = mongoose.model('Parent', parentSchema);


  // Step 1. Create parent with children and find it.
  await Parent.create({
    children: [
      { field: '1' }
    ]
  });
  // Mongoose: parents.insertOne({ _id: ObjectId("5f3395ecd2702ec3f4f70fc4"), children: [ { field: '1' } ], __v: 0}, { session: null })

  // Step 2. Find the parent just created
  const parent = await Parent.findOne();
  // Mongoose: parents.findOne({}, { projection: {} })

  console.log(parent);  // { _id: 5f3395ecd2702ec3f4f70fc4, children: [ { field: '1' } ], __v: 0 }

  // It's possible to find child by id
  let child = parent.children.id('1');

  console.log(child); // { field: '1' }


  // Step 3. Pull (remove) child by id
  parent.children.pull('1');
  // There will be same final result (and debug log `Mongoose: parents.updateOne(...)`) if use one of the following:
  //   parent.children.pull({_id: '1'})
  // or
  //   parent.children.pull({field: '1'})
  //
  // But
    // child.remove(); // throws new Error('For your own good, Mongoose does not know how to remove an EmbeddedDocument that has no _id')



  // Now `children` is empty array
  console.log(parent); // { _id: 5f3395ecd2702ec3f4f70fc4, children: [], __v: 0 }

  // And `child` is null
  child = parent.children.id('1');

  console.log(child); // null


  // Step 4. Save parent and find it again
  const parent_1 = await parent.save();
  // Mongoose: parents.updateOne({ _id: ObjectId("5f3395ecd2702ec3f4f70fc4") }, { '$pull': { children: { _id: { '$in': [ '1' ] } } }, '$inc': { __v: 1 }}, { session: undefined })
  //                                                                                                      ^
  //                                                                                                  Look here

  // `parent_1` equals `parent`
  console.log(parent_1); // { _id: 5f3395ecd2702ec3f4f70fc4, children: [], __v: 1 }


  const parent_2 = await Parent.findOne();
  // Mongoose: parents.findOne({}, { projection: {} })

  // Theoretically `parent_1` and `parent_2` should be equal, but they are NOT.
  console.log(parent_2); // { _id: 5f3395ecd2702ec3f4f70fc4, children: [ { field: '1' } ], __v: 1 }


  await mongoose.disconnect();
}

There problem is here.

What is the expected behavior?
Objects parent, parent_1 and parent_2 should be equal.

Possible solutions:

Solution 0 (just hotfix)
Change this line

        return v._id || v;

with

        return v._doc._id || v;

Solution 1 (bad)
Make .pull to mark all array as modified like .shift(), .pop() etc.

Solution 2 (not so bad)
Make .pull to mark all array as modified if last element was removed.

Solution 3 (better)
Make .pull to understand that pathType of _id is not real but virtual (or maybe new one alias) and should use smth like Model.translateAliases()

Solution 4 (also not bad)
Allow to set option _id with field alias for Subschemas. Example:

const childSchema = new Schema({
  field: {
    type: String,
  }
}, {
  _id: 'field'
});

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.

Node.js: v14.8.0
Mongoose: v5.9.28
MongoDB: v4.4.0
@Kamikadze4GAME
Copy link
Contributor Author

@vkarpov15 sorry for mention, but did you check this bug?

@Kamikadze4GAME Kamikadze4GAME changed the title Wrong object saving after pulling embedded object Wrong document updating after pulling embedded object with _id field alias Aug 25, 2020
@vkarpov15 vkarpov15 added this to the 5.10.2 milestone Aug 26, 2020
@vkarpov15
Copy link
Collaborator

Thanks for the detailed repro and investigation. We're going to go with solution 0, I think that's more consistent with our existing behavior. We currently don't automatically call translateAliases() internally, so I think we should treat parent.children.pull('1') as parent.children.pull({ field: 1 }) in this case, because we still end up casting '1' to { field : 1 }.

vkarpov15 added a commit that referenced this issue Aug 27, 2020
@vkarpov15 vkarpov15 added the confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. label Aug 27, 2020
@Kamikadze4GAME
Copy link
Contributor Author

@vkarpov15 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Projects
None yet
Development

No branches or pull requests

2 participants