Credentials: error handling for unsupported providers#707
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChanges address provider management and error handling across the codebase. AWS provider removed from supported list. Documentation updated with additional providers (Google, Sarvamai, ElevenLabs). Validation logic refactored to fail fast on first error rather than accumulating multiple errors. Error handling improved to convert ValueError exceptions to HTTPException with status code 400. Error messages simplified by removing Pydantic-specific prefixes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app/core/providers.py (1)
2-2: Use built-in generics instead of deprecatedtyping.Dictandtyping.List.Per Python 3.11+ best practices, use
dictandlistdirectly instead of importing fromtyping.Suggested fix
-from typing import Dict, List +from typing import Dict, List # Keep for backward compatibility or remove entirelyAnd update usages:
-PROVIDER_CONFIGS: Dict[Provider, ProviderConfig] = { +PROVIDER_CONFIGS: dict[Provider, ProviderConfig] = {-def get_supported_providers() -> List[str]: +def get_supported_providers() -> list[str]:- required_fields: List[str] + required_fields: list[str]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/core/providers.py` at line 2, Replace deprecated typing generics by using built-in generics: remove "from typing import Dict, List" and update any type annotations that reference Dict and List to use lowercase dict and list (e.g., Dict[str, Any] -> dict[str, Any], List[int] -> list[int]). Locate usages in this module (and any functions/classes that reference Dict or List) such as type annotations in functions, return types, and variable annotations and convert them to the built-in forms to comply with Python 3.11+ best practices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/core/providers.py`:
- Around line 19-20: The test
test_validate_provider_credentials_missing_fields() in
backend/app/tests/core/test_providers.py still calls
validate_provider_credentials("aws", ...) even though "aws" was removed from the
Provider enum in backend/app/core/providers.py; remove the AWS-specific test
case (the validate_provider_credentials("aws", ...) assertion and any related
setup for AWS) so the test only covers existing providers (e.g., the remaining
Provider values used in the file) and update any expected error counts/messages
accordingly.
In `@backend/app/models/onboarding.py`:
- Around line 96-115: The field validator in onboarding.py raises TypeError for
malformed credential entries, which bypasses Pydantic/CRUD handling; change
those TypeError raises to ValueError so Pydantic validators and
backend/app/crud/credentials.py (which only catches ValueError) will convert
these to proper 400 responses—specifically replace the two TypeError usages in
the validator loop (the "Credential must be a dict..." and the "Value for
provider '... must be an object/dict." messages) with ValueError while keeping
the same messages and leave validate_provider(provider_key) as-is.
---
Nitpick comments:
In `@backend/app/core/providers.py`:
- Line 2: Replace deprecated typing generics by using built-in generics: remove
"from typing import Dict, List" and update any type annotations that reference
Dict and List to use lowercase dict and list (e.g., Dict[str, Any] -> dict[str,
Any], List[int] -> list[int]). Locate usages in this module (and any
functions/classes that reference Dict or List) such as type annotations in
functions, return types, and variable annotations and convert them to the
built-in forms to comply with Python 3.11+ best practices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9290652-293c-4f5a-bd3f-6662b6a9690b
📒 Files selected for processing (6)
backend/app/api/docs/credentials/create.mdbackend/app/api/docs/onboarding/onboarding.mdbackend/app/core/providers.pybackend/app/crud/credentials.pybackend/app/models/onboarding.pybackend/app/utils.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/app/tests/crud/test_credentials.py (1)
209-210: Consider moving the import to the top of the file.
HTTPExceptionis imported identically inside bothtest_invalid_provider(line 209) andtest_langfuse_credential_validation(line 261). Moving this import to the top-level alongside the other imports (lines 4-18) would eliminate duplication and follow typical Python conventions.Suggested change
Add to the existing imports at the top of the file:
from app.core.exception_handlers import HTTPExceptionThen remove the local imports at lines 209-210 and 261-262.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/crud/test_credentials.py` around lines 209 - 210, Move the duplicate local import of HTTPException to the module-level imports: add "from app.core.exception_handlers import HTTPException" alongside the other top-level imports, then remove the two in-function imports inside test_invalid_provider and test_langfuse_credential_validation so both tests use the single top-level HTTPException symbol.backend/app/tests/api/routes/test_onboarding.py (1)
304-335: Consider renaming the test function to reflect fail-fast behavior.The function name
test_onboard_project_aggregates_multiple_credential_errorsis misleading since the validation now uses fail-fast behavior and no longer aggregates errors. The docstring was updated but the function name still suggests aggregation.Consider renaming to something like:
-def test_onboard_project_aggregates_multiple_credential_errors( +def test_onboard_project_fails_on_first_credential_error( client: TestClient, superuser_token_headers: dict[str, str], db: Session ) -> None: """Test onboarding reports credential validation errors (fails on first error)."""Regarding the unused
dbparameter flagged by static analysis: this is common in pytest fixtures where the parameter ensures proper test isolation/setup even if not directly referenced in the test body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/api/routes/test_onboarding.py` around lines 304 - 335, Rename the test function test_onboard_project_aggregates_multiple_credential_errors to reflect the new fail-fast validation (for example test_onboard_project_fails_fast_on_credential_error) and update any docstring if needed; keep the unused pytest fixture parameter db in the signature so test isolation/setup remains intact (do not remove db even if static analysis flags it) and ensure the test body (onboard_data, client.post call and the final assertion checking "Unsupported provider") continues to reference the same behavior.backend/app/models/onboarding.py (1)
85-94: Add return type annotation to the validator method.Per coding guidelines, all function parameters and return values should have type hints. The validator is missing its return type.
`@field_validator`("credentials") `@classmethod` - def _validate_credential_list(cls, v: list[dict[str, dict[str, str]]] | None): + def _validate_credential_list(cls, v: list[dict[str, dict[str, str]]] | None) -> list[dict[str, dict[str, str]]] | None: if v is None: return v if not isinstance(v, list): raise ValueError( "Credential must be a list of single-key dicts (e.g., {'openai': {...}})." )As per coding guidelines: "Always add type hints to all function parameters and return values in Python code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/models/onboarding.py` around lines 85 - 94, The field validator method _validate_credential_list is missing a return type annotation; update its signature (the `@field_validator-decorated` classmethod) to declare the return type as the same as the input (e.g., -> list[dict[str, dict[str, str]]] | None) so the function signature includes types for parameters and return value, and ensure any required typing imports are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/models/onboarding.py`:
- Around line 85-94: The field validator method _validate_credential_list is
missing a return type annotation; update its signature (the
`@field_validator-decorated` classmethod) to declare the return type as the same
as the input (e.g., -> list[dict[str, dict[str, str]]] | None) so the function
signature includes types for parameters and return value, and ensure any
required typing imports are present.
In `@backend/app/tests/api/routes/test_onboarding.py`:
- Around line 304-335: Rename the test function
test_onboard_project_aggregates_multiple_credential_errors to reflect the new
fail-fast validation (for example
test_onboard_project_fails_fast_on_credential_error) and update any docstring if
needed; keep the unused pytest fixture parameter db in the signature so test
isolation/setup remains intact (do not remove db even if static analysis flags
it) and ensure the test body (onboard_data, client.post call and the final
assertion checking "Unsupported provider") continues to reference the same
behavior.
In `@backend/app/tests/crud/test_credentials.py`:
- Around line 209-210: Move the duplicate local import of HTTPException to the
module-level imports: add "from app.core.exception_handlers import
HTTPException" alongside the other top-level imports, then remove the two
in-function imports inside test_invalid_provider and
test_langfuse_credential_validation so both tests use the single top-level
HTTPException symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 029dfd91-b5fa-4d35-93f9-feb98c216dd0
📒 Files selected for processing (4)
backend/app/models/onboarding.pybackend/app/tests/api/routes/test_onboarding.pybackend/app/tests/core/test_providers.pybackend/app/tests/crud/test_credentials.py
💤 Files with no reviewable changes (1)
- backend/app/tests/core/test_providers.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| try: | ||
| provider_enum = validate_provider(provider) | ||
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) |
There was a problem hiding this comment.
nitpick: can we reuse APIResponse.failure_response() instead of manually raising?
There was a problem hiding this comment.
We use failure response directly for situations where we are raising 200 status code with a failure payload, this manual raising is needed for the status code and anyway the global exception handler wraps HTTPException into APIResponse.failure_response() automatically, so the client still gets a consistent APIResponse shape, just with the right non-200 status code.
There was a problem hiding this comment.
the error code can be sent in the metadata. but okay we can take that up later
backend/app/models/onboarding.py
Outdated
| if not isinstance(v, list): | ||
| raise TypeError( | ||
| "credential must be a list of single-key dicts (e.g., {'openai': {...}})." | ||
| raise ValueError( |
There was a problem hiding this comment.
changed this to typeerror
backend/app/crud/credentials.py
Outdated
| validate_provider(creds_in.provider) | ||
| validate_provider_credentials(creds_in.provider, creds_in.credential) | ||
| try: | ||
| validate_provider(creds_in.provider) |
There was a problem hiding this comment.
Inside validate_provider its already raising the exception hence wrapping ahain in another try..catch is redundnat.
There was a problem hiding this comment.
the validate provider try catch catches the value error and the try catch we aree wrappin the function with converts it to httpexception
| if errors: | ||
| raise ValueError("credential validation failed:\n" + "\n".join(errors)) | ||
| for item in v: | ||
| if not isinstance(item, dict): |
There was a problem hiding this comment.
Is this post filter required inside OnboardingRequest?Imo a better approach would be to decouple this validation logic from onboarding as it might be hard to maintain.
There was a problem hiding this comment.
while it may be hard to maintain, this helps us catch any issue early on with the request body when it comes to credentials being given, otherwise we will only get to know about it after crud function is hit and we put the verification in that
There was a problem hiding this comment.
sure we can take it up later
5 participants
Summary
Target issue is #708
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
Documentation
Provider Updates
Improvements