Issue 395 - Filter hits statistics backend

sitescripts/filterhits/db.py

+# coding: utf-8
+
+# This file is part of the Adblock Plus web scripts,
+# Copyright (C) 2006-2014 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 MySQLdb
+
+db = None
+
+def connect(user, password, database):
+  global db
+  if not db:
+    db = MySQLdb.connect(
+      user=user,
+      passwd=password,
+      db=database,
+      use_unicode=True, charset="utf8"
+    )
+  return db
+
+def disconnect():

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

It seems the global variable and diconnect() method is uneeded. You can simply
call the connection's close() method in the calling code.

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

Done.

+  global db
+  if db:
+    db.close()
+    db = None
+
+def escape(s):
+  return MySQLdb.escape_string(s)
+
+def query(sql, dict_result=False):
+  """
+   Executes the query given by the provided SQL and returns the results.
+   If dict_result keyword argument is provided + True the results will be
+   returned as a tuple of dictionaries, otherwise a tuple of tuples.
+  """
+  global db

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

Done.

+  try:
+    if dict_result:
+      cursor = db.cursor(MySQLdb.cursors.DictCursor)
+    else:
+      cursor = db.cursor()
+    cursor.execute(sql)
+    results = cursor.fetchall()
+  finally:
+    if cursor:
+      cursor.close()
+  return results
+
+def write(sql):
+  """
+    This writes a given SQL string or iterator of SQL strings to the database.
+    All given SQL will be run as one transaction and rolled back on error.
+  """
+  global db
+
+  if isinstance(sql, str):

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

How about always expecting a sequence here, eliminating the need for a type
switch? If the calling code usually writes only one item you might want to go
with variable arguments, to eliminate boilerplate there.

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

You didn't mind to address or reply to this comment. I still think we should
just always expect an iterable object here, getting rid of this silly type
switch.

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

It just makes the function nicer to use if you only want to write one piece of
SQL to the database and don't need to pass parameters.

`db.write("DELETE FROM BLAH")` instead of `db.write(("DELETE FROM BLAH"),)`.
Especially in the testing code it makes things a little cleaner.

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

There is only one call in your whole code where you pass a single string to the
write() method. This certainly doesn't justify to keep that logic.

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

Fair enough, I didn't realise that.
Done.

+    sql = [sql]
+
+  try:
+    # Commit the changes

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

You don't say? This comment doesn't add any information that the code below
doesn't already tells you on first glance.

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

Done.

+    cursor = db.cursor()
+    for query in sql:
+      if query:
+        [cursor.execute(s) for s in query.split(";") if s]

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

I'd rather use a for loop, than list expressions, if you don't need the result. 

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

Done.

+    db.commit()
+  except MySQLdb.Error:
+    # On error roll them back

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

Same as above.

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

Done.

+    if db:

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

How could a MySQLdb.Error be raised if db didn't exist?

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

I'm not sure but I seem to remember that this check prevents problems so I would
rather leave it in here.

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

If a random change you can't explain, fixes an issue you don't understand, this
isn't necessarily the correct or best way doing it. So figure out what's the
issue here, and if this is indeed the correct way of handling it add a comment
explaining why this non-obvious check is necessary.

Does the connection class maybe implement __nonzero__, returning False when the
connection was closed or rolled back after an error?

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

Well I agree with you of course but I guess the trouble is when there is a gap
of months between writing code and getting it reviewed it's hard to remember the
finer details.

I've removed it now and I can't see any problems. The tests still pass, I can
still submit data and I can still query it out again.

+      db.rollback()
+    raise
+  finally:
+    if cursor:

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

This will result in a NameError if the cursor doesn't exist yet. The code should
rather look like that:

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

Same above.

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

Done.

+      cursor.close()