(superset) 02/06: fix(database): retry transient schema-listing failures during permission sync

[jl...@apache.org]

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 f2ae11a9072a3efa83b2cca6276393e58a8881e8
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Sat Sep 5 21:04:11 2026 +0000

    fix(database): retry transient schema-listing failures during permission sync
    
    add_permissions() gave up on an entire catalog's schema_access grants after
    a single failed attempt at listing its schemas. The exception caught there
    (GenericDBException, a bare alias for the built-in Exception) doesn't
    distinguish a genuinely unlistable catalog from a one-off transient
    hiccup, so a schema that needed a first-time grant during a failing
    attempt was silently never granted, while the import/create still reported
    success -- with the only workaround being to later edit and re-save the
    connection. Retry the schema-listing call once, immediately, before giving
    up on a catalog, without changing which exceptions are tolerated (a
    persistently unlistable catalog, eg AWS RDS's rdsadmin, still fails both
    attempts and is skipped exactly as before).
---
 superset/commands/database/utils.py               | 20 +++++++---
 tests/unit_tests/databases/commands/utils_test.py | 45 +++++++++++++++++++++--
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/superset/commands/database/utils.py b/superset/commands/database/utils.py
index 0c25f8173c2..359985a7150 100644
--- a/superset/commands/database/utils.py
+++ b/superset/commands/database/utils.py
@@ -82,8 +82,20 @@ def add_permissions(database: Database) -> None:
         catalogs = [None]
 
     for catalog in catalogs:
-        try:
-            for schema in database.get_all_schema_names(catalog=catalog, cache=False):
+        # 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
+            for schema in schemas:
                 security_manager.add_permission_view_menu(
                     "schema_access",
                     security_manager.get_schema_perm(
@@ -92,9 +104,7 @@ def add_permissions(database: Database) -> None:
                         schema,
                     ),
                 )
-        except GenericDBException:  # pylint: disable=broad-except
-            logger.warning("Error processing catalog '%s'", catalog)
-            continue
+            break
 
 
 def add_vm(
diff --git a/tests/unit_tests/databases/commands/utils_test.py b/tests/unit_tests/databases/commands/utils_test.py
index 266c4e5d864..1f81c4d30ce 100644
--- a/tests/unit_tests/databases/commands/utils_test.py
+++ b/tests/unit_tests/databases/commands/utils_test.py
@@ -78,14 +78,20 @@ def test_add_permissions_get_default_catalog(mocker: MockerFixture):
 
 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.
+    Test adding permissions to a database when it's created in case the
+    request to get all schemas for one of the catalogs keeps failing --
+    each failing catalog is retried once before being given up on.
     """
     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"]]
+    database.get_all_schema_names.side_effect = [
+        ["schema1"],
+        Exception,
+        Exception,
+        ["schema3"],
+    ]
     add_permission_view_menu = mocker.patch(
         "superset.commands.database.importers.v1.utils.security_manager."
         "add_permission_view_menu"
@@ -102,3 +108,36 @@ def test_add_permissions_handle_failures(mocker: MockerFixture) -> None:
             mocker.call("schema_access", "[my_db].[catalog3].[schema3]"),
         ]
     )
+
+
+def test_add_permissions_retries_transient_failure(mocker: MockerFixture) -> None:
+    """
+    A catalog whose schema listing fails only once (eg a transient driver
+    hiccup) must still get its schema permissions granted via the retry,
+    instead of being permanently skipped like a genuinely unlistable
+    catalog would be.
+    """
+    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"],
+        Exception,
+        ["schema2"],
+    ]
+    add_permission_view_menu = mocker.patch(
+        "superset.commands.database.importers.v1.utils.security_manager."
+        "add_permission_view_menu"
+    )
+
+    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].[schema1]"),
+            mocker.call("schema_access", "[my_db].[catalog2].[schema2]"),
+        ]
+    )