Tests and bug-fix for issue2551119

.. and some other failing tests I came up with when trying to reproduce
the problem of the issue.

Author: schlatterbeck

roundup/backends/rdbms_common.py

@@ -2585,21 +2585,23 @@ def _filter_sql (self, search_matches, filterspec, srt=[], grp=[], retr=0,
                     tn = propclass.table_name
                     nid = propclass.nodeid_name
                     lid = propclass.linkid_name
+                    frum.append(tn)
+                    if p.children or p.need_child_retired:
+                        frum.append('_%s as _%s' % (cn, ln))
+                        where.append('%s.%s=_%s.id'%(tn, lid, ln))
+                        if p.need_child_retired:
+                            where.append('_%s.__retired__=0'%(ln))
+                    # Note: need the where-clause if p has
+                    # children that compute additional restrictions
+                    if  (not p.has_values
+                         or (not isinstance(v, type([])) and v != '-1')
+                         or p.children):
+                        where.append('_%s.id=%s.%s'%(pln, tn, nid))
                     if v in ('-1', ['-1'], []):
                         # only match rows that have count(linkid)=0 in the
                         # corresponding multilink table)
                         where.append(self._subselect(p))
                     else:
-                        frum.append(tn)
-
-                        if p.children or p.need_child_retired:
-                            frum.append('_%s as _%s' % (cn, ln))
-                            where.append('%s.%s=_%s.id'%(tn, lid, ln))
-                            if p.need_child_retired:
-                                where.append('_%s.__retired__=0'%(ln))
-
-                        if not p.has_values or not isinstance(v, type([])):
-                            where.append('_%s.id=%s.%s'%(pln, tn, nid))
                         if p.has_values:
                             if isinstance(v, type([])):
                                 # The where-clause above is conditionally

roundup/hyperdb.py

@@ -465,16 +465,17 @@ class Proptree(object):
 
     The Proptree is also used for transitively searching attributes for
     backends that do not support transitive search (e.g. anydbm). The
-    _val attribute with set_val is used for this.
+    val attribute with set_val is used for this.
     """
 
     def __init__(self, db, cls, name, props, parent=None, retr=False):
         self.db = db
         self.name = name
         self.props = props
         self.parent = parent
-        self._val = None
+        self.val = None
         self.has_values = False
+        self.has_result = False
         self.cls = cls
         self.classname = None
         self.uniqname = None
@@ -634,6 +635,11 @@ def search(self, search_matches=None, sort=True, retired=False):
                             if expr.evaluate(by_id[k]):
                                 items.add(k)
 
+                    # The subquery has found nothing. So it doesn't make
+                    # sense to search further.
+                    if not items:
+                        self.set_val([], force=True)
+                        return self.val
                     filterspec[p.name] = list(sorted(items, key=int))
                 elif isinstance(p.val, type([])):
                     exact = []
@@ -648,13 +654,19 @@ def search(self, search_matches=None, sort=True, retired=False):
                     if subst:
                         filterspec[p.name] = subst
                     elif not exact: # don't set if we have exact criteria
-                        filterspec[p.name] =[ '-1' ] # no match was found
+                        if p.has_result:
+                            # A subquery already has found nothing. So
+                            # it doesn't make sense to search further.
+                            self.set_val([], force=True)
+                            return self.val
+                        else:
+                            filterspec[p.name] = ['-1'] # no match was found
                 else:
                     assert not isinstance(p.val, Exact_Match)
                     filterspec[p.name] = p.val
-        self.val = self.cls._filter(search_matches, filterspec, sort and self,
-                                    retired=retired,
-                                    exact_match_spec=exact_match_spec)
+        self.set_val(self.cls._filter(search_matches, filterspec, sort and self,
+                                      retired=retired,
+                                      exact_match_spec=exact_match_spec))
         return self.val
 
     def sort(self, ids=None):
@@ -742,8 +754,9 @@ def _searchsort(self, ids=None, update=True, dosort=True):
                 pt.sort_result = None
         return ids
 
-    def _set_val(self, val):
-        """ Check if self._val is already defined. If yes, we compute the
+    def set_val(self, val, force=False, result=True):
+        """ Check if self.val is already defined (it is not None and
+            has_values is True). If yes, we compute the
             intersection of the old and the new value(s)
             Note: If self is a Leaf node we need to compute a
             union: Normally we intersect (logical and) different
@@ -753,25 +766,56 @@ def _set_val(self, val):
             generated search will ensure a logical and of all tests for
             equality/substring search.
         """
+        if force:
+            assert val == []
+            assert result
+            self.val = val
+            self.has_values = True
+            self.has_result = True
+            return
         if self.has_values:
-            v = self._val
-            if not isinstance(self._val, type([])):
-                v = [self._val]
+            v = self.val
+            if not isinstance(self.val, type([])):
+                v = [self.val]
             vals = set(v)
             if not isinstance(val, type([])):
                 val = [val]
+        if self.has_result:
+            assert result
             # if cls is None we're a leaf
             if self.cls:
                 vals.intersection_update(val)
             else:
                 vals.update(val)
-            self._val = [v for v in vals]
+            self.val = list(vals)
         else:
-            self._val = val
+            # If a subquery found nothing we don't care if there is an
+            # expression
+            if not self.has_values or not val:
+                self.val = val
+                if result:
+                    self.has_result = True
+            else:
+                if not result:
+                    assert not self.cls
+                    vals.update(val)
+                    self.val = list(vals)
+                else:
+                    assert self.cls
+                    is_expression = min(int(i) for i in self.val) < -1
+                    if is_expression:
+                        # Tag on the ORed values with an AND
+                        l = val
+                        for i in range(len(val)-1):
+                            l.append('-4')
+                        l.append('-3')
+                        self.val = self.val + l
+                    else:
+                        vals.intersection_update(val)
+                        self.val = list(vals)
+                    self.has_result = True
         self.has_values = True
 
-    val = property(lambda self: self._val, _set_val)
-
     def _sort(self, val):
         """Finally sort by the given sortattr.sort_result. Note that we
         do not sort by attrs having attr_sort_done set. The caller is
@@ -1542,11 +1586,11 @@ def _proptree(self, filterspec, exact_match_spec={}, sortattr=[],
                         vv = []
                         for x in v:
                             vv.append(Exact_Match(x))
-                        p.val = vv
+                        p.set_val(vv, result=False)
                     else:
-                        p.val = [Exact_Match(v)]
+                        p.set_val([Exact_Match(v)], result=False)
                 else:
-                    p.val = v
+                    p.set_val(v, result=False)
         multilinks = {}
         for s in sortattr:
             keys = s[1].split('.')

test/db_test_base.py

@@ -93,7 +93,7 @@ def setupTracker(dirname, backend="anydbm", optimize=False):
 def setupSchema(db, create, module):
     mls = module.Class(db, "mls", name=String())
     mls.setkey("name")
-    keyword = module.Class(db, "keyword", name=String())
+    keyword = module.Class(db, "keyword", name=String(), order=Number())
     keyword.setkey("name")
     status = module.Class(db, "status", name=String(), mls=Multilink("mls"))
     status.setkey("name")
@@ -118,7 +118,7 @@ def setupSchema(db, create, module):
         files=Multilink("file"), assignedto=Link('user', quiet=True,
         rev_multilink='issues'), priority=Link('priority'),
         spam=Multilink('msg'), feedback=Link('msg'),
-        keywords=Multilink('keyword'))
+        keywords=Multilink('keyword'), keywords2=Multilink('keyword'))
     stuff = module.Class(db, "stuff", stuff=String())
     session = module.Class(db, 'session', title=String())
     msg = module.FileClass(db, "msg", date=Date(),
@@ -1542,8 +1542,8 @@ def testIndexingPropertiesOnImport(self):
         # import an issue
         title = 'Bzzt'
         nodeid = self.db.issue.import_list(['title', 'messages', 'files',
-            'spam', 'nosy', 'superseder', 'keywords'], [repr(title), '[]',
-            '[]', '[]', '[]', '[]', '[]'])
+            'spam', 'nosy', 'superseder', 'keywords', 'keywords2'],
+            [repr(title), '[]', '[]', '[]', '[]', '[]', '[]', '[]'])
         self.db.commit()
 
         # Content of title attribute is indexed
@@ -2060,6 +2060,37 @@ def testFilteringMultilinkExpression(self):
             ae(filt(None, {kw: ['4', '-1', '-3', '3', '-1', '-4', '-4']}),
                ['1', '2', '3'])
 
+    def testFilteringTwoMultilinksExpression(self):
+        ae, iiter = self.filteringSetup()
+        kw1 = self.db.keyword.create(name='Key1', order=10)
+        kw2 = self.db.keyword.create(name='Key2', order=20)
+        kw3 = self.db.keyword.create(name='Key3', order=30)
+        kw4 = self.db.keyword.create(name='Key4', order=40)
+        self.db.issue.set('1', keywords=[kw1, kw2])
+        self.db.issue.set('2', keywords=[kw1, kw3])
+        self.db.issue.set('3', keywords=[kw2, kw3, kw4])
+        self.db.issue.set('4', keywords=[])
+        self.db.issue.set('1', keywords2=[kw3, kw4])
+        self.db.issue.set('2', keywords2=[kw2, kw3])
+        self.db.issue.set('3', keywords2=[kw1, kw3, kw4])
+        self.db.issue.set('4', keywords2=[])
+        self.db.commit()
+        kw = 'keywords'
+        kw2 = 'keywords2'
+        for filt in iiter():
+            # kw: '1' and '3' kw2: '2' and '3'
+            ae(filt(None, {kw: ['1', '3', '-3'], kw2: ['2', '3', '-3']}), ['2'])
+            # kw: empty kw2: empty
+            ae(filt(None, {kw: ['-1'], kw2: ['-1']}), ['4'])
+            # kw: empty kw2: empty
+            ae(filt(None, {kw: [], kw2: []}), ['4'])
+            # look for both keyword name and order
+            ae(filt(None, {'keywords.name': 'y4', 'keywords.order': 40}), ['3'])
+            # look for both keyword and order non-matching
+            ae(filt(None, {kw: '3', 'keywords.order': 40}), [])
+            # look for both keyword and order non-matching with kw and kw2
+            ae(filt(None, {kw: '3', 'keywords2.order': 40}), ['3'])
+
     def testFilteringRevMultilink(self):
         ae, iiter = self.filteringSetupTransitiveSearch('user')
         ni = 'nosy_issues'
@@ -2102,6 +2133,35 @@ def ls(x):
         self.assertEqual(ls(self.db.user.get('4', ni)), ['1'])
         self.assertEqual(ls(self.db.user.get('5', ni)), ['7'])
 
+    def testFilteringRevMultilinkQ2(self):
+        ae, iiter = self.filteringSetupTransitiveSearch('user')
+        ni = 'nosy_issues'
+        nis = 'nosy_issues.status'
+        self.db.issue.set('6', nosy=['3', '4', '5'])
+        self.db.issue.set('7', nosy=['5'])
+        self.db.commit()
+        # After this setup we have the following values for nosy:
+        # The issues '1', '3', '5', '7' have status '2'
+        # issue   nosy
+        # 1:      4
+        # 2:      5
+        # 3:
+        # 4:
+        # 5:
+        # 6:      3, 4, 5
+        # 7:      5
+        # 8:
+        for filt in iiter():
+            # status of issue is '2'
+            ae(filt(None, {nis: ['2']}),
+               ['4', '5'])
+            # Issue non-empty and status of issue is '2'
+            ae(filt(None, {nis: ['2'], ni:['-1', '-2']}),
+               ['4', '5'])
+            # empty and status '2'
+            # This is the test-case for issue2551119
+            ae(filt(None, {nis: ['2'], ni:['-1']}), [])
+
     def testFilteringRevMultilinkExpression(self):
         ae, iiter = self.filteringSetupTransitiveSearch('user')
         ni = 'nosy_issues'
@@ -2911,6 +2971,7 @@ def stdoutwrite(s):
                       'feedback: <roundup.hyperdb.Link to "msg">\n',
                       'files: <roundup.hyperdb.Multilink to "file">\n',
                       'foo: <roundup.hyperdb.Interval>\n',
+                      'keywords2: <roundup.hyperdb.Multilink to "keyword">\n',
                       'keywords: <roundup.hyperdb.Multilink to "keyword">\n',
                       'messages: <roundup.hyperdb.Multilink to "msg">\n',
                       'nosy: <roundup.hyperdb.Multilink to "user">\n',
@@ -3018,8 +3079,8 @@ def testAddProperty(self):
         keys = sorted(props.keys())
         self.assertEqual(keys, ['activity', 'actor', 'assignedto', 'creation',
             'creator', 'deadline', 'feedback', 'files', 'fixer', 'foo',
-            'id', 'keywords', 'messages', 'nosy', 'priority', 'spam',
-            'status', 'superseder', 'title'])
+            'id', 'keywords', 'keywords2', 'messages', 'nosy', 'priority',
+            'spam', 'status', 'superseder', 'title'])
         self.assertEqual(self.db.issue.get('1', "fixer"), None)
 
     def testRemoveProperty(self):
@@ -3032,8 +3093,8 @@ def testRemoveProperty(self):
         keys = sorted(props.keys())
         self.assertEqual(keys, ['activity', 'actor', 'assignedto', 'creation',
             'creator', 'deadline', 'feedback', 'files', 'foo', 'id',
-            'keywords', 'messages', 'nosy', 'priority', 'spam', 'status',
-            'superseder'])
+            'keywords', 'keywords2', 'messages', 'nosy', 'priority', 'spam',
+            'status', 'superseder'])
         self.assertEqual(self.db.issue.list(), ['1'])
 
     def testAddRemoveProperty(self):
@@ -3047,8 +3108,8 @@ def testAddRemoveProperty(self):
         keys = sorted(props.keys())
         self.assertEqual(keys, ['activity', 'actor', 'assignedto', 'creation',
             'creator', 'deadline', 'feedback', 'files', 'fixer', 'foo', 'id',
-            'keywords', 'messages', 'nosy', 'priority', 'spam', 'status',
-            'superseder'])
+            'keywords', 'keywords2', 'messages', 'nosy', 'priority', 'spam',
+            'status', 'superseder'])
         self.assertEqual(self.db.issue.list(), ['1'])
 
     def testNosyMail(self) :