From f2610e9dca01cb35e831bb89c3fcd5ef17111482 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Fri, 21 Aug 2026 15:59:33 -0700 Subject: [PATCH] fix(sqllab): default PostgreSQL port to 5432 in the dynamic connection form (#43351) Co-authored-by: Claude Sonnet 5 --- .../databases/DatabaseModal/index.test.tsx | 3 +- superset/db_engine_specs/base.py | 9 +- superset/db_engine_specs/postgres.py | 65 ++++ .../integration_tests/databases/api_tests.py | 6 +- .../databases/commands_tests.py | 8 +- .../db_engine_specs/postgres_tests.py | 5 +- .../db_engine_specs/test_postgres.py | 296 ++++++++++++++++++ 7 files changed, 385 insertions(+), 7 deletions(-) diff --git a/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx b/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx index a1b9f00373e..24d4bf69cd2 100644 --- a/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx +++ b/superset-frontend/src/features/databases/DatabaseModal/index.test.tsx @@ -136,6 +136,7 @@ describe('DatabaseModal', () => { format: 'int32', maximum: 65536, minimum: 0, + nullable: true, type: 'integer', }, query: { @@ -153,7 +154,7 @@ describe('DatabaseModal', () => { type: 'string', }, }, - required: ['database', 'host', 'port', 'username'], + required: ['database', 'host', 'username'], type: 'object', }, preferred: true, diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index 91b411ce219..eadfcd641ea 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -3037,6 +3037,11 @@ class BasicParametersMixin: # for Databend this would be `{"sslmode": "disable"}`, eg. encryption_disable_parameters: dict[str, str] = {} + # parameters that `validate_parameters` treats as mandatory; subclasses + # override this to relax a parameter (e.g. `port`) without duplicating + # the rest of `validate_parameters` + required_parameters: set[str] = {"host", "port", "username", "database"} + @classmethod def build_sqlalchemy_uri( # pylint: disable=unused-argument cls, @@ -3108,7 +3113,7 @@ class BasicParametersMixin: """ errors: list[SupersetError] = [] - required = {"host", "port", "username", "database"} + required = cls.required_parameters parameters = properties.get("parameters", {}) present = {key for key in parameters if parameters.get(key, ())} @@ -3137,7 +3142,7 @@ class BasicParametersMixin: return errors port = parameters.get("port", None) - if not port: + if port is None or port == "": return errors try: port = int(port) diff --git a/superset/db_engine_specs/postgres.py b/superset/db_engine_specs/postgres.py index 218661f3265..a5db64b9aaf 100644 --- a/superset/db_engine_specs/postgres.py +++ b/superset/db_engine_specs/postgres.py @@ -25,6 +25,8 @@ from typing import Any, Callable, Optional, TYPE_CHECKING import sqlalchemy as sa 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 @@ -39,6 +41,8 @@ from superset.db_engine_specs.base import ( AURORA_DATA_API_KNOWN_INCOMPATIBILITIES, BaseEngineSpec, BasicParametersMixin, + BasicParametersSchema, + BasicParametersType, DatabaseCategory, TimestampExpression, ) @@ -319,6 +323,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" @@ -330,6 +362,11 @@ class PostgresEngineSpec(BasicParametersMixin, PostgresBaseEngineSpec): supports_grouping_sets = True default_driver = "psycopg2" + parameters_schema = PostgresParametersSchema() + # ``port`` is intentionally not required: a blank port falls back to + # Postgres's own default (``metadata["default_port"]``) in + # ``BasicParametersMixin.build_sqlalchemy_uri`` (overridden below). + required_parameters = {"host", "username", "database"} sqlalchemy_uri_placeholder = ( "postgresql://user:password@host:port/dbname[?key=value&key=value...]" ) @@ -695,6 +732,34 @@ 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 + ) + @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..c794cf74499 100644 --- a/tests/integration_tests/databases/api_tests.py +++ b/tests/integration_tests/databases/api_tests.py @@ -3516,6 +3516,7 @@ class TestDatabaseApi(SupersetTestCase): "description": "Database port", "maximum": 65536, "minimum": 0, + "nullable": True, "type": "integer", }, "query": { @@ -3533,7 +3534,10 @@ class TestDatabaseApi(SupersetTestCase): "type": "string", }, }, - "required": ["database", "host", "port", "username"], + # ``port`` is intentionally not required: a blank port falls + # back to the default (5432) in + # ``PostgresEngineSpec.build_sqlalchemy_uri``. + "required": ["database", "host", "username"], "type": "object", }, "preferred": True, diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py index 734dd55a3f4..2d8b20584d9 100644 --- a/tests/integration_tests/databases/commands_tests.py +++ b/tests/integration_tests/databases/commands_tests.py @@ -1138,6 +1138,10 @@ def test_validate_partial(is_port_open, is_hostname_valid, app_context): def test_validate_partial_invalid_hostname(is_hostname_valid, app_context): """ Test parameter validation when only some parameters are present. + + ``port`` is explicitly ``None`` in the payload -- not required for + Postgres, since a blank/null port falls back to the default 5432 -- and + is correctly absent from the expected "missing" list below. """ 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..d5edc4f9e89 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,9 @@ 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"], } diff --git a/tests/unit_tests/db_engine_specs/test_postgres.py b/tests/unit_tests/db_engine_specs/test_postgres.py index 2cb5126c519..7e3e50c867d 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 @@ -503,6 +504,301 @@ def test_get_schema_names_excludes_only_actual_system_schemas( } +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.base.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.base.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.base.is_hostname_valid", return_value=True) + is_port_open = mocker.patch( + "superset.db_engine_specs.base.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.base.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_explicit_zero_port_is_validated( + mocker: MockerFixture, +) -> None: + """ + DB Eng Specs (postgres): an explicit ``port=0`` must not be silently + treated as blank. ``0`` is falsy in Python, so a naive ``if not port`` + short-circuit (the base method's original bug, inherited by Postgres) + would skip the int/range/``is_port_open`` checks entirely for a real, + explicitly-supplied port value of ``0`` -- which the schema's own + ``Range(min=0, ...)`` validator accepts as valid. This confirms + ``is_port_open`` is actually called (i.e. validation ran) for ``port=0``. + """ + mocker.patch("superset.db_engine_specs.base.is_hostname_valid", return_value=True) + is_port_open = mocker.patch( + "superset.db_engine_specs.base.is_port_open", return_value=True + ) + + properties = {"parameters": _basic_parameters(port=0)} + errors = spec.validate_parameters(properties) # type: ignore[arg-type] + + is_port_open.assert_called_once_with("localhost", 0) + assert errors == [] + + +def test_validate_parameters_explicit_zero_port_reports_closed( + mocker: MockerFixture, +) -> None: + """ + DB Eng Specs (postgres): the other side of the ``port=0`` fix above -- + when the (now-actually-run) open-port check for an explicit ``port=0`` + fails, ``CONNECTION_PORT_CLOSED_ERROR`` is reported like it would be for + any other supplied port. + """ + mocker.patch("superset.db_engine_specs.base.is_hostname_valid", return_value=True) + is_port_open = mocker.patch( + "superset.db_engine_specs.base.is_port_open", return_value=False + ) + + properties = {"parameters": _basic_parameters(port=0)} + errors = spec.validate_parameters(properties) # type: ignore[arg-type] + + is_port_open.assert_called_once_with("localhost", 0) + assert len(errors) == 1 + assert errors[0].error_type == SupersetErrorType.CONNECTION_PORT_CLOSED_ERROR + + +@pytest.mark.parametrize("blank_port", [None, ""]) +def test_validate_parameters_blank_port_never_calls_is_port_open( + blank_port: Optional[str], + mocker: MockerFixture, +) -> None: + """ + DB Eng Specs (postgres): regression lock for the blank-port UX this + whole ticket exists to fix -- ``None`` (an omitted/null port) and ``""`` + (what a cleared HTML number input submits) must both keep + short-circuiting ``validate_parameters`` with zero errors *before* any + port validation runs, and must not be conflated with the ``port=0`` fix + above: ``is_port_open`` must never be called for either blank form. + """ + mocker.patch("superset.db_engine_specs.base.is_hostname_valid", return_value=True) + is_port_open = mocker.patch("superset.db_engine_specs.base.is_port_open") + + properties = {"parameters": _basic_parameters(port=blank_port)} + errors = spec.validate_parameters(properties) # type: ignore[arg-type] + + assert errors == [] + is_port_open.assert_not_called() + + +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). ``PostgresEngineSpec`` inherits ``validate_parameters`` + directly from the base (it only overrides ``required_parameters``), so + this guards that the inherited behavior keeps producing both errors. + """ + mocker.patch("superset.db_engine_specs.base.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", []) + + @pytest.mark.parametrize( ("aggregate", "expected_sql"), [