-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat: Add comprehensive database and encoding error handling exceptions #2363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Added four new exception classes to improve error handling: - DatabaseConnectionError: Enhanced database connection errors with recovery hints - DatabaseTimeoutError: Timeout errors with operation context - EncodingError: UTF-8 and character encoding issues - DataValidationError: Data validation failures in RAG processing These exceptions provide better error messages, contextual information, and recovery suggestions for common database and encoding issues reported in the issues tracker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| class DatabaseConnectionError(APIConnectionError): | ||
| """Raised when database connection fails with recovery suggestions.""" | ||
|
|
||
| def __init__(self, message: str, database_type: str = "", recovery_hint: str = ""): | ||
| self.database_type = database_type | ||
| self.recovery_hint = recovery_hint | ||
| detailed_msg = f"Database connection error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repair syntax errors in database exception block
The newly added database exception definitions are malformed. The block starting with class DatabaseConnectionError is indented inconsistently and the f-string in detailed_msg = f"Database connection error never closes, leaving the file in an unparseable state. Importing lightrag.exceptions will raise a SyntaxError before any of the existing exceptions can be used.
Useful? React with 👍 / 👎.
| if recovery_hint: | ||
| detailed_msg += f"\nRecovery: {recovery_hint}" | ||
| super().__init__(detailed_msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass request object when subclassing APIConnectionError
DatabaseConnectionError inherits from APIConnectionError, whose constructor requires a keyword message and an httpx.Request. Here super().__init__(detailed_msg) omits the required request argument, so instantiating the new exception would immediately raise TypeError instead of producing the intended error message. Either accept a request parameter or derive from a simpler base exception.
Useful? React with 👍 / 👎.
| class DatabaseTimeoutError(APITimeoutError): | ||
| """Raised when database operations timeout.""" | ||
|
|
||
| def __init__(self, message: str, operation: str = "", timeout_seconds: float = 0): | ||
| self.operation = operation | ||
| self.timeout_seconds = timeout_seconds | ||
| detailed_msg = f"Database timeout: {message}" | ||
| if operation: | ||
| detailed_msg += f" during '{operation}'" | ||
| if timeout_seconds > 0: | ||
| detailed_msg += f" (timeout: {timeout_seconds}s)" | ||
| super().__init__(detailed_msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide required request to APITimeoutError
DatabaseTimeoutError subclasses APITimeoutError, which expects an httpx.Request in its initializer. The new constructor builds a custom message but calls super().__init__(detailed_msg) without providing a request, so any instantiation will fail with TypeError before the timeout details are surfaced. Consider accepting a request parameter or inheriting from a different base class that does not require it.
Useful? React with 👍 / 👎.
Added four new exception classes to improve error handling:
These exceptions provide better error messages, contextual information, and recovery suggestions for common database and encoding issues reported in the issues tracker.
Description
[Briefly describe the changes made in this pull request.]
Related Issues
[Reference any related issues or tasks addressed by this pull request.]
Changes Made
[List the specific changes made in this pull request.]
Checklist
Additional Notes
[Add any additional notes or context for the reviewer(s).]