Merge lp:~gary/launchpad/bug676489 into lp:launchpad

[Gary Poster]

[r=benji,edwin-grubbs][ui=none][bug=676489] fix bug 676489 by adding code in the apache log parsing to handle a space in the path.

This change fixes bug 676489 by adding code to handle a space in the path. I added tests for these changes, as well as for an existing code path pertinent to HTTP 1.0 possibly not including the protocol.

To run all the pertinent tests I know about, run ``./bin/test -vvt test_apachelogparser``.

``make lint`` is happy.

Thank you.

[Benji York (benji)]

This branch looks good. Edwin, my mentor, will also be reviewing it
momentarily.

I especially appreciate the thorough test coverage.

A small comment about this bit of code:

path, ignore, protocol = rest.rpartition(' ')
if not path:
    # HTTP 1.0 requests might omit the HTTP version so we cope with them.
    path = protocol

On first reading I'd expect the body of the if to assign a new value to
"protocol" as well (perhaps None).

On second reading I realize that protocol isn't used (other than if it
isn't actually the protocol) and wonder if restraining from naming the
strings until they're known-good might be better:

# HTTP 1.0 requests might omit the HTTP version so we cope with them.
first, ignore, last = rest.partition(' ')
if first:
    path = first
else:
    path = last

Or even the shorter but perhaps obscure:

# HTTP 1.0 requests might omit the HTTP version so we cope with them.
first, ignore, last = rest.partition(' ')
path = first or last

review: Approve (code*)

[Edwin Grubbs (edwin-grubbs)]

Hi Gary,

I like benji's suggestion of using:
  first, ignore, last = rest.rpartition(' ')

I believe our style guide wants a more explicit if-condition though. For example:
  if first == "":

Otherwise, I think the branch is very nice.

-Edwin

review: Approve (code)

[Benji York (benji)]

On Wed, Nov 17, 2010 at 5:27 PM, Edwin Grubbs
<email address hidden> wrote:
> I believe our style guide wants a more explicit if-condition though. For example:
>  if first == "":

Correct: https://dev.launchpad.net/PythonStyleGuide#Truth%20conditionals
--
Benji York

[Gary Poster (gary)]

Thank you both. I changed the code per your suggestion.

=== modified file 'lib/lp/services/apachelogparser/base.py'
--- lib/lp/services/apachelogparser/base.py	2010-09-11 19:25:13 +0000
+++ lib/lp/services/apachelogparser/base.py	2010-11-17 23:22:15 +0000
@@ -204,15 +204,21 @@
 
 def get_method_and_path(request):
     """Extract the method of the request and path of the requested file."""
-    L = request.split()
-    # HTTP 1.0 requests might omit the HTTP version so we must cope with them.
-    if len(L) == 2:
-        method, path = L
+    method, ignore, rest = request.partition(' ')
+    # In the below, the common case is that `first` is the path and `last` is
+    # the protocol.
+    first, ignore, last = rest.rpartition(' ')
+    if first == '':
+        # HTTP 1.0 requests might omit the HTTP version so we cope with them.
+        path = last
+    elif not last.startswith('HTTP'):
+        # We cope with HTTP 1.0 protocol without HTTP version *and* a
+        # space in the path (see bug 676489 for example).
+        path = rest
     else:
-        method, path, protocol = L
-
+        # This is the common case.
+        path = first
     if path.startswith('http://') or path.startswith('https://'):
         uri = URI(path)
         path = uri.path
-
     return method, path

=== modified file 'lib/lp/services/apachelogparser/tests/test_apachelogparser.py'
--- lib/lp/services/apachelogparser/tests/test_apachelogparser.py	2010-10-04 19:50:45 +0000
+++ lib/lp/services/apachelogparser/tests/test_apachelogparser.py	2010-11-17 23:22:15 +0000
@@ -29,6 +29,7 @@
     get_fd_and_file_size,
     get_files_to_parse,
     get_host_date_status_and_request,
+    get_method_and_path,
     parse_file,
     )
 from lp.services.apachelogparser.model.parsedapachelog import ParsedApacheLog
@@ -71,6 +72,35 @@
         date = '[13/Jun/2008:18:38:57 +0100]'
         self.assertEqual(get_day(date), datetime(2008, 6, 13))
 
+    def test_parsing_path_with_missing_protocol(self):
+        request = (r'GET /56222647/deluge-gtk_1.3.0-0ubuntu1_all.deb?'
+                   r'N\x1f\x9b')
+        method, path = get_method_and_path(request)
+        self.assertEqual(method, 'GET')
+        self.assertEqual(
+            path,
+            r'/56222647/deluge-gtk_1.3.0-0ubuntu1_all.deb?N\x1f\x9b')
+
+    def test_parsing_path_with_space(self):
+        # See bug 676489.
+        request = (r'GET /56222647/deluge-gtk_1.3.0-0ubuntu1_all.deb?'
+                   r'N\x1f\x9b Z%7B... HTTP/1.0')
+        method, path = get_method_and_path(request)
+        self.assertEqual(method, 'GET')
+        self.assertEqual(
+            path,
+            r'/56222647/deluge-gtk_1.3.0-0ubuntu1_all.deb?N\x1f\x9b Z%7B...')
+
+    def test_parsing_path_with_space_and_missing_protocol(self):
+        # This is a variation of bug 676489.
+        request = (r'GET /56222647/deluge-gtk_1.3.0-0ubuntu1_all.deb?'
+                   r'N\x1f\x9b Z%7B...')
+        method, path = get_method_and_path(request)
+        self.assertEqual(method, 'GET')
+        self.assertEqual(
+            path,
+            r'/56222647/deluge-gtk_1.3.0-0ubuntu1_all.deb?N\x1f\x9b Z%7B...')
+
 
 class Test_get_fd_and_file_size(TestCase):