add exception handling and validation in Client and EncryptedIndex cl…#47
add exception handling and validation in Client and EncryptedIndex cl…#47aditisingh02 wants to merge 5 commits intocyborginc:mainfrom
Conversation
…asses - Introduced new exceptions for better error handling: CyborgDBInvalidKeyError, CyborgDBNotFoundError, CyborgDBAuthenticationError, and CyborgDBIndexError. - Added URL validation in the Client class to ensure proper base_url format. - Updated methods to raise specific exceptions instead of generic ValueErrors for various failure scenarios. - Enhanced error logging for better debugging and traceability.
There was a problem hiding this comment.
Pull request overview
This PR enhances error handling in the CyborgDB Python SDK by introducing custom exception classes and fixing previously unreachable exception handlers. The changes improve developer experience by providing specific, contextual error information while maintaining backward compatibility.
- Added a comprehensive exception hierarchy with 8 custom exception classes
- Fixed unreachable exception handlers in
EncryptedIndexmethods by reordering catch blocks - Introduced URL validation with early failure detection in
Client.__init__()
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cyborgdb/client/exceptions.py | Defines new custom exception classes with contextual attributes (status codes, resource names, URLs) |
| cyborgdb/client/encrypted_index.py | Replaces generic ValueError with specific custom exceptions and fixes exception handler ordering in get() method |
| cyborgdb/client/client.py | Adds URL validation helper, updates exception handling across all methods, and improves docstrings with type hints |
| cyborgdb/init.py | Exports new exception classes for public API access |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Raises: | ||
| CyborgDBInvalidKeyError: If index_key is invalid | ||
| CyborgDBIndexError: If index creation fails | ||
| CyborgDBAuthenticationError: If authentication fails | ||
| CyborgDBValidationError: If validation fails |
There was a problem hiding this comment.
The docstring is incomplete and missing important information. It should include a description of what the function does, all parameters (index_name, index_key, index_config, embedding_model, metric), and a return value description. The current docstring only includes the Raises section.
| Raises: | |
| CyborgDBInvalidKeyError: If index_key is invalid | |
| CyborgDBIndexError: If index creation fails | |
| CyborgDBAuthenticationError: If authentication fails | |
| CyborgDBValidationError: If validation fails | |
| This method sends a request to the CyborgDB REST API to create an encrypted | |
| index identified by ``index_name`` using the provided 32-byte ``index_key`` | |
| and optional index configuration, embedding model, and distance metric. | |
| Args: | |
| index_name: The unique name of the index to create in the CyborgDB service. | |
| index_key: A 32-byte encryption key used to secure the index; must be | |
| provided as a ``bytes`` object. | |
| index_config: Optional index configuration specifying index type and | |
| parameters (e.g., IVF, IVFPQ, or IVFFlat). If ``None``, a default | |
| ``IndexIVFFlatModel`` configuration is used. | |
| embedding_model: Optional identifier of the embedding model associated | |
| with the index. If provided, it will be stored with the index | |
| metadata. | |
| metric: Optional similarity or distance metric to use for the index | |
| (for example, "cosine" or "euclidean"), if supported by the service. | |
| Returns: | |
| EncryptedIndex: A client-side ``EncryptedIndex`` instance representing | |
| the newly created encrypted index. | |
| Raises: | |
| CyborgDBInvalidKeyError: If ``index_key`` is not a 32-byte ``bytes`` value. | |
| CyborgDBIndexError: If index creation fails on the server side. | |
| CyborgDBAuthenticationError: If authentication with the CyborgDB service fails. | |
| CyborgDBValidationError: If the request payload fails validation. |
| """ | ||
| Load an existing encrypted index by name and key. | ||
| """ | ||
|
|
||
| Raises: | ||
| CyborgDBInvalidKeyError: If index_key is invalid | ||
| CyborgDBNotFoundError: If the index is not found | ||
| CyborgDBAuthenticationError: If authentication fails | ||
| CyborgDBValidationError: If validation fails | ||
| """ |
There was a problem hiding this comment.
The docstring format is incorrect. The closing triple quotes should be on line 323 after the Raises section, not on line 316. Currently the Raises section appears outside the docstring.
| except Exception as e: | ||
| error_msg = f"Get operation failed: {str(e)}" | ||
| logger.error(error_msg) | ||
| raise |
There was a problem hiding this comment.
Overly broad exception handling. Catching all Exceptions and re-raising them without any transformation or additional context is not a useful error handling pattern. This catch block should either be removed (letting exceptions propagate naturally) or should handle specific exception types. The ApiException handler above already covers API-related errors, so this catch-all is redundant.
| except Exception as e: | |
| error_msg = f"Get operation failed: {str(e)}" | |
| logger.error(error_msg) | |
| raise |
| class CyborgDBError(Exception): | ||
| """Base exception class for all CyborgDB client errors.""" | ||
|
|
||
| pass |
There was a problem hiding this comment.
Unnecessary pass statement. The base exception class body can be empty or use an ellipsis (...) instead. The pass statement is redundant when the docstring is present.
| class CyborgDBValidationError(CyborgDBError, ValueError): | ||
| """Raised when input validation fails.""" | ||
|
|
||
| pass |
There was a problem hiding this comment.
Unnecessary pass statement. The exception class body can be empty or use an ellipsis (...) instead. The pass statement is redundant when the docstring is present.
| f"base_url must be a string, got {type(url).__name__}", url=url | ||
| ) | ||
|
|
||
| url = url.strip() |
There was a problem hiding this comment.
The URL is stripped after validation but the stripped value is not used for subsequent checks. The validation checks are performed on the stripped URL but the original URL parameter is passed in the error message. Consider stripping the URL before the type check and using the stripped version consistently throughout the validation.
… classes - Added detailed docstrings for methods to clarify parameters, return types, and exceptions raised. - Improved error messages for exceptions to provide more context on failures. - Removed redundant exception handling in EncryptedIndex methods for cleaner code.
|
@copilot All issues have been addressed: ✅ Fixed |
|
@dupontcyborg @ahellegit @copilot kindly review and let me know if any changes are needed |
Summary
Fixes unreachable exception handlers and adds custom exception classes with URL validation for better error handling.
Changes
EncryptedIndexmethodsClient.__init__()Benefits
ValueErrorBreaking Changes
None - Fully backward compatible.
Example