[pull] master from apache:master

.github/ISSUE_TEMPLATE/bug-report.yml

@@ -1,6 +1,6 @@
 name: Bug report
 description: Report a bug to improve Superset's stability
-labels: ["bug"]
+labels: ["#bug"]
 body:
   - type: markdown
     attributes:

.github/ISSUE_TEMPLATE/cosmetic.md

@@ -1,7 +1,7 @@
 ---
 name: Cosmetic Issue
 about: Describe a cosmetic issue with CSS, positioning, layout, labeling, or similar
-labels: "cosmetic-issue"
+labels: "#bug:cosmetic"
 ---
 
 ## Screenshot

scripts/benchmark_migration.py

@@ -93,7 +93,7 @@ def find_models(module: ModuleType) -> list[type[Model]]:  # noqa: C901
     # where the current model is out-of-sync with the existing table after a
     # downgrade
     sqlalchemy_uri = current_app.config["SQLALCHEMY_DATABASE_URI"]
-    engine = create_engine(sqlalchemy_uri, future=True)
+    engine = create_engine(sqlalchemy_uri)
     Base = automap_base()  # noqa: N806
     Base.prepare(engine, reflect=True)
     seen = set()

superset-frontend/playwright/tests/recently-archived/recently-archived.spec.ts

@@ -29,7 +29,7 @@
  * restore it and asserts — via the API — that it is live again.
  */
 import { test, expect, Page } from '@playwright/test';
-import { apiGet, apiPost } from '../../helpers/api/requests';
+import { apiGet } from '../../helpers/api/requests';
 import { extractIdFromResponse } from '../../helpers/api/assertions';
 import {
   apiPostChart,
@@ -188,58 +188,3 @@ test('permanently deletes an archived item from the view', async ({ page }) => {
     await TYPES[0].softDelete(page, id).catch(() => {});
   }
 });
-
-test('shows an empty message and no rows when the search matches nothing', async ({
-  page,
-}) => {
-  await page.goto('archived/');
-  await expect(page.getByTestId('archived-list-view')).toBeVisible();
-
-  const search = page.getByPlaceholder(/type a value/i);
-  await search.click();
-  await search.fill(`e2e_nonexistent_${Date.now()}`);
-  await search.press('Enter');
-
-  await expect(
-    page.getByText('No results match your filter criteria'),
-  ).toBeVisible();
-  await expect(page.getByTestId('archived-row-restore')).toHaveCount(0);
-});
-
-test('restoring an already-restored row surfaces an error without crashing', async ({
-  page,
-}) => {
-  const name = `e2e_stale_${Date.now()}`;
-  const id = await TYPES[0].create(page, name);
-  // Capture the uuid before soft-delete (a soft-deleted GET returns 404).
-  const { uuid } = (await (await apiGetDashboard(page, id)).json()).result;
-  try {
-    expect((await apiDeleteDashboard(page, id)).ok()).toBeTruthy();
-
-    await openArchive(page, 'Dashboard', name);
-    await expect(page.getByText(name, { exact: false })).toBeVisible();
-
-    // Simulate another actor restoring the object out from under this view.
-    const restored = await apiPost(
-      page,
-      `api/v1/dashboard/${uuid}/restore`,
-      {},
-    );
-    expect(restored.ok()).toBeTruthy();
-
-    // Clicking the now-stale row's Restore yields a 404 → danger toast, no crash.
-    await page
-      .getByRole('row')
-      .filter({ hasText: name })
-      .getByTestId('archived-row-restore')
-      .click();
-    await expect(
-      page.getByText(`Failed to restore ${name}`, { exact: false }),
-    ).toBeVisible({ timeout: 15000 });
-    // The page is still functional (the list view did not crash).
-    await expect(page.getByTestId('archived-list-view')).toBeVisible();
-  } finally {
-    // Re-archive the (possibly) restored dashboard, whatever happened above.
-    await apiDeleteDashboard(page, id).catch(() => {});
-  }
-});

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts

@@ -467,6 +467,14 @@ export function transformSeries(
           return formatter(numericValue);
         }
         if (!onlyTotal) {
+          // A stacked segment with no height begins and ends at the same
+          // coordinate as the top of the segment beneath it, so its label is
+          // drawn over that segment's label. Zero and null have no height, so
+          // they carry no label. The rich tooltip omits zero observations from
+          // a stacked series for the same reason.
+          if (stack && !numericValue) {
+            return '';
+          }
           if (
             numericValue >=
             (thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER)

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts

@@ -20,13 +20,14 @@ import {
   CategoricalColorScale,
   ChartProps,
   TimeGranularity,
+  getNumberFormatter,
 } from '@superset-ui/core';
 import { GenericDataType } from '@apache-superset/core/common';
 import { supersetTheme } from '@apache-superset/core/theme';
 import type { SeriesOption } from 'echarts';
 import type { ScatterSeriesOption } from 'echarts/charts';
 import { EchartsTimeseriesSeriesType } from '../../src';
-import { TIMESERIES_CONSTANTS } from '../../src/constants';
+import { StackControlsValue, TIMESERIES_CONSTANTS } from '../../src/constants';
 import {
   LegendOrientation,
   EchartsTimeseriesChartProps,
@@ -566,3 +567,70 @@ test('getPadding should handle Left position with zero margin correctly', () =>
     getChartPaddingSpy.mockRestore();
   }
 });
+
+/**
+ * #42702: a stacked segment with no height starts and ends at the same
+ * coordinate as the top of the segment beneath it, so a value label on it is
+ * drawn over that segment's label. `percentage_threshold` does not filter these
+ * out: it defaults to 0, and `thresholdValues[dataIndex] || MIN_SAFE_INTEGER`
+ * turns a 0 threshold into "no filtering", which is intentional.
+ */
+const stackedLabel = (
+  numericValue: number | null,
+  opts: Record<string, unknown> = {},
+) => {
+  const series = transformSeries(
+    { id: 'B', name: 'B', data: [[1, numericValue]] } as SeriesOption,
+    mockColorScale,
+    'B',
+    {
+      seriesType: EchartsTimeseriesSeriesType.Bar,
+      stack: StackControlsValue.Stack,
+      showValue: true,
+      onlyTotal: false,
+      formatter: getNumberFormatter(),
+      thresholdValues: [0],
+      ...opts,
+    },
+  ) as SeriesOption & {
+    label: { formatter: (params: unknown) => string };
+  };
+  return series.label.formatter({
+    value: [1, numericValue],
+    dataIndex: 0,
+    seriesIndex: 1,
+    seriesName: 'B',
+  });
+};
+
+test('stacked value labels are omitted for a zero-height segment', () => {
+  expect(stackedLabel(0)).toBe('');
+  expect(stackedLabel(null)).toBe('');
+});
+
+test('stacked value labels are kept for segments that have height', () => {
+  expect(stackedLabel(32)).toBe('32');
+  expect(stackedLabel(-5)).toBe('-5');
+});
+
+test('a zero value keeps its label when the series is not stacked', () => {
+  // Without a stack the label sits on the bar itself, so there is nothing for
+  // it to collide with.
+  expect(stackedLabel(0, { stack: undefined })).toBe('0');
+});
+
+test('percentage_threshold still filters values below the threshold', () => {
+  // 10% of a 100 total. The zero-height guard must not swallow this rule.
+  expect(stackedLabel(5, { thresholdValues: [10] })).toBe('');
+  expect(stackedLabel(50, { thresholdValues: [10] })).toBe('50');
+});
+
+test('only-total labels are unaffected by the zero-height guard', () => {
+  expect(
+    stackedLabel(0, {
+      onlyTotal: true,
+      showValueIndexes: [1],
+      totalStackedValues: [32],
+    }),
+  ).toBe('32');
+});

superset-frontend/src/components/Chart/chartReducer.ts

@@ -223,6 +223,11 @@ export default function chartReducer(
   }
 
   if (action.type in actionHandlers) {
+    // ADD_CHART creates the entry, so it runs without prior state; every other
+    // handler reads state that is absent once the chart has been removed
+    if (action.type !== actions.ADD_CHART && !charts[action.key]) {
+      return charts;
+    }
     return {
       ...charts,
       [action.key]: actionHandlers[action.type](charts[action.key]),

superset-frontend/src/components/Chart/chartReducers.test.ts

@@ -91,4 +91,20 @@ describe('chart reducers', () => {
     expect(newState[chartKey].chartUpdateEndTime).toBeGreaterThan(0);
     expect(newState[chartKey].chartStatus).toEqual('failed');
   });
+
+  test('ignores an action for a chart that is no longer in state', () => {
+    const action = actions.chartUpdateStopped(999, new AbortController());
+    expect(() => chartReducer(charts, action)).not.toThrow();
+    expect(chartReducer(charts, action)).toEqual(charts);
+  });
+
+  test('still adds a chart that is not yet in state', () => {
+    const newChartKey = 2;
+    const newState = chartReducer(
+      charts,
+      actions.addChart({ ...chart, id: newChartKey }, newChartKey),
+    );
+    expect(newState[newChartKey].id).toEqual(newChartKey);
+    expect(newState[chartKey]).toEqual(testChart);
+  });
 });

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

@@ -105,10 +105,17 @@ export const SamplesPane = ({
         1,
       )
         .then(response => {
-          setData(ensureIsArray(response.data));
-          setColnames(ensureIsArray(response.colnames));
-          setColtypes(ensureIsArray(response.coltypes));
-          setRowCount(response.rowcount);
+          // A 200 that carries no `result` payload resolves to undefined here.
+          // Read through it so the pane falls back to its empty state instead
+          // of throwing a TypeError that surfaces as an internal error message.
+          const rows = ensureIsArray(response?.data);
+          setData(rows);
+          setColnames(ensureIsArray(response?.colnames));
+          setColtypes(ensureIsArray(response?.coltypes));
+          // Fall back to the rows actually returned rather than to zero: the
+          // controls only render when there are rows, and a hardcoded 0 would
+          // label a populated table as "0 rows".
+          setRowCount(response?.rowcount ?? rows.length);
           setResponseError('');
           cache.set(queryFormData, true);
           if (queryForce) {

superset-frontend/src/explore/components/DataTablesPane/test/SamplesPane.test.tsx

@@ -60,6 +60,27 @@ describe('SamplesPane', () => {
     400,
   );
 
+  // A 200 response that carries no `result` payload, as reported in #36840.
+  fetchMock.post(
+    'end:/datasource/samples?force=false&datasource_type=table&datasource_id=37&per_page=100&page=1',
+    {},
+  );
+
+  // A 200 whose result carries rows but omits `rowcount`.
+  fetchMock.post(
+    'end:/datasource/samples?force=false&datasource_type=table&datasource_id=38&per_page=100&page=1',
+    {
+      result: {
+        data: [
+          { __timestamp: 1230768000000, genre: 'Action' },
+          { __timestamp: 1230768000010, genre: 'Horror' },
+        ],
+        colnames: ['__timestamp', 'genre'],
+        coltypes: [2, 1],
+      },
+    },
+  );
+
   const setForceQuery = jest.fn();
 
   afterAll(() => {
@@ -114,4 +135,29 @@ describe('SamplesPane', () => {
     expect(queryByText('Action')).toBeVisible();
     expect(queryByText('Horror')).toBeVisible();
   });
+
+  test('renders the empty state when the response carries no result payload', async () => {
+    const props = createSamplesPaneProps({ datasourceId: 37 });
+    const { findByText, queryByRole } = render(<SamplesPane {...props} />, {
+      useRedux: true,
+    });
+
+    expect(
+      await findByText('No samples were returned for this dataset'),
+    ).toBeVisible();
+    // The pane should not leak an internal TypeError through the error alert.
+    expect(queryByRole('alert')).not.toBeInTheDocument();
+  });
+
+  test('counts the returned rows when the response omits rowcount', async () => {
+    const props = createSamplesPaneProps({ datasourceId: 38 });
+    const { findByText, queryByText } = render(<SamplesPane {...props} />, {
+      useRedux: true,
+    });
+
+    expect(await findByText('Action')).toBeVisible();
+    // Falling back to 0 here would label a populated table as "0 rows".
+    expect(queryByText('0 rows')).not.toBeInTheDocument();
+    expect(queryByText('2 rows')).toBeVisible();
+  });
 });