From c3d7104367a14e3fb3cce9c0c149ae45ee7d052e Mon Sep 17 00:00:00 2001 From: Naz Date: Fri, 17 Feb 2023 15:55:49 +0800 Subject: [PATCH] Refactored tags repository to be model-agnostic refs https://github.com/TryGhost/Toolbox/issues/522 - The caching rules for Content API resources like Tags / Posts / Pages / Authors are the same, so it makes sense to make a common repository which would take in a model as a parameter and perform the queries on that level instead of implementing repository-per-model. - With this refactor getting Posts Content API caching would be much simpler change. --- .../server/services/tags-public/service.js | 10 ++--- ghost/core/package.json | 2 +- .../.eslintrc.js | 0 .../README.md | 2 +- ghost/public-resource-repository/index.js | 1 + .../lib/PublicResourcesRepository.js} | 18 ++++----- .../lib/resources-public.js | 1 + .../package.json | 2 +- .../test/.eslintrc.js | 0 .../test/PublicResourcesRepository.test.js} | 40 +++++++++---------- ghost/tags-public/index.js | 1 - ghost/tags-public/lib/tags-public.js | 1 - 12 files changed, 39 insertions(+), 39 deletions(-) rename ghost/{tags-public => public-resource-repository}/.eslintrc.js (100%) rename ghost/{tags-public => public-resource-repository}/README.md (73%) create mode 100644 ghost/public-resource-repository/index.js rename ghost/{tags-public/lib/TagsPublicRepository.js => public-resource-repository/lib/PublicResourcesRepository.js} (75%) create mode 100644 ghost/public-resource-repository/lib/resources-public.js rename ghost/{tags-public => public-resource-repository}/package.json (93%) rename ghost/{tags-public => public-resource-repository}/test/.eslintrc.js (100%) rename ghost/{tags-public/test/TagsPublicRepository.test.js => public-resource-repository/test/PublicResourcesRepository.test.js} (73%) delete mode 100644 ghost/tags-public/index.js delete mode 100644 ghost/tags-public/lib/tags-public.js diff --git a/ghost/core/core/server/services/tags-public/service.js b/ghost/core/core/server/services/tags-public/service.js index 4864f33363..42941dfe98 100644 --- a/ghost/core/core/server/services/tags-public/service.js +++ b/ghost/core/core/server/services/tags-public/service.js @@ -6,7 +6,7 @@ class TagsPublicServiceWrapper { } // Wire up all the dependencies - const models = require('../../models'); + const {TagPublic} = require('../../models'); const adapterManager = require('../adapter-manager'); const config = require('../../../shared/config'); @@ -15,15 +15,15 @@ class TagsPublicServiceWrapper { tagsCache = adapterManager.getAdapter('cache:tagsPublic'); } - const {TagsPublicRepository} = require('@tryghost/tags-public'); + const {PublicResourcesRepository} = require('@tryghost/public-resource-repository'); - this.linkRedirectRepository = new TagsPublicRepository({ - Tag: models.TagPublic, + this.tagsPublicRepository = new PublicResourcesRepository({ + Model: TagPublic, cache: tagsCache }); this.api = { - browse: this.linkRedirectRepository.getAll.bind(this.linkRedirectRepository) + browse: this.tagsPublicRepository.getAll.bind(this.tagsPublicRepository) }; } } diff --git a/ghost/core/package.json b/ghost/core/package.json index b00cafd253..348a0362bb 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -133,7 +133,7 @@ "@tryghost/staff-service": "0.0.0", "@tryghost/stats-service": "0.0.0", "@tryghost/string": "0.2.2", - "@tryghost/tags-public": "0.0.0", + "@tryghost/public-resource-repository": "0.0.0", "@tryghost/tiers": "0.0.0", "@tryghost/tpl": "0.1.21", "@tryghost/update-check-service": "0.0.0", diff --git a/ghost/tags-public/.eslintrc.js b/ghost/public-resource-repository/.eslintrc.js similarity index 100% rename from ghost/tags-public/.eslintrc.js rename to ghost/public-resource-repository/.eslintrc.js diff --git a/ghost/tags-public/README.md b/ghost/public-resource-repository/README.md similarity index 73% rename from ghost/tags-public/README.md rename to ghost/public-resource-repository/README.md index 7e2b4c511b..ed3945ced4 100644 --- a/ghost/tags-public/README.md +++ b/ghost/public-resource-repository/README.md @@ -1,6 +1,6 @@ # Tags Public -services and repositories serving public tags endpoints +services and repositories serving public resource endpoints e.g. Tags/Posts/Authors Content API endpoints ## Usage diff --git a/ghost/public-resource-repository/index.js b/ghost/public-resource-repository/index.js new file mode 100644 index 0000000000..2c97ee8cfb --- /dev/null +++ b/ghost/public-resource-repository/index.js @@ -0,0 +1 @@ +module.exports = require('./lib/resources-public'); diff --git a/ghost/tags-public/lib/TagsPublicRepository.js b/ghost/public-resource-repository/lib/PublicResourcesRepository.js similarity index 75% rename from ghost/tags-public/lib/TagsPublicRepository.js rename to ghost/public-resource-repository/lib/PublicResourcesRepository.js index 13442728c2..b797200758 100644 --- a/ghost/tags-public/lib/TagsPublicRepository.js +++ b/ghost/public-resource-repository/lib/PublicResourcesRepository.js @@ -1,33 +1,33 @@ const _ = require('lodash'); -module.exports = class TagsPublicRepository { +module.exports = class PublicResourcesRepository { /** @type {object} */ - #Tag; + #Model; /** @type {object} */ #cache; /** * @param {object} deps - * @param {object} deps.Tag Bookshelf Model instance of TagPublic + * @param {object} deps.Model Bookshelf Model instance of TagPublic/Post/Author etc. * @param {object} deps.cache cache instance */ constructor(deps) { - this.#Tag = deps.Tag; + this.#Model = deps.Model; this.#cache = deps.cache; } /** - * Queries the database for Tag records + * Queries the database for model records * @param {Object} options model options * @returns */ async #getAllDB(options) { - return this.#Tag.findPage(options); + return this.#Model.findPage(options); } /** - * Gets all tags and caches the returned results + * Retrieves all records from the storage (cache or database) * @param {Object} options * @returns */ @@ -36,13 +36,13 @@ module.exports = class TagsPublicRepository { if (this.#cache) { // make the cache key smaller and don't include context - const permittedOptions = this.#Tag.permittedOptions('findPage') + const permittedOptions = this.#Model.permittedOptions('findPage') .filter(option => (option !== 'context')); const optionsForCacheKey = _.pick(options, permittedOptions); // NOTE: can be more aggressive here with filtering options, // for example, do we care make a distinction for logged - // in member on the tags level? + // in member on the repository level? cacheKey = `get-all-${JSON.stringify(optionsForCacheKey)}`; const cachedResult = await this.#cache.get(cacheKey); diff --git a/ghost/public-resource-repository/lib/resources-public.js b/ghost/public-resource-repository/lib/resources-public.js new file mode 100644 index 0000000000..21156e84e5 --- /dev/null +++ b/ghost/public-resource-repository/lib/resources-public.js @@ -0,0 +1 @@ +module.exports.PublicResourcesRepository = require('./PublicResourcesRepository'); diff --git a/ghost/tags-public/package.json b/ghost/public-resource-repository/package.json similarity index 93% rename from ghost/tags-public/package.json rename to ghost/public-resource-repository/package.json index f55785a5e4..cbde83765a 100644 --- a/ghost/tags-public/package.json +++ b/ghost/public-resource-repository/package.json @@ -1,5 +1,5 @@ { - "name": "@tryghost/tags-public", + "name": "@tryghost/public-resource-repository", "version": "0.0.0", "repository": "https://github.com/TryGhost/Ghost/tree/main/packages/tags-public", "author": "Ghost Foundation", diff --git a/ghost/tags-public/test/.eslintrc.js b/ghost/public-resource-repository/test/.eslintrc.js similarity index 100% rename from ghost/tags-public/test/.eslintrc.js rename to ghost/public-resource-repository/test/.eslintrc.js diff --git a/ghost/tags-public/test/TagsPublicRepository.test.js b/ghost/public-resource-repository/test/PublicResourcesRepository.test.js similarity index 73% rename from ghost/tags-public/test/TagsPublicRepository.test.js rename to ghost/public-resource-repository/test/PublicResourcesRepository.test.js index 70e2086821..95b396c82d 100644 --- a/ghost/tags-public/test/TagsPublicRepository.test.js +++ b/ghost/public-resource-repository/test/PublicResourcesRepository.test.js @@ -1,24 +1,24 @@ const assert = require('assert'); const sinon = require('sinon'); -const {TagsPublicRepository} = require('../index'); +const {PublicResourcesRepository} = require('../index'); // @NOTE: This is a dirty import from the Ghost "core"! // extract it to it's own package and import here as require('@tryghost/adapter-base-cache-memory'); const MemoryCache = require('../../core/core/server/adapters/cache/Memory'); -describe('TagsPublicRepository', function () { +describe('PublicResourcesRepository', function () { afterEach(function () { sinon.restore(); }); describe('getAll', function () { it('calls findPage on the model multiple times when cache NOT present', async function () { - const tagStub = { + const tagModelStub = { findPage: sinon.stub().resolves(), permittedOptions: sinon.stub() }; - const repo = new TagsPublicRepository({ - Tag: tagStub + const repo = new PublicResourcesRepository({ + Model: tagModelStub }); // first call @@ -30,12 +30,12 @@ describe('TagsPublicRepository', function () { limit: 'all' }); - assert.equal(tagStub.findPage.callCount, 2, 'should be called same amount of times as getAll'); - assert.ok(tagStub.findPage.calledWith({limit: 'all'})); + assert.equal(tagModelStub.findPage.callCount, 2, 'should be called same amount of times as getAll'); + assert.ok(tagModelStub.findPage.calledWith({limit: 'all'})); }); it('calls findPage once and uses the cached value on subsequent calls', async function () { - const tagStub = { + const tagModelStub = { findPage: sinon.stub().resolves({ data: [{ get(key) { @@ -46,8 +46,8 @@ describe('TagsPublicRepository', function () { }), permittedOptions: sinon.stub().returns(['limit']) }; - const repo = new TagsPublicRepository({ - Tag: tagStub, + const repo = new PublicResourcesRepository({ + Model: tagModelStub, cache: new MemoryCache() }); @@ -60,14 +60,14 @@ describe('TagsPublicRepository', function () { limit: 'all' }); - assert.equal(tagStub.findPage.callCount, 1, 'should be called once when cache is present'); - assert.ok(tagStub.findPage.calledWith({limit: 'all'})); + assert.equal(tagModelStub.findPage.callCount, 1, 'should be called once when cache is present'); + assert.ok(tagModelStub.findPage.calledWith({limit: 'all'})); assert.equal(dbTags, cacheTags, 'should return the same value from the cache'); }); it('calls findPage multiple times if the record is not present in the cache', async function () { - const tagStub = { + const tagModelStub = { findPage: sinon.stub().resolves({ data: [{ get(key) { @@ -79,8 +79,8 @@ describe('TagsPublicRepository', function () { permittedOptions: sinon.stub().returns(['limit']) }; const cache = new MemoryCache(); - const repo = new TagsPublicRepository({ - Tag: tagStub, + const repo = new PublicResourcesRepository({ + Model: tagModelStub, cache: cache }); @@ -97,12 +97,12 @@ describe('TagsPublicRepository', function () { limit: 'all' }); - assert.equal(tagStub.findPage.callCount, 2, 'should be called every time the item is not in the cache'); - assert.ok(tagStub.findPage.calledWith({limit: 'all'})); + assert.equal(tagModelStub.findPage.callCount, 2, 'should be called every time the item is not in the cache'); + assert.ok(tagModelStub.findPage.calledWith({limit: 'all'})); }); it('works with a cache that has an asynchronous interface', async function () { - const tagStub = { + const tagModelStub = { findPage: sinon.stub().resolves({ data: [{ get(key) { @@ -119,8 +119,8 @@ describe('TagsPublicRepository', function () { set: sinon.stub().resolves() }; - const repo = new TagsPublicRepository({ - Tag: tagStub, + const repo = new PublicResourcesRepository({ + Model: tagModelStub, cache: asyncMemoryCache }); diff --git a/ghost/tags-public/index.js b/ghost/tags-public/index.js deleted file mode 100644 index f6be72c9a4..0000000000 --- a/ghost/tags-public/index.js +++ /dev/null @@ -1 +0,0 @@ -module.exports = require('./lib/tags-public'); diff --git a/ghost/tags-public/lib/tags-public.js b/ghost/tags-public/lib/tags-public.js deleted file mode 100644 index e1178900a9..0000000000 --- a/ghost/tags-public/lib/tags-public.js +++ /dev/null @@ -1 +0,0 @@ -module.exports.TagsPublicRepository = require('./TagsPublicRepository');