From ac5461f9a4efbb521a9aa69f96041cc342f21cb9 Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Mon, 30 Jan 2023 12:50:28 -0800 Subject: [PATCH 01/10] detect collation, remove it, add it back --- poetry.lock | 9 ++++----- singer_sdk/connectors/sql.py | 13 +++++++++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/poetry.lock b/poetry.lock index df426229f..ee6921b19 100644 --- a/poetry.lock +++ b/poetry.lock @@ -998,7 +998,6 @@ category = "main" optional = true python-versions = "*" files = [ - {file = "livereload-2.6.3-py2.py3-none-any.whl", hash = "sha256:ad4ac6f53b2d62bb6ce1a5e6e96f1f00976a32348afedcb4b6d68df2a1d346e4"}, {file = "livereload-2.6.3.tar.gz", hash = "sha256:776f2f865e59fde56490a56bcc6773b6917366bce0c267c60ee8aaf1a0959869"}, ] @@ -1583,7 +1582,7 @@ files = [ cffi = ">=1.4.1" [package.extras] -docs = ["sphinx (>=1.6.5)", "sphinx-rtd-theme"] +docs = ["sphinx (>=1.6.5)", "sphinx_rtd_theme"] tests = ["hypothesis (>=3.27.0)", "pytest (>=3.2.1,!=3.3.0)"] [[package]] @@ -2229,7 +2228,7 @@ importlib-metadata = {version = "*", markers = "python_version < \"3.8\""} [package.extras] aiomysql = ["aiomysql", "greenlet (!=0.4.17)"] -aiosqlite = ["aiosqlite", "greenlet (!=0.4.17)", "typing-extensions (!=3.10.0.1)"] +aiosqlite = ["aiosqlite", "greenlet (!=0.4.17)", "typing_extensions (!=3.10.0.1)"] asyncio = ["greenlet (!=0.4.17)"] asyncmy = ["asyncmy (>=0.2.3,!=0.2.4)", "greenlet (!=0.4.17)"] mariadb-connector = ["mariadb (>=1.0.1,!=1.1.2)"] @@ -2239,14 +2238,14 @@ mssql-pyodbc = ["pyodbc"] mypy = ["mypy (>=0.910)", "sqlalchemy2-stubs"] mysql = ["mysqlclient (>=1.4.0)", "mysqlclient (>=1.4.0,<2)"] mysql-connector = ["mysql-connector-python"] -oracle = ["cx-oracle (>=7)", "cx-oracle (>=7,<8)"] +oracle = ["cx_oracle (>=7)", "cx_oracle (>=7,<8)"] postgresql = ["psycopg2 (>=2.7)"] postgresql-asyncpg = ["asyncpg", "greenlet (!=0.4.17)"] postgresql-pg8000 = ["pg8000 (>=1.16.6,!=1.29.0)"] postgresql-psycopg2binary = ["psycopg2-binary"] postgresql-psycopg2cffi = ["psycopg2cffi"] pymysql = ["pymysql", "pymysql (<1)"] -sqlcipher = ["sqlcipher3-binary"] +sqlcipher = ["sqlcipher3_binary"] [[package]] name = "sqlalchemy2-stubs" diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 5aae327ab..714ce461f 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -959,10 +959,18 @@ def _adapt_column_type( Raises: NotImplementedError: if altering columns is not supported. """ + collation_removed = False current_type: sqlalchemy.types.TypeEngine = self._get_column_type( full_table_name, column_name ) + # remove collation if present and save it + if hasattr(current_type, "collation"): + if current_type.collation: + current_type_collation = current_type.collation + setattr(current_type, "collation", None) + collation_removed = True + # Check if the existing column type and the sql type are the same if str(sql_type) == str(current_type): # The current column and sql type are the same @@ -977,6 +985,11 @@ def _adapt_column_type( # Nothing to do return + # Put the collation level back before altering the column + if hasattr(compatible_sql_type, "collation"): + if collation_removed: + setattr(compatible_sql_type, "collation", current_type_collation) + if not self.allow_column_alter: raise NotImplementedError( "Altering columns is not supported. " From 778a7159d8e2722ea2a226bdbdad1dc7f54c09f9 Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Fri, 3 Feb 2023 09:45:41 -0800 Subject: [PATCH 02/10] added remove_collation and update_collation functions --- singer_sdk/connectors/sql.py | 57 ++++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 714ce461f..f4af52bae 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -943,6 +943,41 @@ def get_column_alter_ddl( }, ) + @staticmethod + def remove_collation( + column_type: sqlalchemy.types.TypeEngine, + ) -> tuple[sqlalchemy.types.TypeEngine, str | None]: + """Removes collation for the given column TypeEngine instance. + + Args: + column_type: Column SQLAlchemy type. + + Returns: + A tuple containing the revised column type and the removed collation. + """ + if hasattr(column_type, "collation") and column_type.collation: + column_type_collation = column_type.collation + setattr(column_type, "collation", None) + return column_type, column_type_collation + return column_type, None + + @staticmethod + def update_collation( + column_type: sqlalchemy.types.TypeEngine, collation: str | None + ) -> sqlalchemy.types.TypeEngine: + """Sets column collation if column type has a collation attribute. + + Args: + column_type: Column SQLAlchemy type. + collation: The colation + + Returns: + The revised column type with passed collation or unchanged column type. + """ + if hasattr(column_type, "collation") and collation: + setattr(column_type, "collation", collation) + return column_type + def _adapt_column_type( self, full_table_name: str, @@ -959,17 +994,18 @@ def _adapt_column_type( Raises: NotImplementedError: if altering columns is not supported. """ - collation_removed = False + # collation_removed = False current_type: sqlalchemy.types.TypeEngine = self._get_column_type( full_table_name, column_name ) # remove collation if present and save it - if hasattr(current_type, "collation"): - if current_type.collation: - current_type_collation = current_type.collation - setattr(current_type, "collation", None) - collation_removed = True + current_type, current_type_collation = self.remove_collation(current_type) + # if hasattr(current_type, "collation"): + # if current_type.collation: + # current_type_collation = current_type.collation + # setattr(current_type, "collation", None) + # collation_removed = True # Check if the existing column type and the sql type are the same if str(sql_type) == str(current_type): @@ -986,9 +1022,12 @@ def _adapt_column_type( return # Put the collation level back before altering the column - if hasattr(compatible_sql_type, "collation"): - if collation_removed: - setattr(compatible_sql_type, "collation", current_type_collation) + compatible_sql_type = self.update_collation( + compatible_sql_type, current_type_collation + ) + # if hasattr(compatible_sql_type, "collation"): + # if collation_removed: + # setattr(compatible_sql_type, "collation", current_type_collation) if not self.allow_column_alter: raise NotImplementedError( From 674fdcd57c151a5462265dec47cb3b82773e26bc Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Fri, 3 Feb 2023 11:52:06 -0800 Subject: [PATCH 03/10] tests for remove_collation and update_collation --- tests/core/test_connector_sql.py | 39 ++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/core/test_connector_sql.py b/tests/core/test_connector_sql.py index 518b17bdf..36480ad04 100644 --- a/tests/core/test_connector_sql.py +++ b/tests/core/test_connector_sql.py @@ -91,3 +91,42 @@ def test_get_column_ddl( ) ) assert statement == rendered_statement + + def test_remove_collation_text_type(self, connector): + remove_collation = getattr(connector, "remove_collation") + test_collation = "SQL_Latin1_General_CP1_CI_AS" + current_type = sqlalchemy.types.Text(collation=test_collation) + current_type, current_type_collation = remove_collation(current_type) + # Check collation was set to None by the function + assert current_type.collation == None + # Check that we get the same collation we put in back out + assert current_type_collation == test_collation + + def test_remove_collation_non_text_type(self, connector): + remove_collation = getattr(connector, "remove_collation") + current_type = sqlalchemy.types.Integer() + current_type, current_type_collation = remove_collation(current_type) + # Check there is not a collation attribute + assert hasattr(current_type, "collation") == False + # Check that we get the same type we put in + assert str(current_type) == "INTEGER" + # Check that this variable is missing + assert not current_type_collation + + def test_update_collation_text_type(self, connector): + update_collation = getattr(connector, "update_collation") + test_collation = "SQL_Latin1_General_CP1_CI_AS" + compatible_type = sqlalchemy.types.Text(collation=None) + compatible_type = update_collation(compatible_type, test_collation) + # Check collation was set to the value we put in + assert compatible_type.collation == test_collation + + def test_update_collation_non_text_type(self, connector): + update_collation = getattr(connector, "update_collation") + test_collation = "SQL_Latin1_General_CP1_CI_AS" + compatible_type = sqlalchemy.types.Integer() + compatible_type = update_collation(compatible_type, test_collation) + # Check there is not a collation attribute + assert hasattr(compatible_type, "collation") == False + # Check that we get the same type we put in + assert str(compatible_type) == "INTEGER" From 9b72f3cfd7244174850db31faacd55b632b4924d Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Fri, 3 Feb 2023 12:18:02 -0800 Subject: [PATCH 04/10] added if statement and tidied up --- singer_sdk/connectors/sql.py | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index f4af52bae..476ea1b14 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -1001,11 +1001,6 @@ def _adapt_column_type( # remove collation if present and save it current_type, current_type_collation = self.remove_collation(current_type) - # if hasattr(current_type, "collation"): - # if current_type.collation: - # current_type_collation = current_type.collation - # setattr(current_type, "collation", None) - # collation_removed = True # Check if the existing column type and the sql type are the same if str(sql_type) == str(current_type): @@ -1022,12 +1017,10 @@ def _adapt_column_type( return # Put the collation level back before altering the column - compatible_sql_type = self.update_collation( - compatible_sql_type, current_type_collation - ) - # if hasattr(compatible_sql_type, "collation"): - # if collation_removed: - # setattr(compatible_sql_type, "collation", current_type_collation) + if current_type_collation: + compatible_sql_type = self.update_collation( + compatible_sql_type, current_type_collation + ) if not self.allow_column_alter: raise NotImplementedError( From 931d953f3246afcb8850413bebc0b21820206a12 Mon Sep 17 00:00:00 2001 From: Dan Norman Date: Fri, 3 Feb 2023 14:06:29 -0700 Subject: [PATCH 05/10] suggestions from code review Co-authored-by: Edgar R. M. --- tests/core/test_connector_sql.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/core/test_connector_sql.py b/tests/core/test_connector_sql.py index 36480ad04..7545b7dfa 100644 --- a/tests/core/test_connector_sql.py +++ b/tests/core/test_connector_sql.py @@ -92,41 +92,41 @@ def test_get_column_ddl( ) assert statement == rendered_statement - def test_remove_collation_text_type(self, connector): - remove_collation = getattr(connector, "remove_collation") + def test_remove_collation_text_type(self): + remove_collation = SQLConnector.remove_collation test_collation = "SQL_Latin1_General_CP1_CI_AS" current_type = sqlalchemy.types.Text(collation=test_collation) current_type, current_type_collation = remove_collation(current_type) # Check collation was set to None by the function - assert current_type.collation == None + assert current_type.collation is None # Check that we get the same collation we put in back out assert current_type_collation == test_collation - def test_remove_collation_non_text_type(self, connector): - remove_collation = getattr(connector, "remove_collation") + def test_remove_collation_non_text_type(self): + remove_collation = SQLConnector.remove_collation current_type = sqlalchemy.types.Integer() current_type, current_type_collation = remove_collation(current_type) # Check there is not a collation attribute - assert hasattr(current_type, "collation") == False + assert not hasattr(current_type, "collation") # Check that we get the same type we put in assert str(current_type) == "INTEGER" # Check that this variable is missing - assert not current_type_collation + assert current_type_collation is None - def test_update_collation_text_type(self, connector): - update_collation = getattr(connector, "update_collation") + def test_update_collation_text_type(self): + update_collation = SQLConnector.update_collation test_collation = "SQL_Latin1_General_CP1_CI_AS" compatible_type = sqlalchemy.types.Text(collation=None) compatible_type = update_collation(compatible_type, test_collation) # Check collation was set to the value we put in assert compatible_type.collation == test_collation - def test_update_collation_non_text_type(self, connector): - update_collation = getattr(connector, "update_collation") + def test_update_collation_non_text_type(self): + update_collation = SQLConnector.update_collation test_collation = "SQL_Latin1_General_CP1_CI_AS" compatible_type = sqlalchemy.types.Integer() compatible_type = update_collation(compatible_type, test_collation) # Check there is not a collation attribute - assert hasattr(compatible_type, "collation") == False + assert not hasattr(compatible_type, "collation") # Check that we get the same type we put in assert str(compatible_type) == "INTEGER" From d65064652584517f8f970e5f650be6840e5f2cd6 Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Fri, 3 Feb 2023 13:59:27 -0800 Subject: [PATCH 06/10] stopped returning the passed column type --- singer_sdk/connectors/sql.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 476ea1b14..ed36ed9dc 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -946,7 +946,8 @@ def get_column_alter_ddl( @staticmethod def remove_collation( column_type: sqlalchemy.types.TypeEngine, - ) -> tuple[sqlalchemy.types.TypeEngine, str | None]: + # ) -> tuple[sqlalchemy.types.TypeEngine, str | None]: + ) -> str | None: """Removes collation for the given column TypeEngine instance. Args: @@ -958,13 +959,17 @@ def remove_collation( if hasattr(column_type, "collation") and column_type.collation: column_type_collation = column_type.collation setattr(column_type, "collation", None) - return column_type, column_type_collation - return column_type, None + # return column_type, column_type_collation + # return column_type, None + return column_type_collation + return None @staticmethod def update_collation( - column_type: sqlalchemy.types.TypeEngine, collation: str | None - ) -> sqlalchemy.types.TypeEngine: + column_type: sqlalchemy.types.TypeEngine, + collation: str | None + # ) -> sqlalchemy.types.TypeEngine: + ) -> None: """Sets column collation if column type has a collation attribute. Args: @@ -1000,7 +1005,8 @@ def _adapt_column_type( ) # remove collation if present and save it - current_type, current_type_collation = self.remove_collation(current_type) + # current_type, current_type_collation = self.remove_collation(current_type) + current_type_collation = self.remove_collation(current_type) # Check if the existing column type and the sql type are the same if str(sql_type) == str(current_type): @@ -1018,9 +1024,10 @@ def _adapt_column_type( # Put the collation level back before altering the column if current_type_collation: - compatible_sql_type = self.update_collation( - compatible_sql_type, current_type_collation - ) + # compatible_sql_type = self.update_collation( + # compatible_sql_type, current_type_collation + # ) + self.update_collation(compatible_sql_type, current_type_collation) if not self.allow_column_alter: raise NotImplementedError( From 9c316f51bbd22b268d563325fe1d2e325ba7f116 Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Fri, 3 Feb 2023 14:07:21 -0800 Subject: [PATCH 07/10] updated tests reflect changes to remove_collation and update_collation --- tests/core/test_connector_sql.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/core/test_connector_sql.py b/tests/core/test_connector_sql.py index 7545b7dfa..8ad7bf9a1 100644 --- a/tests/core/test_connector_sql.py +++ b/tests/core/test_connector_sql.py @@ -96,7 +96,7 @@ def test_remove_collation_text_type(self): remove_collation = SQLConnector.remove_collation test_collation = "SQL_Latin1_General_CP1_CI_AS" current_type = sqlalchemy.types.Text(collation=test_collation) - current_type, current_type_collation = remove_collation(current_type) + current_type_collation = remove_collation(current_type) # Check collation was set to None by the function assert current_type.collation is None # Check that we get the same collation we put in back out @@ -105,7 +105,7 @@ def test_remove_collation_text_type(self): def test_remove_collation_non_text_type(self): remove_collation = SQLConnector.remove_collation current_type = sqlalchemy.types.Integer() - current_type, current_type_collation = remove_collation(current_type) + current_type_collation = remove_collation(current_type) # Check there is not a collation attribute assert not hasattr(current_type, "collation") # Check that we get the same type we put in @@ -117,7 +117,7 @@ def test_update_collation_text_type(self): update_collation = SQLConnector.update_collation test_collation = "SQL_Latin1_General_CP1_CI_AS" compatible_type = sqlalchemy.types.Text(collation=None) - compatible_type = update_collation(compatible_type, test_collation) + update_collation(compatible_type, test_collation) # Check collation was set to the value we put in assert compatible_type.collation == test_collation @@ -125,7 +125,7 @@ def test_update_collation_non_text_type(self): update_collation = SQLConnector.update_collation test_collation = "SQL_Latin1_General_CP1_CI_AS" compatible_type = sqlalchemy.types.Integer() - compatible_type = update_collation(compatible_type, test_collation) + update_collation(compatible_type, test_collation) # Check there is not a collation attribute assert not hasattr(compatible_type, "collation") # Check that we get the same type we put in From 95a88b2c6d275800c61492a6d5450e0b56713c68 Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Fri, 3 Feb 2023 14:39:28 -0800 Subject: [PATCH 08/10] tidied up a little --- singer_sdk/connectors/sql.py | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index ed36ed9dc..ee327813e 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -946,7 +946,6 @@ def get_column_alter_ddl( @staticmethod def remove_collation( column_type: sqlalchemy.types.TypeEngine, - # ) -> tuple[sqlalchemy.types.TypeEngine, str | None]: ) -> str | None: """Removes collation for the given column TypeEngine instance. @@ -954,34 +953,25 @@ def remove_collation( column_type: Column SQLAlchemy type. Returns: - A tuple containing the revised column type and the removed collation. + The removed collation as a string. """ if hasattr(column_type, "collation") and column_type.collation: column_type_collation = column_type.collation setattr(column_type, "collation", None) - # return column_type, column_type_collation - # return column_type, None return column_type_collation - return None @staticmethod def update_collation( - column_type: sqlalchemy.types.TypeEngine, - collation: str | None - # ) -> sqlalchemy.types.TypeEngine: + column_type: sqlalchemy.types.TypeEngine, collation: str | None ) -> None: """Sets column collation if column type has a collation attribute. Args: column_type: Column SQLAlchemy type. collation: The colation - - Returns: - The revised column type with passed collation or unchanged column type. """ if hasattr(column_type, "collation") and collation: setattr(column_type, "collation", collation) - return column_type def _adapt_column_type( self, @@ -999,13 +989,11 @@ def _adapt_column_type( Raises: NotImplementedError: if altering columns is not supported. """ - # collation_removed = False current_type: sqlalchemy.types.TypeEngine = self._get_column_type( full_table_name, column_name ) # remove collation if present and save it - # current_type, current_type_collation = self.remove_collation(current_type) current_type_collation = self.remove_collation(current_type) # Check if the existing column type and the sql type are the same @@ -1024,9 +1012,6 @@ def _adapt_column_type( # Put the collation level back before altering the column if current_type_collation: - # compatible_sql_type = self.update_collation( - # compatible_sql_type, current_type_collation - # ) self.update_collation(compatible_sql_type, current_type_collation) if not self.allow_column_alter: From 08eff69090139c91714eea9fd3850b2e7e6d2b1c Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Fri, 3 Feb 2023 14:53:35 -0800 Subject: [PATCH 09/10] mypy fix added return None to remove_collation --- singer_sdk/connectors/sql.py | 1 + 1 file changed, 1 insertion(+) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index ee327813e..609b88652 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -959,6 +959,7 @@ def remove_collation( column_type_collation = column_type.collation setattr(column_type, "collation", None) return column_type_collation + return None @staticmethod def update_collation( From fd0548bbeb2b4a71f91243bce96e05ae1f6a9a6a Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Fri, 3 Feb 2023 15:02:24 -0800 Subject: [PATCH 10/10] mypy fix remove_collation added str type hint to column_type_collation --- singer_sdk/connectors/sql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 609b88652..d3ec81261 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -956,7 +956,7 @@ def remove_collation( The removed collation as a string. """ if hasattr(column_type, "collation") and column_type.collation: - column_type_collation = column_type.collation + column_type_collation: str = column_type.collation setattr(column_type, "collation", None) return column_type_collation return None