From 713d1155845326c16d964c1828693138033484f2 Mon Sep 17 00:00:00 2001 From: Laszlo Magyar Date: Thu, 11 Mar 2021 10:48:39 +0100 Subject: [PATCH 01/10] Deal with event names longer than 32 chars --- homeassistant/components/recorder/migration.py | 2 ++ homeassistant/components/recorder/models.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/homeassistant/components/recorder/migration.py b/homeassistant/components/recorder/migration.py index 5ab2d9091727a9..3280ff2cc11a39 100644 --- a/homeassistant/components/recorder/migration.py +++ b/homeassistant/components/recorder/migration.py @@ -377,6 +377,8 @@ def _apply_update(engine, new_version, old_version): "created DATETIME(6)", ], ) + elif new_version == 14: + _modify_columns(engine, "events", ["event_type VARCHAR(64)"]) else: raise ValueError(f"No schema migration defined for version {new_version}") diff --git a/homeassistant/components/recorder/models.py b/homeassistant/components/recorder/models.py index a547f3151338b3..207c205a623978 100644 --- a/homeassistant/components/recorder/models.py +++ b/homeassistant/components/recorder/models.py @@ -53,7 +53,7 @@ class Events(Base): # type: ignore } __tablename__ = TABLE_EVENTS event_id = Column(Integer, primary_key=True) - event_type = Column(String(32)) + event_type = Column(String(64)) event_data = Column(Text().with_variant(mysql.LONGTEXT, "mysql")) origin = Column(String(32)) time_fired = Column(DATETIME_TYPE, index=True) From 21442e0c0060121e3664d0aec356e87375d49002 Mon Sep 17 00:00:00 2001 From: Laszlo Magyar Date: Fri, 12 Mar 2021 19:24:27 +0100 Subject: [PATCH 02/10] Handle sqlite, postgresql, mssql skip sqlite add special handling for postgresql and mssql default works for mysql and mariadb (and should work for oracle) --- homeassistant/components/recorder/migration.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/homeassistant/components/recorder/migration.py b/homeassistant/components/recorder/migration.py index 3280ff2cc11a39..e2a9dc8e7470e2 100644 --- a/homeassistant/components/recorder/migration.py +++ b/homeassistant/components/recorder/migration.py @@ -206,6 +206,15 @@ def _add_columns(engine, table_name, columns_def): def _modify_columns(engine, table_name, columns_def): """Modify columns in a table.""" + if engine.dialect.name == "sqlite": + _LOGGER.warning( + "Modifying columns in %s is not supported. " + "If you have issues with the database, " + "you can recreate the database or modify it manually.", + engine.dialect.name + ) + return + _LOGGER.warning( "Modifying columns %s in table %s. Note: this can take several " "minutes on large databases and slow computers. Please " @@ -213,7 +222,13 @@ def _modify_columns(engine, table_name, columns_def): ", ".join(column.split(" ")[0] for column in columns_def), table_name, ) - columns_def = [f"MODIFY {col_def}" for col_def in columns_def] + + if engine.dialect.name == "postgresql": + columns_def = ["ALTER {column} TYPE {type}".format(**dict(zip(["column", "type"], col_def.split(" ", 1)))) for col_def in columns_def] + elif engine.dialect.name == "mssql": + columns_def = [f"ALTER COLUMN {col_def}" for col_def in columns_def] + else: + columns_def = [f"MODIFY {col_def}" for col_def in columns_def] try: engine.execute( From 7405257c8234e2557bd57a663042b5e46b5fdbb5 Mon Sep 17 00:00:00 2001 From: Laszlo Magyar Date: Fri, 12 Mar 2021 20:31:21 +0100 Subject: [PATCH 03/10] black formatted --- homeassistant/components/recorder/migration.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/homeassistant/components/recorder/migration.py b/homeassistant/components/recorder/migration.py index e2a9dc8e7470e2..00b8c786363cfd 100644 --- a/homeassistant/components/recorder/migration.py +++ b/homeassistant/components/recorder/migration.py @@ -211,7 +211,7 @@ def _modify_columns(engine, table_name, columns_def): "Modifying columns in %s is not supported. " "If you have issues with the database, " "you can recreate the database or modify it manually.", - engine.dialect.name + engine.dialect.name, ) return @@ -224,7 +224,12 @@ def _modify_columns(engine, table_name, columns_def): ) if engine.dialect.name == "postgresql": - columns_def = ["ALTER {column} TYPE {type}".format(**dict(zip(["column", "type"], col_def.split(" ", 1)))) for col_def in columns_def] + columns_def = [ + "ALTER {column} TYPE {type}".format( + **dict(zip(["column", "type"], col_def.split(" ", 1))) + ) + for col_def in columns_def + ] elif engine.dialect.name == "mssql": columns_def = [f"ALTER COLUMN {col_def}" for col_def in columns_def] else: From c31cd8176baefef542f1e4dd59c5691524b0c3b2 Mon Sep 17 00:00:00 2001 From: Laszlo Magyar Date: Sat, 13 Mar 2021 00:14:40 +0100 Subject: [PATCH 04/10] Skip SQLite altogether. --- homeassistant/components/recorder/migration.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/homeassistant/components/recorder/migration.py b/homeassistant/components/recorder/migration.py index 00b8c786363cfd..82c7ad436f2483 100644 --- a/homeassistant/components/recorder/migration.py +++ b/homeassistant/components/recorder/migration.py @@ -207,11 +207,12 @@ def _add_columns(engine, table_name, columns_def): def _modify_columns(engine, table_name, columns_def): """Modify columns in a table.""" if engine.dialect.name == "sqlite": - _LOGGER.warning( - "Modifying columns in %s is not supported. " - "If you have issues with the database, " - "you can recreate the database or modify it manually.", - engine.dialect.name, + _LOGGER.debug( + "Skipping to modify columns %s in table %s. " + "Modifying column length in SQLite is unnecessary, " + "it does not impose any length restrictions.", + ", ".join(column.split(" ")[0] for column in columns_def), + table_name, ) return From da99004e7b10c33ad1d7f7269cb7f4fd82538450 Mon Sep 17 00:00:00 2001 From: raman325 <7243222+raman325@users.noreply.github.com> Date: Tue, 6 Apr 2021 18:27:51 -0400 Subject: [PATCH 05/10] add test to verify the output of the function --- tests/components/recorder/test_migrate.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/components/recorder/test_migrate.py b/tests/components/recorder/test_migrate.py index 3bde17ab8ef102..bf7be19d588bea 100644 --- a/tests/components/recorder/test_migrate.py +++ b/tests/components/recorder/test_migrate.py @@ -76,6 +76,26 @@ def test_invalid_update(): migration._apply_update(None, -1, 0) +@pytest.mark.parametrize( + ["engine_type", "substr"], + [ + ("postgresql", "ALTER event_type TYPE VARCHAR(64)"), + ("mssql", "ALTER COLUMN event_type VARCHAR(64)"), + ("mysql", "MODIFY event_type VARCHAR(64)"), + ("sqlite", None), + ], +) +def test_modify_column(engine_type, substr): + """Test that add column will continue if column exists.""" + engine = Mock() + engine.dialect.name = engine_type + migration._modify_columns(engine, "events", ["event_type VARCHAR(64)"]) + if substr is not None: + assert substr in engine.execute.call_args[0][0].text + else: + not engine.execute.called + + def test_forgiving_add_column(): """Test that add column will continue if column exists.""" engine = create_engine("sqlite://", poolclass=StaticPool) From a39146046c59af12630d3cc54da15ed996e545ed Mon Sep 17 00:00:00 2001 From: raman325 <7243222+raman325@users.noreply.github.com> Date: Tue, 6 Apr 2021 18:33:13 -0400 Subject: [PATCH 06/10] fix logic --- tests/components/recorder/test_migrate.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/components/recorder/test_migrate.py b/tests/components/recorder/test_migrate.py index bf7be19d588bea..e3e9090a54a42c 100644 --- a/tests/components/recorder/test_migrate.py +++ b/tests/components/recorder/test_migrate.py @@ -90,10 +90,10 @@ def test_modify_column(engine_type, substr): engine = Mock() engine.dialect.name = engine_type migration._modify_columns(engine, "events", ["event_type VARCHAR(64)"]) - if substr is not None: + if substr: assert substr in engine.execute.call_args[0][0].text else: - not engine.execute.called + assert not engine.execute.called def test_forgiving_add_column(): From d3bf41723db3e90550250d0d910d8e61d609abe0 Mon Sep 17 00:00:00 2001 From: raman325 <7243222+raman325@users.noreply.github.com> Date: Tue, 6 Apr 2021 18:33:46 -0400 Subject: [PATCH 07/10] fix docstring --- tests/components/recorder/test_migrate.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/components/recorder/test_migrate.py b/tests/components/recorder/test_migrate.py index e3e9090a54a42c..c4e0d32adcf066 100644 --- a/tests/components/recorder/test_migrate.py +++ b/tests/components/recorder/test_migrate.py @@ -86,7 +86,7 @@ def test_invalid_update(): ], ) def test_modify_column(engine_type, substr): - """Test that add column will continue if column exists.""" + """Test that modify column generates the expected query.""" engine = Mock() engine.dialect.name = engine_type migration._modify_columns(engine, "events", ["event_type VARCHAR(64)"]) From 34859537633bd0f4f21200b02876038606c5bf41 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 7 Apr 2021 16:18:20 -1000 Subject: [PATCH 08/10] fix schema version from merge --- homeassistant/components/recorder/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/components/recorder/models.py b/homeassistant/components/recorder/models.py index 207c205a623978..b26c523ce40316 100644 --- a/homeassistant/components/recorder/models.py +++ b/homeassistant/components/recorder/models.py @@ -26,7 +26,7 @@ # pylint: disable=invalid-name Base = declarative_base() -SCHEMA_VERSION = 13 +SCHEMA_VERSION = 14 _LOGGER = logging.getLogger(__name__) From 5636c0d112bbcf3e783a1d2a7b9dfa91b92c42a8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 7 Apr 2021 16:28:02 -1000 Subject: [PATCH 09/10] Update homeassistant/components/recorder/migration.py --- homeassistant/components/recorder/migration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/components/recorder/migration.py b/homeassistant/components/recorder/migration.py index 82c7ad436f2483..ee363ab44c5663 100644 --- a/homeassistant/components/recorder/migration.py +++ b/homeassistant/components/recorder/migration.py @@ -208,7 +208,7 @@ def _modify_columns(engine, table_name, columns_def): """Modify columns in a table.""" if engine.dialect.name == "sqlite": _LOGGER.debug( - "Skipping to modify columns %s in table %s. " + "Skipping to modify columns %s in table %s; " "Modifying column length in SQLite is unnecessary, " "it does not impose any length restrictions.", ", ".join(column.split(" ")[0] for column in columns_def), From 76f9cd11500c9a0b23391d3c1e585321ff569a05 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 7 Apr 2021 16:28:06 -1000 Subject: [PATCH 10/10] Update homeassistant/components/recorder/migration.py --- homeassistant/components/recorder/migration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/components/recorder/migration.py b/homeassistant/components/recorder/migration.py index ee363ab44c5663..fa93f6155617c5 100644 --- a/homeassistant/components/recorder/migration.py +++ b/homeassistant/components/recorder/migration.py @@ -210,7 +210,7 @@ def _modify_columns(engine, table_name, columns_def): _LOGGER.debug( "Skipping to modify columns %s in table %s; " "Modifying column length in SQLite is unnecessary, " - "it does not impose any length restrictions.", + "it does not impose any length restrictions", ", ".join(column.split(" ")[0] for column in columns_def), table_name, )