Skip to content

Commit

Permalink
🐛 Fixed duplicate tags created when slugs contain spaces (#14277)
Browse files Browse the repository at this point in the history 
refs https://github.com/TryGhost/Team/issues/1284

When you create a new post with a tag slug that contains spaces, those spaces will get replaced by dashes. But instead of reusing an existing tag, a new tag is always created.

- New tag slugs are cleaned up before matching with existing tags in the Post model onSaving method
- Cleaned up multiple loops in onSaving of Post model
- Cleaned up syntax when cleaning up tag slug
- Added tests for slugs with spaces
- Added test for too long tag slug causing duplication
  • Loading branch information
@SimonBackx
SimonBackx authored Mar 10, 2022
1 parent d8295da commit da9de95
Show file tree
Hide file tree
Showing 3 changed files with 165 additions and 6 deletions.
14 changes: 13 additions & 1 deletion core/server/models/base/plugins/generate-slug.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module.exports = function (Bookshelf) {
* Create a string to act as the permalink for an object.
* @param {Bookshelf['Model']} Model Model type to generate a slug for
* @param {String} base The string for which to generate a slug, usually a title or name
* @param {Object} options Options to pass to findOne
* @param {GenerateSlugOptions} [options] Options to pass to findOne
* @return {Promise<String>} Resolves to a unique slug string
*/
generateSlug: function generateSlug(Model, base, options) {
Expand Down Expand Up @@ -98,6 +98,10 @@ module.exports = function (Bookshelf) {
slug = baseName;
}

if (options && options.skipDuplicateChecks === true) {
return slug;
}

// Test for duplicate slugs.
return checkIfSlugExists(slug);
}
Expand All @@ -107,3 +111,11 @@ module.exports = function (Bookshelf) {
/**
* @type {import('bookshelf')} Bookshelf
*/

/**
* @typedef {object} GenerateSlugOptions
* @property {string} [status] Used for posts, to also filter by post status when generating a slug
* @property {boolean} [importing] Set to true to don't cut the slug on import
* @property {boolean} [shortSlug] If it's a user, let's try to cut it down (unless this is a human request)
* @property {boolean} [skipDuplicateChecks] Don't append unique identifiers when the slug is not unique (this prevents any database queries)
*/
20 changes: 15 additions & 5 deletions core/server/models/post.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const limitService = require('../services/limits');
const mobiledocLib = require('../lib/mobiledoc');
const relations = require('./relations');
const urlUtils = require('../../shared/url-utils');
const {Tag} = require('./tag');

const messages = {
isAlreadyPublished: 'Your post is already published, please reload your page.',
Expand Down Expand Up @@ -552,15 +553,24 @@ Post = ghostBookshelf.Model.extend({
tagsToSave = [];

// and deduplicate upper/lowercase tags
_.each(this.get('tags'), function each(item) {
loopTags: for (const tag of this.get('tags')) {
if (!tag.id && !tag.tag_id && tag.slug) {
// Clean up the provided slugs before we do any matching with existing tags
tag.slug = await ghostBookshelf.Model.generateSlug(
Tag,
tag.slug,
{skipDuplicateChecks: true}
);
}

for (i = 0; i < tagsToSave.length; i = i + 1) {
if (tagsToSave[i].name && item.name && tagsToSave[i].name.toLocaleLowerCase() === item.name.toLocaleLowerCase()) {
return;
if (tagsToSave[i].name && tag.name && tagsToSave[i].name.toLocaleLowerCase() === tag.name.toLocaleLowerCase()) {
continue loopTags;
}
}

tagsToSave.push(item);
});
tagsToSave.push(tag);
}

this.set('tags', tagsToSave);
}
Expand Down
137 changes: 137 additions & 0 deletions test/regression/api/admin/posts.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,143 @@ describe('Posts API (canary)', function () {
res.body.posts[0].tags[1].slug.should.equal('four');
});
});

it('can add with tags - slug with spaces', async function () {
const res = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
.send({
posts: [{
title: 'Tags test 5',
tags: [{slug: 'five spaces'}]
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(201);

should.exist(res.body.posts);
should.exist(res.body.posts[0].title);
res.body.posts[0].title.should.equal('Tags test 5');
res.body.posts[0].tags.length.should.equal(1);
res.body.posts[0].tags[0].slug.should.equal('five-spaces');

// Expected behaviour when creating a slug with spaces:
res.body.posts[0].tags[0].name.should.equal('five-spaces');

// If we create another post again now that the five-spaces tag exists,
// we need to make sure it matches correctly and doesn't create a new tag again

const res2 = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
.send({
posts: [{
title: 'Tags test 6',
tags: [{slug: 'five spaces'}]
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(201);

should.exist(res2.body.posts);
should.exist(res2.body.posts[0].title);
res2.body.posts[0].title.should.equal('Tags test 6');
res2.body.posts[0].tags.length.should.equal(1);
res2.body.posts[0].tags[0].id.should.equal(res.body.posts[0].tags[0].id);
});

it('can add with tags - slug with spaces not automated', async function () {
// Make sure that the matching still works when using a different name
// this is important because it invalidates any solution that would just consider a slug as the name
const res = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
.send({
posts: [{
title: 'Tags test 7',
tags: [{slug: 'six-spaces', name: 'Not automated name for six spaces'}]
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(201);

should.exist(res.body.posts);
should.exist(res.body.posts[0].title);
res.body.posts[0].title.should.equal('Tags test 7');
res.body.posts[0].tags.length.should.equal(1);
res.body.posts[0].tags[0].slug.should.equal('six-spaces');
res.body.posts[0].tags[0].name.should.equal('Not automated name for six spaces');

// If we create another post again now that the five-spaces tag exists,
// we need to make sure it matches correctly and doesn't create a new tag again

const res2 = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
.send({
posts: [{
title: 'Tags test 8',
tags: [{slug: 'six spaces'}]
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(201);

should.exist(res2.body.posts);
should.exist(res2.body.posts[0].title);
res2.body.posts[0].title.should.equal('Tags test 8');
res2.body.posts[0].tags.length.should.equal(1);
res2.body.posts[0].tags[0].id.should.equal(res.body.posts[0].tags[0].id);
});

it('can add with tags - too long slug', async function () {
const tooLongSlug = 'a'.repeat(190);

const res = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
.send({
posts: [{
title: 'Tags test 9',
tags: [{slug: tooLongSlug}]
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(201);

should.exist(res.body.posts);
should.exist(res.body.posts[0].title);
res.body.posts[0].title.should.equal('Tags test 9');
res.body.posts[0].tags.length.should.equal(1);
res.body.posts[0].tags[0].slug.should.equal(tooLongSlug.substring(0, 185));

// If we create another post again now that the very long tag exists,
// we need to make sure it matches correctly and doesn't create a new tag again

const res2 = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
.send({
posts: [{
title: 'Tags test 10',
tags: [{slug: tooLongSlug}]
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(201);

should.exist(res2.body.posts);
should.exist(res2.body.posts[0].title);
res2.body.posts[0].title.should.equal('Tags test 10');
res2.body.posts[0].tags.length.should.equal(1);
res2.body.posts[0].tags[0].id.should.equal(res.body.posts[0].tags[0].id);
});
});

describe('Edit', function () {
Expand Down

0 comments on commit da9de95

Please sign in to comment.