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

Document.array.pull() does not seem to work for populated arrays when pulling an _id string #1635

Closed
JedWatson opened this issue Aug 13, 2013 · 8 comments
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@JedWatson
Copy link
Contributor

I have the following function designed to update the ObjectIds stored in a given document doc, at path, to match those provided in a form submission data by using atomic push and pull functionality.

(ObjectIds are stored in an array like this: schema.add({ myPath: [{type: ObjectId, ref: 'OtherSchema'}] });

function(doc, data, path) {

    var storedValue = doc.populated(path),
        arr = doc.get(path),
        _old = storedValue.map(function(i) { return String(i) }),
        _new = _.compact(data[path].split(','));

    console.log('Stored Value:');
    console.log(storedValue)

    console.log('Data at Path:');
    console.log(arr);

    console.log('Remove:');
    console.log(_.difference(_old, _new));

    // remove ids
    _.difference(_old, _new).forEach(function(val) {
        console.log('pulling ' + val);
        arr.pull(val);
    });

    console.log('Add:');
    console.log(_.difference(_new, _old));

    // add new ids
    _.difference(_new, _old).forEach(function(val) {
        console.log('pushing ' + val);
        arr.push(val);
    });

    console.log('Updated Value:');
    console.log(arr);

}

If the doc has already had path populated, two unexpected behaviours occur:

  1. ObjectIds aren't removed
  2. ObjectIds that are added exist in a half-populated state

Console.log output demonstrating both in the above function:

Stored:
[ 51f9fe338507730000000004 ]
Item Data:
[ { name: 'Jimmy',
  _id: 51f9fe338507730000000004 } ]
Remove:
[ '51f9fe338507730000000004' ]
pulling 51f9fe338507730000000004
Add:
[ '51f9fe338507730000000007' ]
pushing 51f9fe338507730000000007
Updated:
[ { name: 'Jimmy',
  _id: 51f9fe338507730000000004 }, { _id: 51f9fe338507730000000007 } ]

re: 1, this seems like a bug. The documentation here implies that you should be able to remove ids from an array, although it doesn't mention what happens if the array has been populated.

re: 2, as you can see the newly added object looks populated (is an object) but hasn't been loaded from the database yet (so has no name property). What's the best way to handle this? Repopulate the path after saving the document?

It might be best in my particular case to specifically detect that the path being updated has been populated, unpopulate it, make the atomic changes to the array, then repopulate the path after the document has been saved. There doesn't seem to be any api to unpopulate a path, and I don't want to blindly do this:

document.set(path, document.populated(path));

... because I think that would cause the document to think that the entire value of path has been modified.

Any hints on how to best make all this work would be great, Cheers.

@aheckmann
Copy link
Collaborator

  1. yes behavior is unspecified for populated arrays. we should be able to handle this though.
  2. since the array has been populated, values are cast to documents to keep types in that array consistent.

You are correct, you probably don't want to set the entire array and overwrite the entire thing.

Currently, re-populating an array modified under these circumstances is not working. Work arounds

  • manually query for the documents directly
  • do not populate until after adding/removing the array elements, then use doc.populate('path', callback)

@hongkongkiwi
Copy link

This is still an issue in 3.8.13... It would be great if there was an doc.unpopulate(path); method.

@gregwym
Copy link

gregwym commented Aug 11, 2014

+1 for this issue. Have to re-query for new doc instance every time.

@arunsivasankaran
Copy link

+1 for this, looking it up again is inelegant.

@vkarpov15 vkarpov15 added this to the 3.8.19 milestone Oct 17, 2014
@vkarpov15 vkarpov15 modified the milestones: 3.8.20, 3.8.19 Nov 7, 2014
@vkarpov15 vkarpov15 modified the milestones: 3.8.21, 3.8.20 Dec 5, 2014
@vkarpov15 vkarpov15 added this to the 3.8.28 milestone May 10, 2015
@vkarpov15 vkarpov15 modified the milestones: 3.8.29, 3.8.28 May 12, 2015
@vkarpov15 vkarpov15 modified the milestones: 3.8.30, 3.8.29 May 27, 2015
@vkarpov15 vkarpov15 modified the milestones: 3.8.31, 3.8.30 Jun 5, 2015
@vkarpov15 vkarpov15 removed this from the 3.8.31 milestone Jun 16, 2015
@ggepenyan
Copy link

+1

@ggepenyan
Copy link

3 years passed ))

@lineus
Copy link
Collaborator

lineus commented May 10, 2020

point 1 above ( the removal of _ids ) has already been addressed and works as expected here.
point 2 above feels too much like magic to me, @vkarpov15 when adding new docs to a previously populated array, do we want to automatically query the db for the referenced docs?

side note, the request above for doc.unpopulate() has been fulfilled as well, it is doc.depopulate()

@vkarpov15 vkarpov15 added this to the 8.5.3 milestone Jul 23, 2024
@vkarpov15
Copy link
Collaborator

Atomic push and pull isn't possible with MongoDB server in general, consider the following example run with mongo shell. $pull and $push on same array leads to a write conflict error.

MongoDB Enterprise > db.test.updateOne({}, { $push: { children: 'taco' }, $pull: { children: 'luke' } })
WriteError({
	"index" : 0,
	"code" : 40,
	"errmsg" : "Updating the path 'children' would create a conflict at 'children'",
	"op" : {
		"q" : {
			
		},
		"u" : {
			"$push" : {
				"children" : "taco"
			},
			"$pull" : {
				"children" : "luke"
			}
		},
		"multi" : false,
		"upsert" : false
	}
}) :
WriteError({
	"index" : 0,
	"code" : 40,
	"errmsg" : "Updating the path 'children' would create a conflict at 'children'",
	"op" : {
		"q" : {
			
		},
		"u" : {
			"$push" : {
				"children" : "taco"
			},
			"$pull" : {
				"children" : "luke"
			}
		},
		"multi" : false,
		"upsert" : false
	}
})

However, pushing a non-populated element does seem to still cause the half-populated document issue:

'use strict';

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

run().catch(err => {
  console.error(err);
  process.exit(-1);
});

async function run() {
  await mongoose.connect('mongodb://127.0.0.1:27017/mongoose_test');

  const ParentModel = mongoose.model('Test', mongoose.Schema({
    name: String,
    children: [{ type: 'ObjectId', ref: 'Child' }]
  }));
  const ChildModel = mongoose.model('Child', mongoose.Schema({ name: String }));

  const children = await ChildModel.create([{ name: 'Luke' }, { name: 'Leia' }]);
  const newChild = await ChildModel.create({ name: 'Taco' });
  const { _id } = await ParentModel.create({ name: 'Anakin', children });

  const doc = await ParentModel.findById(_id).populate('children');
  doc.children.pull(children[0]._id);
  doc.children.push(newChild._id);
  // 2nd element is a doc with just an `_id`
  console.log(doc.children);
  await doc.save();

  const fromDb = await ParentModel.findById(_id);
  console.log(fromDb.children);
}

We will likely just depopulate the whole array if pushing a plain ObjectId onto a populated array.

vkarpov15 added a commit that referenced this issue Aug 13, 2024
fix(document): make sure depopulate does not convert hydrated arrays to vanilla arrays
@vkarpov15 vkarpov15 modified the milestones: 8.5.3, 8.5.4 Aug 13, 2024
@vkarpov15 vkarpov15 removed this from the 8.5.4 milestone Aug 23, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.5.5, 8.6.1 Aug 23, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.6.1, 8.6.2 Sep 3, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.6.2, 8.7 Sep 10, 2024
vkarpov15 added a commit that referenced this issue Sep 11, 2024
fix: depopulate if `push()` or `addToSet()` with an ObjectId on a populated array
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

8 participants