Merge from regebro-traversalfix branch:

- View and attribute lookup order was changed to the following:
    1. Unacquired attributes
    2. Views
    3. Acquired attributes
  According to consensus in z3-five mailing list:
  http://codespeak.net/pipermail/z3-five/2006q2/001474.html

- The defaultView directive now only looks up views, not 
  attributes.

doc/CHANGES.txt

@@ -38,6 +38,15 @@ Zope Changes
 
       - Collector #2063: cleaned up some mess in MailHost.sendTemplate()
 
+      - View and attribute lookup order was changed to the following:
+           1. Unacquired attributes
+           2. Views
+           3. Acquired attributes
+        According to consensus in z3-five mailing list:
+        http://codespeak.net/pipermail/z3-five/2006q2/001474.html
+        
+      - The defaultView directive now only looks up views, not attributes.
+
   Zope 2.10.0 beta 1 (2006/05/30)
 
     Restructuring

lib/python/OFS/Traversable.py

@@ -190,6 +190,7 @@ class Traversable:
                         continue
 
                 bobo_traverse = _getattr(obj, '__bobo_traverse__', _none)
+                try:
                     if name and name[:1] in '@+':
                         # Process URI segment parameters.
                         ns, nm = nsParse(name)
@@ -236,30 +237,46 @@ class Traversable:
                             if not validated:
                                 raise Unauthorized, name
                     else:
+                        if hasattr(aq_base(obj), name):
                             if restricted:
                                 next = guarded_getattr(obj, name, marker)
                             else:
                                 next = _getattr(obj, name, marker)
-                    if next is marker:
-                        try:
+                        else:
                             try:
                                 next=obj[name]
                             except AttributeError:
                                 # Raise NotFound for easier debugging
                                 # instead of AttributeError: __getitem__
                                 raise NotFound, name
-                        except (NotFound, KeyError): 
+
+                except (AttributeError, NotFound, KeyError), e: 
                     # Try to look for a view
                     next = queryMultiAdapter((obj, self.REQUEST), 
                                              Interface, name)
-                            if next is None:
-                                # Didn't find one, reraise the error:
-                                raise
+
+                    if next is not None:
                         next = next.__of__(obj)
+                    elif bobo_traverse is not None:
+                        # Attribute lookup should not be done after 
+                        # __bobo_traverse__:
+                        raise e
+                    else:
+                        # No view, try acquired attributes
+                        try:
+                            if restricted:
+                                next = guarded_getattr(obj, name, marker)
+                            else:
+                                next = _getattr(obj, name, marker)
+                        except AttributeError:
+                            raise e
+                    if next is marker:
+                        # Nothing found re-raise error
+                        raise e
+                
                 if restricted and not securityManager.validate(
                     obj, obj, _none, next):
                     raise Unauthorized, name
-
                 obj = next
 
             return obj

lib/python/OFS/tests/testTraverse.py

@@ -270,7 +270,7 @@ class TestTraverse( unittest.TestCase ):
         bb = BoboTraversableWithAcquisition()
         bb = bb.__of__(self.root)
         self.failUnlessRaises(Unauthorized,
-                              self.root.folder1.restrictedTraverse, 'folder1')
+                              bb.restrictedTraverse, 'folder1')
 
     def testBoboTraverseToAcquiredAttribute(self):
         # Verify it's possible to use __bobo_traverse__ to an acquired
@@ -308,7 +308,7 @@ class TestTraverse( unittest.TestCase ):
         newSecurityManager( None, UnitTestUser().__of__( self.root ) )
         self.root.stuff = 'stuff here'
         self.failUnlessRaises(Unauthorized,
-                              self.root.folder1.restrictedTraverse, 'stuff')
+                              self.app.folder1.restrictedTraverse, 'stuff')
 
     def testDefaultValueWhenUnathorized(self):
         # Test that traversing to an unauthorized object returns
@@ -335,9 +335,234 @@ class TestTraverse( unittest.TestCase ):
             aq_base(self.root))
 
 
