Conversation
davidmunoznovoa
changed the title
There was a problem hiding this comment.
Pull request overview
This PR extends the cursor patching mechanism to also patch psycopg2.connect directly, ensuring that any code that creates database connections through psycopg2 will use the same transaction cursor as the rest of the test suite.
Key changes:
- Added
_patched_psycopg2_connectfunction to intercept psycopg2 connection creation - Extended
patch()andunpatch()methods to also patch/unpatchpsycopg2.connect - Introduced global variable
_orig_psycopg2_connectto track the original psycopg2 connect function
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from destral.transaction import Transaction | ||
| cursor = Transaction().cursor | ||
| conn = _orig_psycopg2_connect(*args, **kwargs) | ||
| return PatchedConnection(conn, cursor) | ||
|
|
||
|
|
There was a problem hiding this comment.
The _patched_psycopg2_connect function creates a new connection using _orig_psycopg2_connect(*args, **kwargs) but then ignores the connection's cursor. Instead, it uses a cursor from Transaction().cursor and wraps the new connection with this existing cursor. This means the new connection created with the provided arguments will not be used as intended, potentially causing connection argument mismatches or unexpected behavior. The patched function should either return the cursor from the newly created connection or reconsider the logic of creating a new connection if it's going to use a different cursor anyway.
| from destral.transaction import Transaction | |
| cursor = Transaction().cursor | |
| conn = _orig_psycopg2_connect(*args, **kwargs) | |
| return PatchedConnection(conn, cursor) | |
| conn = _orig_psycopg2_connect(*args, **kwargs) | |
| cursor = conn.cursor() | |
| return PatchedConnection(conn, cursor) |
| if _orig_psycopg2_connect is None: | ||
| _orig_psycopg2_connect = psycopg2.connect | ||
| psycopg2.connect = _patched_psycopg2_connect |
There was a problem hiding this comment.
The patching of psycopg2.connect lacks proper restoration tracking. If the patch() method is called multiple times before unpatch() is called (e.g., in nested contexts), the global variable _orig_psycopg2_connect will only be set once (line 113-114), but subsequent calls will still replace psycopg2.connect with the patched version. This could lead to issues in nested patching scenarios. Consider storing the original function in an instance variable similar to how self.orig is used for sql_db.db_connect.
There was a problem hiding this comment.
@copilot add a commit to apply changes based on this feedback and, if possible, from now on comment me in spanish, please.
| def _patched_psycopg2_connect(*args, **kwargs): | ||
| from destral.transaction import Transaction | ||
| cursor = Transaction().cursor | ||
| conn = _orig_psycopg2_connect(*args, **kwargs) | ||
| return PatchedConnection(conn, cursor) |
There was a problem hiding this comment.
The new patched psycopg2 connection functionality lacks test coverage. While there is a test file spec/patch_spec.py that tests PatchedCursor, there are no tests for the new _patched_psycopg2_connect function or the psycopg2 patching behavior added to the patch() and unpatch() methods. Consider adding tests to verify that psycopg2.connect is properly patched and unpatched, and that the patched connection returns the expected cursor.
There was a problem hiding this comment.
@copilot Me podrías hacer los comentarios siempre en castellano, ¿por favor?
|
@davidmunoznovoa I've opened a new pull request, #169, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _patched_psycopg2_connect(*args, **kwargs): | ||
| from destral.transaction import Transaction | ||
| cursor = Transaction().cursor | ||
| conn = _orig_psycopg2_connect(*args, **kwargs) | ||
| return PatchedConnection(conn, cursor) |
There was a problem hiding this comment.
The _patched_psycopg2_connect function lacks documentation explaining its purpose, parameters, and return value. This is especially important since it's patching a widely-used library function and has non-obvious behavior (wrapping connections with a shared cursor).
| import psycopg2 | ||
| global _orig_psycopg2_connect | ||
| logger.info('Patching creation of new cursors') | ||
| self.orig = sql_db.db_connect | ||
| sql_db.db_connect = PatchNewCursors.db_connect | ||
| if _orig_psycopg2_connect is None: | ||
| _orig_psycopg2_connect = psycopg2.connect | ||
| psycopg2.connect = _patched_psycopg2_connect |
There was a problem hiding this comment.
Using a global variable _orig_psycopg2_connect creates potential thread-safety issues. If multiple threads call patch() and unpatch() concurrently, race conditions could occur where the wrong version of psycopg2.connect is saved or restored. Consider using thread-local storage or instance-level tracking to make this thread-safe.
| import psycopg2 | ||
| logger.info('Unpatching creation of new cursors') | ||
| sql_db.db_connect = self.orig | ||
| psycopg2.connect = _orig_psycopg2_connect |
There was a problem hiding this comment.
There is no tracking of whether psycopg2 was actually patched. If the patch method is called multiple times, _orig_psycopg2_connect will only be set on the first call (due to the None check), but unpatch will always restore to this same original. However, if someone calls unpatch when no patch is active, it will still attempt to restore, potentially causing unexpected behavior. Consider tracking patch state in an instance variable.
davidmunoznovoa
force-pushed
the
add-generic-patch-for-psycopg
branch
from
8fba43c to
7006d93
Compare
Objetivo
psycopg2para que sea única y se puedan testear correctamente procedimientos que creen o modifiquen valores en colecciones TimeScaleDB mediantecchloader.Relacionado