fix #2235 for real now

doc/CHANGES.txt

@@ -8,8 +8,10 @@ Zope Changes
 
     Bugs fixed
 
-      - Collector #2235: ZCatalog.manage_catalogObject was triggering __len__
-        of objects that implement it, like containers.
+      - Collector #2235: A number of ZCatalog methods were doing boolean
+        evaluation of objects that implemented __len__ instead of checking
+        them against None. Replace a number of "if not obj" with
+        "if obj is not None".
 
       - Fix yet another resTructuredText glitch, and add tests (test
         backported from 2.9, which was not in fact vulnerable).

lib/python/Products/ZCatalog/ZCatalog.py

@@ -232,7 +232,7 @@ class ZCatalog(Folder, Persistent, Implicit):
 
             for url in urls:
                 obj = self.resolve_path(url)
-                if obj is not None:
+                if obj is None:
                     obj = self.resolve_url(url, REQUEST)
                 if obj is not None:
                     self.catalog_object(obj, url)
@@ -297,7 +297,7 @@ class ZCatalog(Folder, Persistent, Implicit):
             p = paths[i]
             obj = self.resolve_path(p)
             
-            if not obj and hasattr(self, 'REQUEST'):
+            if obj is None and hasattr(self, 'REQUEST'):
                 obj = self.resolve_url(p, self.REQUEST)
             if obj is not None:
                 try:
@@ -615,8 +615,8 @@ class ZCatalog(Folder, Persistent, Implicit):
     def getobject(self, rid, REQUEST=None):
         """Return a cataloged object given a 'data_record_id_'
         """
-        obj = self.aq_parent.unrestrictedTraverse(self.getpath(rid))
-        if not obj:
+        obj = self.aq_parent.unrestrictedTraverse(self.getpath(rid), None)
+        if obj is None:
             if REQUEST is None:
                 REQUEST=self.REQUEST
             obj = self.resolve_url(self.getpath(rid), REQUEST)

lib/python/Products/ZCatalog/tests/testCatalog.py

@@ -28,6 +28,7 @@ import OFS.Application
 from AccessControl.SecurityManagement import setSecurityManager
 from AccessControl.SecurityManagement import noSecurityManager
 from AccessControl import Unauthorized
+from Acquisition import Implicit
 from Products.ZCatalog import Vocabulary
 from Products.ZCatalog.Catalog import Catalog
 from Products.ZCatalog.Catalog import CatalogError
@@ -159,6 +160,31 @@ class zdummyFalse(zdummy):
     def __nonzero__(self):
         return False
 
+# make objects with failing __len__ and __nonzero__
+class dummyLenFail(zdummy):
+    def __init__(self, num, fail):
+        zdummy.__init__(self, num)
+        self.fail = fail
+
+    def __len__(self):
+        self.fail("__len__() was called")
+
+class dummyNonzeroFail(zdummy):
+    def __init__(self, num, fail):
+        zdummy.__init__(self, num)
+        self.fail = fail
+
+    def __nonzero__(self):
+        self.fail("__nonzero__() was called")
+
+class fakeparent(Implicit):
+    # fake parent mapping unrestrictedTraverse to
+    # catalog.resolve_path as simulated by TestZCatalog
+    def __init__(self, d):
+        self.d = d
+
+    def unrestrictedTraverse(self, path, default=None):
+        return self.d.get(path, default)
 
 class TestZCatalog(unittest.TestCase):
 
@@ -246,27 +272,29 @@ class TestZCatalog(unittest.TestCase):
         result = self._catalog(title='9999')
         self.assertEquals(1, len(result))
 
-    def test_manage_catalogObject_does_not_trigger_boolean_eval(self):
-        # make objects with __len__ and __nonzero__
-        class mydummy1:
-            def __init__(self, fail):
-                self.fail = fail
-            def __len__(self):
-                self.fail("__len__() was called")
-        class mydummy2:
-            def __init__(self, fail):
-                self.fail = fail
-            def __nonzero__(self):
-                self.fail("__nonzero__() was called")
-        # store them to be found by the catalog
-        self.d['0'] = mydummy1(self.fail)
-        self.d['1'] = mydummy2(self.fail)
+    def testBooleanEvalOn_manage_catalogObject_(self):
+        self.d['11'] = dummyLenFail(11, self.fail)
+        self.d['12'] = dummyNonzeroFail(12, self.fail)
         # create a fake response that doesn't bomb on manage_catalogObject()
         class myresponse:
             def redirect(self, url):
                 pass
         # this next call should not fail
-        self._catalog.manage_catalogObject(None, myresponse(), 'URL1', urls=('0', '1'))
+        self._catalog.manage_catalogObject(None, myresponse(), 'URL1', urls=('11', '12'))
+
+    def testBooleanEvalOn_refreshCatalog_getobject(self):
+        # wrap catalog under the fake parent
+        catalog = self._catalog.__of__(fakeparent(self.d))
+        # replace entries to test refreshCatalog
+        self.d['0'] = dummyLenFail(0, self.fail)
+        self.d['1'] = dummyNonzeroFail(1, self.fail)
+        # this next call should not fail
+        catalog.refreshCatalog()
+
+        for uid in ('0', '1'):
+            rid = self._catalog.getrid(uid)
+            # neither should these
+            catalog.getobject(rid)
 
 class dummy(ExtensionClass.Base):
     att1 = 'att1'