# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@logilab.fr>
# Date 1331132995 -3600
# Node ID f23782a2cdeea3b1a36da51c8b1b6978199c0e0f
# Parent  302dcb3c68580c21e6c0526db48b2d5a75d2ca43
rqlrewrite: remove element in rewritten when we remove them from the select (closes #2236985)


update _cleanup_inserted to avoid leaving rewritten variable behind when
removing a snipset.

insert_varmap_snippets was impacted too for unclear reason

---

Before


A KeyError was raised when:

* multiple snipset is to be inserted on a statement
* some *supported* snipset adds ambiguity (increase the number of solution)
* some *unsupported* snipset adds new variable
* The new variable require rewritting

::

  File "/home/pyves/src/fcw/cubicweb/rqlrewrite.py", line 185, in rewrite
    newsolutions = self.remove_ambiguities(snippets, newsolutions)
  File "/home/pyves/src/fcw/cubicweb/rqlrewrite.py", line 436, in
remove_ambiguities
    variantes = self.build_variantes(newsolutions)
  File "/home/pyves/src/fcw/cubicweb/devtools/repotest.py", line 340, in
_build_variantes
    variantes = _orig_build_variantes(self, newsolutions)
  File "/home/pyves/src/fcw/cubicweb/rqlrewrite.py", line 470, in
build_variantes
    variante.append( (key, sol[newvar]) )
  KeyError: u'D'





This happen because the mechanism removing unsupported snipset does not remove
entry in ``self.rewritten`` when it removes entry from
``self.select.defined_vars``.

Iteration on ``self.rewritten`` then crash because values of ``rewritten`` are
expected to
be found in solution.

diff -r 302dcb3c6858 -r f23782a2cdee rqlrewrite.py
--- a/rqlrewrite.py	Fri Mar 02 15:15:20 2012 +0100
+++ b/rqlrewrite.py	Wed Mar 07 16:09:55 2012 +0100
@@ -245,7 +245,7 @@
                 # called to reintroduce snippet due to ambiguity creation,
                 # so skip snippets which are not introducing this ambiguity
                 exists = varexistsmap[varmap]
-                if self.exists_snippet[rqlexpr] is exists:
+                if self.exists_snippet.get(rqlexpr) is exists:
                     self.insert_snippet(varmap, rqlexpr.snippet_rqlst, exists)
         if varexistsmap is None and not inserted:
             # no rql expression found matching rql solutions. User has no access right
@@ -481,11 +481,17 @@
 
     def _cleanup_inserted(self, node):
         # cleanup inserted variable references
+        removed = set()
         for vref in node.iget_nodes(n.VariableRef):
             vref.unregister_reference()
             if not vref.variable.stinfo['references']:
                 # no more references, undefine the variable
                 del self.select.defined_vars[vref.name]
+                removed.add(vref.name)
+        for key, newvar in self.rewritten.items(): # I mean items we alter it
+            if newvar in removed:
+                del self.rewritten[key]
+
 
     def _may_be_shared_with(self, sniprel, target):
         """if the snippet relation can be skipped to use a relation from the
diff -r 302dcb3c6858 -r f23782a2cdee test/unittest_rqlrewrite.py
--- a/test/unittest_rqlrewrite.py	Fri Mar 02 15:15:20 2012 +0100
+++ b/test/unittest_rqlrewrite.py	Wed Mar 07 16:09:55 2012 +0100
@@ -443,6 +443,21 @@
                          'OR (EXISTS(C owned_by D, D login "momo"))) '
                          'OR (EXISTS(C owned_by A, A login "hip")), C is Card)')
 
+    def test_multiple_erql_one_bad(self):
+        #: reproduce bug #2236985
+        #: (rqlrewrite fails to remove rewritten entry for unsupported constraint and then crash)
+        #:
+        #: This check a very rare code path triggered by the four condition below
+
+        # 1. c_ok introduce an ambiguity
+        c_ok = ERQLExpression('X concerne R')
+        # 2. c_bad is just plain wrong and won't be kept
+        # 3. but it declare a new variable
+        # 4. this variable require a rewrite
+        c_bad = ERQLExpression('X documented_by R, A in_state R')
+
+        rqlst = parse('Any A, R WHERE A ref R, S is Affaire')
+        rewrite(rqlst, {('A', 'X'): (c_ok, c_bad)}, {})
 
 if __name__ == '__main__':
     unittest_main()