Merge lp:~barry/lazr.restful/387487-subordinate into lp:lazr.restful

[Barry Warsaw]

[Barry Warsaw (barry)]

This fixes lazr.restful default navigation so that subentries which implement the IObject interface can be found. For Launchpad, this will allow ~team/mailman_list to be traversed, since mailing lists only live under the ~team.

[Curtis Hovey (sinzui)]

Hi Barry.

Thanks for undertaking this effort. Since your cover letter was sparce, and the inline documentation is too, I am puzzled by the complexity of the issue described in the pub and simplicity of your solution.

> === modified file 'src/lazr/restful/publisher.py'
> --- src/lazr/restful/publisher.py 2009-03-27 04:25:34 +0000
> +++ src/lazr/restful/publisher.py 2009-07-24 21:16:28 +0000
...

> def traverseName(self, request, ob, name):
> """See `zope.publisher.interfaces.IPublication`.
> @@ -66,6 +66,12 @@
> elif IBytes.providedBy(field):
> return self._traverseToByteStorage(
> request, entry, field, name)
> + elif IObject.providedBy(field):
> + sub_entry = getattr(entry, name, None)
> + if sub_entry is None:
> + raise NotFound(ob, name, request)
> + else:
> + return sub_entry
> elif field is not None:
> return EntryField(entry, field, name)
> else:

This appears to be an easy fix for a hard problem. The problem stated in
the bug is that the suborinate object and field conflict in the name space,
but there is no conflict detection here. Are there any IObject
fields that we expect to be a field? I am sure you thought about this
since you decided this is the right solution.

> === added file 'src/lazr/restful/tests/test_navigation.py'
> --- src/lazr/restful/tests/test_navigation.py 1970-01-01 00:00:00 +0000
> +++ src/lazr/restful/tests/test_navigation.py 2009-07-24 21:16:28 +0000

...

> +class Child:
> + implements(IChildEntry)
> + schema = IChild
> +

I see trailing whitespace.

...

> +class FakePublication:
> + """A fake superclass publication."""
> + def traverseName(self, request, ob, name):
> + return 'no such name: ' + name

I am confused by this. I do not see why this is need below. Are
you testing that the FakePublication.traverseName() is never reached?

> +class NavigationPublication(WebServicePublicationMixin, FakePublication):
> + pass
> +
> +
> +class NavigationTestCase(unittest.TestCase):
> +
> + def test_toplevel_navigation(self):
> + # Test that publication can reach sub-entries.
> + publication = NavigationPublication()
> + obj = publication.traverseName(FakeRequest(), Parent(), 'child')
> + self.assertEqual(obj.one, 'one')
> +
> + def test_toplevel_navigation_without_subentry(self):
> + # Test that publication raises NotFound when subentry attribute
> + # returns None.
> + parent = Parent()
> + parent.child = None
> + publication = NavigationPublication()
> + self.assertRaises(
> + NotFound, publication.traverseName,
> + FakeRequest(), parent, 'child')

review: Needs Fixing (code)

[Barry Warsaw (barry)]

On Jul 25, 2009, at 03:22 AM, Curtis Hovey wrote:

>Thanks for undertaking this effort. Since your cover letter was sparce, and the inline documentation is too, I am puzzled by the complexity of the issue described in the pub and simplicity of your solution.

Hi Curtis, thanks for the review.

My understanding is that the core problem described in the bug report is
caused by a lack of navigation in the publisher. I hit the same problem in
Launchpad trying to navigate from a team to a mailing list. Leonard has not
verified that the bug reported here is the same one I encountered.

Unfortunately, this is a situation where the solution came before the test.
While trying to figure out what was going wrong in Launchpad, Leonard and I
came up with the simple solution you see below. The problem was in crafting a
test for the change. It was a rats nest of lots of additional code to test
the change at the end-user visible layer. Trying to add a subordinate object
to the test scenario laid out in the doctests is far from easy. You have to
add the object, a schema, an entry interface, adapters, etc.

The test that I added might be too narrow, but it does tests the specific code
added. As you observed in irc, there really aren't any direct tests of
navigation that we could find.

>> === modified file 'src/lazr/restful/publisher.py'
>> --- src/lazr/restful/publisher.py 2009-03-27 04:25:34 +0000
>> +++ src/lazr/restful/publisher.py 2009-07-24 21:16:28 +0000
>...
>
>> def traverseName(self, request, ob, name):
>> """See `zope.publisher.interfaces.IPublication`.
>> @@ -66,6 +66,12 @@
>> elif IBytes.providedBy(field):
>> return self._traverseToByteStorage(
>> request, entry, field, name)
>> + elif IObject.providedBy(field):
>> + sub_entry = getattr(entry, name, None)
>> + if sub_entry is None:
>> + raise NotFound(ob, name, request)
>> + else:
>> + return sub_entry
>> elif field is not None:
>> return EntryField(entry, field, name)
>> else:
>
>This appears to be an easy fix for a hard problem. The problem stated in
>the bug is that the suborinate object and field conflict in the name space,
>but there is no conflict detection here. Are there any IObject
>fields that we expect to be a field? I am sure you thought about this
>since you decided this is the right solution.

How would the subordinate object and field conflict? The attribute would have
to be accessed via a single name in the parent object. Maybe I misunderstand
what you mean by "conflict".

>> === added file 'src/lazr/restful/tests/test_navigation.py'
>> --- src/lazr/restful/tests/test_navigation.py 1970-01-01 00:00:00 +0000
>> +++ src/lazr/restful/tests/test_navigation.py 2009-07-24 21:16:28 +0000
>
>...
>
>> +class Child:
>> + implements(IChildEntry)
>> + schema = IChild
>> +
>
>I see trailing whitespace.

Fixed.

>
>...
>
>> +class FakePublication:
>> + """A fake superclass publication."""
>> + def traverseName(self, request, ...

[lp:~barry/lazr.restful/387487-subordinate]

45. By Barry Warsaw on 2009-07-27

reviewer comments

[Curtis Hovey (sinzui)]

Hi Barry.

Thanks for the explanation of the complication. I think, given my new understanding. Thie code is fine to land. Your test solution is the right way to this problem. There may be some tests to that should also be unittest instead of contrived documentation.

review: Approve (code)

[Curtis Hovey (sinzui)]

Barry.

This branch needs to be merged into the launchpad branch and the official branch:
lp:~launchpad-pqm/lazr.restful/trunk and lp:lazr.restful. Please update
src/lazr/restful/NEWS.txt with your change when you merge your official branch.

[lp:~barry/lazr.restful/387487-subordinate]

46. By Barry Warsaw on 2009-08-04

Merge upstream r45. Add NEWS.txt entry.

[Barry Warsaw (barry)]

> Barry.
>
> This branch needs to be merged into the launchpad branch and the official
> branch:
> lp:~launchpad-pqm/lazr.restful/trunk and lp:lazr.restful. Please update
> src/lazr/restful/NEWS.txt with your change when you merge your official
> branch.

Hi Curtis. I merged this into lp:lazr.restful but my current email situation will not allow me to email pqm so I can't request a merge into there. Could you do that for me?

Also, what's the ETA for running Launchpad from released lazr.restful egg?

=== modified file 'src/lazr/restful/publisher.py'
--- src/lazr/restful/publisher.py	2009-03-27 04:25:34 +0000
+++ src/lazr/restful/publisher.py	2009-07-24 21:16:28 +0000
@@ -20,26 +20,26 @@
 from zope.interface import alsoProvides, implementer, implements
 from zope.publisher.interfaces import NotFound
 from zope.publisher.interfaces.browser import IBrowserRequest
-from zope.schema.interfaces import IBytes
+from zope.schema.interfaces import IBytes, IObject
 from zope.security.checker import ProxyFactory
 
 from lazr.uri import URI
 
+from lazr.restful import (
+    CollectionResource, EntryField, EntryFieldResource, EntryResource,
+    ScopedCollection)
 from lazr.restful.interfaces import (
-    IByteStorage, ICollection, IEntry, IEntryField, IHTTPResource,
-    IWebBrowserInitiatedRequest, IWebServiceClientRequest,
-    IWebServiceConfiguration, ICollectionField)
-from lazr.restful import (
-    CollectionResource, EntryField, EntryFieldResource,
-    EntryResource, ScopedCollection)
+    IByteStorage, ICollection, ICollectionField, IEntry, IEntryField,
+    IHTTPResource, IWebBrowserInitiatedRequest, IWebServiceClientRequest,
+    IWebServiceConfiguration)
 
 
 class WebServicePublicationMixin:
     """A mixin for webservice publication.
 
     This should usually be mixed-in with ZopePublication, or Browser,
-    or HTTPPublication"""
-
+    or HTTPPublication.
+    """
 
     def traverseName(self, request, ob, name):
         """See `zope.publisher.interfaces.IPublication`.
