fix(tags): deleting a tag orphans every use/application of it - #235
Open
DocArmoryTech wants to merge 4 commits into
Open
fix(tags): deleting a tag orphans every use/application of it#235DocArmoryTech wants to merge 4 commits into
DocArmoryTech wants to merge 4 commits into
Conversation
TagsTable declared no association to Tagged, and tags_tagged.tag_id carries no foreign key, so Tags::delete removed the row from tags_tags and left every tags_tagged row pointing at an id that no longer existed. The orphans are invisible rather than merely stale. Every query that resolves a tagging joins tags_tags, so the rows vanish from counts while still occupying the unique index on (tag_id, fk_id, fk_model). tags_tags.counter, maintained by the CounterCache that TagBehavior::attachCounters() attaches, is equally blind to them. Observed on a production instance: removing two superseded tags left 429 orphaned taggings, 27% of the table. Total taggings read 1,590 against a reportable count of 1,161, and nothing surfaced the difference -- it was found by comparing a raw COUNT(*) against a joined report. Adds the missing hasMany with dependent => true, so deletion cascades, plus a migration pruning rows orphaned before the association existed. Any instance that has ever deleted a tag has them. cascadeCallbacks is set so the delete goes through the ORM rather than a bulk query, keeping behaviours on Tagged in play. The migration is a no-op where there is nothing to prune, and rebuilds tags_tags.counter afterwards since a raw DELETE bypasses the CounterCache. It is not reversible: the deleted rows referenced tags that no longer exist, so there is nothing to restore them to.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deleting a tag leaves every
tags_taggedrow that referenced it behind,pointing at an id that no longer exists.
Why
TagsTable::initialize()declares no association toTagged:No
hasMany, nodependent => true, andtags_tagged.tag_idcarries noforeign key.
Tags::deletetherefore removes thetags_tagsrow and nothingtouches the taggings.
The orphans are invisible rather than just stale. Every query that resolves a tagging joins
tags_tags, so the rows disappear from counts while still occupying the unique index on(tag_id, fk_id, fk_model).tags_tags.counter, maintained by theCounterCachethatTagBehavior::attachCounters()attaches, is equally blind to them.Changes
TagsTable: addhasMany('Tagged', [... 'dependent' => true, 'cascadeCallbacks' => true]).cascadeCallbackskeeps the delete going through the ORM rather than a bulk query.tags_tags.counterafterwards since a rawDELETEbypasses the CounterCache. Any instance that has ever deleted a tag has these.Notes
The migration is a no-op where there is nothing to prune, and is not reversible i.e. the rows referenced tags that no longer exist, so there is nothing to restore them to.
An alternative to the association is a foreign key on
tags_tagged.tag_idwithON DELETE CASCADE. It is cheaper but bypasses the behaviour layer, so I went with the association.Verifying
Non-zero before the migration on any instance that has deleted a tag; zero after. Creating a tag, applying it, then deleting it should leave no residue.