john-bodley opened a new pull request, #19914: URL: https://github.com/apache/superset/pull/19914
<!--- Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/ Example: fix(dashboard): load charts correctly --> ### SUMMARY This PR fixes a number of issues (some cosmetic) with the SQL username. Specifically: 1. The username was referred to as both `username` and `user_name`. 2. It a number of places the `g.user.username` was passed as the `username` argument of the `get_sqla_engine` function even when it was the default fallback. This added unnecessary complexity, i.e., from reading the code it wasn't apparent when/why the username would be overridden. 3. The logic introduced in https://github.com/apache/superset/pull/17499 wasn't suffice, in actuality it likely added more complexity, as although it tried to ensure that the `get_df` for the alert was executed by the passed in user, if the underlying query used Jinja templating for fetching the the latest partition or similar those queries were executed by the user defined in the SQLAlchemy URI—leveraging the SQL inspector—per [here](https://github.com/apache/superset/blob/aff10a7fad0b6a48c578e70d2746d04bdf4d753c/superset/models/core.py#L511-L514). The fix for (1) was to rename `user_name` to `username` where possible. For (2) and (3) the combined fix was to actually remove overriding the username in functions given the level of nesting, etc. and rely simply on using `g.user.username`, i.e., the logged in user, as the user. For instances where there was no user logged in: alerting, async queries, etc. we simply temporarily override the `g.user` using provided username. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> ### TESTING INSTRUCTIONS CI. Updated existing and added new unit tests. ### 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 - [ ] 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]
