#2687 - Include mimeo python module

modules/adblockplus/files/mimeo.py

 #!/usr/bin/env python3

[Vasily Kuznetsov, 2017/08/04 18:01:46]

Yay! Python 3 FTW!

[f.lopez, 2017/08/07 03:10:14]

Done.

 
 import argparse
-import logging
+import re
+import sys
+import threading
+import traceback
 
 from http.server import BaseHTTPRequestHandler, HTTPServer
 from string import Template
 
-punctuation = """!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~"""

[Vasily Kuznetsov, 2017/08/04 18:01:46]

These things look like constants, they should be named in UPPER_CASE.

[f.lopez, 2017/08/04 18:58:51]

Acknowledged.

[f.lopez, 2017/08/07 03:10:15]

Done.

[f.lopez, 2017/08/07 03:10:15]

Done.

-proxy_token = 'http_'
+# The token used for the headers passed by nginx is: http_
+REGEX = r'\$http_\w+\b'
+DEFAULT_LOG = '$remote_addr - - [$time_local] "$request" $status $bytes_sent'
+_lock = threading.Lock()
+
 
 class Handler(BaseHTTPRequestHandler):

[Vasily Kuznetsov, 2017/08/04 18:01:46]

PEP8 recommends to surround top level classes and functions with 2 blank lines
[1].

Also, you can use flake8 utility[2] (probably there's a plugin for Emacs as
well) to spot such issues for you. We also have a plugin[3] that spots
deviations from eyeo Python coding styleguide[4].

[1]: https://www.python.org/dev/peps/pep-0008/#blank-lines
[2]: http://flake8.pycqa.org/
[3]: https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo
[4]: https://adblockplus.org/coding-style#python

[f.lopez, 2017/08/04 18:58:51]

Acknowledged.

[f.lopez, 2017/08/07 03:10:15]

Done.

[f.lopez, 2017/08/07 03:10:15]

Done.

-    def set_values(self, values, log_format):

[Vasily Kuznetsov, 2017/08/04 18:01:46]

So here you take a dict of variables as a parameter, then modify it (by adding
the variables that are used in `log_format`) and finally return the result. It
might seem like you don't change the dict that was passed, since the function is
returning a dict, so I would say that this kind of API is rather confusing and
error-prone. A more clear option would be to not return anything, making it
clear that the original dict will be modified.

However, I think there's an even cleaner solution:
- Don't pass values to `set_values`, but make a new dict in it.
- Fill this new dict with whatever you extract from the headers based on the
`log_format`.
- Return the new dict and then outside do a
`values.update(self.set_values(log_format))`.
What do you think?

If you follow my suggestion, it might also make sense to rename the function to
`get_header_values` or something like that.

You also only call it once with `self.log_format` as a parameter. Maybe you
don't need this parameter, since you already have `self` inside anyway?

[f.lopez, 2017/08/04 18:58:51]

Ok this makes way more sense, thanks

[f.lopez, 2017/08/07 03:10:15]

Done.

-        for var in log_format.split():
-            if proxy_token in var:
-                name = var.rstrip(punctuation).lstrip(punctuation)

[Vasily Kuznetsov, 2017/08/04 18:01:45]

This seems to be equivalent to `var.strip(punctuation)`, right?

[f.lopez, 2017/08/04 18:58:50]

Acknowledged.

[f.lopez, 2017/08/07 03:10:15]

Done.

[f.lopez, 2017/08/07 03:10:15]

Done.

-                new_var = name[len(proxy_token):].title()

[Vasily Kuznetsov, 2017/08/04 18:01:46]

You seem to be assuming that `proxy_token` is at the beginning of `name`, but
what if it's not? For example if `log_format` is '{foo_http_bar}'? Perhaps it
would be safer to do something like this:

    for var in log_format.split():
        name = var.strip(...)
        if name.startswith(proxy_token):
            # here we're sure that name starts with 'http_'.

[f.lopez, 2017/08/04 18:58:51]

you are right.

[f.lopez, 2017/08/07 03:10:15]

Done.

-                values[name] = self.headers.get(new_var, '-')
+    def get_header_values(self):
+        values = {}
+        headers = re.findall(REGEX, self.format)
+        for name in headers:
+            new_var = name[6:].replace('_', '-')
+            values[name[1:]] = self.headers.get(new_var, '-')
         return values
 
-    def log_mensaje(self, form, args):

[Vasily Kuznetsov, 2017/08/04 18:01:47]

I'd suggest to name this in English. Not everyone speaks Spanish ;)

[f.lopez, 2017/08/04 18:58:51]

Ok I did change it but apparently I forgot to push it to the code review, thanks
for pointing it out.

[f.lopez, 2017/08/07 03:10:15]

Done.

-        message = Template(form)
-        logging.info(message.safe_substitute(args))
+    def send_simple_response(self, status, response=None):
+        self.send_response(status)
+        self.end_headers()
+        if response is None:
+            response = bytes(self.responses[status][0], 'UTF-8')
+        self.wfile.write(response)
+
+    def write_info(self, args):
+        message = Template(self.format).safe_substitute(args) + '\n'
+        with _lock:
+            self.output.write(message)
+            self.output.flush()
 
     def do_POST(self):
         status = 200