+import os, sys
+if __name__ == '__main__':
+    execfile(os.path.join(sys.path[0], 'framework.py'))
+
+
+class SimpleClass(object):
+    """Class with no __bobo_traverse__."""
+
+
+def test_traversable():
+    """
+    Test the behaviour of unrestrictedTraverse and views. The tests are copies
+    from Five.browser.tests.test_traversable, but instead of publishing they
+    do unrestrictedTraverse.
+
+      >>> import Products.Five
+      >>> from Products.Five import zcml
+      >>> zcml.load_config("configure.zcml", Products.Five)
+      >>> from Testing.makerequest import makerequest
+      >>> self.app = makerequest(self.app)
+      
+    ``SimpleContent`` is a traversable class by default.  Its fallback
+    traverser should raise NotFound when traversal fails.  (Note: If
+    we return None in __fallback_traverse__, this test passes but for
+    the wrong reason: None doesn't have a docstring so BaseRequest
+    raises NotFoundError.)
+
+      >>> from Products.Five.tests.testing.simplecontent import manage_addSimpleContent
+      >>> manage_addSimpleContent(self.folder, 'testoid', 'Testoid')
+      >>> from zExceptions import NotFound
+      >>> try:
+      ...    self.folder.testoid.unrestrictedTraverse('doesntexist')
+      ... except NotFound:
+      ...    pass
+      
+    Now let's take class which already has a __bobo_traverse__ method.
+    Five should correctly use that as a fallback.
+
+      >>> configure_zcml = '''
+      ... <configure xmlns="http://namespaces.zope.org/zope"
+      ...            xmlns:meta="http://namespaces.zope.org/meta"
+      ...            xmlns:browser="http://namespaces.zope.org/browser"
+      ...            xmlns:five="http://namespaces.zope.org/five">
+      ... 
+      ... <!-- make the zope2.Public permission work -->
+      ... <meta:redefinePermission from="zope2.Public" to="zope.Public" />
+      ...
+      ... <!-- this view will never be found -->
+      ... <browser:page
+      ...     for="Products.Five.tests.testing.fancycontent.IFancyContent"
+      ...     class="Products.Five.browser.tests.pages.FancyView"
+      ...     attribute="view"
+      ...     name="fancyview"
+      ...     permission="zope2.Public"
+      ...     />
+      ... <!-- these two will -->
+      ... <browser:page
+      ...     for="Products.Five.tests.testing.fancycontent.IFancyContent"
+      ...     class="Products.Five.browser.tests.pages.FancyView"
+      ...     attribute="view"
+      ...     name="raise-attributeerror"
+      ...     permission="zope2.Public"
+      ...     />
+      ... <browser:page
+      ...     for="Products.Five.tests.testing.fancycontent.IFancyContent"
+      ...     class="Products.Five.browser.tests.pages.FancyView"
+      ...     attribute="view"
+      ...     name="raise-keyerror"
+      ...     permission="zope2.Public"
+      ...     />
+      ... </configure>'''
+      >>> zcml.load_string(configure_zcml)
+
+      >>> from Products.Five.tests.testing.fancycontent import manage_addFancyContent
+      >>> info = manage_addFancyContent(self.folder, 'fancy', '')
+
+    In the following test we let the original __bobo_traverse__ method
+    kick in:
+
+      >>> self.folder.fancy.unrestrictedTraverse('something-else').index_html({})
+      'something-else'
+
+    Once we have a custom __bobo_traverse__ method, though, it always
+    takes over.  Therefore, unless it raises AttributeError or
+    KeyError, it will be the only way traversal is done.
+
+      >>> self.folder.fancy.unrestrictedTraverse('fancyview').index_html({})
+      'fancyview'
+      
+
+    Note that during publishing, if the original __bobo_traverse__ method
+    *does* raise AttributeError or KeyError, we can get normal view look-up.
+    In unrestrictedTraverse, we don't. Maybe we should? Needs discussing.
+
+      >>> self.folder.fancy.unrestrictedTraverse('raise-attributeerror')()
+      u'Fancy, fancy'
+
+      >>> self.folder.fancy.unrestrictedTraverse('raise-keyerror')()
+      u'Fancy, fancy'
+
+      >>> try:
+      ...     self.folder.fancy.unrestrictedTraverse('raise-valueerror')
+      ... except ValueError:
+      ...     pass
+
+    In the Zope 2 ZPublisher, an object with a __bobo_traverse__ will not do
+    attribute lookup unless the __bobo_traverse__ method itself does it (i.e.
+    the __bobo_traverse__ is the only element used for traversal lookup).
+    Let's demonstrate:
+
+      >>> from Products.Five.tests.testing.fancycontent import manage_addNonTraversableFancyContent
+      >>> info = manage_addNonTraversableFancyContent(self.folder, 'fancy_zope2', '')
+      >>> self.folder.fancy_zope2.an_attribute = 'This is an attribute'
+      >>> self.folder.fancy_zope2.unrestrictedTraverse('an_attribute').index_html({})
+      'an_attribute'
+
+    Without a __bobo_traverse__ method this would have returned the attribute
+    value 'This is an attribute'.  Let's make sure the same thing happens for
+    an object that has been marked traversable by Five:
+
+      >>> self.folder.fancy.an_attribute = 'This is an attribute'
+      >>> self.folder.fancy.unrestrictedTraverse('an_attribute').index_html({})
+      'an_attribute'
+
+
+    Clean up:
+
+      >>> from zope.app.testing.placelesssetup import tearDown
+      >>> tearDown()
+
+    Verify that after cleanup, there's no cruft left from five:traversable::
+
+      >>> from Products.Five.browser.tests.test_traversable import SimpleClass
+      >>> hasattr(SimpleClass, '__bobo_traverse__')
+      False
+      >>> hasattr(SimpleClass, '__fallback_traverse__')
+      False
+
+      >>> from Products.Five.tests.testing.fancycontent import FancyContent
+      >>> hasattr(FancyContent, '__bobo_traverse__')
+      True
+      >>> hasattr(FancyContent.__bobo_traverse__, '__five_method__')
+      False
+      >>> hasattr(FancyContent, '__fallback_traverse__')
+      False
+    """
+
+def test_view_doesnt_shadow_attribute():
+    """
+    Test that views don't shadow attributes, e.g. items in a folder.
+
+    Let's first define a browser page for object managers called
+    ``eagle``:
+
+      >>> configure_zcml = '''
+      ... <configure xmlns="http://namespaces.zope.org/zope"
+      ...            xmlns:meta="http://namespaces.zope.org/meta"
+      ...            xmlns:browser="http://namespaces.zope.org/browser"
+      ...            xmlns:five="http://namespaces.zope.org/five">
+      ...   <!-- make the zope2.Public permission work -->
+      ...   <meta:redefinePermission from="zope2.Public" to="zope.Public" />
+      ...   <browser:page
+      ...       name="eagle"
+      ...       for="OFS.interfaces.IObjectManager"
+      ...       class="Products.Five.browser.tests.pages.SimpleView"
+      ...       attribute="eagle"
+      ...       permission="zope2.Public"
+      ...       />
+      ...   <browser:page
+      ...       name="mouse"
+      ...       for="OFS.interfaces.IObjectManager"
+      ...       class="Products.Five.browser.tests.pages.SimpleView"
+      ...       attribute="mouse"
+      ...       permission="zope2.Public"
+      ...       />
+      ... </configure>'''
+      >>> import Products.Five
+      >>> from Products.Five import zcml
+      >>> zcml.load_config("configure.zcml", Products.Five)
+      >>> zcml.load_string(configure_zcml)
+
+    Then we create a traversable folder...
+
+      >>> from Products.Five.tests.testing.folder import manage_addFiveTraversableFolder
+      >>> manage_addFiveTraversableFolder(self.folder, 'ftf')
+
+    and add an object called ``eagle`` to it:
+
+      >>> from Products.Five.tests.testing.simplecontent import manage_addIndexSimpleContent
+      >>> manage_addIndexSimpleContent(self.folder.ftf, 'eagle', 'Eagle')
+
+    When we publish the ``ftf/eagle`` now, we expect the attribute to
+    take precedence over the view during traversal:
+
+      >>> self.folder.ftf.unrestrictedTraverse('eagle').index_html({})
+      'Default index_html called'
+
+    Of course, unless we explicitly want to lookup the view using @@:
+
+      >>> self.folder.ftf.unrestrictedTraverse('@@eagle')()
+      u'The eagle has landed'
+
+
+    Some weird implementations of __bobo_traverse__, like the one
+    found in OFS.Application, raise NotFound.  Five still knows how to
+    deal with this, hence views work there too:
+
+      >>> self.app.unrestrictedTraverse('@@eagle')()
+      u'The eagle has landed'
+
+    However, acquired attributes *should* be shadowed. See discussion on
+    http://codespeak.net/pipermail/z3-five/2006q2/001474.html
+    
+      >>> manage_addIndexSimpleContent(self.folder, 'mouse', 'Mouse')
+      >>> self.folder.ftf.unrestrictedTraverse('mouse')()
+      u'The mouse has been eaten by the eagle'
+      
+    Clean up:
+
+      >>> from zope.app.testing.placelesssetup import tearDown
+      >>> tearDown()
+    """
+
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest( unittest.makeSuite( TestTraverse ) )
+    from Testing.ZopeTestCase import FunctionalDocTestSuite
+    suite.addTest( FunctionalDocTestSuite() )
     return suite
 
 if __name__ == '__main__':

lib/python/ZPublisher/BaseRequest.py

@@ -17,6 +17,7 @@ $Id$
 from urllib import quote
 import xmlrpc
 from zExceptions import Forbidden, Unauthorized, NotFound
+from Acquisition import aq_base
 
 from zope.interface import implements, providedBy, Interface
 from zope.component import queryMultiAdapter
@@ -80,24 +81,37 @@ class DefaultPublishTraverse(object):
                     parents[-1:] = list(subobject[:-1])
                     object, subobject = subobject[-2:]
             else:
-                try:
-                    subobject=getattr(object, name)
-                except AttributeError:
+                # Try getting unacquired attributes:
+                if hasattr(aq_base(object), name):
+                    subobject = getattr(object, name)
+                else:
                     subobject=object[name]                    
              
-        except (AttributeError, KeyError, NotFound):
-            # Find a view even if it doesn't start with @@, but only
-            # If nothing else could be found
+        except (AttributeError, KeyError, NotFound), e:
+            # Nothing was found with __bobo_traverse__ or directly on
+            # the object. We try to fall back to a view:
             subobject = queryMultiAdapter((object, request), Interface, name)                
             if subobject is not None:
                 # OFS.Application.__bobo_traverse__ calls
                 # REQUEST.RESPONSE.notFoundError which sets the HTTP
                 # status code to 404
