This is an automated email from the ASF dual-hosted git repository.
richardfogaca pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 5278deaf635 fix(metrics): normalize legacy currency strings (#37455)
5278deaf635 is described below
commit 5278deaf63536fd07f3c1007d2d393f9f076c692
Author: Richard Fogaca Nienkotter
<[email protected]>
AuthorDate: Thu Feb 19 21:25:44 2026 -0300
fix(metrics): normalize legacy currency strings (#37455)
---
.../CurrencyControl/CurrencyControl.test.tsx | 29 ++++-
.../controls/CurrencyControl/CurrencyControl.tsx | 39 +++++--
superset/connectors/sqla/models.py | 3 +-
superset/datasets/schemas.py | 22 +++-
superset/models/sql_types/__init__.py | 7 ++
superset/models/sql_types/base.py | 106 +++++++++++++++++
tests/integration_tests/datasets/api_tests.py | 2 +-
tests/unit_tests/datasets/schema_tests.py | 56 +++++++++
.../unit_tests}/models/sql_types/__init__.py | 0
.../models/sql_types/currency_type_test.py | 129 +++++++++++++++++++++
10 files changed, 377 insertions(+), 16 deletions(-)
diff --git
a/superset-frontend/src/explore/components/controls/CurrencyControl/CurrencyControl.test.tsx
b/superset-frontend/src/explore/components/controls/CurrencyControl/CurrencyControl.test.tsx
index 39a1a64ee29..a40f5750446 100644
---
a/superset-frontend/src/explore/components/controls/CurrencyControl/CurrencyControl.test.tsx
+++
b/superset-frontend/src/explore/components/controls/CurrencyControl/CurrencyControl.test.tsx
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { render } from 'spec/helpers/testing-library';
+import { render, selectOption } from 'spec/helpers/testing-library';
import { CurrencyControl } from './CurrencyControl';
test('CurrencyControl renders position and symbol selects', () => {
@@ -36,3 +36,30 @@ test('CurrencyControl renders position and symbol selects',
() => {
).toBeInTheDocument();
expect(container.querySelectorAll('.ant-select')).toHaveLength(2);
});
+
+test('CurrencyControl handles string currency value', async () => {
+ const onChange = jest.fn();
+ const { container } = render(
+ <CurrencyControl
+ onChange={onChange}
+ value='{"symbol":"USD","symbolPosition":"prefix"}'
+ />,
+ {
+ useRedux: true,
+ initialState: {
+ common: { currencies: ['USD', 'EUR'] },
+ explore: { datasource: {} },
+ },
+ },
+ );
+
+ expect(
+ container.querySelector('[data-test="currency-control-container"]'),
+ ).toBeInTheDocument();
+
+ await selectOption('Suffix', 'Currency prefix or suffix');
+ expect(onChange).toHaveBeenLastCalledWith({
+ symbol: 'USD',
+ symbolPosition: 'suffix',
+ });
+});
diff --git
a/superset-frontend/src/explore/components/controls/CurrencyControl/CurrencyControl.tsx
b/superset-frontend/src/explore/components/controls/CurrencyControl/CurrencyControl.tsx
index 8ec68904440..3da754330fd 100644
---
a/superset-frontend/src/explore/components/controls/CurrencyControl/CurrencyControl.tsx
+++
b/superset-frontend/src/explore/components/controls/CurrencyControl/CurrencyControl.tsx
@@ -29,7 +29,7 @@ import ControlHeader from '../../ControlHeader';
export interface CurrencyControlProps {
onChange: (currency: Partial<Currency>) => void;
- value?: Partial<Currency>;
+ value?: Partial<Currency> | string | null;
symbolSelectOverrideProps?: Partial<SelectProps>;
currencySelectOverrideProps?: Partial<SelectProps>;
symbolSelectAdditionalStyles?: CSSObject;
@@ -59,9 +59,12 @@ export const CURRENCY_SYMBOL_POSITION_OPTIONS = [
{ value: 'suffix', label: t('Suffix') },
];
+const isCurrencyObject = (value: unknown): value is Partial<Currency> =>
+ !!value && typeof value === 'object' && !Array.isArray(value);
+
export const CurrencyControl = ({
onChange,
- value: currency = {},
+ value: rawCurrency = {},
symbolSelectOverrideProps = {},
currencySelectOverrideProps = {},
symbolSelectAdditionalStyles,
@@ -69,6 +72,24 @@ export const CurrencyControl = ({
...props
}: CurrencyControlProps) => {
const theme = useTheme();
+ const normalizedCurrency = useMemo<Partial<Currency>>(() => {
+ if (isCurrencyObject(rawCurrency)) {
+ return rawCurrency;
+ }
+
+ if (typeof rawCurrency === 'string') {
+ try {
+ const parsed = JSON.parse(rawCurrency) as unknown;
+ if (isCurrencyObject(parsed)) {
+ return parsed;
+ }
+ } catch {
+ return {};
+ }
+ }
+
+ return {};
+ }, [rawCurrency]);
const currencies = useSelector<ViewState, string[]>(
state => state.common?.currencies,
);
@@ -155,10 +176,12 @@ export const CurrencyControl = ({
options={CURRENCY_SYMBOL_POSITION_OPTIONS}
placeholder={t('Prefix or suffix')}
onChange={(symbolPosition: string) => {
- onChange({ ...currency, symbolPosition });
+ onChange({ ...normalizedCurrency, symbolPosition });
}}
- onClear={() => onChange({ ...currency, symbolPosition: undefined })}
- value={currency?.symbolPosition}
+ onClear={() =>
+ onChange({ ...normalizedCurrency, symbolPosition: undefined })
+ }
+ value={normalizedCurrency?.symbolPosition}
allowClear
{...symbolSelectOverrideProps}
/>
@@ -167,10 +190,10 @@ export const CurrencyControl = ({
options={currenciesOptions}
placeholder={t('Currency')}
onChange={(symbol: string) => {
- onChange({ ...currency, symbol });
+ onChange({ ...normalizedCurrency, symbol });
}}
- onClear={() => onChange({ ...currency, symbol: undefined })}
- value={currency?.symbol}
+ onClear={() => onChange({ ...normalizedCurrency, symbol: undefined
})}
+ value={normalizedCurrency?.symbol}
allowClear
allowNewOptions
sortComparator={currencySortComparator}
diff --git a/superset/connectors/sqla/models.py
b/superset/connectors/sqla/models.py
index be74a199672..ddd31e78fa7 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -103,6 +103,7 @@ from superset.models.helpers import (
SQLA_QUERY_KEYS,
)
from superset.models.slice import Slice
+from superset.models.sql_types.base import CurrencyType
from superset.sql.parse import Table
from superset.superset_typing import (
AdhocColumn,
@@ -1093,7 +1094,7 @@ class SqlMetric(AuditMixinNullable, ImportExportMixin,
CertificationMixin, Model
metric_type = Column(String(32))
description = Column(utils.MediumText())
d3format = Column(String(128))
- currency = Column(JSON, nullable=True)
+ currency = Column(CurrencyType, nullable=True)
warning_text = Column(Text)
table_id = Column(Integer, ForeignKey("tables.id", ondelete="CASCADE"))
expression = Column(utils.MediumText(), nullable=False)
diff --git a/superset/datasets/schemas.py b/superset/datasets/schemas.py
index 82f1e7faa1c..1a30a6bd799 100644
--- a/superset/datasets/schemas.py
+++ b/superset/datasets/schemas.py
@@ -32,6 +32,7 @@ from marshmallow.validate import Length, OneOf
from superset import security_manager
from superset.connectors.sqla.models import SqlaTable
from superset.exceptions import SupersetMarshmallowValidationError
+from superset.models.sql_types import parse_currency_string
from superset.utils import json
get_delete_ids_schema = {"type": "array", "items": {"type": "integer"}}
@@ -97,6 +98,20 @@ class DatasetMetricCurrencyPutSchema(Schema):
symbolPosition = fields.String(validate=Length(1, 128)) # noqa: N815
+class CurrencyField(fields.Nested):
+ """
+ Nested field that tolerates legacy string payloads for currency.
+ """
+
+ def _deserialize(
+ self, value: Any, attr: str | None, data: dict[str, Any], **kwargs: Any
+ ) -> Any:
+ if isinstance(value, str):
+ value = parse_currency_string(value)
+
+ return super()._deserialize(value, attr, data, **kwargs)
+
+
class DatasetMetricsPutSchema(Schema):
id = fields.Integer()
expression = fields.String(required=True)
@@ -105,7 +120,7 @@ class DatasetMetricsPutSchema(Schema):
metric_name = fields.String(required=True, validate=Length(1, 255))
metric_type = fields.String(allow_none=True, validate=Length(1, 32))
d3format = fields.String(allow_none=True, validate=Length(1, 128))
- currency = fields.Nested(DatasetMetricCurrencyPutSchema, allow_none=True)
+ currency = CurrencyField(DatasetMetricCurrencyPutSchema, allow_none=True)
verbose_name = fields.String(allow_none=True, metadata={Length: (1, 1024)})
warning_text = fields.String(allow_none=True)
uuid = fields.UUID(allow_none=True)
@@ -276,9 +291,6 @@ class ImportV1MetricSchema(Schema):
if isinstance(data.get("extra"), str):
data["extra"] = json.loads(data["extra"])
- if isinstance(data.get("currency"), str):
- data["currency"] = json.loads(data["currency"])
-
return data
metric_name = fields.String(required=True)
@@ -287,7 +299,7 @@ class ImportV1MetricSchema(Schema):
expression = fields.String(required=True)
description = fields.String(allow_none=True)
d3format = fields.String(allow_none=True)
- currency = fields.Nested(ImportMetricCurrencySchema, allow_none=True)
+ currency = CurrencyField(ImportMetricCurrencySchema, allow_none=True)
extra = fields.Dict(allow_none=True)
warning_text = fields.String(allow_none=True)
diff --git a/superset/models/sql_types/__init__.py
b/superset/models/sql_types/__init__.py
index 13a83393a91..05407b9f442 100644
--- a/superset/models/sql_types/__init__.py
+++ b/superset/models/sql_types/__init__.py
@@ -14,3 +14,10 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+
+from superset.models.sql_types.base import CurrencyType, parse_currency_string
+
+__all__ = [
+ "CurrencyType",
+ "parse_currency_string",
+]
diff --git a/superset/models/sql_types/base.py
b/superset/models/sql_types/base.py
new file mode 100644
index 00000000000..355d9df5115
--- /dev/null
+++ b/superset/models/sql_types/base.py
@@ -0,0 +1,106 @@
+# 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.
+"""Base SQL types for Superset models."""
+
+from __future__ import annotations
+
+# pylint: disable=abstract-method
+import ast
+from typing import Any
+
+from sqlalchemy.engine.interfaces import Dialect
+from sqlalchemy.types import JSON, TypeDecorator
+
+from superset.utils import json
+
+
+def _try_parse_currency(value: str) -> Any | None:
+ try:
+ return json.loads(value)
+ except (json.JSONDecodeError, TypeError):
+ pass
+
+ try:
+ return ast.literal_eval(value)
+ except (ValueError, SyntaxError):
+ return None
+
+
+def parse_currency_string(value: str) -> dict[str, Any]:
+ if not value:
+ return {}
+
+ current: Any = value
+ for _ in range(2):
+ if isinstance(current, dict):
+ return current
+ if not isinstance(current, str):
+ return {}
+
+ parsed = _try_parse_currency(current)
+ if parsed is None:
+ return {}
+ if isinstance(parsed, str):
+ current = parsed
+ continue
+ if isinstance(parsed, dict):
+ return parsed
+ return {}
+
+ return {}
+
+
+class CurrencyType(TypeDecorator):
+ """
+ Custom SQLAlchemy type for metric currency that ensures string values
+ are parsed to dicts when read from the database.
+
+ This handles legacy data that was stored as stringified JSON before
+ the currency column was migrated from VARCHAR to JSON type. Some data
+ may have been double-encoded or stored with Python dict formatting.
+
+ Example problematic values this handles:
+ - '{"symbol": "USD", "symbolPosition": "prefix"}' (JSON string)
+ - "{'symbol': 'EUR', 'symbolPosition': 'suffix'}" (Python dict string)
+ """
+
+ impl = JSON
+ cache_ok = True
+
+ def process_result_value(
+ self, value: Any, dialect: Dialect
+ ) -> dict[str, Any] | None:
+ """
+ Process value when reading from database.
+
+ Ensures the returned value is always a dict (or None), even if
+ the stored value is a string representation of a dict.
+
+ Args:
+ value: The value from the database (could be None, dict, or string)
+ dialect: The SQLAlchemy dialect being used
+
+ Returns:
+ A dict representing the currency configuration, or None
+ """
+ if value is None:
+ return None
+
+ if isinstance(value, str):
+ return parse_currency_string(value)
+
+ return value
diff --git a/tests/integration_tests/datasets/api_tests.py
b/tests/integration_tests/datasets/api_tests.py
index 6677f19f8ad..c0692fe9741 100644
--- a/tests/integration_tests/datasets/api_tests.py
+++ b/tests/integration_tests/datasets/api_tests.py
@@ -1510,7 +1510,7 @@ class TestDatasetApi(SupersetTestCase):
{
"metric_name": "test",
"expression": "COUNT(*)",
- "currency": '{"symbol": "USD", "symbolPosition":
"suffix"}',
+ "currency": {"symbol": "", "symbolPosition": ""},
},
]
}
diff --git a/tests/unit_tests/datasets/schema_tests.py
b/tests/unit_tests/datasets/schema_tests.py
index 280a2440c2d..40d63f3af8b 100644
--- a/tests/unit_tests/datasets/schema_tests.py
+++ b/tests/unit_tests/datasets/schema_tests.py
@@ -75,3 +75,59 @@ def test_dataset_put_schema_currency_code_column_optional()
-> None:
"currency_code_column" not in result
or result.get("currency_code_column") is None
)
+
+
+def test_dataset_metrics_put_schema_parses_currency_string() -> None:
+ """Test that DatasetMetricsPutSchema parses string currency payloads."""
+ from superset.datasets.schemas import DatasetMetricsPutSchema
+
+ schema = DatasetMetricsPutSchema()
+ data = {
+ "expression": "SUM(amount)",
+ "metric_name": "sum_amount",
+ "currency": '{"symbol": "EUR", "symbolPosition": "suffix"}',
+ }
+ result = schema.load(data)
+ assert result["currency"] == {"symbol": "EUR", "symbolPosition": "suffix"}
+
+
+def test_dataset_metrics_put_schema_parses_python_dict_string() -> None:
+ """Test that DatasetMetricsPutSchema parses Python dict currency
strings."""
+ from superset.datasets.schemas import DatasetMetricsPutSchema
+
+ schema = DatasetMetricsPutSchema()
+ data = {
+ "expression": "SUM(amount)",
+ "metric_name": "sum_amount",
+ "currency": "{'symbol': 'GBP', 'symbolPosition': 'prefix'}",
+ }
+ result = schema.load(data)
+ assert result["currency"] == {"symbol": "GBP", "symbolPosition": "prefix"}
+
+
+def test_dataset_metrics_put_schema_handles_malformed_currency() -> None:
+ """Test that DatasetMetricsPutSchema normalizes malformed currency
strings."""
+ from superset.datasets.schemas import DatasetMetricsPutSchema
+
+ schema = DatasetMetricsPutSchema()
+ data = {
+ "expression": "SUM(amount)",
+ "metric_name": "sum_amount",
+ "currency": "not valid json",
+ }
+ result = schema.load(data)
+ assert result["currency"] == {}
+
+
+def test_import_v1_metric_schema_parses_currency_string() -> None:
+ """Test that ImportV1MetricSchema parses string currency payloads."""
+ from superset.datasets.schemas import ImportV1MetricSchema
+
+ schema = ImportV1MetricSchema()
+ data = {
+ "metric_name": "sum_amount",
+ "expression": "SUM(amount)",
+ "currency": '{"symbol": "CAD", "symbolPosition": "suffix"}',
+ }
+ result = schema.load(data)
+ assert result["currency"] == {"symbol": "CAD", "symbolPosition": "suffix"}
diff --git a/superset/models/sql_types/__init__.py
b/tests/unit_tests/models/sql_types/__init__.py
similarity index 100%
copy from superset/models/sql_types/__init__.py
copy to tests/unit_tests/models/sql_types/__init__.py
diff --git a/tests/unit_tests/models/sql_types/currency_type_test.py
b/tests/unit_tests/models/sql_types/currency_type_test.py
new file mode 100644
index 00000000000..58f94d3b850
--- /dev/null
+++ b/tests/unit_tests/models/sql_types/currency_type_test.py
@@ -0,0 +1,129 @@
+# 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.
+"""Tests for CurrencyType - a custom SQLAlchemy type for metric currency."""
+
+from unittest.mock import MagicMock
+
+import pytest
+
+from superset.models.sql_types.base import CurrencyType
+
+
[email protected]
+def currency_type() -> CurrencyType:
+ """Create a CurrencyType instance for testing."""
+ return CurrencyType()
+
+
[email protected]
+def mock_dialect() -> MagicMock:
+ """Create a mock SQLAlchemy dialect."""
+ return MagicMock()
+
+
+def test_process_result_value_with_none(
+ currency_type: CurrencyType, mock_dialect: MagicMock
+) -> None:
+ """Test that None values are returned as-is."""
+ result = currency_type.process_result_value(None, mock_dialect)
+ assert result is None
+
+
+def test_process_result_value_with_dict(
+ currency_type: CurrencyType, mock_dialect: MagicMock
+) -> None:
+ """Test that dict values are returned as-is."""
+ value = {"symbol": "USD", "symbolPosition": "prefix"}
+ result = currency_type.process_result_value(value, mock_dialect)
+ assert result == {"symbol": "USD", "symbolPosition": "prefix"}
+
+
+def test_process_result_value_with_empty_dict(
+ currency_type: CurrencyType, mock_dialect: MagicMock
+) -> None:
+ """Test that empty dict values are returned as-is."""
+ result = currency_type.process_result_value({}, mock_dialect)
+ assert result == {}
+
+
+def test_process_result_value_with_json_string(
+ currency_type: CurrencyType, mock_dialect: MagicMock
+) -> None:
+ """Test that JSON string values are parsed to dict."""
+ value = '{"symbol": "EUR", "symbolPosition": "suffix"}'
+ result = currency_type.process_result_value(value, mock_dialect)
+ assert result == {"symbol": "EUR", "symbolPosition": "suffix"}
+
+
+def test_process_result_value_with_python_dict_string(
+ currency_type: CurrencyType, mock_dialect: MagicMock
+) -> None:
+ """Test that Python dict string (single quotes) values are parsed to
dict."""
+ value = "{'symbol': 'GBP', 'symbolPosition': 'prefix'}"
+ result = currency_type.process_result_value(value, mock_dialect)
+ assert result == {"symbol": "GBP", "symbolPosition": "prefix"}
+
+
+def test_process_result_value_with_double_encoded_json_string(
+ currency_type: CurrencyType, mock_dialect: MagicMock
+) -> None:
+ """Test that double-encoded JSON strings are parsed to dict."""
+ value = '"{\\"symbol\\": \\"EUR\\", \\"symbolPosition\\": \\"suffix\\"}"'
+ result = currency_type.process_result_value(value, mock_dialect)
+ assert result == {"symbol": "EUR", "symbolPosition": "suffix"}
+
+
+def test_process_result_value_with_malformed_string(
+ currency_type: CurrencyType, mock_dialect: MagicMock
+) -> None:
+ """Test that malformed string values return empty dict."""
+ value = "not valid json at all"
+ result = currency_type.process_result_value(value, mock_dialect)
+ assert result == {}
+
+
+def test_process_result_value_with_empty_string(
+ currency_type: CurrencyType, mock_dialect: MagicMock
+) -> None:
+ """Test that empty string values return empty dict."""
+ result = currency_type.process_result_value("", mock_dialect)
+ assert result == {}
+
+
+def test_process_result_value_with_partial_json_string(
+ currency_type: CurrencyType, mock_dialect: MagicMock
+) -> None:
+ """Test that partial JSON string (only symbol) is parsed correctly."""
+ value = '{"symbol": "JPY"}'
+ result = currency_type.process_result_value(value, mock_dialect)
+ assert result == {"symbol": "JPY"}
+
+
+def test_cache_ok_is_true(currency_type: CurrencyType) -> None:
+ """Test that cache_ok is True for SQLAlchemy compatibility."""
+ assert currency_type.cache_ok is True
+
+
+def test_impl_is_json(currency_type: CurrencyType) -> None:
+ """Test that the underlying implementation is JSON type."""
+ from sqlalchemy.types import JSON
+
+ impl = currency_type.impl
+ if isinstance(impl, type):
+ assert issubclass(impl, JSON)
+ else:
+ assert isinstance(impl, JSON)
