codeant-ai-for-open-source[bot] commented on code in PR #36767:
URL: https://github.com/apache/superset/pull/36767#discussion_r2635034389
##########
superset/extensions/discovery.py:
##########
@@ -61,7 +61,7 @@ def discover_and_load_extensions(
with ZipFile(supx_file, "r") as zip_file:
files = get_bundle_files_from_zip(zip_file)
extension = get_loaded_extension(files)
- extension_id = extension.manifest["id"]
+ extension_id = extension.manifest.id
Review Comment:
**Suggestion:** Use the canonical `id` attribute on the already-constructed
`LoadedExtension` instead of reaching into the `manifest` object; directly
accessing `extension.manifest.id` is redundant and risks divergence if
`LoadedExtension.id` is the intended canonical source. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
extension_id = extension.id
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
get_loaded_extension(...) constructs a LoadedExtension with id already set
from the manifest.
Accessing extension.id is the canonical, stable place to get the identifier
and avoids
reaching into the manifest object (which is an internal detail). The current
code works,
but switching to extension.id improves clarity and resilience if
LoadedExtension later
diverges from exposing the raw manifest attribute.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/extensions/discovery.py
**Line:** 64:64
**Comment:**
*Logic Error: Use the canonical `id` attribute on the
already-constructed `LoadedExtension` instead of reaching into the `manifest`
object; directly accessing `extension.manifest.id` is redundant and risks
divergence if `LoadedExtension.id` is the intended canonical source.
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>
##########
superset/initialization/__init__.py:
##########
@@ -564,9 +564,9 @@ def init_extensions(self) -> None:
if backend_files := extension.backend:
install_in_memory_importer(backend_files)
- backend = extension.manifest.get("backend")
+ backend = extension.manifest.backend
- if backend and (entrypoints := backend.get("entryPoints")):
+ if backend and (entrypoints := backend.entryPoints):
Review Comment:
**Suggestion:** Accessing `backend.entryPoints` assumes `backend` exposes an
attribute named `entryPoints` (camelCase) and not a dict; this will raise
AttributeError for dict-backed manifests or if the attribute is named
differently. Use a safe lookup that supports both attribute and dict access and
common casing variants. [type error]
**Severity Level:** Minor ⚠️
```suggestion
entrypoints = None
if isinstance(backend, dict):
entrypoints = backend.get("entryPoints") or
backend.get("entrypoints")
else:
entrypoints = getattr(backend, "entryPoints", None) or
getattr(backend, "entrypoints", None)
if entrypoints:
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The code assumes backend is an object with a camelCase attribute. Supporting
dict-backed manifests and common casing variants is a practical, low-risk
change that prevents AttributeError and makes the code robust to different
extension manifest shapes.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/initialization/__init__.py
**Line:** 569:569
**Comment:**
*Type Error: Accessing `backend.entryPoints` assumes `backend` exposes
an attribute named `entryPoints` (camelCase) and not a dict; this will raise
AttributeError for dict-backed manifests or if the attribute is named
differently. Use a safe lookup that supports both attribute and dict access and
common casing variants.
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>
##########
tests/unit_tests/extensions/test_types.py:
##########
@@ -0,0 +1,248 @@
+# 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 extension configuration and manifest Pydantic models."""
+
+import pytest
+from pydantic import ValidationError
+from superset_core.extensions.types import (
+ ContributionConfig,
+ ExtensionConfig,
+ ExtensionConfigBackend,
+ ExtensionConfigFrontend,
+ Manifest,
+ ManifestBackend,
+ ModuleFederationConfig,
+)
+
+# =============================================================================
+# ExtensionConfig (extension.json) tests
+# =============================================================================
+
+
+def test_extension_config_minimal():
+ """Test ExtensionConfig with minimal required fields."""
+ config = ExtensionConfig.model_validate(
+ {
+ "id": "my-extension",
+ "name": "My Extension",
+ }
+ )
+ assert config.id == "my-extension"
+ assert config.name == "My Extension"
+ assert config.version == "0.0.0"
+ assert config.dependencies == []
+ assert config.permissions == []
+ assert config.frontend is None
+ assert config.backend is None
+
+
+def test_extension_config_full():
+ """Test ExtensionConfig with all fields populated."""
+ config = ExtensionConfig.model_validate(
+ {
+ "id": "query_insights",
+ "name": "Query Insights",
+ "version": "1.0.0",
+ "license": "Apache-2.0",
+ "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"]},
+ },
+ "backend": {
+ "entryPoints": ["query_insights.entrypoint"],
+ "files": ["backend/src/query_insights/**/*.py"],
+ },
+ }
+ )
+ assert config.id == "query_insights"
+ assert config.name == "Query Insights"
+ assert config.version == "1.0.0"
+ assert config.license == "Apache-2.0"
+ assert config.description == "A query insights extension"
+ assert config.dependencies == ["other-extension"]
+ assert config.permissions == ["can_read", "can_view"]
+ assert config.frontend is not None
+ assert config.frontend.moduleFederation.exposes == ["./index"]
+ assert config.backend is not None
+ assert config.backend.entryPoints == ["query_insights.entrypoint"]
+ assert config.backend.files == ["backend/src/query_insights/**/*.py"]
+
+
+def test_extension_config_missing_id():
+ """Test ExtensionConfig raises error when id is missing."""
+ with pytest.raises(ValidationError) as exc_info:
+ ExtensionConfig.model_validate({"name": "My Extension"})
+ assert "id" in str(exc_info.value)
+
+
+def test_extension_config_missing_name():
+ """Test ExtensionConfig raises error when name is missing."""
+ with pytest.raises(ValidationError) as exc_info:
+ ExtensionConfig.model_validate({"id": "my-extension"})
+ assert "name" in str(exc_info.value)
+
+
+def test_extension_config_empty_id():
+ """Test ExtensionConfig raises error when id is empty."""
+ with pytest.raises(ValidationError) as exc_info:
+ ExtensionConfig.model_validate({"id": "", "name": "My Extension"})
+ assert "id" in str(exc_info.value)
+
+
+def test_extension_config_invalid_version():
+ """Test ExtensionConfig raises error for invalid version format."""
+ with pytest.raises(ValidationError) as exc_info:
+ ExtensionConfig.model_validate(
+ {"id": "my-extension", "name": "My Extension", "version":
"invalid"}
+ )
+ assert "version" in str(exc_info.value)
+
+
+def test_extension_config_valid_versions():
+ """Test ExtensionConfig accepts valid semantic versions."""
+ for version in ["1.0.0", "0.1.0", "10.20.30", "1.0.0-beta"]:
+ config = ExtensionConfig.model_validate(
+ {"id": "my-extension", "name": "My Extension", "version": version}
+ )
+ assert config.version == version
+
+
+# =============================================================================
+# Manifest (manifest.json) tests
+# =============================================================================
+
+
+def test_manifest_minimal():
+ """Test Manifest with minimal required fields."""
+ manifest = Manifest.model_validate(
+ {
+ "id": "my-extension",
+ "name": "My Extension",
+ }
+ )
+ assert manifest.id == "my-extension"
+ assert manifest.name == "My Extension"
+ assert manifest.frontend is None
+ assert manifest.backend is None
+
+
+def test_manifest_with_frontend():
+ """Test Manifest with frontend section requires remoteEntry."""
+ manifest = Manifest.model_validate(
+ {
+ "id": "my-extension",
+ "name": "My Extension",
+ "frontend": {
+ "remoteEntry": "remoteEntry.abc123.js",
+ "contributions": {},
+ "moduleFederation": {"exposes": ["./index"]},
+ },
+ }
+ )
+ assert manifest.frontend is not None
+ assert manifest.frontend.remoteEntry == "remoteEntry.abc123.js"
+ assert manifest.frontend.moduleFederation.exposes == ["./index"]
+
+
+def test_manifest_frontend_missing_remote_entry():
+ """Test Manifest raises error when frontend is missing remoteEntry."""
+ with pytest.raises(ValidationError) as exc_info:
+ Manifest.model_validate(
+ {
+ "id": "my-extension",
+ "name": "My Extension",
+ "frontend": {"contributions": {}, "moduleFederation": {}},
+ }
+ )
+ assert "remoteEntry" in str(exc_info.value)
+
+
+def test_manifest_with_backend():
+ """Test Manifest with backend section."""
+ manifest = Manifest.model_validate(
+ {
+ "id": "my-extension",
+ "name": "My Extension",
+ "backend": {"entryPoints": ["my_extension.entrypoint"]},
+ }
+ )
+ assert manifest.backend is not None
+ assert manifest.backend.entryPoints == ["my_extension.entrypoint"]
+
+
+def test_manifest_backend_no_files_field():
+ """Test ManifestBackend does not have files field (only in
ExtensionConfig)."""
+ manifest = Manifest.model_validate(
+ {
+ "id": "my-extension",
+ "name": "My Extension",
+ "backend": {"entryPoints": ["my_extension.entrypoint"]},
+ }
+ )
+ assert not hasattr(manifest.backend, "files") or manifest.backend.files is
None
Review Comment:
**Suggestion:** Accessing a nested attribute (`manifest.backend.files`)
directly can raise an AttributeError if `manifest.backend` is None or if
`files` is missing; use a safe getter that handles both cases. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
# Use getattr to safely handle when backend is None or when `files` is
absent
assert getattr(manifest.backend, "files", None) is None
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing assertion is already safe for the test's setup (hasattr guards
access),
but switching to getattr(manifest.backend, "files", None) is a small,
clearer improvement
that preserves behavior without changing semantics. This is a stylistic
cleanup rather
than a bugfix, but it's valid and harmless.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/test_types.py
**Line:** 206:206
**Comment:**
*Null Pointer: Accessing a nested attribute (`manifest.backend.files`)
directly can raise an AttributeError if `manifest.backend` is None or if
`files` is missing; use a safe getter that handles both cases.
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>
##########
superset-core/src/superset_core/extensions/types.py:
##########
@@ -15,49 +15,190 @@
# specific language governing permissions and limitations
# under the License.
-from typing import TypedDict
-
-
-class ModuleFederationConfig(TypedDict):
- exposes: dict[str, str]
- filename: str
- shared: dict[str, str]
- remotes: dict[str, str]
-
-
-class FrontendContributionConfig(TypedDict):
- commands: dict[str, list[dict[str, str]]]
- views: dict[str, list[dict[str, str]]]
- menus: dict[str, list[dict[str, str]]]
-
-
-class FrontendManifest(TypedDict):
- contributions: FrontendContributionConfig
- moduleFederation: ModuleFederationConfig
- remoteEntry: str
-
-
-class BackendManifest(TypedDict):
- entryPoints: list[str]
-
-
-class SharedBase(TypedDict, total=False):
- id: str
- name: str
- dependencies: list[str]
- description: str
- version: str
- frontend: FrontendManifest
- permissions: list[str]
-
-
-class Manifest(SharedBase, total=False):
- backend: BackendManifest
-
-
-class BackendMetadata(BackendManifest):
- files: list[str]
-
-
-class Metadata(SharedBase):
- backend: BackendMetadata
+"""
+Pydantic models for extension configuration and manifest validation.
+
+Two distinct schemas:
+- ExtensionConfig: Source configuration (extension.json) authored by developers
+- Manifest: Built output (manifest.json) generated by the build tool
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from pydantic import BaseModel, Field # noqa: I001
+
+# =============================================================================
+# Shared components
+# =============================================================================
+
+
+class ModuleFederationConfig(BaseModel):
+ """Configuration for Webpack Module Federation."""
+
+ exposes: list[str] = Field(
+ default_factory=list,
+ description="Modules exposed by this extension",
+ )
+ filename: str = Field(
+ default="remoteEntry.js",
+ description="Remote entry filename",
+ )
+ shared: dict[str, Any] = Field(
+ default_factory=dict,
+ description="Shared dependencies configuration",
+ )
+ remotes: dict[str, str] = Field(
+ default_factory=dict,
+ description="Remote module references",
+ )
+
+
+class ContributionConfig(BaseModel):
+ """Configuration for frontend UI contributions."""
+
+ commands: list[dict[str, Any]] = Field(
+ default_factory=list,
+ description="Command contributions",
+ )
+ views: dict[str, list[dict[str, Any]]] = Field(
+ default_factory=dict,
+ description="View contributions by location",
+ )
+ menus: dict[str, Any] = Field(
+ default_factory=dict,
+ description="Menu contributions",
+ )
+
+
+class BaseExtension(BaseModel):
+ """Base fields shared by ExtensionConfig and Manifest."""
+
+ id: str = Field(
+ ...,
+ description="Unique extension identifier",
+ min_length=1,
+ )
+ name: str = Field(
+ ...,
+ description="Human-readable extension name",
+ min_length=1,
+ )
+ version: str = Field(
+ default="0.0.0",
+ description="Semantic version string",
+ pattern=r"^\d+\.\d+\.\d+",
Review Comment:
**Suggestion:** The `Field` parameter `pattern` is not the conventional
Pydantic parameter for string validation and the regex lacks an end anchor;
change to `regex=r"^\d+\.\d+\.\d+$"` so Pydantic enforces a full
semantic-version match. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
regex=r"^\d+\.\d+\.\d+$",
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current Field uses `pattern` (not the documented Field keyword) and
omits an end-anchor, so it can be ignored by Pydantic schema/validation or
allow extra characters after the patch number. Replacing with
`regex=r"^\d+\.\d+\.\d+$"` tightens validation and uses the correct parameter
name that Pydantic expects for string pattern checks, fixing a real validation
issue.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-core/src/superset_core/extensions/types.py
**Line:** 91:91
**Comment:**
*Logic Error: The `Field` parameter `pattern` is not the conventional
Pydantic parameter for string validation and the regex lacks an end anchor;
change to `regex=r"^\d+\.\d+\.\d+$"` so Pydantic enforces a full
semantic-version match.
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>
--
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]
