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

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Collections, defect, P3)

Description

[Scott McCammon [:mccammos]]

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).

Comment 1

[Fred Wenzel [:wenzel]]

The solution for this should go into the app_model's beforeDelete() callback, and iterate over the model's translated_fields array.

Comment 2

[Fred Wenzel [:wenzel]]

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

Comment 3

[Fred Wenzel [:wenzel]]

Attachment: 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.

Comment 4

[Wil Clouser [:clouserw]]

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.

Comment 5

[Jeff Balogh (:jbalogh)]

(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.

Comment 6

[Fred Wenzel [:wenzel]]

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.

Comment 7

[Jeff Balogh (:jbalogh)]

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

Comment 8

[Fred Wenzel [:wenzel]]

(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.

Comment 9

[Fred Wenzel [:wenzel]]

Indeed, the test data was borken :( Fixed in r53944.

Comment 10

[Jeff Balogh (:jbalogh)]

(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.