From 266d7b57c0894bd691eb24a5159e0f03d91e980a Mon Sep 17 00:00:00 2001 From: Dave Page Date: Tue, 9 Jun 2026 14:46:18 +0100 Subject: [PATCH 1/3] Answer False instead of raising when the config database is unreachable The commit message is worth restating because most of the original motivation has already been fixed on master: normalize_database_uri() and the "connection = None" guard landed with #9984, so the NameError itself is gone. What remains is that any failure to reach the database still propagates out of check_external_config_db() rather than being answered, along with the unreachable "return False" left stranded after the return above it. The container entrypoint currently papers over that by discarding stderr and keeping its own "False" default when the helper prints nothing, so the behaviour a user sees does not change. Making the fallback explicit does mean the helper now honours its contract for any other caller, and the comment records why False is the right answer: first launch has to proceed and create the user from PGADMIN_DEFAULT_EMAIL and PGADMIN_DEFAULT_PASSWORD, rather than leaving an installation nobody can log in to. create_engine() is inside the try as well, since it is what rejects a malformed URI, and the engine is now disposed rather than only its connection being closed, so a failed check does not leave a pool behind. Tests cover an unreachable host, a malformed URI and a reachable database with and without a server table. They import the module the way the entrypoint does, as a top level module from its own directory, so they also fail if that arrangement is broken. --- web/pgadmin/utils/check_external_config_db.py | 22 +++- .../tests/test_check_external_config_db.py | 111 ++++++++++++++++++ 2 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 web/pgadmin/utils/tests/test_check_external_config_db.py diff --git a/web/pgadmin/utils/check_external_config_db.py b/web/pgadmin/utils/check_external_config_db.py index e7b57d2fb37..320a5c00b88 100644 --- a/web/pgadmin/utils/check_external_config_db.py +++ b/web/pgadmin/utils/check_external_config_db.py @@ -16,12 +16,22 @@ def check_external_config_db(database_uri): Check if external config database exists if it is being used. """ - engine = create_engine(normalize_database_uri(database_uri)) - connection = None + engine = None try: - connection = engine.connect() - return inspect(engine).has_table("server") + engine = create_engine(normalize_database_uri(database_uri)) + with engine.connect(): + return inspect(engine).has_table("server") + except Exception: + # Anything that stops us reaching the database, a wrong password or + # an unreachable host as much as a malformed URI, is reported as + # "there is no external configuration database". The container + # entrypoint relies on that so first launch still creates the user + # from PGADMIN_DEFAULT_EMAIL and PGADMIN_DEFAULT_PASSWORD (#9984) + # rather than leaving an installation nobody can log in to. return False finally: - if connection: - connection.close() + # Guarded because create_engine() itself rejects a malformed URI, and + # an unbound name in the cleanup path is what caused this bug in the + # first place. + if engine is not None: + engine.dispose() diff --git a/web/pgadmin/utils/tests/test_check_external_config_db.py b/web/pgadmin/utils/tests/test_check_external_config_db.py new file mode 100644 index 00000000000..a818a6e3bf6 --- /dev/null +++ b/web/pgadmin/utils/tests/test_check_external_config_db.py @@ -0,0 +1,111 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Tests for check_external_config_db(). + +The container entrypoint calls this to decide whether an external +configuration database has already been initialised, and treats anything +other than "True" as "no, so run first-launch setup". It therefore has to +answer False rather than raise when the database cannot be reached at all, +which the previous "finally: connection.close()" prevented: engine.connect() +failing left connection unbound and the NameError escaped in place of the +answer. + +The module is imported the way the entrypoint imports it, as a top level +module from the directory it lives in, so that this also fails if that +arrangement is ever broken. +""" + +import os +import sys + +from pgadmin.utils.route import BaseTestGenerator +from regression.python_test_utils import test_utils as utils + +UTILS_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +if UTILS_DIR not in sys.path: + sys.path.append(UTILS_DIR) + +from check_external_config_db import check_external_config_db # noqa: E402 + + +class CheckExternalConfigDBTestCase(BaseTestGenerator): + """check_external_config_db() must answer, not raise.""" + + scenarios = [ + ('An unreachable host answers False', dict( + case='unreachable')), + ('A malformed URI answers False', dict( + case='malformed')), + ('A reachable database with no server table answers False', dict( + case='reachable_without_table')), + ('A reachable database with a server table answers True', dict( + case='reachable_with_table')), + ] + + def setUp(self): + self.created_table = False + self.db_name = self.server['db'] + + def _uri(self): + return 'postgresql://{0}:{1}@{2}:{3}/{4}'.format( + self.server['username'], self.server['db_password'], + self.server['host'], self.server['port'], self.db_name) + + def _connect(self): + return utils.get_db_connection(self.db_name, + self.server['username'], + self.server['db_password'], + self.server['host'], + self.server['port'], + self.server['sslmode']) + + def runTest(self): + if self.case == 'unreachable': + # Port 1 is not something a PostgreSQL server listens on, so the + # connection is refused rather than timing out. + self.assertFalse(check_external_config_db( + 'postgresql://pgadmin:pgadmin@127.0.0.1:1/pgadmin')) + return + + if self.case == 'malformed': + self.assertFalse(check_external_config_db('not a uri at all')) + return + + if self.case == 'reachable_without_table': + self.assertFalse(check_external_config_db(self._uri())) + return + + connection = self._connect() + try: + old_isolation_level = connection.isolation_level + utils.set_isolation_level(connection, 0) + cursor = connection.cursor() + cursor.execute('CREATE TABLE public.server (id serial)') + utils.set_isolation_level(connection, old_isolation_level) + connection.commit() + self.created_table = True + finally: + connection.close() + + self.assertTrue(check_external_config_db(self._uri())) + + def tearDown(self): + if not self.created_table: + return + connection = self._connect() + try: + old_isolation_level = connection.isolation_level + utils.set_isolation_level(connection, 0) + cursor = connection.cursor() + cursor.execute('DROP TABLE public.server') + utils.set_isolation_level(connection, old_isolation_level) + connection.commit() + finally: + connection.close() From 482e6d8672196747fabbb8b772dc497c3d898b1f Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 09:16:02 +0100 Subject: [PATCH 2/3] fix: encode _uri() credentials/socket host, tighten created_table tracking _uri() built the test URI by dropping the configured host straight into the authority component. On the Linux/macOS CI runners that host is a Unix domain socket directory, and a "/" there is parsed as the start of the path rather than part of the host, leaving the host/port undetermined and the database name mangled - which is why the "reachable database with a server table" scenario failed there while passing on Windows (TCP host). Detect a socket-directory host and use libpq's query-parameter form instead, and URL-encode the username and password in both branches. Also record self.created_table immediately after CREATE TABLE succeeds rather than after the isolation-level restore and commit, so tearDown still drops the table if either of those later steps fails; tearDown's DROP now uses IF EXISTS to stay safe either way. --- .../tests/test_check_external_config_db.py | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/web/pgadmin/utils/tests/test_check_external_config_db.py b/web/pgadmin/utils/tests/test_check_external_config_db.py index a818a6e3bf6..9b83bba8f34 100644 --- a/web/pgadmin/utils/tests/test_check_external_config_db.py +++ b/web/pgadmin/utils/tests/test_check_external_config_db.py @@ -24,6 +24,7 @@ import os import sys +from urllib.parse import quote from pgadmin.utils.route import BaseTestGenerator from regression.python_test_utils import test_utils as utils @@ -54,9 +55,25 @@ def setUp(self): self.db_name = self.server['db'] def _uri(self): + username = quote(str(self.server['username']), safe='') + password = quote(str(self.server['db_password']), safe='') + host = self.server['host'] + port = self.server['port'] + + # A Unix domain socket directory (as used on the Linux/macOS test + # runners) can't be embedded in the URI's authority component: a + # "/" there is parsed as the start of the path, not part of the + # host, leaving the host/port undetermined and the database name + # mangled. libpq's URI form for that case instead leaves the + # authority's host empty and passes the socket directory as the + # "host" query parameter. + if '/' in str(host): + return 'postgresql://{0}:{1}@/{2}?host={3}&port={4}'.format( + username, password, self.db_name, + quote(str(host), safe=''), port) + return 'postgresql://{0}:{1}@{2}:{3}/{4}'.format( - self.server['username'], self.server['db_password'], - self.server['host'], self.server['port'], self.db_name) + username, password, host, port, self.db_name) def _connect(self): return utils.get_db_connection(self.db_name, @@ -88,9 +105,12 @@ def runTest(self): utils.set_isolation_level(connection, 0) cursor = connection.cursor() cursor.execute('CREATE TABLE public.server (id serial)') + # Recorded as soon as the table exists, before the isolation + # level restore and commit below, so tearDown still drops it + # if either of those later steps were to fail. + self.created_table = True utils.set_isolation_level(connection, old_isolation_level) connection.commit() - self.created_table = True finally: connection.close() @@ -104,7 +124,7 @@ def tearDown(self): old_isolation_level = connection.isolation_level utils.set_isolation_level(connection, 0) cursor = connection.cursor() - cursor.execute('DROP TABLE public.server') + cursor.execute('DROP TABLE IF EXISTS public.server') utils.set_isolation_level(connection, old_isolation_level) connection.commit() finally: From 5a0cd42670d50c11c5d89a36893959a4e9e2557a Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 16:38:12 +0100 Subject: [PATCH 3/3] Inspect the managed connection rather than the engine inspect(engine) checks out its own connection from the pool, so the one opened by the context manager was never used and the check could open two. Binding the inspector to the managed connection means has_table() runs on the connection whose lifetime the with block controls. --- web/pgadmin/utils/check_external_config_db.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/pgadmin/utils/check_external_config_db.py b/web/pgadmin/utils/check_external_config_db.py index 320a5c00b88..18a055523c2 100644 --- a/web/pgadmin/utils/check_external_config_db.py +++ b/web/pgadmin/utils/check_external_config_db.py @@ -19,8 +19,8 @@ def check_external_config_db(database_uri): engine = None try: engine = create_engine(normalize_database_uri(database_uri)) - with engine.connect(): - return inspect(engine).has_table("server") + with engine.connect() as connection: + return inspect(connection).has_table("server") except Exception: # Anything that stops us reaching the database, a wrong password or # an unreachable host as much as a malformed URI, is reported as