You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/08/20 00:42:59 UTC
(superset) 01/01: fix(sqllab): default PostgreSQL port to 5432 in the dynamic connection form
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-postgres-dynamic-form-default-port
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 8a8940557dc9107fb693bb6a30847e09665c1fce
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Thu Aug 20 00:40:45 2026 +0000
fix(sqllab): default PostgreSQL port to 5432 in the dynamic connection form
---
superset/db_engine_specs/postgres.py | 142 +++++++++++++
tests/integration_tests/databases/api_tests.py | 8 +-
.../integration_tests/databases/commands_tests.py | 18 +-
.../db_engine_specs/postgres_tests.py | 6 +-
tests/unit_tests/db_engine_specs/test_postgres.py | 236 +++++++++++++++++++++
5 files changed, 398 insertions(+), 12 deletions(-)
diff --git a/superset/db_engine_specs/postgres.py b/superset/db_engine_specs/postgres.py
index 53cc97e597b..071a31c761f 100644
--- a/superset/db_engine_specs/postgres.py
+++ b/superset/db_engine_specs/postgres.py
@@ -24,6 +24,8 @@ from re import Pattern
from typing import Any, Callable, Optional, TYPE_CHECKING
from flask_babel import gettext as __
+from marshmallow import fields, pre_load
+from marshmallow.validate import Range
from sqlalchemy import text, types
from sqlalchemy.dialects.postgresql import DOUBLE_PRECISION, ENUM, INTERVAL, JSON
from sqlalchemy.dialects.postgresql.base import PGInspector
@@ -37,6 +39,9 @@ from superset.db_engine_specs.base import (
AURORA_DATA_API_KNOWN_INCOMPATIBILITIES,
BaseEngineSpec,
BasicParametersMixin,
+ BasicParametersSchema,
+ BasicParametersType,
+ BasicPropertiesType,
DatabaseCategory,
TimestampExpression,
)
@@ -46,6 +51,7 @@ from superset.models.sql_lab import Query
from superset.sql.parse import process_jinja_sql
from superset.utils import core as utils, json
from superset.utils.core import GenericDataType, QuerySource
+from superset.utils.network import is_hostname_valid, is_port_open
if TYPE_CHECKING:
from superset.models.core import Database # pragma: no cover
@@ -298,6 +304,34 @@ class PostgresBaseEngineSpec(BaseEngineSpec):
return None
+class PostgresParametersSchema(BasicParametersSchema):
+ """
+ Same as ``BasicParametersSchema``, except ``port`` is optional: a blank
+ port falls back to Postgres's own default (5432) in
+ ``PostgresEngineSpec.build_sqlalchemy_uri``.
+ """
+
+ port = fields.Integer(
+ required=False,
+ allow_none=True,
+ metadata={"description": __("Database port")},
+ validate=Range(min=0, max=2**16, max_inclusive=False),
+ )
+
+ @pre_load
+ def blank_port_to_none(self, data: Any, **kwargs: Any) -> Any:
+ """
+ A cleared number input in the Connect Database form submits ``""``
+ for ``port`` (HTML input values are always strings) rather than
+ omitting the key or sending ``null``. Normalize it to ``None`` so it
+ deserializes cleanly instead of failing with "Not a valid integer.",
+ and is treated as blank -- same as an omitted port -- downstream.
+ """
+ if isinstance(data, dict) and data.get("port") == "":
+ data = {**data, "port": None}
+ return data
+
+
class PostgresEngineSpec(BasicParametersMixin, PostgresBaseEngineSpec):
engine = "postgresql"
engine_name = "PostgreSQL"
@@ -309,6 +343,7 @@ class PostgresEngineSpec(BasicParametersMixin, PostgresBaseEngineSpec):
supports_grouping_sets = True
default_driver = "psycopg2"
+ parameters_schema = PostgresParametersSchema()
sqlalchemy_uri_placeholder = (
"postgresql://user:password@host:port/dbname[?key=value&key=value...]"
)
@@ -674,6 +709,113 @@ class PostgresEngineSpec(BasicParametersMixin, PostgresBaseEngineSpec):
return uri, connect_args
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ """
+ Default a missing/blank port to Postgres's own default (5432) so the
+ dynamic form can connect without requiring the port to be filled in.
+
+ Only an absent key, ``None``, or ``""`` (what a cleared number input
+ submits, since this may be called directly with raw, non-schema-
+ loaded parameters -- see ``ValidateDatabaseParametersCommand``) are
+ treated as blank; an explicitly supplied port -- including ``0`` --
+ is preserved as-is rather than overwritten by a truthiness check.
+ """
+ port = parameters.get("port")
+ resolved_port: int = (
+ cls.metadata["default_port"] if port is None or port == "" else port
+ )
+ parameters_with_default_port: BasicParametersType = {
+ **parameters,
+ "port": resolved_port,
+ }
+ return super().build_sqlalchemy_uri(
+ parameters_with_default_port, encrypted_extra
+ )
+
+ @classmethod
+ def validate_parameters(
+ cls, properties: BasicPropertiesType
+ ) -> list[SupersetError]:
+ """
+ Validates any number of parameters, for progressive validation.
+
+ Same as ``BasicParametersMixin.validate_parameters``, except ``port``
+ is not a required parameter: a blank port is valid, since
+ ``build_sqlalchemy_uri`` falls back to Postgres's own default. Port
+ format/range/open checks still run whenever a port is present.
+ """
+ errors: list[SupersetError] = []
+
+ required = {"host", "username", "database"}
+ parameters = properties.get("parameters", {})
+ present = {key for key in parameters if parameters.get(key, ())}
+
+ if missing := sorted(required - present):
+ errors.append(
+ SupersetError(
+ message=f"One or more parameters are missing: {', '.join(missing)}",
+ error_type=SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR,
+ level=ErrorLevel.WARNING,
+ extra={"missing": missing},
+ ),
+ )
+
+ host = parameters.get("host", None)
+ if not host:
+ return errors
+ if not is_hostname_valid(host):
+ errors.append(
+ SupersetError(
+ message="The hostname provided can't be resolved.",
+ error_type=SupersetErrorType.CONNECTION_INVALID_HOSTNAME_ERROR,
+ level=ErrorLevel.ERROR,
+ extra={"invalid": ["host"]},
+ ),
+ )
+ return errors
+
+ port = parameters.get("port", None)
+ if not port:
+ return errors
+ try:
+ port = int(port)
+ except (ValueError, TypeError):
+ errors.append(
+ SupersetError(
+ message="Port must be a valid integer.",
+ error_type=SupersetErrorType.CONNECTION_INVALID_PORT_ERROR,
+ level=ErrorLevel.ERROR,
+ extra={"invalid": ["port"]},
+ ),
+ )
+ if not (isinstance(port, int) and 0 <= port < 2**16):
+ errors.append(
+ SupersetError(
+ message=(
+ "The port must be an integer between 0 and 65535 (inclusive)."
+ ),
+ error_type=SupersetErrorType.CONNECTION_INVALID_PORT_ERROR,
+ level=ErrorLevel.ERROR,
+ extra={"invalid": ["port"]},
+ ),
+ )
+ elif not is_port_open(host, port):
+ errors.append(
+ SupersetError(
+ message="The port is closed.",
+ error_type=SupersetErrorType.CONNECTION_PORT_CLOSED_ERROR,
+ level=ErrorLevel.ERROR,
+ extra={"invalid": ["port"]},
+ ),
+ )
+
+ return errors
+
@staticmethod
def mutate_db_for_connection_test(database: Database) -> None:
"""
diff --git a/tests/integration_tests/databases/api_tests.py b/tests/integration_tests/databases/api_tests.py
index 48646ce2933..0c98ac92927 100644
--- a/tests/integration_tests/databases/api_tests.py
+++ b/tests/integration_tests/databases/api_tests.py
@@ -3968,8 +3968,8 @@ class TestDatabaseApi(SupersetTestCase):
]
}
- @mock.patch("superset.db_engine_specs.base.is_hostname_valid")
- @mock.patch("superset.db_engine_specs.base.is_port_open")
+ @mock.patch("superset.db_engine_specs.postgres.is_hostname_valid")
+ @mock.patch("superset.db_engine_specs.postgres.is_port_open")
@mock.patch("superset.databases.api.ValidateDatabaseParametersCommand")
def test_validate_parameters_valid_payload(
self,
@@ -4059,7 +4059,7 @@ class TestDatabaseApi(SupersetTestCase):
]
}
- @mock.patch("superset.db_engine_specs.base.is_hostname_valid")
+ @mock.patch("superset.db_engine_specs.postgres.is_hostname_valid")
def test_validate_parameters_invalid_host(self, is_hostname_valid):
is_hostname_valid.return_value = False
@@ -4119,7 +4119,7 @@ class TestDatabaseApi(SupersetTestCase):
]
}
- @mock.patch("superset.db_engine_specs.base.is_hostname_valid")
+ @mock.patch("superset.db_engine_specs.postgres.is_hostname_valid")
def test_validate_parameters_invalid_port_range(self, is_hostname_valid):
is_hostname_valid.return_value = True
diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py
index 734dd55a3f4..2b7540eda70 100644
--- a/tests/integration_tests/databases/commands_tests.py
+++ b/tests/integration_tests/databases/commands_tests.py
@@ -1063,8 +1063,8 @@ class TestTestConnectionDatabaseCommand(SupersetTestCase):
mock_event_logger.assert_called()
-@patch("superset.db_engine_specs.base.is_hostname_valid")
-@patch("superset.db_engine_specs.base.is_port_open")
+@patch("superset.db_engine_specs.postgres.is_hostname_valid")
+@patch("superset.db_engine_specs.postgres.is_port_open")
@patch("superset.commands.database.validate.DatabaseDAO")
def test_validate(
mock_database_dao, # noqa: N803
@@ -1093,8 +1093,8 @@ def test_validate(
command.run()
-@patch("superset.db_engine_specs.base.is_hostname_valid")
-@patch("superset.db_engine_specs.base.is_port_open")
+@patch("superset.db_engine_specs.postgres.is_hostname_valid")
+@patch("superset.db_engine_specs.postgres.is_port_open")
def test_validate_partial(is_port_open, is_hostname_valid, app_context):
"""
Test parameter validation when only some parameters are present.
@@ -1134,10 +1134,14 @@ def test_validate_partial(is_port_open, is_hostname_valid, app_context):
]
-@patch("superset.db_engine_specs.base.is_hostname_valid")
+@patch("superset.db_engine_specs.postgres.is_hostname_valid")
def test_validate_partial_invalid_hostname(is_hostname_valid, app_context):
"""
Test parameter validation when only some parameters are present.
+
+ ``port`` is intentionally absent from the payload (and from the expected
+ "missing" list below): it is no longer a required parameter for
+ Postgres, since a blank port falls back to the default (5432).
"""
is_hostname_valid.return_value = False
@@ -1157,11 +1161,11 @@ def test_validate_partial_invalid_hostname(is_hostname_valid, app_context):
command.run()
assert excinfo.value.errors == [
SupersetError(
- message="One or more parameters are missing: database, port, username",
+ message="One or more parameters are missing: database, username",
error_type=SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR,
level=ErrorLevel.WARNING,
extra={
- "missing": ["database", "port", "username"],
+ "missing": ["database", "username"],
"issue_codes": [
{
"code": 1018,
diff --git a/tests/integration_tests/db_engine_specs/postgres_tests.py b/tests/integration_tests/db_engine_specs/postgres_tests.py
index 236d293df4b..e43b6483c63 100644
--- a/tests/integration_tests/db_engine_specs/postgres_tests.py
+++ b/tests/integration_tests/db_engine_specs/postgres_tests.py
@@ -491,6 +491,7 @@ def test_base_parameters_mixin():
"minimum": 0,
"maximum": 65536,
"description": "Database port",
+ "nullable": True,
},
"password": {"type": "string", "nullable": True, "description": "Password"},
"username": {"type": "string", "nullable": True, "description": "Username"},
@@ -504,7 +505,10 @@ def test_base_parameters_mixin():
"type": "boolean",
},
},
- "required": ["database", "host", "port", "username"],
+ # ``port`` is intentionally not required: a blank port falls back to
+ # Postgres's own default (5432) in ``PostgresEngineSpec.build_sqlalchemy_uri``.
+ "required": ["database", "host", "username"],
+ "additionalProperties": False,
}
diff --git a/tests/unit_tests/db_engine_specs/test_postgres.py b/tests/unit_tests/db_engine_specs/test_postgres.py
index 56e2815549e..97116715480 100644
--- a/tests/unit_tests/db_engine_specs/test_postgres.py
+++ b/tests/unit_tests/db_engine_specs/test_postgres.py
@@ -31,6 +31,7 @@ from superset.db_engine_specs.postgres import (
_check_not_redshift,
PostgresEngineSpec as spec, # noqa: N813
)
+from superset.errors import SupersetErrorType
from superset.exceptions import SupersetSecurityException
from superset.sql.parse import Table
from superset.utils.core import GenericDataType
@@ -501,3 +502,238 @@ def test_get_schema_names_excludes_only_actual_system_schemas(
"pgstats",
"information_schema",
}
+
+
+def _basic_parameters(**overrides: Any) -> dict[str, Any]:
+ parameters: dict[str, Any] = {
+ "username": "user",
+ "password": "pwd",
+ "host": "localhost",
+ "port": 5432,
+ "database": "db",
+ "query": {},
+ }
+ parameters.update(overrides)
+ return parameters
+
+
+def test_build_sqlalchemy_uri_defaults_missing_port_to_5432() -> None:
+ """
+ DB Eng Specs (postgres): ``build_sqlalchemy_uri`` defaults a missing
+ ``port`` key to the class's own declared default (5432) instead of
+ raising a ``KeyError``, so the dynamic form can connect without a port.
+ """
+ parameters = _basic_parameters()
+ del parameters["port"]
+
+ uri = spec.build_sqlalchemy_uri(parameters) # type: ignore[arg-type]
+
+ assert make_url(uri).port == 5432
+ assert spec.metadata["default_port"] == 5432
+
+
+def test_build_sqlalchemy_uri_defaults_blank_port_to_5432() -> None:
+ """
+ DB Eng Specs (postgres): ``build_sqlalchemy_uri`` defaults a blank
+ (``None``) ``port`` value to 5432 rather than emitting ``port=None``.
+ """
+ parameters = _basic_parameters(port=None)
+
+ uri = spec.build_sqlalchemy_uri(parameters) # type: ignore[arg-type]
+
+ assert make_url(uri).port == 5432
+
+
+def test_build_sqlalchemy_uri_respects_explicit_port() -> None:
+ """
+ DB Eng Specs (postgres): an explicitly provided port is still honored
+ and not overridden by the default.
+ """
+ parameters = _basic_parameters(port=5433)
+
+ uri = spec.build_sqlalchemy_uri(parameters) # type: ignore[arg-type]
+
+ assert make_url(uri).port == 5433
+
+
+def test_build_sqlalchemy_uri_preserves_explicit_port_zero() -> None:
+ """
+ DB Eng Specs (postgres): an explicitly supplied port of ``0`` (a value
+ the schema's ``Range(min=0, ...)`` validator accepts) must not be
+ silently overwritten by the default port. A truthiness check like
+ ``port or default`` would incorrectly replace ``0`` with 5432.
+ """
+ parameters = _basic_parameters(port=0)
+
+ uri = spec.build_sqlalchemy_uri(parameters) # type: ignore[arg-type]
+
+ assert make_url(uri).port == 0
+
+
+def test_build_sqlalchemy_uri_defaults_empty_string_port_to_5432() -> None:
+ """
+ DB Eng Specs (postgres): ``build_sqlalchemy_uri`` may be called directly
+ with raw, non-schema-loaded parameters (see
+ ``ValidateDatabaseParametersCommand``), where a cleared number input
+ submits ``""`` rather than ``null``. That must default to 5432 rather
+ than raising when SQLAlchemy tries to parse ``""`` as a port.
+ """
+ parameters = _basic_parameters(port="")
+
+ uri = spec.build_sqlalchemy_uri(parameters) # type: ignore[arg-type]
+
+ assert make_url(uri).port == 5432
+
+
+def test_parameters_schema_blank_port_string_loads_as_none() -> None:
+ """
+ DB Eng Specs (postgres): the Connect Database form's Port field is a
+ number input; clearing it submits ``""`` (HTML input values are always
+ strings), not ``null``. The schema must normalize that to ``None``
+ instead of rejecting it with "Not a valid integer.", so the dynamic
+ form's CONNECT flow (which loads through ``parameters_schema`` before
+ calling ``build_sqlalchemy_uri``) succeeds with a blank port.
+ """
+ loaded = spec.parameters_schema.load(_basic_parameters(port=""))
+
+ assert loaded["port"] is None
+
+
+def test_validate_parameters_blank_port_is_not_a_missing_parameter(
+ mocker: MockerFixture,
+) -> None:
+ """
+ DB Eng Specs (postgres): a blank/missing ``port`` must not trigger
+ ``CONNECTION_MISSING_PARAMETERS_ERROR``, since ``build_sqlalchemy_uri``
+ falls back to the default Postgres port.
+ """
+ mocker.patch(
+ "superset.db_engine_specs.postgres.is_hostname_valid", return_value=True
+ )
+
+ properties = {"parameters": _basic_parameters(port=None)}
+ errors = spec.validate_parameters(properties) # type: ignore[arg-type]
+
+ for error in errors:
+ assert "port" not in (error.extra or {}).get("missing", [])
+ assert error.error_type != SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR
+
+
+def test_validate_parameters_missing_host_still_errors(
+ mocker: MockerFixture,
+) -> None:
+ """
+ DB Eng Specs (postgres): omitting ``host`` still reports it as missing;
+ only ``port`` was made optional.
+ """
+ properties = {"parameters": _basic_parameters(host="", port=None)}
+ errors = spec.validate_parameters(properties) # type: ignore[arg-type]
+
+ assert len(errors) == 1
+ assert errors[0].error_type == SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR
+ assert (errors[0].extra or {})["missing"] == ["host"]
+
+
+def test_validate_parameters_missing_other_required_field_still_errors(
+ mocker: MockerFixture,
+) -> None:
+ """
+ DB Eng Specs (postgres): omitting a still-required field (``database``)
+ continues to be reported, even though ``port`` is blank too.
+ """
+ mocker.patch(
+ "superset.db_engine_specs.postgres.is_hostname_valid", return_value=True
+ )
+
+ properties = {"parameters": _basic_parameters(database="", port=None)}
+ errors = spec.validate_parameters(properties) # type: ignore[arg-type]
+
+ missing_errors = [
+ error
+ for error in errors
+ if error.error_type == SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR
+ ]
+ assert len(missing_errors) == 1
+ assert (missing_errors[0].extra or {})["missing"] == ["database"]
+
+
+def test_validate_parameters_explicit_valid_port_checks_open(
+ mocker: MockerFixture,
+) -> None:
+ """
+ DB Eng Specs (postgres): when a port IS supplied, format/range/open
+ validation is preserved unchanged.
+ """
+ mocker.patch(
+ "superset.db_engine_specs.postgres.is_hostname_valid", return_value=True
+ )
+ is_port_open = mocker.patch(
+ "superset.db_engine_specs.postgres.is_port_open", return_value=True
+ )
+
+ properties = {"parameters": _basic_parameters(port=5432)}
+ errors = spec.validate_parameters(properties) # type: ignore[arg-type]
+
+ assert errors == []
+ is_port_open.assert_called_once_with("localhost", 5432)
+
+
+def test_validate_parameters_invalid_port_still_errors(
+ mocker: MockerFixture,
+) -> None:
+ """
+ DB Eng Specs (postgres): an out-of-range port supplied by the user
+ still produces ``CONNECTION_INVALID_PORT_ERROR``, exactly as before.
+ """
+ mocker.patch(
+ "superset.db_engine_specs.postgres.is_hostname_valid", return_value=True
+ )
+
+ properties = {"parameters": _basic_parameters(port=70000)}
+ errors = spec.validate_parameters(properties) # type: ignore[arg-type]
+
+ assert len(errors) == 1
+ assert errors[0].error_type == SupersetErrorType.CONNECTION_INVALID_PORT_ERROR
+
+
+def test_validate_parameters_non_integer_port_matches_base_parity(
+ mocker: MockerFixture,
+) -> None:
+ """
+ DB Eng Specs (postgres): a non-integer port must produce BOTH errors
+ that ``BasicParametersMixin.validate_parameters`` produces -- the
+ "Port must be a valid integer." error from the failed ``int()``
+ conversion, AND the "must be an integer between 0 and 65535" range
+ error, since the base method does not return early after the former
+ and falls through to the range check (which is also False for a
+ non-int value). A Postgres-specific override that stops after the
+ first error would silently narrow this API's error-reporting contract.
+ """
+ mocker.patch(
+ "superset.db_engine_specs.postgres.is_hostname_valid", return_value=True
+ )
+
+ properties = {"parameters": _basic_parameters(port="not-a-port")}
+ errors = spec.validate_parameters(properties) # type: ignore[arg-type]
+
+ assert len(errors) == 2
+ assert errors[0].message == "Port must be a valid integer."
+ assert errors[0].error_type == SupersetErrorType.CONNECTION_INVALID_PORT_ERROR
+ assert (
+ errors[1].message
+ == "The port must be an integer between 0 and 65535 (inclusive)."
+ )
+ assert errors[1].error_type == SupersetErrorType.CONNECTION_INVALID_PORT_ERROR
+
+
+def test_parameters_schema_port_is_not_required() -> None:
+ """
+ DB Eng Specs (postgres): the JSON schema exposed to the frontend for the
+ Connect Database dynamic form must not mark ``port`` as required, so the
+ modal doesn't block client-side submission when the field is left blank.
+ """
+ json_schema = spec.parameters_json_schema()
+
+ assert "port" not in json_schema.get("required", [])
+ assert "host" in json_schema.get("required", [])
+ assert "database" in json_schema.get("required", [])