Added support for 'FOR UPDATE' lock (#14433)

refs https://github.com/TryGhost/Team/issues/1248

This is the underlying cause of the problems we've seen whilst handling
Stripe webhooks. A transaction ensures that the operations are atomic,
but not that they can run concurrently.

If you have some code which does this, concurrently:
1. Starts a transaction
2. Reads a value
3. Changes the values
4. Ends the transaction

Without applying the `FOR UPDATE` lock - you will have both sequenes
read the same value at step 2.

With the `FOR UPDATE` lock - one of the sequences will hang at step 2,
waiting for the other transaction to end, at which point it will resume
and read the _changed_ value.

Because the `edit` method explicitly does a read followed by a write, we
have also add the `FOR UPDATE` lock to this by default, to avoid any race
conditions. This does however require that `edit` is called within a
transaction. An issue here https://github.com/TryGhost/Team/issues/1503
considers running in a transaction by default.

core/server/models/base/plugins/crud.js

@@ -137,6 +137,10 @@ module.exports = function (Bookshelf) {
                 options.columns = _.intersection(options.columns, this.prototype.permittedAttributes());
             }
 
+            if (options.transacting && options.forUpdate) {
+                options.lock = 'forUpdate';
+            }
+
             return model.fetch(options)
                 .catch((err) => {
                     // CASE: SQL syntax is incorrect
@@ -179,6 +183,10 @@ module.exports = function (Bookshelf) {
                 model.hasTimestamps = false;
             }
 
+            if (options.transacting) {
+                options.lock = 'forUpdate';
+            }
+
             const object = await model.fetch(options);
             if (object) {
                 options.method = 'update';

test/unit/server/models/base/crud.test.js

@@ -104,6 +104,31 @@ describe('Models: crud', function () {
                 should.equal(fetchStub.args[0][0], filteredOptions);
             });
         });
+
+        it('Sets the `lock` option to "forUpdate" when the `forUpdate` and `transacting` options are passed', function () {
+            const data = {
+                id: 670
+            };
+            const unfilteredOptions = {
+                donny: 'donson',
+                forUpdate: true,
+                transacting: {}
+            };
+            const model = models.Base.Model.forge({});
+            const fetchedModel = models.Base.Model.forge({});
+            sinon.spy(models.Base.Model, 'filterOptions');
+            sinon.spy(models.Base.Model, 'filterData');
+            sinon.stub(models.Base.Model, 'forge')
+                .returns(model);
+            const fetchStub = sinon.stub(model, 'fetch')
+                .resolves(fetchedModel);
+
+            const findOneReturnValue = models.Base.Model.findOne(data, unfilteredOptions);
+
+            return findOneReturnValue.then((result) => {
+                should.equal(fetchStub.args[0][0].lock, 'forUpdate');
+            });
+        });
     });
 
     describe('edit', function () {
@@ -137,6 +162,7 @@ describe('Models: crud', function () {
                 should.deepEqual(forgeStub.args[0][0], {id: filteredOptions.id});
 
                 should.equal(fetchStub.args[0][0], filteredOptions);
+                should.equal(fetchStub.args[0][0].lock, undefined);
 
                 const filteredData = filterDataSpy.returnValues[0];
                 should.equal(saveStub.args[0][0], filteredData);
@@ -145,6 +171,25 @@ describe('Models: crud', function () {
             });
         });
 
+        it('sets options.lock to "forUpdate" if options.transacting is present', function () {
+            const data = {
+                base: 'cannon'
+            };
+            const unfilteredOptions = {
+                transacting: {}
+            };
+
+            const model = models.Base.Model.forge({});
+            sinon.stub(models.Base.Model, 'forge')
+                .returns(model);
+            const fetchStub = sinon.stub(model, 'fetch')
+                .resolves();
+
+            return models.Base.Model.findOne(data, unfilteredOptions).then(() => {
+                should.equal(fetchStub.args[0][0].lock, undefined);
+            });
+        });
+
         it('sets model.hasTimestamps to false if options.importing is truthy', function () {
             const data = {
                 base: 'cannon'