Ensure to use psycopg3 is used while connecting to postgres DB when PGADMIN_CONFIG_CONFIG_DATABASE_URI is specified. #9701#9914
Conversation
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
…GADMIN_CONFIG_CONFIG_DATABASE_URI is specified.
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
| database_uri = re.sub( | ||
| r"^postgres(ql)?://", "postgresql+psycopg://", database_uri, count=1 | ||
| ) |
There was a problem hiding this comment.
Fix pycodestyle E501 on Line 22 (currently failing CI).
The substitution call exceeds the configured line length limit; wrapping arguments resolves the pipeline failure without changing behavior.
Proposed fix
if database_uri.startswith(("postgresql://", "postgres://")):
database_uri = re.sub(
- r"^postgres(ql)?://", "postgresql+psycopg://", database_uri, count=1
+ r"^postgres(ql)?://",
+ "postgresql+psycopg://",
+ database_uri,
+ count=1,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| database_uri = re.sub( | |
| r"^postgres(ql)?://", "postgresql+psycopg://", database_uri, count=1 | |
| ) | |
| database_uri = re.sub( | |
| r"^postgres(ql)?://", | |
| "postgresql+psycopg://", | |
| database_uri, | |
| count=1, | |
| ) |
🧰 Tools
🪛 GitHub Actions: Check Python style
[error] 22-22: pycodestyle reported [E501] line too long (80 > 79 characters).
🤖 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 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.
2 participants
Summary by CodeRabbit