-
Notifications
You must be signed in to change notification settings - Fork 14.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(import): ensure charts and datasets overwrite on report import #31888
base: master
Are you sure you want to change the base?
fix(import): ensure charts and datasets overwrite on report import #31888
Conversation
@supersetbot orglabel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Fix Detected |
---|---|---|
Query Context Information Loss ▹ view |
Files scanned
File Path | Reviewed |
---|---|
superset/commands/dashboard/importers/v1/init.py | ✅ |
superset/models/slice.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.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
if "query_context" in config: | ||
config["query_context"] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query Context Information Loss
Tell me more
What is the issue?
The code unconditionally sets query_context to None, which contradicts the developer's intent to ensure datasource information is injected in get_query_context.
Why this matters
Setting query_context to None will prevent any datasource information from being preserved during chart imports, potentially causing data inconsistencies during report execution.
Suggested change ∙ Feature Preview
Instead of setting query_context to None, update it with the correct datasource information:
if "query_context" in config:
config["query_context"]["datasource"] = dataset_uid
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #31888 +/- ##
==========================================
+ Coverage 67.46% 70.26% +2.80%
==========================================
Files 1911 1940 +29
Lines 74966 77871 +2905
Branches 8353 8353
==========================================
+ Hits 50573 54715 +4142
+ Misses 22343 21106 -1237
Partials 2050 2050
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I defer to @betodealmeida and @dpgaspar here, but I can imagine somoene importing a report, and accidentally overwriting a bunch of charts and datasets and impacting the dashboards of untold numbers of users. Seems dangerous to me. It seems like it'd be ideal if instead of overwriting an asset, we could create a version of the asset. That's been discussed a bit, but is dependent on a yet-to-come SIP. |
SUMMARY
Previously, when importing a report in Superset, charts and datasets were not being overwritten, unlike when they were imported separately.
This update fixes the issue by ensuring the overwrite parameter is correctly passed to the import_database, import_dataset, and import_chart functions. Now, when importing a report, charts and datasets will be properly updated, eliminating the need for manual re-importing.
Additionally, query_context handling in slice.py has been improved to correctly pass the datasource to QueryContextFactory.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION