Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bulk adding tags can create "duplicate" posts_tags rows #21414

Open
1 task done
brvnnghi opened this issue Oct 25, 2024 · 6 comments
Open
1 task done

Bulk adding tags can create "duplicate" posts_tags rows #21414

brvnnghi opened this issue Oct 25, 2024 · 6 comments

Comments

@brvnnghi
Copy link

Issue Summary

Tag count doubled when adding an existing tag in a post again via Context Menu.

The correct behaviour would be, if a tag is already existed in a post, it should not be tagged again using "Add a Tag" in Context Menu.

img 2
img 3
img 4

Steps to Reproduce

  1. Create new tag
  2. Tag inside a post
  3. Tag again using Context Menu ("Add a tag")
  4. See tag count = 2 when there's only 1 post

Ghost Version

5.94

Node.js Version

22

How did you install Ghost?

Linux / Local

Database type

MySQL 8

Browser & OS version

No response

Relevant log / error output

No response

Code of Conduct

  • I agree to be friendly and polite to people in this repository
@github-actions github-actions bot added the needs:triage [triage] this needs to be triaged by the Ghost team label Oct 25, 2024
Copy link
Member

kevinansfield commented Oct 28, 2024

Thank you for the report.

The problem code is located here. PRs are welcome 🙂

Aside from the double counting the duplicate post_id+tag_id rows in the posts_tags table does not appear to cause any other issues. Deleting the attached tag from the UI correctly deletes both rows.

If you're aware of any other problems this causes please elaborate.

As for the fix there are two approaches:

  1. Add a check to the bulk edit feature to skip inserting duplicate rows
  2. Add a unique index across the posts_tags.{post_id,tag_id} columns so data integrity is ensured at the database level (this will likely also require approach 1 or a similar handling of errors during the insert, as well as a migration to remove duplicates before the unique index is added)

@kevinansfield kevinansfield added bug [triage] something behaving unexpectedly P4 - Low labels Oct 28, 2024 — with Linear
@github-actions github-actions bot removed the needs:triage [triage] this needs to be triaged by the Ghost team label Oct 28, 2024
@kevinansfield kevinansfield changed the title Double Tag count when adding existing tag via Context Menu Bulk adding tags can create "duplicate" post_tags rows Oct 28, 2024
@kevinansfield kevinansfield changed the title Bulk adding tags can create "duplicate" post_tags rows Bulk adding tags can create "duplicate" posts_tags rows Oct 28, 2024
@kevinansfield kevinansfield removed the bug [triage] something behaving unexpectedly label Oct 28, 2024
@Navarjun
Copy link
Contributor

Navarjun commented Nov 7, 2024

Hello there
I can work on this issue, and create a PR soon.

Please let me know if there's something I should watch out for.
You can assign it to me.

@ErisDS ErisDS added the OSS label Nov 8, 2024 — with Linear
@Navarjun
Copy link
Contributor

@kevinansfield I found 2 links while trying to do the DB migration:

  1. Ancient: https://ghost.org/changelog/database-migrations/
  2. In the docs, but not detailed enough: https://ghost.org/docs/migration/custom/

Regardless, here's where I am:
I can't find the databaseVersion key in defaultSettings.json. So, I am assuming that was removed some time ago.
Consequently, I assumed that I should be able to just create migrations in ghost/core/core/server/data/migrations/versions , creating a new folder with the next version number and writing the up/down migration. (and update schema.js accordingly)

First question: Is that correct? If not, then how does the migration setup work now? (any docs?)

Secondly, I saw some previous commits relating to column migrations for example: f9b0280
How do I get the hash used in the tests, so that the tests succeed?

Thank you in advance!

@cathysarisky
Copy link
Contributor

cathysarisky commented Nov 25, 2024

Not the same bug, but related: #19871 - this is an example of weirdness that results from duplicate entries.

@cathysarisky
Copy link
Contributor

Your second link is about moving content between platforms. It isn't relevant here. And yeah, your first link is ancient. I didn't find any docs when I wrote my first migration last month, either.

I've only done one migration, so am not expert here, but I /think/ I just ran the test and and observed the value required when it failed. Then updated the test file. :)

Other notes: Yes, pick the next version number (minor, not patch), and use that. (If it takes the team a bit to merge this, then it'll need to be updated, but that's the idea.) The string in the file name is supposed to be year-month-day-hh-mm-ss-something-descriptive.

Navarjun added a commit to Navarjun/Ghost that referenced this issue Dec 3, 2024
…sts page

TryGhost#21414
The commit includes a migration which adds a unique index to posts_tags table and handles the related error when bulk adding posts.

The errors are ignored because the tags user wants to add are added/exist. This respects the editor's expectation as posts have the tags as the end result.
@Navarjun
Copy link
Contributor

Navarjun commented Dec 4, 2024

@kevinansfield @ErisDS @cathysarisky I have created a PR for this bug.
Please take a look and let me know if any changes are required.

I did 2 things:

  1. Created a migration to add a unique index to post_id and tag_id
  2. Updated the #bulkAddTags table to avoid duplicates without throwing an error. Essentially, it will ignore any conflicts because of duplication.

Please merge it soon, if you find it worthy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants