# HG changeset patch # User Sylvain Thénault # Date 1492761424 -7200 # Node ID 02b8325720d6ff3ff34100eba88a4c1a23c39bad # Parent d13fc09301bd0892eb45a7109b87aa0f8cca4d1e [rqlrewrite] Fix rewrite on ambiguities introduced by NOT relation or "is IN" type restriction When some inserted RQL snippet generate more solutions than the original RQL, the rewriter attempt to duplicate the snippet for each newly introduced solution. There are though some cases where we do not want this behaviour in case of ambiguities introduced by: * NOT(X relation Y) expression, since it won't be equivalent to NOT(X relation Y1, Y1 is Type1) OR NOT(X relation Y2, Y2 is Type2) ; * EXISTS(X relation Y, Y is IN (Type1, Type2) expression, since it's not actually necessary to split an explicitly introduced ambiguity (and it crash if we attempt to do so, so...). In test, we've to modify the `rewrite()` function because in the newly introduced test we need the same constraint to be applied to two variables in the original query, and this was not supported before. Notice the generated RQL in test is still *NOT CORRECT* "(EXISTS(NOT EXISTS() OR EXISTS(...))", or at least isn't optimal. This will be fixed in a forthcoming changeset. Related to #17074119 diff -r d13fc09301bd -r 02b8325720d6 cubicweb/rqlrewrite.py --- a/cubicweb/rqlrewrite.py Wed Apr 19 09:05:10 2017 +0200 +++ b/cubicweb/rqlrewrite.py Fri Apr 21 09:57:04 2017 +0200 @@ -649,6 +649,10 @@ # the snippet has introduced some ambiguities, we have to resolve them # "manually" variantes = self.build_variantes(newsolutions) + # if all ambiguities have been generated by variables within a "NOT + # EXISTS()#" or with type explicitly specified, we've nothing to change + if not variantes: + return newsolutions # insert "is" where necessary varexistsmap = {} self.removing_ambiguity = True @@ -680,21 +684,32 @@ variantes = set() for sol in newsolutions: variante = [] - for key, newvar in self.rewritten.items(): - variante.append((key, sol[newvar])) - variantes.add(tuple(variante)) - # rebuild variantes as dict - variantes = [dict(v) for v in variantes] - # remove variable which have always the same type - for key in self.rewritten: - it = iter(variantes) - etype = next(it)[key] - for variante in it: - if variante[key] != etype: - break - else: - for variante in variantes: - del variante[key] + for key, var_name in self.rewritten.items(): + var = self.select.defined_vars[var_name] + # skip variable which are only in a NOT EXISTS + if len(var.stinfo['relations']) == 1 and isinstance(var.scope.parent, n.Not): + continue + # skip variable whose type is already explicitly specified + if var.stinfo['typerel']: + continue + variante.append((key, sol[var_name])) + if variante: + variantes.add(tuple(variante)) + + if variantes: + # rebuild variantes as dict + variantes = [dict(v) for v in variantes] + # remove variable which have always the same type + for key in self.rewritten: + it = iter(variantes) + etype = next(it)[key] + for variante in it: + if variante[key] != etype: + break + else: + for variante in variantes: + del variante[key] + return variantes def _cleanup_inserted(self, node): diff -r d13fc09301bd -r 02b8325720d6 cubicweb/test/unittest_rqlrewrite.py --- a/cubicweb/test/unittest_rqlrewrite.py Wed Apr 19 09:05:10 2017 +0200 +++ b/cubicweb/test/unittest_rqlrewrite.py Fri Apr 21 09:57:04 2017 +0200 @@ -67,14 +67,28 @@ def rewrite(rqlst, snippets_map, kwargs, existingvars=None): rewriter = _prepare_rewriter(rqlrewrite.RQLRewriter, kwargs) + # turn {(V1, V2): constraints} into [(varmap, constraints)] snippets = [] + snippet_varmap = {} for v, exprs in sorted(snippets_map.items()): - rqlexprs = [isinstance(snippet, string_types) - and mock_object(snippet_rqlst=parse(u'Any X WHERE '+snippet).children[0], - expression=u'Any X WHERE '+snippet) - or snippet - for snippet in exprs] - snippets.append((dict([v]), rqlexprs)) + rqlexprs = [] + varmap = dict([v]) + for snippet in exprs: + # when the same snippet is impacting several variables, group them + # unless there is some conflicts on the snippet's variable name (we + # only want that for constraint on relations using both S and O) + if snippet in snippet_varmap and not ( + set(varmap.values()) & set(snippet_varmap[snippet].values())): + snippet_varmap[snippet].update(varmap) + continue + snippet_varmap[snippet] = varmap + if isinstance(snippet, string_types): + snippet = mock_object(snippet_rqlst=parse(u'Any X WHERE ' + snippet).children[0], + expression=u'Any X WHERE ' + snippet) + rqlexprs.append(snippet) + if rqlexprs: + snippets.append((varmap, rqlexprs)) + rqlhelper.compute_solutions(rqlst.children[0], {'eid': eid_func_map}, kwargs=kwargs) rewriter.rewrite(rqlst.children[0], snippets, kwargs, existingvars) check_vrefs(rqlst.children[0]) @@ -503,6 +517,36 @@ 'Any O WHERE S use_email O, S is CWUser, O is EmailAddress, ' 'EXISTS(NOT S in_group A, A name "guests", A is CWGroup)') + def test_ambiguous_constraint_not_exists(self): + state_constraint = ( + 'NOT EXISTS(A require_permission S) ' + 'OR EXISTS(B require_permission S, B is Card, O name "state1")' + 'OR EXISTS(C require_permission S, C is Note, O name "state2")' + ) + rqlst = parse(u'Any P WHERE NOT P require_state S') + rewrite(rqlst, {('P', 'S'): (state_constraint,), ('S', 'O'): (state_constraint,)}, {}) + self.assertMultiLineEqual( + rqlst.as_string(), + u'Any P WHERE NOT P require_state S, ' + 'EXISTS(((NOT EXISTS(A require_permission P, A is IN(Card, Note)))' + ' OR (EXISTS(B require_permission P, B is Card, S name "state1")))' + ' OR (EXISTS(C require_permission P, C is Note, S name "state2"))), ' + 'P is CWPermission, S is State') + + def test_ambiguous_using_is_in_function(self): + state_constraint = ( + 'NOT EXISTS(A require_permission S) ' + 'OR EXISTS(B require_permission S, B is IN (Card, Note), O name "state1")' + ) + rqlst = parse(u'Any P WHERE NOT P require_state S') + rewrite(rqlst, {('P', 'S'): (state_constraint,), ('S', 'O'): (state_constraint,)}, {}) + self.assertMultiLineEqual( + rqlst.as_string(), + u'Any P WHERE NOT P require_state S, ' + 'EXISTS((NOT EXISTS(A require_permission P, A is IN(Card, Note))) ' + 'OR (EXISTS(B require_permission P, B is IN(Card, Note), S name "state1"))), ' + 'P is CWPermission, S is State') + from cubicweb.devtools.testlib import CubicWebTC class RewriteFullTC(CubicWebTC):