diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts index 73c1482a0444..915c3392f9a9 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts @@ -33,13 +33,15 @@ import { addJsColumnsToColumns, processMetricsArray, addTooltipColumnsToQuery, + ensureColumnsUnique, } from '../buildQueryUtils'; export interface DeckScatterFormData extends Omit, SqlaFormData { point_radius_fixed?: { - value?: string; + type?: string; + value?: string | number; }; multiplier?: number; point_unit?: string; @@ -75,17 +77,23 @@ export default function buildQuery(formData: DeckScatterFormData) { typeof col === 'string' ? col : col.label || col.sqlExpression || '', ); const withJsColumns = addJsColumnsToColumns(columnStrings, js_columns); + const uniqueColumns = ensureColumnsUnique(withJsColumns); - columns = withJsColumns as QueryFormColumn[]; + columns = uniqueColumns as QueryFormColumn[]; columns = addTooltipColumnsToQuery(columns, tooltip_contents); - const metrics = processMetricsArray([point_radius_fixed?.value]); + const isRadiusMetric = + point_radius_fixed?.type === 'metric' && point_radius_fixed?.value; + const metrics = isRadiusMetric + ? processMetricsArray([String(point_radius_fixed.value)]) + : []; + const filters = addSpatialNullFilters( spatial, ensureIsArray(baseQueryObject.filters || []), ); - const orderby = point_radius_fixed?.value + const orderby = isRadiusMetric ? ([[point_radius_fixed.value, false]] as QueryFormOrderBy[]) : (baseQueryObject.orderby as QueryFormOrderBy[]) || []; diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/spatialUtils.ts b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/spatialUtils.ts index 1572c0933b53..6de510b201f4 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/spatialUtils.ts +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/spatialUtils.ts @@ -109,11 +109,20 @@ export function addSpatialNullFilters( if (!spatial) return filters; const spatialColumns = getSpatialColumns(spatial); - const nullFilters: QueryObjectFilterClause[] = spatialColumns.map(column => ({ - col: column, - op: 'IS NOT NULL', - val: null, - })); + const uniqueSpatialColumns = [...new Set(spatialColumns)]; + const existingFilterCols = new Set( + filters + .filter(filter => filter.op === 'IS NOT NULL') + .map(filter => filter.col), + ); + + const nullFilters: QueryObjectFilterClause[] = uniqueSpatialColumns + .filter(column => !existingFilterCols.has(column)) + .map(column => ({ + col: column, + op: 'IS NOT NULL', + val: null, + })); return [...filters, ...nullFilters]; } diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.ts b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.ts index 6427db900a7f..4dff65a36d31 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.ts +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.ts @@ -134,9 +134,10 @@ export function addPropertiesToFeature>( } export function getMetricLabelFromFormData( - metric: string | { value?: string } | undefined, + metric: string | { type?: string; value?: string | number } | undefined, ): string | undefined { if (!metric) return undefined; if (typeof metric === 'string') return getMetricLabel(metric); - return metric.value ? getMetricLabel(metric.value) : undefined; + if (metric.type === 'fix') return undefined; + return metric.value ? getMetricLabel(String(metric.value)) : undefined; } diff --git a/superset/charts/schemas.py b/superset/charts/schemas.py index a767be42b08e..4e8a7964ed98 100644 --- a/superset/charts/schemas.py +++ b/superset/charts/schemas.py @@ -1142,6 +1142,12 @@ class AnnotationLayerSchema(Schema): ) +def validate_orderby_column(value: Any) -> bool: + if value is None or (isinstance(value, str) and len(value) == 0): + raise validate.ValidationError(_("orderby column must be populated")) + return True + + class ChartDataDatasourceSchema(Schema): description = "Chart datasource" id = Union( @@ -1339,9 +1345,7 @@ class Meta: # pylint: disable=too-few-public-methods fields.Tuple( ( fields.Raw( - validate=[ - Length(min=1, error=_("orderby column must be populated")) - ], + validate=validate_orderby_column, allow_none=False, ), fields.Boolean(), diff --git a/superset/common/query_object.py b/superset/common/query_object.py index 9ef1d10ba92a..204393d586be 100644 --- a/superset/common/query_object.py +++ b/superset/common/query_object.py @@ -189,11 +189,12 @@ def _set_metrics(self, metrics: list[Metric] | None = None) -> None: # 1. 'metric_name' - name of predefined metric # 2. { label: 'label_name' } - legacy format for a predefined metric # 3. { expressionType: 'SIMPLE' | 'SQL', ... } - adhoc metric - def is_str_or_adhoc(metric: Metric) -> bool: - return isinstance(metric, str) or is_adhoc_metric(metric) + # 4. int - metric ID + def is_str_int_or_adhoc(metric: Metric) -> bool: + return isinstance(metric, (str, int)) or is_adhoc_metric(metric) self.metrics = metrics and [ - x if is_str_or_adhoc(x) else x["label"] # type: ignore + x if is_str_int_or_adhoc(x) else x["label"] # type: ignore for x in metrics ] diff --git a/superset/superset_typing.py b/superset/superset_typing.py index d182e5eb490d..488d6b6b9bb4 100644 --- a/superset/superset_typing.py +++ b/superset/superset_typing.py @@ -111,8 +111,8 @@ class ResultSetColumnType(TypedDict): FilterValues: TypeAlias = FilterValue | list[FilterValue] | tuple[FilterValue] FormData: TypeAlias = dict[str, Any] Granularity: TypeAlias = str | dict[str, str | float] -Column: TypeAlias = AdhocColumn | str -Metric: TypeAlias = AdhocMetric | str +Column: TypeAlias = AdhocColumn | str | int +Metric: TypeAlias = AdhocMetric | str | int OrderBy: TypeAlias = tuple[Metric | Column, bool] diff --git a/superset/utils/core.py b/superset/utils/core.py index 9acd7e03c7f2..1308bf56f9fe 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -1232,6 +1232,9 @@ def get_column_name(column: Column, verbose_map: dict[str, Any] | None = None) - verbose_map = verbose_map or {} return verbose_map.get(column, column) + if isinstance(column, int): + return str(column) + raise ValueError("Missing label") @@ -1264,6 +1267,9 @@ def get_metric_name(metric: Metric, verbose_map: dict[str, Any] | None = None) - verbose_map = verbose_map or {} return verbose_map.get(metric, metric) + if isinstance(metric, int): + return str(metric) + raise ValueError(__("Invalid metric object: %(metric)s", metric=str(metric))) diff --git a/tests/unit_tests/charts/test_schemas.py b/tests/unit_tests/charts/test_schemas.py index 5466a0deadd9..a7b53a848a5c 100644 --- a/tests/unit_tests/charts/test_schemas.py +++ b/tests/unit_tests/charts/test_schemas.py @@ -152,3 +152,58 @@ def test_time_grain_validation_with_config_addons(app_context: None) -> None: } result = schema.load(custom_data) assert result["time_grain"] == "PT10M" + + +def test_chart_data_query_object_schema_orderby_validation( + app_context: None, +) -> None: + """Test that orderby field handles strings, integers and objects""" + schema = ChartDataQueryObjectSchema() + + # Valid values should pass + for orderby_value in [ + "column_name", + 123, + {"label": "my_metric", "sqlExpression": "SUM(col)"}, + ]: + result = schema.load( + { + "datasource": {"type": "table", "id": 1}, + "metrics": ["count"], + "orderby": [[orderby_value, False]], + } + ) + assert result["orderby"][0][1] is False + + # None and empty string should fail + for invalid_value in [None, ""]: + with pytest.raises(ValidationError): + schema.load( + { + "datasource": {"type": "table", "id": 1}, + "metrics": ["count"], + "orderby": [[invalid_value, True]], + } + ) + + +def test_chart_data_query_object_schema_metrics_validation( + app_context: None, +) -> None: + """Test that metrics field handles strings, integers and objects""" + schema = ChartDataQueryObjectSchema() + + # Mix of different types should all pass + result = schema.load( + { + "datasource": {"type": "table", "id": 1}, + "metrics": [ + "count", + 123, + {"expressionType": "SQL", "sqlExpression": "SUM(col)", "label": "sum"}, + ], + } + ) + assert result["metrics"][0] == "count" + assert result["metrics"][1] == 123 + assert result["metrics"][2]["label"] == "sum" diff --git a/tests/unit_tests/core_tests.py b/tests/unit_tests/core_tests.py index acd0501d82fa..cfef7868796b 100644 --- a/tests/unit_tests/core_tests.py +++ b/tests/unit_tests/core_tests.py @@ -111,8 +111,6 @@ def test_get_metric_name_invalid_metric(): with pytest.raises(ValueError): # noqa: PT011 get_metric_name(None) - with pytest.raises(ValueError): # noqa: PT011 - get_metric_name(0) with pytest.raises(ValueError): # noqa: PT011 get_metric_name({}) diff --git a/tests/unit_tests/utils/test_core.py b/tests/unit_tests/utils/test_core.py index 37b8ed1877a1..36650e36ef8a 100644 --- a/tests/unit_tests/utils/test_core.py +++ b/tests/unit_tests/utils/test_core.py @@ -1162,3 +1162,35 @@ def test_sanitize_url_blocks_dangerous(): """Test that dangerous URL schemes are blocked.""" assert sanitize_url("javascript:alert('xss')") == "" assert sanitize_url("data:text/html,") == "" + + +def test_get_metric_name_with_integers() -> None: + """Test get_metric_name with integer IDs""" + from superset.utils.core import get_metric_name + + assert get_metric_name("count") == "count" + assert get_metric_name(123) == "123" + assert get_metric_name(456) == "456" + + from superset.superset_typing import AdhocMetric + + adhoc_metric: AdhocMetric = { + "expressionType": "SQL", + "sqlExpression": "SUM(amount)", + "label": "total_amount", + } + assert get_metric_name(adhoc_metric) == "total_amount" + + +def test_get_column_name_with_integers() -> None: + """Test get_column_name with integer IDs""" + from superset.utils.core import get_column_name + + assert get_column_name("age") == "age" + assert get_column_name(123) == "123" + assert get_column_name(789) == "789" + + from superset.superset_typing import AdhocColumn + + column_dict: AdhocColumn = {"label": "customer_name", "sqlExpression": "name"} + assert get_column_name(column_dict) == "customer_name"