fix(sqllab): default PostgreSQL port to 5432 in the dynamic connection form

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,

superset/db_engine_specs/postgres.py

@@ -24,6 +24,8 @@
 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 @@
     AURORA_DATA_API_KNOWN_INCOMPATIBILITIES,
     BaseEngineSpec,
     BasicParametersMixin,
+    BasicParametersSchema,
+    BasicParametersType,
+    BasicPropertiesType,
     DatabaseCategory,
     TimestampExpression,
 )
@@ -46,6 +51,7 @@
 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 @@ def convert_dttm(
         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 @@ def adjust_engine_params(
 
         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:
         """

tests/integration_tests/databases/api_tests.py

@@ -3516,6 +3516,7 @@ def test_available(self, get_available_engine_specs):
                                 "description": "Database port",
                                 "maximum": 65536,
                                 "minimum": 0,
+                                "nullable": True,
                                 "type": "integer",
                             },
                             "query": {
@@ -3533,7 +3534,10 @@ def test_available(self, get_available_engine_specs):
                                 "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,
@@ -3968,8 +3972,8 @@ def test_validate_parameters_missing_fields(self):
             ]
         }
 
-    @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 +4063,7 @@ def test_validate_parameters_invalid_port(self):
             ]
         }
 
-    @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 +4123,7 @@ def test_validate_parameters_invalid_host(self, is_hostname_valid):
             ]
         }
 
-    @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
 

tests/integration_tests/databases/commands_tests.py

@@ -1063,8 +1063,8 @@ def test_connection_db_api_exc(
         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,

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"],
     }