diff --git a/apps/kitchen_mate/frontend/src/api/clip.ts b/apps/kitchen_mate/frontend/src/api/clip.ts index 3511a0f..c012eaa 100644 --- a/apps/kitchen_mate/frontend/src/api/clip.ts +++ b/apps/kitchen_mate/frontend/src/api/clip.ts @@ -8,6 +8,7 @@ import { ApiError, isAuthorizationError, getErrorMessage, + AuthorizationErrorDetail, } from "../types/recipe"; const API_BASE = "/api"; @@ -57,16 +58,18 @@ export async function clipRecipe(url: string, forceLlm = false): Promise if (!response.ok) { const error: ApiError = await response.json(); if (isAuthorizationError(error.detail)) { + const detail = error.detail as AuthorizationErrorDetail; throw new ClipError( - error.detail.message, + detail.message, response.status, - error.detail.error_code, - error.detail.feature + detail.error_code, + detail.feature ); } - const message = - typeof error.detail === "string" ? error.detail : "Failed to clip recipe"; - throw new ClipError(message, response.status); + throw new ClipError( + getErrorMessage(error.detail, "Failed to clip recipe"), + response.status + ); } const data: ClipResponse = await response.json(); @@ -86,18 +89,18 @@ export async function uploadRecipe(file: File): Promise { if (!response.ok) { const error: ApiError = await response.json(); if (isAuthorizationError(error.detail)) { + const detail = error.detail as AuthorizationErrorDetail; throw new ClipError( - error.detail.message, + detail.message, response.status, - error.detail.error_code, - error.detail.feature + detail.error_code, + detail.feature ); } - const message = - typeof error.detail === "string" - ? error.detail - : "Failed to upload recipe"; - throw new ClipError(message, response.status); + throw new ClipError( + getErrorMessage(error.detail, "Failed to upload recipe"), + response.status + ); } return response.json(); diff --git a/apps/kitchen_mate/frontend/src/types/recipe.ts b/apps/kitchen_mate/frontend/src/types/recipe.ts index 6564359..cb56c11 100644 --- a/apps/kitchen_mate/frontend/src/types/recipe.ts +++ b/apps/kitchen_mate/frontend/src/types/recipe.ts @@ -59,7 +59,7 @@ export interface ConvertRequest { } export interface ApiError { - detail: string | AuthorizationErrorDetail; + detail: unknown; } export interface AuthorizationErrorDetail { @@ -82,18 +82,24 @@ export function isAuthorizationError( } /** - * Extract error message from ApiError.detail, handling both string and object formats. + * Extract error message from ApiError.detail, handling string, authorization error, + * and structured error objects (e.g. {message, url, error}). */ -export function getErrorMessage( - detail: string | AuthorizationErrorDetail, - fallback: string -): string { +export function getErrorMessage(detail: unknown, fallback: string): string { if (typeof detail === "string") { return detail; } if (isAuthorizationError(detail)) { return detail.message; } + if ( + typeof detail === "object" && + detail !== null && + "message" in detail && + typeof (detail as Record).message === "string" + ) { + return (detail as Record).message as string; + } return fallback; } diff --git a/apps/kitchen_mate/src/kitchen_mate/database/__init__.py b/apps/kitchen_mate/src/kitchen_mate/database/__init__.py index aaf5b7b..0a3725e 100644 --- a/apps/kitchen_mate/src/kitchen_mate/database/__init__.py +++ b/apps/kitchen_mate/src/kitchen_mate/database/__init__.py @@ -13,6 +13,7 @@ ) from kitchen_mate.database.models import ( Base, + ClipRequestLogModel, KitchenInviteModel, KitchenMemberModel, KitchenModel, @@ -39,6 +40,7 @@ get_user_recipe_with_lineage, get_user_recipes, hash_content, + log_clip_request, revoke_share, save_user_recipe, store_recipe, @@ -58,6 +60,7 @@ "get_session_factory", # Models "Base", + "ClipRequestLogModel", "RecipeModel", "UserRecipeModel", "UserModel", @@ -91,4 +94,5 @@ "get_share_for_user_recipe", "revoke_share", "get_user_recipe_by_id_no_auth", + "log_clip_request", ] diff --git a/apps/kitchen_mate/src/kitchen_mate/database/models.py b/apps/kitchen_mate/src/kitchen_mate/database/models.py index 75e9d26..5773417 100644 --- a/apps/kitchen_mate/src/kitchen_mate/database/models.py +++ b/apps/kitchen_mate/src/kitchen_mate/database/models.py @@ -177,3 +177,23 @@ class KitchenRecipeModel(Base): Index("uq_kitchen_recipe", "kitchen_id", "user_recipe_id", unique=True), Index("idx_kitchen_recipes_kitchen_id", "kitchen_id"), ) + + +class ClipRequestLogModel(Base): + """Log of every /api/clip request for usage tracking.""" + + __tablename__ = "clip_request_logs" + + id: Mapped[str] = mapped_column(String(36), primary_key=True) + user_id: Mapped[str | None] = mapped_column(Text, nullable=True) + ip_address: Mapped[str | None] = mapped_column(Text, nullable=True) + requested_at: Mapped[datetime] = mapped_column(DateTime, nullable=False) + method: Mapped[str | None] = mapped_column(Text, nullable=True) + succeeded: Mapped[bool] = mapped_column(Boolean, nullable=False) + error_detail: Mapped[str | None] = mapped_column(Text, nullable=True) # JSON blob + + __table_args__ = ( + Index("idx_clip_request_logs_user_id", "user_id"), + Index("idx_clip_request_logs_requested_at", "requested_at"), + Index("idx_clip_request_logs_user_requested", "user_id", "requested_at"), + ) diff --git a/apps/kitchen_mate/src/kitchen_mate/database/repositories.py b/apps/kitchen_mate/src/kitchen_mate/database/repositories.py index f64dec8..95aa5ed 100644 --- a/apps/kitchen_mate/src/kitchen_mate/database/repositories.py +++ b/apps/kitchen_mate/src/kitchen_mate/database/repositories.py @@ -15,7 +15,13 @@ from recipe_clipper.models import Recipe from kitchen_mate.database.engine import get_session -from kitchen_mate.database.models import RecipeModel, RecipeShareModel, UserModel, UserRecipeModel +from kitchen_mate.database.models import ( + ClipRequestLogModel, + RecipeModel, + RecipeShareModel, + UserModel, + UserRecipeModel, +) from kitchen_mate.schemas import Parser @@ -846,3 +852,30 @@ async def get_user_recipe_by_id_no_auth(user_recipe_id: str) -> UserRecipe | Non if row is None: return None return _user_recipe_model_to_schema(row) + + +# ============================================================================= +# Clip Request Log Functions +# ============================================================================= + + +async def log_clip_request( + user_id: str | None, + ip_address: str | None, + requested_at: datetime, + method: str | None, + succeeded: bool, + error_detail: dict | None = None, +) -> None: + """Record a single /api/clip request for usage tracking.""" + async with get_session() as session: + model = ClipRequestLogModel( + id=str(uuid.uuid4()), + user_id=user_id, + ip_address=ip_address, + requested_at=requested_at, + method=method, + succeeded=succeeded, + error_detail=json.dumps(error_detail) if error_detail is not None else None, + ) + session.add(model) diff --git a/apps/kitchen_mate/src/kitchen_mate/routes/clip.py b/apps/kitchen_mate/src/kitchen_mate/routes/clip.py index 1aead5d..8ef18cb 100644 --- a/apps/kitchen_mate/src/kitchen_mate/routes/clip.py +++ b/apps/kitchen_mate/src/kitchen_mate/routes/clip.py @@ -4,9 +4,10 @@ import asyncio import logging +from datetime import datetime from typing import Annotated -from fastapi import APIRouter, Depends, File, HTTPException, UploadFile +from fastapi import APIRouter, Depends, File, HTTPException, Request, UploadFile from recipe_clipper.exceptions import ( LLMError, @@ -17,7 +18,7 @@ ) from recipe_clipper.models import Recipe -from kitchen_mate.auth import User +from kitchen_mate.auth import User, get_current_user_optional from kitchen_mate.authorization import ( Permission, TierInfo, @@ -27,7 +28,7 @@ require_permission, ) from kitchen_mate.config import Settings, get_settings -from kitchen_mate.database import get_cached_recipe, store_recipe, update_recipe +from kitchen_mate.database import get_cached_recipe, log_clip_request, store_recipe, update_recipe from kitchen_mate.extraction import LLMNotAllowedError, extract_recipe from kitchen_mate.files import FileValidationError, process_upload, save_to_temp_file from kitchen_mate.schemas import ClipRequest, ClipResponse, ClipUploadResponse, FileInfo, Parser @@ -38,11 +39,21 @@ router = APIRouter() +def _get_client_ip(request: Request) -> str | None: + """Extract the real client IP, checking X-Forwarded-For for proxy setups.""" + forwarded_for = request.headers.get("X-Forwarded-For") + if forwarded_for: + return forwarded_for.split(",")[0].strip() + return request.client.host if request.client else None + + @router.post("/clip") async def clip_recipe( clip_request: ClipRequest, + request: Request, settings: Annotated[Settings, Depends(get_settings)], tier_info: Annotated[TierInfo, Depends(get_tier_info)], + user: Annotated[User | None, Depends(get_current_user_optional)], ) -> ClipResponse: """Extract a recipe from a URL.""" url = str(clip_request.url) @@ -55,11 +66,19 @@ async def clip_recipe( if clip_request.force_llm and not can_use_ai: raise UpgradeRequiredError(feature=Permission.CLIP_AI.value) + requested_at = datetime.now() + ip_address = _get_client_ip(request) + method: str | None = None + succeeded: bool = False + error_detail: dict | None = None + try: # Try cache first (unless force_refresh) if settings.database.enabled and not clip_request.force_refresh: cached = await _get_from_cache(url, clip_request.force_llm) if cached: + method = "cache" + succeeded = True return ClipResponse(recipe=cached.recipe, cached=True) # Extract the recipe @@ -72,26 +91,53 @@ async def clip_recipe( force_llm=clip_request.force_llm, check_content_changed=clip_request.force_refresh and settings.database.enabled, ) + method = parsed_with.value # Cache the result if settings.database.enabled: await _save_to_cache(url, recipe, content_hash, parsed_with) + succeeded = True return ClipResponse(recipe=recipe, cached=False, content_changed=content_changed) except RecipeNotFoundError as error: + error_detail = {"status_code": 404, "message": "No recipe found at the requested url"} + raise HTTPException(status_code=404, detail=error_detail["message"]) from error + except NetworkError as error: + logger.error("Failed to fetch recipe URL '%s': %s", url, error) + error_detail = { + "status_code": 502, + "message": "Failed to fetch recipe. The site may be unavailable or blocking access.", + } raise HTTPException( - status_code=404, detail="No recipe found at the requested url" + status_code=502, + detail={**error_detail, "url": url, "error": str(error)}, ) from error - except NetworkError as error: - raise HTTPException(status_code=502, detail="Failed to fetch URL") from error except LLMNotAllowedError as error: + error_detail = {"status_code": 403, "message": "LLM extraction not permitted for this user"} raise UpgradeRequiredError(feature=Permission.CLIP_AI.value) from error - except (RecipeParsingError, LLMError) as error: - raise HTTPException(status_code=500, detail="Failed to parse recipe") from error - except RecipeClipperError as error: + except (RecipeParsingError, LLMError, RecipeClipperError) as error: + error_detail = {"status_code": 500, "message": "Failed to parse recipe"} raise HTTPException(status_code=500, detail="Failed to parse recipe") from error + finally: + if settings.database.enabled: + try: + await log_clip_request( + user_id=user.id if user is not None else None, + ip_address=ip_address, + requested_at=requested_at, + method=method, + succeeded=succeeded, + error_detail=error_detail, + ) + except Exception: + logger.warning( + "Failed to log clip request for user_id=%s", + user.id if user is not None else None, + exc_info=True, + ) + async def _get_from_cache(url: str, force_llm: bool): """Try to get a recipe from cache.""" diff --git a/apps/kitchen_mate/tests/conftest.py b/apps/kitchen_mate/tests/conftest.py index 3645a3b..406b01a 100644 --- a/apps/kitchen_mate/tests/conftest.py +++ b/apps/kitchen_mate/tests/conftest.py @@ -83,6 +83,18 @@ def settings_pro_tier(client: TestClient) -> Generator[None, None, None]: app.dependency_overrides.clear() +@pytest.fixture +def client_with_db() -> Generator[TestClient, None, None]: + """Test client with database enabled (for request tracking tests).""" + test_settings = Settings( + _env_file=None, cache_enabled=True, supabase_jwt_secret=None, supabase_url=None + ) + app.dependency_overrides[get_settings] = lambda: test_settings + with TestClient(app) as test_client: + yield test_client + app.dependency_overrides.clear() + + @pytest.fixture def settings_with_supabase(client: TestClient) -> Generator[Settings, None, None]: """Override settings with Supabase configuration for HS256 JWT verification.""" diff --git a/apps/kitchen_mate/tests/test_clip.py b/apps/kitchen_mate/tests/test_clip.py index 535f68f..6c73880 100644 --- a/apps/kitchen_mate/tests/test_clip.py +++ b/apps/kitchen_mate/tests/test_clip.py @@ -76,7 +76,8 @@ def test_clip_recipe_network_error(client: TestClient) -> None: ) assert response.status_code == 502 - assert "Failed to fetch URL" in response.json()["detail"] + detail = response.json()["detail"] + assert "Failed to fetch recipe" in detail["message"] def test_clip_recipe_llm_fallback_without_api_key( @@ -316,3 +317,184 @@ def test_clip_recipe_unauthenticated_force_llm_blocked( detail = response.json()["detail"] assert detail["error_code"] == "upgrade_required" assert detail["feature"] == "clip_ai" + + +# ============================================================================= +# Request tracking tests +# ============================================================================= + + +def test_clip_does_not_log_when_database_disabled(client: TestClient) -> None: + """When database is disabled, log_clip_request is never called.""" + from unittest.mock import AsyncMock + + mock_recipe = Recipe( + title="Test Recipe", + ingredients=[], + instructions=["Step 1"], + source_url="https://example.com/recipe", + ) + mock_response = MagicMock() + mock_response.content = "test" + + with patch("kitchen_mate.extraction.fetch_url", return_value=mock_response): + with patch("kitchen_mate.extraction.parse_with_recipe_scrapers", return_value=mock_recipe): + with patch( + "kitchen_mate.routes.clip.log_clip_request", + new_callable=AsyncMock, + ) as mock_log: + response = client.post( + "/api/clip", + json={"url": "https://example.com/recipe", "use_llm_fallback": False}, + ) + + assert response.status_code == 200 + mock_log.assert_not_called() + + +def test_clip_logs_successful_request(client_with_db: TestClient) -> None: + """Successful extraction logs user_id, method='recipe_scrapers', succeeded=True.""" + from unittest.mock import AsyncMock + + mock_recipe = Recipe( + title="Test Recipe", + ingredients=[], + instructions=["Step 1"], + source_url="https://example.com/recipe", + ) + mock_response = MagicMock() + mock_response.content = "test" + + with patch("kitchen_mate.extraction.fetch_url", return_value=mock_response): + with patch("kitchen_mate.extraction.parse_with_recipe_scrapers", return_value=mock_recipe): + with patch("kitchen_mate.routes.clip.get_cached_recipe", return_value=None): + with patch("kitchen_mate.routes.clip.store_recipe", return_value=MagicMock()): + with patch( + "kitchen_mate.routes.clip.log_clip_request", + new_callable=AsyncMock, + ) as mock_log: + response = client_with_db.post( + "/api/clip", + json={"url": "https://example.com/recipe", "use_llm_fallback": False}, + ) + + assert response.status_code == 200 + mock_log.assert_awaited_once() + kwargs = mock_log.call_args.kwargs + assert kwargs["user_id"] is None + assert kwargs["method"] == "recipe_scrapers" + assert kwargs["succeeded"] is True + assert kwargs["error_detail"] is None + assert kwargs["requested_at"] is not None + assert kwargs["ip_address"] is not None + + +def test_clip_logs_cache_hit(client_with_db: TestClient) -> None: + """Cache hit logs method='cache' and succeeded=True.""" + from unittest.mock import AsyncMock + + cached = MagicMock() + cached.recipe = Recipe( + title="Cached Recipe", + ingredients=[], + instructions=["Step 1"], + source_url="https://example.com/recipe", + ) + + with patch("kitchen_mate.database.get_cached_recipe", return_value=cached): + with patch( + "kitchen_mate.routes.clip.log_clip_request", + new_callable=AsyncMock, + ) as mock_log: + response = client_with_db.post( + "/api/clip", + json={"url": "https://example.com/recipe", "use_llm_fallback": False}, + ) + + assert response.status_code == 200 + assert response.json()["cached"] is True + mock_log.assert_awaited_once() + kwargs = mock_log.call_args.kwargs + assert kwargs["method"] == "cache" + assert kwargs["succeeded"] is True + assert kwargs["error_detail"] is None + + +def test_clip_logs_failed_request(client_with_db: TestClient) -> None: + """Network failure logs succeeded=False, method=None, and error_detail with status_code.""" + from unittest.mock import AsyncMock + + from recipe_clipper.exceptions import NetworkError + + with patch( + "kitchen_mate.extraction.fetch_url", + side_effect=NetworkError("Connection failed"), + ): + with patch( + "kitchen_mate.routes.clip.log_clip_request", + new_callable=AsyncMock, + ) as mock_log: + with patch("kitchen_mate.routes.clip.get_cached_recipe", return_value=None): + response = client_with_db.post( + "/api/clip", + json={"url": "https://example.com/recipe", "use_llm_fallback": False}, + ) + + assert response.status_code == 502 + mock_log.assert_awaited_once() + kwargs = mock_log.call_args.kwargs + assert kwargs["method"] is None + assert kwargs["succeeded"] is False + assert kwargs["error_detail"]["status_code"] == 502 + + +def test_clip_does_not_log_upgrade_required_error( + client_with_db: TestClient, settings_free_tier: None +) -> None: + """UpgradeRequiredError (force_llm without permission) must not log.""" + from unittest.mock import AsyncMock + + with patch( + "kitchen_mate.routes.clip.log_clip_request", + new_callable=AsyncMock, + ) as mock_log: + response = client_with_db.post( + "/api/clip", + json={"url": "https://example.com/recipe", "force_llm": True}, + ) + + assert response.status_code == 403 + mock_log.assert_not_called() + + +def test_clip_logging_failure_does_not_affect_response(client_with_db: TestClient) -> None: + """If log_clip_request raises, the HTTP response is unaffected.""" + from unittest.mock import AsyncMock + + mock_recipe = Recipe( + title="Test Recipe", + ingredients=[], + instructions=["Step 1"], + source_url="https://example.com/recipe", + ) + mock_response = MagicMock() + mock_response.content = "test" + + with patch("kitchen_mate.extraction.fetch_url", return_value=mock_response): + with patch("kitchen_mate.extraction.parse_with_recipe_scrapers", return_value=mock_recipe): + with patch("kitchen_mate.database.get_cached_recipe", return_value=None): + with patch("kitchen_mate.database.store_recipe", return_value=MagicMock()): + with patch( + "kitchen_mate.routes.clip.log_clip_request", + new_callable=AsyncMock, + side_effect=RuntimeError("DB is down"), + ): + response = client_with_db.post( + "/api/clip", + json={ + "url": "https://example.com/recipe", + "use_llm_fallback": False, + }, + ) + + assert response.status_code == 200