Merge lp:~leonardr/lazr.restful/more-tests-for-combined-representations into lp:lazr.restful

[Leonard Richardson]

This branch fixes some edge cases and adds some tests that I didn't add to https://code.launchpad.net/~leonardr/lazr.restful/include-html-field-representations/+merge/50174 to keep the code smaller.

1. The ETag for the application/json;include=html representation is the same as the ETag for the application/json representation.

2. If you PUT or PATCH an application/json;include=html representation, lazr.restful will ignore your value for the lp_html attribute. If you PUT the same document as application/json, lazr.restful will complain that you tried to modify a nonexistent attribute.

3. Tested that a request for application/json;include=html sets the parameter in the Content-Type header.

[j.c.sackett (jcsackett)]

This looks good.

review: Approve (code*)

[lp:~leonardr/lazr.restful/more-tests-for-combined-representations]

184. By Leonard Richardson on 2011-02-17

Merge from parent.

185. By Leonard Richardson on 2011-02-17

When dumping an entry to the web layer, dump a JSON+XHTML representation, not a JSON representation.

[Leonard Richardson (leonardr)]

I'm self-approving this. It's almost entirely new tests.

review: Approve

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2011-02-17 20:19:31 +0000
+++ src/lazr/restful/_resource.py	2011-02-17 20:19:31 +0000
@@ -407,6 +407,11 @@
             such as the construction of a representation.
         """
         # Try to find a cached value.
+        if media_type == self.JSON_PLUS_XHTML_TYPE:
+            # The JSON+HTML representation will never change
+            # unless the JSON representation also changes or the
+            # software is upgraded. Use the same ETag for both.
+            media_type = self.JSON_TYPE
         etag = self.etags_by_media_type.get(media_type)
         if etag is not None:
             return etag
@@ -971,7 +976,7 @@
             return None, "Entity-body was not a well-formed JSON document."
         return h, None
 
-    def applyChanges(self, changeset):
+    def applyChanges(self, changeset, media_type=None):
         """Apply a dictionary of key-value pairs as changes to an entry.
 
         :param changeset: A dictionary. Should come from an incoming
