From: Mike Bayer Date: Wed, 27 Jan 2021 19:01:40 +0000 (-0500) Subject: Enable SQL Server testing and fix autogen issues X-Git-Tag: rel_1_5_3~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=407316635cdb8edf95f8ee7bd06a5f2dd01eda26;p=thirdparty%2Fsqlalchemy%2Falembic.git Enable SQL Server testing and fix autogen issues Fixed assorted autogenerate issues with SQL Server: * ignore default reflected identity on primary_key columns * improve server default comparison Updated test_autogen_fks for modern levels of FK capabilities Change-Id: I94b815cedf90422ccd5ceceb765b07d772b505b7 Fixes: #787 --- diff --git a/alembic/autogenerate/compare.py b/alembic/autogenerate/compare.py index e1d8c804..e34114b1 100644 --- a/alembic/autogenerate/compare.py +++ b/alembic/autogenerate/compare.py @@ -847,8 +847,16 @@ def _compare_nullable( alter_column_op.existing_nullable = conn_col_nullable if conn_col_nullable is not metadata_col_nullable: - if sqla_compat._server_default_is_identity( - metadata_col.server_default, conn_col.server_default + if ( + sqla_compat._server_default_is_computed( + metadata_col.server_default, conn_col.server_default + ) + and sqla_compat._nullability_might_be_unset(metadata_col) + or ( + sqla_compat._server_default_is_identity( + metadata_col.server_default, conn_col.server_default + ) + ) ): log.info( "Ignoring nullable change on identity column '%s.%s'", @@ -1025,11 +1033,11 @@ def _compare_identity_default( metadata_col, ): impl = autogen_context.migration_context.impl - diff, ignored_attr = impl._compare_identity_default( + diff, ignored_attr, is_alter = impl._compare_identity_default( metadata_col.server_default, conn_col.server_default ) - return diff + return diff, is_alter @comparators.dispatch_for("column") @@ -1076,7 +1084,7 @@ def _compare_server_default( metadata_default, conn_col_default ): alter_column_op.existing_server_default = conn_col_default - is_diff = _compare_identity_default( + diff, is_alter = _compare_identity_default( autogen_context, alter_column_op, schema, @@ -1085,15 +1093,15 @@ def _compare_server_default( conn_col, metadata_col, ) - if is_diff or (bool(conn_col_default) != bool(metadata_default)): + if is_alter: alter_column_op.modify_server_default = metadata_default - if is_diff: + if diff: log.info( "Detected server default on column '%s.%s': " "identity options attributes %s", tname, cname, - sorted(is_diff), + sorted(diff), ) else: rendered_metadata_default = _render_server_default_for_compare( diff --git a/alembic/ddl/impl.py b/alembic/ddl/impl.py index 923fd8b5..62692d2b 100644 --- a/alembic/ddl/impl.py +++ b/alembic/ddl/impl.py @@ -555,7 +555,15 @@ class DefaultImpl(with_metaclass(ImplMeta)): diff.difference_update(self.identity_attrs_ignore) - return diff, ignored + # returns 3 values: + return ( + # different identity attributes + diff, + # ignored identity attributes + ignored, + # if the two identity should be considered different + bool(diff) or bool(metadata_identity) != bool(inspector_identity), + ) def _compare_identity_options( diff --git a/alembic/ddl/mssql.py b/alembic/ddl/mssql.py index 916011aa..18e09f82 100644 --- a/alembic/ddl/mssql.py +++ b/alembic/ddl/mssql.py @@ -178,6 +178,42 @@ class MSSQLImpl(DefaultImpl): table_name, column, schema=schema, **kw ) + def compare_server_default( + self, + inspector_column, + metadata_column, + rendered_metadata_default, + rendered_inspector_default, + ): + def clean(value): + if value is not None: + value = value.strip() + while value[0] == "(" and value[-1] == ")": + value = value[1:-1] + return value + + return clean(rendered_inspector_default) != clean( + rendered_metadata_default + ) + + def _compare_identity_default(self, metadata_identity, inspector_identity): + diff, ignored, is_alter = super( + MSSQLImpl, self + )._compare_identity_default(metadata_identity, inspector_identity) + + if ( + metadata_identity is None + and inspector_identity is not None + and not diff + and inspector_identity.column is not None + and inspector_identity.column.primary_key + ): + # mssql reflect primary keys with autoincrement as identity + # columns. if no different attributes are present ignore them + is_alter = False + + return diff, ignored, is_alter + class _ExecDropConstraint(Executable, ClauseElement): def __init__(self, tname, colname, type_, schema): diff --git a/alembic/ddl/postgresql.py b/alembic/ddl/postgresql.py index b8fdd99d..6a2f007c 100644 --- a/alembic/ddl/postgresql.py +++ b/alembic/ddl/postgresql.py @@ -319,7 +319,7 @@ def visit_identity_column(element, compiler, **kw): return text else: # alter identity - diff, _ = element.impl._compare_identity_default( + diff, _, _ = element.impl._compare_identity_default( element.default, element.existing_server_default ) identity = element.default diff --git a/alembic/util/sqla_compat.py b/alembic/util/sqla_compat.py index d23f1ebb..7cb3fbd8 100644 --- a/alembic/util/sqla_compat.py +++ b/alembic/util/sqla_compat.py @@ -129,6 +129,17 @@ def _exec_on_inspector(inspector, statement, **params): return inspector.bind.execute(statement, params) +def _nullability_might_be_unset(metadata_column): + if not sqla_14: + return metadata_column.nullable + else: + from sqlalchemy.sql import schema + + return ( + metadata_column._user_defined_nullable is schema.NULL_UNSPECIFIED + ) + + def _server_default_is_computed(*server_default): if not has_computed: return False diff --git a/docs/build/unreleased/787.rst b/docs/build/unreleased/787.rst new file mode 100644 index 00000000..90fce9a2 --- /dev/null +++ b/docs/build/unreleased/787.rst @@ -0,0 +1,8 @@ +.. change:: + :tags: bug, autogenerate, mssql + :tickets: 787 + + Fixed assorted autogenerate issues with SQL Server: + + * ignore default reflected identity on primary_key columns + * improve server default comparison diff --git a/tests/requirements.py b/tests/requirements.py index 49ce8a46..5f23b22b 100644 --- a/tests/requirements.py +++ b/tests/requirements.py @@ -59,7 +59,7 @@ class DefaultRequirements(SuiteRequirements): @property def reflects_fk_options(self): - return exclusions.only_on(["postgresql", "mysql", "mariadb", "sqlite"]) + return exclusions.open() @property def fk_initially(self): @@ -69,15 +69,38 @@ class DefaultRequirements(SuiteRequirements): @property def fk_deferrable(self): """backend supports DEFERRABLE option in foreign keys""" - return exclusions.only_on(["postgresql"]) + return exclusions.only_on(["postgresql", "oracle"]) @property - def flexible_fk_cascades(self): - """target database must support ON UPDATE/DELETE..CASCADE with the - full range of keywords (e.g. NO ACTION, etc.)""" + def fk_deferrable_is_reflected(self): + return self.fk_deferrable + exclusions.fails_on("oracle") - return exclusions.skip_if( - ["oracle"], "target backend has poor FK cascade syntax" + @property + def fk_ondelete_restrict(self): + return exclusions.only_on(["postgresql", "sqlite", "mysql"]) + + @property + def fk_onupdate_restrict(self): + return self.fk_onupdate + exclusions.fails_on(["mssql"]) + + @property + def fk_ondelete_noaction(self): + return exclusions.only_on( + ["postgresql", "mysql", "mariadb", "sqlite", "mssql"] + ) + + @property + def fk_ondelete_is_reflected(self): + return exclusions.fails_on(["mssql"]) + + @property + def fk_onupdate_is_reflected(self): + return self.fk_onupdate + exclusions.fails_on(["mssql"]) + + @property + def fk_onupdate(self): + return exclusions.only_on( + ["postgresql", "mysql", "mariadb", "sqlite", "mssql"] ) @property diff --git a/tests/test_autogen_computed.py b/tests/test_autogen_computed.py index e39300bf..85b1a0c5 100644 --- a/tests/test_autogen_computed.py +++ b/tests/test_autogen_computed.py @@ -118,7 +118,7 @@ class AutogenerateComputedTest(AutogenFixtureTest, TestBase): [mock.call("Computed default on user.foo cannot be modified")], ) - eq_(len(diffs), 0) + eq_(list(diffs), []) @testing.combinations( lambda: (None, None), @@ -157,7 +157,7 @@ class AutogenerateComputedTest(AutogenFixtureTest, TestBase): diffs = self._fixture(m1, m2) eq_(mock_warn.mock_calls, []) - eq_(len(diffs), 0) + eq_(list(diffs), []) @config.requirements.computed_reflects_as_server_default def test_remove_computed_default_on_computed(self): diff --git a/tests/test_autogen_diffs.py b/tests/test_autogen_diffs.py index 96184be7..d847e1e0 100644 --- a/tests/test_autogen_diffs.py +++ b/tests/test_autogen_diffs.py @@ -1880,13 +1880,13 @@ class AutoincrementTest(AutogenFixtureTest, TestBase): Table( "a", m1, - Column("id", Integer, primary_key=True), + Column("id", Integer, primary_key=True, autoincrement=False), Column("x", Integer, autoincrement=True), ) Table( "a", m2, - Column("id", Integer, primary_key=True), + Column("id", Integer, primary_key=True, autoincrement=False), Column("x", BigInteger, autoincrement=True), ) diff --git a/tests/test_autogen_fks.py b/tests/test_autogen_fks.py index 33cf3ccd..1478c794 100644 --- a/tests/test_autogen_fks.py +++ b/tests/test_autogen_fks.py @@ -331,7 +331,6 @@ class AutogenerateForeignKeysTest(AutogenFixtureTest, TestBase): ) diffs = self._fixture(m1, m2) - self._assert_fk_diff( diffs[0], "add_fk", @@ -768,7 +767,6 @@ class IncludeHooksTest(AutogenFixtureTest, TestBase): class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): __backend__ = True - __requires__ = ("flexible_fk_cascades",) def _fk_opts_fixture(self, old_opts, new_opts): m1 = MetaData() @@ -812,71 +810,55 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): return self._fixture(m1, m2) - def _expect_opts_supported(self, deferrable=False, initially=False): - if not config.requirements.reflects_fk_options.enabled: - return False - - if deferrable and not config.requirements.fk_deferrable.enabled: - return False - - if initially and not config.requirements.fk_initially.enabled: - return False - - return True - + @config.requirements.fk_ondelete_is_reflected def test_add_ondelete(self): diffs = self._fk_opts_fixture({}, {"ondelete": "cascade"}) - if self._expect_opts_supported(): - self._assert_fk_diff( - diffs[0], - "remove_fk", - "user", - ["tid"], - "some_table", - ["id"], - ondelete=None, - conditional_name="servergenerated", - ) + self._assert_fk_diff( + diffs[0], + "remove_fk", + "user", + ["tid"], + "some_table", + ["id"], + ondelete=None, + conditional_name="servergenerated", + ) - self._assert_fk_diff( - diffs[1], - "add_fk", - "user", - ["tid"], - "some_table", - ["id"], - ondelete="cascade", - ) - else: - eq_(diffs, []) + self._assert_fk_diff( + diffs[1], + "add_fk", + "user", + ["tid"], + "some_table", + ["id"], + ondelete="cascade", + ) + @config.requirements.fk_ondelete_is_reflected def test_remove_ondelete(self): diffs = self._fk_opts_fixture({"ondelete": "CASCADE"}, {}) - if self._expect_opts_supported(): - self._assert_fk_diff( - diffs[0], - "remove_fk", - "user", - ["tid"], - "some_table", - ["id"], - ondelete="CASCADE", - conditional_name="servergenerated", - ) + self._assert_fk_diff( + diffs[0], + "remove_fk", + "user", + ["tid"], + "some_table", + ["id"], + ondelete="CASCADE", + conditional_name="servergenerated", + ) - self._assert_fk_diff( - diffs[1], - "add_fk", - "user", - ["tid"], - "some_table", - ["id"], - ondelete=None, - ) - else: - eq_(diffs, []) + self._assert_fk_diff( + diffs[1], + "add_fk", + "user", + ["tid"], + "some_table", + ["id"], + ondelete=None, + ) def test_nochange_ondelete(self): """test case sensitivity""" @@ -885,60 +867,57 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): ) eq_(diffs, []) + @config.requirements.fk_onupdate_is_reflected def test_add_onupdate(self): diffs = self._fk_opts_fixture({}, {"onupdate": "cascade"}) - if self._expect_opts_supported(): - self._assert_fk_diff( - diffs[0], - "remove_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate=None, - conditional_name="servergenerated", - ) + self._assert_fk_diff( + diffs[0], + "remove_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate=None, + conditional_name="servergenerated", + ) - self._assert_fk_diff( - diffs[1], - "add_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate="cascade", - ) - else: - eq_(diffs, []) + self._assert_fk_diff( + diffs[1], + "add_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate="cascade", + ) + @config.requirements.fk_onupdate_is_reflected def test_remove_onupdate(self): diffs = self._fk_opts_fixture({"onupdate": "CASCADE"}, {}) - if self._expect_opts_supported(): - self._assert_fk_diff( - diffs[0], - "remove_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate="CASCADE", - conditional_name="servergenerated", - ) + self._assert_fk_diff( + diffs[0], + "remove_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate="CASCADE", + conditional_name="servergenerated", + ) - self._assert_fk_diff( - diffs[1], - "add_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate=None, - ) - else: - eq_(diffs, []) + self._assert_fk_diff( + diffs[1], + "add_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate=None, + ) + @config.requirements.fk_onupdate def test_nochange_onupdate(self): """test case sensitivity""" diffs = self._fk_opts_fixture( @@ -946,6 +925,7 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): ) eq_(diffs, []) + @config.requirements.fk_ondelete_restrict def test_nochange_ondelete_restrict(self): """test the RESTRICT option which MySQL doesn't report on""" @@ -954,6 +934,7 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): ) eq_(diffs, []) + @config.requirements.fk_onupdate_restrict def test_nochange_onupdate_restrict(self): """test the RESTRICT option which MySQL doesn't report on""" @@ -962,6 +943,7 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): ) eq_(diffs, []) + @config.requirements.fk_ondelete_noaction def test_nochange_ondelete_noaction(self): """test the NO ACTION option which generally comes back as None""" @@ -970,6 +952,7 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): ) eq_(diffs, []) + @config.requirements.fk_onupdate def test_nochange_onupdate_noaction(self): """test the NO ACTION option which generally comes back as None""" @@ -978,6 +961,7 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): ) eq_(diffs, []) + @config.requirements.fk_ondelete_restrict def test_change_ondelete_from_restrict(self): """test the RESTRICT option which MySQL doesn't report on""" @@ -986,32 +970,30 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): diffs = self._fk_opts_fixture( {"ondelete": "restrict"}, {"ondelete": "cascade"} ) - if self._expect_opts_supported(): - self._assert_fk_diff( - diffs[0], - "remove_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate=None, - ondelete=mock.ANY, # MySQL reports None, PG reports RESTRICT - conditional_name="servergenerated", - ) + self._assert_fk_diff( + diffs[0], + "remove_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate=None, + ondelete=mock.ANY, # MySQL reports None, PG reports RESTRICT + conditional_name="servergenerated", + ) - self._assert_fk_diff( - diffs[1], - "add_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate=None, - ondelete="cascade", - ) - else: - eq_(diffs, []) + self._assert_fk_diff( + diffs[1], + "add_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate=None, + ondelete="cascade", + ) + @config.requirements.fk_ondelete_restrict def test_change_onupdate_from_restrict(self): """test the RESTRICT option which MySQL doesn't report on""" @@ -1020,63 +1002,59 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): diffs = self._fk_opts_fixture( {"onupdate": "restrict"}, {"onupdate": "cascade"} ) - if self._expect_opts_supported(): - self._assert_fk_diff( - diffs[0], - "remove_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate=mock.ANY, # MySQL reports None, PG reports RESTRICT - ondelete=None, - conditional_name="servergenerated", - ) + self._assert_fk_diff( + diffs[0], + "remove_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate=mock.ANY, # MySQL reports None, PG reports RESTRICT + ondelete=None, + conditional_name="servergenerated", + ) - self._assert_fk_diff( - diffs[1], - "add_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate="cascade", - ondelete=None, - ) - else: - eq_(diffs, []) + self._assert_fk_diff( + diffs[1], + "add_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate="cascade", + ondelete=None, + ) + @config.requirements.fk_ondelete_is_reflected + @config.requirements.fk_onupdate_is_reflected def test_ondelete_onupdate_combo(self): diffs = self._fk_opts_fixture( {"onupdate": "CASCADE", "ondelete": "SET NULL"}, {"onupdate": "RESTRICT", "ondelete": "RESTRICT"}, ) - if self._expect_opts_supported(): - self._assert_fk_diff( - diffs[0], - "remove_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate="CASCADE", - ondelete="SET NULL", - conditional_name="servergenerated", - ) + self._assert_fk_diff( + diffs[0], + "remove_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate="CASCADE", + ondelete="SET NULL", + conditional_name="servergenerated", + ) - self._assert_fk_diff( - diffs[1], - "add_fk", - "user", - ["tid"], - "some_table", - ["id"], - onupdate="RESTRICT", - ondelete="RESTRICT", - ) - else: - eq_(diffs, []) + self._assert_fk_diff( + diffs[1], + "add_fk", + "user", + ["tid"], + "some_table", + ["id"], + onupdate="RESTRICT", + ondelete="RESTRICT", + ) @config.requirements.fk_initially def test_add_initially_deferred(self): @@ -1243,7 +1221,7 @@ class AutogenerateFKOptionsTest(AutogenFixtureTest, TestBase): deferrable=True, ) - @config.requirements.fk_deferrable + @config.requirements.fk_deferrable_is_reflected def test_remove_deferrable(self): diffs = self._fk_opts_fixture({"deferrable": True}, {}) diff --git a/tox.ini b/tox.ini index 7e8f5477..92e742be 100644 --- a/tox.ini +++ b/tox.ini @@ -13,21 +13,12 @@ deps=pytest>4.6 mock sqla13: {[tox]SQLA_REPO}@rel_1_3#egg=sqlalchemy sqlamaster: {[tox]SQLA_REPO}@master#egg=sqlalchemy - postgresql: psycopg2 - mysql: mysqlclient - mysql: pymysql;python_version>="3" - mysql: pymysql<1;python_version<"3" + postgresql: psycopg2>=2.7 + mysql: mysqlclient>=1.4.0 + mysql: pymysql oracle: cx_oracle>=7,<8;python_version<"3" oracle: cx_oracle>=7;python_version>="3" mssql: pyodbc - - -# TODO: can't get these to work w/ git URLs -# postgresql: sqlalchemy[postgresql] -# mysql: sqlalchemy[mysql] -# mysql: sqlalchemy[pymysql] -# oracle: sqlalchemy[oracle] -# mssql: sqlalchemy[mssql] cov: pytest-cov sqlalchemy: sqlalchemy>=1.3.0 mako @@ -50,7 +41,7 @@ setenv= postgresql: POSTGRESQL={env:TOX_POSTGRESQL:--db postgresql} mysql: MYSQL={env:TOX_MYSQL:--db mysql} oracle: ORACLE={env:TOX_ORACLE:--db oracle} --low-connections --write-idents db_idents.txt - mssql: MSSQL={env:TOX_MSSQL:--db pymssql} + mssql: MSSQL={env:TOX_MSSQL:--db mssql} pyoptimize: PYTHONOPTIMIZE=1 pyoptimize: LIMITTESTS="tests/test_script_consumption.py" future: SQLALCHEMY_TESTING_FUTURE_ENGINE=1 @@ -63,7 +54,7 @@ setenv= passenv=ORACLE_HOME NLS_LANG TOX_SQLITE TOX_POSTGRESQL TOX_MYSQL TOX_ORACLE TOX_MSSQL commands= - {env:BASECOMMAND} {env:WORKERS} {env:SQLITE:} {env:POSTGRESQL:} {env:MYSQL:} {env:ORACLE:} {env:BACKENDONLY:} {env:COVERAGE:} {env:LIMITTESTS:} {posargs} + {env:BASECOMMAND} {env:WORKERS} {env:SQLITE:} {env:POSTGRESQL:} {env:MYSQL:} {env:ORACLE:} {env:MSSQL:} {env:BACKENDONLY:} {env:COVERAGE:} {env:LIMITTESTS:} {posargs} {oracle,mssql}: python reap_dbs.py db_idents.txt