Deleting reported media gives server error

[ayleph]

On latest master, deleting media with a report on it leads to a server error and a partially deleted media entry.

Steps to reproduce:

1. Update to latest master & run migrations
2. Upload media entry
3. File a report on that media entry
4. Attempt to delete that media entry

Here's the error log.

Error - <class 'sqlalchemy.exc.IntegrityError'>: (sqlite3.IntegrityError) NOT NULL constraint failed: core__reports.object_id [SQL: u'UPDATE core__reports SET object_id=? WHERE core__reports.id = ?'] [parameters: (None, 1)]
URL: http://pumpdev.goblinrefuge.com/u/machalus/m/42/confirm-delete/
File '/path/to/mediagoblin/lib/python2.7/site-packages/Paste-1.7.5.1-py2.7.egg/paste/exceptions/errormiddleware.py', line 144 in __call__
  app_iter = self.application(environ, sr_checker)
File '/path/to/mediagoblin/mediagoblin/app.py', line 342 in __call__
  return self.call_backend(environ, start_response)
File '/path/to/mediagoblin/lib/python2.7/site-packages/Werkzeug-0.9.6-py2.7.egg/werkzeug/wsgi.py', line 588 in __call__
  return self.app(environ, start_response)
File '/path/to/mediagoblin/mediagoblin/app.py', line 276 in call_backend
  return self._finish_call_backend(request, environ, start_response)
File '/path/to/mediagoblin/mediagoblin/app.py', line 318 in _finish_call_backend
  response = controller(request)
File '/path/to/mediagoblin/mediagoblin/decorators.py', line 281 in wrapper
  return controller(request, media=media, *args, **kwargs)
File '/path/to/mediagoblin/mediagoblin/decorators.py', line 47 in wrapper
  return controller(request, *args, **kwargs)
File '/path/to/mediagoblin/mediagoblin/decorators.py', line 74 in new_controller_func
  return controller(request, *args, **kwargs)
File '/path/to/mediagoblin/mediagoblin/decorators.py', line 135 in wrapper
  return controller(request, *args, **kwargs)
File '/path/to/mediagoblin/mediagoblin/user_pages/views.py', line 338 in media_confirm_delete
  media.delete()
File '/path/to/mediagoblin/mediagoblin/db/models.py', line 735 in delete
  super(MediaEntry, self).delete(**kwargs)
File '/path/to/mediagoblin/mediagoblin/db/base.py', line 132 in delete
  report.save(commit=commit)
File '/path/to/mediagoblin/mediagoblin/db/base.py', line 85 in save
  sess.commit()
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/session.py', line 801 in commit
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/session.py', line 392 in commit
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/session.py', line 372 in _prepare_impl
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/session.py', line 2019 in flush
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/session.py', line 2137 in _flush
File 'build/bdist.linux-x86_64/egg/sqlalchemy/util/langhelpers.py', line 60 in __exit__
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/session.py', line 2101 in _flush
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/unitofwork.py', line 373 in execute
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/unitofwork.py', line 532 in execute
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/persistence.py', line 170 in save_obj
File 'build/bdist.linux-x86_64/egg/sqlalchemy/orm/persistence.py', line 706 in _emit_update_statements
File 'build/bdist.linux-x86_64/egg/sqlalchemy/engine/base.py', line 914 in execute
File 'build/bdist.linux-x86_64/egg/sqlalchemy/sql/elements.py', line 323 in _execute_on_connection
File 'build/bdist.linux-x86_64/egg/sqlalchemy/engine/base.py', line 1010 in _execute_clauseelement
File 'build/bdist.linux-x86_64/egg/sqlalchemy/engine/base.py', line 1146 in _execute_context
File 'build/bdist.linux-x86_64/egg/sqlalchemy/engine/base.py', line 1341 in _handle_dbapi_exception
File 'build/bdist.linux-x86_64/egg/sqlalchemy/util/compat.py', line 200 in raise_from_cause
File 'build/bdist.linux-x86_64/egg/sqlalchemy/engine/base.py', line 1139 in _execute_context
File 'build/bdist.linux-x86_64/egg/sqlalchemy/engine/default.py', line 450 in do_execute
IntegrityError: (sqlite3.IntegrityError) NOT NULL constraint failed: core__reports.object_id [SQL: u'UPDATE core__reports SET object_id=? WHERE core__reports.id = ?'] [parameters: (None, 1)]

