diff --git a/superset-frontend/src/explore/components/controls/ViewQueryModal.test.tsx b/superset-frontend/src/explore/components/controls/ViewQueryModal.test.tsx new file mode 100644 index 000000000000..64a53aceeb22 --- /dev/null +++ b/superset-frontend/src/explore/components/controls/ViewQueryModal.test.tsx @@ -0,0 +1,108 @@ +/** + * 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. + */ + +import { screen, render, waitFor } from 'spec/helpers/testing-library'; +import fetchMock from 'fetch-mock'; +import ViewQueryModal from './ViewQueryModal'; + +const mockFormData = { + datasource: '1__table', + viz_type: 'table', +}; + +const chartDataEndpoint = 'glob:*/api/v1/chart/data*'; + +afterEach(() => { + jest.resetAllMocks(); + fetchMock.restore(); +}); + +test('renders Alert component when query result contains validation error', async () => { + /** + * Regression test for issue #35492 - Phase 1 + * Verifies that validation errors from the backend are displayed in an Alert + * component instead of showing a blank panel + */ + // Mock API response with validation error + fetchMock.post(chartDataEndpoint, { + result: [ + { + error: 'Missing temporal column', + language: 'sql', + }, + ], + }); + + render(, { + useRedux: true, + }); + + // Wait for API call to complete + await waitFor(() => + expect(fetchMock.calls(chartDataEndpoint)).toHaveLength(1), + ); + + // Assert Alert component is rendered with error message + expect(screen.getByRole('alert')).toBeInTheDocument(); + expect(screen.getByText('Missing temporal column')).toBeInTheDocument(); +}); + +test('renders both Alert and SQL query when parsing error occurs', async () => { + /** + * Regression test for issue #35492 - Phase 2 + * Verifies that parsing errors (which occur after SQL generation) display + * both the error message AND the SQL query that failed to parse. + * + * This differs from validation errors (Phase 1) where no SQL was generated. + * For parsing errors, the SQL was successfully compiled but optimization failed. + */ + // Mock API response with parsing error (has both query and error) + fetchMock.post(chartDataEndpoint, { + result: [ + { + query: 'SELECT SUM ( Open', + error: "Error parsing near 'Open' at line 1:17", + language: 'sql', + }, + ], + }); + + render(, { + useRedux: true, + }); + + // Wait for the error message to appear + await waitFor(() => + expect( + screen.getByText("Error parsing near 'Open' at line 1:17"), + ).toBeInTheDocument(), + ); + + // Assert Alert component is rendered with error message + expect(screen.getByRole('alert')).toBeInTheDocument(); + + // Assert SQL query is also displayed + // Note: The SQL is rendered inside a syntax-highlighted code block where + // each keyword is in a separate span element + await waitFor(() => { + expect(screen.getByText('SELECT')).toBeInTheDocument(); + expect(screen.getByText('SUM')).toBeInTheDocument(); + expect(screen.getByText('Open')).toBeInTheDocument(); + }); +}); diff --git a/superset-frontend/src/explore/components/controls/ViewQueryModal.tsx b/superset-frontend/src/explore/components/controls/ViewQueryModal.tsx index 94c78e14c95f..8064f973fd3d 100644 --- a/superset-frontend/src/explore/components/controls/ViewQueryModal.tsx +++ b/superset-frontend/src/explore/components/controls/ViewQueryModal.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { FC, useEffect, useState } from 'react'; +import { FC, Fragment, useEffect, useState } from 'react'; import { ensureIsArray, @@ -24,8 +24,9 @@ import { getClientErrorObject, QueryFormData, } from '@superset-ui/core'; -import { styled } from '@apache-superset/core/ui'; +import { styled, Alert } from '@apache-superset/core/ui'; import { Loading } 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` @@ -87,16 +89,21 @@ const ViewQueryModal: FC = ({ latestQueryFormData }) => { return ( - {result.map((item, index) => - item.query ? ( - - ) : null, - )} + {result.map((item, index) => ( + // Static API response data - index is appropriate for keys + + {item.error && ( + + )} + {item.query && ( + + )} + + ))} ); }; diff --git a/superset/charts/schemas.py b/superset/charts/schemas.py index 5704210c498a..a767be42b08e 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 ad53a03f285d..eeaa860aadb8 100644 --- a/superset/commands/chart/data/get_data_command.py +++ b/superset/commands/chart/data/get_data_command.py @@ -24,6 +24,7 @@ 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 @@ def run(self, **kwargs: Any) -> dict[str, Any]: 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/superset/common/query_actions.py b/superset/common/query_actions.py index a18e40a66bf9..c99bd2a431d8 100644 --- a/superset/common/query_actions.py +++ b/superset/common/query_actions.py @@ -24,7 +24,7 @@ from superset.common.chart_data import ChartDataResultType from superset.common.db_query_status import QueryStatus from superset.connectors.sqla.models import BaseDatasource -from superset.exceptions import QueryObjectValidationError +from superset.exceptions import QueryObjectValidationError, SupersetParseError from superset.utils.core import ( extract_column_dtype, extract_dataframe_dtypes, @@ -86,7 +86,15 @@ def _get_query( try: result["query"] = datasource.get_query_str(query_obj.to_dict()) except QueryObjectValidationError as err: + # Validation errors (missing required fields, invalid config) + # No SQL was generated result["error"] = err.message + except SupersetParseError as err: + # Parsing errors (SQL optimization/parsing failed) + # SQL was generated but couldn't be optimized - show both + if err.error.extra and (sql := err.error.extra.get("sql")) is not None: + result["query"] = sql + result["error"] = err.error.message return result 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 000000000000..13a83393a912 --- /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 000000000000..378e127f216e --- /dev/null +++ b/tests/unit_tests/charts/commands/data/test_get_data_command.py @@ -0,0 +1,325 @@ +# 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, patch + +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) + + +def test_get_query_catches_parsing_error() -> None: + """ + Test that _get_query() catches SupersetParseError and returns both SQL and error. + + When SQL generation succeeds but optimization/parsing fails: + - SQL has already been compiled (stored in error.extra['sql']) + - Error message describes the parsing failure + - Both should be returned to the frontend for display + """ + from superset.common.query_actions import _get_query + from superset.common.query_object import QueryObject + from superset.exceptions import SupersetParseError + + # Create mock query_context and query_obj + mock_query_context = Mock() + mock_query_obj = Mock(spec=QueryObject) + + # Create SupersetParseError with SQL in error.extra + parse_error = SupersetParseError( + sql="SELECT SUM ( Open", # SQL was generated before parsing failed + engine="postgresql", + message="Error parsing near 'Open' at line 1:17", + line=1, + column=17, + ) + + # Mock _get_datasource and datasource.get_query_str to raise the error + with patch("superset.common.query_actions._get_datasource") as mock_get_ds: + mock_datasource = Mock() + mock_datasource.query_language = "sql" + mock_datasource.get_query_str.side_effect = parse_error + mock_get_ds.return_value = mock_datasource + + # GREEN: Exception is caught, values returned (new behavior after fix) + result = _get_query(mock_query_context, mock_query_obj, False) + + # Should return both query (from error.extra['sql']) and error message + assert result["query"] == "SELECT SUM ( Open" + assert result["error"] == "Error parsing near 'Open' at line 1:17" + assert result["language"] == "sql" + + +def test_get_query_handles_parsing_error_with_missing_sql_key() -> None: + """ + Test _get_query() when error.extra exists but 'sql' key is missing. + + Edge case: error.extra = {"other_field": "value"} with no 'sql' key. + Should NOT set result["query"] - prevents null from reaching TypeScript. + Should still return error message for display. + + Ensures defensive programming when extracting SQL from exception extra data. + """ + from superset.common.query_actions import _get_query + from superset.common.query_object import QueryObject + from superset.exceptions import SupersetParseError + + mock_query_context = Mock() + mock_query_obj = Mock(spec=QueryObject) + + parse_error = SupersetParseError( + sql="SELECT * FROM table", + message="Parsing error occurred", + ) + # Mock error.extra to NOT have sql key + parse_error.error.extra = {"other_field": "some_value"} + + with patch("superset.common.query_actions._get_datasource") as mock_get_ds: + mock_datasource = Mock() + mock_datasource.query_language = "sql" + mock_datasource.get_query_str.side_effect = parse_error + mock_get_ds.return_value = mock_datasource + + result = _get_query(mock_query_context, mock_query_obj, False) + + assert "query" not in result + assert result["error"] == "Parsing error occurred" + assert result["language"] == "sql" + + +def test_get_query_handles_parsing_error_with_null_sql_value() -> None: + """ + Test _get_query() when error.extra has 'sql': None explicitly set. + + Edge case: error.extra = {"sql": None} with sql key present but value is None. + Should NOT set result["query"] - prevents null from reaching TypeScript. + Should still return error message for display. + + Ensures defensive programming when extracting SQL from exception extra data. + """ + from superset.common.query_actions import _get_query + from superset.common.query_object import QueryObject + from superset.exceptions import SupersetParseError + + mock_query_context = Mock() + mock_query_obj = Mock(spec=QueryObject) + + parse_error = SupersetParseError( + sql="SELECT * FROM table", + message="Parsing error occurred", + ) + # Mock error.extra to have sql key with None value + parse_error.error.extra = {"sql": None} + + with patch("superset.common.query_actions._get_datasource") as mock_get_ds: + mock_datasource = Mock() + mock_datasource.query_language = "sql" + mock_datasource.get_query_str.side_effect = parse_error + mock_get_ds.return_value = mock_datasource + + result = _get_query(mock_query_context, mock_query_obj, False) + + assert "query" not in result + assert result["error"] == "Parsing error occurred" + assert result["language"] == "sql"