Ensure attribute keys used for bulk update pk set

Fixed bug in :meth:`.Session.bulk_update_mappings` where an alternate-named
primary key attribute would not track properly into the UPDATE statement.

Change-Id: I33e9140f45827772768fa548adcfeb4dbfc2208d
Fixes: #3849

diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index 8a061cdac13d2bfd7b20cf6f59d67b82b2fe2579..4b28e40a71df4127af1094c6dc1050b2335f2240 100644 (file)
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -18,6 +18,14 @@
 .. changelog::
     :version: 1.0.16
 
+    .. change::
+        :tags: bug, orm
+        :tickets: 3849
+        :versions: 1.1.4
+
+        Fixed bug in :meth:`.Session.bulk_update_mappings` where an alternate-named
+        primary key attribute would not track properly into the UPDATE statement.
+
     .. change::
         :tags: bug, mssql
         :tickets: 3810

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 60848097c65b8b65df7418f864a95417c41c478c..87f2a24b022417e6a268900c511110faec1437e3 100644 (file)
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -2019,6 +2019,16 @@ class Mapper(InspectionAttr):
             for table, pks in self._pks_by_table.items()
         )
 
+    @_memoized_configured_property
+    def _pk_attr_keys_by_table(self):
+        return dict(
+            (
+                table,
+                frozenset([self._columntoproperty[col].key for col in pks])
+            )
+            for table, pks in self._pks_by_table.items()
+        )
+
     @_memoized_configured_property
     def _server_default_cols(self):
         return dict(

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index 2bc189c1d3843f9f71fc503adc602c576193a0eb..bf51a2a833003198e297a03038f284f7412183ac 100644 (file)
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -522,7 +522,7 @@ def _collect_update_commands(
                 (propkey_to_col[propkey]._label, state_dict.get(propkey))
                 for propkey in
                 set(propkey_to_col).
-                intersection(mapper._pk_keys_by_table[table])
+                intersection(mapper._pk_attr_keys_by_table[table])
             )
         else:
             pk_params = {}

diff --git a/test/orm/test_bulk.py b/test/orm/test_bulk.py
index 0a51a5ad3500f8796d790199403496b1e49deac0..8a8fd004de4c93d0e467f3c139aebc96e23afc95 100644 (file)
--- a/test/orm/test_bulk.py
+++ b/test/orm/test_bulk.py
@@ -232,6 +232,109 @@ class BulkUDPostfetchTest(BulkTest, fixtures.MappedTest):
         eq_(a1.y, 2)
 
 
+class BulkUDTestAltColKeys(BulkTest, fixtures.MappedTest):
+    @classmethod
+    def define_tables(cls, metadata):
+        Table(
+            'people_keys', metadata,
+            Column(
+                'person_id', Integer,
+                primary_key=True, key='id'),
+            Column('name', String(50), key='personname'))
+
+        Table(
+            'people_attrs', metadata,
+            Column(
+                'person_id', Integer,
+                primary_key=True),
+            Column('name', String(50)))
+
+        Table(
+            'people_both', metadata,
+            Column(
+                'person_id', Integer,
+                primary_key=True, key="id_key"),
+            Column('name', String(50), key='name_key'))
+
+    @classmethod
+    def setup_classes(cls):
+        class PersonKeys(cls.Comparable):
+            pass
+
+        class PersonAttrs(cls.Comparable):
+            pass
+
+        class PersonBoth(cls.Comparable):
+            pass
+
+    @classmethod
+    def setup_mappers(cls):
+        PersonKeys, PersonAttrs, PersonBoth = cls.classes(
+            "PersonKeys", "PersonAttrs", "PersonBoth")
+        people_keys, people_attrs, people_both = cls.tables(
+            "people_keys", "people_attrs", "people_both")
+
+        mapper(PersonKeys, people_keys)
+        mapper(PersonAttrs, people_attrs, properties={
+            'id': people_attrs.c.person_id,
+            'personname': people_attrs.c.name
+        })
+
+        mapper(PersonBoth, people_both, properties={
+            'id': people_both.c.id_key,
+            'personname': people_both.c.name_key
+        })
+
+    def test_insert_keys(self):
+        self._test_insert(self.classes.PersonKeys)
+
+    def test_insert_attrs(self):
+        self._test_insert(self.classes.PersonAttrs)
+
+    def test_insert_both(self):
+        self._test_insert(self.classes.PersonBoth)
+
+    def test_update_keys(self):
+        self._test_update(self.classes.PersonKeys)
+
+    def test_update_attrs(self):
+        self._test_update(self.classes.PersonAttrs)
+
+    def test_update_both(self):
+        # want to make sure that before [ticket:3849], this did not have
+        # a successful behavior or workaround
+        self._test_update(self.classes.PersonBoth)
+
+    def _test_insert(self, person_cls):
+        Person = person_cls
+
+        s = Session()
+        s.bulk_insert_mappings(
+            Person, [{"id": 5, "personname": "thename"}]
+        )
+
+        eq_(
+            s.query(Person).first(),
+            Person(id=5, personname="thename")
+        )
+
+    def _test_update(self, person_cls):
+        Person = person_cls
+
+        s = Session()
+        s.add(Person(id=5, personname="thename"))
+        s.commit()
+
+        s.bulk_update_mappings(
+            Person, [{"id": 5, "personname": "newname"}]
+        )
+
+        eq_(
+            s.query(Person).first(),
+            Person(id=5, personname="newname")
+        )
+
+
 class BulkInheritanceTest(BulkTest, fixtures.MappedTest):
     @classmethod
     def define_tables(cls, metadata):