JackAdams / meteor-transactions

App level transactions for Meteor + Mongo
http://transactions.taonova.com/
MIT License
113 stars 16 forks source link

Support for MongoDB's positional operator in update #52

Open fjorgemota opened 8 years ago

fjorgemota commented 8 years ago

Hello!

I think that it's a good enhacement for this package, but I have no idea of how to implement it actually. See:

Example

Suppose you have a document in a collection items with the following format:

{
    "_id": "a",
    "sub_items": [{
        "id": "b",
        "freq": 0
    }, {
       "id": "c",
       "freq": 2
    }]
}

Suppose you want to increment the sub-field "freq" of the first item of the sub_itemsarray so it's value will be 5. In MongoDB (and in Meteor, too), you can execute:

Items.update({
     _id: "a",
     "sub_items.id": "b"
}, {
    $inc: {
        "sub_items.$.freq": 5
    }
})

And the document will turn into something like:

{
    "_id": "a",
    "sub_items": [{
        "id": "b",
        "freq": 5
    }, {
       "id": "c",
       "freq": 2
    }]
}

That's because of the positional operator (represented as $ in the update doc), which allows MongoDB to detect the exact index of the sub-document in the array and "replace" it in the update doc, so:

Items.update({
     _id: "a",
     "sub_items.id": "b"
}, {
    $inc: {
        "sub_items.$.freq": 5
    }
})

Becomes, for MongoDB:

Items.update({
     _id: "a",
     "sub_items.id": "b"
}, {
    $inc: {
        "sub_items.0.freq": 5
    }
})

Being 0 the index of the sub-document in sub_documents in the array.

Problem

Now, the problem: If I run:

Items.update({
     _id: "a",
     "sub_items.id": "b"
}, {
    $inc: {
        "sub_items.0.freq": 5
    }
}, {
    tx: true
})

In Meteor, the following log is printed:

I20160229-17:50:17.733(-3)? Started "update item" with transaction_id: ZYwRQfSa9ALtvPc6G (auto started)
I20160229-17:50:17.733(-3)? Pushed update command to stack: ZYwRQfSa9ALtvPc6G
I20160229-17:50:17.733(-3)? Auto committed: ZYwRQfSa9ALtvPc6G
I20160229-17:50:17.733(-3)? Beginning commit with transaction_id: ZYwRQfSa9ALtvPc6G
I20160229-17:50:17.746(-3)? Executed update
I20160229-17:50:17.749(-3)? { [MongoError: The positional operator did not find the match needed from the query. Unexpanded update: sub_items.$.freq]
I20160229-17:50:17.749(-3)?   name: 'MongoError',
I20160229-17:50:17.750(-3)?   code: 16837,
I20160229-17:50:17.750(-3)?   err: 'The positional operator did not find the match needed from the query. Unexpanded update: sub_items.$.freq' }
I20160229-17:50:17.750(-3)? Transaction failed
I20160229-17:50:17.752(-3)? { [MongoError: The positional operator did not find the match needed from the query. Unexpanded update: sub_items.$.freq] stack: [Getter] }
I20160229-17:50:17.753(-3)? Rollback failed -- you'll need to check your database manually for corrupted records.
I20160229-17:50:17.753(-3)? Here is a log of the actions that were tried and their inverses:
I20160229-17:50:17.753(-3)? (it was probably one of the inverse actions that caused the problem here)
I20160229-17:50:17.754(-3)? [{"collection":"items","_id":"7WyKD2ZFuxfEPmYvr","action":"update","state":"done","noCheck":true,"update":{"command":"$inc","data":[{"key":"sub_items.$.freq","value":5}]},"inverse":{"command":"$unset","data":[{"key":"sub_items.$.freq","value":1}]}}]
I20160229-17:50:17.758(-3)? Rollback reset transaction manager to clean state

(btw, it's strange the "inverse" command being an $unset (as the command is an $inc, it should not be a $set or $inc with negative value?) here..)

And it's have a known cause: Meteor Transactions picks just _id in the update query, and so MongoDB does not have a way to discover which sub-document in the array it should update.

Maybe we should detect positional operators in the update doc and so allow it's properties used in query (such as sub_items.id, in that example) to be used in the query, too?

What do you think?

Thanks. :)

