TryGhost/Ghost
1
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2024-12-28 13:22:39 +03:00
Code Issues Projects Releases Wiki Activity

🐛 Fixed duplicate tags created when slugs contain spaces (#14277)

Browse Source 
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
This commit is contained in:
Simon Backx 2022-03-10 13:07:00 +01:00 committed by GitHub
parent d8295da817
commit da9de95b74
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 165 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
core/server/models/base/plugins/generate-slug.js
View File

@ -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) {
@ -98,6 +98,10 @@ module.exports = function (Bookshelf) {
slug = baseName;
}
if (options && options.skipDuplicateChecks === true) {
return slug;
}
// Test for duplicate slugs.
return checkIfSlugExists(slug);
}
@ -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
core/server/models/post.js
View File

@ -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.',
@ -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);
}

137
test/regression/api/admin/posts.test.js
View File

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