@@ -980,12 +985,13 @@
         :return: If there was an error, a string error message to be
         propagated to the client.
         """
+        media_type = media_type or self.JSON_TYPE
         changeset = copy.copy(changeset)
         validated_changeset = []
         errors = []
 
-        # The self link and resource type link aren't part of the
-        # schema, so they're handled separately.
+        # Some fields aren't part of the schema, so they're handled
+        # separately.
         modified_read_only_attribute = (u"%s: You tried to modify a "
                                          "read-only attribute.")
         if 'self_link' in changeset:
@@ -1011,11 +1017,17 @@
             del changeset['resource_type_link']
 
         if 'http_etag' in changeset:
-            if changeset['http_etag'] != self.getETag(self.JSON_TYPE):
+            if changeset['http_etag'] != self.getETag(media_type):
                 errors.append(modified_read_only_attribute %
                               'http_etag')
             del changeset['http_etag']
 
+        if 'lp_html' in changeset:
+            if media_type == self.JSON_PLUS_XHTML_TYPE:
+                # The HTML portion of the representation is read-only,
+                # so ignore it.
+                del changeset['lp_html']
+
         # For every field in the schema, see if there's a corresponding
         # field in the changeset.
         for name, field in get_entry_fields_in_write_order(self.entry):
@@ -1140,9 +1152,8 @@
         # fields that don't correspond to some field in the
         # schema. They're all errors.
         for invalid_field in changeset.keys():
-            if not invalid_field.endswith('_html'):
-                errors.append(u"%s: You tried to modify a nonexistent "
-                              "attribute." % invalid_field)
+            errors.append(u"%s: You tried to modify a nonexistent "
+                          "attribute." % invalid_field)
 
         # If there were errors, display them and send a status of 400.
         if len(errors) > 0:
@@ -1244,7 +1255,7 @@
         if value is None:
             return value, error
         changeset = {self.context.name : value}
-        return self.applyChanges(changeset)
+        return self.applyChanges(changeset, media_type)
 
     do_PATCH = do_PUT
 
@@ -1421,7 +1432,8 @@
             if media_type.startswith(self.JSON_TYPE):
                 repr_name, repr_value, html_renderer = (
                     self._field_with_html_renderer(name, field))
-                if media_type == self.JSON_PLUS_XHTML_TYPE and html_renderer != _default_html_renderer:
+                if (media_type == self.JSON_PLUS_XHTML_TYPE
+                    and html_renderer != _default_html_renderer):
                     # This field has an unusual HTML renderer.
                     if repr_value == self.REDACTED_VALUE:
                         # The data is redacted, so the HTML
@@ -1443,6 +1455,11 @@
 
         if len(html_renderings) > 0:
             data['lp_html'] = html_renderings
+        elif media_type == self.JSON_PLUS_XHTML_TYPE:
+            # The client requested application/json;include=lp_html,
+            # but there's no HTML to deliver. Set the Content-Type to
+            # regular application/json.
+            self.request.response.setHeader('Content-Type', self.JSON_TYPE)
         etag = self.getETag(media_type, unmarshalled_field_values)
         data['http_etag'] = etag
         return data
@@ -1532,7 +1549,7 @@
                 return ("You didn't specify a value for the attribute '%s'."
                         % repr_name)
 
-        return self.applyChanges(changeset)
+        return self.applyChanges(changeset, media_type)
 
 
     def do_PATCH(self, media_type, representation):
@@ -1540,7 +1557,7 @@
         changeset, error = self.processAsJSONHash(media_type, representation)
         if error is not None:
             return error
-        return self.applyChanges(changeset)
+        return self.applyChanges(changeset, media_type)
 
     def _applyChangesPostHook(self):
         """See `EntryManipulatingResource`."""

=== modified file 'src/lazr/restful/example/base/tests/field.txt'
--- src/lazr/restful/example/base/tests/field.txt	2011-02-17 20:19:31 +0000
+++ src/lazr/restful/example/base/tests/field.txt	2011-02-17 20:19:31 +0000
@@ -333,11 +333,14 @@
 You can get a combined JSON/HTML representation of an entry by setting
 the "include=lp_html" parameter on the application/json media type.
 
-  >>> json = webservice.get(
-  ...     cookbook_url, 'application/json;include=lp_html').jsonBody()
+  >>> response = webservice.get(
+  ...     cookbook_url, 'application/json;include=lp_html')
+  >>> print response.getheader("Content-Type")
+  application/json;include=lp_html
 
 The cookbook's description is a normal JSON representation...
 
+  >>> json = response.jsonBody()
   >>> print json['description']
   Description
 

=== modified file 'src/lazr/restful/tales.py'
--- src/lazr/restful/tales.py	2011-01-25 13:40:34 +0000
+++ src/lazr/restful/tales.py	2011-02-17 20:19:31 +0000
@@ -178,9 +178,11 @@
         if queryMultiAdapter((self.context, request), IEntry):
             resource = EntryResource(self.context, request)
         else:
-            # Just dump it as JSON.
+            # Just dump it as JSON+XHTML
             resource = self.context
-        return simplejson.dumps(resource, cls=ResourceJSONEncoder)
+        return simplejson.dumps(
+            resource, cls=ResourceJSONEncoder,
+            media_type=EntryResource.JSON_PLUS_XHTML_TYPE)
 
 
 class WadlResourceAPI(RESTUtilityBase):

=== modified file 'src/lazr/restful/tests/test_webservice.py'
--- src/lazr/restful/tests/test_webservice.py	2011-02-17 20:19:31 +0000
+++ src/lazr/restful/tests/test_webservice.py	2011-02-17 20:19:31 +0000
@@ -399,8 +399,8 @@
 
         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}))
+            representation = simplejson.loads(
+                resource.applyChanges({'web_link': existing_web_link}))
             self.assertEquals(representation['web_link'], existing_web_link)
 
 
@@ -476,12 +476,20 @@
             IHasTwoFields, ITextLine, simple_renderer, name)
 
     @contextmanager
-    def resource(self, value_1="value 1", value_2 = "value 2"):
+    def resource(self, value_1="value 1", value_2="value 2"):
         """Simplify the entry_resource call."""
         with self.entry_resource(
-            IHasTwoFields, HasTwoFields, value_1, value_2) as resource:
+            IHasTwoFields, HasTwoFields,
+            unicode(value_1), unicode(value_2)) as resource:
             yield resource
 
+    def test_web_layer_includes_html_with_json_representation(self):
+        self.register_html_field_renderer()
+        with self.resource() as resource:
+            tales_string = test_tales(
+                "entry/webservice:json", entry=resource.entry.context)
+            self.assertTrue("lp_html" in tales_string)
+
     def test_normal_json_representation_omits_lp_html(self):
         self.default_media_type = "application/json"
         self.register_html_field_renderer()
@@ -493,6 +501,9 @@
         with self.resource() as resource:
             json = simplejson.loads(resource.do_GET())
             self.assertFalse('lp_html' in json)
+            self.assertEquals(
+                resource.request.response.getHeader("Content-Type"),
+                "application/json")
 
     def test_field_specific_html_renderer_shows_up_in_lp_html(self):
         self.register_html_field_renderer("a_field")
@@ -501,6 +512,9 @@
             html = json['lp_html']
             self.assertEquals(
                 html['a_field'], simple_renderer(resource.entry.a_field))
+            self.assertEquals(
+                resource.request.response.getHeader("Content-Type"),
+                resource.JSON_PLUS_XHTML_TYPE)
 
     def test_html_renderer_for_class_renders_all_fields_of_that_class(self):
         self.register_html_field_renderer()
@@ -513,6 +527,34 @@
                 html['another_field'],
                 simple_renderer(resource.entry.another_field))
 
+    def test_json_plus_html_etag_is_json_etag(self):
+        self.register_html_field_renderer()
+        with self.resource() as resource:
+            etag_1 = resource.getETag(resource.JSON_TYPE)
+            etag_2 = resource.getETag(resource.JSON_PLUS_XHTML_TYPE)
+            self.assertEquals(etag_1, etag_2)
+
+    def test_acceptchanges_ignores_lp_html_for_json_plus_html_type(self):
+        # The lp_html portion of the representation is ignored during
+        # writes.
+        self.register_html_field_renderer()
+        json = None
+        with self.resource() as resource:
+            json_plus_xhtml = resource.JSON_PLUS_XHTML_TYPE
+            json = simplejson.loads(unicode(
+                    resource._representation(json_plus_xhtml)))
+            resource.applyChanges(json, json_plus_xhtml)
+            self.assertEquals(resource.request.response.getStatus(), 209)
+
+    def test_acceptchanges_does_not_ignore_lp_html_for_bare_json_type(self):
+        self.register_html_field_renderer()
+        json = None
+        with self.resource() as resource:
+            json = simplejson.loads(unicode(
+                    resource._representation(resource.JSON_PLUS_XHTML_TYPE)))
+            resource.applyChanges(json, resource.JSON_TYPE)
+            self.assertEquals(resource.request.response.getStatus(), 400)
+
 
 class UnicodeChoice(EnumeratedType):
     """A choice between an ASCII value and a Unicode value."""