JackAdams / meteor-transactions

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

Update fails when collection contains object element, and rollback not done #38

Open tab00 opened 9 years ago

tab00 commented 9 years ago

I have a collection that contains an element of type 3stack:bignumber.

When { tx: true } is set in the update call, the error "Amount as bn must be a BigNumber" appears. The other update call executes successfully and is not rolled back to the pre-transaction data.

Without { tx: true }, both update calls execute successfully without errors.

I think the problem may occur for any Object element, not just BigNumber, as changing the type to "Object" produced the same kind of error.

Here is the executing code:

    Meteor.startup(function () {
        CollectionA.remove({});
        CollectionB.remove({});

        var userId = "Xis7MYQBJenbdjj5r";

        var holdingId = CollectionA.insert({ userId: userId, amountAsBN: new BigNumber(0) });
        var depositId = CollectionB.insert({ userId: userId, status: "P" });

        var amountAsBN = new BigNumber(0.0012345);

        console.log("CollectionA: ", CollectionA.findOne());
        console.log("CollectionB: ", CollectionB.findOne());

        tx.start("update");

        CollectionB.update({ _id: depositId }, { $set: { status: "E" } }, { tx: true });
        CollectionA.update({ _id: holdingId }, { $set: { amountAsBN: amountAsBN } }, { tx: true });

        tx.commit();

        console.log("CollectionA: ", CollectionA.findOne());
        console.log("CollectionB: ", CollectionB.findOne());
    });

Here are the collection and schema definitions:

Schemas = {};

//CollectionA
{
    CollectionA = new Mongo.Collection("collectionA");

    Schemas.ItemA = new SimpleSchema({
        userId: {
            type: String,
            regEx: SimpleSchema.RegEx.Id,
        },
        amountAsBN: {
            type: BigNumber,
        },
    });

    CollectionA.attachSchema(Schemas.ItemA);
}

//CollectionB
{
    CollectionB = new Mongo.Collection("collectionB");

    Schemas.ItemB = new SimpleSchema({
        userId: {
            type: String,
            regEx: SimpleSchema.RegEx.Id,
        },
        status: {
            type: String,
        },
    });

    CollectionB.attachSchema(Schemas.ItemB);
}
JackAdams commented 9 years ago

Thanks for bringing this up. I see two issues: 1) If an error is thrown, everything in the transaction should be rolled back 2) Types aren't being passed through the transactions code properly Both of these are pretty bad. I'll get to work on it asap.

JackAdams commented 9 years ago

Hi @tab00, I haven't forgotten about this. I've managed to reproduce the error using your code sample but haven't had time to roll out a fix. I can see the problem with 1) (above) but haven't isolated the problem causing 2) yet. Will get to this in the next week or so. Sorry about the delay! Brent

JackAdams commented 8 years ago

I've just released 0.7.5 on Atmosphere, which fixes the rollback issue.

The type problem, however, isn't going to be fixed anytime soon. The object isn't a plain object -- it's prototype contains a big bunch of methods that make it a "BigNumber". There's a package-for-storage and unpackaging process that is used on data before doing the db write, so the type is not going to be a "BigNumber" when it comes out the other side of this (it'll just be a plain object) and Collection2 will (quite rightly) throw an error because the wrong type is being used. The degree of refactoring it would take to get this going is a bit outside of my time budget right now, but it's certainly something that should be addressed at some stage.

Normal objects are working fine with the new version.

tab00 commented 8 years ago

(it'll just be a plain object) and Collection2 will (quite rightly) throw an error because the wrong type is being used.

How about simply converting the plain object back to BigNumber? I do this in my code and it works:

    BNFromObj: function (objBigNumber) {
        if (objBigNumber == undefined) return undefined;

        var bn = new BigNumber(NaN);
        bn.c = objBigNumber.c;
        bn.e = objBigNumber.e;
        bn.s = objBigNumber.s;

        return bn;
    },
JackAdams commented 8 years ago

There would need to be a hook at the right stage of the process to do this manually (i.e. using a function like the one above), and this hook doesn't currently exist.

The hook that turns the plain object into the correct type would need to run just before this function returns.

tab00 commented 8 years ago

Would a transform function do the job? I sounds like it should from the descriptions: http://docs.meteor.com/#/full/mongo_collection https://github.com/aldeed/meteor-simple-schema#blackbox

JackAdams commented 8 years ago

No, I'm pretty sure a transform function wouldn't help in this case. The code needs to be refactored so that it records the type when the mutation data is "packaged for storage" and then triggers the correct custom, user-defined transformation (based on the type that was recorded) when the mutation data is "unpackaged".