Adopt CamelCase Pydantic models - #98
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 CamelCaseModel that serializes snake_case attributes to camelCase JSON by default and migrates many public models, schemas, services, repositories, API handlers, and tests from camelCase public fields to snake_case Python attributes; updates call sites and tests to use the new names. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant HTTP as HTTP handler
participant Schema as Pydantic Schema (CamelCaseModel)
participant Service as Service
participant Repo as Repository / DB
Note over HTTP,Schema: Request body parsed into snake_case fields
HTTP->>Schema: Schema.model_validate(payload)
Schema-->>HTTP: Python object with snake_case attrs
HTTP->>Service: pass schema_obj
Service->>Repo: persist schema_obj.model_dump() (by_alias=True -> camelCase JSON)
Repo-->>Service: ack
Service-->>HTTP: result (schema_obj.model_dump() -> camelCase JSON)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 significantly refactors the backend's data model layer by adopting a standardized approach for Pydantic models. It introduces a 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 #98 +/- ##
==========================================
+ Coverage 73.45% 73.59% +0.13%
==========================================
Files 42 43 +1
Lines 1944 1958 +14
==========================================
+ Hits 1428 1441 +13
- Misses 516 517 +1 ☔ 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 is a great step towards standardizing the backend models by adopting snake_case for Python attributes and camelCase for JSON serialization. The introduction of a CamelCaseModel base class is a clean solution, and the refactoring has been applied consistently across models, services, repositories, and tests. I've found one important security issue related to logging sensitive information, for which I've provided suggestions to resolve.
| auth_code: str = Field( | ||
| ..., description="Authorization code from the frontend", min_length=1 | ||
| ) |
There was a problem hiding this comment.
The auth_code field contains a sensitive one-time authorization code from the OAuth flow. To prevent it from being accidentally logged (as it is now in main.py), it should be treated as a secret. Pydantic's SecretStr type is ideal for this, as it automatically redacts the value when the model is printed or serialized.
You'll also need to add SecretStr to the imports from pydantic at the top of the file.
| auth_code: str = Field( | |
| ..., description="Authorization code from the frontend", min_length=1 | |
| ) | |
| auth_code: SecretStr = Field( | |
| ..., description="Authorization code from the frontend", min_length=1 | |
| ) |
| auth_code=request.authCode, | ||
| redirect_uri=request.redirectUri, | ||
| provider_account_id=request.providerAccountId, | ||
| auth_code=request.auth_code, |
There was a problem hiding this comment.
Following the change of auth_code to SecretStr in the AuthorizeBackendInput model, you need to call .get_secret_value() to access the raw string value when passing it to the service.
| auth_code=request.auth_code, | |
| auth_code=request.auth_code.get_secret_value(), |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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
https://github.com/SuperMuel/Syncademic/blob/4ce1430d8ca3401665f4148274fc3ea6a155574c/backend/api.py#L13
The new UserInfo model inherits from CamelCaseModel, but the file imports it using from backend.backend.models.base import CamelCaseModel. The package only exposes backend.models.*; there is no backend.backend submodule. Importing this module will raise ModuleNotFoundError during application startup, preventing the API from booting.
ℹ️ 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
| "Authorizing backend.", | ||
| extra={ | ||
| "user_id": user_id, | ||
| "redirect_uri": str(request.redirectUri), | ||
| "provider_account_id": request.providerAccountId, | ||
| "request": request.model_dump_json(), | ||
| "redirect_uri": str(request.redirect_uri), |
There was a problem hiding this comment.
The authorization endpoint now logs request.model_dump_json() in the extra dict. Because AuthorizeBackendInput includes the OAuth auth_code, the change records the transient authorization code in application logs for every request. These codes are effectively credentials and should not be logged under normal circumstances. Consider logging only non‑sensitive fields (e.g. provider account ID and redirect URI) or masking the code before logging.
Useful? React with 👍 / 👎.
SuperMuel
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/backend/models/schemas.py (1)
74-99: Bug risk: redirect_uri validator may compare HttpUrl to strIf settings.* URIs are plain strings,
uri not in ALLOWED_REDIRECT_URISwill always be True. Normalize to string for both sides (and prefer a set).-ALLOWED_REDIRECT_URIS = [settings.LOCAL_REDIRECT_URI, settings.PRODUCTION_REDIRECT_URI] +ALLOWED_REDIRECT_URIS = { + str(settings.LOCAL_REDIRECT_URI), + str(settings.PRODUCTION_REDIRECT_URI), +} @@ @field_validator("redirect_uri") @classmethod def _validate_redirect_uri(cls, uri: HttpUrl) -> HttpUrl: """Validate that the redirect URI is allowed""" - if uri not in ALLOWED_REDIRECT_URIS: + if str(uri) not in ALLOWED_REDIRECT_URIS: raise ValueError(f"Invalid redirect URI: `{uri}`") return uri
🧹 Nitpick comments (4)
backend/tests/models/test_base.py (1)
21-24: Avoid brittle JSON string equality in testsAssert via json.loads(...) to a dict instead of raw string comparison; JSON key order/separators can change across versions.
- assert model.model_dump_json() == '{"fooBar":1,"spam":"eggs"}' + import json + assert json.loads(model.model_dump_json()) == {"fooBar": 1, "spam": "eggs"}backend/backend/services/dev_notification_service.py (1)
130-134: Guard against missing schedule_source/urlIf either is None, this will raise. Use a safe fallback to avoid notification failures.
- return f"Profile Title: <code>{sync_profile.title}</code>\nSchedule Source URL: <code>{sync_profile.schedule_source.url}</code>" + url = getattr(getattr(sync_profile, "schedule_source", None), "url", None) or "N/A" + return ( + f"Profile Title: <code>{sync_profile.title}</code>\n" + f"Schedule Source URL: <code>{url}</code>" + )Optionally add a unit test for a profile without a schedule_source.
backend/backend/services/sync_profile_service.py (2)
215-222: Metadata: serialize enums to plain strings for logsPass .value to ensure metadata is JSON/struct-log friendly; avoids Enum objects in logs/telemetry.
- metadata={ + metadata={ "sync_profile_id": profile.id, "user_id": user_id, - "sync_trigger": sync_trigger, - "sync_type": sync_type, + "sync_trigger": sync_trigger.value, + "sync_type": sync_type.value, "source": profile.schedule_source.model_dump(), },
517-525: Use parameterized logging (avoid f-strings)Unify with the PR’s logging goal; use placeholders for deferred formatting.
- logger.info( - f"Validating existing target calendar: {calendar_id}", - extra={"user_id": user_id}, - ) + logger.info( + "Validating existing target calendar: %s", + calendar_id, + extra={"user_id": user_id}, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/backend/api.py(2 hunks)backend/backend/models/authorization.py(1 hunks)backend/backend/models/base.py(1 hunks)backend/backend/models/schemas.py(6 hunks)backend/backend/models/sync_profile.py(5 hunks)backend/backend/models/user.py(2 hunks)backend/backend/repositories/backend_authorization_repository.py(1 hunks)backend/backend/services/ai_ruleset_service.py(2 hunks)backend/backend/services/authorization_service.py(3 hunks)backend/backend/services/dev_notification_service.py(1 hunks)backend/backend/services/ics_service.py(1 hunks)backend/backend/services/sync_profile_service.py(10 hunks)backend/main.py(6 hunks)backend/tests/models/test_authorization.py(4 hunks)backend/tests/models/test_base.py(1 hunks)backend/tests/models/test_schemas.py(9 hunks)backend/tests/models/test_sync_profile.py(11 hunks)backend/tests/repositories/test_backend_authorization_repository.py(2 hunks)backend/tests/repositories/test_mock_sync_profile_repository.py(1 hunks)backend/tests/repositories/test_sync_profile_repository.py(2 hunks)backend/tests/services/test_ai_ruleset_service.py(1 hunks)backend/tests/services/test_ics_service.py(3 hunks)backend/tests/services/test_sync_profile_service.py(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/tests/services/test_ics_service.pybackend/tests/models/test_base.pybackend/tests/repositories/test_backend_authorization_repository.pybackend/backend/services/dev_notification_service.pybackend/backend/models/base.pybackend/backend/api.pybackend/tests/repositories/test_sync_profile_repository.pybackend/backend/services/sync_profile_service.pybackend/tests/services/test_ai_ruleset_service.pybackend/backend/models/user.pybackend/backend/services/authorization_service.pybackend/backend/models/sync_profile.pybackend/backend/services/ai_ruleset_service.pybackend/backend/models/authorization.pybackend/main.pybackend/backend/repositories/backend_authorization_repository.pybackend/tests/models/test_sync_profile.pybackend/tests/models/test_schemas.pybackend/backend/services/ics_service.pybackend/tests/repositories/test_mock_sync_profile_repository.pybackend/tests/services/test_sync_profile_service.pybackend/backend/models/schemas.pybackend/tests/models/test_authorization.py
backend/tests/**/*.py
📄 CodeRabbit inference engine (backend/AGENTS.md)
Use pytest.mark.parametrize when testing multiple inputs
Files:
backend/tests/services/test_ics_service.pybackend/tests/models/test_base.pybackend/tests/repositories/test_backend_authorization_repository.pybackend/tests/repositories/test_sync_profile_repository.pybackend/tests/services/test_ai_ruleset_service.pybackend/tests/models/test_sync_profile.pybackend/tests/models/test_schemas.pybackend/tests/repositories/test_mock_sync_profile_repository.pybackend/tests/services/test_sync_profile_service.pybackend/tests/models/test_authorization.py
🧬 Code graph analysis (18)
backend/tests/models/test_base.py (1)
backend/backend/models/base.py (3)
CamelCaseModel(7-23)model_dump(15-18)model_dump_json(20-23)
backend/tests/repositories/test_backend_authorization_repository.py (3)
backend/backend/models/base.py (1)
model_dump(15-18)backend/backend/repositories/backend_authorization_repository.py (5)
get_authorization(12-19)get_authorization(57-71)FirestoreBackendAuthorizationRepository(45-106)set_authorization(21-26)set_authorization(73-80)backend/backend/models/authorization.py (1)
BackendAuthorization(9-49)
backend/backend/api.py (1)
backend/backend/models/base.py (1)
CamelCaseModel(7-23)
backend/tests/repositories/test_sync_profile_repository.py (1)
backend/backend/models/sync_profile.py (6)
ScheduleSource(80-101)TargetCalendar(104-120)SyncProfileStatus(123-139)SyncProfileStatusType(26-53)SyncTrigger(56-66)SyncType(69-77)
backend/backend/services/sync_profile_service.py (4)
backend/backend/models/sync_profile.py (1)
to_ics_source(97-101)backend/backend/models/base.py (1)
model_dump(15-18)backend/backend/services/google_calendar_service.py (2)
create_new_calendar(66-113)get_calendar_by_id(115-147)backend/backend/services/authorization_service.py (1)
get_provider_account_email(272-282)
backend/tests/services/test_ai_ruleset_service.py (1)
backend/backend/models/sync_profile.py (6)
TargetCalendar(104-120)SyncProfileStatus(123-139)SyncProfileStatusType(26-53)SyncTrigger(56-66)SyncType(69-77)ScheduleSource(80-101)
backend/backend/models/user.py (1)
backend/backend/models/base.py (1)
CamelCaseModel(7-23)
backend/backend/models/sync_profile.py (2)
backend/backend/models/base.py (1)
CamelCaseModel(7-23)backend/backend/models/rules.py (1)
Ruleset(181-193)
backend/backend/services/ai_ruleset_service.py (2)
backend/backend/models/sync_profile.py (1)
to_ics_source(97-101)backend/backend/models/base.py (1)
model_dump(15-18)
backend/backend/models/authorization.py (2)
backend/backend/models/base.py (1)
CamelCaseModel(7-23)backend/backend/models/schemas.py (1)
_validate_provider(86-87)
backend/main.py (4)
backend/backend/services/sync_profile_service.py (1)
delete_sync_profile(342-426)backend/backend/services/exceptions/base.py (1)
SyncademicError(4-17)backend/backend/models/base.py (1)
model_dump_json(20-23)backend/backend/services/authorization_service.py (1)
authorize_backend_with_auth_code(49-153)
backend/tests/models/test_sync_profile.py (1)
backend/backend/models/sync_profile.py (4)
SyncProfileStatus(123-139)SyncProfileStatusType(26-53)SyncTrigger(56-66)SyncType(69-77)
backend/tests/models/test_schemas.py (2)
backend/backend/models/schemas.py (1)
ValidateIcsUrlOutput(14-24)backend/backend/models/sync_profile.py (1)
SyncType(69-77)
backend/backend/services/ics_service.py (1)
backend/backend/models/schemas.py (1)
ValidateIcsUrlOutput(14-24)
backend/tests/repositories/test_mock_sync_profile_repository.py (1)
backend/backend/models/sync_profile.py (6)
ScheduleSource(80-101)TargetCalendar(104-120)SyncProfileStatus(123-139)SyncProfileStatusType(26-53)SyncTrigger(56-66)SyncType(69-77)
backend/tests/services/test_sync_profile_service.py (3)
backend/backend/models/sync_profile.py (2)
ScheduleSource(80-101)TargetCalendar(104-120)backend/backend/services/authorization_service.py (1)
test_authorization(238-270)backend/backend/models/schemas.py (1)
CreateSyncProfileInput(139-154)
backend/backend/models/schemas.py (2)
backend/backend/models/base.py (1)
CamelCaseModel(7-23)backend/backend/models/sync_profile.py (2)
ScheduleSource(80-101)SyncType(69-77)
backend/tests/models/test_authorization.py (1)
backend/backend/models/authorization.py (1)
BackendAuthorization(9-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (41)
backend/backend/models/base.py (1)
7-23: CamelCaseModel looks solidConfig and overrides are correct and minimal; aligns with Pydantic v2 best practices.
backend/backend/api.py (1)
64-70: UserInfo moved to CamelCaseModelGood switch; responses should serialize with camelCase aliases by default.
Please verify with a quick integration test that the /secure response JSON uses camelCase keys (e.g., emailVerified) when served by FastAPI.
backend/tests/services/test_ics_service.py (1)
156-160: Tests updated to snake_case nb_eventsMatches model rename; assertions look correct.
Also applies to: 176-181, 195-199
backend/backend/repositories/backend_authorization_repository.py (1)
77-77: LGTM! Field name updated correctly.The change from
backend_auth.userIdandbackend_auth.providerAccountIdto snake_case attributes is consistent with the BackendAuthorization model's migration to CamelCaseModel and aligns with the parameter naming in other methods of this class.backend/backend/services/ics_service.py (1)
165-166: LGTM! Output field names updated correctly.The field name changes from
nbEventstonb_eventsare consistent with the ValidateIcsUrlOutput schema migration to snake_case, as confirmed in the relevant code snippets.Also applies to: 171-171
backend/backend/services/ai_ruleset_service.py (1)
77-77: LGTM! SyncProfile attribute access updated correctly.The changes from
scheduleSourcetoschedule_sourceare consistent with the SyncProfile model's migration to snake_case attributes. The metadata construction and ICS source retrieval logic remain unchanged.Also applies to: 81-81, 109-109
backend/backend/models/user.py (1)
4-6: LGTM! Models migrated to CamelCaseModel base.The migration of UserProviderData, UserMetadata, and User from BaseModel to CamelCaseModel is consistent with the broader refactor to support snake_case Python attributes with automatic camelCase JSON aliasing. Field names remain unchanged as they were already snake_case.
Also applies to: 9-9, 18-18, 25-25
backend/tests/models/test_authorization.py (1)
13-19: LGTM! Test fixtures updated to snake_case.All test fixtures and assertions have been correctly updated to use snake_case field names (
user_id,provider_account_id,provider_account_email,access_token,refresh_token,expiration_date), matching the BackendAuthorization model's migration to CamelCaseModel-based attributes.Also applies to: 25-30, 52-62, 68-74, 83-87
backend/main.py (3)
197-197: LGTM! Request field access updated to snake_case.The changes from
request.providerAccountIdtorequest.provider_account_idare consistent with the schema migrations. Both functional code and logging extras have been updated correctly.Also applies to: 206-206, 222-222
249-250: LGTM! Sync profile request fields updated correctly.The changes from
request.syncProfileIdandrequest.syncTypeto snake_case equivalents align with the RequestSyncInput and DeleteSyncProfileInput schema changes. All references in logging and service calls are consistent.Also applies to: 330-330, 336-336, 344-344
362-365: LGTM! Authorization request fields updated correctly.The changes to
request.auth_code,request.redirect_uri, andrequest.provider_account_idare consistent with the AuthorizeBackendInput schema migration. Note thatrequest.model_dump_json()on line 362 will now serialize with camelCase keys due to CamelCaseModel's defaultby_alias=Truebehavior.Also applies to: 370-372, 379-380
backend/tests/models/test_sync_profile.py (2)
20-26: LGTM! Test fixtures and attribute access updated to snake_case.The fixture definitions and attribute access have been correctly updated to snake_case (
schedule_source,target_calendar,provider_account_id,provider_account_email,sync_trigger,sync_type), matching the SyncProfile model's migration to CamelCaseModel.Also applies to: 28-32, 43-50
255-257: Correct usage of camelCase for deserialization test.The test data at lines 255-257 correctly uses camelCase keys (
scheduleSource,targetCalendar) to test thatmodel_validate()can deserialize from JSON with camelCase field names, thanks to thepopulate_by_name=Truesetting in CamelCaseModel.backend/tests/models/test_schemas.py (1)
35-36: LGTM! Test assertions updated to snake_case.All test assertions have been correctly updated to use snake_case field names (
nb_events,provider_account_id,color_id,sync_profile_id,sync_type,auth_code,redirect_uri), while test input data correctly continues to use camelCase keys to verify deserialization through CamelCaseModel'spopulate_by_name=Trueconfiguration.Also applies to: 43-43, 68-68, 82-82, 105-107, 119-121, 168-169, 184-184, 213-216, 257-257
backend/backend/services/authorization_service.py (2)
142-151: LGTM!The BackendAuthorization construction correctly uses snake_case field names, consistent with the model migration to CamelCaseModel.
214-221: LGTM!The attribute access correctly uses snake_case field names throughout (
access_token,refresh_token,expiration_date,provider_account_email), consistent with the BackendAuthorization model updates.Also applies to: 282-282
backend/tests/services/test_ai_ruleset_service.py (1)
69-86: LGTM!The test fixture correctly uses snake_case field names (
target_calendar,provider_account_id,provider_account_email,sync_trigger,sync_type,schedule_source) throughout, consistent with the SyncProfile model migration.backend/tests/repositories/test_mock_sync_profile_repository.py (1)
27-45: LGTM!The test fixture correctly uses snake_case field names (
schedule_source,target_calendar,provider_account_id,provider_account_email,sync_trigger,sync_type) consistent with the model migration.backend/tests/repositories/test_backend_authorization_repository.py (3)
37-53: LGTM!The test correctly uses snake_case field names for BackendAuthorization construction and attribute access, consistent with the model migration.
71-76: Good serialization verification.Lines 75-76 correctly verify that the serialized form uses camelCase keys (
accessToken,refreshToken), confirming that CamelCaseModel properly aliases snake_case Python attributes to camelCase JSON for Firestore compatibility.
58-94: LGTM!All BackendAuthorization constructions and attribute accesses correctly use snake_case field names throughout the test file.
Also applies to: 107-137
backend/tests/services/test_sync_profile_service.py (2)
46-60: LGTM!The helper function correctly uses snake_case field names (
schedule_source,target_calendar,provider_account_id,provider_account_email) consistent with the model migration.
944-989: LGTM!All test cases correctly use snake_case field names for model construction and attribute access (
schedule_source,target_calendar,color_id,calendar_id,provider_account_id,provider_account_email), maintaining consistency with the CamelCaseModel migration throughout.Also applies to: 1002-1008, 1045-1061, 1087-1099, 1120-1136
backend/tests/repositories/test_sync_profile_repository.py (2)
33-54: LGTM!The test fixture correctly uses snake_case field names throughout (
schedule_source,target_calendar,provider_account_id,provider_account_email,sync_trigger,sync_type,updated_at,last_successful_sync), consistent with the SyncProfile model migration.
92-98: LGTM!The status update correctly uses snake_case field names (
updated_at,last_successful_sync), maintaining consistency with the CamelCaseModel migration.backend/backend/models/authorization.py (2)
4-6: LGTM!The model correctly migrates from BaseModel to CamelCaseModel, enabling snake_case Python attributes with camelCase JSON serialization for Firestore compatibility.
Also applies to: 9-9
14-43: LGTM!All fields correctly renamed to snake_case (
user_id,provider_account_id,provider_account_email,access_token,refresh_token,expiration_date), and the validator properly targets the renamedexpiration_datefield. The docstring accurately reflects the updated field names.backend/backend/models/sync_profile.py (2)
17-17: LGTM!All model classes correctly migrated from BaseModel to CamelCaseModel (
ScheduleSource,TargetCalendar,SyncProfileStatus,SyncProfile), enabling consistent snake_case Python attributes with camelCase JSON serialization.Also applies to: 80-80, 104-104, 123-123, 158-158
112-120: LGTM!All fields correctly renamed to snake_case (
provider_account_id,provider_account_email,sync_trigger,sync_type,updated_at,schedule_source,target_calendar,last_successful_sync) with corresponding docstring updates. The migration is comprehensive and maintains consistency across all nested models.Also applies to: 128-139, 166-191
backend/backend/services/sync_profile_service.py (5)
127-131: Good: status now captures trigger and typeCapturing sync_trigger and sync_type in status improves traceability.
151-153: Good: snake_case accessors for calendar managerprovider_account_id and calendar_id wired correctly from target_calendar.
190-190: Good: timezone-aware last_successful_syncUses UTC-aware datetime; avoids tz bugs.
549-551: Good: provider account fields now snake_caseprovider_account_id and provider_account_email set on TargetCalendar as expected.
377-379: Good: deletion path also uses snake_case identifiersAuthorization aligned in delete flow.
backend/backend/models/schemas.py (7)
10-18: Schemas moved to CamelCaseModel + nb_events renameLooks correct; JSON will expose camelCase via aliases by default.
27-41: Good: snake_case provider_account_id across list/authorizeInputs/outputs consistent; matches services and repos.
43-56: Good: CreateNewCalendarInput with provider_account_id and color_id constraintsRange checks and defaults look right.
58-68: Good: RequestSync/DeleteSyncProfile use sync_profile_id and SyncTypeAligns with service method signatures.
102-124: Good: CreateNewTargetCalendarInput with color_id coercionString-to-int coercion before validation is handy; constraints fit Google’s range.
127-137: Good: UseExistingTargetCalendarInput snake_case fieldsMatches service layer lookups.
139-154: Approve discriminated union and field naming
CreateSyncProfileInput correctly uses a discriminated union, and no legacy camelCase fields remain.
| logger.info( | ||
| "Creating new target calendar.", | ||
| extra={"user_id": user_id}, | ||
| ) | ||
| cal_result = self._google_calendar_service.create_new_calendar( | ||
| user_id=user_id, | ||
| provider_account_id=request.targetCalendar.providerAccountId, | ||
| provider_account_id=request.target_calendar.provider_account_id, | ||
| summary=request.title, | ||
| description=f"Syncademic calendar for '{request.title}' ({sync_profile_id=})", | ||
| color_id=request.targetCalendar.colorId, | ||
| color_id=request.target_calendar.color_id, | ||
| ) | ||
| calendar_id = cal_result["id"] |
There was a problem hiding this comment.
Prevent orphaned calendars: prefetch email before creation (and consider rollback)
If get_provider_account_email later fails, a new calendar is already created, leaving an orphan. Prefetch the email before branching to reduce this risk. Optionally add a compensating delete on failure.
@@
logger.info(
"Authorization test successful.",
extra={
"user_id": user_id,
"provider_account_id": request.target_calendar.provider_account_id,
},
)
+ # Prefetch to avoid failing after calendar creation
+ provider_account_email = self._authorization_service.get_provider_account_email(
+ user_id, request.target_calendar.provider_account_id
+ )
@@
if request.target_calendar.type == "createNew":
@@
cal_result = self._google_calendar_service.create_new_calendar(
user_id=user_id,
provider_account_id=request.target_calendar.provider_account_id,
summary=request.title,
description=f"Syncademic calendar for '{request.title}' ({sync_profile_id=})",
color_id=request.target_calendar.color_id,
)And when building TargetCalendar (see below), use the prefetched value:
- provider_account_email=self._authorization_service.get_provider_account_email(
- user_id, request.target_calendar.provider_account_id
- ),
+ provider_account_email=provider_account_email,If you want, I can propose a safe rollback (delete calendar on later failure) once a delete API exists in GoogleCalendarService. Based on coding guidelines.
📝 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.
| logger.info( | |
| "Creating new target calendar.", | |
| extra={"user_id": user_id}, | |
| ) | |
| cal_result = self._google_calendar_service.create_new_calendar( | |
| user_id=user_id, | |
| provider_account_id=request.targetCalendar.providerAccountId, | |
| provider_account_id=request.target_calendar.provider_account_id, | |
| summary=request.title, | |
| description=f"Syncademic calendar for '{request.title}' ({sync_profile_id=})", | |
| color_id=request.targetCalendar.colorId, | |
| color_id=request.target_calendar.color_id, | |
| ) | |
| calendar_id = cal_result["id"] | |
| logger.info( | |
| "Authorization test successful.", | |
| extra={ | |
| "user_id": user_id, | |
| "provider_account_id": request.target_calendar.provider_account_id, | |
| }, | |
| ) | |
| # Prefetch to avoid failing after calendar creation | |
| provider_account_email = self._authorization_service.get_provider_account_email( | |
| user_id, | |
| request.target_calendar.provider_account_id, | |
| ) | |
| if request.target_calendar.type == "createNew": | |
| logger.info( | |
| "Creating new target calendar.", | |
| extra={"user_id": user_id}, | |
| ) | |
| cal_result = self._google_calendar_service.create_new_calendar( | |
| user_id=user_id, | |
| provider_account_id=request.target_calendar.provider_account_id, | |
| summary=request.title, | |
| description=f"Syncademic calendar for '{request.title}' ({sync_profile_id=})", | |
| color_id=request.target_calendar.color_id, | |
| ) | |
| calendar_id = cal_result["id"] | |
| # … later, when assembling the model: | |
| target_calendar = TargetCalendar( | |
| provider_account_email=provider_account_email, | |
| # … other fields … | |
| ) |
🤖 Prompt for AI Agents
In backend/backend/services/sync_profile_service.py around lines 495 to 506, the
code creates a new Google calendar before calling get_provider_account_email
which can fail and leave an orphaned calendar; prefetch the provider account
email (call get_provider_account_email) and validate it before creating the
calendar, then pass that prefetched email into TargetCalendar construction
instead of calling the getter later; additionally, add a compensating rollback
path: if any subsequent step fails after calendar creation and
GoogleCalendarService exposes a delete calendar method, call it to remove the
newly created calendar (ensure the delete is attempted in an exception
handler/finally block and log errors if rollback fails).
| schedule_source=request.schedule_source, | ||
| target_calendar=target_calendar, | ||
| status=SyncProfileStatus(type=SyncProfileStatusType.NOT_STARTED), | ||
| # ruleset and ruleset_error are None initially | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Good: SyncProfile uses schedule_source and target_calendar (snake_case)
Model construction matches the new schemas/base aliasing defaults.
Run this to catch any remaining camelCase field usages and logging f-strings across the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Scanning for legacy camelCase identifiers..."
patterns=(
'\btargetCalendar\b'
'\bscheduleSource\b'
'\bproviderAccountId\b'
'\bproviderAccountEmail\b'
'\bcalendarId\b'
'\bcolorId\b'
'\blastSuccessfulSync\b'
)
for p in "${patterns[@]}"; do
echo "Pattern: $p"
rg -nP -C2 --type=py "$p" || true
done
echo
echo "Scanning for f-strings in logging calls..."
rg -nP -C1 --type=py 'logger\.(debug|info|warning|error|exception|critical)\s*\(\s*f"' || trueLength of output: 25324
Convert remaining camelCase identifiers to snake_case
- Replace attribute references in admin pages:
•profile.targetCalendar→profile.target_calendar
•profile.scheduleSource→profile.schedule_source
•profile.lastSuccessfulSync→profile.last_successful_sync
•authorization.providerAccountEmail→authorization.provider_account_email
etc. - Update backend test fixtures and JSON payloads:
"scheduleSource","targetCalendar","providerAccountId","colorId", etc., to snake_case field names.
🤖 Prompt for AI Agents
In backend/backend/services/sync_profile_service.py around lines 559 to 563,
several camelCase identifiers are still being used; update all occurrences to
snake_case (e.g., profile.targetCalendar → profile.target_calendar,
profile.scheduleSource → profile.schedule_source, profile.lastSuccessfulSync →
profile.last_successful_sync, authorization.providerAccountEmail →
authorization.provider_account_email) and ensure any object construction,
attribute access, tests, fixtures, and JSON payloads reference the new
snake_case names (also convert keys like "scheduleSource", "targetCalendar",
"providerAccountId", "colorId" in fixtures/payloads to "schedule_source",
"target_calendar", "provider_account_id", "color_id"); run tests and update
serializers/deserializers or schema mappings that transform incoming/outgoing
JSON to maintain compatibility.
2 participants
Summary
Summary by CodeRabbit