You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sadpandajoe (via GitHub)" <gi...@apache.org> on 2026/09/18 13:30:38 UTC
[PR] fix(sql lab): keep the real Flask session on synchronous runs [superset]
sadpandajoe opened a new pull request, #44428:
URL: https://github.com/apache/superset/pull/44428
### SUMMARY
Introduced by d5c0506faa ("fix: OAuth2 in async DBs", #29461), which wrapped
`get_sql_results` in `app.test_request_context()` so Celery workers — which have
no originating request — can build the OAuth2 redirect URI.
The wrapper is unconditional, but SQL Lab's synchronous executor doesn't go
through Celery: it calls the task object directly
(`SynchronousSqlJsonExecutor._get_sql_results`), from inside the already
authenticated request. So a plain "Run" in SQL Lab pushes a *second*, fabricated
request context inside the real one. Flask binds `session` to the request context
and `g` to the app context, so `g.user` keeps working while `flask.session` is
silently replaced with a brand-new empty one — which is why the reporter saw an
entirely blank session rather than a partially populated one, and why nothing
else in SQL Lab noticed.
With `RLS_IN_SQLLAB` enabled, `execute_sql_statements` reaches `apply_rls` →
`get_predicates_for_table` → `SqlaTable.get_sqla_row_level_filters()`, which
Jinja-renders each RLS filter's clause. A custom `JINJA_CONTEXT_ADDONS` macro
that reads `flask.session` therefore runs against the empty session and returns
nothing, so the RLS predicate silently collapses.
This fabricates the request context only when there isn't already one. The Celery
path is unaffected and still gets its context; the synchronous path now reuses the
real request, so session-dependent Jinja macros behave the same in SQL Lab as they
already do when rendering a chart.
Note this fixes the synchronous path, which is the default. A deployment running
SQL Lab asynchronously still has no browser session inside the worker — that is an
architectural limitation of async execution, not something this change addresses.
### TESTING INSTRUCTIONS
1. Set `RLS_IN_SQLLAB: True` in `FEATURE_FLAGS`.
2. Add a Jinja macro to `JINJA_CONTEXT_ADDONS` in `superset_config.py` that reads
the Flask session, e.g.:
```python
from flask import session
JINJA_CONTEXT_ADDONS = {"rls": lambda: session.get("rls", "(NULL)")}
```
Populate `session["rls"]` during login (or temporarily hardcode a write to it
in a `before_request` hook to test).
3. Create an RLS rule on a dataset whose clause uses the macro, e.g.
`column_name IN {{ rls() }}`.
4. Open a chart on that dataset, and from its menu choose "View query" / open it
in SQL Lab. Run the query in SQL Lab (leave "Run async" off).
5. Before this change, the rendered RLS predicate is empty — the macro sees an
empty session. After it, the predicate renders with the session values, matching
what the chart already produced.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #37695
- [ ] 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
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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
Re: [PR] fix(sql lab): keep the real Flask session on synchronous runs [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44428:
URL: https://github.com/apache/superset/pull/44428#issuecomment-5730711711
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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
Re: [PR] fix(sql lab): keep the real Flask session on synchronous runs [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44428:
URL: https://github.com/apache/superset/pull/44428#issuecomment-5737409767
<!-- Bito Reply -->
The observation that the suggested assertion is insufficient for detecting the regression is noted. Since the specific line identified as the cause of the regression is already present and the current test behavior is understood, no further changes are required.
--
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
Re: [PR] fix(sql lab): keep the real Flask session on synchronous runs [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44428:
URL: https://github.com/apache/superset/pull/44428#issuecomment-5730775287
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44428?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 57.39%. Comparing base ([`a7dab4a`](https://app.codecov.io/gh/apache/superset/commit/a7dab4a5519cfc1cbfd2e2e1c83622cc1110ccde?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`741722d`](https://app.codecov.io/gh/apache/superset/commit/741722d6928395af70139bb235c5955251a99965?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (a7dab4a) and HEAD (741722d). Click for more details.
>
> <details><summary>HEAD has 57 uploads less than BASE</summary>
>
>| Flag | BASE (a7dab4a) | HEAD (741722d) |
>|------|------|------|
>|hive|8|1|
>|python|45|2|
>|presto|8|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44428 +/- ##
===========================================
- Coverage 80.57% 57.39% -23.18%
===========================================
Files 2940 2940
Lines 175316 175316
Branches 40696 40696
===========================================
- Hits 141253 100624 -40629
- Misses 31398 73732 +42334
+ Partials 2665 960 -1705
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44428/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44428/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.25% <100.00%> (ø)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44428/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44428/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44428/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.14% <100.00%> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/44428/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.19% <100.00%> (-45.69%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44428/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44428/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44428?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] fix(sql lab): keep the real Flask session on synchronous runs [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44428:
URL: https://github.com/apache/superset/pull/44428
--
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
Re: [PR] fix(sql lab): keep the real Flask session on synchronous runs [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44428:
URL: https://github.com/apache/superset/pull/44428#discussion_r4051404163
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -559,6 +560,77 @@ def test_get_sql_results_oauth2(mocker: MockerFixture, app) -> None:
app_context.pop()
+def _capture_execution_context(mocker: MockerFixture) -> dict[str, Any]:
+ """
+ Stub out ``execute_sql_statements`` and record the context it runs under.
+ """
+ captured: dict[str, Any] = {}
+
+ def execute_sql_statements(*args: Any, **kwargs: Any) -> dict[str, Any]:
+ captured["rls_tenant"] = session.get("rls_tenant")
+ captured["has_request_context"] = has_request_context()
+ captured["user"] = g.user
+ return {"status": QueryStatus.SUCCESS}
+
+ mocker.patch(
+ "superset.sql_lab.execute_sql_statements",
+ side_effect=execute_sql_statements,
+ )
+ mocker.patch(
+ "superset.sql_lab.security_manager.find_user",
+ return_value="the-user",
+ )
+ return captured
+
+
+def test_get_sql_results_reuses_the_active_request(
+ mocker: MockerFixture, app: SupersetApp
+) -> None:
+ """
+ Test that a synchronous run keeps the real Flask session.
+
+ SQL Lab's synchronous executor invokes this task directly rather than through
+ Celery, so it already runs inside the authenticated request. Fabricating a
+ second request context there would swap the real session for an empty one and
+ break RLS clauses whose Jinja macros read ``flask.session``.
+ """
+ captured = _capture_execution_context(mocker)
+
+ # Pushed/popped manually (rather than via a ``with`` block) so the
+ # ``finally`` below still pops it if an assertion fails, preventing the
+ # request context from leaking into later tests in the same session.
+ app_context = app.test_request_context()
+ app_context.push()
+
+ try:
+ session["rls_tenant"] = "acme"
+ get_sql_results(query_id=1, rendered_query="SELECT 1")
+ finally:
+ app_context.pop()
+
+ assert captured["rls_tenant"] == "acme"
Review Comment:
bito's suggested `has_request_context` assertion can't catch that regression — it is `True` for a nested context too. On the pinned Flask 3.1.3, forcing `app.test_request_context()` unconditionally still passes with only that assertion; this `rls_tenant` line is what reds. No change needed.
--
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