-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix avoid error when no hstore is installed #1120
Conversation
Reviewer's Guide by SourceryThis pull request addresses an issue where errors occur if the 'hstore' extension is not installed in PostgreSQL. It introduces a new File-Level Changes
|
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.
Hey @phenobarbital - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -57,6 +57,11 @@ def _get_reset_query(self): | |||
return None | |||
|
|||
|
|||
class pgRecord(asyncpg.Record): |
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.
suggestion: Consider adding a docstring to the pgRecord class.
Adding a docstring to the pgRecord class would improve code readability and maintainability by explaining its purpose and usage.
class pgRecord(asyncpg.Record): | |
class pgRecord(asyncpg.Record): | |
""" | |
A subclass of asyncpg.Record that allows attribute-style access to record fields. | |
This class overrides the __getattr__ method to enable accessing record fields | |
using dot notation, providing a more convenient and readable way to work with | |
database records. | |
""" |
@@ -71,6 +76,9 @@ def __init__( | |||
self._server_settings = {} | |||
self._dsn = "postgres://{user}:{password}@{host}:{port}/{database}" | |||
super(pgPool, self).__init__(dsn=dsn, loop=loop, params=params, **kwargs) | |||
self._custom_record: bool = kwargs.get("custom_record", False) |
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.
suggestion (bug_risk): Consider validating the type of custom_record.
It might be beneficial to validate that the custom_record parameter is indeed a boolean to avoid potential type-related issues.
self._custom_record: bool = kwargs.get("custom_record", False) | |
custom_record = kwargs.get("custom_record", False) | |
if not isinstance(custom_record, bool): | |
raise ValueError("custom_record must be a boolean") | |
self._custom_record: bool = custom_record |
await connection.set_builtin_type_codec("hstore", codec_name="pg_contrib.hstore") | ||
try: | ||
await connection.set_builtin_type_codec("hstore", codec_name="pg_contrib.hstore") | ||
except Exception: |
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.
issue (bug_risk): Avoid catching broad exceptions.
Catching broad exceptions can make debugging difficult and may hide unexpected errors. Consider catching more specific exceptions.
@@ -203,6 +217,11 @@ async def connect(self): | |||
"tcp_keepalives_idle": "30min", | |||
} | |||
server_settings = {**server_settings, **self._server_settings} | |||
custom_class = {} |
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.
suggestion (performance): Consider initializing custom_class only if needed.
Initializing custom_class only when self._custom_record is True can save some memory and improve readability.
custom_class = {} | |
if self._custom_record: | |
custom_class = {} |
init=self.init_connection, | ||
setup=self.setup_connection, | ||
loop=self._loop, | ||
server_settings=server_settings, | ||
connection_class=NAVConnection, | ||
**custom_class, |
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.
issue (bug_risk): Potential issue with unpacking custom_class.
If custom_class is empty, unpacking it might not cause an issue, but it's generally safer to conditionally include it only when it's not empty.
await self._connection.set_type_codec( | ||
"jsonb", encoder=_encoder, decoder=_decoder, schema="pg_catalog" | ||
) | ||
try: |
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.
issue (bug_risk): Avoid catching broad exceptions.
Catching broad exceptions can make debugging difficult and may hide unexpected errors. Consider catching more specific exceptions.
Summary by Sourcery
This pull request introduces a custom record class for PostgreSQL, adds error handling for missing type codecs, and enhances the PostgreSQL driver with support for custom record classes and configurable statement cache size. Additionally, it removes a redundant method from the ScyllaDB driver and updates the library version to 2.7.9.
pgRecord
for PostgreSQL driver to allow attribute-style access to record fields.hstore
oruuid
type codecs are not available in PostgreSQL.drop_table
method from ScyllaDB driver.