Compare commits

..

11 Commits

Author SHA1 Message Date
Joe Li
242a41b82e fix(reports): correct retry wall-clock bound docs and minor cleanups
Address code-review findings on the webhook retry / executor-resolution
work:

- Webhook retry docs and the backoff comment described the worst-case
  bound as ~180s; backoff checks max_time against the time elapsed
  before each attempt, so the final request can start just under the
  limit and run its full 60s timeout — worst case ~210s.
- Drop the unused executor `username` unpacking in `_get_screenshots`.
- Clarify the Slack v2 atomic-revert comment (restoring in-memory
  values keeps a later commit from persisting a half-migrated set).
- Fix a stale "four call sites" -> "three call sites" docstring and a
  freezegun/backoff elapsed-clock comment in the tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-24 15:46:57 -07:00
Joe Li
faca187b51 fix(reports): mark missing-executor failure as a server error (500)
A missing executor user is a server-side misconfiguration, not a malformed
client request. ReportScheduleExecutorNotFoundError carried status 422, which
get_logger_from_status maps to WARNING — so scheduler.execute logged the
failure but did not mark the Celery task FAILURE, hiding the misconfiguration
from task-state alerting. Master's raw NoneType error was uncaught and landed
as FAILURE; the 422 silently dropped that signal.

Set the status to 500 so get_logger_from_status routes it to EXCEPTION and the
Celery task is marked FAILURE, restoring master's task-state signal while
keeping the dedicated typed error, the ERROR execution-log row, and the owner
notification. Reword the execute.py guard comment to be status-agnostic and add
a regression test pinning the 5xx contract.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-24 11:11:15 -07:00
Joe Li
6a6a4d4f1a fix(reports): address webhook retry review feedback
- Migrate webhook backoff tests off the hand-rolled _FakeBackoffDatetime
  monkeypatch onto freezegun.freeze_time. Persistent-fail and recovering
  tests use auto_tick so max_time is genuinely engaged; assertions are
  property-based (range / governed-by-mock) rather than pinned tick counts.
- Fall back to "(unknown)" for ReportScheduleExecutorNotFoundError when no
  username is available, avoiding a double-space in the message; add a
  regression test and a class docstring.
- Document the ~120s cumulative wall-clock retry bound (worst case ~180s)
  in the alerts-reports admin docs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-24 11:11:15 -07:00
Joe Li
0c02e0a3fb fix(reports): handle missing executor user and revert Slack v2 atomically
Two robustness fixes in the alert/report execution command:

1. Missing executor user: when the configured executor cannot be resolved
   (security_manager.find_user returns None), the content-generation sites
   (_get_screenshots / _get_csv_data / _get_embedded_data) now raise a
   dedicated ReportScheduleExecutorNotFoundError instead of failing later
   with an opaque NoneType error. The guard lives at the content sites so
   it raises inside the state machine's error envelope, preserving the
   ERROR execution-log row and the owner error notification. The run()
   boundary continues to delegate to the state machine (a missing user is
   tolerated there, matching prior behavior) so visibility is unchanged.

2. Slack v2 migration revert: update_report_schedule_slack_v2 now snapshots
   every recipient it mutates and reverts all of them on failure, instead
   of only the loop variable, and no longer raises UnboundLocalError when
   the failure occurs before the loop binds a recipient.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-24 11:11:15 -07:00
Joe Li
4c9b5c54f1 fix(reports): bound webhook notification retry wall-clock time
Webhook notification retries used backoff without a max_time bound, so a
hanging or persistently-failing target could stall a worker for minutes
per bad URL (up to ~5 socket waits at timeout=60 plus retry sleeps),
starving sequential report dispatch.

