Ensure that the web app closes db connections when done with them.

Prior to this commit, there was a bug in Bodhi where requests that
did not access the request's db attribute would not close out their
database connection. This happened notably with the /composes/ API,
which only used the scoped session to perform its queries.

The commit refactors Bodhi to use Pyramid's event system to attach
a callback to all requests so that the database session is
committed or rolled back (depending on errors) no matter which way
the database session is acquired, so long as it is still the scoped
session. This approach is more general, and works for code that
uses the old or new styles of accessing database session.

As a result of this refactor, the get_db_session_for_request()
function no longer involves itself in transaction management and
simply returns a scoped database session.

fixes #2385

Signed-off-by: Randy Barlow <[email protected]>

bodhi/server/__init__.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# Copyright © 2007-2017 Red Hat, Inc. and others.
+# Copyright © 2007-2018 Red Hat, Inc. and others.
 #
 # This file is part of Bodhi.
 #
@@ -47,42 +47,6 @@
 # Request methods
 #
 
-def get_db_session_for_request(request=None):
-    """
-    Return a database session that is meant to be used for the given request.
-
-    It handles rolling back or committing the session based on whether an exception occurred or
-    not. To get a database session that's not tied to the request/response cycle, just use the
-    :data:`Session` scoped session in this module.
-
-    Args:
-        request (pyramid.request.Request): The request object to create a session for.
-
-    Returns:
-        sqlalchemy.orm.session.Session: A database session.
-    """
-    session = request.registry.sessionmaker()
-
-    def cleanup(request):
-        """
-        Commit the database changes if no exceptions occurred.
-
-        This is a post-request hook.
-
-        Args:
-            request (pyramid.request.Request): The current web request.
-        """
-        if request.exception is not None:
-            session.rollback()
-        else:
-            session.commit()
-        session.close()
-
-    request.add_finished_callback(cleanup)
-
-    return session
-
-
 def get_cacheregion(request):
     """
     Return a CacheRegion to be used to cache results.
@@ -245,7 +209,8 @@ def main(global_config, testing=None, session=None, **settings):
     Args:
         global_config (dict): A dictionary with two keys: __file__, a path to the ini file, and
             here, the path to the code.
-        testing (bool or None): Whether or not we are in testing mode.
+        testing (str or None): If this app is contructed by the unit tests, they should set this to
+            a username.
         session (sqlalchemy.orm.session.Session or None): If given, the session will be used instead
             of building a new one.
         settings (dictionary): Unused.
@@ -286,7 +251,7 @@ def main(global_config, testing=None, session=None, **settings):
     else:
         config.registry.sessionmaker = Session
 
-    config.add_request_method(get_db_session_for_request, 'db', reify=True)
+    config.add_request_method(lambda x: Session, 'db', reify=True)
 
     config.add_request_method(get_user, 'user', reify=True)
     config.add_request_method(get_koji, 'koji', reify=True)
@@ -354,6 +319,7 @@ def main(global_config, testing=None, session=None, **settings):
     config.scan('bodhi.server.views')
     config.scan('bodhi.server.services')
     config.scan('bodhi.server.captcha')
+    config.scan('bodhi.server.webapp')
 
     # Though importing in the middle of this function is the darkest of evils, we cannot do it any
     # other way without a backwards-incompatible change. See
@@ -376,5 +342,8 @@ def main(global_config, testing=None, session=None, **settings):
     # return value.
     generic._generate_home_page_stats()
 
+    # Let's close out the db session we used to warm the caches.
+    Session.remove()
+
     log.info('Bodhi ready and at your service!')
     return config.make_wsgi_app()

bodhi/server/webapp.py

@@ -0,0 +1,73 @@
+# -*- coding: utf-8 -*-
+# Copyright © 2018 Red Hat, Inc.
+#
+# This file is part of Bodhi.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+"""
+Define Bodhi's WSGI application.
+
+As of the writing of this docblock, this module is a bit misnamed since the webapp is actually
+defined in bodhi.server.__init__. However, that is an anti-pattern with lots of nasty in-line
+imports due to circular dependencies, and this module is intended to solve that problem.
+Unfortunately, it is a backwards-incompatible change to move main() here, so it will remain in
+__init__ until we make a major Bodhi release. See https://github.com/fedora-infra/bodhi/issues/2294
+"""
+
+from pyramid.events import NewRequest, subscriber
+
+from bodhi import server
+
+
+def _complete_database_session(request):
+    """
+    Commit the database changes if no exceptions occurred.
+
+    This is a post-request hook. It handles rolling back or committing the session based on whether
+    an exception occurred or not. To get a database session that's not tied to the request/response
+    cycle, just use the :data:`Session` scoped session.
+
+    Args:
+        request (pyramid.request.Request): The current web request.
+    """
+    _rollback_or_commit(request)
+    server.Session().close()
+    server.Session.remove()
+
+
+@subscriber(NewRequest)
+def _prepare_request(event):
+    """
+    Add callbacks onto every new request.
+
+    This function adds a callback to clean up the database session when the request is finished.
+
+    Args:
+        event (pyramid.events.NewRequest): The new request event.
+    """
+    event.request.add_finished_callback(_complete_database_session)
+
+
+def _rollback_or_commit(request):
+    """
+    Commit the transaction if there are no exceptions, otherwise rollback.
+
+    Args:
+        request (pyramid.request.Request): The current web request.
+    """
+    if request.exception is not None:
+        server.Session().rollback()
+    else:
+        server.Session().commit()

bodhi/tests/server/base.py

@@ -25,7 +25,7 @@
 from sqlalchemy import event
 import mock
 
-from bodhi.server import bugs, buildsys, models, initialize_db, Session, config, main
+from bodhi.server import bugs, buildsys, models, initialize_db, Session, config, main, webapp
 from bodhi.tests.server import create_update, populate
 
 
@@ -134,6 +134,7 @@ class BaseTestCase(unittest.TestCase):
         'cors_connect_src': 'http://0.0.0.0:6543',
         'cors_origins_ro': 'http://0.0.0.0:6543',
         'cors_origins_rw': 'http://0.0.0.0:6543',
+        'sqlalchemy.url': DEFAULT_DB
     }
 
     def setUp(self):
@@ -163,21 +164,8 @@ def setUp(self):
         bugs.set_bugtracker()
         buildsys.setup_buildsystem({'buildsystem': 'dev'})
 
-        def request_db(request=None):
-            """
-            Replace the db session function with one that doesn't close the session.
-
-            This allows tests to make assertions about the database. Without it, all
-            the changes would be rolled back to when the nested transaction is started.
-            """
-            def cleanup(request):
-                if request.exception is not None:
-                    Session().rollback()
-                else:
-                    Session().commit()
-            request.add_finished_callback(cleanup)
-            return Session()
-        self._request_sesh = mock.patch('bodhi.server.get_db_session_for_request', request_db)
+        self._request_sesh = mock.patch('bodhi.server.webapp._complete_database_session',
+                                        webapp._rollback_or_commit)
         self._request_sesh.start()
 
         # Create the test WSGI app one time. We should avoid creating too many
@@ -186,7 +174,11 @@ def cleanup(request):
         # out how to make Pyramid forget about these.
         global _app
         if _app is None:
-            _app = TestApp(main({}, testing=u'guest', **self.app_settings))
+            # We don't want to call Session.remove() during the unit tests, because that will
+            # trigger the restart_savepoint() callback defined above which will remove the data
+            # added by populate().
+            with mock.patch('bodhi.server.Session.remove'):
+                _app = TestApp(main({}, testing=u'guest', **self.app_settings))
         self.app = _app
 
     def get_csrf_token(self, app=None):

bodhi/tests/server/services/test_comments.py

@@ -258,7 +258,8 @@ def test_anonymous_commenting_with_no_author(self):
             'authtkt.secret': 'whatever',
             'authtkt.secure': True,
         })
-        app = TestApp(main({}, session=self.db, **anonymous_settings))
+        with mock.patch('bodhi.server.Session.remove'):
+            app = TestApp(main({}, session=self.db, **anonymous_settings))
 
         comment = {
             u'update': 'bodhi-2.0-1.fc17',

bodhi/tests/server/services/test_overrides.py

@@ -661,7 +661,9 @@ def test_override_view_not_loggedin(self):
             'authtkt.secret': 'whatever',
             'authtkt.secure': True,
         })
-        app = TestApp(main({}, session=self.db, **anonymous_settings))
+        with mock.patch('bodhi.server.Session.remove'):
+            app = TestApp(main({}, session=self.db, **anonymous_settings))
+
         resp = app.get('/overrides/bodhi-2.0-1.fc17',
                        status=200, headers={'Accept': 'text/html'})
         self.assertNotIn('<span>New Buildroot Override Form Requires JavaScript</span>', resp)

bodhi/tests/server/services/test_releases.py

@@ -55,7 +55,9 @@ def test_anonymous_cant_edit_release(self):
         """Ensure that an unauthenticated user cannot edit a release, since only an admin should."""
         name = u"F22"
         # Create a new app so we are the anonymous user.
-        app = webtest.TestApp(server.main({}, session=self.db, **self.app_settings))
+        with mock.patch('bodhi.server.Session.remove'):
+            app = webtest.TestApp(server.main({}, session=self.db, **self.app_settings))
+
         res = app.get('/releases/%s' % name, status=200)
         r = res.json_body
         r["edited"] = name