# HG changeset patch # User Sylvain Thénault # Date 1374736045 -7200 # Node ID 9448215c73c4fab7b5fdf85e1c49ce7c1f20f221 # Parent 0677e03077fbec3c5ed3a7bbd1f73c4df9a37fc5 [rqlrewrite] fix rqlrewrite unpredictability vs relation sharing. Closes #3036144 The relations index used to determine if relation may be shared only considered a single relation node per relation type. So when the same relation type occurs several time, dict order give unpredictable result. We shall properly consider all relations instead. Tentative test case included (but bug reproduction is by definition unpredictable...) diff -r 0677e03077fb -r 9448215c73c4 rqlrewrite.py --- a/rqlrewrite.py Wed Jul 24 16:55:24 2013 +0200 +++ b/rqlrewrite.py Thu Jul 25 09:07:25 2013 +0200 @@ -173,6 +173,29 @@ return True return False +def _compatible_relation(relations, stmt, sniprel): + """Search among given rql relation nodes if there is one 'compatible' with the + snippet relation, and return it if any, else None. + + A relation is compatible if it: + * belongs to the currently processed statement, + * isn't negged (i.e. direct parent is a NOT node) + * isn't optional (outer join) or similarly as the snippet relation + """ + for rel in relations: + # don't share if relation's scope is not the current statement + if rel.scope is not stmt: + continue + # don't share neged relation + if rel.neged(strict=True): + continue + # don't share optional relation, unless the snippet relation is + # similarly optional + if rel.optional and rel.optional != sniprel.optional: + continue + return rel + return None + def iter_relations(stinfo): # this is a function so that test may return relation in a predictable order @@ -384,9 +407,14 @@ subselect.solutions, self.kwargs) return if varexistsmap is None: - vi['rhs_rels'] = dict( (r.r_type, r) for r in sti['rhsrelations']) - vi['lhs_rels'] = dict( (r.r_type, r) for r in sti['relations'] - if not r in sti['rhsrelations']) + # build an index for quick access to relations + vi['rhs_rels'] = {} + for rel in sti['rhsrelations']: + vi['rhs_rels'].setdefault(rel.r_type, []).append(rel) + vi['lhs_rels'] = {} + for rel in sti['relations']: + if not rel in sti['rhsrelations']: + vi['lhs_rels'].setdefault(rel.r_type, []).append(rel) else: vi['rhs_rels'] = vi['lhs_rels'] = {} previous = None @@ -657,36 +685,33 @@ """if the snippet relation can be skipped to use a relation from the original query, return that relation node """ + if sniprel.neged(strict=True): + return None # no way rschema = self.schema.rschema(sniprel.r_type) stmt = self.current_statement() for vi in self.varinfos: try: if target == 'object': - orel = vi['lhs_rels'][sniprel.r_type] + orels = vi['lhs_rels'][sniprel.r_type] cardindex = 0 ttypes_func = rschema.objects rdef = rschema.rdef else: # target == 'subject': - orel = vi['rhs_rels'][sniprel.r_type] + orels = vi['rhs_rels'][sniprel.r_type] cardindex = 1 ttypes_func = rschema.subjects rdef = lambda x, y: rschema.rdef(y, x) except KeyError: # may be raised by vi['xhs_rels'][sniprel.r_type] continue - # don't share if relation's scope is not the current statement - if orel.scope is not stmt: - continue - # can't share neged relation or relations with different outer join - if (orel.neged(strict=True) or sniprel.neged(strict=True) - or (orel.optional and orel.optional != sniprel.optional)): - continue - # if cardinality is in '?1', we can ignore the snippet relation and use - # variable from the original query + # if cardinality isn't in '?1', we can't ignore the snippet relation + # and use variable from the original query if _has_multiple_cardinality(vi['stinfo']['possibletypes'], rdef, ttypes_func, cardindex): continue - return orel + orel = _compatible_relation(orels, stmt, sniprel) + if orel is not None: + return orel return None def _use_orig_term(self, snippet_varname, term): diff -r 0677e03077fb -r 9448215c73c4 test/data/rewrite/schema.py --- a/test/data/rewrite/schema.py Wed Jul 24 16:55:24 2013 +0200 +++ b/test/data/rewrite/schema.py Thu Jul 25 09:07:25 2013 +0200 @@ -1,4 +1,4 @@ -# copyright 2003-2010 LOGILAB S.A. (Paris, FRANCE), all rights reserved. +# copyright 2003-2013 LOGILAB S.A. (Paris, FRANCE), all rights reserved. # contact http://www.logilab.fr/ -- mailto:contact@logilab.fr # # This file is part of CubicWeb. @@ -38,6 +38,7 @@ 'delete': ('managers', 'owners', ERQLExpression('U login L, X nom L')), 'add': ('managers', 'users',) } + nom = String() class Division(Societe): @@ -75,3 +76,9 @@ object = 'Affaire' inlined = True cardinality = '?*' + +class responsable(RelationDefinition): + subject = 'Societe' + object = 'CWUser' + inlined = True + cardinality = '1*' diff -r 0677e03077fb -r 9448215c73c4 test/unittest_rqlrewrite.py --- a/test/unittest_rqlrewrite.py Wed Jul 24 16:55:24 2013 +0200 +++ b/test/unittest_rqlrewrite.py Thu Jul 25 09:07:25 2013 +0200 @@ -515,8 +515,26 @@ (edef2, {'read': (ERQLExpression('X owned_by U'),)}), (edef3, {'read': (ERQLExpression('X owned_by U'),)})): union = self.process('Any A,AR,X,CD WHERE A concerne X?, A ref AR, X creation_date CD') - self.assertEqual(union.as_string(), 'Any A,AR,X,CD WHERE A concerne X?, A ref AR, A is Affaire WITH X,CD BEING (Any X,CD WHERE X creation_date CD, EXISTS(X owned_by %(A)s), X is IN(Division, Note, Societe))') + self.assertEqual('Any A,AR,X,CD WHERE A concerne X?, A ref AR, A is Affaire ' + 'WITH X,CD BEING (Any X,CD WHERE X creation_date CD, ' + 'EXISTS(X owned_by %(A)s), X is IN(Division, Note, Societe))', + union.as_string()) + + def test_xxxx(self): + edef1 = self.schema['Societe'] + edef2 = self.schema['Division'] + read_expr = ERQLExpression('X responsable E, U has_read_permission E') + with self.temporary_permissions((edef1, {'read': (read_expr,)}), + (edef2, {'read': (read_expr,)})): + union = self.process('Any X,AA,AC,AD ORDERBY AD DESC ' + 'WHERE X responsable E, X nom AA, ' + 'X responsable AC?, AC modification_date AD') + self.assertEqual('Any X,AA,AC,AD ORDERBY AD DESC ' + 'WHERE X responsable E, X nom AA, ' + 'X responsable AC?, AC modification_date AD, ' + 'AC is CWUser, E is CWUser, X is IN(Division, Societe)', + union.as_string()) if __name__ == '__main__': unittest_main()