Launchpad itself

Merge lp:~matsubara/launchpad/bug-606184-pageid-for-collections into lp:launchpad

  1. bug-606184-pageid-for-collections
  2. Merge into devel
Proposed by Diogo Matsubara
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 11467
Proposed branch: lp:~matsubara/launchpad/bug-606184-pageid-for-collections
Merge into: lp:launchpad
Diff against target: 93 lines (+50/-2)
2 files modified
lib/canonical/launchpad/webapp/servers.py (+18/-1)
lib/canonical/launchpad/webapp/tests/test_pageid.py (+32/-1)
To merge this branch: bzr merge lp:~matsubara/launchpad/bug-606184-pageid-for-collections
Reviewer Review Type Date Requested Status
Gary Poster (community) Approve
Review via email: mp+33801@code.launchpad.net

Commit message

Fixes bug 606184 by including the origin page resource in the page id for ICollectionResource views.

Description of the change

== Summary ==

This branch fixes bug 606184 by including the origin page resource in the page
id for ICollectionResource views.

== Proposed fix ==

When the page id is generated for API requests, the code inspects if the view
is a ICollectionResource, then inspects the type_url attribute and add part of
it as the collection identifier to the page id.

== Pre-implementation notes ==

I had a pre-imp with Gary. Initially Gary suggested that this could be
accomplished with adapters and I tried that route but got stuck. Then we
agreed to change the implementation to be simpler so I could move this forward.

== Implementation details ==

The origin page resource comes from ICollectionResource.type_url, so I used
that as the identifier for the given collection and added that as part of the
page id. That way when engineers are analysing such OOPS summaries they'll be
able to easily see if the OOPS is in their domain or not, as requested in
the bug report.

== Tests ==

bin/test -tt test_pageid

== Demo and Q/A ==

Once the code lands on staging, it's enough to check a staging oops summary
and see if the "scopedcollection:collectionresource" oopses contain the
additional information about the origin page resource.

== Lint ==

No lint left over in the two files modified by this branch.

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

merge-conditional

Cool, thank you.

My only comment is that I'd like to see more of a comment explaining what is going on. things to explain: what is a collection_identifier? How do you use it to relate a page id to actual code, if you are trying to debug an OOPS? Extra credit: what code is responsible for generating the collection_identifier?

Please answer at least the first two questions, and also add the information to the page ids section of https://dev.launchpad.net/Foundations/Webservice

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/webapp/servers.py'
2--- lib/canonical/launchpad/webapp/servers.py 2010-08-20 20:31:18 +0000
3+++ lib/canonical/launchpad/webapp/servers.py 2010-08-27 16:03:48 +0000
4@@ -13,6 +13,7 @@
5 import xmlrpclib
6
7 from lazr.restful.interfaces import (
8+ ICollectionResource,
9 IWebServiceConfiguration,
10 IWebServiceVersion,
11 )
12@@ -1151,9 +1152,25 @@
13 root_object_interface = IWebServiceApplication
14
15 def constructPageID(self, view, context):
16- """Add the web service named operation (if any) to the page ID."""
17+ """Add the web service named operation (if any) to the page ID.
18+
19+ See https://dev.launchpad.net/Foundations/Webservice for more
20+ information about WebService page IDs.
21+ """
22 pageid = super(WebServicePublication, self).constructPageID(
23 view, context)
24+ if ICollectionResource.providedBy(view):
25+ # collection_identifier is a way to differentiate between
26+ # CollectionResource objects. CollectionResource objects are
27+ # objects that serve a list of Entry resources through the
28+ # WebService, so by querying the CollectionResource.type_url
29+ # attribute we're able to find out the resource type the
30+ # collection holds. See lazr.restful._resource.py to see how
31+ # the type_url is constructed.
32+ # We don't need the full URL, just the type of the resource.
33+ collection_identifier = view.type_url.split('/')[-1]
34+ if collection_identifier:
35+ pageid += ':' + collection_identifier
36 op = (view.request.get('ws.op')
37 or view.request.query_string_params.get('ws.op'))
38 if op:
39
40=== modified file 'lib/canonical/launchpad/webapp/tests/test_pageid.py'
41--- lib/canonical/launchpad/webapp/tests/test_pageid.py 2010-08-20 20:31:18 +0000
42+++ lib/canonical/launchpad/webapp/tests/test_pageid.py 2010-08-27 16:03:48 +0000
43@@ -5,7 +5,8 @@
44 __metaclass__ = type
45
46
47-import unittest
48+from zope.interface import implements
49+from lazr.restful.interfaces import ICollectionResource
50
51 from canonical.launchpad.webapp.servers import WebServicePublication
52 from lp.testing import TestCase
53@@ -34,6 +35,16 @@
54 self.request = FakeRequest()
55
56
57+class FakeCollectionResourceView(FakeView):
58+ """A view object that provides ICollectionResource."""
59+ implements(ICollectionResource)
60+
61+ def __init__(self):
62+ super(FakeCollectionResourceView, self).__init__()
63+ self.type_url = (
64+ u'https://launchpad.dev/api/devel/#milestone-page-resource')
65+
66+
67 class TestWebServicePageIDs(TestCase):
68 """Ensure that the web service enhances the page ID correctly."""
69
70@@ -68,3 +79,23 @@
71 self.view.request.query_string_params['ws.op'] = 'operation-name-2'
72 self.assertEqual(
73 self.makePageID(), 'FakeContext:FakeView:operation-name-2')
74+
75+
76+class TestCollectionResourcePageIDs(TestCase):
77+ """Ensure page ids for collections display the origin page resource."""
78+
79+ def setUp(self):
80+ super(TestCollectionResourcePageIDs, self).setUp()
81+ self.publication = WebServicePublication(db=None)
82+ self.view = FakeCollectionResourceView()
83+ self.context = FakeContext()
84+
85+ def makePageID(self):
86+ return self.publication.constructPageID(self.view, self.context)
87+
88+ def test_origin_pageid_for_collection(self):
89+ # When the view provides a ICollectionResource, make sure the origin
90+ # page resource is included in the page ID.
91+ self.assertEqual(
92+ self.makePageID(),
93+ 'FakeContext:FakeCollectionResourceView:#milestone-page-resource')