[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.

authorAurelien Campeas <aurelien.campeas@logilab.fr>
changeset5de859b95988
branchdefault
phasepublic
hiddenno
parent revision#b6b00bb1e528 [web/test] fix typo in doc string
child revision#69aa09ec38d5 [session] drop session-as-a-context-manager, #098f33dc1180 [session,repo] remove last trace of "cnxprops", #27253355de86 [session] fix super call in Connection.__init__, #bcc92804ea25 [devtools] adapt stresstester to current APIs, #c13e9fbfffa8 [devtools/repotest] use the RepoAccess object, #74b04a88d28a [test] fix test_printable_value_bytes with current pygments
files modified by this revision
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
# HG changeset patch
# User Aurelien Campeas <aurelien.campeas@logilab.fr>
# Date 1402581284 -7200
# Thu Jun 12 15:54:44 2014 +0200
# Node ID 5de859b95988ca6ad576e9765380c53ec5ef88b7
# Parent b6b00bb1e528698ce8c82e9f9800c7f5ab9ff88d
[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.

diff --git a/cubicweb/devtools/__init__.py b/cubicweb/devtools/__init__.py
@@ -104,17 +104,11 @@
1      * session are closed,
2      * cnxsets are closed
3      * system source is shutdown
4      """
5      if not repo._needs_refresh:
6 -        for sessionid in list(repo._sessions):
7 -            warnings.warn('%s Open session found while turning repository off'
8 -                          %sessionid, RuntimeWarning)
9 -            try:
10 -                repo.close(sessionid)
11 -            except BadConnectionId: #this is strange ? thread issue ?
12 -                print('XXX unknown session', sessionid)
13 +        repo.close_sessions()
14          for cnxset in repo.cnxsets:
15              cnxset.close(True)
16          repo.system_source.shutdown()
17          repo._needs_refresh = True
18          repo._has_started = False
diff --git a/cubicweb/devtools/testlib.py b/cubicweb/devtools/testlib.py
@@ -265,13 +265,11 @@
19              req.set_cnx(cnx)
20              yield req
21 
22      def close(self):
23          """Close the session associated to the RepoAccess"""
24 -        if self._session is not None:
25 -            self._repo.close(self._session.sessionid)
26 -        self._session = None
27 +        self._session.close()
28 
29      @contextmanager
30      def shell(self):
31          from cubicweb.server.migractions import ServerMigrationHelper
32          with self._session.new_cnx() as cnx:
@@ -455,11 +453,11 @@
33          MAILBOX[:] = []  # reset mailbox
34 
35      def tearDown(self):
36          # XXX hack until logilab.common.testlib is fixed
37          if self._admin_session is not None:
38 -            self.repo.close(self._admin_session.sessionid)
39 +            self._admin_session.close()
40              self._admin_session = None
41          while self._cleanups:
42              cleanup, args, kwargs = self._cleanups.pop(-1)
43              cleanup(*args, **kwargs)
44          self.repo.turn_repo_off()
diff --git a/cubicweb/repoapi.py b/cubicweb/repoapi.py
@@ -45,15 +45,11 @@
45 
46  def connect(repo, login, **kwargs):
47      """Take credential and return associated Connection.
48 
49      raise AuthenticationError if the credential are invalid."""
50 -    sessionid = repo.connect(login, **kwargs)
51 -    session = repo._get_session(sessionid)
52 -    # XXX the autoclose_session should probably be handle on the session directly
53 -    # this is something to consider once we have proper server side Connection.
54 -    return Connection(session)
55 +    return repo.new_session(login, **kwargs).new_cnx()
56 
57  def anonymous_cnx(repo):
58      """return a Connection for Anonymous user.
59 
60      raises an AuthenticationError if anonymous usage is not allowed
diff --git a/cubicweb/server/repository.py b/cubicweb/server/repository.py
@@ -631,11 +631,11 @@
61                                                for attr in query_attrs),
62                                 query_attrs)
63              return rset.rows
64 
65      def new_session(self, login, **kwargs):
66 -        """open a new session for a given user
67 +        """open a *new* session for a given user
68 
69          raise `AuthenticationError` if the authentication failed
70          raise `ConnectionError` if we can't open a connection
71          """
72          cnxprops = kwargs.pop('cnxprops', None)
@@ -653,49 +653,36 @@
73              # commit connection at this point in case write operation has been
74              # done during `session_open` hooks
75              cnx.commit()
76          return session
77 
78 +    @deprecated('[3.23] use .new_session instead (and get a plain session object)')
79      def connect(self, login, **kwargs):
80 -        """open a new session for a given user and return its sessionid """
81          return self.new_session(login, **kwargs).sessionid
82 
83 -    def close(self, sessionid, txid=None, checkshuttingdown=True):
84 -        """close the session with the given id"""
85 -        session = self._get_session(sessionid, txid=txid,
86 -                                    checkshuttingdown=checkshuttingdown)
87 -        # operation uncommited before close are rolled back before hook is called
88 -        with session.new_cnx() as cnx:
89 -            self.hm.call_hooks('session_close', cnx)
90 -            # commit connection at this point in case write operation has been
91 -            # done during `session_close` hooks
92 -            cnx.commit()
93 -        session.close()
94 -        del self._sessions[sessionid]
95 -        self.info('closed session %s for user %s', sessionid, session.user.login)
96 +    @deprecated('[3.23] use session.close() directly')
97 +    def close(self, sessionid):
98 +        self._get_session(sessionid).close()
99 
100      # session handling ########################################################
101 
102      def close_sessions(self):
103          """close every opened sessions"""
104 -        for sessionid in list(self._sessions):
105 -            try:
106 -                self.close(sessionid, checkshuttingdown=False)
107 -            except Exception: # XXX BaseException?
108 -                self.exception('error while closing session %s' % sessionid)
109 +        for session in list(self._sessions.values()):
110 +            session.close()
111 
112      def clean_sessions(self):
113          """close sessions not used since an amount of time specified in the
114          configuration
115          """
116          mintime = time() - self.cleanup_session_time
117          self.debug('cleaning session unused since %s',
118                     strftime('%H:%M:%S', localtime(mintime)))
119          nbclosed = 0
120 -        for session in self._sessions.values():
121 +        for session in list(self._sessions.values()):
122              if session.timestamp < mintime:
123 -                self.close(session.sessionid)
124 +                session.close()
125                  nbclosed += 1
126          return nbclosed
127 
128      @contextmanager
129      def internal_cnx(self):
diff --git a/cubicweb/server/session.py b/cubicweb/server/session.py
@@ -1015,11 +1015,19 @@
130          self._timestamp = Timestamp()
131          self.data = {}
132          self.closed = False
133 
134      def close(self):
135 +        if self.closed:
136 +            self.warning('closing already closed session %s', self.sessionid)
137 +            return
138 +        with self.new_cnx() as cnx:
139 +            self.repo.hm.call_hooks('session_close', cnx)
140 +            cnx.commit()
141 +            del self.repo._sessions[self.sessionid]
142          self.closed = True
143 +        self.info('closed session %s for user %s', self.sessionid, self.user.login)
144 
145      def __enter__(self):
146          return self
147 
148      def __exit__(self, *args):
diff --git a/cubicweb/server/test/unittest_ldapsource.py b/cubicweb/server/test/unittest_ldapsource.py
@@ -236,13 +236,13 @@
149          with self.admin_access.repo_cnx() as cnx:
150              # ensure we won't be logged against
151              self.assertRaises(AuthenticationError,
152                                source.authenticate, cnx, 'toto', 'toto')
153              self.assertTrue(source.authenticate(cnx, 'syt', 'syt'))
154 -        sessionid = self.repo.connect('syt', password='syt')
155 -        self.assertTrue(sessionid)
156 -        self.repo.close(sessionid)
157 +        session = self.repo.new_session('syt', password='syt')
158 +        self.assertTrue(session)
159 +        session.close()
160 
161      def test_base(self):
162          with self.admin_access.repo_cnx() as cnx:
163              # check a known one
164              rset = cnx.execute('CWUser X WHERE X login %(login)s', {'login': 'syt'})
@@ -319,11 +319,11 @@
165              config['user-filter'] = u'(%s=%s)' % ('telephoneNumber', '109')
166              source.repo_source.update_config(source, config)
167              cnx.commit()
168          with self.repo.internal_cnx() as cnx:
169              self.pull(cnx)
170 -        self.assertRaises(AuthenticationError, self.repo.connect, 'syt', password='syt')
171 +        self.assertRaises(AuthenticationError, self.repo.new_session, 'syt', password='syt')
172          with self.admin_access.repo_cnx() as cnx:
173              self.assertEqual(cnx.execute('Any N WHERE U login "syt", '
174                                           'U in_state S, S name N').rows[0][0],
175                               'deactivated')
176              self.assertEqual(cnx.execute('Any N WHERE U login "adim", '
@@ -348,11 +348,11 @@
177          read syt, pull, check activation
178          """
179          self.delete_ldap_entry('uid=syt,ou=People,dc=cubicweb,dc=test')
180          with self.repo.internal_cnx() as cnx:
181              self.pull(cnx)
182 -        self.assertRaises(AuthenticationError, self.repo.connect, 'syt', password='syt')
183 +        self.assertRaises(AuthenticationError, self.repo.new_session, 'syt', password='syt')
184          with self.admin_access.repo_cnx() as cnx:
185              self.assertEqual(cnx.execute('Any N WHERE U login "syt", '
186                                           'U in_state S, S name N').rows[0][0],
187                               'deactivated')
188          with self.repo.internal_cnx() as cnx:
@@ -394,20 +394,20 @@
189              # reactivate user (which source is still ldap-feed)
190              user = cnx.execute('CWUser U WHERE U login "syt"').get_entity(0, 0)
191              user.cw_adapt_to('IWorkflowable').fire_transition('activate')
192              cnx.commit()
193              with self.assertRaises(AuthenticationError):
194 -                self.repo.connect('syt', password='syt')
195 +                self.repo.new_session('syt', password='syt')
196 
197              # ok now let's try to make it a system user
198              cnx.execute('SET X cw_source S WHERE X eid %(x)s, S name "system"', {'x': user.eid})
199              cnx.commit()
200          # and that we can now authenticate again
201 -        self.assertRaises(AuthenticationError, self.repo.connect, 'syt', password='toto')
202 -        sessionid = self.repo.connect('syt', password='syt')
203 -        self.assertTrue(sessionid)
204 -        self.repo.close(sessionid)
205 +        self.assertRaises(AuthenticationError, self.repo.new_session, 'syt', password='toto')
206 +        session = self.repo.new_session('syt', password='syt')
207 +        self.assertTrue(session)
208 +        session.close()
209 
210 
211  class LDAPFeedGroupTC(LDAPFeedTestBase):
212      """
213      A testcase for group support in ldapfeed.
diff --git a/cubicweb/server/test/unittest_repository.py b/cubicweb/server/test/unittest_repository.py
@@ -16,11 +16,10 @@
214  #
215  # You should have received a copy of the GNU Lesser General Public License along
216  # with CubicWeb.  If not, see <http://www.gnu.org/licenses/>.
217  """unit tests for module cubicweb.server.repository"""
218 
219 -import threading
220  import time
221  import logging
222 
223  from six.moves import range
224 
@@ -37,11 +36,10 @@
225  from cubicweb.devtools.repotest import tuplify
226  from cubicweb.server import hook
227  from cubicweb.server.sqlutils import SQL_PREFIX
228  from cubicweb.server.hook import Hook
229  from cubicweb.server.sources import native
230 -from cubicweb.server.session import SessionClosedError
231 
232 
233  class RepositoryTC(CubicWebTC):
234      """ singleton providing access to a persistent storage for entities
235      and relation
@@ -76,13 +74,13 @@
236      def test_all_entities_have_cw_source(self):
237          with self.admin_access.repo_cnx() as cnx:
238              self.assertFalse(cnx.execute('Any X WHERE NOT X cw_source S'))
239 
240      def test_connect(self):
241 -        cnxid = self.repo.connect(self.admlogin, password=self.admpassword)
242 -        self.assertTrue(cnxid)
243 -        self.repo.close(cnxid)
244 +        session = self.repo.new_session(self.admlogin, password=self.admpassword)
245 +        self.assertTrue(session.sessionid)
246 +        session.close()
247          self.assertRaises(AuthenticationError,
248                            self.repo.connect, self.admlogin, password='nimportnawak')
249          self.assertRaises(AuthenticationError,
250                            self.repo.connect, self.admlogin, password='')
251          self.assertRaises(AuthenticationError,
@@ -99,13 +97,13 @@
252              cnx.execute('INSERT CWUser X: X login %(login)s, X upassword %(passwd)s, '
253                          'X in_group G WHERE G name "users"',
254                          {'login': u"barnab�", 'passwd': u"h�h�h�".encode('UTF8')})
255              cnx.commit()
256          repo = self.repo
257 -        cnxid = repo.connect(u"barnab�", password=u"h�h�h�".encode('UTF8'))
258 -        self.assertTrue(cnxid)
259 -        repo.close(cnxid)
260 +        session = repo.new_session(u"barnab�", password=u"h�h�h�".encode('UTF8'))
261 +        self.assertTrue(session.sessionid)
262 +        session.close()
263 
264      def test_rollback_on_execute_validation_error(self):
265          class ValidationErrorAfterHook(Hook):
266              __regid__ = 'valerror-after-hook'
267              __select__ = Hook.__select__ & is_instance('CWGroup')
@@ -144,13 +142,13 @@
268                  self.assertFalse(cnx.execute('Any X WHERE X is CWGroup, X name "toto"'))
269 
270 
271      def test_close(self):
272          repo = self.repo
273 -        cnxid = repo.connect(self.admlogin, password=self.admpassword)
274 -        self.assertTrue(cnxid)
275 -        repo.close(cnxid)
276 +        session = repo.new_session(self.admlogin, password=self.admpassword)
277 +        self.assertTrue(session.sessionid)
278 +        session.close()
279 
280 
281      def test_initial_schema(self):
282          schema = self.repo.schema
283          # check order of attributes is respected
@@ -194,17 +192,16 @@
284          ownedby = schema.rschema('owned_by')
285          self.assertEqual(ownedby.objects('CWEType'), ('CWUser',))
286 
287      def test_internal_api(self):
288          repo = self.repo
289 -        cnxid = repo.connect(self.admlogin, password=self.admpassword)
290 -        session = repo._get_session(cnxid)
291 +        session = repo.new_session(self.admlogin, password=self.admpassword)
292          with session.new_cnx() as cnx:
293              self.assertEqual(repo.type_and_source_from_eid(2, cnx),
294                               ('CWGroup', None, 'system'))
295              self.assertEqual(repo.type_from_eid(2, cnx), 'CWGroup')
296 -        repo.close(cnxid)
297 +        session.close()
298 
299      def test_public_api(self):
300          self.assertEqual(self.repo.get_schema(), self.repo.schema)
301          self.assertEqual(self.repo.source_defs(), {'system': {'type': 'native',
302                                                                'uri': 'system',
diff --git a/cubicweb/server/test/unittest_security.py b/cubicweb/server/test/unittest_security.py
@@ -82,17 +82,19 @@
303          """
304          with self.repo.internal_cnx() as cnx:
305              oldhash = cnx.system_sql("SELECT cw_upassword FROM cw_CWUser "
306                                           "WHERE cw_login = 'oldpassword'").fetchone()[0]
307              oldhash = self.repo.system_source.binary_to_str(oldhash)
308 -            self.repo.close(self.repo.connect('oldpassword', password='oldpassword'))
309 +            session = self.repo.new_session('oldpassword', password='oldpassword')
310 +            session.close()
311              newhash = cnx.system_sql("SELECT cw_upassword FROM cw_CWUser "
312                                       "WHERE cw_login = 'oldpassword'").fetchone()[0]
313              newhash = self.repo.system_source.binary_to_str(newhash)
314              self.assertNotEqual(oldhash, newhash)
315              self.assertTrue(newhash.startswith(b'$6$'))
316 -            self.repo.close(self.repo.connect('oldpassword', password='oldpassword'))
317 +            session = self.repo.new_session('oldpassword', password='oldpassword')
318 +            session.close()
319              newnewhash = cnx.system_sql("SELECT cw_upassword FROM cw_CWUser WHERE "
320                                          "cw_login = 'oldpassword'").fetchone()[0]
321              newnewhash = self.repo.system_source.binary_to_str(newnewhash)
322              self.assertEqual(newhash, newnewhash)
323 
@@ -298,11 +300,12 @@
324              ueid = self.create_user(cnx, u'user').eid
325          with self.new_access(u'user').repo_cnx() as cnx:
326              cnx.execute('SET X upassword %(passwd)s WHERE X eid %(x)s',
327                         {'x': ueid, 'passwd': b'newpwd'})
328              cnx.commit()
329 -        self.repo.close(self.repo.connect('user', password='newpwd'))
330 +        session = self.repo.new_session('user', password='newpwd')
331 +        session.close()
332 
333      def test_user_cant_change_other_upassword(self):
334          with self.admin_access.repo_cnx() as cnx:
335              ueid = self.create_user(cnx, u'otheruser').eid
336          with self.new_access(u'iaminusersgrouponly').repo_cnx() as cnx:
diff --git a/cubicweb/web/views/authentication.py b/cubicweb/web/views/authentication.py
@@ -168,7 +168,6 @@
337              session = self._authenticate(login, authinfo)
338              return session, login
339          raise AuthenticationError()
340 
341      def _authenticate(self, login, authinfo):
342 -        sessionid = self.repo.connect(login, **authinfo)
343 -        return self.repo._sessions[sessionid]
344 +        return self.repo.new_session(login, **authinfo)
diff --git a/cubicweb/web/views/sessions.py b/cubicweb/web/views/sessions.py
@@ -174,7 +174,6 @@
345          """close session on logout or on invalid session detected (expired out,
346          corrupted...)
347          """
348          self.info('closing http session %s' % session.sessionid)
349          self._sessions.pop(session.sessionid, None)
350 -        if not session.closed:
351 -            session.repo.close(session.sessionid)
352 +        session.close()
obsoletes