From 8a609fd83d23bed8d0fff48d5679b3724fba895c Mon Sep 17 00:00:00 2001 From: Joe Li Date: Mon, 18 May 2026 20:02:56 -0700 Subject: [PATCH] refactor(explore): simplify form_data parse + add no-datasource regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /review-code on the post-round-5 slice flagged two minor issues genuinely introduced by 89919a1c96: * `loads_request_json` already swallows `TypeError`/`JSONDecodeError` and returns `{}`, so the explicit `try/except ValueError` wrapping it was dead defensive code. Replace it with an `isinstance(..., dict)` guard — that also closes the pre-existing `?form_data=42` edge case where a non-dict top-level JSON value would `AttributeError` on `.get()`. * The no-datasource fall-through path (the loop the fix unblocked) had no targeted regression test. `test_slices` and `test_slice_id_is_always_logged_correctly_on_web_request` exercised it only indirectly via `Slice.slice_url`. Add a sibling to `test_explore_redirect` that asserts `/explore/?form_data={"slice_id":1}` returns 200 (SPA render), not 302. Co-Authored-By: Claude Opus 4.7 (1M context) --- superset/views/explore.py | 9 ++++----- tests/integration_tests/core_tests.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/superset/views/explore.py b/superset/views/explore.py index 3023052539b..66499e5d7a2 100644 --- a/superset/views/explore.py +++ b/superset/views/explore.py @@ -44,11 +44,10 @@ class ExploreView(BaseSupersetView): # so `get_redirect_url` would return the same URL — falling back to # SPA rendering avoids a 302 loop. if request_form_data := request.args.get("form_data"): - try: - parsed_form_data = loads_request_json(request_form_data) - except ValueError: - parsed_form_data = {} - if parsed_form_data.get("datasource"): + parsed_form_data = loads_request_json(request_form_data) + if isinstance(parsed_form_data, dict) and parsed_form_data.get( + "datasource" + ): from superset.views.core import Superset # avoid circular import return redirect(Superset.get_redirect_url()) diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index ba52c69fb67..955024ba95a 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -882,6 +882,16 @@ class TestCore(SupersetTestCase): rv = self.client.get(f"/explore/?form_data={quote(json.dumps(form_data))}") assert rv.headers["Location"] == f"/explore/?form_data_key={random_key}" + def test_explore_no_datasource_renders_spa(self): + # `Slice.slice_url` emits form_data carrying only `slice_id`; without a + # datasource the cache-and-redirect contract can't produce a different + # URL, so ExploreView.root must fall through to the SPA instead of + # 302-looping back to itself. + self.login(ADMIN_USERNAME) + form_data = {"slice_id": 1} + rv = self.client.get(f"/explore/?form_data={quote(json.dumps(form_data))}") + assert rv.status_code == 200 + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_has_table(self): if backend() in ("sqlite", "mysql"):