From: Mike Bayer Date: Mon, 14 Oct 2019 00:33:24 +0000 (-0400) Subject: Use separate label generator for column_label naming convention X-Git-Tag: rel_1_3_11~40^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=53b7b8de6535f5bb6930bddf6a8fb87ba56bddfb;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Use separate label generator for column_label naming convention Fixed bug where a table that would have a column label overlap with a plain column name, such as "foo.id AS foo_id" vs. "foo.foo_id", would prematurely generate the ``._label`` attribute for a column before this overlap could be detected due to the use of the ``index=True`` or ``unique=True`` flag on the column in conjunction with the default naming convention of ``"column_0_label"``. This would then lead to failures when ``._label`` were used later to generate a bound parameter name, in particular those used by the ORM when generating the WHERE clause for an UPDATE statement. The issue has been fixed by using an alternate ``._label`` accessor for DDL generation that does not affect the state of the :class:`.Column`. The accessor also bypasses the key-deduplication step as it is not necessary for DDL, the naming is now consistently ``"_"`` without any subsequent numeric symbols when used in DDL. Fixes: #4911 Change-Id: Iabf5fd3250738d800d6e41a2a3a27a7ce2405e7d (cherry picked from commit 41dc71ad2fc1963a44e5f308f53aed6b8d7d662a) --- diff --git a/doc/build/changelog/unreleased_13/4911.rst b/doc/build/changelog/unreleased_13/4911.rst new file mode 100644 index 0000000000..02db4b809a --- /dev/null +++ b/doc/build/changelog/unreleased_13/4911.rst @@ -0,0 +1,19 @@ +.. change:: + :tags: bug, schema + :tickets: 4911 + + Fixed bug where a table that would have a column label overlap with a plain + column name, such as "foo.id AS foo_id" vs. "foo.foo_id", would prematurely + generate the ``._label`` attribute for a column before this overlap could + be detected due to the use of the ``index=True`` or ``unique=True`` flag on + the column in conjunction with the default naming convention of + ``"column_0_label"``. This would then lead to failures when ``._label`` + were used later to generate a bound parameter name, in particular those + used by the ORM when generating the WHERE clause for an UPDATE statement. + The issue has been fixed by using an alternate ``._label`` accessor for DDL + generation that does not affect the state of the :class:`.Column`. The + accessor also bypasses the key-deduplication step as it is not necessary + for DDL, the naming is now consistently ``"_"`` + without any subsequent numeric symbols when used in DDL. + + diff --git a/lib/sqlalchemy/dialects/mssql/base.py b/lib/sqlalchemy/dialects/mssql/base.py index ad5f418350..d01acab125 100644 --- a/lib/sqlalchemy/dialects/mssql/base.py +++ b/lib/sqlalchemy/dialects/mssql/base.py @@ -2063,9 +2063,9 @@ class MSDDLCompiler(compiler.DDLCompiler): return "" text = "" if constraint.name is not None: - text += "CONSTRAINT %s " % self.preparer.format_constraint( - constraint - ) + formatted_name = self.preparer.format_constraint(constraint) + if formatted_name is not None: + text += "CONSTRAINT %s " % formatted_name text += "UNIQUE " clustered = constraint.dialect_options["mssql"]["clustered"] diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index e395caeaa8..810a4d9286 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -2972,6 +2972,10 @@ class DDLCompiler(Compiled): text = "CREATE " if index.unique: text += "UNIQUE " + if index.name is None: + raise exc.CompileError( + "CREATE INDEX requires that the index have a name" + ) text += "INDEX %s ON %s (%s)" % ( self._prepared_index_name(index, include_schema=include_schema), preparer.format_table( @@ -2988,6 +2992,11 @@ class DDLCompiler(Compiled): def visit_drop_index(self, drop): index = drop.element + + if index.name is None: + raise exc.CompileError( + "DROP INDEX requires that the index have a name" + ) return "\nDROP INDEX " + self._prepared_index_name( index, include_schema=True ) @@ -3201,7 +3210,8 @@ class DDLCompiler(Compiled): text = "" if constraint.name is not None: formatted_name = self.preparer.format_constraint(constraint) - text += "CONSTRAINT %s " % formatted_name + if formatted_name is not None: + text += "CONSTRAINT %s " % formatted_name text += "UNIQUE (%s)" % ( ", ".join(self.preparer.quote(c.name) for c in constraint) ) diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index b7148bf501..22834dd191 100644 --- a/lib/sqlalchemy/sql/elements.py +++ b/lib/sqlalchemy/sql/elements.py @@ -4025,7 +4025,11 @@ class ColumnClause(Immutable, ColumnElement): def _render_label_in_columns_clause(self): return self.table is not None - def _gen_label(self, name): + @property + def _ddl_label(self): + return self._gen_label(self.name, dedupe_on_key=False) + + def _gen_label(self, name, dedupe_on_key=True): t = self.table if self.is_literal: @@ -4049,15 +4053,22 @@ class ColumnClause(Immutable, ColumnElement): assert not isinstance(label, quoted_name) label = quoted_name(label, t.name.quote) - # ensure the label name doesn't conflict with that - # of an existing column - if label in t.c: - _label = label - counter = 1 - while _label in t.c: - _label = label + "_" + str(counter) - counter += 1 - label = _label + if dedupe_on_key: + # ensure the label name doesn't conflict with that of an + # existing column. note that this implies that any Column + # must **not** set up its _label before its parent table has + # all of its other Column objects set up. There are several + # tables in the test suite which will fail otherwise; example: + # table "owner" has columns "name" and "owner_name". Therefore + # column owner.name cannot use the label "owner_name", it has + # to be "owner_name_1". + if label in t.c: + _label = label + counter = 1 + while _label in t.c: + _label = label + "_" + str(counter) + counter += 1 + label = _label return _as_truncated(label) diff --git a/lib/sqlalchemy/sql/naming.py b/lib/sqlalchemy/sql/naming.py index 68a6190cfc..3fca4e35bc 100644 --- a/lib/sqlalchemy/sql/naming.py +++ b/lib/sqlalchemy/sql/naming.py @@ -67,7 +67,7 @@ class ConventionDict(object): return self._column_X(idx).name def _key_column_X_label(self, idx): - return self._column_X(idx)._label + return self._column_X(idx)._ddl_label def _key_referred_table_name(self): fk = self.const.elements[0] diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py index e2fda26bd6..e3c3d96b96 100644 --- a/lib/sqlalchemy/sql/schema.py +++ b/lib/sqlalchemy/sql/schema.py @@ -1500,7 +1500,12 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause): "To create indexes with a specific name, create an " "explicit Index object external to the Table." ) - Index(None, self, unique=bool(self.unique), _column_flag=True) + table.append_constraint( + Index( + None, self.key, unique=bool(self.unique), _column_flag=True + ) + ) + elif self.unique: if isinstance(self.unique, util.string_types): raise exc.ArgumentError( diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py index 16ad4a1c5e..2e8d4deebc 100644 --- a/test/sql/test_metadata.py +++ b/test/sql/test_metadata.py @@ -31,8 +31,12 @@ from sqlalchemy import Unicode from sqlalchemy import UniqueConstraint from sqlalchemy import util from sqlalchemy.engine import default +from sqlalchemy.schema import AddConstraint +from sqlalchemy.schema import CreateIndex +from sqlalchemy.schema import DropIndex from sqlalchemy.sql import elements from sqlalchemy.sql import naming +from sqlalchemy.sql.elements import _NONE_NAME from sqlalchemy.testing import assert_raises from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import AssertsCompiledSQL @@ -4409,6 +4413,97 @@ class NamingConventionTest(fixtures.TestBase, AssertsCompiledSQL): return u1 + def _colliding_name_fixture(self, naming_convention, id_flags): + m1 = MetaData(naming_convention=naming_convention) + + t1 = Table( + "foo", + m1, + Column("id", Integer, **id_flags), + Column("foo_id", Integer), + ) + return t1 + + def test_colliding_col_label_from_index_flag(self): + t1 = self._colliding_name_fixture( + {"ix": "ix_%(column_0_label)s"}, {"index": True} + ) + + idx = list(t1.indexes)[0] + + # name is generated up front. alembic really prefers this + eq_(idx.name, "ix_foo_id") + self.assert_compile( + CreateIndex(idx), "CREATE INDEX ix_foo_id ON foo (id)" + ) + + def test_colliding_col_label_from_unique_flag(self): + t1 = self._colliding_name_fixture( + {"uq": "uq_%(column_0_label)s"}, {"unique": True} + ) + + const = [c for c in t1.constraints if isinstance(c, UniqueConstraint)] + uq = const[0] + + # name is generated up front. alembic really prefers this + eq_(uq.name, "uq_foo_id") + + self.assert_compile( + AddConstraint(uq), + "ALTER TABLE foo ADD CONSTRAINT uq_foo_id UNIQUE (id)", + ) + + def test_colliding_col_label_from_index_obj(self): + t1 = self._colliding_name_fixture({"ix": "ix_%(column_0_label)s"}, {}) + + idx = Index(None, t1.c.id) + is_(idx, list(t1.indexes)[0]) + eq_(idx.name, "ix_foo_id") + self.assert_compile( + CreateIndex(idx), "CREATE INDEX ix_foo_id ON foo (id)" + ) + + def test_colliding_col_label_from_unique_obj(self): + t1 = self._colliding_name_fixture({"uq": "uq_%(column_0_label)s"}, {}) + uq = UniqueConstraint(t1.c.id) + const = [c for c in t1.constraints if isinstance(c, UniqueConstraint)] + is_(const[0], uq) + eq_(const[0].name, "uq_foo_id") + self.assert_compile( + AddConstraint(const[0]), + "ALTER TABLE foo ADD CONSTRAINT uq_foo_id UNIQUE (id)", + ) + + def test_colliding_col_label_from_index_flag_no_conv(self): + t1 = self._colliding_name_fixture({"ck": "foo"}, {"index": True}) + + idx = list(t1.indexes)[0] + + # this behavior needs to fail, as of #4911 since we are testing it, + # ensure it raises a CompileError. In #4289 we may want to revisit + # this in some way, most likely specifically to Postgresql only. + assert_raises_message( + exc.CompileError, + "CREATE INDEX requires that the index have a name", + CreateIndex(idx).compile, + ) + + assert_raises_message( + exc.CompileError, + "DROP INDEX requires that the index have a name", + DropIndex(idx).compile, + ) + + def test_colliding_col_label_from_unique_flag_no_conv(self): + t1 = self._colliding_name_fixture({"ck": "foo"}, {"unique": True}) + + const = [c for c in t1.constraints if isinstance(c, UniqueConstraint)] + is_(const[0].name, None) + + self.assert_compile( + AddConstraint(const[0]), "ALTER TABLE foo ADD UNIQUE (id)" + ) + def test_uq_name(self): u1 = self._fixture( naming_convention={"uq": "uq_%(table_name)s_%(column_0_name)s"} @@ -4774,11 +4869,11 @@ class NamingConventionTest(fixtures.TestBase, AssertsCompiledSQL): u1 = Table("user", m1, Column("x", Boolean())) # constraint is not hit - eq_( + is_( [c for c in u1.constraints if isinstance(c, CheckConstraint)][ 0 ].name, - "_unnamed_", + _NONE_NAME, ) # but is hit at compile time self.assert_compile( @@ -4840,11 +4935,11 @@ class NamingConventionTest(fixtures.TestBase, AssertsCompiledSQL): u1 = Table("user", m1, Column("x", Boolean())) # constraint gets special _defer_none_name - eq_( + is_( [c for c in u1.constraints if isinstance(c, CheckConstraint)][ 0 ].name, - "_unnamed_", + _NONE_NAME, ) # no issue with native boolean self.assert_compile( @@ -4866,11 +4961,11 @@ class NamingConventionTest(fixtures.TestBase, AssertsCompiledSQL): u1 = Table("user", m1, Column("x", Boolean())) # constraint gets special _defer_none_name - eq_( + is_( [c for c in u1.constraints if isinstance(c, CheckConstraint)][ 0 ].name, - "_unnamed_", + _NONE_NAME, ) self.assert_compile( @@ -4902,3 +4997,14 @@ class NamingConventionTest(fixtures.TestBase, AssertsCompiledSQL): eq_(t2a.primary_key.name, t2b.primary_key.name) eq_(t2b.primary_key.name, "t2_pk") + + def test_expression_index(self): + m = MetaData(naming_convention={"ix": "ix_%(column_0_label)s"}) + t = Table("t", m, Column("q", Integer), Column("p", Integer)) + ix = Index(None, t.c.q + 5) + t.append_constraint(ix) + + # huh. pretty cool + self.assert_compile( + CreateIndex(ix), "CREATE INDEX ix_t_q ON t (q + 5)" + ) diff --git a/test/sql/test_update.py b/test/sql/test_update.py index 514076daab..bb6b3df763 100644 --- a/test/sql/test_update.py +++ b/test/sql/test_update.py @@ -292,6 +292,26 @@ class UpdateTest(_UpdateFromTestBase, fixtures.TablesTest, AssertsCompiledSQL): "UPDATE foo SET id=:id, foo_id=:foo_id WHERE foo.id = :foo_id_1", ) + def test_labels_no_collision_index(self): + """test for [ticket:4911] """ + + t = Table( + "foo", + MetaData(), + Column("id", Integer, index=True), + Column("foo_id", Integer), + ) + + self.assert_compile( + t.update().where(t.c.id == 5), + "UPDATE foo SET id=:id, foo_id=:foo_id WHERE foo.id = :id_1", + ) + + self.assert_compile( + t.update().where(t.c.id == bindparam(key=t.c.id._label)), + "UPDATE foo SET id=:id, foo_id=:foo_id WHERE foo.id = :foo_id_1", + ) + def test_inline_defaults(self): m = MetaData() foo = Table("foo", m, Column("id", Integer))