Separate use of database sessions

This patch separates the use of database session in different MBS components
and do not mix them together.

In general, MBS components could be separated as the REST API (implemented
based on Flask) and non-REST API including the backend build workflow
(implemented as a fedmsg consumer on top of fedmsg-hub and running
independently) and library shared by them. As a result, there are two kind of
database session used in MBS, one is created and managed by Flask-SQLAlchemy,
and another one is created from SQLAclhemy Session API directly. The goal of
this patch is to make ensure session object is used properly in the right
place.

All the changes follow these rules:

* REST API related code uses the session object db.session created and
  managed by Flask-SQLAlchemy.
* Non-REST API related code uses the session object created with SQLAlchemy
  Session API. Function make_db_session does that.
* Shared code does not created a new session object as much as possible.
  Instead, it accepts an argument db_session.

The first two rules are applicable to tests as well.

Major changes:

* Switch tests back to run with a file-based SQLite database.
* make_session is renamed to make_db_session and SQLAlchemy connection pool
  options are applied for PostgreSQL backend.
* Frontend Flask related code uses db.session
* Shared code by REST API and backend build workflow accepts SQLAlchemy session
  object as an argument. For example, resolver class is constructed with a
  database session, and some functions accepts an argument for database session.
* Build workflow related code use session object returned from make_db_session
  and ensure db.session is not used.
* Only tests for views use db.session, and other tests use db_session fixture
  to access database.
* All argument name session, that is for database access, are renamed to
  db_session.
* Functions model_tests_init_data, reuse_component_init_data and
  reuse_shared_userspace_init_data, which creates fixture data for
  tests, are converted into pytest fixtures from original function
  called inside setup_method or a test method. The reason of this
  conversion is to use fixture ``db_session`` rather than create a
  new one. That would also benefit the whole test suite to reduce the
  number of SQLAlchemy session objects.

Signed-off-by: Chenxiong Qi <cqi@redhat.com>

tests/test_utils/test_greenwave.py

@@ -24,8 +24,6 @@
 import json
 from mock import patch, Mock
 import pytest
-from module_build_service import conf
-from module_build_service.models import make_session
 from module_build_service.utils.greenwave import greenwave
 from tests import clean_database, make_module
 
@@ -36,7 +34,7 @@ class TestGreenwaveQuery():
         clean_database()
 
     @patch("module_build_service.utils.greenwave.requests")
