[session, repository] deprecate repo.connect and move .close reponsibility to session object
authorAurelien Campeas <aurelien.campeas@logilab.fr>
Thu, 12 Jun 2014 15:54:44 +0200
changeset 11195 5de859b95988
parent 11194 b6b00bb1e528
child 11196 74b04a88d28a
[session, repository] deprecate repo.connect and move .close reponsibility to session object Repository.new_session just returns a plain session object, and `.connect` (which returns a sessionid) is deprecated. For .close:: session.close() # done ! The session will bear the responsibility to call the "session_close" event but that's better than the previous idiom:: repo.close(session.sessionid) which involves both objects. Related to #1381328. Related to #2919309.
cubicweb/devtools/__init__.py
cubicweb/devtools/testlib.py
cubicweb/repoapi.py
cubicweb/server/repository.py
cubicweb/server/session.py
cubicweb/server/test/unittest_ldapsource.py
cubicweb/server/test/unittest_repository.py
cubicweb/server/test/unittest_security.py
cubicweb/web/views/authentication.py
cubicweb/web/views/sessions.py
--- a/cubicweb/devtools/__init__.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/devtools/__init__.py	Thu Jun 12 15:54:44 2014 +0200
@@ -106,13 +106,7 @@
     * system source is shutdown
     """
     if not repo._needs_refresh:
-        for sessionid in list(repo._sessions):
-            warnings.warn('%s Open session found while turning repository off'
-                          %sessionid, RuntimeWarning)
-            try:
-                repo.close(sessionid)
-            except BadConnectionId: #this is strange ? thread issue ?
-                print('XXX unknown session', sessionid)
+        repo.close_sessions()
         for cnxset in repo.cnxsets:
             cnxset.close(True)
         repo.system_source.shutdown()
--- a/cubicweb/devtools/testlib.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/devtools/testlib.py	Thu Jun 12 15:54:44 2014 +0200
@@ -267,9 +267,7 @@
 
     def close(self):
         """Close the session associated to the RepoAccess"""
-        if self._session is not None:
-            self._repo.close(self._session.sessionid)
-        self._session = None
+        self._session.close()
 
     @contextmanager
     def shell(self):
@@ -457,7 +455,7 @@
     def tearDown(self):
         # XXX hack until logilab.common.testlib is fixed
         if self._admin_session is not None:
-            self.repo.close(self._admin_session.sessionid)
+            self._admin_session.close()
             self._admin_session = None
         while self._cleanups:
             cleanup, args, kwargs = self._cleanups.pop(-1)
--- a/cubicweb/repoapi.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/repoapi.py	Thu Jun 12 15:54:44 2014 +0200
@@ -47,11 +47,7 @@
     """Take credential and return associated Connection.
 
     raise AuthenticationError if the credential are invalid."""
-    sessionid = repo.connect(login, **kwargs)
-    session = repo._get_session(sessionid)
-    # XXX the autoclose_session should probably be handle on the session directly
-    # this is something to consider once we have proper server side Connection.
-    return Connection(session)
+    return repo.new_session(login, **kwargs).new_cnx()
 
 def anonymous_cnx(repo):
     """return a Connection for Anonymous user.
--- a/cubicweb/server/repository.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/server/repository.py	Thu Jun 12 15:54:44 2014 +0200
@@ -633,7 +633,7 @@
             return rset.rows
 
     def new_session(self, login, **kwargs):
-        """open a new session for a given user
+        """open a *new* session for a given user
 
         raise `AuthenticationError` if the authentication failed
         raise `ConnectionError` if we can't open a connection
@@ -655,33 +655,20 @@
             cnx.commit()
         return session
 
+    @deprecated('[3.23] use .new_session instead (and get a plain session object)')
     def connect(self, login, **kwargs):
-        """open a new session for a given user and return its sessionid """
         return self.new_session(login, **kwargs).sessionid
 
-    def close(self, sessionid, txid=None, checkshuttingdown=True):
-        """close the session with the given id"""
-        session = self._get_session(sessionid, txid=txid,
-                                    checkshuttingdown=checkshuttingdown)
-        # operation uncommited before close are rolled back before hook is called
-        with session.new_cnx() as cnx:
-            self.hm.call_hooks('session_close', cnx)
-            # commit connection at this point in case write operation has been
-            # done during `session_close` hooks
-            cnx.commit()
-        session.close()
-        del self._sessions[sessionid]
-        self.info('closed session %s for user %s', sessionid, session.user.login)
+    @deprecated('[3.23] use session.close() directly')
+    def close(self, sessionid):
+        self._get_session(sessionid).close()
 
     # session handling ########################################################
 
     def close_sessions(self):
         """close every opened sessions"""
-        for sessionid in list(self._sessions):
-            try:
-                self.close(sessionid, checkshuttingdown=False)
-            except Exception: # XXX BaseException?
-                self.exception('error while closing session %s' % sessionid)
+        for session in list(self._sessions.values()):
+            session.close()
 
     def clean_sessions(self):
         """close sessions not used since an amount of time specified in the
@@ -691,9 +678,9 @@
         self.debug('cleaning session unused since %s',
                    strftime('%H:%M:%S', localtime(mintime)))
         nbclosed = 0
-        for session in self._sessions.values():
+        for session in list(self._sessions.values()):
             if session.timestamp < mintime:
-                self.close(session.sessionid)
+                session.close()
                 nbclosed += 1
         return nbclosed
 
--- a/cubicweb/server/session.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/server/session.py	Thu Jun 12 15:54:44 2014 +0200
@@ -1017,7 +1017,15 @@
         self.closed = False
 
     def close(self):
+        if self.closed:
+            self.warning('closing already closed session %s', self.sessionid)
+            return
+        with self.new_cnx() as cnx:
+            self.repo.hm.call_hooks('session_close', cnx)
+            cnx.commit()
+            del self.repo._sessions[self.sessionid]
         self.closed = True
+        self.info('closed session %s for user %s', self.sessionid, self.user.login)
 
     def __enter__(self):
         return self
--- a/cubicweb/server/test/unittest_ldapsource.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/server/test/unittest_ldapsource.py	Thu Jun 12 15:54:44 2014 +0200
@@ -238,9 +238,9 @@
             self.assertRaises(AuthenticationError,
                               source.authenticate, cnx, 'toto', 'toto')
             self.assertTrue(source.authenticate(cnx, 'syt', 'syt'))
-        sessionid = self.repo.connect('syt', password='syt')
-        self.assertTrue(sessionid)
-        self.repo.close(sessionid)
+        session = self.repo.new_session('syt', password='syt')
+        self.assertTrue(session)
+        session.close()
 
     def test_base(self):
         with self.admin_access.repo_cnx() as cnx:
@@ -321,7 +321,7 @@
             cnx.commit()
         with self.repo.internal_cnx() as cnx:
             self.pull(cnx)
-        self.assertRaises(AuthenticationError, self.repo.connect, 'syt', password='syt')
+        self.assertRaises(AuthenticationError, self.repo.new_session, 'syt', password='syt')
         with self.admin_access.repo_cnx() as cnx:
             self.assertEqual(cnx.execute('Any N WHERE U login "syt", '
                                          'U in_state S, S name N').rows[0][0],
@@ -350,7 +350,7 @@
         self.delete_ldap_entry('uid=syt,ou=People,dc=cubicweb,dc=test')
         with self.repo.internal_cnx() as cnx:
             self.pull(cnx)
-        self.assertRaises(AuthenticationError, self.repo.connect, 'syt', password='syt')
+        self.assertRaises(AuthenticationError, self.repo.new_session, 'syt', password='syt')
         with self.admin_access.repo_cnx() as cnx:
             self.assertEqual(cnx.execute('Any N WHERE U login "syt", '
                                          'U in_state S, S name N').rows[0][0],
@@ -396,16 +396,16 @@
             user.cw_adapt_to('IWorkflowable').fire_transition('activate')
             cnx.commit()
             with self.assertRaises(AuthenticationError):
-                self.repo.connect('syt', password='syt')
+                self.repo.new_session('syt', password='syt')
 
             # ok now let's try to make it a system user
             cnx.execute('SET X cw_source S WHERE X eid %(x)s, S name "system"', {'x': user.eid})
             cnx.commit()
         # and that we can now authenticate again
-        self.assertRaises(AuthenticationError, self.repo.connect, 'syt', password='toto')
-        sessionid = self.repo.connect('syt', password='syt')
-        self.assertTrue(sessionid)
-        self.repo.close(sessionid)
+        self.assertRaises(AuthenticationError, self.repo.new_session, 'syt', password='toto')
+        session = self.repo.new_session('syt', password='syt')
+        self.assertTrue(session)
+        session.close()
 
 
 class LDAPFeedGroupTC(LDAPFeedTestBase):
--- a/cubicweb/server/test/unittest_repository.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/server/test/unittest_repository.py	Thu Jun 12 15:54:44 2014 +0200
@@ -18,7 +18,6 @@
 # with CubicWeb.  If not, see <http://www.gnu.org/licenses/>.
 """unit tests for module cubicweb.server.repository"""
 
-import threading
 import time
 import logging
 
@@ -39,7 +38,6 @@
 from cubicweb.server.sqlutils import SQL_PREFIX
 from cubicweb.server.hook import Hook
 from cubicweb.server.sources import native
-from cubicweb.server.session import SessionClosedError
 
 
 class RepositoryTC(CubicWebTC):
@@ -78,9 +76,9 @@
             self.assertFalse(cnx.execute('Any X WHERE NOT X cw_source S'))
 
     def test_connect(self):
-        cnxid = self.repo.connect(self.admlogin, password=self.admpassword)
-        self.assertTrue(cnxid)
-        self.repo.close(cnxid)
+        session = self.repo.new_session(self.admlogin, password=self.admpassword)
+        self.assertTrue(session.sessionid)
+        session.close()
         self.assertRaises(AuthenticationError,
                           self.repo.connect, self.admlogin, password='nimportnawak')
         self.assertRaises(AuthenticationError,
@@ -101,9 +99,9 @@
                         {'login': u"barnabé", 'passwd': u"héhéhé".encode('UTF8')})
             cnx.commit()
         repo = self.repo
-        cnxid = repo.connect(u"barnabé", password=u"héhéhé".encode('UTF8'))
-        self.assertTrue(cnxid)
-        repo.close(cnxid)
+        session = repo.new_session(u"barnabé", password=u"héhéhé".encode('UTF8'))
+        self.assertTrue(session.sessionid)
+        session.close()
 
     def test_rollback_on_execute_validation_error(self):
         class ValidationErrorAfterHook(Hook):
@@ -146,9 +144,9 @@
 
     def test_close(self):
         repo = self.repo
-        cnxid = repo.connect(self.admlogin, password=self.admpassword)
-        self.assertTrue(cnxid)
-        repo.close(cnxid)
+        session = repo.new_session(self.admlogin, password=self.admpassword)
+        self.assertTrue(session.sessionid)
+        session.close()
 
 
     def test_initial_schema(self):
@@ -196,13 +194,12 @@
 
     def test_internal_api(self):
         repo = self.repo
-        cnxid = repo.connect(self.admlogin, password=self.admpassword)
-        session = repo._get_session(cnxid)
+        session = repo.new_session(self.admlogin, password=self.admpassword)
         with session.new_cnx() as cnx:
             self.assertEqual(repo.type_and_source_from_eid(2, cnx),
                              ('CWGroup', None, 'system'))
             self.assertEqual(repo.type_from_eid(2, cnx), 'CWGroup')
-        repo.close(cnxid)
+        session.close()
 
     def test_public_api(self):
         self.assertEqual(self.repo.get_schema(), self.repo.schema)
--- a/cubicweb/server/test/unittest_security.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/server/test/unittest_security.py	Thu Jun 12 15:54:44 2014 +0200
@@ -84,13 +84,15 @@
             oldhash = cnx.system_sql("SELECT cw_upassword FROM cw_CWUser "
                                          "WHERE cw_login = 'oldpassword'").fetchone()[0]
             oldhash = self.repo.system_source.binary_to_str(oldhash)
-            self.repo.close(self.repo.connect('oldpassword', password='oldpassword'))
+            session = self.repo.new_session('oldpassword', password='oldpassword')
+            session.close()
             newhash = cnx.system_sql("SELECT cw_upassword FROM cw_CWUser "
                                      "WHERE cw_login = 'oldpassword'").fetchone()[0]
             newhash = self.repo.system_source.binary_to_str(newhash)
             self.assertNotEqual(oldhash, newhash)
             self.assertTrue(newhash.startswith(b'$6$'))
-            self.repo.close(self.repo.connect('oldpassword', password='oldpassword'))
+            session = self.repo.new_session('oldpassword', password='oldpassword')
+            session.close()
             newnewhash = cnx.system_sql("SELECT cw_upassword FROM cw_CWUser WHERE "
                                         "cw_login = 'oldpassword'").fetchone()[0]
             newnewhash = self.repo.system_source.binary_to_str(newnewhash)
@@ -300,7 +302,8 @@
             cnx.execute('SET X upassword %(passwd)s WHERE X eid %(x)s',
                        {'x': ueid, 'passwd': b'newpwd'})
             cnx.commit()
-        self.repo.close(self.repo.connect('user', password='newpwd'))
+        session = self.repo.new_session('user', password='newpwd')
+        session.close()
 
     def test_user_cant_change_other_upassword(self):
         with self.admin_access.repo_cnx() as cnx:
--- a/cubicweb/web/views/authentication.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/web/views/authentication.py	Thu Jun 12 15:54:44 2014 +0200
@@ -170,5 +170,4 @@
         raise AuthenticationError()
 
     def _authenticate(self, login, authinfo):
-        sessionid = self.repo.connect(login, **authinfo)
-        return self.repo._sessions[sessionid]
+        return self.repo.new_session(login, **authinfo)
--- a/cubicweb/web/views/sessions.py	Wed Mar 09 14:11:47 2016 +0100
+++ b/cubicweb/web/views/sessions.py	Thu Jun 12 15:54:44 2014 +0200
@@ -176,5 +176,4 @@
         """
         self.info('closing http session %s' % session.sessionid)
         self._sessions.pop(session.sessionid, None)
-        if not session.closed:
-            session.repo.close(session.sessionid)
+        session.close()