feat: Update database permissions in async mode #32231

Vitor-Avila · 2025-02-12T05:38:32Z

SUMMARY

Note: To avoid nested transactions, this PR depends on: #32401

Currently, in order to make a new schema or catalog available to be used in Roles, users would need to perform a dummy update to the DB connection (which triggers a permission re-sync). Also, this re-sync is executed in synchronous mode, meaning that the DB connection dialog (and the API request) are kept open until it's fully processed.

This is not ideal for a few different reasons:

The DB connection is actually already updated -- the re-sync happens as a consequence of the update.
The DB credentials might have access to several catalogs (with countless schemas each), which would take a considerable time.
The API request is on hold until the operation finishes, holding un-necessary resources.

This PR moves this logic to dedicated commands, supporting the sync mode approach (default/current behavior) and also an async mode (delegating the operation to a celery task). A dedicated endpoint is also added, allowing users to trigger a re-sync without having to edit the connection.

I'm planning on doing a follow up specifically for the update DB connection flow, but decided to have a first PR mostly focused on adding the feature.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

TESTING INSTRUCTIONS

Tests added.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

superset/commands/database/resync_permissions.py

Vitor-Avila · 2025-02-12T05:42:17Z

superset/commands/database/utils.py


 logger = logging.getLogger(__name__)


+def ping(engine: Engine) -> bool:


Logic moved from superset/commands/database/test_connection.py.

superset/tasks/permissions.py

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Silent Error Handling in Database Ping ▹ view
	N+1 Query Pattern in Permission Updates ▹ view
	Inconsistent Catalog Connection Error Handling ▹ view
	Incomplete Error Handling in Permission Resync ▹ view
	Unsafe User Impersonation in Celery Task ▹ view
	Unsafe User Existence Check ▹ view	✅
	Misspelled Error Message ▹ view
	Default Async Mode Contradicts Intent ▹ view

Files scanned

File Path	Reviewed
superset/commands/database/utils.py	✅
superset/tasks/permissions.py	✅
superset/constants.py	✅
superset/commands/database/resync_permissions_async.py	✅
superset/commands/database/exceptions.py	✅
superset/commands/database/update.py	✅
superset/views/base.py	✅
superset/commands/database/resync_permissions.py	✅
superset-frontend/src/pages/DatabaseList/index.tsx	✅
superset/databases/api.py	✅
superset/config.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ support@korbit.ai

superset-frontend/src/pages/DatabaseList/index.tsx

superset/tasks/permissions.py

superset/config.py

korbit-ai · 2025-02-12T05:46:30Z

superset/commands/database/resync_permissions.py

+            for dataset in DatabaseDAO.get_datasets(
+                self.db_connection_id,
+                catalog=catalog,
+                schema=schema,
+            ):
+                dataset.catalog_perm = new_catalog_perm_name
+                dataset.schema_perm = new_schema_perm_name
+                for chart in DatasetDAO.get_related_objects(dataset.id)["charts"]:
+                    chart.catalog_perm = new_catalog_perm_name
+                    chart.schema_perm = new_schema_perm_name


N+1 Query Pattern in Permission Updates

Tell me more

What is the issue?

Nested loops performing database queries for each dataset and its related charts, without batching or bulk updates.

Why this matters

This can lead to N+1 query performance issues when dealing with large numbers of datasets and charts, causing excessive database round trips.

Suggested change ∙ Feature Preview

Batch the permission updates using bulk database operations. Consider using a bulk update query or collecting all objects first and then updating them in a single transaction:

datasets = DatabaseDAO.get_datasets(self.db_connection_id, catalog=catalog, schema=schema) dataset_ids = [d.id for d in datasets] # Bulk update datasets DatasetDAO.bulk_update_permissions(dataset_ids, catalog_perm=new_catalog_perm_name, schema_perm=new_schema_perm_name) # Bulk update related charts DatasetDAO.bulk_update_chart_permissions(dataset_ids, catalog_perm=new_catalog_perm_name, schema_perm=new_schema_perm_name)

_{💬 Chat with Korbit by mentioning @korbit-ai.}

superset/commands/database/utils.py

superset/commands/database/resync_permissions_async.py

superset/tasks/permissions.py

superset/commands/database/resync_permissions.py

superset/commands/database/exceptions.py

betodealmeida

This is such a huge improvement, @Vitor-Avila! 🌮🌮🌮🌮🌮

superset-frontend/src/pages/DatabaseList/index.tsx

superset/commands/database/resync_permissions.py

superset/commands/database/update.py

superset/databases/api.py

superset/tasks/permissions.py

betodealmeida

Looks good, just a few questions about the async mode.

superset/commands/database/sync_permissions.py

betodealmeida

Looks great!

betodealmeida · 2025-02-24T23:25:53Z

superset/commands/database/update.py

@@ -88,7 +87,14 @@ def run(self) -> Model:
        database.set_sqlalchemy_uri(database.sqlalchemy_uri)
        ssh_tunnel = self._handle_ssh_tunnel(database)
        try:
