-
Notifications
You must be signed in to change notification settings - Fork 14k
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: Migration to fix out of sync schema_perm in charts and datasets #24884
fix: Migration to fix out of sync schema_perm in charts and datasets #24884
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24884 +/- ##
==========================================
- Coverage 69.00% 69.00% -0.01%
==========================================
Files 1906 1906
Lines 74141 74142 +1
Branches 8209 8208 -1
==========================================
- Hits 51161 51160 -1
Misses 20859 20859
- Partials 2121 2123 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM
…pache#24884) (cherry picked from commit 07992c1)
🏷️ preset:2023.31 |
SUMMARY
Currently, a bug exists in Superset where sometimes when updating a dataset's schema, the dataset and any charts belonging to the dataset do not have their
schema_perm
attribute get updated properly. I've identified that this bug is the result of this line which checks if a dataset's schema was changed after a dataset update. Usually when the schema changes, both conditions are true/truthy, however sometimeshistory_schema.deleted
is empty/falsy, which results in theschema_perm
not getting updated. I was not able to find a cause for this inconsistency.This PR does not actually fix this bug; it just fixes any currently affected charts and datasets. The reasoning behind this is that there was another bug recently fixed which caused some datasets to be created without a schema, resulting in users needing the update dataset schemas more frequently than usual. This made the
schema_perm
bug here more impactful.Additionally, actually fixing this bug is not simple. I initially thought we could remove the
history_schema.deleted
condition, but it turns out that the other conditionhistory_schema.has_changes()
is pretty much always true even when the schema has not changed, which would result in many unnecessary queries. I believe the long-term fix for this is either to move away from sqlalchemy event listeners/attribute history and instead handle this logic in the DAO layer and/or to rehaul the data model for data access permissions so that these permission strings are no longer neededBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION