codeant-ai-for-open-source[bot] commented on code in PR #37967: URL: https://github.com/apache/superset/pull/37967#discussion_r2806348103
########## tests/unit_tests/mcp_service/system/tool/test_get_current_user.py: ########## @@ -0,0 +1,352 @@ +# 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 current_user in get_instance_info and created_by_fk filtering.""" + +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client +from pydantic import ValidationError + +from superset.mcp_service.app import mcp +from superset.mcp_service.chart.schemas import ChartFilter +from superset.mcp_service.dashboard.schemas import DashboardFilter +from superset.mcp_service.system.schemas import InstanceInfo, UserInfo +from superset.utils import json + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + [email protected] +def mcp_server(): + return mcp + + [email protected](autouse=True) +def mock_auth(): + """Mock authentication for all tests.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +# --------------------------------------------------------------------------- +# Helper to build a minimal InstanceInfo +# --------------------------------------------------------------------------- +def _make_instance_info(**kwargs): + """Build a minimal InstanceInfo with defaults; override with kwargs.""" + from datetime import datetime, timezone + + from superset.mcp_service.system.schemas import ( + DashboardBreakdown, + DatabaseBreakdown, + InstanceSummary, + PopularContent, + RecentActivity, + ) + + defaults = { + "instance_summary": InstanceSummary( + total_dashboards=0, + total_charts=0, + total_datasets=0, + total_databases=0, + total_users=0, + total_roles=0, + total_tags=0, + avg_charts_per_dashboard=0.0, + ), + "recent_activity": RecentActivity( + dashboards_created_last_30_days=0, + charts_created_last_30_days=0, + datasets_created_last_30_days=0, + dashboards_modified_last_7_days=0, + charts_modified_last_7_days=0, + datasets_modified_last_7_days=0, + ), + "dashboard_breakdown": DashboardBreakdown( + published=0, + unpublished=0, + certified=0, + with_charts=0, + without_charts=0, + ), + "database_breakdown": DatabaseBreakdown(by_type={}), + "popular_content": PopularContent(top_tags=[], top_creators=[]), + "timestamp": datetime.now(timezone.utc), + } + defaults.update(kwargs) + return InstanceInfo(**defaults) + + +# --------------------------------------------------------------------------- +# Schema-level tests: UserInfo +# --------------------------------------------------------------------------- + + +def test_user_info_all_fields(): + """Test UserInfo with all fields populated.""" + user = UserInfo( + id=42, + username="sophie", + first_name="Sophie", + last_name="Test", + email="[email protected]", + ) + assert user.id == 42 + assert user.username == "sophie" + assert user.first_name == "Sophie" + assert user.last_name == "Test" + assert user.email == "[email protected]" + + +def test_user_info_with_minimal_fields(): + """Test UserInfo with only required fields (all optional).""" + user = UserInfo(id=1, username="admin") + assert user.id == 1 + assert user.username == "admin" + assert user.first_name is None + assert user.last_name is None + assert user.email is None + + +def test_user_info_serialization_roundtrip(): + """Test UserInfo can be serialized to dict and back.""" + user = UserInfo(id=7, username="testuser", first_name="Test", email="[email protected]") + data = user.model_dump() + assert data["id"] == 7 + assert data["username"] == "testuser" + assert data["first_name"] == "Test" + assert data["last_name"] is None + assert data["email"] == "[email protected]" + + # Reconstruct + user2 = UserInfo(**data) + assert user2 == user + + +# --------------------------------------------------------------------------- +# Schema-level tests: InstanceInfo.current_user +# --------------------------------------------------------------------------- + + +def test_instance_info_current_user_default_none(): + """Test that InstanceInfo.current_user defaults to None.""" + info = _make_instance_info() + assert info.current_user is None + + +def test_instance_info_with_current_user(): + """Test that InstanceInfo accepts a current_user UserInfo.""" + user = UserInfo( + id=42, + username="sophie", + first_name="Sophie", + last_name="Test", + email="[email protected]", + ) + info = _make_instance_info(current_user=user) + assert info.current_user is not None + assert info.current_user.id == 42 + assert info.current_user.username == "sophie" + assert info.current_user.first_name == "Sophie" + assert info.current_user.last_name == "Test" + assert info.current_user.email == "[email protected]" + + +def test_instance_info_current_user_in_serialized_output(): + """Test current_user appears when InstanceInfo is serialized to JSON.""" + user = UserInfo(id=1, username="admin", first_name="Admin") + info = _make_instance_info(current_user=user) + data = json.loads(info.model_dump_json()) + assert "current_user" in data + assert data["current_user"]["id"] == 1 + assert data["current_user"]["username"] == "admin" + assert data["current_user"]["first_name"] == "Admin" + + +def test_instance_info_none_current_user_in_serialized_output(): + """Test current_user is null when not set in serialized output.""" + info = _make_instance_info() + data = json.loads(info.model_dump_json()) + assert "current_user" in data + assert data["current_user"] is None + + +# --------------------------------------------------------------------------- +# Tool-level tests: get_instance_info via MCP Client +# --------------------------------------------------------------------------- + + +class TestGetInstanceInfoCurrentUserViaMCP: + """Test get_instance_info tool returns current_user via MCP client.""" + + @patch( + "superset.mcp_service.system.tool.get_instance_info.InstanceInfoCore.run_tool" + ) + @pytest.mark.asyncio + async def test_get_instance_info_returns_current_user( + self, mock_run_tool, mcp_server + ): + """Test that get_instance_info populates current_user from g.user.""" + mock_run_tool.return_value = _make_instance_info() + + mock_g_user = Mock() + mock_g_user.id = 5 + mock_g_user.username = "sophie" + mock_g_user.first_name = "Sophie" + mock_g_user.last_name = "Beaumont" + mock_g_user.email = "[email protected]" + + with patch("flask.g") as mock_g: + mock_g.user = mock_g_user + + async with Client(mcp_server) as client: + result = await client.call_tool("get_instance_info", {"request": {}}) + + data = json.loads(result.content[0].text) + assert "current_user" in data + cu = data["current_user"] + assert cu["id"] == 5 + assert cu["username"] == "sophie" + assert cu["first_name"] == "Sophie" + assert cu["last_name"] == "Beaumont" + assert cu["email"] == "[email protected]" + + @patch( + "superset.mcp_service.system.tool.get_instance_info.InstanceInfoCore.run_tool" + ) + @pytest.mark.asyncio + async def test_get_instance_info_no_user_returns_null( + self, mock_run_tool, mcp_server + ): + """Test that current_user is null when g.user is not set.""" + mock_run_tool.return_value = _make_instance_info() + + with patch("flask.g") as mock_g: + # Simulate no user on g + del mock_g.user Review Comment: **Suggestion:** The test that simulates the absence of `g.user` uses `del mock_g.user` on a MagicMock, but `getattr(mock_g, "user", None)` will still return a new Mock instead of `None`, so the production code will think a user exists and try to build a `UserInfo` from a Mock, causing validation errors and making the test not actually cover the "no user" case. Replace the deletion with explicitly setting `mock_g.user = None` so `getattr(g, "user", None)` correctly returns `None` and `current_user` remains `None` as asserted. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Unit test fails with AttributeError in local and CI runs. - ⚠️ Intended "no current user" behavior remains unverified. ``` </details> ```suggestion # Simulate no user on g by explicitly setting user to None mock_g.user = None ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the specific unit test with the PR code: `pytest tests/unit_tests/mcp_service/system/tool/test_get_current_user.py::TestGetInstanceInfoCurrentUserViaMCP::test_get_instance_info_no_user_returns_null`. 2. The test enters `TestGetInstanceInfoCurrentUserViaMCP.test_get_instance_info_no_user_returns_null` at `tests/unit_tests/mcp_service/system/tool/test_get_current_user.py:239`, where `InstanceInfoCore.run_tool` is patched and `with patch("flask.g") as mock_g:` is executed at line 246. 3. At line 248, `del mock_g.user` is executed on the `MagicMock` instance returned by `patch("flask.g")`. Since `mock_g.user` has never been set on this `MagicMock`, its `__delattr__` implementation raises `AttributeError: Mock object has no attribute 'user'`. 4. The test fails with an error before the MCP `Client` call at lines 250–251 can execute, so the code path that should handle "no user on g" (the behavior this test claims to verify) is never reached. The suggested change to `mock_g.user = None` avoids the `AttributeError` and allows the test to run while still causing `getattr(g, "user", None)`-style access in the production code to return `None`. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/system/tool/test_get_current_user.py **Line:** 247:248 **Comment:** *Logic Error: The test that simulates the absence of `g.user` uses `del mock_g.user` on a MagicMock, but `getattr(mock_g, "user", None)` will still return a new Mock instead of `None`, so the production code will think a user exists and try to build a `UserInfo` from a Mock, causing validation errors and making the test not actually cover the "no user" case. Replace the deletion with explicitly setting `mock_g.user = None` so `getattr(g, "user", None)` correctly returns `None` and `current_user` remains `None` as asserted. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37967&comment_hash=21c8ed14ff4233d38314750f82c413c7743bfdd114cceab596068ea0baaaf822&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37967&comment_hash=21c8ed14ff4233d38314750f82c413c7743bfdd114cceab596068ea0baaaf822&reaction=dislike'>👎</a> -- 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]
