Repository navigation
fix dashboard revenue isolation and precision #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| # Python runtime artifacts produced by the bind-mounted backend container. | ||
| __pycache__/ | ||
| *.py[cod] | ||
| .pytest_cache/ | ||
| *.tsbuildinfo |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,25 +1,78 @@ | ||
| from fastapi import APIRouter, Depends, HTTPException | ||
| from typing import Dict, Any | ||
| from app.services.cache import get_revenue_summary | ||
| import logging | ||
| from typing import Any | ||
|
|
||
| from fastapi import APIRouter, Depends, HTTPException, Query, status | ||
| from sqlalchemy.exc import SQLAlchemyError | ||
|
|
||
| from app.core.auth import authenticate_request as get_current_user | ||
| from app.models.auth import AuthenticatedUser | ||
| from app.services.cache import get_revenue_summary | ||
| from app.services.reservations import PropertyNotFoundError, list_tenant_properties | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| router = APIRouter() | ||
|
|
||
|
|
||
| def _tenant_id(current_user: AuthenticatedUser) -> str: | ||
| """Require tenant context instead of silently using a shared fallback.""" | ||
| if not current_user.tenant_id: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="A tenant context is required for dashboard data", | ||
| ) | ||
| return current_user.tenant_id | ||
|
|
||
|
|
||
| @router.get("/dashboard/properties") | ||
| async def get_dashboard_properties( | ||
| current_user: AuthenticatedUser = Depends(get_current_user), | ||
| ) -> dict[str, list[dict[str, str]]]: | ||
| """List only the current tenant's selectable dashboard properties.""" | ||
| try: | ||
| properties = await list_tenant_properties(_tenant_id(current_user)) | ||
| return {"items": properties} | ||
| except SQLAlchemyError as error: | ||
| logger.exception("Unable to load dashboard properties: %s", error) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_503_SERVICE_UNAVAILABLE, | ||
| detail="Property data is temporarily unavailable", | ||
| ) from error | ||
|
|
||
|
|
||
| @router.get("/dashboard/summary") | ||
| async def get_dashboard_summary( | ||
| property_id: str, | ||
| current_user: dict = Depends(get_current_user) | ||
| ) -> Dict[str, Any]: | ||
|
|
||
| tenant_id = getattr(current_user, "tenant_id", "default_tenant") or "default_tenant" | ||
|
|
||
| revenue_data = await get_revenue_summary(property_id, tenant_id) | ||
|
|
||
| total_revenue_float = float(revenue_data['total']) | ||
|
|
||
| year: int = Query(default=2024, ge=2000, le=2100), | ||
| month: int = Query(default=3, ge=1, le=12), | ||
| current_user: AuthenticatedUser = Depends(get_current_user), | ||
| ) -> dict[str, Any]: | ||
| """Return exact monthly revenue for one tenant-owned property.""" | ||
| try: | ||
| revenue_data = await get_revenue_summary( | ||
| property_id, | ||
| _tenant_id(current_user), | ||
| year=year, | ||
| month=month, | ||
| ) | ||
| except PropertyNotFoundError as error: | ||
| # Do not reveal whether the property exists for another tenant. | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Property not found") from error | ||
| except SQLAlchemyError as error: | ||
| logger.exception("Unable to load dashboard revenue: %s", error) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_503_SERVICE_UNAVAILABLE, | ||
| detail="Revenue data is temporarily unavailable", | ||
| ) from error | ||
|
|
||
| return { | ||
| "property_id": revenue_data['property_id'], | ||
| "total_revenue": total_revenue_float, | ||
| "currency": revenue_data['currency'], | ||
| "reservations_count": revenue_data['count'] | ||
| "property_id": revenue_data["property_id"], | ||
| "property_name": revenue_data["property_name"], | ||
| "year": revenue_data["year"], | ||
| "month": revenue_data["month"], | ||
| "timezone": revenue_data["timezone"], | ||
| # A decimal string avoids introducing a binary floating-point value at | ||
| # the API boundary. | ||
| "total_revenue": revenue_data["total"], | ||
| "currency": revenue_data["currency"], | ||
| "reservations_count": revenue_data["count"], | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,60 +1,78 @@ | ||
| import asyncio | ||
| from sqlalchemy.ext.asyncio import create_async_engine, AsyncSession, async_sessionmaker | ||
| from sqlalchemy.pool import QueuePool | ||
| import logging | ||
|
|
||
| from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine | ||
|
|
||
| from ..config import settings | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| def _async_database_url(database_url: str) -> str: | ||
| """Return a SQLAlchemy URL that uses the asyncpg driver.""" | ||
| if database_url.startswith("postgresql+asyncpg://"): | ||
| return database_url | ||
| if database_url.startswith("postgresql://"): | ||
| return database_url.replace("postgresql://", "postgresql+asyncpg://", 1) | ||
| if database_url.startswith("postgres://"): | ||
| return database_url.replace("postgres://", "postgresql+asyncpg://", 1) | ||
| return database_url | ||
|
|
||
|
|
||
| class DatabasePool: | ||
| def __init__(self): | ||
| """Lazily initialized async SQLAlchemy session factory.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| self.engine = None | ||
| self.session_factory = None | ||
|
|
||
| async def initialize(self): | ||
| """Initialize database connection pool""" | ||
| try: | ||
| # Create async engine with connection pooling | ||
| database_url = f"postgresql+asyncpg://{settings.supabase_db_user}:{settings.supabase_db_password}@{settings.supabase_db_host}:{settings.supabase_db_port}/{settings.supabase_db_name}" | ||
|
|
||
| self.session_factory: async_sessionmaker[AsyncSession] | None = None | ||
| self._initialization_lock = asyncio.Lock() | ||
|
|
||
| async def initialize(self) -> None: | ||
| """Initialize the shared pool once using the configured DATABASE_URL.""" | ||
| if self.session_factory is not None: | ||
| return | ||
|
|
||
| async with self._initialization_lock: | ||
| if self.session_factory is not None: | ||
| return | ||
|
|
||
| database_url = _async_database_url(settings.database_url) | ||
| self.engine = create_async_engine( | ||
| database_url, | ||
| poolclass=QueuePool, | ||
| pool_size=20, # Number of connections to maintain | ||
| max_overflow=30, # Additional connections when needed | ||
| pool_pre_ping=True, # Validate connections | ||
| pool_recycle=3600, # Recycle connections every hour | ||
| echo=False # Set to True for SQL debugging | ||
| pool_size=settings.database_pool_size, | ||
| max_overflow=settings.database_max_overflow, | ||
| pool_timeout=settings.database_pool_timeout, | ||
| pool_pre_ping=True, | ||
| pool_recycle=settings.database_pool_recycle, | ||
| echo=False, | ||
| ) | ||
|
|
||
| self.session_factory = async_sessionmaker( | ||
| bind=self.engine, | ||
| class_=AsyncSession, | ||
| expire_on_commit=False | ||
| expire_on_commit=False, | ||
| ) | ||
|
|
||
| logger.info("✅ Database connection pool initialized") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"❌ Database pool initialization failed: {e}") | ||
| self.engine = None | ||
| self.session_factory = None | ||
|
|
||
| async def close(self): | ||
| """Close database connections""" | ||
| if self.engine: | ||
| await self.engine.dispose() | ||
|
|
||
| async def get_session(self) -> AsyncSession: | ||
| """Get database session from pool""" | ||
| if not self.session_factory: | ||
| raise Exception("Database pool not initialized") | ||
| logger.info("Database connection pool initialized") | ||
|
|
||
| async def close(self) -> None: | ||
| """Dispose of the database engine during application shutdown.""" | ||
| engine = self.engine | ||
| self.engine = None | ||
| self.session_factory = None | ||
| if engine is not None: | ||
|
Comment on lines
+57
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The Suggested FixThe Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
| await engine.dispose() | ||
|
|
||
| def get_session(self) -> AsyncSession: | ||
| """Create a session from the initialized factory.""" | ||
| if self.session_factory is None: | ||
| raise RuntimeError("Database pool has not been initialized") | ||
| return self.session_factory() | ||
|
|
||
| # Global database pool instance | ||
|
|
||
| # One shared pool prevents a new engine from being created for every request. | ||
| db_pool = DatabasePool() | ||
|
|
||
| async def get_db_session() -> AsyncSession: | ||
| """Dependency to get database session""" | ||
|
|
||
| async def get_db_session(): | ||
| """FastAPI dependency that yields an initialized database session.""" | ||
| await db_pool.initialize() | ||
| async with db_pool.get_session() as session: | ||
| yield session | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,55 @@ | ||
| import json | ||
| import redis.asyncio as redis | ||
| from typing import Dict, Any | ||
| import logging | ||
| import os | ||
| from typing import Any | ||
|
|
||
| import redis.asyncio as redis | ||
|
|
||
| # Initialize Redis client (typically configured centrally). | ||
| from app.services.reservations import calculate_monthly_revenue, get_tenant_property | ||
| from app.services.revenue_reporting import revenue_cache_key | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Redis is an optimization only; revenue always remains available from Postgres | ||
| # when a cache operation fails. | ||
| redis_client = redis.Redis.from_url(os.getenv("REDIS_URL", "redis://localhost:6379/0")) | ||
|
|
||
| async def get_revenue_summary(property_id: str, tenant_id: str) -> Dict[str, Any]: | ||
| """ | ||
| Fetches revenue summary, utilizing caching to improve performance. | ||
|
|
||
| async def get_revenue_summary( | ||
| property_id: str, | ||
| tenant_id: str, | ||
| *, | ||
| year: int, | ||
| month: int, | ||
| ) -> dict[str, Any]: | ||
| """Fetch an authorized, property-local monthly revenue summary. | ||
|
|
||
| The ownership check deliberately happens before the cache lookup. That makes | ||
| a cached value inaccessible to a tenant that does not own the property. | ||
| """ | ||
| cache_key = f"revenue:{property_id}" | ||
|
|
||
| # Try to get from cache | ||
| cached = await redis_client.get(cache_key) | ||
| if cached: | ||
| return json.loads(cached) | ||
|
|
||
| # Revenue calculation is delegated to the reservation service. | ||
| from app.services.reservations import calculate_total_revenue | ||
|
|
||
| # Calculate revenue | ||
| result = await calculate_total_revenue(property_id, tenant_id) | ||
|
|
||
| # Cache the result for 5 minutes | ||
| await redis_client.setex(cache_key, 300, json.dumps(result)) | ||
|
|
||
| property_record = await get_tenant_property(property_id, tenant_id) | ||
| cache_key = revenue_cache_key(tenant_id, property_id, year, month) | ||
|
|
||
| try: | ||
| cached = await redis_client.get(cache_key) | ||
| if cached: | ||
| return json.loads(cached) | ||
| except Exception as error: # Cache availability must not break reporting. | ||
| logger.warning("Revenue cache read failed: %s", error) | ||
|
|
||
| result = await calculate_monthly_revenue( | ||
| property_id, | ||
| tenant_id, | ||
| month, | ||
| year, | ||
| property_timezone=property_record["timezone"], | ||
| ) | ||
| result["property_name"] = property_record["name"] | ||
| result["timezone"] = property_record["timezone"] | ||
|
|
||
| try: | ||
| await redis_client.setex(cache_key, 300, json.dumps(result)) | ||
| except Exception as error: # Cache availability must not break reporting. | ||
| logger.warning("Revenue cache write failed: %s", error) | ||
|
|
||
| return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The
_async_database_urlfunction silently passes through unrecognized URL schemes, causingcreate_async_engineto fail if a synchronous driver is configured viaDATABASE_URL.Severity: MEDIUM
Suggested Fix
In
_async_database_url, validate the URL scheme. If the scheme is not one of the recognized async-compatible schemes, raise aValueErrorwith an informative message to prevent the application from starting with an invalid configuration.Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.