launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/profile-sql-bind-values into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/profile-sql-bind-values/+merge/48872

== Summary ==

SQL statements included with OOPs reports and ++profile++ reports now have their bind variables included in the log.
The option LP_DEBUG_SQL_VALUES is also removed since it is now obsolete.

== Implementation ==

This branch was landed earlier but had to be rolled back. The issue was a single mode of failure: '%' characters in SQL statements of the form "... LIKE '%'..." needed to be quoted (replaced with '%%') or else the string format operation used to poke in the bind variable values would fail. The quoting can be achieved with a single regexp search and replace:

quoted_statement = re.sub('%(\W)', r"%%\1", statement)

== Tests ==

Re-run failing tests:

bin/test -vvt nascentupload
bin/test -vvt test_bugtask_search

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/webapp/adapter.py


-- 
https://code.launchpad.net/~wallyworld/launchpad/profile-sql-bind-values/+merge/48872
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/profile-sql-bind-values into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
--- lib/canonical/launchpad/webapp/adapter.py	2011-02-02 09:47:21 +0000
+++ lib/canonical/launchpad/webapp/adapter.py	2011-02-08 04:04:11 +0000
@@ -24,7 +24,10 @@
     QueryCanceledError,
     )
 import pytz
-from storm.database import register_scheme
+from storm.database import (
+    Connection,
+    register_scheme,
+    )
 from storm.databases.postgres import (
     Postgres,
     PostgresConnection,
@@ -598,8 +601,6 @@
 
     def __init__(self):
         self._debug_sql = bool(os.environ.get('LP_DEBUG_SQL'))
-        self._debug_sql_bind_values = bool(
-            os.environ.get('LP_DEBUG_SQL_VALUES'))
         self._debug_sql_extra = bool(os.environ.get('LP_DEBUG_SQL_EXTRA'))
 
     def connection_raw_execute(self, connection, raw_cursor,
@@ -607,13 +608,13 @@
         if self._debug_sql_extra:
             traceback.print_stack()
             sys.stderr.write("." * 70 + "\n")
+        param_strings = list(Connection.to_database(params))
+        # We need to ensure % symbols used for LIKE statements etc are
+        # properly quoted or else the string format operation will fail.
+        quoted_statement = re.sub('%(\W)', r"%%\1", statement)
+        statement_with_values = quoted_statement % tuple(param_strings)
         if self._debug_sql or self._debug_sql_extra:
-            stmt_to_log = statement
-            if self._debug_sql_bind_values:
-                from storm.database import Connection
-                param_strings = Connection.to_database(params)
-                stmt_to_log = statement % tuple(param_strings)
-            sys.stderr.write(stmt_to_log + "\n")
+            sys.stderr.write(statement_with_values + "\n")
             sys.stderr.write("-" * 70 + "\n")
         # store the last executed statement as an attribute on the current
         # thread
@@ -622,7 +623,7 @@
         if request_starttime is None:
             return
         action = get_request_timeline(get_current_browser_request()).start(
-            'SQL-%s' % connection._database.name, statement)
+            'SQL-%s' % connection._database.name, statement_with_values)
         connection._lp_statement_action = action
 
     def connection_raw_execute_success(self, connection, raw_cursor,