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

@@ -39,6 +39,7 @@
     "c8": {
         "exclude": [
             "src/CollectionRepository.ts",
+            "src/UniqueChecker.ts",
             "src/**/*.d.ts",
             "test/**/*.ts"
         ]

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<Collection>) {
+    public async edit(data: Partial<Collection>, 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;

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) {

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<boolean> {
+        const entity = await this.repository.getBySlug(slug);
+        return entity === null;
+    }
+}

ghost/collections/src/UniqueChecker.ts

@@ -0,0 +1,3 @@
+export interface UniqueChecker {
+    isUniqueSlug(slug: string): Promise<boolean>
+}

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');

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');
+    });
+});