Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion superset/extensions/discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

from superset.extensions.types import LoadedExtension
from superset.extensions.utils import get_bundle_files_from_zip, get_loaded_extension
from superset.utils import json

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -59,8 +60,27 @@ def discover_and_load_extensions(

try:
with ZipFile(supx_file, "r") as zip_file:
# Read the manifest first to get the extension ID for the
# supx:// path
try:
manifest_content = zip_file.read("manifest.json")
manifest_data = json.loads(manifest_content)
extension_id = manifest_data["id"]
except (KeyError, json.JSONDecodeError) as e:
logger.error(
"Failed to read extension ID from manifest in %s: %s",
supx_file,
e,
)
continue

# Use supx:// scheme for tracebacks
source_base_path = f"supx://{extension_id}"

files = get_bundle_files_from_zip(zip_file)
extension = get_loaded_extension(files)
extension = get_loaded_extension(
files, source_base_path=source_base_path
)
logger.info(
"Loaded extension '%s' from %s", extension.id, supx_file
)
Expand Down
3 changes: 3 additions & 0 deletions superset/extensions/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,6 @@ class LoadedExtension:
frontend: dict[str, bytes]
backend: dict[str, bytes]
version: str
source_base_path: (
str # Base path for traceback filenames (absolute path or supx:// URL)
)
Comment on lines +37 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Breaking change: adding source_base_path as a required dataclass field (no default) will cause existing code that constructs LoadedExtension without this new argument to raise a TypeError at runtime. Make the field optional or provide a default to preserve backward compatibility. [possible bug]

Severity Level: Critical 🚨
- ❌ Extension loading fails with TypeError during instantiation.
- ❌ Background tasks using extensions fail at startup/runtime.
- ⚠️ Backwards compatibility for third‑party extensions broken.
Suggested change
source_base_path: (
str # Base path for traceback filenames (absolute path or supx:// URL)
)
source_base_path: str = "" # Base path for traceback filenames (absolute path or supx:// URL)
Steps of Reproduction ✅
1. Open a Python REPL or run any process that imports the dataclass: `from
superset.extensions.types import LoadedExtension` (file:
`superset/extensions/types.py:30-39`).

2. Instantiate the dataclass with the pre-PR argument set used across the codebase, e.g.:

   `LoadedExtension(id="ext1", name="ext", manifest=..., frontend={}, backend={},
   version="1.0")`

   (this mirrors how LoadedExtension objects were constructed before adding the new
   field).

3. Observe Python raising a TypeError similar to:

   "TypeError: __init__() missing 1 required positional argument: 'source_base_path'"

   This is raised from the dataclass-generated __init__ defined by `LoadedExtension` (see
   `superset/extensions/types.py:30-39`).

4. Runtime effect: any code path that previously constructed LoadedExtension without the
new argument (e.g. extension registration/loader) will fail at instantiation time. Because
the PR description and stacktrace examples reference extension execution, this will
manifest when extensions are loaded or registered during startup or when tasks execute
(see `superset/tasks/executor.py:108` in PR description).
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset/extensions/types.py
**Line:** 37:39
**Comment:**
	*Possible Bug: Breaking change: adding `source_base_path` as a required dataclass field (no default) will cause existing code that constructs `LoadedExtension` without this new argument to raise a TypeError at runtime. Make the field optional or provide a default to preserve backward compatibility.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an issue, as this feature is still in development

55 changes: 48 additions & 7 deletions superset/extensions/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,30 @@ def exec_module(self, module: Any) -> None:
)
if self.is_package:
module.__path__ = []
exec(self.source, module.__dict__) # noqa: S102
# Compile with filename for proper tracebacks
code = compile(self.source, self.origin, "exec")
exec(code, module.__dict__) # noqa: S102


class InMemoryFinder(importlib.abc.MetaPathFinder):
def __init__(self, file_dict: dict[str, bytes]) -> None:
def __init__(self, file_dict: dict[str, bytes], source_base_path: str) -> None:
self.modules: dict[str, Tuple[Any, Any, Any]] = {}

# Detect if this is a virtual path (supx://) or filesystem path
is_virtual_path = source_base_path.startswith("supx://")

for path, content in file_dict.items():
mod_name, is_package = self._get_module_name(path)
self.modules[mod_name] = (content, is_package, path)

# Reconstruct full path for tracebacks
if is_virtual_path:
# Virtual paths always use forward slashes
# e.g., supx://extension-id/backend/src/tasks.py
full_path = f"{source_base_path}/backend/src/{path}"
else:
full_path = str(Path(source_base_path) / "backend" / "src" / path)

self.modules[mod_name] = (content, is_package, full_path)

def _get_module_name(self, file_path: str) -> Tuple[str, bool]:
parts = list(Path(file_path).parts)
Expand All @@ -88,8 +103,19 @@ def find_spec(self, fullname: str, path: Any, target: Any = None) -> Any | None:
return None


def install_in_memory_importer(file_dict: dict[str, bytes]) -> None:
finder = InMemoryFinder(file_dict)
def install_in_memory_importer(
file_dict: dict[str, bytes], source_base_path: str
) -> None:
"""
Install an in-memory module importer for extension backend code.

:param file_dict: Dictionary mapping relative file paths to their content
:param source_base_path: Base path for traceback filenames. For LOCAL_EXTENSIONS,
this should be an absolute filesystem path to the dist directory.
For EXTENSIONS_PATH (.supx files), this should be a supx:// URL
(e.g., "supx://extension-id").
"""
finder = InMemoryFinder(file_dict, source_base_path)
sys.meta_path.insert(0, finder)


Expand Down Expand Up @@ -121,7 +147,19 @@ def get_bundle_files_from_path(base_path: str) -> Generator[BundleFile, None, No
yield BundleFile(name=rel_path, content=content)


def get_loaded_extension(files: Iterable[BundleFile]) -> LoadedExtension:
def get_loaded_extension(
files: Iterable[BundleFile], source_base_path: str
) -> LoadedExtension:
"""
Load an extension from bundle files.

:param files: Iterable of BundleFile objects containing the extension files
:param source_base_path: Base path for traceback filenames. For LOCAL_EXTENSIONS,
this should be an absolute filesystem path to the dist directory.
For EXTENSIONS_PATH (.supx files), this should be a supx:// URL
(e.g., "supx://extension-id").
:returns: LoadedExtension instance
"""
manifest: Manifest | None = None
frontend: dict[str, bytes] = {}
backend: dict[str, bytes] = {}
Expand Down Expand Up @@ -158,6 +196,7 @@ def get_loaded_extension(files: Iterable[BundleFile]) -> LoadedExtension:
frontend=frontend,
backend=backend,
version=manifest.version,
source_base_path=source_base_path,
)


Expand Down Expand Up @@ -190,7 +229,9 @@ def get_extensions() -> dict[str, LoadedExtension]:
# Load extensions from LOCAL_EXTENSIONS configuration (filesystem paths)
for path in current_app.config["LOCAL_EXTENSIONS"]:
files = get_bundle_files_from_path(path)
extension = get_loaded_extension(files)
# Use absolute filesystem path to dist directory for tracebacks
abs_dist_path = str((Path(path) / "dist").resolve())
extension = get_loaded_extension(files, source_base_path=abs_dist_path)
extension_id = extension.manifest.id
extensions[extension_id] = extension
logger.info(
Expand Down
5 changes: 4 additions & 1 deletion superset/initialization/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,10 @@ def init_extensions(self) -> None:

for extension in extensions.values():
if backend_files := extension.backend:
install_in_memory_importer(backend_files)
install_in_memory_importer(
backend_files,
source_base_path=extension.source_base_path,
)

backend = extension.manifest.backend

Expand Down
Loading