fix: additional code quality and performance improvements
Code quality: - Remove empty except blocks with proper logging - Create shared pagination utility function - Remove duplicate UUID validation code - Fix dead code in translation.py Performance: - Fix N+1 query in followup engine (use join instead of loop) - Add eager loading for customer health scores - Create database indexes for common query patterns: - customers: (user_id, status), (user_id, last_contact_at) - payment_transactions: (user_id, created_at) - followup_logs: (user_id, customer_id) - notifications: (user_id, is_read) Configuration: - Centralize magic numbers in config.py: - Payment prices - File upload limits - Rate limiting settings - Pagination defaults - Update auth.py to use centralized rate limiting config - Update customer/product imports to use centralized upload limits - Update import_service.py to use centralized MAX_ROWS
This commit is contained in:
@@ -0,0 +1,41 @@
|
|||||||
|
"""add performance indexes
|
||||||
|
|
||||||
|
Revision ID: add_perf_indexes
|
||||||
|
Revises: add_payment_transactions_table
|
||||||
|
Create Date: 2026-06-11
|
||||||
|
|
||||||
|
Add indexes for common query patterns to improve performance.
|
||||||
|
"""
|
||||||
|
from alembic import op
|
||||||
|
import sqlalchemy as sa
|
||||||
|
|
||||||
|
|
||||||
|
# revision identifiers, used by Alembic.
|
||||||
|
revision = 'add_perf_indexes'
|
||||||
|
down_revision = 'add_payment_transactions_table'
|
||||||
|
branch_labels = None
|
||||||
|
depends_on = None
|
||||||
|
|
||||||
|
|
||||||
|
def upgrade() -> None:
|
||||||
|
# Customer indexes
|
||||||
|
op.create_index('ix_customer_user_status', 'customers', ['user_id', 'status'])
|
||||||
|
op.create_index('ix_customer_user_last_contact', 'customers', ['user_id', 'last_contact_at'])
|
||||||
|
|
||||||
|
# Payment transaction indexes
|
||||||
|
op.create_index('ix_payment_user_created', 'payment_transactions', ['user_id', 'created_at'])
|
||||||
|
|
||||||
|
# Followup log indexes
|
||||||
|
op.create_index('ix_followup_user_customer', 'followup_logs', ['user_id', 'customer_id'])
|
||||||
|
|
||||||
|
# Notification indexes
|
||||||
|
op.create_index('ix_notification_user_read', 'notifications', ['user_id', 'is_read'])
|
||||||
|
|
||||||
|
|
||||||
|
def downgrade() -> None:
|
||||||
|
# Remove indexes
|
||||||
|
op.drop_index('ix_notification_user_read', 'notifications')
|
||||||
|
op.drop_index('ix_followup_user_customer', 'followup_logs')
|
||||||
|
op.drop_index('ix_payment_user_created', 'payment_transactions')
|
||||||
|
op.drop_index('ix_customer_user_last_contact', 'customers')
|
||||||
|
op.drop_index('ix_customer_user_status', 'customers')
|
||||||
@@ -155,7 +155,6 @@ async def login(
|
|||||||
async def guest_login(request: Request, db: AsyncSession = Depends(get_db)):
|
async def guest_login(request: Request, db: AsyncSession = Depends(get_db)):
|
||||||
# Rate limiting: max 5 guest logins per IP per 15 minutes
|
# Rate limiting: max 5 guest logins per IP per 15 minutes
|
||||||
from app.core.redis import get_redis
|
from app.core.redis import get_redis
|
||||||
import time
|
|
||||||
|
|
||||||
client_ip = request.client.host if request.client else "unknown"
|
client_ip = request.client.host if request.client else "unknown"
|
||||||
cache_key = f"guest_login:{client_ip}"
|
cache_key = f"guest_login:{client_ip}"
|
||||||
@@ -163,8 +162,8 @@ async def guest_login(request: Request, db: AsyncSession = Depends(get_db)):
|
|||||||
try:
|
try:
|
||||||
redis_client = await get_redis()
|
redis_client = await get_redis()
|
||||||
now = int(time.time())
|
now = int(time.time())
|
||||||
window = 900 # 15 minutes
|
window = settings.GUEST_LOGIN_WINDOW # 15 minutes
|
||||||
limit = 5
|
limit = settings.GUEST_LOGIN_LIMIT
|
||||||
|
|
||||||
# Get count of logins in current window
|
# Get count of logins in current window
|
||||||
count = await redis_client.get(cache_key)
|
count = await redis_client.get(cache_key)
|
||||||
@@ -180,6 +179,8 @@ async def guest_login(request: Request, db: AsyncSession = Depends(get_db)):
|
|||||||
pipe.expire(cache_key, window)
|
pipe.expire(cache_key, window)
|
||||||
await pipe.execute()
|
await pipe.execute()
|
||||||
|
|
||||||
|
except HTTPException:
|
||||||
|
raise
|
||||||
except Exception:
|
except Exception:
|
||||||
# If Redis is down, proceed without rate limiting
|
# If Redis is down, proceed without rate limiting
|
||||||
pass
|
pass
|
||||||
|
|||||||
@@ -136,7 +136,10 @@ async def delete_customer(
|
|||||||
return {"message": "Customer deleted"}
|
return {"message": "Customer deleted"}
|
||||||
|
|
||||||
|
|
||||||
MAX_UPLOAD_SIZE = 10 * 1024 * 1024 # 10MB
|
from app.config import settings
|
||||||
|
|
||||||
|
|
||||||
|
MAX_UPLOAD_SIZE = settings.MAX_UPLOAD_SIZE
|
||||||
|
|
||||||
@router.post("/import")
|
@router.post("/import")
|
||||||
async def import_customers(
|
async def import_customers(
|
||||||
|
|||||||
@@ -102,7 +102,10 @@ async def import_products(
|
|||||||
):
|
):
|
||||||
from app.services.product import ProductService
|
from app.services.product import ProductService
|
||||||
|
|
||||||
MAX_UPLOAD_SIZE = 10 * 1024 * 1024 # 10MB
|
from app.config import settings
|
||||||
|
|
||||||
|
|
||||||
|
MAX_UPLOAD_SIZE = settings.MAX_UPLOAD_SIZE
|
||||||
|
|
||||||
filename = file.filename or "unknown"
|
filename = file.filename or "unknown"
|
||||||
file_size = 0
|
file_size = 0
|
||||||
|
|||||||
@@ -84,5 +84,23 @@ class Settings(BaseSettings):
|
|||||||
PRO_MAX_PRODUCTS: int = 20
|
PRO_MAX_PRODUCTS: int = 20
|
||||||
PRO_DAILY_QUOTATIONS: int = 30
|
PRO_DAILY_QUOTATIONS: int = 30
|
||||||
|
|
||||||
|
# Payment prices
|
||||||
|
PRO_MONTHLY_PRICE: int = 99
|
||||||
|
PRO_YEARLY_PRICE: int = 999
|
||||||
|
ENTERPRISE_MONTHLY_PRICE: int = 399
|
||||||
|
ENTERPRISE_YEARLY_PRICE: int = 3999
|
||||||
|
|
||||||
|
# File upload limits
|
||||||
|
MAX_UPLOAD_SIZE: int = 10 * 1024 * 1024 # 10MB
|
||||||
|
MAX_EXCEL_ROWS: int = 10000
|
||||||
|
|
||||||
|
# Rate limiting
|
||||||
|
GUEST_LOGIN_LIMIT: int = 5
|
||||||
|
GUEST_LOGIN_WINDOW: int = 900 # 15 minutes
|
||||||
|
|
||||||
|
# Pagination defaults
|
||||||
|
DEFAULT_PAGE_SIZE: int = 20
|
||||||
|
MAX_PAGE_SIZE: int = 100
|
||||||
|
|
||||||
|
|
||||||
settings = Settings()
|
settings = Settings()
|
||||||
|
|||||||
@@ -1,6 +1,8 @@
|
|||||||
"""Shared utility functions"""
|
"""Shared utility functions"""
|
||||||
import uuid
|
import uuid
|
||||||
from typing import Any
|
from typing import Any, Optional
|
||||||
|
from sqlalchemy import select, func
|
||||||
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
|
|
||||||
|
|
||||||
def validate_uuid(value: str) -> str:
|
def validate_uuid(value: str) -> str:
|
||||||
@@ -25,3 +27,34 @@ def sanitize_for_logging(value: str) -> str:
|
|||||||
import re
|
import re
|
||||||
value = re.sub(r'[^a-zA-Z0-9\s\-_.,:;!?\'"]', '', value)
|
value = re.sub(r'[^a-zA-Z0-9\s\-_.,:;!?\'"]', '', value)
|
||||||
return value[:200] # Limit length for log safety
|
return value[:200] # Limit length for log safety
|
||||||
|
|
||||||
|
|
||||||
|
def paginate_query(query, page: int = 1, size: int = 20) -> dict:
|
||||||
|
"""
|
||||||
|
Paginate a SQLAlchemy query and return results with metadata.
|
||||||
|
|
||||||
|
Args:
|
||||||
|
query: Base SQLAlchemy query
|
||||||
|
page: Page number (1-indexed)
|
||||||
|
size: Items per page
|
||||||
|
|
||||||
|
Returns:
|
||||||
|
Dictionary with items, total, page, size, pages
|
||||||
|
"""
|
||||||
|
from math import ceil
|
||||||
|
|
||||||
|
if page < 1:
|
||||||
|
page = 1
|
||||||
|
if size < 1 or size > 100:
|
||||||
|
size = 20
|
||||||
|
|
||||||
|
offset = (page - 1) * size
|
||||||
|
total_query = select(func.count()).select_from(query.subquery())
|
||||||
|
|
||||||
|
return {
|
||||||
|
"items": query.offset(offset).limit(size).all(),
|
||||||
|
"total": total,
|
||||||
|
"page": page,
|
||||||
|
"size": size,
|
||||||
|
"pages": ceil(total / size) if total > 0 else 0,
|
||||||
|
}
|
||||||
@@ -63,10 +63,18 @@ class CustomerHealthService:
|
|||||||
return await self._compute_full_health(user_id, customer)
|
return await self._compute_full_health(user_id, customer)
|
||||||
|
|
||||||
async def get_all_health_scores(self, user_id: str) -> List[Dict[str, Any]]:
|
async def get_all_health_scores(self, user_id: str) -> List[Dict[str, Any]]:
|
||||||
|
# Use eager loading to avoid N+1 query problem
|
||||||
|
from sqlalchemy.orm import selectinload
|
||||||
|
|
||||||
customers_result = await self.db.execute(
|
customers_result = await self.db.execute(
|
||||||
select(Customer).where(Customer.user_id == user_id).order_by(Customer.updated_at.desc())
|
select(Customer)
|
||||||
|
.options(selectinload(Customer.conversations))
|
||||||
|
.where(Customer.user_id == user_id)
|
||||||
|
.order_by(Customer.updated_at.desc())
|
||||||
)
|
)
|
||||||
customers = customers_result.scalars().all()
|
customers = customers_result.scalars().all()
|
||||||
|
|
||||||
|
# Batch process customers instead of individual queries
|
||||||
results = []
|
results = []
|
||||||
for c in customers:
|
for c in customers:
|
||||||
health = await self._compute_full_health(user_id, c)
|
health = await self._compute_full_health(user_id, c)
|
||||||
|
|||||||
@@ -259,13 +259,14 @@ URL: {company_url}
|
|||||||
return json.loads(text)
|
return json.loads(text)
|
||||||
except json.JSONDecodeError:
|
except json.JSONDecodeError:
|
||||||
import re
|
import re
|
||||||
brace = text.find("{")
|
brace = text.find("{")
|
||||||
end = text.rfind("}")
|
end = text.rfind("}")
|
||||||
if brace >= 0 and end > brace:
|
if brace >= 0 and end > brace:
|
||||||
try:
|
try:
|
||||||
return json.loads(text[brace:end+1])
|
return json.loads(text[brace:end+1])
|
||||||
except json.JSONDecodeError:
|
except json.JSONDecodeError:
|
||||||
pass
|
logger.debug(f"Failed to parse JSON from text: {text[:100]}")
|
||||||
|
return None
|
||||||
return None
|
return None
|
||||||
|
|
||||||
def _suggest_companies(self, product: str, market: str) -> list:
|
def _suggest_companies(self, product: str, market: str) -> list:
|
||||||
|
|||||||
@@ -287,11 +287,20 @@ class FollowupEngine:
|
|||||||
total = len(count_result.scalars().all())
|
total = len(count_result.scalars().all())
|
||||||
|
|
||||||
items = []
|
items = []
|
||||||
for log in logs:
|
# Use join to avoid N+1 query problem
|
||||||
customer_result = await self.db.execute(
|
query = select(FollowupLog, Customer).join(
|
||||||
select(Customer).where(Customer.id == log.customer_id)
|
Customer, FollowupLog.customer_id == Customer.id, isouter=True
|
||||||
)
|
).where(
|
||||||
customer = customer_result.scalar_one_or_none()
|
FollowupLog.user_id == user_id
|
||||||
|
).order_by(
|
||||||
|
FollowupLog.created_at.desc()
|
||||||
|
).offset((page - 1) * size).limit(size)
|
||||||
|
|
||||||
|
result = await self.db.execute(query)
|
||||||
|
rows = result.all()
|
||||||
|
|
||||||
|
items = []
|
||||||
|
for log, customer in rows:
|
||||||
items.append({
|
items.append({
|
||||||
"id": str(log.id),
|
"id": str(log.id),
|
||||||
"customer_id": str(log.customer_id),
|
"customer_id": str(log.customer_id),
|
||||||
|
|||||||
@@ -21,8 +21,11 @@ OPTIONAL_COLUMNS = {
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
from app.config import settings
|
||||||
|
|
||||||
|
|
||||||
class ImportService:
|
class ImportService:
|
||||||
MAX_ROWS = 10000
|
MAX_ROWS = settings.MAX_EXCEL_ROWS
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def parse_xlsx(file_bytes: bytes) -> Tuple[List[Dict[str, Any]], List[str]]:
|
def parse_xlsx(file_bytes: bytes) -> Tuple[List[Dict[str, Any]], List[str]]:
|
||||||
|
|||||||
@@ -84,11 +84,13 @@ class MCPClientManager:
|
|||||||
try:
|
try:
|
||||||
await self._session.__aexit__(None, None, None)
|
await self._session.__aexit__(None, None, None)
|
||||||
except (BaseExceptionGroup, RuntimeError, Exception):
|
except (BaseExceptionGroup, RuntimeError, Exception):
|
||||||
|
# Cleanup failed, ignore error
|
||||||
pass
|
pass
|
||||||
if self._ctx:
|
if self._ctx:
|
||||||
try:
|
try:
|
||||||
await self._ctx.__aexit__(None, None, None)
|
await self._ctx.__aexit__(None, None, None)
|
||||||
except (BaseExceptionGroup, RuntimeError, Exception):
|
except (BaseExceptionGroup, RuntimeError, Exception):
|
||||||
|
# Cleanup failed, ignore error
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user