Fix: Restore get_provider_credential behavior to return None when Langfuse credentials are missing #415
Conversation
WalkthroughAdjusted credential handling: query functions now return empty list or None instead of raising; deletion and API read routes explicitly raise HTTP 404 when credentials are absent. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as CredentialsAPI
participant CRUD as CredentialsCRUD
participant DB
Client->>API: GET /org/{org_id}/creds
API->>CRUD: get_creds_by_org(org_id)
CRUD->>DB: query creds
DB-->>CRUD: []
CRUD-->>API: []
API-->>Client: 200 OK (empty list)
Client->>API: GET /provider/{provider_id}/cred
API->>CRUD: get_provider_credential(provider_id)
CRUD->>DB: query provider cred
DB-->>CRUD: none
CRUD-->>API: None
alt None -> API decides missing
API-->>Client: 404 Not Found
else found
API-->>Client: 200 OK (credential)
end
Client->>API: DELETE /provider/{provider_id}/cred
API->>CRUD: remove_provider_credential(provider_id)
CRUD->>CRUD: get_provider_credential(provider_id)
alt credential exists
CRUD->>DB: delete credential
DB-->>CRUD: success
CRUD-->>API: success
API-->>Client: 204 No Content
else missing
CRUD-->>API: raise 404
API-->>Client: 404 Not Found
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
backend/app/api/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)backend/app/tests/crud/test_credentials.py (2)
🔇 Additional comments (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
110-111: Update docstring to reflect actual behavior.The docstring states "Raises: HTTPException: If no credentials are found" but the function now returns an empty list instead of raising an exception.
Apply this diff to update the docstring:
- Raises: - HTTPException: If no credentials are found
128-128: Update return type hint to include None.The function now returns
Nonewhen credentials are not found (line 159), but the type hint only declaresdict[str, Any] | Credential.As per coding guidelines
Apply this diff to update the type hint:
-) -> dict[str, Any] | Credential: +) -> dict[str, Any] | Credential | None:
144-145: Update docstring to reflect actual behavior.The docstring states "Raises: HTTPException: If credentials are not found" but the function now returns
Noneinstead of raising an exception (line 159), which aligns with the PR objectives.Apply this diff to update the docstring:
- Raises: - HTTPException: If credentials are not found + Returns None if credentials are not found for the specified provider.
🧹 Nitpick comments (1)
backend/app/crud/credentials.py (1)
271-271: Simplify redundant None check.The
existing_creds is Nonecheck is unnecessary sinceget_creds_by_orgalways returns a list (line 117), neverNone.Apply this diff to simplify:
- if existing_creds is None or len(existing_creds) == 0: + if not existing_creds:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/crud/credentials.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/crud/credentials.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/credentials.py
⏰ 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/crud/credentials.py (2)
222-231: LGTM! Proper existence validation before deletion.The explicit check for credential existence before attempting deletion follows best practices and provides clearer error handling.
266-274: LGTM! Proper existence validation before deletion.The explicit check for credentials before attempting deletion is consistent with the pattern in
remove_provider_credentialand provides clearer error handling.
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)
110-111: Update the docstring to reflect the actual behavior.The docstring states "Raises: HTTPException: If no credentials are found", but the function no longer raises an exception. It returns an empty list when no credentials exist, which aligns with the PR objectives.
Apply this diff to fix the docstring:
Returns: list[Credential]: List of credentials - - Raises: - HTTPException: If no credentials are found """
139-145: Update the docstring to document the None return case.The docstring describes the return values when credentials are found but doesn't mention that
Noneis returned when credentials don't exist for the specified provider. This should be documented for API clarity.Apply this diff to update the docstring:
Returns: - dict[str, Any] | Credential: + dict[str, Any] | Credential | None: - If `full` is True, returns the full Credential SQLModel object. - Otherwise, returns the decrypted credentials as a dictionary. - - Raises: - HTTPException: If credentials are not found + - Returns None if credentials are not found for the specified provider. """
🧹 Nitpick comments (2)
backend/app/crud/credentials.py (2)
222-227: Passfull=Trueto avoid unnecessary credential decryption.The existence check only needs to verify that credentials exist, but by not passing
full=True, the function unnecessarily decrypts credentials (line 157) only to check if the result isNone. Passingfull=Truewould return theCredentialobject directly, avoiding the decryption overhead.Apply this diff to improve performance:
creds = get_provider_credential( session=session, org_id=org_id, project_id=project_id, provider=provider, + full=True, )
271-271: Remove redundant None check.The condition
existing_creds is Noneis redundant. Theget_creds_by_orgfunction has a return type oflist[Credential]and usessession.exec(statement).all()(line 117), which always returns a list—either empty or populated—but neverNone. Only the length check is necessary.Apply this diff to simplify the condition:
- if existing_creds is None or len(existing_creds) == 0: + if len(existing_creds) == 0:Or more idiomatically:
- if existing_creds is None or len(existing_creds) == 0: + if not existing_creds:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/crud/credentials.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/crud/credentials.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/credentials.py
🧬 Code graph analysis (1)
backend/app/crud/credentials.py (1)
backend/app/models/credentials.py (1)
Credential(48-97)
⏰ 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/crud/credentials.py (2)
128-128: LGTM! The changes correctly restore the expected behavior.The update to return
Nonewhen credentials are not found aligns with the PR objectives and restores the expected behavior for the Response API and other modules. The return type annotation is correctly updated to includeNone, making the API contract clear.Also applies to: 159-159
222-231: Good defensive programming for delete operations.Adding an explicit existence check before deletion follows good practices and provides clear 404 responses when attempting to delete non-existent credentials. This separates concerns between query operations (which gracefully return None) and write operations (which validate resource existence).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary
In PR #360, the get_provider_credential method was modified to raise an exception when credentials were missing.
However, the Response API and other modules still expect this method to return None in such cases.
As a result, the Response API currently throws an error when Langfuse credentials are not configured.
Reverted all method logic changed in PR to return as per previous behaviour.
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
Bug Fixes
Tests