From: Mike Bayer Date: Mon, 14 Oct 2024 15:15:21 +0000 (-0400) Subject: match ORM mapped cols to PK in interpret_returning_rows X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=553d02b30eba13f3db4595d7a15e51222f864e13;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git match ORM mapped cols to PK in interpret_returning_rows Fixed bug in ORM "update with WHERE clause" feature where an explicit ``.returning()`` would interfere with the "fetch" synchronize strategy due to an assumption that the ORM mapped class featured the primary key columns in a specific position within the RETURNING. This has been fixed to use appropriate ORM column targeting. the _interpret_returning_rows method looked to be mostly not used as far as its joined inheritance features, which appear to have never been used as joined inheritance mappers are skipped. Fixes: #11997 Change-Id: I38fe3a84cdeb2eef38fe00d8b9a6a2b56f434bc6 --- diff --git a/doc/build/changelog/unreleased_20/11997.rst b/doc/build/changelog/unreleased_20/11997.rst new file mode 100644 index 0000000000..b2390977e1 --- /dev/null +++ b/doc/build/changelog/unreleased_20/11997.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, orm + :tickets: 11997 + + Fixed bug in ORM "update with WHERE clause" feature where an explicit + ``.returning()`` would interfere with the "fetch" synchronize strategy due + to an assumption that the ORM mapped class featured the primary key columns + in a specific position within the RETURNING. This has been fixed to use + appropriate ORM column targeting. diff --git a/lib/sqlalchemy/orm/bulk_persistence.py b/lib/sqlalchemy/orm/bulk_persistence.py index a9408f1cce..3c033be585 100644 --- a/lib/sqlalchemy/orm/bulk_persistence.py +++ b/lib/sqlalchemy/orm/bulk_persistence.py @@ -864,53 +864,39 @@ class BulkUDCompileState(ORMDMLState): return return_crit @classmethod - def _interpret_returning_rows(cls, mapper, rows): - """translate from local inherited table columns to base mapper - primary key columns. + def _interpret_returning_rows(cls, result, mapper, rows): + """return rows that indicate PK cols in mapper.primary_key position + for RETURNING rows. - Joined inheritance mappers always establish the primary key in terms of - the base table. When we UPDATE a sub-table, we can only get - RETURNING for the sub-table's columns. + Prior to 2.0.36, this method seemed to be written for some kind of + inheritance scenario but the scenario was unused for actual joined + inheritance, and the function instead seemed to perform some kind of + partial translation that would remove non-PK cols if the PK cols + happened to be first in the row, but not otherwise. The joined + inheritance walk feature here seems to have never been used as it was + always skipped by the "local_table" check. - Here, we create a lookup from the local sub table's primary key - columns to the base table PK columns so that we can get identity - key values from RETURNING that's against the joined inheritance - sub-table. - - the complexity here is to support more than one level deep of - inheritance, where we have to link columns to each other across - the inheritance hierarchy. + As of 2.0.36 the function strips away non-PK cols and provides the + PK cols for the table in mapper PK order. """ - if mapper.local_table is not mapper.base_mapper.local_table: - return rows - - # this starts as a mapping of - # local_pk_col: local_pk_col. - # we will then iteratively rewrite the "value" of the dict with - # each successive superclass column - local_pk_to_base_pk = {pk: pk for pk in mapper.local_table.primary_key} - - for mp in mapper.iterate_to_root(): - if mp.inherits is None: - break - elif mp.local_table is mp.inherits.local_table: - continue - - t_to_e = dict(mp._table_to_equated[mp.inherits.local_table]) - col_to_col = {sub_pk: super_pk for super_pk, sub_pk in t_to_e[mp]} - for pk, super_ in local_pk_to_base_pk.items(): - local_pk_to_base_pk[pk] = col_to_col[super_] + try: + if mapper.local_table is not mapper.base_mapper.local_table: + # TODO: dive more into how a local table PK is used for fetch + # sync, not clear if this is correct as it depends on the + # downstream routine to fetch rows using + # local_table.primary_key order + pk_keys = result._tuple_getter(mapper.local_table.primary_key) + else: + pk_keys = result._tuple_getter(mapper.primary_key) + except KeyError: + # can't use these rows, they don't have PK cols in them + # this is an unusual case where the user would have used + # .return_defaults() + return [] - lookup = { - local_pk_to_base_pk[lpk]: idx - for idx, lpk in enumerate(mapper.local_table.primary_key) - } - primary_key_convert = [ - lookup[bpk] for bpk in mapper.base_mapper.primary_key - ] - return [tuple(row[idx] for idx in primary_key_convert) for row in rows] + return [pk_keys(row) for row in rows] @classmethod def _get_matched_objects_on_criteria(cls, update_options, states): @@ -1778,9 +1764,8 @@ class BulkORMUpdate(BulkUDCompileState, UpdateDMLState): returned_defaults_rows = result.returned_defaults_rows if returned_defaults_rows: pk_rows = cls._interpret_returning_rows( - target_mapper, returned_defaults_rows + result, target_mapper, returned_defaults_rows ) - matched_rows = [ tuple(row) + (update_options._identity_token,) for row in pk_rows @@ -2110,7 +2095,7 @@ class BulkORMDelete(BulkUDCompileState, DeleteDMLState): if returned_defaults_rows: pk_rows = cls._interpret_returning_rows( - target_mapper, returned_defaults_rows + result, target_mapper, returned_defaults_rows ) matched_rows = [ diff --git a/test/orm/dml/test_update_delete_where.py b/test/orm/dml/test_update_delete_where.py index 8d9feaf63c..da8efa44fa 100644 --- a/test/orm/dml/test_update_delete_where.py +++ b/test/orm/dml/test_update_delete_where.py @@ -3329,6 +3329,7 @@ class OnUpdatePopulationTest(fixtures.TestBase): ], ) @testing.variation("synchronize", ["auto", "fetch", "evaluate"]) + @testing.variation("pk_order", ["first", "middle"]) def test_update_populate_existing( self, decl_base, @@ -3336,15 +3337,20 @@ class OnUpdatePopulationTest(fixtures.TestBase): use_onupdate, use_returning, synchronize, + pk_order, ): """test #11912 and #11917""" class Employee(ComparableEntity, decl_base): __tablename__ = "employee" - uuid: Mapped[uuid.UUID] = mapped_column(primary_key=True) + if pk_order.first: + uuid: Mapped[uuid.UUID] = mapped_column(primary_key=True) user_name: Mapped[str] = mapped_column(String(200), nullable=False) + if pk_order.middle: + uuid: Mapped[uuid.UUID] = mapped_column(primary_key=True) + if use_onupdate.server: some_server_value: Mapped[str] = mapped_column( server_onupdate=FetchedValue()