openlp-core team mailing list archive

[Felipe Polo-Wood <felipe@xxxxxxxxxxxxx>]

Felipe Polo-Wood has proposed merging lp:~felipe-q/openlp/fix_httprouter into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)

For more details, see:
https://code.launchpad.net/~felipe-q/openlp/fix_httprouter/+merge/194569

HttpRouter.serve_file was never sending the response headers back to the client (this can be seen by watching the network traffic on the request for http://localhost:4316).  Forgiving client browsers were assuming the content-type from the extension.
Another small problem was that the headers were set twice in the case of a 404 (although this did not have any user impact).
The code was missing the send_response and the end_headers calls.
The new code breaks the detection of the type from the sending of the headers so the appropriate response headers are always sent, and in the right order.

-- 
https://code.launchpad.net/~felipe-q/openlp/fix_httprouter/+merge/194569
Your team OpenLP Core is requested to review the proposed merge of lp:~felipe-q/openlp/fix_httprouter into lp:openlp.

=== modified file 'openlp/plugins/remotes/lib/httprouter.py'
--- openlp/plugins/remotes/lib/httprouter.py	2013-10-13 20:36:42 +0000
+++ openlp/plugins/remotes/lib/httprouter.py	2013-11-08 18:54:40 +0000
@@ -346,30 +346,14 @@
         path = os.path.normpath(os.path.join(self.html_dir, file_name))
         if not path.startswith(self.html_dir):
             return self.do_not_found()
-        ext = os.path.splitext(file_name)[1]
-        html = None
-        if ext == '.html':
-            self.send_header('Content-type', 'text/html')
-            variables = self.template_vars
-            html = Template(filename=path, input_encoding='utf-8', output_encoding='utf-8').render(**variables)
-        elif ext == '.css':
-            self.send_header('Content-type', 'text/css')
-        elif ext == '.js':
-            self.send_header('Content-type', 'application/javascript')
-        elif ext == '.jpg':
-            self.send_header('Content-type', 'image/jpeg')
-        elif ext == '.gif':
-            self.send_header('Content-type', 'image/gif')
-        elif ext == '.ico':
-            self.send_header('Content-type', 'image/x-icon')
-        elif ext == '.png':
-            self.send_header('Content-type', 'image/png')
-        else:
-            self.send_header('Content-type', 'text/plain')
+        content = None
+        ext, content_type = self.get_content_type(path)
         file_handle = None
         try:
-            if html:
-                content = html
+            if ext == '.html':
+                variables = self.template_vars
+                content = Template(filename=path, input_encoding='utf-8',
+                    output_encoding='utf-8').render(**variables)
             else:
                 file_handle = open(path, 'rb')
                 log.debug('Opened %s' % path)
@@ -380,8 +364,30 @@
         finally:
             if file_handle:
                 file_handle.close()
+        self.send_response(200)
+        self.send_header('Content-type', content_type)
+        self.end_headers()
         return content
 
+    def get_content_type(self, file_name):
+        """
+        Examines the extension of the file and determines
+        what header to send back
+        Returns the extension found
+        """
+        content_type = 'text/plain'
+        file_types = {'.html': 'text/html',
+            '.css': 'text/css',
+            '.js': 'application/javascript',
+            '.jpg': 'image/jpeg',
+            '.gif': 'image/gif',
+            '.ico': 'image/x-icon',
+            '.png': 'image/png'
+        }
+        ext = os.path.splitext(file_name)[1]
+        content_type = file_types.get(ext, 'text/plain')
+        return ext, content_type
+
     def poll(self):
         """
         Poll OpenLP to determine the current slide number and item name.

=== modified file 'tests/functional/openlp_plugins/remotes/test_remotetab.py'
--- tests/functional/openlp_plugins/remotes/test_remotetab.py	2013-10-13 20:36:42 +0000
+++ tests/functional/openlp_plugins/remotes/test_remotetab.py	2013-11-08 18:54:40 +0000
@@ -62,7 +62,7 @@
         """
         Create the UI
         """
-        fd, self.ini_file = mkstemp('.ini')
+        self.fd, self.ini_file = mkstemp('.ini')
         Settings().set_filename(self.ini_file)
         self.application = QtGui.QApplication.instance()
         Settings().extend_default_settings(__default_settings__)
@@ -76,6 +76,7 @@
         del self.application
         del self.parent
         del self.form
+        os.close(self.fd)
         os.unlink(self.ini_file)
 
     def get_ip_address_default_test(self):

=== modified file 'tests/functional/openlp_plugins/remotes/test_router.py'
--- tests/functional/openlp_plugins/remotes/test_router.py	2013-10-13 20:36:42 +0000
+++ tests/functional/openlp_plugins/remotes/test_router.py	2013-11-08 18:54:40 +0000
@@ -59,7 +59,7 @@
         """
         Create the UI
         """
-        fd, self.ini_file = mkstemp('.ini')
+        self.fd, self.ini_file = mkstemp('.ini')
         Settings().set_filename(self.ini_file)
         self.application = QtGui.QApplication.instance()
         Settings().extend_default_settings(__default_settings__)
@@ -70,6 +70,7 @@
         Delete all the C++ objects at the end so that we don't have a segfault
         """
         del self.application
+        os.close(self.fd)
         os.unlink(self.ini_file)
 
     def password_encrypter_test(self):
@@ -109,4 +110,19 @@
         assert function['function'] == mocked_function, \
             'The mocked function should match defined value.'
         assert function['secure'] == False, \
-            'The mocked function should not require any security.'
\ No newline at end of file
+            'The mocked function should not require any security.'
+
+    def get_content_type_test(self):
+        """
+        Test the get_content_type logic
+        """
+        headers = [ ['test.html', 'text/html'], ['test.css', 'text/css'],
+            ['test.js', 'application/javascript'], ['test.jpg', 'image/jpeg'],
+            ['test.gif', 'image/gif'], ['test.ico', 'image/x-icon'],
+            ['test.png', 'image/png'], ['test.whatever', 'text/plain'],
+            ['test', 'text/plain'], ['', 'text/plain'],
+            ['/test/test.html', 'text/html'],
+            ['c:\\test\\test.html', 'text/html']]
+        for header in headers:
+            ext, content_type = self.router.get_content_type(header[0])
+            self.assertEqual(content_type, header[1], 'Mismatch of content type')