SQL foreign key error when attempting to delete user with blog

[ayleph]

A member of my instance created a blog entry and then tried to delete his account, resulting in this error in my logs.

IntegrityError: (IntegrityError) update or delete on table "core__users" violates foreign key constraint "mediatype__blogs_author_fkey" on table "mediatype__blogs"
DETAIL:  Key (id)=(23284) is still referenced from table "mediatype__blogs".
 'DELETE FROM core__users WHERE core__users.id = %(id)s' {'id': 23284}

[ayleph]

Attempting to delete a user with a blog is still an issue in v0.9.0. I'm raising the severity on this.

[ayleph]

I'm going to go ahead and assign a milestone, but I really think it should be targeted sooner than the 1.0 release.

[Robert]

I'm pretty new to mediagoblin, so sorry if this is a dumb question, but could this be fixed by adding a plugin hook for user deletion? It seems that if you're allowing mediatypes/plugins to add their own sql tables, then there should be hooks so that they can delete their data the way that makes sense for them when a user deletes their account. Adding a hook for user deletion (and maybe on media_entry deletion too) would let them do that.

[Boris Bobrov]

The code that should have done that is in ​https://git.savannah.gnu.org/cgit/mediagoblin.git/tree/mediagoblin/db/models.py?id=c3356889c9ad4965539a7bd63b672f9669816771#n258 and below. It is quite bad that blog plugin allows does references user. I think this reference should be fixed.

Also, i wonder, how can we forbid plugins reference things outside of them...

[Robert]

I agree that we might not want the blog plugin to reference the core_users table, but it seems like the plugin will have to reference the core_media_entries table, unless we want to rewrite the same core media logic for every plugin. Therefore I don't think it would be feasible to block plugins from accessing core tables altogether, we should just add hooks for them to clean up the necessary entries. I'm going to see how easy it would be to implement that.

[Robert]

I tested this patch on master branch as of 2017-06-12, and was able to successfully delete a user with a blog and blog posts. I went ahead and submitted a patch, but I have two questions/potential issues that I'd like feedback on, and I can then resubmit the patch as needed.

First is the name of the new hook, 'delete_user'. I didn't see any naming convention, so I tried to keep it simple, but there might be some rules/precedents that I'm unaware of. Second, I wasn't sure if there was a place in the docs where new hooks are supposed to be documented - if a page exists, I'd be happy to write a few sentences on the intended purpose/utility of the new hook.

[ayleph]

Thanks for the patch! Looks pretty straightforward. Adding a hook to mess with the database seems like a large enough change that we need to get buy-in from others, but I can't come up with a cleaner way of handling this issue myself.

There are a few style issues I'd like to see cleaned up to be consistent with other code: adding a space between # and text, adding a space between multiple arguments of a function, and keeping 2 blank lines between functions.

[Boris Bobrov]

The cleaner solution would be to use cascades: ​http://docs.sqlalchemy.org/en/latest/orm/cascades.html . It would require changing the model and creating alembic migration for blog media type. Robert, do you want to give it a try?

[Robert]

Yeah, I'll look into it sometime during the next few days. Thanks for the feedback!

[Robert]

I added the backref cascade, and the blogs are being properly deleted again! Let me know if there are any other issues.

[Boris Bobrov]

Oh wow, i was actually thinking about something else, but this one seems to work totally fine. Ayleph, could you please confirm that it works for you?

[ayleph]

I spun up a new test instance running latest master, then I created a new user and added some blog entries. I tried deleting without this patch and confirmed the issue still existed. Then I applied the patch, and I was able to delete the account successfully. Looks good to me!

smithrob, we generally include contributors in the AUTHORS file. If you're okay with us adding you, what name should we use to recognize you?

[Boris Bobrov]

Thanks, i've pushed the patch. Robert, i've added you as "Robert Smith", please ping me if you want me to change it.

[Robert]

Thanks breton, glad I could help! Robert Smith is the right name :)