From: Mike Bayer Date: Sun, 24 Apr 2022 20:19:16 +0000 (-0400) Subject: backport 6f02d5edd88fe247 to 1.4 X-Git-Tag: rel_1_4_36~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e32937fa6a7dcc3d5087aa1f41049373ab9e4038;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git backport 6f02d5edd88fe247 to 1.4 in 6f02d5edd88fe2475629438b0730181a2b00c5fe some cleanup to ForeignKey repaired the use case of ForeignKey objects referring to table name alone, by adding more robust column resolution logic. This change also fixes an issue where the "referred column" naming convention key uses the resolved referred column earlier than usual when a ForeignKey is setting up its constraint. Fixed bug where :class:`.ForeignKeyConstraint` naming conventions using the ``referred_column_0`` naming convention key would not work if the foreign key constraint were set up as a :class:`.ForeignKey` object rather than an explicit :class:`.ForeignKeyConstraint` object. As this change makes use of a backport of some fixes from version 2.0, an additional little-known feature that has likely been broken for many years is also fixed which is that a :class:`.ForeignKey` object may refer to a referred table by name of the table alone without using a column name, if the name of the referent column is the same as that of the referred column. The ``referred_column_0`` naming convention key was not previously not tested with the :class:`.ForeignKey` object, only :class:`.ForeignKeyConstraint`, and this bug reveals that the feature has never worked correctly unless :class:`.ForeignKeyConstraint` is used for all FK constraints. This bug traces back to the original introduction of the feature introduced for :ticket:`3989`. Fixes: #7958 Change-Id: I230d43e9deba5dff889b9e7fee6cd4d3aa2496d3 --- diff --git a/doc/build/changelog/unreleased_14/7958.rst b/doc/build/changelog/unreleased_14/7958.rst new file mode 100644 index 0000000000..057647bd87 --- /dev/null +++ b/doc/build/changelog/unreleased_14/7958.rst @@ -0,0 +1,20 @@ +.. change:: + :tags: bug, schema + :tickets: 7958 + + Fixed bug where :class:`.ForeignKeyConstraint` naming conventions using the + ``referred_column_0`` naming convention key would not work if the foreign + key constraint were set up as a :class:`.ForeignKey` object rather than an + explicit :class:`.ForeignKeyConstraint` object. As this change makes use of + a backport of some fixes from version 2.0, an additional little-known + feature that has likely been broken for many years is also fixed which is + that a :class:`.ForeignKey` object may refer to a referred table by name of + the table alone without using a column name, if the name of the referent + column is the same as that of the referred column. + + The ``referred_column_0`` naming convention key was not previously not + tested with the :class:`.ForeignKey` object, only + :class:`.ForeignKeyConstraint`, and this bug reveals that the feature has + never worked correctly unless :class:`.ForeignKeyConstraint` is used for + all FK constraints. This bug traces back to the original introduction of + the feature introduced for :ticket:`3989`. diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py index d91e4e1661..aa904fcf5a 100644 --- a/lib/sqlalchemy/sql/schema.py +++ b/lib/sqlalchemy/sql/schema.py @@ -2433,10 +2433,6 @@ class ForeignKey(DialectKWArgs, SchemaItem): return parenttable, tablekey, colname def _link_to_col_by_colstring(self, parenttable, table, colname): - if not hasattr(self.constraint, "_referred_table"): - self.constraint._referred_table = table - else: - assert self.constraint._referred_table is table _column = None if colname is None: @@ -2444,8 +2440,12 @@ class ForeignKey(DialectKWArgs, SchemaItem): # was specified as table name only, in which case we # match the column name to the same column on the # parent. - key = self.parent - _column = table.c.get(self.parent.key, None) + # this use case wasn't working in later 1.x series + # as it had no test coverage; fixed in 2.0 + parent = self.parent + assert parent is not None + key = parent.key + _column = table.c.get(key, None) elif self.link_to_name: key = colname for c in table.c: @@ -2465,10 +2465,10 @@ class ForeignKey(DialectKWArgs, SchemaItem): key, ) - self._set_target_column(_column) + return _column def _set_target_column(self, column): - assert isinstance(self.parent.table, Table) + assert self.parent is not None # propagate TypeEngine to parent if it didn't have one if self.parent.type._isnull: @@ -2518,14 +2518,11 @@ class ForeignKey(DialectKWArgs, SchemaItem): "parent MetaData" % parenttable ) else: - raise exc.NoReferencedColumnError( - "Could not initialize target column for " - "ForeignKey '%s' on table '%s': " - "table '%s' has no column named '%s'" - % (self._colspec, parenttable.name, tablekey, colname), - tablekey, - colname, + table = parenttable.metadata.tables[tablekey] + return self._link_to_col_by_colstring( + parenttable, table, colname ) + elif hasattr(self._colspec, "__clause_element__"): _column = self._colspec.__clause_element__() return _column @@ -2545,6 +2542,11 @@ class ForeignKey(DialectKWArgs, SchemaItem): def _set_remote_table(self, table): parenttable, tablekey, colname = self._resolve_col_tokens() self._link_to_col_by_colstring(parenttable, table, colname) + + _column = self._link_to_col_by_colstring(parenttable, table, colname) + self._set_target_column(_column) + assert self.constraint is not None + self.constraint._validate_dest_table(table) def _remove_from_metadata(self, metadata): @@ -2583,10 +2585,14 @@ class ForeignKey(DialectKWArgs, SchemaItem): if table_key in parenttable.metadata.tables: table = parenttable.metadata.tables[table_key] try: - self._link_to_col_by_colstring(parenttable, table, colname) + _column = self._link_to_col_by_colstring( + parenttable, table, colname + ) except exc.NoReferencedColumnError: # this is OK, we'll try later pass + else: + self._set_target_column(_column) parenttable.metadata._fk_memos[fk_key].append(self) elif hasattr(self._colspec, "__clause_element__"): _column = self._colspec.__clause_element__() diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py index 7205c88233..50cf253379 100644 --- a/test/sql/test_metadata.py +++ b/test/sql/test_metadata.py @@ -761,7 +761,10 @@ class MetaDataTest(fixtures.TestBase, ComparesTables): "%s" ", name='someconstraint')" % repr(ck.sqltext), ), - (ColumnDefault(("foo", "bar")), "ColumnDefault(('foo', 'bar'))"), + ( + ColumnDefault(("foo", "bar")), + "ColumnDefault(('foo', 'bar'))", + ), ): eq_(repr(const), exp) @@ -919,6 +922,46 @@ class ToMetaDataTest(fixtures.TestBase, AssertsCompiledSQL, ComparesTables): a2 = a.to_metadata(m2) assert b2.c.y.references(a2.c.x) + def test_fk_w_no_colname(self): + """test a ForeignKey that refers to table name only. the column + name is assumed to be the same col name on parent table. + + this is a little used feature from long ago that nonetheless is + still in the code. + + The feature was found to be not working but is repaired for + SQLAlchemy 2.0. + + """ + m1 = MetaData() + a = Table("a", m1, Column("x", Integer)) + b = Table("b", m1, Column("x", Integer, ForeignKey("a"))) + assert b.c.x.references(a.c.x) + + m2 = MetaData() + b2 = b.to_metadata(m2) + a2 = a.to_metadata(m2) + assert b2.c.x.references(a2.c.x) + + def test_fk_w_no_colname_name_missing(self): + """test a ForeignKey that refers to table name only. the column + name is assumed to be the same col name on parent table. + + this is a little used feature from long ago that nonetheless is + still in the code. + + """ + m1 = MetaData() + a = Table("a", m1, Column("x", Integer)) + b = Table("b", m1, Column("y", Integer, ForeignKey("a"))) + + with expect_raises_message( + exc.NoReferencedColumnError, + "Could not initialize target column for ForeignKey 'a' on " + "table 'b': table 'a' has no column named 'y'", + ): + assert b.c.y.references(a.c.x) + def test_column_collection_constraint_w_ad_hoc_columns(self): """Test ColumnCollectionConstraint that has columns that aren't part of the Table. @@ -5303,6 +5346,29 @@ class NamingConventionTest(fixtures.TestBase, AssertsCompiledSQL): a1.append_constraint(fk) eq_(fk.name, "fk_address_user_id_user_id") + @testing.combinations(True, False, argnames="col_has_type") + def test_fk_ref_local_referent_has_no_type(self, col_has_type): + """test #7958""" + + metadata = MetaData( + naming_convention={ + "fk": "fk_%(referred_column_0_name)s", + } + ) + Table("a", metadata, Column("id", Integer, primary_key=True)) + b = Table( + "b", + metadata, + Column("id", Integer, primary_key=True), + Column("aid", ForeignKey("a.id")) + if not col_has_type + else Column("aid", Integer, ForeignKey("a.id")), + ) + fks = list( + c for c in b.constraints if isinstance(c, ForeignKeyConstraint) + ) + eq_(fks[0].name, "fk_id") + def test_custom(self): def key_hash(const, table): return "HASH_%s" % table.name