@@ -66,6 +66,12 @@
                 elif IBytes.providedBy(field):
                     return self._traverseToByteStorage(
                         request, entry, field, name)
+                elif IObject.providedBy(field):
+                    sub_entry = getattr(entry, name, None)
+                    if sub_entry is None:
+                        raise NotFound(ob, name, request)
+                    else:
+                        return sub_entry
                 elif field is not None:
                     return EntryField(entry, field, name)
                 else:

=== added file 'src/lazr/restful/tests/test_navigation.py'
--- src/lazr/restful/tests/test_navigation.py	1970-01-01 00:00:00 +0000
+++ src/lazr/restful/tests/test_navigation.py	2009-07-24 21:16:28 +0000
@@ -0,0 +1,92 @@
+# Copyright 2008 Canonical Ltd.  All rights reserved.
+
+"""Tests of lazr.restful navigation."""
+
+__metaclass__ = type
+
+import unittest
+
+from zope.interface import Interface, implements
+from zope.publisher.interfaces import NotFound
+from zope.schema import Text, Object
+
+from lazr.restful.interfaces import IEntry
+from lazr.restful.publisher import WebServicePublicationMixin
+
+
+class IChild(Interface):
+    one = Text(title=u'One')
+    two = Text(title=u'Two')
+
+
+class IChildEntry(IChild, IEntry):
+    pass
+
+
+class IParent(Interface):
+    three = Text(title=u'Three')
+    child = Object(schema=IChild)
+
+
+class IParentEntry(IParent, IEntry):
+    pass
+
+
+class Child:
+    implements(IChildEntry)
+    schema = IChild
+    
+    one = u'one'
+    two = u'two'
+
+
+class Parent:
+    implements(IParentEntry)
+    schema = IParent
+
+    three = u'three'
+    child = Child()
+
+    @property
+    def context(self):
+        return self
+
+
+class FakeRequest:
+    """A fake request satisfying `traverseName()`."""
+
+    def getTraversalStack(self):
+        return ()
+
+
+class FakePublication:
+    """A fake superclass publication."""
+    def traverseName(self, request, ob, name):
+        return 'no such name: ' + name
+
+
+class NavigationPublication(WebServicePublicationMixin, FakePublication):
+    pass
+
+
+class NavigationTestCase(unittest.TestCase):
+
+    def test_toplevel_navigation(self):
+        # Test that publication can reach sub-entries.
+        publication = NavigationPublication()
+        obj = publication.traverseName(FakeRequest(), Parent(), 'child')
+        self.assertEqual(obj.one, 'one')
+
+    def test_toplevel_navigation_without_subentry(self):
+        # Test that publication raises NotFound when subentry attribute
+        # returns None.
+        parent = Parent()
+        parent.child = None
+        publication = NavigationPublication()
+        self.assertRaises(
+            NotFound, publication.traverseName,
+            FakeRequest(), parent, 'child')
+
+
+def additional_tests():
+    return unittest.TestLoader().loadTestsFromName(__name__)