From 693c1b494022b2216a087a78b9d039001d9ef806 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 9 Apr 2021 17:02:18 -0700 Subject: [PATCH 1/4] log all errors from db create --- superset/commands/exceptions.py | 3 +++ superset/databases/commands/create.py | 5 +++- .../databases/commands_tests.py | 23 +++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/superset/commands/exceptions.py b/superset/commands/exceptions.py index 40c059765b6e7..432c1d4f6e8db 100644 --- a/superset/commands/exceptions.py +++ b/superset/commands/exceptions.py @@ -66,6 +66,9 @@ def add(self, exception: ValidationError) -> None: def add_list(self, exceptions: List[ValidationError]) -> None: self._invalid_exceptions.extend(exceptions) + def get_list_classnames(self) -> List[str]: + return [ex.__class__.__name__ for ex in self._invalid_exceptions] + def normalized_messages(self) -> Dict[Any, Any]: errors: Dict[Any, Any] = {} for exception in self._invalid_exceptions: diff --git a/superset/databases/commands/create.py b/superset/databases/commands/create.py index ffcb018c0678c..e91ccec45c591 100644 --- a/superset/databases/commands/create.py +++ b/superset/databases/commands/create.py @@ -92,6 +92,9 @@ def validate(self) -> None: exception = DatabaseInvalidError() exception.add_list(exceptions) event_logger.log_with_context( - action=f"db_connection_failed.{exception.__class__.__name__}" + action="db_connection_failed.{}.{}".format( + exception.__class__.__name__, + ".".join(exception.get_list_classnames()), + ) ) raise exception diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py index 199b08ce108ce..536a4bcc2e662 100644 --- a/tests/integration_tests/databases/commands_tests.py +++ b/tests/integration_tests/databases/commands_tests.py @@ -26,7 +26,9 @@ from superset.commands.exceptions import CommandInvalidError from superset.commands.importers.exceptions import IncorrectVersionError from superset.connectors.sqla.models import SqlaTable +from superset.databases.commands.create import CreateDatabaseCommand from superset.databases.commands.exceptions import ( + DatabaseInvalidError, DatabaseNotFoundError, DatabaseSecurityUnsafeError, DatabaseTestConnectionDriverError, @@ -64,6 +66,27 @@ ) +class TestCreateDatabaseCommand(SupersetTestCase): + @mock.patch( + "superset.databases.commands.test_connection.event_logger.log_with_context" + ) + def test_create_duplicate_error(self, mock_logger): + example_db = get_example_database() + command = CreateDatabaseCommand( + security_manager.find_user("admin"), + {"database_name": example_db.database_name}, + ) + with pytest.raises(DatabaseInvalidError) as excinfo: + command.run() + assert str(excinfo.value) == ("Database parameters are invalid.") + # logger should list classnames of all errors + mock_logger.assert_called_with( + action="db_connection_failed.DatabaseInvalidError." + "DatabaseRequiredFieldValidationError." + "DatabaseExistsValidationError" + ) + + class TestExportDatabasesCommand(SupersetTestCase): @skip("Flaky") @patch("superset.security.manager.g") From 0cdc02dd39a11d23f2ea56d1ed737453eedfab6b Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 16 Apr 2021 19:01:10 -0700 Subject: [PATCH 2/4] return unique set of errors --- superset/commands/exceptions.py | 4 +++- .../databases/commands_tests.py | 18 +++++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/superset/commands/exceptions.py b/superset/commands/exceptions.py index 432c1d4f6e8db..4a021e62c766d 100644 --- a/superset/commands/exceptions.py +++ b/superset/commands/exceptions.py @@ -67,7 +67,9 @@ def add_list(self, exceptions: List[ValidationError]) -> None: self._invalid_exceptions.extend(exceptions) def get_list_classnames(self) -> List[str]: - return [ex.__class__.__name__ for ex in self._invalid_exceptions] + return list( + dict.fromkeys([ex.__class__.__name__ for ex in self._invalid_exceptions]) + ) def normalized_messages(self) -> Dict[Any, Any]: errors: Dict[Any, Any] = {} diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py index 536a4bcc2e662..fb775bbd10570 100644 --- a/tests/integration_tests/databases/commands_tests.py +++ b/tests/integration_tests/databases/commands_tests.py @@ -81,11 +81,27 @@ def test_create_duplicate_error(self, mock_logger): assert str(excinfo.value) == ("Database parameters are invalid.") # logger should list classnames of all errors mock_logger.assert_called_with( - action="db_connection_failed.DatabaseInvalidError." + action="db_connection_failed." + "DatabaseInvalidError." "DatabaseRequiredFieldValidationError." "DatabaseExistsValidationError" ) + @mock.patch( + "superset.databases.commands.test_connection.event_logger.log_with_context" + ) + def test_multiple_error_logging(self, mock_logger): + command = CreateDatabaseCommand(security_manager.find_user("admin"), {}) + with pytest.raises(DatabaseInvalidError) as excinfo: + command.run() + assert str(excinfo.value) == ("Database parameters are invalid.") + # logger should list a unique set of errors with no duplicates + mock_logger.assert_called_with( + action="db_connection_failed." + "DatabaseInvalidError." + "DatabaseRequiredFieldValidationError" + ) + class TestExportDatabasesCommand(SupersetTestCase): @skip("Flaky") From 1d29e9f3019f65946a44bba9435e6d6656265f04 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Thu, 23 Sep 2021 14:20:44 -0700 Subject: [PATCH 3/4] sort set for exceptions list --- superset/commands/exceptions.py | 2 +- tests/integration_tests/databases/commands_tests.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/superset/commands/exceptions.py b/superset/commands/exceptions.py index 4a021e62c766d..0190af329770b 100644 --- a/superset/commands/exceptions.py +++ b/superset/commands/exceptions.py @@ -68,7 +68,7 @@ def add_list(self, exceptions: List[ValidationError]) -> None: def get_list_classnames(self) -> List[str]: return list( - dict.fromkeys([ex.__class__.__name__ for ex in self._invalid_exceptions]) + sorted({ ex.__class__.__name__ for ex in self._invalid_exceptions}) ) def normalized_messages(self) -> Dict[Any, Any]: diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py index fb775bbd10570..c90550ed69903 100644 --- a/tests/integration_tests/databases/commands_tests.py +++ b/tests/integration_tests/databases/commands_tests.py @@ -83,8 +83,8 @@ def test_create_duplicate_error(self, mock_logger): mock_logger.assert_called_with( action="db_connection_failed." "DatabaseInvalidError." - "DatabaseRequiredFieldValidationError." - "DatabaseExistsValidationError" + "DatabaseExistsValidationError." + "DatabaseRequiredFieldValidationError" ) @mock.patch( From 910fa8b502ca94b4a952d7d308c7b81364dd7ed3 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Tue, 1 Mar 2022 15:42:45 -0800 Subject: [PATCH 4/4] run black --- superset/commands/exceptions.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/superset/commands/exceptions.py b/superset/commands/exceptions.py index 0190af329770b..8b4b717f31d72 100644 --- a/superset/commands/exceptions.py +++ b/superset/commands/exceptions.py @@ -67,9 +67,7 @@ def add_list(self, exceptions: List[ValidationError]) -> None: self._invalid_exceptions.extend(exceptions) def get_list_classnames(self) -> List[str]: - return list( - sorted({ ex.__class__.__name__ for ex in self._invalid_exceptions}) - ) + return list(sorted({ex.__class__.__name__ for ex in self._invalid_exceptions})) def normalized_messages(self) -> Dict[Any, Any]: errors: Dict[Any, Any] = {}