Merge lp:~rye/python-oops-tools/local-charset-in-url into lp:python-oops-tools

[Roman Yepishev]

Force quoting the URL if the string cannot be presented as ascii during loading.

[Steve Kowalik (stevenk)]

This looks like excellent work, thank you.

My two niggling comments about this is we have a coding convention that comments are full sentences, so please end your comment with a full stop, and please add a comment to the start of your test case.

[Steve Kowalik (stevenk)]

This looks like excellent work, thank you.

My two niggling comments about this is we have a coding convention that comments are full sentences, so please end your comment with a full stop, and please add a comment to the start of your test case.

review: Approve (code)

[Robert Collins (lifeless)]

This looks like it should be an else path to the prior case: if we get a bytestring, check that its ascii. (And the right way to do that would be decode('ascii') not an encode.

review: Needs Fixing

[Robert Collins (lifeless)]

I've tidied this up in a branch of mine, which I'm going to self-approve (the only functional change was to make this code the else: clause of the if (type) is unicode: condition.

=== modified file 'src/oopstools/oops/models.py'
--- src/oopstools/oops/models.py	2011-10-25 02:25:14 +0000
+++ src/oopstools/oops/models.py	2011-10-25 12:59:23 +0000
@@ -371,6 +371,15 @@
         # We have gotten a ringer, URL's are bytestrings. Encode to UTF8 to get
         # a bytestring and urllib.quote to get a url.
         url = urllib.quote(url.encode('utf8'))
+
+    try:
+        url.encode('ascii')
+    except UnicodeDecodeError:
+        # The URL cannot be represented as ascii but it is not unicode
+        # either. There may be a byte in local encoding and we need to quote
+        # it without any conversion
+        url = urllib.quote(url)
+
     informational = oops.get('informational', 'False').lower() == 'true'
     oops_date = oops.get('time')
     if oops_date is None:
 
=== modified file 'src/oopstools/oops/test/test_dboopsloader.py'
--- src/oopstools/oops/test/test_dboopsloader.py	2011-10-24 05:21:01 +0000
+++ src/oopstools/oops/test/test_dboopsloader.py	2011-10-25 12:59:23 +0000
@@ -154,3 +154,11 @@
         report = { 'url': 'foo', 'id': 'testnotopichandling'}
         oops = parsed_oops_to_model_oops(report, 'bug_880641')
         self.assertEqual('', oops.pageid)
+
+    def test_broken_url_handling(self):
+        broken_url = '/somep\xe1th'
+        report = { 'url': broken_url, 'id': 'testbrokenurl'}
+        expected_url = urllib.quote(broken_url)
+        oops = parsed_oops_to_model_oops(report, 'test_broken_url_handling')
+        self.assertEqual(expected_url, oops.url)
+