-    def test_greenwave_query_decision(self, mock_requests):
+    def test_greenwave_query_decision(self, mock_requests, db_session):
         resp_status = 200
         resp_content = {
             "applicable_policies": ["osci_compose_modules"],
@@ -61,9 +59,8 @@ class TestGreenwaveQuery():
         response.status_code = resp_status
         mock_requests.post.return_value = response
 
-        with make_session(conf) as db_session:
-            fake_build = make_module(db_session, "pkg:0.1:1:c1", requires_list={"platform": "el8"})
-            got_response = greenwave.query_decision(fake_build, prod_version="xxxx-8")
+        fake_build = make_module(db_session, "pkg:0.1:1:c1", requires_list={"platform": "el8"})
+        got_response = greenwave.query_decision(fake_build, prod_version="xxxx-8")
 
         assert got_response == resp_content
         assert json.loads(mock_requests.post.call_args_list[0][1]["data"]) == {
@@ -157,7 +154,7 @@ class TestGreenwaveQuery():
 
     @pytest.mark.parametrize("policies_satisfied", (True, False))
     @patch("module_build_service.utils.greenwave.requests")
-    def test_greenwave_check_gating(self, mock_requests, policies_satisfied):
+    def test_greenwave_check_gating(self, mock_requests, policies_satisfied, db_session):
         resp_status = 200
         policies_content = {
             "policies": [
@@ -179,8 +176,7 @@ class TestGreenwaveQuery():
         mock_requests.get.return_value = responses[0]
         mock_requests.post.side_effect = responses[1:]
 
-        with make_session(conf) as db_session:
-            fake_build = make_module(db_session, "pkg:0.1:1:c1", requires_list={"platform": "el8"})
-            result = greenwave.check_gating(fake_build)
+        fake_build = make_module(db_session, "pkg:0.1:1:c1", requires_list={"platform": "el8"})
+        result = greenwave.check_gating(fake_build)
 
         assert result == policies_satisfied

tests/test_utils/test_ursine.py

@@ -20,7 +20,6 @@
 from mock import patch, Mock
 
 from module_build_service import conf
-from module_build_service.models import make_session
 from module_build_service.utils import ursine
 from tests import make_module, clean_database
 
@@ -130,28 +129,28 @@ class TestGetModulemdsFromUrsineContent:
         clean_database()
 
     @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession")
-    def test_return_empty_if_no_ursine_build_tag_is_found(self, ClientSession):
-        session = ClientSession.return_value
+    def test_return_empty_if_no_ursine_build_tag_is_found(self, ClientSession, db_session):
+        koji_session = ClientSession.return_value
 
         # No module koji_tag in ursine content yet. This will result in empty
         # ursine modulemds is returned.
-        session.getFullInheritance.return_value = [{"name": "tag-1.0-build"}]
-        session.getExternalRepoList.return_value = [{
+        koji_session.getFullInheritance.return_value = [{"name": "tag-1.0-build"}]
+        koji_session.getExternalRepoList.return_value = [{
             "external_repo_name": "tag-1.0-external-repo",
             "url": "http://example.com/repos/tag-4-build/latest/$arch/",
         }]
 
-        modulemds = ursine.get_modulemds_from_ursine_content("tag")
+        modulemds = ursine.get_modulemds_from_ursine_content(db_session, "tag")
         assert [] == modulemds
 
     @patch.object(conf, "koji_tag_prefixes", new=["module"])
     @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession")
-    def test_get_modulemds(self, ClientSession):
-        session = ClientSession.return_value
+    def test_get_modulemds(self, ClientSession, db_session):
+        koji_session = ClientSession.return_value
 
         # Ensure to to get build tag for further query of ursine content.
         # For this test, the build tag is tag-4-build
-        session.getExternalRepoList.return_value = [{
+        koji_session.getExternalRepoList.return_value = [{
             "external_repo_name": "tag-1.0-external-repo",
             "url": "http://example.com/repos/tag-4-build/latest/$arch/",
         }]
@@ -169,7 +168,7 @@ class TestGetModulemdsFromUrsineContent:
                 ]
             raise ValueError("{} is not handled by test.".format(tag))
 
-        session.getFullInheritance.side_effect = mock_getFullInheritance
+        koji_session.getFullInheritance.side_effect = mock_getFullInheritance
 
         # Defaults to DB resolver, so create fake module builds and store them
         # into database to ensure they can be queried.
@@ -181,25 +180,24 @@ class TestGetModulemdsFromUrsineContent:
         # From the behavior of following code, the reason of the error is
         # mixing use of db.session and make_session, the latter one is called
         # from function ``get_modulemds_from_ursine_content``.
-        with make_session(conf) as db_session:
-            mmd_name1s2020c = make_module(
-                db_session,
-                "name1:s:2020:c", xmd={"mbs": {"koji_tag": "module-name1-s-2020-c"}})
-            mmd_name2s2021c = make_module(
-                db_session,
-                "name2:s:2021:c", xmd={"mbs": {"koji_tag": "module-name2-s-2021-c"}})
+        mmd_name1s2020c = make_module(
+            db_session,
+            "name1:s:2020:c", xmd={"mbs": {"koji_tag": "module-name1-s-2020-c"}})
+        mmd_name2s2021c = make_module(
+            db_session,
+            "name2:s:2021:c", xmd={"mbs": {"koji_tag": "module-name2-s-2021-c"}})
 
-            koji_tag = "tag"  # It's ok to use arbitrary tag name.
-            with patch.object(conf, "koji_external_repo_url_prefix", new="http://example.com/"):
-                modulemds = ursine.get_modulemds_from_ursine_content(koji_tag)
+        koji_tag = "tag"  # It's ok to use arbitrary tag name.
+        with patch.object(conf, "koji_external_repo_url_prefix", new="http://example.com/"):
+            modulemds = ursine.get_modulemds_from_ursine_content(db_session, koji_tag)
 
-            test_nsvcs = [item.get_nsvc() for item in modulemds]
-            test_nsvcs.sort()
+        test_nsvcs = [item.get_nsvc() for item in modulemds]
+        test_nsvcs.sort()
 
-            expected_nsvcs = [mmd_name1s2020c.mmd().get_nsvc(), mmd_name2s2021c.mmd().get_nsvc()]
-            expected_nsvcs.sort()
+        expected_nsvcs = [mmd_name1s2020c.mmd().get_nsvc(), mmd_name2s2021c.mmd().get_nsvc()]
+        expected_nsvcs.sort()
 
-        session.getExternalRepoList.assert_called_once_with(koji_tag)
+        koji_session.getExternalRepoList.assert_called_once_with(koji_tag)
         assert expected_nsvcs == test_nsvcs
 
 
@@ -216,7 +214,7 @@ class TestRecordStreamCollisionModules:
         original_xmd = fake_mmd.get_xmd()
 
         with patch.object(ursine, "log") as log:
-            ursine.handle_stream_collision_modules(fake_mmd)
+            ursine.handle_stream_collision_modules(db_session, fake_mmd)
             assert 2 == log.info.call_count
             find_stream_collision_modules.assert_not_called()
 
@@ -241,7 +239,7 @@ class TestRecordStreamCollisionModules:
         get_modulemds_from_ursine_content.return_value = []
 
         with patch.object(ursine, "log") as log:
-            ursine.handle_stream_collision_modules(fake_mmd)
+            ursine.handle_stream_collision_modules(db_session, fake_mmd)
             assert 2 == log.info.call_count
 
         # Ensure stream_collision_modules is set.
@@ -272,7 +270,7 @@ class TestRecordStreamCollisionModules:
         }
         fake_mmd = make_module(db_session, "name1:s:2020:c", xmd=xmd, store_to_db=False)
 
-        def mock_get_ursine_modulemds(koji_tag):
+        def mock_get_ursine_modulemds(db_session, koji_tag):
             if koji_tag == "module-rhel-8.0-build":
                 return [
                     # This is the one
@@ -325,7 +323,7 @@ class TestRecordStreamCollisionModules:
         koji_session = ClientSession.return_value
         koji_session.listTaggedRPMS.side_effect = mock_listTaggedRPMS
 
-        ursine.handle_stream_collision_modules(fake_mmd)
+        ursine.handle_stream_collision_modules(db_session, fake_mmd)
 
         xmd = fake_mmd.get_xmd()
         buildrequires = xmd["mbs"]["buildrequires"]
@@ -346,9 +344,11 @@ class TestFindStreamCollisionModules:
     """Test ursine.find_stream_collision_modules"""
 
     @patch("module_build_service.utils.ursine.get_modulemds_from_ursine_content")
-    def test_no_modulemds_found_from_ursine_content(self, get_modulemds_from_ursine_content):
+    def test_no_modulemds_found_from_ursine_content(
+        self, get_modulemds_from_ursine_content, db_session
+    ):
         get_modulemds_from_ursine_content.return_value = []
-        assert not ursine.find_stream_collision_modules({}, "koji_tag")
+        assert not ursine.find_stream_collision_modules(db_session, {}, "koji_tag")
 
     @patch("module_build_service.utils.ursine.get_modulemds_from_ursine_content")
     def test_no_collisions_found(self, get_modulemds_from_ursine_content, db_session):
@@ -358,7 +358,8 @@ class TestFindStreamCollisionModules:
             make_module(db_session, "modules:2:1:c2", store_to_db=False),
             make_module(db_session, "modulet:3:1:c3", store_to_db=False),
         ]
-        assert [] == ursine.find_stream_collision_modules(xmd_mbs_buildrequires, "koji_tag")
+        assert [] == ursine.find_stream_collision_modules(
+            db_session, xmd_mbs_buildrequires, "koji_tag")
 
     @patch("module_build_service.utils.ursine.get_modulemds_from_ursine_content")
     def test_collision_modules_are_found(self, get_modulemds_from_ursine_content, db_session):
@@ -370,5 +371,6 @@ class TestFindStreamCollisionModules:
         ]
         get_modulemds_from_ursine_content.return_value = fake_modules
 
-        modules = ursine.find_stream_collision_modules(xmd_mbs_buildrequires, "koji_tag")
+        modules = ursine.find_stream_collision_modules(
+            db_session, xmd_mbs_buildrequires, "koji_tag")
         assert [fake_modules[1].get_nsvc()] == modules

tests/test_utils/test_utils_mse.py

@@ -24,7 +24,7 @@ import pytest
 import module_build_service.utils
 from module_build_service import Modulemd, models
 from module_build_service.errors import StreamAmbigous
-from tests import db, clean_database, make_module, init_data, read_staged_data
+from tests import clean_database, make_module, init_data, read_staged_data
 
 
 class TestUtilsModuleStreamExpansion:
@@ -34,19 +34,17 @@ class TestUtilsModuleStreamExpansion:
     def teardown_method(self, test_method):
         clean_database()
 
-    def _get_mmds_required_by_module_recursively(self, module_build):
+    def _get_mmds_required_by_module_recursively(self, module_build, db_session):
         """
         Convenience wrapper around get_mmds_required_by_module_recursively
         returning the list with nsvc strings of modules returned by this the wrapped
         method.
         """
         mmd = module_build.mmd()
-        module_build_service.utils.expand_mse_streams(db.session, mmd)
-        modules = module_build_service.utils.get_mmds_required_by_module_recursively(mmd)
-        nsvcs = [
-            m.get_nsvc()
-            for m in modules
-        ]
+        module_build_service.utils.expand_mse_streams(db_session, mmd)
+        modules = module_build_service.utils.get_mmds_required_by_module_recursively(
+            db_session, mmd)
+        nsvcs = [m.get_nsvc() for m in modules]
         return nsvcs
 
     def _generate_default_modules(self, db_session):
@@ -164,10 +162,10 @@ class TestUtilsModuleStreamExpansion:
         if stream_ambigous:
             with pytest.raises(StreamAmbigous):
                 module_build_service.utils.generate_expanded_mmds(
-                    db.session, module_build.mmd(), raise_if_stream_ambigous=True)
+                    db_session, module_build.mmd(), raise_if_stream_ambigous=True)
         else:
             module_build_service.utils.generate_expanded_mmds(
-                db.session, module_build.mmd(), raise_if_stream_ambigous=True)
+                db_session, module_build.mmd(), raise_if_stream_ambigous=True)
 
         # Check that if stream is ambigous and we define the stream, it does not raise
         # an exception.
@@ -177,13 +175,13 @@ class TestUtilsModuleStreamExpansion:
                 name, stream = ns.split(":")
                 default_streams[name] = stream
             module_build_service.utils.generate_expanded_mmds(
-                db.session,
+                db_session,
                 module_build.mmd(),
                 raise_if_stream_ambigous=True,
                 default_streams=default_streams,
             )
 
-        mmds = module_build_service.utils.generate_expanded_mmds(db.session, module_build.mmd())
+        mmds = module_build_service.utils.generate_expanded_mmds(db_session, module_build.mmd())
 
         buildrequires_per_mmd_xmd = set()
         buildrequires_per_mmd_buildrequires = set()
@@ -322,7 +320,7 @@ class TestUtilsModuleStreamExpansion:
     def test_get_required_modules_simple(self, requires, build_requires, expected, db_session):
         module_build = make_module(db_session, "app:1:0:c1", requires, build_requires)
         self._generate_default_modules(db_session)
-        nsvcs = self._get_mmds_required_by_module_recursively(module_build)
+        nsvcs = self._get_mmds_required_by_module_recursively(module_build, db_session)
         assert set(nsvcs) == set(expected)
 
     def _generate_default_modules_recursion(self, db_session):
@@ -367,7 +365,7 @@ class TestUtilsModuleStreamExpansion:
     def test_get_required_modules_recursion(self, requires, build_requires, expected, db_session):
         module_build = make_module(db_session, "app:1:0:c1", requires, build_requires)
         self._generate_default_modules_recursion(db_session)
-        nsvcs = self._get_mmds_required_by_module_recursively(module_build)
+        nsvcs = self._get_mmds_required_by_module_recursively(module_build, db_session)
         assert set(nsvcs) == set(expected)
 
     def _generate_default_modules_modules_multiple_stream_versions(self, db_session):
@@ -399,10 +397,10 @@ class TestUtilsModuleStreamExpansion:
     ):
         module_build = make_module(db_session, "app:1:0:c1", requires, build_requires)
         self._generate_default_modules_modules_multiple_stream_versions(db_session)
-        nsvcs = self._get_mmds_required_by_module_recursively(module_build)
+        nsvcs = self._get_mmds_required_by_module_recursively(module_build, db_session)
         assert set(nsvcs) == set(expected)
 
-    def test__get_base_module_mmds(self):
+    def test__get_base_module_mmds(self, db_session):
         """Ensure the correct results are returned without duplicates."""
         init_data(data_size=1, multiple_stream_versions=True)
         mmd = module_build_service.utils.load_mmd(read_staged_data("testmodule_v2.yaml"))
@@ -415,7 +413,7 @@ class TestUtilsModuleStreamExpansion:
         mmd.remove_dependencies(deps)
         mmd.add_dependencies(new_deps)
 
-        mmds = module_build_service.utils.mse._get_base_module_mmds(mmd)
+        mmds = module_build_service.utils.mse._get_base_module_mmds(db_session, mmd)
         expected = set(["platform:f29.0.0", "platform:f29.1.0", "platform:f29.2.0"])
         # Verify no duplicates were returned before doing set operations
         assert len(mmds["ready"]) == len(expected)
@@ -440,7 +438,7 @@ class TestUtilsModuleStreamExpansion:
 
         make_module(db_session, "platform:lp29.1.1:12:c11", {}, {}, virtual_streams=virtual_streams)
 
-        mmds = module_build_service.utils.mse._get_base_module_mmds(mmd)
+        mmds = module_build_service.utils.mse._get_base_module_mmds(db_session, mmd)
         if virtual_streams == ["f29"]:
             expected = set(
                 ["platform:f29.0.0", "platform:f29.1.0", "platform:f29.2.0", "platform:lp29.1.1"])
@@ -458,15 +456,16 @@ class TestUtilsModuleStreamExpansion:
         "module_build_service.config.Config.allow_only_compatible_base_modules",
         new_callable=PropertyMock, return_value=False
     )
-    def test__get_base_module_mmds_virtual_streams_only_major_versions(self, cfg):
+    def test__get_base_module_mmds_virtual_streams_only_major_versions(self, cfg, db_session):
         """Ensure the correct results are returned without duplicates."""
         init_data(data_size=1, multiple_stream_versions=["foo28", "foo29", "foo30"])
 
         # Mark platform:foo28 as garbage to test that it is still considered as compatible.
-        platform = models.ModuleBuild.query.filter_by(name="platform", stream="foo28").first()
+        platform = db_session.query(models.ModuleBuild).filter_by(
+            name="platform", stream="foo28").first()
         platform.state = "garbage"
-        db.session.add(platform)
-        db.session.commit()
+        db_session.add(platform)
+        db_session.commit()
 
         mmd = module_build_service.utils.load_mmd(read_staged_data("testmodule_v2"))
         deps = mmd.get_dependencies()[0]
@@ -477,7 +476,7 @@ class TestUtilsModuleStreamExpansion:
         mmd.remove_dependencies(deps)
         mmd.add_dependencies(new_deps)
 
-        mmds = module_build_service.utils.mse._get_base_module_mmds(mmd)
+        mmds = module_build_service.utils.mse._get_base_module_mmds(db_session, mmd)
         expected = {}
         expected["ready"] = set(["platform:foo29", "platform:foo30"])
         expected["garbage"] = set(["platform:foo28"])