From 0408198af034127ea1bbf85a8fa51d59c04e8c6a Mon Sep 17 00:00:00 2001 From: forbes-0023 Date: Thu, 5 Mar 2026 08:00:57 -0600 Subject: [PATCH] feat(addon-loader): strengthen manifest validation at parse time (#388) - Change AddonManifest.error (str) to .errors (list[str]) so all problems are accumulated in a single pass instead of masking subsequent failures. - Validate load_priority type, version string format (dotted-numeric), and context ID syntax (alphanumeric + dots/underscores) during parse_manifest(). - Add validate_dependencies() cross-addon check that verifies all declared dependencies reference discovered addon names, called between parsing and validation in the pipeline. - Rewrite validate_manifest() to run all checks (version bounds, workbench path, Init.py presence) and collect errors instead of early-returning on the first failure. - Simplify resolve_load_order() by removing the inline unknown-dependency check (now handled earlier by validate_dependencies()). - Update _print_load_summary() to join multiple errors with semicolons. Closes #388 --- src/Mod/Create/addon_loader.py | 146 ++++++++++++++++++++------------- 1 file changed, 88 insertions(+), 58 deletions(-) diff --git a/src/Mod/Create/addon_loader.py b/src/Mod/Create/addon_loader.py index d1eea81cc6..482dc9deb6 100644 --- a/src/Mod/Create/addon_loader.py +++ b/src/Mod/Create/addon_loader.py @@ -6,6 +6,7 @@ import enum import os +import re import sys import time import xml.etree.ElementTree as ET @@ -50,12 +51,14 @@ class AddonManifest: # Runtime state state: AddonState = AddonState.DISCOVERED - error: str = "" + errors: list[str] = field(default_factory=list) load_time_ms: float = 0.0 contexts: list[str] = field(default_factory=list) def __repr__(self): - return f"AddonManifest(name={self.name!r}, version={self.version!r}, state={self.state.value})" + return ( + f"AddonManifest(name={self.name!r}, version={self.version!r}, state={self.state.value})" + ) # --------------------------------------------------------------------------- @@ -191,6 +194,16 @@ def scan_addons(mods_dir: str) -> list[AddonManifest]: # Parsing # --------------------------------------------------------------------------- +# Validation patterns +_VERSION_RE = re.compile(r"^\d+(\.\d+)*$") +_CONTEXT_ID_RE = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9_.]*$") + + +def _valid_version(v: str) -> bool: + """Check that a version string is dotted-numeric (e.g. '0.1.0').""" + return bool(_VERSION_RE.match(v)) + + # FreeCAD package.xml namespace _PKG_NS = "https://wiki.freecad.org/Package_Metadata" @@ -224,10 +237,8 @@ def parse_manifest(manifest: AddonManifest): root = tree.getroot() except ET.ParseError as e: manifest.state = AddonState.FAILED - manifest.error = f"XML parse error: {e}" - FreeCAD.Console.PrintWarning( - f"Create: Failed to parse {manifest.package_xml_path}: {e}\n" - ) + manifest.errors.append(f"XML parse error: {e}") + FreeCAD.Console.PrintWarning(f"Create: Failed to parse {manifest.package_xml_path}: {e}\n") return # Standard fields @@ -259,22 +270,43 @@ def parse_manifest(manifest: AddonManifest): manifest.has_kindred_element = True manifest.min_create_version = _text(kindred, "min_create_version") or None manifest.max_create_version = _text(kindred, "max_create_version") or None + + # Validate version string formats + if manifest.min_create_version and not _valid_version(manifest.min_create_version): + manifest.errors.append( + f"min_create_version is not a valid version: {manifest.min_create_version!r}" + ) + if manifest.max_create_version and not _valid_version(manifest.max_create_version): + manifest.errors.append( + f"max_create_version is not a valid version: {manifest.max_create_version!r}" + ) + + # Validate load_priority is an integer priority_str = _text(kindred, "load_priority") if priority_str: try: manifest.load_priority = int(priority_str) except ValueError: - pass + manifest.errors.append(f"load_priority must be an integer, got: {priority_str!r}") + deps = _find(kindred, "dependencies") if deps is not None: for dep in _findall(deps, "dependency"): if dep.text and dep.text.strip(): manifest.dependencies.append(dep.text.strip()) + + # Validate context ID syntax ctxs = _find(kindred, "contexts") if ctxs is not None: for ctx in _findall(ctxs, "context"): - if ctx.text and ctx.text.strip(): - manifest.contexts.append(ctx.text.strip()) + cid = ctx.text.strip() if ctx.text else "" + if cid: + if _CONTEXT_ID_RE.match(cid): + manifest.contexts.append(cid) + else: + manifest.errors.append( + f"Invalid context ID {cid!r}: must be alphanumeric, dots, or underscores" + ) FreeCAD.Console.PrintLog( f"Create: Parsed {manifest.name} v{manifest.version} from {manifest.package_xml_path}\n" @@ -294,8 +326,27 @@ def _parse_version(v: str) -> tuple: return (0, 0, 0) +def validate_dependencies(manifests: list[AddonManifest]): + """Check that all declared dependencies reference discovered addons. + + Called after parsing all manifests so the full set of names is known. + Errors are accumulated on each manifest. + """ + known = {m.name for m in manifests} + for m in manifests: + if m.state == AddonState.FAILED: + continue + for dep in m.dependencies: + if dep not in known: + m.errors.append(f"Unknown dependency: {dep!r}") + + def validate_manifest(manifest: AddonManifest, create_version: str) -> bool: - """Check version compatibility and path existence. Returns True if valid.""" + """Check version compatibility and path existence. + + All checks run to completion so that every problem is reported. + Returns True if the manifest is valid for loading. + """ if manifest.state == AddonState.FAILED: return False @@ -303,39 +354,29 @@ def validate_manifest(manifest: AddonManifest, create_version: str) -> bool: if manifest.min_create_version: if cv < _parse_version(manifest.min_create_version): - manifest.state = AddonState.SKIPPED - manifest.error = f"Requires Create >= {manifest.min_create_version}, running {create_version}" - FreeCAD.Console.PrintWarning( - f"Create: Skipping {manifest.name}: {manifest.error}\n" + manifest.errors.append( + f"Requires Create >= {manifest.min_create_version}, running {create_version}" ) - return False if manifest.max_create_version: if cv > _parse_version(manifest.max_create_version): - manifest.state = AddonState.SKIPPED - manifest.error = f"Requires Create <= {manifest.max_create_version}, running {create_version}" - FreeCAD.Console.PrintWarning( - f"Create: Skipping {manifest.name}: {manifest.error}\n" + manifest.errors.append( + f"Requires Create <= {manifest.max_create_version}, running {create_version}" ) - return False if not os.path.isdir(manifest.workbench_path): - manifest.state = AddonState.SKIPPED - manifest.error = f"Workbench path not found: {manifest.workbench_path}" - FreeCAD.Console.PrintWarning( - f"Create: Skipping {manifest.name}: {manifest.error}\n" - ) - return False + manifest.errors.append(f"Workbench path not found: {manifest.workbench_path}") + else: + # Only check Init files if the directory exists + has_init = os.path.isfile(os.path.join(manifest.workbench_path, "Init.py")) + has_gui = os.path.isfile(os.path.join(manifest.workbench_path, "InitGui.py")) + if not has_init and not has_gui: + manifest.errors.append(f"No Init.py or InitGui.py in {manifest.workbench_path}") - # At least one of Init.py or InitGui.py must exist - has_init = os.path.isfile(os.path.join(manifest.workbench_path, "Init.py")) - has_gui = os.path.isfile(os.path.join(manifest.workbench_path, "InitGui.py")) - if not has_init and not has_gui: + if manifest.errors: manifest.state = AddonState.SKIPPED - manifest.error = f"No Init.py or InitGui.py in {manifest.workbench_path}" - FreeCAD.Console.PrintWarning( - f"Create: Skipping {manifest.name}: {manifest.error}\n" - ) + for err in manifest.errors: + FreeCAD.Console.PrintWarning(f"Create: {manifest.name}: {err}\n") return False manifest.state = AddonState.VALIDATED @@ -347,9 +388,7 @@ def validate_manifest(manifest: AddonManifest, create_version: str) -> bool: # --------------------------------------------------------------------------- -def resolve_load_order( - manifests: list[AddonManifest], mods_dir: str -) -> list[AddonManifest]: +def resolve_load_order(manifests: list[AddonManifest], mods_dir: str) -> list[AddonManifest]: """Sort addons by dependencies, then by (load_priority, name). If no addons declare a element, fall back to the legacy @@ -370,15 +409,10 @@ def resolve_load_order( ts = TopologicalSorter() for m in manifests: - # Only include dependencies that are actually discovered + if m.state in (AddonState.SKIPPED, AddonState.FAILED): + continue known_deps = [d for d in m.dependencies if d in by_name] - unknown_deps = [d for d in m.dependencies if d not in by_name] - for dep in unknown_deps: - m.state = AddonState.SKIPPED - m.error = f"Missing dependency: {dep}" - FreeCAD.Console.PrintWarning(f"Create: Skipping {m.name}: {m.error}\n") - if m.state != AddonState.SKIPPED: - ts.add(m.name, *known_deps) + ts.add(m.name, *known_deps) try: # Process level by level so we can sort within each topological level @@ -387,11 +421,7 @@ def resolve_load_order( while ts.is_active(): ready = list(ts.get_ready()) # Sort each level by (priority, name) for determinism - ready.sort( - key=lambda n: ( - (by_name[n].load_priority, n) if n in by_name else (999, n) - ) - ) + ready.sort(key=lambda n: (by_name[n].load_priority, n) if n in by_name else (999, n)) for name in ready: ts.done(name) order.extend(ready) @@ -400,7 +430,7 @@ def resolve_load_order( f"Create: Dependency cycle detected: {e}. Falling back to priority order.\n" ) return sorted( - [m for m in manifests if m.state != AddonState.SKIPPED], + [m for m in manifests if m.state not in (AddonState.SKIPPED, AddonState.FAILED)], key=lambda m: (m.load_priority, m.name), ) @@ -408,7 +438,7 @@ def resolve_load_order( result = [] for name in order: m = by_name.get(name) - if m is not None and m.state != AddonState.SKIPPED: + if m is not None and m.state not in (AddonState.SKIPPED, AddonState.FAILED): result.append(m) return result @@ -465,12 +495,10 @@ def _load_addon(manifest: AddonManifest, gui: bool = False): else: manifest.load_time_ms += elapsed manifest.state = AddonState.LOADED - FreeCAD.Console.PrintLog( - f"Create: Loaded {manifest.name} {init_file} ({elapsed:.0f}ms)\n" - ) + FreeCAD.Console.PrintLog(f"Create: Loaded {manifest.name} {init_file} ({elapsed:.0f}ms)\n") except Exception as e: manifest.state = AddonState.FAILED - manifest.error = str(e) + manifest.errors.append(str(e)) FreeCAD.Console.PrintWarning(f"Create: Failed to load {manifest.name}: {e}\n") @@ -500,8 +528,8 @@ def _print_load_summary(registry: AddonRegistry, phase: str): state_str = m.state.value.upper() time_str = f"{m.load_time_ms:.0f}ms" if m.load_time_ms > 0 else "-" line = f" {m.name:<{max_name}} {state_str:<12} {time_str:>6}" - if m.error: - line += f" ({m.error})" + if m.errors: + line += f" ({'; '.join(m.errors)})" lines.append(line) FreeCAD.Console.PrintLog("\n".join(lines) + "\n") @@ -527,6 +555,8 @@ def load_addons(gui: bool = False): for m in manifests: parse_manifest(m) + validate_dependencies(manifests) + create_version = _get_create_version() validated = [m for m in manifests if validate_manifest(m, create_version)] ordered = resolve_load_order(validated, mods_dir)