Merge lp:~leonardr/lazr.restful/web-link into lp:lazr.restful

[Leonard Richardson]

This branch fixes numerous errors and oversights in my initial implementation of the web_link parameter, which caused test failures and other problems as I integrated web_link into Launchpad.

1. If you send a PUT or PATCH request that mentions web_link, you'll get an error if and only if you tried to _change_ the web_link.

2. I created a property 'publish_web_link' in the entry adapter utility which is used consistently when deciding whether or not a given entry type supports web_link.

3. I fixed a bug in browser_request_to_web_service_request which was taking the incoming body stream, reading it, and using the resulting string as the 'body' of the new request. Now we just pass the stream right along into the new request.

4. I added a 'web_link' <param> tag to the WADL definition of every resource type that publishes web_link. This will tell lazr.restfulclient about the web links, so they can actually be used.

[Tim Penhey (thumper)]

> if self.adapter_utility.publish_web_link:
> # Objects in the web service correspond to pages on some website.
> # Provide the link to the corresponding page on the website.
> browser_request = IWebBrowserOriginatingRequest(self.request)

Can we be sure here that there is an adapter for request to a
IWebBrowserOriginatingRequest?

I'm pretty sure that you can rewrite:
  queryAdapter(web_service_request, IWebBrowserOriginatingRequest)
as
  IWebBrowserOriginatingRequest(web_service_request, None)

Not sure if you think this is better or not.

review: Needs Information

[Leonard Richardson (leonardr)]

If there is no IWebBrowserOriginatingRequest adapter, then publish_web_link will always return false, so we can be sure.

IWebBrowserOriginatingRequest(web_service_request, None) is nicer. I'll change it.

[lp:~leonardr/lazr.restful/web-link]

164. By Leonard Richardson on 2011-02-01

Improved the adaptation from a web service request to a website request.

[Tim Penhey (thumper)]

review: Approve

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2011-01-26 16:50:13 +0000
+++ src/lazr/restful/_resource.py	2011-02-01 16:44:09 +0000
@@ -54,7 +54,6 @@
     getMultiAdapter,
     getSiteManager,
     getUtility,
-    queryAdapter,
     queryMultiAdapter,
     )
 from zope.component.interfaces import ComponentLookupError
@@ -1021,10 +1020,14 @@
             del changeset['self_link']
 
         if 'web_link' in changeset:
-            if changeset['web_link'] != absoluteURL(
-                self.entry.context, get_current_browser_request()):
-                errors.append(modified_read_only_attribute % 'web_link')
-            del changeset['web_link']
+            browser_request = IWebBrowserOriginatingRequest(
+                self.request, None)
+            if browser_request is not None:
+                existing_web_link = absoluteURL(
+                    self.entry.context, browser_request)
+                if changeset['web_link'] != existing_web_link:
+                    errors.append(modified_read_only_attribute % 'web_link')
+                del changeset['web_link']
 
         if 'resource_type_link' in changeset:
             if changeset['resource_type_link'] != self.type_url:
