You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/05/01 22:58:42 UTC
[GitHub] [superset] john-bodley opened a new pull request, #19914: fix: Refactor SQL username logic
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #19914: fix: Refactor SQL engine username logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #19914:
URL: https://github.com/apache/superset/pull/19914#discussion_r863261153
##########
tests/integration_tests/base_tests.py:
##########
@@ -369,9 +369,9 @@ def run_sql(
ctas_method=CtasMethod.TABLE,
template_params="{}",
):
- if user_name:
+ if username:
self.logout()
- self.login(username=(user_name or "admin"))
+ self.login(username=username)
Review Comment:
Given that on like #372 `username` is truthy there's no need for the `username or "admin"` logic.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #19914: fix: Refactor SQL engine username logic
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #19914:
URL: https://github.com/apache/superset/pull/19914#discussion_r862531806
##########
superset/databases/commands/test_connection.py:
##########
@@ -74,42 +75,43 @@ def run(self) -> None:
database.set_sqlalchemy_uri(uri)
database.db_engine_spec.mutate_db_for_connection_test(database)
- username = self._actor.username if self._actor is not None else None
- engine = database.get_sqla_engine(user_name=username)
- event_logger.log_with_context(
- action="test_connection_attempt",
- engine=database.db_engine_spec.__name__,
- )
- with closing(engine.raw_connection()) as conn:
- try:
- alive = func_timeout(
- int(
- app.config[
- "TEST_DATABASE_CONNECTION_TIMEOUT"
- ].total_seconds()
- ),
- engine.dialect.do_ping,
- args=(conn,),
- )
- except (sqlite3.ProgrammingError, RuntimeError):
- # SQLite can't run on a separate thread, so ``func_timeout`` fails
- # RuntimeError catches the equivalent error from duckdb.
- alive = engine.dialect.do_ping(conn)
- except FunctionTimedOut as ex:
- raise SupersetTimeoutException(
- error_type=SupersetErrorType.CONNECTION_DATABASE_TIMEOUT,
- message=(
- "Please check your connection details and database settings, "
- "and ensure that your database is accepting connections, "
- "then try connecting again."
- ),
- level=ErrorLevel.ERROR,
- extra={"sqlalchemy_uri": database.sqlalchemy_uri},
- ) from ex
- except Exception: # pylint: disable=broad-except
- alive = False
- if not alive:
- raise DBAPIError(None, None, None)
+
+ with override_user(self._actor):
Review Comment:
Same logic as previously, though indented due to the context manager.
##########
superset/db_engine_specs/base.py:
##########
@@ -393,10 +393,7 @@ def get_engine(
schema: Optional[str] = None,
source: Optional[str] = None,
) -> Engine:
- user_name = utils.get_username()
- return database.get_sqla_engine(
- schema=schema, nullpool=True, user_name=user_name, source=source
- )
+ return database.get_sqla_engine(schema=schema, source=source)
Review Comment:
`nullpool=True` is the default.
##########
superset/databases/commands/validate.py:
##########
@@ -115,22 +116,23 @@ def run(self) -> None:
)
database.set_sqlalchemy_uri(sqlalchemy_uri)
database.db_engine_spec.mutate_db_for_connection_test(database)
- username = self._actor.username if self._actor is not None else None
- engine = database.get_sqla_engine(user_name=username)
- try:
- with closing(engine.raw_connection()) as conn:
- alive = engine.dialect.do_ping(conn)
- except Exception as ex:
- url = make_url_safe(sqlalchemy_uri)
- context = {
- "hostname": url.host,
- "password": url.password,
- "port": url.port,
- "username": url.username,
- "database": url.database,
- }
- errors = database.db_engine_spec.extract_errors(ex, context)
- raise DatabaseTestConnectionFailedError(errors) from ex
+
+ with override_user(self._actor):
Review Comment:
Same logic as previously, though indented due to the context manager.
##########
superset/reports/commands/alert.py:
##########
@@ -145,18 +147,21 @@ def _execute_query(self) -> pd.DataFrame:
limited_rendered_sql = self._report_schedule.database.apply_limit_to_sql(
rendered_sql, ALERT_SQL_LIMIT
)
- query_username = app.config["THUMBNAIL_SELENIUM_USER"]
- start = default_timer()
- df = self._report_schedule.database.get_df(
- sql=limited_rendered_sql, username=query_username
- )
- stop = default_timer()
- logger.info(
- "Query for %s took %.2f ms",
- self._report_schedule.name,
- (stop - start) * 1000.0,
- )
- return df
+
+ with override_user(
Review Comment:
Same logic as previously, though indented due to the context manager.
##########
superset/sql_lab.py:
##########
@@ -149,37 +156,35 @@ def get_sql_results( # pylint: disable=too-many-arguments
rendered_query: str,
return_results: bool = True,
store_results: bool = False,
- user_name: Optional[str] = None,
+ username: Optional[str] = None,
start_time: Optional[float] = None,
expand_data: bool = False,
log_params: Optional[Dict[str, Any]] = None,
) -> Optional[Dict[str, Any]]:
"""Executes the sql query returns the results."""
with session_scope(not ctask.request.called_directly) as session:
-
- try:
- return execute_sql_statements(
- query_id,
- rendered_query,
- return_results,
- store_results,
- user_name,
- session=session,
- start_time=start_time,
- expand_data=expand_data,
- log_params=log_params,
- )
- except Exception as ex: # pylint: disable=broad-except
- logger.debug("Query %d: %s", query_id, ex)
- stats_logger.incr("error_sqllab_unhandled")
- query = get_query(query_id, session)
- return handle_query_error(ex, query, session)
+ with override_user(current_app.app_builder.sm.find_user(username=username)):
Review Comment:
Same logic as previously, though indented due to the context manager.
##########
superset/sql_lab.py:
##########
@@ -149,37 +156,35 @@ def get_sql_results( # pylint: disable=too-many-arguments
rendered_query: str,
return_results: bool = True,
store_results: bool = False,
- user_name: Optional[str] = None,
+ username: Optional[str] = None,
Review Comment:
We still need to pass in the `username` to the async Celery task and then override.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dpgaspar commented on a diff in pull request #19914: fix: Refactor SQL engine username logic
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #19914:
URL: https://github.com/apache/superset/pull/19914#discussion_r869167018
##########
superset/utils/core.py:
##########
@@ -1400,6 +1401,30 @@ def get_username() -> Optional[str]:
return None
+@contextmanager
+def override_user(user: Optional[User]) -> Iterator[Any]:
+ """
+ Temporarily override the current user (if defined) per `flask.g`.
+
+ Sometimes, often in the context of async Celery tasks, it is useful to switch the
+ current user (which may be undefined) to different one, execute some SQLAlchemy
+ tasks and then revert back to the original one.
+
+ :param user: The override user
+ """
+
+ # pylint: disable=assigning-non-slot
+ if hasattr(g, "user"):
+ current = g.user
+ g.user = user
+ yield
+ g.user = current
+ else:
+ g.user = user
+ yield
+ delattr(g, "user")
Review Comment:
`g` is thread safe, it's created here: https://github.com/pallets/flask/blob/bd56d19b167822a9a23e2e9e2a07ccccc36baa8d/src/flask/globals.py#L59
Uses werkzeug Local: https://werkzeug.palletsprojects.com/en/1.0.x/local/#module-werkzeug.local
##########
superset/utils/core.py:
##########
@@ -1400,6 +1401,30 @@ def get_username() -> Optional[str]:
return None
+@contextmanager
+def override_user(user: Optional[User]) -> Iterator[Any]:
Review Comment:
should it be `def override_user(user: Optional[User]) -> Iterator[None]:` ?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley merged pull request #19914: fix: Refactor SQL engine username logic
Posted by GitBox <gi...@apache.org>.
john-bodley merged PR #19914:
URL: https://github.com/apache/superset/pull/19914
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #19914: fix: Refactor SQL engine username logic
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19914:
URL: https://github.com/apache/superset/pull/19914#issuecomment-1115482484
# [Codecov](https://codecov.io/gh/apache/superset/pull/19914?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19914](https://codecov.io/gh/apache/superset/pull/19914?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4dffc59) into [master](https://codecov.io/gh/apache/superset/commit/aff10a7fad0b6a48c578e70d2746d04bdf4d753c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aff10a7) will **decrease** coverage by `12.72%`.
> The diff coverage is `39.82%`.
> :exclamation: Current head 4dffc59 differs from pull request most recent head 77d90ff. Consider uploading reports for the commit 77d90ff to get more accurate results
```diff
@@ Coverage Diff @@
## master #19914 +/- ##
===========================================
- Coverage 66.52% 53.80% -12.73%
===========================================
Files 1714 1715 +1
Lines 65052 65063 +11
Branches 6722 6723 +1
===========================================
- Hits 43279 35006 -8273
- Misses 20061 28346 +8285
+ Partials 1712 1711 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.78% <31.95%> (+0.02%)` | :arrow_up: |
| python | `56.43% <34.02%> (-25.98%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `48.05% <19.58%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19914?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ns/legacy-plugin-chart-heatmap/src/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWhlYXRtYXAvc3JjL2NvbnRyb2xQYW5lbC50cw==) | `66.66% <ø> (ø)` | |
| [.../legacy-plugin-chart-histogram/src/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcGx1Z2luLWNoYXJ0LWhpc3RvZ3JhbS9zcmMvY29udHJvbFBhbmVsLnRz) | `60.00% <ø> (ø)` | |
| [...gins/legacy-preset-chart-nvd3/src/NVD3Controls.tsx](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LW52ZDMvc3JjL05WRDNDb250cm9scy50c3g=) | `100.00% <ø> (ø)` | |
| [...frontend/plugins/plugin-chart-echarts/src/types.ts](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvdHlwZXMudHM=) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `33.15% <ø> (ø)` | |
| [superset-frontend/src/types/bootstrapTypes.ts](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3R5cGVzL2Jvb3RzdHJhcFR5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.08% <ø> (-0.34%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `77.12% <ø> (-12.93%)` | :arrow_down: |
| [superset/datasets/dao.py](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvZGFvLnB5) | `46.20% <0.00%> (-47.59%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `39.21% <0.00%> (-49.49%)` | :arrow_down: |
| ... and [296 more](https://codecov.io/gh/apache/superset/pull/19914/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19914?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19914?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [aff10a7...77d90ff](https://codecov.io/gh/apache/superset/pull/19914?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org