You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by va...@apache.org on 2024/12/13 14:45:33 UTC
(superset) branch fix/database-import created (now 9bcc11f50e)
This is an automated email from the ASF dual-hosted git repository.
vavila pushed a change to branch fix/database-import
in repository https://gitbox.apache.org/repos/asf/superset.git
at 9bcc11f50e fix(database import): Gracefully handle error to get catalog schemas
This branch includes the following new commits:
new 9bcc11f50e fix(database import): Gracefully handle error to get catalog schemas
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: fix(database import): Gracefully handle error to get catalog schemas
Posted by va...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
vavila pushed a commit to branch fix/database-import
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 9bcc11f50e3b3689c829a2c3bd84347df727b041
Author: Vitor Avila <vi...@preset.io>
AuthorDate: Fri Dec 13 11:45:18 2024 -0300
fix(database import): Gracefully handle error to get catalog schemas
---
superset/commands/database/create.py | 25 +------
superset/commands/database/importers/v1/utils.py | 38 +----------
superset/commands/database/utils.py | 67 +++++++++++++++++++
.../databases/commands/importers/v1/import_test.py | 28 --------
tests/unit_tests/databases/commands/utils_test.py | 76 ++++++++++++++++++++++
5 files changed, 146 insertions(+), 88 deletions(-)
diff --git a/superset/commands/database/create.py b/superset/commands/database/create.py
index 0eb8d43b37..8349399282 100644
--- a/superset/commands/database/create.py
+++ b/superset/commands/database/create.py
@@ -39,9 +39,9 @@ from superset.commands.database.ssh_tunnel.exceptions import (
SSHTunnelInvalidError,
)
from superset.commands.database.test_connection import TestConnectionDatabaseCommand
+from superset.commands.database.utils import add_permissions
from superset.daos.database import DatabaseDAO
from superset.databases.ssh_tunnel.models import SSHTunnel
-from superset.db_engine_specs.base import GenericDBException
from superset.exceptions import OAuth2RedirectError, SupersetErrorsException
from superset.extensions import event_logger, security_manager
from superset.models.core import Database
@@ -99,28 +99,7 @@ class CreateDatabaseCommand(BaseCommand):
).run()
# add catalog/schema permissions
- if database.db_engine_spec.supports_catalog:
- catalogs = database.get_all_catalog_names(
- cache=False,
- ssh_tunnel=ssh_tunnel,
- )
- for catalog in catalogs:
- security_manager.add_permission_view_menu(
- "catalog_access",
- security_manager.get_catalog_perm(
- database.database_name, catalog
- ),
- )
- else:
- # add a dummy catalog for DBs that don't support them
- catalogs = [None]
-
- for catalog in catalogs:
- try:
- self.add_schema_permissions(database, catalog, ssh_tunnel)
- except GenericDBException: # pylint: disable=broad-except
- logger.warning("Error processing catalog '%s'", catalog)
- continue
+ add_permissions(database, ssh_tunnel)
except (
SSHTunnelInvalidError,
SSHTunnelCreateFailedError,
diff --git a/superset/commands/database/importers/v1/utils.py b/superset/commands/database/importers/v1/utils.py
index c9927a033d..0098bfa26d 100644
--- a/superset/commands/database/importers/v1/utils.py
+++ b/superset/commands/database/importers/v1/utils.py
@@ -19,6 +19,7 @@ import logging
from typing import Any
from superset import app, db, security_manager
+from superset.commands.database.utils import add_permissions
from superset.commands.exceptions import ImportFailedError
from superset.databases.ssh_tunnel.models import SSHTunnel
from superset.databases.utils import make_url_safe
@@ -86,40 +87,3 @@ def import_database(
logger.warning(ex.message)
return database
-
-
-def add_permissions(database: Database, ssh_tunnel: SSHTunnel) -> None:
- """
- Add DAR for catalogs and schemas.
- """
- if database.db_engine_spec.supports_catalog:
- catalogs = database.get_all_catalog_names(
- cache=False,
- ssh_tunnel=ssh_tunnel,
- )
-
- for catalog in catalogs:
- security_manager.add_permission_view_menu(
- "catalog_access",
- security_manager.get_catalog_perm(
- database.database_name,
- catalog,
- ),
- )
- else:
- catalogs = [None]
-
- for catalog in catalogs:
- for schema in database.get_all_schema_names(
- catalog=catalog,
- cache=False,
- ssh_tunnel=ssh_tunnel,
- ):
- security_manager.add_permission_view_menu(
- "schema_access",
- security_manager.get_schema_perm(
- database.database_name,
- catalog,
- schema,
- ),
- )
diff --git a/superset/commands/database/utils.py b/superset/commands/database/utils.py
new file mode 100644
index 0000000000..ea0ce1a27e
--- /dev/null
+++ b/superset/commands/database/utils.py
@@ -0,0 +1,67 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import logging
+
+from superset import security_manager
+from superset.databases.ssh_tunnel.models import SSHTunnel
+from superset.db_engine_specs.base import GenericDBException
+from superset.models.core import Database
+
+logger = logging.getLogger(__name__)
+
+
+def add_permissions(database: Database, ssh_tunnel: SSHTunnel | None) -> None:
+ """
+ Add DAR for catalogs and schemas.
+ """
+ if database.db_engine_spec.supports_catalog:
+ catalogs = database.get_all_catalog_names(
+ cache=False,
+ ssh_tunnel=ssh_tunnel,
+ )
+
+ for catalog in catalogs:
+ security_manager.add_permission_view_menu(
+ "catalog_access",
+ security_manager.get_catalog_perm(
+ database.database_name,
+ catalog,
+ ),
+ )
+ else:
+ catalogs = [None]
+
+ for catalog in catalogs:
+ try:
+ for schema in database.get_all_schema_names(
+ catalog=catalog,
+ cache=False,
+ ssh_tunnel=ssh_tunnel,
+ ):
+ security_manager.add_permission_view_menu(
+ "schema_access",
+ security_manager.get_schema_perm(
+ database.database_name,
+ catalog,
+ schema,
+ ),
+ )
+ except GenericDBException: # pylint: disable=broad-except
+ logger.warning("Error processing catalog '%s'", catalog)
+ continue
diff --git a/tests/unit_tests/databases/commands/importers/v1/import_test.py b/tests/unit_tests/databases/commands/importers/v1/import_test.py
index f101216bbf..06be5bc16f 100644
--- a/tests/unit_tests/databases/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/databases/commands/importers/v1/import_test.py
@@ -23,7 +23,6 @@ from pytest_mock import MockerFixture
from sqlalchemy.orm.session import Session
from superset import db
-from superset.commands.database.importers.v1.utils import add_permissions
from superset.commands.exceptions import ImportFailedError
from superset.utils import json
@@ -216,30 +215,3 @@ def test_import_database_with_user_impersonation(
database = import_database(config)
assert database.impersonate_user is True
-
-
-def test_add_permissions(mocker: MockerFixture) -> None:
- """
- Test adding permissions to a database when it's imported.
- """
- database = mocker.MagicMock()
- database.database_name = "my_db"
- database.db_engine_spec.supports_catalog = True
- database.get_all_catalog_names.return_value = ["catalog1", "catalog2"]
- database.get_all_schema_names.side_effect = [["schema1"], ["schema2"]]
- ssh_tunnel = mocker.MagicMock()
- add_permission_view_menu = mocker.patch(
- "superset.commands.database.importers.v1.utils.security_manager."
- "add_permission_view_menu"
- )
-
- add_permissions(database, ssh_tunnel)
-
- add_permission_view_menu.assert_has_calls(
- [
- mocker.call("catalog_access", "[my_db].[catalog1]"),
- mocker.call("catalog_access", "[my_db].[catalog2]"),
- mocker.call("schema_access", "[my_db].[catalog1].[schema1]"),
- mocker.call("schema_access", "[my_db].[catalog2].[schema2]"),
- ]
- )
diff --git a/tests/unit_tests/databases/commands/utils_test.py b/tests/unit_tests/databases/commands/utils_test.py
new file mode 100644
index 0000000000..e8f27d0416
--- /dev/null
+++ b/tests/unit_tests/databases/commands/utils_test.py
@@ -0,0 +1,76 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from pytest_mock import MockerFixture
+
+from superset.commands.database.utils import add_permissions
+
+
+def test_add_permissions(mocker: MockerFixture) -> None:
+ """
+ Test adding permissions to a database when it's created.
+ """
+ database = mocker.MagicMock()
+ database.database_name = "my_db"
+ database.db_engine_spec.supports_catalog = True
+ database.get_all_catalog_names.return_value = ["catalog1", "catalog2"]
+ database.get_all_schema_names.side_effect = [["schema1"], ["schema2"]]
+ ssh_tunnel = mocker.MagicMock()
+ add_permission_view_menu = mocker.patch(
+ "superset.commands.database.importers.v1.utils.security_manager."
+ "add_permission_view_menu"
+ )
+
+ add_permissions(database, ssh_tunnel)
+
+ add_permission_view_menu.assert_has_calls(
+ [
+ mocker.call("catalog_access", "[my_db].[catalog1]"),
+ mocker.call("catalog_access", "[my_db].[catalog2]"),
+ mocker.call("schema_access", "[my_db].[catalog1].[schema1]"),
+ mocker.call("schema_access", "[my_db].[catalog2].[schema2]"),
+ ]
+ )
+
+
+def test_add_permissions_handle_failures(mocker: MockerFixture) -> None:
+ """
+ Test adding permissions to a database when it's created in case
+ the request to get all schemas for one fo the catalogs fail.
+ """
+ database = mocker.MagicMock()
+ database.database_name = "my_db"
+ database.db_engine_spec.supports_catalog = True
+ database.get_all_catalog_names.return_value = ["catalog1", "catalog2", "catalog3"]
+ database.get_all_schema_names.side_effect = [["schema1"], Exception, ["schema3"]]
+ ssh_tunnel = mocker.MagicMock()
+ add_permission_view_menu = mocker.patch(
+ "superset.commands.database.importers.v1.utils.security_manager."
+ "add_permission_view_menu"
+ )
+
+ add_permissions(database, ssh_tunnel)
+
+ add_permission_view_menu.assert_has_calls(
+ [
+ mocker.call("catalog_access", "[my_db].[catalog1]"),
+ mocker.call("catalog_access", "[my_db].[catalog2]"),
+ mocker.call("catalog_access", "[my_db].[catalog3]"),
+ mocker.call("schema_access", "[my_db].[catalog1].[schema1]"),
+ mocker.call("schema_access", "[my_db].[catalog3].[schema3]"),
+ ]
+ )