[entity] ensure the .related(entities=False) parameter is honored all the way down (closes #2755994)
authorAurelien Campeas <aurelien.campeas@logilab.fr>
Tue, 19 Mar 2013 15:18:22 +0100
changeset 8735 5567a5117aeb
parent 8734 3530b7494195
child 8736 b84a233cb8b0
[entity] ensure the .related(entities=False) parameter is honored all the way down (closes #2755994) As of today, such a call will always fill the relation cache by calling .entities() on every single related rset entry. As a consequence, the `limit` parameter handling also had to be fixed. It was bogus in the following ways: * not used in the related_rql, hence potentially huge database requests, but also actually * foolishly used in the .entities()-calling cache routine we now bypass (this changeset ticket's main topic) Now: * we set a limit on the rql expression, and * forbid caching if given a non-None limit (as we don't want to make the cache handling code more complicated than it is already) With this, entity.unrelated gets a better limit implementation (so the code in related/unrelated is nice and symmetric) Risk: * _cw_relation_cache disappears completely, which is good, but this is Python, so you never know ...
entity.py
test/unittest_entity.py
test/unittest_rset.py
--- a/entity.py	Tue Mar 19 15:17:34 2013 +0100
+++ b/entity.py	Tue Mar 19 15:18:22 2013 +0100
@@ -997,30 +997,38 @@
           :exc:`Unauthorized`, else (the default), the exception is propagated
         """
         rtype = str(rtype)
-        try:
-            return self._cw_relation_cache(rtype, role, entities, limit)
-        except KeyError:
-            pass
+        if limit is None:
+            # we cannot do much wrt cache on limited queries
+            cache_key = '%s_%s' % (rtype, role)
+            if cache_key in self._cw_related_cache:
+                return self._cw_related_cache[cache_key][entities]
         if not self.has_eid():
             if entities:
                 return []
             return self._cw.empty_rset()
-        rql = self.cw_related_rql(rtype, role)
+        rql = self.cw_related_rql(rtype, role, limit=limit)
         try:
             rset = self._cw.execute(rql, {'x': self.eid})
         except Unauthorized:
             if not safe:
                 raise
             rset = self._cw.empty_rset()
-        self.cw_set_relation_cache(rtype, role, rset)
-        return self.related(rtype, role, limit, entities)
+        if entities:
+            if limit is None:
+                self.cw_set_relation_cache(rtype, role, rset)
+                return self.related(rtype, role, limit, entities)
+            return list(rset.entities())
+        else:
+            return rset
 
-    def cw_related_rql(self, rtype, role='subject', targettypes=None):
+    def cw_related_rql(self, rtype, role='subject', targettypes=None, limit=None):
         vreg = self._cw.vreg
         rschema = vreg.schema[rtype]
         select = Select()
         mainvar, evar = select.get_variable('X'), select.get_variable('E')
         select.add_selected(mainvar)
+        if limit is not None:
+            select.set_limit(limit)
         select.add_eid_restriction(evar, 'x', 'Substitute')
         if role == 'subject':
             rel = make_relation(evar, rtype, (mainvar,), VariableRef)
@@ -1075,7 +1083,7 @@
     # generic vocabulary methods ##############################################
 
     def cw_unrelated_rql(self, rtype, targettype, role, ordermethod=None,
-                         vocabconstraints=True, lt_infos={}):
+                         vocabconstraints=True, lt_infos={}, limit=None):
         """build a rql to fetch `targettype` entities unrelated to this entity
         using (rtype, role) relation.
 
@@ -1104,6 +1112,8 @@
             searchedvar = subjvar = select.get_variable('S')
             evar = objvar = select.get_variable('O')
         select.add_selected(searchedvar)
+        if limit is not None:
+            select.set_limit(limit)
         # initialize some variables according to `self` existence
         if rdef.role_cardinality(neg_role(role)) in '?1':
             # if cardinality in '1?', we want a target entity which isn't
@@ -1197,14 +1207,10 @@
         by a given relation, with self as subject or object
         """
         try:
-            rql, args = self.cw_unrelated_rql(rtype, targettype, role,
-                                              ordermethod, lt_infos=lt_infos)
+            rql, args = self.cw_unrelated_rql(rtype, targettype, role, limit=limit,
+                                              ordermethod=ordermethod, lt_infos=lt_infos)
         except Unauthorized:
             return self._cw.empty_rset()
-        # XXX should be set in unrelated rql when manipulating the AST
-        if limit is not None:
-            before, after = rql.split(' WHERE ', 1)
-            rql = '%s LIMIT %s WHERE %s' % (before, limit, after)
         return self._cw.execute(rql, args)
 
     # relations cache handling #################################################
@@ -1215,18 +1221,6 @@
         """
         return self._cw_related_cache.get('%s_%s' % (rtype, role))
 
-    def _cw_relation_cache(self, rtype, role, entities=True, limit=None):
-        """return values for the given relation if it's cached on the instance,
-        else raise `KeyError`
-        """
-        res = self._cw_related_cache['%s_%s' % (rtype, role)][entities]
-        if limit is not None and limit < len(res):
-            if entities:
-                res = res[:limit]
-            else:
-                res = res.limit(limit)
-        return res
-
     def cw_set_relation_cache(self, rtype, role, rset):
         """set cached values for the given relation"""
         if rset:
--- a/test/unittest_entity.py	Tue Mar 19 15:17:34 2013 +0100
+++ b/test/unittest_entity.py	Tue Mar 19 15:18:22 2013 +0100
@@ -141,6 +141,9 @@
         self.execute('SET X tags Y WHERE X is Tag, Y is Personne')
         self.assertEqual(len(p.related('tags', 'object', limit=2)), 2)
         self.assertEqual(len(p.related('tags', 'object')), 4)
+        p.cw_clear_all_caches()
+        self.assertEqual(len(p.related('tags', 'object', entities=True, limit=2)), 2)
+        self.assertEqual(len(p.related('tags', 'object', entities=True)), 4)
 
     def test_cw_instantiate_relation(self):
         req = self.request()
--- a/test/unittest_rset.py	Tue Mar 19 15:17:34 2013 +0100
+++ b/test/unittest_rset.py	Tue Mar 19 15:18:22 2013 +0100
@@ -345,9 +345,9 @@
         e = rset.get_entity(0, 0)
         # if any of the assertion below fails with a KeyError, the relation is not cached
         # related entities should be an empty list
-        self.assertEqual(e._cw_relation_cache('primary_email', 'subject', True), ())
+        self.assertEqual(e._cw_related_cache['primary_email_subject'][True], ())
         # related rset should be an empty rset
-        cached = e._cw_relation_cache('primary_email', 'subject', False)
+        cached = e._cw_related_cache['primary_email_subject'][False]
         self.assertIsInstance(cached, ResultSet)
         self.assertEqual(cached.rowcount, 0)