Ensure to use psycopg3 is used while connecting to postgres DB when PGADMIN_CONFIG_CONFIG_DATABASE_URI is specified. #9701#9914
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPostgreSQL connection URIs are normalized: occurrences of ChangesPostgreSQL URI Scheme Normalization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/utils/check_external_config_db.py (1)
22-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
connectionis unbound in thefinallyblock whenengine.connect()raises.If
engine.connect()at line 23 throws (bad credentials, host unreachable, etc.),connectionis never assigned. Theexcept Exception: return Falsebranch begins executing, but Python runsfinallybefore the return, hittingconnection.close()→NameError: name 'connection' is not defined. ThisNameErrorpropagates out, completely overriding the intendedFalsereturn, so callers never get the graceful failure.🐛 Proposed fix
+ connection = None try: connection = engine.connect() if inspect(engine).has_table("server"): return True return False except Exception: return False finally: - connection.close() + if connection is not None: + connection.close()🤖 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/check_external_config_db.py` around lines 22 - 30, The finally block closes a connection variable that may not be assigned if engine.connect() raises; initialize connection = None before the try (or use a context manager with engine.connect()) and in the finally only call connection.close() if connection is not None (or rely on the context manager to auto-close). Update the try/except/finally around engine.connect(), inspect(engine).has_table("server"), and connection.close() to guard against an unbound NameError.
🤖 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.
Inline comments:
In `@web/pgadmin/utils/check_external_config_db.py`:
- Around line 18-20: The E125 pycodestyle error comes from the broken
continuation in the if statement using database_uri.startswith across two lines;
change the condition to use a single startswith call with a tuple (e.g.
database_uri.startswith(("postgresql://", "postgres://"))) so the if is
single-line, then keep the existing replacement logic that rewrites ":// " to
"+psycopg://" on database_uri (referencing the database_uri variable, the
startswith check, and the existing replace call).
---
Outside diff comments:
In `@web/pgadmin/utils/check_external_config_db.py`:
- Around line 22-30: The finally block closes a connection variable that may not
be assigned if engine.connect() raises; initialize connection = None before the
try (or use a context manager with engine.connect()) and in the finally only
call connection.close() if connection is not None (or rely on the context
manager to auto-close). Update the try/except/finally around engine.connect(),
inspect(engine).has_table("server"), and connection.close() to guard against an
unbound NameError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 923b9d2a-08a5-407f-a036-74b2a2bafd5d
📒 Files selected for processing (1)
web/pgadmin/utils/check_external_config_db.py
Code reviewFound 2 issues:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Fixed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/utils/check_external_config_db.py (1)
25-33:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard connection cleanup when
engine.connect()fails.If connect raises,
connectionis never bound, andfinally: connection.close()can raiseUnboundLocalError, masking the intendedFalsereturn path.Proposed fix
engine = create_engine(database_uri) try: - connection = engine.connect() - if inspect(engine).has_table("server"): - return True - return False + with engine.connect() as connection: + return inspect(connection).has_table("server") except Exception: return False - finally: - connection.close()🤖 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/check_external_config_db.py` around lines 25 - 33, The finally block can raise UnboundLocalError if engine.connect() fails because connection is not set; update the check_external_config_db logic that calls engine.connect() and uses connection.close() by either initializing connection = None before the try or, better, using a context manager (engine.connect()) to ensure safe cleanup; specifically adjust the block around engine.connect(), inspect(engine).has_table("server") and connection.close() so you only call connection.close() when connection is truthy (or use "with engine.connect() as connection:" to eliminate the manual close).
🧹 Nitpick comments (1)
web/pgadmin/__init__.py (1)
340-344: ⚡ Quick winCentralize DB URI normalization to one shared helper.
This rewrite now exists in both
web/pgadmin/__init__.pyandweb/pgadmin/utils/check_external_config_db.py; extracting a single helper will prevent future drift between pre-check and runtime assignment paths.🤖 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/__init__.py` around lines 340 - 344, Extract the DB URI normalization logic currently duplicated in the module that sets app.config['SQLALCHEMY_DATABASE_URI'] and in check_external_config_db.py into a single shared helper (e.g., a function named normalize_database_uri) placed in the existing utils module used by both files; replace the inline re.sub call in web/pgadmin/__init__.py (the block that builds _db_uri) and the corresponding code in check_external_config_db.py to call normalize_database_uri(config.CONFIG_DATABASE_URI) so both pre-check and runtime assignment use the identical transformation.
🤖 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.
Inline comments:
In `@web/pgadmin/utils/check_external_config_db.py`:
- Around line 21-23: The re.sub call that assigns database_uri exceeds the line
length; break the arguments across multiple lines to satisfy pycodestyle E501
without changing behavior. Locate the assignment to database_uri where re.sub is
called and format it like: re.sub( pattern, replacement, database_uri, count=1 )
with each argument on its own line (or pattern/replacement on one line and
database_uri/count on the next) so the long line is wrapped but the regex,
replacement string ("postgresql+psycopg://") and count=1 are unchanged.
---
Outside diff comments:
In `@web/pgadmin/utils/check_external_config_db.py`:
- Around line 25-33: The finally block can raise UnboundLocalError if
engine.connect() fails because connection is not set; update the
check_external_config_db logic that calls engine.connect() and uses
connection.close() by either initializing connection = None before the try or,
better, using a context manager (engine.connect()) to ensure safe cleanup;
specifically adjust the block around engine.connect(),
inspect(engine).has_table("server") and connection.close() so you only call
connection.close() when connection is truthy (or use "with engine.connect() as
connection:" to eliminate the manual close).
---
Nitpick comments:
In `@web/pgadmin/__init__.py`:
- Around line 340-344: Extract the DB URI normalization logic currently
duplicated in the module that sets app.config['SQLALCHEMY_DATABASE_URI'] and in
check_external_config_db.py into a single shared helper (e.g., a function named
normalize_database_uri) placed in the existing utils module used by both files;
replace the inline re.sub call in web/pgadmin/__init__.py (the block that builds
_db_uri) and the corresponding code in check_external_config_db.py to call
normalize_database_uri(config.CONFIG_DATABASE_URI) so both pre-check and runtime
assignment use the identical transformation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da20045a-80cb-4264-bd30-6761a636dd7f
📒 Files selected for processing (2)
web/pgadmin/__init__.pyweb/pgadmin/utils/check_external_config_db.py
|
I encountered this error while upgrading pgAdmin from version |
…GADMIN_CONFIG_CONFIG_DATABASE_URI is specified.
yogeshmahajan-1903
force-pushed
the
master
branch
from
f34a5b3 to
1b7ad09
Compare
3 participants
Summary by CodeRabbit