Merge lp:~leonardr/launchpad/multi-part-etag into lp:launchpad/db-devel

[Leonard Richardson]

This branch does two things:

1. It upgrades lazr.restful to solve bug 336866. It also adds a Launchpad-specific test of the buggy behavior (even though the same behavior is tested in lazr.restful) so that Launchpad users can understand the behavior and so we'll have a starting point if the bug or similar bugs come back.

2. It makes some cosmetic changes to the generation of the apidoc. It generates the index file before generating the version-specific documentation (this is solely for my convenience, so I don't have to wait for all the docs to be generated when debugging the index generation), it simplifies the generation code slightly, and it styles the apidoc index.html with a small subset of the stylesheet used in the version-specific apidoc. This makes the apidoc index look consistent with the version-specific docs, instead of looking like a generic placeholder.

[Gary Poster (gary)]

Great! The lazr.restful change seems like a great solution, and it is good to have the test in the launchpad tree.

The pagetemplate simplifications are small, especially in comparison to the importance of the fix for 336866, but they make me happy. :-)

Thanks

Gary

[Gary Poster (gary)]

review: Approve

=== modified file 'lib/canonical/launchpad/pagetests/webservice/apidoc.txt'
--- lib/canonical/launchpad/pagetests/webservice/apidoc.txt	2010-03-15 16:14:46 +0000
+++ lib/canonical/launchpad/pagetests/webservice/apidoc.txt	2010-03-17 20:31:01 +0000
@@ -7,6 +7,7 @@
     >>> browser.open('http://launchpad.dev/+apidoc')
     >>> print extract_text(browser.contents)
     Launchpad Web Service API
+    ...
     Launchpad web service API documentation
     ...
     Active versions

=== added file 'lib/canonical/launchpad/pagetests/webservice/conditional-write.txt'
--- lib/canonical/launchpad/pagetests/webservice/conditional-write.txt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/pagetests/webservice/conditional-write.txt	2010-03-17 20:31:01 +0000
@@ -0,0 +1,103 @@
+Conditional writes may work even when the ETags aren't identical
+================================================================
+
+This code is tested in lazr.restful, but since it's proven a
+longstanding problem in Launchpad, we're giving it a
+Launchpad-specific test as well. This will give us a place to start if
+the problem crops up again.
+
+Here's a bug: it has an ETag and values for fields like
+'date_last_message'.
+
+  >>> url = '/bugs/1'
+  >>> bug = webservice.get(url).jsonBody()
+  >>> old_etag = bug['http_etag']
+  >>> old_date_last_message = bug['date_last_message']
+
+When we add a message to a bug, 'date_last_message' is changed as a
+side effect.
+
+  >>> print webservice.named_post(
+  ...     url, 'newMessage', subject="subject", content="content")
+  HTTP/1.1 201 Created
+  ...
+
+  >>> new_bug = webservice.get(url).jsonBody()
+  >>> new_date_last_message = new_bug['date_last_message']
+  >>> old_date_last_message == new_date_last_message
+  False
+
+Because 'date_last_message' changed, the bug resource's ETag also
+changed:
+
+  >>> new_etag = new_bug['http_etag']
+  >>> old_etag == new_etag
+  False
+
+A conditional GET request using the old ETag will fail, and the client
+will hear about the new value for 'date_last_message'.
+
+  >>> print webservice.get(url, headers={'If-None-Match' : old_etag})
+  HTTP/1.1 200 Ok
+  ...
+
+But what if we want to PATCH the bug object after adding a message?
+Logically speaking, the PATCH should go through. 'date_last_message' has
+changed, but that's not a field that a client can modify
+directly. There's no chance that my PATCH will modify
+'date_last_message' in a way that conflicts with someone else's
+PATCH. But in general, conditional write requests only succeed if the
+client's value for If-Match exactly matches the server-side ETag.
+
+lazr.restful resolves this by splitting the ETag into two parts. The
+first part changes only on changes to fields that clients cannot
+modify directly, like 'date_last_message':
+
+  >>> old_read, old_write = old_etag.rsplit('-', 1)
+  >>> new_read, new_write = new_etag.rsplit('-', 1)
+  >>> old_read == new_read
+  False
+
+The second part changes only on changes to fields that a client could
+modify directly.
+
+  >>> old_write == new_write
+  True
+
+So long as the second part of the submitted ETag matches, a
+conditional write will succeed.
+
+  >>> import simplejson
+  >>> data = simplejson.dumps({'title' : 'New title'})
+  >>> headers = {'If-Match': old_etag}
+  >>> print webservice.patch(url, 'application/json', data, headers=headers)
+  HTTP/1.1 209 Content Returned
+  ...
+
+Of course, now the resource has been modified by a client, and the
+ETag has changed.
+
+  >>> newer_etag = webservice.get(url).jsonBody()['http_etag']
+  >>> newer_read, newer_write = newer_etag.rsplit('-', 1)
+
+Both portions of the ETag has changed: the write portion because we
+just changed 'description', and the read portion because
+'date_last_updated' changed as a side effect.
+
+  >>> new_read == newer_read
+  False
+  >>> new_write == newer_write
+  False
+
+A conditional write will fail when the write portion of the submitted
+ETag doesn't match, even if the read portion matches.
+
+  >>> headers = {'If-Match': new_etag}
+  >>> print webservice.patch(url, 'application/json', data, headers=headers)
+  HTTP/1.1 412 Precondition Failed
+  ...
+
+When two clients attempt overlapping modifications of the same
+resource, the later one still gets a 412 error. If an unwritable field
+changes, a conditional read will fail, but a conditional write will
+succeed, even though the ETags don't match exactly.

