Optimistic Locking doesn't work for update

[tstearn]

Issue type:

[ ] question
[ ] bug report
[ ] feature request
[ ] documentation issue

Database system/driver:

[ ] cordova
[ ] mongodb
[ ] mssql
[ ] mysql / mariadb
[ ] oracle
[x ] postgres
[ ] sqlite
[ ] sqljs
[ ] react-native
[ ] expo

TypeORM version:

[x ] latest
[ ] @next
[ ] 0.x.x (or put your version here)

Steps to reproduce or a small repository showing the problem:
Hi All,

Trying to use the @Version annotation to implement optimistic locking for ensuring entity updates don't stomp on newer data. I couldn't find an example using QueryRunner along with update(), but I tried it anyway like this:

await transactionalRepository.createQueryBuilder("c")
.setLock("optimistic", caseEntity.version)
.update()
.set(this.createCaseUpdateObj(caseEntity))
.where("id = :id", {id: caseEntity.id})
.execute();

However, this operation succeeds no matter what I pass to setLock for version. I CAN ensure that I don't stomp on newer data by augmenting my query like so:

await transactionalRepository.createQueryBuilder("c")
.update()
.set(this.createCaseUpdateObj(caseEntity))
.where("id = :id", {id: caseEntity.id})
.andWhere("version = :version", {version: caseEntity.version})
.execute();

However, the problem now is I have no way of finding out whether my update was actually applied. To do that, I need to know how many rows where updated by my query, which does not appear to be returned by this method.

Are there plans to make setLock() handle this case for update() (most ORMs support this)? If not, are there plans to return the number of rows updated by an operation like the one above? If so, any idea of timeframe?

I think until then I'd probably need to drop down to lower level postgres driver to safely perform an update operation.

[tstearn]

I see that issue 1308 would let me check to see how many rows were updated, which would allow me to safely implement an Optimistic Lock for update. This would be a fine intermediate result.

However, I think longer term actually making setLock() with update() work like this would be a more intuitive and reusable API, especially given this is how other ORMs behave.

[tstearn]

Also, doing this via QueryRunner doesn't seem like the best choice to me. This would mean the client needs to walk the object graph and individually find and save every object that is version.

[tstearn]

I stumbled on a reliable way of making this work for Postgres:

//The version where clause below prevents an update from occurring if a newer version is already in the database
//Now, we need to determine if the update did occur so we can signal the user, if not.
//The traditional way to do this would be to check to see how many rows where updated - if it's zero, the updated didn't go through due to version
//TypeORM doesn't return the number of rows affected, but can use .returning().
//This takes an array of fields to return the updated values for.
//If the update doesn't go through due to the version where clause, this array will be empty
const result = await manager.createQueryBuilder()
.update(entityName)
.set(updateFields)
.where("id = :id", {id: entity.id})
.andWhere("version = :version", {version: entity.version})
.returning("version")
.execute();

const updateSucceeded = result.raw.length !== 0;
if(!updateSucceeded) {
		throw new OptimisticLockError(`Update did not go through due to outdated version ${updateParams.entity.version}`);
} 

As discussed in the comment above, using .returning() gives a reliable indicator of whether the update went through. Combining this with the version checking where clause makes an acceptable solution.

Having the .setLock() and @Version feature work together to accomplish this would be better, but this is OK for now.

[GitStorageOne]

Can't find proper sample of complex scenario, that include:

• insert entity if there is no such entity
• update entity with relations if entity data version has expected value.

Everything should be data safe and not affected by race conditions.

Seems, something like this, should work

for insert

  // Transaction manager + lock
  getManager().transaction((transactionalEntityManager) => {
    return transactionalEntityManager
      .createQueryBuilder(User, 'user')
      .setLock('pessimistic_write')
      .whereInIds(userDataShape.id)
      .getOne()
      .then((existData) => {
        // check that object not exists
        if (existData) {
          throw new Error('dbInsertErrorDuplicateKey');
        }
        // create new entity
        const newUser = User.create(userDataShape);
        // save
        return transactionalEntityManager
          .save(newUser)
          .then((result) => {
            // done
          });
      });
  }).catch((err) => {
    // err
  });

for update:

// Transaction manager + lock
getManager().transaction((transactionalEntityManager) => {
  return transactionalEntityManager
    .createQueryBuilder(User, 'user')
    .setLock('pessimistic_write')
    .whereInIds(inputDataShape.id)
    .getOne()
    .then((existData) => {
      // check that object exists
      if (!existData) {
        throw new Error('dbRecordNotFoundById');
      }
      // check object version
      if (existData.dataVersion !== inputDataShape.dataVersion) {
        throw new Error('objectVersionOutdated');
      }
      // update entity
      Object.assign(existData, inputDataShape);
      // save
      return transactionalEntityManager
        .save(existData)
        .then((result) => {
          // done
        });
    });
}).catch((err) => {
  // err
});

[tstearn]

Thanks @GitStorageOne , but I don't believe this solves the problem. The issue is that save() itself knows nothing about the version and doesn't guard against updating a record that has a later version than what's being passed in.

If I took the approach above, with two concurrent transactions, the second in would fail to acquire the write lock. But if I built auto-retry in, a retry would merrily overwrite what's in the table with old version content.

The key issue is that the update query must be version aware. The queries generated by save() are not. I'm able to make QueryBuilder update queries aware manually, with where and returning() clauses above.

The ultimate solution would be to have save() generate the proper SQL (like mine above) if the entity has a property annotated by @Version. This would be so much better because it could work with a whole object tree.

[GitStorageOne]

I've tested behavior with postgresql. Scenario looks like this:

1. Pessimistic lock, locks the row (until the end of transaction).
   Any other transactions, that perform change, will wait.
   Include select query's with "FOR UPDATE" statement (which produced by the .setLock('pessimistic_write')).
2. In same time, row available for select query's.
3. Next, from code we ensure that object version is correct, changing data and performing save operation. When transaction will be committed or rolled back, row lock will be released.

Lets say, we have many concurrent save operations, with code, that i showed above.
Only first code execution will change the object, all other concurrent save operations will fail on data version check.

The ultimate solution would be to have save() generate the proper SQL

That's true.

[pleerock]

update executes a simple UPDATE query, no more than that. If you want complete save functionality use save method instead.

[nicomouss]

@tstearn #3608 should solve your problem.