consider existing bindparam()s in an INSERT/UPDATE not part of column_keys but are named after a column?

[Mike Bayer]

i.e.

from sqlalchemy import *
from sqlalchemy.sql import table, column

t = table('foo', column('x'), column('y'))

u = t.update().where(t.c.x==bindparam('x'))

print u.compile(column_keys=['y']('x',))

would produce:

UPDATE foo SET y=:y WHERE foo.x = :x

this is actually tricky since visit_update() handles the SET clause before the WHERE. Reversing the order of those so that we know what was named in the WHERE then breaks the bind param ordering on positional bind sets. There's ways to get around this but all of which would complicate the internals of visit_update().

[Mike Bayer]

Otherwise consider adding an exception when a bindparam() is detected with a column name as a key.

[Mike Bayer]

we're going to go with exceptions. this is hard.

here's a patch. consider moving the check to be unconditional, since as it is, it allows a compile() to occur, but not any compile() within an execute() that actually sets up those bind parameter names, so its sort of useless.

Index: lib/sqlalchemy/sql/compiler.py
===================================================================
--- lib/sqlalchemy/sql/compiler.py      (revision 6427)
+++ lib/sqlalchemy/sql/compiler.py      (working copy)
@@ -475,12 +475,19 @@

     def visit_bindparam(self, bindparam, **kwargs):
         name = self._truncate_bindparam(bindparam)
+
         if name in self.binds:
+
+            if (self.isinsert or self.isupdate) and ((self.column_keys and name in self.column_keys) \
+                    or (not self.statement.parameters and name in self.statement.table.c)):
+                raise exc.CompileError("XXXFIXME This will be an error message referring to your usage of bind parameter '%s' during INSERT or UPDATE." % bindparam.key)
+
             existing = self.binds[name](name)
             if existing is not bindparam and (existing.unique or bindparam.unique):
                 raise exc.CompileError(
                         "Bind parameter '%s' conflicts with unique bind parameter of the same name" % bindparam.key
                     )
+            
         self.binds[bindparam.key](bindparam.key) = self.binds[name](name) = bindparam
         return self.bindparam_string(name)

@@ -796,7 +803,9 @@

         if stmt.parameters is not None:
             for k, v in stmt.parameters.iteritems():
-                parameters.setdefault(sql._column_as_key(k), v)
+                v2 = parameters.setdefault(sql._column_as_key(k), v)
+                if v2 is not v and not sql._is_literal(v):
+                    raise exc.CompileError("Expression '%s' would be replaced by execution-time literal" % v)

         # create a list of column assignment clauses as tuples
         values = [things to test:

{{{
from sqlalchemy import *
from sqlalchemy.sql import table, column

t = table('foo', column('x'), column('y'))

u = t.update().where(t.c.x==bindparam('x'))

print u.compile()
print u.values(x=7).compile()
print u.values(y=7).compile()
print u.values(x=7).compile(column_keys=['x', 'y'](]

here's)) print u.values(x=3 + bindparam('x')).compile(column_keys='y') print u.compile(column_keys='y')

i = t.insert().values(x=3 + bindparam('x')) print i.compile() print i.compile(column_keys='y')

i = t.insert().values(x=3 + bindparam('x2')) print i.compile(column_keys=[i.compile() print i.compile(column_keys='x', 'y' print)) print i.compile(column_keys='y')

}}}

[Mike Bayer]

OK, here's another patch that costs us 5 function calls on the update/insert tests, doesn't blow up zoomark, allows all those tests to work in a nonsurprising way:

Index: lib/sqlalchemy/sql/compiler.py
===================================================================
--- lib/sqlalchemy/sql/compiler.py  (revision 6427)
+++ lib/sqlalchemy/sql/compiler.py  (working copy)
@@ -776,12 +776,17 @@
         self.prefetch = [        self.returning = [](]
)

+        bind_names = set()
+        visitors.traverse(stmt, {}, {'bindparam':lambda x:bind_names.add(x.key)})
+        if stmt.parameters:
+            bind_names.update(stmt.parameters)
+            
         # no parameters in the statement, no parameters in the
         # compiled params - return binds for all columns
         if self.column_keys is None and stmt.parameters is None:
             return [                        (c, self._create_crud_bind_param(c, None, required=True)) 
-                        for c in stmt.table.columns
+                        for c in stmt.table.columns if c.key not in bind_names
                     ](
)

         required = object()
@@ -792,7 +797,7 @@
             parameters = {}
         else:
             parameters = dict((sql._column_as_key(key), required)
-                              for key in self.column_keys)
+                              for key in self.column_keys if key not in bind_names)

         if stmt.parameters is not None:
             for k, v in stmt.parameters.iteritems():

[Mike Bayer]

the latter worked out really well. a43a0e8b68c28b72404e85e4b4e8999443dd3fc5.

[Mike Bayer]

Reopening for two reasons.

1. we need 5+ method calls for every insert() and update() even though this is an extremely unusual case ?

2. it doesn't work completely:

   table with "x" and "y":

   t.insert().values(x=bindparam('y'), y=5)
   

   produces:

   INSERT INTO foo (x, y) VALUES (:y, :y)
   

the "raise an error" approach is fine. the compilation is impacted by the specific columns you name in execute(), but so what - those are still structural and not data driven. otherwise you can get unpredictable behavior as above.

[Mike Bayer]

most of those test cases are still possible - just a few more, including the flat out wrong ones, now raise in ebe7f7b15e03a30ae14263f16fed8a18c35eebd9.

[Mike Bayer]

Removing milestone: 0.6.0 (automated comment)