Issue 395 - Filter hits statistics backend

sitescripts/filterhits/web/query.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 os, MySQLdb, json
+import json
+import os
+import traceback
 from urlparse import parse_qsl
 
+import MySQLdb
+
 from sitescripts.web import url_handler
-from sitescripts.utils import cached, get_config, setupStderr
-from sitescripts.filterhits import common
+from sitescripts.utils import cached, setupStderr
 from sitescripts.filterhits import db
+from sitescripts.filterhits.web import common
 
-def query(domain=None, filter=None, skip=0, take=20, order_by="hits DESC", **_):

[Sebastian Noack, 2015/02/17 14:59:17]

Any reason why you silently ignore additional keyword arguments?

[kzar, 2015/02/24 18:05:11]

I do that because we're taking the parameters straight from the query string. We
want to ignore any passed parameters that we don't care about here.

[Sebastian Noack, 2015/02/26 16:39:25]

I see.

+def query(domain=None, filter=None, skip=0, take=20, order="DESC", order_by="frequency", **_):
   """
   Returns the SQL and parameters needed to perform a query of the filterhits data.
   """
-  sql = """SELECT SQL_CALC_FOUND_ROWS domain, filter, hits
-           FROM geometrical_mean as g
-           LEFT JOIN filters as f ON f.sha1=g.filter_sha1
+  sql = """SELECT SQL_CALC_FOUND_ROWS domain, filter, frequency
+           FROM frequencies as freq
+           LEFT JOIN filters as f ON f.sha1=freq.filter_sha1
            %s
            ORDER BY %s
            LIMIT %%s, %%s"""
 
-  where_fields = [(s, "%" + p + "%") for s, p in (("domain", domain),

[Sebastian Noack, 2015/02/17 14:59:17]

It's best practice to use format string when concatenating more than two string.
Format strings are generally more readable and also faster. Yes, I know that you
have to escape the percent-sign then. ;)

[kzar, 2015/02/24 18:05:11]

I disagree that `"%%%s%%" % p` is easier to read than `"%" + p + "%"`.

[Sebastian Noack, 2015/02/26 16:39:25]

Fair enough.

-                                                ("filter", filter)) if p]

[Sebastian Noack, 2015/02/17 14:59:17]

Nit: Seems that the indentation is a little off here.

[kzar, 2015/02/24 18:05:11]

Done.

-  where = " AND ".join([f[0] + " LIKE %s" for f in where_fields])
-  where_sql = "WHERE " + where if where else ""
+  where = zip(*[("%s LIKE %%s" % s, "%%%s%%" % p) for s, p in (("domain", domain),
+                                                               ("filter", filter)) if p])
+  if where:
+    where_fields, params = where
+    where_sql = "WHERE " + " AND ".join(where_fields)
+  else:
+    where_sql = ""
+    params = []
 
-  order_by, order = order_by.split()

[Sebastian Noack, 2015/02/17 14:59:17]

Strings aren't proper data structures. So how about turning the order_by
argument into a tuple like ("hists", "DESC"). Or maybe even better, using two
arguments, one for the sort column, and another one to indicate the sort
direction.

[kzar, 2015/02/24 18:05:11]

Done.

-  order = order.upper() if order.upper() in ["ASC", "DESC"] else "ASC"

[Sebastian Noack, 2015/02/17 14:59:17]

Nit: When a sequence doesn't need to be modified use tuples instead lists.

[kzar, 2015/02/24 18:05:11]

Done.

-  order_by_sql = "`%s` %s" % (MySQLdb.escape_string(order_by), order)
+  order = order.upper() if order.upper() in ("ASC", "DESC") else "ASC"
+  if order_by not in ["filter", "domain", "frequency"]:
+    order_by = "frequency"
+  order_by_sql = "`%s` %s" % (order_by, order)
 
-  params = [f[1] for f in where_fields] + [int(skip), int(take)]
+  params = list(params) + [int(skip), int(take)]
   return [sql % (where_sql, order_by_sql)] + params
 
 @url_handler("/query")
 def query_handler(environ, start_response):
   setupStderr(environ["wsgi.errors"])
-  config = get_config()
-  params = dict(parse_qsl(environ.get('QUERY_STRING', '')))
+  params = dict(parse_qsl(environ.get("QUERY_STRING", "")))
 
   try:
-    db_connection = db.connect(config.get("filterhitstats", "dbuser"),
-                               config.get("filterhitstats", "dbpassword"),
-                               config.get("filterhitstats", "database"))
-    results = db.query(db_connection, *query(**params), dict_result=True)
-    total = db.query(db_connection, "SELECT FOUND_ROWS()")[0][0]
+    db_connection = db.connect()
+    try:
+      results = db.query(db_connection, *query(**params), dict_result=True)
+      total = db.query(db_connection, "SELECT FOUND_ROWS()")[0][0]
+    finally:
+      db_connection.close()
   except MySQLdb.Error:
-    return common.showError("Failed to query database!", start_response,
-                            "500 Database error")
-  finally:
-    if db_connection:

[Sebastian Noack, 2015/02/17 14:59:17]

This will result in a NameError, in case db_connection failed to connect. You
need following pattern here:

try:
  connection = db.connect(...)
  try:
    ...
  finally:
    connection.close()
except MySQLdb.Error:
  ...

[kzar, 2015/02/24 18:05:11]

Done.

-      db_connection.close()
+    traceback.print_exc()
+    return common.show_error("Failed to query database!", start_response,
+                             "500 Database error")
 
   try:
     echo = int(params["echo"])
   except (ValueError, KeyError):
     echo = 0
 
-  response_headers = [("Content-type", "application/json")]
+  response_headers = [("Content-type", "application/json; charset=utf-8")]
   start_response("200 OK", response_headers)
   return [json.dumps({"results": results, "echo": echo,
-                      "total": total, "count": len(results)})]
+                      "total": total, "count": len(results)},
+                     ensure_ascii=False).encode("utf-8")]