- major simplification of _collect_update_commands. in particular,

we only call upon the history API fully for primary key columns.
We also now skip the whole step of looking at PK columns and using
any history at all if no net changes are detected on the object.

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 1e129185705a5fd5d9c1950b61e8e2463391bec5..14dc5d7f87e59557945956e4468d4339230f7652 100644 (file)
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -1893,6 +1893,19 @@ class Mapper(InspectionAttr):
 
     """
 
+    @_memoized_configured_property
+    def _propkey_to_col(self):
+        return dict(
+            (
+                table,
+                dict(
+                    (self._columntoproperty[col].key, col)
+                    for col in columns
+                )
+            )
+            for table, columns in self._cols_by_table.items()
+        )
+
     @_memoized_configured_property
     def _col_to_propkey(self):
         return dict(

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index d511c0816c95b06129e31bc796c3e107b4b80149..228cfef3aab10e18afabb269ccb77b64e3576ad9 100644 (file)
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -304,96 +304,70 @@ def _collect_update_commands(base_mapper, uowtransaction,
         params = {}
         value_params = {}
 
-        hasdata = hasnull = False
+        propkey_to_col = mapper._propkey_to_col[table]
 
-        for col in mapper._cols_by_table[table]:
-            if col is mapper.version_id_col:
-                params[col._label] = \
-                    mapper._get_committed_state_attr_by_column(
-                        row_switch or state,
-                        row_switch and row_switch.dict
-                        or state_dict,
-                        col)
+        for propkey in set(propkey_to_col).intersection(state.committed_state):
+            value = state_dict[propkey]
+            col = propkey_to_col[propkey]
 
-                prop = mapper._columntoproperty[col]
-                history = state.manager[prop.key].impl.get_history(
-                    state, state_dict, attributes.PASSIVE_NO_INITIALIZE
-                )
-                if history.added:
-                    params[col.key] = history.added[0]
-                    hasdata = True
+            if not state.manager[propkey].impl.is_equal(
+                    value, state.committed_state[propkey]):
+                if isinstance(value, sql.ClauseElement):
+                    value_params[col] = value
                 else:
-                    if mapper.version_id_generator is not False:
-                        val = mapper.version_id_generator(params[col._label])
-                        params[col.key] = val
-
-                        # HACK: check for history, in case the
-                        # history is only
-                        # in a different table than the one
-                        # where the version_id_col is.
-                        for prop in mapper._columntoproperty.values():
-                            history = (
-                                state.manager[prop.key].impl.get_history(
-                                    state, state_dict,
-                                    attributes.PASSIVE_NO_INITIALIZE))
-                            if history.added:
-                                hasdata = True
+                    params[col.key] = value
+
+        if mapper.version_id_col is not None:
+            col = mapper.version_id_col
+            params[col._label] = \
+                mapper._get_committed_state_attr_by_column(
+                    row_switch if row_switch else state,
+                    row_switch.dict if row_switch else state_dict,
+                    col)
+
+            if col.key not in params and \
+                    mapper.version_id_generator is not False:
+                val = mapper.version_id_generator(params[col._label])
+                params[col.key] = val
+
+        if not (params or value_params):
+            continue
+
+        pk_params = {}
+        for col in pks:
+            propkey = mapper._columntoproperty[col].key
+            history = state.manager[propkey].impl.get_history(
+                state, state_dict, attributes.PASSIVE_OFF)
+
+            if row_switch and not history.deleted and history.added:
+                # row switch present.  convert a row that thought
+                # it would be an INSERT into an UPDATE, by removing
+                # the PK value from the SET clause and instead putting
+                # it in the WHERE clause.
+                del params[col.key]
+                pk_params[col._label] = history.added[0]
+            elif history.added:
+                # we're updating the PK value.
+                assert history.deleted, (
+                    "New PK value without an old one not "
+                    "possible for an UPDATE")
+                # check if an UPDATE of the PK value
+                # has already occurred as a result of ON UPDATE CASCADE.
+                # If so, use the new value to locate the row.
+                if ("pk_cascaded", state, col) in uowtransaction.attributes:
+                    pk_params[col._label] = history.added[0]
+                else:
+                    # else, use the old value to locate the row
+                    pk_params[col._label] = history.deleted[0]
             else:
-                prop = mapper._columntoproperty[col]
-                history = state.manager[prop.key].impl.get_history(
-                    state, state_dict,
-                    attributes.PASSIVE_OFF if col in pks else
-                    attributes.PASSIVE_NO_INITIALIZE)
-                if history.added:
-                    if isinstance(history.added[0],
-                                  sql.ClauseElement):
-                        value_params[col] = history.added[0]
-                    else:
-                        value = history.added[0]
-                        params[col.key] = value
-
-                    if col in pks:
-                        if history.deleted and \
-                                not row_switch:
-                            # if passive_updates and sync detected
-                            # this was a  pk->pk sync, use the new
-                            # value to locate the row, since the
-                            # DB would already have set this
-                            if ("pk_cascaded", state, col) in \
-                                    uowtransaction.attributes:
-                                value = history.added[0]
-                                params[col._label] = value
-                            else:
-                                # use the old value to
-                                # locate the row
-                                value = history.deleted[0]
-                                params[col._label] = value
-                            hasdata = True
-                        else:
-                            # row switch logic can reach us here
-                            # remove the pk from the update params
-                            # so the update doesn't
-                            # attempt to include the pk in the
-                            # update statement
-                            del params[col.key]
-                            value = history.added[0]
-                            params[col._label] = value
-                        if value is None:
-                            hasnull = True
-                    else:
-                        hasdata = True
-                elif col in pks:
-                    value = history.unchanged[0]
-                    if value is None:
-                        hasnull = True
-                    params[col._label] = value
+                pk_params[col._label] = history.unchanged[0]
 
-        if hasdata:
-            if hasnull:
+        if params or value_params:
+            if None in pk_params.values():
                 raise orm_exc.FlushError(
-                    "Can't update table "
-                    "using NULL for primary "
+                    "Can't update table using NULL for primary "
                     "key value")
+            params.update(pk_params)
             update.append((state, state_dict, params, mapper,
                            connection, value_params))
     return update

diff --git a/test/orm/test_dynamic.py b/test/orm/test_dynamic.py
index bc47ba3f3c23d039d8483cae03640ff50437c13f..950ff19539b50ad4b73195cf63f669b8d45f0cd4 100644 (file)
--- a/test/orm/test_dynamic.py
+++ b/test/orm/test_dynamic.py
@@ -509,10 +509,6 @@ class UOWTest(
         self.assert_sql_execution(
             testing.db,
             sess.flush,
-            CompiledSQL(
-                "SELECT users.id AS users_id, users.name AS users_name "
-                "FROM users WHERE users.id = :param_1",
-                lambda ctx: [{"param_1": u1_id}]),
             CompiledSQL(
                 "SELECT addresses.id AS addresses_id, addresses.email_address "
                 "AS addresses_email_address FROM addresses "
@@ -523,7 +519,11 @@ class UOWTest(
                 "UPDATE addresses SET user_id=:user_id WHERE addresses.id = "
                 ":addresses_id",
                 lambda ctx: [{'addresses_id': a2_id, 'user_id': None}]
-            )
+            ),
+            CompiledSQL(
+                "SELECT users.id AS users_id, users.name AS users_name "
+                "FROM users WHERE users.id = :param_1",
+                lambda ctx: [{"param_1": u1_id}]),
         )
 
     def test_rollback(self):

diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py
index 122fe2514bf20f5975eda2d9fc13ffe3c475b973..374a77237cc72e29c52824bf83ac2a84e28c4d41 100644 (file)
--- a/test/orm/test_unitofworkv2.py
+++ b/test/orm/test_unitofworkv2.py
@@ -1277,6 +1277,8 @@ class RowswitchAccountingTest(fixtures.MappedTest):
         old = attributes.get_history(p3, 'child')[2][0]
         assert old in sess
 
+        # essentially no SQL should emit here,
+        # because we've replaced the row with another identical one
         sess.flush()
 
         assert p3.child._sa_instance_state.session_id == sess.hash_key