(superset) branch ultraset-fix-37695 created (now 741722d6928)

[jl...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

sadpandajoe pushed a change to branch ultraset-fix-37695
in repository https://gitbox.apache.org/repos/asf/superset.git


      at 741722d6928 fix(sql lab): keep the real Flask session on synchronous runs

This branch includes the following new commits:

     new 741722d6928 fix(sql lab): keep the real Flask session on synchronous runs

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.

(superset) 01/01: fix(sql lab): keep the real Flask session on synchronous runs

[jl...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

sadpandajoe pushed a commit to branch ultraset-fix-37695
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 741722d6928395af70139bb235c5955251a99965
Author: sadpandajoe <47...@users.noreply.github.com>
AuthorDate: Fri Sep 18 13:01:47 2026 +0000

    fix(sql lab): keep the real Flask session on synchronous runs
    
    `get_sql_results` unconditionally opened `app.test_request_context()`,
    which was added in d5c0506faa (#29461) so Celery workers have a request
    context to build the OAuth2 redirect URI from.
    
    SQL Lab's synchronous executor calls that task directly rather than
    through Celery, so it already runs inside the authenticated request.
    Pushing a second, fabricated context there replaced the real Flask
    session with an empty one, so RLS clauses whose Jinja macros read
    `flask.session` saw nothing. Flask binds `session` to the request
    context but `g` to the app context, so `g.user` kept working and only
    session-dependent code broke.
    
    Fabricate the request context only when there isn't one, leaving the
    Celery path unchanged.
    
    Fixes #37695
    
    Co-Authored-By: Claude Opus 5 <no...@anthropic.com>
---
 superset/sql_lab.py              | 11 ++++--
 tests/unit_tests/sql_lab_test.py | 72 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/superset/sql_lab.py b/superset/sql_lab.py
index 234843def92..dfb3124948f 100644
--- a/superset/sql_lab.py
+++ b/superset/sql_lab.py
@@ -20,7 +20,7 @@ import logging
 import sys
 import traceback
 import uuid
-from contextlib import closing
+from contextlib import closing, nullcontext
 from datetime import datetime
 from sys import getsizeof
 from typing import Any, cast, Optional, TYPE_CHECKING, TypeVar, Union
@@ -28,7 +28,7 @@ from typing import Any, cast, Optional, TYPE_CHECKING, TypeVar, Union
 import backoff
 import msgpack
 from celery.exceptions import SoftTimeLimitExceeded
-from flask import current_app as app, has_app_context
+from flask import current_app as app, has_app_context, has_request_context
 from flask_babel import gettext as __
 
 from superset import (
@@ -231,7 +231,12 @@ def get_sql_results(  # pylint: disable=too-many-arguments
     log_params: Optional[dict[str, Any]] = None,
 ) -> Optional[dict[str, Any]]:
     """Executes the sql query returns the results."""
-    with app.test_request_context():
+    # A Celery worker has no originating request, and one is needed to build the
+    # OAuth2 redirect URI, so fabricate a request context there. SQL Lab's
+    # synchronous executor calls this task directly from inside the authenticated
+    # request, where a nested context would replace the real Flask session with an
+    # empty one -- breaking RLS clauses whose Jinja macros read ``flask.session``.
+    with nullcontext() if has_request_context() else app.test_request_context():
         with override_user(security_manager.find_user(username)):
             try:
                 return execute_sql_statements(
diff --git a/tests/unit_tests/sql_lab_test.py b/tests/unit_tests/sql_lab_test.py
index 2428eb6f076..e3e2a65957c 100644
--- a/tests/unit_tests/sql_lab_test.py
+++ b/tests/unit_tests/sql_lab_test.py
@@ -23,6 +23,7 @@ from urllib.parse import parse_qs, urlparse
 from uuid import UUID
 
 import pytest
+from flask import g, has_request_context, session
 from freezegun import freeze_time
 from pytest_mock import MockerFixture
 from sqlalchemy import text
@@ -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"
+    assert captured["user"] == "the-user"
+
+
+def test_get_sql_results_builds_a_request_when_there_is_none(
+    mocker: MockerFixture, app: SupersetApp
+) -> None:
+    """
+    Test that a Celery run still gets a request context of its own.
+
+    A worker has no originating request, and the OAuth2 flow needs a request
+    context to build its redirect URI, so one must still be created there.
+    """
+    captured = _capture_execution_context(mocker)
+
+    with app.app_context():
+        assert not has_request_context()
+        get_sql_results(query_id=1, rendered_query="SELECT 1", username="alice")
+
+    assert captured["has_request_context"] is True
+    assert captured["user"] == "the-user"
+
+
 def test_apply_rls(mocker: MockerFixture) -> None:
     """
     Test the ``apply_rls`` helper function.