🎨 run database population in transaction (#7448)

* 🎨 run database population in transaction refs #6574, refs #7432 - create transaction for creating tables - if an error occurs or a container get's destroyed before population finishes, transaction is rolled back * 🎨 simplify transaction creation and test
TryGhost · Sep 30, 2016 · 1867e1a · 1867e1a
1 parent 6842259
commit 1867e1a
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 24 deletions.
diff --git a/core/server/data/migration/fixtures/populate.js b/core/server/data/migration/fixtures/populate.js
@@ -56,7 +56,7 @@ createOwner = function createOwner(logger, modelOptions) {
         password:         coreUtils.uid(50)
     };
 
-    return models.Role.findOne({name: 'Owner'}).then(function (ownerRole) {
+    return models.Role.findOne({name: 'Owner'}, modelOptions).then(function (ownerRole) {
         if (ownerRole) {
             user.roles = [ownerRole.id];
 

diff --git a/core/server/data/migration/populate.js b/core/server/data/migration/populate.js
@@ -1,9 +1,11 @@
 // # Populate
 // Create a brand new database for a new install of ghost
 var Promise = require('bluebird'),
+    _ = require('lodash'),
     commands = require('../schema').commands,
     fixtures = require('./fixtures'),
     errors = require('../../errors'),
+    db = require('../../data/db'),
     schema = require('../schema').tables,
     schemaTables = Object.keys(schema),
     populate, logger;
@@ -30,20 +32,24 @@ populate = function populate(options) {
             context: {
                 internal: true
             }
-        },
-        tableSequence = Promise.mapSeries(schemaTables, function createTable(table) {
-            logger.info('Creating table: ' + table);
-            return commands.createTable(table);
-        });
+        };
 
     logger.info('Creating tables...');
 
-    if (tablesOnly) {
-        return tableSequence;
-    }
+    return db.knex.transaction(function populateDatabaseInTransaction(transaction) {
+        return Promise.mapSeries(schemaTables, function createTable(table) {
+            logger.info('Creating table: ' + table);
+            return commands.createTable(table, transaction);
+        }).then(function populateFixtures() {
+            if (tablesOnly) {
+                return;
+            }
 
-    return tableSequence.then(function () {
-        return fixtures.populate(logger, modelOptions);
+            return fixtures.populate(logger, _.merge({}, {transacting: transaction}, modelOptions));
+        });
+    }).catch(function populateDatabaseError(err) {
+        logger.warn('rolling back...');
+        return Promise.reject(new errors.InternalServerError('Unable to populate database: ' + err.message));
     });
 };
 

diff --git a/core/test/unit/migration_spec.js b/core/test/unit/migration_spec.js
@@ -173,11 +173,12 @@ describe('Migrations', function () {
         var createStub, fixturesStub;
 
         beforeEach(function () {
-            createStub = sandbox.stub(schema.commands, 'createTable').returns(new Promise.resolve());
             fixturesStub = sandbox.stub(fixtures, 'populate').returns(new Promise.resolve());
         });
 
         it('should create all tables, and populate fixtures', function (done) {
+            createStub = sandbox.stub(schema.commands, 'createTable').returns(new Promise.resolve());
+
             populate().then(function (result) {
                 should.not.exist(result);
 
@@ -190,18 +191,29 @@ describe('Migrations', function () {
             }).catch(done);
         });
 
-        it('should should only create tables, with tablesOnly setting', function (done) {
-            populate({tablesOnly: true}).then(function (result) {
-                should.exist(result);
-                result.should.be.an.Array().with.lengthOf(schemaTables.length);
+        it('should rollback if error occurs', function (done) {
+            var i = 0;
 
-                createStub.called.should.be.true();
-                createStub.callCount.should.be.eql(schemaTables.length);
-                createStub.firstCall.calledWith(schemaTables[0]).should.be.true();
-                createStub.lastCall.calledWith(schemaTables[schemaTables.length - 1]).should.be.true();
-                fixturesStub.called.should.be.false();
-                done();
-            }).catch(done);
+            createStub = sandbox.stub(schema.commands, 'createTable', function () {
+                i = i + 1;
+
+                if (i > 10) {
+                    return new Promise.reject(new Error('error on table creation :('));
+                }
+
+                return new Promise.resolve();
+            });
+
+            populate()
+                .then(function () {
+                    done(new Error('should throw an error for database population'));
+                })
+                .catch(function (err) {
+                    should.exist(err);
+                    (err instanceof errors.InternalServerError).should.eql(true);
+                    createStub.callCount.should.eql(11);
+                    done();
+                });
         });
     });
 

diff --git a/core/test/utils/index.js b/core/test/utils/index.js
@@ -466,7 +466,13 @@ getFixtureOps = function getFixtureOps(toDos) {
     // Database initialisation
     if (toDos.init || toDos.default) {
         fixtureOps.push(function initDB() {
-            return migration.populate({tablesOnly: tablesOnly});
+            return new Promise(function initDb(resolve, reject) {
+                migration.populate({tablesOnly: tablesOnly})
+                    .then(function () {
+                        resolve();
+                    })
+                    .catch(reject);
+            });
         });
 
         delete toDos.default;