e-oj / Fawn

Transactions for MongoDB (See the README)
https://www.npmjs.com/package/fawn
MIT License
485 stars 54 forks source link

Not rolling back for update and remove (with _id) #34

Closed skarif2 closed 6 years ago

skarif2 commented 7 years ago

I have tried to use Fawn with mongoose. I have a sequence of save, update and update. If the second update fails the save is rolling back but not the first update.

const task = Fawn.Task();
task.save(Deposit, saveDeposit)
  .update(User, { _id: saveUser._id }, { $inc: { balance: amount } })
  .update(RechargeCard, { _id: saveRechargeCard.used }, { used: true })
  .run({ useMongoose: true })
  .then((results) => {
       res.json({
            status: 'success',
            results
       });
   })
   .catch((err) => {
       res.json({
            status: 'error',
            message: err
       });
   });

Is there any problem with Fawn or I'm making any mistake.

skarif2 commented 7 years ago

saveRechargeCard.used is not an ObjectId. I did it intentionally so an error will occur.

e-oj commented 7 years ago

What version of Fawn are you using?

e-oj commented 7 years ago

Also, does it work without mongoose (useMongoose: false)?

skarif2 commented 7 years ago

Using useMongoose: false works but the problem is it returns success on the second update where it should return error cause saveRechargeCard.used is Boolean value not ObjectId and moreover it doesn't update the RechargeCard collection. If I use useMongoose: true and instead of using mongoose model use collection name like'deposits', 'users' etc it does the same thing.

e-oj commented 7 years ago

Alright. What version of Fawn?

skarif2 commented 7 years ago

2.1.0

e-oj commented 7 years ago

Looks like a bug. It has something to do with the objectId being converted to a string. I'll look into it.

e-oj commented 7 years ago

Having a string "id" field for queries would be a temporary fix

skarif2 commented 7 years ago

I think it has something to do with mongoose validation.

const task = Fawn.Task();
task.update(User, { _id: saveUser._id }, { $inc: { balance: amount } })
  .update(RechargeCard, { _id: saveRechargeCard._id }, { used: true })
  .save(Deposit, saveDeposit)
  .run({ useMongoose: true })
  .then((results) => {
       res.json({
            status: 'success',
            results
       });
   })
   .catch((err) => {
       res.json({
            status: 'error',
            message: err
       });
   });

In the above code, I made a schema validation error in saveDeposit object and it returned the error currently but the first two updates didn't rollback, they made permanent change in db.

e-oj commented 7 years ago

Well, what happens is, Fawn uses the update condition to find documents about to be changed, then it stores a copy of the found docs for rollbacks. In the db, _id is stored as an ObjectId, not a string but Fawn converts the _id to a string (all objects passed to a task's functions are converted to json strings) and queries the db for an _id equal to that string. But an ObjectId is not equal to any string, so that query returns 0 matched documents and there's basically no state we can rollback to.

e-oj commented 7 years ago

I was able to reproduce this without validations on the schema. It explains why this affects the remove function as well

skarif2 commented 6 years ago

Hey Emmanuel, is there any estimated date that we are having an update? I need to use Fawn in one of my production application. I'm working on that application day and night so can't help you with a pr. Really sorry for that.

e-oj commented 6 years ago

No problem. It's the last two weeks of the semester so I'm up to my neck with final exams and projects but I'll work towards pushing an update by the end of this week.

e-oj commented 6 years ago

This has been fixed. Thanks for catching this bug. Upgrade your Fawn version and it should be fine. Feel free to reopen this issue if it persists.

mnguyen25 commented 4 years ago

@e-oj I'm using 2.1.5 with Mongoose 5.4.23 and MongoDB 3.4.21 and still having the exact issue. Here's the code sample:


const PaymentCard = mongoose.model("PaymentCard");
transaction.update(PaymentCard, { _id: "5e846331f14f686e96aa3c8a" }, { $set: { userId: "5836505a26ff6890074294d0" } });
transaction.update(PaymentCard, { _id: "id" }, { $set: { userId: "5836505a26ff6890074294d0" } });
transaction.run({ useMongoose: true })
    .then(logger.debug)
    .catch(err => logger.error(err));

The first update `$set: { userId: "5836505a26ff6890074294d0" }` doesn't roll back.
s4suryapal commented 4 years ago

Hi, I am facing same issue. How to solve it ? If we use any other field rather than _id, its working. Having issue with _id only.

s4suryapal commented 4 years ago

I fixed it with following :

try {
  await new Fawn.Task()
  .update('collection', 
               { _id: mongoose.Types.ObjectId(req.params.id) } , 
               { name: 'test', desc: 'description })
  .run({useMongoose: true});
} 
catch(err) { console.log(err); }
mnguyen25 commented 4 years ago

Hi @s4suryapal your code throws an error because of a parsing error when you try to parse an invalid ObjectId using mongoose.Types.ObjectId which is prior to the Fawn call. Fawn wasn't called when the error was thrown.