From 40ee2043e007b340075e4a93053330f917144faf Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Tue, 7 May 2024 15:24:20 +0100 Subject: [PATCH] Reduced Admin search re-indexes (#20154) closes https://linear.app/tryghost/issue/MOM-97 The 30s search content expiry didn't really make sense and caused unnecessary delays and server load now that search will be more widely used within the editor. - replaced concept of time-based expiry with explicit expiry - content still fetched on query if not already loaded or marked as stale - added `.expireContent()` method on search service to allow explicit expiry - updated editor to pre-fetch search content when not already loaded or marked as stale - removes delay when first using internal linking search inside the editor - updated post model to expire search content on save - expires on published post save or delete - expires on publish and unpublish - updated tag model to expire content on create/save/delete - only expires when name or url is changed - updated user model to expire on save/delete - only expires when name or url is changed - does not handle creation because that's done server-side via invites --- ghost/admin/app/controllers/lexical-editor.js | 4 + ghost/admin/app/models/post.js | 14 ++++ ghost/admin/app/models/tag.js | 20 ++++- ghost/admin/app/models/user.js | 33 ++++++-- ghost/admin/app/routes/lexical-editor/edit.js | 2 +- ghost/admin/app/serializers/tag.js | 1 + ghost/admin/app/serializers/user.js | 9 +++ ghost/admin/app/services/search.js | 20 +++-- ghost/admin/mirage/config/tags.js | 5 +- ghost/admin/mirage/serializers/post.js | 18 +++++ ghost/admin/mirage/serializers/tag.js | 9 ++- ghost/admin/mirage/serializers/user.js | 27 ++++--- .../acceptance/editor/publish-flow-test.js | 12 +++ .../tests/integration/models/post-test.js | 80 +++++++++++++++++++ .../tests/integration/models/tag-test.js | 71 ++++++++++++++++ .../tests/integration/models/user-test.js | 63 +++++++++++++++ 16 files changed, 350 insertions(+), 38 deletions(-) create mode 100644 ghost/admin/tests/integration/models/post-test.js create mode 100644 ghost/admin/tests/integration/models/tag-test.js create mode 100644 ghost/admin/tests/integration/models/user-test.js diff --git a/ghost/admin/app/controllers/lexical-editor.js b/ghost/admin/app/controllers/lexical-editor.js index 0b3b68ce30..e9644d7759 100644 --- a/ghost/admin/app/controllers/lexical-editor.js +++ b/ghost/admin/app/controllers/lexical-editor.js @@ -112,6 +112,7 @@ export default class LexicalEditorController extends Controller { @service notifications; @service router; @service slugGenerator; + @service search; @service session; @service settings; @service ui; @@ -887,9 +888,12 @@ export default class LexicalEditorController extends Controller { @restartableTask *backgroundLoaderTask() { yield this.store.query('snippet', {limit: 'all'}); + if (this.post.displayName === 'page' && this.feature.get('collections') && this.feature.get('collectionsCard')) { yield this.store.query('collection', {limit: 'all'}); } + + this.search.refreshContentTask.perform(); this.syncMobiledocSnippets(); } diff --git a/ghost/admin/app/models/post.js b/ghost/admin/app/models/post.js index 3cbc4b01a7..1ffb06d8d0 100644 --- a/ghost/admin/app/models/post.js +++ b/ghost/admin/app/models/post.js @@ -71,6 +71,7 @@ export default Model.extend(Comparable, ValidationEngine, { feature: service(), ghostPaths: service(), clock: service(), + search: service(), settings: service(), membersUtils: service(), @@ -439,5 +440,18 @@ export default Model.extend(Comparable, ValidationEngine, { let publishedAtBlogTZ = this.publishedAtBlogTZ; let publishedAtUTC = publishedAtBlogTZ ? publishedAtBlogTZ.utc() : null; this.set('publishedAtUTC', publishedAtUTC); + }, + + // when a published post is updated, unpublished, or deleted we expire the search content cache + save() { + const [oldStatus] = this.changedAttributes().status || []; + + return this._super(...arguments).then((res) => { + if (this.status === 'published' || oldStatus === 'published') { + this.search.expireContent(); + } + + return res; + }); } }); diff --git a/ghost/admin/app/models/tag.js b/ghost/admin/app/models/tag.js index 62bb948553..fdbef01c9d 100644 --- a/ghost/admin/app/models/tag.js +++ b/ghost/admin/app/models/tag.js @@ -4,10 +4,13 @@ import {equal} from '@ember/object/computed'; import {inject as service} from '@ember/service'; export default Model.extend(ValidationEngine, { + search: service(), + validationType: 'tag', name: attr('string'), slug: attr('string'), + url: attr('string'), description: attr('string'), metaTitle: attr('string'), metaDescription: attr('string'), @@ -40,9 +43,22 @@ export default Model.extend(ValidationEngine, { }, save() { - if (this.get('changedAttributes.name') && !this.isDeleted) { + const nameChanged = !!this.changedAttributes().name; + + if (nameChanged && !this.isDeleted) { this.updateVisibility(); } - return this._super(...arguments); + + const {url} = this; + + return this._super(...arguments).then((savedModel) => { + const urlChanged = url !== savedModel.url; + + if (nameChanged || urlChanged || this.isDeleted) { + this.search.expireContent(); + } + + return savedModel; + }); } }); diff --git a/ghost/admin/app/models/user.js b/ghost/admin/app/models/user.js index 8374974d93..a2c3b2f96a 100644 --- a/ghost/admin/app/models/user.js +++ b/ghost/admin/app/models/user.js @@ -10,10 +10,19 @@ import {inject as service} from '@ember/service'; import {task} from 'ember-concurrency'; export default BaseModel.extend(ValidationEngine, { + ajax: service(), + ghostPaths: service(), + notifications: service(), + search: service(), + session: service(), + + config: inject(), + validationType: 'user', name: attr('string'), slug: attr('string'), + url: attr('string'), email: attr('string'), profileImage: attr('string'), coverImage: attr('string'), @@ -44,12 +53,6 @@ export default BaseModel.extend(ValidationEngine, { mentionNotifications: attr(), milestoneNotifications: attr(), donationNotifications: attr(), - ghostPaths: service(), - ajax: service(), - session: service(), - notifications: service(), - - config: inject(), // TODO: Once client-side permissions are in place, // remove the hard role check. @@ -141,5 +144,21 @@ export default BaseModel.extend(ValidationEngine, { } catch (error) { this.notifications.showAPIError(error, {key: 'user.change-password'}); } - }).drop() + }).drop(), + + save() { + const nameChanged = !!this.changedAttributes().name; + + const {url} = this; + + return this._super(...arguments).then((savedModel) => { + const urlChanged = url !== savedModel.url; + + if (nameChanged || urlChanged || this.isDeleted) { + this.search.expireContent(); + } + + return savedModel; + }); + } }); diff --git a/ghost/admin/app/routes/lexical-editor/edit.js b/ghost/admin/app/routes/lexical-editor/edit.js index 2a6079b699..d4cf641786 100644 --- a/ghost/admin/app/routes/lexical-editor/edit.js +++ b/ghost/admin/app/routes/lexical-editor/edit.js @@ -37,7 +37,7 @@ export default class EditRoute extends AuthenticatedRoute { const records = await this.store.query(modelName, query); let post = records.firstObject; - // CASE: Post is in mobiledoc — convert to lexical or redirect + // CASE: Post is in mobiledoc — convert to lexical if (post.mobiledoc) { post = await post.save({adapterOptions: {convertToLexical: 1}}); } diff --git a/ghost/admin/app/serializers/tag.js b/ghost/admin/app/serializers/tag.js index 49ba8efa45..b807451987 100644 --- a/ghost/admin/app/serializers/tag.js +++ b/ghost/admin/app/serializers/tag.js @@ -13,6 +13,7 @@ export default class TagSerializer extends ApplicationSerializer { // Properties that exist on the model but we don't want sent in the payload delete json.count; + delete json.url; return json; } diff --git a/ghost/admin/app/serializers/user.js b/ghost/admin/app/serializers/user.js index 04bfd38a43..bd7bde4569 100644 --- a/ghost/admin/app/serializers/user.js +++ b/ghost/admin/app/serializers/user.js @@ -19,4 +19,13 @@ export default class UserSerializer extends ApplicationSerializer.extend(Embedde return super.extractSingle(...arguments); } + + serialize() { + const json = super.serialize(...arguments); + + // Read-only virtual properties + delete json.url; + + return json; + } } diff --git a/ghost/admin/app/services/search.js b/ghost/admin/app/services/search.js index ccebac2911..a308a4cd9f 100644 --- a/ghost/admin/app/services/search.js +++ b/ghost/admin/app/services/search.js @@ -1,5 +1,6 @@ import RSVP from 'rsvp'; import Service from '@ember/service'; +import {action} from '@ember/object'; import {isBlank, isEmpty} from '@ember/utils'; import {pluralize} from 'ember-inflector'; import {inject as service} from '@ember/service'; @@ -11,8 +12,7 @@ export default class SearchService extends Service { @service store; content = []; - contentExpiresAt = false; - contentExpiry = 30000; + isContentStale = true; searchables = [ { @@ -45,6 +45,11 @@ export default class SearchService extends Service { } ]; + @action + expireContent() { + this.isContentStale = true; + } + @task({restartable: true}) *searchTask(term) { if (isBlank(term)) { @@ -92,14 +97,13 @@ export default class SearchService extends Service { } @task({drop: true}) - *refreshContentTask() { - const now = new Date(); - const contentExpiresAt = this.contentExpiresAt; - - if (contentExpiresAt > now) { + *refreshContentTask({forceRefresh = false} = {}) { + if (!forceRefresh && !this.isContentStale) { return true; } + this.isContentStale = true; + const content = []; const promises = this.searchables.map(searchable => this._loadSearchable(searchable, content)); @@ -111,7 +115,7 @@ export default class SearchService extends Service { console.error(error); } - this.contentExpiresAt = new Date(now.getTime() + this.contentExpiry); + this.isContentStale = false; } async _loadSearchable(searchable, content) { diff --git a/ghost/admin/mirage/config/tags.js b/ghost/admin/mirage/config/tags.js index 255c4a45f4..c4702783f7 100644 --- a/ghost/admin/mirage/config/tags.js +++ b/ghost/admin/mirage/config/tags.js @@ -14,14 +14,13 @@ export default function mockTags(server) { return tags.create(attrs); }); - server.get('/tags/', paginatedResponse('tags')); - server.get('/tags/slug/:slug/', function ({tags}, {params: {slug}}) { // TODO: remove post_count unless requested? return tags.findBy({slug}); }); + server.get('/tags/', paginatedResponse('tags')); + server.get('/tags/:id/'); server.put('/tags/:id/'); - server.del('/tags/:id/'); } diff --git a/ghost/admin/mirage/serializers/post.js b/ghost/admin/mirage/serializers/post.js index 1c9d7f5a13..a8e4f2bda4 100644 --- a/ghost/admin/mirage/serializers/post.js +++ b/ghost/admin/mirage/serializers/post.js @@ -10,5 +10,23 @@ export default BaseSerializer.extend({ includes.push('authors'); return includes; + }, + + serialize(postModelOrCollection, request) { + const updatePost = (post) => { + if (post.status === 'published') { + post.update('url', `http://localhost:4200/${post.slug}/`); + } else { + post.update('url', `http://localhost:4200/p/`); + } + }; + + if (this.isModel(postModelOrCollection)) { + updatePost(postModelOrCollection); + } else { + postModelOrCollection.models.forEach(updatePost); + } + + return BaseSerializer.prototype.serialize.call(this, postModelOrCollection, request); } }); diff --git a/ghost/admin/mirage/serializers/tag.js b/ghost/admin/mirage/serializers/tag.js index 6906521de0..76f90f85ed 100644 --- a/ghost/admin/mirage/serializers/tag.js +++ b/ghost/admin/mirage/serializers/tag.js @@ -1,16 +1,17 @@ import BaseSerializer from './application'; export default BaseSerializer.extend({ - // make the tag.count.posts value dynamic + // make the tag.count.posts and url values dynamic serialize(tagModelOrCollection, request) { - let updatePostCount = (tag) => { + let updatePost = (tag) => { tag.update('count', {posts: tag.postIds.length}); + tag.update('url', `http://localhost:4200/tag/${tag.slug}/`); }; if (this.isModel(tagModelOrCollection)) { - updatePostCount(tagModelOrCollection); + updatePost(tagModelOrCollection); } else { - tagModelOrCollection.models.forEach(updatePostCount); + tagModelOrCollection.models.forEach(updatePost); } return BaseSerializer.prototype.serialize.call(this, tagModelOrCollection, request); diff --git a/ghost/admin/mirage/serializers/user.js b/ghost/admin/mirage/serializers/user.js index a9e929d935..1627c2660a 100644 --- a/ghost/admin/mirage/serializers/user.js +++ b/ghost/admin/mirage/serializers/user.js @@ -1,5 +1,4 @@ import BaseSerializer from './application'; -import {RestSerializer} from 'miragejs'; export default BaseSerializer.extend({ embed: true, @@ -12,19 +11,21 @@ export default BaseSerializer.extend({ return []; }, - serialize(object, request) { - if (this.isCollection(object)) { - return BaseSerializer.prototype.serialize.call(this, object, request); + serialize(userModelOrCollection, request) { + const updateUser = (user) => { + user.update('url', `http://localhost:4200/author/${user.slug}/`); + + if (user.postCount) { + user.update('count', {posts: user.posts.models.length}); + } + }; + + if (this.isModel(userModelOrCollection)) { + updateUser(userModelOrCollection); + } else { + userModelOrCollection.models.forEach(updateUser); } - let {user} = RestSerializer.prototype.serialize.call(this, object, request); - - if (object.postCount) { - let posts = object.posts.models.length; - - user.count = {posts}; - } - - return {users: [user]}; + return BaseSerializer.prototype.serialize.call(this, userModelOrCollection, request); } }); diff --git a/ghost/admin/tests/acceptance/editor/publish-flow-test.js b/ghost/admin/tests/acceptance/editor/publish-flow-test.js index 41b262805b..5cbb3c95f6 100644 --- a/ghost/admin/tests/acceptance/editor/publish-flow-test.js +++ b/ghost/admin/tests/acceptance/editor/publish-flow-test.js @@ -51,6 +51,18 @@ describe('Acceptance: Publish flow', function () { expect(find('[data-test-modal="publish-flow"]'), 'publish flow modal').to.not.exist; }); + it('populates search index when opening', async function () { + await loginAsRole('Administrator', this.server); + + const search = this.owner.lookup('service:search'); + expect(search.isContentStale).to.be.true; + + const post = this.server.create('post', {status: 'draft'}); + await visit(`/editor/post/${post.id}`); + + expect(search.isContentStale).to.be.false; + }); + it('handles timezones correctly when scheduling'); // email unavailable state occurs when diff --git a/ghost/admin/tests/integration/models/post-test.js b/ghost/admin/tests/integration/models/post-test.js new file mode 100644 index 0000000000..506521b0d6 --- /dev/null +++ b/ghost/admin/tests/integration/models/post-test.js @@ -0,0 +1,80 @@ +import {describe, it} from 'mocha'; +import {expect} from 'chai'; +import {setupMirage} from 'ember-cli-mirage/test-support'; +import {setupTest} from 'ember-mocha'; + +describe('Integration: Model: post', function () { + const hooks = setupTest(); + setupMirage(hooks); + + let store; + + beforeEach(function () { + store = this.owner.lookup('service:store'); + }); + + describe('search expiry', function () { + let search; + + beforeEach(function () { + search = this.owner.lookup('service:search'); + search.isContentStale = false; + }); + + it('expires on published save', async function () { + const serverPost = this.server.create('post', {status: 'published'}); + + const postModel = await store.find('post', serverPost.id); + await postModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.true; + }); + + it('expires on published delete', async function () { + const serverPost = this.server.create('post', {status: 'published'}); + + const postModel = await store.find('post', serverPost.id); + await postModel.destroyRecord(); + + expect(search.isContentStale, 'stale flag after delete').to.be.true; + }); + + it('expires when publishing', async function () { + const serverPost = this.server.create('post', {status: 'draft'}); + + const postModel = await store.find('post', serverPost.id); + postModel.status = 'published'; + await postModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.true; + }); + + it('expires when unpublishing', async function () { + const serverPost = this.server.create('post', {status: 'published'}); + + const postModel = await store.find('post', serverPost.id); + postModel.status = 'draft'; + await postModel.save(); + + expect(search.isContentStale, 'stale flag after unpublish').to.be.true; + }); + + it('does not expire on draft save', async function () { + const serverPost = this.server.create('post', {status: 'draft'}); + + const postModel = await store.find('post', serverPost.id); + await postModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.false; + }); + + it('does not expire on draft delete', async function () { + const serverPost = this.server.create('post', {status: 'draft'}); + + const postModel = await store.find('post', serverPost.id); + await postModel.destroyRecord(); + + expect(search.isContentStale, 'stale flag after save').to.be.false; + }); + }); +}); diff --git a/ghost/admin/tests/integration/models/tag-test.js b/ghost/admin/tests/integration/models/tag-test.js new file mode 100644 index 0000000000..47cabe7de8 --- /dev/null +++ b/ghost/admin/tests/integration/models/tag-test.js @@ -0,0 +1,71 @@ +import {describe, it} from 'mocha'; +import {expect} from 'chai'; +import {setupMirage} from 'ember-cli-mirage/test-support'; +import {setupTest} from 'ember-mocha'; + +describe('Integration: Model: tag', function () { + const hooks = setupTest(); + setupMirage(hooks); + + let store; + + beforeEach(function () { + store = this.owner.lookup('service:store'); + }); + + describe('search expiry', function () { + let search; + + beforeEach(function () { + search = this.owner.lookup('service:search'); + search.isContentStale = false; + }); + + it('expires on create', async function () { + const tagModel = await store.createRecord('tag'); + tagModel.name = 'Test tag'; + await tagModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.true; + }); + + it('expires on delete', async function () { + const serverTag = this.server.create('tag'); + + const tagModel = await store.find('tag', serverTag.id); + await tagModel.destroyRecord(); + + expect(search.isContentStale, 'stale flag after delete').to.be.true; + }); + + it('expires when name changed', async function () { + const serverTag = this.server.create('tag'); + + const tagModel = await store.find('tag', serverTag.id); + tagModel.name = 'New name'; + await tagModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.true; + }); + + it('expires when url changed', async function () { + const serverTag = this.server.create('tag'); + + const tagModel = await store.find('tag', serverTag.id); + tagModel.slug = 'new-slug'; + await tagModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.true; + }); + + it('does not expire on non-name change', async function () { + const serverTag = this.server.create('tag'); + + const tagModel = await store.find('tag', serverTag.id); + tagModel.description = 'New description'; + await tagModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.false; + }); + }); +}); diff --git a/ghost/admin/tests/integration/models/user-test.js b/ghost/admin/tests/integration/models/user-test.js new file mode 100644 index 0000000000..982808b309 --- /dev/null +++ b/ghost/admin/tests/integration/models/user-test.js @@ -0,0 +1,63 @@ +import {describe, it} from 'mocha'; +import {expect} from 'chai'; +import {setupMirage} from 'ember-cli-mirage/test-support'; +import {setupTest} from 'ember-mocha'; + +describe('Integration: Model: user', function () { + const hooks = setupTest(); + setupMirage(hooks); + + let store; + + beforeEach(function () { + store = this.owner.lookup('service:store'); + }); + + describe('search expiry', function () { + let search; + + beforeEach(function () { + search = this.owner.lookup('service:search'); + search.isContentStale = false; + }); + + it('expires on delete', async function () { + const serverUser = this.server.create('user'); + + const userModel = await store.find('user', serverUser.id); + await userModel.destroyRecord(); + + expect(search.isContentStale, 'stale flag after delete').to.be.true; + }); + + it('expires when name changed', async function () { + const serverUser = this.server.create('user'); + + const userModel = await store.find('user', serverUser.id); + userModel.name = 'New name'; + await userModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.true; + }); + + it('expires when url changed', async function () { + const serverUser = this.server.create('user'); + + const userModel = await store.find('user', serverUser.id); + userModel.slug = 'new-slug'; + await userModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.true; + }); + + it('does not expire on non-name change', async function () { + const serverUser = this.server.create('user'); + + const userModel = await store.find('user', serverUser.id); + userModel.description = 'New description'; + await userModel.save(); + + expect(search.isContentStale, 'stale flag after save').to.be.false; + }); + }); +});