Credentials: Add support for org credentials #179
Conversation
…pecific credentials - Introduced a new column 'provider' in the credential table to support multiple credential providers. - Updated API routes to handle provider-specific credential operations, including creation, retrieval, updating, and deletion. - Enhanced validation for provider credentials and added support for multiple providers in the data model. - Refactored existing credential handling functions to accommodate the new structure and improve error handling. - Ensured backward compatibility by maintaining existing functionality while expanding capabilities.
…reation, enhance readability, and ensure proper handling of provider-specific data.
…roper checks for credential existence; update tests for accuracy in response validation.
… and improved error responses; update tests for accurate status codes and messages.
…redential fields; improve error responses for organization checks and unexpected exceptions.
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
jerome-white
left a comment
jerome-white
left a comment
There was a problem hiding this comment.
The most significant issues I see are management of exceptions: raises inside try-except blocks. Consider refactoring. If that is too much trouble, "resolve" my comments manually and move on
| if "Unsupported provider" in str(e): | ||
| raise HTTPException(status_code=400, detail=str(e)) | ||
| raise HTTPException(status_code=404, detail=str(e)) | ||
| except Exception as e: |
There was a problem hiding this comment.
Because of these outer exception handlers, and most notably because of the base exception handling here, none of the exceptions in the function body will be raised on their own. Notably, all HTTPExceptions raised in the function body will be cause by this parent exception, which will return 500 to the user. Because the body-exceptions raise different status codes, I imagine this -- raising 500 always -- was not the intention.
| if not creds: | ||
| raise HTTPException(status_code=404, detail="Credentials not found") | ||
| return APIResponse.success_response([cred.to_public() for cred in creds]) | ||
| except HTTPException: |
There was a problem hiding this comment.
Consider restructuring the code so that this doesn't have to be re-raised. get_creds_by_org is probably the only call within this function that needs to be a in a try-except block
| if provider_creds is None: | ||
| raise HTTPException( | ||
| status_code=404, detail="Provider credentials not found" | ||
| ) | ||
| return APIResponse.success_response(provider_creds) |
There was a problem hiding this comment.
Do these lines need to be in the try-except block? If not, it would alleviate the need for the HTTPException to be caught (a few lines down)
| if not creds_in or not creds_in.provider or not creds_in.credential: | ||
| raise HTTPException( | ||
| status_code=400, detail="Provider and credential must be provided" | ||
| ) |
There was a problem hiding this comment.
Does this need to be in a try-except?
|
|
||
| def to_public(self) -> "CredsPublic": | ||
| """Convert the database model to a public model with decrypted credentials.""" | ||
| from app.core.security import decrypt_credentials |
There was a problem hiding this comment.
Is there a reason for importing within the method? (Instead of in the global scope?) It's fine if the global scope leads to import cycles; but inside the function means the import will be called every time the function is, which won't break the code, but will have performance implications
backend/app/core/providers.py
Outdated
| try: | ||
| return Provider(provider.lower()) | ||
| except ValueError: | ||
| supported = ", ".join([p.value for p in Provider]) |
There was a problem hiding this comment.
Remove the brackets to prevent Python from instantiating the list:
supported = ", ".join(p.value for p in Provider)
|
|
||
| def get_supported_providers() -> List[str]: | ||
| """Return a list of all supported provider names.""" | ||
| return [p.value for p in Provider] |
There was a problem hiding this comment.
Do you really need to return the list, or would a generator do the trick?
yield from (p.value for p in Provider)
If most callers iterate the list, or try to find something in it, a generator would be more efficient
| Example: {"openai": {"api_key": "..."}, "gemini": {"api_key": "..."}} | ||
| """ | ||
|
|
||
| credential: Dict[str, Any] = Field( |
There was a problem hiding this comment.
so then we take the provider and credentials together as key value pair, and then later gets dissected into two separate column? Why could we not take provider name separately and the credentials separately.
There was a problem hiding this comment.
Because that way, we would be restricting the behavior of data insertion by the user.
AkhileshNegi
force-pushed
the
feature/add-provider-org-credentials
branch
from
44fd11b to
e2d20fa
Compare
Summary
Target issue is #149
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