[ayleph]

The error comes from the following code in mediagoblin/db/base.py.

    def delete(self, commit=True, deletion=None):
        """ Delete the object either using soft or hard deletion """
        # Get the setting in the model args if none has been specified.
        if deletion is None:
            deletion = self.deletion_mode

        # If the item is in any collection then it should be removed, this will
        # cause issues if it isn't. See #5382.
        # Import here to prevent cyclic imports.
        from mediagoblin.db.models import CollectionItem, GenericModelReference, \
                                          Report, Notification

        # Some of the models don't have an "id" field which means they can't be
        # used with GMR, these models won't be in collections because they
        # can't be. We can skip all of this.
        if hasattr(self, "id"):
            # First find the GenericModelReference for this object
            gmr = GenericModelReference.query.filter_by(
                obj_pk=self.id,
                model_type=self.__tablename__
            ).first()

            # If there is no gmr then we've got lucky, a GMR is a requirement of
            # being in a collection.
            if gmr is not None:
                # Delete collections found
                items = CollectionItem.query.filter_by(
                    object_id=gmr.id
                )   
                items.delete()

                # Delete notifications found
                notifications = Notification.query.filter_by(
                    object_id=gmr.id
                )   
                notifications.delete()
    
                # Set None on reports found
                reports = Report.query.filter_by(
                    object_id=gmr.id
                )   
                for report in reports:
                    report.object_id = None
                    report.save(commit=commit)

I see two easy workarounds here. I think I prefer the second solution below, but I'm not sure if it might lead to other issues.

1. Delete the report outright when the media is deleted. This is achieved by the following change.

diff --git a/mediagoblin/db/base.py b/mediagoblin/db/base.py
index 0f17a3a..0ab0d84 100644
--- a/mediagoblin/db/base.py
+++ b/mediagoblin/db/base.py
@@ -128,8 +128,7 @@ class GMGTableBase(object):
                     object_id=gmr.id
                 )
                 for report in reports:
-                    report.object_id = None
-                    report.save(commit=commit)
+                    report.delete()

2. Leave report intact when media is deleted for archival/historical purposes. This is achieved by the following change. Following the report link seems to work fine without error. Instead of showing the reported media, the link displays "CONTENT BY {user} HAS BEEN DELETED" in a probably-too-large font.

diff --git a/mediagoblin/db/base.py b/mediagoblin/db/base.py
index 0f17a3a..849acd8 100644
--- a/mediagoblin/db/base.py
+++ b/mediagoblin/db/base.py
@@ -122,14 +122,6 @@ class GMGTableBase(object):
                     object_id=gmr.id
                 )
                 notifications.delete()
-                
-                # Set None on reports found
-                reports = Report.query.filter_by(
-                    object_id=gmr.id
-                )
-                for report in reports:
-                    report.object_id = None
-                    report.save(commit=commit)

[ayleph]

I tested the two proposed solutions above and they both seemed to work okay for me. Marking this for review so someone can weigh in.

[ayleph]

The code which causes this error was introduced in ​8de5a931. That commit was created as a fix for #5390. While 5390 specifically mentions only comment notifications, the commit in question also deleted media reports. In my (limited) testing, media reports work fine when the media is deleted. I would propose adopting solution 2 from above, essentially reverting the report changes to leave media reports in the database after the corresponding media are deleted.

[Christopher Allan Webber]

Well, I'd merge it, but I really don't know the full rationale of why this was added, and I'm afraid I might make a mistake and remove something important. I think tsyesika needs to look over this one.

[Jessica Tallon]

Okay I've fixed this. The problem wasn't that we were setting None on it when you tried to delete the media. There was a migration that i made quite a while ago which at first was missing making Report.object_id nullable. Everyone who migrated during that period without the fixed migration continued to have that field set to NOT NULL. This is a problem because when you delete the media, there is no ID for the media anymore, the media has been deleted ergo it should be null. I should have created a migration back then when i fixed the migration but for whatever reason I didn't.

As of ​7555d10 there is now a migration, you should be able to pull, migrate and then this issue should be fixed. Thanks for your bug report and fix :)

[ayleph]

Confirming I can now delete media which have had reports filed. Thanks for the fix!