launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/unicode-sql-log into lp:launchpad.

Commit message:
Log SQL statements as Unicode to avoid confusing page rendering when the visible_render_time flag is on.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1617336 in Launchpad itself: "GitRef:+index oopses when recent commits include non-ASCII author name and visible_render_time is on"
  https://bugs.launchpad.net/launchpad/+bug/1617336

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/unicode-sql-log/+merge/306089

Log SQL statements as Unicode to avoid confusing page rendering when the visible_render_time flag is on.

I think our SQL statements are (nearly?) always UTF-8, but even if I'm wrong, decoding with errors='replace' should ensure that this is safe.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/unicode-sql-log into lp:launchpad.

=== modified file 'lib/lp/services/webapp/adapter.py'
--- lib/lp/services/webapp/adapter.py	2015-07-08 16:05:11 +0000
+++ lib/lp/services/webapp/adapter.py	2016-09-19 12:14:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -151,7 +151,7 @@
 
     def afterCompletion(self, txn):
         action = get_request_timeline(get_current_browser_request()).start(
-            "SQL-nostore", 'Transaction completed, status: %s' % txn.status)
+            u"SQL-nostore", u'Transaction completed, status: %s' % txn.status)
         action.finish()
 
 
@@ -653,6 +653,9 @@
         if params:
             statement_to_log = raw_cursor.mogrify(
                 statement, tuple(connection.to_database(params)))
+        if isinstance(statement_to_log, bytes):
+            statement_to_log = statement_to_log.decode(
+                'UTF-8', errors='replace')
         # Record traceback to log, if requested.
         print_traceback = self._debug_sql_extra
         log_sql = getattr(_local, 'sql_logging', None)
@@ -690,11 +693,11 @@
                 # SQL execution.
                 connection._lp_statement_info = (
                     int(time() * 1000),
-                    'SQL-%s' % connection._database.name,
+                    u'SQL-%s' % connection._database.name,
                     statement_to_log)
             return
         action = get_request_timeline(get_current_browser_request()).start(
-            'SQL-%s' % connection._database.name, statement_to_log)
+            u'SQL-%s' % connection._database.name, statement_to_log)
         connection._lp_statement_action = action
 
     def connection_raw_execute_success(self, connection, raw_cursor,

=== modified file 'lib/lp/services/webapp/doc/test_adapter.txt'
--- lib/lp/services/webapp/doc/test_adapter.txt	2013-06-20 05:50:00 +0000
+++ lib/lp/services/webapp/doc/test_adapter.txt	2016-09-19 12:14:03 +0000
@@ -337,7 +337,7 @@
 the database.
 
     >>> get_request_statements()
-    [(0, ..., 'SQL-main-master', 'SELECT 2', ...)]
+    [(0, ..., u'SQL-main-master', u'SELECT 2', ...)]
 
 
 When a RequestExpired exception is raised, the current