Merge lp:~leonardr/lazr.restful/fix-unicode-error into lp:lazr.restful

[Leonard Richardson]

This branch changes a number of related places where Python error strings are merged with strings that may be Unicode strings. If this happens, it can cause encoding errors.

I've added a test to verify that the one case that actually causes encoding errors right now is fixed by this change.

[lp:~leonardr/lazr.restful/fix-unicode-error]

157. By Leonard Richardson on 2010-10-25

Revved version.

158. By Leonard Richardson on 2010-10-25

Changed the doctest to a unit test.

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'src/lazr/restful/NEWS.txt'
--- src/lazr/restful/NEWS.txt	2010-10-25 13:24:52 +0000
+++ src/lazr/restful/NEWS.txt	2010-10-25 16:32:51 +0000
@@ -2,7 +2,7 @@
 NEWS for lazr.restful
 =====================
 
-0.15.0 (2010-10-22)
+0.14.1 (2010-10-24)
 ===================
 
 Fixed a unicode encoding bug that precluded reporting exceptions with

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2010-10-25 13:24:52 +0000
+++ src/lazr/restful/_resource.py	2010-10-25 16:32:51 +0000
@@ -993,8 +993,8 @@
 
         # The self link and resource type link aren't part of the
         # schema, so they're handled separately.
-        modified_read_only_attribute = ("%s: You tried to modify a "
-                                        "read-only attribute.")
+        modified_read_only_attribute = (u"%s: You tried to modify a "
+                                         "read-only attribute.")
         if 'self_link' in changeset:
             if changeset['self_link'] != absoluteURL(self.entry.context,
                                                      self.request):
@@ -1059,8 +1059,8 @@
                 # error message if the new value is not identical to the
                 # current one.
                 if value != current_value:
-                    errors.append("%s: You tried to modify a collection "
-                                  "attribute." % repr_name)
+                    errors.append(u"%s: You tried to modify a collection "
+                                   "attribute." % repr_name)
                 continue
 
             if IBytes.providedBy(field):
@@ -1073,8 +1073,8 @@
                                       % repr_name)
                     else:
                         errors.append(
-                            "%s: To modify this field you need to send a PUT "
-                            "request to its URI (%s)."
+                            u"%s: To modify this field you need to send a PUT "
+                             "request to its URI (%s)."
                             % (repr_name, current_value))
                 continue
 
@@ -1087,8 +1087,8 @@
                 # possible for Vocabulary fields to specify a schema
                 # class the way IObject fields can.
                 if value != None and not field.schema.providedBy(value):
-                    errors.append("%s: Your value points to the "
-                                  "wrong kind of object" % repr_name)
+                    errors.append(u"%s: Your value points to the "
+                                   "wrong kind of object" % repr_name)
                     continue
 
             # Obtain the current value of the field.  This gives us an easy
@@ -1130,24 +1130,24 @@
                             error = e.args[0]
                         else:
                             error = "Constraint not satisfied."
-                        errors.append("%s: %s" % (repr_name, error))
+                        errors.append(u"%s: %s" % (repr_name, error))
                         continue
                     except RequiredMissing, e:
                         error = "Missing required value."
-                        errors.append("%s: %s" % (repr_name, error))
+                        errors.append(u"%s: %s" % (repr_name, error))
                     except (ValueError, ValidationError), e:
                         error = str(e)
                         if error == "":
                             error = "Validation error"
-                        errors.append("%s: %s" % (repr_name, error))
+                        errors.append(u"%s: %s" % (repr_name, error))
                         continue
                 validated_changeset.append((field, value))
         # If there are any fields left in the changeset, they're
         # fields that don't correspond to some field in the
         # schema. They're all errors.
         for invalid_field in changeset.keys():
