From 1ab8b602424c80a572ebda16e2e839673d623bfe Mon Sep 17 00:00:00 2001 From: Michael Barrett Date: Thu, 20 Aug 2026 10:52:37 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20inflated=20post=20counts?= =?UTF-8?q?=20on=20tags=20(#30108)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/ghost/issue/ONC-1945 The bulk "Add tag" action inserted a `posts_tags` row for every selected post without checking whether the post already had the tag. `posts_tags` has no unique constraint, so re-applying a tag to a selection that already carried it silently created a redundant row per post Those rows are invisible everywhere in the product, because the read path deduplicates by tag `id` before anything renders them. The one place they leak through is the tag post count, which counted join rows rather than distinct posts and so reported roughly double This PR fixes both halves. The bulk action now filters out post/tag pairs that already exist, matching the guard already in place in the users service, and also deduplicates the requested tags so passing the same tag twice in one request no longer inserts twice. The count now uses `countDistinct` so it reports posts rather than links Counting distinct matters independently of the write-side fix: it makes existing affected sites report correctly on upgrade, without a data migration. Removing the redundant rows would change which tag is primary on some posts, and therefore post URLs on sites using a `{primary_tag}` permalink, so leaving them in place is the lower-risk option --- ghost/core/core/server/models/tag.js | 2 +- .../server/services/posts/posts-service.js | 85 ++++++++-- .../test/e2e-api/admin/posts-bulk.test.js | 156 ++++++++++++++++++ ghost/core/test/e2e-api/admin/tags.test.js | 35 ++++ .../core/test/unit/server/models/tag.test.js | 2 +- 5 files changed, 264 insertions(+), 16 deletions(-) diff --git a/ghost/core/core/server/models/tag.js b/ghost/core/core/server/models/tag.js index 31177ad0a80..276cec66157 100644 --- a/ghost/core/core/server/models/tag.js +++ b/ghost/core/core/server/models/tag.js @@ -176,7 +176,7 @@ Tag = ghostBookshelf.Model.extend({ return { posts(modelOrCollection, options) { modelOrCollection.query('columns', 'tags.*', (qb) => { - qb.count('posts.id') + qb.countDistinct('posts.id') .from('posts') .leftOuterJoin('posts_tags', 'posts.id', 'posts_tags.post_id') .whereRaw('posts_tags.tag_id = tags.id') diff --git a/ghost/core/core/server/services/posts/posts-service.js b/ghost/core/core/server/services/posts/posts-service.js index 2558d235247..0f4e32109dc 100644 --- a/ghost/core/core/server/services/posts/posts-service.js +++ b/ghost/core/core/server/services/posts/posts-service.js @@ -175,8 +175,12 @@ class PostsService { message: tpl(messages.invalidTags) }); } + // null and undefined mean the field was not supplied, anything else + // that is not a string is a malformed value rather than an absent one + const isMalformed = value => value !== undefined && value !== null && typeof value !== 'string'; + for (const tag of data.meta.tags) { - if (typeof tag !== 'object') { + if (!tag || typeof tag !== 'object') { throw new errors.IncorrectUsageError({ message: tpl(messages.invalidTags) }); @@ -186,6 +190,11 @@ class PostsService { message: tpl(messages.invalidTags) }); } + if (isMalformed(tag.id) || isMalformed(tag.name)) { + throw new errors.IncorrectUsageError({ + message: tpl(messages.invalidTags) + }); + } } const bulkResult = await this.#bulkAddTags({tags: data.meta.tags}, {filter: options.filter, context: options.context}); @@ -217,28 +226,76 @@ class PostsService { }); } + // The same tag can be passed more than once in a single request. Dedupe + // before creating, otherwise a repeated name-only tag is created twice + const seen = new Set(); + const tags = data.tags.filter((tag) => { + const key = tag.id ? `id:${tag.id}` : `name:${tag.name.toLocaleLowerCase()}`; + + if (seen.has(key)) { + return false; + } + + seen.add(key); + return true; + }); + // Create tags that don't exist - for (const tag of data.tags) { - if (!tag.id) { - const createdTag = await this.models.Tag.add(tag, {transacting: options.transacting, context: options.context}); - tag.id = createdTag.id; + for (const tag of tags) { + if (tag.id) { + continue; } + + // The tag may already exist. Match on the slug it generates rather + // than the name, because the slug is unique and case insensitive on + // every supported database. Derive it the way Tag.onSaving does, so + // a supplied slug is looked up rather than silently ignored + const slug = await this.models.Base.Model.generateSlug(this.models.Tag, tag.slug || tag.name, {skipDuplicateChecks: true}); + const existingTag = await this.models.Tag.findOne({slug}, {transacting: options.transacting}); + + if (existingTag) { + tag.id = existingTag.id; + continue; + } + + const createdTag = await this.models.Tag.add(tag, {transacting: options.transacting, context: options.context}); + tag.id = createdTag.id; } const postRows = await this.#getFilteredBulkPostQuery(options).select('posts.id'); + const postIds = postRows.map(post => post.id); + + const tagIds = [...new Set(tags.map(tag => tag.id))]; + + // Posts in the selection may already carry the tag, filter those pairs out + const existingRows = tagIds.length && postIds.length ? + await options.transacting('posts_tags') + .whereIn('tag_id', tagIds) + .whereIn('post_id', postIds) + .select('post_id', 'tag_id') : + []; + const existing = new Set(existingRows.map(row => `${row.post_id}:${row.tag_id}`)); + + const postTags = []; + for (const tagId of tagIds) { + for (const postId of postIds) { + if (existing.has(`${postId}:${tagId}`)) { + continue; + } - const postTags = data.tags.reduce((pt, tag) => { - return pt.concat(postRows.map((post) => { - return { + postTags.push({ id: (new ObjectId()).toHexString(), - post_id: post.id, - tag_id: tag.id, + post_id: postId, + tag_id: tagId, sort_order: 0 - }; - })); - }, []); + }); + } + } + + if (postTags.length) { + await options.transacting('posts_tags').insert(postTags); + } - await options.transacting('posts_tags').insert(postTags); await this.models.Post.addActions('edited', postRows.map(p => p.id), options); return { diff --git a/ghost/core/test/e2e-api/admin/posts-bulk.test.js b/ghost/core/test/e2e-api/admin/posts-bulk.test.js index 9488bf5ed08..59d9f6bd0ea 100644 --- a/ghost/core/test/e2e-api/admin/posts-bulk.test.js +++ b/ghost/core/test/e2e-api/admin/posts-bulk.test.js @@ -233,6 +233,162 @@ describe('Posts Bulk API', function () { } }); + it('Does not add a tag to posts that already have it', async function () { + const filter = 'status:[published]'; + const tag = await models.Tag.findOne({slug: fixtureManager.get('tags', 0).slug}); + assert(tag); + + const addTag = async () => { + await agent + .put('/posts/bulk/?filter=' + encodeURIComponent(filter)) + .body({bulk: {action: 'addTag', meta: {tags: [{id: tag.id}]}}}) + .expectStatus(200); + }; + + // Apply the tag, then apply the same tag again - which the bulk action in Admin allows + await addTag(); + await addTag(); + + const duplicates = await models.Base.knex('posts_tags') + .where('tag_id', tag.id) + .select('post_id') + .count('* as rows') + .groupBy('post_id') + .having('rows', '>', 1); + + assert.equal(duplicates.length, 0, `Expect no duplicate posts_tags rows, got ${JSON.stringify(duplicates)}`); + }); + + it('Does not add the same tag twice when it is passed twice', async function () { + const filter = 'status:[draft]'; + const tag = await models.Tag.findOne({slug: fixtureManager.get('tags', 2).slug}); + assert(tag); + + await agent + .put('/posts/bulk/?filter=' + encodeURIComponent(filter)) + .body({bulk: {action: 'addTag', meta: {tags: [{id: tag.id}, {id: tag.id}]}}}) + .expectStatus(200); + + const duplicates = await models.Base.knex('posts_tags') + .where('tag_id', tag.id) + .select('post_id') + .count('* as rows') + .groupBy('post_id') + .having('rows', '>', 1); + + assert.equal(duplicates.length, 0, `Expect no duplicate posts_tags rows, got ${JSON.stringify(duplicates)}`); + }); + + it('Only creates one tag when the same new tag name is passed twice', async function () { + const filter = 'status:[draft]'; + const name = 'Repeated new tag'; + + await agent + .put('/posts/bulk/?filter=' + encodeURIComponent(filter)) + .body({bulk: {action: 'addTag', meta: {tags: [{name}, {name}]}}}) + .expectStatus(200); + + const tags = await models.Tag.findAll({filter: `name:'${name}'`}); + assert.equal(tags.length, 1, `Expect a single tag to be created, got ${JSON.stringify(tags.models.map(t => t.get('slug')))}`); + + const duplicates = await models.Base.knex('posts_tags') + .where('tag_id', tags.models[0].id) + .select('post_id') + .count('* as rows') + .groupBy('post_id') + .having('rows', '>', 1); + + assert.equal(duplicates.length, 0, `Expect no duplicate posts_tags rows, got ${JSON.stringify(duplicates)}`); + }); + + it('Reuses an existing tag when it is added by name', async function () { + const filter = 'status:[draft]'; + const existing = await models.Tag.add({name: 'Already here'}, {context: {internal: true}}); + + const countTagsLike = async () => { + const rows = await models.Base.knex('tags') + .whereRaw('LOWER(name) = ?', ['already here']) + .select('id'); + return rows.length; + }; + + // Same name, and the same name in a different case, both belong to + // the tag that is already there + for (const name of ['Already here', 'ALREADY HERE']) { + await agent + .put('/posts/bulk/?filter=' + encodeURIComponent(filter)) + .body({bulk: {action: 'addTag', meta: {tags: [{name}]}}}) + .expectStatus(200); + + assert.equal(await countTagsLike(), 1, `Expect no new tag to be created for "${name}"`); + } + + const posts = await models.Post.findAll({filter, status: 'all', withRelated: ['tags']}); + for (const post of posts) { + assert(post.related('tags').find(t => t.id === existing.id), `Expect post ${post.id} to have the existing tag`); + } + }); + + it('Reuses an existing tag when it is added by slug', async function () { + const filter = 'status:[draft]'; + const existing = await models.Tag.add({name: 'Slug owner', slug: 'slug-owner'}, {context: {internal: true}}); + + // Tag.add honours a supplied slug, so the lookup has to use it too + await agent + .put('/posts/bulk/?filter=' + encodeURIComponent(filter)) + .body({bulk: {action: 'addTag', meta: {tags: [{name: 'A different name', slug: 'slug-owner'}]}}}) + .expectStatus(200); + + const owners = await models.Base.knex('tags').where('slug', 'like', 'slug-owner%').select('slug'); + assert.deepEqual(owners.map(t => t.slug), ['slug-owner'], 'Expect no second tag to be created'); + + const posts = await models.Post.findAll({filter, status: 'all', withRelated: ['tags']}); + for (const post of posts) { + assert(post.related('tags').find(t => t.id === existing.id), `Expect post ${post.id} to have the existing tag`); + } + }); + + it('Does not confuse a tag id with another tag of the same name', async function () { + const filter = 'status:[draft]'; + const existing = await models.Tag.findOne({slug: fixtureManager.get('tags', 1).slug}); + assert(existing); + + // A tag named after another tag's id - contrived, but the two are + // deduplicated against each other if they share a key space + await agent + .put('/posts/bulk/?filter=' + encodeURIComponent(filter)) + .body({bulk: {action: 'addTag', meta: {tags: [{id: existing.id}, {name: existing.id}]}}}) + .expectStatus(200); + + const named = await models.Tag.findAll({filter: `name:'${existing.id}'`}); + assert.equal(named.length, 1, 'Expect the tag named after the id to still be created'); + }); + + it('Rejects tags that are not usable objects', async function () { + const validTag = await models.Tag.findOne({slug: fixtureManager.get('tags', 0).slug}); + const invalidTags = [ + [null], + ['a-tag-id'], + [1], + [true], + [{id: 1}], + [{name: 1}], + [{name: {}}], + // Falsy non-strings are supplied values too, not absent ones + [{id: 0, name: 'Falsy id'}], + [{id: false, name: 'Falsy id'}], + [{name: 0, id: validTag.id}], + [{name: false, id: validTag.id}] + ]; + + for (const tags of invalidTags) { + await agent + .put('/posts/bulk/?filter=' + encodeURIComponent('status:[draft]')) + .body({bulk: {action: 'addTag', meta: {tags}}}) + .expectStatus(400); + } + }); + it('Can add multiple tags to posts and create new tags', async function () { const filter = 'status:[draft]'; const tag = await models.Tag.findOne({id: fixtureManager.get('tags', 1).id}); diff --git a/ghost/core/test/e2e-api/admin/tags.test.js b/ghost/core/test/e2e-api/admin/tags.test.js index 180081851ae..4bacf8bc417 100644 --- a/ghost/core/test/e2e-api/admin/tags.test.js +++ b/ghost/core/test/e2e-api/admin/tags.test.js @@ -1,10 +1,12 @@ const assert = require('node:assert/strict'); +const ObjectId = require('bson-objectid').default; const {assertExists} = require('../../utils/assertions'); const sinon = require('sinon'); const supertest = require('supertest'); const testUtils = require('../../utils'); const config = require('../../../core/shared/config'); const localUtils = require('./utils'); +const models = require('../../../core/server/models'); const urlUtilsHelper = require('../../utils/url-utils'); describe('Tag API', function () { @@ -89,6 +91,39 @@ describe('Tag API', function () { assert.equal(new URL(jsonResponse.tags[0].url).pathname, '/tag/getting-started/'); }); + it('Counts posts, not posts_tags rows', async function () { + const tagId = testUtils.getExistingData().tags[0].id; + + const readCount = async () => { + const res = await request + .get(localUtils.API.getApiQuery(`tags/${tagId}/?include=count.posts`)) + .set('Origin', config.get('url')) + .expect(200); + + return res.body.tags[0].count.posts; + }; + + const before = await readCount(); + assert(before > 0, 'Expect the tag to have posts for this test to work'); + + const existing = await models.Base.knex('posts_tags') + .where('tag_id', tagId) + .select('post_id'); + const redundant = existing.map(row => ({ + id: new ObjectId().toHexString(), + post_id: row.post_id, + tag_id: tagId, + sort_order: 0 + })); + await models.Base.knex('posts_tags').insert(redundant); + + try { + assert.equal(await readCount(), before, 'Expect duplicate posts_tags rows not to inflate count.posts'); + } finally { + await models.Base.knex('posts_tags').whereIn('id', redundant.map(r => r.id)).del(); + } + }); + it('Can add a tag', async function () { const tag = testUtils.DataGenerator.forKnex.createTag(); diff --git a/ghost/core/test/unit/server/models/tag.test.js b/ghost/core/test/unit/server/models/tag.test.js index ce7dd4baa75..2dd3436bafb 100644 --- a/ghost/core/test/unit/server/models/tag.test.js +++ b/ghost/core/test/unit/server/models/tag.test.js @@ -42,7 +42,7 @@ describe('Unit: models/tag', function () { }).then(() => { assert.equal(queries.length, 1); - assert.equal(queries[0].sql, 'select `tags`.*, (select count(`posts`.`id`) from `posts` left outer join `posts_tags` on `posts`.`id` = `posts_tags`.`post_id` where posts_tags.tag_id = tags.id) as `count__posts` from `tags` where `count`.`posts` >= ? order by `count__posts` DESC'); + assert.equal(queries[0].sql, 'select `tags`.*, (select count(distinct `posts`.`id`) from `posts` left outer join `posts_tags` on `posts`.`id` = `posts_tags`.`post_id` where posts_tags.tag_id = tags.id) as `count__posts` from `tags` where `count`.`posts` >= ? order by `count__posts` DESC'); assert.deepEqual(queries[0].bindings, [ 1 ]);