aminghadersohi commented on code in PR #41924:
URL: https://github.com/apache/superset/pull/41924#discussion_r3573706315
##########
tests/unit_tests/mcp_service/database/tool/test_database_tools.py:
##########
@@ -378,3 +379,69 @@ async def
test_list_databases_does_not_expose_sensitive_credential_columns(
# Verify the exploit path: DAO must never receive sensitive column names.
dao_columns = mock_list.call_args.kwargs["columns"]
assert not sensitive.intersection(dao_columns)
+
+
+# ---------------------------------------------------------------------------
+# Pagination edge cases
+# ---------------------------------------------------------------------------
+
+
+class TestListDatabasesRequestPagination:
+ """Schema-level pagination boundary tests — ``page`` is PositiveInt and
+ ``page_size`` is constrained to (0, MAX_PAGE_SIZE]."""
+
+ def test_page_zero_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListDatabasesRequest(page=0)
+
+ def test_negative_page_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListDatabasesRequest(page=-1)
+
+ def test_page_size_zero_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListDatabasesRequest(page_size=0)
+
+ def test_page_size_over_max_rejected(self):
+ with pytest.raises(
+ ValidationError,
+ match=f"less than or equal to {MAX_PAGE_SIZE}",
+ ):
+ ListDatabasesRequest(page_size=MAX_PAGE_SIZE + 1)
+
+ def test_page_size_at_max_accepted(self):
+ request = ListDatabasesRequest(page_size=MAX_PAGE_SIZE)
+ assert request.page_size == MAX_PAGE_SIZE
+
+
[email protected]
+async def
test_list_databases_invalid_page_size_surfaces_as_tool_error(mcp_server):
Review Comment:
Added explicit parameter/return annotations for the added test or fixture in
8c643dd92d.
##########
tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py:
##########
@@ -272,3 +274,69 @@ async def
test_get_rls_filter_info_guest_denied(mcp_server):
data = json.loads(result.content[0].text)
assert data["error_type"] == "Forbidden"
assert "guest" in data["error"].lower()
+
+
+# ---------------------------------------------------------------------------
+# Pagination edge cases
+# ---------------------------------------------------------------------------
+
+
+class TestListRlsFiltersRequestPagination:
+ """Schema-level pagination boundary tests — ``page`` is PositiveInt and
+ ``page_size`` is constrained to (0, MAX_PAGE_SIZE]."""
+
+ def test_page_zero_rejected(self):
Review Comment:
Added explicit parameter/return annotations for the added test or fixture in
8c643dd92d.
##########
tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py:
##########
@@ -272,3 +274,69 @@ async def
test_get_rls_filter_info_guest_denied(mcp_server):
data = json.loads(result.content[0].text)
assert data["error_type"] == "Forbidden"
assert "guest" in data["error"].lower()
+
+
+# ---------------------------------------------------------------------------
+# Pagination edge cases
+# ---------------------------------------------------------------------------
+
+
+class TestListRlsFiltersRequestPagination:
+ """Schema-level pagination boundary tests — ``page`` is PositiveInt and
+ ``page_size`` is constrained to (0, MAX_PAGE_SIZE]."""
+
+ def test_page_zero_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListRlsFiltersRequest(page=0)
+
+ def test_negative_page_rejected(self):
Review Comment:
Added explicit parameter/return annotations for the added test or fixture in
8c643dd92d.
##########
tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py:
##########
@@ -272,3 +274,69 @@ async def
test_get_rls_filter_info_guest_denied(mcp_server):
data = json.loads(result.content[0].text)
assert data["error_type"] == "Forbidden"
assert "guest" in data["error"].lower()
+
+
+# ---------------------------------------------------------------------------
+# Pagination edge cases
+# ---------------------------------------------------------------------------
+
+
+class TestListRlsFiltersRequestPagination:
+ """Schema-level pagination boundary tests — ``page`` is PositiveInt and
+ ``page_size`` is constrained to (0, MAX_PAGE_SIZE]."""
+
+ def test_page_zero_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListRlsFiltersRequest(page=0)
+
+ def test_negative_page_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListRlsFiltersRequest(page=-1)
+
+ def test_page_size_zero_rejected(self):
Review Comment:
Added explicit parameter/return annotations for the added test or fixture in
8c643dd92d.
##########
tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py:
##########
@@ -272,3 +274,69 @@ async def
test_get_rls_filter_info_guest_denied(mcp_server):
data = json.loads(result.content[0].text)
assert data["error_type"] == "Forbidden"
assert "guest" in data["error"].lower()
+
+
+# ---------------------------------------------------------------------------
+# Pagination edge cases
+# ---------------------------------------------------------------------------
+
+
+class TestListRlsFiltersRequestPagination:
+ """Schema-level pagination boundary tests — ``page`` is PositiveInt and
+ ``page_size`` is constrained to (0, MAX_PAGE_SIZE]."""
+
+ def test_page_zero_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListRlsFiltersRequest(page=0)
+
+ def test_negative_page_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListRlsFiltersRequest(page=-1)
+
+ def test_page_size_zero_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListRlsFiltersRequest(page_size=0)
+
+ def test_page_size_over_max_rejected(self):
+ with pytest.raises(
+ ValidationError,
+ match=f"less than or equal to {MAX_PAGE_SIZE}",
+ ):
+ ListRlsFiltersRequest(page_size=MAX_PAGE_SIZE + 1)
+
+ def test_page_size_at_max_accepted(self):
+ request = ListRlsFiltersRequest(page_size=MAX_PAGE_SIZE)
+ assert request.page_size == MAX_PAGE_SIZE
+
+
[email protected]
+async def
test_list_rls_filters_invalid_page_size_surfaces_as_tool_error(mcp_server):
+ """page_size=0 is rejected before the tool body runs, surfacing as a
+ structured ToolError rather than a raw 500."""
+ async with Client(mcp_server) as client:
+ with pytest.raises(ToolError, match="greater than 0"):
+ await client.call_tool("list_rls_filters", {"request":
{"page_size": 0}})
Review Comment:
Added explicit parameter/return annotations for the added test or fixture in
8c643dd92d.
##########
tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py:
##########
@@ -272,3 +274,69 @@ async def
test_get_rls_filter_info_guest_denied(mcp_server):
data = json.loads(result.content[0].text)
assert data["error_type"] == "Forbidden"
assert "guest" in data["error"].lower()
+
+
+# ---------------------------------------------------------------------------
+# Pagination edge cases
+# ---------------------------------------------------------------------------
+
+
+class TestListRlsFiltersRequestPagination:
+ """Schema-level pagination boundary tests — ``page`` is PositiveInt and
+ ``page_size`` is constrained to (0, MAX_PAGE_SIZE]."""
+
+ def test_page_zero_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListRlsFiltersRequest(page=0)
+
+ def test_negative_page_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListRlsFiltersRequest(page=-1)
+
+ def test_page_size_zero_rejected(self):
+ with pytest.raises(ValidationError, match="greater than 0"):
+ ListRlsFiltersRequest(page_size=0)
+
+ def test_page_size_over_max_rejected(self):
+ with pytest.raises(
+ ValidationError,
+ match=f"less than or equal to {MAX_PAGE_SIZE}",
+ ):
+ ListRlsFiltersRequest(page_size=MAX_PAGE_SIZE + 1)
+
+ def test_page_size_at_max_accepted(self):
+ request = ListRlsFiltersRequest(page_size=MAX_PAGE_SIZE)
+ assert request.page_size == MAX_PAGE_SIZE
+
+
[email protected]
+async def
test_list_rls_filters_invalid_page_size_surfaces_as_tool_error(mcp_server):
+ """page_size=0 is rejected before the tool body runs, surfacing as a
+ structured ToolError rather than a raw 500."""
+ async with Client(mcp_server) as client:
+ with pytest.raises(ToolError, match="greater than 0"):
+ await client.call_tool("list_rls_filters", {"request":
{"page_size": 0}})
+
+
+@patch("superset.daos.security.RLSDAO.list")
[email protected]
+async def test_list_rls_filters_page_beyond_last_page_returns_empty(
+ mock_list, mcp_server
+):
Review Comment:
Added explicit parameter/return annotations for the added test or fixture in
8c643dd92d.
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_compatible_dimensions.py:
##########
@@ -282,3 +282,91 @@ async def
test_get_compatible_dimensions_external_not_found(
assert data["success"] is False
assert data["error_type"] == "NotFound"
+
+
[email protected]
+async def test_get_compatible_dimensions_builtin_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """Explicitly empty selected_metrics/selected_dimensions is not an error.
+
+ An empty selection is the natural starting state of a query builder
+ (nothing picked yet), so it must return the full groupby-enabled column
+ set rather than a validation failure.
+ """
+ mock_ds = _make_dataset(42)
Review Comment:
Added the requested explicit local type annotation in 8c643dd92d.
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_compatible_dimensions.py:
##########
@@ -282,3 +282,91 @@ async def
test_get_compatible_dimensions_external_not_found(
assert data["success"] is False
assert data["error_type"] == "NotFound"
+
+
[email protected]
+async def test_get_compatible_dimensions_builtin_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """Explicitly empty selected_metrics/selected_dimensions is not an error.
+
+ An empty selection is the natural starting state of a query builder
+ (nothing picked yet), so it must return the full groupby-enabled column
+ set rather than a validation failure.
+ """
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_compatible_dimensions",
+ {
+ "request": {
+ "dataset_id": 42,
+ "selected_metrics": [],
+ "selected_dimensions": [],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
Review Comment:
Added the requested explicit local type annotation in 8c643dd92d.
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_compatible_dimensions.py:
##########
@@ -282,3 +282,91 @@ async def
test_get_compatible_dimensions_external_not_found(
assert data["success"] is False
assert data["error_type"] == "NotFound"
+
+
[email protected]
+async def test_get_compatible_dimensions_builtin_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """Explicitly empty selected_metrics/selected_dimensions is not an error.
+
+ An empty selection is the natural starting state of a query builder
+ (nothing picked yet), so it must return the full groupby-enabled column
+ set rather than a validation failure.
+ """
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_compatible_dimensions",
+ {
+ "request": {
+ "dataset_id": 42,
+ "selected_metrics": [],
+ "selected_dimensions": [],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is True
+ names = {d["name"] for d in data["compatible_dimensions"]}
Review Comment:
Added the requested explicit local type annotation in 8c643dd92d.
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_compatible_dimensions.py:
##########
@@ -282,3 +282,91 @@ async def
test_get_compatible_dimensions_external_not_found(
assert data["success"] is False
assert data["error_type"] == "NotFound"
+
+
[email protected]
+async def test_get_compatible_dimensions_builtin_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """Explicitly empty selected_metrics/selected_dimensions is not an error.
+
+ An empty selection is the natural starting state of a query builder
+ (nothing picked yet), so it must return the full groupby-enabled column
+ set rather than a validation failure.
+ """
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_compatible_dimensions",
+ {
+ "request": {
+ "dataset_id": 42,
+ "selected_metrics": [],
+ "selected_dimensions": [],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is True
+ names = {d["name"] for d in data["compatible_dimensions"]}
+ assert names == {"region", "category"}
+
+
[email protected]
+async def test_get_compatible_dimensions_external_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """External views handle an explicitly empty selection without error."""
+ mock_view = _make_view(5)
Review Comment:
Added the requested explicit local type annotation in 8c643dd92d.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
