lazr.restful

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

  1. 387487-subordinate
  2. Merge into trunk
Proposed by Barry Warsaw
Status: Merged
Approved by: Barry Warsaw
Approved revision: 46
Merge reported by: Barry Warsaw
Merged at revision: not available
Proposed branch: lp:~barry/lazr.restful/387487-subordinate
Merge into: lp:lazr.restful
Diff against target: None lines
To merge this branch: bzr merge lp:~barry/lazr.restful/387487-subordinate
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+9263@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Barry Warsaw (barry) wrote :

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.

Revision history for this message
Curtis Hovey (sinzui) wrote :

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)
Revision history for this message
Barry Warsaw (barry) wrote :
Download full text (4.2 KiB)

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

Read more...

lp:~barry/lazr.restful/387487-subordinate updated
45. By Barry Warsaw

reviewer comments

Revision history for this message
Curtis Hovey (sinzui) wrote :

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)
Revision history for this message
Curtis Hovey (sinzui) wrote :

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 updated
46. By Barry Warsaw

Merge upstream r45. Add NEWS.txt entry.

Revision history for this message
Barry Warsaw (barry) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
20from zope.interface import alsoProvides, implementer, implements20from zope.interface import alsoProvides, implementer, implements
21from zope.publisher.interfaces import NotFound21from zope.publisher.interfaces import NotFound
22from zope.publisher.interfaces.browser import IBrowserRequest22from zope.publisher.interfaces.browser import IBrowserRequest
23from zope.schema.interfaces import IBytes23from zope.schema.interfaces import IBytes, IObject
24from zope.security.checker import ProxyFactory24from zope.security.checker import ProxyFactory
2525
26from lazr.uri import URI26from lazr.uri import URI
2727
28from lazr.restful import (
29 CollectionResource, EntryField, EntryFieldResource, EntryResource,
30 ScopedCollection)
28from lazr.restful.interfaces import (31from lazr.restful.interfaces import (
29 IByteStorage, ICollection, IEntry, IEntryField, IHTTPResource,32 IByteStorage, ICollection, ICollectionField, IEntry, IEntryField,
30 IWebBrowserInitiatedRequest, IWebServiceClientRequest,33 IHTTPResource, IWebBrowserInitiatedRequest, IWebServiceClientRequest,
31 IWebServiceConfiguration, ICollectionField)34 IWebServiceConfiguration)
32from lazr.restful import (
33 CollectionResource, EntryField, EntryFieldResource,
34 EntryResource, ScopedCollection)
3535
3636
37class WebServicePublicationMixin:37class WebServicePublicationMixin:
38 """A mixin for webservice publication.38 """A mixin for webservice publication.
3939
40 This should usually be mixed-in with ZopePublication, or Browser,40 This should usually be mixed-in with ZopePublication, or Browser,
41 or HTTPPublication"""41 or HTTPPublication.
4242 """
4343
44 def traverseName(self, request, ob, name):44 def traverseName(self, request, ob, name):
45 """See `zope.publisher.interfaces.IPublication`.45 """See `zope.publisher.interfaces.IPublication`.
@@ -66,6 +66,12 @@
66 elif IBytes.providedBy(field):66 elif IBytes.providedBy(field):
67 return self._traverseToByteStorage(67 return self._traverseToByteStorage(
68 request, entry, field, name)68 request, entry, field, name)
69 elif IObject.providedBy(field):
70 sub_entry = getattr(entry, name, None)
71 if sub_entry is None:
72 raise NotFound(ob, name, request)
73 else:
74 return sub_entry
69 elif field is not None:75 elif field is not None:
70 return EntryField(entry, field, name)76 return EntryField(entry, field, name)
71 else:77 else:
7278
=== 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 @@
1# Copyright 2008 Canonical Ltd. All rights reserved.
2
3"""Tests of lazr.restful navigation."""
4
5__metaclass__ = type
6
7import unittest
8
9from zope.interface import Interface, implements
10from zope.publisher.interfaces import NotFound
11from zope.schema import Text, Object
12
13from lazr.restful.interfaces import IEntry
14from lazr.restful.publisher import WebServicePublicationMixin
15
16
17class IChild(Interface):
18 one = Text(title=u'One')
19 two = Text(title=u'Two')
20
21
22class IChildEntry(IChild, IEntry):
23 pass
24
25
26class IParent(Interface):
27 three = Text(title=u'Three')
28 child = Object(schema=IChild)
29
30
31class IParentEntry(IParent, IEntry):
32 pass
33
34
35class Child:
36 implements(IChildEntry)
37 schema = IChild
38
39 one = u'one'
40 two = u'two'
41
42
43class Parent:
44 implements(IParentEntry)
45 schema = IParent
46
47 three = u'three'
48 child = Child()
49
50 @property
51 def context(self):
52 return self
53
54
55class FakeRequest:
56 """A fake request satisfying `traverseName()`."""
57
58 def getTraversalStack(self):
59 return ()
60
61
62class FakePublication:
63 """A fake superclass publication."""
64 def traverseName(self, request, ob, name):
65 return 'no such name: ' + name
66
67
68class NavigationPublication(WebServicePublicationMixin, FakePublication):
69 pass
70
71
72class NavigationTestCase(unittest.TestCase):
73
74 def test_toplevel_navigation(self):
75 # Test that publication can reach sub-entries.
76 publication = NavigationPublication()
77 obj = publication.traverseName(FakeRequest(), Parent(), 'child')
78 self.assertEqual(obj.one, 'one')
79
80 def test_toplevel_navigation_without_subentry(self):
81 # Test that publication raises NotFound when subentry attribute
82 # returns None.
83 parent = Parent()
84 parent.child = None
85 publication = NavigationPublication()
86 self.assertRaises(
87 NotFound, publication.traverseName,
88 FakeRequest(), parent, 'child')
89
90
91def additional_tests():
92 return unittest.TestLoader().loadTestsFromName(__name__)

Subscribers

People subscribed via source and target branches