Skip to content

Conversation

@dankor
Copy link
Contributor

@dankor dankor commented Jul 12, 2024

SUMMARY

Extend API to handle uuid.

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Fixes The default URL generated by Dashboard has security risks! #29879
  • 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

@github-actions github-actions bot added the api Related to the REST API label Jul 12, 2024
@dosubot dosubot bot added api:charts Related to the REST endpoints of charts api:dashboard Related to the REST endpoints of the Dashboard labels Jul 12, 2024
@dankor dankor mentioned this pull request Jul 12, 2024
3 tasks
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@codecov
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

❌ Patch coverage is 95.23810% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.79%. Comparing base (2179081) to head (2e73419).
⚠️ Report is 850 commits behind head on master.

Files with missing lines Patch % Lines
superset/models/slice.py 57.14% 2 Missing and 1 partial ⚠️
superset/utils/cache.py 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #29573       +/-   ##
===========================================
+ Coverage        0   72.79%   +72.79%     
===========================================
  Files           0      574      +574     
  Lines           0    41769    +41769     
  Branches        0     4401     +4401     
===========================================
+ Hits            0    30407    +30407     
- Misses          0    10192    +10192     
- Partials        0     1170     +1170     
Flag Coverage Δ
hive 47.07% <73.80%> (?)
mysql 71.80% <95.23%> (?)
postgres 71.86% <95.23%> (?)
presto 50.77% <75.00%> (?)
python 72.76% <95.23%> (?)
sqlite 71.42% <95.23%> (?)
unit 100.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@betodealmeida
Copy link
Member

Hi, @dankor! Welcome to the Superset GitHub!

Can you add some context on why these fields are needed? Thanks!

@michael-s-molina michael-s-molina added the review:checkpoint Last PR reviewed during the daily review standup label Jul 12, 2024
@dankor
Copy link
Contributor Author

dankor commented Jul 13, 2024

Hi, @dankor! Welcome to the Superset GitHub!

Can you add some context on why these fields are needed? Thanks!

Hi @betodealmeida! Sure! I'm working on establishing a robust and granular deployment process for Superset. The core issue here is similar to why we expose these UUIDs in the import/export API: we need persistent identifiers across multiple Superset instances.

You might suggest extending the import/export API instead, but we’re encountering several challenges:

  • No support for tags, roles, etc.
  • Deprecated resources aren’t cleaned up.
  • Archives are much harder to debug compared to classic CRUD operations.

What I’m really aiming for is a declarative way to manage Superset resources (let’s call it a "report-as-code" approach). Imagine having a development environment where everyone can experiment freely. Once a report is ready, it can be committed to git, reviewed, and then deployed to higher environments.

Ideally, I envision mature version of this, but supporting all types of resources. You can check out a proof-of-concept in this video, though no code is available.

My plan is to implement this feature incrementally, starting with minor changes and aligning as closely as possible with upstream.

Hope this makes sense! Let me know if you're open to these kinds of contributions and what concerns the community might have.

@michael-s-molina michael-s-molina removed the review:checkpoint Last PR reviewed during the daily review standup label Jul 15, 2024
@dankor
Copy link
Contributor Author

dankor commented Jul 29, 2024

Hey all! I'm just wondering if there are any upcoming plans or next steps here. Could you please let me what to expect?

@rusackas
Copy link
Member

