From 197390d33e1257494ddf2529612fed191e035470 Mon Sep 17 00:00:00 2001 From: David Blain Date: Fri, 17 May 2024 17:32:09 +0200 Subject: [PATCH 1/5] fix: Don't log a warning message if placeholder is None and make sure if the placeholder is invalid that the warning message is logged correctly --- airflow/providers/common/sql/hooks/sql.py | 17 +++---- .../providers/common/sql/hooks/test_dbapi.py | 46 +++++++++++++++++++ 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/airflow/providers/common/sql/hooks/sql.py b/airflow/providers/common/sql/hooks/sql.py index 070bf0647cfc6..13c488bc0bef6 100644 --- a/airflow/providers/common/sql/hooks/sql.py +++ b/airflow/providers/common/sql/hooks/sql.py @@ -185,14 +185,15 @@ def __init__(self, *args, schema: str | None = None, log_sql: bool = True, **kwa def placeholder(self): conn = self.get_connection(getattr(self, self.conn_name_attr)) placeholder = conn.extra_dejson.get("placeholder") - if placeholder in SQL_PLACEHOLDERS: - return placeholder - self.log.warning( - "Placeholder defined in Connection '%s' is not listed in 'DEFAULT_SQL_PLACEHOLDERS' " - "and got ignored. Falling back to the default placeholder '%s'.", - placeholder, - self._placeholder, - ) + if placeholder: + if placeholder in SQL_PLACEHOLDERS: + return placeholder + self.log.warning( + "Placeholder defined in Connection '%s' is not listed in 'DEFAULT_SQL_PLACEHOLDERS' " + "and got ignored. Falling back to the default placeholder '%s'.", + self.conn_name_attr, + self._placeholder, + ) return self._placeholder def get_conn(self): diff --git a/tests/providers/common/sql/hooks/test_dbapi.py b/tests/providers/common/sql/hooks/test_dbapi.py index 872b7b360bf5a..b41a7a2137bac 100644 --- a/tests/providers/common/sql/hooks/test_dbapi.py +++ b/tests/providers/common/sql/hooks/test_dbapi.py @@ -421,6 +421,52 @@ def test_get_uri_without_auth_and_empty_host(self): ) assert self.db_hook.get_uri() == "conn-type://@:3306/schema?charset=utf-8" + def test_placeholder(self): + self.db_hook.get_connection = mock.MagicMock( + return_value=Connection( + conn_type="conn-type", + login=None, + password=None, + schema="schema", + port=3306, + ) + ) + assert self.db_hook.placeholder == "%s" + + def test_placeholder_with_valid_placeholder_in_extra(self): + self.db_hook.get_connection = mock.MagicMock( + return_value=Connection( + conn_type="conn-type", + login=None, + password=None, + schema="schema", + port=3306, + extra=json.dumps({"placeholder": "?"}), + ) + ) + assert self.db_hook.placeholder == "?" + + def test_placeholder_with_invalid_placeholder_in_extra(self): + self.db_hook.get_connection = mock.MagicMock( + return_value=Connection( + conn_type="conn-type", + login=None, + password=None, + schema="schema", + port=3306, + extra=json.dumps({"placeholder": "!"}), + ) + ) + + assert self.db_hook.placeholder == "%s" + + self.db_hook.log.warning.assert_called_once_with( + "Placeholder defined in Connection '%s' is not listed in 'DEFAULT_SQL_PLACEHOLDERS' " + "and got ignored. Falling back to the default placeholder '%s'.", + "test_conn_id", + self.db_hook._placeholder, + ) + def test_run_log(self): statement = "SQL" self.db_hook.run(statement) From 7ffef40f3a991bdcdbadaca41e1a75425c1161bb Mon Sep 17 00:00:00 2001 From: David Blain Date: Fri, 17 May 2024 17:40:33 +0200 Subject: [PATCH 2/5] refactor: Also make sure to verify that log.warning isn't invoked when placeholder is valid --- tests/providers/common/sql/hooks/test_dbapi.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/providers/common/sql/hooks/test_dbapi.py b/tests/providers/common/sql/hooks/test_dbapi.py index b41a7a2137bac..0f004cfa783c7 100644 --- a/tests/providers/common/sql/hooks/test_dbapi.py +++ b/tests/providers/common/sql/hooks/test_dbapi.py @@ -433,6 +433,8 @@ def test_placeholder(self): ) assert self.db_hook.placeholder == "%s" + self.db_hook.log.warning.assert_not_called() + def test_placeholder_with_valid_placeholder_in_extra(self): self.db_hook.get_connection = mock.MagicMock( return_value=Connection( @@ -446,6 +448,8 @@ def test_placeholder_with_valid_placeholder_in_extra(self): ) assert self.db_hook.placeholder == "?" + self.db_hook.log.warning.assert_not_called() + def test_placeholder_with_invalid_placeholder_in_extra(self): self.db_hook.get_connection = mock.MagicMock( return_value=Connection( From 7a95668d95f9263f59caba5ce9b051248b1a6341 Mon Sep 17 00:00:00 2001 From: David Blain Date: Fri, 17 May 2024 18:39:38 +0200 Subject: [PATCH 3/5] refactor: All assertions regarding the logging are now done through caplog --- .../providers/common/sql/hooks/test_dbapi.py | 61 +++++++++++-------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/tests/providers/common/sql/hooks/test_dbapi.py b/tests/providers/common/sql/hooks/test_dbapi.py index 0f004cfa783c7..4f62d7b87b01b 100644 --- a/tests/providers/common/sql/hooks/test_dbapi.py +++ b/tests/providers/common/sql/hooks/test_dbapi.py @@ -46,11 +46,11 @@ def setup_method(self, **kwargs): self.conn = mock.MagicMock() self.conn.cursor.return_value = self.cur self.conn.schema.return_value = "test_schema" + self.conn.extra_dejson = {} conn = self.conn class DbApiHookMock(DbApiHook): conn_name_attr = "test_conn_id" - log = mock.MagicMock(spec=logging.Logger) @classmethod def get_connection(cls, conn_id: str) -> Connection: @@ -63,6 +63,7 @@ def get_conn(self): self.db_hook_no_log_sql = DbApiHookMock(log_sql=False) self.db_hook_schema_override = DbApiHookMock(schema="schema-override") self.db_hook.supports_executemany = False + self.db_hook.log.setLevel(logging.DEBUG) def test_get_records(self): statement = "SQL" @@ -193,11 +194,12 @@ def test_insert_rows_replace_executemany_hana_dialect(self): sql = f"UPSERT {table} VALUES (%s) WITH PRIMARY KEY" self.cur.executemany.assert_any_call(sql, rows) - def test_insert_rows_as_generator(self): + def test_insert_rows_as_generator(self, caplog): table = "table" rows = [("What's",), ("up",), ("world",)] - self.db_hook.insert_rows(table, iter(rows)) + with caplog.at_level(logging.DEBUG): + self.db_hook.insert_rows(table, iter(rows)) assert self.conn.close.call_count == 1 assert self.cur.close.call_count == 1 @@ -205,18 +207,22 @@ def test_insert_rows_as_generator(self): sql = f"INSERT INTO {table} VALUES (%s)" - self.db_hook.log.debug.assert_called_with("Generated sql: %s", sql) - self.db_hook.log.info.assert_called_with("Done loading. Loaded a total of %s rows into %s", 3, table) + assert any(f"Generated sql: {sql}" in message for message in caplog.messages) + assert any( + f"Done loading. Loaded a total of 3 rows into {table}" in message + for message in caplog.messages + ) for row in rows: self.cur.execute.assert_any_call(sql, row) - def test_insert_rows_as_generator_supports_executemany(self): + def test_insert_rows_as_generator_supports_executemany(self, caplog): table = "table" rows = [("What's",), ("up",), ("world",)] - self.db_hook.supports_executemany = True - self.db_hook.insert_rows(table, iter(rows)) + with caplog.at_level(logging.DEBUG): + self.db_hook.supports_executemany = True + self.db_hook.insert_rows(table, iter(rows)) assert self.conn.close.call_count == 1 assert self.cur.close.call_count == 1 @@ -224,8 +230,13 @@ def test_insert_rows_as_generator_supports_executemany(self): sql = f"INSERT INTO {table} VALUES (%s)" - self.db_hook.log.debug.assert_called_with("Generated sql: %s", sql) - self.db_hook.log.info.assert_called_with("Done loading. Loaded a total of %s rows into %s", 3, table) + assert any(f"Generated sql: {sql}" in message for message in caplog.messages) + assert any(f"Loaded 3 rows into {table} so far" in message for message in caplog.messages) + assert any( + f"Done loading. Loaded a total of 3 rows into {table}" in message + for message in caplog.messages + ) + self.cur.executemany.assert_any_call(sql, rows) def test_get_uri_schema_not_none(self): @@ -421,7 +432,7 @@ def test_get_uri_without_auth_and_empty_host(self): ) assert self.db_hook.get_uri() == "conn-type://@:3306/schema?charset=utf-8" - def test_placeholder(self): + def test_placeholder(self, caplog): self.db_hook.get_connection = mock.MagicMock( return_value=Connection( conn_type="conn-type", @@ -432,10 +443,9 @@ def test_placeholder(self): ) ) assert self.db_hook.placeholder == "%s" + assert not caplog.messages - self.db_hook.log.warning.assert_not_called() - - def test_placeholder_with_valid_placeholder_in_extra(self): + def test_placeholder_with_valid_placeholder_in_extra(self, caplog): self.db_hook.get_connection = mock.MagicMock( return_value=Connection( conn_type="conn-type", @@ -447,10 +457,9 @@ def test_placeholder_with_valid_placeholder_in_extra(self): ) ) assert self.db_hook.placeholder == "?" + assert not caplog.messages - self.db_hook.log.warning.assert_not_called() - - def test_placeholder_with_invalid_placeholder_in_extra(self): + def test_placeholder_with_invalid_placeholder_in_extra(self, caplog): self.db_hook.get_connection = mock.MagicMock( return_value=Connection( conn_type="conn-type", @@ -463,23 +472,21 @@ def test_placeholder_with_invalid_placeholder_in_extra(self): ) assert self.db_hook.placeholder == "%s" - - self.db_hook.log.warning.assert_called_once_with( - "Placeholder defined in Connection '%s' is not listed in 'DEFAULT_SQL_PLACEHOLDERS' " - "and got ignored. Falling back to the default placeholder '%s'.", - "test_conn_id", - self.db_hook._placeholder, + assert any( + f"Placeholder defined in Connection 'test_conn_id' is not listed in 'DEFAULT_SQL_PLACEHOLDERS' " + "and got ignored. Falling back to the default placeholder '%s'." + in message for message in caplog.messages ) - def test_run_log(self): + def test_run_log(self, caplog): statement = "SQL" self.db_hook.run(statement) - assert self.db_hook.log.info.call_count == 2 + assert len(caplog.messages) == 2 - def test_run_no_log(self): + def test_run_no_log(self, caplog): statement = "SQL" self.db_hook_no_log_sql.run(statement) - assert self.db_hook_no_log_sql.log.info.call_count == 1 + assert len(caplog.messages) == 1 def test_run_with_handler(self): sql = "SQL" From e5c2b2e89eea35a83c6b3f9a0a2db8e018b970b4 Mon Sep 17 00:00:00 2001 From: David Blain Date: Fri, 17 May 2024 19:05:24 +0200 Subject: [PATCH 4/5] refactor: Reformatted logging assertions --- tests/providers/common/sql/hooks/test_dbapi.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/providers/common/sql/hooks/test_dbapi.py b/tests/providers/common/sql/hooks/test_dbapi.py index 4f62d7b87b01b..e4bda04f5803d 100644 --- a/tests/providers/common/sql/hooks/test_dbapi.py +++ b/tests/providers/common/sql/hooks/test_dbapi.py @@ -209,8 +209,7 @@ def test_insert_rows_as_generator(self, caplog): assert any(f"Generated sql: {sql}" in message for message in caplog.messages) assert any( - f"Done loading. Loaded a total of 3 rows into {table}" in message - for message in caplog.messages + f"Done loading. Loaded a total of 3 rows into {table}" in message for message in caplog.messages ) for row in rows: @@ -233,8 +232,7 @@ def test_insert_rows_as_generator_supports_executemany(self, caplog): assert any(f"Generated sql: {sql}" in message for message in caplog.messages) assert any(f"Loaded 3 rows into {table} so far" in message for message in caplog.messages) assert any( - f"Done loading. Loaded a total of 3 rows into {table}" in message - for message in caplog.messages + f"Done loading. Loaded a total of 3 rows into {table}" in message for message in caplog.messages ) self.cur.executemany.assert_any_call(sql, rows) @@ -473,7 +471,7 @@ def test_placeholder_with_invalid_placeholder_in_extra(self, caplog): assert self.db_hook.placeholder == "%s" assert any( - f"Placeholder defined in Connection 'test_conn_id' is not listed in 'DEFAULT_SQL_PLACEHOLDERS' " + "Placeholder defined in Connection 'test_conn_id' is not listed in 'DEFAULT_SQL_PLACEHOLDERS' " "and got ignored. Falling back to the default placeholder '%s'." in message for message in caplog.messages ) From 36c2ea948fa053bdfbbc0a329bffadbfd3c8f157 Mon Sep 17 00:00:00 2001 From: David Blain Date: Fri, 17 May 2024 21:49:18 +0200 Subject: [PATCH 5/5] refactor: Reformatted logging assertion --- tests/providers/common/sql/hooks/test_dbapi.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/providers/common/sql/hooks/test_dbapi.py b/tests/providers/common/sql/hooks/test_dbapi.py index e4bda04f5803d..86b7b76563773 100644 --- a/tests/providers/common/sql/hooks/test_dbapi.py +++ b/tests/providers/common/sql/hooks/test_dbapi.py @@ -472,8 +472,8 @@ def test_placeholder_with_invalid_placeholder_in_extra(self, caplog): assert self.db_hook.placeholder == "%s" assert any( "Placeholder defined in Connection 'test_conn_id' is not listed in 'DEFAULT_SQL_PLACEHOLDERS' " - "and got ignored. Falling back to the default placeholder '%s'." - in message for message in caplog.messages + "and got ignored. Falling back to the default placeholder '%s'." in message + for message in caplog.messages ) def test_run_log(self, caplog):