-            errors.append("%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:

=== modified file 'src/lazr/restful/docs/webservice.txt'
--- src/lazr/restful/docs/webservice.txt	2010-08-06 15:28:40 +0000
+++ src/lazr/restful/docs/webservice.txt	2010-10-25 16:32:51 +0000
@@ -2026,40 +2026,40 @@
 matches the hostname you're requesting. If they don't match, your
 request will fail.
 
-    >>> change_joy_author(u'http://not.the.same.host' + path)
-    'author_link: No such object...'
+    >>> print change_joy_author(u'http://not.the.same.host' + path)
+    author_link: No such object...
 
 One possible source of hostname mismatches is the HTTP port. If the
 web service is served from a strange port, you'll need to specify that
 port in the URLs you send.
 
-    >>> change_joy_author(u'http://api.cookbooks.dev' + path,
-    ...                   host='api.cookbooks.dev:9000')
-    'author_link: No such object...'
+    >>> print change_joy_author(u'http://api.cookbooks.dev' + path,
+    ...                         host='api.cookbooks.dev:9000')
+    author_link: No such object...
 
-    >>> change_joy_author(u'http://api.cookbooks.dev:9000' + path,
-    ...                   host='api.cookbooks.dev:9000')
-    '{...}'
+    >>> print change_joy_author(u'http://api.cookbooks.dev:9000' + path,
+    ...                         host='api.cookbooks.dev:9000')
+    {...}
 
 You don't have to specify the default port in the URLs you send, even
 if you specified it when you made the request.
 
-    >>> change_joy_author(u'http://api.cookbooks.dev' + path,
-    ...                   host='api.cookbooks.dev:80')
-    '{...}'
-
-    >>> change_joy_author(u'http://api.cookbooks.dev:80' + path,
-    ...                   host='api.cookbooks.dev')
-    '{...}'
-
-    >>> change_joy_author(u'https://api.cookbooks.dev' + path,
-    ...                   host='api.cookbooks.dev:443')
-    'author_link: No such object...'
+    >>> print change_joy_author(u'http://api.cookbooks.dev' + path,
+    ...                         host='api.cookbooks.dev:80')
+    {...}
+
+    >>> print change_joy_author(u'http://api.cookbooks.dev:80' + path,
+    ...                         host='api.cookbooks.dev')
+    {...}
+
+    >>> print change_joy_author(u'https://api.cookbooks.dev' + path,
+    ...                         host='api.cookbooks.dev:443')
+    author_link: No such object...
 
     >>> webservice_configuration.use_https = True
-    >>> change_joy_author(u'https://api.cookbooks.dev' + path,
-    ...                   host='api.cookbooks.dev:443')
-    '{...}'
+    >>> print change_joy_author(u'https://api.cookbooks.dev' + path,
+    ...                         host='api.cookbooks.dev:443')
+    {...}
     >>> webservice_configuration.use_https = False
 
 If an entry has an IResourceDELETEOperation registered for it, you can

=== modified file 'src/lazr/restful/marshallers.py'
--- src/lazr/restful/marshallers.py	2010-10-25 13:24:52 +0000
+++ src/lazr/restful/marshallers.py	2010-10-25 16:32:51 +0000
@@ -85,7 +85,7 @@
             request_port = default_port
 
         if not isinstance(url, basestring):
-            raise ValueError("got '%s', expected string: %r" % (
+            raise ValueError(u"got '%s', expected string: %r" % (
                 type(url).__name__, url))
         if url.startswith('/'):
             # It's a relative URI. Resolve it relative to the root of this
@@ -349,7 +349,7 @@
         try:
             return self.field.vocabulary.getTermByToken(str(value)).value
         except LookupError:
-            raise ValueError("%r isn't a valid token" % value)
+            raise ValueError(u"%r isn't a valid token" % value)
 
 
 class DateTimeFieldMarshaller(SimpleFieldMarshaller):
@@ -559,9 +559,9 @@
             resource = self.dereference_url(value)
         except NotFound:
             # The URL doesn't correspond to any real object.
-            raise ValueError('No such object "%s".' % value)
+            raise ValueError(u'No such object "%s".' % value)
         except InvalidURIError:
-            raise ValueError('"%s" is not a valid URI.' % value)
+            raise ValueError(u'"%s" is not a valid URI.' % value)
         # We looked up the URL and got the thing at the other end of
         # the URL: a resource. But internally, a resource isn't a
         # valid value for any schema field. Instead we want the object

=== modified file 'src/lazr/restful/tests/test_webservice.py'
--- src/lazr/restful/tests/test_webservice.py	2010-08-25 13:21:39 +0000
+++ src/lazr/restful/tests/test_webservice.py	2010-10-25 16:32:51 +0000
@@ -15,10 +15,11 @@
 from zope.interface import implements, Interface
 from zope.interface.interface import InterfaceClass
 from zope.publisher.browser import TestRequest
-from zope.schema import Date, Datetime, TextLine
+from zope.schema import Choice, Date, Datetime, TextLine
 from zope.security.management import newInteraction, queryInteraction
 from zope.traversing.browser.interfaces import IAbsoluteURL
 
+from lazr.enum import EnumeratedType, Item
 from lazr.restful import ResourceOperation
 from lazr.restful.fields import Reference
 from lazr.restful.interfaces import (
@@ -158,8 +159,9 @@
         request = getUtility(IWebServiceConfiguration).createRequest(
             StringIO(""), {})
 
-        entry = entry_class(HasRestrictedField(""), request)
-        resource = EntryResource(HasRestrictedField(""), request)
+        data_object = HasRestrictedField("")
+        entry = entry_class(data_object, request)
+        resource = EntryResource(data_object, request)
         entry.schema['a_field'].restrict_to_interface = IHasRestrictedField
         self.assertEquals(resource.entry.a_field, '')
         resource.applyChanges({'a_field': u'a_value'})
@@ -174,6 +176,62 @@
                           {'a_field': u'a_new_value'})
         self.assertEquals(resource.entry.a_field, 'a_value')
 
+
+class UnicodeChoice(EnumeratedType):
+    """A choice between an ASCII value and a Unicode value."""
+    ASCII = Item("Ascii", "Ascii choice")
+    UNICODE = Item(u"Uni\u00e7ode", "Uni\u00e7ode choice")
+
+
+class ICanBeSetToUnicodeValue(Interface):
+    """An entry with an InterfaceRestrictedField."""
+    export_as_webservice_entry()
+    a_field = exported(Choice(
+        vocabulary=UnicodeChoice,
+        title=u"A value that might be ASCII or Unicode.",
+        required=False, default=None))
+
+
+class CanBeSetToUnicodeValue:
+    """An implementation of ICanBeSetToUnicodeValue."""
+    implements(ICanBeSetToUnicodeValue)
+    def __init__(self, value):
+        self.a_field = value
+
+
+class UnicodeErrorTestCase(WebServiceTestCase):
+    """Test that Unicode error strings are properly passed through."""
+
+    testmodule_objects = [CanBeSetToUnicodeValue, ICanBeSetToUnicodeValue]
+
+    def setUp(self):
+        super(UnicodeErrorTestCase, self).setUp()
+        getGlobalSiteManager().registerAdapter(
+            DummyAbsoluteURL,
+            [ICanBeSetToUnicodeValue, IWebServiceClientRequest], IAbsoluteURL)
+
+    def test_unicode_error(self):
+        entry_class = get_resource_factory(ICanBeSetToUnicodeValue, IEntry)
+        request = getUtility(IWebServiceConfiguration).createRequest(
+            StringIO(""), {})
+
+        data_object = CanBeSetToUnicodeValue("")
+        entry = entry_class(data_object, request)
+        resource = EntryResource(data_object, request)
+
+        # This will raise an exception, which will cause the request
+        # to fail with a 400 error code.
+        error = resource.applyChanges({'a_field': u'No such value'})
+        self.assertEqual(request.response.getStatus(), 400)
+
+        # The error message is a Unicode string which mentions both
+        # the ASCII value and the Unicode value,
+        expected_error = (
+            u'a_field: Invalid value "No such value". Acceptable values '
+            u'are: Ascii, Uni\u00e7ode')
+        self.assertEquals(error, expected_error)
+
+
 class WadlAPITestCase(WebServiceTestCase):
     """Test the docstring generation."""
 

=== modified file 'src/lazr/restful/version.txt'
--- src/lazr/restful/version.txt	2010-10-25 13:24:52 +0000
+++ src/lazr/restful/version.txt	2010-10-25 16:32:51 +0000
@@ -1,1 +1,1 @@
-0.15.0
+0.14.1