fix: handle empty catalog when DB supports them #29840

betodealmeida · 2024-08-02T22:04:50Z

SUMMARY

Handle edge cases when catalog is passed as null but the database supports catalog. This happens when "Allow changing catalogs" is disabled in the database. In these cases we need to use the default catalog, so that permissions checks work as expected.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Add a database with catalogs (eg, Postgres)

Catalog permissions are created (eg, [Postgres].[superset])
Schema permissions are created (eg, [Postgres].[superset].[public])

Create a user A with catalog-level permissions to the default catalog

User can see schemas in SQL Lab
User can choose schema in SQL Lab
User can choose table in SQL Lab
User can run queries
User can create a new dataset
User can see charts built from the dataset

Create a user B with schema-level permissions to the default catalog

User can see only 1 schema in SQL Lab
User can choose table in SQL Lab
User can run queries
User can create a new dataset
User can see charts built from the dataset

Create a user C with catalog-level permissions to a non-default catalog

User can't see schemas/tables in SQL Lab
User can't run queries in SQL Lab

Create a user D with schema-level permissions to a non-default catalog

User can't see schemas/tables in SQL Lab
User can't run queries in SQL Lab

Enable multi-catalog

Admin should see all catalogs
A, B, C, D should only see the catalogs they have access to
Repeat steps from (2)

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

rusackas · 2024-08-05T16:47:13Z

I just put this in draft mode whilst it doesn't have a title/description :) Best of luck with whatever it is! :D

betodealmeida · 2024-08-06T22:51:15Z

scripts/change_detector.py

@@ -52,7 +52,7 @@
 def fetch_files_github_api(url: str):  # type: ignore
    """Fetches data using GitHub API."""
    req = Request(url)
-    req.add_header("Authorization", f"token {GITHUB_TOKEN}")
+    req.add_header("Authorization", f"Bearer {GITHUB_TOKEN}")


Note: In most cases, you can use Authorization: Bearer or Authorization: token to pass a token. However, if you are passing a JSON web token (JWT), you must use Authorization: Bearer. [source]

sadpandajoe · 2024-08-07T19:00:28Z

@supersetbot label 4.1

Vitor-Avila · 2024-08-09T18:48:13Z

superset/databases/api.py

