Code review comment for lp:~barry/lazr.restful/387487-subordinate

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, 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?

Right, it's an artifact of an earlier incarnation. It's not needed to pass
the tests, so I've removed it.

I'd like to get some feedback from Leonard, but I'm hoping the current test
will suffice. Incremental diff follows.

=== modified file 'src/lazr/restful/tests/test_navigation.py'
--- src/lazr/restful/tests/test_navigation.py 2009-07-24 21:16:28 +0000
+++ src/lazr/restful/tests/test_navigation.py 2009-07-27 02:26:21 +0000
@@ -35,7 +35,7 @@
 class Child:
     implements(IChildEntry)
     schema = IChild
-
+
     one = u'one'
     two = u'two'

@@ -59,13 +59,7 @@
         return ()

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