Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-04-08 02:52:39 -05:00
Code Issues Activity

🐛 Fixed performance regression introduced in 4.1.0 (#12807)

Browse source 
closes https://github.com/TryGhost/Ghost/issues/12791
closes https://github.com/TryGhost/Team/issues/566

https://github.com/TryGhost/Ghost/pull/12787 introduced a significant performance regression due to a misunderstanding of when Bookshelf calls `.format()` ([related upstream issue](https://github.com/bookshelf/bookshelf/issues/668)). We expected `.format()` to only be called on save but it's also called when Bookshelf performs fetching and eager loading which happens frequently. `.format()` can be a heavy method as it needs to parse and serialize html and markdown so it should be performed as infrequently as possible.

- override `sync()` in the base model so we can call our own `.formatOnWrite()` method to transform attributes on `update` and `insert` operations
  - this was the only feasible location in Bookshelf I could find that is low enough level to not require modifying model instance attributes
  - gives models the option to perform heavy transform operations only when writing to the database compared to the usual `.format()` method that is also called on fetch in many situations
This commit is contained in:
Kevin Ansfield 2021-03-23 09:11:24 +00:00 • committed by GitHub
parent 6d853ff43f
commit 426cbeec0f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 84 additions and 56 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

31
core/server/models/base/index.js
View file

@ -273,6 +273,32 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
proto.initialize.call(this);
},
/**
* Bookshelf's .format() is run when fetching as well as saving.
* We need a way to transform attributes only on save so we override
* .sync() which is run on every database operation where we can
* run any transforms needed only on insert and update operations
*/
sync: function sync() {
const parentSync = proto.sync.apply(this, arguments);
const originalUpdateSync = parentSync.update;
const originalInsertSync = parentSync.insert;
const self = this;
// deep clone attrs to avoid modifying underlying model attributes by reference
parentSync.update = function update(attrs) {
attrs = self.formatOnWrite(_.cloneDeep(attrs));
return originalUpdateSync.apply(this, [attrs]);
};
parentSync.insert = function insert(attrs) {
attrs = self.formatOnWrite(_.cloneDeep(attrs));
return originalInsertSync.apply(this, [attrs]);
};
return parentSync;
},
/**
* Do not call `toJSON`. This can remove properties e.g. password.
* @returns {*}
@ -591,6 +617,11 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
return this.fixDatesWhenSave(attrs);
},
// overridable function for models to format attrs only when saving to db
formatOnWrite: function formatOnWrite(attrs) {
return attrs;
},
// format data and bool when fetching from DB
parse: function parse(attrs) {
return this.fixBools(this.fixDatesWhenFetch(attrs));

79
core/server/models/post.js
View file

@ -80,45 +80,6 @@ Post = ghostBookshelf.Model.extend({
}
},
format() {
const attrs = ghostBookshelf.Model.prototype.format.apply(this, arguments);
// ensure all URLs are stored as transform-ready with __GHOST_URL__ representing config.url
const urlTransformMap = {
mobiledoc: 'mobiledocToTransformReady',
html: 'htmlToTransformReady',
plaintext: 'markdownToTransformReady',
custom_excerpt: 'htmlToTransformReady',
codeinjection_head: 'htmlToTransformReady',
codeinjection_foot: 'htmlToTransformReady',
feature_image: 'toTransformReady',
og_image: 'toTransformReady',
twitter_image: 'toTransformReady',
canonical_url: {
method: 'toTransformReady',
options: {
ignoreProtocol: false
}
}
};
Object.entries(urlTransformMap).forEach(([attr, transform]) => {
let method = transform;
let transformOptions = {};
if (typeof transform === 'object') {
method = transform.method;
transformOptions = transform.options || {};
}
if (attrs[attr]) {
attrs[attr] = urlUtils[method](attrs[attr], transformOptions);
}
});
return attrs;
},
parse() {
const attrs = ghostBookshelf.Model.prototype.parse.apply(this, arguments);
@ -143,6 +104,44 @@ Post = ghostBookshelf.Model.extend({
return attrs;
},
// Alternative to Bookshelf's .format() that is only called when writing to db
formatOnWrite(attrs) {
// Ensure all URLs are stored as transform-ready with __GHOST_URL__ representing config.url
const urlTransformMap = {
mobiledoc: 'mobiledocToTransformReady',
html: 'htmlToTransformReady',
plaintext: 'markdownToTransformReady',
custom_excerpt: 'htmlToTransformReady',
codeinjection_head: 'htmlToTransformReady',
codeinjection_foot: 'htmlToTransformReady',
feature_image: 'toTransformReady',
og_image: 'toTransformReady',
twitter_image: 'toTransformReady',
canonical_url: {
method: 'toTransformReady',
options: {
ignoreProtocol: false
}
}
};
Object.entries(urlTransformMap).forEach(([attrToTransform, transform]) => {
let method = transform;
let transformOptions = {};
if (typeof transform === 'object') {
method = transform.method;
transformOptions = transform.options || {};
}
if (attrs[attrToTransform]) {
attrs[attrToTransform] = urlUtils[method](attrs[attrToTransform], transformOptions);
}
});
return attrs;
},
/**
* The base model keeps only the columns, which are defined in the schema.
* We have to add the relations on top, otherwise bookshelf-relations
@ -372,7 +371,7 @@ Post = ghostBookshelf.Model.extend({
});
},
onSaving: async function onSaving(model, attr, options) {
onSaving: async function onSaving(model, attrs, options) {
options = options || {};
const self = this;

4
core/server/models/posts-meta.js
View file

@ -4,9 +4,7 @@ const urlUtils = require('../../shared/url-utils');
const PostsMeta = ghostBookshelf.Model.extend({
tableName: 'posts_meta',
format() {
const attrs = ghostBookshelf.Model.prototype.format.apply(this, arguments);
formatOnWrite(attrs) {
['og_image', 'twitter_image'].forEach((attr) => {
if (attrs[attr]) {
attrs[attr] = urlUtils.toTransformReady(attrs[attr]);

4
core/server/models/settings.js
View file

@ -146,6 +146,10 @@ Settings = ghostBookshelf.Model.extend({
}
}
return attrs;
},
formatOnWrite(attrs) {
if (attrs.value && ['cover_image', 'logo', 'icon', 'portal_button_icon', 'og_image', 'twitter_image'].includes(attrs.key)) {
attrs.value = urlUtils.toTransformReady(attrs.value);
}

4
core/server/models/snippet.js
View file

@ -4,9 +4,7 @@ const urlUtils = require('../../shared/url-utils');
const Snippet = ghostBookshelf.Model.extend({
tableName: 'snippets',
format() {
const attrs = ghostBookshelf.Model.prototype.format.apply(this, arguments);
formatOnWrite(attrs) {
if (attrs.mobiledoc) {
attrs.mobiledoc = urlUtils.mobiledocToTransformReady(attrs.mobiledoc);
}

4
core/server/models/tag.js
View file

@ -16,9 +16,7 @@ Tag = ghostBookshelf.Model.extend({
};
},
format() {
const attrs = ghostBookshelf.Model.prototype.format.apply(this, arguments);
formatOnWrite(attrs) {
const urlTransformMap = {
feature_image: 'toTransformReady',
og_image: 'toTransformReady',

2
test/regression/models/model_posts_spec.js
View file

@ -1128,7 +1128,7 @@ describe('Post Model', function () {
}).catch(done);
});
it('uses parse/transform to store urls as transform-ready and read as absolute ', function (done) {
it('it stores urls as transform-ready and reads as absolute', function (done) {
const post = {
title: 'Absolute->Transform-ready URL Transform Test',
mobiledoc: '{"version":"0.3.1","atoms":[],"cards":[["image",{"src":"http://127.0.0.1:2369/content/images/card.jpg"}]],"markups":[["a",["href","http://127.0.0.1:2369/test"]]],"sections":[[1,"p",[[0,[0],1,"Testing"]]],[10,0]]}',

12
test/unit/models/settings_spec.js
View file

@ -185,22 +185,22 @@ describe('Unit: models/settings', function () {
it('transforms urls when persisting to db', function () {
const setting = models.Settings.forge();
let returns = setting.format({key: 'cover_image', value: 'http://127.0.0.1:2369/cover_image.png', type: 'string'});
let returns = setting.formatOnWrite({key: 'cover_image', value: 'http://127.0.0.1:2369/cover_image.png', type: 'string'});
should.equal(returns.value, '__GHOST_URL__/cover_image.png');
returns = setting.format({key: 'logo', value: 'http://127.0.0.1:2369/logo.png', type: 'string'});
returns = setting.formatOnWrite({key: 'logo', value: 'http://127.0.0.1:2369/logo.png', type: 'string'});
should.equal(returns.value, '__GHOST_URL__/logo.png');
returns = setting.format({key: 'icon', value: 'http://127.0.0.1:2369/icon.png', type: 'string'});
returns = setting.formatOnWrite({key: 'icon', value: 'http://127.0.0.1:2369/icon.png', type: 'string'});
should.equal(returns.value, '__GHOST_URL__/icon.png');
returns = setting.format({key: 'portal_button_icon', value: 'http://127.0.0.1:2369/portal_button_icon.png', type: 'string'});
returns = setting.formatOnWrite({key: 'portal_button_icon', value: 'http://127.0.0.1:2369/portal_button_icon.png', type: 'string'});
should.equal(returns.value, '__GHOST_URL__/portal_button_icon.png');
returns = setting.format({key: 'og_image', value: 'http://127.0.0.1:2369/og_image.png', type: 'string'});
returns = setting.formatOnWrite({key: 'og_image', value: 'http://127.0.0.1:2369/og_image.png', type: 'string'});
should.equal(returns.value, '__GHOST_URL__/og_image.png');
returns = setting.format({key: 'twitter_image', value: 'http://127.0.0.1:2369/twitter_image.png', type: 'string'});
returns = setting.formatOnWrite({key: 'twitter_image', value: 'http://127.0.0.1:2369/twitter_image.png', type: 'string'});
should.equal(returns.value, '__GHOST_URL__/twitter_image.png');
});
});