Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#5308 closed defect (fixed)

SQL foreign key error when attempting to delete user with blog

Reported by: ayleph Owned by:
Priority: critical Milestone: 1.0
Component: programming Keywords: blog, foreign key, sql
Cc: Parent Tickets: #5438

Description

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}

Subtickets

Attachments (2)

0001-Re-5308-added-delete_user-hook.patch (2.7 KB) - added by Robert 3 years ago.
0001-Re-5308-added-cascade-to-blog-mediatype.patch (1.9 KB) - added by Robert 3 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 4 years ago by ayleph

Parent Tickets: 5438

comment:2 Changed 4 years ago by ayleph

Priority: majorcritical

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

comment:3 Changed 4 years ago by ayleph

Milestone: 1.0

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

comment:4 Changed 3 years ago by 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.

comment:5 Changed 3 years ago by 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...

comment:6 Changed 3 years ago by 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.

Changed 3 years ago by Robert

comment:7 Changed 3 years ago by Robert

Status: newreview

comment:8 Changed 3 years ago by 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.

comment:9 Changed 3 years ago by 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.

comment:10 in reply to:  9 Changed 3 years ago by 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?

comment:11 Changed 3 years ago by Robert

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

Last edited 3 years ago by Robert (previous) (diff)

comment:12 Changed 3 years ago by Boris Bobrov

Owner: set to Robert
Status: reviewin_progress

comment:13 Changed 3 years ago by Robert

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

comment:14 Changed 3 years ago by Robert

Owner: Robert deleted
Status: in_progressreview

comment:15 Changed 3 years ago by 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?

comment:16 Changed 3 years ago by 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?

comment:17 Changed 3 years ago by 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.

comment:18 Changed 3 years ago by Boris Bobrov

Resolution: fixed
Status: reviewclosed

comment:19 Changed 3 years ago by Robert

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

Note: See TracTickets for help on using tickets.