From 0a3e36cd620eb05a91f1ab5f646bdaca1ff5b292 Mon Sep 17 00:00:00 2001 From: "Fabien \"egg\" O'Carroll" Date: Wed, 28 Jun 2023 22:33:32 +0100 Subject: [PATCH] Ensured uniqueness of `slug` in collections We require that slugs are unique as slugs can/are used for routing purposes and act as an identifier for a resource. As this is a core business rule, we want to encode it in the entity so that it can be unit tested, and be enforced regardless of underlying persistence layer --- ghost/collections/package.json | 1 + ghost/collections/src/Collection.ts | 28 +++++++++-- ghost/collections/src/CollectionsService.ts | 5 +- .../src/RepositoryUniqueChecker.ts | 13 ++++++ ghost/collections/src/UniqueChecker.ts | 3 ++ ghost/collections/test/Collection.test.ts | 46 +++++++++++++++++-- .../test/RepositoryUniqueChecker.test.ts | 32 +++++++++++++ 7 files changed, 119 insertions(+), 9 deletions(-) create mode 100644 ghost/collections/src/RepositoryUniqueChecker.ts create mode 100644 ghost/collections/src/UniqueChecker.ts create mode 100644 ghost/collections/test/RepositoryUniqueChecker.test.ts diff --git a/ghost/collections/package.json b/ghost/collections/package.json index cfee5989f2..970dde8d33 100644 --- a/ghost/collections/package.json +++ b/ghost/collections/package.json @@ -39,6 +39,7 @@ "c8": { "exclude": [ "src/CollectionRepository.ts", + "src/UniqueChecker.ts", "src/**/*.d.ts", "test/**/*.ts" ] diff --git a/ghost/collections/src/Collection.ts b/ghost/collections/src/Collection.ts index 108adf2200..91262f46b5 100644 --- a/ghost/collections/src/Collection.ts +++ b/ghost/collections/src/Collection.ts @@ -1,3 +1,4 @@ +import {UniqueChecker} from './UniqueChecker'; import {ValidationError} from '@tryghost/errors'; import tpl from '@tryghost/tpl'; import nql = require('@tryghost/nql'); @@ -11,7 +12,8 @@ const messages = { message: 'Invalid filter provided for automatic Collection', context: 'Automatic type of collection should always have a filter value' }, - noTitleProvided: 'Title must be provided' + noTitleProvided: 'Title must be provided', + slugMustBeUnique: 'Slug must be unique' }; type CollectionPost = { @@ -23,7 +25,23 @@ type CollectionPost = { export class Collection { id: string; title: string; - slug: string; + private _slug: string; + get slug() { + return this._slug; + } + + async setSlug(slug: string, uniqueChecker: UniqueChecker) { + if (slug === this.slug) { + return; + } + if (await uniqueChecker.isUniqueSlug(slug)) { + this._slug = slug; + } else { + throw new ValidationError({ + message: tpl(messages.slugMustBeUnique) + }); + } + } description: string; type: 'manual' | 'automatic'; filter: string | null; @@ -48,7 +66,7 @@ export class Collection { } } - public edit(data: Partial) { + public async edit(data: Partial, uniqueChecker: UniqueChecker) { if (this.type === 'automatic' && (data.filter === null || data.filter === '')) { throw new ValidationError({ message: tpl(messages.invalidFilterProvided.message), @@ -61,7 +79,7 @@ export class Collection { } if (data.slug !== undefined) { - this.slug = data.slug; + await this.setSlug(data.slug, uniqueChecker); } if (data.description !== undefined) { @@ -126,7 +144,7 @@ export class Collection { private constructor(data: any) { this.id = data.id; this.title = data.title; - this.slug = data.slug; + this._slug = data.slug; this.description = data.description; this.type = data.type; this.filter = data.filter; diff --git a/ghost/collections/src/CollectionsService.ts b/ghost/collections/src/CollectionsService.ts index b24eafff57..5d738fcaa0 100644 --- a/ghost/collections/src/CollectionsService.ts +++ b/ghost/collections/src/CollectionsService.ts @@ -7,6 +7,7 @@ import {MethodNotAllowedError, NotFoundError} from '@tryghost/errors'; import {PostDeletedEvent} from './events/PostDeletedEvent'; import {PostAddedEvent} from './events/PostAddedEvent'; import {PostEditedEvent} from './events/PostEditedEvent'; +import {RepositoryUniqueChecker} from './RepositoryUniqueChecker'; const messages = { cannotDeleteBuiltInCollectionError: { @@ -94,11 +95,13 @@ export class CollectionsService { private DomainEvents: { subscribe: (event: any, handler: (e: any) => void) => void; }; + private uniqueChecker: RepositoryUniqueChecker; constructor(deps: CollectionsServiceDeps) { this.collectionsRepository = deps.collectionsRepository; this.postsRepository = deps.postsRepository; this.DomainEvents = deps.DomainEvents; + this.uniqueChecker = new RepositoryUniqueChecker(this.collectionsRepository); } private toDTO(collection: Collection): CollectionDTO { @@ -290,7 +293,7 @@ export class CollectionsService { } const collectionData = this.fromDTO(data); - await collection.edit(collectionData); + await collection.edit(collectionData, this.uniqueChecker); if (collection.type === 'manual' && data.posts) { for (const post of data.posts) { diff --git a/ghost/collections/src/RepositoryUniqueChecker.ts b/ghost/collections/src/RepositoryUniqueChecker.ts new file mode 100644 index 0000000000..ec682a5aeb --- /dev/null +++ b/ghost/collections/src/RepositoryUniqueChecker.ts @@ -0,0 +1,13 @@ +import {CollectionRepository} from './CollectionRepository'; +import {UniqueChecker} from './UniqueChecker'; + +export class RepositoryUniqueChecker implements UniqueChecker { + constructor( + private repository: CollectionRepository + ) {} + + async isUniqueSlug(slug: string): Promise { + const entity = await this.repository.getBySlug(slug); + return entity === null; + } +} diff --git a/ghost/collections/src/UniqueChecker.ts b/ghost/collections/src/UniqueChecker.ts new file mode 100644 index 0000000000..68336357e2 --- /dev/null +++ b/ghost/collections/src/UniqueChecker.ts @@ -0,0 +1,3 @@ +export interface UniqueChecker { + isUniqueSlug(slug: string): Promise +} diff --git a/ghost/collections/test/Collection.test.ts b/ghost/collections/test/Collection.test.ts index 7e36c323da..ee9ddd8223 100644 --- a/ghost/collections/test/Collection.test.ts +++ b/ghost/collections/test/Collection.test.ts @@ -2,6 +2,12 @@ import assert from 'assert'; import ObjectID from 'bson-objectid'; import {Collection} from '../src/index'; +const uniqueChecker = { + async isUniqueSlug() { + return true; + } +}; + describe('Collection', function () { it('Create Collection entity', async function () { const collection = await Collection.create({ @@ -132,6 +138,40 @@ describe('Collection', function () { }); }); + describe('setSlug', function () { + it('Does not bother checking uniqueness if slug is unchanged', async function () { + const collection = await Collection.create({ + slug: 'test-collection', + title: 'Testing edits', + type: 'automatic', + filter: 'featured:true' + }); + + await collection.setSlug('test-collection', { + isUniqueSlug: () => { + throw new Error('Should not have checked uniqueness'); + } + }); + }); + + it('Throws an error if slug is not unique', async function () { + const collection = await Collection.create({ + slug: 'test-collection', + title: 'Testing edits', + type: 'automatic', + filter: 'featured:true' + }); + + assert.rejects(async () => { + await collection.setSlug('not-unique', { + async isUniqueSlug() { + return false; + } + }); + }); + }); + }); + describe('edit', function () { it('Can edit Collection values', async function () { const collection = await Collection.create({ @@ -143,10 +183,10 @@ describe('Collection', function () { assert.equal(collection.title, 'Testing edits'); - collection.edit({ + await collection.edit({ title: 'Edited title', slug: 'edited-slug' - }); + }, uniqueChecker); assert.equal(collection.title, 'Edited title'); assert.equal(collection.slug, 'edited-slug'); @@ -162,7 +202,7 @@ describe('Collection', function () { assert.rejects(async () => { await collection.edit({ filter: null - }); + }, uniqueChecker); }, (err: any) => { assert.equal(err.message, 'Invalid filter provided for automatic Collection', 'Error message should match'); assert.equal(err.context, 'Automatic type of collection should always have a filter value', 'Error message should match'); diff --git a/ghost/collections/test/RepositoryUniqueChecker.test.ts b/ghost/collections/test/RepositoryUniqueChecker.test.ts new file mode 100644 index 0000000000..7e46a51633 --- /dev/null +++ b/ghost/collections/test/RepositoryUniqueChecker.test.ts @@ -0,0 +1,32 @@ +import assert from 'assert/strict'; +import {CollectionsRepositoryInMemory} from '../src/CollectionsRepositoryInMemory'; +import {Collection} from '../src/Collection'; +import {RepositoryUniqueChecker} from '../src/RepositoryUniqueChecker'; + +describe('RepositoryUniqueChecker', function () { + let uniqueChecker: RepositoryUniqueChecker; + + beforeEach(async function () { + const repository = new CollectionsRepositoryInMemory(); + const collection = await Collection.create({ + title: 'Test', + slug: 'not-unique' + }); + repository.save(collection); + uniqueChecker = new RepositoryUniqueChecker(repository); + }); + + it('should return true if slug is unique', async function () { + const actual = await uniqueChecker.isUniqueSlug('unique'); + const expected = true; + + assert.equal(actual, expected, 'The slug "unique" should be unique'); + }); + + it('should return false if slug is not unique', async function () { + const actual = await uniqueChecker.isUniqueSlug('not-unique'); + const expected = false; + + assert.equal(actual, expected, 'The slug "not-unique" should not be unique'); + }); +});