From d947fa5ca04fd1fee48ab79b3dec53527632b838 Mon Sep 17 00:00:00 2001 From: Samir Boulahtit Date: Tue, 18 Nov 2025 23:32:07 +0100 Subject: [PATCH] removing legacy code on path_rewrite_middleware --- docs/architecture/middleware.md | 66 +++------- docs/architecture/multi-tenant.md | 8 +- docs/architecture/overview.md | 23 ++-- docs/architecture/request-flow.md | 24 ++-- docs/architecture/url-routing/overview.md | 52 ++++---- docs/backend/middleware-reference.md | 51 ++++---- docs/index.md | 13 +- middleware/path_rewrite_middleware.py | 63 ---------- .../test_theme_logging_path_decorators.py | 116 +----------------- 9 files changed, 114 insertions(+), 302 deletions(-) delete mode 100644 middleware/path_rewrite_middleware.py diff --git a/docs/architecture/middleware.md b/docs/architecture/middleware.md index dc8d9fa5..4882a7f9 100644 --- a/docs/architecture/middleware.md +++ b/docs/architecture/middleware.md @@ -64,28 +64,9 @@ Injects: request.state.vendor = **See**: [Multi-Tenant System](multi-tenant.md) for routing modes -### 3. Path Rewrite Middleware +**Note on Path-Based Routing:** Previous implementations used a `PathRewriteMiddleware` to rewrite paths at runtime. This has been replaced with **double router mounting** in `main.py`, where shop routes are registered twice with different prefixes (`/shop` and `/vendors/{vendor_code}/shop`). This approach is simpler and uses FastAPI's native routing capabilities. -**Purpose**: Rewrite request paths for proper FastAPI routing - -**What it does**: -- Uses the `clean_path` extracted by VendorContextMiddleware -- Rewrites `request.scope['path']` to remove vendor prefix -- Allows FastAPI routes to match correctly - -**Example** (Path-Based Development Mode): -``` -Original path: /vendors/WIZAMART/shop/products -Clean path: /shop/products (set by VendorContextMiddleware) -↓ -Path Rewrite Middleware changes request path to: /shop/products -↓ -FastAPI router can now match: @app.get("/shop/products") -``` - -**Why it's needed**: FastAPI routes don't include vendor prefix, so we strip it - -### 4. Context Detection Middleware +### 3. Context Detection Middleware **Purpose**: Determine the type/context of the request @@ -115,7 +96,7 @@ else: **Why it's useful**: Error handlers and templates adapt based on context -### 5. Theme Context Middleware +### 4. Theme Context Middleware **Purpose**: Load vendor-specific theme settings @@ -146,13 +127,12 @@ else: graph TD A[Client Request] --> B[1. LoggingMiddleware] B --> C[2. VendorContextMiddleware] - C --> D[3. PathRewriteMiddleware] - D --> E[4. ContextDetectionMiddleware] - E --> F[5. ThemeContextMiddleware] - F --> G[6. FastAPI Router] - G --> H[Route Handler] - H --> I[Response] - I --> J[Client] + C --> D[3. ContextDetectionMiddleware] + D --> E[4. ThemeContextMiddleware] + E --> F[5. FastAPI Router] + F --> G[Route Handler] + G --> H[Response] + H --> I[Client] ``` ### Why This Order Matters @@ -164,25 +144,21 @@ graph TD - Must log errors from all other middleware 2. **VendorContextMiddleware second** - - Must run before PathRewriteMiddleware (provides clean_path) - - Must run before ContextDetectionMiddleware (provides vendor) + - Must run before ContextDetectionMiddleware (provides vendor and clean_path) - Must run before ThemeContextMiddleware (provides vendor_id) -3. **PathRewriteMiddleware third** - - Depends on clean_path from VendorContextMiddleware - - Must run before ContextDetectionMiddleware (rewrites path) - -4. **ContextDetectionMiddleware fourth** +3. **ContextDetectionMiddleware third** - Uses clean_path from VendorContextMiddleware - - Uses rewritten path from PathRewriteMiddleware - Provides context_type for ThemeContextMiddleware -5. **ThemeContextMiddleware last** +4. **ThemeContextMiddleware last** - Depends on vendor from VendorContextMiddleware - Depends on context_type from ContextDetectionMiddleware **Breaking this order will break the application!** +**Note:** Path-based routing (e.g., `/vendors/{code}/shop/*`) is handled by double router mounting in `main.py`, not by middleware. + ## Request State Variables Middleware components inject these variables into `request.state`: @@ -191,7 +167,7 @@ Middleware components inject these variables into `request.state`: |----------|--------|------|---------|-------------| | `vendor` | VendorContextMiddleware | Vendor | Theme, Templates | Current vendor object | | `vendor_id` | VendorContextMiddleware | int | Queries, Theme | Current vendor ID | -| `clean_path` | VendorContextMiddleware | str | PathRewrite, Context | Path without vendor prefix | +| `clean_path` | VendorContextMiddleware | str | Context | Path without vendor prefix (for context detection) | | `context_type` | ContextDetectionMiddleware | RequestContext | Theme, Error handlers | Request context enum | | `theme` | ThemeContextMiddleware | dict | Templates | Vendor theme config | @@ -260,25 +236,21 @@ async def get_products(request: Request): ↓ Sets: request.state.vendor_id = 1 ↓ Sets: request.state.clean_path = "/shop/products" -3. PathRewriteMiddleware - ↓ Path already clean (no rewrite needed for subdomain mode) - ↓ request.scope['path'] = "/shop/products" - -4. ContextDetectionMiddleware +3. ContextDetectionMiddleware ↓ Analyzes path: "/shop/products" ↓ Has vendor: Yes ↓ Not admin/api/vendor dashboard ↓ Sets: request.state.context_type = RequestContext.SHOP -5. ThemeContextMiddleware +4. ThemeContextMiddleware ↓ Loads theme for vendor_id = 1 ↓ Sets: request.state.theme = {...theme config...} -6. FastAPI Router +5. FastAPI Router ↓ Matches route: @app.get("/shop/products") ↓ Calls handler function -7. Route Handler +6. Route Handler ↓ Accesses: request.state.vendor_id ↓ Queries: products WHERE vendor_id = 1 ↓ Renders template with vendor data diff --git a/docs/architecture/multi-tenant.md b/docs/architecture/multi-tenant.md index e8b4243a..a6307828 100644 --- a/docs/architecture/multi-tenant.md +++ b/docs/architecture/multi-tenant.md @@ -452,10 +452,12 @@ Host: myplatform.com - Sets: request.state.vendor = - Sets: request.state.clean_path = "/shop/products" -2. PathRewriteMiddleware - - Rewrites: request.scope['path'] = "/shop/products" +2. FastAPI Router + - Routes registered with prefix: /vendors/{vendor_code}/shop + - Matches: /vendors/WIZAMART/shop/products + - vendor_code path parameter = "WIZAMART" -3-4. Same as previous examples +3-4. Same as previous examples (Context, Theme middleware) ``` ## Testing Multi-Tenancy diff --git a/docs/architecture/overview.md b/docs/architecture/overview.md index 2168a884..428578bf 100644 --- a/docs/architecture/overview.md +++ b/docs/architecture/overview.md @@ -82,20 +82,19 @@ Custom middleware handles: graph TB A[Client Request] --> B[Logging Middleware] B --> C[Vendor Context Middleware] - C --> D[Path Rewrite Middleware] - D --> E[Context Detection Middleware] - E --> F[Theme Context Middleware] - F --> G{Request Type?} + C --> D[Context Detection Middleware] + D --> E[Theme Context Middleware] + E --> F{Request Type?} - G -->|API /api/*| H[API Router] - G -->|Admin /admin/*| I[Admin Page Router] - G -->|Vendor /vendor/*| J[Vendor Page Router] - G -->|Shop /shop/*| K[Shop Page Router] + F -->|API /api/*| G[API Router] + F -->|Admin /admin/*| H[Admin Page Router] + F -->|Vendor /vendor/*| I[Vendor Page Router] + F -->|Shop /shop/*| J[Shop Page Router] - H --> L[JSON Response] - I --> M[Admin HTML] - J --> N[Vendor HTML] - K --> O[Shop HTML] + G --> K[JSON Response] + H --> L[Admin HTML] + I --> M[Vendor HTML] + J --> N[Shop HTML] ``` **See:** [Request Flow](request-flow.md) for detailed journey diff --git a/docs/architecture/request-flow.md b/docs/architecture/request-flow.md index 081c5113..651b7d3f 100644 --- a/docs/architecture/request-flow.md +++ b/docs/architecture/request-flow.md @@ -112,25 +112,29 @@ request.state.vendor_id = 1 request.state.clean_path = "/shop/products" ``` -### 4. PathRewriteMiddleware +### 4. Router Matching (FastAPI Native) **What happens**: -- Checks if `clean_path` is different from original path -- If different, rewrites the request path for FastAPI routing +- FastAPI matches the request path against registered routes +- For path-based development mode, routes are registered with two prefixes: + - `/shop/*` for subdomain/custom domain + - `/vendors/{vendor_code}/shop/*` for path-based development **Example** (Path-Based Mode): ```python -# Input (path-based development mode) -original_path = "/vendors/WIZAMART/shop/products" -clean_path = "/shop/products" # From VendorContextMiddleware +# In main.py - Double router mounting +app.include_router(shop_pages.router, prefix="/shop") +app.include_router(shop_pages.router, prefix="/vendors/{vendor_code}/shop") -# Path rewrite -if clean_path != original_path: - request.scope['path'] = clean_path - request._url = request.url.replace(path=clean_path) +# Request: /vendors/WIZAMART/shop/products +# Matches: Second router (/vendors/{vendor_code}/shop) +# Route: @router.get("/products") +# vendor_code available as path parameter = "WIZAMART" ``` +**Note:** Previous implementations used `PathRewriteMiddleware` to rewrite paths. This has been replaced with FastAPI's native routing via double router mounting. + **Request State After**: No changes to state, but internal path updated ### 5. ContextDetectionMiddleware diff --git a/docs/architecture/url-routing/overview.md b/docs/architecture/url-routing/overview.md index 68e91d22..a2d96b62 100644 --- a/docs/architecture/url-routing/overview.md +++ b/docs/architecture/url-routing/overview.md @@ -345,40 +345,38 @@ In Jinja2 template: --- -## Potential Issue: Path-Based Development Mode +## Path-Based Routing Implementation -⚠️ **Current Implementation Gap:** +**Current Solution: Double Router Mounting** -The `vendor_context_middleware` sets `clean_path` for path-based URLs, but this isn't used for FastAPI routing. - -**Problem:** -- Incoming: `GET http://localhost:8000/vendors/acme/shop/products` -- Routes registered: `@router.get("/shop/products")` -- FastAPI tries to match `/vendors/acme/shop/products` against `/shop/products` -- Result: ❌ 404 Not Found - -**Solution (Recommended):** - -Add a path rewriting middleware in `main.py`: +The application handles path-based routing by registering shop routes **twice** with different prefixes: ```python -async def path_rewrite_middleware(request: Request, call_next): - """Rewrite path for path-based vendor routing in development mode.""" - if hasattr(request.state, 'clean_path'): - # Replace request path for FastAPI routing - request._url = request._url.replace(path=request.state.clean_path) - return await call_next(request) - -# In main.py, add after vendor_context_middleware: -app.middleware("http")(path_rewrite_middleware) -``` - -Or alternatively, mount the router twice: -```python +# In main.py app.include_router(shop_pages.router, prefix="/shop") -app.include_router(shop_pages.router, prefix="/vendors/{vendor_code}/shop") # Path-based development mode +app.include_router(shop_pages.router, prefix="/vendors/{vendor_code}/shop") ``` +**How This Works:** + +1. **For Subdomain/Custom Domain Mode:** + - URL: `https://acme.wizamart.com/shop/products` + - Matches: First router with `/shop` prefix + - Route: `@router.get("/products")` → Full path: `/shop/products` + +2. **For Path-Based Development Mode:** + - URL: `http://localhost:8000/vendors/acme/shop/products` + - Matches: Second router with `/vendors/{vendor_code}/shop` prefix + - Route: `@router.get("/products")` → Full path: `/vendors/{vendor_code}/shop/products` + - Bonus: `vendor_code` available as path parameter! + +**Benefits:** +- ✅ No middleware complexity or path manipulation +- ✅ FastAPI native routing +- ✅ Explicit and maintainable +- ✅ Vendor code accessible via path parameter when needed +- ✅ Both deployment modes supported cleanly + --- ## Authentication in Multi-Tenant Shop diff --git a/docs/backend/middleware-reference.md b/docs/backend/middleware-reference.md index b837251d..4fcdf502 100644 --- a/docs/backend/middleware-reference.md +++ b/docs/backend/middleware-reference.md @@ -200,23 +200,31 @@ Middleware for request/response logging and performance monitoring. --- -## Path Rewriting +## Path-Based Routing Solution -### path_rewrite_middleware +**Modern Approach: Double Router Mounting** -Middleware function that rewrites request paths for path-based vendor routing. +Instead of using middleware to rewrite paths, the application registers shop routes **twice** with different prefixes: -**Purpose:** -Allows `/vendors/VENDORCODE/shop/products` (path-based development mode) to be internally routed as `/shop/products` for proper FastAPI route matching. +```python +# In main.py +app.include_router(shop_pages.router, prefix="/shop") +app.include_router(shop_pages.router, prefix="/vendors/{vendor_code}/shop") +``` -**Execution Order:** -Must run AFTER VendorContextMiddleware and BEFORE ContextDetectionMiddleware. +**How It Works:** +- **Subdomain/Custom Domain Mode**: Routes match `/shop/*` prefix +- **Path-Based Development Mode**: Routes match `/vendors/{vendor_code}/shop/*` prefix +- FastAPI handles routing naturally without path manipulation +- Vendor code is available as a path parameter when needed -::: middleware.path_rewrite_middleware.path_rewrite_middleware - options: - show_source: true - heading_level: 4 - show_root_heading: false +**Benefits:** +- ✅ No middleware complexity +- ✅ Explicit route definitions +- ✅ FastAPI native routing +- ✅ Vendor code accessible via path parameter + +**Note:** Previous implementations used `path_rewrite_middleware` to rewrite paths at runtime. This approach has been deprecated in favor of double mounting, which is simpler and more maintainable. --- @@ -228,18 +236,19 @@ The middleware stack must be configured in the correct order for proper function graph TD A[Request] --> B[LoggingMiddleware] B --> C[VendorContextMiddleware] - C --> D[PathRewriteMiddleware] - D --> E[ContextMiddleware] - E --> F[ThemeContextMiddleware] - F --> G[Application Routes] - G --> H[Response] + C --> D[ContextMiddleware] + D --> E[ThemeContextMiddleware] + E --> F[Application Routes] + F --> G[Response] ``` **Critical Dependencies:** -1. **VendorContextMiddleware** must run first to detect vendor -2. **PathRewriteMiddleware** needs `clean_path` from VendorContext -3. **ContextMiddleware** needs rewritten path -4. **ThemeContextMiddleware** needs context type +1. **LoggingMiddleware** runs first for request timing +2. **VendorContextMiddleware** detects vendor and sets clean_path +3. **ContextMiddleware** detects context type (API/Admin/Vendor/Shop) +4. **ThemeContextMiddleware** loads vendor theme based on context + +**Note:** Path-based routing (e.g., `/vendors/{code}/shop/*`) is handled by double router mounting in `main.py`, not by middleware. --- diff --git a/docs/index.md b/docs/index.md index f4fba206..8efe62d6 100644 --- a/docs/index.md +++ b/docs/index.md @@ -289,13 +289,12 @@ Understanding how a request flows through Wizamart: graph LR A[Client Request] --> B[LoggingMiddleware] B --> C[VendorContextMiddleware] - C --> D[PathRewriteMiddleware] - D --> E[ContextDetectionMiddleware] - E --> F[ThemeContextMiddleware] - F --> G[FastAPI Router] - G --> H{Request Type} - H -->|API| I[JSON Response] - H -->|Page| J[HTML Template] + C --> D[ContextDetectionMiddleware] + D --> E[ThemeContextMiddleware] + E --> F[FastAPI Router] + F --> G{Request Type} + G -->|API| H[JSON Response] + G -->|Page| I[HTML Template] ``` **Learn more**: [Request Flow](architecture/request-flow.md) diff --git a/middleware/path_rewrite_middleware.py b/middleware/path_rewrite_middleware.py deleted file mode 100644 index 692702c0..00000000 --- a/middleware/path_rewrite_middleware.py +++ /dev/null @@ -1,63 +0,0 @@ -# middleware/path_rewrite_middleware.py -""" -Path Rewrite Middleware - -Rewrites request paths for path-based vendor routing. -This allows /vendor/VENDORCODE/shop/products to be routed as /shop/products - -MUST run AFTER vendor_context_middleware and BEFORE context_middleware. -""" -import logging -from fastapi import Request -from starlette.datastructures import URL - -logger = logging.getLogger(__name__) - - -async def path_rewrite_middleware(request: Request, call_next): - """ - Middleware to rewrite request paths for vendor context. - - If vendor_context_middleware set request.state.clean_path, this middleware - will rewrite the request path to use the clean path instead. - - This allows FastAPI route matching to work correctly with path-based routing. - - Example: - Original: /vendor/WIZAMART/shop/products - Clean path: /shop/products - After rewrite: Request is routed as if path was /shop/products - - MUST run after vendor_context_middleware (which sets clean_path) - MUST run before context_middleware (which needs to see the clean path) - """ - - # Check if vendor_context_middleware set a clean_path - if hasattr(request.state, 'clean_path'): - clean_path = request.state.clean_path - original_path = request.url.path - - # Only rewrite if clean_path is different from original path - if clean_path != original_path: - logger.debug( - f"[PATH_REWRITE] Rewriting path", - extra={ - "original_path": original_path, - "clean_path": clean_path, - "vendor": getattr(request.state, 'vendor', 'NOT SET'), - } - ) - - # Rewrite the path by modifying the request's scope - # This affects how FastAPI's router will see the path - request.scope['path'] = clean_path - - # Also update request._url to reflect the change - # This ensures request.url.path returns the rewritten path - old_url = request.url - new_url = old_url.replace(path=clean_path) - request._url = new_url - - # Continue to next middleware/handler - response = await call_next(request) - return response diff --git a/tests/unit/middleware/test_theme_logging_path_decorators.py b/tests/unit/middleware/test_theme_logging_path_decorators.py index a90d252a..48d976b8 100644 --- a/tests/unit/middleware/test_theme_logging_path_decorators.py +++ b/tests/unit/middleware/test_theme_logging_path_decorators.py @@ -1,17 +1,18 @@ # tests/unit/middleware/test_theme_logging_path_decorators.py """ -Comprehensive unit tests for remaining middleware components: +Comprehensive unit tests for middleware components: - ThemeContextMiddleware and ThemeContextManager - LoggingMiddleware -- path_rewrite_middleware - rate_limit decorator Tests cover: - Theme loading and caching - Request/response logging -- Path rewriting for vendor routing - Rate limit decorators - Edge cases and error handling + +Note: path_rewrite_middleware has been deprecated in favor of double router mounting. +See main.py for current implementation using app.include_router() with different prefixes. """ import pytest @@ -25,7 +26,6 @@ from middleware.theme_context import ( get_current_theme, ) from middleware.logging_middleware import LoggingMiddleware -from middleware.path_rewrite_middleware import path_rewrite_middleware from middleware.decorators import rate_limit from app.exceptions.base import RateLimitException @@ -351,98 +351,6 @@ class TestLoggingMiddleware: assert process_time >= 0.1 -# ============================================================================= -# Path Rewrite Middleware Tests -# ============================================================================= - -@pytest.mark.unit -class TestPathRewriteMiddleware: - """Test suite for path_rewrite_middleware.""" - - @pytest.mark.asyncio - async def test_rewrites_path_when_clean_path_different(self): - """Test path is rewritten when clean_path differs from original.""" - request = Mock(spec=Request) - request.url = Mock(path="/vendor/testvendor/shop/products") - request.state = Mock(clean_path="/shop/products") - request.scope = {"path": "/vendor/testvendor/shop/products"} - - call_next = AsyncMock(return_value=Mock()) - - await path_rewrite_middleware(request, call_next) - - # Path should be rewritten in scope - assert request.scope["path"] == "/shop/products" - call_next.assert_called_once_with(request) - - @pytest.mark.asyncio - async def test_does_not_rewrite_when_paths_same(self): - """Test path is not rewritten when clean_path same as original.""" - request = Mock(spec=Request) - original_path = "/shop/products" - request.url = Mock(path=original_path) - request.state = Mock(clean_path=original_path) - request.scope = {"path": original_path} - - call_next = AsyncMock(return_value=Mock()) - - await path_rewrite_middleware(request, call_next) - - # Path should remain unchanged - assert request.scope["path"] == original_path - call_next.assert_called_once() - - @pytest.mark.asyncio - async def test_does_nothing_when_no_clean_path(self): - """Test middleware does nothing when no clean_path set.""" - request = Mock(spec=Request) - request.url = Mock(path="/shop/products") - request.state = Mock(spec=[]) # No clean_path attribute - original_path = "/shop/products" - request.scope = {"path": original_path} - - call_next = AsyncMock(return_value=Mock()) - - await path_rewrite_middleware(request, call_next) - - # Path should remain unchanged - assert request.scope["path"] == original_path - call_next.assert_called_once() - - @pytest.mark.asyncio - async def test_updates_request_url(self): - """Test middleware updates request._url.""" - request = Mock(spec=Request) - original_url = Mock(path="/vendor/test/shop") - request.url = original_url - request.url.replace = Mock(return_value=Mock(path="/shop")) - request.state = Mock(clean_path="/shop") - request.scope = {"path": "/vendor/test/shop"} - - call_next = AsyncMock(return_value=Mock()) - - await path_rewrite_middleware(request, call_next) - - # URL replace should have been called - request.url.replace.assert_called_once_with(path="/shop") - - @pytest.mark.asyncio - async def test_preserves_vendor_context(self): - """Test middleware preserves vendor context in request.state.""" - request = Mock(spec=Request) - request.url = Mock(path="/vendor/testvendor/products") - mock_vendor = Mock() - request.state = Mock(clean_path="/products", vendor=mock_vendor) - request.scope = {"path": "/vendor/testvendor/products"} - - call_next = AsyncMock(return_value=Mock()) - - await path_rewrite_middleware(request, call_next) - - # Vendor should still be accessible - assert request.state.vendor is mock_vendor - - # ============================================================================= # Rate Limit Decorator Tests # ============================================================================= @@ -560,22 +468,6 @@ class TestMiddlewareEdgeCases: # Verify database was closed mock_db.close.assert_called_once() - @pytest.mark.asyncio - async def test_path_rewrite_with_query_parameters(self): - """Test path rewrite preserves query parameters.""" - request = Mock(spec=Request) - original_url = Mock(path="/vendor/test/shop?page=1") - request.url = original_url - request.url.replace = Mock(return_value=Mock(path="/shop?page=1")) - request.state = Mock(clean_path="/shop?page=1") - request.scope = {"path": "/vendor/test/shop?page=1"} - - call_next = AsyncMock(return_value=Mock()) - - await path_rewrite_middleware(request, call_next) - - request.url.replace.assert_called_once_with(path="/shop?page=1") - def test_theme_default_immutability(self): """Test that getting default theme doesn't share state.""" theme1 = ThemeContextManager.get_default_theme()