Bug in Model.update (?)

[ORESoftware]

I have this straightforward query:

var conditions = { userId: 20 };
var updateData = { "$push" : { following: 48 } };
var opts =  {overwrite: true, safe:true, upsert:false, multi:false};

    Model.update(conditions, updateData, opts, function (err, result) {

        if (err) {
            next(err);
        }
        else if (result) {
            res.temp = {
                data: result,
                status: 200
            };
            next();
        }
        else {
            next(new Error('grave error in model.save method'));
        }
    });

no error is thrown and the result is returned, but the query is not executing the right thing -

using this

mongoose.set('debug', true, function (coll, method, query, doc) {
    log.debug('query executed:', coll, method, query, doc);
});

i see that the query is in fact executing this:

Mongoose: users.update({userId:20, {},  {overwrite: true, safe:true, upsert:false, multi:false})

so the second argument is an empty object, but I am 100% certain that the correct data is being passed to that update call.

What could be going wrong? Thanks

[ORESoftware]

I tried this on multiple machines, problem seems to exist

[ORESoftware]

I tried the same query with

https://github.com/mongodb/node-mongodb-native

like so:

    var collection = db.collection('users');

    collection.updateOne(conditions, updateData, opts, function (err, result) {

            if (err) {
                next(err);
            }
            else if (result) {
                res.temp = {
                    data: result,
                    status: 200
                };
                next();
            }
            else {
                next(new Error('grave error in model.save method'));
            }

        });

and it works, so not really sure what is going on

[mleanos]

@ORESoftware Can you share your schema? Also, does the update perform as you'd expect, if you remove the opts parameter?

Lastly, could you explain what you're trying achieve with the specified opts?

I'm trying to debug this issue, and I need a little more context.

[ORESoftware]

thanks

here is the schema:

var mongoose = require('mongoose');
var bcrypt = require('bcrypt-nodejs');
var autoIncrement = require('mongoose-auto-increment');
var imagePlugin = require('./plugins/image');
var _ = require('underscore');
var regexp = require('../services/regexp');
var FormValidation = require('../services/form-validation');
var completions = require('./plugins/completions');
var publicJSON = require('./plugins/public-json');

var topicSchema = mongoose.Schema({

    topicId: Number,

    name: String,

    hashtag: String,

    image: {
        large: String,
        medium: String,
        small: String
    },

    followers: [],

    creatorId: Number,

    dateCreated: {
        type: Date,
        default: Date.now()
    },

    dateUpdated: {
        type: Date,
        default: Date.now()
    },

    createdBy: Number,
    updatedBy: Number

});

topicSchema.methods = {};

topicSchema.statics = {
    getIdName: function getIdName() {
        return 'topicId';
    },
    getCollectionName: function getCollectionName() {
        return 'topics';
    }
};

topicSchema.plugin(autoIncrement.plugin, {model: 'Topic', field: 'topicId', startAt: 1});
topicSchema.plugin(completions, {id: 'topicId', handle: 'hashtag', searchOn: ['hashtag', 'name']});
topicSchema.plugin(publicJSON, {multisetFields: ['image', 'name', 'hashtag']});
module.exports = mongoose.model('Topic', topicSchema);

the opts I am passing in are like so:

var _ = require('underscore');

module.exports = function httpRequestMiddleware$PUT(req, res, next) {

    var data = req.lectalApiData;

    var Model = data.Model;
    var conditions = data.conditions || {};
    var count = data.limit || 0;
    var isLean = data.lean !== false;  //default value for isLean to true
    var id = data.id;
    var updateData = data.updateData;
    var options = data.options || {};

    if (Object.keys(updateData).length < 1) {
        return next(new Error('no body in PUT request'));
    }

    var opts = _.defaults(options, {
        upsert: false,
        multi: false, //By default, the update() method updates a single document. Set the Multi Parameter to update all documents that match the query criteria.
        safe: true,
        overwrite: true
    });

    console.log('conditions', conditions); // I debugged and also logged the values
    console.log('updateData', updateData);

    //Model.update(conditions, updateData, opts).exec(function (err, result) {  //mongoose debug set to true does not get invoked when using exec() ? so we use the below

    Model.update(conditions, updateData, opts, function (err, result) {

        if (err) {
            next(err);
        }
        else if (result) {
            res.temp = {
                data: result,
                status: 200
            };
            next();
        }
        else {
            next(new Error('grave error in model.save method'));
        }
    });

};

I call the above with this:

router.put('/follow/by_id/:id', function (req, res, next) {

    var user = req.user || req.guestUser;
    var userId = user.userId;
    var topicId = parseInt(req.params.id);

    var conditions = {
        'topicId': topicId,
        'following.userId': {
            '$ne': userId
        }
    };

    var data = {
        '$push': {
            'following': {
                'dateFollowed': new Date(),
                'userId': userId
            }
        }
    };

    req.lectalApiData = {
        Model: Topic,
        conditions: conditions,
        updateData: data,
        lean: true,
        count: 1,
        options: {
            multi: false
        }
    };

    next();

}, httpMiddleware.PUT_MODEL);

also, I checked and am on the latest version of Mongoose

[ORESoftware]

I ran the code on my local machine and also pushed to heroku and ran it from their servers and both installations of mongoose / mongo had the same problem, and furthermore the require('mongodb') module works, so I feel it must be some bug. I am happy to work with you as much as needed to get it figured it out

[mleanos]

@ORESoftware The update query looks incorrect to me. It's trying to $push to field called following, which doesn't appear to exist based on your Schema.

Try changing the $push query to use "followers".

[vkarpov15]

@mleanos is right, mongoose will convert it to an empty object by default because followers is not in the schema. If you really mean to push to an array that isn't in your schema, set the strict option to false. See Update API docs and notes on the strict option

[ORESoftware]

ok thanks guys, I am sure you're right, will double check. however, isn't bad to convert it to an empty object? What this does, of course, is to delete entire records from the DB...isn't this bad? Shouldn't it just fail fast with a error message instead of doing some default behavior which is quite dangerous??

[ORESoftware]

sorry to say it, I am still seeing the same problem. the mismatched property on the schema was not the underlying issue, because this same problem was actually also happening for a different schema that I have - the example I provided was not the only model for which this issue was happening.

how else can we figure out what the problem is? I could walk through mongoose, but I am fearful that I would hit some C/C++ quickly.

[mleanos]

@ORESoftware I wrote a simple test that mimics your use case. I found that the update seems to have undesired behavior when you pass in overwrite: true as an option. Everything seems to function properly when I remove the overwrite option. Is there a particular reason why you're using this option? To me, it doesn't seem like an appropriate use with a $push update; just not something I would do. It appears to be removing all data, other than the _id

With that said, I'm still wondering why exactly this behavior occurs when using the overwrite option.

Does anyone have any insight into why this may be happening? I haven't dug deep into what the overwrite option is doing in cases like this. When I change the update to a simple $set operation, and use the overwrite option, everything works as expected.

My rudimentary test is below:

it('handles $push update (gh-3546)', function(done) {
    var db = start();

    var schema = mongoose.Schema({
      topicId: Number,
      name: String,
      followers: []
    });

    var follower1 = {
      userId: 500,
      dateFollowed: Date.now()
    };

    var follower2 = {
      userId: 200,
      dateFolled: Date.now()
    };

    var doc = {
      topicId: 100,
      name: 'name_' + random(), 
      followers: [follower1]
    };

    var M = db.model('gh-3546', schema, 'gh-3546_' + random());

    M.create(doc, function (err, topic) {
      console.log('new topic: ', topic);
      assert.ifError(err);

      var conditions = {
        'topicId': doc.topicId,
        'followers': {
          '$nin': [follower2.userId]
        }
      };

      var update = { $push : { followers: follower2 } };

      var opts =  { /* overwrite: true, */ safe: true, upsert: false, multi: false};

      M.update(conditions, update, opts, function (err, response) {
        console.log('update response: ', response);
        assert.ifError(err);

        M.findById(topic._id, function (err, updated) {
          console.log('updated topic: ', updated);
          assert.ifError(err);

          assert.equal(2, updated.followers.length);
          assert.equal(follower1.userId, updated.followers[0].userId);
          assert.equal(follower2.userId, updated.followers[1].userId);

          db.close(done);
        });
      })
    });
  });

[ORESoftware]

Thanks - no idea what overwrite:true really means - I have looked at the Mongo / Mongoose docs many times and never found any documentation for it - it's in my code to simply document that the option exists

Perhaps the purpose of overwrite true is to actually do the behavior that we are seeing. For example, If you look at the mongoose debug output, it is actually running

Model.update({someId:someValue, {}, {overwrite:true, ...etc})

so it clearly showing us that it's "overwriting" everything - essentially a delete which is really bad

[vkarpov15]

@mleanos thanks for figuring out the issue, he's right. overwrite: true means overwriting the entire doc in the db, which is how mongodb updates work by default. Overwrite is false by default to make it easier to avoid shooting yourself in the foot with this behavior.

[mleanos]

@vkarpov15 Thank you for clarifying that. I'm curious as to why we see different behavior when a $set operation is used with overwrite: true; the field gets updated correctly, and the other fields are not removed.

I would expect a $push operation to properly add to the array, even when the overwrite option is used.

[ORESoftware]

note: overwrite:true does not yield the same results using require('mongodb') so there is something incongruous.

in other words, I have been using the same overwrite:true option with the mongodb lib and it hasn't been deleting my data. I'd prefer to keep using Mongoose than using mongodb module.

[vkarpov15]

overwrite is a mongoose-specific option. The MongoDB driver exposes the default mongodb behavior, which is "overwrite by default unless you specify a key that starts with $".

Basically, don't specify the overwrite option unless you explicitly mean that you're going to overwrite the whole document. I think the name makes that sufficiently clear.

[ORESoftware]

the name is clear enough, in retrospect :)

[mleanos]

@ORESoftware I suggest that you don't use the overwrite option for your described use case. It isn't needed.

@vkarpov15 I think I understand the intention of the overwrite option. However, I still can't see a case where this would ever be useful. Can you describe such a scenario?

As to the behavior that I'm seeing, it's not consistent with what the name is conveying. Nor, is it consistent with the various update operations.

The MongoDB driver exposes the default mongodb behavior, which is "overwrite by default unless you specify a key that starts with $".

If I'm understanding this correctly, a "key" in this context is something like $set, $addToSet, $push, $pull, etc. Is that right?

What has me mostly confused is the difference in the behavior that I'm seeing when I use a $set operation vs a $push, with the update method. The former updates the field correctly, and leaves everything else in the document in tact; the latter doesn't perform the push, and it removes all data from the document (except the _id field).

What originally peaked my interest in this issue, is due to an issue I was having some time ago with the $push operation. So I wanted to see if it was related. I don't believe it is. However, I'm thinking I have found an issue here. Take the below two examples..

Updates the name field & leaves all other data in tact.

it('handles $push update (gh-3546)', function(done) {
    var db = start();

    var schema = mongoose.Schema({
      topicId: Number,
      name: String,
      followers: [Number]
    });

    var doc = {
      topicId: 100,
      name: 'name_' + random(), 
      followers: [500]
    };

    var M = db.model('gh-3546', schema, 'gh-3546_' + random());

    M.create(doc, function (err, topic) {
      assert.ifError(err);

      var update = { $set: { name: 'blahblah' } };
      var opts =  { overwrite: true, new: true, safe: true, upsert: false, multi: false};

      M.update({ topicId: doc.topicId }, update, opts, function (err, response) {
        assert.ifError(err);

        db.close(done);
      })
    });
  });

Doesn't perform $push, and clears out all data in the document (except for _id).

it('handles $push update (gh-3546)', function(done) {
    var db = start();

    var schema = mongoose.Schema({
      topicId: Number,
      name: String,
      followers: [Number]
    });

    var doc = {
      topicId: 100,
      name: 'name_' + random(), 
      followers: [500]
    };

    var M = db.model('gh-3546', schema, 'gh-3546_' + random());

    M.create(doc, function (err, topic) {
      assert.ifError(err);

      var update = { $push: { followers: 200 } };
      var opts =  { overwrite: true, new: true, safe: true, upsert: false, multi: false};

      M.update({ topicId: doc.topicId }, update, opts, function (err, response) {
        assert.ifError(err);

        db.close(done);
      })
    });
  });

When I use the $push operation, this line is hit; Thus, it removes the $push key from the update object. https://github.com/Automattic/mongoose/blob/master/lib/query.js#L2340

What's also interesting, is in this case, the op parameter coming into _walkUpdatePath is "$set", and the obj is { '$push': { followers: 200 } }

I know this is a long post, but I'd like to understand what's going on here. Any insight?

[vkarpov15]

Unfortunately that's how mongoose casting for overwrite: true was originally written: https://github.com/Automattic/mongoose/commit/1ff276641d822999e0c2f92cd7fdda214495c4da, and it's basically lived on that way.The general idea is that the only time you set overwrite: true is when you really mean to overwrite the whole document. However, you're right, the current behavior has some quirks - the only reason why the $set case works was a tweak introduced in #2515 and https://github.com/Automattic/mongoose/commit/1bf1e3b16916de2fc5b5da7e0ccae1179636a4a5. I'll open up a new issue to track this.

[vkarpov15]

Fixed re #3564

[mleanos]

@vkarpov15 Thank you! I've tested, and confirm this is fixed.

@ORESoftware Did that solve the issue on your side?