@@ -1428,12 +1431,10 @@
         """
         data = {}
         data['self_link'] = absoluteURL(self.context, self.request)
-        browser_request = queryAdapter(
-            self.request, IWebBrowserOriginatingRequest)
-        if (browser_request is not None
-            and self.adapter_utility.publish_web_link):
-            # Objects in the web server correspond to objects on some website.
-            # Provide the link to the correspnding object on the website.
+        if self.adapter_utility.publish_web_link:
+            # Objects in the web service correspond to pages on some website.
+            # Provide the link to the corresponding page on the website.
+            browser_request = IWebBrowserOriginatingRequest(self.request)
             data['web_link'] = absoluteURL(self.context, browser_request)
         data['resource_type_link'] = self.type_url
         unmarshalled_field_values = {}
@@ -2169,8 +2170,14 @@
 
     @property
     def publish_web_link(self):
-        """Should this object type have a web_link published?"""
-        return self._get_tagged_value('publish_web_link')
+        """Return true if this entry should have a web_link."""
+        # If we can't adapt a web service request to a website
+        # request, we shouldn't publish a web_link for *any* entry.
+        web_service_request = get_current_web_service_request()
+        website_request = IWebBrowserOriginatingRequest(
+            web_service_request, None)
+        return website_request is not None and self._get_tagged_value(
+            'publish_web_link')
 
     @property
     def singular_type(self):

=== modified file 'src/lazr/restful/docs/webservice.txt'
--- src/lazr/restful/docs/webservice.txt	2011-01-21 21:12:17 +0000
+++ src/lazr/restful/docs/webservice.txt	2011-02-01 16:44:09 +0000
@@ -555,13 +555,19 @@
     >>> from lazr.restful.interfaces import IEntry, LAZR_WEBSERVICE_NAME
     >>> class IAuthorEntry(IAuthor, IEntry):
     ...     """The part of an author we expose through the web service."""
-    ...     taggedValue(LAZR_WEBSERVICE_NAME, dict(singular="author",
-    ...                                            plural="authors"))
+    ...     taggedValue(
+    ...         LAZR_WEBSERVICE_NAME,
+    ...         dict(
+    ...             singular="author", plural="authors",
+    ...             publish_web_link=True))
 
     >>> class ICommentEntry(IComment, IEntry):
     ...     """The part of a comment we expose through the web service."""
-    ...     taggedValue(LAZR_WEBSERVICE_NAME,
-    ...         dict(singular="comment", plural="comments"))
+    ...     taggedValue(
+    ...         LAZR_WEBSERVICE_NAME,
+    ...         dict(
+    ...             singular="comment", plural="comments",
+    ...             publish_web_link=True))
 
 Most of the time, it doesn't work to expose to the web service the same data
 model we expose internally. Usually there are fields we don't want to expose,
@@ -581,8 +587,11 @@
     >>> class IDishEntry(IEntry):
     ...     "The part of a dish that we expose through the web service."
     ...     recipes = CollectionField(value_type=Reference(schema=IRecipe))
-    ...     taggedValue(LAZR_WEBSERVICE_NAME,
-    ...         dict(singular="dish", plural="dishes"))
+    ...     taggedValue(
+    ...         LAZR_WEBSERVICE_NAME,
+    ...         dict(
+    ...             singular="dish", plural="dishes",
+    ...             publish_web_link=True))
 
 In the following code block we define an interface that exposes the underlying
 ``Recipe``'s name but not its ID. References to associated objects (like the
@@ -595,8 +604,11 @@
     ...     dish = Reference(schema=IDish)
     ...     instructions = Text(title=u"Name", required=True)
     ...     comments = CollectionField(value_type=Reference(schema=IComment))
-    ...     taggedValue(LAZR_WEBSERVICE_NAME,
-    ...         dict(singular="recipe", plural="recipes"))
+    ...     taggedValue(
+    ...         LAZR_WEBSERVICE_NAME,
+    ...         dict(
+    ...             singular="recipe", plural="recipes",
+    ...             publish_web_link=True))
 
     >>> from lazr.restful.fields import ReferenceChoice
     >>> class ICookbookEntry(IEntry):
@@ -607,8 +619,11 @@
     ...     recipes = CollectionField(value_type=Reference(schema=IRecipe))
     ...     comments = CollectionField(value_type=Reference(schema=IComment))
     ...     cover = Bytes(0, 5000, title=u"An image of the cookbook's cover.")
-    ...     taggedValue(LAZR_WEBSERVICE_NAME,
-    ...         dict(singular="cookbook", plural="cookbooks"))
+    ...     taggedValue(
+    ...         LAZR_WEBSERVICE_NAME,
+    ...         dict(
+    ...             singular="cookbook", plural="cookbooks",
+    ...             publish_web_link=True))
 
 The ``author`` field is a choice between ``Author`` objects. To make sure
 that the ``Author`` objects are properly marshalled to JSON, we need to

=== modified file 'src/lazr/restful/publisher.py'
--- src/lazr/restful/publisher.py	2011-01-21 21:12:17 +0000
+++ src/lazr/restful/publisher.py	2011-02-01 16:44:09 +0000
@@ -305,7 +305,7 @@
     if web_service_version is None:
         web_service_version = config.active_versions[-1]
 
-    body = website_request.bodyStream.getCacheStream().read()
+    body = website_request.bodyStream.getCacheStream()
     environ = dict(website_request.environment)
     # Zope picks up on SERVER_URL when setting the _app_server attribute
     # of the new request.

=== modified file 'src/lazr/restful/tales.py'
--- src/lazr/restful/tales.py	2011-01-21 21:12:17 +0000
+++ src/lazr/restful/tales.py	2011-02-01 16:44:09 +0000
@@ -441,6 +441,10 @@
         return self.utility.entry_page_representation_id
 
     @property
+    def publish_web_link(self):
+        return self.utility.publish_web_link
+
+    @property
     def all_fields(self):
         "Return all schema fields for the object."
         return [field for name, field in

=== modified file 'src/lazr/restful/templates/wadl-root.pt'
--- src/lazr/restful/templates/wadl-root.pt	2010-08-09 20:05:08 +0000
+++ src/lazr/restful/templates/wadl-root.pt	2011-02-01 16:44:09 +0000
@@ -223,6 +223,13 @@
         <wadl:doc>The canonical link to this resource.</wadl:doc>
         <link tal:attributes="resource_type context/wadl_entry:type_link" />
       </param>
+      <param style="plain" name="web_link" path="$['web_link']"
+             tal:condition="context/wadl_entry:publish_web_link">
+        <wadl:doc>
+          The canonical human-addressable web link to this resource.
+        </wadl:doc>
+        <link />
+      </param>
       <param style="plain" name="resource_type_link"
              path="$['resource_type_link']">
         <wadl:doc>

=== modified file 'src/lazr/restful/tests/test_webservice.py'
--- src/lazr/restful/tests/test_webservice.py	2011-01-26 16:50:13 +0000
+++ src/lazr/restful/tests/test_webservice.py	2011-02-01 16:44:09 +0000
@@ -6,10 +6,12 @@
 
 from contextlib import contextmanager
 from cStringIO import StringIO
+from lxml import etree
 from operator import attrgetter
 from textwrap import dedent
 import random
 import re
+import simplejson
 import unittest
 
 from zope.component import getGlobalSiteManager, getUtility
@@ -24,6 +26,8 @@
     )
 from zope.traversing.browser.interfaces import IAbsoluteURL
 
+from wadllib.application import Application
+
 from lazr.enum import EnumeratedType, Item
 from lazr.restful import (
     EntryField,
@@ -133,6 +137,8 @@
 class EntryTestCase(WebServiceTestCase):
     """A test suite that defines an entry class."""
 
+    WADL_NS = "{http://research.sun.com/wadl/2006/10}"
+
     class DummyWebsiteRequest:
         """A request to the website, as opposed to the web service."""
         implements(IWebBrowserOriginatingRequest)
@@ -142,20 +148,29 @@
         URL = 'http://www.website.url/'
 
     @contextmanager
+    def request(self):
+        request = getUtility(IWebServiceConfiguration).createRequest(
+            StringIO(""), {})
+        newInteraction(request)
+        yield request
+        endInteraction()
+
+    @property
+    def wadl(self):
+        """Get a parsed WADL description of the web service."""
+        with self.request() as request:
+            return request.publication.application.toWADL().encode('utf-8')
+
+    @contextmanager
     def entry_resource(self, entry_interface, entry_implementation):
         """Create a request to an entry resource, and yield the resource."""
         entry_class = get_resource_factory(entry_interface, IEntry)
-        request = getUtility(IWebServiceConfiguration).createRequest(
-            StringIO(""), {})
-        newInteraction(request)
-
         data_object = entry_implementation("")
-        entry = entry_class(data_object, request)
-        resource = EntryResource(data_object, request)
-
-        yield resource
-
-        endInteraction()
+
+        with self.request() as request:
+            entry = entry_class(data_object, request)
+            resource = EntryResource(data_object, request)
+            yield resource
 
     def _register_url_adapter(self, entry_interface):
         """Register an IAbsoluteURL implementation for an interface."""
@@ -221,6 +236,26 @@
             self.assertEquals(
                 representation['web_link'], self.DummyWebsiteURL.URL)
 
+    def test_wadl_includes_web_link_when_available(self):
+        # If an entry includes a web_link, this information will
+        # show up in the WADL description of the entry.
+        service_root = "https://webservice_test/2.0/"
+        self._register_website_url_space(IHasOneField)
+
+        doc = etree.parse(StringIO(self.wadl))
+        # Verify that the 'has_one_field-full' representation includes
+        # a 'web_link' param.
+        representation = [
+            rep for rep in doc.findall('%srepresentation' % self.WADL_NS)
+            if rep.get('id') == 'has_one_field-full'][0]
+        param = [
+            param for param in representation.findall(
+                '%sparam' % self.WADL_NS)
+            if param.get('name') == 'web_link'][0]
+
+        # Verify that the 'web_link' param includes a 'link' tag.
+        self.assertFalse(param.find('%slink' % self.WADL_NS) is None)
+
     def test_entry_omits_web_link_when_not_available(self):
         # When there is no way of turning a webservice request into a
         # website request, the 'web_link' attribute is missing from
@@ -233,6 +268,13 @@
                 representation['self_link'], DummyAbsoluteURL.URL)
             self.assertFalse('web_link' in representation)
 
+    def test_wadl_omits_web_link_when_not_available(self):
+        # When there is no way of turning a webservice request into a
+        # website request, the 'web_link' attribute is missing from
+        # WADL descriptions of entries.
+        self._register_url_adapter(IHasOneField)
+        self.assertFalse('web_link' in self.wadl)
+
 
 class IHasNoWebLink(Interface):
     """An entry that does not publish a web_link."""
@@ -292,6 +334,42 @@
 
 class TestEntryWrite(EntryTestCase):
 
+    testmodule_objects = [IHasOneField, HasOneField]
+
+
+    def test_applyChanges_rejects_nonexistent_web_link(self):
+        # If web_link is not published, applyChanges rejects a request
+        # that references it.
+        with self.entry_resource(IHasOneField, HasOneField) as resource:
+            errors = resource.applyChanges({'web_link': u'some_value'})
+            self.assertEquals(
+                errors,
+                'web_link: You tried to modify a nonexistent attribute.')
+
+    def test_applyChanges_rejects_changed_web_link(self):
+        """applyChanges rejects an attempt to change web_link ."""
+        self._register_website_url_space(IHasOneField)
+
+        with self.entry_resource(IHasOneField, HasOneField) as resource:
+            errors = resource.applyChanges({'web_link': u'some_value'})
+            self.assertEquals(
+                errors,
+                'web_link: You tried to modify a read-only attribute.')
+
+    def test_applyChanges_accepts_unchanged_web_link(self):
+        # applyChanges accepts a reference to web_link, as long as the
+        # value isn't actually being changed.
+        self._register_website_url_space(IHasOneField)
+
+        with self.entry_resource(IHasOneField, HasOneField) as resource:
+            existing_web_link = resource.toDataForJSON()['web_link']
+            representation = simplejson.loads(resource.applyChanges(
+                {'web_link': existing_web_link}))
+            self.assertEquals(representation['web_link'], existing_web_link)
+
+
+class TestEntryWriteForRestrictedField(EntryTestCase):
+
     testmodule_objects = [IHasRestrictedField, HasRestrictedField]
 
     def test_applyChanges_binds_to_resource_context(self):