It looks like CI is stuck because of some tests failing (which I'm re-running), and pre-commit checks (i.e. linting) so you will need to run the Pre-commit hooks

Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:

pip3 install -r requirements/integration.txt
pre-commit install
A series of checks will now run when you make a git commit.

You can then run pre-commit manually:

pre-commit run --all-files

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list_columns is used by the frontend to load the datasets/charts/dashboards pages. Projecting all column/metric information will significantly impact performance for environments that have thousands of columns/metrics. If you want to query for that information you can request them using the q parameter.

@dankor
Copy link
Contributor Author

dankor commented Jul 29, 2024

Hi @rusackas ! Thanks! I fixed the linting.

@michael-s-molina Fair point. Actually, I have added this fields in order to q them:

{
  "columns": [
    "always_filter_main_dttm",
    "cache_timeout",
    "catalog",
    "columns.advanced_data_type",
    "columns.column_name",
    "columns.description",
    "columns.expression",
    "columns.extra",
    "columns.filterable",
    "columns.groupby",
    "columns.is_active",
    "columns.is_dttm",
    "columns.python_date_format",
    "columns.type",
    "columns.verbose_name",
    "default_endpoint",
    "description",
    "extra",
    "fetch_values_predicate",
    "filter_select_enabled",
    "is_managed_externally",
    "is_sqllab_view",
    "main_dttm_col",
    "metrics.d3format",
    "metrics.description",
    "metrics.expression",
    "metrics.extra",
    "metrics.metric_name",
    "metrics.metric_type",
    "metrics.verbose_name",
    "metrics.warning_text",
    "normalize_columns",
    "offset",
    "schema",
    "sql",
    "table_name",
    "template_params",
    "uuid",
    "database.uuid"
  ],
  "filters": [
    {
      "col": "uuid",
      "opr": "eq",
      "value": "7d2a123a-1952-46e1-b9d0-4d41e1e042c1"
    }
  ]
}

Otherwise these fields won't be listed with q.

Do you suppose to keep list_columns unchanged and perform two requests instead?

  • Run list_columns with uuid filter and get id.
  • Run show_select_columns against obtained id with all required fields.

I'd tend to have one request if possible, but I assume the front-end changes may be too big, right?

@michael-s-molina
Copy link
Member

michael-s-molina commented Jul 29, 2024

I'd tend to have one request if possible, but I assume the front-end changes may be too big, right?

You would need to change the requests for all pages but I'm also worried that the default implementation of the endpoint will perform these joins (columns and metrics belong to separate tables).

@dpgaspar Is there a way we can achieve this without multiple requests?

@dankor
Copy link
Contributor Author

dankor commented Aug 8, 2024

I'm wondering if we can come up with some action plan here. It currently seems quite frontend-oriented. I'd like to find a common ground in order to both fulfill my use-case and contribute it upstream, so community can benefit too. What's your vision here?

@rusackas
Copy link
Member

I think the main thing is that we can't impact performance. @dpgaspar is currently out of office, but may be able to provide suggestions upon his return of how to handle this at the API layer.

I'm also wondering if this might be a precursor to a (fairly common) request to change URLs from being incremental integer IDs to UUIDs (e.g. here)

It might also tie into this issue.

Maybe we can just solve the performance concern and let this through, but at some point we need to have a SIP proposed about how/where UUIDs should be used more systemically by Superset. I think that's the real path forward - broadening the discussion to formulate a plan to use UUIDs everywhere relevant.

@dpgaspar
Copy link
Member

I'd tend to have one request if possible, but I assume the front-end changes may be too big, right?

You would need to change the requests for all pages but I'm also worried that the default implementation of the endpoint will perform these joins (columns and metrics belong to separate tables).

@dpgaspar Is there a way we can achieve this without multiple requests?

If we include columns and metrics on a dataset payload, these results won't be paginated. So I advise making multiple requests

@dankor
Copy link
Contributor Author

dankor commented Aug 26, 2024

Hey @betodealmeida @rusackas @michael-s-molina @dpgaspar !

Could you please take another look? I've reworked this PR to eliminate the need for listing calls when looking up a single item by UUID. I was inspired by the dashboard GET approach, where the dashboard API supports three different PKs: ID, slug, and UUID. I've only implemented this for the chart so far, to get your feedback on the new approach. I hope it makes more sense now. Looking forward to hearing from you.

@github-actions github-actions bot added i18n:japanese Translation related to Japanese language doc Namespace | Anything related to documentation embedded plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages labels Aug 18, 2025
@github-actions github-actions bot removed i18n Namespace | Anything related to localization risk:db-migration PRs that require a DB migration i18n:french Translation related to French language i18n:japanese Translation related to Japanese language doc Namespace | Anything related to documentation embedded plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages labels Aug 18, 2025
@dankor dankor changed the title feat(api): extending uuid field in chart API feat(api): Added uuid filed support to dataset, chart, dashboard API Aug 18, 2025
@mistercrunch
Copy link
Member

Amazing, thanks for the contribution!

@mistercrunch mistercrunch merged commit 31e2143 into apache:master Aug 18, 2025
52 checks passed
@sadpandajoe sadpandajoe added the risk:breaking-change Issues or PRs that will introduce breaking changes label Aug 19, 2025
sadpandajoe added a commit that referenced this pull request Aug 19, 2025
Fix TypeError in Slice.get() method introduced in PR #29573. The method was
incorrectly using filter_by() with a BinaryExpression from id_or_uuid_filter(),
which expects keyword arguments. Changed to use filter() which accepts
BinaryExpressions.

This fixes:
- Chart thumbnail generation failures
- 9 failing tests (8 event logger, 1 multi-tenant)
- All calls to Slice.get() with ID or UUID strings

Added comprehensive test coverage:
- Unit tests for Slice.get() method with mock verification
- Integration tests for both ID and UUID lookup scenarios
- Tests to prevent regression of filter_by usage

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@github-actions github-actions bot added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 6.0.0 First shipped in 6.0.0 labels Dec 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api:charts Related to the REST endpoints of charts api:dashboard Related to the REST endpoints of the Dashboard api Related to the REST API 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels risk:breaking-change Issues or PRs that will introduce breaking changes size/L 🚢 6.0.0 First shipped in 6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The default URL generated by Dashboard has security risks!

8 participants