Compare commits

..

1 Commits

Author SHA1 Message Date
amaannawab923
348d924c92 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.
2026-06-24 17:34:48 +05:30
34 changed files with 245 additions and 899 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. 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.
Superset automatically retries webhook deliveries on `429 Too Many Requests` and `5xx` server errors using exponential backoff.
### Kubernetes-specific

View File

@@ -90,13 +90,6 @@ 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');
@@ -164,14 +157,15 @@ const buildQuery: BuildQuery<TableChartFormData> = (
metrics.concat(percentMetrics),
getMetricLabel,
);
contributionPostProcessing = {
operation: 'contribution',
options: {
columns: percentMetricLabels,
rename_columns: percentMetricLabels.map(x => `%${x}`),
postProcessing = [
{
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)) {
@@ -664,13 +658,7 @@ const buildQuery: BuildQuery<TableChartFormData> = (
extras: totalsExtras, // Use extras with AG Grid WHERE removed
row_limit: 0,
row_offset: 0,
// 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]
: [],
post_processing: [],
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,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__';

View File

@@ -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 =

View File

@@ -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) ?? [];

View File

@@ -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 =

View File

@@ -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];
}

View File

@@ -852,75 +852,6 @@ 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

@@ -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']);
});

View File

@@ -86,13 +86,6 @@ 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');
@@ -144,6 +137,12 @@ 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,
@@ -163,14 +162,23 @@ export const buildQuery: BuildQuery<TableChartFormData> = (
getMetricLabel,
);
contributionPostProcessing = {
operation: 'contribution',
options: {
columns: percentMetricLabels,
rename_columns: percentMetricLabels.map(m => `%${m}`),
},
};
postProcessing.push(contributionPostProcessing);
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}`),
},
});
}
}
// Add the operator for the time comparison if some is selected
@@ -349,13 +357,7 @@ export const buildQuery: BuildQuery<TableChartFormData> = (
columns: [],
row_limit: 0,
row_offset: 0,
// 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]
: [],
post_processing: [],
order_desc: undefined,
orderby: undefined,
});

View File

@@ -236,83 +236,6 @@ 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,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,
};

View File

@@ -42,7 +42,6 @@ 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 {
@@ -338,7 +337,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
// Update document title when dashboard title changes
useEffect(() => {
if (pageTitle) {
document.title = sanitizeDocumentTitle(pageTitle);
document.title = pageTitle;
}
}, [pageTitle]);

View File

@@ -66,7 +66,6 @@ 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';
@@ -398,7 +397,7 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
// Update document title when slice name changes
useEffect(() => {
if (props.sliceName) {
document.title = sanitizeDocumentTitle(props.sliceName);
document.title = 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

@@ -1,35 +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 { 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

@@ -1,27 +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.
*/
/**
* 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,7 +16,6 @@
# under the License.
import math
from typing import Optional
from flask_babel import lazy_gettext as _
@@ -201,26 +200,6 @@ 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, TYPE_CHECKING, Union
from typing import Any, Optional, Union
from uuid import UUID
import pandas as pd
@@ -37,7 +37,6 @@ from superset.commands.report.exceptions import (
ReportScheduleDataFrameFailedError,
ReportScheduleDataFrameTimeout,
ReportScheduleExecuteUnexpectedError,
ReportScheduleExecutorNotFoundError,
ReportScheduleNotFoundError,
ReportSchedulePreviousWorkingError,
ReportScheduleScreenshotFailedError,
@@ -78,42 +77,14 @@ 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
@@ -164,17 +135,9 @@ 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
@@ -206,15 +169,8 @@ class BaseReportState:
}
)
except Exception as ex:
# 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
# Revert to v1 to preserve configuration (requires manual fix)
recipient.type = ReportRecipientType.SLACK
msg = f"Failed to update slack recipients to v2: {str(ex)}"
logger.exception(msg)
raise UpdateFailedError(msg) from ex
@@ -466,7 +422,11 @@ class BaseReportState:
"""
start_time = datetime.utcnow()
user, _ = resolve_executor_user(self._report_schedule)
_, username = get_executor(
executors=app.config["ALERT_REPORTS_EXECUTORS"],
model=self._report_schedule,
)
user = security_manager.find_user(username)
max_width = app.config["ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH"]
@@ -552,7 +512,11 @@ class BaseReportState:
def _get_csv_data(self) -> bytes:
start_time = datetime.utcnow()
url = self._get_url(result_format=ChartDataResultFormat.CSV)
user, username = resolve_executor_user(self._report_schedule)
_, username = get_executor(
executors=app.config["ALERT_REPORTS_EXECUTORS"],
model=self._report_schedule,
)
user = security_manager.find_user(username)
auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user)
if self._report_schedule.chart.query_context is None:
@@ -602,7 +566,11 @@ class BaseReportState:
start_time = datetime.utcnow()
url = self._get_url(result_format=ChartDataResultFormat.JSON)
user, username = resolve_executor_user(self._report_schedule)
_, username = get_executor(
executors=app.config["ALERT_REPORTS_EXECUTORS"],
model=self._report_schedule,
)
user = security_manager.find_user(username)
auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user)
if self._report_schedule.chart.query_context is None:
@@ -733,7 +701,7 @@ class BaseReportState:
error_text = "Unexpected missing csv file"
if error_text:
return NotificationContent(
name=sanitize_title(self._report_schedule.name),
name=self._report_schedule.name,
text=error_text,
header_data=header_data,
url=url,
@@ -746,15 +714,15 @@ class BaseReportState:
embedded_data = self._get_embedded_data()
if self._report_schedule.email_subject:
name = sanitize_title(self._report_schedule.email_subject)
name = self._report_schedule.email_subject
else:
if self._report_schedule.chart:
name = sanitize_title(
name = (
f"{self._report_schedule.name}: "
f"{self._report_schedule.chart.slice_name}"
)
else:
name = sanitize_title(
name = (
f"{self._report_schedule.name}: "
f"{self._report_schedule.dashboard.dashboard_title}"
)
@@ -853,7 +821,7 @@ class BaseReportState:
self._execution_id,
)
notification_content = NotificationContent(
name=sanitize_title(name), text=message, header_data=header_data, url=url
name=name, text=message, header_data=header_data, url=url
)
# filter recipients to recipients who are also owners
@@ -1207,18 +1175,6 @@ 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,21 +128,7 @@ 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,
# 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,
backoff.expo, NotificationUnprocessableException, factor=10, base=2, max_tries=5
)
@statsd_gauge("reports.webhook.send")
def send(self) -> None:

View File

@@ -14,23 +14,10 @@
# 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,21 +71,13 @@ def _prophet_fit_and_predict( # pylint: disable=too-many-arguments
)
if df["ds"].dt.tz:
df["ds"] = df["ds"].dt.tz_convert(None)
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
model.fit(df)
future = model.make_future_dataframe(periods=periods, freq=freq)
forecast = model.predict(future)[["ds", "yhat", "yhat_lower", "yhat_upper"]]
return forecast.join(df.set_index("ds"), on="ds").set_index(["ds"])
def prophet( # pylint: disable=too-many-arguments # noqa: C901
def prophet( # pylint: disable=too-many-arguments
df: DataFrame,
time_grain: str,
periods: int,
@@ -144,8 +136,6 @@ def prophet( # pylint: disable=too-many-arguments # noqa: C901
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,8 +26,6 @@ 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,
@@ -78,18 +76,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: "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.MONTH: "M",
TimeGrain.QUARTER: "Q",
TimeGrain.YEAR: "A",
TimeGrain.WEEK_STARTING_SUNDAY: "W-SUN",
TimeGrain.WEEK_STARTING_MONDAY: "W-MON",
TimeGrain.WEEK_ENDING_SATURDAY: "W-SAT",

View File

@@ -28,7 +28,6 @@ from superset.commands.exceptions import UpdateFailedError
from superset.commands.report.exceptions import (
ReportScheduleAlertGracePeriodError,
ReportScheduleCsvFailedError,
ReportScheduleExecutorNotFoundError,
ReportSchedulePreviousWorkingError,
ReportScheduleScreenshotFailedError,
ReportScheduleScreenshotTimeout,
@@ -1097,103 +1096,6 @@ 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.
@@ -1269,122 +1171,6 @@ 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,7 +16,6 @@
# under the License.
from datetime import datetime
from importlib.util import find_spec
from unittest.mock import patch
import pandas as pd
import pytest
@@ -187,43 +186,6 @@ 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,11 +18,9 @@
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
@@ -271,156 +269,3 @@ 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, sanitize_title
from superset.utils.file import get_filename
@pytest.mark.parametrize(
@@ -34,9 +34,6 @@ from superset.utils.file import get_filename, sanitize_title
("你好", 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(
@@ -44,20 +41,3 @@ 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