[FIX] carepoint: Fix session handling

* Add method to get session using db name
* Add explicit options for session maker
* Return records from inside of transaction
* Localize session handling

Author: lasley

carepoint/db/carepoint.py

@@ -67,8 +67,8 @@ def __init__(
         """
 
         super(Carepoint, self).__init__()
-        global env, dbs
-        self.env = env
+        global dbs
+        self.env = {}
         self.dbs = dbs
         self.iter_refresh = False
         params = {
@@ -85,7 +85,12 @@ def __init__(
         if not self.dbs.get('cph'):
             self.dbs['cph'] = Db(**params)
         if not self.env.get('cph'):
-            self.env['cph'] = sessionmaker(bind=self.dbs['cph'])
+            self.env['cph'] = sessionmaker(
+                autocommit=False,
+                autoflush=False,
+                bind=self.dbs['cph'],
+                expire_on_commit=True,
+            )
         if smb_user is None:
             self.smb_creds = {
                 'user': user,
@@ -97,9 +102,13 @@ def __init__(
                 'passwd': smb_passwd,
             }
 
+    def _get_model_session(self, model_obj):
+        """ It yields a session for the model_obj """
+        return self._get_session(model_obj.__dbname__)
+
     @contextmanager
-    def _get_session(self, model_obj):
-        session = self.env[model_obj.__dbname__]()
+    def _get_session(self, db_name):
+        session = self.env[db_name]()
         try:
             yield session
             session.commit()
@@ -223,13 +232,13 @@ def read(self, model_obj, record_id, with_entities=None):
         :type with_entities: list or None
         :rtype: :class:`sqlalchemy.engine.ResultProxy`
         """
-        with self._get_session(model_obj) as session:
+        with self._get_model_session(model_obj) as session:
             res = session.query(model_obj).get(record_id)
             if with_entities:
                 res.with_entities(*self._create_entities(
                     model_obj, with_entities
                 ))
-        return res
+            return res
 
     def search(self, model_obj, filters=None, with_entities=None):
         """ Search table by filters and return records
@@ -241,7 +250,7 @@ def search(self, model_obj, filters=None, with_entities=None):
         :type with_entities: list or None
         :rtype: :class:`sqlalchemy.engine.ResultProxy`
         """
-        with self._get_session(model_obj) as session:
+        with self._get_model_session(model_obj) as session:
             if filters is None:
                 filters = {}
             filters = self._unwrap_filters(model_obj, filters)
@@ -250,7 +259,7 @@ def search(self, model_obj, filters=None, with_entities=None):
                 res.with_entities(*self._create_entities(
                     model_obj, with_entities
                 ))
-        return res
+            return res
 
     def create(self, model_obj, vals):
         """ Wrapper to create a record in Carepoint
@@ -260,10 +269,10 @@ def create(self, model_obj, vals):
         :type vals: dict
         :rtype: :class:`sqlalchemy.ext.declarative.Declarative`
         """
-        with self._get_session(model_obj) as session:
+        with self._get_model_session(model_obj) as session:
             record = model_obj(**vals)
             session.add(record)
-        return record
+            return record
 
     def update(self, model_obj, record_id, vals):
         """ Wrapper to update a record in Carepoint
@@ -275,11 +284,11 @@ def update(self, model_obj, record_id, vals):
         :type vals: dict
         :rtype: :class:`sqlalchemy.ext.declarative.Declarative`
         """
-        with self._get_session(model_obj) as session:
+        with self._get_model_session(model_obj) as session:
             record = self.read(model_obj, record_id)
             for key, val in vals.items():
                 setattr(record, key, val)
-        return record
+            return record
 
     def delete(self, model_obj, record_id):
         """ Wrapper to delete a record in Carepoint
@@ -290,7 +299,7 @@ def delete(self, model_obj, record_id):
         :return: Whether the record was found, and deleted
         :rtype: bool
         """
-        with self._get_session(model_obj) as session:
+        with self._get_model_session(model_obj) as session:
             record = self.read(model_obj, record_id)
             result_cnt = record.count()
             if result_cnt == 0:
@@ -308,17 +317,16 @@ def get_pks(self, model_obj):
         """
         return tuple(k.name for k in inspect(model_obj).primary_key)
 
-    def get_next_sequence(self, sequence_name):
+    def get_next_sequence(self, sequence_name, db_name='cph'):
         """ It generates and returns the next int in sequence
         Params:
             sequence_name: ``str`` Name of the sequence in Carepoint DB
+            db_name: ``str`` Name of DB containing sequence stored proc
         Return:
             Integer to use as pk
         """
-        conn = self.dbs['cph'].connect()
-        trans = conn.begin()
-        try:    
-            res = conn.execute(
+        with self._get_session(db_name) as session:  
+            res = session.execute(
                 text(
                     "SET NOCOUNT ON;"
                     "DECLARE @out int = 0;"
@@ -330,13 +338,7 @@ def get_next_sequence(self, sequence_name):
                 seq_name=sequence_name,
             )
             id_int = res.fetchall()[0][0]
-            trans.commit()
-        except:
-            trans.rollback()
-            raise
-        finally:
-            conn.close()
-        return id_int
+            return id_int
 
     def __getattr__(self, key):
         """ Re-implement __getattr__ to use __getitem__ if attr not found """

carepoint/tests/db/test_carepoint.py

@@ -37,7 +37,7 @@ def __get_model_obj(self):
 
     @contextmanager
     def __get_mock_session(self, unwrapped=True):
-        with mock.patch.object(self.carepoint, '_get_session') as mk:
+        with mock.patch.object(self.carepoint, '_get_model_session') as mk:
             enter, session = mock.MagicMock(), mock.MagicMock()
             enter.return_value = enter
             mk.return_value = session
@@ -107,47 +107,47 @@ def test_model_methods(self):
     # Test the session handler
     #
 
-    def test_get_session_gets_session(self):
+    def test_get_model_session_gets_session(self):
         """ It should get session for the database """
         model_obj = self.__get_model_obj()
         with mock.patch.object(self.carepoint, 'env') as env:
-            with self.carepoint._get_session(model_obj):
+            with self.carepoint._get_model_session(model_obj):
                 pass
             env[model_obj.__dbname__].assert_called_once_with()
 
-    def test_get_session_yields_session(self):
+    def test_get_model_session_yields_session(self):
         """ It should yield session for the database """
         model_obj = self.__get_model_obj()
         with mock.patch.object(self.carepoint, 'env') as env:
-            with self.carepoint._get_session(model_obj) as res:
+            with self.carepoint._get_model_session(model_obj) as res:
                 self.assertEqual(
                     env[model_obj.__dbname__](),
                     res,
                 )
 
-    def test_get_session_commit(self):
+    def test_get_model_session_commit(self):
         """ It should commit session after yield """
         model_obj = self.__get_model_obj()
         with mock.patch.object(self.carepoint, 'env') as env:
-            with self.carepoint._get_session(model_obj):
+            with self.carepoint._get_model_session(model_obj):
                 pass
             env[model_obj.__dbname__]().commit.assert_called_once_with()
 
-    def test_get_session_rollback(self):
+    def test_get_model_session_rollback(self):
         """ It should roll session back on error """
         model_obj = self.__get_model_obj()
         with mock.patch.object(self.carepoint, 'env') as env:
             env[model_obj.__dbname__]().commit.side_effect = EndTestException
             with self.assertRaises(EndTestException):
-                with self.carepoint._get_session(model_obj):
+                with self.carepoint._get_model_session(model_obj):
                     pass
             env[model_obj.__dbname__]().rollback.assert_called_once_with()
 
-    def test_get_session_close(self):
+    def test_get_model_session_close(self):
         """ It should always close session """
         model_obj = self.__get_model_obj()
         with mock.patch.object(self.carepoint, 'env') as env:
-            with self.carepoint._get_session(model_obj):
+            with self.carepoint._get_model_session(model_obj):
                 pass
             env[model_obj.__dbname__]().close.assert_called_once_with()
 
@@ -279,66 +279,35 @@ def test_search_returns_response(self):
                 )
 
     # Get Next Sequence
-    def test_get_next_sequence_connect(self):
-        """ It should establish connection to underlying DB """
-        with mock.patch.object(self.carepoint, 'dbs') as dbs:
-            self.carepoint.get_next_sequence(None)
-            dbs['cph'].connect.assert_called_once_with()
-
-    def test_get_next_sequence_transaction(self):
-        """ It should create a new transaction """
-        with mock.patch.object(self.carepoint, 'dbs') as dbs:
-            self.carepoint.get_next_sequence(None)
-            dbs['cph'].connect().begin.assert_called_once_with()
+
+    def test_get_next_sequence_session(self):
+        """ It should get session for db """
+        expect = 'expect'
+        with mock.patch.object(self.carepoint, '_get_session') as mk:
+            res = self.carepoint.get_next_sequence(None, expect)
+            mk.assert_called_once_with(expect)
 
     @mock.patch('carepoint.db.carepoint.text')
     def test_get_next_sequence_execute(self, text):
         """ It should execute stored procedure on connection """
         expect = 'expect'
-        with mock.patch.object(self.carepoint, 'dbs') as dbs:
+        with mock.patch.object(self.carepoint, '_get_session') as mk:
             self.carepoint.get_next_sequence(expect)
-            dbs['cph'].connect().execute.assert_called_once_with(
+            mk().__enter__().execute.assert_called_once_with(
                 text(), seq_name=expect,
             )
 
     def test_get_next_sequence_fetch(self):
         """ It should return result of fetch """
-        with mock.patch.object(self.carepoint, 'dbs') as dbs:
+        with mock.patch.object(self.carepoint, '_get_session') as mk:
             res = self.carepoint.get_next_sequence(None)
-            expect = dbs['cph'].connect().execute().fetchall()[0][0]
+            expect = mk().__enter__().execute().fetchall()[0][0]
             self.assertEqual(
                 expect, res,
             )
 
-    def test_get_next_sequence_commit(self):
-        """ It should commit the transaction """
-        with mock.patch.object(self.carepoint, 'dbs') as dbs:
-            self.carepoint.get_next_sequence(None)
-            dbs['cph'].connect().begin().commit.assert_called_once_with()
-
-    def test_get_next_sequence_transaction(self):
-        """ It should create a new transaction """
-        with mock.patch.object(self.carepoint, 'dbs') as dbs:
-            self.carepoint.get_next_sequence(None)
-            dbs['cph'].connect().begin.assert_called_once_with()
-
-    def test_get_next_sequence_rollback(self):
-        """ It should rollback transaction and raise on error """
-        with mock.patch.object(self.carepoint, 'dbs') as dbs:
-            conn = dbs['cph'].connect()
-            conn.execute.side_effect = EndTestException
-            with self.assertRaises(EndTestException):
-                self.carepoint.get_next_sequence(None)
-            conn.begin().rollback.assert_called_once_with()
-
-    def test_get_next_sequence_close(self):
-        """ It should close the connection """
-        with mock.patch.object(self.carepoint, 'dbs') as dbs:
-            self.carepoint.get_next_sequence(None)
-            dbs['cph'].connect().close.assert_called_once_with()
-
     # Create
-    def test_create_calls_get_session_with_model_obj(self):
+    def test_create_calls_get_model_session_with_model_obj(self):
         model_obj = mock.MagicMock()
         with self.__get_mock_session(False) as mk:
             self.carepoint.create(model_obj, {})
@@ -370,7 +339,7 @@ def test_create_returns_new_record(self):
             self.assertEqual(response, response_expect)
 
     # Update
-    def test_update_calls_get_session_with_model_obj(self):
+    def test_update_calls_get_model_session_with_model_obj(self):
         model_obj = self.__get_model_obj()
         record_id = 1
         with self.__get_mock_session(False) as mk:
@@ -413,7 +382,7 @@ def test_update_returns_record(self):
                 self.assertEqual(read(), response)
 
     # Delete
-    def test_delete_calls_get_session_with_model_obj(self):
+    def test_delete_calls_get_model_session_with_model_obj(self):
         model_obj = self.__get_model_obj()
         record_id = 1
         with self.__get_mock_session(False) as mk:

setup.py

@@ -9,10 +9,10 @@
 
 setup_vals = {
     'name': 'carepoint',
-    'version': '0.1.3',
+    'version': '0.1.4',
     'author': 'LasLabs Inc.',
     'author_email': 'support@laslabs.com',
-    'description': 'This library will allow you to interact with CarePoint'
+    'description': 'This library will allow you to interact with CarePoint '
                    'using Python.',
     'url': 'https://github.com/laslabs/python-carepoint',
     'license': 'MIT',