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", [])