Refactor content deletion

- Simplify the `init` method in `models/index.js` so that it no longer
  returns a promise. Easier to use.
- Eliminates the `deleteAllContent` method from `models/index.js` as it
  can all be handled at the API layer in a single spot.
- Optimize `destroyAllContent` in `api/db.js`. Eliminates
  double-fetching every post from the database and converting it to
  JSON. Also only fetches ids from the database instead of the entire
  model.
- Eliminates the custom static method `destroy` in the Post model in
  favor of handling detaching tag relations in a single place (the
  `destroying` event). This also eliminates a big source of unneeded
  database round trips--needing to get post ids to feed into
  `Post.destroy()` which then re-fetches the post again.
This commit is contained in:
Jason Williams 2016-03-01 22:42:01 -06:00
parent 229c2925a6
commit 9fe573a0c5
16 changed files with 79 additions and 124 deletions

View File

@ -577,8 +577,7 @@ var _ = require('lodash'),
migration = require('./core/server/data/migration');
migration.reset().then(function () {
return models.init();
}).then(function () {
models.init();
return migration.init();
}).then(function () {
done();

View File

@ -109,16 +109,23 @@ db = {
* @returns {Promise} Success
*/
deleteAllContent: function (options) {
var tasks;
var tasks,
queryOpts = {columns: 'id'};
options = options || {};
function deleteContent() {
return Promise.resolve(models.deleteAllContent())
.return({db: []})
.catch(function (error) {
return Promise.reject(new errors.InternalServerError(error.message || error));
});
var collections = [
models.Post.findAll(queryOpts),
models.Tag.findAll(queryOpts)
];
return Promise.each(collections, function then(Collection) {
return Collection.invokeThen('destroy');
}).return({db: []})
.catch(function (error) {
throw new errors.InternalServerError(error.message || error);
});
}
tasks = [

View File

@ -212,18 +212,19 @@ posts = {
var tasks;
/**
* ### Model Query
* Make the call to the Model layer
* @param {Object} options
* @returns {Object} options
* @function deletePost
* @param {Object} options
* @return {Object} JSON representation of the deleted post
*/
function modelQuery(options) {
// Removing a post needs to include all posts.
options.status = 'all';
return posts.read(options).then(function (result) {
return dataProvider.Post.destroy(options).then(function () {
return result;
});
function deletePost(options) {
var Post = dataProvider.Post,
data = _.defaults({status: 'all'}, options),
fetchOpts = _.defaults({require: true}, options);
return Post.findOne(data, fetchOpts).then(function (post) {
return post.destroy(options).return({posts: [post.toJSON()]});
}).catch(Post.NotFoundError, function () {
throw new errors.NotFoundError(i18n.t('errors.api.posts.postNotFound'));
});
}
@ -232,20 +233,12 @@ posts = {
utils.validate(docName, {opts: utils.idDefaultOptions}),
utils.handlePermissions(docName, 'destroy'),
utils.convertOptions(allowedIncludes),
modelQuery
deletePost
];
// Pipeline calls each task passing the result of one to be the arguments for the next
return pipeline(tasks, options).then(function formatResponse(result) {
var deletedObj = result;
if (deletedObj.posts) {
_.each(deletedObj.posts, function (post) {
post.statusChanged = true;
});
}
return deletedObj;
return pipeline(tasks, options).tap(function formatResponse(response) {
response.posts[0].statusChanged = true;
});
}
};

View File

@ -66,7 +66,7 @@ function init(options) {
return config.checkDeprecated();
}).then(function () {
// Initialise the models
return models.init();
models.init();
}).then(function () {
// Initialize migrations
return migrations.init();

View File

@ -2,8 +2,7 @@
* Dependencies
*/
var Promise = require('bluebird'),
_ = require('lodash'),
var _ = require('lodash'),
exports,
models;
@ -36,32 +35,8 @@ function init() {
models.forEach(function (name) {
_.extend(exports, require('./' + name));
});
return Promise.resolve();
}
/**
* TODO: move to some other place
*/
// ### deleteAllContent
// Delete all content from the database (posts, tags, tags_posts)
exports.deleteAllContent = function deleteAllContent() {
var self = this;
return self.Post.findAll().then(function then(posts) {
return Promise.all(_.map(posts.toJSON(), function mapper(post) {
return self.Post.destroy({id: post.id});
}));
}).then(function () {
return self.Tag.findAll().then(function then(tags) {
return Promise.all(_.map(tags.toJSON(), function mapper(tag) {
return self.Tag.destroy({id: tag.id});
}));
});
});
};
/**
* Expose `init`
*/

View File

@ -82,11 +82,13 @@ Post = ghostBookshelf.Model.extend({
}
});
this.on('destroying', function onDestroying(model) {
if (model.previous('status') === 'published') {
model.emitChange('unpublished');
}
model.emitChange('deleted');
this.on('destroying', function (model/*, attr, options*/) {
return model.load('tags').call('related', 'tags').call('detach').then(function then() {
if (model.previous('status') === 'published') {
model.emitChange('unpublished');
}
model.emitChange('deleted');
});
});
},
@ -373,8 +375,9 @@ Post = ghostBookshelf.Model.extend({
// whitelists for the `options` hash argument on methods, by method name.
// these are the only options that can be passed to Bookshelf / Knex.
validOptions = {
findOne: ['importing', 'withRelated'],
findOne: ['importing', 'withRelated', 'require'],
findPage: ['page', 'limit', 'columns', 'filter', 'order', 'status', 'staticPages'],
findAll: ['columns'],
add: ['importing']
};
@ -520,44 +523,27 @@ Post = ghostBookshelf.Model.extend({
});
},
/**
* ### Destroy
* @extends ghostBookshelf.Model.destroy to clean up tag relations
* **See:** [ghostBookshelf.Model.destroy](base.js.html#destroy)
*/
destroy: function destroy(options) {
var id = options.id;
options = this.filterOptions(options, 'destroy');
return this.forge({id: id}).fetch({withRelated: ['tags']}).then(function destroyTags(post) {
return post.related('tags').detach().then(function destroyPosts() {
return post.destroy(options);
});
});
},
/**
* ### destroyByAuthor
* @param {[type]} options has context and id. Context is the user doing the destroy, id is the user to destroy
*/
destroyByAuthor: function destroyByAuthor(options) {
destroyByAuthor: Promise.method(function destroyByAuthor(options) {
var postCollection = Posts.forge(),
authorId = options.id;
options = this.filterOptions(options, 'destroyByAuthor');
if (authorId) {
return postCollection.query('where', 'author_id', '=', authorId).fetch(options).then(function destroyTags(results) {
return Promise.map(results.models, function mapper(post) {
return post.related('tags').detach(null, options).then(function destroyPosts() {
return post.destroy(options);
});
});
}, function (error) {
return Promise.reject(new errors.InternalServerError(error.message || error));
});
if (!authorId) {
throw new errors.NotFoundError(i18n.t('errors.models.post.noUserFound'));
}
return Promise.reject(new errors.NotFoundError(i18n.t('errors.models.post.noUserFound')));
},
return postCollection.query('where', 'author_id', '=', authorId)
.fetch(options)
.call('invokeThen', 'destroy', options)
.catch(function (error) {
throw new errors.InternalServerError(error.message || error);
});
}),
permissible: function permissible(postModelOrId, action, context, loadedPermissions, hasUserPermission, hasAppPermission) {
var self = this,

View File

@ -75,7 +75,8 @@ Tag = ghostBookshelf.Model.extend({
// whitelists for the `options` hash argument on methods, by method name.
// these are the only options that can be passed to Bookshelf / Knex.
validOptions = {
findPage: ['page', 'limit', 'columns', 'filter', 'order']
findPage: ['page', 'limit', 'columns', 'filter', 'order'],
findAll: ['columns']
};
if (validOptions[methodName]) {

View File

@ -819,7 +819,7 @@ describe('Post Model', function () {
post.tags[0].id.should.equal(firstItemData.id);
// Destroy the post
return PostModel.destroy(firstItemData);
return results.destroy();
}).then(function (response) {
var deleted = response.toJSON();
@ -857,7 +857,7 @@ describe('Post Model', function () {
post.tags[0].id.should.equal(firstItemData.id);
// Destroy the post
return PostModel.destroy(firstItemData);
return results.destroy(firstItemData);
}).then(function (response) {
var deleted = response.toJSON();
@ -894,7 +894,7 @@ describe('Post Model', function () {
page.page.should.be.true();
// Destroy the page
return PostModel.destroy(firstItemData);
return results.destroy(firstItemData);
}).then(function (response) {
var deleted = response.toJSON();
@ -930,7 +930,7 @@ describe('Post Model', function () {
page.id.should.equal(firstItemData.id);
// Destroy the page
return PostModel.destroy(firstItemData);
return results.destroy(firstItemData);
}).then(function (response) {
var deleted = response.toJSON();

View File

@ -31,9 +31,9 @@ var should = require('should'),
describe('Auth Strategies', function () {
var next;
before(function (done) {
before(function () {
// Loads all the models
Models.init().then(done).catch(done);
Models.init();
});
beforeEach(function () {

View File

@ -9,9 +9,9 @@ var sinon = require('sinon'),
describe('OAuth', function () {
var next, req, res, sandbox;
before(function (done) {
before(function () {
// Loads all the models
Models.init().then(done).catch(done);
Models.init();
});
beforeEach(function () {

View File

@ -19,15 +19,13 @@ var should = require('should'),
describe('Fixtures', function () {
var loggerStub;
beforeEach(function (done) {
beforeEach(function () {
loggerStub = {
info: sandbox.stub(),
warn: sandbox.stub()
};
models.init().then(function () {
done();
}).catch(done);
models.init();
});
afterEach(function () {

View File

@ -976,20 +976,18 @@ describe('Migrations', function () {
describe('FixClientSecret', function () {
var fixClientSecret, queryStub, clientForgeStub, clientEditStub, toStringStub, cryptoStub;
beforeEach(function (done) {
beforeEach(function () {
fixClientSecret = migration.__get__('fixClientSecret');
queryStub = {
query: sandbox.stub().returnsThis(),
fetch: sandbox.stub()
};
models.init().then(function () {
toStringStub = {toString: sandbox.stub().returns('TEST')};
cryptoStub = sandbox.stub(crypto, 'randomBytes').returns(toStringStub);
clientForgeStub = sandbox.stub(models.Clients, 'forge').returns(queryStub);
clientEditStub = sandbox.stub(models.Client, 'edit');
done();
}).catch(done);
models.init();
toStringStub = {toString: sandbox.stub().returns('TEST')};
cryptoStub = sandbox.stub(crypto, 'randomBytes').returns(toStringStub);
clientForgeStub = sandbox.stub(models.Clients, 'forge').returns(queryStub);
clientEditStub = sandbox.stub(models.Client, 'edit');
});
it('should do nothing if there are no incorrect secrets', function (done) {

View File

@ -14,9 +14,8 @@ should.equal(true, true);
describe('Access Rules', function () {
beforeEach(function () {
return models.init().then(function () {
ghostBookshelf = models.Base;
});
models.init();
ghostBookshelf = models.Base;
});
afterEach(function () {

View File

@ -12,9 +12,8 @@ var should = require('should'),
describe('Filter', function () {
before(function () {
return models.init().then(function () {
ghostBookshelf = models.Base;
});
models.init();
ghostBookshelf = models.Base;
});
beforeEach(function () {

View File

@ -19,8 +19,8 @@ describe('Permissions', function () {
});
describe('actions map', function () {
before(function (done) {
Models.init().then(done).catch(done);
before(function () {
Models.init();
});
beforeEach(function () {

View File

@ -471,9 +471,9 @@ setup = function setup() {
args = arguments;
return function (done) {
return Models.init().then(function () {
return initFixtures.apply(self, args);
}).then(function () {
Models.init();
return initFixtures.apply(self, args).then(function () {
done();
}).catch(done);
};