Merge pull request #1415 from sebgie/import-transaction

Add transactions for import

core/server/data/import/000.js

@@ -78,32 +78,32 @@ function preProcessPostTags(tableData) {
     return tableData;
 }
 
-function importTags(ops, tableData) {
+function importTags(ops, tableData, transaction) {
     tableData = stripProperties(['id'], tableData);
     _.each(tableData, function (tag) {
-        ops.push(models.Tag.read({name: tag.name}).then(function (_tag) {
+        ops.push(models.Tag.findOne({name: tag.name}, {transacting: transaction}).then(function (_tag) {
             if (!_tag) {
-                return models.Tag.add(tag);
+                return models.Tag.add(tag, {transacting: transaction});
             }
             return when.resolve(_tag);
         }));
     });
 }
 
-function importPosts(ops, tableData) {
+function importPosts(ops, tableData, transaction) {
     tableData = stripProperties(['id'], tableData);
     _.each(tableData, function (post) {
-        ops.push(models.Post.add(post));
+        ops.push(models.Post.add(post, {transacting: transaction}));
     });
 }
 
-function importUsers(ops, tableData) {
+function importUsers(ops, tableData, transaction) {
     tableData = stripProperties(['id'], tableData);
     tableData[0].id = 1;
-    ops.push(models.User.edit(tableData[0]));
+    ops.push(models.User.edit(tableData[0], {transacting: transaction}));
 }
 
-function importSettings(ops, tableData) {
+function importSettings(ops, tableData, transaction) {
     // for settings we need to update individual settings, and insert any missing ones
     // the one setting we MUST NOT update is the databaseVersion settings
     var blackList = ['databaseVersion'];
@@ -112,48 +112,72 @@ function importSettings(ops, tableData) {
         return blackList.indexOf(data.key) === -1;
     });
 
-    ops.push(models.Settings.edit(tableData));
+    ops.push(models.Settings.edit(tableData, transaction));
 }
 
 // No data needs modifying, we just import whatever tables are available
 Importer000.prototype.basicImport = function (data) {
     var ops = [],
         tableData = data.data;
+    return models.Base.transaction(function (t) {
 
-    // Do any pre-processing of relationships (we can't depend on ids)
+        // Do any pre-processing of relationships (we can't depend on ids)
-    if (tableData.posts_tags && tableData.posts && tableData.tags) {
+        if (tableData.posts_tags && tableData.posts && tableData.tags) {
-        tableData = preProcessPostTags(tableData);
+            tableData = preProcessPostTags(tableData);
-    }
+        }
 
-    // Import things in the right order:
+        // Import things in the right order:
-    if (tableData.tags && tableData.tags.length) {
+        if (tableData.tags && tableData.tags.length) {
-        importTags(ops, tableData.tags);
+            importTags(ops, tableData.tags, t);
-    }
+        }
 
-    if (tableData.posts && tableData.posts.length) {
+        if (tableData.posts && tableData.posts.length) {
-        importPosts(ops, tableData.posts);
+            importPosts(ops, tableData.posts, t);
-    }
+        }
 
-    if (tableData.users && tableData.users.length) {
+        if (tableData.users && tableData.users.length) {
-        importUsers(ops, tableData.users);
+            importUsers(ops, tableData.users, t);
-    }
+        }
 
-    if (tableData.settings && tableData.settings.length) {
+        if (tableData.settings && tableData.settings.length) {
-        importSettings(ops, tableData.settings);
+            importSettings(ops, tableData.settings, t);
-    }
+        }
 
-    /** do nothing with these tables, the data shouldn't have changed from the fixtures
+        /** do nothing with these tables, the data shouldn't have changed from the fixtures
-     *   permissions
+         *   permissions
-     *   roles
+         *   roles
-     *   permissions_roles
+         *   permissions_roles
-     *   permissions_users
+         *   permissions_users
-     *   roles_users
+         *   roles_users
-     */
+         */
 
-    return when.all(ops).then(function (results) {
+        // Write changes to DB, if successful commit, otherwise rollback
-        return when.resolve(results);
+        // when.all() does not work as expected, when.settle() does.
-    }, function (err) {
+        when.settle(ops).then(function (descriptors) {
-        return when.reject("Error importing data: " + err.message || err, err.stack);
+            var rej = false,
+                error = '';
+            descriptors.forEach(function (d) {
+                if (d.state === 'rejected') {
+                    error += _.isEmpty(error) ? '' : '</br>';
+                    if (!_.isEmpty(d.reason.clientError)) {
+                        error += d.reason.clientError;
+                    } else if (!_.isEmpty(d.reason.message)) {
+                        error += d.reason.message;
+                    }
+                    rej = true;
+                }
+            });
+            if (rej) {
+                t.rollback(error);
+            } else {
+                t.commit();
+            }
+        });
+    }).then(function () {
+        //TODO: could return statistics of imported items
+        return when.resolve();
+    }, function (error) {
+        return when.reject("Error importing data: " + error);
     });
 };
 

core/server/models/base.js

@@ -90,8 +90,12 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
             slugTryCount = 1,
             // Look for a post with a matching slug, append an incrementing number if so
             checkIfSlugExists = function (slugToFind) {
-                readOptions = _.extend(readOptions || {}, { slug: slugToFind });
+                var args = {slug: slugToFind};
-                return Model.read(readOptions).then(function (found) {
+                //status is needed for posts
+                if (readOptions && readOptions.status) {
+                    args.status = readOptions.status;
+                }
+                return Model.findOne(args, readOptions).then(function (found) {
                     var trimSpace;
 
                     if (!found) {
@@ -177,7 +181,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
     edit: function (editedObj, options) {
         options = options || {};
         return this.forge({id: editedObj.id}).fetch(options).then(function (foundObj) {
-            return foundObj.save(editedObj);
+            return foundObj.save(editedObj, options);
         });
     },
 
@@ -192,7 +196,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
      */
     add: function (newObj, options) {
         options = options || {};
-        return this.forge(newObj).save(options);
+        return this.forge(newObj).save(null, options);
     },
 
     create: function () {

core/server/models/index.js

@@ -7,6 +7,7 @@ module.exports = {
     Permission: require('./permission').Permission,
     Settings: require('./settings').Settings,
     Tag: require('./tag').Tag,
+    Base: require('./base'),
     init: function () {
         return migrations.init();
     },

core/server/models/post.js

@@ -39,11 +39,12 @@ Post = ghostBookshelf.Model.extend({
 
     validate: function () {
         ghostBookshelf.validator.check(this.get('title'), "Post title cannot be blank").notEmpty();
-
+        ghostBookshelf.validator.check(this.get('title'), 'Post title maximum length is 150 characters.').len(0, 150);
         return true;
     },
 
-    saving: function () {
+    saving: function (newPage, attr, options) {
+        /*jslint unparam:true*/
         var self = this;
 
         // Remove any properties which don't belong on the post model
@@ -65,14 +66,15 @@ Post = ghostBookshelf.Model.extend({
 
         if (this.hasChanged('slug')) {
             // Pass the new slug through the generator to strip illegal characters, detect duplicates
-            return this.generateSlug(Post, this.get('slug'), { status: 'all' })
+            return this.generateSlug(Post, this.get('slug'), {status: 'all', transacting: options.transacting})
                 .then(function (slug) {
                     self.set({slug: slug});
                 });
         }
     },
 
-    creating: function () {
+    creating: function (newPage, attr, options) {
+        /*jslint unparam:true*/
         // set any dynamic default properties
         var self = this;
 
@@ -84,15 +86,17 @@ Post = ghostBookshelf.Model.extend({
 
         if (!this.get('slug')) {
             // Generating a slug requires a db call to look for conflicting slugs
-            return this.generateSlug(Post, this.get('title'), { status: 'all' })
+            return this.generateSlug(Post, this.get('title'), {status: 'all', transacting: options.transacting})
                 .then(function (slug) {
                     self.set({slug: slug});
                 });
         }
     },
 
-    updateTags: function (newTags) {
+    updateTags: function (newTags, attr, options) {
+        /*jslint unparam:true*/
         var self = this;
+        options = options || {};
 
 
         if (newTags === this) {
@@ -103,7 +107,8 @@ Post = ghostBookshelf.Model.extend({
             return;
         }
 
-        return Post.forge({id: this.id}).fetch({withRelated: ['tags']}).then(function (thisPostWithTags) {
+        return Post.forge({id: this.id}).fetch({withRelated: ['tags'], transacting: options.transacting}).then(function (thisPostWithTags) {
+
             var existingTags = thisPostWithTags.related('tags').toJSON(),
                 tagOperations = [],
                 tagsToDetach = [],
@@ -117,7 +122,7 @@ Post = ghostBookshelf.Model.extend({
             });
 
             if (tagsToDetach.length > 0) {
-                tagOperations.push(self.tags().detach(tagsToDetach));
+                tagOperations.push(self.tags().detach(tagsToDetach, options));
             }
 
             // Next check if new tags are all exactly the same as what is set on the model
@@ -129,17 +134,22 @@ Post = ghostBookshelf.Model.extend({
             });
 
             if (!_.isEmpty(tagsToAttach)) {
-                return Tags.forge().query('whereIn', 'name', _.pluck(tagsToAttach, 'name')).fetch().then(function (matchingTags) {
+                return Tags.forge().query('whereIn', 'name', _.pluck(tagsToAttach, 'name')).fetch(options).then(function (matchingTags) {
                     _.each(matchingTags.toJSON(), function (matchingTag) {
-                        tagOperations.push(self.tags().attach(matchingTag.id));
+                        tagOperations.push(self.tags().attach(matchingTag.id, options));
                         tagsToAttach = _.reject(tagsToAttach, function (tagToAttach) {
                             return tagToAttach.name === matchingTag.name;
                         });
                     });
 
                     _.each(tagsToAttach, function (tagToCreateAndAttach) {
-                        var createAndAttachOperation = Tag.add({name: tagToCreateAndAttach.name}).then(function (createdTag) {
+                        var createAndAttachOperation,
-                            return self.tags().attach(createdTag.id, createdTag.name);
+                            opt = options.method;
+                        //TODO: remove when refactor; ugly fix to overcome bookshelf
+                        options.method = 'insert';
+                        createAndAttachOperation = Tag.add({name: tagToCreateAndAttach.name}, options).then(function (createdTag) {
+                            options.method = opt;
+                            return self.tags().attach(createdTag.id, createdTag.name, options);
                         });
 
 
@@ -339,13 +349,12 @@ Post = ghostBookshelf.Model.extend({
         // Otherwise, you shall not pass.
         return when.reject();
     },
-
     add: function (newPostData, options) {
         var self = this;
         return ghostBookshelf.Model.add.call(this, newPostData, options).then(function (post) {
             // associated models can't be created until the post has an ID, so run this after
-            return when(post.updateTags(newPostData.tags)).then(function () {
+            return when(post.updateTags(newPostData.tags, null, options)).then(function () {
-                return self.findOne({status: 'all', id: post.id});
+                return self.findOne({status: 'all', id: post.id}, options);
             });
         });
     },
@@ -353,7 +362,10 @@ Post = ghostBookshelf.Model.extend({
         var self = this;
 
         return ghostBookshelf.Model.edit.call(this, editedPost, options).then(function (editedObj) {
-            return self.findOne({status: 'all', id: editedObj.id});
+            return when(editedObj.updateTags(editedPost.tags, null, options)).then(function () {
+                return self.findOne({status: 'all', id: editedObj.id}, options);
+            });
+            //return self.findOne({status: 'all', id: editedObj.id}, options);
         });
     },
     destroy: function (_identifier, options) {

core/server/models/settings.js

@@ -95,7 +95,7 @@ Settings = ghostBookshelf.Model.extend({
         return ghostBookshelf.Model.read.call(this, _key);
     },
 
-    edit: function (_data) {
+    edit: function (_data, t) {
         var settings = this;
         if (!Array.isArray(_data)) {
             _data = [_data];
@@ -103,11 +103,12 @@ Settings = ghostBookshelf.Model.extend({
         return when.map(_data, function (item) {
             // Accept an array of models as input
             if (item.toJSON) { item = item.toJSON(); }
-            return settings.forge({ key: item.key }).fetch().then(function (setting) {
+            return settings.forge({ key: item.key }).fetch({transacting: t}).then(function (setting) {
+
                 if (setting) {
-                    return setting.set('value', item.value).save();
+                    return setting.set('value', item.value).save(null, {transacting: t});
                 }
-                return settings.forge({ key: item.key, value: item.value }).save();
+                return settings.forge({ key: item.key, value: item.value }).save(null, {transacting: t});
 
             }, errors.logAndThrowError);
         });

core/test/unit/import_spec.js

@@ -91,5 +91,105 @@ describe("Import", function () {
                 done();
             }).then(null, done);
         });
+
+        it("doesn't imports invalid post data from 000", function (done) {
+            var exportData;
+
+            // migrate to current version
+            migration.migrateUp().then(function () {
+                // Load the fixtures
+                return fixtures.populateFixtures();
+            }).then(function () {
+                // Initialise the default settings
+                return Settings.populateDefaults();
+            }).then(function () {
+                // export the version 000 data ready to import
+                // TODO: Should have static test data here?
+                return exporter();
+            }).then(function (exported) {
+                exportData = exported;
+                //change title to 151 characters
+                exportData.data.posts[0].title = new Array(152).join('a');
+                exportData.data.posts[0].tags = 'Tag';
+                return importer("000", exportData);
+            }).then(function () {
+                (1).should.eql(0, 'Data import should not resolve promise.');
+            }, function (error) {
+                error.should.eql('Error importing data: Post title maximum length is 150 characters.');
+                when.all([
+                    knex("users").select(),
+                    knex("posts").select(),
+                    knex("settings").select(),
+                    knex("tags").select()
+                ]).then(function (importedData) {
+                    should.exist(importedData);
+                    importedData.length.should.equal(4, 'Did not get data successfully');
+
+                    // we always have 0 users as there isn't one in fixtures
+                    importedData[0].length.should.equal(0, 'There should not be a user');
+                    // import no longer requires all data to be dropped, and adds posts
+                    importedData[1].length.should.equal(exportData.data.posts.length, 'Wrong number of posts');
+
+                    // test settings
+                    importedData[2].length.should.be.above(0, 'Wrong number of settings');
+                    _.findWhere(importedData[2], {key: "databaseVersion"}).value.should.equal("000", 'Wrong database version');
+
+                    // test tags
+                    importedData[3].length.should.equal(exportData.data.tags.length, 'no new tags');
+
+                    done();
+                });
+                
+            }).then(null, done);
+        });
+        it("doesn't imports invalid settings data from 000", function (done) {
+            var exportData;
+
+            // migrate to current version
+            migration.migrateUp().then(function () {
+                // Load the fixtures
+                return fixtures.populateFixtures();
+            }).then(function () {
+                // Initialise the default settings
+                return Settings.populateDefaults();
+            }).then(function () {
+                // export the version 000 data ready to import
+                // TODO: Should have static test data here?
+                return exporter();
+            }).then(function (exported) {
+                exportData = exported;
+                //change to blank settings key
+                exportData.data.settings[3].key = null;
+                return importer("000", exportData);
+            }).then(function () {
+                (1).should.eql(0, 'Data import should not resolve promise.');
+            }, function (error) {
+                error.should.eql('Error importing data: Setting key cannot be blank');
+                when.all([
+                    knex("users").select(),
+                    knex("posts").select(),
+                    knex("settings").select(),
+                    knex("tags").select()
+                ]).then(function (importedData) {
+                    should.exist(importedData);
+                    importedData.length.should.equal(4, 'Did not get data successfully');
+
+                    // we always have 0 users as there isn't one in fixtures
+                    importedData[0].length.should.equal(0, 'There should not be a user');
+                    // import no longer requires all data to be dropped, and adds posts
+                    importedData[1].length.should.equal(exportData.data.posts.length, 'Wrong number of posts');
+
+                    // test settings
+                    importedData[2].length.should.be.above(0, 'Wrong number of settings');
+                    _.findWhere(importedData[2], {key: "databaseVersion"}).value.should.equal("000", 'Wrong database version');
+
+                    // test tags
+                    importedData[3].length.should.equal(exportData.data.tags.length, 'no new tags');
+
+                    done();
+                });
+                
+            }).then(null, done);
+        });
     });
 });