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-2014 Eyeo GmbH
+# 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.utils import cached, setupStderr
+from sitescripts.filterhits import db
+from sitescripts.filterhits.web import common
 

[Sebastian Noack, 2015/02/11 16:00:12]

Nit: The newline should be between the corelib and own module.

[kzar, 2015/02/17 10:52:24]

Done.

-import sitescripts.filterhits.common as common

[Sebastian Noack, 2015/02/11 16:00:12]

Nit: You are misusing the "import .. as" syntax here. That is what "from ..
import" is for:

from sitescripts.filterhits import common
from sitescripts.filterhits import db

[kzar, 2015/02/17 10:52:24]

Done.

-import sitescripts.filterhits.db as db
-
-def query_sql(domain=None, filter=None, skip=0, take=20, order_by="hits DESC", **_):
-  sql = """SELECT SQL_CALC_FOUND_ROWS domain, filter, hits
-           FROM geometrical_mean as g
-           LEFT JOIN filters as f ON f.md5=g.filter_md5
+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, frequency
+           FROM frequencies as freq
+           LEFT JOIN filters as f ON f.sha1=freq.filter_sha1
            %s
            ORDER BY %s
-           LIMIT %d, %d;"""
-  where = ["domain LIKE '%%%s%%'" % db.escape(domain) if domain else None,
-           "filter LIKE '%%%s%%'" % db.escape(filter) if filter else None]
-  where = " AND ".join([f for f in where if f])
-  where = "WHERE " + where if where else ""
-  return sql % (where, db.escape(order_by), int(skip), int(take))
+           LIMIT %%s, %%s"""
+
+  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 = 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 = list(params) + [int(skip), int(take)]
+  return [sql % (where_sql, order_by_sql)] + params
 
 @url_handler("/query")
-def query(environ, start_response):
+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.connect(config.get("filterhitstats", "dbuser"),

[Sebastian Noack, 2015/02/11 16:00:12]

It seems this pattern is repeated. How about retrieving the user, password and
database inside the connect() method, or at least if it's not provided otherwise
(e.g. when testing)?

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

What's about this comment?

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

I prefer to do it this way, during development I experimented with a few
approaches but IMHO it's dangerous to set the database by default. All it takes
is one test to be written that forgets to pass the database name and you've
messed up your real database.

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

That doesn't make any sense. The database and credentials come from the
configuration anyway. I'm merely criticizing that you duplicate the code
retrieving those settings.

[kzar, 2015/02/28 19:39:56]

OK, I changed my mind here and I've now changed it.
Done.

-               config.get("filterhitstats", "dbpassword"),
-               config.get("filterhitstats", "database"))
-    results = db.query(query_sql(**params), dict_result=True)
-    total = db.query("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:
-    db.disconnect()
+    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")]