You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/09/07 04:51:23 UTC
(superset) 03/06: fix(database): preserve exception tolerance scope around schema-listing retry
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-dar-import-schema-permissions
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 77937345c1526ce2ec8920d80dab7fa5f6d55b2b
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Sat Sep 5 21:42:58 2026 +0000
fix(database): preserve exception tolerance scope around schema-listing retry
The retry added around add_permissions()'s per-catalog schema-listing call
narrowed the pre-existing try/except's scope: previously, an exception
raised while creating a schema's permission view (not just while listing
schemas) was tolerated the same way, aborting the rest of that catalog
without failing the whole import. Isolate the retry into a helper so the
surrounding try/except keeps covering both the listing call and the
permission-view-creation loop, exactly as it did before, and add a test
that pins this scope so a future change to the retry can't narrow it again
silently.
---
superset/commands/database/utils.py | 38 ++++++++++++-------
tests/unit_tests/databases/commands/utils_test.py | 45 +++++++++++++++++++++++
2 files changed, 69 insertions(+), 14 deletions(-)
diff --git a/superset/commands/database/utils.py b/superset/commands/database/utils.py
index 359985a7150..5db1f769fa0 100644
--- a/superset/commands/database/utils.py
+++ b/superset/commands/database/utils.py
@@ -51,6 +51,25 @@ def ping(engine: Engine) -> bool:
return engine.dialect.do_ping(conn)
+def _get_all_schema_names_with_retry(
+ database: Database, catalog: str | None
+) -> set[str]:
+ """
+ Retry the live schema-listing call once before giving up on a catalog.
+
+ Some catalogs are visible but not listable (eg the ``rdsadmin`` catalog on
+ AWS RDS), but the exception caught by the caller doesn't distinguish that
+ from a one-off transient hiccup (eg schema metadata not yet visible right
+ after it was created). A single retry lets a schema that needs a
+ first-time grant survive a fluke without tolerating a persistently
+ unlistable catalog for any longer than before.
+ """
+ try:
+ return database.get_all_schema_names(catalog=catalog, cache=False)
+ except GenericDBException: # pylint: disable=broad-except
+ return database.get_all_schema_names(catalog=catalog, cache=False)
+
+
def add_permissions(database: Database) -> None:
"""
Add DAR for catalogs and schemas.
@@ -82,19 +101,8 @@ def add_permissions(database: Database) -> None:
catalogs = [None]
for catalog in catalogs:
- # A single failure while listing a catalog's schemas is tolerated (some
- # catalogs are visible but not listable, eg the ``rdsadmin`` catalog on
- # AWS RDS), but the exception caught here doesn't distinguish that from
- # a one-off transient hiccup (eg schema metadata not yet visible right
- # after it was created). Retry once, immediately, so a schema that
- # needs a first-time grant isn't permanently skipped over a fluke.
- for attempt in range(2):
- try:
- schemas = database.get_all_schema_names(catalog=catalog, cache=False)
- except GenericDBException: # pylint: disable=broad-except
- if attempt:
- logger.warning("Error processing catalog '%s'", catalog)
- continue
+ try:
+ schemas = _get_all_schema_names_with_retry(database, catalog)
for schema in schemas:
security_manager.add_permission_view_menu(
"schema_access",
@@ -104,7 +112,9 @@ def add_permissions(database: Database) -> None:
schema,
),
)
- break
+ except GenericDBException: # pylint: disable=broad-except
+ logger.warning("Error processing catalog '%s'", catalog)
+ continue
def add_vm(
diff --git a/tests/unit_tests/databases/commands/utils_test.py b/tests/unit_tests/databases/commands/utils_test.py
index 1f81c4d30ce..353b6dcd38e 100644
--- a/tests/unit_tests/databases/commands/utils_test.py
+++ b/tests/unit_tests/databases/commands/utils_test.py
@@ -141,3 +141,48 @@ def test_add_permissions_retries_transient_failure(mocker: MockerFixture) -> Non
mocker.call("schema_access", "[my_db].[catalog2].[schema2]"),
]
)
+
+
+def test_add_permissions_tolerates_failure_creating_permission_view(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A failure while granting a schema's permission (as opposed to while
+ listing the catalog's schemas) must be tolerated the same way it was
+ before the schema-listing retry was introduced: the rest of that
+ catalog is abandoned, but the next catalog is still processed. The
+ retry is scoped to the schema-listing call only, so it must not narrow
+ this pre-existing tolerance.
+ """
+ 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 = [
+ ["schema1a", "schema1b"],
+ ["schema2"],
+ ]
+ add_permission_view_menu = mocker.patch(
+ "superset.commands.database.importers.v1.utils.security_manager."
+ "add_permission_view_menu"
+ )
+ add_permission_view_menu.side_effect = [
+ None, # catalog_access [catalog1]
+ None, # catalog_access [catalog2]
+ Exception, # schema_access [catalog1].[schema1a] -- fails
+ None, # schema_access [catalog2].[schema2]
+ ]
+
+ add_permissions(database)
+
+ 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].[schema1a]"),
+ mocker.call("schema_access", "[my_db].[catalog2].[schema2]"),
+ ]
+ )
+ # [catalog1].[schema1b] must never be attempted: the exception raised
+ # while granting [schema1a] aborts the rest of catalog1, same as before.
+ assert add_permission_view_menu.call_count == 4