This is an automated email from the ASF dual-hosted git repository. jli pushed a commit to branch issue-35492-fix-show-query-with-error in repository https://gitbox.apache.org/repos/asf/superset.git
commit f04b81ebd9d7e0421f125d33caad13db04f6a0ae Author: Joe Li <[email protected]> AuthorDate: Mon Nov 3 16:48:52 2025 -0800 fix(explore): show validation errors in View Query modal When chart queries fail due to validation errors (e.g., missing required fields, invalid metrics), the View Query modal now displays helpful error messages instead of showing a blank panel. This restores functionality that was broken in 2023 when ChartDataCommand was added. The command raised exceptions for ALL errors, but should only fail-fast for data execution requests, not query-only requests. Changes: - Backend: Skip error check in ChartDataCommand for result_type='query' - Schema: Make query field optional (validation errors return error without SQL) - Frontend: Display Alert component for errors, use item.language from backend - Tests: Add 7 unit tests including regression test to prevent future breakage Fixes #35492 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --- .../explore/components/controls/ViewQueryModal.tsx | 19 +- superset/charts/schemas.py | 9 +- superset/commands/chart/data/get_data_command.py | 8 +- tests/unit_tests/charts/commands/data/__init__.py | 16 ++ .../charts/commands/data/test_get_data_command.py | 209 +++++++++++++++++++++ 5 files changed, 252 insertions(+), 9 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/ViewQueryModal.tsx b/superset-frontend/src/explore/components/controls/ViewQueryModal.tsx index d84e1671b7..db37998b8a 100644 --- a/superset-frontend/src/explore/components/controls/ViewQueryModal.tsx +++ b/superset-frontend/src/explore/components/controls/ViewQueryModal.tsx @@ -25,7 +25,8 @@ import { getClientErrorObject, QueryFormData, } from '@superset-ui/core'; -import { Loading } from '@superset-ui/core/components'; +import { Loading, Alert } from '@superset-ui/core/components'; +import { SupportedLanguage } from '@superset-ui/core/components/CodeSyntaxHighlighter'; import { getChartDataRequest } from 'src/components/Chart/chartAction'; import ViewQuery from 'src/explore/components/controls/ViewQuery'; @@ -34,8 +35,9 @@ interface Props { } type Result = { - query: string; - language: string; + query?: string; + language: SupportedLanguage; + error?: string; }; const ViewQueryModalContainer = styled.div` @@ -88,12 +90,19 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData }) => { return ( <ViewQueryModalContainer> {result.map((item, index) => - item.query ? ( + item.error ? ( + <Alert + key={`error-${index}`} + type="error" + message={item.error} + closable={false} + /> + ) : item.query ? ( <ViewQuery key={`query-${index}`} datasource={latestQueryFormData.datasource} sql={item.query} - language="sql" + language={item.language} /> ) : null, )} diff --git a/superset/charts/schemas.py b/superset/charts/schemas.py index 5704210c49..a767be42b0 100644 --- a/superset/charts/schemas.py +++ b/superset/charts/schemas.py @@ -1482,9 +1482,12 @@ class ChartDataResponseResult(Schema): allow_none=None, ) query = fields.String( - metadata={"description": "The executed query statement"}, - required=True, - allow_none=False, + metadata={ + "description": "The executed query statement. May be absent when " + "validation errors occur." + }, + required=False, + allow_none=True, ) status = fields.String( metadata={"description": "Status of the query"}, diff --git a/superset/commands/chart/data/get_data_command.py b/superset/commands/chart/data/get_data_command.py index ad53a03f28..eeaa860aad 100644 --- a/superset/commands/chart/data/get_data_command.py +++ b/superset/commands/chart/data/get_data_command.py @@ -24,6 +24,7 @@ from superset.commands.chart.exceptions import ( ChartDataCacheLoadError, ChartDataQueryFailedError, ) +from superset.common.chart_data import ChartDataResultType from superset.common.query_context import QueryContext from superset.exceptions import CacheLoadError @@ -48,8 +49,13 @@ class ChartDataCommand(BaseCommand): except CacheLoadError as ex: raise ChartDataCacheLoadError(ex.message) from ex + # Skip error check for query-only requests - errors are returned in payload + # This allows View Query modal to display validation errors for query in payload["queries"]: - if query.get("error"): + if ( + query.get("error") + and self._query_context.result_type != ChartDataResultType.QUERY + ): raise ChartDataQueryFailedError( _("Error: %(error)s", error=query["error"]) ) diff --git a/tests/unit_tests/charts/commands/data/__init__.py b/tests/unit_tests/charts/commands/data/__init__.py new file mode 100644 index 0000000000..13a83393a9 --- /dev/null +++ b/tests/unit_tests/charts/commands/data/__init__.py @@ -0,0 +1,16 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. diff --git a/tests/unit_tests/charts/commands/data/test_get_data_command.py b/tests/unit_tests/charts/commands/data/test_get_data_command.py new file mode 100644 index 0000000000..64de546e29 --- /dev/null +++ b/tests/unit_tests/charts/commands/data/test_get_data_command.py @@ -0,0 +1,209 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from unittest.mock import Mock + +import pytest + +from superset.commands.chart.data.get_data_command import ChartDataCommand +from superset.commands.chart.exceptions import ChartDataQueryFailedError +from superset.common.chart_data import ChartDataResultType +from superset.common.query_context import QueryContext + + +def test_query_result_type_allows_validation_error_payload() -> None: + """ + Regression test: Ensure result_type='query' with error payload returns + the error instead of raising ChartDataQueryFailedError. + + This locks in the behavior where validation errors are passed through + to the frontend for display in ViewQueryModal. + + Context: + - GitHub Issue #35492 + - Superset 4.1.3 allowed errors to pass through + - Command reorganization in 2023 broke this behavior + - This test ensures errors pass through for query-only requests + """ + # Mock QueryContext with result_type=QUERY + mock_query_context = Mock(spec=QueryContext) + mock_query_context.result_type = ChartDataResultType.QUERY + mock_query_context.get_payload.return_value = { + "queries": [{"error": "Missing temporal column", "language": "sql"}] + } + + command = ChartDataCommand(mock_query_context) + + # Should NOT raise - this is the key assertion for the regression test + result = command.run() + + # Verify error is passed through in the response + assert result["queries"][0]["error"] == "Missing temporal column" + assert result["queries"][0]["language"] == "sql" + assert "query" not in result["queries"][0] # No SQL for validation errors + + +def test_full_result_type_raises_on_error() -> None: + """ + Test that result_type='full' with error raises ChartDataQueryFailedError. + + This ensures data requests continue to fail fast when errors occur, + maintaining existing behavior for non-query requests. + """ + # Mock QueryContext with result_type=FULL + mock_query_context = Mock(spec=QueryContext) + mock_query_context.result_type = ChartDataResultType.FULL + mock_query_context.get_payload.return_value = { + "queries": [{"error": "Invalid column name"}] + } + + command = ChartDataCommand(mock_query_context) + + # Should raise exception for data requests + with pytest.raises(ChartDataQueryFailedError) as exc_info: + command.run() + + assert "Invalid column name" in str(exc_info.value) + + +def test_results_result_type_raises_on_error() -> None: + """ + Test that result_type='results' with error raises ChartDataQueryFailedError. + + Ensures fail-fast behavior is preserved for results-only requests. + """ + # Mock QueryContext with result_type=RESULTS + mock_query_context = Mock(spec=QueryContext) + mock_query_context.result_type = ChartDataResultType.RESULTS + mock_query_context.get_payload.return_value = { + "queries": [{"error": "Database connection failed"}] + } + + command = ChartDataCommand(mock_query_context) + + # Should raise exception for results requests + with pytest.raises(ChartDataQueryFailedError) as exc_info: + command.run() + + assert "Database connection failed" in str(exc_info.value) + + +def test_query_result_type_returns_successful_query() -> None: + """ + Test that result_type='query' without error returns query successfully. + + Ensures no regression for successful query requests. + """ + # Mock QueryContext with result_type=QUERY and successful query + mock_query_context = Mock(spec=QueryContext) + mock_query_context.result_type = ChartDataResultType.QUERY + mock_query_context.get_payload.return_value = { + "queries": [{"query": "SELECT * FROM table", "language": "sql"}] + } + + command = ChartDataCommand(mock_query_context) + + # Should return query successfully + result = command.run() + + assert result["queries"][0]["query"] == "SELECT * FROM table" + assert result["queries"][0]["language"] == "sql" + assert "error" not in result["queries"][0] + + +def test_full_result_type_returns_successful_data() -> None: + """ + Test that result_type='full' without error returns data successfully. + + Ensures no regression for successful data requests. + """ + # Mock QueryContext with result_type=FULL and successful data + mock_query_context = Mock(spec=QueryContext) + mock_query_context.result_type = ChartDataResultType.FULL + mock_query_context.get_payload.return_value = { + "queries": [{"data": [{"col1": "value1"}], "colnames": ["col1"]}] + } + + command = ChartDataCommand(mock_query_context) + + # Should return data successfully + result = command.run() + + assert result["queries"][0]["data"] == [{"col1": "value1"}] + assert result["queries"][0]["colnames"] == ["col1"] + assert "error" not in result["queries"][0] + + +def test_query_result_type_with_multiple_queries_and_mixed_results() -> None: + """ + Test that result_type='query' handles multiple queries with mixed results. + + Some queries may succeed while others have validation errors. + All should be returned without raising exceptions. + """ + # Mock QueryContext with multiple queries + mock_query_context = Mock(spec=QueryContext) + mock_query_context.result_type = ChartDataResultType.QUERY + mock_query_context.get_payload.return_value = { + "queries": [ + {"query": "SELECT * FROM table1", "language": "sql"}, + {"error": "Missing required field", "language": "sql"}, + {"query": "SELECT * FROM table2", "language": "sql"}, + ] + } + + command = ChartDataCommand(mock_query_context) + + # Should return all queries without raising + result = command.run() + + # Verify first query succeeded + assert result["queries"][0]["query"] == "SELECT * FROM table1" + assert "error" not in result["queries"][0] + + # Verify second query has error + assert result["queries"][1]["error"] == "Missing required field" + assert "query" not in result["queries"][1] + + # Verify third query succeeded + assert result["queries"][2]["query"] == "SELECT * FROM table2" + assert "error" not in result["queries"][2] + + +def test_full_result_type_fails_fast_on_first_error_in_multiple_queries() -> None: + """ + Test that result_type='full' raises on first error even with multiple queries. + + Ensures fail-fast behavior when multiple queries are present. + """ + # Mock QueryContext with multiple queries where first has error + mock_query_context = Mock(spec=QueryContext) + mock_query_context.result_type = ChartDataResultType.FULL + mock_query_context.get_payload.return_value = { + "queries": [ + {"error": "First query failed"}, + {"data": [{"col1": "value1"}]}, + ] + } + + command = ChartDataCommand(mock_query_context) + + # Should raise on first error without processing remaining queries + with pytest.raises(ChartDataQueryFailedError) as exc_info: + command.run() + + assert "First query failed" in str(exc_info.value)
