You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/09/24 20:47:17 UTC
(superset) 01/01: fix: guard the isdigit()-then-int() pattern against non-decimal digits
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 <ev...@rusackas.com>
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 <ev...@preset.io>
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
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):