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.
--- 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
--- 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()