Add max_time=120 to the backoff decorator on WebhookNotification.send.
factor/base/max_tries are unchanged, so legitimately-transient 5xx
targets are still retried; max_time only caps total wall-clock so a bad
target cannot monopolize a worker. max_time is checked between attempts,
so the final in-flight request still runs its full timeout.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-24 11:11:15 -07:00
abhyudaytomar
09c7ba14df fix(export): sanitize control characters in titles to prevent export failures (#39294)
Co-authored-by: Abhyuday Tomar <abhyuday.tomar@exotel.com>
Co-authored-by: Evan <evan@preset.io>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-24 11:03:46 -07:00
Elizabeth Thompson
3ec4bd23c4 fix(deps): restore np.nan in offset_metrics_df empty branch (#41267)
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-06-24 10:49:38 -07:00
yousoph
f6ce105450 fix(pandas-postprocessing): handle prophet errors and validate minimum data points for forecast (#41180)
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-06-24 10:44:23 -07:00
Evan Rusackas
7bb4e82a82 fix(dashboard): Remove 308 redirect when creating new dashboards (#41343)
Co-authored-by: ericsong <eric.song@example.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-24 10:31:31 -07:00
Kamil Gabryjelski
2d78a8733c fix(plugin-chart-ag-grid-table): show correct percent-metric totals in summary row (#41247)
Signed-off-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
2026-06-24 19:21:00 +02:00
Evan Rusackas
3261d10270 chore(frontend): enforce TypeScript-only source files (#41385)
Co-authored-by: Claude Code <noreply@anthropic.com>
2026-06-24 05:54:37 -07:00
34 changed files with 899 additions and 245 deletions

View File

@@ -160,7 +160,7 @@ When enabled, Superset rejects webhook configurations that use `http://` URLs.
#### Retry Behavior
Superset automatically retries webhook deliveries on `429 Too Many Requests` and `5xx` server errors using exponential backoff.
Superset automatically retries webhook deliveries on `429 Too Many Requests` and `5xx` server errors using exponential backoff. Retries are bounded to roughly 120 seconds of cumulative wall-clock time (worst case ~210 seconds, because the bound is checked against the time elapsed before each attempt, so the final request can begin just under the limit and still run its full request timeout), after which the delivery is abandoned.
### Kubernetes-specific

View File

@@ -90,6 +90,13 @@ const buildQuery: BuildQuery<TableChartFormData> = (
let { metrics, orderby = [], columns = [] } = baseQueryObject;
const { extras = {} } = baseQueryObject;
let postProcessing: PostProcessingRule[] = [];
// Capture the percent-metric `contribution` rule so it can be reused for
// the totals query below. The totals query must rename percent-metric
// columns the same way (`metric` -> `%metric`) so the footer can look them
// up; without it the totals row renders 0.000%. We deliberately reuse only
// this rule and not the full `postProcessing` array, which may also contain
// a time-comparison operator that must not run on the single totals row.
let contributionPostProcessing: PostProcessingRule | undefined;
const nonCustomNorInheritShifts = ensureIsArray(
formData.time_compare,
).filter((shift: string) => shift !== 'custom' && shift !== 'inherit');
@@ -157,15 +164,14 @@ const buildQuery: BuildQuery<TableChartFormData> = (
metrics.concat(percentMetrics),
getMetricLabel,
);
postProcessing = [
{
operation: 'contribution',
options: {
columns: percentMetricLabels,
rename_columns: percentMetricLabels.map(x => `%${x}`),
},
contributionPostProcessing = {
operation: 'contribution',
options: {
columns: percentMetricLabels,
rename_columns: percentMetricLabels.map(x => `%${x}`),
},
];
};
postProcessing = [contributionPostProcessing];
}
// Add the operator for the time comparison if some is selected
if (!isEmpty(timeOffsets)) {
@@ -658,7 +664,13 @@ const buildQuery: BuildQuery<TableChartFormData> = (
extras: totalsExtras, // Use extras with AG Grid WHERE removed
row_limit: 0,
row_offset: 0,
post_processing: [],
// Reapply only the percent-metric contribution rule so the totals row
// exposes `%metric` keys (value/value = 100% on the single aggregated
// row). The time-comparison operator from the main query is omitted on
// purpose; it must not run against the single-row totals query.
post_processing: contributionPostProcessing
? [contributionPostProcessing]
: [],
order_desc: undefined, // we don't need orderby stuff here,
orderby: undefined, // because this query will be used for get total aggregation.
});

View File

@@ -44,8 +44,3 @@ 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__';

View File

@@ -24,7 +24,6 @@ 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`
${() => `
@@ -164,13 +163,13 @@ export const NumericCellRenderer = (
let arrow = '';
let arrowColor = '';
if (hasBasicColorFormatters && col?.metricName) {
const rowFormatter = getRowBasicColorFormatter(
node,
node?.rowIndex,
basicColorFormatters,
)?.[col.metricName];
arrow = rowFormatter?.mainArrow;
arrowColor = rowFormatter?.arrowColor?.toLowerCase();
arrow =
basicColorFormatters?.[node?.rowIndex as number]?.[col.metricName]
?.mainArrow;
arrowColor =
basicColorFormatters?.[node?.rowIndex as number]?.[
col.metricName
]?.arrowColor?.toLowerCase();
}
const alignment =

View File

@@ -46,7 +46,6 @@ 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,
@@ -704,23 +703,6 @@ 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) ?? [];

View File

@@ -24,7 +24,6 @@ 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;
@@ -85,11 +84,8 @@ const getCellStyle = (params: CellStyleParams) => {
col?.metricName &&
node?.rowPinned !== 'bottom'
) {
backgroundColor = getRowBasicColorFormatter(
node,
rowIndex,
basicColorFormatters,
)?.[col.metricName]?.backgroundColor;
backgroundColor =
basicColorFormatters?.[rowIndex]?.[col.metricName]?.backgroundColor;
}
const textAlign =

View File

@@ -1,51 +0,0 @@
/**
* 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];
}

View File

@@ -852,6 +852,75 @@ describe('plugin-chart-ag-grid-table', () => {
expect(totalsQuery.columns).toEqual([]);
expect(totalsQuery.row_limit).toBe(0);
});
test('should reapply percent-metric contribution op to totals query', () => {
// Regression test for #37627: when a percent metric is configured and
// Show Summary (show_totals) is enabled, the totals query must rename
// percent-metric columns (`metric` -> `%metric`) so the footer can
// look them up. Otherwise the totals row renders 0.000%.
const { queries } = buildQuery({
...basicFormData,
metrics: ['count'],
percent_metrics: ['count'],
show_totals: true,
query_mode: QueryMode.Aggregate,
});
// No server pagination -> queries[1] is the totals query.
const totalsQuery = queries[1];
const contributionRule = {
operation: 'contribution',
options: {
columns: ['count'],
rename_columns: ['%count'],
},
};
expect(queries[0].post_processing).toContainEqual(contributionRule);
expect(totalsQuery.post_processing).toEqual([contributionRule]);
});
test('should omit time-comparison op from totals post_processing', () => {
// The totals query must reuse ONLY the contribution rule; the
// time-comparison operator from the main query must not run against
// the single-row totals query.
const { queries } = buildQuery({
...basicFormData,
metrics: ['count'],
percent_metrics: ['count'],
show_totals: true,
query_mode: QueryMode.Aggregate,
time_compare: ['1 year ago'],
comparison_type: 'values',
});
const totalsQuery = queries[1];
// Exactly one op (contribution) — the time-comparison operator from the
// main query must not be carried over to the single-row totals query.
expect(totalsQuery.post_processing).toHaveLength(1);
expect(totalsQuery.post_processing?.[0]).toMatchObject({
operation: 'contribution',
});
// The reused rule matches the main query's contribution rule verbatim.
expect(totalsQuery.post_processing?.[0]).toEqual(
queries[0].post_processing?.find(
op => op?.operation === 'contribution',
),
);
});
test('should leave totals post_processing empty without percent metrics', () => {
const { queries } = buildQuery({
...basicFormData,
metrics: ['count'],
show_totals: true,
query_mode: QueryMode.Aggregate,
});
const totalsQuery = queries[1];
expect(totalsQuery.post_processing).toEqual([]);
});
});
describe('Integration - all filter types together', () => {

View File

@@ -1,65 +0,0 @@
/**
* 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']);
});

View File

@@ -86,6 +86,13 @@ export const buildQuery: BuildQuery<TableChartFormData> = (
let { metrics, orderby = [], columns = [] } = baseQueryObject;
const { extras = {} } = baseQueryObject;
const postProcessing: PostProcessingRule[] = [];
// Capture the percent-metric `contribution` rule so it can be reused for
// the totals query below. Without it the totals row's percent-metric
// columns are keyed `metric` instead of `%metric`, so the footer renders
// 0.000%. We reuse only this rule and not the full `postProcessing` array,
// which may also contain a time-comparison operator that must not run on
// the single totals row.
let contributionPostProcessing: PostProcessingRule | undefined;
const nonCustomNorInheritShifts = ensureIsArray(
formData.time_compare,
).filter((shift: string) => shift !== 'custom' && shift !== 'inherit');
@@ -137,12 +144,6 @@ export const buildQuery: BuildQuery<TableChartFormData> = (
orderby = [[metrics[0], false]];
}
// add postprocessing for percent metrics only when in aggregation mode
type PercentMetricCalculationMode = 'row_limit' | 'all_records';
const calculationMode: PercentMetricCalculationMode =
(formData.percent_metric_calculation as PercentMetricCalculationMode) ||
'row_limit';
if (percentMetrics && percentMetrics.length > 0) {
const percentMetricsLabelsWithTimeComparison = isTimeComparison(
formData,
@@ -162,23 +163,14 @@ export const buildQuery: BuildQuery<TableChartFormData> = (
getMetricLabel,
);
if (calculationMode === 'all_records') {
postProcessing.push({
operation: 'contribution',
options: {
columns: percentMetricLabels,
rename_columns: percentMetricLabels.map(m => `%${m}`),
},
});
} else {
postProcessing.push({
operation: 'contribution',
options: {
columns: percentMetricLabels,
rename_columns: percentMetricLabels.map(m => `%${m}`),
},
});
}
contributionPostProcessing = {
operation: 'contribution',
options: {
columns: percentMetricLabels,
rename_columns: percentMetricLabels.map(m => `%${m}`),
},
};
postProcessing.push(contributionPostProcessing);
}
// Add the operator for the time comparison if some is selected
@@ -357,7 +349,13 @@ export const buildQuery: BuildQuery<TableChartFormData> = (
columns: [],
row_limit: 0,
row_offset: 0,
post_processing: [],
// Reapply only the percent-metric contribution rule so the totals row
// exposes `%metric` keys (value/value = 100% on the single aggregated
// row). The time-comparison operator from the main query is omitted on
// purpose; it must not run against the single-row totals query.
post_processing: contributionPostProcessing
? [contributionPostProcessing]
: [],
order_desc: undefined,
orderby: undefined,
});

View File

@@ -236,6 +236,83 @@ describe('plugin-chart-table', () => {
expect(queries).toHaveLength(1);
expect(queries[0].post_processing).toEqual([]);
});
test('should reapply contribution op to totals query in row_limit mode', () => {
// Regression test for #37627: with a percent metric and Show Summary
// (show_totals) enabled, the totals query must rename percent-metric
// columns (`metric` -> `%metric`) so the footer can look them up.
// Otherwise the totals row renders 0.000%.
const formData = {
...baseFormDataWithPercents,
show_totals: true,
};
const { queries } = buildQuery(formData);
// row_limit mode + show_totals -> [main, totals].
expect(queries).toHaveLength(2);
const contributionRule = {
operation: 'contribution',
options: {
columns: ['sum_sales'],
rename_columns: ['%sum_sales'],
},
};
expect(queries[1]).toMatchObject({
columns: [],
post_processing: [contributionRule],
});
});
test('should omit time-comparison op from totals post_processing', () => {
// The totals query must reuse ONLY the contribution rule; the
// time-comparison operator from the main query must not run against
// the single-row totals query.
const formData = {
...baseFormDataWithPercents,
show_totals: true,
time_compare: ['1 year ago'],
comparison_type: 'values',
};
const { queries } = buildQuery(formData);
// row_limit mode + show_totals -> [main, totals].
expect(queries).toHaveLength(2);
const totalsQuery = queries[1];
// Exactly one op (contribution) — the time-comparison operator from the
// main query must not be carried over to the single-row totals query.
expect(totalsQuery.post_processing).toHaveLength(1);
expect(totalsQuery.post_processing?.[0]).toMatchObject({
operation: 'contribution',
});
// The reused rule matches the main query's contribution rule verbatim.
expect(totalsQuery.post_processing?.[0]).toEqual(
queries[0].post_processing?.find(
op => op?.operation === 'contribution',
),
);
});
test('should leave totals post_processing empty without percent metrics', () => {
const formData = {
...basicFormData,
query_mode: QueryMode.Aggregate,
metrics: ['count'],
percent_metrics: [],
groupby: ['category'],
show_totals: true,
};
const { queries } = buildQuery(formData);
expect(queries).toHaveLength(2);
expect(queries[1].post_processing).toEqual([]);
});
});
describe('Testing for server pagination with search filter', () => {

View File

@@ -632,6 +632,35 @@ 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
*/
@@ -666,6 +695,22 @@ 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}', {
@@ -706,22 +751,23 @@ function main() {
if (files.length === 0) {
// eslint-disable-next-line no-console
console.log('No files to check.');
return;
} 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);
}
});
}
// 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`);
@@ -740,4 +786,5 @@ module.exports = {
checkNoFaIcons,
checkI18nTemplates,
checkUntranslatedStrings,
checkTypeScriptOnlySource,
};

View File

@@ -42,6 +42,7 @@ import { getActiveFilters } from 'src/dashboard/util/activeDashboardFilters';
import { LocalStorageKeys, setItem } from 'src/utils/localStorageHelpers';
import { URL_PARAMS } from 'src/constants';
import { getUrlParam } from 'src/utils/urlUtils';
import { sanitizeDocumentTitle } from 'src/utils/sanitizeDocumentTitle';
import { setDatasetsStatus } from 'src/dashboard/actions/dashboardState';
import { DASHBOARD_HEADER_ID } from 'src/dashboard/util/constants';
import {
@@ -337,7 +338,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
// Update document title when dashboard title changes
useEffect(() => {
if (pageTitle) {
document.title = pageTitle;
document.title = sanitizeDocumentTitle(pageTitle);
}
}, [pageTitle]);

View File

@@ -66,6 +66,7 @@ import {
LOG_ACTIONS_CHANGE_EXPLORE_CONTROLS,
} from 'src/logger/LogUtils';
import { getUrlParam } from 'src/utils/urlUtils';
import { sanitizeDocumentTitle } from 'src/utils/sanitizeDocumentTitle';
import cx from 'classnames';
import * as chartActions from 'src/components/Chart/chartAction';
import { fetchDatasourceMetadata } from 'src/dashboard/actions/datasources';
@@ -397,7 +398,7 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
// Update document title when slice name changes
useEffect(() => {
if (props.sliceName) {
document.title = props.sliceName;
document.title = sanitizeDocumentTitle(props.sliceName);
}
}, [props.sliceName]);

View File

@@ -243,7 +243,7 @@ test('handles create dashboard button click', async () => {
const createButton = screen.getByRole('button', { name: /dashboard$/i });
await userEvent.click(createButton);
expect(assignMock).toHaveBeenCalledWith('/dashboard/new');
expect(assignMock).toHaveBeenCalledWith('/dashboard/new/');
locationSpy.mockRestore();
});

View File

@@ -203,7 +203,7 @@ function DashboardTable({
name: t('Dashboard'),
buttonStyle: 'secondary',
onClick: () => {
navigateTo('/dashboard/new', { assign: true });
navigateTo('/dashboard/new/', { assign: true });
},
},
{

View File

@@ -57,7 +57,7 @@ const LABELS = {
const REDIRECTS = {
create: {
[WelcomeTable.Charts]: '/chart/add',
[WelcomeTable.Dashboards]: '/dashboard/new',
[WelcomeTable.Dashboards]: '/dashboard/new/',
// navigateTo() applies the application root internally; keep this
// relative so the prefix isn't added twice.
[WelcomeTable.SavedQueries]: '/sqllab?new=true',

View File

@@ -89,7 +89,7 @@ const dropdownItems = [
},
{
label: 'Dashboard',
url: '/dashboard/new',
url: '/dashboard/new/',
icon: 'fa-fw fa-dashboard',
perm: 'can_write',
view: 'Dashboard',

View File

@@ -105,7 +105,7 @@ const dropdownItems = [
},
{
label: 'Dashboard',
url: '/dashboard/new',
url: '/dashboard/new/',
icon: 'fa-fw fa-dashboard',
perm: 'can_write',
view: 'Dashboard',

View File

@@ -232,7 +232,7 @@ const RightMenu = ({
},
{
label: t('Dashboard'),
url: '/dashboard/new',
url: '/dashboard/new/',
icon: (
<Icons.DashboardOutlined data-test={`menu-item-${t('Dashboard')}`} />
),

View File

@@ -762,7 +762,7 @@ function DashboardList(props: DashboardListProps) {
name: t('Dashboard'),
buttonStyle: 'primary',
onClick: () => {
navigateTo('/dashboard/new', { assign: true });
navigateTo('/dashboard/new/', { assign: true });
},
});
}

View File

@@ -0,0 +1,35 @@
/**
* 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 { sanitizeDocumentTitle } from './sanitizeDocumentTitle';
test('removes all C0 control characters including tab/LF/CR', () => {
expect(sanitizeDocumentTitle('a\x08b')).toBe('ab');
expect(sanitizeDocumentTitle('x\x09y')).toBe('xy');
expect(sanitizeDocumentTitle('x\ny')).toBe('xy');
expect(sanitizeDocumentTitle('x\ry')).toBe('xy');
});
test('removes DEL and C1 controls', () => {
expect(sanitizeDocumentTitle('a\x7Fb')).toBe('ab');
expect(sanitizeDocumentTitle('a\x9Fb')).toBe('ab');
});
test('leaves normal text unchanged', () => {
expect(sanitizeDocumentTitle('Dashboard 你好')).toBe('Dashboard 你好');
});

View File

@@ -0,0 +1,27 @@
/**
* 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.
*/
/**
* Strip all C0/C1 control characters (U+0000U+001F, U+007FU+009F).
* Headless browsers (Playwright/Chromium) can hang or crash when document.title
* contains characters such as U+0008 (backspace).
*/
export function sanitizeDocumentTitle(title: string): string {
return title.replace(/[\x00-\x1F\x7F-\x9F]/g, '');
}

View File

@@ -16,6 +16,7 @@
# under the License.
import math
from typing import Optional
from flask_babel import lazy_gettext as _
@@ -200,6 +201,26 @@ class ReportScheduleDataFrameFailedError(CommandException):
message = _("Report Schedule execution failed when generating a dataframe.")
class ReportScheduleExecutorNotFoundError(CommandException):
"""Raised when the configured report executor user cannot be resolved."""
# 5xx (not 4xx): a missing executor user is a server-side misconfiguration,
# not a malformed client request. The status drives get_logger_from_status,
# which marks the Celery task FAILURE for 5xx — keeping the executor problem
# visible to ops task-state alerting. A 4xx would log a WARNING and leave the
# task non-FAILURE, hiding the misconfiguration from that signal.
status = 500
def __init__(self, username: str = "", exception: Optional[Exception] = None):
super().__init__(
_(
"Report Schedule executor user %(username)s was not found.",
username=f'"{username}"' if username else "(unknown)",
),
exception,
)
class ReportScheduleExecuteUnexpectedError(CommandException):
message = _("Report Schedule execution got an unexpected error.")

View File

@@ -17,7 +17,7 @@
import logging
from collections.abc import Sequence
from datetime import datetime, timedelta
from typing import Any, Optional, Union
from typing import Any, Optional, TYPE_CHECKING, Union
from uuid import UUID
import pandas as pd
@@ -37,6 +37,7 @@ from superset.commands.report.exceptions import (
ReportScheduleDataFrameFailedError,
ReportScheduleDataFrameTimeout,
ReportScheduleExecuteUnexpectedError,
ReportScheduleExecutorNotFoundError,
ReportScheduleNotFoundError,
ReportSchedulePreviousWorkingError,
ReportScheduleScreenshotFailedError,
@@ -77,14 +78,42 @@ from superset.utils import json
from superset.utils.core import HeaderDataType, override_user, recipients_string_to_list
from superset.utils.csv import get_chart_csv_data, get_chart_dataframe
from superset.utils.decorators import logs_context, transaction
from superset.utils.file import sanitize_title
from superset.utils.pdf import build_pdf_from_screenshots
from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot
from superset.utils.slack import get_channels_with_search, SlackChannelTypes
from superset.utils.urls import get_url_path
if TYPE_CHECKING:
from flask_appbuilder.security.sqla.models import User
logger = logging.getLogger(__name__)
def resolve_executor_user(model: ReportSchedule) -> tuple["User", str]:
"""
Resolve the executor user for a report schedule.
Determines the configured executor username via ``get_executor`` and looks up
the corresponding user. A deleted/disabled user or a misconfigured
``ALERT_REPORTS_EXECUTORS`` makes ``security_manager.find_user`` return
``None``; rather than passing ``None`` into the webdriver/auth flow (which
fails with an opaque NoneType error), raise a dedicated, actionable error.
:returns: the ``(user, username)`` pair — the username is returned alongside
the user because several call sites log it after resolution.
:raises ReportScheduleExecutorNotFoundError: if the executor user is missing.
"""
_, username = get_executor(
executors=app.config["ALERT_REPORTS_EXECUTORS"],
model=model,
)
user = security_manager.find_user(username)
if user is None:
raise ReportScheduleExecutorNotFoundError(username)
return user, username
class BaseReportState:
current_states: list[ReportState] = []
initial: bool = False
@@ -135,9 +164,17 @@ class BaseReportState:
Update the report schedule type and channels for all slack recipients to v2.
V2 uses ids instead of names for channels.
"""
# Track each recipient mutated in this pass with its original (type,
# config) so a partial failure can revert ALL of them — not just the
# loop variable. Restoring the in-memory values to their loaded state
# keeps a later commit from persisting a half-migrated set.
mutated: list[tuple[ReportRecipients, ReportRecipientType, str]] = []
try:
for recipient in self._report_schedule.recipients:
if recipient.type == ReportRecipientType.SLACK:
mutated.append(
(recipient, recipient.type, recipient.recipient_config_json)
)
recipient.type = ReportRecipientType.SLACKV2
slack_recipients = json.loads(recipient.recipient_config_json)
# V1 method allowed to use leading `#` in the channel name
@@ -169,8 +206,15 @@ class BaseReportState:
}
)
except Exception as ex:
# Revert to v1 to preserve configuration (requires manual fix)
recipient.type = ReportRecipientType.SLACK
# Revert every mutated recipient to v1 (both type AND config) to
# preserve configuration (requires manual fix). Reverting the full
# set — not just the loop variable — keeps earlier recipients
# consistent; iterating the snapshot also avoids the UnboundLocalError
# that a bare loop-variable reference raises on a pre-iteration
# failure (which would mask the real error).
for reverted_recipient, original_type, original_config in mutated:
reverted_recipient.type = original_type
reverted_recipient.recipient_config_json = original_config
msg = f"Failed to update slack recipients to v2: {str(ex)}"
logger.exception(msg)
raise UpdateFailedError(msg) from ex
@@ -422,11 +466,7 @@ class BaseReportState:
"""
start_time = datetime.utcnow()
_, username = get_executor(
executors=app.config["ALERT_REPORTS_EXECUTORS"],
model=self._report_schedule,
)
user = security_manager.find_user(username)
user, _ = resolve_executor_user(self._report_schedule)
max_width = app.config["ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH"]
@@ -512,11 +552,7 @@ class BaseReportState:
def _get_csv_data(self) -> bytes:
start_time = datetime.utcnow()
url = self._get_url(result_format=ChartDataResultFormat.CSV)
_, username = get_executor(
executors=app.config["ALERT_REPORTS_EXECUTORS"],
model=self._report_schedule,
)
user = security_manager.find_user(username)
user, username = resolve_executor_user(self._report_schedule)
auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user)
if self._report_schedule.chart.query_context is None:
@@ -566,11 +602,7 @@ class BaseReportState:
start_time = datetime.utcnow()
url = self._get_url(result_format=ChartDataResultFormat.JSON)
_, username = get_executor(
executors=app.config["ALERT_REPORTS_EXECUTORS"],
model=self._report_schedule,
)
user = security_manager.find_user(username)
user, username = resolve_executor_user(self._report_schedule)
auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user)
if self._report_schedule.chart.query_context is None:
@@ -701,7 +733,7 @@ class BaseReportState:
error_text = "Unexpected missing csv file"
if error_text:
return NotificationContent(
name=self._report_schedule.name,
name=sanitize_title(self._report_schedule.name),
text=error_text,
header_data=header_data,
url=url,
@@ -714,15 +746,15 @@ class BaseReportState:
embedded_data = self._get_embedded_data()
if self._report_schedule.email_subject:
name = self._report_schedule.email_subject
name = sanitize_title(self._report_schedule.email_subject)
else:
if self._report_schedule.chart:
name = (
name = sanitize_title(
f"{self._report_schedule.name}: "
f"{self._report_schedule.chart.slice_name}"
)
else:
name = (
name = sanitize_title(
f"{self._report_schedule.name}: "
f"{self._report_schedule.dashboard.dashboard_title}"
)
@@ -821,7 +853,7 @@ class BaseReportState:
self._execution_id,
)
notification_content = NotificationContent(
name=name, text=message, header_data=header_data, url=url
name=sanitize_title(name), text=message, header_data=header_data, url=url
)
# filter recipients to recipients who are also owners
@@ -1175,6 +1207,18 @@ class AsyncExecuteReportScheduleCommand(BaseCommand):
if not self._model:
raise ReportScheduleExecuteUnexpectedError()
# Resolve the executor at the run() boundary the same way master
# does: tolerate a missing user (find_user -> None) so the state
# machine still runs and its error envelope writes the ERROR
# execution-log row and sends the owner notification. The dedicated
# ReportScheduleExecutorNotFoundError guard lives at the content
# sites (_get_screenshots / _get_csv_data / _get_embedded_data),
# which raise inside that envelope. Guarding here instead would
# surface the executor error above the state machine, suppressing
# both the log row and the owner notification. The alert-query path
# (AlertCommand) is intentionally left on master behavior — a
# missing executor there surfaces as a query error, not the
# dedicated executor error; tightening it is out of scope here.
_, username = get_executor(
executors=app.config["ALERT_REPORTS_EXECUTORS"],
model=self._model,

View File

@@ -1979,7 +1979,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
if offset_metrics_df.empty:
offset_metrics_df = pd.DataFrame(
{
col: [np.NaN]
col: [np.nan]
for col in join_keys + list(metrics_mapping.values())
}
)

View File

@@ -128,7 +128,21 @@ class WebhookNotification(BaseNotification):
raise NotificationParamException("Webhook URL target host is not allowed.")
@backoff.on_exception(
backoff.expo, NotificationUnprocessableException, factor=10, base=2, max_tries=5
backoff.expo,
NotificationUnprocessableException,
factor=10,
base=2,
max_tries=5,
# Bound total wall-clock retry time. Without this, a hanging or
# persistently failing target can stall a worker for minutes per bad URL
# (up to ~5 socket waits at timeout=60 plus ~150s of retry sleeps),
# starving sequential report dispatch. backoff evaluates max_time against
# the elapsed time sampled BEFORE each attempt, so the last request can
# start just under the bound and still run its full timeout: worst case
# ~210s (~150s elapsed at the final pre-attempt check + one 60s request),
# not 120s. factor is intentionally kept at 10 so legitimately-transient
# 5xx targets are not abandoned early.
max_time=120,
)
@statsd_gauge("reports.webhook.send")
def send(self) -> None:

View File

@@ -14,10 +14,23 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
import re
from werkzeug.utils import secure_filename
# All C0 (U+0000U+001F) and C1 (U+007FU+009F) control characters.
# Stripping every control char (including tab, LF, CR) keeps titles safe for
# SMTP headers, Content-Disposition filenames, and headless-browser document.title.
_CONTROL_CHARS_RE = re.compile(r"[\x00-\x1f\x7f-\x9f]")
def sanitize_title(title: str) -> str:
"""Remove all C0/C1 control characters from a title string."""
return _CONTROL_CHARS_RE.sub("", title)
def get_filename(model_name: str, model_id: int, skip_id: bool = False) -> str:
model_name = sanitize_title(model_name)
slug = secure_filename(model_name)
filename = slug if skip_id else f"{slug}_{model_id}"
return filename if slug else str(model_id)

View File

@@ -71,13 +71,21 @@ def _prophet_fit_and_predict( # pylint: disable=too-many-arguments
)
if df["ds"].dt.tz:
df["ds"] = df["ds"].dt.tz_convert(None)
model.fit(df)
future = model.make_future_dataframe(periods=periods, freq=freq)
forecast = model.predict(future)[["ds", "yhat", "yhat_lower", "yhat_upper"]]
try:
model.fit(df)
future = model.make_future_dataframe(periods=periods, freq=freq)
forecast = model.predict(future)[["ds", "yhat", "yhat_lower", "yhat_upper"]]
except Exception as ex: # noqa: BLE001
raise InvalidPostProcessingError(
_(
"Unable to generate forecast: %(error)s",
error=str(ex),
)
) from ex
return forecast.join(df.set_index("ds"), on="ds").set_index(["ds"])
def prophet( # pylint: disable=too-many-arguments
def prophet( # pylint: disable=too-many-arguments # noqa: C901
df: DataFrame,
time_grain: str,
periods: int,
@@ -136,6 +144,8 @@ def prophet( # pylint: disable=too-many-arguments
raise InvalidPostProcessingError(_("DataFrame must include temporal column"))
if len(df.columns) < 2:
raise InvalidPostProcessingError(_("DataFrame include at least one series"))
if len(df) < 2:
raise InvalidPostProcessingError(_("Forecast requires at least 2 data points"))
target_df = DataFrame()

View File

@@ -26,6 +26,8 @@ from pandas import DataFrame, NamedAgg
from superset.constants import TimeGrain
from superset.exceptions import InvalidPostProcessingError
_PANDAS_VERSION = tuple(int(x) for x in pd.__version__.split(".")[:2])
NUMPY_FUNCTIONS: dict[str, Callable[..., Any]] = {
"average": np.average,
"argmin": np.argmin,
@@ -76,18 +78,18 @@ ALLOWLIST_CUMULATIVE_FUNCTIONS = (
)
PROPHET_TIME_GRAIN_MAP: dict[str, str] = {
TimeGrain.SECOND: "S",
TimeGrain.SECOND: "s",
TimeGrain.MINUTE: "min",
TimeGrain.FIVE_MINUTES: "5min",
TimeGrain.TEN_MINUTES: "10min",
TimeGrain.FIFTEEN_MINUTES: "15min",
TimeGrain.THIRTY_MINUTES: "30min",
TimeGrain.HOUR: "H",
TimeGrain.HOUR: "h",
TimeGrain.DAY: "D",
TimeGrain.WEEK: "W",
TimeGrain.MONTH: "M",
TimeGrain.QUARTER: "Q",
TimeGrain.YEAR: "A",
TimeGrain.MONTH: "ME" if _PANDAS_VERSION >= (2, 2) else "M",
TimeGrain.QUARTER: "QE" if _PANDAS_VERSION >= (2, 2) else "Q",
TimeGrain.YEAR: "YE" if _PANDAS_VERSION >= (2, 2) else "A",
TimeGrain.WEEK_STARTING_SUNDAY: "W-SUN",
TimeGrain.WEEK_STARTING_MONDAY: "W-MON",
TimeGrain.WEEK_ENDING_SATURDAY: "W-SAT",

View File

@@ -28,6 +28,7 @@ from superset.commands.exceptions import UpdateFailedError
from superset.commands.report.exceptions import (
ReportScheduleAlertGracePeriodError,
ReportScheduleCsvFailedError,
ReportScheduleExecutorNotFoundError,
ReportSchedulePreviousWorkingError,
ReportScheduleScreenshotFailedError,
ReportScheduleScreenshotTimeout,
@@ -1096,6 +1097,103 @@ def test_screenshot_width_calculation(
)
def _executor_report_state(mocker: MockerFixture) -> BaseReportState:
report_schedule = create_report_schedule(mocker)
# _get_csv_data/_get_embedded_data build a chart-data URL from chart_id
# before resolving the executor; give it a concrete value so URL building
# succeeds and the executor resolution is actually reached.
report_schedule.chart_id = 1
report_schedule.force_screenshot = False
return BaseReportState(
report_schedule=report_schedule,
scheduled_dttm=datetime.now(),
execution_id=UUID("084e7ee6-5557-4ecd-9632-b7f39c9ec524"),
)
@pytest.mark.parametrize(
"method_name",
["_get_screenshots", "_get_csv_data", "_get_embedded_data"],
)
def test_get_content_raises_when_executor_user_missing(
app: SupersetApp, mocker: MockerFixture, method_name: str
) -> None:
"""
When the configured executor user cannot be resolved
(``security_manager.find_user`` returns ``None``), each content path raises a
dedicated ``ReportScheduleExecutorNotFoundError`` naming the username at the
resolution boundary, rather than passing ``None`` further into the
webdriver/auth flow.
"""
app.config.update(
{
"ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH": 1600,
"WEBDRIVER_WINDOW": {"slice": (800, 600), "dashboard": (800, 600)},
"ALERT_REPORTS_EXECUTORS": {},
}
)
report_state = _executor_report_state(mocker)
with (
patch("superset.commands.report.execute.security_manager") as mock_sm,
patch("superset.commands.report.execute.get_executor") as mock_get_executor,
patch("superset.commands.report.execute.machine_auth_provider_factory"),
):
mock_get_executor.return_value = ("executor", "ghost_user")
mock_sm.find_user = mocker.MagicMock(return_value=None)
with pytest.raises(ReportScheduleExecutorNotFoundError, match="ghost_user"):
getattr(report_state, method_name)()
def test_executor_not_found_error_message_without_username() -> None:
"""
When no username is available, the message falls back to ``(unknown)``
rather than leaving a double space ("...executor user was not found.").
"""
message = str(ReportScheduleExecutorNotFoundError().message)
assert "(unknown)" in message
assert "user was" not in message
def test_executor_not_found_error_status_is_server_error() -> None:
"""
The executor-not-found error is a 5xx so ``get_logger_from_status`` marks the
Celery task ``FAILURE`` (a missing executor is a server-side misconfiguration
that ops task-state alerting must still see), not a 4xx that would log a
``WARNING`` and leave the task non-``FAILURE``.
"""
assert ReportScheduleExecutorNotFoundError().status == 500
def test_resolve_executor_user_returns_user_and_username(
app: SupersetApp, mocker: MockerFixture
) -> None:
"""
Happy path: when the executor user exists, the helper returns the
``(user, username)`` tuple unchanged — locking the no-behavior-change exit
criterion for the three call sites.
"""
from superset.commands.report.execute import resolve_executor_user
app.config.update({"ALERT_REPORTS_EXECUTORS": {}})
report_schedule = create_report_schedule(mocker)
mock_user = mocker.MagicMock()
with (
patch("superset.commands.report.execute.security_manager") as mock_sm,
patch("superset.commands.report.execute.get_executor") as mock_get_executor,
):
mock_get_executor.return_value = ("executor", "real_user")
mock_sm.find_user = mocker.MagicMock(return_value=mock_user)
user, username = resolve_executor_user(report_schedule)
assert user is mock_user
assert username == "real_user"
def test_update_recipient_to_slack_v2(mocker: MockerFixture):
"""
Test converting a Slack recipient to Slack v2 format.
@@ -1171,6 +1269,122 @@ def test_update_recipient_to_slack_v2_missing_channels(mocker: MockerFixture):
mock_cmmd.update_report_schedule_slack_v2()
def test_update_recipient_to_slack_v2_reverts_all_on_partial_failure(
mocker: MockerFixture,
) -> None:
"""
When the second of two Slack recipients fails channel resolution, BOTH
recipients are fully reverted — type AND exact original
``recipient_config_json`` string — not just the loop variable's type. This
prevents the intervening ``create_log`` commit from flushing a half-migrated,
inconsistent state.
"""
def channels_side_effect(search_string, types, exact_match):
if search_string == "Channel-1":
return [
{
"id": "id_channel_1",
"name": "Channel-1",
"is_member": True,
"is_private": False,
}
]
# Second recipient: no channel found → length mismatch → UpdateFailedError
return []
mocker.patch(
"superset.commands.report.execute.get_channels_with_search",
side_effect=channels_side_effect,
)
original_config_1 = json.dumps({"target": "Channel-1"})
original_config_2 = json.dumps({"target": "Channel-2"})
mock_report_schedule = ReportSchedule(
name="Test Report",
recipients=[
ReportRecipients(
type=ReportRecipientType.SLACK,
recipient_config_json=original_config_1,
),
ReportRecipients(
type=ReportRecipientType.SLACK,
recipient_config_json=original_config_2,
),
],
)
mock_cmmd = BaseReportState(
mock_report_schedule, "January 1, 2021", "execution_id_example"
)
with pytest.raises(UpdateFailedError):
mock_cmmd.update_report_schedule_slack_v2()
first, second = mock_report_schedule.recipients
# The first recipient was mutated to v2 before the second failed; it must be
# reverted to its exact original type AND config string.
assert first.type == ReportRecipientType.SLACK
assert first.recipient_config_json == original_config_1
assert second.type == ReportRecipientType.SLACK
assert second.recipient_config_json == original_config_2
def test_update_recipient_to_slack_v2_pre_iteration_failure(
mocker: MockerFixture,
) -> None:
"""
A failure raised while accessing/iterating the recipients (before the loop
variable is bound) surfaces as ``UpdateFailedError``, not a ``NameError``
that would mask the real error.
"""
class _ExplodingRecipients:
def __iter__(self):
raise RuntimeError("recipients exploded")
mock_report_schedule = mocker.MagicMock()
mock_report_schedule.recipients = _ExplodingRecipients()
mock_cmmd = BaseReportState(
mock_report_schedule, "January 1, 2021", "execution_id_example"
)
with pytest.raises(UpdateFailedError):
mock_cmmd.update_report_schedule_slack_v2()
def test_update_recipient_to_slack_v2_no_slack_recipients_is_noop(
mocker: MockerFixture,
) -> None:
"""
With no SLACK recipients there is nothing to migrate: the method returns
without raising and leaves the non-Slack recipients untouched.
"""
mock_search = mocker.patch(
"superset.commands.report.execute.get_channels_with_search",
)
mock_report_schedule = ReportSchedule(
recipients=[
ReportRecipients(
type=ReportRecipientType.EMAIL,
recipient_config_json=json.dumps({"target": "user@example.com"}),
),
],
)
mock_cmmd: BaseReportState = BaseReportState(
mock_report_schedule, "January 1, 2021", "execution_id_example"
)
mock_cmmd.update_report_schedule_slack_v2()
assert mock_cmmd._report_schedule.recipients[0].type == ReportRecipientType.EMAIL
assert (
mock_cmmd._report_schedule.recipients[0].recipient_config_json
== '{"target": "user@example.com"}'
)
mock_search.assert_not_called()
# ---------------------------------------------------------------------------
# Tier 1: _update_query_context + create_log
# ---------------------------------------------------------------------------

View File

@@ -16,6 +16,7 @@
# under the License.
from datetime import datetime
from importlib.util import find_spec
from unittest.mock import patch
import pandas as pd
import pytest
@@ -186,6 +187,43 @@ def test_prophet_incorrect_time_grain():
)
def test_prophet_insufficient_data():
single_row_df = pd.DataFrame(
{
DTTM_ALIAS: [datetime(2022, 1, 1)],
"sales": [100.0],
}
)
with pytest.raises(InvalidPostProcessingError, match="at least 2 data points"):
prophet(
df=single_row_df,
time_grain="P1D",
periods=3,
confidence_interval=0.9,
)
def test_prophet_fit_error():
if find_spec("prophet") is None:
pytest.skip("prophet not installed")
with patch(
"superset.utils.pandas_postprocessing.prophet._prophet_fit_and_predict"
) as mock_fit:
mock_fit.side_effect = InvalidPostProcessingError(
"Unable to generate forecast: Dataframe has fewer than 2 non-NaN rows."
)
with pytest.raises(
InvalidPostProcessingError, match="Unable to generate forecast"
):
prophet(
df=prophet_df,
time_grain="P1D",
periods=3,
confidence_interval=0.9,
)
def test_prophet_uncertainty_lower_bound_can_be_negative_for_negative_series():
"""
Regression for #21734: when the input series contains negative values,

View File

@@ -18,9 +18,11 @@
import pandas as pd
import pytest
from freezegun import freeze_time
from superset.reports.notifications.exceptions import (
NotificationParamException,
NotificationUnprocessableException,
)
from superset.reports.notifications.webhook import WebhookNotification
from superset.utils.core import HeaderDataType
@@ -269,3 +271,156 @@ def test_send_treats_redirect_as_failure(monkeypatch, mock_header_data) -> None:
with pytest.raises(NotificationParamException, match="redirect"):
webhook_notification.send()
def _make_webhook(mock_header_data) -> WebhookNotification:
from superset.reports.models import ReportRecipients, ReportRecipientType
from superset.reports.notifications.base import NotificationContent
content = NotificationContent(
name="test alert", header_data=mock_header_data, description="Test description"
)
return WebhookNotification(
recipient=ReportRecipients(
type=ReportRecipientType.WEBHOOK,
recipient_config_json='{"target": "https://example.com/webhook"}',
),
content=content,
)
class _MockServerErrorResponse:
status_code = 500
text = ""
def _allow_internal_app() -> type:
class MockCurrentApp:
config = {
"ALERT_REPORTS_WEBHOOK_HTTPS_ONLY": True,
"ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS": True,
}
return MockCurrentApp
def test_send_backoff_bounded_by_max_time(monkeypatch, mock_header_data) -> None:
"""
A persistently failing (500) target gives up on wall-time (``max_time``),
not just ``max_tries``. ``freeze_time(auto_tick_seconds=30)`` advances the
clock on every time access (backoff measures elapsed via ``datetime.now``,
which freezegun freezes and ticks), so cumulative elapsed crosses ``max_time=120``
before ``max_tries=5`` is exhausted. We assert the discriminating *property*
— gave up on wall-time strictly before exhausting ``max_tries`` — rather than
a pinned count, because freezegun's per-call tick count is an opaque
implementation detail. If ``max_time`` is removed this rises to the full 5
(RED); the flat-clock companion test below anchors that 5 is the ceiling.
The terminal exception type is unchanged on giveup.
"""
webhook_notification = _make_webhook(mock_header_data)
post_calls: list[int] = []
def fake_post(*args, **kwargs) -> _MockServerErrorResponse:
post_calls.append(1)
return _MockServerErrorResponse()
monkeypatch.setattr(
"superset.reports.notifications.webhook.current_app", _allow_internal_app()
)
monkeypatch.setattr(
"superset.reports.notifications.webhook.feature_flag_manager.is_feature_enabled",
lambda flag: True,
)
monkeypatch.setattr(
"superset.reports.notifications.webhook.requests.post", fake_post
)
monkeypatch.setattr("backoff._sync.time.sleep", lambda *a, **k: None)
with freeze_time("2020-01-01", auto_tick_seconds=30):
with pytest.raises(NotificationUnprocessableException):
webhook_notification.send()
# 1 < count < max_tries(5): wall-time bound fired before tries exhausted.
assert 1 < len(post_calls) < 5
def test_send_flat_clock_falls_back_to_max_tries(monkeypatch, mock_header_data) -> None:
"""
Characterization (NOT a RED discriminator): with the clock held flat,
``max_time`` can never fire, so ``max_tries=5`` governs and exactly 5 POSTs
happen. Passes on both buggy and fixed code; its job is to prove the 3-vs-5
delta in ``test_send_backoff_bounded_by_max_time`` is attributable to
wall-time, not to ``max_tries``.
"""
webhook_notification = _make_webhook(mock_header_data)
post_calls: list[int] = []
def fake_post(*args, **kwargs) -> _MockServerErrorResponse:
post_calls.append(1)
return _MockServerErrorResponse()
monkeypatch.setattr(
"superset.reports.notifications.webhook.current_app", _allow_internal_app()
)
monkeypatch.setattr(
"superset.reports.notifications.webhook.feature_flag_manager.is_feature_enabled",
lambda flag: True,
)
monkeypatch.setattr(
"superset.reports.notifications.webhook.requests.post", fake_post
)
monkeypatch.setattr("backoff._sync.time.sleep", lambda *a, **k: None)
with freeze_time("2020-01-01"):
with pytest.raises(NotificationUnprocessableException):
webhook_notification.send()
assert len(post_calls) == 5
def test_send_max_time_does_not_abandon_recovering_target(
monkeypatch, mock_header_data
) -> None:
"""
No-regression guard: a target that fails twice (500) then succeeds on the
3rd attempt still succeeds, and ``max_time=120`` is not set so low it
abandons recovery. The clock advances a small +5s per access so ``max_time``
is genuinely engaged (cumulative elapsed across 3 attempts stays comfortably
under 120), unlike a flat clock which would pin elapsed at 0 and let the test
pass at any ``max_time`` — including a misconfigured ``max_time=1``. Lower
``max_time`` enough and this test correctly fails.
"""
webhook_notification = _make_webhook(mock_header_data)
post_calls: list[int] = []
class _OkResponse:
status_code = 200
text = ""
def fake_post(*args, **kwargs):
post_calls.append(1)
if len(post_calls) < 3:
return _MockServerErrorResponse()
return _OkResponse()
monkeypatch.setattr(
"superset.reports.notifications.webhook.current_app", _allow_internal_app()
)
monkeypatch.setattr(
"superset.reports.notifications.webhook.feature_flag_manager.is_feature_enabled",
lambda flag: True,
)
monkeypatch.setattr(
"superset.reports.notifications.webhook.requests.post", fake_post
)
monkeypatch.setattr("backoff._sync.time.sleep", lambda *a, **k: None)
with freeze_time("2020-01-01", auto_tick_seconds=5):
webhook_notification.send()
# The clock advances (+5s/access) so ``max_time`` is genuinely engaged, yet
# cumulative elapsed across 3 attempts stays well under 120: the recovering
# target (``fake_post`` succeeds on the 3rd) is not abandoned. A flat clock
# would pin elapsed at 0 and pass at any ``max_time``, including a broken
# ``max_time=1`` — this non-zero tick keeps the guard real.
assert len(post_calls) == 3

View File

@@ -16,7 +16,7 @@
# under the License.
import pytest
from superset.utils.file import get_filename
from superset.utils.file import get_filename, sanitize_title
@pytest.mark.parametrize(
@@ -34,6 +34,9 @@ from superset.utils.file import get_filename
("你好", 475, True, "475"),
("Energy Sankey 你好", 475, False, "Energy_Sankey_475"),
("Energy Sankey 你好", 475, True, "Energy_Sankey"),
("Energy\x08Sankey", 132, False, "EnergySankey_132"),
("Energy\x08Sankey", 132, True, "EnergySankey"),
("Sales\x7fReport", 1, False, "SalesReport_1"),
],
)
def test_get_filename(
@@ -41,3 +44,20 @@ def test_get_filename(
) -> None:
original_filename = get_filename(model_name, model_id, skip_id)
assert expected_filename == original_filename
@pytest.mark.parametrize(
("raw", "expected"),
[
("normal", "normal"),
("a\x08b", "ab"),
("x\x09y", "xy"),
("x\ny", "xy"),
("x\ry", "xy"),
("\x00\x01\x02", ""),
("a\x7fb", "ab"),
("a\x9fb", "ab"),
],
)
def test_sanitize_title(raw: str, expected: str) -> None:
assert sanitize_title(raw) == expected