Migrate HTTP Cloud Functions endpoints to FastAPI - #100
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces a DomainServices dataclass and dependency, replaces global service usage with dependency-injected services, adds domain-aware error handling and several calendar/authorization/sync endpoints, and adds initialization logging in multiple Firestore repository classes. Settings and test env defaults were adjusted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant FastAPI as API
participant DI as get_domain_services()
participant AppState as app.state.DomainServices
participant DS as DomainServices
participant Calendar as GoogleCalendarService
participant Auth as AuthorizationService
participant Sync as SyncProfileService
Client->>API: POST /calendars/list
API->>DI: resolve dependency
DI->>AppState: read DomainServices
AppState-->>DI: DomainServices instance
DI-->>API: services
API->>Calendar: services.google_calendar_service.list_calendars(...)
Calendar-->>API: result
API-->>Client: 200 OK (calendars)
alt Service raises SyncademicError
Calendar-->>API: SyncademicError
API->>API: syncademic_error_response(error, ERROR_MAPPING)
API-->>Client: HTTP error (mapped status + body)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 |
Summary of ChangesHello @SuperMuel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural change by migrating existing HTTP Cloud Functions logic to a FastAPI framework. The primary objective is to harness FastAPI's powerful capabilities for API development, such as dependency injection and structured error handling. This refactoring centralizes the management of backend domain services, enhancing their accessibility and testability across various API endpoints, and establishes new routes for core application functionalities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
- Coverage 72.31% 69.96% -2.36%
==========================================
Files 43 43
Lines 1994 2081 +87
==========================================
+ Hits 1442 1456 +14
- Misses 552 625 +73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does an excellent job of migrating several HTTP Cloud Functions endpoints to a FastAPI application. The implementation follows modern FastAPI best practices, such as using a dependency-injected domain service container initialized during the application's lifespan and centralizing domain exception handling. The new structure is clean and significantly improves the architecture. I've added a couple of suggestions to further enhance maintainability by reducing code duplication.
| try: | ||
| calendars = services.google_calendar_service.list_calendars( | ||
| user_id=current_user.uid, | ||
| provider_account_id=payload.provider_account_id, | ||
| ) | ||
| except SyncademicError as exc: | ||
| raise_syncademic_http_error(exc, error_mapping=services.error_mapping) |
There was a problem hiding this comment.
This try...except block for handling SyncademicError is repeated across multiple new endpoints. To reduce boilerplate and centralize error handling, consider using a FastAPI exception handler.
You could define a handler like this:
from fastapi.responses import JSONResponse
@app.exception_handler(SyncademicError)
async def syncademic_exception_handler(request: Request, exc: SyncademicError):
services: DomainServices | None = getattr(request.app.state, "domain_services", None)
if not services:
# Fallback if services are not available for some reason
return JSONResponse(
status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
content={"message": "Domain services are not available"},
)
status_code, message = services.error_mapping.to_fastapi_status_code_and_message(exc)
detail: dict[str, Any] = {"message": message}
if exc.details:
detail["details"] = exc.details
logger.error(
"Syncademic error mapped to HTTP response.",
extra={
"error_type": type(exc).__name__,
"status_code": status_code,
},
)
return JSONResponse(
status_code=status_code,
content=detail,
)This would allow you to simplify endpoints like list_user_calendars_endpoint to be more focused on the business logic, without the repetitive try...except block.
| self._db = db or firestore.Client() | ||
| firebase_project_id = getattr(self._db, "project", None) | ||
| logger.info( | ||
| "Initialized %s with Firebase project: %s", | ||
| self.__class__.__name__, | ||
| firebase_project_id, | ||
| ) |
There was a problem hiding this comment.
This initialization logic, including setting self._db and logging the project ID, is duplicated across FirestoreBackendAuthorizationRepository, FirestoreSyncProfileRepository, and FirestoreSyncStatsRepository.
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, you could extract this common logic into a base class.
For example:
class BaseFirestoreRepository:
def __init__(self, db: firestore.Client | None = None):
self._db = db or firestore.Client()
firebase_project_id = getattr(self._db, "project", None)
logger.info(
"Initialized %s with Firebase project: %s",
self.__class__.__name__,
firebase_project_id,
)
class FirestoreBackendAuthorizationRepository(BaseFirestoreRepository, IBackendAuthorizationRepository):
def __init__(self, db: firestore.Client | None = None):
super().__init__(db)
# ... repository methodsThis would centralize the initialization logic, making it easier to manage and modify in the future.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Syncademic/backend/backend/api.py
Lines 108 to 109 in 313b3f7
Require explicit bucket and break default Firebase storage
The new build_domain_services constructs the cache storage with storage.bucket(settings.FIREBASE_STORAGE_BUCKET). FIREBASE_STORAGE_BUCKET defaults to "mock-storage-bucket", so unless a real bucket name is injected via configuration the FastAPI app will always point to a non‑existent bucket. The previous Cloud Functions code relied on storage.bucket() without a name, which automatically bound to the default bucket from the initialized Firebase app and worked without extra env vars. With this change, deployments that previously relied on the default bucket will now fail to read/write calendar data. Consider falling back to the Firebase default when the setting is missing instead of forcing a name.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/backend/repositories/sync_stats_repository.py (1)
65-67: UTC mismatch: date.today() ignores UTC and can miscount near midnight.Docstrings state UTC, but
date.today()uses server local time. Use UTC date to avoid off‑by‑one errors.Apply:
-from datetime import date +from datetime import date, datetime, timezone ... - if day is None: - day = date.today() + if day is None: + day = datetime.now(timezone.utc).date() ... - if day is None: - day = date.today() + if day is None: + day = datetime.now(timezone.utc).date()Also applies to: 91-93
backend/backend/repositories/backend_authorization_repository.py (1)
72-72: Doc ID collisions: concatenating IDs without a delimiter can corrupt data.
user_id + provider_account_idrisks collisions (e.g., "ab"+"c" == "a"+"bc"). Use a delimiter or namespaced path.Apply:
class FirestoreBackendAuthorizationRepository(IBackendAuthorizationRepository): + def _doc_id(self, user_id: str, provider_account_id: str) -> str: + # Delimiter prevents collisions; encode if delimiter can appear in inputs. + return f"{user_id}:{provider_account_id}" @@ - doc_id = user_id + provider_account_id + doc_id = self._doc_id(user_id, provider_account_id) @@ - doc_id = backend_auth.user_id + backend_auth.provider_account_id + doc_id = self._doc_id(backend_auth.user_id, backend_auth.provider_account_id) @@ - doc_id = user_id + provider_account_id + doc_id = self._doc_id(user_id, provider_account_id) @@ - doc_id = user_id + provider_account_id + doc_id = self._doc_id(user_id, provider_account_id)Also applies to: 86-86, 95-95, 104-104
backend/backend/api.py (3)
258-266: Critical: Root endpoint leaks full settings, including secrets.Returning
settings.model_dump()can expose secrets/keys. Remove or strictly sanitize.Apply:
@app.get("/") async def root() -> dict[str, Any]: """Root endpoint providing basic API information and available endpoints.""" return { "name": "Syncademic API", "version": "0.1.0", "endpoints": {"health": "/health", "docs": "/docs", "redoc": "/redoc"}, - "settings": settings.model_dump(), + # Expose only non-sensitive, high-level metadata if needed. + "env": settings.ENV, }If you must expose config, build an explicit allowlist of non-sensitive fields. As per coding guidelines.
301-304: Avoid logging full ICS URLs (may contain secrets).URLs can embed tokens/query params. Log a redacted form.
Apply:
- logger.info( - "Validating ICS URL via FastAPI", - extra={"user_id": current_user.uid, "url": payload.url}, - ) + from urllib.parse import urlparse + parsed = urlparse(payload.url) + redacted_url = f"{parsed.scheme}://{parsed.netloc}{parsed.path}" + logger.info( + "Validating ICS URL via FastAPI", + extra={"user_id": current_user.uid, "url": redacted_url}, + )Also consider dropping
user_idor hashing it when logging. Based on coding guidelines.
94-149: Initialize Firebase Admin SDK before accessing storage.bucket
Add a call tofirebase_admin.initialize_app(cred, {"storageBucket": settings.FIREBASE_STORAGE_BUCKET})immediately after credentials are configured (around lines 68–76) so thatstorage.bucket(...)won’t fail. Also ensureFIREBASE_STORAGE_BUCKETis overridden in production (default is"mock-storage-bucket").
🧹 Nitpick comments (7)
backend/backend/repositories/sync_stats_repository.py (1)
50-55: Init logging is fine; consider structured field for project id.Keep the message, but also attach
firebase_project_idviaextrafor better queryability in structured logs.Apply:
- logger.info( - "Initialized %s with Firebase project: %s", - self.__class__.__name__, - firebase_project_id, - ) + logger.info( + "Initialized %s with Firebase project: %s", + self.__class__.__name__, + firebase_project_id, + extra={"firebase_project_id": firebase_project_id}, + )backend/backend/repositories/sync_profile_repository.py (1)
98-102: Reduce PII/log noise in repository reads.
logger.infofor every read withuser_idandsync_profile_idcan be noisy and leaks identifiers. Preferdebugand/or hash/truncate IDs.Apply:
- logger.info( + logger.debug( "Getting sync profile %s for user %s", sync_profile_id, user_id, )backend/backend/repositories/backend_authorization_repository.py (1)
41-43: Nit: typo in docstring.
“acutally” -> “actually”.- and provider_account_id, otherwise False. It doesn't acutally check if the token is expired or + and provider_account_id, otherwise False. It doesn't actually check if the token is expired orbackend/backend/services/exceptions/mapping.py (2)
30-36: Broaden docstrings to reflect dual environments.Class/docstrings still mention only Firebase Functions though this module now serves FastAPI too.
Apply:
-class ErrorMapping: - """Maps domain exceptions to Firebase Functions error codes""" +class ErrorMapping: + """Maps domain exceptions to Firebase Functions codes and FastAPI HTTP statuses"""
1-1: Decouple from firebase_functions in exception mapping
Inbackend/backend/services/exceptions/mapping.py, replacefrom firebase_functions import https_fnwith a minimal internal HTTP status enum and map tohttps_fnonly at the edge.firebase_functionsis already pinned inbackend/uv.lockand installed by your Dockerfile, so this refactor won’t break deployments.backend/backend/api.py (2)
314-339: Prefer Pydantic response models over plain dicts.For consistency and schema clarity, define outputs (e.g.,
ListUserCalendarsOutput,OperationResult) instead ofdict[str, Any]/dict[str, bool].Example:
-@app.post("/calendars/list") -def list_user_calendars_endpoint(...) -> dict[str, Any]: +from backend.models.schemas import ListUserCalendarsOutput +@app.post("/calendars/list", response_model=ListUserCalendarsOutput) +def list_user_calendars_endpoint(... ) -> ListUserCalendarsOutput: @@ - return {"calendars": calendars} + return ListUserCalendarsOutput(calendars=calendars)Apply similar
response_modelpatterns to/sync/request,/sync-profiles/{id}, and/authorization/backend.Also applies to: 377-405, 407-432, 434-462
224-245: Include error details in logs for triage.You already attach type and status; include
error.detailslength or keys when present for faster debugging (without logging sensitive values).Apply:
- logger.error( + logger.error( "Syncademic error mapped to HTTP response.", extra={ "error_type": type(error).__name__, "status_code": status_code, + "has_details": bool(getattr(error, "details", {})), + "detail_keys": list(getattr(error, "details", {}).keys()) if getattr(error, "details", {}) else [], }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/backend/api.py(7 hunks)backend/backend/repositories/backend_authorization_repository.py(2 hunks)backend/backend/repositories/sync_profile_repository.py(1 hunks)backend/backend/repositories/sync_stats_repository.py(2 hunks)backend/backend/services/exceptions/mapping.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
backend/**/*.py
📄 CodeRabbit inference engine (backend/AGENTS.md)
backend/**/*.py: Prefer Pydantic models over raw dictionaries for input validation
Favor functional, declarative style with pure functions and immutable data structures where practical
Handle errors and edge cases at the beginning of functions
Use early returns for error conditions to avoid deeply nested if statements
Place the happy path last in the function for improved readability
Avoid unnecessary else statements; use the if-return pattern instead
Use guard clauses to handle preconditions and invalid states early
Implement proper error logging and user-friendly error messages
Use assert to catch developer errors
Use docstrings for all public functions, methods, and classes
Write docstrings for public APIs that explain usage and purpose, not implementation
Focus docstrings on why to use and how to use correctly
Skip docstrings for trivial functions where name and type hints are self-explanatory
Prefer self-documenting code over comments; add comments for complex business logic or why-decisions
Use type hints throughout the codebase
Prefer built-in typing syntax like list[str | None] over List[Optional[str]]
Write testable code by minimizing patching and favoring dependency injection
Files:
backend/backend/repositories/backend_authorization_repository.pybackend/backend/repositories/sync_stats_repository.pybackend/backend/services/exceptions/mapping.pybackend/backend/repositories/sync_profile_repository.pybackend/backend/api.py
🧬 Code graph analysis (3)
backend/backend/repositories/backend_authorization_repository.py (1)
backend/backend/logging_config.py (1)
FirebaseFunctionsHandler(20-45)
backend/backend/services/exceptions/mapping.py (6)
backend/backend/services/exceptions/auth.py (3)
BaseAuthorizationError(4-7)ProviderUserIdMismatchError(16-25)UnauthorizedError(10-13)backend/backend/services/exceptions/base.py (1)
SyncademicError(4-17)backend/backend/services/exceptions/ics.py (1)
IcsSourceError(10-13)backend/backend/services/exceptions/ruleset.py (2)
RulesetGenerationError(10-13)RulesetValidationError(16-19)backend/backend/services/exceptions/sync.py (3)
DailySyncLimitExceededError(16-19)SyncInProgressError(22-25)SyncProfileNotFoundError(10-13)backend/backend/services/exceptions/target_calendar.py (2)
TargetCalendarAccessError(16-19)TargetCalendarNotFoundError(10-13)
backend/backend/api.py (12)
backend/backend/ai/ruleset_builder.py (1)
RulesetBuilder(63-139)backend/backend/models/sync_profile.py (1)
SyncTrigger(56-66)backend/backend/models/base.py (2)
CamelCaseModel(7-23)model_dump(15-18)backend/backend/models/schemas.py (9)
AuthorizeBackendInput(74-99)CreateSyncProfileInput(139-154)DeleteSyncProfileInput(65-68)IsAuthorizedInput(33-36)IsAuthorizedOutput(39-40)ListUserCalendarsInput(27-30)RequestSyncInput(58-62)ValidateIcsUrlInput(10-11)ValidateIcsUrlOutput(14-24)backend/backend/repositories/backend_authorization_repository.py (1)
FirestoreBackendAuthorizationRepository(48-115)backend/backend/services/ai_ruleset_service.py (1)
AiRulesetService(17-136)backend/backend/services/authorization_service.py (3)
AuthorizationService(29-282)test_authorization(238-270)authorize_backend_with_auth_code(49-153)backend/backend/services/exceptions/base.py (1)
SyncademicError(4-17)backend/backend/services/exceptions/mapping.py (2)
ErrorMapping(30-93)to_fastapi_status_code_and_message(79-93)backend/backend/services/google_calendar_service.py (2)
GoogleCalendarService(13-147)list_calendars(17-64)backend/backend/services/ics_service.py (2)
IcsService(27-172)validate_ics_url(154-172)backend/backend/services/sync_profile_service.py (4)
SyncProfileService(39-620)synchronize(83-201)delete_sync_profile(342-426)create_sync_profile(428-620)
🔇 Additional comments (3)
backend/backend/repositories/backend_authorization_repository.py (1)
59-64: Init logging LGTM.
Consistent with other repos; no issues.backend/backend/services/exceptions/mapping.py (1)
19-27: FastAPI mapping addition looks correct.Status translations align with typical HTTP equivalents; helper cleanly reuses Firebase mapping.
Also applies to: 78-93
backend/backend/api.py (1)
85-92: DomainServices container LGTM.
Clear DI surface; aligns with guidelines to favor DI and testability.
| firebase_project_id = getattr(self._db, "project", None) | ||
| logger.info( | ||
| "Initialized %s with Firebase project: %s", | ||
| self.__class__.__name__, | ||
| firebase_project_id, | ||
| ) |
There was a problem hiding this comment.
Incorrect Firestore import and client usage; will break at runtime.
from firebase_admin.firestore import firestore does not expose Client; calling firestore.Client() will fail. Use google.cloud.firestore.Client or firebase_admin.firestore.client() consistently.
Apply:
-from firebase_admin.firestore import firestore
+from google.cloud import firestore
@@
- self._db = db or firestore.Client()
+ self._db = db or firestore.Client()Alternatively (if you prefer firebase_admin helper):
-from firebase_admin.firestore import firestore
+from firebase_admin import firestore
@@
- self._db = db or firestore.Client()
+ self._db = db or firestore.client()Pick one approach and keep it consistent with other repositories (e.g., sync_stats_repository uses google.cloud.firestore). Based on learnings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| firebase_project_id = getattr(self._db, "project", None) | |
| logger.info( | |
| "Initialized %s with Firebase project: %s", | |
| self.__class__.__name__, | |
| firebase_project_id, | |
| ) | |
| ++ b/backend/backend/repositories/sync_profile_repository.py | |
| @@ -1,7 +1,7 @@ | |
| from google.cloud import firestore | |
| class SyncProfileRepository: | |
| def __init__(self, db: firestore.Client = None): | |
| self._db = db or firestore.Client() | |
| firebase_project_id = getattr(self._db, "project", None) | |
| logger.info( | |
| "Initialized %s with Firebase project: %s", |
🤖 Prompt for AI Agents
In backend/backend/repositories/sync_profile_repository.py around lines 75 to
80, the code imports firestore from firebase_admin and calls firestore.Client(),
which is invalid at runtime; replace the import and client instantiation to use
the same approach as other repos (prefer google.cloud.firestore.Client) or use
the firebase_admin helper consistently. Update imports to: from google.cloud
import firestore and instantiate client = firestore.Client(), or alternatively
import firebase_admin.firestore and call firebase_admin.firestore.client();
ensure only one approach is used project-wide (match sync_stats_repository),
remove the incorrect firebase_admin.firestore import, and adjust any type
hints/usages accordingly.
2 participants
Summary
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Improvements
Tests / Chores