fix: accept prepare/binary kwargs in DictCursor.execute#10030
Conversation
psycopg.Connection.execute(query, params, *, prepare=None, binary=False)
delegates to its underlying cursor as
cur.execute(query, params, prepare=prepare). The DictCursor /
AsyncDictCursor in pgadmin.utils.driver.psycopg3.cursor narrowed the
signature to (self, query, params=None), so any caller that uses the
high-level Connection.execute() path with a cursor_factory=DictCursor
connection hits
TypeError: execute() got an unexpected keyword argument 'prepare'
In particular this breaks psycopg_pool.ConnectionPool.check_connection,
which sends conn.execute("") to validate every checkout — every
connection then looks broken to the pool and getconn() times out.
Forward prepare and binary to the underlying psycopg.Cursor /
psycopg.AsyncCursor execute. Defaults match psycopg's, so existing
callers see no behavior change. Adds a small regression test that
asserts both classes expose the kwargs as keyword-only parameters.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR extends pgAdmin's psycopg3 cursor wrappers to accept and forward keyword-only prepare and binary parameters in their execute() methods and adds a regression test to validate those signatures. Changespsycopg3 cursor execute keyword argument support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/pgadmin/utils/driver/psycopg3/cursor.py (1)
282-294: 💤 Low valueConsider adding
self._odt_desc = Nonereset for consistency.The synchronous
DictCursor.execute(line 197) resetsself._odt_desc = Nonebefore executing, but the async version does not. While the asyncordered_description()method (lines 278-280) always refreshes the description (unlike the sync version which caches), this inconsistency could lead to confusion and maintainability issues.Suggested alignment with sync version
def execute(self, query, params=None, *, prepare=None, binary=None): """ Execute function Mirrors ``DictCursor.execute`` so this cursor stays substitutable for ``psycopg.AsyncCursor`` when used via ``AsyncConnection.execute``. """ + self._odt_desc = None try: return asyncio.run( self._execute(query, params, prepare=prepare, binary=binary) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/utils/driver/psycopg3/cursor.py` around lines 282 - 294, The async execute method on this cursor should mirror the sync DictCursor.execute by resetting the ordered-description cache; before calling asyncio.run(self._execute(...)) in execute, set self._odt_desc = None to clear any stale description so ordered_description() behavior remains consistent with the sync path (references: execute, _execute, _odt_desc, ordered_description).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/pgadmin/utils/driver/psycopg3/cursor.py`:
- Around line 282-294: The async execute method on this cursor should mirror the
sync DictCursor.execute by resetting the ordered-description cache; before
calling asyncio.run(self._execute(...)) in execute, set self._odt_desc = None to
clear any stale description so ordered_description() behavior remains consistent
with the sync path (references: execute, _execute, _odt_desc,
ordered_description).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e945aab5-7749-4656-a241-a4e1aa745c95
📒 Files selected for processing (2)
web/pgadmin/utils/driver/psycopg3/cursor.pyweb/pgadmin/utils/tests/test_psycopg3_cursor_signature.py
There was a problem hiding this comment.
Pull request overview
Fixes psycopg3 DictCursor/AsyncDictCursor compatibility with psycopg.Connection.execute(..., prepare=..., binary=...) by accepting and forwarding those keyword-only parameters, preventing TypeError when pgAdmin uses a cursor_factory=DictCursor connection (notably in pooling health checks).
Changes:
- Extend
DictCursor.execute()andAsyncDictCursor.execute()signatures to acceptprepareandbinaryas keyword-only args and forward them to psycopg. - Update async execution path to pass through the new kwargs to the underlying async cursor execution method.
- Add a regression test asserting both cursor classes expose
prepareandbinaryas keyword-only parameters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
web/pgadmin/utils/driver/psycopg3/cursor.py |
Accepts/forwards prepare and binary kwargs to restore substitutability with psycopg cursors. |
web/pgadmin/utils/tests/test_psycopg3_cursor_signature.py |
Adds regression coverage to ensure the new kwargs are present and keyword-only. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous docstring framed the test as being about Connection.execute compatibility and listed prepare as the only kwarg that Connection.execute forwards. That left the binary assertion unmotivated. Reframe around psycopg.Cursor substitutability — DictCursor inherits from psycopg.Cursor, whose execute() exposes both prepare and binary as keyword-only — and call out that Connection.execute is the most visible failure path (it forwards prepare but uses binary to set cur.format instead of forwarding it). No code change.
|
@coderabbitai re: the
The Copilot comment about the test docstring was valid and is addressed in 6d9c119. |
|
Tip For best results, initiate chat on the files or code changes.
Thanks for the thorough explanation; noted for future reviews. ᓚᘏᗢ ✏️ Learnings added
|
Problem
psycopg.Connection.execute(query, params, *, prepare=None, binary=False)delegates to its underlying cursor ascur.execute(query, params, prepare=prepare). pgAdmin'sDictCursor/AsyncDictCursor(inweb/pgadmin/utils/driver/psycopg3/cursor.py) narrowed the signature to(self, query, params=None), so any caller that uses the high-levelConnection.execute()path with acursor_factory=DictCursorconnection trips:Concrete failure mode
The most visible victim is
psycopg_pool.ConnectionPool.check_connection, which sendsconn.execute("")to validate every checkout. With aDictCursor-using pool every connection looks broken to the pool — checkouts loop in the recovery path untilPoolTimeout. This blockscheck=ConnectionPool.check_connectionfrom being usable at all when the pool is built withconnection_classthat setscursor_factory=DictCursor.Minimal reproducer:
Fix
Forward
prepareandbinary(keyword-only) to the underlyingpsycopg.Cursor/psycopg.AsyncCursor. Defaults match psycopg's own (prepare=None,binary=None), so existing callers see no behavior change. RestoresLiskov-substitutability with the standard psycopg cursors.Test plan
make check-pep8— cleanDictCursor.executeandAsyncDictCursor.executeexposeprepareandbinaryas keyword-only parameterspgadmin.utils.tests.test_psycopg3_cursor_signature— passes underregression/runtests.py --pkg utils.tests(bothDictCursorandAsyncDictCursorscenarios)utils.testspackage — 30 passed / 0 failed / 4 unrelated skipsSummary by CodeRabbit
New Features
Tests