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

Shuffling composite _id key issue #4542

Closed
Nainterceptor opened this issue Sep 19, 2016 · 2 comments
Closed

Shuffling composite _id key issue #4542

Nainterceptor opened this issue Sep 19, 2016 · 2 comments
Milestone

Comments

@Nainterceptor
Copy link
Contributor

Hello,

Issue : When I'm saving an object with a composite _id, update will be silently ignored.

Proof of the issue :

const co = require('co');
const mongoose = require('mongoose');

mongoose.Promise = global.Promise;
const { Schema } = mongoose;

co(function*() {
    yield mongoose.connect('mongodb://localhost/test');
    console.log('foo');
    const schema = new Schema({
        _id: {
            key1: String,
            key2: String,
        },
        content: String,
    });

    const Model = mongoose.model('test', schema);

    let object = new Model();
    object._id = {key1: "foo", key2: "bar"}; //Saved
    object = yield object.save();
    object.content = 'Hello';
    object = yield object.save(); //Return object with right content, but not saved
    console.log(object);
}).catch(e => console.log(e));

Investigating :
Sorting of keys in the ID is not insured, so {key1, key2} will find the doc, but {key2,key1} will match with 0 docs.
When Mongoose call update, (btw, update is deprecated now, please replace by updateOne) where may be with the bad order for keys.
On the FindOne, _id object may be shuffled. So, on update, bad sorted _id may be embed into the updateOne, so the save() will fail. (Return 0 documents modified, but no error throwed).

Workaround :
I'm using a workaround, but with a potential performance issue (not benchmarked), I've overrided the Model of mongoose, to decompose _id key. {_id: {key1, key2}} will become {"_id.key1", "_id.key2"} With this syntax, order is not important.

To override model, please see PR #4541, then you can do something like that (findById overrided too, for example) :

const co = require('co');
const mongoose = require('mongoose');
const MongooseModel = require('mongoose/lib/model');

mongoose.Promise = global.Promise;
const { Schema } = mongoose;

const oldWhere = MongooseModel.prototype.$__where;

function decompositionOfKeys(root, object) {
    const criteria = {};
    const compoundIdKeys = Object.keys(object);
    for (const compoundIdKey of compoundIdKeys) {
        criteria[[root, compoundIdKey].join('.')] = object[compoundIdKey];
    }
    return criteria;
}

MongooseModel.prototype.$__where = function __where(where) {
    where = oldWhere.call(this, where);
    if (where._id && !where._id.str) {
        const id = where._id;
        delete where._id;
        Object.assign(where, decompositionOfKeys('_id', id));
    }
    return where;
};

MongooseModel.prototype.findById = function findById(id, ...rest) {
    const criteria = {};
    if (id.str) {
        criteria._id = id;
    } else {
        Object.assign(criteria, decompositionOfKeys('_id', id));
    }
    return this.findOne(criteria, ...rest);
};

mongoose.Model = MongooseModel;

co(function*() {
    yield mongoose.connect('mongodb://localhost/test');
    const schema = new Schema({
        _id: {
            key1: String,
            key2: String,
        },
        content: String,
    });

    const Model = mongoose.model('test', schema);

    let object = new Model();
    object._id = {key1: "foo", key2: "bar"}; //Saved
    object = yield object.save();
    object.content = 'Hello';
    object = yield object.save(); //Return object with right content, and is saved
    console.log(object);
}).catch(e => console.log(e));

Have a nice day,
Gaël

@vkarpov15
Copy link
Collaborator

Good catch! Will fix this for next release

@vkarpov15
Copy link
Collaborator

So looking into this, this is actually a very difficult problem tied into #2749, my least favorite legacy mongoose feature. Fixing is gonna take another week or two, thanks for your patience 🍻 🌴

@vkarpov15 vkarpov15 modified the milestones: 4.6.3, 4.6.2, 4.6.4 Oct 1, 2016
vkarpov15 added a commit that referenced this issue Oct 16, 2016
Conflicts:
	lib/schema/embedded.js
vkarpov15 added a commit that referenced this issue Oct 16, 2016
vkarpov15 added a commit that referenced this issue Oct 16, 2016
vkarpov15 added a commit that referenced this issue Oct 17, 2016
tusbar added a commit to IONISx/edx-modulestore-node that referenced this issue Oct 25, 2016
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

No branches or pull requests

2 participants