New drivers #1279
New drivers #1279
Conversation
Reviewer's Guide by SourceryThis pull request introduces new asynchronous drivers for Oracle and MongoDB, enhances the JDBC driver with improved error handling and resource management, updates dependencies, and adds a new example script for testing JDBC with PostgreSQL. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @phenobarbital - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider implementing connection pooling for the JDBC driver to improve performance and resource management.
- Standardize error handling across all drivers to ensure consistent behavior and easier debugging.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| self._connection.cursor, | ||
| executor=self._executor | ||
| ) | ||
| await self._thread_func( |
There was a problem hiding this comment.
🚨 issue (security): Use parameterized queries to prevent SQL injection
In the execute and execute_many methods, ensure that you're using parameterized queries instead of string formatting for SQL statements. This is crucial for preventing SQL injection vulnerabilities.
| fetchrow = fetch_one | ||
| fetchone = fetch_one | ||
|
|
||
| async def write( |
There was a problem hiding this comment.
suggestion: Consider refactoring the write method for better maintainability
The write method is quite complex. Consider breaking it down into smaller, more focused functions. This would improve readability, maintainability, and make it easier to test individual components of the write operation.
async def write(self, data):
return await self._write_operation(data)
async def _write_operation(self, data):
# Existing write logic goes here
| return self | ||
|
|
||
| connect = connection | ||
|
|
||
| async def close(self, timeout: int = 10) -> None: | ||
| async def close(self, timeout: int = 5) -> None: |
There was a problem hiding this comment.
suggestion: Move JVM shutdown logic to a separate method
The JVM shutdown logic in the close method is quite complex. Consider moving this logic into a separate method for better organization and potential reuse in other parts of the driver.
async def close(self, timeout: int = 5) -> None:
await self._shutdown_jvm(timeout)
async def _shutdown_jvm(self, timeout: int) -> None:
print("JVM started: ", jpype.isJVMStarted())
if not self._connected or not self._connection:
return
| _jvmArgs.append("-Djava.class.path=" + path) | ||
| _jvmArgs.append("-Xmx12000m") | ||
| _jvmArgs.append(f"-Xmx{self.max_memory}m") | ||
| _jvmArgs.append("-Dfile.encoding=UTF8") |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Merge consecutive list appends into a single extend (
merge-list-appends-into-extend) - Use f-string instead of string concatenation (
use-fstring-for-concatenation)
| _jvmArgs.append("-Djava.class.path=" + path) | |
| _jvmArgs.append("-Xmx12000m") | |
| _jvmArgs.append(f"-Xmx{self.max_memory}m") | |
| _jvmArgs.append("-Dfile.encoding=UTF8") | |
| _jvmArgs.append(f"-Djava.class.path={path}") | |
| _jvmArgs.extend((f"-Xmx{self.max_memory}m", "-Dfile.encoding=UTF8")) |
| if "does not exist" in str(ex): | ||
| raise DriverError( | ||
| f"Database does not exist: {self._params.get('database')}" | ||
| ) |
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
| ) | |
| ) from ex |
| if not self._database: | ||
| raise DriverError( | ||
| "No database selected. Use 'use' method to select it." | ||
| ) |
There was a problem hiding this comment.
suggestion (code-quality): Hoist conditional out of nested conditional (hoist-if-from-if)
| if not self._database: | |
| raise DriverError( | |
| "No database selected. Use 'use' method to select it." | |
| ) | |
| if not self._database: | |
| raise DriverError( | |
| "No database selected. Use 'use' method to select it." | |
| ) |
| if not self._database: | ||
| raise DriverError( | ||
| "No database selected. Use 'use' method to select it." | ||
| ) |
There was a problem hiding this comment.
suggestion (code-quality): Hoist conditional out of nested conditional (hoist-if-from-if)
| if not self._database: | |
| raise DriverError( | |
| "No database selected. Use 'use' method to select it." | |
| ) | |
| if not self._database: | |
| raise DriverError( | |
| "No database selected. Use 'use' method to select it." | |
| ) |
| result = None | ||
| if not self._database: | ||
| self._database = self.use(database=self._database_name) | ||
| if not self._database: |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Hoist conditional out of nested conditional (
hoist-if-from-if) - Explicitly raise from a previous error (
raise-from-previous-error)
| result = None | ||
| if not self._database: | ||
| self._database = self.use(database=self._database_name) | ||
| if not self._database: |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Hoist conditional out of nested conditional (
hoist-if-from-if) - Explicitly raise from a previous error (
raise-from-previous-error)
|
|
||
| # Build bulk operations | ||
| operations = [] | ||
| if if_exists == 'replace': |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Simplify conditional into switch-like form [×2] (
switch) - Inline variable that is immediately returned (
inline-immediately-returned-variable) - Explicitly raise from a previous error (
raise-from-previous-error) - Don't assign to builtin variable
filter(avoid-builtin-shadow)
Explanation
Python has a number of builtin variables: functions and constants that
form a part of the language, such as list, getattr, and type
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.
Summary by Sourcery
Add new drivers for Oracle and MongoDB to support asynchronous database operations. Enhance the JDBC driver with configurable JVM memory settings and improve error handling. Update dependencies and add a new example test script for JDBC connections.
New Features:
Enhancements:
Build:
Tests:
Chores: