feat: add creator engagement metrics groundwork #251
Conversation
📝 WalkthroughWalkthroughDatabase connection handling now URL-encodes passwords and resolves HOST to an IPv4 address. Application startup adds a retry-enabled database initialization sequence. A new engagement API endpoint and supporting schemas and service compute aggregated engagement metrics from stored user posts. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client
participant API as FastAPI / Route
participant DB as Async DB Session
participant Service as Engagement Service
Client->>API: POST /engagement/compute (EngagementRequest)
API->>DB: get_db() (dependency)
DB-->>API: AsyncSession
API->>DB: query UserPost by creator_id
DB-->>API: List[UserPost]
API->>Service: compute_engagement_metrics(posts, follower_count)
Service->>Service: aggregate likes/comments/shares\ncompute averages\ncompute engagement_rate
Service-->>API: metrics dict
API-->>Client: 200 OK (EngagementMetrics)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Backend/app/main.py (1)
40-51: Retry logic is reasonable, but consider logging the exception type.The retry mechanism with 5 attempts and 2-second delays is appropriate for transient DB connection issues. The fallback to continue without DB initialization is a reasonable choice for development, but could mask serious configuration errors in production.
Consider using
logging.warninginstead ofBackend/app/routes/engagement.py (1)
4-4: Unused import.
Optionalis imported but not used in this file.🔎 Proposed fix
-from typing import OptionalBackend/app/services/engagement_service.py (2)
3-8: UseTYPE_CHECKINGfor conditional type imports.The try/except pattern for imports is fragile and unconventional. For type-only imports, use
typing.TYPE_CHECKINGwhich is the standard pattern and avoids runtime overhead.🔎 Proposed fix
-from typing import List, Optional, Dict, Any +from typing import TYPE_CHECKING, List, Optional, Dict, Any -try: - from app.models.models import UserPost # for typing clarity -except Exception: - # Fallback for type hints if import is not available at runtime - class UserPost: # type: ignore - engagement_metrics: Dict[str, Any] +if TYPE_CHECKING: + from app.models.models import UserPostThen update the function signature to use a string annotation or
Any:def compute_engagement_metrics(posts: List["UserPost"], follower_count: Optional[int]) -> Dict[str, Any]:
26-35: Defensive metric extraction is good, butint()could raise on invalid data.The
int(metrics.get("likes", 0) or 0)pattern handlesNoneand missing keys well. However, ifengagement_metricscontains a non-numeric string (e.g.,"likes": "invalid"),int()will raise aValueError.Consider wrapping in a helper or try/except if the data source isn't fully trusted:
🔎 Optional: safer int extraction
def safe_int(value, default=0): try: return int(value) if value else default except (ValueError, TypeError): return default
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Backend/app/db/db.pyBackend/app/main.pyBackend/app/routes/engagement.pyBackend/app/schemas/engagement.pyBackend/app/services/engagement_service.py
🧰 Additional context used
🧬 Code graph analysis (2)
Backend/app/routes/engagement.py (3)
Backend/app/db/db.py (1)
get_db(49-51)Backend/app/services/engagement_service.py (2)
UserPost(7-8)compute_engagement_metrics(11-64)Backend/app/schemas/engagement.py (2)
EngagementRequest(5-7)EngagementMetrics(10-21)
Backend/app/main.py (1)
Backend/app/db/seed.py (1)
seed_db(6-57)
🪛 Ruff (0.14.10)
Backend/app/services/engagement_service.py
5-5: Do not catch blind exception: Exception
(BLE001)
Backend/app/routes/engagement.py
15-15: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
Backend/app/main.py
38-38: Unused function argument: app
(ARG001)
46-46: Do not catch blind exception: Exception
(BLE001)
Backend/app/db/db.py
24-24: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
Backend/app/main.py (2)
32-34: Clean extraction of startup logic.Bundling
create_tables()andseed_db()into a single async function improves readability and makes the retry logic inlifespancleaner.
72-72: Engagement router correctly registered.The router is properly included alongside existing routes.
Backend/app/db/db.py (2)
13-13: Good addition of URL encoding for password.Using
quote_plushandles special characters in passwords that would otherwise break the connection URL.
28-29: Password masking in debug output is good security practice.Printing the masked URL helps with debugging while avoiding accidental credential exposure in logs.
Backend/app/routes/engagement.py (1)
14-20: Endpoint implementation is clean and functional.The endpoint correctly:
- Uses dependency injection for the DB session
- Queries posts by creator_id
- Delegates computation to the service layer
- Returns properly typed response
One consideration: returning empty metrics (all zeros) when
creator_iddoesn't exist or has no posts is a valid design choice, but you may want to add a check to return a 404 if the creator doesn't exist at all. This would help distinguish between "creator with no posts" vs "invalid creator_id".Backend/app/schemas/engagement.py (2)
5-7: Request schema is well-defined.Using
strforcreator_idaligns with the UUID string format used elsewhere in the codebase.
10-21: Response schema covers all computed metrics.The schema properly includes totals, averages, and the optional engagement rate. The types align with the service layer output.
Consider adding
Fieldconstraints in a future iteration (e.g.,ge=0for counts) to enforce data validity at the schema level.Backend/app/services/engagement_service.py (1)
39-52: Edge case handling is correct.The logic properly handles:
- Empty posts list (returns 0.0 averages)
- Missing or zero follower_count (returns
Nonefor engagement_rate)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Backend/app/db/db.py (4)
14-14: Consider validating required environment variables.The fallback to an empty string for a missing password will create an invalid connection string that fails later at engine creation. While the error is caught, validating required credentials upfront would provide clearer error messages at startup.
🔎 Proposed validation approach
Add validation after loading environment variables:
# Fetch database credentials USER = os.getenv("user") PASSWORD_RAW = os.getenv("password") HOST = os.getenv("host") PORT = os.getenv("port") DBNAME = os.getenv("dbname") # Validate required credentials required_vars = {"user": USER, "password": PASSWORD_RAW, "host": HOST, "port": PORT, "dbname": DBNAME} missing = [k for k, v in required_vars.items() if not v] if missing: raise ValueError(f"Missing required database environment variables: {', '.join(missing)}") PASSWORD = quote_plus(PASSWORD_RAW)
20-33: Host resolution guard addresses previous review concern.The
if HOST and PORT:guard successfully addresses the past review comment about potentialNoneHOST causing aTypeError. The fallback logic is sound.However, consider refining the exception handling for better diagnostics:
🔎 Optional refinement for exception handling
# Resolve IPv4 address for HOST to avoid intermittent DNS issues on Windows if HOST and PORT: try: addrinfo = socket.getaddrinfo( HOST, int(PORT), socket.AF_INET, socket.SOCK_STREAM, ) HOST_IP = addrinfo[0][4][0] if addrinfo else HOST - except Exception: + except (ValueError, socket.gaierror, OSError) as e: + print(f"⚠️ DNS resolution failed for {HOST}, using hostname directly: {e}") HOST_IP = HOST else: HOST_IP = HOSTThis catches the specific exceptions (
ValueErrorfrom invalid PORT,socket.gaierrorfrom DNS failures,OSErrorfrom network issues) and provides better visibility when resolution fails.
37-37: Consider making debug output conditional.The debug print statement is helpful for troubleshooting but always executes. Consider making it conditional based on a debug flag or log level for production environments.
🔎 Example with environment-based debug flag
DATABASE_URL = f"postgresql+asyncpg://{USER}:{PASSWORD}@{HOST_IP}:{PORT}/{DBNAME}" -print(f"DB URL: postgresql+asyncpg://{USER}:***@{HOST_IP}:{PORT}/{DBNAME}") +if os.getenv("DEBUG", "").lower() in ("true", "1"): + print(f"DB URL: postgresql+asyncpg://{USER}:***@{HOST_IP}:{PORT}/{DBNAME}")
42-44: Consider making SQL echo configurable.The
echo=Trueparameter enables verbose SQL logging, which is useful for development but can be excessive in production. Consider making it configurable via an environment variable.🔎 Example with environment-based echo flag
engine = create_async_engine( DATABASE_URL, - echo=True, + echo=os.getenv("SQL_ECHO", "false").lower() in ("true", "1"), connect_args={"ssl": "require"}, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Backend/app/db/db.py
🧰 Additional context used
🪛 Ruff (0.14.10)
Backend/app/db/db.py
30-30: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
Backend/app/db/db.py (1)
6-7: LGTM! Imports are correctly placed.The new imports for
quote_plus(password URL-encoding) andsocket(DNS resolution) are properly positioned at the module level and are used appropriately later in the file.
1 participant
Summary
This PR introduces the foundational engagement metrics pipeline for creators.
What’s included
Notes
Testing
Closes #22 (partial – engagement groundwork)
Tested via Swagger UI; happy to add screenshots if needed.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.