fix(dashboard): stretch Chart Data modal results grid to fill available height

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/ThemedAgGridReact.test.tsx

@@ -246,34 +246,6 @@ test('wraps component with proper container div', () => {
   expect(wrapper).toHaveAttribute('data-themed-ag-grid', 'true');
 });
 
-test('applies non-transparent backgrounds to native menus, tooltips and overlays', () => {
-  const customTheme = {
-    ...supersetTheme,
-    colorBgElevated: '#f2f2f2',
-  };
-
-  render(
-    <ThemeProvider theme={customTheme}>
-      <ThemedAgGridReact rowData={mockRowData} columnDefs={mockColumnDefs} />
-    </ThemeProvider>,
-  );
-
-  const agGrid = screen.getByTestId('ag-grid-react');
-  const theme = JSON.parse(agGrid.getAttribute('data-theme') || '{}');
-
-  // ag-grid's own context/column menus, side bar, tooltips and overlays are
-  // rendered against these params rather than `backgroundColor` (which is
-  // intentionally 'transparent' so the surrounding app shows through the
-  // grid body). Without explicit values they inherit transparency too,
-  // making native menus/popups unreadable.
-  expect(theme.chromeBackgroundColor).toBe('#f2f2f2');
-  expect(theme.menuBackgroundColor).toBe('#f2f2f2');
-  expect(theme.menuBorder).toBe(true);
-  expect(theme.sideBarBackgroundColor).toBe('#f2f2f2');
-  expect(theme.tooltipBackgroundColor).toBe('#f2f2f2');
-  expect(theme.modalOverlayBackgroundColor).toBe('#f2f2f2');
-});
-
 test('handles missing theme gracefully', () => {
   const incompleteTheme = {
     ...supersetTheme,

superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx

@@ -104,17 +104,6 @@ export const ThemedAgGridReact = forwardRef<
       foregroundColor: theme.colorText,
       browserColorScheme: isDarkMode ? 'dark' : 'light',
 
-      // Native menus, popups, side bar, tooltips and loading/no-rows overlays
-      // are rendered against these params rather than `backgroundColor`
-      // (which is intentionally transparent). Without explicit values they
-      // inherit transparency too, making them unreadable.
-      chromeBackgroundColor: theme.colorBgElevated,
-      menuBackgroundColor: theme.colorBgElevated,
-      menuBorder: true,
-      sideBarBackgroundColor: theme.colorBgElevated,
-      tooltipBackgroundColor: theme.colorBgElevated,
-      modalOverlayBackgroundColor: theme.colorBgElevated,
-
       // Header styling
       headerBackgroundColor: theme.colorFillTertiary,
       headerTextColor: theme.colorTextHeading,

superset-frontend/src/dashboard/util/permissionUtils.test.ts

@@ -259,6 +259,21 @@ describe('isUserEditorOrAdmin', () => {
   test('returns false when editors is omitted', () => {
     expect(isUserEditorOrAdmin(outsiderUser)).toEqual(false);
   });
+
+  test('returns true when the user is granted editorship only through extra_editors', () => {
+    expect(isUserEditorOrAdmin(editorUser, [], [10])).toEqual(true);
+  });
+
+  test('unions editors and extra_editors rather than preferring one', () => {
+    const nonMatchingSubject: Subject = { id: 999, label: 'Other', type: 1 };
+    expect(isUserEditorOrAdmin(editorUser, [nonMatchingSubject], [10])).toEqual(
+      true,
+    );
+  });
+
+  test('returns false when extra_editors names other subjects', () => {
+    expect(isUserEditorOrAdmin(editorUser, [], [999])).toEqual(false);
+  });
 });
 
 // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks

superset-frontend/src/dashboard/util/permissionUtils.ts

@@ -55,9 +55,6 @@ export const isUserInSubjects = (
   );
 };
 
-const isUserInEditors = (editors: Subject[] = []): boolean =>
-  isUserInSubjects(editors);
-
 export const isUserAdmin = (
   user?: UserWithPermissionsAndRoles | UndefinedUser,
 ) =>
@@ -66,10 +63,12 @@ export const isUserAdmin = (
     role => role.toLowerCase() === ADMIN_ROLE_NAME.toLowerCase(),
   );
 
+/** `extraEditors` is editorship granted via a deployment's EXTRA_EDITORS_RESOLVER. */
 export const isUserEditorOrAdmin = (
   user?: UserWithPermissionsAndRoles | UndefinedUser,
   editors: Subject[] = [],
-): boolean => isUserInEditors(editors) || isUserAdmin(user);
+  extraEditors?: SubjectRef[] | null,
+): boolean => isUserInSubjects(editors, extraEditors) || isUserAdmin(user);
 
 /**
  * Editorship of *dashboard*, matching the server's `is_editor`: the explicit

superset-frontend/src/explore/components/DataTablesPane/components/ResultsPaneOnDashboard.tsx

@@ -88,7 +88,12 @@ export const ResultsPaneOnDashboard = ({
 
   return (
     <Wrapper>
-      <Tabs activeKey={activeTabKey} onChange={setActiveTabKey} items={items} />
+      <Tabs
+        fullHeight
+        activeKey={activeTabKey}
+        onChange={setActiveTabKey}
+        items={items}
+      />
     </Wrapper>
   );
 };

superset-frontend/src/features/charts/ChartCard.tsx

@@ -96,7 +96,11 @@ export default function ChartCard({
   const canEdit = hasPerm('can_write');
   const canDelete = hasPerm('can_write');
   const canExport = hasPerm('can_export');
-  const allowEdit = isUserEditorOrAdmin(user, chart.editors);
+  const allowEdit = isUserEditorOrAdmin(
+    user,
+    chart.editors,
+    chart.extra_editors,
+  );
   const menuItems: MenuItem[] = [];
 
   if (canEdit) {

superset-frontend/src/features/dashboards/DashboardCard.tsx

@@ -83,7 +83,11 @@ function DashboardCard({
   const canEdit = hasPerm('can_write');
   const canDelete = hasPerm('can_write');
   const canExport = hasPerm('can_export');
-  const allowEdit = isUserEditorOrAdmin(user, dashboard.editors);
+  const allowEdit = isUserEditorOrAdmin(
+    user,
+    dashboard.editors,
+    dashboard.extra_editors,
+  );
   const digest = dashboard.changed_on_utc || dashboard.changed_on;
   const thumbnailUrl =
     isFeatureEnabled(FeatureFlag.Thumbnails) && dashboard.id && digest

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-frontend/src/pages/ChartList/index.tsx

@@ -650,7 +650,11 @@ function ChartList(props: ChartListProps) {
       },
       {
         Cell: ({ row: { original } }: CellProps<Chart>) => {
-          const allowEdit = isUserEditorOrAdmin(user, original.editors);
+          const allowEdit = isUserEditorOrAdmin(
+            user,
+            original.editors,
+            original.extra_editors,
+          );
           const openEditModal = () => openChartEditModal(original);
           const handleExport = () => handleBulkChartExport([original]);
           if (!canEdit && !canDelete && !canExport) {

superset-frontend/src/pages/DashboardList/index.tsx

@@ -122,6 +122,8 @@ export interface Dashboard {
   description?: string;
   thumbnail_url?: string | null;
   editors?: Subject[];
+  // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
+  extra_editors?: number[];
   viewers?: Subject[];
   tags: TagType[];
   created_by: object;
@@ -505,7 +507,11 @@ function DashboardList(props: DashboardListProps) {
       },
       {
         Cell: ({ row: { original } }: CellProps<Dashboard>) => {
-          const allowEdit = isUserEditorOrAdmin(user, original.editors);
+          const allowEdit = isUserEditorOrAdmin(
+            user,
+            original.editors,
+            original.extra_editors,
+          );
           const handleDelete = () =>
             handleDashboardDelete(
               original,

superset-frontend/src/types/Chart.ts

@@ -45,6 +45,8 @@ export interface Chart {
   cache_timeout: number | null;
   thumbnail_url?: string;
   editors?: Subject[];
+  // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
+  extra_editors?: number[];
   viewers?: Subject[];
   tags?: TagType[];
   last_saved_at?: string;

superset-frontend/src/views/CRUD/types.ts

@@ -67,6 +67,8 @@ export interface Dashboard {
   url: string;
   thumbnail_url?: string | null;
   editors?: Subject[];
+  // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
+  extra_editors?: number[];
   viewers?: Subject[];
   loading?: boolean;
 }

superset/charts/api.py

@@ -92,7 +92,10 @@ from superset.exceptions import (
 )
 from superset.extensions import event_logger, security_manager
 from superset.models.slice import Slice
-from superset.security.manager import get_extra_editor_subject_ids
+from superset.security.manager import (
+    get_extra_editor_subject_ids,
+    get_extra_editors_by_pk,
+)
 from superset.subjects.filters import (
     FilterRelatedSubjects,
     subject_type_filter,
@@ -410,6 +413,15 @@ class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi):
         except ChartNotFoundError:
             return self.response_404()
 
+    def pre_get_list(self, data: dict[str, Any]) -> None:
+        """Attach ``extra_editors`` to each row, matching the single-object GET."""
+        super().pre_get_list(data)
+        ids = data.get("ids", [])
+        extra_editors_by_id = get_extra_editors_by_pk(Slice, ids)
+        for row, row_id in zip(data.get("result", []), ids, strict=False):
+            if row_id in extra_editors_by_id:
+                row["extra_editors"] = extra_editors_by_id[row_id]
+
     @expose("/<pk>/deck_layers/", methods=("GET",))
     @protect()
     @safe

superset/common/form_data_query_context.py

@@ -213,7 +213,9 @@ def orderby_from_form_data(
     # The drag-and-drop "sort by" control persists a list; the frontend unwraps it
     # with ``ensureIsArray(...)[0]`` (``plugin-chart-table/src/buildQuery.ts:67``).
     # Read raw, a list would nest inside ``orderby`` and fail the query.
-    raw_sort_metric = form_data.get("timeseries_limit_metric")
+    raw_sort_metric = form_data.get("series_limit_metric") or form_data.get(
+        "timeseries_limit_metric"
+    )
     sort_metric = (
         next(iter(as_list(raw_sort_metric)), None) if raw_sort_metric else None
     ) or (metrics[0] if form_data.get("sort_by_metric") else None)

superset/dashboards/api.py

@@ -142,7 +142,10 @@ from superset.extensions import event_logger, security_manager
 from superset.models.dashboard import Dashboard
 from superset.models.embedded_dashboard import EmbeddedDashboard
 from superset.security.guest_token import GuestUser
-from superset.security.manager import get_extra_editor_subject_ids
+from superset.security.manager import (
+    get_extra_editor_subject_ids,
+    get_extra_editors_by_pk,
+)
 from superset.subjects.filters import (
     FilterRelatedSubjects,
     subject_type_filter,
@@ -433,6 +436,15 @@ class DashboardRestApi(
         """
         return super().get_list(**kwargs)
 
+    def pre_get_list(self, data: dict[str, Any]) -> None:
+        """Attach ``extra_editors`` to each row, matching the single-object GET."""
+        super().pre_get_list(data)
+        ids = data.get("ids", [])
+        extra_editors_by_id = get_extra_editors_by_pk(Dashboard, ids)
+        for row, row_id in zip(data.get("result", []), ids, strict=False):
+            if row_id in extra_editors_by_id:
+                row["extra_editors"] = extra_editors_by_id[row_id]
+
     list_select_columns = list_columns + ["changed_on", "created_on", "changed_by_fk"]
     order_columns = [
         "changed_by.first_name",

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)

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:
         """

superset/examples/birth_names.py

@@ -290,7 +290,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 groupby=["name"],
                 adhoc_filters=[gen_filter("gender", "girl")],
                 row_limit=50,
-                timeseries_limit_metric=metric,
+                series_limit_metric=metric,
                 metrics=[metric],
             ),
             editors=[],
@@ -321,7 +321,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 groupby=["name"],
                 adhoc_filters=[gen_filter("gender", "boy")],
                 row_limit=50,
-                timeseries_limit_metric=metric,
+                series_limit_metric=metric,
                 metrics=[metric],
             ),
             editors=[],
@@ -498,7 +498,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 viz_type="echarts_timeseries_line",
                 granularity_sqla="ds",
                 groupby=["name"],
-                timeseries_limit_metric={
+                series_limit_metric={
                     "expressionType": "SIMPLE",
                     "column": {
                         "column_name": "num_california",
@@ -522,7 +522,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 metrics=metrics,
                 groupby=["name"],
                 row_limit=50,
-                timeseries_limit_metric={
+                series_limit_metric={
                     "expressionType": "SIMPLE",
                     "column": {
                         "column_name": "num_california",

superset/examples/usa_births_names/charts/Boys.yaml

@@ -36,8 +36,8 @@ params:
   metrics:
   - sum__num
   row_limit: 50
+  series_limit_metric: sum__num
   time_range: '100 years ago : now'
-  timeseries_limit_metric: sum__num
   viz_type: table
 query_context: null
 slice_name: Boys

superset/examples/usa_births_names/charts/Girls.yaml

@@ -36,8 +36,8 @@ params:
   metrics:
   - sum__num
   row_limit: 50
+  series_limit_metric: sum__num
   time_range: '100 years ago : now'
-  timeseries_limit_metric: sum__num
   viz_type: table
 query_context: null
 slice_name: Girls

superset/security/manager.py

@@ -170,6 +170,36 @@ def get_extra_editor_subject_ids(resource: Model) -> list[int]:
     return subject_ids
 
 
+def get_extra_editors_by_pk(
+    model_cls: type[Model], primary_keys: list[Any]
+) -> dict[Any, list[int]]:
+    """
+    Resolve extra editor subject IDs for a batch of resources, keyed by
+    primary key. List responses only have serialized rows, not model
+    instances, so this re-queries the page's rows in one batched query.
+    """
+    if not primary_keys or not (
+        has_app_context() and current_app.config.get("EXTRA_EDITORS_RESOLVER")
+    ):
+        return {}
+
+    # pylint: disable=import-outside-toplevel
+    from superset import db
+    from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES
+
+    pk_col = inspect(model_cls).primary_key[0]
+    resources = (
+        db.session.query(model_cls)
+        .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {model_cls}})
+        .filter(pk_col.in_(primary_keys))
+        .all()
+    )
+    return {
+        getattr(resource, pk_col.name): get_extra_editor_subject_ids(resource)
+        for resource in resources
+    }
+
+
 def _render_permission_instructions_link(
     *,
     datasource_id: str = "",

tests/integration_tests/dashboards/api_tests.py

@@ -922,6 +922,55 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
             db.session.delete(dashboard)
             db.session.commit()
 
+    def test_get_dashboards_list_omits_extra_editors_by_default(self):
+        """No EXTRA_EDITORS_RESOLVER configured: list rows omit extra_editors."""
+        admin = self.get_user("admin")
+        dashboard = self.insert_dashboard(
+            "no_extra_editors_list_dashboard",
+            "no-extra-editors-list-dashboard",
+            [admin.id],
+        )
+        try:
+            self.login(ADMIN_USERNAME)
+            rv = self.client.get("api/v1/dashboard/")
+            assert rv.status_code == 200
+            data = json.loads(rv.data.decode("utf-8"))
+            row = next(
+                d
+                for d in data["result"]
+                if d["dashboard_title"] == dashboard.dashboard_title
+            )
+            assert "extra_editors" not in row
+        finally:
+            db.session.delete(dashboard)
+            db.session.commit()
+
+    @with_config({"EXTRA_EDITORS_RESOLVER": lambda resource: [123]})
+    def test_get_dashboards_list_includes_extra_editors_when_resolver_configured(
+        self,
+    ):
+        """List rows get extra_editors too, mirroring the single-object GET."""
+        admin = self.get_user("admin")
+        dashboard = self.insert_dashboard(
+            "extra_editors_list_dashboard",
+            "extra-editors-list-dashboard",
+            [admin.id],
+        )
+        try:
+            self.login(ADMIN_USERNAME)
+            rv = self.client.get("api/v1/dashboard/")
+            assert rv.status_code == 200
+            data = json.loads(rv.data.decode("utf-8"))
+            row = next(
+                d
+                for d in data["result"]
+                if d["dashboard_title"] == dashboard.dashboard_title
+            )
+            assert row["extra_editors"] == [123]
+        finally:
+            db.session.delete(dashboard)
+            db.session.commit()
+
     def test_get_charts_admin_sees_existing_charts(self):
         """Regression for #25890: GET /api/v1/chart/ as an Admin user should
         return existing charts, not an empty list."""
@@ -944,6 +993,41 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
             db.session.delete(chart)
             db.session.commit()
 
+    def test_get_charts_list_omits_extra_editors_by_default(self):
+        """No EXTRA_EDITORS_RESOLVER configured: list rows omit extra_editors."""
+        admin = self.get_user("admin")
+        chart = self.insert_chart(
+            "no_extra_editors_list_chart", [admin.id], 1, params="{}"
+        )
+        try:
+            self.login(ADMIN_USERNAME)
+            rv = self.client.get("api/v1/chart/")
+            assert rv.status_code == 200
+            data = json.loads(rv.data.decode("utf-8"))
+            row = next(c for c in data["result"] if c["slice_name"] == chart.slice_name)
+            assert "extra_editors" not in row
+        finally:
+            db.session.delete(chart)
+            db.session.commit()
+
+    @with_config({"EXTRA_EDITORS_RESOLVER": lambda resource: [123]})
+    def test_get_charts_list_includes_extra_editors_when_resolver_configured(self):
+        """List rows get extra_editors too, mirroring the single-object GET."""
+        admin = self.get_user("admin")
+        chart = self.insert_chart(
+            "extra_editors_list_chart", [admin.id], 1, params="{}"
+        )
+        try:
+            self.login(ADMIN_USERNAME)
+            rv = self.client.get("api/v1/chart/")
+            assert rv.status_code == 200
+            data = json.loads(rv.data.decode("utf-8"))
+            row = next(c for c in data["result"] if c["slice_name"] == chart.slice_name)
+            assert row["extra_editors"] == [123]
+        finally:
+            db.session.delete(chart)
+            db.session.commit()
+
     def test_get_dashboards_filter(self):
         """
         Dashboard API: Test get dashboards filter

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,

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,

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

tests/unit_tests/common/test_form_data_query_context.py

@@ -218,6 +218,31 @@ def test_orderby_uses_timeseries_limit_metric_and_order_desc() -> None:
     assert query["orderby"] == [["revenue", True]]
 
 
+def test_orderby_uses_series_limit_metric_and_order_desc() -> None:
+    # series_limit_metric is the current field name; timeseries_limit_metric is
+    # the deprecated alias kept above for back-compat with old saved charts.
+    form_data = {
+        "metrics": ["count"],
+        "groupby": ["c"],
+        "series_limit_metric": "revenue",
+        "order_desc": False,
+    }
+    query = build_query_context_from_form_data(form_data, DATASOURCE)["queries"][0]
+    assert query["orderby"] == [["revenue", True]]
+
+
+def test_orderby_prefers_series_limit_metric_over_deprecated_alias() -> None:
+    form_data = {
+        "metrics": ["count"],
+        "groupby": ["c"],
+        "series_limit_metric": "revenue",
+        "timeseries_limit_metric": "profit",
+        "order_desc": False,
+    }
+    query = build_query_context_from_form_data(form_data, DATASOURCE)["queries"][0]
+    assert query["orderby"] == [["revenue", True]]
+
+
 def test_orderby_pie_sort_by_metric() -> None:
     form_data = {"metric": "count", "groupby": ["c"], "sort_by_metric": True}
     query = build_query_context_from_form_data(form_data, DATASOURCE, viz_type="pie")[

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