-            self._refresh_catalogs(database, original_database_name, ssh_tunnel)
+            current_username = get_username()
+            SyncPermissionsCommand(


So much better!

betodealmeida · 2025-02-24T23:27:10Z

tests/conftest.py

@@ -106,3 +107,35 @@ def data_loader(
    return PandasDataLoader(
        example_db_engine, pandas_loader_configuration, table_to_df_convertor
    )
+
+
+def with_config(override_config: dict[str, Any]):


betodealmeida

This is awesome!

betodealmeida · 2025-02-28T21:37:01Z

superset/commands/database/sync_permissions.py

+                old_db_connection_name=old_db_connection_name,
+                db_connection=db_connection,
+                ssh_tunnel=ssh_tunnel,
+            ).sync_database_permissions()


The fact that we're calling a specific method of the command instead of .run() makes me think that this should've been a function instead of a method (since technically we always want to call only .run() when interacting with a command). But this is fine for now, it's just a nit.

pull-request-size bot added the size/XXL label Feb 12, 2025

github-actions bot added the api Related to the REST API label Feb 12, 2025

dosubot bot added change:backend Requires changing the backend data:databases Related to database configurations and connections labels Feb 12, 2025

Vitor-Avila commented Feb 12, 2025

View reviewed changes

superset/commands/database/resync_permissions.py Outdated Show resolved Hide resolved

Vitor-Avila commented Feb 12, 2025

View reviewed changes

superset/tasks/permissions.py Outdated Show resolved Hide resolved

korbit-ai bot reviewed Feb 12, 2025

View reviewed changes

Vitor-Avila commented Feb 12, 2025

View reviewed changes

superset/commands/database/resync_permissions.py Outdated Show resolved Hide resolved

Vitor-Avila requested review from betodealmeida and dpgaspar February 12, 2025 15:14

Antonio-RiveroMartnez reviewed Feb 12, 2025

View reviewed changes

superset/commands/database/exceptions.py Outdated Show resolved Hide resolved

betodealmeida reviewed Feb 12, 2025

View reviewed changes

Vitor-Avila force-pushed the feat/async-db-perm-sync branch from 848482a to 2d60c32 Compare February 18, 2025 00:23

Vitor-Avila requested review from betodealmeida and Antonio-RiveroMartnez February 18, 2025 04:36

betodealmeida reviewed Feb 18, 2025

View reviewed changes

superset/commands/database/sync_permissions.py Outdated Show resolved Hide resolved

superset/commands/database/sync_permissions.py Outdated Show resolved Hide resolved

Vitor-Avila requested a review from betodealmeida February 24, 2025 18:25

betodealmeida approved these changes Feb 24, 2025

View reviewed changes

Vitor-Avila mentioned this pull request Feb 27, 2025

fix: prevent nested transactions #32401

Merged

9 tasks

Vitor-Avila added 10 commits February 28, 2025 12:02

Initial setup for testing

1d668d4

Fixing API config

142a1f7

Improving the approach

70f1c53

Changing order

8f86eec

feat: Update database permissions in async mode

2e035dc

Renaming resync to sync everywhere

ee88cf3

Making the new API endpoint SIP-40 compatible

fd6cb56

Centralizing logic in a single command

77eb482

Extra safety check on user perm check + better error status

2b46d6c

Fixing eslint issue

4c00209

Vitor-Avila added 19 commits February 28, 2025 12:02

Fixing API response + standardizing client error handling

4e96bed

Fixing catalog exception + using nested transaction for update command

fc1670c

Fixing tests

ac7f56a

Fixing more tests

fbc682f

Fixing more tests

18cd66b

Hopefully fixing tests

cbbbd79

Trying again to fix tests

61b207e

More patching

8802029

setting SSH app configs

47f1bf4

Mocking SyncPermsCommand

cfe306a

Removing no longer valid unit tests

5a61b7c

Addressing PR feedback

c3a6820

Fixing unit tests

97933d8

Fixing integration tests

51c187e

Fixing typo in comment

9236062

New approach after syncing with Beto

c524e67

Fixing integration tests

024cc1f

More fixes on integration tests

d42459f

Moving FAB overrides to utils

9fe2c74

Vitor-Avila force-pushed the feat/async-db-perm-sync branch from f394d86 to 9fe2c74 Compare February 28, 2025 15:03

Vitor-Avila added 2 commits February 28, 2025 14:34

Fixing integration tests

9e2bcf9

Adding unit tests for new helpers

91904b5

Vitor-Avila requested a review from betodealmeida February 28, 2025 18:27

betodealmeida approved these changes Feb 28, 2025

View reviewed changes

Vitor-Avila merged commit d79f7b2 into master Mar 1, 2025
48 checks passed

Vitor-Avila deleted the feat/async-db-perm-sync branch March 1, 2025 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update database permissions in async mode #32231

feat: Update database permissions in async mode #32231

Vitor-Avila commented Feb 12, 2025 •

edited

Loading

Vitor-Avila Feb 12, 2025

korbit-ai bot left a comment •

edited

Loading

korbit-ai bot Feb 12, 2025

betodealmeida left a comment

betodealmeida left a comment

betodealmeida left a comment

betodealmeida Feb 24, 2025

betodealmeida Feb 24, 2025

betodealmeida left a comment

betodealmeida Feb 28, 2025


		logger = logging.getLogger(__name__)


		def ping(engine: Engine) -> bool:

feat: Update database permissions in async mode #32231

feat: Update database permissions in async mode #32231

Conversation

Vitor-Avila commented Feb 12, 2025 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Vitor-Avila Feb 12, 2025

Choose a reason for hiding this comment

korbit-ai bot left a comment • edited Loading

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Interacting with Korbit

Customizing Korbit

Feedback and Support

korbit-ai bot Feb 12, 2025

Choose a reason for hiding this comment

N+1 Query Pattern in Permission Updates

What is the issue?

Why this matters

Suggested change ∙ Feature Preview

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida Feb 24, 2025

Choose a reason for hiding this comment

betodealmeida Feb 24, 2025

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida Feb 28, 2025

Choose a reason for hiding this comment

Vitor-Avila commented Feb 12, 2025 •

edited

Loading

korbit-ai bot left a comment •

edited

Loading