JackAdams commented 8 years ago

Yes, you're right. That inverse is weird. I'll look into that.

The fact that the transaction code just picks up the _id field shouldn't affect the update part -- as long as the right document is there, the positional operator should be able to do its thing.

For instance:

Items.update({
     _id: "a",
     "sub_items.id": "b"
}, {
    $inc: {
        "sub_items.0.freq": 5
    }
})

should give an identical result to:

Items.update({
     _id: "a"
}, {
    $inc: {
        "sub_items.0.freq": 5
    }
})

[EDIT - No, it shouldn't - see correction of my wrongheadedness in comments below]

The first hash is just about getting the right document to operate on. The second hash is the one that does the updates.

All this said, yes, there's obviously a problem with the update logic. I'll try to figure out why it's not just working out of the box (as it should).

fjorgemota commented 8 years ago

Maybe I have not explained right. But, the query in question is:

Items.update({
     _id: "a",
     "sub_items.id": "b"
}, {
    $inc: {
        "sub_items.$.freq": 5
    }
})

This is the real query. the sub_items.0.freq is just what MongoDB understands by finding that sub_items.id = "a" is in the index 0 of the sub_items array.

More information about the positional operator can be found here.

JackAdams commented 8 years ago

Haha... you're absolutely right. I wasn't thinking clearly about how the positional operator works.

This is going to be difficult to support in the transactions package without a substantial refactor of its inner workings, so no promises about this. I do have plans to improve support for mongo operators, but this is one of the trickier ones.

I appreciate the time you put into investigating this issue so thoroughly.

fjorgemota commented 8 years ago

No problem.

As I said before, I think that it's a good enhacement for this package, but I have no idea of how to implement it actually.. So just feel free to tag it as an enhancement issue for the future. :P

But I'm yet find strange the inverse of the operation created. It worth a look. :)

JackAdams commented 8 years ago

Yes, I'll certainly take a look at that weird inverse. I really need to write a bunch more tests for this package -- coverage is spotty at best ...

adammoisa commented 8 years ago

Hi has there been any solution for this? Is there a way to mimic the positional operator until support for it is added?

JackAdams commented 8 years ago

No, there hasn't. Sorry. At the moment, what I'd do is retrieve the array, manipulate it in app code (probably using underscore or lodash), then overwrite the whole array with an update using the $set operator. Not cool, I know.

@fjorgemota is right, this would be a good enhancement for the package and is kind of a glaring omission at the moment.

That said, with a 3 day old baby, I'm not sure I'm going to have time to make the necessary adjustments anytime soon.

adammoisa commented 8 years ago

@JackAdams @fjorgemota

I figured out an alternate solution that IMHO might be a bit cleaner...

I use loadash to determine the arrayIndex of the object I want to update in the array. Then I create a custom index updater object and pass it to $addToSet which adds it only if it isn't there. In example:

var array = [{key: "value"},{key: "value2"}];
var _id = an_id;

var document = Collection.findOne({_id: an_id, "array.key" : "value"});
var index = _.findIndex(document.array, {key: "value"});

var customUpdateObj = {};
//Set custom query object with an int instead of a $ which is supported by minimongo
customUpdateObj["ingredientDetails." + index + ".stores"] = e;

Collection.update({_id: an_id, "array.key" : "value"},
    {
        //Only add to array in object if doesnt exist. If want to insert no matter what, use $push
        $addToSet: customUpdateObj
    }
)
JackAdams commented 8 years ago

Oh nice. Yeah, that's better and would totally work. In fact, that sounds familiar -- I've got a feeling I might have done that in my code in a few places over the years.

Leekao commented 5 years ago

So... no progress?

linksach3 commented 5 years ago

It still doesn't work for me, any update?

JackAdams commented 5 years ago

No, sorry. Work on this package is back-burnered for the time being. Too many other things taking priority at the moment. I've always got time to review and merge a good PR though. :-)