codeant-ai-for-open-source[bot] commented on code in PR #44148: URL: https://github.com/apache/superset/pull/44148#discussion_r4044691011
########## tests/unit_tests/mcp_service/chart/test_geographic_chart.py: ########## @@ -0,0 +1,1059 @@ +# 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. + +"""Typed geographic contracts, native query semantics, and boundary parity.""" + +from copy import deepcopy +from decimal import Decimal +from pathlib import Path +from typing import Any +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client +from pydantic import TypeAdapter, ValidationError +from sqlalchemy.exc import SQLAlchemyError + +from superset.mcp_service.app import mcp +from superset.mcp_service.chart.chart_helpers import build_query_dicts_from_form_data +from superset.mcp_service.chart.chart_utils import ( + map_config_to_form_data, + merge_chart_form_data, +) +from superset.mcp_service.chart.compile import _compile_chart +from superset.mcp_service.chart.preview_utils import ( + _generate_ascii_preview_from_data, + _generate_vega_lite_preview_from_data, +) +from superset.mcp_service.chart.query_result import normalize_chart_query_result +from superset.mcp_service.chart.schemas import ( + ChartConfig, + ChartError, + GenerateChartRequest, + GenerateExploreLinkRequest, + UpdateChartRequest, +) +from superset.mcp_service.chart.tool.get_chart_type_schema import ( + _CHART_EXAMPLES, + _get_chart_type_schema_impl, +) +from superset.utils import json +from superset.utils.geographic import resolve_geographic_value, resolve_region +from superset.utils.geographic_regions import REGIONS + +KINDS = ("country_map", "world_map", "deck_scatter") +# Reuse the compiled union schema; each validation still creates a fresh config. +CHART_CONFIG_ADAPTER = TypeAdapter(ChartConfig) + + +def config_for(kind: str) -> Any: + """Parse the published example rather than duplicating a private contract.""" + return CHART_CONFIG_ADAPTER.validate_python(_CHART_EXAMPLES[kind][0]) + + +def form_for(kind: str) -> dict[str, Any]: + """Map the same config consumed by the three public tools.""" + return map_config_to_form_data(config_for(kind)) + + +def result_for(kind: str) -> dict[str, Any]: + """Native query results before frontend display transforms.""" + row = ( + {"state": "CA", "SUM(sales)": 10} + if kind == "country_map" + else {"country": "US", "SUM(sales)": 10} + if kind == "world_map" + else {"latitude": 37.8, "longitude": -122.4} + ) + return {"queries": [{"data": [row]}]} + + +def invalid_result_for(kind: str) -> dict[str, Any]: + """Keep valid metrics while failing the actual geographic value contract.""" + result = result_for(kind) + row = result["queries"][0]["data"][0] + if kind == "country_map": + row["state"] = "BC" + elif kind == "world_map": + row["country"] = "not-a-country" + else: + row["latitude"] = 91 + return result + + [email protected]("kind", KINDS) +def test_geographic_example_configs_are_independent(kind: str) -> None: + """Sharing a compiled schema must not share mutable config instances.""" + first = config_for(kind) + second = config_for(kind) + assert first is not second + first.row_limit = 1 + assert second.row_limit == 10000 + assert config_for(kind).row_limit == 10000 + + [email protected]("kind", KINDS) +def test_geographic_schema_examples_and_all_request_unions(kind: str) -> None: + """Each entry point uses the required, bounded shared discriminator.""" + example = _CHART_EXAMPLES[kind][0] + schema = _get_chart_type_schema_impl(kind)["schema"] + assert "chart_type" in schema["required"] + assert schema["additionalProperties"] is False + assert schema["properties"]["row_limit"]["maximum"] == 10000 + for model, identity in ( + (GenerateChartRequest, {"dataset_id": 3}), + (GenerateExploreLinkRequest, {"dataset_id": 3}), + (UpdateChartRequest, {"identifier": 1}), + ): + assert ( + model.model_validate({**identity, "config": example}).config.chart_type + == kind + ) + for patch_ in ( + {"row_limit": 10001}, + {"row_limit": True}, + {"row_limit": "100"}, + {"bogus": 1}, + ): + with pytest.raises(ValidationError): + CHART_CONFIG_ADAPTER.validate_python({**example, **patch_}) + with pytest.raises(ValidationError): + CHART_CONFIG_ADAPTER.validate_python( + {k: v for k, v in example.items() if k != "chart_type"} + ) + + [email protected]( + "country,value,format_,expected", + [ + ("usa", "CA", "abbreviation", "US-CA"), + ("usa", "ca", "abbreviation", "US-CA"), + ("usa", "California", "name", "US-CA"), + ("usa", "us-ca", "iso_3166_2", "US-CA"), + ("canada", "BC", "abbreviation", "CA-BC"), + ("australia", "Victoria", "name", "AU-VIC"), + ("australia", "NSW", "abbreviation", "AU-NSW"), + ("australia", "Queensland", "name", "AU-QLD"), + ("japan", "Tokyo", "name", "JP-13"), + ("japan", "Osaka", "name", "JP-27"), + ("uk", "Isle of Wight", "name", "GB-IOW"), + ], +) +def test_region_resolution( + country: str, value: str, format_: str, expected: str +) -> None: + """All formats resolve only to identifiers present in the chosen geometry.""" + assert resolve_region(value, country, format_) == expected + + [email protected]( + "value", + [ + "BC", + "Victoria", + "NSW", + "Queensland", + "Tokyo", + "Osaka", + "Isle of Wight", + None, + 1, + "", + "CA ", + ], +) +def test_us_rejects_non_us_and_malformed_values(value: object) -> None: + """Cross-country values never silently disappear from a map.""" + with pytest.raises(ValueError, match="country=usa"): + resolve_region(value, "usa", "abbreviation") + + +def test_exact_first_and_ambiguous_folded_names() -> None: + """Do not let a case-insensitive dictionary overwrite distinct names.""" + pairs = [("Region", "A"), ("REGION", "B")] + assert resolve_geographic_value("Region", pairs) == "A" + with pytest.raises(ValueError, match="ambiguous"): + resolve_geographic_value("region", pairs) + with pytest.raises(ValueError, match="ambiguous"): + resolve_geographic_value("Region", pairs + [("Region", "C")]) + + [email protected]("country", REGIONS) +def test_region_data_matches_frontend_geometry(country: str) -> None: + """Updating geometry requires updating its bounded backend lookup too.""" + root = Path(__file__).resolve().parents[4] + path = ( + root + / "superset-frontend/plugins/plugin-chart-country-map/src/countries" + / f"{country}.geojson" + ) + expected = sorted( + { + ( + f["properties"]["ISO"], + f["properties"].get("NAME_2") or f["properties"]["NAME_1"], + ) + for f in json.loads(path.read_text())["features"] + } + ) + assert REGIONS[country] == expected Review Comment: **Suggestion:** The expected pairs are sorted, but `REGIONS[country]` preserves GeoJSON order, so the parity test fails for UK before checking contents. **Assessment:** ๐ `Major` ยท ๐ `Occurrence: Often` ยท ๐ท๏ธ `Logic error` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=70a7955d58f44cd6b73658313f97766d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=70a7955d58f44cd6b73658313f97766d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <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/chart/test_geographic_chart.py **Line:** 204:213 **Comment:** *Logic Error: The expected pairs are sorted, but `REGIONS[country]` preserves GeoJSON order, so the parity test fails for UK before checking contents. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44148&comment_hash=83867e09ed986828151fb3f5ff339ab6664d67900f9b9698dce4d8d1b3e3bbe3&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44148&comment_hash=83867e09ed986828151fb3f5ff339ab6664d67900f9b9698dce4d8d1b3e3bbe3&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]
