fix(plugin-chart-ag-grid-table): keep basic conditional formatting aligned after sort (#105973)

The basic (increase/decrease) color formatters were built in the original
query order and read positionally by the displayed AG Grid rowIndex. Once the
table was sorted client-side, the displayed index no longer matched the data
order, so the green/red background and arrow indicators were applied to the
wrong rows.

Attach each row's formatter to its row data object (non-enumerable, so it never
leaks into exports/cross-filters) so it travels with the row through sorting,
and resolve it via getRowBasicColorFormatter in both getCellStyle and
NumericCellRenderer, falling back to the positional lookup. Add unit tests.

UPDATING.md

@@ -149,18 +149,6 @@ Runbook to adopt:
 2. Set that value on the tunnel's `server_host_key` (via the database/SSH tunnel API or UI payload).
 3. Optionally set `SSH_TUNNEL_STRICT_HOST_KEY_CHECKING = True` in `superset_config.py` to require host-key verification on all tunnels.
 
-### SMTP server certificate validation enabled by default
-
-`SMTP_SSL_SERVER_AUTH` now defaults to `True` (previously `False`). With this default, STARTTLS/SSL connections to the configured SMTP server validate the server's TLS certificate against the system trusted CA store. This makes outbound email (alerts and reports) verify the mail server's identity out of the box.
-
-If your SMTP server presents a self-signed certificate, or a certificate that is not trusted by the system CA store, email delivery may now fail with a certificate verification error. To restore the previous behavior of skipping certificate validation, set the following in `superset_config.py`:
-
-```python
-SMTP_SSL_SERVER_AUTH = False
-```
-
-The recommended fix is to add the SMTP server's certificate (or its issuing CA) to the system trust store rather than disabling validation.
-
 ### Dataset import validates catalog against the target connection
 
 Importing a dataset now validates the `catalog` field against the target database connection. When the connection has multi-catalog disabled (`allow_multi_catalog` off) and the dataset's catalog is not the connection's default catalog, the import fails instead of silently persisting the non-default catalog. This matches the validation already enforced on the dataset update path and prevents imported datasets from querying an unintended database.

superset-frontend/plugins/plugin-chart-ag-grid-table/src/consts.ts

@@ -44,3 +44,8 @@ export const FILTER_CONDITION_BODY_INDEX = {
 } as const;
 
 export const ROW_NUMBER_COL_ID = '__row_number__';
+
+// Non-enumerable key used to attach a row's basic (increase/decrease) color
+// formatter to the row data object so it travels with the row through AG Grid
+// client-side sorting (#105973).
+export const BASIC_COLOR_FORMATTERS_ROW_KEY = '__basicColorFormatters__';

superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx

@@ -24,6 +24,7 @@ import {
 import { CustomCellRendererProps } from '@superset-ui/core/components/ThemedAgGridReact';
 import { BasicColorFormatterType, InputColumn, ValueRange } from '../types';
 import { useIsDark } from '../utils/useTableTheme';
+import getRowBasicColorFormatter from '../utils/getRowBasicColorFormatter';
 
 const StyledTotalCell = styled.div`
   ${() => `
@@ -163,13 +164,13 @@ export const NumericCellRenderer = (
   let arrow = '';
   let arrowColor = '';
   if (hasBasicColorFormatters && col?.metricName) {
-    arrow =
-      basicColorFormatters?.[node?.rowIndex as number]?.[col.metricName]
-        ?.mainArrow;
-    arrowColor =
-      basicColorFormatters?.[node?.rowIndex as number]?.[
-        col.metricName
-      ]?.arrowColor?.toLowerCase();
+    const rowFormatter = getRowBasicColorFormatter(
+      node,
+      node?.rowIndex,
+      basicColorFormatters,
+    )?.[col.metricName];
+    arrow = rowFormatter?.mainArrow;
+    arrowColor = rowFormatter?.arrowColor?.toLowerCase();
   }
 
   const alignment =

superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts

@@ -46,6 +46,7 @@ import {
 } from '@superset-ui/chart-controls';
 import isEqualColumns from './utils/isEqualColumns';
 import DateWithFormatter from './utils/DateWithFormatter';
+import { BASIC_COLOR_FORMATTERS_ROW_KEY } from './consts';
 import {
   DataColumnMeta,
   TableChartProps,
@@ -703,6 +704,23 @@ const transformProps = (
 
   const basicColorFormatters =
     comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns);
+
+  // Attach each row's basic (increase/decrease) color formatter to the row data
+  // object so it travels with the row through AG Grid client-side sorting.
+  // basicColorFormatters is built in the original query order and was previously
+  // read positionally by the displayed rowIndex, which applied colors to the
+  // wrong rows once the table was sorted (#105973). The property is
+  // non-enumerable so it never leaks into exports, cross-filters or spreads.
+  if (basicColorFormatters) {
+    passedData.forEach((row, index) => {
+      Object.defineProperty(row, BASIC_COLOR_FORMATTERS_ROW_KEY, {
+        value: basicColorFormatters[index],
+        enumerable: false,
+        configurable: true,
+        writable: true,
+      });
+    });
+  }
   const columnColorFormatters =
     getColorFormatters(conditionalFormatting, passedData, theme) ?? [];
 

superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts

@@ -24,6 +24,7 @@ import {
 } from '@superset-ui/chart-controls';
 import { CellClassParams } from '@superset-ui/core/components/ThemedAgGridReact';
 import { BasicColorFormatterType, InputColumn } from '../types';
+import getRowBasicColorFormatter from './getRowBasicColorFormatter';
 
 type CellStyleParams = CellClassParams & {
   hasColumnColorFormatters: boolean | undefined;
@@ -84,8 +85,11 @@ const getCellStyle = (params: CellStyleParams) => {
     col?.metricName &&
     node?.rowPinned !== 'bottom'
   ) {
-    backgroundColor =
-      basicColorFormatters?.[rowIndex]?.[col.metricName]?.backgroundColor;
+    backgroundColor = getRowBasicColorFormatter(
+      node,
+      rowIndex,
+      basicColorFormatters,
+    )?.[col.metricName]?.backgroundColor;
   }
 
   const textAlign =

superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getRowBasicColorFormatter.ts

@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { BASIC_COLOR_FORMATTERS_ROW_KEY } from '../consts';
+import { BasicColorFormatterType } from '../types';
+
+type RowFormatters = { [key: string]: BasicColorFormatterType };
+
+/**
+ * Resolves the basic (increase/decrease) color formatters for a given AG Grid
+ * row node.
+ *
+ * The formatter is attached to the row data object itself (see transformProps),
+ * so it follows the row through client-side sorting. Looking it up positionally
+ * by the displayed `rowIndex` was wrong once the user sorted the table, because
+ * the displayed index no longer matched the original data order (#105973).
+ *
+ * Falls back to the positional array for safety when no attached formatter is
+ * present.
+ */
+export default function getRowBasicColorFormatter(
+  node: { data?: Record<string, unknown> } | undefined,
+  rowIndex: number | null | undefined,
+  basicColorFormatters: RowFormatters[] | undefined,
+): RowFormatters | undefined {
+  const attached = node?.data?.[BASIC_COLOR_FORMATTERS_ROW_KEY] as
+    | RowFormatters
+    | undefined;
+  if (attached) {
+    return attached;
+  }
+  if (rowIndex == null) {
+    return undefined;
+  }
+  return basicColorFormatters?.[rowIndex];
+}

superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getRowBasicColorFormatter.test.ts

@@ -0,0 +1,65 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import getRowBasicColorFormatter from '../../src/utils/getRowBasicColorFormatter';
+import { BASIC_COLOR_FORMATTERS_ROW_KEY } from '../../src/consts';
+
+const red = { sales: { backgroundColor: 'red', mainArrow: '↓', arrowColor: 'red' } };
+const green = {
+  sales: { backgroundColor: 'green', mainArrow: '↑', arrowColor: 'green' },
+};
+
+// Positional array in the original (unsorted) query order: row 0 -> green, row 1 -> red.
+const positional = [green, red] as any;
+
+test('uses the formatter attached to the row, not the displayed rowIndex (#105973)', () => {
+  // After sorting, the row whose original formatter is `red` is displayed first
+  // (rowIndex 0). The positional lookup would wrongly return `green`.
+  const rowData: Record<string, unknown> = { sales: 5 };
+  Object.defineProperty(rowData, BASIC_COLOR_FORMATTERS_ROW_KEY, {
+    value: red,
+    enumerable: false,
+  });
+  const node = { data: rowData };
+
+  expect(getRowBasicColorFormatter(node, 0, positional)).toBe(red);
+  expect(
+    getRowBasicColorFormatter(node, 0, positional)?.sales.backgroundColor,
+  ).toBe('red');
+});
+
+test('falls back to positional lookup when no formatter is attached', () => {
+  const node = { data: { sales: 5 } };
+  expect(getRowBasicColorFormatter(node, 1, positional)).toBe(red);
+});
+
+test('returns undefined when nothing matches', () => {
+  expect(getRowBasicColorFormatter(undefined, null, positional)).toBeUndefined();
+  expect(
+    getRowBasicColorFormatter({ data: {} }, null, positional),
+  ).toBeUndefined();
+});
+
+test('attached formatter is non-enumerable so it does not leak into the row', () => {
+  const rowData: Record<string, unknown> = { sales: 5 };
+  Object.defineProperty(rowData, BASIC_COLOR_FORMATTERS_ROW_KEY, {
+    value: green,
+    enumerable: false,
+  });
+  expect(Object.keys(rowData)).toEqual(['sales']);
+});

superset-frontend/scripts/check-custom-rules.js

@@ -632,35 +632,6 @@ function processFile(filepath) {
   }
 }
 
-/**
- * Application source trees that must be authored in TypeScript. Matches the
- * top-level `src/` directory as well as each package/plugin `src/` directory.
- */
-const TS_ONLY_SOURCE_PATTERN =
-  /^(src|packages\/[^/]+\/src|plugins\/[^/]+\/src)\//;
-
-/**
- * Enforce the TypeScript-only frontend convention: no `.js`/`.jsx` files may be
- * added under the application source trees (including test files). Build
- * artifacts and root-level config files (e.g. `.storybook/preview.jsx`,
- * `webpack.config.js`) live outside these trees and are intentionally allowed.
- *
- * @param {string[]} candidateFiles paths relative to `superset-frontend/`
- */
-function checkTypeScriptOnlySource(candidateFiles) {
-  candidateFiles.forEach(file => {
-    if (TS_ONLY_SOURCE_PATTERN.test(file) && /\.(js|jsx)$/.test(file)) {
-      // eslint-disable-next-line no-console
-      console.error(
-        `${RED}✗${RESET} ${file}: frontend source must be TypeScript. ` +
-          `Rename to .ts/.tsx (the codebase is mid-migration to full ` +
-          `TypeScript; no new .js/.jsx files in src/).`,
-      );
-      errorCount += 1;
-    }
-  });
-}
-
 /**
  * Main function
  */
@@ -695,22 +666,6 @@ function main() {
     /packages\/superset-ui-core\/src\/color\/index\.ts/, // Core brand color constants
   ];
 
-  // Enforce TypeScript-only source. Run this on the raw file list (before the
-  // ignore patterns below strip out tests/stories) so that e.g. a new
-  // `*.test.jsx` is still rejected.
-  const tsOnlyCandidates =
-    args.length === 0
-      ? glob.sync('{src,packages/*/src,plugins/*/src}/**/*.{js,jsx}', {
-          ignore: [
-            '**/node_modules/**',
-            '**/esm/**',
-            '**/lib/**',
-            '**/dist/**',
-          ],
-        })
-      : args.map(f => f.replace(/^superset-frontend\//, ''));
-  checkTypeScriptOnlySource(tsOnlyCandidates);
-
   // If no files specified, check all
   if (files.length === 0) {
     files = glob.sync('src/**/*.{ts,tsx,js,jsx}', {
@@ -751,23 +706,22 @@ function main() {
   if (files.length === 0) {
     // eslint-disable-next-line no-console
     console.log('No files to check.');
-  } else {
-    // eslint-disable-next-line no-console
-    console.log(
-      `Checking ${files.length} files for Superset custom rules...\n`,
-    );
-
-    files.forEach(file => {
-      // Resolve the file path
-      const resolvedPath = path.resolve(file);
-      if (fs.existsSync(resolvedPath)) {
-        processFile(resolvedPath);
-      } else if (fs.existsSync(file)) {
-        processFile(file);
-      }
-    });
+    return;
   }
 
+  // eslint-disable-next-line no-console
+  console.log(`Checking ${files.length} files for Superset custom rules...\n`);
+
+  files.forEach(file => {
+    // Resolve the file path
+    const resolvedPath = path.resolve(file);
+    if (fs.existsSync(resolvedPath)) {
+      processFile(resolvedPath);
+    } else if (fs.existsSync(file)) {
+      processFile(file);
+    }
+  });
+
   // eslint-disable-next-line no-console
   console.log(`\n${errorCount} errors, ${warningCount} warnings`);
 
@@ -786,5 +740,4 @@ module.exports = {
   checkNoFaIcons,
   checkI18nTemplates,
   checkUntranslatedStrings,
-  checkTypeScriptOnlySource,
 };

superset/config.py

@@ -1766,14 +1766,9 @@ SMTP_USER = "superset"
 SMTP_PORT = 25
 SMTP_PASSWORD = "superset"  # noqa: S105
 SMTP_MAIL_FROM = "superset@superset.com"
-# If True creates a default SSL context with ssl.Purpose.SERVER_AUTH using the
-# default system root CA certificates. This makes STARTTLS/SSL connections to the
-# SMTP server validate the server's certificate against the trusted CA store.
-# Defaults to True so the mail server identity is verified out of the box. Set to
-# False to restore the previous behavior of skipping certificate validation (for
-# example, when using a self-signed certificate that is not in the system CA
-# store).
-SMTP_SSL_SERVER_AUTH = True
+# If True creates a default SSL context with ssl.Purpose.CLIENT_AUTH using the
+# default system root CA certificates.
+SMTP_SSL_SERVER_AUTH = False
 # Socket timeout (in seconds) for the SMTP connection used when sending
 # alert/report emails. Without a timeout the underlying socket blocks
 # indefinitely if the SMTP server becomes unreachable, which leaves report

tests/integration_tests/email_tests.py

@@ -37,18 +37,9 @@ logger = logging.getLogger(__name__)
 
 
 class TestEmailSmtp(SupersetTestCase):
-    SMTP_CONFIG_KEYS = ("SMTP_SSL", "SMTP_SSL_SERVER_AUTH", "SMTP_STARTTLS")
-
-    def setUp(self) -> None:
-        self._original_smtp_config = {
-            key: current_app.config[key] for key in self.SMTP_CONFIG_KEYS
-        }
+    def setUp(self):
         current_app.config["SMTP_SSL"] = False
 
-    def tearDown(self) -> None:
-        current_app.config.update(self._original_smtp_config)
-        super().tearDown()
-
     @mock.patch("superset.utils.core.send_mime_email")
     def test_send_smtp(self, mock_send_mime):
         attachment = tempfile.NamedTemporaryFile()
@@ -219,7 +210,6 @@ class TestEmailSmtp(SupersetTestCase):
     @mock.patch("smtplib.SMTP")
     def test_send_mime_ssl(self, mock_smtp, mock_smtp_ssl):
         current_app.config["SMTP_SSL"] = True
-        current_app.config["SMTP_SSL_SERVER_AUTH"] = False
         mock_smtp.return_value = mock.Mock()
         mock_smtp_ssl.return_value = mock.Mock()
         utils.send_mime_email(

tests/unit_tests/config_test.py

@@ -349,123 +349,3 @@ def test_theme_default_logo_defaults() -> None:
     assert config.LOGO_TARGET_PATH is None
     assert config.THEME_DEFAULT["token"]["brandLogoHref"] == "/"
     assert config.THEME_DEFAULT["token"]["brandLogoUrl"] == config.APP_ICON
-
-
-def test_smtp_ssl_server_auth_defaults_to_true() -> None:
-    """
-    The shipped default for SMTP_SSL_SERVER_AUTH validates the SMTP server's
-    TLS certificate. Operators can still opt out by overriding it to False.
-    """
-    from superset import config
-
-    assert config.SMTP_SSL_SERVER_AUTH is True
-
-
-def _smtp_config(**overrides: Any) -> dict[str, Any]:
-    """
-    Build a minimal SMTP config dict for ``send_mime_email`` tests, with
-    plaintext transport defaults; keyword ``overrides`` replace any key.
-    """
-    config = {
-        "SMTP_HOST": "localhost",
-        "SMTP_PORT": 25,
-        "SMTP_USER": "",
-        "SMTP_PASSWORD": "",
-        "SMTP_STARTTLS": False,
-        "SMTP_SSL": False,
-        "SMTP_SSL_SERVER_AUTH": True,
-    }
-    config.update(overrides)
-    return config
-
-
-def test_send_mime_email_ssl_server_auth_passes_context(
-    mocker: MockerFixture,
-) -> None:
-    """
-    With SMTP_SSL and SMTP_SSL_SERVER_AUTH enabled, ``send_mime_email`` builds a
-    default SSL context and threads it through to ``smtplib.SMTP_SSL`` so the
-    server certificate is validated.
-    """
-    from email.mime.multipart import MIMEMultipart
-
-    from superset.utils import core as utils
-
-    create_default_context = mocker.patch(
-        "superset.utils.core.ssl.create_default_context"
-    )
-    smtp_ssl = mocker.patch("smtplib.SMTP_SSL")
-    smtp = mocker.patch("smtplib.SMTP")
-
-    utils.send_mime_email(
-        "from",
-        ["to"],
-        MIMEMultipart(),
-        _smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=True),
-        dryrun=False,
-    )
-
-    create_default_context.assert_called_once_with()
-    assert not smtp.called
-    smtp_ssl.assert_called_once_with(
-        "localhost", 25, context=create_default_context.return_value, timeout=30
-    )
-
-
-def test_send_mime_email_starttls_server_auth_passes_context(
-    mocker: MockerFixture,
-) -> None:
-    """
-    With STARTTLS and SMTP_SSL_SERVER_AUTH enabled, ``send_mime_email`` builds a
-    default SSL context and threads it through to ``starttls`` so the server
-    certificate is validated.
-    """
-    from email.mime.multipart import MIMEMultipart
-
-    from superset.utils import core as utils
-
-    create_default_context = mocker.patch(
-        "superset.utils.core.ssl.create_default_context"
-    )
-    smtp = mocker.patch("smtplib.SMTP")
-
-    utils.send_mime_email(
-        "from",
-        ["to"],
-        MIMEMultipart(),
-        _smtp_config(SMTP_STARTTLS=True, SMTP_SSL_SERVER_AUTH=True),
-        dryrun=False,
-    )
-
-    create_default_context.assert_called_once_with()
-    smtp.return_value.starttls.assert_called_once_with(
-        context=create_default_context.return_value
-    )
-
-
-def test_send_mime_email_server_auth_disabled_skips_context(
-    mocker: MockerFixture,
-) -> None:
-    """
-    When SMTP_SSL_SERVER_AUTH is disabled no SSL context is built and ``None`` is
-    passed through, preserving the opt-out (certificate validation skipped).
-    """
-    from email.mime.multipart import MIMEMultipart
-
-    from superset.utils import core as utils
-
-    create_default_context = mocker.patch(
-        "superset.utils.core.ssl.create_default_context"
-    )
-    smtp_ssl = mocker.patch("smtplib.SMTP_SSL")
-
-    utils.send_mime_email(
-        "from",
-        ["to"],
-        MIMEMultipart(),
-        _smtp_config(SMTP_SSL=True, SMTP_SSL_SERVER_AUTH=False),
-        dryrun=False,
-    )
-
-    assert not create_default_context.called
-    smtp_ssl.assert_called_once_with("localhost", 25, context=None, timeout=30)