diff --git a/.architecture-rules.yaml b/.architecture-rules.yaml index cc6fd228..71cb16e5 100644 --- a/.architecture-rules.yaml +++ b/.architecture-rules.yaml @@ -348,13 +348,23 @@ model_rules: - "from_attributes = True" - id: "MDL-004" - name: "Database models use singular table names" + name: "Database tables use plural names" severity: "warning" description: | - Database table names should be singular lowercase (e.g., 'vendor' not 'vendors'). + Database table names should be plural lowercase following industry standard + conventions (Rails, Django, Laravel, most ORMs). A table represents a + collection of entities, so plural names are natural: 'users', 'orders', + 'products'. This reads naturally in SQL: SELECT * FROM users WHERE id = 1. + + Examples: + - Good: users, vendors, products, orders, order_items, cart_items + - Bad: user, vendor, product, order + + Junction/join tables use both entity names in plural: + - Good: vendor_users, order_items, product_translations pattern: file_pattern: "models/database/**/*.py" - check: "table_naming" + check: "table_naming_plural" # ============================================================================ # EXCEPTION HANDLING RULES @@ -573,6 +583,65 @@ javascript_rules: loading = false; } + - id: "JS-008" + name: "Use apiClient for API calls, not raw fetch()" + severity: "error" + description: | + All API calls must use the apiClient helper instead of raw fetch(). + The apiClient automatically: + - Adds Authorization header with JWT token from cookies + - Sets Content-Type headers + - Handles error responses consistently + - Provides logging integration + + WRONG (raw fetch): + const response = await fetch('/api/v1/admin/products/123'); + + RIGHT (apiClient): + const response = await apiClient.get('/admin/products/123'); + const result = await apiClient.post('/admin/products', data); + await apiClient.delete('/admin/products/123'); + pattern: + file_pattern: "static/**/js/**/*.js" + anti_patterns: + - "fetch\\('/api/" + - 'fetch\\("/api/' + - "fetch\\(`/api/" + exceptions: + - "init-api-client.js" # The apiClient implementation itself + + - id: "JS-009" + name: "Use Utils.showToast() for notifications, not alert() or window.showToast" + severity: "error" + description: | + All user notifications must use Utils.showToast() from static/shared/js/utils.js. + Never use browser alert() dialogs or undefined window.showToast. + + Utils.showToast() provides: + - Consistent styling (Tailwind-based toast in bottom-right corner) + - Automatic fade-out after duration + - Color-coded types (success=green, error=red, warning=yellow, info=blue) + + WRONG (browser dialog): + alert('Product saved successfully'); + alert(errorMessage); + + WRONG (undefined function): + window.showToast('Success', 'success'); + if (window.showToast) { window.showToast(...); } else { alert(...); } + + RIGHT (Utils helper): + Utils.showToast('Product saved successfully', 'success'); + Utils.showToast('Failed to save product', 'error'); + Utils.showToast('Please fill all required fields', 'warning'); + pattern: + file_pattern: "static/**/js/**/*.js" + anti_patterns: + - "alert\\(" + - "window\\.showToast" + exceptions: + - "utils.js" # The Utils implementation itself + # ============================================================================ # TEMPLATE RULES (Jinja2) # ============================================================================ diff --git a/docs/development/naming-conventions.md b/docs/development/naming-conventions.md index cbc4948a..9b1e7cdc 100644 --- a/docs/development/naming-conventions.md +++ b/docs/development/naming-conventions.md @@ -285,17 +285,42 @@ frontend/ ### Database Naming -**Table Names**: Use singular, lowercase with underscores +**Table Names**: Use plural, lowercase with underscores (industry standard) + +Following Rails, Django, Laravel conventions - tables represent collections of entities, +so plural names are natural and read well in SQL queries. + ```sql --- ✅ Correct -inventory -inventory_movements +-- ✅ Correct (plural table names) +users +vendors +products +orders +customers +order_items +cart_items vendor_users --- ❌ Incorrect -inventories -inventory_movement -vendorusers +-- ❌ Incorrect (singular table names) +user +vendor +product +order +customer +``` + +**Rationale**: +- A table holds a *collection* of records (many users, many products) +- SQL reads naturally: `SELECT * FROM users WHERE id = 1` +- Follows conventions of most ORMs (ActiveRecord, Django ORM, Laravel Eloquent) +- Used by ~80% of modern web frameworks + +**Junction/Join Tables**: Combine both entity names in plural +```sql +-- ✅ Correct +vendor_users -- Links vendors and users +order_items -- Links orders and products +product_translations -- Translations for products ``` **Column Names**: Use singular, descriptive names @@ -410,7 +435,7 @@ class Customer: - [ ] File names follow singular/plural conventions - [ ] Class names use appropriate terminology (inventory vs stock) - [ ] API endpoints use plural resource names -- [ ] Database models use singular names +- [ ] Database table names use plural (industry standard) - [ ] Variables names match their content (singular vs plural) ### Automated Checks @@ -428,13 +453,13 @@ Consider implementing linting rules or pre-commit hooks to enforce: | Component | Naming | Example | |-----------|--------|---------| | API Endpoint Files | PLURAL | `products.py`, `orders.py` | -| Database Models | SINGULAR | `product.py`, `order.py` | +| Database Models (files) | SINGULAR | `product.py`, `order.py` | | Schema/Pydantic | SINGULAR | `product.py`, `order.py` | | Services | SINGULAR + service | `product_service.py` | | Exceptions | SINGULAR | `product.py`, `order.py` | | Middleware | SIMPLE NOUN | `auth.py`, `logging.py`, `context.py` | | Middleware Tests | test_{name}.py | `test_auth.py`, `test_logging.py` | -| Database Tables | SINGULAR | `product`, `inventory` | +| **Database Tables** | **PLURAL** | `users`, `products`, `orders` | | Database Columns | SINGULAR | `vendor_id`, `created_at` | | API Endpoints | PLURAL | `/products`, `/orders` | | Functions (single) | SINGULAR | `create_product()` | diff --git a/models/schema/auth.py b/models/schema/auth.py index 6606fc8a..4483db22 100644 --- a/models/schema/auth.py +++ b/models/schema/auth.py @@ -153,3 +153,5 @@ class VendorUserResponse(BaseModel): email: str role: str is_active: bool + + model_config = {"from_attributes": True} diff --git a/scripts/validate_architecture.py b/scripts/validate_architecture.py index 29759768..8217d385 100755 --- a/scripts/validate_architecture.py +++ b/scripts/validate_architecture.py @@ -151,6 +151,15 @@ class ArchitectureValidator: # Validate exception handling self._validate_exceptions(target) + # Validate naming conventions + self._validate_naming_conventions(target) + + # Validate auth patterns + self._validate_auth_patterns(target) + + # Validate middleware + self._validate_middleware(target) + # Validate JavaScript self._validate_javascript(target) @@ -369,6 +378,309 @@ class ArchitectureValidator: suggestion="Replace window.apiClient with apiClient", ) + # JS-003: Check Alpine components spread ...data() for base layout inheritance + # Only check files that define Alpine component functions (function adminXxx() { return {...} }) + self._check_alpine_data_spread(file_path, content, lines) + + # JS-004: Check Alpine components set currentPage + self._check_alpine_current_page(file_path, content, lines) + + # JS-005: Check initialization guard + self._check_init_guard(file_path, content, lines) + + # JS-006: Check async error handling + self._check_async_error_handling(file_path, content, lines) + + # JS-007: Check loading state management + self._check_loading_state(file_path, content, lines) + + # JS-008: Check for raw fetch() calls instead of apiClient + self._check_fetch_vs_api_client(file_path, content, lines) + + # JS-009: Check for alert() or window.showToast instead of Utils.showToast() + self._check_toast_usage(file_path, content, lines) + + def _check_toast_usage(self, file_path: Path, content: str, lines: list[str]): + """JS-009: Check for alert() or window.showToast instead of Utils.showToast()""" + # Skip utils.js (where showToast is defined) + if "utils.js" in file_path.name: + return + + # Check for file-level noqa comment + if "noqa: js-009" in content.lower(): + return + + for i, line in enumerate(lines, 1): + stripped = line.strip() + + # Skip comments + if stripped.startswith("//") or stripped.startswith("/*"): + continue + + # Skip lines with inline noqa comment + if "noqa: js-009" in line.lower(): + continue + + # Check for alert() usage + if re.search(r"\balert\s*\(", line): + self._add_violation( + rule_id="JS-009", + rule_name="Use Utils.showToast() for notifications", + severity=Severity.ERROR, + file_path=file_path, + line_number=i, + message="Browser alert() dialog - use Utils.showToast() for consistent UX", + context=stripped[:80], + suggestion="Replace alert('message') with Utils.showToast('message', 'success'|'error'|'warning'|'info')", + ) + + # Check for window.showToast usage + if "window.showToast" in line: + self._add_violation( + rule_id="JS-009", + rule_name="Use Utils.showToast() for notifications", + severity=Severity.ERROR, + file_path=file_path, + line_number=i, + message="window.showToast is undefined - use Utils.showToast()", + context=stripped[:80], + suggestion="Replace window.showToast('msg', 'type') with Utils.showToast('msg', 'type')", + ) + + def _check_alpine_data_spread(self, file_path: Path, content: str, lines: list[str]): + """JS-003: Check that Alpine components inherit base layout data using ...data()""" + # Skip utility/init files that aren't page components + skip_patterns = ["init-", "api-client", "log-config", "utils", "helpers"] + if any(pattern in file_path.name for pattern in skip_patterns): + return + + # Check for noqa comment + if "noqa: js-003" in content.lower(): + return + + # Look for Alpine component function pattern: function adminXxx() { return { ... } } + # These are page-level components that should inherit from data() + component_pattern = re.compile( + r"function\s+(admin\w+|vendor\w+|shop\w+)\s*\(\s*\)\s*\{", + re.IGNORECASE + ) + + for match in component_pattern.finditer(content): + func_name = match.group(1) + func_start = match.start() + line_num = content[:func_start].count('\n') + 1 + + # Find the return statement with object literal + # Look for "return {" within reasonable distance + search_region = content[func_start:func_start + 500] + + if "return {" in search_region: + # Check if ...data() is present in the return object + # Look for pattern like "return {\n ...data()," or similar + return_match = re.search(r"return\s*\{([^}]{0,200})", search_region, re.DOTALL) + if return_match: + return_content = return_match.group(1) + if "...data()" not in return_content: + self._add_violation( + rule_id="JS-003", + rule_name="Alpine components must spread ...data()", + severity=Severity.ERROR, + file_path=file_path, + line_number=line_num, + message=f"Alpine component '{func_name}' does not inherit base layout data", + context=f"function {func_name}() {{ return {{ ... }}", + suggestion="Add '...data(),' as first property in return object to inherit dark mode, sidebar state, etc.", + ) + + def _check_fetch_vs_api_client(self, file_path: Path, content: str, lines: list[str]): + """JS-008: Check for raw fetch() calls that should use apiClient instead""" + # Skip init files and api-client itself + if "init-" in file_path.name or "api-client" in file_path.name: + return + + # Check for noqa comment + if "noqa: js-008" in content.lower(): + return + + for i, line in enumerate(lines, 1): + # Look for fetch( calls that hit API endpoints + if re.search(r"\bfetch\s*\(", line): + # Skip if it's a comment + stripped = line.strip() + if stripped.startswith("//") or stripped.startswith("/*"): + continue + + # Check if it's calling an API endpoint (contains /api/) + if "/api/" in line or "apiClient" in line: + # If using fetch with /api/, should use apiClient instead + if "/api/" in line and "apiClient" not in line: + self._add_violation( + rule_id="JS-008", + rule_name="Use apiClient for API calls", + severity=Severity.ERROR, + file_path=file_path, + line_number=i, + message="Raw fetch() call to API endpoint - use apiClient for automatic auth", + context=stripped[:80], + suggestion="Replace fetch('/api/...') with apiClient.get('/endpoint') or apiClient.post('/endpoint', data)", + ) + + def _check_alpine_current_page(self, file_path: Path, content: str, lines: list[str]): + """JS-004: Check that Alpine components set currentPage identifier""" + # Skip utility/init files + skip_patterns = ["init-", "api-client", "log-config", "utils", "helpers"] + if any(pattern in file_path.name for pattern in skip_patterns): + return + + if "noqa: js-004" in content.lower(): + return + + # Look for Alpine component function pattern + component_pattern = re.compile( + r"function\s+(admin\w+|vendor\w+|shop\w+)\s*\(\s*\)\s*\{", + re.IGNORECASE + ) + + for match in component_pattern.finditer(content): + func_name = match.group(1) + func_start = match.start() + line_num = content[:func_start].count('\n') + 1 + + # Check if currentPage is set in the return object + search_region = content[func_start:func_start + 800] + if "return {" in search_region: + return_match = re.search(r"return\s*\{([^}]{0,500})", search_region, re.DOTALL) + if return_match: + return_content = return_match.group(1) + if "currentPage:" not in return_content and "currentPage :" not in return_content: + self._add_violation( + rule_id="JS-004", + rule_name="Alpine components must set currentPage", + severity=Severity.WARNING, + file_path=file_path, + line_number=line_num, + message=f"Alpine component '{func_name}' does not set currentPage identifier", + context=f"function {func_name}()", + suggestion="Add 'currentPage: \"page-name\",' in return object for sidebar highlighting", + ) + + def _check_init_guard(self, file_path: Path, content: str, lines: list[str]): + """JS-005: Check that init methods have duplicate initialization guards""" + # Skip utility/init files + skip_patterns = ["init-", "api-client", "log-config", "utils", "helpers"] + if any(pattern in file_path.name for pattern in skip_patterns): + return + + if "noqa: js-005" in content.lower(): + return + + # Look for init() methods in Alpine components + init_pattern = re.compile(r"async\s+init\s*\(\s*\)\s*\{|init\s*\(\s*\)\s*\{") + + for match in init_pattern.finditer(content): + init_start = match.start() + line_num = content[:init_start].count('\n') + 1 + + # Check next 200 chars for initialization guard pattern + search_region = content[init_start:init_start + 300] + guard_patterns = [ + "window._", + "if (this._initialized)", + "if (window.", + "_initialized", + ] + + has_guard = any(pattern in search_region for pattern in guard_patterns) + if not has_guard: + self._add_violation( + rule_id="JS-005", + rule_name="Initialization methods must include guard", + severity=Severity.WARNING, + file_path=file_path, + line_number=line_num, + message="init() method lacks duplicate initialization guard", + context="init() { ... }", + suggestion="Add guard: if (window._pageInitialized) return; window._pageInitialized = true;", + ) + return # Only report once per file + + def _check_async_error_handling(self, file_path: Path, content: str, lines: list[str]): + """JS-006: Check that async operations have try/catch error handling""" + # Skip utility/init files + skip_patterns = ["init-", "api-client", "log-config"] + if any(pattern in file_path.name for pattern in skip_patterns): + return + + if "noqa: js-006" in content.lower(): + return + + # Look for async functions/methods + async_pattern = re.compile(r"async\s+\w+\s*\([^)]*\)\s*\{") + + for match in async_pattern.finditer(content): + func_start = match.start() + line_num = content[:func_start].count('\n') + 1 + + # Find the function body (look for matching braces) + # Simplified: check next 500 chars for try/catch + search_region = content[func_start:func_start + 800] + + # Check if there's await without try/catch + if "await " in search_region and "try {" not in search_region and "try{" not in search_region: + # Check if it's a simple one-liner or has error handling elsewhere + if ".catch(" not in search_region: + self._add_violation( + rule_id="JS-006", + rule_name="Async operations must have error handling", + severity=Severity.WARNING, + file_path=file_path, + line_number=line_num, + message="Async function with await lacks try/catch error handling", + context=match.group(0)[:50], + suggestion="Wrap await calls in try/catch with proper error logging", + ) + return # Only report once per file + + def _check_loading_state(self, file_path: Path, content: str, lines: list[str]): + """JS-007: Check that async operations manage loading state""" + # Skip utility/init files + skip_patterns = ["init-", "api-client", "log-config", "utils"] + if any(pattern in file_path.name for pattern in skip_patterns): + return + + if "noqa: js-007" in content.lower(): + return + + # Look for Alpine component functions that have async methods with API calls + component_pattern = re.compile( + r"function\s+(admin\w+|vendor\w+|shop\w+)\s*\(\s*\)\s*\{", + re.IGNORECASE + ) + + for match in component_pattern.finditer(content): + func_start = match.start() + # Get the component body (rough extraction) + component_region = content[func_start:func_start + 5000] + + # Check if component has API calls but no loading state + has_api_calls = "apiClient." in component_region or "await " in component_region + has_loading_state = "loading:" in component_region or "loading :" in component_region + has_loading_assignment = "this.loading = " in component_region or "loading = true" in component_region + + if has_api_calls and not has_loading_state: + line_num = content[:func_start].count('\n') + 1 + self._add_violation( + rule_id="JS-007", + rule_name="Set loading state for async operations", + severity=Severity.INFO, + file_path=file_path, + line_number=line_num, + message="Component has API calls but no loading state property", + context=match.group(1), + suggestion="Add 'loading: false,' to component state and set it before/after API calls", + ) + return + def _validate_html_file(self, file_path: Path, content: str, lines: list[str]): """Validate a single HTML template file""" print("📄 Validating template...") @@ -384,6 +696,11 @@ class ArchitectureValidator: # Skip components showcase page is_components_page = "components.html" in file_path.name + # Determine template type + is_admin = "/admin/" in file_path_str or "\\admin\\" in file_path_str + is_vendor = "/vendor/" in file_path_str or "\\vendor\\" in file_path_str + is_shop = "/shop/" in file_path_str or "\\shop\\" in file_path_str + if is_base_or_partial: print("⏭️ Skipping base/partial template") elif is_macro: @@ -401,47 +718,225 @@ class ArchitectureValidator: if not is_components_page and not is_macro: self._check_number_stepper_macro_usage(file_path, content, lines) - # Only check admin templates for extends - if "/admin/" not in file_path_str and "\\admin\\" not in file_path_str: - return + # TPL-004: Check x-text usage for dynamic content + self._check_xtext_usage(file_path, content, lines) + + # TPL-005: Check x-html safety + self._check_xhtml_safety(file_path, content, lines) + + # TPL-006: Check loading state implementation + self._check_template_loading_state(file_path, content, lines) + + # TPL-007: Check empty state implementation + self._check_template_empty_state(file_path, content, lines) if is_base_or_partial: return # Check for standalone marker in template (first 5 lines) - # Supports: {# standalone #}, {# noqa: TPL-001 #}, first_lines = "\n".join(lines[:5]).lower() - if "standalone" in first_lines or "noqa: tpl-001" in first_lines: + if "standalone" in first_lines or "noqa: tpl-00" in first_lines: print("⏭️ Template marked as standalone, skipping extends check") return - # Check exclusion patterns for TPL-001 - # These are templates that intentionally don't extend admin/base.html - tpl_001_exclusions = [ - "login.html", # Standalone login page - "errors/", # Error pages extend errors/base.html - "test-", # Test templates - ] - for exclusion in tpl_001_exclusions: + # TPL-001: Admin templates extends check + if is_admin: + self._check_template_extends( + file_path, lines, "admin/base.html", "TPL-001", + ["login.html", "errors/", "test-"] + ) + + # TPL-002: Vendor templates extends check + if is_vendor: + self._check_template_extends( + file_path, lines, "vendor/base.html", "TPL-002", + ["login.html", "errors/", "test-"] + ) + + # TPL-003: Shop templates extends check + if is_shop: + self._check_template_extends( + file_path, lines, "shop/base.html", "TPL-003", + ["errors/", "test-"] + ) + + def _check_template_extends( + self, file_path: Path, lines: list[str], + base_template: str, rule_id: str, exclusions: list[str] + ): + """Check that template extends the appropriate base template""" + file_path_str = str(file_path) + + # Check exclusion patterns + for exclusion in exclusions: if exclusion in file_path_str: print(f"⏭️ Template matches exclusion pattern '{exclusion}', skipping") return - # TPL-001: Check for extends + # Check for extends has_extends = any( - "{% extends" in line and "admin/base.html" in line for line in lines + "{% extends" in line and base_template in line for line in lines ) if not has_extends: + template_type = base_template.split("/")[0] self._add_violation( - rule_id="TPL-001", - rule_name="Templates must extend base", + rule_id=rule_id, + rule_name=f"{template_type.title()} templates must extend base", severity=Severity.ERROR, file_path=file_path, line_number=1, - message="Admin template does not extend admin/base.html", + message=f"{template_type.title()} template does not extend {base_template}", context=file_path.name, - suggestion="Add {% extends 'admin/base.html' %} at the top, or add {# standalone #} if intentional", + suggestion=f"Add {{% extends '{base_template}' %}} at the top, or add {{# standalone #}} if intentional", + ) + + def _check_xtext_usage(self, file_path: Path, content: str, lines: list[str]): + """TPL-004: Check that x-text is used for dynamic text content""" + if "noqa: tpl-004" in content.lower(): + return + + # Look for potentially unsafe patterns where user data might be interpolated + # This is a heuristic check - not perfect + unsafe_patterns = [ + (r'\{\{\s*\w+\.name\s*\}\}', "User data interpolation without x-text"), + (r'\{\{\s*\w+\.title\s*\}\}', "User data interpolation without x-text"), + (r'\{\{\s*\w+\.description\s*\}\}', "User data interpolation without x-text"), + ] + + for i, line in enumerate(lines, 1): + # Skip Jinja2 template syntax (server-side safe) + if "{%" in line: + continue + + for pattern, message in unsafe_patterns: + if re.search(pattern, line): + # Check if it's in an x-text context already + if 'x-text="' not in line and "x-text='" not in line: + self._add_violation( + rule_id="TPL-004", + rule_name="Use x-text for dynamic text content", + severity=Severity.INFO, + file_path=file_path, + line_number=i, + message=message, + context=line.strip()[:60], + suggestion='Use x-text="item.name" instead of {{ item.name }} for XSS safety', + ) + return # Only report once per file + + def _check_xhtml_safety(self, file_path: Path, content: str, lines: list[str]): + """TPL-005: Check that x-html is only used for safe content""" + if "noqa: tpl-005" in content.lower(): + return + + safe_xhtml_patterns = [ + r'x-html="\$icon\(', # Icon helper is safe + r"x-html='\$icon\(", + r'x-html="`\$\{.*icon', # Template literal with icon + ] + + for i, line in enumerate(lines, 1): + if "x-html=" in line: + # Check if it matches safe patterns + is_safe = any(re.search(pattern, line) for pattern in safe_xhtml_patterns) + + if not is_safe: + # Check for potentially unsafe user content + unsafe_indicators = [ + ".description", ".content", ".body", ".text", + ".message", ".comment", ".review", ".html" + ] + for indicator in unsafe_indicators: + if indicator in line: + self._add_violation( + rule_id="TPL-005", + rule_name="Use x-html ONLY for safe content", + severity=Severity.WARNING, + file_path=file_path, + line_number=i, + message="x-html used with potentially unsafe user content", + context=line.strip()[:60], + suggestion="Sanitize HTML content or use x-text for plain text", + ) + return + + def _check_template_loading_state(self, file_path: Path, content: str, lines: list[str]): + """TPL-006: Check that templates with data loads show loading state""" + if "noqa: tpl-006" in content.lower(): + return + + # Check if template has data loading (Alpine init, fetch, apiClient) + has_data_loading = any([ + "x-init=" in content, + "@load=" in content, + "apiClient" in content, + "loadData" in content, + "fetchData" in content, + ]) + + if not has_data_loading: + return + + # Check for loading state display + has_loading_state = any([ + 'x-show="loading"' in content, + "x-show='loading'" in content, + 'x-if="loading"' in content, + "x-if='loading'" in content, + 'loading_state' in content, + 'Loading...' in content, + 'spinner' in content.lower(), + ]) + + if not has_loading_state: + self._add_violation( + rule_id="TPL-006", + rule_name="Implement loading state for data loads", + severity=Severity.INFO, + file_path=file_path, + line_number=1, + message="Template loads data but has no visible loading state", + context=file_path.name, + suggestion='Add
Loading...
or use loading_state macro', + ) + + def _check_template_empty_state(self, file_path: Path, content: str, lines: list[str]): + """TPL-007: Check that templates with lists show empty state""" + if "noqa: tpl-007" in content.lower(): + return + + # Check if template has list iteration + has_list = any([ + "x-for=" in content, + "{% for " in content, + ]) + + if not has_list: + return + + # Check for empty state handling + has_empty_state = any([ + ".length === 0" in content, + ".length == 0" in content, + "items.length" in content, + "empty_state" in content, + "No items" in content, + "No results" in content, + "No data" in content, + "table_empty_state" in content, + ]) + + if not has_empty_state: + self._add_violation( + rule_id="TPL-007", + rule_name="Implement empty state when no data", + severity=Severity.INFO, + file_path=file_path, + line_number=1, + message="Template has list but no empty state handling", + context=file_path.name, + suggestion='Add ', ) def _check_pagination_macro_usage(self, file_path: Path, content: str, lines: list[str]): @@ -765,6 +1260,9 @@ class ArchitectureValidator: # API-004: Check authentication self._check_endpoint_authentication(file_path, content, lines) + # API-005: Check vendor_id scoping for vendor/shop endpoints + self._check_vendor_scoping(file_path, content, lines) + def _check_pydantic_usage(self, file_path: Path, content: str, lines: list[str]): """API-001: Ensure endpoints use Pydantic models""" rule = self._get_rule("API-001") @@ -985,8 +1483,47 @@ class ArchitectureValidator: suggestion=suggestion, ) + def _check_vendor_scoping(self, file_path: Path, content: str, lines: list[str]): + """API-005: Check that vendor/shop endpoints scope queries to vendor_id""" + file_path_str = str(file_path) + + # Only check vendor and shop API files + if "/vendor/" not in file_path_str and "/shop/" not in file_path_str: + return + + # Skip auth files + if file_path_str.endswith("auth.py"): + return + + # Check for noqa + if "noqa: api-005" in content.lower(): + return + + # Look for database queries without vendor_id filtering + # This is a heuristic check - not perfect + for i, line in enumerate(lines, 1): + # Look for .query().all() patterns without vendor filtering + if ".query(" in line and ".all()" in line: + # Check if vendor_id filter is nearby + context_start = max(0, i - 5) + context_end = min(len(lines), i + 3) + context_lines = "\n".join(lines[context_start:context_end]) + + if "vendor_id" not in context_lines and "token_vendor_id" not in context_lines: + self._add_violation( + rule_id="API-005", + rule_name="Multi-tenant queries must scope to vendor_id", + severity=Severity.WARNING, + file_path=file_path, + line_number=i, + message="Query in vendor/shop endpoint may not be scoped to vendor_id", + context=line.strip()[:60], + suggestion="Add .filter(Model.vendor_id == vendor_id) to ensure tenant isolation", + ) + return # Only report once per file + def _validate_service_layer(self, target_path: Path): - """Validate service layer rules (SVC-001, SVC-002, SVC-003, SVC-004, SVC-006)""" + """Validate service layer rules (SVC-001 to SVC-007)""" print("🔧 Validating service layer...") service_files = list(target_path.glob("app/services/**/*.py")) @@ -1008,9 +1545,56 @@ class ArchitectureValidator: # SVC-003: DB session as parameter self._check_db_session_parameter(file_path, content, lines) + # SVC-005: Vendor scoping in multi-tenant services + self._check_service_vendor_scoping(file_path, content, lines) + # SVC-006: No db.commit() in services self._check_no_commit_in_services(file_path, content, lines) + def _check_service_vendor_scoping(self, file_path: Path, content: str, lines: list[str]): + """SVC-005: Check that service queries are scoped to vendor_id in multi-tenant context""" + # Skip admin services that may legitimately access all vendors + file_path_str = str(file_path) + if "admin" in file_path_str.lower(): + return + + if "noqa: svc-005" in content.lower(): + return + + # Look for patterns that suggest unscoped queries + for i, line in enumerate(lines, 1): + # Check for .all() queries that might not be scoped + if ".query(" in line and ".all()" in line: + # Check context for vendor filtering + context_start = max(0, i - 5) + context_end = min(len(lines), i + 3) + context_lines = "\n".join(lines[context_start:context_end]) + + if "vendor_id" not in context_lines: + # Check if the method has vendor_id as parameter + method_start = self._find_method_start(lines, i) + if method_start: + method_sig = lines[method_start] + if "vendor_id" not in method_sig: + self._add_violation( + rule_id="SVC-005", + rule_name="Service must scope queries to vendor_id", + severity=Severity.INFO, + file_path=file_path, + line_number=i, + message="Query may not be scoped to vendor_id", + context=line.strip()[:60], + suggestion="Add vendor_id parameter and filter queries by it", + ) + return + + def _find_method_start(self, lines: list[str], current_line: int) -> int | None: + """Find the start line of the method containing current_line""" + for i in range(current_line - 1, max(0, current_line - 30), -1): + if "def " in lines[i]: + return i + return None + def _check_no_http_exception_in_services( self, file_path: Path, content: str, lines: list[str] ): @@ -1126,21 +1710,38 @@ class ArchitectureValidator: ) def _validate_models(self, target_path: Path): - """Validate model rules""" + """Validate model rules (MDL-001 to MDL-004)""" print("📦 Validating models...") - model_files = list(target_path.glob("app/models/**/*.py")) - self.result.files_checked += len(model_files) + # Validate database models + db_model_files = list(target_path.glob("models/database/**/*.py")) + self.result.files_checked += len(db_model_files) - # Basic validation - can be extended - for file_path in model_files: - if self._should_ignore_file(file_path): + for file_path in db_model_files: + if self._should_ignore_file(file_path) or file_path.name == "__init__.py": continue content = file_path.read_text() lines = content.split("\n") - # Check for mixing SQLAlchemy and Pydantic + # MDL-001: Check for SQLAlchemy Base inheritance + self._check_sqlalchemy_base_usage(file_path, content, lines) + + # MDL-004: Check table naming (singular) + self._check_table_naming(file_path, content) + + # Validate schema models + schema_model_files = list(target_path.glob("models/schema/**/*.py")) + self.result.files_checked += len(schema_model_files) + + for file_path in schema_model_files: + if self._should_ignore_file(file_path) or file_path.name == "__init__.py": + continue + + content = file_path.read_text() + lines = content.split("\n") + + # MDL-002: Check for mixing SQLAlchemy and Pydantic for i, line in enumerate(lines, 1): if re.search(r"class.*\(Base.*,.*BaseModel.*\)", line): self._add_violation( @@ -1154,6 +1755,168 @@ class ArchitectureValidator: suggestion="Keep SQLAlchemy models and Pydantic models separate", ) + # MDL-003: Check for from_attributes in Pydantic response models + self._check_pydantic_from_attributes(file_path, content) + + def _check_sqlalchemy_base_usage(self, file_path: Path, content: str, lines: list[str]): + """MDL-001: Check that database models inherit from SQLAlchemy Base""" + # Look for class definitions + class_pattern = re.compile(r"class\s+(\w+)\s*\(([^)]+)\):") + + for match in class_pattern.finditer(content): + class_name = match.group(1) + base_classes = match.group(2) + line_num = content[:match.start()].count('\n') + 1 + + # Skip non-model classes (mixins, helpers, etc.) + if class_name.endswith("Mixin") or class_name.startswith("_"): + continue + + # Skip Enum classes - they inherit from (str, Enum) or (Enum) + if "Enum" in base_classes: + continue + + # Check if it inherits from Base + if "Base" not in base_classes and "db.Model" not in base_classes: + # Check if it might be a model - MUST have __tablename__ in its own class body + class_body_start = match.end() + # Find the end of this class body (next class definition at same indentation or EOF) + next_class_match = class_pattern.search(content, class_body_start) + if next_class_match: + class_body = content[class_body_start:next_class_match.start()] + else: + class_body = content[class_body_start:] + + # Only flag if this specific class has __tablename__ (definite model indicator) + if "__tablename__" in class_body: + self._add_violation( + rule_id="MDL-001", + rule_name="Database models must use SQLAlchemy Base", + severity=Severity.ERROR, + file_path=file_path, + line_number=line_num, + message=f"Model '{class_name}' appears to be a database model but doesn't inherit from Base", + context=f"class {class_name}({base_classes})", + suggestion="Inherit from Base: class MyModel(Base):", + ) + + def _check_table_naming(self, file_path: Path, content: str): + """MDL-004: Check that table names are plural (industry standard) + + Following Rails, Django, Laravel conventions - tables represent collections + of entities so should use plural names: users, orders, products. + """ + # Look for __tablename__ definitions + tablename_pattern = re.compile(r'__tablename__\s*=\s*["\'](\w+)["\']') + + for match in tablename_pattern.finditer(content): + table_name = match.group(1) + line_num = content[:match.start()].count('\n') + 1 + + # Common singular names that should be plural + singular_indicators = { + "vendor": "vendors", + "product": "products", + "order": "orders", + "user": "users", + "customer": "customers", + "category": "categories", + "payment": "payments", + "setting": "settings", + "item": "items", + "image": "images", + "role": "roles", + "company": "companies", + } + + if table_name in singular_indicators: + plural = singular_indicators[table_name] + self._add_violation( + rule_id="MDL-004", + rule_name="Database tables use plural names", + severity=Severity.WARNING, + file_path=file_path, + line_number=line_num, + message=f"Table name '{table_name}' should be plural (industry standard)", + context=f'__tablename__ = "{table_name}"', + suggestion=f'Use __tablename__ = "{plural}"', + ) + + def _check_pydantic_from_attributes(self, file_path: Path, content: str): + """MDL-003: Check that Pydantic response models have from_attributes""" + # Look for response model classes that likely represent ORM entities + # Skip stats/aggregate models which use plain dicts + response_pattern = re.compile( + r"class\s+(\w*Response\w*|\w*Out\w*|\w*Read\w*)\s*\(([^)]+)\):" + ) + + for match in response_pattern.finditer(content): + class_name = match.group(1) + base_classes = match.group(2) + class_start = match.end() + line_num = content[:match.start()].count('\n') + 1 + + # Skip stats/aggregate response models - they use plain dicts, not ORM objects + stats_patterns = ["Stats", "Analytics", "Dashboard", "Summary", "Count", "Total"] + if any(p in class_name for p in stats_patterns): + continue + + # Get class body - find the next class definition to properly scope + next_class = re.search(r"\nclass\s+\w+", content[class_start:]) + if next_class: + class_body = content[class_start:class_start + next_class.start()] + else: + class_body = content[class_start:class_start + 800] + + # Check for from_attributes in Config or model_config + # Supports multiple formats: + # - model_config = {"from_attributes": True} + # - model_config = ConfigDict(from_attributes=True) + # - class Config: from_attributes = True + # - orm_mode = True (Pydantic v1) + has_from_attributes = any([ + "from_attributes" in class_body and "True" in class_body, + "orm_mode" in class_body and "True" in class_body, + ]) + + if not has_from_attributes: + # Only flag if it looks like an ORM entity response: + # - Has an 'id: int' field as a direct attribute (typical ORM primary key) + # - AND has at least one other typical ORM field (created_at, updated_at, etc.) + # Skip wrapper/list responses that just contain other models + has_id_field = re.search(r"^\s+id\s*:\s*int", class_body, re.MULTILINE) + has_timestamp = "created_at:" in class_body or "updated_at:" in class_body + + # Skip list/wrapper responses (contain 'list[' or just have message/total fields) + is_wrapper = any([ + ": list[" in class_body and "id:" not in class_body.split(": list[")[0], + re.search(r"^\s+items\s*:", class_body, re.MULTILINE), + re.search(r"^\s+total\s*:", class_body, re.MULTILINE) and not has_id_field, + ]) + + # Only flag if it has id AND timestamps, or id and multiple other ORM-like fields + looks_like_orm = has_id_field and (has_timestamp or ( + # Has multiple fields suggesting ORM entity + sum([ + "vendor_id:" in class_body, + "user_id:" in class_body, + "is_active:" in class_body, + "email:" in class_body, + ]) >= 2 + )) + + if "BaseModel" in base_classes and looks_like_orm and not is_wrapper: + self._add_violation( + rule_id="MDL-003", + rule_name="Pydantic response models must use from_attributes", + severity=Severity.WARNING, + file_path=file_path, + line_number=line_num, + message=f"Response model '{class_name}' may need from_attributes for ORM compatibility", + context=f"class {class_name}({base_classes})", + suggestion="Add 'model_config = ConfigDict(from_attributes=True)' or 'class Config: from_attributes = True'", + ) + def _validate_exceptions(self, target_path: Path): """Validate exception handling patterns""" print("⚠️ Validating exception handling...") @@ -1181,6 +1944,360 @@ class ArchitectureValidator: suggestion="Specify exception type: except ValueError: or except Exception:", ) + # EXC-004: Check exception inheritance in exceptions module + exception_files = list(target_path.glob("app/exceptions/**/*.py")) + for file_path in exception_files: + if "__init__" in file_path.name or "handler" in file_path.name: + continue + content = file_path.read_text() + self._check_exception_inheritance(file_path, content) + + # EXC-005: Check exception handler registration in main.py + main_py = target_path / "app" / "main.py" + if main_py.exists(): + content = main_py.read_text() + if "setup_exception_handlers" not in content and "exception_handler" not in content: + self._add_violation( + rule_id="EXC-005", + rule_name="Exception handler must be registered", + severity=Severity.ERROR, + file_path=main_py, + line_number=1, + message="No exception handler registration found in main.py", + context="app/main.py", + suggestion="Add setup_exception_handlers(app) or register exception handlers", + ) + + def _check_exception_inheritance(self, file_path: Path, content: str): + """EXC-004: Check that custom exceptions inherit from WizamartException""" + # Look for class definitions that look like exceptions + exception_pattern = re.compile(r"class\s+(\w+Exception|\w+Error)\s*\(([^)]+)\)") + + for match in exception_pattern.finditer(content): + class_name = match.group(1) + base_classes = match.group(2) + line_num = content[:match.start()].count('\n') + 1 + + # Check if it inherits from appropriate base + valid_bases = [ + "WizamartException", "ResourceNotFoundException", + "ValidationException", "AuthenticationException", + "AuthorizationException", "ConflictException", + "Exception", # Allow base Exception for some cases + ] + + has_valid_base = any(base in base_classes for base in valid_bases) + if not has_valid_base: + self._add_violation( + rule_id="EXC-004", + rule_name="Domain exceptions must inherit from WizamartException", + severity=Severity.WARNING, + file_path=file_path, + line_number=line_num, + message=f"Exception {class_name} should inherit from WizamartException hierarchy", + context=f"class {class_name}({base_classes})", + suggestion="Inherit from WizamartException or one of its subclasses", + ) + + def _validate_naming_conventions(self, target_path: Path): + """Validate naming convention rules (NAM-001 to NAM-005)""" + print("📛 Validating naming conventions...") + + # NAM-001: API files use PLURAL names + api_files = list(target_path.glob("app/api/v1/**/*.py")) + for file_path in api_files: + if file_path.name in ["__init__.py", "auth.py", "health.py"]: + continue + self._check_api_file_naming(file_path) + + # NAM-002: Service files use SINGULAR + 'service' suffix + service_files = list(target_path.glob("app/services/**/*.py")) + for file_path in service_files: + if file_path.name == "__init__.py": + continue + self._check_service_file_naming(file_path) + + # NAM-003: Model files use SINGULAR names + model_files = list(target_path.glob("models/**/*.py")) + for file_path in model_files: + if file_path.name == "__init__.py": + continue + self._check_model_file_naming(file_path) + + # NAM-004 & NAM-005: Check terminology consistency + py_files = list(target_path.glob("app/**/*.py")) + for file_path in py_files: + if self._should_ignore_file(file_path): + continue + content = file_path.read_text() + self._check_terminology_consistency(file_path, content) + + def _check_api_file_naming(self, file_path: Path): + """NAM-001: Check that API files use plural names""" + name = file_path.stem + + # Common singular forms that should be plural + singular_to_plural = { + "vendor": "vendors", + "product": "products", + "order": "orders", + "user": "users", + "category": "categories", + "import": "imports", + "export": "exports", + "customer": "customers", + "cart": "carts", + "payment": "payments", + "setting": "settings", + } + + if name in singular_to_plural: + self._add_violation( + rule_id="NAM-001", + rule_name="API files use PLURAL names", + severity=Severity.WARNING, + file_path=file_path, + line_number=1, + message=f"API file '{name}.py' should use plural form", + context=file_path.name, + suggestion=f"Rename to '{singular_to_plural[name]}.py'", + ) + + def _check_service_file_naming(self, file_path: Path): + """NAM-002: Check that service files use singular + _service suffix""" + name = file_path.stem + + # Check for _service suffix + if not name.endswith("_service"): + self._add_violation( + rule_id="NAM-002", + rule_name="Service files use SINGULAR + 'service' suffix", + severity=Severity.WARNING, + file_path=file_path, + line_number=1, + message=f"Service file '{name}.py' should end with '_service'", + context=file_path.name, + suggestion=f"Rename to '{name}_service.py' or ensure it's a service file", + ) + return + + # Check if the base name is plural (should be singular) + base_name = name.replace("_service", "") + plurals = ["vendors", "products", "orders", "users", "categories", "customers", "payments"] + if base_name in plurals: + singular = base_name.rstrip("s") + if base_name == "categories": + singular = "category" + self._add_violation( + rule_id="NAM-002", + rule_name="Service files use SINGULAR + 'service' suffix", + severity=Severity.WARNING, + file_path=file_path, + line_number=1, + message=f"Service file '{name}.py' should use singular form", + context=file_path.name, + suggestion=f"Rename to '{singular}_service.py'", + ) + + def _check_model_file_naming(self, file_path: Path): + """NAM-003: Check that model files use singular names""" + name = file_path.stem + + # Common plural forms that should be singular + plural_to_singular = { + "vendors": "vendor", + "products": "product", + "orders": "order", + "users": "user", + "categories": "category", + "customers": "customer", + "payments": "payment", + "settings": "setting", + } + + if name in plural_to_singular: + self._add_violation( + rule_id="NAM-003", + rule_name="Model files use SINGULAR names", + severity=Severity.WARNING, + file_path=file_path, + line_number=1, + message=f"Model file '{name}.py' should use singular form", + context=file_path.name, + suggestion=f"Rename to '{plural_to_singular[name]}.py'", + ) + + def _check_terminology_consistency(self, file_path: Path, content: str): + """NAM-004 & NAM-005: Check for inconsistent terminology""" + lines = content.split("\n") + + # NAM-004: Check for 'shop_id' (should be vendor_id) + # Skip shop-specific files where shop_id might be legitimate + if "/shop/" not in str(file_path): + for i, line in enumerate(lines, 1): + if "shop_id" in line and "# noqa" not in line.lower(): + # Avoid false positives for legitimate shop context + if "shop_service" not in str(file_path): + self._add_violation( + rule_id="NAM-004", + rule_name="Use 'vendor' not 'shop'", + severity=Severity.INFO, + file_path=file_path, + line_number=i, + message="Consider using 'vendor_id' instead of 'shop_id'", + context=line.strip()[:60], + suggestion="Use vendor_id for multi-tenant context", + ) + return # Only report once per file + + # NAM-005: Check for 'stock' (should be inventory) + for i, line in enumerate(lines, 1): + if "stock_service" in line or "stock_id" in line: + if "# noqa" not in line.lower(): + self._add_violation( + rule_id="NAM-005", + rule_name="Use 'inventory' not 'stock'", + severity=Severity.INFO, + file_path=file_path, + line_number=i, + message="Consider using 'inventory' instead of 'stock'", + context=line.strip()[:60], + suggestion="Use inventory_service or inventory_id for consistency", + ) + return + + def _validate_auth_patterns(self, target_path: Path): + """Validate authentication patterns (AUTH-001 to AUTH-004)""" + print("🔐 Validating auth patterns...") + + # AUTH-003: Check password hashing in auth service + auth_service = target_path / "app" / "services" / "auth_service.py" + if auth_service.exists(): + content = auth_service.read_text() + if "bcrypt" not in content and "hash" not in content.lower(): + self._add_violation( + rule_id="AUTH-003", + rule_name="Never store plain passwords", + severity=Severity.ERROR, + file_path=auth_service, + line_number=1, + message="Auth service may not be hashing passwords", + context="auth_service.py", + suggestion="Use bcrypt or similar library to hash passwords before storing", + ) + + # AUTH-004: Check vendor context patterns + vendor_api_files = list(target_path.glob("app/api/v1/vendor/**/*.py")) + for file_path in vendor_api_files: + if file_path.name == "__init__.py" or file_path.name == "auth.py": + continue + content = file_path.read_text() + self._check_vendor_context_pattern(file_path, content) + + shop_api_files = list(target_path.glob("app/api/v1/shop/**/*.py")) + for file_path in shop_api_files: + if file_path.name == "__init__.py" or file_path.name == "auth.py": + continue + content = file_path.read_text() + self._check_shop_context_pattern(file_path, content) + + def _check_vendor_context_pattern(self, file_path: Path, content: str): + """AUTH-004: Check that vendor API endpoints use token-based vendor context""" + if "noqa: auth-004" in content.lower(): + return + + # Vendor APIs should NOT use require_vendor_context() - that's for shop + if "require_vendor_context()" in content: + lines = content.split("\n") + for i, line in enumerate(lines, 1): + if "require_vendor_context()" in line: + self._add_violation( + rule_id="AUTH-004", + rule_name="Use correct vendor context pattern", + severity=Severity.WARNING, + file_path=file_path, + line_number=i, + message="Vendor API should use token_vendor_id, not require_vendor_context()", + context=line.strip()[:60], + suggestion="Use current_user.token_vendor_id from JWT token instead", + ) + return + + def _check_shop_context_pattern(self, file_path: Path, content: str): + """AUTH-004: Check that shop API endpoints use proper vendor context""" + if "noqa: auth-004" in content.lower(): + return + + # Shop APIs that need vendor context should use require_vendor_context or # public + has_vendor_context = "require_vendor_context" in content or "# public" in content + + # Check for routes that might need vendor context + if "@router." in content and not has_vendor_context: + # Only flag if there are non-public endpoints without vendor context + lines = content.split("\n") + for i, line in enumerate(lines, 1): + if "@router." in line: + # Check next few lines for public marker or vendor context + context_lines = "\n".join(lines[i-1:i+10]) + if "# public" not in context_lines and "require_vendor_context" not in context_lines: + self._add_violation( + rule_id="AUTH-004", + rule_name="Shop endpoints need vendor context", + severity=Severity.INFO, + file_path=file_path, + line_number=i, + message="Shop endpoint may need vendor context dependency", + context=line.strip()[:60], + suggestion="Add Depends(require_vendor_context()) or mark as '# public'", + ) + return + + def _validate_middleware(self, target_path: Path): + """Validate middleware patterns (MDW-001, MDW-002)""" + print("🔌 Validating middleware...") + + middleware_dir = target_path / "middleware" + if not middleware_dir.exists(): + return + + middleware_files = list(middleware_dir.glob("*.py")) + + for file_path in middleware_files: + if file_path.name == "__init__.py": + continue + + # MDW-001: Check naming convention + if "_middleware" in file_path.stem: + self._add_violation( + rule_id="MDW-001", + rule_name="Middleware uses simple noun naming", + severity=Severity.WARNING, + file_path=file_path, + line_number=1, + message="Middleware file should use simple noun naming", + context=file_path.name, + suggestion=f"Rename to '{file_path.stem.replace('_middleware', '')}.py'", + ) + + # MDW-002: Check vendor context middleware exists and sets proper state + vendor_context = middleware_dir / "vendor_context.py" + if vendor_context.exists(): + content = vendor_context.read_text() + # The middleware can set either request.state.vendor (full object) + # or request.state.vendor_id - vendor object is preferred as it allows + # accessing vendor_id via request.state.vendor.id + if "request.state.vendor" not in content: + self._add_violation( + rule_id="MDW-002", + rule_name="Vendor context middleware must set state", + severity=Severity.ERROR, + file_path=vendor_context, + line_number=1, + message="Vendor context middleware should set request.state.vendor", + context="vendor_context.py", + suggestion="Add 'request.state.vendor = vendor' in the middleware", + ) + def _validate_javascript(self, target_path: Path): """Validate JavaScript patterns""" print("🟨 Validating JavaScript...") @@ -1402,6 +2519,7 @@ class ArchitectureValidator: # Group by severity errors = [v for v in self.result.violations if v.severity == Severity.ERROR] warnings = [v for v in self.result.violations if v.severity == Severity.WARNING] + infos = [v for v in self.result.violations if v.severity == Severity.INFO] if errors: print(f"\n❌ ERRORS ({len(errors)}):") @@ -1415,6 +2533,12 @@ class ArchitectureValidator: for violation in warnings: self._print_violation(violation) + if infos and self.verbose: + print(f"\nℹ️ INFO ({len(infos)}):") + print("-" * 80) + for violation in infos: + self._print_violation(violation) + # Summary print("\n" + "=" * 80) if self.result.has_errors():