[rset] kill the rset._rqlst cache

Right now it "works" for the standard, internal uses.

However when we will fold ClientConnection and Connection, it will hurt, because suddenly we get more cache hits, and the following situation would become commonplace:

  • there is an un-annotated _rqlst given by the querier
  • some view (e.g. facets) requests the .syntax_tree, which takes a copy of _rqlst
  • the view actually expects the rql syntax tree to be annotated, but it was not, hence we crash.

Related to #3837233.

authorAurelien Campeas <aurelien.campeas@logilab.fr>
changeset824e1c797eb2
branchdefault
phasedraft
hiddenyes
parent revision#f7e574156e80 [source/native] session -> cnx
child revision#fe0b4c2efd17 [entity/req] pass the _cw object to cw_instantiate
files modified by this revision
repoapi.py
rset.py
server/querier.py
test/unittest_rset.py
# HG changeset patch
# User Aurelien Campeas <aurelien.campeas@logilab.fr>
# Date 1401209244 -7200
# Tue May 27 18:47:24 2014 +0200
# Node ID 824e1c797eb28493465944aa541657dffa8b90d8
# Parent f7e574156e801ece15b3adc2e4307faa157a76d9
[rset] kill the rset._rqlst cache

Right now it "works" for the standard, internal uses.

However when we will fold ClientConnection and Connection, it will
hurt, because suddenly we get more cache hits, and the following
situation would become commonplace:

* there is an un-annotated _rqlst given by the querier

* some view (e.g. facets) requests the .syntax_tree, which takes a
copy of _rqlst

* the view actually expects the rql syntax tree to be annotated, but
it was not, hence we crash.


Related to #3837233.

diff --git a/repoapi.py b/repoapi.py
@@ -210,14 +210,10 @@
1          # Connection yet so we use this trick to unsure the session have the
2          # proper cnx loaded. This can be simplified one we have Standalone
3          # Connection object
4          rset = self._cnx.execute(*args, **kwargs)
5          rset.req = self
6 -        # XXX keep the same behavior as the old dbapi
7 -        # otherwise multiple tests break.
8 -        # The little internet kitten is very sad about this situation.
9 -        rset._rqlst = None
10          return rset
11 
12      @_open_only
13      def commit(self, *args, **kwargs):
14          try:
diff --git a/rset.py b/rset.py
@@ -17,10 +17,12 @@
15  # with CubicWeb.  If not, see <http://www.gnu.org/licenses/>.
16  """The `ResultSet` class which is returned as result of an rql query"""
17 
18  __docformat__ = "restructuredtext en"
19 
20 +from warnings import warn
21 +
22  from logilab.common.decorators import cached, clear_cache, copy_cache
23 
24  from rql import nodes, stmts
25 
26  from cubicweb import NotAnEntity, NoResultError, MultipleResultsError
@@ -44,10 +46,13 @@
27      :type rql: str or unicode
28      :param rql: the original RQL query string
29      """
30 
31      def __init__(self, results, rql, args=None, description=None, rqlst=None):
32 +        if rqlst is not None:
33 +            warn('[3.20] rqlst parameter is deprecated',
34 +                 DeprecationWarning, stacklevel=2)
35          self.rows = results
36          self.rowcount = results and len(results) or 0
37          # original query and arguments
38          self.rql = rql
39          self.args = args
@@ -55,14 +60,10 @@
40          # maybe discarded if specified when the query has been executed
41          if description is None:
42              self.description = []
43          else:
44              self.description = description
45 -        # parsed syntax tree
46 -        if rqlst is not None:
47 -            rqlst.schema = None # reset schema in case of pyro transfert
48 -        self._rqlst = rqlst
49          # set to (limit, offset) when a result set is limited using the
50          # .limit method
51          self.limited = None
52          # set by the cursor which returned this resultset
53          self.req = None
@@ -548,22 +549,15 @@
54                      rel_cols[(attr, role)] = i
55          return eid_col, attr_cols, rel_cols
56 
57      @cached
58      def syntax_tree(self):
59 -        """return the syntax tree (:class:`rql.stmts.Union`) for the originating
60 -        query. You can expect it to have solutions computed but it won't be
61 -        annotated (you usually don't need that for simple introspection).
62 +        """return the syntax tree (:class:`rql.stmts.Union`) for the
63 +        originating query. You can expect it to have solutions
64 +        computed and it will be properly annotated.
65          """
66 -        if self._rqlst:
67 -            rqlst = self._rqlst.copy()
68 -            # to avoid transport overhead when pyro is used, the schema has been
69 -            # unset from the syntax tree
70 -            rqlst.schema = self.req.vreg.schema
71 -        else:
72 -            rqlst = self.req.vreg.parse(self.req, self.rql, self.args)
73 -        return rqlst
74 +        return self.req.vreg.parse(self.req, self.rql, self.args)
75 
76      @cached
77      def column_types(self, col):
78          """return the list of different types in the column with the given col
79 
diff --git a/server/querier.py b/server/querier.py
@@ -1,6 +1,6 @@
80 -# copyright 2003-2013 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
81 +# copyright 2003-2014 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
82  # contact http://www.logilab.fr/ -- mailto:contact@logilab.fr
83  #
84  # This file is part of CubicWeb.
85  #
86  # CubicWeb is free software: you can redistribute it and/or modify it under the
@@ -575,11 +575,10 @@
87                  self._rql_ck_cache[rql] = eidkeys
88                  if eidkeys:
89                      cachekey = self._repo.querier_cache_key(cnx, rql, args,
90                                                              eidkeys)
91              self._rql_cache[cachekey] = rqlst
92 -        orig_rqlst = rqlst
93          if rqlst.TYPE != 'select':
94              if cnx.read_security:
95                  check_no_password_selected(rqlst)
96              # write query, ensure connection's mode is 'write' so connections
97              # won't be released until commit/rollback
@@ -644,11 +643,11 @@
98                  todetermine = zip(xrange(len(plan.selected)), repeat(False))
99                  descr = _build_descr(cnx, results, basedescr, todetermine)
100              # FIXME: get number of affected entities / relations on non
101              # selection queries ?
102          # return a result set object
103 -        return ResultSet(results, rql, args, descr, orig_rqlst)
104 +        return ResultSet(results, rql, args, descr)
105 
106      # these are overridden by set_log_methods below
107      # only defining here to prevent pylint from complaining
108      info = warning = error = critical = exception = debug = lambda msg,*a,**kw: None
109 
diff --git a/test/unittest_rset.py b/test/unittest_rset.py
@@ -98,11 +98,11 @@
110              params2 = dict(pair.split('=') for pair in info1[3].split('&'))
111              self.assertDictEqual(params1, params2)
112 
113      def test_pickle(self):
114          del self.rset.req
115 -        self.assertEqual(len(pickle.dumps(self.rset)), 392)
116 +        self.assertEqual(len(pickle.dumps(self.rset)), 376)
117 
118      def test_build_url(self):
119          with self.admin_access.web_request() as req:
120              baseurl = req.base_url()
121              self.compare_urls(req.build_url('view', vid='foo', rql='yo'),