This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch fix/isdigit-unicode-sibling-instances in repository https://gitbox.apache.org/repos/asf/superset.git
commit d10e9a258e3ec2165921ff4459c19a094006335d Author: Evan Rusackas <[email protected]> AuthorDate: Thu Sep 24 13:46:35 2026 -0700 fix: guard the isdigit()-then-int() pattern against non-decimal digits #44615 fixed cast_to_num() for strings like "²" or "①": str.isdigit() is true for Unicode "digit" characters (category No), but int() only accepts "decimal" characters (category Nd), so isdigit()-then-int() raises ValueError on them. The identical pattern repeats across DAO and mcp_service code resolving a string identifier to a numeric ID: isdigit() says "looks numeric", then int() crashes. Swapped isdigit() -> isdecimal() at each unguarded call site: - superset/daos/base.py, daos/datasource.py, models/slice.py: shared id-or-uuid resolution used across every DAO and Slice lookup. - superset/utils/date_parser.py: ordinal parsing in handle_nth_of(). - superset/mcp_service/chart/{chart_helpers,chart_utils}.py and the chart/dashboard/explore mcp_service tool modules (generate_chart, get_chart_sql, update_chart_preview, dataset_validator, delete_dashboard, generate_explore_link): dataset_id/identifier resolution on MCP tool entry points, directly reachable from external LLM-client input. Left three other isdigit() call sites alone: one already wraps int() in try/except (commands/dashboard/export.py), one is a heuristic with no int() call (get_chart_preview.py's form-data-key guess), and one is a SQL/KQL tokenizer regex branch (sql/parse.py) that's a different risk shape and deserves its own review rather than a mechanical swap. Added regression tests for the three most widely-shared call sites (BaseDAO.find_by_id_or_uuid, DatasourceDAO.get_datasource, Slice's id_or_uuid_filter) confirming a non-decimal digit string falls through to the uuid branch instead of crashing. Co-Authored-By: Evan Rusackas <[email protected]> Co-Authored-By: Claude Sonnet 5 <[email protected]> --- superset/daos/base.py | 2 +- superset/daos/datasource.py | 2 +- superset/mcp_service/chart/chart_helpers.py | 4 +-- superset/mcp_service/chart/chart_utils.py | 2 +- superset/mcp_service/chart/tool/generate_chart.py | 7 +++--- superset/mcp_service/chart/tool/get_chart_sql.py | 4 +-- .../mcp_service/chart/tool/update_chart_preview.py | 4 +-- .../chart/validation/dataset_validator.py | 2 +- .../mcp_service/dashboard/tool/delete_dashboard.py | 2 +- .../explore/tool/generate_explore_link.py | 2 +- superset/models/slice.py | 2 +- superset/utils/date_parser.py | 2 +- tests/unit_tests/dao/base_dao_test.py | 29 +++++++++++++++++++++- tests/unit_tests/datasource/dao_tests.py | 16 ++++++++++++ tests/unit_tests/models/slice_test.py | 4 +++ 15 files changed, 66 insertions(+), 18 deletions(-) diff --git a/superset/daos/base.py b/superset/daos/base.py index 46528206819..c3966c93dc1 100644 --- a/superset/daos/base.py +++ b/superset/daos/base.py @@ -265,7 +265,7 @@ class BaseDAO(CoreBaseDAO[T], Generic[T]): id_column = getattr(cls.model_cls, cls.id_column_name) uuid_column = getattr(cls.model_cls, cls.uuid_column_name) - if model_id_or_uuid.isdigit(): + if model_id_or_uuid.isdecimal(): filter = id_column == int(model_id_or_uuid) else: filter = uuid_column == model_id_or_uuid diff --git a/superset/daos/datasource.py b/superset/daos/datasource.py index 93c52c88e25..eb3832e8979 100644 --- a/superset/daos/datasource.py +++ b/superset/daos/datasource.py @@ -65,7 +65,7 @@ class DatasourceDAO(BaseDAO[Datasource]): model = cls.sources[datasource_type] - if str(database_id_or_uuid).isdigit(): + if str(database_id_or_uuid).isdecimal(): filter = model.id == int(database_id_or_uuid) else: try: diff --git a/superset/mcp_service/chart/chart_helpers.py b/superset/mcp_service/chart/chart_helpers.py index f191697e72c..47e6927cb51 100644 --- a/superset/mcp_service/chart/chart_helpers.py +++ b/superset/mcp_service/chart/chart_helpers.py @@ -151,7 +151,7 @@ def find_chart_by_identifier( {"query_options": query_options} if query_options is not None else {} ) if isinstance(identifier, int) or ( - isinstance(identifier, str) and identifier.isdigit() + isinstance(identifier, str) and identifier.isdecimal() ): chart_id = int(identifier) if isinstance(identifier, str) else identifier return ChartDAO.find_by_id(chart_id, **extra) @@ -958,7 +958,7 @@ def resolve_form_data_datasource( if not datasource_id and (combined := form_data.get("datasource")): if isinstance(combined, str) and "__" in combined: parts = combined.split("__", 1) - datasource_id = int(parts[0]) if parts[0].isdigit() else parts[0] + datasource_id = int(parts[0]) if parts[0].isdecimal() else parts[0] datasource_type = parts[1] if len(parts) > 1 else None if not datasource_id and chart: diff --git a/superset/mcp_service/chart/chart_utils.py b/superset/mcp_service/chart/chart_utils.py index adf06de1acc..5bbf38c21eb 100644 --- a/superset/mcp_service/chart/chart_utils.py +++ b/superset/mcp_service/chart/chart_utils.py @@ -217,7 +217,7 @@ def generate_explore_link( try: if isinstance(dataset_id, int) or ( - isinstance(dataset_id, str) and dataset_id.isdigit() + isinstance(dataset_id, str) and dataset_id.isdecimal() ): numeric_dataset_id = ( int(dataset_id) if isinstance(dataset_id, str) else dataset_id diff --git a/superset/mcp_service/chart/tool/generate_chart.py b/superset/mcp_service/chart/tool/generate_chart.py index 8f5aff64cf2..2e9948d5a46 100644 --- a/superset/mcp_service/chart/tool/generate_chart.py +++ b/superset/mcp_service/chart/tool/generate_chart.py @@ -344,7 +344,8 @@ async def generate_chart( # noqa: C901 with event_logger.log_context(action="mcp.generate_chart.dataset_lookup"): dataset = None if isinstance(request.dataset_id, int) or ( - isinstance(request.dataset_id, str) and request.dataset_id.isdigit() + isinstance(request.dataset_id, str) + and request.dataset_id.isdecimal() ): dataset_id = ( int(request.dataset_id) @@ -618,7 +619,7 @@ async def generate_chart( # noqa: C901 from superset.daos.dataset import DatasetDAO if isinstance(request.dataset_id, int) or ( - isinstance(request.dataset_id, str) and request.dataset_id.isdigit() + isinstance(request.dataset_id, str) and request.dataset_id.isdecimal() ): candidate_id = ( int(request.dataset_id) @@ -748,7 +749,7 @@ async def generate_chart( # noqa: C901 # Convert dataset_id to int only if numeric if ( isinstance(request.dataset_id, str) - and request.dataset_id.isdigit() + and request.dataset_id.isdecimal() ): dataset_id_for_preview = int(request.dataset_id) elif isinstance(request.dataset_id, int): diff --git a/superset/mcp_service/chart/tool/get_chart_sql.py b/superset/mcp_service/chart/tool/get_chart_sql.py index 82cec075c41..d441b3a02df 100644 --- a/superset/mcp_service/chart/tool/get_chart_sql.py +++ b/superset/mcp_service/chart/tool/get_chart_sql.py @@ -125,7 +125,7 @@ def _find_chart_by_identifier( from superset.daos.chart import ChartDAO if isinstance(identifier, int) or ( - isinstance(identifier, str) and identifier.isdigit() + isinstance(identifier, str) and identifier.isdecimal() ): chart_id = int(identifier) if isinstance(identifier, str) else identifier return ChartDAO.find_by_id(chart_id) @@ -275,7 +275,7 @@ def _resolve_datasource_name( if not datasource_id and (combined := form_data.get("datasource")): if isinstance(combined, str) and "__" in combined: parts = combined.split("__", 1) - datasource_id = int(parts[0]) if parts[0].isdigit() else parts[0] + datasource_id = int(parts[0]) if parts[0].isdecimal() else parts[0] datasource_type = parts[1] if len(parts) > 1 else "table" if not datasource_id: diff --git a/superset/mcp_service/chart/tool/update_chart_preview.py b/superset/mcp_service/chart/tool/update_chart_preview.py index ff278144717..5b458ba26af 100644 --- a/superset/mcp_service/chart/tool/update_chart_preview.py +++ b/superset/mcp_service/chart/tool/update_chart_preview.py @@ -81,7 +81,7 @@ def _find_dataset(dataset_id: int | str) -> Any | None: from superset.mcp_service.auth import has_dataset_access if isinstance(dataset_id, int) or ( - isinstance(dataset_id, str) and dataset_id.isdigit() + isinstance(dataset_id, str) and dataset_id.isdecimal() ): dataset = DatasetDAO.find_by_id(int(dataset_id)) else: @@ -264,7 +264,7 @@ def update_chart_preview( # noqa: C901 from superset.daos.dataset import DatasetDAO if isinstance(request.dataset_id, int) or ( - isinstance(request.dataset_id, str) and request.dataset_id.isdigit() + isinstance(request.dataset_id, str) and request.dataset_id.isdecimal() ): dataset = DatasetDAO.find_by_id(int(request.dataset_id)) else: diff --git a/superset/mcp_service/chart/validation/dataset_validator.py b/superset/mcp_service/chart/validation/dataset_validator.py index d7cb7158b56..0872abe733d 100644 --- a/superset/mcp_service/chart/validation/dataset_validator.py +++ b/superset/mcp_service/chart/validation/dataset_validator.py @@ -437,7 +437,7 @@ class DatasetValidator: from superset.daos.dataset import DatasetDAO if isinstance(dataset_id, int) or ( - isinstance(dataset_id, str) and dataset_id.isdigit() + isinstance(dataset_id, str) and dataset_id.isdecimal() ): dataset = DatasetDAO.find_by_id(int(dataset_id)) else: diff --git a/superset/mcp_service/dashboard/tool/delete_dashboard.py b/superset/mcp_service/dashboard/tool/delete_dashboard.py index b078ebe7ffa..4f7754903ff 100644 --- a/superset/mcp_service/dashboard/tool/delete_dashboard.py +++ b/superset/mcp_service/dashboard/tool/delete_dashboard.py @@ -51,7 +51,7 @@ def _find_dashboard_by_identifier(identifier: int | str) -> "Dashboard | None": from superset.daos.dashboard import DashboardDAO if isinstance(identifier, int) or ( - isinstance(identifier, str) and identifier.isdigit() + isinstance(identifier, str) and identifier.isdecimal() ): return DashboardDAO.find_by_id(int(identifier)) # Try UUID, then fall back to slug. diff --git a/superset/mcp_service/explore/tool/generate_explore_link.py b/superset/mcp_service/explore/tool/generate_explore_link.py index 12c4c87f1b3..95799596514 100644 --- a/superset/mcp_service/explore/tool/generate_explore_link.py +++ b/superset/mcp_service/explore/tool/generate_explore_link.py @@ -125,7 +125,7 @@ async def generate_explore_link( with event_logger.log_context(action="mcp.generate_explore_link.dataset_check"): dataset = None if isinstance(request.dataset_id, int) or ( - isinstance(request.dataset_id, str) and request.dataset_id.isdigit() + isinstance(request.dataset_id, str) and request.dataset_id.isdecimal() ): dataset_id_int = ( int(request.dataset_id) diff --git a/superset/models/slice.py b/superset/models/slice.py index fcf81f1f4cc..4d3105b17d1 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -487,7 +487,7 @@ class Slice( # pylint: disable=too-many-public-methods def id_or_uuid_filter(id_or_uuid: str | int) -> BinaryExpression: if isinstance(id_or_uuid, int): return Slice.id == id_or_uuid - if id_or_uuid.isdigit(): + if id_or_uuid.isdecimal(): return Slice.id == int(id_or_uuid) return Slice.uuid == id_or_uuid diff --git a/superset/utils/date_parser.py b/superset/utils/date_parser.py index 19c6452c315..dcb85e75e65 100644 --- a/superset/utils/date_parser.py +++ b/superset/utils/date_parser.py @@ -349,7 +349,7 @@ def handle_nth_of( "DATETRUNC(..., year) : DATEADD(DATETRUNC(..., year), 1, week)" """ # Convert ordinal to number - n = ORDINAL_MAP.get(ordinal.lower(), int(ordinal) if ordinal.isdigit() else 1) + n = ORDINAL_MAP.get(ordinal.lower(), int(ordinal) if ordinal.isdecimal() else 1) relative_base = get_relative_base(unit, relative_start) effective_scope = scope.lower() if scope else "this" diff --git a/tests/unit_tests/dao/base_dao_test.py b/tests/unit_tests/dao/base_dao_test.py index 578bf59bb64..a04eb7eb7cc 100644 --- a/tests/unit_tests/dao/base_dao_test.py +++ b/tests/unit_tests/dao/base_dao_test.py @@ -19,7 +19,7 @@ Unit tests for BaseDAO functionality using mocks and no database operations. """ -from unittest.mock import Mock, patch +from unittest.mock import MagicMock, Mock, patch import pytest from sqlalchemy import Boolean, Column, Integer, String @@ -335,6 +335,33 @@ def test_find_by_id_or_uuid_statement_error_still_returns_none(): assert TestDAO.find_by_id_or_uuid("not-a-uuid") is None +def test_find_by_id_or_uuid_non_decimal_digit_treated_as_uuid(): + """A string like "²" (superscript 2) is Unicode "digit" but not + "decimal": str.isdigit() is True for it, yet int() rejects it. Must be + routed to the uuid branch instead of crashing on int(model_id_or_uuid).""" + + with ( + patch("superset.daos.base.db") as mock_db, + patch("superset.daos.base.getattr") as mock_getattr, + ): + mock_session = Mock() + mock_db.session = mock_session + id_column, uuid_column = MagicMock(), MagicMock() + id_filter, uuid_filter = Mock(name="id_filter"), Mock(name="uuid_filter") + id_column.__eq__.return_value = id_filter + uuid_column.__eq__.return_value = uuid_filter + mock_getattr.side_effect = [id_column, uuid_column] + + mock_query = Mock() + mock_session.query.return_value = mock_query + mock_query.filter.return_value = mock_query + mock_query.one_or_none.return_value = None + + TestDAO.find_by_id_or_uuid("²") + + mock_query.filter.assert_called_once_with(uuid_filter) + + def test_find_by_column_operational_error_propagates(): """_find_by_column catches StatementError to absorb coercion errors; an OperationalError (a StatementError subclass) must still propagate.""" diff --git a/tests/unit_tests/datasource/dao_tests.py b/tests/unit_tests/datasource/dao_tests.py index 5e106164051..783796186f7 100644 --- a/tests/unit_tests/datasource/dao_tests.py +++ b/tests/unit_tests/datasource/dao_tests.py @@ -142,6 +142,22 @@ def test_not_found_datasource(session_with_data: Session) -> None: ) +def test_get_datasource_non_decimal_digit_raises_value_incorrect( + session_with_data: Session, +) -> None: + """ "²" (superscript 2) is str.isdigit() but not str.isdecimal(), so + it must be routed to the uuid branch (and fail as an invalid uuid) rather + than crash on int(database_id_or_uuid).""" + from superset.daos.datasource import DatasourceDAO + from superset.daos.exceptions import DatasourceValueIsIncorrect + + with pytest.raises(DatasourceValueIsIncorrect): + DatasourceDAO.get_datasource( + datasource_type="table", + database_id_or_uuid="²", + ) + + def test_escape_ilike_fragment() -> None: from superset.daos.datasource import _escape_ilike_fragment diff --git a/tests/unit_tests/models/slice_test.py b/tests/unit_tests/models/slice_test.py index 128d6a165de..21990f0a8ba 100644 --- a/tests/unit_tests/models/slice_test.py +++ b/tests/unit_tests/models/slice_test.py @@ -81,6 +81,10 @@ class TestSlice: ("uuid_format", "550e8400-e29b-41d4-a716-446655440000"), ("invalid_string", "not-a-number"), ("integer_id", 123), + # str.isdigit() is True for "²" (superscript 2) but int() + # rejects it; must not raise and must fall through to the uuid + # branch instead of the int(id_or_uuid) branch. + ("non_decimal_digit", "²"), ] ) def test_id_or_uuid_filter(self, test_name, input_value):