-        content = bytes(self.response, "UTF-8")
+        content = bytes(self.response, 'UTF-8')
         values = {
-            "remote_addr": self.address_string(),
-            "time_local": self.log_date_time_string(),
-            "request": self.requestline,
-            "status": status,
-            "bytes_sent": len(content),
-            "Content-Type": "text/plain",
-            "Content-Length": len(content),
+            'remote_addr': self.address_string(),
+            'time_local': self.log_date_time_string(),
+            'request': self.requestline,
+            'status': status,
+            'bytes_sent': len(content),
         }
-        values = self.set_values(values, self.log_format)
-        self.log_mensaje(self.log_format, values)
-        self.send_response(status)
-        self.send_header("Content-Type",values["Content-Type"])

[Vasily Kuznetsov, 2017/08/04 18:01:46]

Our style guide recommends using single quotes in single-line strings that don't
contain a single quote inside.

[f.lopez, 2017/08/04 18:58:50]

Gonna use what you suggested before for the code standards.

[f.lopez, 2017/08/07 03:10:14]

Done.

-        self.send_header("Content-Length", values["Content-Length"])
-        self.end_headers()
-        self.wfile.write(content)
-        self.log_request(200)
+        values.update(self.get_header_values())
+        try:
+            self.write_info(values)
+            self.send_simple_response(status, content)
+        except:
+            traceback.print_exc(file=sys.stderr)
+            self.send_simple_response(500)
 
-def run(server_class=HTTPServer, handler_class=Handler, port=8000, log_format=''):

[Vasily Kuznetsov, 2017/08/04 18:01:46]

Do you really need this function? There's a lot of parameter passing, defaults
and stuff, but unless somebody is going to import this module and call `run()`,
it doesn't seem to be worth it. Maybe just inline its content where it's called
below?

[f.lopez, 2017/08/04 18:58:51]

Acknowledged.

[f.lopez, 2017/08/07 03:10:15]

Done.

-    server_address = ('', port)
-    httpd = server_class(server_address, handler_class)
-    httpd.serve_forever()
 
 if __name__ == '__main__':
     parser = argparse.ArgumentParser()
-    parser.add_argument('port', action='store',

[Vasily Kuznetsov, 2017/08/04 18:01:46]

Why is "port" a positional argument while all others are keyword arguments? This
seems a bit arbitrary.

[f.lopez, 2017/08/04 18:58:51]

Acknowledged.

[f.lopez, 2017/08/07 03:10:14]

Done.

+    parser.add_argument('--port', action='store',
                         default=8000, type=int,
                         nargs='?',
-                        help='Specify alternate port [default: 8000]')
+                        help='Port to use [default: 8000]')
     parser.add_argument('--response', action='store',

[Vasily Kuznetsov, 2017/08/04 18:01:45]

This is an optional parameter with no default. If it's not supplied, wouldn't
`args.response` be `None`?

[f.lopez, 2017/08/04 18:58:51]

Acknowledged.

[f.lopez, 2017/08/07 03:10:15]

Done.

+                        type=str, nargs='?', default='OK',
+                        help='The response send to the client')
+    parser.add_argument('--format', action='store',
                         type=str, nargs='?',
-                        help='The response send to the client')
-    parser.add_argument('--log_format', action='store',
-                        type=str, nargs='?',
-                        help='Specify the format of the log ouput')

[Vasily Kuznetsov, 2017/08/04 18:01:47]

I don't think you need the word "Specify" here. The `help` argument is a
description of the argument, so I don't think it should be an imperative
sentence. For the other arguments you are doing it right IMO.

[f.lopez, 2017/08/04 18:58:51]

Acknowledged.

[f.lopez, 2017/08/07 03:10:14]

Done.

-    parser.add_argument('--log_file', action='store',
-                        type=str, nargs='?',
+                        default=DEFAULT_LOG,
+                        help='Format of the log ouput')
+    parser.add_argument('output', action='store',
+                        type=str, nargs='?', default='-',
                         help='The file where the logs will be written')
     args = parser.parse_args()
-    handler_class = Handler
-    logging.basicConfig(filename=args.log_file, level=logging.INFO)

[Vasily Kuznetsov, 2017/08/04 18:01:46]

What do you think about moving this line 2 lines down, so that it's just before
`run(...)`? It would be nicer if all the stuff related to `handler_class` was
together.

[f.lopez, 2017/08/04 18:58:51]

I'm not using the function anymore but I'll still move this line.

[f.lopez, 2017/08/07 03:10:14]

Done.

-    setattr(handler_class, 'log_format', args.log_format)
-    setattr(handler_class, 'response', args.response)
-    run(handler_class=handler_class, port=args.port, log_format=args.log_format)
-
+    if args.output and args.output != '-':
+        fh = open(args.output, 'a')
+    else:
+        fh = open(sys.stdout.fileno(), 'w', closefd=False)
+    try:
+        Handler.output = fh
+        Handler.format = args.format
+        Handler.response = args.response
+        server_address = ('', args.port)
+        httpd = HTTPServer(server_address, Handler)
+        httpd.serve_forever()
+    finally:
+        fh.close()