jfrag1 opened a new pull request, #24884: URL: https://github.com/apache/superset/pull/24884
<!--- Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/ Example: fix(dashboard): load charts correctly --> ### SUMMARY <!--- Describe the change below, including rationale and design decisions --> Currently, a bug exists in Superset where sometimes when updating a dataset's schema, the dataset and any charts belonging to the dataset's `schema_perm` attribute does not get updated properly. I've identified that this bug is the result of [this line](https://github.com/apache/superset/blob/554ef07eeda094f5bcc081e7a265e0b7408e0868/superset/security/manager.py#L1368) which checks if a dataset's schema was changed after a dataset update. Usually when the schema changes, both conditions are true/truthy, however sometimes `history_schema.deleted` is empty/falsy, which results in the `schema_perm` not getting updated. 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](https://github.com/apache/superset/pull/24815) 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 condition `history_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 needed ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> ### TESTING INSTRUCTIONS <!--- Required! What steps can be taken to manually verify the changes? --> ### ADDITIONAL INFORMATION <!--- Check any relevant boxes with "x" --> <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue --> - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [x] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [x] Migration is atomic, supports rollback & is backwards-compatible - [x] Confirm DB migration upgrade and downgrade tested - [x] Runtime estimates and downtime expectations provided - no downtime - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
