From: Mike Bayer Date: Mon, 20 May 2019 17:29:55 +0000 (-0400) Subject: Use CHANGE COLUMN for MySQL / MariaDB DateTime server default change X-Git-Tag: rel_1_0_11~12 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=adf0534f0d59bd6defa24fd7b343cb4590be6e2e;p=thirdparty%2Fsqlalchemy%2Falembic.git Use CHANGE COLUMN for MySQL / MariaDB DateTime server default change Fixed issue where MySQL databases need to use CHANGE COLUMN when altering a server default of CURRENT_TIMESTAMP, NOW() and probably other functions that are only usable with DATETIME/TIMESTAMP columns. While MariaDB supports both CHANGE and ALTER COLUMN in this case, MySQL databases only support CHANGE. So the new logic is that if the server default change is against a DateTime-oriented column, the CHANGE format is used unconditionally, as in the vast majority of cases the server default is to be CURRENT_TIMESTAMP which may also be potentially bundled with an "ON UPDATE CURRENT_TIMESTAMP" directive, which SQLAlchemy does not currently support as a distinct field. Change-Id: I67cf75c2fba640e737f5ffd79a8ad5636b9eeccc Fixes: #564 --- diff --git a/alembic/ddl/mysql.py b/alembic/ddl/mysql.py index 504eb4c6..b5141499 100644 --- a/alembic/ddl/mysql.py +++ b/alembic/ddl/mysql.py @@ -44,13 +44,15 @@ class MySQLImpl(DefaultImpl): existing_comment=None, **kw ): - if name is not None: + if name is not None or self._is_mysql_allowed_functional_default( + type_ if type_ is not None else existing_type, server_default + ): self._exec( MySQLChangeColumn( table_name, column_name, schema=schema, - newname=name, + newname=name if name is not None else column_name, nullable=nullable if nullable is not None else existing_nullable @@ -107,6 +109,13 @@ class MySQLImpl(DefaultImpl): super(MySQLImpl, self).drop_constraint(const) + def _is_mysql_allowed_functional_default(self, type_, server_default): + return ( + type_ is not None + and type_._type_affinity is sqltypes.DateTime + and server_default is not None + ) + def compare_server_default( self, inspector_column, @@ -134,7 +143,29 @@ class MySQLImpl(DefaultImpl): ) return rendered_inspector_default != rendered_metadata_default elif rendered_inspector_default and rendered_metadata_default: - # adjust for "function()" vs. "FUNCTION" + # adjust for "function()" vs. "FUNCTION" as can occur particularly + # for the CURRENT_TIMESTAMP function on newer MariaDB versions + + # SQLAlchemy MySQL dialect bundles ON UPDATE into the server + # default; adjust for this possibly being present. + onupdate_ins = re.match( + r"(.*) (on update.*?)(?:\(\))?$", + rendered_inspector_default.lower(), + ) + onupdate_met = re.match( + r"(.*) (on update.*?)(?:\(\))?$", + rendered_metadata_default.lower(), + ) + + if onupdate_ins: + if not onupdate_met: + return True + elif onupdate_ins.group(2) != onupdate_met.group(2): + return True + + rendered_inspector_default = onupdate_ins.group(1) + rendered_metadata_default = onupdate_met.group(1) + return re.sub( r"(.*?)(?:\(\))?$", r"\1", rendered_inspector_default.lower() ) != re.sub( diff --git a/alembic/testing/fixtures.py b/alembic/testing/fixtures.py index f1dec6c6..59be5712 100644 --- a/alembic/testing/fixtures.py +++ b/alembic/testing/fixtures.py @@ -220,7 +220,7 @@ class AlterColRoundTripFixture(object): self.metadata.drop_all(self.conn) self.conn.close() - def _run_alter_col(self, from_, to_): + def _run_alter_col(self, from_, to_, compare=None): column = Column( from_.get("name", "colname"), from_.get("type", String(10)), @@ -254,15 +254,23 @@ class AlterColRoundTripFixture(object): insp = inspect(self.conn) new_col = insp.get_columns("x")[0] - eq_(new_col["name"], to_["name"] if "name" in to_ else column.name) - self._compare_type(new_col["type"], to_.get("type", old_col["type"])) - eq_(new_col["nullable"], to_.get("nullable", column.nullable)) + if compare is None: + compare = to_ + + eq_( + new_col["name"], + compare["name"] if "name" in compare else column.name, + ) + self._compare_type( + new_col["type"], compare.get("type", old_col["type"]) + ) + eq_(new_col["nullable"], compare.get("nullable", column.nullable)) self._compare_server_default( new_col["type"], new_col.get("default", None), - to_.get("type", old_col["type"]), - to_["server_default"].text - if "server_default" in to_ + compare.get("type", old_col["type"]), + compare["server_default"].text + if "server_default" in compare else column.server_default.arg.text if column.server_default is not None else None, diff --git a/docs/build/unreleased/564.rst b/docs/build/unreleased/564.rst new file mode 100644 index 00000000..2a90926d --- /dev/null +++ b/docs/build/unreleased/564.rst @@ -0,0 +1,19 @@ +.. change:: + :tags: bug, mysql, operations + :tickets: 564 + + Fixed issue where MySQL databases need to use CHANGE COLUMN when altering a + server default of CURRENT_TIMESTAMP, NOW() and probably other functions + that are only usable with DATETIME/TIMESTAMP columns. While MariaDB + supports both CHANGE and ALTER COLUMN in this case, MySQL databases only + support CHANGE. So the new logic is that if the server default change is + against a DateTime-oriented column, the CHANGE format is used + unconditionally, as in the vast majority of cases the server default is to + be CURRENT_TIMESTAMP which may also be potentially bundled with an "ON + UPDATE CURRENT_TIMESTAMP" directive, which SQLAlchemy does not currently + support as a distinct field. The fix addiionally improves the server + default comparison logic when the "ON UPDATE" clause is present and + there are parenthesis to be adjusted for as is the case on some MariaDB + versions. + + diff --git a/tests/test_mysql.py b/tests/test_mysql.py index 6e0db82e..fa857b57 100644 --- a/tests/test_mysql.py +++ b/tests/test_mysql.py @@ -1,5 +1,6 @@ from sqlalchemy import Boolean from sqlalchemy import Column +from sqlalchemy import DATETIME from sqlalchemy import func from sqlalchemy import Integer from sqlalchemy import MetaData @@ -15,6 +16,7 @@ from alembic.testing import assert_raises_message from alembic.testing import config from alembic.testing.env import clear_staging_env from alembic.testing.env import staging_env +from alembic.testing.fixtures import AlterColRoundTripFixture from alembic.testing.fixtures import op_fixture from alembic.testing.fixtures import TestBase @@ -178,6 +180,22 @@ class MySQLOpTest(TestBase): op.alter_column("t", "c", server_default="1") context.assert_("ALTER TABLE t ALTER COLUMN c SET DEFAULT '1'") + def test_alter_column_modify_datetime_default(self): + # use CHANGE format when the datatype is DATETIME or TIMESTAMP, + # as this is needed for a functional default which is what you'd + # get with a DATETIME/TIMESTAMP. Will also work in the very unlikely + # case the default is a fixed timestamp value. + context = op_fixture("mysql") + op.alter_column( + "t", + "c", + existing_type=DATETIME(), + server_default=text("CURRENT_TIMESTAMP"), + ) + context.assert_( + "ALTER TABLE t CHANGE c c DATETIME NULL DEFAULT CURRENT_TIMESTAMP" + ) + def test_col_not_nullable(self): context = op_fixture("mysql") op.alter_column("t1", "c1", nullable=False, existing_type=Integer) @@ -392,6 +410,66 @@ class MySQLOpTest(TestBase): ) +class MySQLBackendOpTest(AlterColRoundTripFixture, TestBase): + __only_on__ = "mysql" + __backend__ = True + + def test_add_timestamp_server_default_current_timestamp(self): + self._run_alter_col( + {"type": TIMESTAMP()}, + {"server_default": text("CURRENT_TIMESTAMP")}, + ) + + def test_add_datetime_server_default_current_timestamp(self): + self._run_alter_col( + {"type": DATETIME()}, {"server_default": text("CURRENT_TIMESTAMP")} + ) + + def test_add_timestamp_server_default_now(self): + self._run_alter_col( + {"type": TIMESTAMP()}, + {"server_default": text("NOW()")}, + compare={"server_default": text("CURRENT_TIMESTAMP")}, + ) + + def test_add_datetime_server_default_now(self): + self._run_alter_col( + {"type": DATETIME()}, + {"server_default": text("NOW()")}, + compare={"server_default": text("CURRENT_TIMESTAMP")}, + ) + + def test_add_timestamp_server_default_current_timestamp_bundle_onupdate( + self + ): + # note SQLAlchemy reflection bundles the ON UPDATE part into the + # server default reflection see + # https://github.com/sqlalchemy/sqlalchemy/issues/4652 + self._run_alter_col( + {"type": TIMESTAMP()}, + { + "server_default": text( + "CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP" + ) + }, + ) + + def test_add_datetime_server_default_current_timestamp_bundle_onupdate( + self + ): + # note SQLAlchemy reflection bundles the ON UPDATE part into the + # server default reflection see + # https://github.com/sqlalchemy/sqlalchemy/issues/4652 + self._run_alter_col( + {"type": DATETIME()}, + { + "server_default": text( + "CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP" + ) + }, + ) + + class MySQLDefaultCompareTest(TestBase): __only_on__ = "mysql" __backend__ = True @@ -463,6 +541,16 @@ class MySQLDefaultCompareTest(TestBase): def test_compare_timestamp_current_timestamp_diff(self): self._compare_default_roundtrip(TIMESTAMP(), None, "CURRENT_TIMESTAMP") + def test_compare_timestamp_current_timestamp_bundle_onupdate(self): + self._compare_default_roundtrip( + TIMESTAMP(), "CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP" + ) + + def test_compare_timestamp_current_timestamp_diff_bundle_onupdate(self): + self._compare_default_roundtrip( + TIMESTAMP(), None, "CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP" + ) + def test_compare_integer_from_none(self): self._compare_default_roundtrip(Integer(), None, "0") diff --git a/tests/test_op.py b/tests/test_op.py index 5bb379db..0312790b 100644 --- a/tests/test_op.py +++ b/tests/test_op.py @@ -1025,7 +1025,6 @@ class EnsureOrigObjectFromToTest(TestBase): ) -# MARKMARK class BackendAlterColumnTest(AlterColRoundTripFixture, TestBase): __backend__ = True