chore: update API routers, validation, and docs for module system
- app/api/v1/admin/__init__.py: Updated router imports - app/api/v1/vendor/__init__.py: Updated router imports - app/exceptions/code_quality.py: Added module exception imports - scripts/validate_architecture.py: Added module validation rules - .architecture-rules/_main.yaml: Include module.yaml rules - docs/proposals/module-migration-plan.md: Updated migration status Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -221,6 +221,9 @@ class ArchitectureValidator:
|
||||
# Validate language/i18n rules
|
||||
self._validate_language_rules(target)
|
||||
|
||||
# Validate module structure
|
||||
self._validate_modules(target)
|
||||
|
||||
return self.result
|
||||
|
||||
def validate_file(self, file_path: Path, quiet: bool = False) -> ValidationResult:
|
||||
@@ -3906,6 +3909,386 @@ class ArchitectureValidator:
|
||||
suggestion="Specify the constraint name to drop",
|
||||
)
|
||||
|
||||
def _validate_modules(self, target_path: Path):
|
||||
"""Validate module structure rules (MOD-001 to MOD-012)"""
|
||||
print("📦 Validating module structure...")
|
||||
|
||||
modules_path = target_path / "app" / "modules"
|
||||
if not modules_path.exists():
|
||||
return
|
||||
|
||||
# Get all module directories (excluding __pycache__ and special files)
|
||||
module_dirs = [
|
||||
d for d in modules_path.iterdir()
|
||||
if d.is_dir() and d.name != "__pycache__" and not d.name.startswith(".")
|
||||
]
|
||||
|
||||
for module_dir in module_dirs:
|
||||
module_name = module_dir.name
|
||||
definition_file = module_dir / "definition.py"
|
||||
init_file = module_dir / "__init__.py"
|
||||
|
||||
# MOD-009: Check for definition.py (required for auto-discovery)
|
||||
if not definition_file.exists():
|
||||
# Only report if it looks like a module (has __init__.py or other .py files)
|
||||
has_py_files = any(f.suffix == ".py" for f in module_dir.iterdir() if f.is_file())
|
||||
if has_py_files or init_file.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-009",
|
||||
rule_name="Module must have definition.py for auto-discovery",
|
||||
severity=Severity.ERROR,
|
||||
file_path=module_dir / "__init__.py" if init_file.exists() else module_dir,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' missing definition.py - won't be auto-discovered",
|
||||
context="Module directory exists but no definition.py",
|
||||
suggestion="Create definition.py with ModuleDefinition instance",
|
||||
)
|
||||
continue
|
||||
|
||||
# Check for __init__.py
|
||||
if not init_file.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-009",
|
||||
rule_name="Module must have definition.py for auto-discovery",
|
||||
severity=Severity.ERROR,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' missing __init__.py",
|
||||
context="definition.py exists but no __init__.py",
|
||||
suggestion="Create __init__.py to make module importable",
|
||||
)
|
||||
|
||||
# Read definition to check if is_self_contained
|
||||
definition_content = definition_file.read_text()
|
||||
|
||||
is_self_contained = "is_self_contained=True" in definition_content or "is_self_contained = True" in definition_content
|
||||
|
||||
if not is_self_contained:
|
||||
continue
|
||||
|
||||
# MOD-001: Check required directories for self-contained modules
|
||||
required_dirs = ["services", "models", "schemas", "routes"]
|
||||
for req_dir in required_dirs:
|
||||
dir_path = module_dir / req_dir
|
||||
if not dir_path.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-001",
|
||||
rule_name="Self-contained modules must have required directories",
|
||||
severity=Severity.ERROR,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' is self-contained but missing '{req_dir}/' directory",
|
||||
context=f"is_self_contained=True",
|
||||
suggestion=f"Create '{req_dir}/' directory with __init__.py",
|
||||
)
|
||||
elif req_dir != "routes":
|
||||
# Check for __init__.py
|
||||
init_file = dir_path / "__init__.py"
|
||||
if not init_file.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-001",
|
||||
rule_name="Self-contained modules must have required directories",
|
||||
severity=Severity.ERROR,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' missing '{req_dir}/__init__.py'",
|
||||
context=f"is_self_contained=True",
|
||||
suggestion=f"Create '{req_dir}/__init__.py' with exports",
|
||||
)
|
||||
|
||||
# Check routes/api/ exists
|
||||
routes_api_path = module_dir / "routes" / "api"
|
||||
if not routes_api_path.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-001",
|
||||
rule_name="Self-contained modules must have required directories",
|
||||
severity=Severity.WARNING,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' is self-contained but missing 'routes/api/' directory",
|
||||
context=f"is_self_contained=True",
|
||||
suggestion="Create 'routes/api/' directory for API endpoints",
|
||||
)
|
||||
|
||||
# MOD-002: Check services contain actual code, not re-exports
|
||||
services_dir = module_dir / "services"
|
||||
if services_dir.exists():
|
||||
self._check_module_contains_actual_code(
|
||||
services_dir, module_name, "services", "MOD-002",
|
||||
["from app.services.", "from app/services/"]
|
||||
)
|
||||
|
||||
# MOD-003: Check schemas contain actual code, not re-exports
|
||||
schemas_dir = module_dir / "schemas"
|
||||
if schemas_dir.exists():
|
||||
self._check_module_contains_actual_code(
|
||||
schemas_dir, module_name, "schemas", "MOD-003",
|
||||
["from models.schema.", "from models/schema/"]
|
||||
)
|
||||
|
||||
# MOD-004: Check routes import from module, not legacy locations
|
||||
routes_dir = module_dir / "routes"
|
||||
if routes_dir.exists():
|
||||
for route_file in routes_dir.rglob("*.py"):
|
||||
if route_file.name == "__init__.py":
|
||||
continue
|
||||
content = route_file.read_text()
|
||||
lines = content.split("\n")
|
||||
for i, line in enumerate(lines, 1):
|
||||
if "from app.services." in line:
|
||||
self._add_violation(
|
||||
rule_id="MOD-004",
|
||||
rule_name="Module routes must use module-internal implementations",
|
||||
severity=Severity.WARNING,
|
||||
file_path=route_file,
|
||||
line_number=i,
|
||||
message=f"Route imports from legacy 'app.services' instead of module services",
|
||||
context=line.strip()[:80],
|
||||
suggestion=f"Import from 'app.modules.{module_name}.services' or '..services'",
|
||||
)
|
||||
|
||||
# MOD-005: Check for templates/static if module has UI (menu_items)
|
||||
has_menu_items = "menu_items" in definition_content and "FrontendType." in definition_content
|
||||
# Check if menu_items has actual entries (not just empty dict)
|
||||
has_actual_menu = (
|
||||
has_menu_items and
|
||||
not re.search(r"menu_items\s*=\s*\{\s*\}", definition_content) and
|
||||
('"' in definition_content[definition_content.find("menu_items"):definition_content.find("menu_items")+200] if "menu_items" in definition_content else False)
|
||||
)
|
||||
|
||||
if has_actual_menu:
|
||||
templates_dir = module_dir / "templates"
|
||||
static_dir = module_dir / "static"
|
||||
|
||||
if not templates_dir.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-005",
|
||||
rule_name="Modules with UI must have templates and static directories",
|
||||
severity=Severity.WARNING,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' has menu_items but missing 'templates/' directory",
|
||||
context="has_menu_items=True",
|
||||
suggestion=f"Create 'templates/{module_name}/admin/' and/or 'templates/{module_name}/vendor/'",
|
||||
)
|
||||
|
||||
if not static_dir.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-005",
|
||||
rule_name="Modules with UI must have templates and static directories",
|
||||
severity=Severity.WARNING,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' has menu_items but missing 'static/' directory",
|
||||
context="has_menu_items=True",
|
||||
suggestion="Create 'static/admin/js/' and/or 'static/vendor/js/'",
|
||||
)
|
||||
|
||||
# MOD-006: Check for locales (info level)
|
||||
locales_dir = module_dir / "locales"
|
||||
if not locales_dir.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-006",
|
||||
rule_name="Module locales should exist for internationalization",
|
||||
severity=Severity.INFO,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' is missing 'locales/' directory for translations",
|
||||
context="is_self_contained=True",
|
||||
suggestion="Create 'locales/' with en.json, de.json, fr.json, lu.json",
|
||||
)
|
||||
|
||||
# MOD-008: Check for exceptions.py
|
||||
exceptions_file = module_dir / "exceptions.py"
|
||||
exceptions_pkg = module_dir / "exceptions" / "__init__.py"
|
||||
if not exceptions_file.exists() and not exceptions_pkg.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-008",
|
||||
rule_name="Self-contained modules must have exceptions.py",
|
||||
severity=Severity.WARNING,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' is missing 'exceptions.py' for module-specific exceptions",
|
||||
context="is_self_contained=True",
|
||||
suggestion="Create 'exceptions.py' with module-specific exception classes",
|
||||
)
|
||||
|
||||
# MOD-010: Check route files export 'router' variable
|
||||
self._check_route_exports(module_dir, module_name)
|
||||
|
||||
# MOD-011: Check tasks directory has __init__.py
|
||||
tasks_dir = module_dir / "tasks"
|
||||
if tasks_dir.exists():
|
||||
tasks_init = tasks_dir / "__init__.py"
|
||||
if not tasks_init.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-011",
|
||||
rule_name="Module tasks must have __init__.py for Celery discovery",
|
||||
severity=Severity.WARNING,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' has tasks/ but missing tasks/__init__.py",
|
||||
context="tasks/ directory exists",
|
||||
suggestion="Create 'tasks/__init__.py' for Celery auto-discovery",
|
||||
)
|
||||
|
||||
# MOD-012: Check locales has all language files
|
||||
if locales_dir.exists():
|
||||
required_langs = ["en.json", "de.json", "fr.json", "lu.json"]
|
||||
missing_langs = [lang for lang in required_langs if not (locales_dir / lang).exists()]
|
||||
if missing_langs:
|
||||
self._add_violation(
|
||||
rule_id="MOD-012",
|
||||
rule_name="Module locales should have all supported language files",
|
||||
severity=Severity.INFO,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module '{module_name}' missing locale files: {', '.join(missing_langs)}",
|
||||
context="locales/ directory exists",
|
||||
suggestion=f"Add translation files: {', '.join(missing_langs)}",
|
||||
)
|
||||
|
||||
# MOD-007: Validate definition paths match directory structure
|
||||
self._validate_module_definition_paths(module_dir, module_name, definition_file, definition_content)
|
||||
|
||||
def _check_module_contains_actual_code(
|
||||
self,
|
||||
dir_path: Path,
|
||||
module_name: str,
|
||||
dir_type: str,
|
||||
rule_id: str,
|
||||
anti_patterns: list[str]
|
||||
):
|
||||
"""Check if module directory contains actual code vs re-exports"""
|
||||
py_files = list(dir_path.glob("*.py"))
|
||||
|
||||
# Track if we found any file with actual code (not just re-exports)
|
||||
has_actual_code = False
|
||||
|
||||
for py_file in py_files:
|
||||
if py_file.name == "__init__.py":
|
||||
continue
|
||||
|
||||
content = py_file.read_text()
|
||||
lines = content.split("\n")
|
||||
|
||||
# Check if file has actual class/function definitions
|
||||
has_definitions = any(
|
||||
re.match(r"^(class|def|async def)\s+\w+", line)
|
||||
for line in lines
|
||||
)
|
||||
|
||||
# Check if file only has re-exports
|
||||
has_reexport = any(
|
||||
any(pattern in line for pattern in anti_patterns)
|
||||
for line in lines
|
||||
)
|
||||
|
||||
if has_definitions and not has_reexport:
|
||||
has_actual_code = True
|
||||
elif has_reexport and not has_definitions:
|
||||
# File is a re-export only
|
||||
for i, line in enumerate(lines, 1):
|
||||
for pattern in anti_patterns:
|
||||
if pattern in line:
|
||||
self._add_violation(
|
||||
rule_id=rule_id,
|
||||
rule_name=f"Module {dir_type} must contain actual code, not re-exports",
|
||||
severity=Severity.WARNING,
|
||||
file_path=py_file,
|
||||
line_number=i,
|
||||
message=f"File re-exports from legacy location instead of containing actual code",
|
||||
context=line.strip()[:80],
|
||||
suggestion=f"Move actual code to this file and update legacy to re-export from here",
|
||||
)
|
||||
break
|
||||
|
||||
def _check_route_exports(self, module_dir: Path, module_name: str):
|
||||
"""MOD-010: Check that route files export 'router' variable for auto-discovery."""
|
||||
routes_dir = module_dir / "routes"
|
||||
if not routes_dir.exists():
|
||||
return
|
||||
|
||||
# Check api and pages subdirectories
|
||||
for route_type in ["api", "pages"]:
|
||||
type_dir = routes_dir / route_type
|
||||
if not type_dir.exists():
|
||||
continue
|
||||
|
||||
for route_file in ["admin.py", "vendor.py", "shop.py"]:
|
||||
file_path = type_dir / route_file
|
||||
if not file_path.exists():
|
||||
continue
|
||||
|
||||
content = file_path.read_text()
|
||||
|
||||
# Check for 'router' variable definition
|
||||
has_router = (
|
||||
"router = APIRouter" in content or
|
||||
"router: APIRouter" in content or
|
||||
"\nrouter = " in content
|
||||
)
|
||||
|
||||
if not has_router:
|
||||
self._add_violation(
|
||||
rule_id="MOD-010",
|
||||
rule_name="Module routes must export router variable for auto-discovery",
|
||||
severity=Severity.WARNING,
|
||||
file_path=file_path,
|
||||
line_number=1,
|
||||
message=f"Route file missing 'router' variable for auto-discovery",
|
||||
context=f"routes/{route_type}/{route_file}",
|
||||
suggestion="Add: router = APIRouter() and use @router.get/post decorators",
|
||||
)
|
||||
|
||||
def _validate_module_definition_paths(
|
||||
self,
|
||||
module_dir: Path,
|
||||
module_name: str,
|
||||
definition_file: Path,
|
||||
definition_content: str
|
||||
):
|
||||
"""MOD-007: Validate that paths in definition match actual directories"""
|
||||
path_checks = [
|
||||
("services_path", "services/__init__.py"),
|
||||
("models_path", "models/__init__.py"),
|
||||
("schemas_path", "schemas/__init__.py"),
|
||||
("templates_path", "templates"),
|
||||
("locales_path", "locales"),
|
||||
]
|
||||
|
||||
for path_attr, expected_path in path_checks:
|
||||
# Check if path is defined in definition
|
||||
if f'{path_attr}="' in definition_content or f"{path_attr}='" in definition_content:
|
||||
expected_full_path = module_dir / expected_path
|
||||
if not expected_full_path.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-007",
|
||||
rule_name="Module definition must match directory structure",
|
||||
severity=Severity.ERROR,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module definition specifies '{path_attr}' but '{expected_path}' does not exist",
|
||||
context=f"{path_attr}=app.modules.{module_name}.*",
|
||||
suggestion=f"Create '{expected_path}' or remove '{path_attr}' from definition",
|
||||
)
|
||||
|
||||
# Check exceptions_path specially (can be .py or package)
|
||||
if 'exceptions_path="' in definition_content or "exceptions_path='" in definition_content:
|
||||
exceptions_file = module_dir / "exceptions.py"
|
||||
exceptions_pkg = module_dir / "exceptions" / "__init__.py"
|
||||
if not exceptions_file.exists() and not exceptions_pkg.exists():
|
||||
self._add_violation(
|
||||
rule_id="MOD-007",
|
||||
rule_name="Module definition must match directory structure",
|
||||
severity=Severity.ERROR,
|
||||
file_path=definition_file,
|
||||
line_number=1,
|
||||
message=f"Module definition specifies 'exceptions_path' but no exceptions module exists",
|
||||
context=f"exceptions_path=app.modules.{module_name}.exceptions",
|
||||
suggestion="Create 'exceptions.py' or 'exceptions/__init__.py'",
|
||||
)
|
||||
|
||||
def _get_rule(self, rule_id: str) -> dict[str, Any]:
|
||||
"""Get rule configuration by ID"""
|
||||
# Look in different rule categories
|
||||
@@ -3919,6 +4302,7 @@ class ArchitectureValidator:
|
||||
"frontend_component_rules",
|
||||
"language_rules",
|
||||
"migration_rules",
|
||||
"module_rules",
|
||||
]:
|
||||
rules = self.config.get(category, [])
|
||||
for rule in rules:
|
||||
|
||||
Reference in New Issue
Block a user