Merge lp:~mbp/bzr/http-messages into lp:bzr

[Martin Pool]

show http error body text with pycurl

This, split out of <https://code.edge.launchpad.net/~mbp/bzr/497274-http-405/+merge/16229>, makes bzr print out a roughly readable version of the http error response body when we get an error from pycurl. I found it useful interactively. I have not yet added tests for it, except for a simple example doctest unit test. It would be good to get some. I guess, because we really want to test we're dealing properly with pycurl, we probably want to return an actual error from a tame http server.

[Andrew Bennetts (spiv)]

> This, split out of
> <https://code.edge.launchpad.net/~mbp/bzr/497274-http-405/+merge/16229>, makes
> bzr print out a roughly readable version of the http error response body when
> we get an error from pycurl. I found it useful interactively. I have not yet
> added tests for it, except for a simple example doctest unit test. It would
> be good to get some. I guess, because we really want to test we're dealing
> properly with pycurl, we probably want to return an actual error from a tame
> http server.

I'm basically in favour, but I have a question: how does this react if the
response body is very large? It's not uncommon for an error page to be many
screenfuls of text (even with rudimentary HTML stripping). Overwhelming a
terminal's scrollback so that bzr's error message is lost would be a regression.
So I think it would be good to limit how much of a large body we display.

[Martin Pool (mbp)]

On 8 October 2010 16:10, Andrew Bennetts <email address hidden> wrote:
>> This, split out of
>> <https://code.edge.launchpad.net/~mbp/bzr/497274-http-405/+merge/16229>, makes
>> bzr print out a roughly readable version of the http error response body when
>> we get an error from pycurl.  I found it useful interactively.  I have not yet
>> added tests for it, except for a simple example doctest unit test.  It would
>> be good to get some.  I guess, because we really want to test we're dealing
>> properly with pycurl, we probably want to return an actual error from a tame
>> http server.
>
> I'm basically in favour, but I have a question: how does this react if the
> response body is very large?  It's not uncommon for an error page to be many
> screenfuls of text (even with rudimentary HTML stripping).  Overwhelming a
> terminal's scrollback so that bzr's error message is lost would be a regression.
> So I think it would be good to limit how much of a large body we display.

That's a good point. If it is very long, there's no great guarantee
that the readable part will be near the top rather than starting off
with masses of js or css. But even then it's probably better than
losing the bzr message altogether.

--
Martin

[Andrew Bennetts (spiv)]

> That's a good point. If it is very long, there's no great guarantee
> that the readable part will be near the top rather than starting off
> with masses of js or css. But even then it's probably better than
> losing the bzr message altogether.

Right. I forgot to mention but I raised this because we did encounter this
problem at some point in the past, so it definitely can happen.

I suppose the pedantic answer is to make sure we set (or don't) the Accept
header appropriately, and if the server insists on replying with text/html to
something that can't render it then there's not much we can reasonably do about
that, apart from "munge into something vaguely terminal friendly". Your patch +
limiting the output basically does that.

[lp:~mbp/bzr/http-messages]

4916. By Martin Pool on 2010-10-08

Add test for unhtml_roughly, and truncate at 1000 bytes

[Martin Pool (mbp)]

It would be nice if http servers did send you a text/plain error message if you expressed a preference for that, but I wouldn't hold my breath. :)

This adds truncation and a test.

Is transport.http the most reasonable place for this? It's not exactly http but it is where we're likely to be given html without wanting it.

I'll see about adding something in test_http_transport.py

[lp:~mbp/bzr/http-messages]

4917. By Martin Pool on 2010-10-08

resolve news

[Martin Pool (mbp)]

Now that, after <https://code.edge.launchpad.net/~mbp/bzr/597791-http-tests/+merge/37941> test_http is more approachable, perhaps we could add an actual integration test. It would be nice to know we're using pycurl the right way.

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-10-08 01:47:43 +0000
+++ NEWS	2010-10-08 10:18:06 +0000
@@ -81,6 +81,10 @@
   which is now deprecated.
   (Marius Kruger, #400554)
 
+* When using the pycurl client module, Bazaar shows some of the text from
+  http server error messages.
+  (Martin Pool, #656667)
+
 Documentation
 *************
 

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-10-08 07:13:14 +0000
+++ bzrlib/tests/__init__.py	2010-10-08 10:18:06 +0000
@@ -3882,6 +3882,7 @@
         'bzrlib.tests',
         'bzrlib.tests.fixtures',
         'bzrlib.timestamp',
+        'bzrlib.transport.http',
         'bzrlib.version_info_formats.format_custom',
         ]
 

=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py	2010-09-01 18:43:32 +0000
+++ bzrlib/tests/test_transport.py	2010-10-08 10:18:06 +0000
@@ -31,6 +31,7 @@
 from bzrlib.transport import (
     chroot,
     fakenfs,
+    http,
     local,
     memory,
     pathfilter,
@@ -993,3 +994,14 @@
         # And the rest are threads
         for t in started[1:]:
             t.join()
+
+
+class TestUnhtml(tests.TestCase):
+
+    """Tests for unhtml_roughly"""
+
+    def test_truncation(self):
+        fake_html = "<p>something!\n" * 1000
+        result = http.unhtml_roughly(fake_html)
+        self.assertEquals(len(result), 1000)
+        self.assertStartsWith(result, " something!")

=== modified file 'bzrlib/transport/http/__init__.py'
--- bzrlib/transport/http/__init__.py	2010-06-01 13:01:20 +0000
+++ bzrlib/transport/http/__init__.py	2010-10-08 10:18:06 +0000
@@ -666,3 +666,14 @@
     def _finished_reading(self):
         """See SmartClientMediumRequest._finished_reading."""
         pass
+
+
+def unhtml_roughly(maybe_html, length_limit=1000):
+    """Very approximate html->text translation, for presenting error bodies.
+
+    :param length_limit: Truncate the result to this many characters.
+
+    >>> unhtml_roughly("<b>bad</b> things happened\\n")
+    ' bad  things happened '
+    """
+    return re.subn(r"(<[^>]*>|\n|&nbsp;)", " ", maybe_html)[0][:length_limit]

=== modified file 'bzrlib/transport/http/_pycurl.py'
--- bzrlib/transport/http/_pycurl.py	2010-06-01 13:01:20 +0000
+++ bzrlib/transport/http/_pycurl.py	2010-10-08 10:18:06 +0000
@@ -44,6 +44,7 @@
     ca_bundle,
     HttpTransportBase,
     response,
+    unhtml_roughly,
     )
 
 try:
@@ -290,22 +291,36 @@
         return code, response.handle_response(abspath, code, msg, data)
 
 
-    def _raise_curl_http_error(self, curl, info=None):
+    def _raise_curl_http_error(self, curl, info=None, body=None):
+        """Common curl->bzrlib error translation.
+
+        Some methods may choose to override this for particular cases.
+
+        The URL and code are automatically included as appropriate.
+
+        :param info: Extra information to include in the message.
+        :param body: File-like object from which the body of the page can be read.
+        """
         code = curl.getinfo(pycurl.HTTP_CODE)
         url = curl.getinfo(pycurl.EFFECTIVE_URL)
-        # Some error codes can be handled the same way for all
-        # requests
+        if body is not None:
+            response_body = body.read()
+            plaintext_body = unhtml_roughly(response_body)
+        else:
+            response_body = None
+            plaintext_body = ''
         if code == 403:
             raise errors.TransportError(
                 'Server refuses to fulfill the request (403 Forbidden)'
-                ' for %s' % url)
+                ' for %s: %s' % (url, plaintext_body))
         else:
             if info is None:
                 msg = ''
             else:
                 msg = ': ' + info
             raise errors.InvalidHttpResponse(
-                url, 'Unable to handle http code %d%s' % (code,msg))
+                url, 'Unable to handle http code %d%s: %s' 
+                % (code, msg, plaintext_body))
 
     def _debug_cb(self, kind, text):
         if kind in (pycurl.INFOTYPE_HEADER_IN, pycurl.INFOTYPE_DATA_IN,