fix(firestore, ios): transaction atomicity failure fix

[russellwheatley]

fixes #2805

Notes

• The fix; aborted variable was being used as abort (maybe use constants for property names?)
• Removed __block from variable on line 122 as it isn't used in a block.
• Removed complete variable as the completeBlock shouldn't run before the transaction has occurred so it'll never be set to true. The completeBlock should also run just once in that context so setting complete to true shouldn't matter. I could be wrong so happy to change if that's the case.

[codecov]

Codecov Report

Merging #3599 into master will increase coverage by 1.52%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3599      +/-   ##
==========================================
+ Coverage   54.39%   55.91%   +1.52%     
==========================================
  Files         112      108       -4     
  Lines        3668     3436     -232     
  Branches      194        0     -194     
==========================================
- Hits         1995     1921      -74     
+ Misses       1602     1515      -87     
+ Partials       71        0      -71     

[Salakar]

Needs a JS test to confirm transaction rollback.

[russellwheatley]

Please note: Rollback appears to be successful when you need to rollback on a single property (incrementing a number property for example). I was only able to reproduce the bug when it was two separate properties updated in a db record.

[Salakar]

PR title please 🙃

[Salakar]

I'm not sure the test is sufficiently confirming the previous behaviour in #2805 has been fixed, I rolled back your changes and this test still passes;

which indicates to me that it's not fixing previous behaviour?

If it helps I made a test out of the users code that passes, not checked if it fails previously:

   it.only('should roll back any updates that failed', async () => {
      await firebase
        .firestore()
        .collection('v6/transactions/matches')
        .doc('123')
        .set({
          turn: 0,
          score_user_1: 0,
          score_user_2: 0,
        });

      const createMove = async (uid, matchId, turn, delay = 0) => {
        // console.warn(`createMove ${uid} turn: ${turn}`);
        const matchRef = firebase
          .firestore()
          .collection('v6/transactions/matches')
          .doc(matchId);

        return firebase.firestore().runTransaction(async transaction => {
          // console.warn(`BEGIN transaction ${uid} turn: ${turn}`);
          // READ
          // console.warn(`READ BEGIN ${uid}`);
          const match = await transaction.get(matchRef);
          const matchData = match.data();
          // console.warn(`READ DONE ${uid} - turn: ${matchData.turn}`);

          // CHECK & PROCESS
          if (matchData.turn !== turn) {
            const message = `Sorry ${uid}! Another player was faster!`;
            // console.warn(`ABORT transaction ${uid} turn: ${turn}`);
            throw new Error(message);
          }
          const newTurn = matchData.turn || 1;
          const newScore = matchData[`score_${uid}`] || 1;

          // WRITE
          const updateObject = {
            turn: newTurn,
            [`score_${uid}`]: newScore,
          };
          // console.warn(
          //   `WRITE ${uid} - turn: ${updateObject.turn} score_${uid}: ${updateObject[`score_${uid}`]}`,
          // );

          await Utils.sleep(delay);

          // THIS UPDATE SHOULD BE ROLLED BACK
          // when the updateFunction is aborted second attempt
          transaction.update(matchRef, updateObject);
          // console.warn(`END transaction ${uid}`);
        });
      };

      const promises = [createMove('user_1', '123', 0, 500), createMove('user_2', '123', 0, 200)];

      try {
        await Promise.all(promises);
      } catch (e) {
        e.message.should.equal(`Sorry user_1! Another player was faster!`);
      }

      const result = await firebase
        .firestore()
        .collection('v6/transactions/matches')
        .doc('123')
        .get();

      result.data().should.eql(jet.contextify({ turn: 1, score_user_2: 1, score_user_1: 0 }));
    });

[Salakar]

Going to go ahead an merge this, though still could do with the test updating, will track on go/weekly for next week