Issue 395 - Filter hits statistics backend

sitescripts/filterhits/web/submit.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2015 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import json
-import MySQLdb

[Sebastian Noack, 2015/03/31 07:55:21]

Nit: Third-party modules go between corelib and own module imports.

[kzar, 2015/03/31 09:48:56]

Done.

+import os
 import tempfile
 import time
 import traceback
-import os
 from datetime import datetime
 from errno import EEXIST
+
+import MySQLdb
 
 from sitescripts.web import url_handler
 from sitescripts.utils import get_config, setupStderr
 from sitescripts.filterhits import db, geometrical_mean
 from sitescripts.filterhits.web import common
 
 def log_filterhits(data, basepath, query_string):
   """
   This logs the provided filterhits data as JSON to a file named after
   the current timestamp in a directory named after the current date.
   """
   now = time.gmtime()
 
   dir_name = time.strftime("%Y-%m-%d", now)
   path = os.path.join(basepath, dir_name)
   try:
     os.makedirs(path)
   except OSError as e:
     if e.errno != EEXIST:
       raise
 
   with tempfile.NamedTemporaryFile(
     prefix = str(int(time.mktime(now))) + "-",
     suffix = ".log",
     dir = path,
     delete = False
   ) as f:
-    print >> f, "[%s] %s" % (time.strftime('%d/%b/%Y:%H:%M:%S', now), query_string)
+    print >> f, "[%s] %s" % (time.strftime("%d/%b/%Y:%H:%M:%S", now), query_string)
     json.dump(data, f)
     return f.name
 
 @url_handler("/submit")
 def submit(environ, start_response):
   setupStderr(environ["wsgi.errors"])
   config = get_config()
 
   # Check that this is a POST request
   if environ["REQUEST_METHOD"] != "POST":
@@ skipping 21 matching lines @@
   # Update the geometrical_mean aggregations in the database
   interval = config.get("filterhitstats", "interval")
   try:
     db_connection = db.connect()
     try:
       db.write(db_connection, geometrical_mean.update(interval, data))
     finally:
       db_connection.close()
   except:
     # Updating the aggregations in the database failed for whatever reason,
-    # log the details but continue to return 200 OK to the client to avoid
-    # re-transmission of the data.
+    # log the details but continue to return 204 to the client to avoid the
+    # re-transmission of data.
     processing_error_log = os.path.join(config.get("filterhitstats", "log_dir"),
                                         "processing-errors.log")
     with open(processing_error_log, "a+") as f:
       message = "Problem processing data file %s:\n%s" % (
         log_file, traceback.format_exc()
       )
-      print >> f, "[%s] %s" % (datetime.now().strftime('%d/%b/%Y:%H:%M:%S %z'), message)
+      print >> f, "[%s] %s" % (datetime.now().strftime("%d/%b/%Y:%H:%M:%S %z"), message)
 
-  # Send back a 200 OK response
-  response_headers = [("Content-type", "text/plain")]

[Sebastian Noack, 2015/03/31 07:55:21]

Do we actually need to specify a content type for no content?

[kzar, 2015/03/31 09:48:56]

I guess not, I've removed it.

[Wladimir Palant, 2015/03/31 13:42:31]

Yes, we do - the browser still needs to know how to interpret the document, even
if it is empty.

[Sebastian Noack, 2015/03/31 13:46:53]

Well, if we would also set the correct status code - i.e. "204 No Content" - the
browser shouldn't expect any content anyway.

[kzar, 2015/04/01 19:09:39]

OK so I tested returning a 200 with and without the content type and both worked
for me, but if Wladimir says the browser still might need it I'm inclined to
believe him. You're right that 204 is technically a more correct status code but
my vote is to return 200 still as in practice maybe a client would check for
that exact code.

So I vote we just return 200 with the possibly redundant content type, as far as
I can see it is the most likely to work as we want and I don't think there are
any down sides apart from transmitting a few extra bytes.

-  start_response("200 OK", response_headers)
+  # Send back a 204 No Content
+  start_response("204 No Content", [])
   return []