Merge lp:~stub/storm/bug-322206 into lp:storm

[Stuart Bishop]

[Stuart Bishop (stub)]

Fix for Bug #322206 as discussed in the bug report.

This branch makes psycopg2 2.0.7 or higher required for Storm PostgreSQL support.

This branch removes the quoting workaround required for earlier versions of psycopg, where Storm inserted an 'E' before string representations to ensure strings are always validly quoted.

[James Henstridge (jamesh)]

Looks good to me. Only two points:

1. Is distutils.version part of the public API of distutils, or an implementation detail?

2. If we are going to require 2.0.7, we can also update PostgresTimeoutTracer to check for QueryCanceledError rather than checking the exception message. That doesn't need to happen in this branch though.

review: Approve

[Stuart Bishop (stub)]

On Mon, Jun 29, 2009 at 5:32 PM, James Henstridge<email address hidden> wrote:
> Review: Approve
> Looks good to me.  Only two points:
>
> 1. Is distutils.version part of the public API of distutils, or an implementation detail?

Its been around since at least Python 2.3, is not using private naming conventions and contains more documentation than code. Its not documented in the reference manual, but not that much of distutils is. I'd say it is public enough.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

[Thomas Herve (therve)]

A nice cleanup. Two comments:

1) RawStrVariable import can now be removed.

2) The bug links to edge, it may be nicer to link to production lp.

+1!

review: Approve

[Stuart Bishop (stub)]

> A nice cleanup. Two comments:
>
> 1) RawStrVariable import can now be removed.
>
> 2) The bug links to edge, it may be nicer to link to production lp.

I've fixed both of these points. Thanks!

[lp:~stub/storm/bug-322206]

310. By Stuart Bishop on 2009-07-06

Fix bug link and remove unnecessary import

=== modified file 'storm/databases/postgres.py'
--- storm/databases/postgres.py	2009-05-15 08:43:21 +0000
+++ storm/databases/postgres.py	2009-07-06 13:20:11 +0000
@@ -1,5 +1,5 @@
 #
-# Copyright (c) 2006, 2007 Canonical
+# Copyright (c) 2006-2009 Canonical
 #
 # Written by Gustavo Niemeyer <[e-mail]>
 #
@@ -19,13 +19,23 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 from datetime import datetime, date, time, timedelta
+from distutils.version import LooseVersion
 
 from storm.databases import dummy
 
+# PostgreSQL support in Storm requires psycopg2 2.0.7 or greater.
+# Earlier versions of psyocpg2 contain data loss bugs.
+# See https://bugs.launchpad.net/storm/+bug/322206 for more details.
+REQUIRED_PSYCOPG2_VERSION = LooseVersion('2.0.7')
+PSYCOPG2_VERSION = None
 try:
     import psycopg2
-    import psycopg2.extensions
-except:
+    PSYCOPG2_VERSION = LooseVersion(psycopg2.__version__.split(' ')[0])
+    if PSYCOPG2_VERSION < REQUIRED_PSYCOPG2_VERSION:
+        psycopg2 = dummy
+    else:
+        import psycopg2.extensions
+except ImportError:
     psycopg2 = dummy
 
 from storm.expr import (
@@ -33,7 +43,7 @@
     Sequence, Like, SQLToken, COLUMN, COLUMN_NAME, COLUMN_PREFIX, TABLE,
     compile, compile_select, compile_insert, compile_set_expr, compile_like,
     compile_sql_token)
-from storm.variables import Variable, ListVariable, RawStrVariable
+from storm.variables import Variable, ListVariable
 from storm.database import Database, Connection, Result
 from storm.exceptions import (
     install_exceptions, DatabaseError, DatabaseModuleError, InterfaceError,
@@ -193,24 +203,6 @@
     return compile_sql_token(compile, expr, state)
 
 
-def compile_str_variable_with_E(compile, variable, state):
-    """Include an E just before the placeholder of string variables.
-
-    PostgreSQL 8.2 will issue a warning without it, and psycopg
-    will use the plain '' rather than E''.  The problem is being
-    tracked at the following URL::
-
-        http://www.initd.org/tracker/psycopg/ticket/202
-
-    """
-    state.parameters.append(variable)
-    if type(variable.get(to_db=True)) is str:
-        return "E?"
-    return "?"
-
-psycopg_needs_E = None
-
-
 class PostgresResult(Result):
 
     def get_insert_identity(self, primary_key, primary_variables):
@@ -314,7 +306,9 @@
 
     def __init__(self, uri):
         if psycopg2 is dummy:
-            raise DatabaseModuleError("'psycopg2' module not found")
+            raise DatabaseModuleError(
+                "'psycopg2' >= %s not found. Found %s."
+                % (REQUIRED_PSYCOPG2_VERSION, PSYCOPG2_VERSION))
         self._dsn = make_dsn(uri)
         isolation = uri.options.get("isolation", "serializable")
         isolation_mapping = {
@@ -340,29 +334,8 @@
                 cursor.execute("SHOW server_version_num")
             except psycopg2.ProgrammingError:
                 self._version = 0
-                raw_connection.rollback()
             else:
                 self._version = int(cursor.fetchone()[0])
-
-            # This will conditionally change the compilation of binary
-            # variables (RawStrVariable) to preceed the placeholder with an
-            # 'E', if psycopg isn't doing it by itself.
-            #
-            # The "failing" code path isn't unittested because that
-            # would depend on a working psycopg version, or in an older
-            # postgresql version.  Both branches were manually tested
-            # for correctness at some point.
-            global psycopg_needs_E
-            try:
-                # If psycopg behaves correctly, this will break trying
-                # to run a query with EE''.
-                cursor.execute("SELECT E%s", (psycopg2.Binary(""),))
-            except psycopg2.ProgrammingError:
-                psycopg_needs_E = False
-            else:
-                psycopg_needs_E = True
-                compile.when(RawStrVariable)(compile_str_variable_with_E)
-
             raw_connection.rollback()
 
         raw_connection.set_client_encoding("UTF8")