Ensure to use psycopg3 is used while connecting to postgres DB when PGADMIN_CONFIG_CONFIG_DATABASE_URI is specified. #9701#9914
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPostgreSQL connection URIs are normalized: occurrences of ChangesPostgreSQL URI Scheme Normalization
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 |
![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
asheshv
left a comment
asheshv
left a comment
There was a problem hiding this comment.
Thanks for the fix — the core change is correct. A couple of follow-ups before merge:
1. Extract the rewrite into a shared helper.
The same re.sub(r"^postgres(ql)?://", "postgresql+psycopg://", ...) lives in both web/pgadmin/__init__.py and web/pgadmin/utils/check_external_config_db.py. A single normalize_database_uri(uri) helper (e.g. under web/pgadmin/utils/) used by both call sites prevents drift if another driver (asyncpg, cockroachdb, …) ever needs similar treatment, and removes the asymmetric startswith guard that currently exists in only one of the two sites.
2. Add a unit test for the rewrite.
Small but valuable for regression prevention. Suggested cases:
postgres://u:p@h/db→postgresql+psycopg://u:p@h/dbpostgresql://u:p@h/db→postgresql+psycopg://u:p@h/dbpostgresql+psycopg://…→ unchanged (idempotency)sqlite:///…→ unchanged
Nits (optional):
r"^postgres(ql)?://"→r"^postgres(?:ql)?://"(non-capturing group; cosmetic)._db_uriinweb/pgadmin/__init__.pyis single-use — there.subresult can be assigned toSQLALCHEMY_DATABASE_URIdirectly.- A release-notes entry referencing #9701 would be nice.
Otherwise LGTM — the regex is anchored, count=1, idempotent for already-prefixed URIs, and both URI-writing paths (create_app and the Docker pre-flight via check_external_config_db) are covered.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/evaluate_config.py`:
- Around line 119-120: The assignment to
custom_config_settings['CONFIG_DATABASE_URI'] is too long for pycodestyle; split
the expression so the line length is below the limit by wrapping the call to
normalize_database_uri across lines (for example, use parentheses to break the
RHS onto a new line or assign the input to a short temporary variable and pass
that into normalize_database_uri). Update the statement referencing
custom_config_settings['CONFIG_DATABASE_URI'] and normalize_database_uri(...)
accordingly so the resulting lines comply with the style guide.
In `@web/pgadmin/utils/db_utils.py`:
- Line 19: The long return statement that rewrites the URI using re.sub is
exceeding line length; split the expression across multiple lines to satisfy
pycodestyle E501 (for example, break after the comma and wrap the pattern/string
in parentheses or assign parts to short variables) while keeping the same call:
re.sub(r"^postgres(?:ql)?://", "postgresql+psycopg://", uri, count=1). Ensure
you preserve the exact regex, replacement and count=1 argument and only reformat
for line length.
🪄 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: 1359f7db-a511-46f5-a733-e5fb385fc212
📒 Files selected for processing (4)
web/pgadmin/__init__.pyweb/pgadmin/evaluate_config.pyweb/pgadmin/utils/db_utils.pyweb/pgadmin/utils/tests/test_db_utils.py
|
@asheshv @yogeshmahajan-1903 - After updating from ...
File "/pgadmin4/pgadmin/utils/check_external_config_db.py", line 18, in check_external_config_db
raise exc.ArgumentError(
"Could not parse SQLAlchemy URL from given URL string"
)
...Unfortunately, the suggestion in #9701 (comment) to adjust the connection string didn't work for me. |
yogeshmahajan-1903
force-pushed
the
master
branch
from
e4c029e to
2b1861f
Compare
|
Could you please provide an example for your DB_URI? |
3 participants
Summary by CodeRabbit