All users were logged out of Bugzilla on October 13th, 2018

Deleting a collection does not delete its strings from the translation table

VERIFIED FIXED in 5.3

Status

P3
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: smccammon, Assigned: wenzel)

Tracking

unspecified

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
Querying the translations table after deleting a collection shows all the deleted collection's name and description strings still present.

Expected behavior:
Deleting a collection also deletes translations referenced by name and description foreign keys.

Theoretically there is no rule preventing these translations from being referenced by other foreign keys, so deleting these rows could potentially fail due to foreign key constraints (which is perfectly ok).
Priority: -- → P3
Target Milestone: --- → 5.1
(Assignee)

Comment 1

9 years ago
The solution for this should go into the app_model's beforeDelete() callback, and iterate over the model's translated_fields array.
Target Milestone: 5.1 → 5.2
Assignee: nobody → fwenzel
(Assignee)

Comment 2

9 years ago
(I have a feeling everytime I drop random thoughts in a bug, I end up having to fix it ;) )
(Assignee)

Comment 3

9 years ago
Created attachment 405081 [details] [diff] [review]
Patch, rev. 1

This deletes translations before deleting the actual collection. We should (still) do this in the app_model, but for that we'll need to change all translation foreign keys to ON DELETE SET NULL which is out of scope here. But I shall file another bug for it.
Attachment #405081 - Flags: review?(clouserw)
(Assignee)

Updated

9 years ago
Blocks: 521041
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
Comment on attachment 405081 [details] [diff] [review]
Patch, rev. 1

I don't like running the DELETE in a loop.  It would be cool to have some kind of a delayed SQL processor where we could just drop queries in a bucket and this other thing runs them when it gets a chance.
Attachment #405081 - Flags: review?(clouserw) → review+
(In reply to comment #4)
> (From update of attachment 405081 [details] [diff] [review])
> I don't like running the DELETE in a loop.  It would be cool to have some kind
> of a delayed SQL processor where we could just drop queries in a bucket and
> this other thing runs them when it gets a chance.

I've discussed this briefly with davedash, since he wants to do some addon post-save processing that needs to be fancier than trigger sql.

Hooking up gearman would be perfect here.  The web request would shoot a message to gearmand and keep going, and a queue processor on some other machine would pick up the message and do some work.
(Assignee)

Comment 6

9 years ago
That doesn't seem like a bad idea, this way we could have these requests processed asynchronously.

I'll land this for now, then we can improve it later. Aren't we having a maintenance release one of these milestones?

r53309.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: push-needed
Resolution: --- → FIXED
Keywords: push-needed
from internet relay chat:

> you added ON DELETE SET NULL to the translations foreign keys in collections
> but that field is NOT NULL in the translations table
> so mysql won't let me create a new database
> and I don't know what's going to happen when that on delete trigger is hit
> it seems like ON DELETE CASCADE is what we want
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 5.2 → 5.3
(Assignee)

Comment 8

9 years ago
(In reply to comment #7)
> > it seems like ON DELETE CASCADE is what we want

I don't think so. If you have two tables, a and b, and a references b ON DELETE CASCADE, then *a* is deleted when the corresponding entry in *b* is removed. In other words, ON DELETE CASCADE would delete the *collection* when the corresponding *translation* is deleted, not vice-versa. It would be nice if we could define a "backwards ON DELETE CASCADE" but since that doesn't (seem to?) exist, ON DELETE SET NULL is as good as it gets: It sets the "name" (etc.) field NULL if the corresponding translation is deleted.

I will try to import our test data from scratch and make sure that works as expected. Maybe something is missing there.
(Assignee)

Comment 9

9 years ago
Indeed, the test data was borken :( Fixed in r53944.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(In reply to comment #8)
> (In reply to comment #7)
> > > it seems like ON DELETE CASCADE is what we want
> 
> I don't think so. If you have two tables, a and b, and a references b ON DELETE
> CASCADE, then *a* is deleted when the corresponding entry in *b* is removed. In
> other words, ON DELETE CASCADE would delete the *collection* when the
> corresponding *translation* is deleted, not vice-versa. It would be nice if we
> could define a "backwards ON DELETE CASCADE" but since that doesn't (seem to?)
> exist, ON DELETE SET NULL is as good as it gets: It sets the "name" (etc.)
> field NULL if the corresponding translation is deleted.

Ah, thanks.  The docs talk about parent and child and had me befuddled until I saw an example schema.  Your patch makes everything load happily for me.
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.