aminghadersohi commented on code in PR #36933:
URL: https://github.com/apache/superset/pull/36933#discussion_r2932169790
##########
tests/unit_tests/extensions/test_types.py:
##########
@@ -61,6 +61,18 @@ def test_extension_config_full():
"description": "A query insights extension",
"dependencies": ["other-extension"],
"permissions": ["can_read", "can_view"],
+ "frontend": {
+ "contributions": {
+ "views": {
+ "sqllab": {
+ "panels": [
+ {"id": "query_insights.main", "name": "Query
Insights"}
+ ]
+ }
+ }
+ },
+ "moduleFederation": {"exposes": ["./index"]},
+ },
Review Comment:
This is a false positive. The test `test_extension_config_full` is testing
the `ExtensionConfig` model, which does **not** have a `frontend` field — only
`backend`. Looking at the Pydantic model in
`superset-core/src/superset_core/extensions/types.py`:
```python
class ExtensionConfig(BaseExtension):
backend: ExtensionConfigBackend | None = Field(default=None, ...)
```
The `frontend` key in the test input data (line 64) is simply ignored by
Pydantic's `model_validate()` since `ExtensionConfig` doesn't define that
field. The test correctly asserts on fields that `ExtensionConfig` actually has
(`publisher`, `name`, `version`, `backend`, etc.). Frontend assertions exist in
the separate `test_manifest_with_frontend` test (line 192), which tests the
`Manifest` model that does have a `frontend` field.
The `frontend` data in this test input is intentionally there to verify that
extra fields don't cause validation errors — it's testing robustness, not
frontend parsing.
##########
superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py:
##########
@@ -0,0 +1,286 @@
+# 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.
+"""
+MCP tool: get_embeddable_chart
+
+Creates an embeddable chart iframe with guest token authentication.
+This enables AI agents to generate charts that can be displayed in
+external applications via iframe.
+"""
+
+import logging
+from datetime import datetime, timedelta, timezone
+
+from fastmcp import Context
+from flask import g
+from superset_core.mcp.decorators import tool
+
+from superset.commands.exceptions import CommandException
+from superset.mcp_service.chart.chart_utils import (
+ map_config_to_form_data,
+ resolve_dataset,
+)
+from superset.mcp_service.embedded_chart.schemas import (
+ GetEmbeddableChartRequest,
+ GetEmbeddableChartResponse,
+)
+from superset.mcp_service.utils.schema_utils import parse_request
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.security.guest_token import (
+ GuestTokenResource,
Review Comment:
The circular import concern here is incorrect.
`superset.security.guest_token` is a leaf module with no imports from the
`mcp_service` package, so there is no circular import risk.
These TypedDicts (`GuestTokenResource`, `GuestTokenRlsRule`,
`GuestTokenUser`) are used at runtime to construct dict literals with type
annotations (lines 210, 216, 224):
```python
guest_user: GuestTokenUser = {
"username": f"mcp_embed_{username}",
...
}
resources: list[GuestTokenResource] = [
{"type": GuestTokenResourceType.CHART_PERMALINK, "id": permalink_key}
]
rls_rules: list[GuestTokenRlsRule] = [...]
```
While local variable annotations aren't evaluated at runtime in Python
3.10+, these TypedDicts serve as documentation and enable static type checking
on the dict structure. Moving them to `TYPE_CHECKING` would save a negligible
import overhead for a module that has no circular dependency issue. This is
consistent with how the rest of the Superset codebase imports these types
(e.g., `superset/embedded_chart/api.py` lines 42-46 import the same types at
the module level).
--
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]
