Added logs for User and Creds Endpoint #322
Conversation
WalkthroughAdds module-level loggers and logging calls across API routes, core utils, provider validation, and CRUD layers; credentials CRUD now validates providers/fields, encrypts credentials before storage, adds retrieval helpers, and updates Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as "API Route\n(handlers)"
participant Core as "CRUD / Core\n(validators, encrypt)"
participant DB as "Database"
participant Log as "Logger"
Client->>API: request (create/update/read/delete)
API->>Core: validate input, check org/project, call provider validators
alt validation fails
Core->>Log: error (provider/fields/org/project)
Core-->>API: raise error
API-->>Client: HTTP error
else validation passes
Core->>Core: encrypt credentials (if applicable)
Core->>DB: persist (insert/update/delete)
alt DB error (IntegrityError)
DB->>Core: error
Core->>DB: rollback
Core->>Log: error (org/project/provider, exc)
Core-->>API: raise error
API-->>Client: HTTP error
else success
DB-->>Core: success
Core->>Log: info (created/updated/removed with ids)
Core-->>API: result
API-->>Client: 200/201 + payload
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
backend/app/utils.py (1)
191-198: Avoid leaking internal exception details in HTTP 500 responseReturning str(e) to clients can leak sensitive info. Log the full exception (with stack), but return a generic message.
- logger.error( - f"[get_openai_client] Failed to configure OpenAI client. | project_id: {project_id} | error: {str(e)}", - exc_info=True, - ) - raise HTTPException( - status_code=500, - detail=f"Failed to configure OpenAI client: {str(e)}", - ) + logger.error( + "[get_openai_client] Failed to configure OpenAI client. | project_id=%s", + project_id, + exc_info=True, + ) + raise HTTPException( + status_code=500, + detail="Failed to configure OpenAI client.", + )backend/app/core/util.py (1)
42-51: Fix type hints: return Optional client on failureBoth functions return (None, False) on failure, which doesn’t match the current signature.
-def configure_langfuse(credentials: dict) -> tuple[Langfuse, bool]: +def configure_langfuse(credentials: dict) -> tuple[Langfuse | None, bool]:-def configure_openai(credentials: dict) -> tuple[OpenAI, bool]: +def configure_openai(credentials: dict) -> tuple[OpenAI | None, bool]:Also applies to: 76-85
🧹 Nitpick comments (13)
backend/app/utils.py (1)
180-182: Recalibrate log level and use parameterized logging to avoid string interpolation overheadThis is a client/config error returning 400. Consider logging at warning (not error) and use parameterized logging with contextual fields.
- logger.error( - f"[get_openai_client] OpenAI credentials not found. | project_id: {project_id}" - ) + logger.warning( + "[get_openai_client] OpenAI credentials not found. | org_id=%s | project_id=%s", + org_id, + project_id, + )backend/app/core/util.py (2)
20-26: Include traceback and parameterized logging for unknown exception handlerAdd exc_info and avoid eager f-string formatting. Consider keeping client detail generic to prevent info leaks.
- logger.warning( - 'Unexpected exception "{}": {}'.format( - type(error).__name__, - error, - ) - ) + logger.warning( + 'Unexpected exception "%s": %s', + type(error).__name__, + error, + exc_info=True, + ) - raise HTTPException(status_code=status_code, detail=str(error)) + raise HTTPException(status_code=status_code, detail="Unexpected error")
72-73: Log with traceback for client configuration errorsInclude stack traces for easier diagnosis and avoid f-strings in logging.
- logger.error(f"Failed to configure Langfuse: {str(e)}") + logger.error("Failed to configure Langfuse: %s", e, exc_info=True)- logger.error(f"Failed to configure OpenAI client: {str(e)}") + logger.error("Failed to configure OpenAI client: %s", e, exc_info=True)Also applies to: 94-95
backend/app/core/providers.py (1)
52-54: Log level and parameterization for validation errorsThese are client-side validation failures; warning is more appropriate than error. Also use parameterized logging.
- logger.error( - f"[validate_provider] Unsupported provider: {provider}. Supported providers are: {supported}" - ) + logger.warning( + "[validate_provider] Unsupported provider: %s | supported=%s", + provider, + supported, + )- logger.error( - f"[validate_provider_credentials] Missing required fields for {provider}: {', '.join(missing_fields)}" - ) + logger.warning( + "[validate_provider_credentials] Missing required fields | provider=%s | missing=%s", + provider, + ", ".join(missing_fields), + )Also applies to: 76-78
backend/app/api/routes/users.py (1)
188-188: Consider log level alignment for 404/403 cases
- 404 not found (by id) often merits info/warning, not error (noise in error budgets).
- 403 self-delete attempts is misuse → error is fine.
- logger.error(f"[update_user_endpoint] User with id {user_id} not found") + logger.warning("[update_user_endpoint] User not found | user_id=%s", user_id)- logger.error(f"[delete_user] User with id {user_id} not found") + logger.warning("[delete_user] User not found | user_id=%s", user_id)Keep:
logger.error("[delete_user] Attempt to delete self by superuser")Also applies to: 217-217, 221-221
backend/app/api/routes/credentials.py (3)
1-1: Good: module-level logger added; consider structured, lazy logging
- Keep the logger; it’s needed. Prefer structured logging (extra=...) and lazy formatting instead of f-strings to avoid eager interpolation and to improve log parsing.
Example:
-logger.error(f"[read_credential] No credentials found for organization {org_id}, project_id {project_id}") +logger.error( + "[read_credential] No credentials found", + extra={"org_id": org_id, "project_id": project_id} +)Also applies to: 23-23
39-46: Project/org validation: truthiness check and log level
- Use
is not Noneforproject_idto avoid false negatives if0is ever a valid ID.- This is a client error; consider
logger.warninginstead oflogger.errorto reduce error-noise for expected 4xx paths.-# Validate project if provided -if creds_in.project_id: +// Validate project if provided +if creds_in.project_id is not None: project = validate_project(session, creds_in.project_id) if project.organization_id != creds_in.organization_id: - logger.error( + logger.warning( f"[create_new_credential] Project {creds_in.project_id} does not belong to organization {creds_in.organization_id}" )
91-93: Use warning/info for expected 4xx outcomes to reduce error noise
- Not-found and bad-input cases are expected control-flow in APIs. Consider
logger.warning(orinfoif very common) instead oferrorfor:
- read_credential: none found
- read_provider_credential: none found
- update_credential: invalid input
- delete_provider_credential: none found
- delete_all_credentials: none found
Also applies to: 117-119, 135-135, 169-171, 195-197
backend/app/crud/credentials.py (5)
1-2: Modernize typing; align with Ruff hints (UP006/UP035)
- Prefer built-in generics (
list,dict) overtyping.List/typing.Dict. Update return annotations accordingly.-from typing import Optional, Dict, Any, List, Union +from typing import Optional, Any, Union ... -def set_creds_for_org(*, session: Session, creds_add: CredsCreate) -> List[Credential]: +def set_creds_for_org(*, session: Session, creds_add: CredsCreate) -> list[Credential]: ... -) -> Optional[Union[Dict[str, Any], Credential]]: +) -> Optional[Union[dict[str, Any], Credential]]: ... -def get_creds_by_org(... ) -> List[Credential]: +def get_creds_by_org(... ) -> list[Credential]: ... -def get_providers(... ) -> List[str]: +def get_providers(... ) -> list[str]: ... -def update_creds_for_org(... ) -> List[Credential]: +def update_creds_for_org(... ) -> list[Credential]: ... -def remove_creds_for_org(... ) -> List[Credential]: +def remove_creds_for_org(... ) -> list[Credential]:Also applies to: 18-18
23-26: CRUD layer should avoid HTTP concerns; raise domain errors
- Logging is fine. Prefer raising a domain exception (e.g., ValueError or a custom
CredentialsValidationError) here and let the API layer translate to HTTP. Currently this mixes HTTP in CRUD in some places and ValueError in others.
52-55: Preferlogger.exceptionand avoid duplicating exception text
logger.exception("msg")automatically includes stack trace. Avoid bothstr(e)in the message andexc_info=Truewhich duplicates info.-logger.error( - f"[set_creds_for_org] Integrity error while adding credentials | organization_id {creds_add.organization_id}, project_id {creds_add.project_id}, provider {provider}: {str(e)}", - exc_info=True, -) +logger.exception( + "[set_creds_for_org] Integrity error while adding credentials", + extra={"organization_id": creds_add.organization_id, "project_id": creds_add.project_id, "provider": provider}, +)
59-61: Consider atomicity for multi-provider create; enhance success log
- If multiple providers are created in one call, current per-provider commit can leave partial state on failure. Consider wrapping the loop in a transaction so the operation is atomic.
- Include count/providers in the success log for easier traceability.
-logger.info( - f"[set_creds_for_org] Successfully created credentials | organization_id {creds_add.organization_id}, project_id {creds_add.project_id}" -) +logger.info( + "[set_creds_for_org] Successfully created credentials", + extra={ + "organization_id": creds_add.organization_id, + "project_id": creds_add.project_id, + "provider_count": len(created_credentials), + "providers": [c.provider for c in created_credentials], + }, +)
162-166: CRUD not-found: avoid raising HTTPException from data layer
- Logging is good. For separation of concerns, return
Noneor raise a domain-specificNotFoundand let the API layer map it toHTTPException(404, ...). This keeps CRUD reusable outside HTTP contexts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/api/routes/credentials.py(11 hunks)backend/app/api/routes/users.py(11 hunks)backend/app/core/providers.py(3 hunks)backend/app/core/util.py(4 hunks)backend/app/crud/credentials.py(6 hunks)backend/app/crud/user.py(3 hunks)backend/app/utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/app/api/routes/credentials.py (1)
backend/app/tests/api/routes/test_creds.py (4)
test_update_credentials(183-217)test_set_credential(32-62)test_update_credentials_failed_update(220-260)test_set_credentials_for_invalid_project_org_relationship(65-87)
backend/app/crud/user.py (3)
backend/app/core/security.py (2)
get_password_hash(93-103)verify_password(79-90)backend/app/models/user.py (3)
User(48-60)UserCreate(16-17)UserUpdate(27-29)backend/app/tests/crud/test_user.py (1)
test_update_user(79-91)
backend/app/core/providers.py (2)
backend/app/tests/core/test_providers.py (2)
test_validate_provider_credentials_missing_fields(17-31)test_validate_provider_invalid(9-14)backend/app/tests/crud/test_credentials.py (1)
test_invalid_provider(187-200)
🪛 Ruff (0.12.2)
backend/app/core/providers.py
2-2: typing.Dict is deprecated, use dict instead
(UP035)
2-2: typing.List is deprecated, use list instead
(UP035)
2-2: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
backend/app/crud/credentials.py
2-2: typing.Dict is deprecated, use dict instead
(UP035)
2-2: typing.List is deprecated, use list instead
(UP035)
18-18: Use list instead of List for type annotation
Replace with list
(UP006)
⏰ 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: checks (3.11.7, 6)
🔇 Additional comments (2)
backend/app/core/util.py (1)
12-12: LGTM: module-level logger setupLogger initialization looks good and aligns with the PR’s observability objective.
backend/app/crud/credentials.py (1)
174-176: No secrets are logged; only IDs and metadata are presentVerified that all logger calls matching
api_key,secret,token, orpasswordonly output non-sensitive identifiers or status messages—no actual secret values are ever included.Occurrences found:
- backend/app/crud/api_key.py:77 – logs only
api_key_id- backend/app/api/routes/users.py:113 – logs user email, not the password itself
- backend/app/api/routes/api_keys.py:93 – logs only
api_key_idInfo-level logging for successes and warnings for missing keys are appropriate and contain no decrypted credential material.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
backend/app/api/routes/users.py (10)
53-55: Lower severity for expected 400 and remove unnecessary f-string (Ruff F541)Attempting to create a user with an existing email is a client error (400/409). Prefer warning over error. Also, f-string has no placeholders; Ruff flags this (F541).
Apply this diff:
- logger.error( - f"[create_user_endpoint] Attempt to create user with existing email" - ) + logger.warning( + "[create_user_endpoint] Attempt to create user with existing email" + )Run this to ensure no remaining f-strings in logging calls across the repo:
#!/bin/bash # Find f-strings used directly in logger calls rg -nP -C2 '\blogger\.(debug|info|warning|error|exception|critical)\s*\(\s*f"' --glob '!**/node_modules/**' --glob '!**/dist/**'
82-82: Use warning and drop f-prefix (F541) for conflict caseUpdating to an existing email results in 409 (expected client error). Also remove the extraneous f.
- logger.error(f"[update_user_me] Attempt to update user with existing email") + logger.warning("[update_user_me] Attempt to update user with existing email")
91-91: Parameterize log arguments instead of f-stringsPrefer parameterized logging for consistency and to defer formatting.
- logger.info(f"[update_user_me] User updated with id: {current_user.id}") + logger.info("[update_user_me] User updated | user_id=%s", current_user.id)
111-111: Parameterize log argumentsSame rationale as above; avoid f-strings in logger calls.
- logger.info(f"[update_password_me] Password updated for user: {current_user.id}") + logger.info("[update_password_me] Password updated | user_id=%s", current_user.id)
129-129: Parameterize log argumentsAlign with parameterized logging style.
- logger.info(f"[delete_user_me] User deleted: {current_user.id}") + logger.info("[delete_user_me] User deleted | user_id=%s", current_user.id)
143-143: Lower severity for expected 400 and drop f-prefix (F541)Registration conflict is a normal client error (400). Use warning and remove f-string.
- logger.error(f"[register_user] Attempt to create user with existing email") + logger.warning("[register_user] Attempt to create user with existing email")
184-184: Use warning for 404 and parameterizeNot-found is an expected client error; prefer warning. Parameterize to avoid eager formatting.
- logger.error(f"[update_user_endpoint] User with id {user_id} not found") + logger.warning("[update_user_endpoint] User not found | user_id=%s", user_id)
193-195: Use warning and remove f-prefix (F541) for conflict caseEmail conflict on update is a client error. Use warning; remove f-string with no placeholders.
- logger.error( - f"[update_user_endpoint] Attempt to update user with existing email" - ) + logger.warning( + "[update_user_endpoint] Attempt to update user with existing email" + )
213-213: Use warning and parameterize for 404Deleting a non-existent user is an expected condition; log as warning and parameterize.
- logger.error(f"[delete_user] User with id {user_id} not found") + logger.warning("[delete_user] User not found | user_id=%s", user_id)
224-224: Parameterize log argumentsSwitch to parameterized logging for consistency.
- logger.info(f"[delete_user] User deleted: {user.id}") + logger.info("[delete_user] User deleted | user_id=%s", user.id)
🧹 Nitpick comments (2)
backend/app/api/routes/users.py (2)
123-123: Keep error severity; consider adding contextPrivilege-violation attempt is correctly logged as error. Consider adding user_id for traceability.
- logger.error("[delete_user_me] Attempt to delete superuser account by itself") + logger.error( + "[delete_user_me] Attempt to delete superuser account by itself | user_id=%s", + current_user.id, + )
217-217: Keep error severity; consider adding user contextSelf-deletion attempt by superuser is misuse; error is appropriate. Consider logging the user_id.
- logger.error("[delete_user] Attempt to delete self by superuser") + logger.error( + "[delete_user] Attempt to delete self by superuser | user_id=%s", + current_user.id, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/app/api/routes/credentials.py(10 hunks)backend/app/api/routes/users.py(11 hunks)backend/app/crud/user.py(3 hunks)backend/app/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/utils.py
- backend/app/api/routes/credentials.py
- backend/app/crud/user.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/api/routes/users.py (4)
backend/app/crud/collection.py (1)
delete(107-115)backend/app/crud/document.py (1)
delete(124-133)backend/app/tests/api/routes/test_users.py (3)
test_update_password_me(173-209)test_update_user(306-329)test_update_user_me(150-170)backend/app/api/routes/project_user.py (1)
add_user(23-53)
🪛 Ruff (0.12.2)
backend/app/api/routes/users.py
54-54: f-string without any placeholders
Remove extraneous f prefix
(F541)
82-82: f-string without any placeholders
Remove extraneous f prefix
(F541)
143-143: f-string without any placeholders
Remove extraneous f prefix
(F541)
194-194: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ 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: checks (3.11.7, 6)
🔇 Additional comments (2)
backend/app/api/routes/users.py (2)
1-1: LGTM: proper logging import addedImporting logging at module level is appropriate for route-level logging.
29-29: LGTM: module-level logger initializationlogger = logging.getLogger(name) is the right pattern for namespaced logs.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/crud/credentials.py (2)
72-85: Bug: get_key_by_org inspects encrypted string; decrypt before accessing api_keycreds.credential is stored encrypted. Checking
"api_key" in creds.credentialwill always fail and returns None. Decrypt first, handle errors, and return the key.def get_key_by_org( @@ ) -> Optional[str]: @@ - creds = session.exec(statement).first() - - if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] - - return None + creds = session.exec(statement).first() + if not creds or not creds.credential: + return None + try: + decrypted = decrypt_credentials(creds.credential) + except ValueError as e: + logger.error( + "[get_key_by_org] Failed to decrypt credentials", + extra={"organization_id": org_id, "provider": provider, "project_id": project_id}, + exc_info=True, + ) + return None + # Return the API key if present + return decrypted.get("api_key")
191-199: Guard against missing provider credentials before soft deleteIf creds is None, subsequent attribute access will raise. Handle 404 here to make the function safer for reuse beyond the route layer.
- creds = session.exec(statement).first() - - # Soft delete - creds.is_active = False + creds = session.exec(statement).first() + if creds is None: + logger.error( + "[remove_provider_credential] Credentials not found", + extra={"organization_id": org_id, "provider": provider, "project_id": project_id}, + ) + raise HTTPException(status_code=404, detail="Credentials not found for this provider") + + # Soft delete + creds.is_active = False
♻️ Duplicate comments (5)
backend/app/core/providers.py (1)
2-2: Adopt built‑in generics and drop unused Optional import (Ruff UP035, F401)This file still uses typing.List/Dict and imports Optional which is unused. Switch to built-ins and remove the unused import. This mirrors prior review guidance.
-from typing import Dict, List, Optional +from typing import Optional # If not used elsewhere, remove this line @@ - required_fields: List[str] + required_fields: list[str] @@ -PROVIDER_CONFIGS: Dict[Provider, ProviderConfig] = { +PROVIDER_CONFIGS: dict[Provider, ProviderConfig] = { @@ -def validate_provider_credentials(provider: str, credentials: Dict[str, str]) -> None: +def validate_provider_credentials(provider: str, credentials: dict[str, str]) -> None: @@ -def get_supported_providers() -> List[str]: +def get_supported_providers() -> list[str]:If Optional isn't referenced anywhere, delete its import line entirely.
Also applies to: 21-21, 25-25, 60-60, 84-84
backend/app/api/routes/credentials.py (3)
59-61: Nice: duplicate credentials logged as warning (keep 400 for tests)This matches earlier feedback and keeps test expectations intact.
153-165: Dead branch: validate_provider never returns falsy; convert to try/except and pass provider string to CRUDThe “if not provider_enum” path is unreachable. Catch ValueError and convert to HTTP 400. Also pass .value to CRUD for clarity.
- provider_enum = validate_provider(provider) - if not provider_enum: - raise HTTPException(status_code=400, detail="Invalid provider") + try: + provider_enum = validate_provider(provider) + except ValueError as e: + logger.error( + "[delete_provider_credential] Invalid provider", + extra={"organization_id": org_id, "project_id": project_id, "provider": provider}, + ) + raise HTTPException(status_code=400, detail=str(e)) @@ - provider=provider_enum, + provider=provider_enum.value,
73-77: Bug: raising built-in Exception drops status_code/detail; use HTTPExceptionClients will not receive a proper HTTP response body/status with Exception(...). Replace with FastAPI/Starlette HTTPException and keep structured logging.
- logger.error( - f"[create_new_credential] Failed to create credentials | organization_id: {creds_in.organization_id}, project_id: {creds_in.project_id}" - ) - raise Exception(status_code=500, detail="Failed to create credentials") + logger.error( + "[create_new_credential] Failed to create credentials", + extra={"organization_id": creds_in.organization_id, "project_id": creds_in.project_id}, + ) + raise HTTPException(status_code=500, detail="Failed to create credentials")backend/app/crud/credentials.py (1)
200-202: Consistent logging style: use extra fields for success logsApplies to update/remove/all-remove success logs to align with structured logging approach recommended above.
Example for update:
- logger.info( - f"[update_creds_for_org] Successfully updated credentials | organization_id {org_id}, provider {creds_in.provider}, project_id {creds_in.project_id}" - ) + logger.info( + "[update_creds_for_org] Successfully updated credentials", + extra={"organization_id": org_id, "provider": creds_in.provider, "project_id": creds_in.project_id}, + )Also applies to: 223-225, 174-176, 59-61
🧹 Nitpick comments (17)
backend/app/core/providers.py (2)
52-54: Prefer structured logging with extra={} for unsupported providerThe f-string embeds contextual fields into the message, which is harder to parse downstream. Use a stable message and attach fields via extra.
- logger.error( - f"[validate_provider] Unsupported provider | provider: {provider}, supported_providers: {supported}" - ) + logger.error( + "[validate_provider] Unsupported provider", + extra={"provider": provider, "supported_providers": supported}, + )
76-78: Likewise, attach missing_fields via extra rather than interpolatingStructured fields are easier to search/aggregate and avoid message churn.
- logger.error( - f"[validate_provider_credentials] Missing required fields | provider: {provider}, missing_fields: {', '.join(missing_fields)}" - ) + logger.error( + "[validate_provider_credentials] Missing required fields", + extra={"provider": provider, "missing_fields": missing_fields}, + )backend/app/api/routes/credentials.py (9)
42-44: Downgrade to warning for client-side validation errors“Project does not belong to organization” is a 400-level client error; logging it at error inflates error rates. Use warning and attach fields via extra.
- logger.error( - f"[create_new_credential] Project does not belong to organization | organization_id: {creds_in.organization_id}, project_id: {creds_in.project_id}" - ) + logger.warning( + "[create_new_credential] Project does not belong to organization", + extra={"organization_id": creds_in.organization_id, "project_id": creds_in.project_id}, + )
91-93: Not-found is expected; lower log level and add structured fields404 on read is common; prefer info or warning and attach fields via extra. Avoid error unless it indicates an internal failure.
- logger.error( - f"[read_credential] No credentials found | organization_id: {org_id}, project_id: {project_id}" - ) + logger.info( + "[read_credential] Credentials not found", + extra={"organization_id": org_id, "project_id": project_id}, + )
109-115: Be explicit: pass provider string value to CRUD and add structured logging consistentlyvalidate_provider returns a str Enum; pass .value to be explicit to SQLModel/typing.
- provider_creds = get_provider_credential( + provider_creds = get_provider_credential( session=session, org_id=org_id, - provider=provider_enum, + provider=provider_enum.value, project_id=project_id, )
117-119: 404 path: lower severity and attach fields via extraSame reasoning as the org-level 404.
- logger.error( - f"[read_provider_credential] No credentials found | organization_id: {org_id}, provider: {provider}" - ) + logger.info( + "[read_provider_credential] Provider credentials not found", + extra={"organization_id": org_id, "provider": provider, "project_id": project_id}, + )
135-135: Input validation: add structured context and avoid interpolated messageAttach org_id/provider presence to aid debugging without logging secrets.
- logger.error(f"[update_credential] Invalid input | organization_id: {org_id}") + logger.error( + "[update_credential] Invalid input", + extra={ + "organization_id": org_id, + "provider_provided": bool(getattr(creds_in, "provider", None)), + "credential_provided": bool(getattr(creds_in, "credential", None)), + }, + )
166-168: 404 path: tone down severity and add fields via extraAlign log level with expected-not-found semantics.
- logger.error( - f"[delete_provider_credential] Provider credentials not found | organization_id: {org_id}, provider: {provider}, project_id: {project_id}" - ) + logger.info( + "[delete_provider_credential] Provider credentials not found", + extra={"organization_id": org_id, "provider": provider, "project_id": project_id}, + )
191-197: Delete-all 404: log as info/warning with structured fieldsSame rationale as other not-found reads.
- logger.error( - f"[delete_all_credentials] Credentials not found | organization_id: {org_id}, project_id: {project_id}" - ) + logger.info( + "[delete_all_credentials] Credentials not found", + extra={"organization_id": org_id, "project_id": project_id}, + )
2-2: Use built-in generics in response_model and drop typing.List importModern Python + Pydantic work with list[...] in type params; removes the need for typing.List.
-from typing import List +from typing import Any # if needed elsewhere; otherwise remove @@ - response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]], @@ - response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]], @@ - response_model=APIResponse[List[CredsPublic]], + response_model=APIResponse[list[CredsPublic]],If List is otherwise unused, remove the import line entirely.
Also applies to: 31-31, 85-85, 128-128
51-57: Normalize provider keys when checking duplicatesInput may use mixed case ("OpenAI"); normalize to lowercase for the duplicate check to match stored provider values.
- for provider in creds_in.credential.keys(): + for provider in creds_in.credential.keys(): + provider = provider.lower() existing_cred = get_provider_credential( session=session, org_id=creds_in.organization_id, provider=provider, project_id=creds_in.project_id, )backend/app/crud/credentials.py (6)
23-26: Client input error: consider warning + structured fields“No credentials provided” is a 400; logging at warning with extra keeps error rates clean while preserving context.
- logger.error( - f"[set_creds_for_org] No credentials provided | project_id: {creds_add.project_id}" - ) + logger.warning( + "[set_creds_for_org] No credentials provided", + extra={"project_id": creds_add.project_id}, + )
51-55: Structure IntegrityError logs; include provider/org/project in extraAttaching fields via extra improves observability and avoids message formatting churn. exc_info=True is good—keep it.
- logger.error( - f"[set_creds_for_org] Integrity error while adding credentials | organization_id {creds_add.organization_id}, project_id {creds_add.project_id}, provider {provider}: {str(e)}", - exc_info=True, - ) + logger.error( + "[set_creds_for_org] Integrity error while adding credentials", + extra={ + "organization_id": creds_add.organization_id, + "project_id": creds_add.project_id, + "provider": provider, + }, + exc_info=True, + )
59-61: Success log: prefer structured fieldsFor consistency with other suggestions.
- logger.info( - f"[set_creds_for_org] Successfully created credentials | organization_id {creds_add.organization_id}, project_id {creds_add.project_id}" - ) + logger.info( + "[set_creds_for_org] Successfully created credentials", + extra={"organization_id": creds_add.organization_id, "project_id": creds_add.project_id}, + )
91-95: Query construction: avoid where(True); build conditions list insteadPassing True into where() is odd and may confuse readers/linters. Compose conditions and splat them.
Example (apply similarly to all queries in this file):
conditions = [ Credential.organization_id == org_id, Credential.is_active.is_(True), ] if project_id is not None: conditions.append(Credential.project_id == project_id) statement = select(Credential).where(*conditions)Also applies to: 118-123, 152-159, 186-190, 210-214
2-2: Adopt built-in generics (dict/list) and trim typing imports (Ruff UP035/UP006)Replace Dict/List with dict/list; keep Optional/Any/Union as needed. Also update return annotations accordingly.
-from typing import Optional, Dict, Any, List, Union +from typing import Optional, Any, Union @@ -def set_creds_for_org(*, session: Session, creds_add: CredsCreate) -> List[Credential]: +def set_creds_for_org(*, session: Session, creds_add: CredsCreate) -> list[Credential]: @@ -) -> Optional[Union[Dict[str, Any], Credential]]: +) -> Optional[Union[dict[str, Any], Credential]]: @@ -def get_providers( - *, session: Session, org_id: int, project_id: Optional[int] = None -) -> List[str]: +def get_providers( + *, session: Session, org_id: int, project_id: Optional[int] = None +) -> list[str]: @@ -def update_creds_for_org( - *, session: Session, org_id: int, creds_in: CredsUpdate -) -> List[Credential]: +def update_creds_for_org( + *, session: Session, org_id: int, creds_in: CredsUpdate +) -> list[Credential]: @@ -def remove_creds_for_org( - *, session: Session, org_id: int, project_id: Optional[int] = None -) -> List[Credential]: +def remove_creds_for_org( + *, session: Session, org_id: int, project_id: Optional[int] = None +) -> list[Credential]:Also applies to: 18-18, 71-71, 89-90, 107-108, 131-134, 139-144
28-49: Atomicity/perf: commit once per batch using a transactionset_creds_for_org commits inside the loop; a failure mid-loop can leave partial provider rows. Enclose the loop in a single transaction and commit once.
- for provider, credentials in creds_add.credential.items(): - ... - try: - session.add(credential) - session.commit() - session.refresh(credential) - created_credentials.append(credential) - except IntegrityError as e: - session.rollback() - ... + try: + with session as s: + with s.begin(): # single transaction + for provider, credentials in creds_add.credential.items(): + validate_provider(provider) + validate_provider_credentials(provider, credentials) + encrypted_credentials = encrypt_credentials(credentials) + credential = Credential( + organization_id=creds_add.organization_id, + project_id=creds_add.project_id, + is_active=creds_add.is_active, + provider=provider.lower(), + credential=encrypted_credentials, + ) + credential.inserted_at = now() + s.add(credential) + created_credentials.append(credential) + # refresh outside loop + for c in created_credentials: + session.refresh(c) + except IntegrityError as e: + logger.error("[set_creds_for_org] Integrity error while adding credentials", exc_info=True, + extra={"organization_id": creds_add.organization_id, "project_id": creds_add.project_id}) + raise ValueError(f"Error while adding credentials: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
backend/app/api/routes/credentials.py(10 hunks)backend/app/api/routes/users.py(11 hunks)backend/app/core/providers.py(3 hunks)backend/app/crud/credentials.py(6 hunks)backend/app/crud/user.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/crud/user.py
- backend/app/api/routes/users.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/api/routes/credentials.py (1)
backend/app/tests/api/routes/test_creds.py (3)
test_update_credentials(183-217)test_set_credential(32-62)test_multiple_provider_credentials(390-448)
backend/app/crud/credentials.py (3)
backend/app/models/credentials.py (3)
Credential(50-94)CredsCreate(19-28)CredsUpdate(31-47)backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)backend/app/core/security.py (2)
encrypt_credentials(144-161)decrypt_credentials(164-181)
backend/app/core/providers.py (2)
backend/app/tests/core/test_providers.py (2)
test_validate_provider_credentials_missing_fields(17-31)test_validate_provider_invalid(9-14)backend/app/tests/crud/test_credentials.py (1)
test_invalid_provider(187-200)
🪛 Ruff (0.12.2)
backend/app/crud/credentials.py
2-2: typing.Dict is deprecated, use dict instead
(UP035)
2-2: typing.List is deprecated, use list instead
(UP035)
18-18: Use list instead of List for type annotation
Replace with list
(UP006)
backend/app/core/providers.py
2-2: typing.Dict is deprecated, use dict instead
(UP035)
2-2: typing.List is deprecated, use list instead
(UP035)
2-2: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
🔇 Additional comments (4)
backend/app/core/providers.py (1)
1-1: Good: module-level logger initializationUsing a module-scoped logger via logging.getLogger(name) is correct and consistent with the rest of the PR.
Also applies to: 6-7
backend/app/api/routes/credentials.py (1)
23-23: Good: added module-level loggerConsistent with the project-wide logging changes.
backend/app/crud/credentials.py (2)
15-15: Good: module-level logger addedConsistent with logging across the PR.
162-167: Good: consistent 404 with context logged earlierMatches API expectations and communicates failure clearly.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/crud/credentials.py (2)
47-51: Make insert atomic; avoid per-item commits in loop.Commit-on-each-iteration risks partial writes and hurts performance. Batch inside a single transaction and refresh once.
# Outline: refactor set_creds_for_org() body created_credentials: list[Credential] = [] try: with session.begin(): # atomic for provider, credentials in creds_add.credential.items(): validate_provider(provider) validate_provider_credentials(provider, credentials) enc = encrypt_credentials(credentials) cred = Credential( organization_id=organization_id, project_id=project_id, is_active=creds_add.is_active, provider=provider, credential=enc, ) cred.inserted_at = now() session.add(cred) created_credentials.append(cred) for cred in created_credentials: session.refresh(cred) except IntegrityError: logger.exception( f"[set_creds_for_org] Integrity error | organization_id: {organization_id}, project_id: {project_id}" ) raise HTTPException(status_code=409, detail="Duplicate or constraint violation")
75-85: Decryption bug: attempting to index an encrypted string.
Credential.credentialis encrypted; current code checks substring and indexes a str. Decrypt first.@@ - if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] - - return None + if not creds or not creds.credential: + return None + data = decrypt_credentials(creds.credential) + return data.get("api_key")Optional: validate provider up-front.
@@ - """Fetches the API key from the credentials for the given organization and provider.""" + """Fetches the API key from the credentials for the given organization and provider.""" + validate_provider(provider)
♻️ Duplicate comments (2)
backend/app/crud/credentials.py (2)
25-27: Unify log format (labels, separators) across the file.Use a single style:
[fn] ... | organization_id: X, project_id: Y, provider: Z. Current lines mixlabel valueandlabel: value.Also applies to: 55-57, 61-63, 167-168, 178-180, 205-206, 228-229
25-27: Fix AttributeError and normalize log context fields (use function params, not model attrs).
CredsCreatehas no organization_id/project_id fields; f-strings here will raise at runtime. Also make labels consistent (organization_id:etc.).Apply:
@@ - logger.error( - f"[set_creds_for_org] No credentials provided | project_id: {creds_add.project_id}" - ) + logger.error( + f"[set_creds_for_org] No credentials provided | organization_id: {organization_id}, project_id: {project_id}" + ) @@ - logger.error( - f"[set_creds_for_org] Integrity error while adding credentials | organization_id {creds_add.organization_id}, project_id {creds_add.project_id}, provider {provider}: {str(e)}", - exc_info=True, - ) - raise ValueError( - f"Error while adding credentials for provider {provider}: {str(e)}" - ) + logger.exception( + f"[set_creds_for_org] Integrity error while adding credentials | organization_id: {organization_id}, project_id: {project_id}, provider: {provider}" + ) + raise HTTPException( + status_code=409, + detail=f"Error while adding credentials for provider {provider}", + ) @@ - logger.info( - f"[set_creds_for_org] Successfully created credentials | organization_id {creds_add.organization_id}, project_id {creds_add.project_id}" - ) + logger.info( + f"[set_creds_for_org] Successfully created credentials | organization_id: {organization_id}, project_id: {project_id}" + )Also applies to: 55-57, 61-63
🧹 Nitpick comments (2)
backend/app/crud/credentials.py (2)
228-229: Minor: include removed count and normalize labels.- logger.info( - f"[remove_creds_for_org] Successfully removed all the credentials | organization_id {org_id}, project_id {project_id}" - ) + logger.info( + f"[remove_creds_for_org] Successfully removed credentials | organization_id: {org_id}, project_id: {project_id}, removed_count: {len(creds)}" + )
2-2: Modernize typing to builtin generics (ruff UP035).Swap
typing.List/typing.Dictforlist/dictand simplify unions.-from typing import Optional, Dict, Any, List, Union +from typing import Optional, Any, Union @@ -) -> List[Credential]: +) -> list[Credential]: @@ -) -> Optional[str]: +) -> str | None: @@ -) -> Optional[Union[Dict[str, Any], Credential]]: +) -> dict[str, Any] | Credential | None: @@ -) -> List[str]: +) -> list[str]: @@ -) -> List[Credential]: +) -> list[Credential]: @@ -) -> List[Credential]: +) -> list[Credential]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/app/api/routes/credentials.py(8 hunks)backend/app/crud/credentials.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/routes/credentials.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/crud/credentials.py (3)
backend/app/models/credentials.py (4)
Credential(48-92)CredsCreate(19-29)CredsUpdate(32-45)CredsBase(9-16)backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)backend/app/core/security.py (2)
encrypt_credentials(144-161)decrypt_credentials(164-181)
🪛 Ruff (0.12.2)
backend/app/crud/credentials.py
2-2: typing.Dict is deprecated, use dict instead
(UP035)
2-2: typing.List is deprecated, use list instead
(UP035)
🔇 Additional comments (1)
backend/app/crud/credentials.py (1)
1-1: Module logger setup: LGTM.Also applies to: 15-15
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/crud/credentials.py (3)
47-53: Wrap per-provider inserts in a single transaction to avoid partial writes.
Current per-row commit can leave org in a half-created state if later providers fail.Apply:
- try: - session.add(credential) - session.commit() - session.refresh(credential) - created_credentials.append(credential) - except IntegrityError as e: + try: + with session.begin(): + session.add(credential) + session.flush() # ensure PKs populated + session.refresh(credential) + created_credentials.append(credential) + except IntegrityError as e: session.rollback()
83-86: Bug: attempting to read "api_key" from encrypted blob. Decrypt first.
As stored, Credential.credential is an encrypted string; current check will always fail.Apply:
- if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] + if creds and creds.credential: + try: + data = decrypt_credentials(creds.credential) + except ValueError as e: + logger.error( + f"[get_key_by_org] Failed to decrypt credentials | organization_id: {org_id}, provider: {provider}, project_id: {project_id}", + exc_info=True, + ) + return None + return data.get("api_key")
145-147: Requireproject_idinupdate_creds_for_org
Calls toupdate_creds_for_orgonly appear in the test suite (1) and no production code invokes it; withoutproject_id, selecting byorganization_id+provideris ambiguous because there’s no uniqueness constraint on those columns. Change the signature toproject_id: intand remove theif project_id is not Noneguard in the WHERE clause—update the test accordingly.
♻️ Duplicate comments (2)
backend/app/crud/credentials.py (2)
25-27: Include organization_id and use consistent key: value style in the error log.
Earlier feedback asked for uniform formatting; this spot still diverges.Apply:
- logger.error( - f"[set_creds_for_org] No credentials provided | project_id: {project_id}" - ) + logger.error( + f"[set_creds_for_org] No credentials provided | organization_id: {organization_id}, project_id: {project_id}" + )
195-201: Guard missing provider before soft delete; return 404 instead of NPE.
Prior feedback still applies; accessing attrs on None will crash.Apply:
creds = session.exec(statement).first() - # Soft delete - creds.is_active = False - creds.updated_at = now() + if creds is None: + logger.error( + f"[remove_provider_credential] Credentials not found | organization_id: {org_id}, provider: {provider}, project_id: {project_id}" + ) + raise HTTPException(status_code=404, detail="Credentials not found for this provider") + + # Soft delete + creds.is_active = False + creds.updated_at = now() @@ - logger.info( - f"[remove_provider_credential] Successfully removed credentials for provider | organization_id {org_id}, provider {provider}, project_id {project_id}" - ) + logger.info( + f"[remove_provider_credential] Successfully removed credentials for provider | organization_id: {org_id}, provider: {provider}, project_id: {project_id}" + )Also applies to: 204-206
🧹 Nitpick comments (7)
backend/app/crud/credentials.py (7)
55-57: Standardize log formatting; avoid mixing key styles.
Also no need to wrap e with str(); exc_info will capture traceback.Apply:
- logger.error( - f"[set_creds_for_org] Integrity error while adding credentials | organization_id {organization_id}, project_id {project_id}, provider {provider}: {str(e)}", - exc_info=True, - ) + logger.error( + f"[set_creds_for_org] Integrity error while adding credentials | organization_id: {organization_id}, project_id: {project_id}, provider: {provider}", + exc_info=True, + )Optional: consider surfacing this as HTTP 409 Conflict (or a domain error) for uniqueness violations to distinguish client/data issues from generic ValueError.
61-63: Use consistent key: value formatting in success log.- logger.info( - f"[set_creds_for_org] Successfully created credentials | organization_id {organization_id}, project_id {project_id}" - ) + logger.info( + f"[set_creds_for_org] Successfully created credentials | organization_id: {organization_id}, project_id: {project_id}" + )
32-34: Normalize provider casing at write-time to prevent case-mismatch lookups.
Store canonical enum value; you already validate.- validate_provider(provider) - validate_provider_credentials(provider, credentials) + provider_enum = validate_provider(provider) + validate_provider_credentials(provider_enum.value, credentials) ... - provider=provider, + provider=provider_enum.value,Note: If existing rows contain mixed casing, consider a small data migration.
Also applies to: 39-45
165-168: Good fix replacing creds_in.project_id; keep log format consistent.
Switch “organization …” to “organization_id: …”.- logger.error( - f"[update_creds_for_org] Credentials not found | organization {org_id}, provider {creds_in.provider}, project_id {project_id}" - ) + logger.error( + f"[update_creds_for_org] Credentials not found | organization_id: {org_id}, provider: {creds_in.provider}, project_id: {project_id}" + )
178-180: Consistent key: value formatting in success log.- logger.info( - f"[update_creds_for_org] Successfully updated credentials | organization_id {org_id}, provider {creds_in.provider}, project_id {project_id}" - ) + logger.info( + f"[update_creds_for_org] Successfully updated credentials | organization_id: {org_id}, provider: {creds_in.provider}, project_id: {project_id}" + )
227-229: Consistent key: value formatting in bulk-remove success log.- logger.info( - f"[remove_creds_for_org] Successfully removed all the credentials | organization_id {org_id}, project_id {project_id}" - ) + logger.info( + f"[remove_creds_for_org] Successfully removed all the credentials | organization_id: {org_id}, project_id: {project_id}" + )
2-2: Use builtin generics (PEP 585) instead of typing.List/Dict.
Minor modernization; Ruff UP035 flagged this.Example:
- from typing import Optional, Any, Union
- Replace List[Credential] -> list[Credential], Dict[str, Any] -> dict[str, Any]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/app/api/routes/credentials.py(8 hunks)backend/app/crud/credentials.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/routes/credentials.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/crud/credentials.py (4)
backend/app/models/credentials.py (4)
Credential(48-92)CredsCreate(19-29)CredsUpdate(32-45)CredsBase(9-16)backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)backend/app/core/security.py (2)
encrypt_credentials(144-161)decrypt_credentials(164-181)backend/app/tests/crud/test_credentials.py (3)
test_update_creds_for_org(98-124)test_set_credentials_for_org(21-48)test_remove_creds_for_org(154-184)
🪛 Ruff (0.12.2)
backend/app/crud/credentials.py
2-2: typing.Dict is deprecated, use dict instead
(UP035)
2-2: typing.List is deprecated, use list instead
(UP035)
🔇 Additional comments (1)
backend/app/crud/credentials.py (1)
1-1: Module logger setup looks good.
Consistent with project-wide logging pattern.Also applies to: 15-15
| project_id=_current_user.project_id, | ||
| ) | ||
| if existing_cred: | ||
| logger.warning( |
There was a problem hiding this comment.
can we add user_id also helps in long term to see who updated the credentials
Summary
Target issue is #253 and #254
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit