betodealmeida commented on a change in pull request #13468:
URL: https://github.com/apache/superset/pull/13468#discussion_r589493114
##########
File path: superset/databases/commands/create.py
##########
@@ -50,9 +50,13 @@ def run(self) -> Model:
try:
TestConnectionDatabaseCommand(self._actor,
self._properties).run()
- except Exception:
- db.session.rollback()
- raise DatabaseConnectionFailedError()
+ except Exception: # pylint: disable=broad-except
+ with event_logger(
+ action="db_connection_failed",
+ engine=database.db_engine_spec.__name__,
+ ):
+ db.session.rollback()
+ raise DatabaseConnectionFailedError()
Review comment:
For places where we do a rollback we should it **outside** the context
manager, otherwise if the logging fails the transaction won't be rolled back:
```suggestion
db.session.rollback()
event_logger.log_with_context(
action="db_connection_failed",
engine=database.db_engine_spec.__name__,
)
raise DatabaseConnectionFailedError()
```
Reversing the order ensures that even if the logging fails the rollback will
still go through.
##########
File path: tests/databases/commands_tests.py
##########
@@ -508,3 +520,68 @@ def test_import_v1_rollback(self, mock_import_dataset):
# verify that the database was not added
new_num_databases = db.session.query(Database).count()
assert new_num_databases == num_databases
+
+
+class TestTestConnectionDatabaseCommand(SupersetTestCase):
+
@mock.patch("superset.databases.dao.DatabaseDAO.build_db_for_connection_test")
+ @mock.patch("superset.databases.commands.test_connection.event_logger")
+ def test_connection_db_exception(
+ self, mock_event_logger, mock_build_db_for_connection_test
+ ):
+ """Test that users can't export databases they don't have access to"""
Review comment:
Update docstring here and in the other 2 tests 😄
##########
File path: superset/databases/commands/test_connection.py
##########
@@ -55,25 +56,44 @@ def run(self) -> None:
impersonate_user=self._properties.get("impersonate_user",
False),
encrypted_extra=self._properties.get("encrypted_extra", "{}"),
)
- if database is not None:
- database.set_sqlalchemy_uri(uri)
- database.db_engine_spec.mutate_db_for_connection_test(database)
- username = self._actor.username if self._actor is not None
else None
- engine = database.get_sqla_engine(user_name=username)
+ if database is None:
+ raise DBAPIError("Database is not found", None, None)
Review comment:
We can actually remove this and fix the type annotation in
`DatabaseDAO.build_db_for_connection_test`, the class method never returns
`None`.
##########
File path: superset/databases/commands/create.py
##########
@@ -63,8 +67,11 @@ def run(self) -> Model:
security_manager.add_permission_view_menu("database_access",
database.perm)
db.session.commit()
except DAOCreateFailedError as ex:
- logger.exception(ex.exception)
- raise DatabaseCreateFailedError()
+ with event_logger(
+ action=f"db_creation_failed.{ex.__class__.__name__}",
+ engine=database.db_engine_spec.__name__,
+ ):
+ raise DatabaseCreateFailedError()
Review comment:
Total nit, but in places where we just raise an exception inside the
context manager like here and in lines 94-97 I think it would make more sense
to use the `log_with_context` function instead:
```suggestion
event_logger.log_with_context(
action=f"db_creation_failed.{ex.__class__.__name__}",
engine=database.db_engine_spec.__name__,
)
raise DatabaseCreateFailedError()
```
##########
File path: tests/databases/commands_tests.py
##########
@@ -508,3 +520,68 @@ def test_import_v1_rollback(self, mock_import_dataset):
# verify that the database was not added
new_num_databases = db.session.query(Database).count()
assert new_num_databases == num_databases
+
+
+class TestTestConnectionDatabaseCommand(SupersetTestCase):
+
@mock.patch("superset.databases.dao.DatabaseDAO.build_db_for_connection_test")
+ @mock.patch("superset.databases.commands.test_connection.event_logger")
+ def test_connection_db_exception(
+ self, mock_event_logger, mock_build_db_for_connection_test
+ ):
+ """Test that users can't export databases they don't have access to"""
+ database = get_example_database()
+ mock_build_db_for_connection_test.side_effect = Exception(
+ "An error has occurred!"
+ )
+ db_uri = database.sqlalchemy_uri_decrypted
+ json_payload = {"sqlalchemy_uri": db_uri}
+ command_without_db_name = TestConnectionDatabaseCommand(
+ security_manager.find_user("admin"), json_payload
+ )
+
+ with self.assertRaises(DatabaseTestConnectionUnexpectedError):
+ command_without_db_name.run()
Review comment:
Now that we're using `pytest`, it's better to run tests using plain
`assert`s instead, since it gives more context when the test fails. Also, we
can check that the exception raised has the right error message:
```suggestion
with pytest.raises(DatabaseTestConnectionUnexpectedError) as excinfo:
command_without_db_name.run()
assert str(excinfo.value) == (
"Unexpected error occurred, please check your logs for details"
)
```
Same in the other 2 tests below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
