From 523337cabafc7d6ae2db4c444a348abb97b74132 Mon Sep 17 00:00:00 2001 From: Daniel Lockyer Date: Mon, 10 Feb 2025 12:58:34 +0100 Subject: [PATCH] Removed experimental URL cache fix https://linear.app/ghost/issue/ENG-1803/remove-url-cache-code - this feature is experimental and was designed in order to speed up our URL service init by storing a cache of the object - however, it was never really finished and we've had a few bugs with it - to avoid further issues, this commit removes it - along the way, I've discovered that our tests REQUIRE the URL cache in order to pass, which I thought was weird. Turns out it's because they incorrectly set cache values into the URL service that are outdated, which meant our snapshots are wrong - this is why several of the snapshots have been updated, because URLs have changed - sadly this commit touches so many files, but this feature was really spread around the codebase - this technically removes test util API support for a split backend/frontend but that wasn't properly finished. I'd like to get that working, but first we need to unpick the mess we've got ourselves into --- ghost/core/core/boot.js | 8 +- .../server/services/url/LocalFileCache.js | 75 -- .../core/server/services/url/UrlService.js | 52 +- ghost/core/core/server/services/url/index.js | 22 +- .../config/env/config.testing-browser.json | 3 +- .../config/env/config.testing-mysql.json | 3 +- .../shared/config/env/config.testing.json | 3 +- ghost/core/core/shared/labs.js | 1 - .../admin/__snapshots__/config.test.js.snap | 1 - .../admin/__snapshots__/pages.test.js.snap | 4 +- .../admin/__snapshots__/posts.test.js.snap | 4 +- ghost/core/test/e2e-api/admin/sso.test.js | 2 + ghost/core/test/e2e-api/admin/utils.js | 7 +- .../content/__snapshots__/posts.test.js.snap | 6 +- ghost/core/test/e2e-api/content/utils.js | 7 +- .../__snapshots__/posts.test.js.snap | 4 +- .../__snapshots__/cards.test.js.snap | 4 +- .../__snapshots__/authentication.test.js.snap | 12 +- ghost/core/test/regression/api/admin/utils.js | 7 +- .../core/test/regression/api/content/utils.js | 7 +- .../services/url/LocalFileCache.test.js | 88 -- .../test/unit/shared/config/loader.test.js | 1 - ghost/core/test/unit/shared/labs.test.js | 10 +- ghost/core/test/utils/e2e-framework.js | 36 +- ghost/core/test/utils/e2e-utils.js | 2 +- .../test/utils/fixtures/urls/resources.json | 935 ------------------ ghost/core/test/utils/fixtures/urls/urls.json | 928 ----------------- ghost/core/test/utils/url-service-utils.js | 4 +- 28 files changed, 55 insertions(+), 2181 deletions(-) delete mode 100644 ghost/core/core/server/services/url/LocalFileCache.js delete mode 100644 ghost/core/test/unit/server/services/url/LocalFileCache.test.js delete mode 100644 ghost/core/test/utils/fixtures/urls/resources.json delete mode 100644 ghost/core/test/utils/fixtures/urls/urls.json diff --git a/ghost/core/core/boot.js b/ghost/core/core/boot.js index e5ce1b21b00..cf7e75a741b 100644 --- a/ghost/core/core/boot.js +++ b/ghost/core/core/boot.js @@ -79,9 +79,8 @@ async function initDatabase({config}) { * @param {object} options.ghostServer * @param {object} options.config * @param {object} options.bootLogger - * @param {boolean} options.frontend */ -async function initCore({ghostServer, config, bootLogger, frontend}) { +async function initCore({ghostServer, config, bootLogger}) { debug('Begin: initCore'); // URL Utils is a bit slow, put it here so the timing is visible separate from models @@ -118,8 +117,7 @@ async function initCore({ghostServer, config, bootLogger, frontend}) { onFinished: () => { bootLogger.metric('url-service', urlServiceStart); bootLogger.log('URL Service Ready'); - }, - urlCache: !frontend // hacky parameter to make the cache initialization kick in as we can't initialize labs before the boot + } }); debug('End: Url Service'); @@ -563,7 +561,7 @@ async function bootGhost({backend = true, frontend = true, server = true} = {}) // Step 4 - Load Ghost with all its services debug('Begin: Load Ghost Services & Apps'); - await initCore({ghostServer, config, bootLogger, frontend}); + await initCore({ghostServer, config, bootLogger}); // Instrument the knex instance and connection pool if prometheus is enabled // Needs to be after initCore because the pool is destroyed and recreated in initCore, which removes the event listeners diff --git a/ghost/core/core/server/services/url/LocalFileCache.js b/ghost/core/core/server/services/url/LocalFileCache.js deleted file mode 100644 index 70eb5aacb65..00000000000 --- a/ghost/core/core/server/services/url/LocalFileCache.js +++ /dev/null @@ -1,75 +0,0 @@ -const fs = require('fs-extra'); -const path = require('path'); - -class LocalFileCache { - /** - * @param {Object} options - * @param {String} options.storagePath - cached storage path - * @param {Boolean} options.writeDisabled - controls if cache can write - */ - constructor({storagePath, writeDisabled}) { - const urlsStoragePath = path.join(storagePath, 'urls.json'); - const resourcesCachePath = path.join(storagePath, 'resources.json'); - - this.storagePaths = { - urls: urlsStoragePath, - resources: resourcesCachePath - }; - this.writeDisabled = writeDisabled; - } - - /** - * Handles reading and parsing JSON from the filesystem. - * In case the file is corrupted or does not exist, returns null. - * @param {String} filePath path to read from - * @returns {Promise} - * @private - */ - async readCacheFile(filePath) { - let cacheExists = false; - let cacheData = null; - - try { - await fs.stat(filePath); - cacheExists = true; - } catch (e) { - cacheExists = false; - } - - if (cacheExists) { - try { - const cacheFile = await fs.readFile(filePath, 'utf8'); - cacheData = JSON.parse(cacheFile); - } catch (e) { - //noop as we'd start a long boot process if there are any errors in the file - } - } - - return cacheData; - } - - /** - * - * @param {'urls'|'resources'} type - * @returns {Promise} - */ - async read(type) { - return await this.readCacheFile(this.storagePaths[type]); - } - - /** - * - * @param {'urls'|'resources'} type of data to persist - * @param {Object} data - data to be persisted - * @returns {Promise} - */ - async write(type, data) { - if (this.writeDisabled) { - return null; - } - - return fs.writeFile(this.storagePaths[type], JSON.stringify(data, null, 4)); - } -} - -module.exports = LocalFileCache; diff --git a/ghost/core/core/server/services/url/UrlService.js b/ghost/core/core/server/services/url/UrlService.js index 0aba0c1b5ef..a8f934b1daa 100644 --- a/ghost/core/core/server/services/url/UrlService.js +++ b/ghost/core/core/server/services/url/UrlService.js @@ -1,7 +1,6 @@ const debug = require('@tryghost/debug')('services:url:service'); const _ = require('lodash'); const errors = require('@tryghost/errors'); -const labs = require('../../../shared/labs'); const UrlGenerator = require('./UrlGenerator'); const Queue = require('./Queue'); const Urls = require('./Urls'); @@ -15,16 +14,8 @@ const resourcesConfig = require('./config'); * It will tell you if the url generation is in progress or not. */ class UrlService { - /** - * - * @param {Object} [options] - * @param {Object} [options.cache] - cache handler instance - * @param {Function} [options.cache.read] - read cache by type - * @param {Function} [options.cache.write] - write into cache by type - */ - constructor({cache} = {}) { + constructor() { this.utils = urlUtils; - this.cache = cache; this.onFinished = null; this.finished = false; this.urlGenerators = []; @@ -304,44 +295,21 @@ class UrlService { * @description Initializes components needed for the URL Service to function * @param {Object} options * @param {Function} [options.onFinished] - callback when url generation is finished - * @param {Boolean} [options.urlCache] - whether to init using url cache or not */ - async init({onFinished, urlCache} = {}) { + async init({onFinished} = {}) { this.onFinished = onFinished; - let persistedUrls; - let persistedResources; - - if (this.cache && (labs.isSet('urlCache') || urlCache)) { - persistedUrls = await this.cache.read('urls'); - persistedResources = await this.cache.read('resources'); - } - - if (persistedUrls && persistedResources) { - this.urls.urls = persistedUrls; - this.resources.data = persistedResources; - this.resources.initEventListeners(); - - this._onQueueEnded('init'); - } else { - this.resources.initEventListeners(); - await this.resources.fetchResources(); - // CASE: all resources are fetched, start the queue - this.queue.start({ - event: 'init', - tolerance: 100, - requiredSubscriberCount: 1 - }); - } + this.resources.initEventListeners(); + await this.resources.fetchResources(); + // CASE: all resources are fetched, start the queue + this.queue.start({ + event: 'init', + tolerance: 100, + requiredSubscriberCount: 1 + }); } async shutdown() { - if (!labs.isSet('urlCache')) { - return null; - } - - await this.cache.write('urls', this.urls.urls); - await this.cache.write('resources', this.resources.getAll()); } /** diff --git a/ghost/core/core/server/services/url/index.js b/ghost/core/core/server/services/url/index.js index 7fefa323a63..f4ded79f787 100644 --- a/ghost/core/core/server/services/url/index.js +++ b/ghost/core/core/server/services/url/index.js @@ -1,26 +1,6 @@ -const config = require('../../../shared/config'); -const LocalFileCache = require('./LocalFileCache'); const UrlService = require('./UrlService'); -// NOTE: instead of a path we could give UrlService a "data-resolver" of some sort -// so it doesn't have to contain the logic to read data at all. This would be -// a possible improvement in the future -let writeDisabled = false; -let storagePath = config.getContentPath('data'); - -// TODO: remove this hack in favor of loading from the content path when it's possible to do so -// by mocking content folders in pre-boot phase -if (process.env.NODE_ENV.startsWith('test')){ - storagePath = config.get('paths').urlCache; - - // NOTE: prevents test suites from overwriting cache fixtures. - // A better solution would be injecting a different implementation of the - // cache based on the environment, this approach should do the trick for now - writeDisabled = true; -} - -const cache = new LocalFileCache({storagePath, writeDisabled}); -const urlService = new UrlService({cache}); +const urlService = new UrlService(); // Singleton module.exports = urlService; diff --git a/ghost/core/core/shared/config/env/config.testing-browser.json b/ghost/core/core/shared/config/env/config.testing-browser.json index 30daa9c3f1d..698b33cb75d 100644 --- a/ghost/core/core/shared/config/env/config.testing-browser.json +++ b/ghost/core/core/shared/config/env/config.testing-browser.json @@ -79,8 +79,7 @@ "useMinFiles": false, "paths": { "fixtures": "test/utils/fixtures/fixtures", - "defaultSettings": "test/utils/fixtures/default-settings-browser.json", - "urlCache": "test/utils/fixtures/urls" + "defaultSettings": "test/utils/fixtures/default-settings-browser.json" }, "enableDeveloperExperiments": true } diff --git a/ghost/core/core/shared/config/env/config.testing-mysql.json b/ghost/core/core/shared/config/env/config.testing-mysql.json index 84102e76ab9..4f8bcc5b673 100644 --- a/ghost/core/core/shared/config/env/config.testing-mysql.json +++ b/ghost/core/core/shared/config/env/config.testing-mysql.json @@ -80,7 +80,6 @@ "useMinFiles": false, "paths": { "fixtures": "test/utils/fixtures/fixtures", - "defaultSettings": "test/utils/fixtures/default-settings.json", - "urlCache": "test/utils/fixtures/urls" + "defaultSettings": "test/utils/fixtures/default-settings.json" } } diff --git a/ghost/core/core/shared/config/env/config.testing.json b/ghost/core/core/shared/config/env/config.testing.json index 5f9873de0e7..2d70941f7e7 100644 --- a/ghost/core/core/shared/config/env/config.testing.json +++ b/ghost/core/core/shared/config/env/config.testing.json @@ -76,7 +76,6 @@ "useMinFiles": false, "paths": { "fixtures": "test/utils/fixtures/fixtures", - "defaultSettings": "test/utils/fixtures/default-settings.json", - "urlCache": "test/utils/fixtures/urls" + "defaultSettings": "test/utils/fixtures/default-settings.json" } } diff --git a/ghost/core/core/shared/labs.js b/ghost/core/core/shared/labs.js index 8b0d2e00309..186b759cf2d 100644 --- a/ghost/core/core/shared/labs.js +++ b/ghost/core/core/shared/labs.js @@ -44,7 +44,6 @@ const BETA_FEATURES = [ const ALPHA_FEATURES = [ 'NestPlayground', - 'urlCache', 'emailCustomization', 'mailEvents', 'collectionsCard', diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap index ad7ee1480d9..cd778295eac 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snap @@ -31,7 +31,6 @@ Object { "postsX": true, "stripeAutomaticTax": true, "themeErrorsNotification": true, - "urlCache": true, "webmentions": true, }, "mail": "", diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/pages.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/pages.test.js.snap index fd6baad54e9..5f0fce2d0c8 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/pages.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/pages.test.js.snap @@ -647,7 +647,7 @@ exports[`Pages API Convert can convert a mobiledoc page to lexical 2: [headers] Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "4063", + "content-length": "4069", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -753,7 +753,7 @@ exports[`Pages API Convert can convert a mobiledoc page to lexical 4: [headers] Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "4063", + "content-length": "4069", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snap index 517a2ffb9cf..68172aa42cb 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/posts.test.js.snap @@ -874,7 +874,7 @@ exports[`Posts API Convert can convert a mobiledoc post to lexical 2: [headers] Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "4100", + "content-length": "4108", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -983,7 +983,7 @@ exports[`Posts API Convert can convert a mobiledoc post to lexical 4: [headers] Object { "access-control-allow-origin": "http://127.0.0.1:2369", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": "4100", + "content-length": "4108", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, diff --git a/ghost/core/test/e2e-api/admin/sso.test.js b/ghost/core/test/e2e-api/admin/sso.test.js index 068717f278f..3bb3c715506 100644 --- a/ghost/core/test/e2e-api/admin/sso.test.js +++ b/ghost/core/test/e2e-api/admin/sso.test.js @@ -9,6 +9,8 @@ describe('SSO API', function () { let agent; before(async function () { + models.init(); + // Configure mock SSO adapter that always returns owner const owner = await models.User.getOwnerUser(); diff --git a/ghost/core/test/e2e-api/admin/utils.js b/ghost/core/test/e2e-api/admin/utils.js index 5b59e1d11ca..9110cd6a34e 100644 --- a/ghost/core/test/e2e-api/admin/utils.js +++ b/ghost/core/test/e2e-api/admin/utils.js @@ -243,11 +243,6 @@ module.exports = { }, async startGhost(overrides = {}) { - const defaults = { - backend: true, - frontend: false - }; - - return await testUtils.startGhost(Object.assign(defaults, overrides)); + return await testUtils.startGhost(overrides); } }; diff --git a/ghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snap b/ghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snap index c2f42af62e8..b551b7dce3c 100644 --- a/ghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snap +++ b/ghost/core/test/e2e-api/content/__snapshots__/posts.test.js.snap @@ -1322,7 +1322,7 @@ Most successful subscription businesses publish a mix of free and paid posts to "profile_image": null, "slug": "slimer-mcectoplasm", "twitter": null, - "url": "http://127.0.0.1:2369/404/", + "url": "http://127.0.0.1:2369/author/slimer-mcectoplasm/", "website": null, }, ], @@ -1616,7 +1616,7 @@ exports[`Posts Content API Can filter posts by authors 2: [headers] 1`] = ` Object { "access-control-allow-origin": "*", "cache-control": "public, max-age=0", - "content-length": "55184", + "content-length": "55206", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, @@ -2878,7 +2878,7 @@ exports[`Posts Content API Can include relations 2: [headers] 1`] = ` Object { "access-control-allow-origin": "*", "cache-control": "public, max-age=0", - "content-length": "65513", + "content-length": "65535", "content-type": "application/json; charset=utf-8", "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, diff --git a/ghost/core/test/e2e-api/content/utils.js b/ghost/core/test/e2e-api/content/utils.js index 4f1c552ab0c..1cc0a54e135 100644 --- a/ghost/core/test/e2e-api/content/utils.js +++ b/ghost/core/test/e2e-api/content/utils.js @@ -97,11 +97,6 @@ module.exports = { return testUtils.DataGenerator.Content.api_keys[1].secret; }, async startGhost(overrides = {}) { - const defaults = { - backend: true, - frontend: false - }; - - return await testUtils.startGhost(Object.assign(defaults, overrides)); + return await testUtils.startGhost(overrides); } }; diff --git a/ghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snap b/ghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snap index 8620e388510..9d2ce390154 100644 --- a/ghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snap +++ b/ghost/core/test/e2e-webhooks/__snapshots__/posts.test.js.snap @@ -511,7 +511,7 @@ Object { "tour": null, "twitter": null, "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, - "url": "http://127.0.0.1:2369/author/joe-bloggs/", + "url": "http://127.0.0.1:2369/404/", "website": null, }, ], @@ -602,7 +602,7 @@ Header Level 3 "tour": null, "twitter": null, "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, - "url": "http://127.0.0.1:2369/author/joe-bloggs/", + "url": "http://127.0.0.1:2369/404/", "website": null, }, "primary_tag": null, diff --git a/ghost/core/test/integration/services/email-service/__snapshots__/cards.test.js.snap b/ghost/core/test/integration/services/email-service/__snapshots__/cards.test.js.snap index ea1df98fbfe..92d154dfd21 100644 --- a/ghost/core/test/integration/services/email-service/__snapshots__/cards.test.js.snap +++ b/ghost/core/test/integration/services/email-service/__snapshots__/cards.test.js.snap @@ -1968,7 +1968,7 @@ Ghost: Independent technology for modern publishingBeautiful, modern publishing