aminghadersohi opened a new pull request, #45864: URL: https://github.com/apache/superset/pull/45864
### SUMMARY Renaming a database (`PUT /api/v1/database/<id>`) can fail with `'NoneType' object has no attribute 'id'` when one of its datasets has no `datasource_access` view menu under the old database name. Root cause: on rename, `database_after_update` calls `_update_vm_datasources_access`, which renames each dataset view menu with `UPDATE ab_view_menu ... WHERE name = <old dataset perm>` and then reads the row back by the new name with `_find_view_menu_on_sqla_event`. If the old dataset view menu is missing, the UPDATE matches no row, the read returns `None`, and `view_menu_.id` raises. When the old database permission is missing too, the logs show "Could not find previous database permission ..." first; that warning comes from the database path, which already recreates the permission and does not crash. The crash comes from the dataset path. Changes in `superset/security/manager.py`: - `_update_vm_datasources_access`: if the renamed dataset view menu cannot be read back, log a warning and create the `datasource_access` permission under the new name (this matches what `_update_dataset_perm` already does when the old dataset permission is missing). The dataset and chart `perm` columns are still rewritten. - `_find_permission_on_sqla_event` / `_find_view_menu_on_sqla_event` return `None` when no row matches instead of raising `AttributeError`. - The other callers handle `None`: `_update_vm_database_access` skips the update hook, and `_insert_pvm_on_sqla_event` logs and returns if it cannot read back a row it just inserted (permission, view menu or permission view). Other code that renames a view menu and then reads it back, and why it is safe: - `_update_vm_database_access`: renames by the id of a permission view it has already found, and skips the read-back when there is none. - `_update_dataset_perm`: checks that the old dataset view menu exists before renaming, and inserts a new permission otherwise. - `semantic_layer_before_update` and its view cascade, `semantic_view_before_update`: check that the old view menu exists before renaming (insert, or skip, otherwise). - `_insert_pvm_on_sqla_event`: re-reads rows it has just inserted on the same connection. Those reads are guarded now. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A ### TESTING INSTRUCTIONS ``` pytest tests/unit_tests/security/database_rename_permissions_test.py ``` `test_rename_database_with_missing_permission` creates a database with one dataset, deletes the old database permission, the dataset permission or both, then renames the database. Without the fix, the `dataset` and `database-and-dataset` cases fail with `AttributeError: 'NoneType' object has no attribute 'id'`. With the fix all three pass, and the database and dataset permissions exist under the new name. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] 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 -- 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]