-                request.RESPONSE.setStatus(200)
+                request.response.setStatus(200)
                 # We don't need to do the docstring security check
                 # for views, so lets skip it and return the object here.
                 return subobject.__of__(object)
-            raise
+            
+            # And lastly, of there is no view, try acquired attributes, but
+            # only if there is no __bobo_traverse__:
+            if not hasattr(object,'__bobo_traverse__'):
+                try:
+                    subobject=getattr(object, name)
+                    # Again, clear any error status created by __bobo_traverse__
+                    # because we actually found something:
+                    request.response.setStatus(200)
+                    return subobject
+                except AttributeError:
+                    pass
+            raise e
 
         # Ensure that the object has a docstring, or that the parent
         # object has a pseudo-docstring for the object. Objects that
@@ -133,7 +147,9 @@ class DefaultPublishTraverse(object):
         # deprecated. So we handle that here:
         default_name = queryDefaultViewName(self.context, request)
         if default_name is not None:
-            return self.context, (default_name,)
+            # Adding '@@' here forces this to be a view.
+            # A neater solution might be desireable.
+            return self.context, ('@@' + default_name,)
         return self.context, ()
         
 

lib/python/ZPublisher/tests/testBaseRequest.py

@@ -247,9 +247,149 @@ class TestBaseRequest(TestCase):
         self.assertRaises(NotFound, r.traverse, 'folder/simpleSet')
         self.assertRaises(NotFound, r.traverse, 'folder/simpleFrozenSet')
 
+from ZPublisher import NotFound
+
+import zope.interface
+import zope.component
+import zope.testing.cleanup
+import zope.traversing.namespace
+from zope.publisher.browser import IBrowserRequest
+from zope.publisher.browser import IDefaultBrowserLayer
+from zope.traversing.interfaces import ITraversable
+
+
+class IDummy(zope.interface.Interface):
+    """IDummy"""
+
+class DummyObjectZ3(DummyObjectBasic):
+    zope.interface.implements(IDummy)
+    def __init__(self, name):
+        self.name = name
+
+class DummyObjectZ3WithAttr(DummyObjectZ3):
+    def meth(self):
+        """doc"""
+        return 'meth on %s' % self.name
+    def methonly(self):
+        """doc"""
+        return 'methonly on %s' % self.name
+
+class DummyView(Implicit):
+    def __init__(self, content, request):
+        self.content = content
+        self.request = request
+    def __call__(self):
+        return 'view on %s' % (self.content.name)
+
+class TestBaseRequestZope3Views(TestCase):
+
+    def setUp(self):
+        zope.testing.cleanup.cleanUp()
+        self.root = DummyObjectBasic()
+        folder = self.root._setObject('folder', DummyObjectZ3('folder'))
+        folder._setObject('obj', DummyObjectZ3('obj'))
+        folder._setObject('withattr', DummyObjectZ3WithAttr('withattr'))
+        folder2 = self.root._setObject('folder2',
+                                       DummyObjectZ3WithAttr('folder2'))
+        folder2._setObject('obj2', DummyObjectZ3('obj2'))
+        folder2._setObject('withattr2', DummyObjectZ3WithAttr('withattr2'))
+        gsm = zope.component.getGlobalSiteManager()
+
+        # The request needs to implement the proper interface
+        zope.interface.classImplements(BaseRequest, IDefaultBrowserLayer)
+
+        # Define our 'meth' view
+        gsm.registerAdapter(DummyView, (IDummy, IDefaultBrowserLayer), None,
+                            'meth')
+
+        # Bind the 'view' namespace (for @@ traversal)
+        gsm.registerAdapter(zope.traversing.namespace.view,
+                            (IDummy, IDefaultBrowserLayer), ITraversable,
+                            'view')
+
+    def tearDown(self):
+        zope.testing.cleanup.cleanUp()
+
+    def makeBaseRequest(self):
+        response = HTTPResponse()
+        environment = {
+            'URL': '',
+            'PARENTS': [self.root],
+            'steps': [],
+            '_hacked_path': 0,
+            '_test_counter': 0,
+            'response': response,
+            }
+        return BaseRequest(environment)
+
+    def setDefaultViewName(self, name):
+        from zope.component.interfaces import IDefaultViewName
+        gsm = zope.component.getGlobalSiteManager()
+        gsm.registerAdapter(name, (IDummy, IBrowserRequest), IDefaultViewName,
+                            '')
+
+    def test_traverse_view(self):
+        """simple view"""
+        r = self.makeBaseRequest()
+        ob = r.traverse('folder/obj/meth')
+        self.assertEqual(ob(), 'view on obj')
+        ob = r.traverse('folder/obj/@@meth')
+        self.assertEqual(ob(), 'view on obj')
+        # using default view
+        self.setDefaultViewName('meth')
+        ob = r.traverse('folder/obj')
+        self.assertEqual(ob(), 'view on obj')
+
+    def test_traverse_view_attr_local(self):
+        """method on object used first"""
+        r = self.makeBaseRequest()
+        ob = r.traverse('folder/withattr/meth')
+        self.assertEqual(ob(), 'meth on withattr')
+        ob = r.traverse('folder/withattr/@@meth')
+        self.assertEqual(ob(), 'view on withattr')
+        # using default view
+        self.setDefaultViewName('meth')
+        ob = r.traverse('folder/withattr')
+        self.assertEqual(ob(), 'view on withattr')
+
+    def test_traverse_view_attr_above(self):
+        """view takes precedence over acquired attribute"""
+        r = self.makeBaseRequest()
+        ob = r.traverse('folder2/obj2/meth')
+        self.assertEqual(ob(), 'view on obj2') # used to be buggy (acquired)
+        ob = r.traverse('folder2/obj2/@@meth')
+        self.assertEqual(ob(), 'view on obj2')
+        # using default view
+        self.setDefaultViewName('meth')
+        ob = r.traverse('folder2/obj2')
+        self.assertEqual(ob(), 'view on obj2')
+
+    def test_traverse_view_attr_local2(self):
+        """method with other method above"""
+        r = self.makeBaseRequest()
+        ob = r.traverse('folder2/withattr2/meth')
+        self.assertEqual(ob(), 'meth on withattr2')
+        ob = r.traverse('folder2/withattr2/@@meth')
+        self.assertEqual(ob(), 'view on withattr2')
+        # using default view
+        self.setDefaultViewName('meth')
+        ob = r.traverse('folder2/withattr2')
+        self.assertEqual(ob(), 'view on withattr2')
+
+    def test_traverse_view_attr_acquired(self):
+        """normal acquired attribute without view"""
+        r = self.makeBaseRequest()
+        ob = r.traverse('folder2/obj2/methonly')
+        self.assertEqual(ob(), 'methonly on folder2')
+        self.assertRaises(NotFound, r.traverse, 'folder2/obj2/@@methonly')
+        # using default view
+        self.setDefaultViewName('methonly')
+        self.assertRaises(NotFound, r.traverse, 'folder2/obj2')
 
 def test_suite():
-    return TestSuite( ( makeSuite(TestBaseRequest), ) )
+    return TestSuite( ( makeSuite(TestBaseRequest),
+                        makeSuite(TestBaseRequestZope3Views),
+                    ) )
 
 if __name__ == '__main__':
     main(defaultTest='test_suite')