=== modified file 'utilities/apidoc-index.pt'
--- utilities/apidoc-index.pt	2010-03-11 21:23:17 +0000
+++ utilities/apidoc-index.pt	2010-03-17 20:31:01 +0000
@@ -7,28 +7,47 @@
 >
   <head>
     <title>Launchpad Web Service API</title>
+
+    <style type="text/css">
+            body {
+                font-family: sans-serif;
+                font-size: 0.85em;
+                margin: 2em 8em;
+            }
+            h1 {
+                font-size: 2.5em;
+            }
+            h2 {
+                border-bottom: 1px solid black;
+                margin-top: 1em;
+                margin-bottom: 0.5em;
+                font-size: 2em;
+               }
+            ul {
+                padding-left: 1.75em;
+            }
+            p + ul, p + ol, p + dl {
+                margin-top: 0em;
+            }
+    </style>
   </head>
 
   <body>
 
     <h1>Launchpad web service API documentation</h1>
 
-    <p tal:content="structure config/service_description">
+    <p tal:content="structure options/config/service_description">
       Description of the Launchpad web service.
     </p>
 
     <h2>Active versions</h2>
 
     <ul>
-      <li tal:repeat="version_and_description versions_and_descriptions">
-
-        <tal:version tal:define="version version_and_description/version;
-                         description version_and_description/description">
-
+      <li tal:repeat="version options/config/active_versions">
         <a tal:attributes="href string:$version.html"
            tal:content="version">Version name</a>:
-        <tal:description replace="description" />
-        </tal:version>
+        <tal:description
+           replace="options/config/version_descriptions/?version" />
       </li>
     </ul>
 

=== modified file 'utilities/create-lp-wadl-and-apidoc.py'
--- utilities/create-lp-wadl-and-apidoc.py	2010-03-11 19:58:22 +0000
+++ utilities/create-lp-wadl-and-apidoc.py	2010-03-17 20:31:01 +0000
@@ -34,10 +34,18 @@
     WebServiceApplication.cached_wadl = None # do not use cached file version
     execute_zcml_for_scripts()
     config = getUtility(IWebServiceConfiguration)
+    directory, ignore = os.path.split(path_template)
 
     stylesheet = pkg_resources.resource_filename(
         'launchpadlib', 'wadl-to-refhtml.xsl')
 
+    # First, create an index.html with links to all the HTML
+    # documentation files we're about to generate.
+    template_file = 'apidoc-index.pt'
+    template = PageTemplateFile(template_file)
+    f = open(os.path.join(directory, "index.html"), 'w')
+    f.write(template(config=config))
+
     # Request the WADL from the root resource.
     # We do this by creating a request object asking for a WADL
     # representation.
@@ -63,7 +71,6 @@
 
         # Now, convert the WADL into an human-readable description and
         # put the HTML in the same directory as the WADL.
-        directory, ignore = os.path.split(path_template)
         html_filename = os.path.join(directory, version + ".html")
         print "Writing apidoc for version %s to %s" % (
             version, html_filename)
@@ -71,22 +78,6 @@
         subprocess.Popen(['xsltproc', stylesheet, filename], stdout=stdout)
         stdout.close()
 
-    # Finally, create an index.html with links to all the HTML
-    # documentation files we just generated.
-    template_file = 'apidoc-index.pt'
-    template = PageTemplateFile(template_file)
-    namespace = template.pt_getContext()
-    namespace['config'] = config
-    versions_and_descriptions = []
-    for version in config.active_versions:
-        versions_and_descriptions.append(
-            dict(version=version,
-                 description=config.version_descriptions[version]))
-    namespace['versions_and_descriptions'] = versions_and_descriptions
-
-    f = open(os.path.join(directory, "index.html"), 'w')
-    f.write(template.pt_render(namespace))
-
     return 0
 
 if __name__ == '__main__':

=== modified file 'versions.cfg'
--- versions.cfg	2010-03-11 20:53:58 +0000
+++ versions.cfg	2010-03-17 20:31:01 +0000
@@ -28,7 +28,7 @@
 lazr.delegates = 1.1.0
 lazr.enum = 1.1.2
 lazr.lifecycle = 1.1
-lazr.restful = 0.9.23
+lazr.restful = 0.9.24
 lazr.restfulclient = 0.9.12
 lazr.smtptest = 1.1
 lazr.testing = 0.1.1