Skip to content

Commit 2f41fce

Browse files
sadpandajoeclaude
andcommitted
fix(explore): add defensive null checks for SQL extraction in View Query modal
Addresses PR feedback for safer SQL extraction from parsing errors: - Use explicit None check instead of truthiness to preserve falsy SQL like "" - Split edge case tests into separate functions for better granularity - Remove time-specific references from test docstrings Changes: - query_actions.py: Check `is not None` to avoid discarding valid empty SQL - test_get_data_command.py: Split combined test into two independent tests - test_get_query_handles_parsing_error_with_missing_sql_key - test_get_query_handles_parsing_error_with_null_sql_value 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 345a93e commit 2f41fce

2 files changed

Lines changed: 76 additions & 6 deletions

File tree

superset/common/query_actions.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,8 @@ def _get_query(
9292
except SupersetParseError as err:
9393
# Parsing errors (SQL optimization/parsing failed)
9494
# SQL was generated but couldn't be optimized - show both
95-
if err.error.extra:
96-
result["query"] = err.error.extra.get("sql")
95+
if err.error.extra and (sql := err.error.extra.get("sql")) is not None:
96+
result["query"] = sql
9797
result["error"] = err.error.message
9898
return result
9999

tests/unit_tests/charts/commands/data/test_get_data_command.py

Lines changed: 74 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -217,10 +217,6 @@ def test_get_query_catches_parsing_error() -> None:
217217
- SQL has already been compiled (stored in error.extra['sql'])
218218
- Error message describes the parsing failure
219219
- Both should be returned to the frontend for display
220-
221-
This is Phase 2 of the fix for GitHub Issue #35492.
222-
Phase 1 fixed validation errors (before SQL generation).
223-
Phase 2 fixes parsing errors (after SQL generation, during optimization).
224220
"""
225221
from superset.common.query_actions import _get_query
226222
from superset.common.query_object import QueryObject
@@ -253,3 +249,77 @@ def test_get_query_catches_parsing_error() -> None:
253249
assert result["query"] == "SELECT SUM ( Open"
254250
assert result["error"] == "Error parsing near 'Open' at line 1:17"
255251
assert result["language"] == "sql"
252+
253+
254+
def test_get_query_handles_parsing_error_with_missing_sql_key() -> None:
255+
"""
256+
Test _get_query() when error.extra exists but 'sql' key is missing.
257+
258+
Edge case: error.extra = {"other_field": "value"} with no 'sql' key.
259+
Should NOT set result["query"] - prevents null from reaching TypeScript.
260+
Should still return error message for display.
261+
262+
Ensures defensive programming when extracting SQL from exception extra data.
263+
"""
264+
from superset.common.query_actions import _get_query
265+
from superset.common.query_object import QueryObject
266+
from superset.exceptions import SupersetParseError
267+
268+
mock_query_context = Mock()
269+
mock_query_obj = Mock(spec=QueryObject)
270+
271+
parse_error = SupersetParseError(
272+
sql="SELECT * FROM table",
273+
message="Parsing error occurred",
274+
)
275+
# Mock error.extra to NOT have sql key
276+
parse_error.error.extra = {"other_field": "some_value"}
277+
278+
with patch("superset.common.query_actions._get_datasource") as mock_get_ds:
279+
mock_datasource = Mock()
280+
mock_datasource.query_language = "sql"
281+
mock_datasource.get_query_str.side_effect = parse_error
282+
mock_get_ds.return_value = mock_datasource
283+
284+
result = _get_query(mock_query_context, mock_query_obj, False)
285+
286+
assert "query" not in result
287+
assert result["error"] == "Parsing error occurred"
288+
assert result["language"] == "sql"
289+
290+
291+
def test_get_query_handles_parsing_error_with_null_sql_value() -> None:
292+
"""
293+
Test _get_query() when error.extra has 'sql': None explicitly set.
294+
295+
Edge case: error.extra = {"sql": None} with sql key present but value is None.
296+
Should NOT set result["query"] - prevents null from reaching TypeScript.
297+
Should still return error message for display.
298+
299+
Ensures defensive programming when extracting SQL from exception extra data.
300+
"""
301+
from superset.common.query_actions import _get_query
302+
from superset.common.query_object import QueryObject
303+
from superset.exceptions import SupersetParseError
304+
305+
mock_query_context = Mock()
306+
mock_query_obj = Mock(spec=QueryObject)
307+
308+
parse_error = SupersetParseError(
309+
sql="SELECT * FROM table",
310+
message="Parsing error occurred",
311+
)
312+
# Mock error.extra to have sql key with None value
313+
parse_error.error.extra = {"sql": None}
314+
315+
with patch("superset.common.query_actions._get_datasource") as mock_get_ds:
316+
mock_datasource = Mock()
317+
mock_datasource.query_language = "sql"
318+
mock_datasource.get_query_str.side_effect = parse_error
319+
mock_get_ds.return_value = mock_datasource
320+
321+
result = _get_query(mock_query_context, mock_query_obj, False)
322+
323+
assert "query" not in result
324+
assert result["error"] == "Parsing error occurred"
325+
assert result["language"] == "sql"

0 commit comments

Comments
 (0)