@@ -1159,7 +1159,7 @@ def select_star(
        self.incr_stats("init", self.select_star.__name__)
        try:
            result = database.select_star(
-                Table(table_name, schema_name),
+                Table(table_name, schema_name, database.get_default_catalog()),


Should catalog be added to the API path as well (when the table is not from the default one)? Not related with this PR, just asking if it's something we need to do.

Yeah, but I I couldn't find any API calls to this endpoint — it seems like we return this in the database request now, instead of calling /select_star/, which is why I didn't change the code to pass a parameter. I think we should keep this endpoint until the next major version and then get rid of it.

gotcha! thank you

Vitor-Avila

LGTM!

Vitor-Avila · 2024-08-09T18:53:51Z

tests/integration_tests/databases/api_tests.py

@@ -1563,34 +1563,6 @@ def test_get_select_star_not_allowed(self):
        rv = self.client.get(uri)
        self.assertEqual(rv.status_code, 404)

-    def test_get_select_star_datasource_access(self):


Non-blocking comment: would be good if you could mention why these tests were removed.

These tests were poorly written and relied on weird side effects.

(cherry picked from commit 39209c2)

ERGO1995 · 2025-01-23T17:24:37Z

superset/connectors/sqla/models.py

@@ -1963,7 +1965,7 @@ class and any keys added via `ExtraCache`.
        if self.has_extra_cache_key_calls(query_obj):
            sqla_query = self.get_sqla_query(**query_obj)
            extra_cache_keys += sqla_query.extra_cache_keys
-        return extra_cache_keys
+        return list(set(extra_cache_keys))


Why we replace this line? Actually not doing great job in our case related to #31975

This is just a way of deduping the keys, what problems are you having with it?

in our case it break the chart key retreiving !
we've got some logs saying cache retreiving failed !
Query works on sql-lab but crash in chart or dashboard ( works if force refresh as it does not go to cache )
We are using a lot of url_params reading at the dataset execution.

But how does it break? Is there a stacktrace?i

All this is doing is deduping a list. If it's causing errors, the reason must be upstream of this code.

Main error was a 422
with "Error loading data from cache"uwsgi_superset.log:2025-01-23
And a warning like 15:10:33,731:WARNING:superset.common.utils.query_cache_manager:force_cached (QueryContext): value not found for key 8bda314040760dc2b33b861fb943fc52
Flipping this line make verything works.
Maybe the key for cache creation is not deduplicate and we may have some duplicate on the key witch fail retriving

WIP

1618903

pull-request-size bot added the size/M label Aug 2, 2024

Fixing tests

944df5c

pull-request-size bot added size/L and removed size/M labels Aug 4, 2024

rusackas marked this pull request as draft August 5, 2024 16:46

betodealmeida changed the title ~~WIP~~ fix: handle empty catalog when DB supports them Aug 6, 2024

github-actions bot added the preset-io label Aug 6, 2024

betodealmeida added 2 commits August 6, 2024 11:42

Fix more tests

23dc294

Fix more tests

fd9290a

michael-s-molina added the review:draft label Aug 6, 2024

Remove broken tests

03059e8

github-actions bot added the api Related to the REST API label Aug 6, 2024

Fix SQL Lab

5e7d979

betodealmeida force-pushed the use-default-catalog branch from 013d840 to 5e7d979 Compare August 6, 2024 20:26

Fix cache keys

9023b48

betodealmeida force-pushed the use-default-catalog branch from c03577f to 9023b48 Compare August 6, 2024 21:22

betodealmeida marked this pull request as ready for review August 6, 2024 21:49

dosubot bot added authentication:access-control Rlated to access control data Namespace | Anything related to data, including databases configurations, datasets, etc. labels Aug 6, 2024

betodealmeida commented Aug 6, 2024

View reviewed changes

Undo test change

c5a9797

github-actions bot added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Aug 7, 2024

betodealmeida mentioned this pull request Aug 9, 2024

fix: set default catalog #29608

Closed

9 tasks

Vitor-Avila reviewed Aug 9, 2024

View reviewed changes

Vitor-Avila approved these changes Aug 9, 2024

View reviewed changes

Vitor-Avila reviewed Aug 9, 2024

View reviewed changes

betodealmeida merged commit 39209c2 into master Aug 13, 2024
35 checks passed

sadpandajoe pushed a commit that referenced this pull request Aug 13, 2024

fix: handle empty catalog when DB supports them (#29840)

9677fa9

(cherry picked from commit 39209c2)

rusackas deleted the use-default-catalog branch September 27, 2024 20:55

github-actions bot added 🍒 4.1.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Nov 14, 2024

mistercrunch added the 🍒 4.1.1 label Nov 27, 2024

ERGO1995 mentioned this pull request Jan 23, 2025

4.1.1 migration troubleshooting with url_param() current_username() keywords in sql dataset #31975

Open

3 tasks

ERGO1995 reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle empty catalog when DB supports them #29840

fix: handle empty catalog when DB supports them #29840

betodealmeida commented Aug 2, 2024 •

edited

Loading

rusackas commented Aug 5, 2024

betodealmeida Aug 6, 2024

Vitor-Avila Aug 9, 2024

sadpandajoe commented Aug 7, 2024

Vitor-Avila Aug 9, 2024

betodealmeida Aug 13, 2024

Vitor-Avila Aug 13, 2024

Vitor-Avila left a comment

Vitor-Avila Aug 9, 2024

betodealmeida Aug 13, 2024

Vitor-Avila Aug 13, 2024

ERGO1995 Jan 23, 2025

betodealmeida Jan 23, 2025

Airliquide76 Jan 23, 2025

betodealmeida Jan 23, 2025

Airliquide76 Jan 23, 2025

fix: handle empty catalog when DB supports them #29840

fix: handle empty catalog when DB supports them #29840

Conversation

betodealmeida commented Aug 2, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

rusackas commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadpandajoe commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vitor-Avila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida commented Aug 2, 2024 •

edited

Loading