Skip to content

Commit

Permalink
🐛 fix delete by author as transaction (#7145)
Browse files Browse the repository at this point in the history
closes #7137

Deleting the content from the database runs in a transaction. see
https://github.com/TryGhost/Ghost/blob/master/core/server/api/users.js#L390

`destroyByAuthor` is one of the operations we trigger to delete all the conent, see https://github.com/TryGhost/Ghost/blob/master/core/server/models/post.js#L647

The post model has a specific hook for deleting content to delete the relations as well, see https://github.com/TryGhost/Ghost/blob/master/core/server/models/post.js#L122

This hook is part of the transaction. But the `options` are ignored. `(model/*, attr, options*/)` 
We use the `options` to forward the transaction reference, which we need to pass into the bookshelf queries. So `return model.load('tags').call('related', 'tags').call('detach')` does not forward the transaction and that's why it stucks when deleting the content.
  • Loading branch information
kirrg001 authored and ErisDS committed Sep 19, 2016
1 parent a98e134 commit 9349e99
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 11 deletions.
4 changes: 2 additions & 2 deletions core/server/models/base/token.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ Basetoken = ghostBookshelf.Model.extend({
.query('where', 'expires', '<', Date.now())
.fetch(options)
.then(function then(collection) {
collection.invokeThen('destroy', options);
return collection.invokeThen('destroy', options);
});
},
/**
Expand All @@ -53,7 +53,7 @@ Basetoken = ghostBookshelf.Model.extend({
.query('where', 'user_id', '=', userId)
.fetch(options)
.then(function then(collection) {
collection.invokeThen('destroy', options);
return collection.invokeThen('destroy', options);
});
}

Expand Down
28 changes: 20 additions & 8 deletions core/server/models/post.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,24 @@ Post = ghostBookshelf.Model.extend({
}
});

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');
});
this.on('destroying', function (model, options) {
return model.load('tags', options)
.then(function (response) {
if (!response.related || !response.related('tags') || !response.related('tags').length) {
return;
}

return Promise.mapSeries(response.related('tags').models, function (tag) {
return baseUtils.tagUpdate.detachTagFromPost(model, tag, options)();
});
})
.then(function () {
if (model.previous('status') === 'published') {
model.emitChange('unpublished');
}

model.emitChange('deleted');
});
});
},

Expand Down Expand Up @@ -654,7 +665,8 @@ Post = ghostBookshelf.Model.extend({
throw new errors.NotFoundError(i18n.t('errors.models.post.noUserFound'));
}

return postCollection.query('where', 'author_id', '=', authorId)
return postCollection
.query('where', 'author_id', '=', authorId)
.fetch(options)
.call('invokeThen', 'destroy', options)
.catch(function (error) {
Expand Down
94 changes: 93 additions & 1 deletion core/test/integration/api/api_users_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ var testUtils = require('../../utils'),
Promise = require('bluebird'),
_ = require('lodash'),

// Stuff we are testing
// Stuff we are testing
models = require('../../../server/models'),
UserAPI = require('../../../server/api/users'),
mail = require('../../../server/api/mail'),
db = require('../../../server/data/db'),

context = testUtils.context,
userIdFor = testUtils.users.ids,
Expand All @@ -21,6 +22,7 @@ describe('Users API', function () {
beforeEach(testUtils.setup(
'users:roles', 'users', 'user:token', 'perms:user', 'perms:role', 'perms:setting', 'perms:init', 'posts'
));

afterEach(testUtils.teardown);

function checkForErrorType(type, done) {
Expand Down Expand Up @@ -692,6 +694,96 @@ describe('Users API', function () {
});

describe('Destroy', function () {
describe('General Tests', function () {
it('ensure posts get deleted', function (done) {
var postIdsToDelete = [], postIsToKeep = [], options = {};

Promise.mapSeries(testUtils.DataGenerator.forKnex.posts, function (post, i) {
post = _.cloneDeep(post);

if (i % 2) {
post.author_id = userIdFor.editor;
post.status = 'published';
post.tags = testUtils.DataGenerator.forKnex.tags.slice(0, 1);
return models.Post.add(post, _.merge({}, options, context.editor));
} else {
post.author_id = userIdFor.author;
post.status = 'published';
post.tags = testUtils.DataGenerator.forKnex.tags.slice(2, 4);
return models.Post.add(post, _.merge({}, options, context.author));
}
}).then(function () {
return models.Post.findAll(_.merge({}, {
context: context.editor.context,
filter: 'author_id:' + userIdFor.editor,
include: ['tags']
}, options));
}).then(function (posts) {
posts.models.length.should.eql(3);
posts.models[0].relations.tags.length.should.eql(1);

_.each(posts.models, function (post) {
postIdsToDelete.push(post.get('id'));
});

return models.Post.findAll(_.merge({
context: context.author.context,
filter: 'author_id:' + userIdFor.author,
include: ['tags']
}, options));
}).then(function (posts) {
posts.models.length.should.eql(3);
posts.models[0].relations.tags.length.should.eql(2);

_.each(posts.models, function (post) {
postIsToKeep.push(post.get('id'));
});

return Promise.mapSeries(postIdsToDelete, function (id) {
return db.knex('posts_tags').where('post_id', id);
});
}).then(function (result) {
_.flatten(result).length.should.eql(3);

return db.knex('tags');
}).then(function (allTags) {
allTags.length.should.eql(5);

return UserAPI.destroy(_.extend({}, context.owner, _.merge({}, options, {id: userIdFor.editor})));
}).then(function () {
return models.User.findOne(_.merge({}, options, {id: userIdFor.editor}));
}).then(function (user) {
should.not.exist(user);
return models.User.findOne(_.merge({}, options, {id: userIdFor.author}));
}).then(function (user) {
should.exist(user);
return models.Post.findAll(_.merge({}, options, {filter: 'author_id:' + userIdFor.editor}));
}).then(function (posts) {
posts.models.length.should.eql(0);

return models.Post.findAll(_.merge({}, options, {filter: 'author_id:' + userIdFor.author}));
}).then(function (posts) {
posts.models.length.should.eql(3);

return Promise.mapSeries(postIdsToDelete, function (id) {
return db.knex('posts_tags').where('post_id', id);
});
}).then(function (result) {
_.flatten(result).length.should.eql(0);

return Promise.mapSeries(postIsToKeep, function (id) {
return db.knex('posts_tags').where('post_id', id);
});
}).then(function (result) {
_.flatten(result).length.should.eql(6);
return db.knex('tags');
}).then(function (allTags) {
allTags.length.should.eql(5);
done();
}).catch(done);
});
});

describe('Owner', function () {
it('CANNOT destroy self', function (done) {
UserAPI.destroy(_.extend({}, context.owner, {id: userIdFor.owner}))
Expand Down

0 comments on commit 9349e99

Please sign in to comment.