- work from an "explicit is better than implicit" perspective

here and require a flag on update() to use parameter ordering
rather than table ordering.   more docs/changelog notes are needed

5 files changed, 123 insertions, 92 deletions

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index ef1ffbd94..768c1146a 100644
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -1258,12 +1258,15 @@ class BulkUpdate(BulkUD):
                 "Invalid expression type: %r" % key)
 
     def _do_exec(self):
-        if isinstance(self.values, (list, tuple)):
-            values = tuple((self._resolve_string_to_expr(k), v)
-                           for k, v in self.values)
-        else:
-            values = dict((self._resolve_string_to_expr(k), v)
-                          for k, v in self.values.items())
+
+        values = [
+            (self._resolve_string_to_expr(k), v)
+            for k, v in (
+                self.values.items() if hasattr(self.values, 'items')
+                else self.values)
+        ]
+        if not self.update_kwargs.get('preserve_parameter_order', False):
+            values = dict(values)
 
         update_stmt = sql.update(self.primary_table,
                                  self.context.whereclause, values,
diff --git a/lib/sqlalchemy/sql/crud.py b/lib/sqlalchemy/sql/crud.py
index 8b236cc56..67a8f09de 100644
--- a/lib/sqlalchemy/sql/crud.py
+++ b/lib/sqlalchemy/sql/crud.py
@@ -14,7 +14,6 @@ from .. import exc
 from . import elements
 import operator
 
-
 REQUIRED = util.symbol('REQUIRED', """
 Placeholder for the value within a :class:`.BindParameter`
 which is required to be present when the statement is passed
@@ -62,25 +61,15 @@ def _get_crud_params(compiler, stmt, **kw):
     _column_as_key, _getattr_col_key, _col_bind_name = \
         _key_getters_for_crud_column(compiler)
 
-    # We have to keep parameters' order if we are doing an update and the
-    # statement paramenters are a list or tuple of pairs.  It would also work
-    # without isupdate check, but adding it shortcircuits the boolean operation
-    # resulting in false for all inserts.
-    if stmt._preserve_parameter_order:
-        stmt_parameters = util.OrderedDict(stmt_parameters)
-        dict_type = util.OrderedDict
-    else:
-        dict_type = dict
-
     # if we have statement parameters - set defaults in the
     # compiled params
     if compiler.column_keys is None:
-        parameters = dict_type()
+        parameters = {}
     else:
-        parameters = dict_type((_column_as_key(key), REQUIRED)
-                               for key in compiler.column_keys
-                               if not stmt_parameters or
-                               key not in stmt_parameters)
+        parameters = dict((_column_as_key(key), REQUIRED)
+                          for key in compiler.column_keys
+                          if not stmt_parameters or
+                          key not in stmt_parameters)
 
     # create a list of column assignment clauses as tuples
     values = []
@@ -108,8 +97,7 @@ def _get_crud_params(compiler, stmt, **kw):
         _scan_cols(
             compiler, stmt, parameters,
             _getattr_col_key, _column_as_key,
-            _col_bind_name, check_columns, values, kw,
-            keep_order=stmt._preserve_parameter_order)
+            _col_bind_name, check_columns, values, kw)
 
     if parameters and stmt_parameters:
         check = set(parameters).intersection(
@@ -214,23 +202,24 @@ def _scan_insert_from_select_cols(
 
 def _scan_cols(
     compiler, stmt, parameters, _getattr_col_key,
-        _column_as_key, _col_bind_name, check_columns, values, kw, keep_order):
+        _column_as_key, _col_bind_name, check_columns, values, kw):
 
     need_pks, implicit_returning, \
         implicit_return_defaults, postfetch_lastrowid = \
         _get_returning_modifiers(compiler, stmt)
 
-    cols = stmt.table.columns
-
-    if keep_order:
-        # Order columns with parameters first, preserving their original order,
-        # and then the rest of the columns
-        keys = tuple(parameters.keys()) if parameters else tuple()
-        table_cols = tuple(cols)
-        cols = sorted(table_cols,
-                      key=(lambda x: keys.index(_getattr_col_key(x))
-                           if _getattr_col_key(x) in keys
-                           else len(keys) + table_cols.index(x)))
+    if stmt._parameter_ordering:
+        parameter_ordering = [
+            _column_as_key(key) for key in stmt._parameter_ordering
+        ]
+        ordered_keys = set(parameter_ordering)
+        cols = [
+            stmt.table.c[key] for key in parameter_ordering
+        ] + [
+            c for c in stmt.table.c if c.key not in ordered_keys
+        ]
+    else:
+        cols = stmt.table.columns
 
     for c in cols:
         col_key = _getattr_col_key(c)
diff --git a/lib/sqlalchemy/sql/dml.py b/lib/sqlalchemy/sql/dml.py
index 7243e56e1..435bfb2aa 100644
--- a/lib/sqlalchemy/sql/dml.py
+++ b/lib/sqlalchemy/sql/dml.py
@@ -15,7 +15,6 @@ from .elements import ClauseElement, _literal_as_text, Null, and_, _clone, \
 from .selectable import _interpret_as_from, _interpret_as_select, HasPrefixes
 from .. import util
 from .. import exc
-from sqlalchemy.sql import schema
 
 
 class UpdateBase(DialectKWArgs, HasPrefixes, Executable, ClauseElement):
@@ -28,31 +27,40 @@ class UpdateBase(DialectKWArgs, HasPrefixes, Executable, ClauseElement):
     _execution_options = \
         Executable._execution_options.union({'autocommit': True})
     _hints = util.immutabledict()
+    _parameter_ordering = None
     _prefixes = ()
 
     def _process_colparams(self, parameters):
         def process_single(p):
             if isinstance(p, (list, tuple)):
-                return dict((c.key, pval) for c, pval in zip(self.table.c, p))
+                return dict(
+                    (c.key, pval)
+                    for c, pval in zip(self.table.c, p)
+                )
             else:
                 return p
 
-        if parameters and isinstance(parameters, (list, tuple)):
-            p0 = parameters[0]
-            is_lt = isinstance(p0, (list, tuple))
-            # If it's an ordered dict in the form of value pairs return it
-            if is_lt and len(p0) == 2 and isinstance(p0[0], schema.Column):
-                return parameters, False, True
+        if self._preserve_parameter_order and parameters is not None:
+            if not isinstance(parameters, list) or \
+                    (parameters and not isinstance(parameters[0], tuple)):
+                raise ValueError(
+                    "When preserve_parameter_order is True, "
+                    "values() only accepts a list of 2-tuples")
+            self._parameter_ordering = [key for key, value in parameters]
+
+            return dict(parameters), False
 
-            if is_lt or isinstance(p0, dict):
-                if not self._supports_multi_parameters:
-                    raise exc.InvalidRequestError(
-                        "This construct does not support "
-                        "multiple parameter sets.")
+        if (isinstance(parameters, (list, tuple)) and parameters and
+                isinstance(parameters[0], (list, tuple, dict))):
 
-                return [process_single(p) for p in parameters], True, False
+            if not self._supports_multi_parameters:
+                raise exc.InvalidRequestError(
+                    "This construct does not support "
+                    "multiple parameter sets.")
 
-        return process_single(parameters), False, False
+            return [process_single(p) for p in parameters], True
+        else:
+            return process_single(parameters), False
 
     def params(self, *arg, **kw):
         """Set the parameters for the statement.
@@ -186,9 +194,8 @@ class ValuesBase(UpdateBase):
 
     def __init__(self, table, values, prefixes):
         self.table = _interpret_as_from(table)
-
-        self.parameters, self._has_multi_parameters, \
-            self._preserve_parameter_order = self._process_colparams(values)
+        self.parameters, self._has_multi_parameters = \
+            self._process_colparams(values)
         if prefixes:
             self._setup_prefixes(prefixes)
 
@@ -243,8 +250,11 @@ class ValuesBase(UpdateBase):
                                 {"name": "yet another name"},
                             ])
 
-         In the case of an :class:`.Update`
-         construct, only the single dictionary/tuple form is accepted,
+         In the case of an :class:`.Update`, an additional form is accepted
+         only when the :paramref:`.Update.preserve_parameter_order` parameter
+         is specified, which is a list of 2-tuples representing the SET
+         clause in order.   When this flag is not set,
+         only the single dictionary/tuple form is accepted,
          else an exception is raised.  It is also an exception case to
          attempt to mix the single-/multiple- value styles together,
          either through multiple :meth:`.ValuesBase.values` calls
@@ -320,14 +330,12 @@ class ValuesBase(UpdateBase):
             v = {}
 
         if self.parameters is None:
-            self.parameters, self._has_multi_parameters, \
-                self._preserve_parameter_order = self._process_colparams(v)
+            self.parameters, self._has_multi_parameters = \
+                self._process_colparams(v)
         else:
             if self._has_multi_parameters:
                 self.parameters = list(self.parameters)
-                p, self._has_multi_parameters, \
-                    self._preserve_parameter_order = self._process_colparams(v)
-
+                p, self._has_multi_parameters = self._process_colparams(v)
                 if not self._has_multi_parameters:
                     raise exc.ArgumentError(
                         "Can't mix single-values and multiple values "
@@ -336,8 +344,7 @@ class ValuesBase(UpdateBase):
                 self.parameters.extend(p)
             else:
                 self.parameters = self.parameters.copy()
-                p, self._has_multi_parameters, \
-                    self._preserve_parameter_order = self._process_colparams(v)
+                p, self._has_multi_parameters = self._process_colparams(v)
                 if self._has_multi_parameters:
                     raise exc.ArgumentError(
                         "Can't mix single-values and multiple values "
@@ -556,8 +563,8 @@ class Insert(ValuesBase):
             raise exc.InvalidRequestError(
                 "This construct already inserts value expressions")
 
-        self.parameters, self._has_multi_parameters, \
-            self._preserve_parameter_order = self._process_colparams(
+        self.parameters, self._has_multi_parameters = \
+            self._process_colparams(
                 dict((_column_as_key(n), Null()) for n in names))
 
         self.select_names = names
@@ -590,6 +597,7 @@ class Update(ValuesBase):
                  prefixes=None,
                  returning=None,
                  return_defaults=False,
+                 preserve_parameter_order=False,
                  **dialect_kw):
         """Construct an :class:`.Update` object.
 
@@ -652,6 +660,14 @@ class Update(ValuesBase):
           be available in the dictionary returned from
           :meth:`.ResultProxy.last_updated_params`.
 
+        :param preserve_parameter_order: if True, the update statement is
+          expected to receive paramters **only** via the :meth:`.Update.values`
+          method, and they must be passed as a list of key/value pairs.
+          The ultimate UPDATE statement will emit the SET clause maintaining
+          this order.
+
+          .. versionadded:: 1.0.10
+
         If both ``values`` and compile-time bind parameters are present, the
         compile-time bind parameters override the information specified
         within ``values`` on a per-key basis.
@@ -693,6 +709,7 @@ class Update(ValuesBase):
 
 
         """
+        self._preserve_parameter_order = preserve_parameter_order
         ValuesBase.__init__(self, table, values, prefixes)
         self._bind = bind
         self._returning = returning
diff --git a/test/orm/test_query.py b/test/orm/test_query.py
index 458637453..3fe7948da 100644
--- a/test/orm/test_query.py
+++ b/test/orm/test_query.py
@@ -3907,8 +3907,8 @@ class SessionBindTest(QueryTest):
         User = self.classes.User
         session = Session()
 
-        # Do an update using unordered dict and check that the parametes used
-        # are unordered
+        # Do an update using unordered dict and check that the parameters used
+        # are ordered in table order
         with self._assert_bind_args(session) as mock_args:
             session.query(User).filter(User.id == 15).update(
                 {'name': 'foob', 'id': 123})
@@ -3920,34 +3920,38 @@ class SessionBindTest(QueryTest):
         User = self.classes.User
         session = Session()
 
-        # Do an update using an ordered dict and check that the parametes used
-        # are unordered
+        # Do an update using ordered dict and check that the parameters used
+        # are ordered in table order
         with self._assert_bind_args(session) as mock_args:
             session.query(User).filter(User.id == 15).update(
                 util.OrderedDict((('name', 'foob'), ('id', 123))))
             params_type = type(mock_args.mock_calls[0][2]['clause'].parameters)
             assert params_type is dict
 
-    def test_bulk_update_with_order(self):
+    def test_bulk_update_preserve_parameter_order(self):
         User = self.classes.User
         session = Session()
 
         # Do update using a tuple and check that order is preserved
         with self._assert_bind_args(session) as mock_args:
             session.query(User).filter(User.id == 15).update(
-                (('id', 123), ('name', 'foob')))
-            cols = [c[0].name for c
-                    in mock_args.mock_calls[0][2]['clause'].parameters]
-            assert ['id', 'name'] == cols
+                (('id', 123), ('name', 'foob')),
+                update_args={"preserve_parameter_order": True})
+            cols = [c.key
+                    for c in mock_args.mock_calls[0][2]
+                    ['clause']._parameter_ordering]
+            eq_(['id', 'name'], cols)
 
         # Now invert the order and use a list instead, and check that order is
         # also preserved
         with self._assert_bind_args(session) as mock_args:
             session.query(User).filter(User.id == 15).update(
-                [('id', 123), ('name', 'foob')])
-            cols = [c[0].name for c
-                    in mock_args.mock_calls[0][2]['clause'].parameters]
-            assert ['id', 'name'] == cols
+                [('id', 123), ('name', 'foob')],
+                update_args={"preserve_parameter_order": True})
+            cols = [c.key
+                    for c in mock_args.mock_calls[0][2]
+                    ['clause']._parameter_ordering]
+            eq_(['id', 'name'], cols)
 
     def test_bulk_delete_no_sync(self):
         User = self.classes.User
diff --git a/test/sql/test_update.py b/test/sql/test_update.py
index 059c3ad6d..0b01071a8 100644
--- a/test/sql/test_update.py
+++ b/test/sql/test_update.py
@@ -183,13 +183,15 @@ class UpdateTest(_UpdateFromTestBase, fixtures.TablesTest, AssertsCompiledSQL):
             'mytable.myid = hoho(:hoho_1) AND '
             'mytable.name = :param_2 || mytable.name || :param_3')
 
-    def test_update_12(self):
+    def test_update_ordered_parameters_1(self):
         table1 = self.tables.mytable
 
-        # Confirm that we can pass values as tuple value pairs
-        values = (
+        # Confirm that we can pass values as list value pairs
+        # note these are ordered *differently* from table.c
+        values = [
+            (table1.c.name, table1.c.name + 'lala'),
             (table1.c.myid, func.do_stuff(table1.c.myid, literal('hoho'))),
-            (table1.c.name, table1.c.name + 'lala'))
+        ]
         self.assert_compile(
             update(
                 table1,
@@ -197,22 +199,26 @@ class UpdateTest(_UpdateFromTestBase, fixtures.TablesTest, AssertsCompiledSQL):
                     table1.c.name == literal('foo') +
                     table1.c.name +
                     literal('lala')),
+                preserve_parameter_order=True,
                 values=values),
             'UPDATE mytable '
             'SET '
-            'myid=do_stuff(mytable.myid, :param_1), '
-            'name=(mytable.name || :name_1) '
+            'name=(mytable.name || :name_1), '
+            'myid=do_stuff(mytable.myid, :param_1) '
             'WHERE '
             'mytable.myid = hoho(:hoho_1) AND '
             'mytable.name = :param_2 || mytable.name || :param_3')
 
-    def test_update_13(self):
+    def test_update_ordered_parameters_2(self):
         table1 = self.tables.mytable
 
         # Confirm that we can pass values as list value pairs
+        # note these are ordered *differently* from table.c
         values = [
-            (table1.c.myid, func.do_stuff(table1.c.myid, literal('hoho'))),
-            (table1.c.name, table1.c.name + 'lala')]
+            (table1.c.name, table1.c.name + 'lala'),
+            ('description', 'some desc'),
+            (table1.c.myid, func.do_stuff(table1.c.myid, literal('hoho')))
+        ]
         self.assert_compile(
             update(
                 table1,
@@ -220,19 +226,31 @@ class UpdateTest(_UpdateFromTestBase, fixtures.TablesTest, AssertsCompiledSQL):
                     table1.c.name == literal('foo') +
                     table1.c.name +
                     literal('lala')),
-                values=values),
+                preserve_parameter_order=True).values(values),
             'UPDATE mytable '
             'SET '
-            'myid=do_stuff(mytable.myid, :param_1), '
-            'name=(mytable.name || :name_1) '
+            'name=(mytable.name || :name_1), '
+            'description=:description, '
+            'myid=do_stuff(mytable.myid, :param_1) '
             'WHERE '
             'mytable.myid = hoho(:hoho_1) AND '
             'mytable.name = :param_2 || mytable.name || :param_3')
 
-    def test_update_14(self):
+    def test_update_preserve_order_reqs_listtups(self):
+        table1 = self.tables.mytable
+        testing.assert_raises_message(
+            ValueError,
+            "When preserve_parameter_order is True, values\(\) "
+            "only accepts a list of 2-tuples",
+            table1.update(preserve_parameter_order=True).values,
+            {"description": "foo", "name": "bar"}
+        )
+
+    def test_update_ordereddict(self):
         table1 = self.tables.mytable
 
-        # Confirm that ordered dicts are treated as normal dicts
+        # Confirm that ordered dicts are treated as normal dicts,
+        # columns sorted in table order
         values = util.OrderedDict((
             (table1.c.name, table1.c.name + 'lala'),
             (table1.c.myid, func.do_stuff(table1.c.myid, literal('hoho')))))