mirror of
https://github.com/apache/superset.git
synced 2026-08-27 18:41:20 +00:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
 geido
|
e8446b88a1 |
Vendored
-6
@@ -120,12 +120,6 @@
|
||||
"lifecycle": "testing",
|
||||
"description": "Enables filter functionality in Alerts and Reports"
|
||||
},
|
||||
{
|
||||
"name": "ALERT_REPORTS_RETRY",
|
||||
"default": false,
|
||||
"lifecycle": "testing",
|
||||
"description": "Enables automatic retry functionality for failed report executions"
|
||||
},
|
||||
{
|
||||
"name": "ALERT_REPORT_SLACK_V2",
|
||||
"default": true,
|
||||
|
||||
+3
-4
@@ -94,7 +94,7 @@ dependencies = [
|
||||
"parsedatetime",
|
||||
"paramiko>=3.4.0, <4.0", # 4.0 removed DSSKey, still referenced by sshtunnel
|
||||
"pgsanity",
|
||||
"Pillow>=12.3.0, <13", # raise floor to match resolved pin; closes SCA false-positive on 11.x-range CVEs already fixed in 12.3.0
|
||||
"Pillow>=11.0.0, <13",
|
||||
"polyline>=2.0.4, <3.0",
|
||||
"pydantic>=2.8.0",
|
||||
"pyparsing>=3.3.2, <4",
|
||||
@@ -103,7 +103,7 @@ dependencies = [
|
||||
"pygeohash",
|
||||
"pyarrow>=25.0.1, <26", # before upgrading pyarrow, check that all db dependencies support this, see e.g. https://github.com/apache/superset/pull/34693
|
||||
"pyyaml>=6.0.3, <7.0.0",
|
||||
"PyJWT>=2.13.0, <3.0", # raise floor to match resolved pin; closes SCA false-positive on 2.4.x-range CVEs already fixed in 2.13.0
|
||||
"PyJWT>=2.4.0, <3.0",
|
||||
"redis>=5.0.0, <9.0",
|
||||
"rison>=2.0.1, <3.0",
|
||||
|
||||
@@ -185,8 +185,7 @@ excel = ["xlrd>=2.0.2, <2.1"]
|
||||
# installing this extra is only required to actually run exports.
|
||||
excel-export = ["boto3"]
|
||||
fastmcp = [
|
||||
"fastmcp>=3.4.7,<4.0",
|
||||
"mcp>=1.29.1,<2.0",
|
||||
"fastmcp>=3.4.6,<4.0",
|
||||
# tiktoken backs the response-size-guard token estimator. Without
|
||||
# it, the middleware falls back to a coarser character-based
|
||||
# heuristic that under-counts JSON-heavy MCP responses.
|
||||
|
||||
@@ -547,10 +547,8 @@ matplotlib==3.9.0
|
||||
# via prophet
|
||||
mccabe==0.7.0
|
||||
# via pylint
|
||||
mcp==1.29.1
|
||||
# via
|
||||
# apache-superset
|
||||
# fastmcp-slim
|
||||
mcp==1.24.0
|
||||
# via fastmcp-slim
|
||||
mdurl==0.1.2
|
||||
# via
|
||||
# -c requirements/base-constraint.txt
|
||||
|
||||
@@ -113,7 +113,7 @@
|
||||
// === Import plugin rules ===
|
||||
"import/named": "error",
|
||||
"import/export": "error",
|
||||
"import/no-named-as-default": "warn",
|
||||
"import/no-named-as-default": "error",
|
||||
"import/no-named-as-default-member": "error",
|
||||
"import/no-mutable-exports": "error",
|
||||
"import/no-amd": "error",
|
||||
|
||||
Generated
+51
-17
@@ -100,7 +100,7 @@
|
||||
"geostyler-style": "11.0.2",
|
||||
"geostyler-wfs-parser": "^3.0.1",
|
||||
"google-auth-library": "^11.0.2",
|
||||
"immer": "^11.1.18",
|
||||
"immer": "^11.1.17",
|
||||
"interweave": "^13.1.1",
|
||||
"jquery": "^4.0.0",
|
||||
"js-levenshtein": "^1.1.6",
|
||||
@@ -249,7 +249,7 @@
|
||||
"jest-websocket-mock": "^2.5.0",
|
||||
"js-yaml-loader": "^1.2.2",
|
||||
"jsdom": "^30.0.1",
|
||||
"lerna": "^10.0.1",
|
||||
"lerna": "^10.0.0",
|
||||
"lightningcss": "^1.33.0",
|
||||
"mini-css-extract-plugin": "^2.10.2",
|
||||
"minimizer-webpack-plugin": "^5.6.1",
|
||||
@@ -17528,6 +17528,23 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"node_modules/conventional-changelog/node_modules/conventional-commits-parser": {
|
||||
"version": "7.1.2",
|
||||
"resolved": "https://registry.npmjs.org/conventional-commits-parser/-/conventional-commits-parser-7.1.2.tgz",
|
||||
"integrity": "sha512-O+x4N2yH+ijvqWlIyTHsXTAP+algNWgGbjY2duCe8w2vUMvUB95cLRslCPfTMQyLAKlet3bhZTdu6ozn4M+QJQ==",
|
||||
"dev": true,
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
"@simple-libs/stream-utils": "^2.0.0",
|
||||
"argue-cli": "^3.1.0"
|
||||
},
|
||||
"bin": {
|
||||
"conventional-commits-parser": "dist/cli/index.js"
|
||||
},
|
||||
"engines": {
|
||||
"node": ">=22"
|
||||
}
|
||||
},
|
||||
"node_modules/conventional-commits-filter": {
|
||||
"version": "6.0.1",
|
||||
"resolved": "https://registry.npmjs.org/conventional-commits-filter/-/conventional-commits-filter-6.0.1.tgz",
|
||||
@@ -17539,9 +17556,9 @@
|
||||
}
|
||||
},
|
||||
"node_modules/conventional-commits-parser": {
|
||||
"version": "7.1.2",
|
||||
"resolved": "https://registry.npmjs.org/conventional-commits-parser/-/conventional-commits-parser-7.1.2.tgz",
|
||||
"integrity": "sha512-O+x4N2yH+ijvqWlIyTHsXTAP+algNWgGbjY2duCe8w2vUMvUB95cLRslCPfTMQyLAKlet3bhZTdu6ozn4M+QJQ==",
|
||||
"version": "7.1.0",
|
||||
"resolved": "https://registry.npmjs.org/conventional-commits-parser/-/conventional-commits-parser-7.1.0.tgz",
|
||||
"integrity": "sha512-DPp6hkUjvwIivxbkrTiLXeRswNv1A/4GFA2X6scXma0AMa9632V3TwxmrlkUIEtUktiM3Ln+RrSH2xlP3/jUTw==",
|
||||
"dev": true,
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
@@ -17602,6 +17619,23 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"node_modules/conventional-recommended-bump/node_modules/conventional-commits-parser": {
|
||||
"version": "7.1.2",
|
||||
"resolved": "https://registry.npmjs.org/conventional-commits-parser/-/conventional-commits-parser-7.1.2.tgz",
|
||||
"integrity": "sha512-O+x4N2yH+ijvqWlIyTHsXTAP+algNWgGbjY2duCe8w2vUMvUB95cLRslCPfTMQyLAKlet3bhZTdu6ozn4M+QJQ==",
|
||||
"dev": true,
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
"@simple-libs/stream-utils": "^2.0.0",
|
||||
"argue-cli": "^3.1.0"
|
||||
},
|
||||
"bin": {
|
||||
"conventional-commits-parser": "dist/cli/index.js"
|
||||
},
|
||||
"engines": {
|
||||
"node": ">=22"
|
||||
}
|
||||
},
|
||||
"node_modules/convert-source-map": {
|
||||
"version": "2.0.0",
|
||||
"resolved": "https://registry.npmjs.org/convert-source-map/-/convert-source-map-2.0.0.tgz",
|
||||
@@ -19106,9 +19140,9 @@
|
||||
}
|
||||
},
|
||||
"node_modules/dompurify": {
|
||||
"version": "3.4.14",
|
||||
"resolved": "https://registry.npmjs.org/dompurify/-/dompurify-3.4.14.tgz",
|
||||
"integrity": "sha512-dVoH9z+MY+C9IilgGCk3YfFqjLi3fChm2OiKJMzh6axrJ5qwxqWaZamgmHrpv22CN/KdbZJuGEGgfQoL00LTdg==",
|
||||
"version": "3.4.13",
|
||||
"resolved": "https://registry.npmjs.org/dompurify/-/dompurify-3.4.13.tgz",
|
||||
"integrity": "sha512-2vmYIoqjze2d+kakP8S/nS5shfsl587kzwEjcGlTdiksUVgFHnFCsLYDVj/JNqJVOQZGSYBTmuycv0PodwmnMQ==",
|
||||
"license": "(MPL-2.0 OR Apache-2.0)",
|
||||
"optionalDependencies": {
|
||||
"@types/trusted-types": "^2.0.7"
|
||||
@@ -23890,9 +23924,9 @@
|
||||
"license": "MIT"
|
||||
},
|
||||
"node_modules/immer": {
|
||||
"version": "11.1.18",
|
||||
"resolved": "https://registry.npmjs.org/immer/-/immer-11.1.18.tgz",
|
||||
"integrity": "sha512-EQyQtLiYW029lyoczMl/Hh4Xu7cDecSc58JRYpHyL4tIAu3eqd1yJzQX04d2BZHDkzFFvm6qJEJWOtfDSWAXbQ==",
|
||||
"version": "11.1.17",
|
||||
"resolved": "https://registry.npmjs.org/immer/-/immer-11.1.17.tgz",
|
||||
"integrity": "sha512-8Vu44Y0MuMBlTQz/jQ8HEMYNq/bBqk87MnBwYR5mC8AthfhEXidZ5aT/oA/CUqboa8THKltnD9L3xyqhU/Sy1Q==",
|
||||
"license": "MIT",
|
||||
"funding": {
|
||||
"type": "opencollective",
|
||||
@@ -27242,9 +27276,9 @@
|
||||
"license": "Apache-2.0"
|
||||
},
|
||||
"node_modules/lerna": {
|
||||
"version": "10.0.1",
|
||||
"resolved": "https://registry.npmjs.org/lerna/-/lerna-10.0.1.tgz",
|
||||
"integrity": "sha512-ibmMaBmH/sR2HpZzgvpEI5/6bCgMp0AISIFZ/cQcCzRVH2EgPapBYHXS6A6NgI6vRJNE4pgnxQbNiSq00HVSjA==",
|
||||
"version": "10.0.0",
|
||||
"resolved": "https://registry.npmjs.org/lerna/-/lerna-10.0.0.tgz",
|
||||
"integrity": "sha512-U1Rkz2lMZEGstg7h6vw2LfuGNTomXANZ+mX80+3pR0L2RWWcXI/gZJya9EDq4wTY+1AoUzhNNUnlAEWdFugisw==",
|
||||
"dev": true,
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
@@ -27260,7 +27294,7 @@
|
||||
"conventional-changelog": "8.1.0",
|
||||
"conventional-changelog-angular": "9.2.1",
|
||||
"conventional-commits-filter": "6.0.1",
|
||||
"conventional-commits-parser": "7.1.2",
|
||||
"conventional-commits-parser": "7.1.0",
|
||||
"conventional-recommended-bump": "12.1.0",
|
||||
"cosmiconfig": "9.0.0",
|
||||
"dedent": "1.5.3",
|
||||
@@ -27291,7 +27325,7 @@
|
||||
"signal-exit": "3.0.7",
|
||||
"ssri": "12.0.0",
|
||||
"string-width": "^4.2.3",
|
||||
"tar": "7.5.22",
|
||||
"tar": "7.5.20",
|
||||
"tinyglobby": "0.2.12",
|
||||
"validate-npm-package-license": "3.0.4",
|
||||
"validate-npm-package-name": "6.0.2",
|
||||
@@ -42952,7 +42986,7 @@
|
||||
"d3-time": "^3.1.0",
|
||||
"d3-time-format": "^4.1.0",
|
||||
"dayjs": "^1.11.23",
|
||||
"dompurify": "^3.4.14",
|
||||
"dompurify": "^3.4.13",
|
||||
"fetch-retry": "^6.0.0",
|
||||
"handlebars": "^4.7.9",
|
||||
"jed": "^1.1.1",
|
||||
|
||||
@@ -177,7 +177,7 @@
|
||||
"geostyler-style": "11.0.2",
|
||||
"geostyler-wfs-parser": "^3.0.1",
|
||||
"google-auth-library": "^11.0.2",
|
||||
"immer": "^11.1.18",
|
||||
"immer": "^11.1.17",
|
||||
"interweave": "^13.1.1",
|
||||
"jquery": "^4.0.0",
|
||||
"js-levenshtein": "^1.1.6",
|
||||
@@ -326,7 +326,7 @@
|
||||
"jest-websocket-mock": "^2.5.0",
|
||||
"js-yaml-loader": "^1.2.2",
|
||||
"jsdom": "^30.0.1",
|
||||
"lerna": "^10.0.1",
|
||||
"lerna": "^10.0.0",
|
||||
"lightningcss": "^1.33.0",
|
||||
"mini-css-extract-plugin": "^2.10.2",
|
||||
"minimizer-webpack-plugin": "^5.6.1",
|
||||
|
||||
@@ -68,7 +68,7 @@
|
||||
"d3-time": "^3.1.0",
|
||||
"d3-time-format": "^4.1.0",
|
||||
"dayjs": "^1.11.23",
|
||||
"dompurify": "^3.4.14",
|
||||
"dompurify": "^3.4.13",
|
||||
"fetch-retry": "^6.0.0",
|
||||
"handlebars": "^4.7.9",
|
||||
"jed": "^1.1.1",
|
||||
|
||||
@@ -1,153 +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 { useState } from 'react';
|
||||
import { fireEvent, render, screen } from '@superset-ui/core/spec';
|
||||
import { Input } from '../Input';
|
||||
import { Modal } from './Modal';
|
||||
|
||||
const drag = (
|
||||
target: Element,
|
||||
from: [number, number],
|
||||
to: [number, number],
|
||||
) => {
|
||||
fireEvent.mouseDown(target, { clientX: from[0], clientY: from[1] });
|
||||
fireEvent.mouseMove(document, { clientX: to[0], clientY: to[1] });
|
||||
fireEvent.mouseUp(document);
|
||||
};
|
||||
|
||||
const isDragged = () => !!document.querySelector('.react-draggable-dragged');
|
||||
|
||||
describe('Modal draggable', () => {
|
||||
test('dragging from the title bar moves the modal', () => {
|
||||
render(
|
||||
<Modal show onHide={() => {}} title="Edit Dataset" draggable name="test">
|
||||
<Input data-test="field" defaultValue="value" />
|
||||
</Modal>,
|
||||
);
|
||||
|
||||
const trigger = document.querySelector('.draggable-trigger') as HTMLElement;
|
||||
drag(trigger, [100, 50], [150, 90]);
|
||||
|
||||
expect(isDragged()).toBe(true);
|
||||
});
|
||||
|
||||
test('dragging inside modal content does not move the modal', () => {
|
||||
render(
|
||||
<Modal show onHide={() => {}} title="Edit Dataset" draggable name="test">
|
||||
<Input data-test="field" defaultValue="first_view_event" />
|
||||
</Modal>,
|
||||
);
|
||||
|
||||
const input = screen.getByTestId('field');
|
||||
drag(input, [200, 400], [260, 430]);
|
||||
|
||||
expect(isDragged()).toBe(false);
|
||||
});
|
||||
|
||||
test('dragging inside modal content does not move the modal, even after an unrelated re-render while the title was hovered', () => {
|
||||
// Regression test: the title bar used to gate dragging with a
|
||||
// hover-tracked boolean (mouseover/mouseout on `.draggable-trigger`)
|
||||
// instead of react-draggable's own `handle` prop. Because the title
|
||||
// element was defined as an inline component recreated on every
|
||||
// render, any unrelated state change while the cursor was over the
|
||||
// title (e.g. typing in any field) force-remounted it without a real
|
||||
// mouseout ever firing, leaving dragging permanently enabled -- so
|
||||
// selecting text anywhere in the modal dragged the whole modal
|
||||
// instead.
|
||||
function Harness() {
|
||||
const [tick, setTick] = useState(0);
|
||||
return (
|
||||
<Modal
|
||||
show
|
||||
onHide={() => {}}
|
||||
title="Edit Dataset"
|
||||
draggable
|
||||
name="test"
|
||||
>
|
||||
<button
|
||||
type="button"
|
||||
data-test="rerender"
|
||||
onClick={() => setTick(tick + 1)}
|
||||
>
|
||||
rerender
|
||||
</button>
|
||||
<Input data-test="field" defaultValue="first_view_event" />
|
||||
</Modal>
|
||||
);
|
||||
}
|
||||
|
||||
render(<Harness />);
|
||||
|
||||
const trigger = document.querySelector('.draggable-trigger') as HTMLElement;
|
||||
fireEvent.mouseOver(trigger);
|
||||
fireEvent.click(screen.getByTestId('rerender'));
|
||||
|
||||
const input = screen.getByTestId('field');
|
||||
drag(input, [200, 400], [260, 430]);
|
||||
|
||||
expect(isDragged()).toBe(false);
|
||||
});
|
||||
|
||||
test('dragging is disabled entirely when draggable is not set', () => {
|
||||
render(
|
||||
<Modal show onHide={() => {}} title="Edit Dataset" name="test">
|
||||
<Input data-test="field" defaultValue="value" />
|
||||
</Modal>,
|
||||
);
|
||||
|
||||
expect(document.querySelector('.draggable-trigger')).toBeNull();
|
||||
});
|
||||
|
||||
test('draggableConfig cannot re-enable dragging on a non-draggable modal', () => {
|
||||
render(
|
||||
<Modal
|
||||
show
|
||||
onHide={() => {}}
|
||||
title="Edit Dataset"
|
||||
name="test"
|
||||
draggableConfig={{ disabled: false }}
|
||||
>
|
||||
<Input data-test="field" defaultValue="value" />
|
||||
</Modal>,
|
||||
);
|
||||
|
||||
expect(document.querySelector('.draggable-trigger')).toBeNull();
|
||||
});
|
||||
|
||||
test('draggableConfig can still opt a draggable modal out of dragging', () => {
|
||||
render(
|
||||
<Modal
|
||||
show
|
||||
onHide={() => {}}
|
||||
title="Edit Dataset"
|
||||
draggable
|
||||
name="test"
|
||||
draggableConfig={{ disabled: true }}
|
||||
>
|
||||
<Input data-test="field" defaultValue="value" />
|
||||
</Modal>,
|
||||
);
|
||||
|
||||
const trigger = document.querySelector('.draggable-trigger') as HTMLElement;
|
||||
drag(trigger, [100, 50], [150, 90]);
|
||||
|
||||
expect(isDragged()).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -269,6 +269,7 @@ const CustomModal = ({
|
||||
);
|
||||
const draggableRef = useRef<HTMLDivElement>(null);
|
||||
const [bounds, setBounds] = useState<DraggableBounds>({});
|
||||
const [dragDisabled, setDragDisabled] = useState<boolean>(true);
|
||||
const theme = useTheme();
|
||||
|
||||
const handleOnHide = () => {
|
||||
@@ -338,7 +339,19 @@ const CustomModal = ({
|
||||
}, [hideFooter, resizableConfig]);
|
||||
|
||||
const ModalTitle = () =>
|
||||
draggable ? <div className="draggable-trigger">{title}</div> : <>{title}</>;
|
||||
draggable ? (
|
||||
<div
|
||||
className="draggable-trigger"
|
||||
onMouseOver={() => dragDisabled && setDragDisabled(false)}
|
||||
onMouseOut={() => !dragDisabled && setDragDisabled(true)}
|
||||
onFocus={() => dragDisabled && setDragDisabled(false)}
|
||||
onBlur={() => !dragDisabled && setDragDisabled(true)}
|
||||
>
|
||||
{title}
|
||||
</div>
|
||||
) : (
|
||||
<>{title}</>
|
||||
);
|
||||
|
||||
return (
|
||||
<StyledModal
|
||||
@@ -365,19 +378,13 @@ const CustomModal = ({
|
||||
modalRender={modal =>
|
||||
resizable || draggable ? (
|
||||
<Draggable
|
||||
disabled={!draggable || dragDisabled}
|
||||
bounds={bounds ?? false}
|
||||
onStart={(event, uiData) => onDragStart(event, uiData)}
|
||||
{...draggableConfig}
|
||||
// `disabled` and `handle` are applied after the spread so callers
|
||||
// can't use `draggableConfig` to re-enable dragging on a
|
||||
// non-draggable modal or move the drag handle off the title bar.
|
||||
// A caller opting a draggable modal out via
|
||||
// `draggableConfig.disabled` is still honored.
|
||||
disabled={!draggable || !!draggableConfig?.disabled}
|
||||
handle={draggable ? '.draggable-trigger' : undefined}
|
||||
// Pass nodeRef so react-draggable does not fall back to
|
||||
// ReactDOM.findDOMNode (deprecated in React 18+ Strict Mode).
|
||||
nodeRef={draggableRef}
|
||||
{...draggableConfig}
|
||||
>
|
||||
{resizable ? (
|
||||
<Resizable className="resizable" {...getResizableConfig}>
|
||||
|
||||
@@ -28,7 +28,6 @@ export enum FeatureFlag {
|
||||
AlertReportSlackV2 = 'ALERT_REPORT_SLACK_V2',
|
||||
AlertReportWebhook = 'ALERT_REPORT_WEBHOOK',
|
||||
AlertReportsFilter = 'ALERT_REPORTS_FILTER',
|
||||
AlertReportsRetry = 'ALERT_REPORTS_RETRY',
|
||||
AllowFullCsvExport = 'ALLOW_FULL_CSV_EXPORT',
|
||||
ChartPluginsExperimental = 'CHART_PLUGINS_EXPERIMENTAL',
|
||||
ConfirmDashboardDiff = 'CONFIRM_DASHBOARD_DIFF',
|
||||
|
||||
@@ -175,18 +175,6 @@ const config: ControlPanelConfig = {
|
||||
label: t('X Axis'),
|
||||
expanded: true,
|
||||
controlSetRows: [
|
||||
[
|
||||
{
|
||||
name: 'show_x_axis',
|
||||
config: {
|
||||
type: 'CheckboxControl',
|
||||
label: t('Show X axis'),
|
||||
renderTrigger: true,
|
||||
default: true,
|
||||
description: t('Show or hide the X axis line, ticks, and labels'),
|
||||
},
|
||||
},
|
||||
],
|
||||
[
|
||||
{
|
||||
name: 'x_axis_label',
|
||||
@@ -195,8 +183,6 @@ const config: ControlPanelConfig = {
|
||||
label: t('X Axis Label'),
|
||||
renderTrigger: true,
|
||||
default: '',
|
||||
visibility: ({ controls }) =>
|
||||
controls?.show_x_axis?.value !== false,
|
||||
},
|
||||
},
|
||||
],
|
||||
@@ -207,8 +193,6 @@ const config: ControlPanelConfig = {
|
||||
...sharedControls.x_axis_time_format,
|
||||
default: DEFAULT_TIME_FORMAT,
|
||||
description: `${D3_TIME_FORMAT_DOCS}.`,
|
||||
visibility: ({ controls }) =>
|
||||
controls?.show_x_axis?.value !== false,
|
||||
},
|
||||
},
|
||||
],
|
||||
@@ -229,8 +213,6 @@ const config: ControlPanelConfig = {
|
||||
clearable: false,
|
||||
renderTrigger: true,
|
||||
description: t('The way the ticks are laid out on the X-axis'),
|
||||
visibility: ({ controls }) =>
|
||||
controls?.show_x_axis?.value !== false,
|
||||
},
|
||||
},
|
||||
],
|
||||
@@ -240,20 +222,6 @@ const config: ControlPanelConfig = {
|
||||
label: t('Y Axis'),
|
||||
expanded: true,
|
||||
controlSetRows: [
|
||||
[
|
||||
{
|
||||
name: 'show_y_axis',
|
||||
config: {
|
||||
type: 'CheckboxControl',
|
||||
label: t('Show Y axis'),
|
||||
renderTrigger: true,
|
||||
default: true,
|
||||
description: t(
|
||||
'Show or hide the Y axis line, ticks, gridlines, and labels',
|
||||
),
|
||||
},
|
||||
},
|
||||
],
|
||||
[
|
||||
{
|
||||
name: 'y_axis_label',
|
||||
@@ -262,8 +230,6 @@ const config: ControlPanelConfig = {
|
||||
label: t('Y Axis Label'),
|
||||
renderTrigger: true,
|
||||
default: '',
|
||||
visibility: ({ controls }) =>
|
||||
controls?.show_y_axis?.value !== false,
|
||||
},
|
||||
},
|
||||
],
|
||||
@@ -273,10 +239,6 @@ const config: ControlPanelConfig = {
|
||||
},
|
||||
],
|
||||
controlOverrides: {
|
||||
// Note: y_axis_format and currency_format are intentionally NOT gated on
|
||||
// show_y_axis. They drive `defaultFormatter`, which formats the bar labels
|
||||
// and tooltips as well as the axis, so they must stay configurable even
|
||||
// when the Y axis itself is hidden.
|
||||
groupby: {
|
||||
label: t('Breakdowns'),
|
||||
description:
|
||||
|
||||
@@ -185,8 +185,6 @@ export default function transformProps(
|
||||
xTicksLayout,
|
||||
xAxisTimeFormat,
|
||||
showLegend,
|
||||
showXAxis = true,
|
||||
showYAxis = true,
|
||||
yAxisLabel,
|
||||
xAxisLabel,
|
||||
yAxisFormat,
|
||||
@@ -435,10 +433,8 @@ export default function transformProps(
|
||||
grid: {
|
||||
...defaultGrid,
|
||||
top: theme.sizeUnit * 7,
|
||||
// Reclaim the axis-oriented padding when an axis is hidden so an
|
||||
// axis-free chart gets a clean, tight layout instead of empty margins.
|
||||
bottom: theme.sizeUnit * (showXAxis ? 7 : 3),
|
||||
left: theme.sizeUnit * (showYAxis ? 5 : 2),
|
||||
bottom: theme.sizeUnit * 7,
|
||||
left: theme.sizeUnit * 5,
|
||||
right: theme.sizeUnit * 7,
|
||||
},
|
||||
legend: {
|
||||
@@ -447,7 +443,6 @@ export default function transformProps(
|
||||
data: [legendNames.INCREASE, legendNames.DECREASE, legendNames.TOTAL],
|
||||
},
|
||||
xAxis: {
|
||||
show: showXAxis,
|
||||
data: xAxisData,
|
||||
type: 'category',
|
||||
name: xAxisLabel,
|
||||
@@ -459,7 +454,6 @@ export default function transformProps(
|
||||
},
|
||||
yAxis: {
|
||||
...defaultYAxis,
|
||||
show: showYAxis,
|
||||
type: 'value',
|
||||
nameTextStyle: {
|
||||
padding: [0, 0, theme.sizeUnit * 5, 0],
|
||||
|
||||
@@ -55,10 +55,8 @@ export type EchartsWaterfallFormData = QueryFormData &
|
||||
xAxisLabel: string;
|
||||
xAxisTimeFormat?: string;
|
||||
xTicksLayout?: WaterfallFormXTicksLayout;
|
||||
showXAxis: boolean;
|
||||
yAxisLabel: string;
|
||||
yAxisFormat: string;
|
||||
showYAxis: boolean;
|
||||
increaseLabel?: string;
|
||||
decreaseLabel?: string;
|
||||
totalLabel?: string;
|
||||
@@ -67,8 +65,6 @@ export type EchartsWaterfallFormData = QueryFormData &
|
||||
|
||||
export const DEFAULT_FORM_DATA: Partial<EchartsWaterfallFormData> = {
|
||||
showLegend: true,
|
||||
showXAxis: true,
|
||||
showYAxis: true,
|
||||
};
|
||||
|
||||
export interface EchartsWaterfallChartProps extends ChartProps {
|
||||
|
||||
@@ -114,23 +114,6 @@ export const textStyleSchema = z.object({
|
||||
// Style Schemas
|
||||
// =============================================================================
|
||||
|
||||
/** Repeating tile pattern painted over a fill, e.g. hatching */
|
||||
export const decalSchema = z.object({
|
||||
symbol: z.union([symbolTypeSchema, z.array(symbolTypeSchema)]).optional(),
|
||||
symbolSize: z.number().optional(),
|
||||
symbolKeepAspect: z.boolean().optional(),
|
||||
color: colorSchema.optional(),
|
||||
backgroundColor: colorSchema.optional(),
|
||||
dashArrayX: z
|
||||
.union([z.number(), z.array(z.union([z.number(), z.array(z.number())]))])
|
||||
.optional(),
|
||||
dashArrayY: z.union([z.number(), z.array(z.number())]).optional(),
|
||||
/** Radians, not degrees. */
|
||||
rotation: z.number().optional(),
|
||||
maxTileWidth: z.number().optional(),
|
||||
maxTileHeight: z.number().optional(),
|
||||
});
|
||||
|
||||
export const lineStyleSchema = z.object({
|
||||
color: colorSchema.optional(),
|
||||
width: z.number().optional(),
|
||||
@@ -167,7 +150,6 @@ export const itemStyleSchema = z.object({
|
||||
shadowOffsetX: z.number().optional(),
|
||||
shadowOffsetY: z.number().optional(),
|
||||
opacity: z.number().min(0).max(1).optional(),
|
||||
decal: decalSchema.optional(),
|
||||
});
|
||||
|
||||
// =============================================================================
|
||||
@@ -836,7 +818,6 @@ export type TextStyleOption = z.infer<typeof textStyleSchema>;
|
||||
export type LineStyleOption = z.infer<typeof lineStyleSchema>;
|
||||
export type AreaStyleOption = z.infer<typeof areaStyleSchema>;
|
||||
export type ItemStyleOption = z.infer<typeof itemStyleSchema>;
|
||||
export type DecalOption = z.infer<typeof decalSchema>;
|
||||
export type LabelOption = z.infer<typeof labelSchema>;
|
||||
export type TitleOption = z.infer<typeof titleSchema>;
|
||||
export type LegendOption = z.infer<typeof legendSchema>;
|
||||
|
||||
-91
@@ -593,94 +593,3 @@ test('strips tooltip extraCssText instead of passing raw CSS through', () => {
|
||||
expect(result.success).toBe(true);
|
||||
expect(result.data).toEqual({ tooltip: { show: true } });
|
||||
});
|
||||
|
||||
test('accepts a decal pattern on itemStyle', () => {
|
||||
const result = parseEChartOptions(
|
||||
`{ series: { itemStyle: { decal: {
|
||||
symbol: 'rect',
|
||||
dashArrayX: [1, 0],
|
||||
dashArrayY: [2, 4],
|
||||
rotation: -0.7853981633974483,
|
||||
color: 'rgba(0, 0, 0, 0.2)',
|
||||
} } } }`,
|
||||
);
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
expect(result.data).toEqual({
|
||||
series: {
|
||||
itemStyle: {
|
||||
decal: {
|
||||
symbol: 'rect',
|
||||
dashArrayX: [1, 0],
|
||||
dashArrayY: [2, 4],
|
||||
rotation: -0.7853981633974483,
|
||||
color: 'rgba(0, 0, 0, 0.2)',
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
test('accepts the full decal shape, including per-row dash arrays', () => {
|
||||
// `dashArrayX` nests one level to offset rows from each other; `dashArrayY`
|
||||
// has no equivalent.
|
||||
const result = parseEChartOptions(
|
||||
`{ series: { itemStyle: { decal: {
|
||||
symbol: ['rect', 'circle'],
|
||||
symbolSize: 0.8,
|
||||
symbolKeepAspect: false,
|
||||
color: '#383838',
|
||||
backgroundColor: 'transparent',
|
||||
dashArrayX: [[1, 0], [0, 1]],
|
||||
dashArrayY: 5,
|
||||
rotation: 0,
|
||||
maxTileWidth: 512,
|
||||
maxTileHeight: 512,
|
||||
} } } }`,
|
||||
);
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
expect(
|
||||
(result.data as { series: { itemStyle: { decal: unknown } } }).series
|
||||
.itemStyle.decal,
|
||||
).toEqual({
|
||||
symbol: ['rect', 'circle'],
|
||||
symbolSize: 0.8,
|
||||
symbolKeepAspect: false,
|
||||
color: '#383838',
|
||||
backgroundColor: 'transparent',
|
||||
dashArrayX: [
|
||||
[1, 0],
|
||||
[0, 1],
|
||||
],
|
||||
dashArrayY: 5,
|
||||
rotation: 0,
|
||||
maxTileWidth: 512,
|
||||
maxTileHeight: 512,
|
||||
});
|
||||
});
|
||||
|
||||
test('strips unknown keys from a decal rather than passing them through', () => {
|
||||
const result = parseEChartOptions(
|
||||
`{ series: { itemStyle: { decal: { symbol: 'rect', onclick: 'alert(1)' } } } }`,
|
||||
);
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
expect(result.data).toEqual({
|
||||
series: { itemStyle: { decal: { symbol: 'rect' } } },
|
||||
});
|
||||
});
|
||||
|
||||
test('rejects a decal whose values are of the wrong type', () => {
|
||||
// Unknown keys are stripped; a known key with a bad type is an error.
|
||||
const input = `{ series: { itemStyle: { decal: { rotation: 'sideways' } } } }`;
|
||||
|
||||
expect(() => parseEChartOptions(input)).toThrow(EChartOptionsParseError);
|
||||
try {
|
||||
parseEChartOptions(input);
|
||||
} catch (error) {
|
||||
expect((error as EChartOptionsParseError).errorType).toBe(
|
||||
'validation_error',
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
@@ -166,56 +166,3 @@ test('hide totals', () => {
|
||||
['-', '-'],
|
||||
]);
|
||||
});
|
||||
|
||||
const buildAxes = (extraFormData: Record<string, unknown>) => {
|
||||
const chartProps = new ChartProps({
|
||||
formData: { ...formData, ...extraFormData },
|
||||
width: 800,
|
||||
height: 600,
|
||||
queriesData: [{ data }],
|
||||
theme: supersetTheme,
|
||||
});
|
||||
const transformedProps = transformProps(
|
||||
chartProps as unknown as EchartsWaterfallChartProps,
|
||||
);
|
||||
return {
|
||||
xAxis: transformedProps.echartOptions.xAxis as any,
|
||||
yAxis: transformedProps.echartOptions.yAxis as any,
|
||||
grid: transformedProps.echartOptions.grid as any,
|
||||
};
|
||||
};
|
||||
|
||||
test('shows both axes by default', () => {
|
||||
const { xAxis, yAxis } = buildAxes({});
|
||||
expect(xAxis.show).not.toBe(false);
|
||||
expect(yAxis.show).not.toBe(false);
|
||||
});
|
||||
|
||||
test('hides the whole X axis when showXAxis is false', () => {
|
||||
const { xAxis } = buildAxes({ showXAxis: false });
|
||||
// echarts hides the axis line, ticks, labels, name, and gridlines when
|
||||
// `show` is false — a single flag rather than a set of sub-flags.
|
||||
expect(xAxis.show).toBe(false);
|
||||
});
|
||||
|
||||
test('hides the whole Y axis when showYAxis is false', () => {
|
||||
const { yAxis } = buildAxes({ showYAxis: false });
|
||||
expect(yAxis.show).toBe(false);
|
||||
});
|
||||
|
||||
test('reclaims the bottom grid margin when the X axis is hidden', () => {
|
||||
const { grid: shown } = buildAxes({ showXAxis: true });
|
||||
const { grid: hidden } = buildAxes({ showXAxis: false });
|
||||
// The bottom margin reserves room for the X-axis labels and name; with the
|
||||
// axis hidden that space should be reclaimed for a clean, axis-free layout.
|
||||
expect(hidden.bottom).toBeLessThan(shown.bottom);
|
||||
// Hiding the X axis must not shrink the Y-axis (left) margin.
|
||||
expect(hidden.left).toBe(shown.left);
|
||||
});
|
||||
|
||||
test('reclaims the left grid margin when the Y axis is hidden', () => {
|
||||
const { grid: shown } = buildAxes({ showYAxis: true });
|
||||
const { grid: hidden } = buildAxes({ showYAxis: false });
|
||||
expect(hidden.left).toBeLessThan(shown.left);
|
||||
expect(hidden.bottom).toBe(shown.bottom);
|
||||
});
|
||||
|
||||
@@ -107,11 +107,6 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
|
||||
);
|
||||
const metricsArr = ensureIsArray(formData.metrics);
|
||||
let data: QueryData[];
|
||||
// The rows that conditional formatting derives its color scale from. Only the
|
||||
// leaf (detail) cells belong in that domain: the totals are aggregates of the
|
||||
// very cells being shaded, so letting them in makes the grand total the max
|
||||
// and leaves every detail cell nearly unshaded.
|
||||
let colorScaleRows: DataRecord[];
|
||||
if (allMetricsAdditive(metricsArr)) {
|
||||
// Additive fast-path: a single full-detail query was issued; synthesize
|
||||
// each rollup level by reducing the leaf rows on the client (see SIP.md).
|
||||
@@ -133,11 +128,6 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
|
||||
data: synthesized[i] as DataRecord[],
|
||||
groupby: combination,
|
||||
}));
|
||||
// The query returned the leaf rows and nothing else, so no totals can leak
|
||||
// into the domain. Use those raw rows rather than the synthesized leaf
|
||||
// level, whose reduction coerces values through `Number` and drops
|
||||
// non-numeric ones -- that would shift the domain for additive metrics.
|
||||
colorScaleRows = leafRows;
|
||||
} else {
|
||||
// Non-additive: a single GROUPING SETS query returned all rollup levels
|
||||
// tagged with GROUPING() markers; split the combined result back into one
|
||||
@@ -155,13 +145,6 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
|
||||
data: splitRows[i] as DataRecord[],
|
||||
groupby: combination,
|
||||
}));
|
||||
// This result *does* carry the rollup levels, so pick out the leaf level --
|
||||
// the one grouping every dimension, same definition the splitter uses.
|
||||
const leafIndex = levelLabels.findIndex(labels => {
|
||||
const grouped = new Set(labels);
|
||||
return allGroupbyLabels.every(label => grouped.has(label));
|
||||
});
|
||||
colorScaleRows = (splitRows[leafIndex] as DataRecord[]) ?? [];
|
||||
}
|
||||
// The full-granularity query has the most colnames -- use it for column/type
|
||||
// metadata and formatters.
|
||||
@@ -238,7 +221,7 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
|
||||
);
|
||||
const metricColorFormatters = getColorFormatters(
|
||||
pivotConditionalFormatting,
|
||||
colorScaleRows,
|
||||
mainQuery.data,
|
||||
theme,
|
||||
);
|
||||
|
||||
|
||||
-149
@@ -452,152 +452,3 @@ test('additive metrics: synthesizes rollup levels from a single leaf query', ()
|
||||
{ region: 'EU', v: 5 },
|
||||
]);
|
||||
});
|
||||
|
||||
test('conditional formatting scales over leaf cells only, not rollup totals', () => {
|
||||
const gm = (col: string) => `${col}__superset_grouping`;
|
||||
// `metrics` below is a plain string, i.e. a saved-metric reference.
|
||||
// `isAdditiveMetric` treats every string as non-additive no matter what it is
|
||||
// named, because form data does not reveal the aggregate behind a saved
|
||||
// metric -- so a saved metric labelled "SUM(sales)" (as in the report this
|
||||
// regression comes from) takes the non-additive path despite the name. That
|
||||
// path issues a single GROUPING SETS query whose result carries the rollup
|
||||
// levels alongside the leaf rows, so with both totals toggles on the grand
|
||||
// total (100) is part of that result.
|
||||
const row = (
|
||||
productLine: string | null,
|
||||
dealSize: string | null,
|
||||
sales: number,
|
||||
) => ({
|
||||
product_line: productLine,
|
||||
deal_size: dealSize,
|
||||
'SUM(sales)': sales,
|
||||
[gm('product_line')]: productLine === null ? 1 : 0,
|
||||
[gm('deal_size')]: dealSize === null ? 1 : 0,
|
||||
});
|
||||
const totalsChartProps = new ChartProps<QueryFormData>({
|
||||
formData: {
|
||||
...formData,
|
||||
combineMetric: false,
|
||||
transposePivot: false,
|
||||
metricsLayout: MetricsLayoutEnum.ROWS,
|
||||
groupbyRows: ['product_line'],
|
||||
groupbyColumns: ['deal_size'],
|
||||
metrics: ['SUM(sales)'],
|
||||
colTotals: true,
|
||||
rowTotals: true,
|
||||
conditionalFormatting: [
|
||||
{
|
||||
colorScheme: '#ACE1C4',
|
||||
column: 'SUM(sales)',
|
||||
operator: '>',
|
||||
targetValue: 0,
|
||||
},
|
||||
],
|
||||
},
|
||||
width: 800,
|
||||
height: 600,
|
||||
queriesData: [
|
||||
{
|
||||
data: [
|
||||
// leaf cells
|
||||
row('Classic Cars', 'Small', 10),
|
||||
row('Classic Cars', 'Large', 20),
|
||||
row('Motorcycles', 'Small', 30),
|
||||
row('Motorcycles', 'Large', 40),
|
||||
// row totals
|
||||
row('Classic Cars', null, 30),
|
||||
row('Motorcycles', null, 70),
|
||||
// column totals
|
||||
row(null, 'Small', 40),
|
||||
row(null, 'Large', 60),
|
||||
// grand total
|
||||
row(null, null, 100),
|
||||
],
|
||||
colnames: [
|
||||
'product_line',
|
||||
'deal_size',
|
||||
'SUM(sales)',
|
||||
gm('product_line'),
|
||||
gm('deal_size'),
|
||||
],
|
||||
coltypes: [1, 1, 0, 0, 0],
|
||||
},
|
||||
],
|
||||
hooks: { setDataMask },
|
||||
filterState: { selectedFilters: {} },
|
||||
datasource: { verboseMap: {}, columnFormats: {} },
|
||||
theme: supersetTheme,
|
||||
});
|
||||
|
||||
const { getColorFromValue } =
|
||||
transformProps(totalsChartProps).metricColorFormatters[0];
|
||||
// The largest leaf cell must be fully saturated. Including the grand total
|
||||
// in the domain would stretch it to 100 and leave this cell washed out.
|
||||
expect(getColorFromValue(40)).toEqual('#ACE1C4FF');
|
||||
});
|
||||
|
||||
test('conditional formatting on the additive path uses the raw leaf query rows', () => {
|
||||
// Counterpart to the test above for the additive fast path. Its query returns
|
||||
// the leaf rows only, so the domain is those rows verbatim -- deliberately not
|
||||
// the synthesized leaf level, whose reduction would coerce values through
|
||||
// `Number` and drop non-numeric ones.
|
||||
const additiveChartProps = new ChartProps<QueryFormData>({
|
||||
formData: {
|
||||
...formData,
|
||||
combineMetric: false,
|
||||
transposePivot: false,
|
||||
metricsLayout: MetricsLayoutEnum.ROWS,
|
||||
groupbyRows: ['product_line'],
|
||||
groupbyColumns: ['deal_size'],
|
||||
metrics: [
|
||||
{
|
||||
expressionType: 'SIMPLE',
|
||||
aggregate: 'SUM',
|
||||
column: { column_name: 'sales' },
|
||||
label: 'SUM(sales)',
|
||||
},
|
||||
],
|
||||
colTotals: true,
|
||||
rowTotals: true,
|
||||
conditionalFormatting: [
|
||||
{
|
||||
colorScheme: '#ACE1C4',
|
||||
column: 'SUM(sales)',
|
||||
operator: '>',
|
||||
targetValue: 0,
|
||||
},
|
||||
],
|
||||
},
|
||||
width: 800,
|
||||
height: 600,
|
||||
queriesData: [
|
||||
{
|
||||
data: [
|
||||
{
|
||||
product_line: 'Classic Cars',
|
||||
deal_size: 'Small',
|
||||
'SUM(sales)': 10,
|
||||
},
|
||||
{
|
||||
product_line: 'Classic Cars',
|
||||
deal_size: 'Large',
|
||||
'SUM(sales)': 20,
|
||||
},
|
||||
{ product_line: 'Motorcycles', deal_size: 'Small', 'SUM(sales)': 30 },
|
||||
{ product_line: 'Motorcycles', deal_size: 'Large', 'SUM(sales)': 40 },
|
||||
],
|
||||
colnames: ['product_line', 'deal_size', 'SUM(sales)'],
|
||||
coltypes: [1, 1, 0],
|
||||
},
|
||||
],
|
||||
hooks: { setDataMask },
|
||||
filterState: { selectedFilters: {} },
|
||||
datasource: { verboseMap: {}, columnFormats: {} },
|
||||
theme: supersetTheme,
|
||||
});
|
||||
|
||||
const { getColorFromValue } =
|
||||
transformProps(additiveChartProps).metricColorFormatters[0];
|
||||
// Scale spans the leaf cells (max 40), never the client-side grand total 100.
|
||||
expect(getColorFromValue(40)).toEqual('#ACE1C4FF');
|
||||
});
|
||||
|
||||
@@ -1138,8 +1138,7 @@ export default function TableChart<D extends DataRecord = DataRecord>(
|
||||
?.backgroundColor || backgroundColor;
|
||||
arrow =
|
||||
column.label === comparisonLabels[0]
|
||||
? (basicColorColumnFormatters[row.index]?.[column.key]
|
||||
?.mainArrow ?? arrow)
|
||||
? basicColorColumnFormatters[row.index]?.[column.key]?.mainArrow
|
||||
: '';
|
||||
}
|
||||
const rowSurfaceColor =
|
||||
@@ -1195,36 +1194,30 @@ export default function TableChart<D extends DataRecord = DataRecord>(
|
||||
}
|
||||
`;
|
||||
|
||||
// Plain inline style (rather than the `css` prop) so the arrow's
|
||||
// color is guaranteed to apply regardless of whether the consuming
|
||||
// app's build wires up the emotion JSX pragma for the `css` prop --
|
||||
// notably, this codebase's own Jest/Babel config does not, which
|
||||
// silently no-ops any `css` prop on a plain DOM element.
|
||||
let arrowStyles: CSSProperties = {
|
||||
color:
|
||||
let arrowStyles = css`
|
||||
color: ${
|
||||
basicColorFormatters &&
|
||||
basicColorFormatters[row.index]?.[originKey]?.arrowColor ===
|
||||
ColorSchemeEnum.Green
|
||||
? theme.colorSuccess
|
||||
: theme.colorError,
|
||||
marginRight: theme.sizeUnit,
|
||||
};
|
||||
: theme.colorError
|
||||
};
|
||||
margin-right: ${theme.sizeUnit}px;
|
||||
`;
|
||||
|
||||
if (
|
||||
basicColorColumnFormatters &&
|
||||
basicColorColumnFormatters?.length > 0
|
||||
) {
|
||||
const columnArrowColor =
|
||||
basicColorColumnFormatters[row.index]?.[column.key]?.arrowColor;
|
||||
if (columnArrowColor) {
|
||||
arrowStyles = {
|
||||
color:
|
||||
columnArrowColor === ColorSchemeEnum.Green
|
||||
? theme.colorSuccess
|
||||
: theme.colorError,
|
||||
marginRight: theme.sizeUnit,
|
||||
arrowStyles = css`
|
||||
color: ${
|
||||
basicColorColumnFormatters[row.index]?.[column.key]
|
||||
?.arrowColor === ColorSchemeEnum.Green
|
||||
? theme.colorSuccess
|
||||
: theme.colorError
|
||||
};
|
||||
}
|
||||
margin-right: ${theme.sizeUnit}px;
|
||||
`;
|
||||
}
|
||||
|
||||
const cellProps = {
|
||||
@@ -1309,12 +1302,12 @@ export default function TableChart<D extends DataRecord = DataRecord>(
|
||||
className="dt-truncate-cell"
|
||||
style={columnWidth ? { width: columnWidth } : undefined}
|
||||
>
|
||||
{arrow && <span style={arrowStyles}>{arrow}</span>}
|
||||
{arrow && <span css={arrowStyles}>{arrow}</span>}
|
||||
{text}
|
||||
</div>
|
||||
) : (
|
||||
<>
|
||||
{arrow && <span style={arrowStyles}>{arrow}</span>}
|
||||
{arrow && <span css={arrowStyles}>{arrow}</span>}
|
||||
{text}
|
||||
</>
|
||||
)}
|
||||
|
||||
@@ -2111,14 +2111,7 @@ describe('plugin-chart-table', () => {
|
||||
|
||||
expect(() =>
|
||||
render(
|
||||
ProviderWrapper({
|
||||
children: (
|
||||
<TableChart
|
||||
{...propsWithMissingFormatterEntry}
|
||||
sticky={false}
|
||||
/>
|
||||
),
|
||||
}),
|
||||
<TableChart {...propsWithMissingFormatterEntry} sticky={false} />,
|
||||
),
|
||||
).not.toThrow();
|
||||
|
||||
@@ -2132,24 +2125,8 @@ describe('plugin-chart-table', () => {
|
||||
'rgba(0, 150, 0, 0.2)',
|
||||
);
|
||||
|
||||
// the row missing a formatter entry falls back to the row-level
|
||||
// comparison arrow instead of losing it: before the fix, this row's
|
||||
// arrow was silently cleared (and its color, computed the same way,
|
||||
// would have flipped to the "decrease" color) whenever the
|
||||
// column-specific lookup for this row was undefined.
|
||||
const arrowCell = screen
|
||||
.getAllByTitle('110')
|
||||
.find(cell => cell.querySelector('span'));
|
||||
expect(arrowCell).toHaveTextContent('↑110');
|
||||
expect(getComputedStyle(arrowCell!).background).toContain(
|
||||
'rgba(0, 150, 0, 0.2)',
|
||||
);
|
||||
// the fallback arrow itself must also keep the "increase" color --
|
||||
// asserting only the cell background would still pass if the arrow's
|
||||
// own color had regressed to the "decrease" color.
|
||||
expect(arrowCell!.querySelector('span')).toHaveStyle({
|
||||
color: supersetTheme.colorSuccess,
|
||||
});
|
||||
// the row missing a formatter entry still renders its raw value
|
||||
expect(screen.getAllByTitle('110').length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
test('preserves client-side search text across temporal table rerenders', async () => {
|
||||
|
||||
@@ -61,53 +61,6 @@ describe('sqlLabReducer', () => {
|
||||
});
|
||||
});
|
||||
|
||||
test('should default extra_json to an empty object when extra is unset', () => {
|
||||
// `extra` is nullable in the metadata database, and JSON.parse(extra || '')
|
||||
// is guaranteed to throw because '' is never valid JSON, so one such row
|
||||
// took down the whole reducer.
|
||||
const incomingDb = {
|
||||
...databases.result[0],
|
||||
extra: null,
|
||||
};
|
||||
const incomingDbId = Number(incomingDb.id);
|
||||
|
||||
const action = actions.setDatabases([incomingDb] as any);
|
||||
|
||||
const newState = sqlLabReducer(initialState, action);
|
||||
|
||||
expect(newState.databases[incomingDbId]).toEqual({
|
||||
...incomingDb,
|
||||
extra_json: {},
|
||||
});
|
||||
});
|
||||
|
||||
test('defaults extra_json when a database has malformed extra', () => {
|
||||
const incomingDb = { ...databases.result[0], extra: '{not json' };
|
||||
|
||||
const newState = sqlLabReducer(
|
||||
initialState,
|
||||
actions.setDatabases([incomingDb] as any),
|
||||
);
|
||||
|
||||
expect(newState.databases[Number(incomingDb.id)].extra_json).toEqual({});
|
||||
});
|
||||
|
||||
test('keeps a valid extra payload', () => {
|
||||
const incomingDb = {
|
||||
...databases.result[0],
|
||||
extra: '{"engine_params": {"pool_size": 5}}',
|
||||
};
|
||||
|
||||
const newState = sqlLabReducer(
|
||||
initialState,
|
||||
actions.setDatabases([incomingDb] as any),
|
||||
);
|
||||
|
||||
expect(newState.databases[Number(incomingDb.id)].extra_json).toEqual({
|
||||
engine_params: { pool_size: 5 },
|
||||
});
|
||||
});
|
||||
|
||||
// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
|
||||
describe('Query editors actions', () => {
|
||||
let newState: SqlLabState;
|
||||
|
||||
@@ -36,27 +36,6 @@ import {
|
||||
|
||||
type SqlLabState = SqlLabRootState['sqlLab'];
|
||||
|
||||
/**
|
||||
* A database's `extra` column is free-form and frequently empty: it is nullable
|
||||
* in the metadata database and the API returns it verbatim. `JSON.parse` cannot
|
||||
* represent that, and `JSON.parse(extra || '')` is guaranteed to throw, since
|
||||
* the empty string is never valid JSON — so a single database row with no
|
||||
* `extra` took down the whole SET_DATABASES reducer and with it SQL Lab.
|
||||
* Malformed JSON is treated the same way: one bad row must not cost the user
|
||||
* every other database.
|
||||
*/
|
||||
function parseDatabaseExtra(extra: unknown): Record<string, unknown> {
|
||||
if (typeof extra !== 'string' || extra.trim() === '') {
|
||||
return {};
|
||||
}
|
||||
try {
|
||||
const parsed = JSON.parse(extra);
|
||||
return parsed && typeof parsed === 'object' ? parsed : {};
|
||||
} catch {
|
||||
return {};
|
||||
}
|
||||
}
|
||||
|
||||
function alterUnsavedQueryEditorState(
|
||||
state: SqlLabState,
|
||||
updatedState: Partial<QueryEditor>,
|
||||
@@ -748,7 +727,7 @@ export default function sqlLabReducer(
|
||||
(action.databases as any[])!.forEach((db: any) => {
|
||||
databases[db.id] = {
|
||||
...db,
|
||||
extra_json: parseDatabaseExtra(db.extra),
|
||||
extra_json: JSON.parse(db.extra || ''),
|
||||
};
|
||||
});
|
||||
return {
|
||||
|
||||
@@ -723,13 +723,11 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
|
||||
force_screenshot: false,
|
||||
include_cta: true,
|
||||
grace_period: undefined,
|
||||
...(isFeatureEnabled(FeatureFlag.AlertReportsRetry) && {
|
||||
retry_on_failure: false,
|
||||
retry_max_attempts: 3,
|
||||
send_failed_reports: false,
|
||||
retry_notify_owners: true,
|
||||
retry_notify_recipients: false,
|
||||
}),
|
||||
retry_on_failure: false,
|
||||
retry_max_attempts: 3,
|
||||
send_failed_reports: false,
|
||||
retry_notify_owners: true,
|
||||
retry_notify_recipients: false,
|
||||
};
|
||||
|
||||
const fetchDashboardFilterValues = async (
|
||||
@@ -2764,7 +2762,7 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
|
||||
</>
|
||||
),
|
||||
},
|
||||
...(isReport && isFeatureEnabled(FeatureFlag.AlertReportsRetry)
|
||||
...(isReport
|
||||
? [
|
||||
{
|
||||
key: 'error-handling',
|
||||
|
||||
@@ -452,23 +452,7 @@ test('submit failure dispatches danger toast and keeps modal open', async () =>
|
||||
// Error Handling section tests
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const enableRetryFlag = () => {
|
||||
mockedIsFeatureEnabled.mockImplementation(
|
||||
(featureFlag: string) =>
|
||||
featureFlag === FeatureFlag.AlertReports ||
|
||||
featureFlag === FeatureFlag.AlertReportsRetry,
|
||||
);
|
||||
};
|
||||
|
||||
test('Error Handling section is hidden when ALERT_REPORTS_RETRY flag is off', () => {
|
||||
const store = createStore({}, reducerIndex);
|
||||
render(<ReportModal {...defaultProps} />, { useRedux: true, store });
|
||||
|
||||
expect(screen.queryByText('Error Handling')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
test('Error Handling section is visible when ALERT_REPORTS_RETRY flag is on', () => {
|
||||
enableRetryFlag();
|
||||
test('Error Handling section is visible and Enable Retries checkbox is unchecked by default', () => {
|
||||
const store = createStore({}, reducerIndex);
|
||||
render(<ReportModal {...defaultProps} />, { useRedux: true, store });
|
||||
|
||||
@@ -480,7 +464,6 @@ test('Error Handling section is visible when ALERT_REPORTS_RETRY flag is on', ()
|
||||
});
|
||||
|
||||
test('conditional retry fields are hidden when Enable Retries is unchecked', () => {
|
||||
enableRetryFlag();
|
||||
const store = createStore({}, reducerIndex);
|
||||
render(<ReportModal {...defaultProps} />, { useRedux: true, store });
|
||||
|
||||
@@ -490,7 +473,6 @@ test('conditional retry fields are hidden when Enable Retries is unchecked', ()
|
||||
});
|
||||
|
||||
test('conditional retry fields appear when Enable Retries is checked', async () => {
|
||||
enableRetryFlag();
|
||||
const store = createStore({}, reducerIndex);
|
||||
render(<ReportModal {...defaultProps} />, { useRedux: true, store });
|
||||
|
||||
@@ -507,7 +489,6 @@ test('conditional retry fields appear when Enable Retries is checked', async ()
|
||||
});
|
||||
|
||||
test('retry fields are included in the POST body when Enable Retries is enabled', async () => {
|
||||
enableRetryFlag();
|
||||
fetchMock.post(REPORT_ENDPOINT, { result: {} }, { name: 'post-retry' });
|
||||
const store = createStore({}, reducerIndex);
|
||||
render(
|
||||
|
||||
@@ -26,12 +26,7 @@ import {
|
||||
} from 'react';
|
||||
|
||||
import { t } from '@apache-superset/core/translation';
|
||||
import {
|
||||
getClientErrorObject,
|
||||
isFeatureEnabled,
|
||||
FeatureFlag,
|
||||
VizType,
|
||||
} from '@superset-ui/core';
|
||||
import { getClientErrorObject, VizType } from '@superset-ui/core';
|
||||
import { Alert } from '@apache-superset/core/components';
|
||||
import { SupersetTheme } from '@apache-superset/core/theme';
|
||||
import { useDispatch, useSelector } from 'react-redux';
|
||||
@@ -201,13 +196,11 @@ function ReportModal({
|
||||
crontab: currentReport.crontab,
|
||||
report_format: currentReport.report_format || defaultNotificationFormat,
|
||||
timezone: currentReport.timezone,
|
||||
...(isFeatureEnabled(FeatureFlag.AlertReportsRetry) && {
|
||||
retry_on_failure: currentReport.retry_on_failure ?? false,
|
||||
retry_max_attempts: currentReport.retry_max_attempts ?? 3,
|
||||
send_failed_reports: currentReport.send_failed_reports ?? false,
|
||||
retry_notify_owners: currentReport.retry_notify_owners ?? true,
|
||||
retry_notify_recipients: currentReport.retry_notify_recipients ?? false,
|
||||
}),
|
||||
retry_on_failure: currentReport.retry_on_failure ?? false,
|
||||
retry_max_attempts: currentReport.retry_max_attempts ?? 3,
|
||||
send_failed_reports: currentReport.send_failed_reports ?? false,
|
||||
retry_notify_owners: currentReport.retry_notify_owners ?? true,
|
||||
retry_notify_recipients: currentReport.retry_notify_recipients ?? false,
|
||||
};
|
||||
|
||||
setCurrentReport({ isSubmitting: true, error: undefined });
|
||||
@@ -485,8 +478,7 @@ function ReportModal({
|
||||
/>
|
||||
{isChart && renderMessageContentSection}
|
||||
{(!isChart || !isTextBasedChart) && renderCustomWidthSection}
|
||||
{isFeatureEnabled(FeatureFlag.AlertReportsRetry) &&
|
||||
renderErrorHandlingSection}
|
||||
{renderErrorHandlingSection}
|
||||
</StyledBottomSection>
|
||||
{currentReport.error && (
|
||||
<Alert
|
||||
|
||||
@@ -72,7 +72,6 @@ from superset.views.base_api import statsd_metrics
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from superset.common.query_context import QueryContext
|
||||
from superset.models.slice import Slice
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -259,7 +258,6 @@ class ChartDataRestApi(ChartRestApi):
|
||||
datasource=query_context.datasource,
|
||||
add_extra_log_payload=add_extra_log_payload,
|
||||
dashboard_filter_context=dashboard_filter_context,
|
||||
slice_=chart,
|
||||
)
|
||||
|
||||
@expose("/data", methods=("POST",))
|
||||
@@ -477,7 +475,6 @@ class ChartDataRestApi(ChartRestApi):
|
||||
filename: str | None = None,
|
||||
expected_rows: int | None = None,
|
||||
dashboard_filter_context: DashboardFilterContext | None = None,
|
||||
slice_: Slice | None = None,
|
||||
) -> Response:
|
||||
if isinstance(result, ChartDataExecutionResult):
|
||||
execution_result: ChartDataExecutionResult | None = result
|
||||
@@ -515,12 +512,6 @@ class ChartDataRestApi(ChartRestApi):
|
||||
|
||||
is_csv_format = result_format == ChartDataResultFormat.CSV
|
||||
|
||||
# A chart's saved query context rarely carries a slice_id in its
|
||||
# form data, so the query context factory can't resolve the slice
|
||||
# for it; routes that already hold the chart pass it explicitly
|
||||
# and the factory-resolved slice covers the rest.
|
||||
slice_ = slice_ or materialized_result["query_context"].slice_
|
||||
|
||||
# Check if we should use streaming for large datasets
|
||||
if is_csv_format and self._should_use_streaming(
|
||||
materialized_result,
|
||||
@@ -531,12 +522,9 @@ class ChartDataRestApi(ChartRestApi):
|
||||
form_data,
|
||||
filename=filename,
|
||||
expected_rows=expected_rows,
|
||||
slice_=slice_,
|
||||
)
|
||||
|
||||
export_filename = filename or self._get_default_export_filename(
|
||||
form_data, slice_
|
||||
)
|
||||
export_filename = filename or self._get_default_export_filename(form_data)
|
||||
# `generate_download_headers` always appends the format extension,
|
||||
# so strip a matching one here to avoid doubled extensions (e.g.
|
||||
# "chart.csv.csv") if the caller already included it.
|
||||
@@ -608,45 +596,22 @@ class ChartDataRestApi(ChartRestApi):
|
||||
return self.response_400(message=f"Unsupported result_format: {result_format}")
|
||||
|
||||
@staticmethod
|
||||
def _get_default_export_filename(
|
||||
form_data: dict[str, Any] | None,
|
||||
slice_: Slice | None = None,
|
||||
) -> str:
|
||||
def _get_default_export_filename(form_data: dict[str, Any] | None) -> str:
|
||||
"""
|
||||
Build a fallback export filename (without extension) from the chart's
|
||||
name so downloaded files are easy to identify, instead of the
|
||||
generic timestamp-only default used by ``generate_download_headers``.
|
||||
|
||||
The name comes from the first usable candidate: an explicit
|
||||
``slice_name`` in the form data, the name of the chart the export
|
||||
was requested for, the ``viz_type``, and finally a generic "export".
|
||||
|
||||
Used whenever the client hasn't supplied an explicit filename, by
|
||||
both the streaming and non-streaming chart data export responses.
|
||||
"""
|
||||
timestamp = datetime.now().strftime("%Y%m%d_%H%M%S")
|
||||
candidates = (
|
||||
form_data.get("slice_name") if form_data else None,
|
||||
slice_.slice_name if slice_ is not None else None,
|
||||
form_data.get("viz_type") if form_data else None,
|
||||
)
|
||||
|
||||
chart_name = "export"
|
||||
for candidate in candidates:
|
||||
if not isinstance(candidate, str):
|
||||
continue
|
||||
# secure_filename strips a name written entirely in a non-latin
|
||||
# alphabet down to an empty string; skip such candidates so the
|
||||
# filename keeps a meaningful segment.
|
||||
if safe_candidate := secure_filename(candidate):
|
||||
chart_name = safe_candidate
|
||||
break
|
||||
|
||||
# Chart names can be up to 250 characters; cap the name segment so
|
||||
# the whole filename (prefix, timestamp and extension included)
|
||||
# stays within the 255-character single-component limit common to
|
||||
# NTFS, ext4 and APFS.
|
||||
chart_name = chart_name[:150]
|
||||
if form_data and form_data.get("slice_name"):
|
||||
chart_name = form_data["slice_name"]
|
||||
elif form_data and form_data.get("viz_type"):
|
||||
chart_name = form_data["viz_type"]
|
||||
|
||||
return secure_filename(f"superset_{chart_name}_{timestamp}")
|
||||
|
||||
@@ -680,7 +645,6 @@ class ChartDataRestApi(ChartRestApi):
|
||||
expected_rows: int | None = None,
|
||||
add_extra_log_payload: Callable[..., None] | None = None,
|
||||
dashboard_filter_context: DashboardFilterContext | None = None,
|
||||
slice_: Slice | None = None,
|
||||
) -> Response:
|
||||
"""Get data response and optionally log is_cached information."""
|
||||
try:
|
||||
@@ -705,7 +669,6 @@ class ChartDataRestApi(ChartRestApi):
|
||||
filename,
|
||||
expected_rows,
|
||||
dashboard_filter_context=dashboard_filter_context,
|
||||
slice_=slice_,
|
||||
)
|
||||
|
||||
def _extract_export_params_from_request(self) -> tuple[str | None, int | None]:
|
||||
@@ -813,14 +776,13 @@ class ChartDataRestApi(ChartRestApi):
|
||||
form_data: dict[str, Any] | None = None,
|
||||
filename: str | None = None,
|
||||
expected_rows: int | None = None,
|
||||
slice_: Slice | None = None,
|
||||
) -> Response:
|
||||
"""Create a streaming CSV response for large datasets."""
|
||||
query_context = result["query_context"]
|
||||
|
||||
# Use filename from frontend if provided, otherwise generate one
|
||||
if not filename:
|
||||
filename = f"{self._get_default_export_filename(form_data, slice_)}.csv"
|
||||
filename = f"{self._get_default_export_filename(form_data)}.csv"
|
||||
else:
|
||||
# Sanitize the client-provided filename before placing it in the
|
||||
# Content-Disposition header to avoid header/path injection.
|
||||
|
||||
@@ -18,13 +18,11 @@ import logging
|
||||
from functools import partial
|
||||
from typing import Any, Optional
|
||||
|
||||
from jinja2.exceptions import TemplateError
|
||||
from sqlalchemy.exc import SQLAlchemyError
|
||||
|
||||
from superset import db
|
||||
from superset.commands.explore.permalink.base import BaseExplorePermalinkCommand
|
||||
from superset.daos.key_value import KeyValueDAO
|
||||
from superset.exceptions import SupersetTemplateException
|
||||
from superset.explore.permalink.exceptions import ExplorePermalinkCreateFailedError
|
||||
from superset.explore.utils import check_access as check_chart_access
|
||||
from superset.key_value.exceptions import (
|
||||
@@ -60,10 +58,7 @@ class CreateExplorePermalinkCommand(BaseExplorePermalinkCommand):
|
||||
d_id, d_type = self.datasource.split("__")
|
||||
datasource_id = int(d_id)
|
||||
datasource_type = DatasourceType(d_type)
|
||||
try:
|
||||
check_chart_access(datasource_id, self.chart_id, datasource_type)
|
||||
except TemplateError as ex:
|
||||
raise SupersetTemplateException(str(ex)) from ex
|
||||
check_chart_access(datasource_id, self.chart_id, datasource_type)
|
||||
value = {
|
||||
"chartId": self.chart_id,
|
||||
"datasourceId": datasource_id,
|
||||
|
||||
@@ -17,13 +17,11 @@
|
||||
import logging
|
||||
from typing import Optional
|
||||
|
||||
from jinja2.exceptions import TemplateError
|
||||
from sqlalchemy.exc import SQLAlchemyError
|
||||
|
||||
from superset.commands.dataset.exceptions import DatasetNotFoundError
|
||||
from superset.commands.explore.permalink.base import BaseExplorePermalinkCommand
|
||||
from superset.daos.key_value import KeyValueDAO
|
||||
from superset.exceptions import SupersetTemplateException
|
||||
from superset.explore.permalink.exceptions import ExplorePermalinkGetFailedError
|
||||
from superset.explore.permalink.types import ExplorePermalinkValue
|
||||
from superset.explore.utils import check_access as check_chart_access
|
||||
@@ -56,10 +54,7 @@ class GetExplorePermalinkCommand(BaseExplorePermalinkCommand):
|
||||
datasource_type = DatasourceType(
|
||||
value.get("datasourceType", DatasourceType.TABLE)
|
||||
)
|
||||
try:
|
||||
check_chart_access(datasource_id, chart_id, datasource_type)
|
||||
except TemplateError as ex:
|
||||
raise SupersetTemplateException(str(ex)) from ex
|
||||
check_chart_access(datasource_id, chart_id, datasource_type)
|
||||
return value
|
||||
return None
|
||||
except (
|
||||
|
||||
@@ -1620,9 +1620,6 @@ class BaseReportState:
|
||||
(caller should ``return`` without re-raising), or False if the caller
|
||||
should fall through to its own error handling path.
|
||||
"""
|
||||
if not feature_flag_manager.is_feature_enabled("ALERT_REPORTS_RETRY"):
|
||||
return False
|
||||
|
||||
retry_on_failure: bool = self._report_schedule.retry_on_failure
|
||||
if not retry_on_failure:
|
||||
return False
|
||||
@@ -1738,8 +1735,7 @@ class ReportNotTriggeredErrorState(BaseReportState):
|
||||
# retry delay, consider the retry chain dead and let the new window
|
||||
# proceed (e.g., apply_async failed after committing RETRYING).
|
||||
if (
|
||||
feature_flag_manager.is_feature_enabled("ALERT_REPORTS_RETRY")
|
||||
and self._report_schedule.last_state == ReportState.RETRYING
|
||||
self._report_schedule.last_state == ReportState.RETRYING
|
||||
and self._is_retry_window_stale()
|
||||
):
|
||||
max_delay: int = app.config.get(
|
||||
@@ -1771,8 +1767,6 @@ class ReportNotTriggeredErrorState(BaseReportState):
|
||||
return
|
||||
self.send()
|
||||
# Clear any retry state from previous failed attempts in this window.
|
||||
# Always reset on success regardless of feature flag — prevents
|
||||
# stale counters from being reused if the flag is later re-enabled.
|
||||
self._reset_retry_counter()
|
||||
warning_message = (
|
||||
";".join(self._execution_warnings) if self._execution_warnings else None
|
||||
@@ -2062,8 +2056,7 @@ class ReportSuccessState(BaseReportState):
|
||||
raise
|
||||
|
||||
# send() succeeded — clear retry state and log success. Any execution
|
||||
# warnings are incorporated by create_log(). Always reset regardless
|
||||
# of feature flag to prevent stale counters.
|
||||
# warnings are incorporated by create_log().
|
||||
self._reset_retry_counter()
|
||||
self.update_report_schedule_and_log(ReportState.SUCCESS, error_message=None)
|
||||
|
||||
|
||||
@@ -22,7 +22,7 @@ from flask_appbuilder.models.sqla import Model
|
||||
from flask_babel import gettext as _
|
||||
from marshmallow import ValidationError
|
||||
|
||||
from superset import is_feature_enabled, security_manager
|
||||
from superset import security_manager
|
||||
from superset.commands.base import UpdateMixin
|
||||
from superset.commands.report.base import BaseReportScheduleCommand
|
||||
from superset.commands.report.exceptions import (
|
||||
@@ -193,24 +193,23 @@ class UpdateReportScheduleCommand(UpdateMixin, BaseReportScheduleCommand):
|
||||
include_viewers=False,
|
||||
)
|
||||
|
||||
# Validate retry config when the feature is enabled.
|
||||
if is_feature_enabled("ALERT_REPORTS_RETRY"):
|
||||
# Fall back to the existing DB value for fields not in the payload.
|
||||
send_failed = self._properties.get(
|
||||
"send_failed_reports", self._model.send_failed_reports
|
||||
)
|
||||
retry_enabled = self._properties.get(
|
||||
"retry_on_failure", self._model.retry_on_failure
|
||||
)
|
||||
if send_failed and not retry_enabled:
|
||||
msg = _("send_failed_reports requires retry_on_failure to be enabled")
|
||||
exceptions.append(ValidationError({"send_failed_reports": [msg]}))
|
||||
# Validate retry config: send_failed_reports requires retry_on_failure.
|
||||
# Fall back to the existing DB value for fields not in the payload.
|
||||
send_failed = self._properties.get(
|
||||
"send_failed_reports", self._model.send_failed_reports
|
||||
)
|
||||
retry_enabled = self._properties.get(
|
||||
"retry_on_failure", self._model.retry_on_failure
|
||||
)
|
||||
if send_failed and not retry_enabled:
|
||||
msg = _("send_failed_reports requires retry_on_failure to be enabled")
|
||||
exceptions.append(ValidationError({"send_failed_reports": [msg]}))
|
||||
|
||||
# Retries are only supported for reports, not alerts.
|
||||
report_type = self._properties.get("type", self._model.type)
|
||||
if report_type == ReportScheduleType.ALERT and retry_enabled:
|
||||
msg = _("Retries are not supported for alerts")
|
||||
exceptions.append(ValidationError({"retry_on_failure": [msg]}))
|
||||
# Retries are only supported for reports, not alerts.
|
||||
report_type = self._properties.get("type", self._model.type)
|
||||
if report_type == ReportScheduleType.ALERT and retry_enabled:
|
||||
msg = _("Retries are not supported for alerts")
|
||||
exceptions.append(ValidationError({"retry_on_failure": [msg]}))
|
||||
|
||||
if exceptions:
|
||||
raise ReportScheduleInvalidError(exceptions=exceptions)
|
||||
|
||||
@@ -34,7 +34,6 @@ from superset.exceptions import (
|
||||
from superset.models.sql_lab import Query
|
||||
from superset.sqllab.utils import apply_display_max_row_configuration_if_require
|
||||
from superset.utils import core as utils
|
||||
from superset.utils.database import warm_and_release_connection
|
||||
from superset.utils.dates import now_as_float
|
||||
from superset.views.utils import _deserialize_results_payload
|
||||
|
||||
@@ -110,17 +109,6 @@ class SqlExecutionResultsCommand(BaseCommand):
|
||||
status=400,
|
||||
) from ex
|
||||
|
||||
# Release the DB connection back to the pool before the S3 fetch and the
|
||||
# CPU-bound decompress/deserialize/expand work below: none of that needs
|
||||
# the DB, and holding a connection for their duration (which can run well
|
||||
# past this endpoint's client-side timeout for large results) is what
|
||||
# exhausts the small per-worker SQLAlchemy pool when several large-result
|
||||
# downloads land concurrently on the same gunicorn worker. `database` is
|
||||
# warmed first since `_deserialize_results_payload` needs
|
||||
# `self._query.database.db_engine_spec` later in `run()`, after the
|
||||
# connection has been released.
|
||||
warm_and_release_connection(self._query, "database")
|
||||
|
||||
# Now fetch results from backend (query exists, so this is a valid request)
|
||||
read_from_results_backend_start = now_as_float()
|
||||
self._blob = results_backend.get(self._key)
|
||||
|
||||
@@ -759,9 +759,6 @@ DEFAULT_FEATURE_FLAGS: dict[str, bool] = {
|
||||
# @lifecycle: testing
|
||||
# @docs: https://superset.apache.org/docs/configuration/alerts-reports
|
||||
"ALERT_REPORTS": False,
|
||||
# Enables automatic retry functionality for failed report executions
|
||||
# @lifecycle: testing
|
||||
"ALERT_REPORTS_RETRY": False,
|
||||
# Enables Slack V2 integration for Alerts and Reports.
|
||||
# Defaults to True; the legacy Slack v1 path is deprecated and will be removed
|
||||
# in the next major release. Operators must grant the Slack bot both the
|
||||
|
||||
@@ -290,9 +290,6 @@ class TrinoEngineSpec(PrestoBaseEngineSpec):
|
||||
if user_token is not None:
|
||||
http_session = requests.Session()
|
||||
http_session.headers.update({"Authorization": f"Bearer {user_token}"})
|
||||
# Persists `verify` to the new `http_session`
|
||||
if "verify" in connect_args:
|
||||
http_session.verify = connect_args["verify"]
|
||||
connect_args["http_session"] = http_session
|
||||
|
||||
return url, engine_kwargs
|
||||
|
||||
@@ -31,7 +31,6 @@ from superset.commands.dataset.exceptions import (
|
||||
from superset.commands.explore.permalink.create import CreateExplorePermalinkCommand
|
||||
from superset.commands.explore.permalink.get import GetExplorePermalinkCommand
|
||||
from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP
|
||||
from superset.exceptions import SupersetTemplateException
|
||||
from superset.explore.permalink.exceptions import ExplorePermalinkInvalidStateError
|
||||
from superset.explore.permalink.schemas import ExplorePermalinkStateSchema
|
||||
from superset.extensions import event_logger
|
||||
@@ -108,8 +107,6 @@ class ExplorePermalinkRestApi(BaseSupersetApi):
|
||||
return self.response(403, message=str(ex))
|
||||
except (ChartNotFoundError, DatasetNotFoundError) as ex:
|
||||
return self.response(404, message=str(ex))
|
||||
except SupersetTemplateException as ex:
|
||||
return self.response(ex.status, message=str(ex))
|
||||
|
||||
@expose("/permalink/<string:key>", methods=("GET",))
|
||||
@protect()
|
||||
@@ -165,5 +162,3 @@ class ExplorePermalinkRestApi(BaseSupersetApi):
|
||||
return self.response(403, message=str(ex))
|
||||
except (ChartNotFoundError, DatasetNotFoundError) as ex:
|
||||
return self.response(404, message=str(ex))
|
||||
except SupersetTemplateException as ex:
|
||||
return self.response(ex.status, message=str(ex))
|
||||
|
||||
@@ -125,8 +125,8 @@ Available tools:
|
||||
|
||||
Dashboard Management:
|
||||
- list_dashboards: List dashboards with advanced filters (1-based pagination; deleted_state='only'/'include' surfaces trashed dashboards the caller may restore)
|
||||
- get_dashboard_info: Resolve a dashboard by ID/UUID/slug or shared /dashboard/p/<key>/ permalink, including its active-tab and filter state
|
||||
- get_dashboard_layout: Get parsed tabs and chart positions by dashboard identifier or shared permalink, including the permalink's active-tab and filter context
|
||||
- get_dashboard_info: Get detailed dashboard information by ID
|
||||
- get_dashboard_layout: Get parsed tabs and chart positions for a dashboard (companion to get_dashboard_info when its omitted_fields hint flags position_json)
|
||||
- get_dashboard_datasets: List the datasets used by a dashboard's charts, with columns and metrics (context for configuring native filters)
|
||||
- generate_dashboard: Create a dashboard from chart IDs (requires write access)
|
||||
- update_dashboard: Update an existing dashboard's title/description/slug/published/layout/theme/CSS (requires write access; editorship-checked per-instance)
|
||||
|
||||
@@ -26,16 +26,18 @@ from typing import Any, Dict, List, TYPE_CHECKING
|
||||
from fastmcp import Context
|
||||
from flask import current_app
|
||||
from sqlalchemy.exc import SQLAlchemyError
|
||||
from sqlalchemy.orm import subqueryload
|
||||
from sqlalchemy.orm import scoped_session, subqueryload
|
||||
from superset_core.mcp.decorators import tool, ToolAnnotations
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from superset.models.slice import Slice
|
||||
|
||||
from superset.charts.data.form_data import set_query_context_form_data
|
||||
from superset.commands.exceptions import CommandException
|
||||
from superset.exceptions import OAuth2Error, OAuth2RedirectError, SupersetException
|
||||
from superset.extensions import event_logger
|
||||
from superset.extensions import db, event_logger
|
||||
from superset.mcp_service import guest_scope
|
||||
from superset.mcp_service.chart.chart_helpers import (
|
||||
build_query_context_from_form_data,
|
||||
@@ -63,6 +65,19 @@ from superset.utils.core import GenericDataType
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _request_session() -> "Session":
|
||||
"""Return the ``Session`` backing ``db.session``.
|
||||
|
||||
``db.session`` is a ``scoped_session`` proxy, and assigning a session
|
||||
option such as ``expire_on_commit`` on the proxy does not reach the
|
||||
``Session`` it wraps -- the assignment lands on the proxy object and is
|
||||
silently ignored. Callers that need to change session behavior therefore
|
||||
have to resolve the underlying ``Session`` first.
|
||||
"""
|
||||
session = db.session
|
||||
return session() if isinstance(session, scoped_session) else session
|
||||
|
||||
|
||||
def _requested_filter_columns(extra_form_data: dict[str, Any] | None) -> set[str]:
|
||||
"""Return simple column names explicitly requested through extra form data."""
|
||||
if not extra_form_data:
|
||||
@@ -358,6 +373,19 @@ async def get_chart_data( # noqa: C901
|
||||
)
|
||||
effective_force = _compute_effective_force(request)
|
||||
|
||||
# The chart is fetched once below and then read from for the rest of this
|
||||
# call. Every event_logger.log_context block here commits the request
|
||||
# session on exit (DBEventLogger.log), and SQLAlchemy expires an
|
||||
# instance's loaded columns on commit. The per-call session can also be
|
||||
# removed while the call is still in flight (see
|
||||
# superset/mcp_service/session_scope.py), which detaches that Slice --
|
||||
# and reading an expired column off a detached instance raises
|
||||
# DetachedInstanceError. Keeping the columns loaded across those commits
|
||||
# means the already-fetched chart stays readable either way.
|
||||
session = _request_session()
|
||||
expire_on_commit = session.expire_on_commit
|
||||
session.expire_on_commit = False
|
||||
|
||||
try:
|
||||
await ctx.report_progress(1, 4, "Looking up chart")
|
||||
from superset.utils import json as utils_json
|
||||
@@ -1026,6 +1054,8 @@ async def get_chart_data( # noqa: C901
|
||||
return ChartError(
|
||||
error=f"Failed to get chart data: {str(e)}", error_type="InternalError"
|
||||
)
|
||||
finally:
|
||||
session.expire_on_commit = expire_on_commit
|
||||
|
||||
|
||||
async def _query_from_form_data( # noqa: C901
|
||||
|
||||
@@ -1,312 +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.
|
||||
|
||||
"""Validation for dashboard layouts supplied through MCP tools."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from collections.abc import Collection
|
||||
from typing import Any
|
||||
|
||||
_ROOT_ID = "ROOT_ID"
|
||||
_GRID_ID = "GRID_ID"
|
||||
_HEADER_ID = "HEADER_ID"
|
||||
_VERSION_KEY = "DASHBOARD_VERSION_KEY"
|
||||
_CHART_TYPE = "CHART"
|
||||
# Chart IDs are database integers; a decimal string longer than a 64-bit value
|
||||
# is malformed input rather than an ID that could ever resolve.
|
||||
_MAX_CHART_ID_DIGITS = 19
|
||||
|
||||
# Keep in sync with the frontend's parent/child contract in
|
||||
# superset-frontend/src/dashboard/util/isValidChild.ts, which admits a child
|
||||
# only when both its type and its parent's nesting depth are allowed. The
|
||||
# values below are that file's ``parentMaxDepthLookup``; traversal is iterative
|
||||
# so deeply nested input cannot overflow Python's call stack.
|
||||
_ROOT_DEPTH = 0
|
||||
_DEPTH_ONE = _ROOT_DEPTH + 1
|
||||
_DEPTH_THREE = _ROOT_DEPTH + 3
|
||||
_DEPTH_FOUR = _ROOT_DEPTH + 4
|
||||
_DEPTH_FIVE = _ROOT_DEPTH + 5
|
||||
|
||||
_PARENT_MAX_DEPTH: dict[str, dict[str, int]] = {
|
||||
"ROOT": {"GRID": _ROOT_DEPTH, "TABS": _ROOT_DEPTH},
|
||||
"GRID": {
|
||||
"CHART": _DEPTH_ONE,
|
||||
"COLUMN": _DEPTH_ONE,
|
||||
"DIVIDER": _DEPTH_ONE,
|
||||
"DYNAMIC": _DEPTH_ONE,
|
||||
"HEADER": _DEPTH_ONE,
|
||||
"MARKDOWN": _DEPTH_ONE,
|
||||
"ROW": _DEPTH_ONE,
|
||||
"TABS": _DEPTH_ONE,
|
||||
},
|
||||
"ROW": {
|
||||
"CHART": _DEPTH_FOUR,
|
||||
"COLUMN": _DEPTH_FOUR,
|
||||
"DYNAMIC": _DEPTH_FOUR,
|
||||
"MARKDOWN": _DEPTH_FOUR,
|
||||
},
|
||||
"TABS": {"TAB": _DEPTH_THREE},
|
||||
"TAB": {
|
||||
"CHART": _DEPTH_FIVE,
|
||||
"COLUMN": _DEPTH_THREE,
|
||||
"DIVIDER": _DEPTH_FIVE,
|
||||
"DYNAMIC": _DEPTH_FIVE,
|
||||
"HEADER": _DEPTH_FIVE,
|
||||
"MARKDOWN": _DEPTH_FIVE,
|
||||
"ROW": _DEPTH_THREE,
|
||||
"TABS": _DEPTH_THREE,
|
||||
},
|
||||
"COLUMN": {
|
||||
"CHART": _DEPTH_FIVE,
|
||||
"DIVIDER": _DEPTH_THREE,
|
||||
"HEADER": _DEPTH_FIVE,
|
||||
"MARKDOWN": _DEPTH_FIVE,
|
||||
"ROW": _DEPTH_THREE,
|
||||
"TABS": _DEPTH_THREE,
|
||||
},
|
||||
"CHART": {},
|
||||
"DIVIDER": {},
|
||||
"DYNAMIC": {},
|
||||
"HEADER": {},
|
||||
"MARKDOWN": {},
|
||||
}
|
||||
_ALLOWED_CHILD_TYPES: dict[str, frozenset[str]] = {
|
||||
parent_type: frozenset(child_depths)
|
||||
for parent_type, child_depths in _PARENT_MAX_DEPTH.items()
|
||||
}
|
||||
# TABS and TAB deliberately render their children at their own depth; every
|
||||
# other container increments it. See the worked examples in isValidChild.ts.
|
||||
_DEPTH_TRANSPARENT_TYPES = frozenset({"TABS", "TAB"})
|
||||
_CONTAINER_TYPES = frozenset(
|
||||
component_type
|
||||
for component_type, child_types in _ALLOWED_CHILD_TYPES.items()
|
||||
if child_types
|
||||
)
|
||||
_META_REQUIRED_TYPES = frozenset(_ALLOWED_CHILD_TYPES) - {"ROOT", "GRID"}
|
||||
|
||||
|
||||
def normalize_chart_id(value: Any) -> int | None:
|
||||
"""Normalize an integer or canonical decimal-string chart ID.
|
||||
|
||||
Only canonical decimal strings are accepted. Leading-zero forms such as
|
||||
``"007"`` are rejected so that layout lookups and the ``json_metadata``
|
||||
cleanup in ``remove_chart_from_dashboard`` — which keys off
|
||||
``str(chart_id)`` — cannot disagree about whether a reference matches and
|
||||
leave stale references behind. The digit bound keeps ``int()`` away from
|
||||
CPython's integer string conversion limit, which would otherwise raise
|
||||
``ValueError`` out of the validator instead of returning a structured
|
||||
error; no real chart ID approaches it.
|
||||
"""
|
||||
if isinstance(value, bool):
|
||||
return None
|
||||
if isinstance(value, int):
|
||||
return value if value > 0 else None
|
||||
if (
|
||||
isinstance(value, str)
|
||||
and value.isascii()
|
||||
and value.isdecimal()
|
||||
and len(value) <= _MAX_CHART_ID_DIGITS
|
||||
and not value.startswith("0")
|
||||
):
|
||||
return int(value)
|
||||
return None
|
||||
|
||||
|
||||
def _validate_component_shapes( # noqa: C901
|
||||
layout: dict[str, Any],
|
||||
) -> tuple[dict[str, dict[str, Any]], str | None]:
|
||||
"""Validate and return every component object in a raw layout mapping."""
|
||||
if layout.get(_VERSION_KEY) != "v2":
|
||||
return {}, f"{_VERSION_KEY} must be the string 'v2'."
|
||||
|
||||
components: dict[str, dict[str, Any]] = {}
|
||||
for component_id, component in layout.items():
|
||||
if component_id == _VERSION_KEY:
|
||||
continue
|
||||
if not isinstance(component, dict):
|
||||
return {}, f"Layout value {component_id} must be a component object."
|
||||
if component.get("id") != component_id:
|
||||
return {}, f"Layout component {component_id} must have the same id value."
|
||||
|
||||
component_type = component.get("type")
|
||||
if not isinstance(component_type, str) or component_type not in (
|
||||
_ALLOWED_CHILD_TYPES
|
||||
):
|
||||
return {}, f"Layout component {component_id} has unsupported type."
|
||||
if component_type == "DYNAMIC":
|
||||
return {}, (
|
||||
f"Layout component {component_id} uses DYNAMIC, which cannot be "
|
||||
"safely validated by the server."
|
||||
)
|
||||
|
||||
children = component.get("children")
|
||||
if component_type in _CONTAINER_TYPES and children is None:
|
||||
return {}, f"Layout component {component_id} must define children."
|
||||
if children is not None and (
|
||||
not isinstance(children, list)
|
||||
or not all(isinstance(child_id, str) for child_id in children)
|
||||
):
|
||||
return {}, f"Layout component {component_id}.children must be a list."
|
||||
if component_type not in _CONTAINER_TYPES and children not in (None, []):
|
||||
return {}, f"Layout component {component_id} cannot have children."
|
||||
if component_type == "TABS" and not children:
|
||||
return {}, f"Tabs component {component_id} must contain at least one tab."
|
||||
if component_type in _META_REQUIRED_TYPES and not isinstance(
|
||||
component.get("meta"), dict
|
||||
):
|
||||
return {}, f"Layout component {component_id}.meta must be an object."
|
||||
|
||||
components[component_id] = component
|
||||
|
||||
return components, None
|
||||
|
||||
|
||||
def _validate_edges(
|
||||
components: dict[str, dict[str, Any]],
|
||||
) -> tuple[dict[str, str], str | None]:
|
||||
"""Validate graph edges and return each component's actual parent."""
|
||||
parent_by_child: dict[str, str] = {}
|
||||
for parent_id, parent in components.items():
|
||||
parent_type = parent["type"]
|
||||
for child_id in parent.get("children") or []:
|
||||
child = components.get(child_id)
|
||||
if child is None:
|
||||
return {}, f"Layout references missing component {child_id}."
|
||||
if child["type"] not in _ALLOWED_CHILD_TYPES[parent_type]:
|
||||
return {}, (
|
||||
f"Layout component {child_id} cannot be a child of {parent_id}."
|
||||
)
|
||||
if child_id in parent_by_child:
|
||||
return {}, f"Layout component {child_id} has more than one parent."
|
||||
parent_by_child[child_id] = parent_id
|
||||
|
||||
if _ROOT_ID in parent_by_child:
|
||||
return {}, "ROOT_ID must not have a parent."
|
||||
return parent_by_child, None
|
||||
|
||||
|
||||
def _find_cycle(components: dict[str, dict[str, Any]]) -> str | None:
|
||||
"""Return a component ID in a cycle using an iterative depth-first walk."""
|
||||
state: dict[str, int] = {}
|
||||
for start_id in components:
|
||||
if state.get(start_id) == 2:
|
||||
continue
|
||||
stack: list[tuple[str, bool]] = [(start_id, False)]
|
||||
while stack:
|
||||
component_id, exiting = stack.pop()
|
||||
if exiting:
|
||||
state[component_id] = 2
|
||||
continue
|
||||
if state.get(component_id) == 1:
|
||||
return component_id
|
||||
if state.get(component_id) == 2:
|
||||
continue
|
||||
state[component_id] = 1
|
||||
stack.append((component_id, True))
|
||||
for child_id in reversed(components[component_id].get("children") or []):
|
||||
stack.append((child_id, False))
|
||||
return None
|
||||
|
||||
|
||||
def validate_dashboard_layout( # noqa: C901
|
||||
layout: dict[str, Any], expected_chart_ids: Collection[int]
|
||||
) -> str | None:
|
||||
"""Return an error when an MCP layout replacement is unsafe to persist.
|
||||
|
||||
Superset renders only components reachable from ``ROOT_ID`` but indexes all
|
||||
chart nodes during hydration. This validates renderer-required component
|
||||
shape and graph topology, then requires the reachable charts to match the
|
||||
dashboard's associated charts before allowing a full replacement.
|
||||
|
||||
``HEADER_ID`` is dashboard metadata rather than a rendered tree child.
|
||||
Superset also retains an empty, detached ``GRID_ID`` when top-level tabs are
|
||||
used; both are allowed as explicit reserved-node exceptions.
|
||||
"""
|
||||
components, error = _validate_component_shapes(layout)
|
||||
if error:
|
||||
return error
|
||||
|
||||
root = components.get(_ROOT_ID)
|
||||
if root is None or root.get("type") != "ROOT":
|
||||
return "Layout must contain a ROOT_ID component with type ROOT."
|
||||
root_children = root.get("children") or []
|
||||
if len(root_children) != 1:
|
||||
return "ROOT_ID must contain exactly one GRID or TABS component."
|
||||
|
||||
parent_by_child, error = _validate_edges(components)
|
||||
if error:
|
||||
return error
|
||||
if cycle_id := _find_cycle(components):
|
||||
return f"Layout contains a cycle at {cycle_id}."
|
||||
|
||||
visited: set[str] = set()
|
||||
reachable_chart_ids: set[int] = set()
|
||||
# The frontend treats ``parents`` as derived metadata and recomputes it
|
||||
# during hydration. Saved layouts can therefore omit it or retain stale
|
||||
# values after drag-and-drop; the validated child edges are authoritative.
|
||||
# Depth is likewise derived here rather than trusted, and is only defined
|
||||
# for reachable nodes, so the nesting limits are checked on this walk.
|
||||
stack: list[tuple[str, int]] = [(_ROOT_ID, _ROOT_DEPTH)]
|
||||
while stack:
|
||||
component_id, depth = stack.pop()
|
||||
component = components[component_id]
|
||||
component_type = component["type"]
|
||||
visited.add(component_id)
|
||||
|
||||
if component_type == _CHART_TYPE:
|
||||
chart_id = normalize_chart_id(component["meta"].get("chartId"))
|
||||
if chart_id is None:
|
||||
return (
|
||||
f"Chart component {component_id} must have a positive integer "
|
||||
"or decimal-string chartId."
|
||||
)
|
||||
reachable_chart_ids.add(chart_id)
|
||||
|
||||
child_depth = depth if component_type in _DEPTH_TRANSPARENT_TYPES else depth + 1
|
||||
for child_id in reversed(component.get("children") or []):
|
||||
# ``_validate_edges`` already accepted this parent/child type pair,
|
||||
# so a missing entry here is impossible.
|
||||
if depth > _PARENT_MAX_DEPTH[component_type][components[child_id]["type"]]:
|
||||
return (
|
||||
f"Layout component {child_id} is nested too deeply under "
|
||||
f"{component_id}."
|
||||
)
|
||||
stack.append((child_id, child_depth))
|
||||
|
||||
top_level_type = components[root_children[0]]["type"]
|
||||
for component_id, component in components.items():
|
||||
if component_id in visited:
|
||||
continue
|
||||
if component_id == _HEADER_ID and component["type"] == "HEADER":
|
||||
continue
|
||||
if (
|
||||
component_id == _GRID_ID
|
||||
and top_level_type == "TABS"
|
||||
and component["type"] == "GRID"
|
||||
and component.get("children") == []
|
||||
):
|
||||
continue
|
||||
return f"Layout component {component_id} is unreachable from ROOT_ID."
|
||||
|
||||
expected = set(expected_chart_ids)
|
||||
if missing := sorted(expected - reachable_chart_ids):
|
||||
return f"Layout would hide dashboard charts: {missing}."
|
||||
if unknown := sorted(reachable_chart_ids - expected):
|
||||
return f"Layout references charts not associated with the dashboard: {unknown}."
|
||||
|
||||
return None
|
||||
@@ -1,184 +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.
|
||||
|
||||
"""Helpers for resolving dashboard permalink keys and shared URLs."""
|
||||
|
||||
import logging
|
||||
from dataclasses import dataclass
|
||||
from typing import Callable, Generic, TypeVar
|
||||
from urllib.parse import urlparse
|
||||
|
||||
from flask import g, has_request_context
|
||||
|
||||
from superset.commands.dashboard.exceptions import DashboardAccessDeniedError
|
||||
from superset.commands.dashboard.permalink.get import GetDashboardPermalinkCommand
|
||||
from superset.dashboards.permalink.exceptions import DashboardPermalinkGetFailedError
|
||||
from superset.dashboards.permalink.types import DashboardPermalinkValue
|
||||
from superset.mcp_service.auth import load_user_with_relationships
|
||||
from superset.mcp_service.dashboard.schemas import (
|
||||
redact_filter_state_data_model_metadata,
|
||||
)
|
||||
from superset.mcp_service.privacy import user_can_view_data_model_metadata
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
LookupResultT = TypeVar("LookupResultT")
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class DashboardLookupResult(Generic[LookupResultT]):
|
||||
"""Result of resolving either a dashboard identifier or permalink."""
|
||||
|
||||
result: LookupResultT | None
|
||||
permalink_key: str | None = None
|
||||
permalink_value: DashboardPermalinkValue | None = None
|
||||
resolved_from_permalink: bool = False
|
||||
"""True when the dashboard itself was selected from the permalink."""
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class DashboardPermalinkState:
|
||||
"""Permalink state belonging to a resolved dashboard."""
|
||||
|
||||
key: str
|
||||
state: dict[str, object]
|
||||
|
||||
|
||||
def extract_dashboard_permalink_key(value: str) -> str:
|
||||
"""Return a key from a dashboard permalink URL, or the bare input."""
|
||||
path_parts = [part for part in urlparse(value).path.split("/") if part]
|
||||
if len(path_parts) >= 3 and path_parts[-3:-1] == ["dashboard", "p"]:
|
||||
return path_parts[-1]
|
||||
return value
|
||||
|
||||
|
||||
def refresh_request_user_for_permalink_access() -> None:
|
||||
"""Reload the request user before permalink access checks."""
|
||||
if not has_request_context() or not getattr(g, "user", None):
|
||||
return
|
||||
current_user = g.user
|
||||
if getattr(current_user, "is_anonymous", False):
|
||||
return
|
||||
username = getattr(current_user, "username", None)
|
||||
email = getattr(current_user, "email", None)
|
||||
if not username and not email:
|
||||
return
|
||||
refreshed_user = (
|
||||
load_user_with_relationships(username=username)
|
||||
if username
|
||||
else load_user_with_relationships(email=email)
|
||||
)
|
||||
if refreshed_user is not None:
|
||||
g.user = refreshed_user
|
||||
|
||||
|
||||
def get_dashboard_permalink(
|
||||
key_or_url: str,
|
||||
) -> tuple[str, DashboardPermalinkValue] | None:
|
||||
"""Resolve a dashboard permalink key or shared URL, returning its state."""
|
||||
key = extract_dashboard_permalink_key(key_or_url)
|
||||
refresh_request_user_for_permalink_access()
|
||||
try:
|
||||
value = GetDashboardPermalinkCommand(key).run()
|
||||
except (DashboardAccessDeniedError, DashboardPermalinkGetFailedError) as ex:
|
||||
logger.info("Dashboard permalink could not be resolved: %s", ex)
|
||||
return None
|
||||
return (key, value) if value else None
|
||||
|
||||
|
||||
def lookup_dashboard_reference(
|
||||
*,
|
||||
identifier: int | str | None,
|
||||
permalink_key: str | None,
|
||||
lookup: Callable[[int | str], LookupResultT],
|
||||
is_found: Callable[[LookupResultT], bool],
|
||||
) -> DashboardLookupResult[LookupResultT]:
|
||||
"""Look up a dashboard while preserving identifier precedence.
|
||||
|
||||
A supplied identifier selects the dashboard and an explicit permalink only
|
||||
contributes state. Shared permalink URLs and permalink-only requests select
|
||||
the dashboard embedded in the permalink. Ambiguous bare strings use normal
|
||||
identifier lookup first, then fall back to permalink resolution.
|
||||
"""
|
||||
key = permalink_key
|
||||
identifier_is_permalink_url = False
|
||||
if isinstance(identifier, str):
|
||||
extracted_key = extract_dashboard_permalink_key(identifier)
|
||||
identifier_is_permalink_url = extracted_key != identifier
|
||||
if identifier_is_permalink_url:
|
||||
key = extracted_key
|
||||
|
||||
if identifier is not None and not identifier_is_permalink_url:
|
||||
result = lookup(identifier)
|
||||
if is_found(result):
|
||||
resolved = get_dashboard_permalink(key) if key else None
|
||||
return DashboardLookupResult(
|
||||
result=result,
|
||||
permalink_key=resolved[0] if resolved else key,
|
||||
permalink_value=resolved[1] if resolved else None,
|
||||
)
|
||||
if permalink_key is not None or not isinstance(identifier, str):
|
||||
return DashboardLookupResult(result=result, permalink_key=key)
|
||||
else:
|
||||
result = None
|
||||
|
||||
reference = key or (identifier if isinstance(identifier, str) else None)
|
||||
resolved = get_dashboard_permalink(reference) if reference else None
|
||||
if resolved is None:
|
||||
return DashboardLookupResult(result=result, permalink_key=reference)
|
||||
key, value = resolved
|
||||
return DashboardLookupResult(
|
||||
result=lookup(value["dashboardId"]),
|
||||
permalink_key=key,
|
||||
permalink_value=value,
|
||||
resolved_from_permalink=True,
|
||||
)
|
||||
|
||||
|
||||
def get_matching_dashboard_permalink_state(
|
||||
lookup_result: DashboardLookupResult[LookupResultT],
|
||||
dashboard_id: int | None,
|
||||
dashboard_uuid: str | None = None,
|
||||
dashboard_slug: str | None = None,
|
||||
) -> DashboardPermalinkState | None:
|
||||
"""Return the permalink state when it belongs to the dashboard.
|
||||
|
||||
``CreateDashboardPermalinkCommand`` stores ``dashboardId`` as the dashboard
|
||||
UUID string, while older permalinks may hold a numeric ID or a slug, so the
|
||||
reference is compared against every identifier the dashboard answers to.
|
||||
"""
|
||||
value = lookup_result.permalink_value
|
||||
key = lookup_result.permalink_key
|
||||
if value is None or key is None:
|
||||
return None
|
||||
if not lookup_result.resolved_from_permalink:
|
||||
# The identifier selected the dashboard, so the permalink only
|
||||
# contributes state when it points at that same dashboard.
|
||||
reference = value.get("dashboardId")
|
||||
known_identifiers = {
|
||||
str(candidate)
|
||||
for candidate in (dashboard_id, dashboard_uuid, dashboard_slug)
|
||||
if candidate is not None
|
||||
}
|
||||
if reference is None or str(reference) not in known_identifiers:
|
||||
return None
|
||||
|
||||
raw_state = value.get("state")
|
||||
state: dict[str, object] = dict(raw_state) if isinstance(raw_state, dict) else {}
|
||||
if not user_can_view_data_model_metadata():
|
||||
state = redact_filter_state_data_model_metadata(state)
|
||||
return DashboardPermalinkState(key=key, state=state)
|
||||
@@ -242,7 +242,7 @@ DEFAULT_GET_DASHBOARD_INFO_COLUMNS: List[str] = [
|
||||
|
||||
|
||||
class GetDashboardInfoRequest(MetadataCacheControl):
|
||||
"""Request schema for dashboard identifiers and shared permalink URLs.
|
||||
"""Request schema for get_dashboard_info with support for ID, UUID, or slug.
|
||||
|
||||
When permalink_key is provided, the tool will retrieve the dashboard's filter
|
||||
state from the permalink, allowing you to see what filters the user has applied
|
||||
@@ -253,23 +253,21 @@ class GetDashboardInfoRequest(MetadataCacheControl):
|
||||
model_config = ConfigDict(populate_by_name=True)
|
||||
|
||||
identifier: Annotated[
|
||||
int | str | None,
|
||||
int | str,
|
||||
Field(
|
||||
description=(
|
||||
"Dashboard ID, UUID, slug, bare permalink key, or a shared URL "
|
||||
"containing /superset/dashboard/p/<key>/. Omit when "
|
||||
"permalink_key is provided."
|
||||
"Dashboard identifier - can be numeric ID, UUID string, or slug"
|
||||
),
|
||||
default=None,
|
||||
validation_alias=AliasChoices("identifier", "id", "dashboard_id"),
|
||||
),
|
||||
]
|
||||
permalink_key: str | None = Field(
|
||||
default=None,
|
||||
description=(
|
||||
"Key from a shared dashboard URL such as "
|
||||
"'/superset/dashboard/p/<key>/'. Resolves the dashboard and returns "
|
||||
"the shared active-tab and filter context; no identifier is required."
|
||||
"Optional permalink key for retrieving dashboard filter state. When a "
|
||||
"user applies filters in a dashboard, the state can be persisted in a "
|
||||
"permalink. If provided, the tool returns the filter configuration "
|
||||
"from that permalink."
|
||||
),
|
||||
)
|
||||
select_columns: Annotated[
|
||||
@@ -297,57 +295,16 @@ class GetDashboardInfoRequest(MetadataCacheControl):
|
||||
parsed = parse_json_or_list(value, "select_columns")
|
||||
return parsed if parsed else list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)
|
||||
|
||||
@model_validator(mode="after")
|
||||
def _require_identifier_or_permalink(self) -> "GetDashboardInfoRequest":
|
||||
identifier_is_blank = self.identifier is None or (
|
||||
isinstance(self.identifier, str) and not self.identifier.strip()
|
||||
)
|
||||
permalink_is_blank = (
|
||||
self.permalink_key is None or not self.permalink_key.strip()
|
||||
)
|
||||
if identifier_is_blank and permalink_is_blank:
|
||||
raise ValueError("Provide identifier or permalink_key")
|
||||
return self
|
||||
|
||||
|
||||
class GetDashboardLayoutRequest(BaseModel):
|
||||
"""Request a dashboard layout by its identifier or shared permalink.
|
||||
|
||||
Permalink requests resolve the dashboard while preserving shared active-tab
|
||||
and filter state in the response.
|
||||
"""
|
||||
"""Request schema for get_dashboard_layout."""
|
||||
|
||||
identifier: Annotated[
|
||||
int | str | None,
|
||||
int | str,
|
||||
Field(
|
||||
default=None,
|
||||
description=(
|
||||
"Dashboard ID, UUID, slug, bare permalink key, or a shared URL "
|
||||
"containing /superset/dashboard/p/<key>/. Omit when "
|
||||
"permalink_key is provided."
|
||||
),
|
||||
description="Dashboard identifier - can be numeric ID, UUID string, or slug"
|
||||
),
|
||||
]
|
||||
permalink_key: str | None = Field(
|
||||
default=None,
|
||||
description=(
|
||||
"Key from a shared dashboard URL such as "
|
||||
"'/superset/dashboard/p/<key>/'. Resolves the dashboard and includes "
|
||||
"the shared active-tab and filter context in the layout response."
|
||||
),
|
||||
)
|
||||
|
||||
@model_validator(mode="after")
|
||||
def _require_identifier_or_permalink(self) -> "GetDashboardLayoutRequest":
|
||||
identifier_is_blank = self.identifier is None or (
|
||||
isinstance(self.identifier, str) and not self.identifier.strip()
|
||||
)
|
||||
permalink_is_blank = (
|
||||
self.permalink_key is None or not self.permalink_key.strip()
|
||||
)
|
||||
if identifier_is_blank and permalink_is_blank:
|
||||
raise ValueError("Provide identifier or permalink_key")
|
||||
return self
|
||||
|
||||
|
||||
class GetDashboardDatasetsRequest(BaseModel):
|
||||
@@ -805,11 +762,9 @@ class UpdateDashboardRequest(BaseModel):
|
||||
None,
|
||||
description=(
|
||||
"Optional replacement layout (Superset's position_json dict). "
|
||||
"When set, fully replaces the existing layout and must keep every "
|
||||
"dashboard chart reachable from ROOT_ID, with consistent children "
|
||||
"and parents. Do not use this field for incremental edits: MCP does "
|
||||
"not currently expose the complete raw layout tree needed to safely "
|
||||
"round-trip a replacement. Prefer purpose-built dashboard tools."
|
||||
"When set, fully replaces the existing layout. Get the current "
|
||||
"layout via ``get_dashboard_info`` first if you want to make "
|
||||
"incremental changes."
|
||||
),
|
||||
)
|
||||
json_metadata_overrides: Dict[str, Any] | None = Field(
|
||||
@@ -1496,20 +1451,6 @@ class DashboardLayout(BaseModel):
|
||||
default=False,
|
||||
description="False when position_json is missing or empty",
|
||||
)
|
||||
permalink_key: str | None = Field(
|
||||
None, description="Resolved key when the input was a dashboard permalink"
|
||||
)
|
||||
filter_state: Dict[str, Any] | None = Field(
|
||||
None,
|
||||
description=(
|
||||
"Shared dashboard state, including activeTabs, anchor, dataMask, "
|
||||
"chartStates, and urlParams when present."
|
||||
),
|
||||
)
|
||||
is_permalink_state: bool = Field(
|
||||
False,
|
||||
description="True when filter_state was resolved from a dashboard permalink",
|
||||
)
|
||||
|
||||
|
||||
def _parse_json_metadata(json_metadata_str: str | None) -> Dict[str, Any] | None:
|
||||
|
||||
@@ -27,27 +27,51 @@ from datetime import datetime, timezone
|
||||
from typing import Any
|
||||
|
||||
from fastmcp import Context
|
||||
from flask import g, has_request_context
|
||||
from sqlalchemy.orm import subqueryload
|
||||
from superset_core.mcp.decorators import tool, ToolAnnotations
|
||||
|
||||
from superset.dashboards.permalink.exceptions import DashboardPermalinkGetFailedError
|
||||
from superset.dashboards.permalink.types import DashboardPermalinkValue
|
||||
from superset.extensions import event_logger
|
||||
from superset.mcp_service.dashboard.permalink import (
|
||||
DashboardLookupResult,
|
||||
get_matching_dashboard_permalink_state,
|
||||
lookup_dashboard_reference,
|
||||
)
|
||||
from superset.mcp_service.auth import load_user_with_relationships
|
||||
from superset.mcp_service.dashboard.schemas import (
|
||||
dashboard_serializer,
|
||||
DashboardError,
|
||||
DashboardInfo,
|
||||
DEFAULT_GET_DASHBOARD_INFO_COLUMNS,
|
||||
GetDashboardInfoRequest,
|
||||
redact_filter_state_data_model_metadata,
|
||||
)
|
||||
from superset.mcp_service.mcp_core import ModelGetInfoCore
|
||||
from superset.mcp_service.privacy import user_can_view_data_model_metadata
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _refresh_request_user_for_permalink_access() -> None:
|
||||
"""Reload the request user before permalink access checks."""
|
||||
if not has_request_context() or not getattr(g, "user", None):
|
||||
return
|
||||
|
||||
current_user = g.user
|
||||
if getattr(current_user, "is_anonymous", False):
|
||||
return
|
||||
|
||||
username = getattr(current_user, "username", None)
|
||||
email = getattr(current_user, "email", None)
|
||||
if not username and not email:
|
||||
return
|
||||
|
||||
refreshed_user = (
|
||||
load_user_with_relationships(username=username)
|
||||
if username
|
||||
else load_user_with_relationships(email=email)
|
||||
)
|
||||
if refreshed_user is not None:
|
||||
g.user = refreshed_user
|
||||
|
||||
|
||||
def _apply_permalink_state(
|
||||
result: DashboardInfo,
|
||||
permalink_key: str,
|
||||
@@ -63,30 +87,19 @@ def _apply_permalink_state(
|
||||
)
|
||||
|
||||
|
||||
def _lookup_dashboard(
|
||||
tool: ModelGetInfoCore,
|
||||
request: GetDashboardInfoRequest,
|
||||
) -> tuple[
|
||||
DashboardInfo | DashboardError,
|
||||
DashboardLookupResult[DashboardInfo | DashboardError],
|
||||
]:
|
||||
"""Resolve an ordinary identifier or dashboard permalink, then run lookup."""
|
||||
lookup_result = lookup_dashboard_reference(
|
||||
identifier=request.identifier,
|
||||
permalink_key=request.permalink_key,
|
||||
lookup=tool.run_tool,
|
||||
is_found=lambda result: isinstance(result, DashboardInfo),
|
||||
)
|
||||
result = lookup_result.result
|
||||
if result is None:
|
||||
# Only reachable when the dashboard had to come from a permalink, so the
|
||||
# identifier's own "not found" error (when there is one) is preserved.
|
||||
result = DashboardError.create(
|
||||
"Dashboard permalink could not be resolved. It may be invalid or "
|
||||
"expired; ask for a fresh shared dashboard link.",
|
||||
"permalink_not_found",
|
||||
)
|
||||
return result, lookup_result
|
||||
def _get_permalink_state(permalink_key: str) -> DashboardPermalinkValue | None:
|
||||
"""Retrieve dashboard filter state from permalink.
|
||||
|
||||
Returns the permalink value containing dashboardId and state if found,
|
||||
None otherwise.
|
||||
"""
|
||||
from superset.commands.dashboard.permalink.get import GetDashboardPermalinkCommand
|
||||
|
||||
try:
|
||||
return GetDashboardPermalinkCommand(permalink_key).run()
|
||||
except DashboardPermalinkGetFailedError as e:
|
||||
logger.warning("Failed to retrieve permalink state: %s", e)
|
||||
return None
|
||||
|
||||
|
||||
@tool(
|
||||
@@ -102,7 +115,7 @@ async def get_dashboard_info(
|
||||
request: GetDashboardInfoRequest, ctx: Context
|
||||
) -> dict[str, Any] | DashboardError:
|
||||
"""
|
||||
Get dashboard metadata by ID, UUID, slug, or dashboard permalink.
|
||||
Get dashboard metadata by ID, UUID, or slug.
|
||||
|
||||
Returns title, charts, and layout details.
|
||||
|
||||
@@ -114,9 +127,9 @@ async def get_dashboard_info(
|
||||
with ``filters=[{"col": "dashboards", "opr": "eq", "value": <dashboard
|
||||
id>}]`` and page through the results using ``page``/``page_size``.
|
||||
|
||||
If the user gives you a shared URL containing ``/dashboard/p/<key>/``, pass
|
||||
the URL or bare key as ``identifier`` (or use ``permalink_key`` alone). The
|
||||
response includes the dashboard ID plus active tab and filter state.
|
||||
When permalink_key is provided, also returns the filter state from that
|
||||
permalink, allowing you to see what filters the user has applied to the
|
||||
dashboard (not just the default filter state).
|
||||
|
||||
Example usage:
|
||||
```json
|
||||
@@ -128,6 +141,7 @@ async def get_dashboard_info(
|
||||
With permalink (filter state from URL):
|
||||
```json
|
||||
{
|
||||
"identifier": 123,
|
||||
"permalink_key": "abc123def456"
|
||||
}
|
||||
```
|
||||
@@ -168,44 +182,64 @@ async def get_dashboard_info(
|
||||
query_options=eager_options,
|
||||
)
|
||||
|
||||
result, lookup_result = _lookup_dashboard(tool, request)
|
||||
permalink_key = lookup_result.permalink_key
|
||||
permalink_value = lookup_result.permalink_value
|
||||
result = tool.run_tool(request.identifier)
|
||||
|
||||
if isinstance(result, DashboardInfo):
|
||||
# If permalink_key is provided, retrieve filter state
|
||||
if permalink_key:
|
||||
if request.permalink_key:
|
||||
await ctx.info(
|
||||
"Retrieving filter state from permalink: permalink_key=%s"
|
||||
% (permalink_key,)
|
||||
% (request.permalink_key,)
|
||||
)
|
||||
_refresh_request_user_for_permalink_access()
|
||||
permalink_value = _get_permalink_state(request.permalink_key)
|
||||
|
||||
if permalink_value:
|
||||
permalink_state = get_matching_dashboard_permalink_state(
|
||||
lookup_result,
|
||||
result.id,
|
||||
result.uuid,
|
||||
result.slug,
|
||||
)
|
||||
if permalink_state is None:
|
||||
# Verify the permalink belongs to the requested dashboard
|
||||
# dashboardId in permalink is stored as str, result.id is int
|
||||
permalink_dashboard_id = permalink_value.get("dashboardId")
|
||||
try:
|
||||
permalink_dashboard_id_int = (
|
||||
int(permalink_dashboard_id)
|
||||
if permalink_dashboard_id
|
||||
else None
|
||||
)
|
||||
except (ValueError, TypeError):
|
||||
permalink_dashboard_id_int = None
|
||||
|
||||
if (
|
||||
permalink_dashboard_id_int is not None
|
||||
and permalink_dashboard_id_int != result.id
|
||||
):
|
||||
await ctx.warning(
|
||||
"permalink_key belongs to a different dashboard; "
|
||||
"ignoring permalink filter state."
|
||||
"permalink_key dashboardId (%s) does not match "
|
||||
"requested dashboard id (%s); ignoring permalink "
|
||||
"filter state." % (permalink_dashboard_id, result.id)
|
||||
)
|
||||
else:
|
||||
# Extract the state from permalink value
|
||||
# Handle None or non-dict state gracefully
|
||||
raw_state = permalink_value.get("state")
|
||||
permalink_state = (
|
||||
dict(raw_state) if isinstance(raw_state, dict) else {}
|
||||
)
|
||||
if not user_can_view_data_model_metadata():
|
||||
permalink_state = redact_filter_state_data_model_metadata(
|
||||
permalink_state
|
||||
)
|
||||
result = _apply_permalink_state(
|
||||
result,
|
||||
permalink_state.key,
|
||||
permalink_state.state,
|
||||
request.permalink_key,
|
||||
permalink_state,
|
||||
)
|
||||
|
||||
await ctx.info(
|
||||
"Filter state retrieved from permalink: "
|
||||
"has_dataMask=%s, has_chartStates=%s, has_activeTabs=%s"
|
||||
% (
|
||||
"dataMask" in permalink_state.state,
|
||||
"chartStates" in permalink_state.state,
|
||||
"activeTabs" in permalink_state.state,
|
||||
"dataMask" in permalink_state,
|
||||
"chartStates" in permalink_state,
|
||||
"activeTabs" in permalink_state,
|
||||
)
|
||||
)
|
||||
else:
|
||||
@@ -229,7 +263,7 @@ async def get_dashboard_info(
|
||||
# override select_columns, ensure filter_state is present so the
|
||||
# caller gets the data they came for.
|
||||
effective_select_columns = list(request.select_columns)
|
||||
if result.is_permalink_state and effective_select_columns == list(
|
||||
if request.permalink_key and effective_select_columns == list(
|
||||
DEFAULT_GET_DASHBOARD_INFO_COLUMNS
|
||||
):
|
||||
effective_select_columns.append("filter_state")
|
||||
|
||||
@@ -31,10 +31,6 @@ from fastmcp import Context
|
||||
from superset_core.mcp.decorators import tool, ToolAnnotations
|
||||
|
||||
from superset.extensions import event_logger
|
||||
from superset.mcp_service.dashboard.permalink import (
|
||||
get_matching_dashboard_permalink_state,
|
||||
lookup_dashboard_reference,
|
||||
)
|
||||
from superset.mcp_service.dashboard.schemas import (
|
||||
dashboard_layout_serializer,
|
||||
DashboardError,
|
||||
@@ -59,7 +55,7 @@ async def get_dashboard_layout(
|
||||
request: GetDashboardLayoutRequest, ctx: Context
|
||||
) -> DashboardLayout | DashboardError:
|
||||
"""
|
||||
Get parsed dashboard layout by ID, UUID, slug, or dashboard permalink.
|
||||
Get parsed dashboard layout by ID, UUID, or slug.
|
||||
|
||||
Returns the tabs and chart positions extracted from the dashboard's
|
||||
position_json. get_dashboard_info omits position_json to keep responses
|
||||
@@ -67,10 +63,6 @@ async def get_dashboard_layout(
|
||||
explain which charts live under which tab, or to locate a chart by
|
||||
its parent tab).
|
||||
|
||||
If the user gives you a shared URL containing ``/dashboard/p/<key>/``, pass
|
||||
the URL or bare key as ``identifier`` (or use ``permalink_key`` alone). The
|
||||
response identifies the active tab and includes the shared filter state.
|
||||
|
||||
Example usage:
|
||||
```json
|
||||
{
|
||||
@@ -94,40 +86,9 @@ async def get_dashboard_layout(
|
||||
supports_slug=True,
|
||||
logger=logger,
|
||||
)
|
||||
lookup_result = lookup_dashboard_reference(
|
||||
identifier=request.identifier,
|
||||
permalink_key=request.permalink_key,
|
||||
lookup=core.run_tool,
|
||||
is_found=lambda value: isinstance(value, DashboardLayout),
|
||||
)
|
||||
result = lookup_result.result
|
||||
if result is None:
|
||||
# Only reachable when the dashboard had to come from a permalink,
|
||||
# so an identifier's own "not found" error is preserved below.
|
||||
return DashboardError.create(
|
||||
"Dashboard permalink could not be resolved. It may be invalid "
|
||||
"or expired; ask for a fresh shared dashboard link.",
|
||||
"permalink_not_found",
|
||||
)
|
||||
result = core.run_tool(request.identifier)
|
||||
|
||||
if isinstance(result, DashboardLayout):
|
||||
if lookup_result.permalink_value:
|
||||
permalink_state = get_matching_dashboard_permalink_state(
|
||||
lookup_result, result.id, result.uuid
|
||||
)
|
||||
if permalink_state:
|
||||
payload = result.model_dump(mode="python")
|
||||
payload.update(
|
||||
permalink_key=permalink_state.key,
|
||||
filter_state=permalink_state.state,
|
||||
is_permalink_state=True,
|
||||
)
|
||||
result = DashboardLayout.model_validate(payload)
|
||||
else:
|
||||
await ctx.warning(
|
||||
"permalink_key belongs to a different dashboard; ignoring "
|
||||
"its active-tab and filter state."
|
||||
)
|
||||
await ctx.info(
|
||||
"Dashboard layout retrieved: id=%s, tab_count=%s, chart_count=%s, "
|
||||
"has_layout=%s"
|
||||
|
||||
@@ -35,7 +35,6 @@ from superset_core.mcp.decorators import tool, ToolAnnotations
|
||||
|
||||
from superset.commands.exceptions import CommandException, ForbiddenError
|
||||
from superset.extensions import event_logger
|
||||
from superset.mcp_service.dashboard.layout_validation import normalize_chart_id
|
||||
from superset.mcp_service.dashboard.schemas import (
|
||||
DashboardInfo,
|
||||
RemoveChartFromDashboardRequest,
|
||||
@@ -60,12 +59,14 @@ def _find_chart_keys(layout: Dict[str, Any], chart_id: int) -> list[str]:
|
||||
A chart can legitimately appear more than once in a layout (e.g. under
|
||||
multiple tabs), so all occurrences are returned.
|
||||
"""
|
||||
# Accept both int and string chartId — position_json is user/frontend-authored
|
||||
# and imported or hand-edited layouts may store chartId as a string.
|
||||
return [
|
||||
key
|
||||
for key, node in layout.items()
|
||||
if isinstance(node, dict)
|
||||
and node.get("type") == "CHART"
|
||||
and normalize_chart_id((node.get("meta") or {}).get("chartId")) == chart_id
|
||||
and (node.get("meta") or {}).get("chartId") in (chart_id, str(chart_id))
|
||||
]
|
||||
|
||||
|
||||
|
||||
@@ -33,7 +33,6 @@ from superset_core.mcp.decorators import tool, ToolAnnotations
|
||||
from superset.commands.dashboard.exceptions import DashboardNotFoundError
|
||||
from superset.exceptions import SupersetSecurityException
|
||||
from superset.extensions import db, event_logger
|
||||
from superset.mcp_service.dashboard.layout_validation import validate_dashboard_layout
|
||||
from superset.mcp_service.dashboard.schemas import (
|
||||
dashboard_serializer,
|
||||
DashboardError,
|
||||
@@ -238,14 +237,6 @@ def _validate_update_request(
|
||||
from superset.dashboards.schemas import validate_css
|
||||
from superset.tags.models import ObjectType
|
||||
|
||||
if request.position_json is not None:
|
||||
chart_ids = [chart.id for chart in dashboard.slices]
|
||||
if error := validate_dashboard_layout(request.position_json, chart_ids):
|
||||
return DashboardError(
|
||||
error=f"Dashboard layout is invalid: {error}",
|
||||
error_type="InvalidDashboardLayout",
|
||||
)
|
||||
|
||||
# Empty string clears CSS (no validation needed); only validate real content.
|
||||
if request.css:
|
||||
try:
|
||||
@@ -298,10 +289,9 @@ async def update_dashboard(
|
||||
) -> UpdateDashboardResponse | DashboardError:
|
||||
"""Patch an existing dashboard's layout, theme, styling, or metadata.
|
||||
|
||||
Companion to ``generate_dashboard`` for incremental metadata and styling
|
||||
edits. An LLM can:
|
||||
Companion to ``generate_dashboard`` for incremental edits. An LLM can:
|
||||
|
||||
- Replace ``position_json`` only when it already has the complete raw tree
|
||||
- Set or replace ``position_json`` after auto-generation
|
||||
- Apply brand ``label_colors`` and ``color_scheme`` via
|
||||
``json_metadata_overrides``
|
||||
- Inject ``css`` to hide chrome on print-ready dashboards
|
||||
|
||||
@@ -20,19 +20,10 @@ from typing import Any, Optional, Union
|
||||
from croniter import croniter
|
||||
from flask import current_app
|
||||
from flask_babel import gettext as _
|
||||
from marshmallow import (
|
||||
EXCLUDE,
|
||||
fields,
|
||||
pre_load,
|
||||
Schema,
|
||||
validate,
|
||||
validates,
|
||||
validates_schema,
|
||||
)
|
||||
from marshmallow import EXCLUDE, fields, Schema, validate, validates, validates_schema
|
||||
from marshmallow.validate import Length, Range, ValidationError
|
||||
from pytz import all_timezones
|
||||
|
||||
from superset import is_feature_enabled
|
||||
from superset.reports.models import (
|
||||
ReportCreationMethod,
|
||||
ReportDataFormat,
|
||||
@@ -189,34 +180,7 @@ class ReportRecipientSchema(Schema):
|
||||
validate_addresses("bccTarget", config.get("bccTarget"), required=False)
|
||||
|
||||
|
||||
_RETRY_FIELD_KEYS = (
|
||||
"retry_on_failure",
|
||||
"retry_max_attempts",
|
||||
"send_failed_reports",
|
||||
"retry_notify_owners",
|
||||
"retry_notify_recipients",
|
||||
)
|
||||
|
||||
|
||||
class RetryFieldStripMixin:
|
||||
"""Strip retry fields from the raw payload before validation when the
|
||||
feature is off. Using ``@pre_load`` ensures that field-level validators
|
||||
(e.g. ``Range`` on ``retry_max_attempts``) are never reached for values
|
||||
that will be discarded anyway."""
|
||||
|
||||
@pre_load
|
||||
def strip_retry_fields_if_disabled(
|
||||
self,
|
||||
data: dict[str, Any],
|
||||
**kwargs: Any,
|
||||
) -> dict[str, Any]:
|
||||
if not is_feature_enabled("ALERT_REPORTS_RETRY"):
|
||||
for key in _RETRY_FIELD_KEYS:
|
||||
data.pop(key, None)
|
||||
return data
|
||||
|
||||
|
||||
class ReportSchedulePostSchema(RetryFieldStripMixin, Schema):
|
||||
class ReportSchedulePostSchema(Schema):
|
||||
type = fields.String(
|
||||
metadata={"description": type_description},
|
||||
allow_none=False,
|
||||
@@ -391,8 +355,6 @@ class ReportSchedulePostSchema(RetryFieldStripMixin, Schema):
|
||||
data: dict[str, Any],
|
||||
**kwargs: Any,
|
||||
) -> None:
|
||||
if not is_feature_enabled("ALERT_REPORTS_RETRY"):
|
||||
return
|
||||
if data.get("send_failed_reports") and not data.get("retry_on_failure"):
|
||||
raise ValidationError(
|
||||
{
|
||||
@@ -434,7 +396,7 @@ class ReportScheduleSubscribeSchema(ReportSchedulePostSchema):
|
||||
unknown = EXCLUDE
|
||||
|
||||
|
||||
class ReportSchedulePutSchema(RetryFieldStripMixin, Schema):
|
||||
class ReportSchedulePutSchema(Schema):
|
||||
type = fields.String(
|
||||
metadata={"description": type_description},
|
||||
required=False,
|
||||
|
||||
+7
-3
@@ -67,7 +67,6 @@ from superset.utils.core import (
|
||||
QuerySource,
|
||||
zlib_compress,
|
||||
)
|
||||
from superset.utils.database import warm_and_release_connection
|
||||
from superset.utils.dates import now_as_float
|
||||
from superset.utils.decorators import stats_timing
|
||||
from superset.utils.rls import apply_rls
|
||||
@@ -288,8 +287,13 @@ def execute_query( # pylint: disable=too-many-statements, too-many-locals # no
|
||||
# that stays idle for the query duration; if the query runs longer
|
||||
# than the DB's idle_in_transaction_session_timeout the connection
|
||||
# is killed, leaving the query stuck in "running" state forever.
|
||||
db.session.refresh(query)
|
||||
warm_and_release_connection(query, "database")
|
||||
db.session.expire_on_commit = False
|
||||
try:
|
||||
db.session.refresh(query)
|
||||
_ = query.database
|
||||
db.session.commit()
|
||||
finally:
|
||||
db.session.expire_on_commit = True
|
||||
with event_logger.log_context(
|
||||
action="execute_sql",
|
||||
database=database,
|
||||
|
||||
@@ -87,42 +87,6 @@ def remove_database(database: Database) -> None:
|
||||
db.session.flush()
|
||||
|
||||
|
||||
def warm_and_release_connection(instance: Any, *relationships: str) -> None:
|
||||
"""
|
||||
Eagerly load the named relationships on ``instance``, then release the
|
||||
current session's DB connection back to the pool without detaching any
|
||||
object in the session.
|
||||
|
||||
Prefer this over ``db.session.close()`` before slow, non-DB work (a
|
||||
long-running cursor execution, a results-backend fetch, CPU-bound
|
||||
decompress/deserialize work) that still needs attributes already
|
||||
loaded on session objects: ``close()`` detaches every object in the
|
||||
session -- including ``g.user``, not just ``instance`` -- so a later
|
||||
attribute access anywhere in the request can raise on a detached
|
||||
instance or silently open a fresh connection. Committing with
|
||||
``expire_on_commit`` disabled instead releases the connection while
|
||||
keeping objects attached and their already-loaded attributes valid.
|
||||
"""
|
||||
# pylint: disable=import-outside-toplevel
|
||||
from superset import db
|
||||
|
||||
for relationship in relationships:
|
||||
getattr(instance, relationship)
|
||||
|
||||
# ``db.session`` is a ``scoped_session`` proxy: it only forwards a fixed
|
||||
# allowlist of attributes to the real ``Session`` (bind, dirty, deleted,
|
||||
# new, identity_map, is_active, autoflush, no_autoflush, info).
|
||||
# ``expire_on_commit`` isn't on that list, so setting it on ``db.session``
|
||||
# directly would silently no-op -- it has to be set on the real Session
|
||||
# returned by calling the proxy.
|
||||
session = db.session()
|
||||
session.expire_on_commit = False
|
||||
try:
|
||||
session.commit() # pylint: disable=consider-using-transaction
|
||||
finally:
|
||||
session.expire_on_commit = True
|
||||
|
||||
|
||||
def apply_mariadb_ddl_fix() -> None:
|
||||
"""
|
||||
Fix MariaDB "NO CYCLE" syntax issue - MariaDB uses "NOCYCLE" (no space).
|
||||
|
||||
+8
-33
@@ -60,22 +60,8 @@ def quote_formulas(df: pd.DataFrame) -> pd.DataFrame:
|
||||
"""
|
||||
Make sure to quote any formulas for security reasons.
|
||||
"""
|
||||
# Columns are addressed by position rather than by label: a dataframe can
|
||||
# carry duplicate column labels (the verbose_map rename in
|
||||
# QueryContextProcessor.get_data can collapse two columns onto the same
|
||||
# name), and ``df[label]`` then yields a DataFrame instead of a Series.
|
||||
# ``DataFrame.apply`` would hand whole columns to the mapper rather than
|
||||
# individual cells, silently leaving formulas unquoted.
|
||||
for idx in range(len(df.columns)):
|
||||
series = df.iloc[:, idx]
|
||||
# ``is_string_dtype`` rather than an ``object`` comparison: pandas 3
|
||||
# gives string columns a dedicated ``str`` dtype, which an object-only
|
||||
# check (as the ``select_dtypes(include="object")`` this replaced) would
|
||||
# skip, silently leaving formulas unquoted.
|
||||
if pd.api.types.is_object_dtype(series.dtype) or pd.api.types.is_string_dtype(
|
||||
series.dtype
|
||||
):
|
||||
df.isetitem(idx, series.map(_quote_formula))
|
||||
for col in df.select_dtypes(include="object").columns:
|
||||
df[col] = df[col].apply(_quote_formula)
|
||||
|
||||
# Column headers and index labels are written to the sheet as well, and
|
||||
# pivot exports promote data values into both (a hostile warehouse string
|
||||
@@ -118,31 +104,20 @@ def apply_column_types(
|
||||
:param column_types: The types of the columns
|
||||
:return: The dataframe with the column types applied
|
||||
"""
|
||||
# Columns are addressed by position for the same reason as in
|
||||
# ``quote_formulas``: duplicate column labels make ``df[label]`` return a
|
||||
# DataFrame, and ``DataFrame`` has no ``dtype``. Slicing column_types keeps
|
||||
# the lenient pairing the previous ``zip(..., strict=False)`` provided.
|
||||
for idx, column_type in enumerate(column_types[: len(df.columns)]):
|
||||
series = df.iloc[:, idx]
|
||||
for column, column_type in zip(df.columns, column_types, strict=False):
|
||||
if column_type == GenericDataType.NUMERIC:
|
||||
try:
|
||||
series = pd.to_numeric(series)
|
||||
df[column] = pd.to_numeric(df[column])
|
||||
# if the number is too large, convert it to a string
|
||||
# Excel does not support numbers larger than 10^15
|
||||
series = series.apply(
|
||||
df[column] = df[column].apply(
|
||||
lambda x: (
|
||||
str(x) if isinstance(x, (int, float)) and abs(x) > 10**15 else x
|
||||
)
|
||||
)
|
||||
except ValueError:
|
||||
series = series.astype(str)
|
||||
elif isinstance(series.dtype, pd.DatetimeTZDtype):
|
||||
df[column] = df[column].astype(str)
|
||||
elif isinstance(df[column].dtype, pd.DatetimeTZDtype):
|
||||
# timezones are not supported
|
||||
series = series.astype(str)
|
||||
else:
|
||||
continue
|
||||
# ``isetitem`` replaces the column at that position, which is both
|
||||
# unambiguous under duplicate labels and free of the in-place dtype
|
||||
# casting that ``iloc`` assignment attempts.
|
||||
df.isetitem(idx, series)
|
||||
df[column] = df[column].astype(str)
|
||||
return df
|
||||
|
||||
+3
-68
@@ -14,7 +14,6 @@
|
||||
# KIND, either express or implied. See the License for the
|
||||
# specific language governing permissions and limitations
|
||||
# under the License.
|
||||
import hashlib
|
||||
import re
|
||||
|
||||
from werkzeug.utils import secure_filename
|
||||
@@ -24,78 +23,14 @@ from werkzeug.utils import secure_filename
|
||||
# SMTP headers, Content-Disposition filenames, and headless-browser document.title.
|
||||
_CONTROL_CHARS_RE = re.compile(r"[\x00-\x1f\x7f-\x9f]")
|
||||
|
||||
# Model names can be up to 500 characters, which produces export entries that exceed
|
||||
# both the 255 character filename limit common to most filesystems and, once the
|
||||
# archive prefix ("chart_export_<timestamp>/charts/") and the extraction directory are
|
||||
# added, the 260 character path limit (MAX_PATH) Windows applies when unzipping.
|
||||
# The extraction directory is unbounded, so no cap can guarantee MAX_PATH is met; 128
|
||||
# keeps every component well within the filesystem limit and leaves typical export
|
||||
# paths comfortably short while retaining enough of the name to stay recognizable.
|
||||
MAX_FILENAME_LENGTH = 128
|
||||
|
||||
# Characters that are meaningless or invalid at the end of a filename. A trailing dot
|
||||
# in particular is silently dropped by Windows.
|
||||
_TRAILING_CHARS = "._-"
|
||||
|
||||
# Short content hash used as a disambiguator when ``skip_id=True`` forces truncation.
|
||||
# Eight hex characters (32 bits) avoid accidental collisions among truncated export
|
||||
# names while leaving most of the slug readable. Hashed from the full slug (not the
|
||||
# model id) so database folder names stay stable across export commands.
|
||||
_HASH_LENGTH = 8
|
||||
|
||||
|
||||
def sanitize_title(title: str) -> str:
|
||||
"""Remove all C0/C1 control characters from a title string."""
|
||||
return _CONTROL_CHARS_RE.sub("", title)
|
||||
|
||||
|
||||
def _name_hash(slug: str) -> str:
|
||||
return hashlib.sha256(slug.encode()).hexdigest()[:_HASH_LENGTH]
|
||||
|
||||
|
||||
def get_filename(
|
||||
model_name: str,
|
||||
model_id: int,
|
||||
skip_id: bool = False,
|
||||
max_length: int = MAX_FILENAME_LENGTH,
|
||||
) -> str:
|
||||
"""
|
||||
Build a filesystem-safe filename for a model, truncated to ``max_length``.
|
||||
|
||||
When ``skip_id`` is false the model id is appended, which already keeps names
|
||||
unique under truncation. When ``skip_id`` is true and the slug must be
|
||||
truncated, a short hash of the full slug is appended instead so names that
|
||||
only differ past the cut (e.g. two long database names) do not collide and
|
||||
silently overwrite each other in an export archive.
|
||||
|
||||
:param model_name: the human readable name of the model
|
||||
:param model_id: the model's primary key, appended unless ``skip_id`` is set
|
||||
:param skip_id: whether to omit the id suffix
|
||||
:param max_length: maximum length of the returned name, suffix included
|
||||
:returns: the sanitized name, or the id alone when nothing usable remains
|
||||
"""
|
||||
def get_filename(model_name: str, model_id: int, skip_id: bool = False) -> str:
|
||||
model_name = sanitize_title(model_name)
|
||||
# `secure_filename` transliterates to ASCII, so length in characters equals
|
||||
# length in bytes and the string can be truncated safely by slicing.
|
||||
slug = secure_filename(model_name)
|
||||
if not slug:
|
||||
return str(model_id)
|
||||
|
||||
if skip_id:
|
||||
if len(slug) <= max_length:
|
||||
return slug
|
||||
|
||||
hash_suffix = f"_{_name_hash(slug)}"
|
||||
if len(hash_suffix) > max_length:
|
||||
# Degenerate custom max_length: cannot fit a disambiguator.
|
||||
return str(model_id)
|
||||
|
||||
truncated = slug[: max_length - len(hash_suffix)].rstrip(_TRAILING_CHARS)
|
||||
return f"{truncated}{hash_suffix}" if truncated else hash_suffix[1:]
|
||||
|
||||
id_suffix = f"_{model_id}"
|
||||
max_slug_length = max(max_length - len(id_suffix), 0)
|
||||
if len(slug) > max_slug_length:
|
||||
slug = slug[:max_slug_length].rstrip(_TRAILING_CHARS)
|
||||
|
||||
return f"{slug}{id_suffix}" if slug else str(model_id)
|
||||
filename = slug if skip_id else f"{slug}_{model_id}"
|
||||
return filename if slug else str(model_id)
|
||||
|
||||
@@ -3147,7 +3147,6 @@ def test__send_with_server_errors(notification_mock, logger_mock):
|
||||
|
||||
|
||||
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
|
||||
@with_feature_flags(ALERT_REPORTS_RETRY=True)
|
||||
@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
|
||||
@patch("superset.reports.notifications.email.send_email_smtp")
|
||||
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
|
||||
@@ -3192,7 +3191,6 @@ def test_retry_on_failure_schedules_retry(
|
||||
|
||||
|
||||
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
|
||||
@with_feature_flags(ALERT_REPORTS_RETRY=True)
|
||||
@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
|
||||
@patch("superset.commands.report.execute.BaseReportState.send_retry_notification")
|
||||
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
|
||||
@@ -3249,7 +3247,6 @@ def test_retry_exhausted_transitions_to_error(
|
||||
|
||||
|
||||
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
|
||||
@with_feature_flags(ALERT_REPORTS_RETRY=True)
|
||||
@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
|
||||
@patch("superset.commands.report.execute.BaseReportState.send_final_failure_report")
|
||||
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
|
||||
@@ -3294,7 +3291,6 @@ def test_send_failed_reports_sends_to_recipients(
|
||||
|
||||
|
||||
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
|
||||
@with_feature_flags(ALERT_REPORTS_RETRY=True)
|
||||
@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
|
||||
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
|
||||
def test_retrying_state_schedules_another_retry(
|
||||
@@ -3340,7 +3336,6 @@ def test_retrying_state_schedules_another_retry(
|
||||
|
||||
|
||||
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
|
||||
@with_feature_flags(ALERT_REPORTS_RETRY=True)
|
||||
@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
|
||||
@patch("superset.reports.notifications.email.send_email_smtp")
|
||||
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
|
||||
@@ -3377,7 +3372,6 @@ def test_retry_disabled_preserves_default_error_path(
|
||||
|
||||
|
||||
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
|
||||
@with_feature_flags(ALERT_REPORTS_RETRY=True)
|
||||
@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
|
||||
@patch("superset.commands.report.execute.BaseReportState.send_retry_notification")
|
||||
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
|
||||
@@ -3420,7 +3414,6 @@ def test_retry_notify_owners_sends_notification(
|
||||
|
||||
|
||||
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
|
||||
@with_feature_flags(ALERT_REPORTS_RETRY=True)
|
||||
@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
|
||||
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
|
||||
def test_new_crontab_window_skipped_while_retrying(
|
||||
@@ -3469,7 +3462,6 @@ def test_new_crontab_window_skipped_while_retrying(
|
||||
|
||||
|
||||
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
|
||||
@with_feature_flags(ALERT_REPORTS_RETRY=True)
|
||||
@patch("superset.commands.report.execute.ReportNotTriggeredErrorState._schedule_retry")
|
||||
@patch("superset.reports.notifications.email.send_email_smtp")
|
||||
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
|
||||
|
||||
@@ -14,7 +14,6 @@
|
||||
# KIND, either express or implied. See the License for the
|
||||
# specific language governing permissions and limitations
|
||||
# under the License.
|
||||
from typing import Any
|
||||
from unittest import mock
|
||||
from unittest.mock import Mock, patch
|
||||
|
||||
@@ -23,13 +22,10 @@ import pytest
|
||||
from flask import current_app
|
||||
from flask_babel import gettext as __
|
||||
from jinja2.exceptions import TemplateError, TemplateSyntaxError
|
||||
from sqlalchemy import inspect as sa_inspect
|
||||
from sqlalchemy.orm import object_session
|
||||
|
||||
from superset import db, sql_lab
|
||||
from superset.commands.sql_lab import estimate, export, results
|
||||
from superset.common.db_query_status import QueryStatus
|
||||
from superset.db_engine_specs.base import BaseEngineSpec
|
||||
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
|
||||
from superset.exceptions import (
|
||||
SerializationError,
|
||||
@@ -39,7 +35,6 @@ from superset.exceptions import (
|
||||
)
|
||||
from superset.models.core import Database # noqa: F401
|
||||
from superset.models.sql_lab import Query
|
||||
from superset.result_set import SupersetResultSet
|
||||
from superset.sqllab.limiting_factor import LimitingFactor
|
||||
from superset.sqllab.schemas import EstimateQueryCostSchema
|
||||
from superset.utils import core as utils
|
||||
@@ -434,93 +429,6 @@ class TestSqlExecutionResultsCommand(SupersetTestCase):
|
||||
)
|
||||
assert ex_info.value.status == 400
|
||||
|
||||
@pytest.mark.usefixtures("create_database_and_query")
|
||||
@patch("superset.commands.sql_lab.results.results_backend_use_msgpack", False)
|
||||
def test_validation_releases_db_connection_before_fetching_from_results_backend(
|
||||
self,
|
||||
) -> None:
|
||||
# The DB connection must be released back to the pool (via
|
||||
# warm_and_release_connection(), not a full session close -- see
|
||||
# ``test_validation_warms_database_relationship_before_releasing_connection``
|
||||
# for why) before the (potentially slow) results-backend fetch, so a
|
||||
# large download doesn't hold a connection out of the pool for its
|
||||
# duration.
|
||||
#
|
||||
# This spies on ``warm_and_release_connection`` itself rather than on
|
||||
# ``db.session.commit`` -- the latter is a ``scoped_session`` proxy
|
||||
# method, and patching it wouldn't be observed by the real
|
||||
# ``Session`` object that ``warm_and_release_connection`` commits
|
||||
# (see the fix for the analogous ``expire_on_commit`` proxy pitfall).
|
||||
call_order: list[str] = []
|
||||
original_warm_and_release_connection = results.warm_and_release_connection
|
||||
|
||||
def tracked_warm_and_release_connection(
|
||||
instance: Any, *relationships: str
|
||||
) -> None:
|
||||
call_order.append("connection_released")
|
||||
original_warm_and_release_connection(instance, *relationships)
|
||||
|
||||
def tracked_get(key: str) -> None:
|
||||
call_order.append("results_backend_get")
|
||||
return None
|
||||
|
||||
results.results_backend = mock.Mock()
|
||||
results.results_backend.get.side_effect = tracked_get
|
||||
|
||||
command = results.SqlExecutionResultsCommand("abc_query", 1000)
|
||||
|
||||
admin = self.get_user("admin")
|
||||
with current_app.test_request_context():
|
||||
with override_user(admin):
|
||||
with mock.patch(
|
||||
"superset.commands.sql_lab.results.warm_and_release_connection",
|
||||
side_effect=tracked_warm_and_release_connection,
|
||||
):
|
||||
with pytest.raises(SupersetErrorException):
|
||||
# ``get`` returns ``None`` above, so validation goes
|
||||
# on to raise the "results missing" (410) error --
|
||||
# irrelevant here, we only care about the call order
|
||||
# leading up to it.
|
||||
command.validate()
|
||||
|
||||
assert call_order == ["connection_released", "results_backend_get"]
|
||||
|
||||
@pytest.mark.usefixtures("create_database_and_query")
|
||||
@patch("superset.commands.sql_lab.results.results_backend_use_msgpack", False)
|
||||
def test_validation_warms_database_relationship_before_releasing_connection(
|
||||
self,
|
||||
) -> None:
|
||||
# ``run`` needs ``self._query.database.db_engine_spec`` after the
|
||||
# connection has been released by ``validate``. The relationship
|
||||
# must therefore already be loaded by then, and the query must stay
|
||||
# attached to the session (unlike a full ``db.session.close()``,
|
||||
# which would detach every object in the session -- including
|
||||
# ``g.user`` -- not just the query), or accessing it later would
|
||||
# either raise (detached instance with an unloaded attribute) or
|
||||
# silently open a fresh, unwanted connection.
|
||||
data = [{"col_0": i} for i in range(104)]
|
||||
payload = {
|
||||
"status": QueryStatus.SUCCESS,
|
||||
"query": {"rows": 104},
|
||||
"data": data,
|
||||
}
|
||||
serialized_payload = sql_lab._serialize_payload(payload, False)
|
||||
compressed = utils.zlib_compress(serialized_payload)
|
||||
|
||||
results.results_backend = mock.Mock()
|
||||
results.results_backend.get.return_value = compressed
|
||||
|
||||
command = results.SqlExecutionResultsCommand("abc_query", 1000)
|
||||
|
||||
admin = self.get_user("admin")
|
||||
with current_app.test_request_context():
|
||||
with override_user(admin):
|
||||
command.validate()
|
||||
|
||||
assert object_session(command._query) is not None
|
||||
assert "database" not in sa_inspect(command._query).unloaded
|
||||
assert command._query.database is not None
|
||||
|
||||
@pytest.mark.usefixtures("create_database_and_query")
|
||||
@patch("superset.commands.sql_lab.results.results_backend_use_msgpack", False)
|
||||
def test_run_succeeds(self) -> None:
|
||||
@@ -544,51 +452,4 @@ class TestSqlExecutionResultsCommand(SupersetTestCase):
|
||||
|
||||
assert result.get("status") == "success"
|
||||
assert result["query"].get("rows") == 104
|
||||
|
||||
@pytest.mark.usefixtures("create_database_and_query")
|
||||
@patch("superset.commands.sql_lab.results.results_backend_use_msgpack", True)
|
||||
def test_run_succeeds_with_msgpack(self) -> None:
|
||||
# ``query.database.db_engine_spec`` is only touched in the
|
||||
# ``use_msgpack=True`` branch of ``_deserialize_results_payload`` --
|
||||
# which is the production default. All the other tests here run
|
||||
# with msgpack off, so this exercises the full ``run()`` path with
|
||||
# msgpack on, to catch a regression that leaves ``query.database``
|
||||
# unloaded or detached after ``validate()`` releases the connection.
|
||||
cursor_descr = (
|
||||
("a", "string", None, None, None, None, True),
|
||||
("b", "int", None, None, None, None, True),
|
||||
("c", "float", None, None, None, None, True),
|
||||
)
|
||||
result_set = SupersetResultSet(
|
||||
[("a", 4, 4.0)],
|
||||
cursor_descr,
|
||||
BaseEngineSpec,
|
||||
)
|
||||
(
|
||||
serialized_data,
|
||||
selected_columns,
|
||||
all_columns,
|
||||
expanded_columns,
|
||||
) = sql_lab._serialize_and_expand_data(result_set, BaseEngineSpec(), True)
|
||||
payload = {
|
||||
"status": QueryStatus.SUCCESS,
|
||||
"query": {"rows": 1},
|
||||
"data": serialized_data,
|
||||
"columns": all_columns,
|
||||
"selected_columns": selected_columns,
|
||||
"expanded_columns": expanded_columns,
|
||||
}
|
||||
serialized_payload = sql_lab._serialize_payload(payload, True)
|
||||
compressed = utils.zlib_compress(serialized_payload)
|
||||
|
||||
results.results_backend = mock.Mock()
|
||||
results.results_backend.get.return_value = compressed
|
||||
|
||||
admin = self.get_user("admin")
|
||||
with current_app.test_request_context():
|
||||
with override_user(admin):
|
||||
command = results.SqlExecutionResultsCommand("abc_query", 1000)
|
||||
result = command.run()
|
||||
|
||||
assert result.get("status") == "success"
|
||||
assert result["data"]
|
||||
assert result.get("data") == data
|
||||
|
||||
@@ -16,7 +16,6 @@
|
||||
# under the License.
|
||||
from __future__ import annotations
|
||||
|
||||
import inspect
|
||||
from typing import Any, TYPE_CHECKING
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
@@ -859,253 +858,3 @@ def test_send_chart_response_does_not_double_extension_for_csv_filename() -> Non
|
||||
content_disposition = response.headers["Content-Disposition"]
|
||||
assert "my_export.csv.csv" not in content_disposition
|
||||
assert "my_export.csv" in content_disposition
|
||||
|
||||
|
||||
def test_default_export_filename_prefers_explicit_slice_name() -> None:
|
||||
"""An explicit slice_name in the form data wins over the chart object."""
|
||||
filename = ChartDataRestApi._get_default_export_filename(
|
||||
{"slice_name": "Explicit Name", "viz_type": "table"},
|
||||
MagicMock(slice_name="Saved Chart"),
|
||||
)
|
||||
|
||||
assert filename.startswith("superset_Explicit_Name_")
|
||||
|
||||
|
||||
def test_default_export_filename_uses_chart_name() -> None:
|
||||
"""
|
||||
Regression test: the chart's own name must be used for the export
|
||||
filename. Real clients never place slice_name inside the form data
|
||||
(Slice.form_data only injects slice_id, viz_type and datasource), so
|
||||
the name has to come from the resolved Slice object.
|
||||
"""
|
||||
filename = ChartDataRestApi._get_default_export_filename(
|
||||
{"viz_type": "table"},
|
||||
MagicMock(slice_name="Revenue by Region"),
|
||||
)
|
||||
|
||||
assert filename.startswith("superset_Revenue_by_Region_")
|
||||
|
||||
|
||||
def test_default_export_filename_ignores_non_string_chart_name() -> None:
|
||||
"""
|
||||
A slice whose name is not a plain string (e.g. a mock in tests, or an
|
||||
unloaded attribute) must not leak its repr into the filename.
|
||||
"""
|
||||
filename = ChartDataRestApi._get_default_export_filename(
|
||||
{"viz_type": "table"},
|
||||
MagicMock(), # slice_name is itself a MagicMock, not a str
|
||||
)
|
||||
|
||||
assert filename.startswith("superset_table_")
|
||||
|
||||
|
||||
def test_default_export_filename_falls_back_when_name_sanitizes_to_nothing() -> None:
|
||||
"""
|
||||
secure_filename reduces a chart name written entirely in a non-latin
|
||||
alphabet to an empty string; the viz_type must be used instead so the
|
||||
filename keeps a meaningful segment.
|
||||
"""
|
||||
filename = ChartDataRestApi._get_default_export_filename(
|
||||
{"viz_type": "table"},
|
||||
MagicMock(slice_name="销售报表"),
|
||||
)
|
||||
|
||||
assert filename.startswith("superset_table_")
|
||||
|
||||
|
||||
def test_default_export_filename_without_any_candidate() -> None:
|
||||
"""With no form data and no slice the generic fallback is preserved."""
|
||||
filename = ChartDataRestApi._get_default_export_filename(None, None)
|
||||
|
||||
assert filename.startswith("superset_export_")
|
||||
|
||||
|
||||
def test_default_export_filename_caps_very_long_chart_names() -> None:
|
||||
"""
|
||||
Chart names can be up to 250 characters; the generated filename must
|
||||
stay within the 255-character single-component limit of common
|
||||
filesystems once the extension is appended, or consumers that honor
|
||||
Content-Disposition verbatim (curl -OJ, wget) fail to save the file.
|
||||
"""
|
||||
filename = ChartDataRestApi._get_default_export_filename(
|
||||
{"viz_type": "table"},
|
||||
MagicMock(slice_name="x" * 250),
|
||||
)
|
||||
|
||||
assert filename.startswith("superset_" + "x" * 150)
|
||||
assert "x" * 151 not in filename
|
||||
assert len(filename) + len(".xlsx") <= 255
|
||||
|
||||
|
||||
def test_send_chart_response_uses_query_context_slice_name() -> None:
|
||||
"""
|
||||
POST /api/v1/chart/data: when the submitted form data carries a
|
||||
slice_id, the query context factory resolves the Slice, and its name
|
||||
must reach the CSV filename even though slice_name itself is absent
|
||||
from the form data.
|
||||
"""
|
||||
query_context = MagicMock()
|
||||
query_context.result_type = ChartDataResultType.FULL
|
||||
query_context.result_format = ChartDataResultFormat.CSV
|
||||
query_context.slice_ = MagicMock(slice_name="Quarterly Revenue")
|
||||
|
||||
result = {
|
||||
"query_context": query_context,
|
||||
"queries": [{"data": "col_a,col_b\n1,2\n"}],
|
||||
}
|
||||
|
||||
api = ChartDataRestApi()
|
||||
with (
|
||||
patch("superset.charts.data.api.security_manager") as mock_security_manager,
|
||||
patch("superset.charts.data.api.is_feature_enabled", return_value=False),
|
||||
):
|
||||
mock_security_manager.can_access.return_value = True
|
||||
response = api._send_chart_response(
|
||||
result, form_data={"viz_type": "table", "row_limit": 10}
|
||||
)
|
||||
|
||||
assert "Quarterly_Revenue" in response.headers["Content-Disposition"]
|
||||
|
||||
|
||||
def test_send_chart_response_uses_route_slice_when_context_has_none() -> None:
|
||||
"""
|
||||
GET /api/v1/chart/<pk>/data/: a chart's saved query context rarely
|
||||
carries a slice_id, so the route passes the chart it loaded and that
|
||||
name must be used for the filename.
|
||||
"""
|
||||
query_context = MagicMock()
|
||||
query_context.result_type = ChartDataResultType.FULL
|
||||
query_context.result_format = ChartDataResultFormat.CSV
|
||||
query_context.slice_ = None
|
||||
|
||||
result = {
|
||||
"query_context": query_context,
|
||||
"queries": [{"data": "col_a,col_b\n1,2\n"}],
|
||||
}
|
||||
|
||||
api = ChartDataRestApi()
|
||||
with (
|
||||
patch("superset.charts.data.api.security_manager") as mock_security_manager,
|
||||
patch("superset.charts.data.api.is_feature_enabled", return_value=False),
|
||||
):
|
||||
mock_security_manager.can_access.return_value = True
|
||||
response = api._send_chart_response(
|
||||
result,
|
||||
form_data={"viz_type": "table", "row_limit": 10},
|
||||
slice_=MagicMock(slice_name="Saved Chart"),
|
||||
)
|
||||
|
||||
assert "Saved_Chart" in response.headers["Content-Disposition"]
|
||||
|
||||
|
||||
def test_send_chart_response_prefers_route_slice_over_query_context() -> None:
|
||||
"""The chart the route loaded wins over the factory-resolved slice."""
|
||||
query_context = MagicMock()
|
||||
query_context.result_type = ChartDataResultType.FULL
|
||||
query_context.result_format = ChartDataResultFormat.CSV
|
||||
query_context.slice_ = MagicMock(slice_name="Context Chart")
|
||||
|
||||
result = {
|
||||
"query_context": query_context,
|
||||
"queries": [{"data": "col_a,col_b\n1,2\n"}],
|
||||
}
|
||||
|
||||
api = ChartDataRestApi()
|
||||
with (
|
||||
patch("superset.charts.data.api.security_manager") as mock_security_manager,
|
||||
patch("superset.charts.data.api.is_feature_enabled", return_value=False),
|
||||
):
|
||||
mock_security_manager.can_access.return_value = True
|
||||
response = api._send_chart_response(
|
||||
result,
|
||||
form_data={"viz_type": "table", "row_limit": 10},
|
||||
slice_=MagicMock(slice_name="Route Chart"),
|
||||
)
|
||||
|
||||
content_disposition = response.headers["Content-Disposition"]
|
||||
assert "Route_Chart" in content_disposition
|
||||
assert "Context_Chart" not in content_disposition
|
||||
|
||||
|
||||
def test_streaming_csv_response_uses_chart_name(app: SupersetApp) -> None:
|
||||
"""
|
||||
The generated streaming CSV filename (used when the client didn't
|
||||
supply one) must include the chart's name, matching the non-streaming
|
||||
path.
|
||||
"""
|
||||
query_context = MagicMock()
|
||||
query_context.result_type = ChartDataResultType.FULL
|
||||
query_context.result_format = ChartDataResultFormat.CSV
|
||||
query_context.slice_ = MagicMock(slice_name="Quarterly Revenue")
|
||||
|
||||
result = {
|
||||
"query_context": query_context,
|
||||
"queries": [{"data": "col_a,col_b\n1,2\n"}],
|
||||
}
|
||||
|
||||
api = ChartDataRestApi()
|
||||
with (
|
||||
app.test_request_context("/api/v1/chart/data"),
|
||||
patch("superset.charts.data.api.security_manager") as mock_security_manager,
|
||||
patch("superset.charts.data.api.is_feature_enabled", return_value=False),
|
||||
patch.object(ChartDataRestApi, "_should_use_streaming", return_value=True),
|
||||
patch("superset.charts.data.api.StreamingCSVExportCommand"),
|
||||
):
|
||||
mock_security_manager.can_access.return_value = True
|
||||
response = api._send_chart_response(
|
||||
result, form_data={"viz_type": "table", "row_limit": 10}
|
||||
)
|
||||
|
||||
assert "Quarterly_Revenue" in response.headers["Content-Disposition"]
|
||||
|
||||
|
||||
def test_get_data_response_forwards_slice_to_chart_response(
|
||||
app: SupersetApp,
|
||||
) -> None:
|
||||
"""The slice_ passed by a route must reach _send_chart_response."""
|
||||
command = MagicMock()
|
||||
chart = MagicMock(slice_name="Saved Chart")
|
||||
api = ChartDataRestApi()
|
||||
|
||||
with (
|
||||
app.test_request_context("/api/v1/chart/data"),
|
||||
patch.object(ChartDataRestApi, "_send_chart_response") as mock_send,
|
||||
):
|
||||
api._get_data_response(command, slice_=chart)
|
||||
|
||||
assert mock_send.call_args.kwargs["slice_"] is chart
|
||||
|
||||
|
||||
def test_get_data_route_passes_loaded_chart_to_data_response(
|
||||
app: SupersetApp,
|
||||
) -> None:
|
||||
"""
|
||||
Mutation guard: GET /api/v1/chart/<pk>/data/ must hand the chart it
|
||||
loaded to _get_data_response. A chart's saved query context rarely
|
||||
carries a slice_id for the query context factory to resolve, so
|
||||
dropping this wiring silently regresses the export filename to the
|
||||
viz_type while every other test stays green.
|
||||
"""
|
||||
chart = MagicMock(slice_name="Saved Chart")
|
||||
chart.query_context = json.dumps({"datasource": {"id": 1, "type": "table"}})
|
||||
chart.params = "{}"
|
||||
|
||||
api = ChartDataRestApi()
|
||||
api.datamodel = MagicMock()
|
||||
api.datamodel.get.return_value = chart
|
||||
api._base_filters = []
|
||||
|
||||
# Reach past the route decorators (protect, statsd, event logger) to
|
||||
# exercise the route body itself.
|
||||
get_data = inspect.unwrap(ChartDataRestApi.get_data)
|
||||
|
||||
with (
|
||||
app.test_request_context("/api/v1/chart/1/data/?format=csv"),
|
||||
patch.object(ChartDataRestApi, "_create_query_context_from_form"),
|
||||
patch("superset.charts.data.api.ChartDataCommand"),
|
||||
patch("superset.charts.data.api.is_feature_enabled", return_value=False),
|
||||
patch.object(ChartDataRestApi, "_get_data_response") as mock_response,
|
||||
):
|
||||
get_data(api, 1)
|
||||
|
||||
assert mock_response.call_args.kwargs["slice_"] is chart
|
||||
|
||||
@@ -1,16 +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.
|
||||
@@ -1,45 +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.
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
from jinja2.exceptions import TemplateError, TemplateSyntaxError
|
||||
|
||||
from superset.commands.explore.permalink.create import CreateExplorePermalinkCommand
|
||||
from superset.exceptions import SupersetTemplateException
|
||||
|
||||
check_chart_access = "superset.commands.explore.permalink.create.check_chart_access"
|
||||
|
||||
|
||||
def test_create_permalink_malformed_jinja_template() -> None:
|
||||
# ``check_chart_access`` funnels into ``raise_for_access`` which re-parses the
|
||||
# query's unrendered Jinja via ``process_jinja_sql`` and can raise a raw
|
||||
# ``TemplateError`` (e.g. an unclosed ``{% if %}``). ``TemplateSyntaxError`` is
|
||||
# a subclass of ``TemplateError``. It must surface as a
|
||||
# ``SupersetTemplateException`` (422), not propagate as an opaque 500.
|
||||
assert issubclass(TemplateSyntaxError, TemplateError)
|
||||
|
||||
command = CreateExplorePermalinkCommand(
|
||||
{"formData": {"datasource": "1__table", "slice_id": 1}}
|
||||
)
|
||||
|
||||
with patch(
|
||||
check_chart_access,
|
||||
side_effect=TemplateSyntaxError("unexpected end of template", lineno=1),
|
||||
):
|
||||
with pytest.raises(SupersetTemplateException):
|
||||
command.run()
|
||||
@@ -1,57 +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.
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
from jinja2.exceptions import TemplateError, TemplateSyntaxError
|
||||
|
||||
from superset.commands.explore.permalink.get import GetExplorePermalinkCommand
|
||||
from superset.exceptions import SupersetTemplateException
|
||||
from superset.utils.core import DatasourceType
|
||||
|
||||
check_chart_access = "superset.commands.explore.permalink.get.check_chart_access"
|
||||
decode_permalink_id = "superset.commands.explore.permalink.get.decode_permalink_id"
|
||||
get_value = "superset.daos.key_value.KeyValueDAO.get_value"
|
||||
|
||||
|
||||
def test_get_permalink_malformed_jinja_template() -> None:
|
||||
# ``check_chart_access`` funnels into ``raise_for_access`` which re-parses the
|
||||
# query's unrendered Jinja via ``process_jinja_sql`` and can raise a raw
|
||||
# ``TemplateError`` (e.g. an unclosed ``{% if %}``). ``TemplateSyntaxError`` is
|
||||
# a subclass of ``TemplateError``. It must surface as a
|
||||
# ``SupersetTemplateException`` (422), not propagate as an opaque 500.
|
||||
assert issubclass(TemplateSyntaxError, TemplateError)
|
||||
|
||||
command = GetExplorePermalinkCommand("thisisallmocked")
|
||||
|
||||
with (
|
||||
patch(decode_permalink_id, return_value="123456"),
|
||||
patch(
|
||||
get_value,
|
||||
return_value={
|
||||
"chartId": 1,
|
||||
"datasourceId": 1,
|
||||
"datasourceType": DatasourceType.TABLE.value,
|
||||
},
|
||||
),
|
||||
patch(
|
||||
check_chart_access,
|
||||
side_effect=TemplateSyntaxError("unexpected end of template", lineno=1),
|
||||
),
|
||||
):
|
||||
with pytest.raises(SupersetTemplateException):
|
||||
command.run()
|
||||
@@ -513,14 +513,10 @@ def test_alert_with_nonexistent_database_rejected(mocker: MockerFixture) -> None
|
||||
# --- Retry config validation on update ---
|
||||
|
||||
|
||||
_PATCH_RETRY_FLAG = "superset.commands.report.update.is_feature_enabled"
|
||||
|
||||
|
||||
def test_update_rejects_retry_on_alert(mocker: MockerFixture) -> None:
|
||||
"""Enabling retries on an alert schedule is rejected."""
|
||||
model = _make_model(mocker, model_type=ReportScheduleType.ALERT, database_id=5)
|
||||
_setup_mocks(mocker, model)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
|
||||
cmd = UpdateReportScheduleCommand(model_id=1, data={"retry_on_failure": True})
|
||||
with pytest.raises(ReportScheduleInvalidError) as exc_info:
|
||||
@@ -533,7 +529,6 @@ def test_update_rejects_send_failed_without_retry(mocker: MockerFixture) -> None
|
||||
"""send_failed_reports=True requires retry_on_failure=True."""
|
||||
model = _make_model(mocker, model_type=ReportScheduleType.REPORT, database_id=None)
|
||||
_setup_mocks(mocker, model)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
|
||||
cmd = UpdateReportScheduleCommand(model_id=1, data={"send_failed_reports": True})
|
||||
with pytest.raises(ReportScheduleInvalidError) as exc_info:
|
||||
@@ -546,7 +541,6 @@ def test_update_accepts_retry_on_report(mocker: MockerFixture) -> None:
|
||||
"""Enabling retries on a report schedule is accepted."""
|
||||
model = _make_model(mocker, model_type=ReportScheduleType.REPORT, database_id=None)
|
||||
_setup_mocks(mocker, model)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
|
||||
cmd = UpdateReportScheduleCommand(
|
||||
model_id=1, data={"retry_on_failure": True, "retry_max_attempts": 5}
|
||||
|
||||
@@ -1753,104 +1753,3 @@ def test_unmask_encrypted_extra() -> None:
|
||||
"auth_params": {"username": "alice", "password": "old-password"},
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
def test_impersonate_user_non_trino_backend() -> None:
|
||||
"""
|
||||
Test impersonate_user for non-Trino backends.
|
||||
"""
|
||||
from superset.db_engine_specs.trino import TrinoEngineSpec
|
||||
|
||||
url = make_url("presto://user@host:443/catalog/schema")
|
||||
engine_kwargs: dict[str, Any] = {"connect_args": {}}
|
||||
|
||||
_, new_kwargs = TrinoEngineSpec.impersonate_user(
|
||||
database=MagicMock(),
|
||||
username="alice",
|
||||
user_token=None,
|
||||
url=url,
|
||||
engine_kwargs=engine_kwargs,
|
||||
)
|
||||
|
||||
assert new_kwargs["connect_args"] == {}
|
||||
|
||||
|
||||
def test_impersonate_user_without_token() -> None:
|
||||
"""
|
||||
Test impersonate_user when there isn't a `user_token`.
|
||||
|
||||
Without a user token only the `user` connect arg is set; no HTTP session is
|
||||
built, so the driver keeps handling `verify` itself.
|
||||
"""
|
||||
from superset.db_engine_specs.trino import TrinoEngineSpec
|
||||
|
||||
url = make_url("trino://host:443/catalog/schema")
|
||||
engine_kwargs: dict[str, Any] = {"connect_args": {"verify": False}}
|
||||
|
||||
_, new_kwargs = TrinoEngineSpec.impersonate_user(
|
||||
database=MagicMock(),
|
||||
username="alice",
|
||||
user_token=None,
|
||||
url=url,
|
||||
engine_kwargs=engine_kwargs,
|
||||
)
|
||||
|
||||
assert new_kwargs["connect_args"] == {"user": "alice", "verify": False}
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"verify",
|
||||
[None, False, True, "/path/to/ca-bundle.pem"],
|
||||
)
|
||||
def test_impersonate_user_with_token(verify: Any) -> None:
|
||||
"""
|
||||
Test impersonate_user with a `user_token`.
|
||||
|
||||
With a user token an HTTP session carrying the bearer token is injected. Trino only
|
||||
applies `verify` to a session it builds itself, so the setting has to be copied to
|
||||
ours.
|
||||
"""
|
||||
from superset.db_engine_specs.trino import TrinoEngineSpec
|
||||
|
||||
url = make_url("trino://host:443/catalog/schema")
|
||||
engine_kwargs: dict[str, Any] = {"connect_args": {"verify": verify}}
|
||||
|
||||
_, new_kwargs = TrinoEngineSpec.impersonate_user(
|
||||
database=MagicMock(),
|
||||
username="alice",
|
||||
user_token="user-token", # noqa: S106
|
||||
url=url,
|
||||
engine_kwargs=engine_kwargs,
|
||||
)
|
||||
|
||||
connect_args = new_kwargs["connect_args"]
|
||||
assert connect_args["user"] == "alice"
|
||||
http_session = connect_args["http_session"]
|
||||
assert http_session.headers["Authorization"] == "Bearer user-token"
|
||||
assert http_session.verify == verify
|
||||
# The original connect arg is left in place for the driver.
|
||||
assert connect_args["verify"] == verify
|
||||
|
||||
|
||||
def test_impersonate_user_with_token_no_verify_configured() -> None:
|
||||
"""
|
||||
Test impersonate_user with a `user_token` and no `verify` connect arg.
|
||||
|
||||
Without the key the session keeps the `requests` default, which verifies certs.
|
||||
"""
|
||||
from superset.db_engine_specs.trino import TrinoEngineSpec
|
||||
|
||||
url = make_url("trino://host:443/catalog/schema")
|
||||
engine_kwargs: dict[str, Any] = {"connect_args": {}}
|
||||
|
||||
_, new_kwargs = TrinoEngineSpec.impersonate_user(
|
||||
database=MagicMock(),
|
||||
username="alice",
|
||||
user_token="user-token", # noqa: S106
|
||||
url=url,
|
||||
engine_kwargs=engine_kwargs,
|
||||
)
|
||||
|
||||
connect_args = new_kwargs["connect_args"]
|
||||
assert "verify" not in connect_args
|
||||
assert connect_args["http_session"].verify is True
|
||||
|
||||
@@ -20,7 +20,7 @@ Tests for the get_chart_data request schema and chart type fallback handling.
|
||||
"""
|
||||
|
||||
import importlib
|
||||
from contextlib import nullcontext
|
||||
from contextlib import contextmanager, nullcontext
|
||||
from types import SimpleNamespace
|
||||
from typing import Any
|
||||
from unittest.mock import MagicMock
|
||||
@@ -2532,3 +2532,183 @@ async def test_query_from_form_data_refreshed_reflects_force_refresh_only(
|
||||
assert isinstance(result, ChartData)
|
||||
assert result.cache_status is not None
|
||||
assert result.cache_status.refreshed is expected_refreshed
|
||||
|
||||
|
||||
class _CommittingEventLogger:
|
||||
"""Stand-in for the default ``DBEventLogger`` plus MCP session teardown.
|
||||
|
||||
Two real behaviors are reproduced, in the order production hits them:
|
||||
|
||||
1. ``DBEventLogger.log()`` ends every ``event_logger.log_context`` block
|
||||
with ``db.session.commit()`` (``superset/utils/log.py``), and SQLAlchemy
|
||||
expires every loaded attribute of the fetched ``Slice`` on commit.
|
||||
2. ``superset/mcp_service/session_scope.py`` documents that the request
|
||||
session can be removed while a tool call is still in flight, which
|
||||
detaches that ``Slice``. ``Session.close()`` reproduces exactly that:
|
||||
it expunges every instance and drops the transaction.
|
||||
|
||||
Once both have happened, the next attribute read on the chart raises
|
||||
``DetachedInstanceError``.
|
||||
"""
|
||||
|
||||
def __init__(self, session: Any) -> None:
|
||||
self._session = session
|
||||
self.blocks = 0
|
||||
|
||||
@contextmanager
|
||||
def log_context(self, *_args: Any, **_kwargs: Any) -> Any:
|
||||
yield lambda **_kw: None
|
||||
self._session.commit()
|
||||
self._session.close()
|
||||
self.blocks += 1
|
||||
|
||||
|
||||
def _make_persisted_chart(**overrides: Any) -> Any:
|
||||
"""Create a real ``Slice`` row in a throwaway in-memory session."""
|
||||
from sqlalchemy import create_engine
|
||||
from sqlalchemy.orm import sessionmaker
|
||||
|
||||
from superset.models.slice import Slice
|
||||
|
||||
engine = create_engine("sqlite://")
|
||||
# Slice is versioned by sqlalchemy-continuum, whose before_flush hook
|
||||
# writes to version_transaction, so the whole metadata has to exist.
|
||||
Slice.metadata.create_all(engine)
|
||||
session = sessionmaker(bind=engine)()
|
||||
|
||||
attrs: dict[str, Any] = {
|
||||
"id": 42,
|
||||
"slice_name": "Sales by region",
|
||||
"viz_type": "table",
|
||||
"datasource_id": 1,
|
||||
"datasource_type": "table",
|
||||
"params": json.dumps({"viz_type": "table"}),
|
||||
"query_context": None,
|
||||
}
|
||||
attrs.update(overrides)
|
||||
session.add(Slice(**attrs))
|
||||
session.commit()
|
||||
session.close()
|
||||
|
||||
return session.get(Slice, attrs["id"]), session
|
||||
|
||||
|
||||
@contextmanager
|
||||
def _detaching_get_chart_data(session: Any, validation: Any, chart: Any) -> Any:
|
||||
"""Patch get_chart_data so its session commits and is removed mid-call."""
|
||||
from unittest.mock import Mock, patch
|
||||
|
||||
module = importlib.import_module("superset.mcp_service.chart.tool.get_chart_data")
|
||||
|
||||
user = Mock()
|
||||
user.id = 1
|
||||
user.username = "admin"
|
||||
|
||||
with (
|
||||
patch("superset.mcp_service.auth.get_user_from_request", return_value=user),
|
||||
patch.object(module, "event_logger", _CommittingEventLogger(session)),
|
||||
patch.object(module, "find_chart_by_identifier", return_value=chart),
|
||||
patch.object(module, "validate_chart_dataset", return_value=validation),
|
||||
# ``db`` is only imported by the fixed module; ``raising=False``
|
||||
# semantics keep this patch usable against the unfixed one too.
|
||||
patch.object(module, "db", SimpleNamespace(session=session), create=True),
|
||||
):
|
||||
yield module
|
||||
|
||||
|
||||
class TestDetachedInstanceError:
|
||||
"""``get_chart_data`` must survive its session committing then being removed.
|
||||
|
||||
The tool fetches a ``Slice`` once and then reads columns off it for the
|
||||
rest of a long ``async`` call. Every ``event_logger.log_context`` exit
|
||||
commits the request session, which expires those columns; if the per-call
|
||||
session is then removed while the call is still running, the next read
|
||||
raises ``DetachedInstanceError``. That is caught by the tool's broad
|
||||
``except (..., SQLAlchemyError, ...)`` handler, so the caller gets
|
||||
"Failed to get chart data: Instance <Slice ...> is not bound to a Session"
|
||||
instead of the chart's data.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
async def _call(identifier: Any = 42) -> dict[str, Any]:
|
||||
from fastmcp import Client
|
||||
|
||||
from superset.mcp_service.app import mcp
|
||||
|
||||
async with Client(mcp) as client:
|
||||
result = await client.call_tool(
|
||||
"get_chart_data", {"request": {"identifier": identifier}}
|
||||
)
|
||||
return json.loads(result.content[0].text)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_first_chart_reads_survive_commit_and_removal(self) -> None:
|
||||
"""The reads immediately after the lookup must not hit a dead session."""
|
||||
chart, session = _make_persisted_chart()
|
||||
validation = SimpleNamespace(
|
||||
is_valid=False, error="Dataset is gone", warnings=[]
|
||||
)
|
||||
|
||||
with _detaching_get_chart_data(session, validation, chart):
|
||||
data = await self._call()
|
||||
|
||||
assert "not bound to a Session" not in str(data), (
|
||||
f"get_chart_data leaked a SQLAlchemy session error to the caller: {data}"
|
||||
)
|
||||
assert data["error_type"] == "DatasetNotAccessible"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_later_chart_reads_survive_commit_and_removal(self) -> None:
|
||||
"""Reads further down the tool (params, query_context, viz_type) too."""
|
||||
chart, session = _make_persisted_chart(params=json.dumps({}))
|
||||
validation = SimpleNamespace(is_valid=True, error=None, warnings=[])
|
||||
|
||||
with _detaching_get_chart_data(session, validation, chart):
|
||||
data = await self._call()
|
||||
|
||||
assert "not bound to a Session" not in str(data), (
|
||||
f"get_chart_data leaked a SQLAlchemy session error to the caller: {data}"
|
||||
)
|
||||
# Reaching this branch means chart.query_context, chart.params,
|
||||
# chart.viz_type, chart.datasource_id/type and chart.id were all read
|
||||
# successfully after the session had committed and been removed.
|
||||
assert data["error_type"] == "MissingQueryContext"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_scoped_session_proxy_is_unwrapped(self) -> None:
|
||||
"""The guard must reach the real Session, not a scoped_session proxy.
|
||||
|
||||
``db.session`` is a ``scoped_session``, which does not forward
|
||||
``expire_on_commit`` assignment to the Session it wraps -- setting the
|
||||
flag on the proxy is a silent no-op. Guarding the proxy instead of the
|
||||
underlying Session would leave the bug fully intact in production.
|
||||
"""
|
||||
from sqlalchemy.orm import scoped_session
|
||||
|
||||
chart, session = _make_persisted_chart()
|
||||
proxy = scoped_session(lambda: session)
|
||||
validation = SimpleNamespace(
|
||||
is_valid=False, error="Dataset is gone", warnings=[]
|
||||
)
|
||||
|
||||
with _detaching_get_chart_data(proxy, validation, chart):
|
||||
data = await self._call()
|
||||
|
||||
assert "not bound to a Session" not in str(data), (
|
||||
f"Chart columns expired through the scoped_session proxy: {data}"
|
||||
)
|
||||
assert data["error_type"] == "DatasetNotAccessible"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_expire_on_commit_is_restored(self) -> None:
|
||||
"""The guard must not leak its relaxed setting past the tool call."""
|
||||
chart, session = _make_persisted_chart()
|
||||
validation = SimpleNamespace(
|
||||
is_valid=False, error="Dataset is gone", warnings=[]
|
||||
)
|
||||
assert session.expire_on_commit is True
|
||||
|
||||
with _detaching_get_chart_data(session, validation, chart):
|
||||
await self._call()
|
||||
|
||||
assert session.expire_on_commit is True
|
||||
|
||||
@@ -38,7 +38,6 @@ from superset.mcp_service.dashboard.schemas import (
|
||||
DuplicateDashboardResponse,
|
||||
GenerateDashboardRequest,
|
||||
GetDashboardInfoRequest,
|
||||
GetDashboardLayoutRequest,
|
||||
ListDashboardsRequest,
|
||||
ManageDashboardOwnersResponse,
|
||||
ManageDashboardRolesResponse,
|
||||
@@ -976,55 +975,6 @@ class TestRequestSchemaAliasChoices:
|
||||
)
|
||||
assert req.select_columns == ["id", "dashboard_title"]
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"payload",
|
||||
[
|
||||
{},
|
||||
{"identifier": ""},
|
||||
{"identifier": " "},
|
||||
{"permalink_key": ""},
|
||||
{"permalink_key": " "},
|
||||
{"identifier": " ", "permalink_key": " "},
|
||||
],
|
||||
)
|
||||
def test_get_dashboard_info_requires_reference(
|
||||
self, payload: dict[str, Any]
|
||||
) -> None:
|
||||
with pytest.raises(ValidationError, match="identifier or permalink_key"):
|
||||
GetDashboardInfoRequest.model_validate(payload)
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"payload",
|
||||
[
|
||||
{"identifier": 42},
|
||||
{"permalink_key": "shared-key"},
|
||||
{"identifier": 42, "permalink_key": "shared-key"},
|
||||
],
|
||||
)
|
||||
def test_get_dashboard_layout_accepts_reference(
|
||||
self, payload: dict[str, Any]
|
||||
) -> None:
|
||||
request = GetDashboardLayoutRequest.model_validate(payload)
|
||||
assert request.identifier == payload.get("identifier")
|
||||
assert request.permalink_key == payload.get("permalink_key")
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"payload",
|
||||
[
|
||||
{},
|
||||
{"identifier": ""},
|
||||
{"identifier": " "},
|
||||
{"permalink_key": ""},
|
||||
{"permalink_key": " "},
|
||||
{"identifier": " ", "permalink_key": " "},
|
||||
],
|
||||
)
|
||||
def test_get_dashboard_layout_requires_reference(
|
||||
self, payload: dict[str, Any]
|
||||
) -> None:
|
||||
with pytest.raises(ValidationError, match="identifier or permalink_key"):
|
||||
GetDashboardLayoutRequest.model_validate(payload)
|
||||
|
||||
def test_list_dashboards_select_columns_columns_alias(self) -> None:
|
||||
req = ListDashboardsRequest.model_validate(
|
||||
{"columns": ["id", "dashboard_title"]}
|
||||
|
||||
@@ -1,401 +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.
|
||||
|
||||
"""Tests for MCP dashboard layout validation."""
|
||||
|
||||
from copy import deepcopy
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
import yaml
|
||||
|
||||
from superset.mcp_service.dashboard.layout_validation import (
|
||||
validate_dashboard_layout,
|
||||
)
|
||||
|
||||
|
||||
def _grid_layout() -> dict[str, Any]:
|
||||
return {
|
||||
"DASHBOARD_VERSION_KEY": "v2",
|
||||
"ROOT_ID": {
|
||||
"children": ["GRID_ID"],
|
||||
"id": "ROOT_ID",
|
||||
"type": "ROOT",
|
||||
},
|
||||
"GRID_ID": {
|
||||
"children": ["ROW-1"],
|
||||
"id": "GRID_ID",
|
||||
"parents": ["ROOT_ID"],
|
||||
"type": "GRID",
|
||||
},
|
||||
"ROW-1": {
|
||||
"children": ["CHART-1"],
|
||||
"id": "ROW-1",
|
||||
"meta": {},
|
||||
"parents": ["ROOT_ID", "GRID_ID"],
|
||||
"type": "ROW",
|
||||
},
|
||||
"CHART-1": {
|
||||
"children": [],
|
||||
"id": "CHART-1",
|
||||
"meta": {"chartId": 1},
|
||||
"parents": ["ROOT_ID", "GRID_ID", "ROW-1"],
|
||||
"type": "CHART",
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
def test_valid_layout() -> None:
|
||||
assert validate_dashboard_layout(_grid_layout(), {1}) is None
|
||||
|
||||
|
||||
def test_valid_empty_grid() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["GRID_ID"]["children"] = []
|
||||
del layout["ROW-1"]
|
||||
del layout["CHART-1"]
|
||||
|
||||
assert validate_dashboard_layout(layout, set()) is None
|
||||
|
||||
|
||||
def test_accepts_decimal_string_chart_id() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["CHART-1"]["meta"]["chartId"] = "1"
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) is None
|
||||
|
||||
|
||||
def test_valid_top_level_tabs_with_reserved_nodes() -> None:
|
||||
layout = {
|
||||
"DASHBOARD_VERSION_KEY": "v2",
|
||||
"ROOT_ID": {
|
||||
"children": ["TABS-1"],
|
||||
"id": "ROOT_ID",
|
||||
"type": "ROOT",
|
||||
},
|
||||
"GRID_ID": {
|
||||
"children": [],
|
||||
"id": "GRID_ID",
|
||||
"parents": ["ROOT_ID"],
|
||||
"type": "GRID",
|
||||
},
|
||||
"HEADER_ID": {
|
||||
"id": "HEADER_ID",
|
||||
"meta": {"text": "Tabbed dashboard"},
|
||||
"type": "HEADER",
|
||||
},
|
||||
"TABS-1": {
|
||||
"children": ["TAB-1"],
|
||||
"id": "TABS-1",
|
||||
"meta": {},
|
||||
"parents": ["ROOT_ID"],
|
||||
"type": "TABS",
|
||||
},
|
||||
"TAB-1": {
|
||||
"children": ["CHART-1"],
|
||||
"id": "TAB-1",
|
||||
"meta": {"text": "Overview"},
|
||||
"parents": ["ROOT_ID", "TABS-1"],
|
||||
"type": "TAB",
|
||||
},
|
||||
"CHART-1": {
|
||||
"id": "CHART-1",
|
||||
"meta": {"chartId": 1},
|
||||
"parents": ["ROOT_ID", "TABS-1", "TAB-1"],
|
||||
"type": "CHART",
|
||||
},
|
||||
}
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) is None
|
||||
|
||||
|
||||
def test_rejects_unreachable_chart() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["GRID_ID"]["children"] = []
|
||||
|
||||
error = validate_dashboard_layout(layout, {1})
|
||||
|
||||
assert error == "Layout component ROW-1 is unreachable from ROOT_ID."
|
||||
|
||||
|
||||
def test_rejects_missing_child_reference() -> None:
|
||||
layout = _grid_layout()
|
||||
del layout["ROW-1"]
|
||||
|
||||
error = validate_dashboard_layout(layout, {1})
|
||||
|
||||
assert error == "Layout references missing component ROW-1."
|
||||
|
||||
|
||||
def test_rejects_cycle() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["GRID_ID"]["children"] = []
|
||||
layout["ROW-1"]["children"] = ["COLUMN-1"]
|
||||
layout["ROW-1"]["parents"] = ["COLUMN-1"]
|
||||
layout["COLUMN-1"] = {
|
||||
"children": ["ROW-1"],
|
||||
"id": "COLUMN-1",
|
||||
"meta": {},
|
||||
"parents": ["ROW-1"],
|
||||
"type": "COLUMN",
|
||||
}
|
||||
|
||||
error = validate_dashboard_layout(layout, {1})
|
||||
|
||||
assert error == "Layout contains a cycle at ROW-1."
|
||||
|
||||
|
||||
def test_accepts_stale_parents_metadata() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["CHART-1"]["parents"] = ["ROOT_ID", "GRID_ID"]
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) is None
|
||||
|
||||
|
||||
def test_accepts_missing_parents_metadata() -> None:
|
||||
layout = _grid_layout()
|
||||
del layout["GRID_ID"]["parents"]
|
||||
del layout["CHART-1"]["parents"]
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) is None
|
||||
|
||||
|
||||
def test_accepts_saved_example_layout_with_stale_parents() -> None:
|
||||
fixture_path = (
|
||||
Path(__file__).parents[4]
|
||||
/ "superset"
|
||||
/ "examples"
|
||||
/ "video_game_sales"
|
||||
/ "dashboard.yaml"
|
||||
)
|
||||
with fixture_path.open(encoding="utf-8") as fixture:
|
||||
layout = yaml.safe_load(fixture)["position"]
|
||||
chart_ids = {
|
||||
component["meta"]["chartId"]
|
||||
for component in layout.values()
|
||||
if isinstance(component, dict) and component.get("type") == "CHART"
|
||||
}
|
||||
|
||||
assert validate_dashboard_layout(layout, chart_ids) is None
|
||||
|
||||
|
||||
def test_rejects_component_in_invalid_parent() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["ROW-1"]["type"] = "TABS"
|
||||
|
||||
error = validate_dashboard_layout(layout, {1})
|
||||
|
||||
assert error == "Layout component CHART-1 cannot be a child of ROW-1."
|
||||
|
||||
|
||||
def test_rejects_unsupported_component_type() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["ROW-1"]["type"] = "UNKNOWN"
|
||||
|
||||
error = validate_dashboard_layout(layout, {1})
|
||||
|
||||
assert error == "Layout component ROW-1 has unsupported type."
|
||||
|
||||
|
||||
def test_rejects_layout_that_hides_associated_chart() -> None:
|
||||
layout = deepcopy(_grid_layout())
|
||||
layout["CHART-1"]["meta"]["chartId"] = 2
|
||||
|
||||
error = validate_dashboard_layout(layout, {1, 2})
|
||||
|
||||
assert error == "Layout would hide dashboard charts: [1]."
|
||||
|
||||
|
||||
def test_rejects_chart_not_associated_with_dashboard() -> None:
|
||||
error = validate_dashboard_layout(_grid_layout(), set())
|
||||
|
||||
assert error == "Layout references charts not associated with the dashboard: [1]."
|
||||
|
||||
|
||||
def test_rejects_empty_root() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["ROOT_ID"]["children"] = []
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) == (
|
||||
"ROOT_ID must contain exactly one GRID or TABS component."
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_empty_tabs() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["ROOT_ID"]["children"] = ["TABS-1"]
|
||||
layout["TABS-1"] = {
|
||||
"children": [],
|
||||
"id": "TABS-1",
|
||||
"meta": {},
|
||||
"parents": ["ROOT_ID"],
|
||||
"type": "TABS",
|
||||
}
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) == (
|
||||
"Tabs component TABS-1 must contain at least one tab."
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_non_component_top_level_value() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["BROKEN"] = None
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) == (
|
||||
"Layout value BROKEN must be a component object."
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_invalid_version() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["DASHBOARD_VERSION_KEY"] = None
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) == (
|
||||
"DASHBOARD_VERSION_KEY must be the string 'v2'."
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_missing_renderer_metadata() -> None:
|
||||
layout = _grid_layout()
|
||||
del layout["ROW-1"]["meta"]
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) == (
|
||||
"Layout component ROW-1.meta must be an object."
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_dynamic_component() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["CHART-1"]["type"] = "DYNAMIC"
|
||||
layout["CHART-1"]["meta"] = {"componentKey": "unknown"}
|
||||
|
||||
assert validate_dashboard_layout(layout, set()) == (
|
||||
"Layout component CHART-1 uses DYNAMIC, which cannot be safely "
|
||||
"validated by the server."
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_malformed_string_chart_id() -> None:
|
||||
layout = _grid_layout()
|
||||
layout["CHART-1"]["meta"]["chartId"] = "1.0"
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) == (
|
||||
"Chart component CHART-1 must have a positive integer or "
|
||||
"decimal-string chartId."
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_leading_zero_string_chart_id() -> None:
|
||||
# ``remove_chart_from_dashboard`` cleans json_metadata by ``str(chart_id)``,
|
||||
# so accepting "001" here would let a chart be detached while stale "001"
|
||||
# references survive in expanded_slices and timed_refresh_immune_slices.
|
||||
layout = _grid_layout()
|
||||
layout["CHART-1"]["meta"]["chartId"] = "001"
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) == (
|
||||
"Chart component CHART-1 must have a positive integer or "
|
||||
"decimal-string chartId."
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_oversized_string_chart_id() -> None:
|
||||
# Guards CPython's integer string conversion limit: an unbounded int()
|
||||
# would raise ValueError out of the tool instead of returning an error.
|
||||
layout = _grid_layout()
|
||||
layout["CHART-1"]["meta"]["chartId"] = "9" * 10_000
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) == (
|
||||
"Chart component CHART-1 must have a positive integer or "
|
||||
"decimal-string chartId."
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_nesting_beyond_frontend_depth_limit() -> None:
|
||||
# isValidChild.ts caps COLUMN > ROW at a parent depth of three. Nesting
|
||||
# ROW > COLUMN > ROW > COLUMN > ROW pushes the innermost COLUMN past it.
|
||||
layout = _grid_layout()
|
||||
layout["ROW-1"]["children"] = ["COLUMN-1"]
|
||||
layout["COLUMN-1"] = {
|
||||
"children": ["ROW-2"],
|
||||
"id": "COLUMN-1",
|
||||
"meta": {},
|
||||
"type": "COLUMN",
|
||||
}
|
||||
layout["ROW-2"] = {
|
||||
"children": ["COLUMN-2"],
|
||||
"id": "ROW-2",
|
||||
"meta": {},
|
||||
"type": "ROW",
|
||||
}
|
||||
layout["COLUMN-2"] = {
|
||||
"children": ["ROW-3"],
|
||||
"id": "COLUMN-2",
|
||||
"meta": {},
|
||||
"type": "COLUMN",
|
||||
}
|
||||
layout["ROW-3"] = {
|
||||
"children": ["CHART-1"],
|
||||
"id": "ROW-3",
|
||||
"meta": {},
|
||||
"type": "ROW",
|
||||
}
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) == (
|
||||
"Layout component ROW-3 is nested too deeply under COLUMN-2."
|
||||
)
|
||||
|
||||
|
||||
def test_accepts_maximum_supported_nesting_depth() -> None:
|
||||
# The deepest arrangement isValidChild.ts documents as valid:
|
||||
# root > grid > row > column > row > chart.
|
||||
layout = _grid_layout()
|
||||
layout["ROW-1"]["children"] = ["COLUMN-1"]
|
||||
layout["COLUMN-1"] = {
|
||||
"children": ["ROW-2"],
|
||||
"id": "COLUMN-1",
|
||||
"meta": {},
|
||||
"type": "COLUMN",
|
||||
}
|
||||
layout["ROW-2"] = {
|
||||
"children": ["CHART-1"],
|
||||
"id": "ROW-2",
|
||||
"meta": {},
|
||||
"type": "ROW",
|
||||
}
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) is None
|
||||
|
||||
|
||||
def test_accepts_tabs_without_consuming_depth() -> None:
|
||||
# TABS and TAB render children at their own depth, so a tab-wrapped row
|
||||
# must remain valid at the depth its enclosing container already had.
|
||||
layout = _grid_layout()
|
||||
layout["GRID_ID"]["children"] = ["TABS-1"]
|
||||
layout["TABS-1"] = {
|
||||
"children": ["TAB-1"],
|
||||
"id": "TABS-1",
|
||||
"meta": {},
|
||||
"type": "TABS",
|
||||
}
|
||||
layout["TAB-1"] = {
|
||||
"children": ["ROW-1"],
|
||||
"id": "TAB-1",
|
||||
"meta": {},
|
||||
"type": "TAB",
|
||||
}
|
||||
|
||||
assert validate_dashboard_layout(layout, {1}) is None
|
||||
@@ -1,374 +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.
|
||||
|
||||
"""Tests for MCP dashboard permalink helpers."""
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
from flask import g
|
||||
|
||||
from superset.commands.dashboard.exceptions import DashboardAccessDeniedError
|
||||
from superset.mcp_service.dashboard.permalink import (
|
||||
DashboardLookupResult,
|
||||
extract_dashboard_permalink_key,
|
||||
get_dashboard_permalink,
|
||||
get_matching_dashboard_permalink_state,
|
||||
lookup_dashboard_reference,
|
||||
refresh_request_user_for_permalink_access,
|
||||
)
|
||||
|
||||
FOUND = "dashboard"
|
||||
PERMALINK_URL = "https://example.test/dashboard/p/shared-key/"
|
||||
|
||||
|
||||
def _lookup_finding(*found_identifiers: int | str):
|
||||
"""Build a ``lookup`` callable that only resolves ``found_identifiers``."""
|
||||
known = {str(identifier) for identifier in found_identifiers}
|
||||
|
||||
def lookup(identifier: int | str) -> str | None:
|
||||
return FOUND if str(identifier) in known else None
|
||||
|
||||
return lookup
|
||||
|
||||
|
||||
def _is_found(result: str | None) -> bool:
|
||||
return result is not None
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("value", "expected"),
|
||||
[
|
||||
("bare-key", "bare-key"),
|
||||
("/superset/dashboard/p/shared-key/", "shared-key"),
|
||||
(
|
||||
"https://example.test/prefix/dashboard/p/shared-key/?foo=bar#tab",
|
||||
"shared-key",
|
||||
),
|
||||
(
|
||||
"https://example.test/dashboard/not-p/shared-key/",
|
||||
"https://example.test/dashboard/not-p/shared-key/",
|
||||
),
|
||||
],
|
||||
)
|
||||
def test_extract_dashboard_permalink_key(value: str, expected: str) -> None:
|
||||
assert extract_dashboard_permalink_key(value) == expected
|
||||
|
||||
|
||||
@patch(
|
||||
"superset.commands.dashboard.permalink.get.GetDashboardPermalinkCommand.run",
|
||||
side_effect=DashboardAccessDeniedError(),
|
||||
)
|
||||
@patch(
|
||||
"superset.mcp_service.dashboard.permalink.refresh_request_user_for_permalink_access"
|
||||
)
|
||||
def test_get_dashboard_permalink_hides_access_denial(mock_refresh, mock_run) -> None:
|
||||
assert get_dashboard_permalink("inaccessible-key") is None
|
||||
mock_refresh.assert_called_once_with()
|
||||
mock_run.assert_called_once_with()
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("username", "email", "expected_kwargs"),
|
||||
[
|
||||
("admin", None, {"username": "admin"}),
|
||||
(None, "admin@example.com", {"email": "admin@example.com"}),
|
||||
],
|
||||
)
|
||||
def test_refresh_request_user_for_permalink_access(
|
||||
app, username: str | None, email: str | None, expected_kwargs: dict[str, str]
|
||||
) -> None:
|
||||
current_user = type(
|
||||
"CurrentUser",
|
||||
(),
|
||||
{"username": username, "email": email, "is_anonymous": False},
|
||||
)()
|
||||
refreshed_user = object()
|
||||
with (
|
||||
patch(
|
||||
"superset.mcp_service.dashboard.permalink.load_user_with_relationships",
|
||||
return_value=refreshed_user,
|
||||
) as mock_load,
|
||||
app.test_request_context("/mcp"),
|
||||
):
|
||||
g.user = current_user
|
||||
refresh_request_user_for_permalink_access()
|
||||
assert g.user is refreshed_user
|
||||
|
||||
mock_load.assert_called_once_with(**expected_kwargs)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("username", "email", "is_anonymous"),
|
||||
[("anonymous", "anonymous@example.com", True), (None, None, False)],
|
||||
)
|
||||
def test_refresh_request_user_for_permalink_access_skips_unresolvable_user(
|
||||
app, username: str | None, email: str | None, is_anonymous: bool
|
||||
) -> None:
|
||||
current_user = type(
|
||||
"CurrentUser",
|
||||
(),
|
||||
{"username": username, "email": email, "is_anonymous": is_anonymous},
|
||||
)()
|
||||
with (
|
||||
patch(
|
||||
"superset.mcp_service.dashboard.permalink.load_user_with_relationships"
|
||||
) as mock_load,
|
||||
app.test_request_context("/mcp"),
|
||||
):
|
||||
g.user = current_user
|
||||
refresh_request_user_for_permalink_access()
|
||||
assert g.user is current_user
|
||||
|
||||
mock_load.assert_not_called()
|
||||
|
||||
|
||||
def test_refresh_request_user_for_permalink_access_keeps_user_when_reload_fails(
|
||||
app,
|
||||
) -> None:
|
||||
current_user = type(
|
||||
"CurrentUser",
|
||||
(),
|
||||
{"username": "admin", "email": None, "is_anonymous": False},
|
||||
)()
|
||||
with (
|
||||
patch(
|
||||
"superset.mcp_service.dashboard.permalink.load_user_with_relationships",
|
||||
return_value=None,
|
||||
) as mock_load,
|
||||
app.test_request_context("/mcp"),
|
||||
):
|
||||
g.user = current_user
|
||||
refresh_request_user_for_permalink_access()
|
||||
assert g.user is current_user
|
||||
|
||||
mock_load.assert_called_once_with(username="admin")
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("reference", "expected_match"),
|
||||
[
|
||||
# CreateDashboardPermalinkCommand stores str(dashboard.uuid).
|
||||
("3f1a2b6c-9d4e-4f80-9c2a-7b1d5e6f8a90", True),
|
||||
# Legacy permalinks may hold the numeric id or the slug.
|
||||
("42", True),
|
||||
("sales-dashboard", True),
|
||||
("99", False),
|
||||
("00000000-0000-0000-0000-000000000000", False),
|
||||
],
|
||||
)
|
||||
def test_get_matching_dashboard_permalink_state_accepts_every_identifier(
|
||||
app, reference: str, expected_match: bool
|
||||
) -> None:
|
||||
lookup_result = DashboardLookupResult(
|
||||
result=object(),
|
||||
permalink_key="key-1",
|
||||
permalink_value={"dashboardId": reference, "state": {"activeTabs": ["TAB-A"]}},
|
||||
)
|
||||
with app.test_request_context("/mcp"):
|
||||
state = get_matching_dashboard_permalink_state(
|
||||
lookup_result,
|
||||
42,
|
||||
"3f1a2b6c-9d4e-4f80-9c2a-7b1d5e6f8a90",
|
||||
"sales-dashboard",
|
||||
)
|
||||
|
||||
assert (state is not None) is expected_match
|
||||
|
||||
|
||||
def test_get_matching_dashboard_permalink_state_skips_check_when_permalink_resolved(
|
||||
app,
|
||||
) -> None:
|
||||
"""The permalink-only path already selected the dashboard from the permalink,
|
||||
so its state is never re-verified against the resolved identifiers.
|
||||
"""
|
||||
lookup_result = DashboardLookupResult(
|
||||
result=object(),
|
||||
permalink_key="key-1",
|
||||
permalink_value={"dashboardId": "whatever", "state": {"activeTabs": ["TAB-A"]}},
|
||||
resolved_from_permalink=True,
|
||||
)
|
||||
with app.test_request_context("/mcp"):
|
||||
state = get_matching_dashboard_permalink_state(lookup_result, 42)
|
||||
|
||||
assert state is not None
|
||||
assert state.key == "key-1"
|
||||
|
||||
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
def test_lookup_dashboard_reference_identifier_only(mock_get_permalink) -> None:
|
||||
result = lookup_dashboard_reference(
|
||||
identifier=42,
|
||||
permalink_key=None,
|
||||
lookup=_lookup_finding(42),
|
||||
is_found=_is_found,
|
||||
)
|
||||
|
||||
assert result.result == FOUND
|
||||
assert result.permalink_key is None
|
||||
assert result.permalink_value is None
|
||||
assert result.resolved_from_permalink is False
|
||||
mock_get_permalink.assert_not_called()
|
||||
|
||||
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
def test_lookup_dashboard_reference_identifier_wins_but_permalink_adds_state(
|
||||
mock_get_permalink,
|
||||
) -> None:
|
||||
"""An explicit identifier selects the dashboard; the permalink only adds state."""
|
||||
value = {"dashboardId": "42", "state": {"activeTabs": ["TAB-A"]}}
|
||||
mock_get_permalink.return_value = ("key-1", value)
|
||||
|
||||
result = lookup_dashboard_reference(
|
||||
identifier=42,
|
||||
permalink_key="key-1",
|
||||
lookup=_lookup_finding(42),
|
||||
is_found=_is_found,
|
||||
)
|
||||
|
||||
assert result.result == FOUND
|
||||
assert result.permalink_key == "key-1"
|
||||
assert result.permalink_value == value
|
||||
assert result.resolved_from_permalink is False
|
||||
|
||||
|
||||
@patch(
|
||||
"superset.mcp_service.dashboard.permalink.get_dashboard_permalink",
|
||||
return_value=None,
|
||||
)
|
||||
def test_lookup_dashboard_reference_keeps_dashboard_when_permalink_unresolvable(
|
||||
mock_get_permalink,
|
||||
) -> None:
|
||||
result = lookup_dashboard_reference(
|
||||
identifier=42,
|
||||
permalink_key="expired-key",
|
||||
lookup=_lookup_finding(42),
|
||||
is_found=_is_found,
|
||||
)
|
||||
|
||||
assert result.result == FOUND
|
||||
assert result.permalink_key == "expired-key"
|
||||
assert result.permalink_value is None
|
||||
|
||||
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
def test_lookup_dashboard_reference_numeric_identifier_not_found(
|
||||
mock_get_permalink,
|
||||
) -> None:
|
||||
"""A numeric identifier never falls back to permalink resolution."""
|
||||
result = lookup_dashboard_reference(
|
||||
identifier=99,
|
||||
permalink_key=None,
|
||||
lookup=_lookup_finding(),
|
||||
is_found=_is_found,
|
||||
)
|
||||
|
||||
assert result.result is None
|
||||
assert result.permalink_key is None
|
||||
mock_get_permalink.assert_not_called()
|
||||
|
||||
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
def test_lookup_dashboard_reference_identifier_not_found_with_explicit_permalink(
|
||||
mock_get_permalink,
|
||||
) -> None:
|
||||
"""An explicit permalink_key keeps the identifier's own not-found result."""
|
||||
result = lookup_dashboard_reference(
|
||||
identifier="missing-slug",
|
||||
permalink_key="key-1",
|
||||
lookup=_lookup_finding(),
|
||||
is_found=_is_found,
|
||||
)
|
||||
|
||||
assert result.result is None
|
||||
assert result.permalink_key == "key-1"
|
||||
mock_get_permalink.assert_not_called()
|
||||
|
||||
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
def test_lookup_dashboard_reference_shared_url_identifier(mock_get_permalink) -> None:
|
||||
value = {"dashboardId": "42", "state": {"activeTabs": ["TAB-A"]}}
|
||||
mock_get_permalink.return_value = ("shared-key", value)
|
||||
|
||||
result = lookup_dashboard_reference(
|
||||
identifier=PERMALINK_URL,
|
||||
permalink_key=None,
|
||||
lookup=_lookup_finding(42),
|
||||
is_found=_is_found,
|
||||
)
|
||||
|
||||
assert result.result == FOUND
|
||||
assert result.permalink_key == "shared-key"
|
||||
assert result.permalink_value == value
|
||||
assert result.resolved_from_permalink is True
|
||||
mock_get_permalink.assert_called_once_with("shared-key")
|
||||
|
||||
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
def test_lookup_dashboard_reference_permalink_only(mock_get_permalink) -> None:
|
||||
value = {"dashboardId": "42", "state": {"activeTabs": ["TAB-A"]}}
|
||||
mock_get_permalink.return_value = ("key-1", value)
|
||||
|
||||
result = lookup_dashboard_reference(
|
||||
identifier=None,
|
||||
permalink_key="key-1",
|
||||
lookup=_lookup_finding(42),
|
||||
is_found=_is_found,
|
||||
)
|
||||
|
||||
assert result.result == FOUND
|
||||
assert result.permalink_key == "key-1"
|
||||
assert result.resolved_from_permalink is True
|
||||
|
||||
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
def test_lookup_dashboard_reference_bare_string_falls_back_to_permalink(
|
||||
mock_get_permalink,
|
||||
) -> None:
|
||||
"""An ambiguous bare string tries identifier lookup before permalink lookup."""
|
||||
value = {"dashboardId": "42", "state": {"activeTabs": ["TAB-A"]}}
|
||||
mock_get_permalink.return_value = ("maybe-key", value)
|
||||
|
||||
result = lookup_dashboard_reference(
|
||||
identifier="maybe-key",
|
||||
permalink_key=None,
|
||||
lookup=_lookup_finding(42),
|
||||
is_found=_is_found,
|
||||
)
|
||||
|
||||
assert result.result == FOUND
|
||||
assert result.permalink_key == "maybe-key"
|
||||
assert result.resolved_from_permalink is True
|
||||
mock_get_permalink.assert_called_once_with("maybe-key")
|
||||
|
||||
|
||||
@patch(
|
||||
"superset.mcp_service.dashboard.permalink.get_dashboard_permalink",
|
||||
return_value=None,
|
||||
)
|
||||
def test_lookup_dashboard_reference_unresolvable_reference(mock_get_permalink) -> None:
|
||||
result = lookup_dashboard_reference(
|
||||
identifier="nonexistent",
|
||||
permalink_key=None,
|
||||
lookup=_lookup_finding(),
|
||||
is_found=_is_found,
|
||||
)
|
||||
|
||||
assert result.result is None
|
||||
assert result.permalink_key == "nonexistent"
|
||||
assert result.permalink_value is None
|
||||
assert result.resolved_from_permalink is False
|
||||
@@ -26,13 +26,15 @@ from unittest.mock import Mock, patch
|
||||
import pytest
|
||||
from fastmcp import Client
|
||||
from fastmcp.exceptions import ToolError
|
||||
from flask import g
|
||||
|
||||
from superset.mcp_service.app import mcp
|
||||
from superset.mcp_service.dashboard.schemas import (
|
||||
DashboardError,
|
||||
DashboardInfo,
|
||||
ListDashboardsRequest,
|
||||
)
|
||||
from superset.mcp_service.dashboard.tool.get_dashboard_info import (
|
||||
_refresh_request_user_for_permalink_access,
|
||||
)
|
||||
from superset.utils import json
|
||||
|
||||
logging.basicConfig(level=logging.DEBUG)
|
||||
@@ -465,14 +467,15 @@ async def test_get_dashboard_info_permalink_does_not_double_sanitize(
|
||||
"superset.mcp_service.dashboard.schemas.user_can_view_data_model_metadata",
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
"superset.mcp_service.dashboard.permalink."
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"user_can_view_data_model_metadata",
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
"superset.mcp_service.dashboard.permalink.get_dashboard_permalink",
|
||||
return_value=("permalink-1", permalink_value),
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"_get_permalink_state",
|
||||
return_value=permalink_value,
|
||||
),
|
||||
):
|
||||
async with Client(mcp_server) as client:
|
||||
@@ -548,14 +551,15 @@ async def test_get_dashboard_info_permalink_key_includes_filter_state(
|
||||
"superset.mcp_service.dashboard.schemas.user_can_view_data_model_metadata",
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
"superset.mcp_service.dashboard.permalink."
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"user_can_view_data_model_metadata",
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
"superset.mcp_service.dashboard.permalink.get_dashboard_permalink",
|
||||
return_value=("some-key", permalink_value),
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"_get_permalink_state",
|
||||
return_value=permalink_value,
|
||||
),
|
||||
):
|
||||
async with Client(mcp_server) as client:
|
||||
@@ -571,151 +575,121 @@ async def test_get_dashboard_info_permalink_key_includes_filter_state(
|
||||
assert result.data["permalink_key"] == "some-key"
|
||||
|
||||
|
||||
@patch("superset.mcp_service.mcp_core.ModelGetInfoCore.run_tool")
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_info_resolves_permalink_without_identifier(
|
||||
mock_permalink, mock_run_tool, mcp_server
|
||||
def test_refresh_request_user_for_permalink_access(
|
||||
app,
|
||||
):
|
||||
mock_permalink.return_value = (
|
||||
"shared-key",
|
||||
{"dashboardId": "42", "state": {"activeTabs": ["TAB-A"], "dataMask": {}}},
|
||||
)
|
||||
mock_run_tool.return_value = DashboardInfo(id=42, dashboard_title="Sales Dashboard")
|
||||
refreshed_user = Mock()
|
||||
refreshed_user.username = "admin"
|
||||
refreshed_user.roles = []
|
||||
refreshed_user.groups = []
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_info", {"request": {"permalink_key": "shared-key"}}
|
||||
)
|
||||
current_user = Mock()
|
||||
current_user.username = "admin"
|
||||
current_user.email = None
|
||||
current_user.is_anonymous = False
|
||||
|
||||
assert result.data["id"] == 42
|
||||
assert result.data["permalink_key"] == "shared-key"
|
||||
assert result.data["filter_state"]["activeTabs"] == [_wrapped("TAB-A")]
|
||||
mock_run_tool.assert_called_once_with("42")
|
||||
with (
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"load_user_with_relationships",
|
||||
return_value=refreshed_user,
|
||||
) as mock_load_user_with_relationships,
|
||||
app.test_request_context("/mcp"),
|
||||
):
|
||||
g.user = current_user
|
||||
_refresh_request_user_for_permalink_access()
|
||||
|
||||
mock_load_user_with_relationships.assert_called_once_with(username="admin")
|
||||
assert g.user is refreshed_user
|
||||
|
||||
|
||||
@patch(
|
||||
"superset.mcp_service.dashboard.permalink.get_dashboard_permalink",
|
||||
return_value=None,
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_info_invalid_permalink_is_actionable(
|
||||
mock_permalink, mcp_server
|
||||
def test_refresh_request_user_for_permalink_access_uses_email_when_username_missing(
|
||||
app,
|
||||
):
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_info", {"request": {"permalink_key": "expired-key"}}
|
||||
refreshed_user = Mock()
|
||||
refreshed_user.email = "admin@example.com"
|
||||
|
||||
current_user = Mock()
|
||||
current_user.username = None
|
||||
current_user.email = "admin@example.com"
|
||||
current_user.is_anonymous = False
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"load_user_with_relationships",
|
||||
return_value=refreshed_user,
|
||||
) as mock_load_user_with_relationships,
|
||||
app.test_request_context("/mcp"),
|
||||
):
|
||||
g.user = current_user
|
||||
_refresh_request_user_for_permalink_access()
|
||||
|
||||
mock_load_user_with_relationships.assert_called_once_with(
|
||||
email="admin@example.com"
|
||||
)
|
||||
|
||||
assert result.data["error_type"] == "permalink_not_found"
|
||||
assert "fresh shared dashboard link" in result.data["error"]
|
||||
assert g.user is refreshed_user
|
||||
|
||||
|
||||
@patch("superset.mcp_service.mcp_core.ModelGetInfoCore.run_tool")
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_info_identifier_takes_precedence_over_permalink(
|
||||
mock_permalink, mock_run_tool, mcp_server
|
||||
):
|
||||
mock_permalink.return_value = (
|
||||
"dashboard-20-key",
|
||||
{"dashboardId": "20", "state": {"activeTabs": ["TAB-20"]}},
|
||||
)
|
||||
mock_run_tool.return_value = DashboardInfo(
|
||||
id=10, dashboard_title="Requested Dashboard"
|
||||
)
|
||||
def test_refresh_request_user_for_permalink_access_skips_anonymous_user(app):
|
||||
current_user = Mock()
|
||||
current_user.username = "anonymous"
|
||||
current_user.email = "anonymous@example.com"
|
||||
current_user.is_anonymous = True
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_info",
|
||||
{"request": {"identifier": 10, "permalink_key": "dashboard-20-key"}},
|
||||
)
|
||||
with (
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"load_user_with_relationships",
|
||||
) as mock_load_user_with_relationships,
|
||||
app.test_request_context("/mcp"),
|
||||
):
|
||||
g.user = current_user
|
||||
_refresh_request_user_for_permalink_access()
|
||||
|
||||
assert result.data["id"] == 10
|
||||
assert result.data["is_permalink_state"] is False
|
||||
assert "filter_state" not in result.data
|
||||
mock_run_tool.assert_called_once_with(10)
|
||||
mock_load_user_with_relationships.assert_not_called()
|
||||
assert g.user is current_user
|
||||
|
||||
|
||||
@patch("superset.mcp_service.mcp_core.ModelGetInfoCore.run_tool")
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_info_permalink_with_uuid_dashboard_id(
|
||||
mock_permalink, mock_run_tool, mcp_server
|
||||
):
|
||||
"""CreateDashboardPermalinkCommand stores dashboardId as the dashboard UUID,
|
||||
so an explicit identifier plus that permalink must still yield filter state.
|
||||
"""
|
||||
dashboard_uuid = "3f1a2b6c-9d4e-4f80-9c2a-7b1d5e6f8a90"
|
||||
mock_permalink.return_value = (
|
||||
"uuid-key",
|
||||
{"dashboardId": dashboard_uuid, "state": {"activeTabs": ["TAB-A"]}},
|
||||
)
|
||||
mock_run_tool.return_value = DashboardInfo(
|
||||
id=42, dashboard_title="Sales Dashboard", uuid=dashboard_uuid
|
||||
)
|
||||
def test_refresh_request_user_for_permalink_access_skips_missing_identifier(app):
|
||||
current_user = Mock()
|
||||
current_user.username = None
|
||||
current_user.email = None
|
||||
current_user.is_anonymous = False
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_info",
|
||||
{"request": {"identifier": 42, "permalink_key": "uuid-key"}},
|
||||
)
|
||||
with (
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"load_user_with_relationships",
|
||||
) as mock_load_user_with_relationships,
|
||||
app.test_request_context("/mcp"),
|
||||
):
|
||||
g.user = current_user
|
||||
_refresh_request_user_for_permalink_access()
|
||||
|
||||
assert result.data["id"] == 42
|
||||
assert result.data["is_permalink_state"] is True
|
||||
assert result.data["permalink_key"] == "uuid-key"
|
||||
assert result.data["filter_state"]["activeTabs"] == [_wrapped("TAB-A")]
|
||||
mock_load_user_with_relationships.assert_not_called()
|
||||
assert g.user is current_user
|
||||
|
||||
|
||||
@patch("superset.mcp_service.mcp_core.ModelGetInfoCore.run_tool")
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_info_permalink_with_slug_dashboard_id(
|
||||
mock_permalink, mock_run_tool, mcp_server
|
||||
):
|
||||
"""Pre-3.1 permalinks can carry a slug in dashboardId."""
|
||||
mock_permalink.return_value = (
|
||||
"slug-key",
|
||||
{"dashboardId": "sales-dashboard", "state": {"activeTabs": ["TAB-A"]}},
|
||||
)
|
||||
mock_run_tool.return_value = DashboardInfo(
|
||||
id=42, dashboard_title="Sales Dashboard", slug="sales-dashboard"
|
||||
)
|
||||
def test_refresh_request_user_for_permalink_access_keeps_user_when_reload_fails(app):
|
||||
current_user = Mock()
|
||||
current_user.username = "admin"
|
||||
current_user.email = None
|
||||
current_user.is_anonymous = False
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_info",
|
||||
{"request": {"identifier": 42, "permalink_key": "slug-key"}},
|
||||
)
|
||||
with (
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"load_user_with_relationships",
|
||||
return_value=None,
|
||||
) as mock_load_user_with_relationships,
|
||||
app.test_request_context("/mcp"),
|
||||
):
|
||||
g.user = current_user
|
||||
_refresh_request_user_for_permalink_access()
|
||||
|
||||
assert result.data["is_permalink_state"] is True
|
||||
assert result.data["filter_state"]["activeTabs"] == [_wrapped("TAB-A")]
|
||||
|
||||
|
||||
@patch("superset.mcp_service.mcp_core.ModelGetInfoCore.run_tool")
|
||||
@patch(
|
||||
"superset.mcp_service.dashboard.permalink.get_dashboard_permalink",
|
||||
return_value=None,
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_info_unknown_slug_keeps_not_found_error(
|
||||
mock_permalink, mock_run_tool, mcp_server
|
||||
):
|
||||
"""A plain slug typo keeps its own not-found error instead of asking the
|
||||
user for a shared link they never mentioned.
|
||||
"""
|
||||
mock_run_tool.return_value = DashboardError.create(
|
||||
"DashboardInfo with identifier 'sales-dashbord' not found", "not_found"
|
||||
)
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_info", {"request": {"identifier": "sales-dashbord"}}
|
||||
)
|
||||
|
||||
assert result.data["error_type"] == "not_found"
|
||||
assert "sales-dashbord" in result.data["error"]
|
||||
assert "fresh shared dashboard link" not in result.data["error"]
|
||||
mock_load_user_with_relationships.assert_called_once_with(username="admin")
|
||||
assert g.user is current_user
|
||||
|
||||
|
||||
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
|
||||
@@ -952,14 +926,15 @@ async def test_get_dashboard_info_restricted_user_redacts_permalink_filter_state
|
||||
"superset.mcp_service.dashboard.schemas.user_can_view_data_model_metadata",
|
||||
return_value=False,
|
||||
),
|
||||
patch(
|
||||
"superset.mcp_service.dashboard.permalink."
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"user_can_view_data_model_metadata",
|
||||
return_value=False,
|
||||
),
|
||||
patch(
|
||||
"superset.mcp_service.dashboard.permalink.get_dashboard_permalink",
|
||||
return_value=("abc123", permalink_value),
|
||||
patch.object(
|
||||
get_dashboard_info_module,
|
||||
"_get_permalink_state",
|
||||
return_value=permalink_value,
|
||||
),
|
||||
):
|
||||
async with Client(mcp_server) as client:
|
||||
|
||||
@@ -259,143 +259,6 @@ async def test_get_dashboard_layout_not_found(mock_find, mcp_server):
|
||||
assert data["error_type"] == "not_found"
|
||||
|
||||
|
||||
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_layout_resolves_shared_permalink(
|
||||
mock_permalink, mock_find, mcp_server
|
||||
):
|
||||
mock_permalink.return_value = (
|
||||
"shared-key",
|
||||
{
|
||||
"dashboardId": "42",
|
||||
"state": {
|
||||
"activeTabs": ["TAB-2"],
|
||||
"dataMask": {"FILTER-1": {"filterState": {"value": "EMEA"}}},
|
||||
},
|
||||
},
|
||||
)
|
||||
mock_find.return_value = _build_dashboard_mock(
|
||||
dashboard_id=42, position_json=_tabbed_layout()
|
||||
)
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_layout",
|
||||
{
|
||||
"request": {
|
||||
"identifier": "https://example.test/superset/dashboard/p/shared-key/"
|
||||
}
|
||||
},
|
||||
)
|
||||
data = json.loads(result.content[0].text)
|
||||
|
||||
assert data["id"] == 42
|
||||
assert data["permalink_key"] == "shared-key"
|
||||
assert data["is_permalink_state"] is True
|
||||
assert data["filter_state"]["activeTabs"] == [_wrapped("TAB-2")]
|
||||
assert mock_find.call_args_list[-1].args == (42,)
|
||||
|
||||
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_layout_invalid_permalink_is_actionable(
|
||||
mock_permalink, mcp_server
|
||||
):
|
||||
mock_permalink.return_value = None
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_layout", {"request": {"permalink_key": "expired-key"}}
|
||||
)
|
||||
data = json.loads(result.content[0].text)
|
||||
|
||||
assert data["error_type"] == "permalink_not_found"
|
||||
assert "fresh shared dashboard link" in data["error"]
|
||||
|
||||
|
||||
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_layout_identifier_takes_precedence_over_permalink(
|
||||
mock_permalink, mock_find, mcp_server
|
||||
):
|
||||
mock_permalink.return_value = (
|
||||
"dashboard-20-key",
|
||||
{"dashboardId": "20", "state": {"activeTabs": ["TAB-20"]}},
|
||||
)
|
||||
mock_find.return_value = _build_dashboard_mock(
|
||||
dashboard_id=10, position_json=_tabbed_layout()
|
||||
)
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_layout",
|
||||
{"request": {"identifier": 10, "permalink_key": "dashboard-20-key"}},
|
||||
)
|
||||
data = json.loads(result.content[0].text)
|
||||
|
||||
assert data["id"] == 10
|
||||
assert data["is_permalink_state"] is False
|
||||
mock_find.assert_called_once_with(10, query_options=None)
|
||||
|
||||
|
||||
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
|
||||
@patch("superset.mcp_service.dashboard.permalink.get_dashboard_permalink")
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_layout_permalink_with_uuid_dashboard_id(
|
||||
mock_permalink, mock_find, mcp_server
|
||||
):
|
||||
"""CreateDashboardPermalinkCommand stores dashboardId as the dashboard UUID,
|
||||
so an explicit identifier plus that permalink must still yield filter state.
|
||||
"""
|
||||
dashboard_uuid = "3f1a2b6c-9d4e-4f80-9c2a-7b1d5e6f8a90"
|
||||
mock_permalink.return_value = (
|
||||
"uuid-key",
|
||||
{"dashboardId": dashboard_uuid, "state": {"activeTabs": ["TAB-2"]}},
|
||||
)
|
||||
mock_find.return_value = _build_dashboard_mock(
|
||||
dashboard_id=42, uuid=dashboard_uuid, position_json=_tabbed_layout()
|
||||
)
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_layout",
|
||||
{"request": {"identifier": 42, "permalink_key": "uuid-key"}},
|
||||
)
|
||||
data = json.loads(result.content[0].text)
|
||||
|
||||
assert data["id"] == 42
|
||||
assert data["is_permalink_state"] is True
|
||||
assert data["permalink_key"] == "uuid-key"
|
||||
assert data["filter_state"]["activeTabs"] == [_wrapped("TAB-2")]
|
||||
|
||||
|
||||
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
|
||||
@patch(
|
||||
"superset.mcp_service.dashboard.permalink.get_dashboard_permalink",
|
||||
return_value=None,
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_dashboard_layout_unknown_slug_keeps_not_found_error(
|
||||
mock_permalink, mock_find, mcp_server
|
||||
):
|
||||
"""A plain slug typo keeps its own not-found error instead of asking the
|
||||
user for a shared link they never mentioned.
|
||||
"""
|
||||
mock_find.return_value = None
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"get_dashboard_layout", {"request": {"identifier": "sales-dashbord"}}
|
||||
)
|
||||
data = json.loads(result.content[0].text)
|
||||
|
||||
assert data["error_type"] == "not_found"
|
||||
assert "sales-dashbord" in data["error"]
|
||||
assert "fresh shared dashboard link" not in data["error"]
|
||||
|
||||
|
||||
def test_extract_layout_handles_invalid_json():
|
||||
tabs, charts = _extract_layout_from_position("{ not json")
|
||||
assert tabs == []
|
||||
|
||||
@@ -104,20 +104,7 @@ class TestUpdateDashboard:
|
||||
)
|
||||
mock_get.return_value = dash
|
||||
|
||||
position = {
|
||||
"DASHBOARD_VERSION_KEY": "v2",
|
||||
"ROOT_ID": {
|
||||
"id": "ROOT_ID",
|
||||
"type": "ROOT",
|
||||
"children": ["GRID_ID"],
|
||||
},
|
||||
"GRID_ID": {
|
||||
"id": "GRID_ID",
|
||||
"type": "GRID",
|
||||
"parents": ["ROOT_ID"],
|
||||
"children": [],
|
||||
},
|
||||
}
|
||||
position = {"ROOT_ID": {"type": "ROOT", "children": ["GRID_ID"]}}
|
||||
overrides = {
|
||||
"label_colors": {"Electronics": "#4C78A8"},
|
||||
"cross_filters_enabled": False,
|
||||
@@ -189,87 +176,6 @@ class TestUpdateDashboard:
|
||||
assert payload["dashboard"]["dashboard_title"] == modified_title
|
||||
assert payload["dashboard"]["description"] == modified_description
|
||||
|
||||
@patch("superset.daos.dashboard.DashboardDAO.get_by_id_or_slug")
|
||||
@patch("superset.extensions.db.session")
|
||||
@pytest.mark.asyncio
|
||||
async def test_invalid_layout_does_not_replace_existing_content(
|
||||
self, mock_session: Mock, mock_get: Mock, mcp_server: object
|
||||
) -> None:
|
||||
original_position = json.dumps(
|
||||
{
|
||||
"DASHBOARD_VERSION_KEY": "v2",
|
||||
"ROOT_ID": {
|
||||
"id": "ROOT_ID",
|
||||
"type": "ROOT",
|
||||
"children": ["GRID_ID"],
|
||||
},
|
||||
"GRID_ID": {
|
||||
"id": "GRID_ID",
|
||||
"type": "GRID",
|
||||
"parents": ["ROOT_ID"],
|
||||
"children": ["CHART-old"],
|
||||
},
|
||||
"CHART-old": {
|
||||
"id": "CHART-old",
|
||||
"type": "CHART",
|
||||
"parents": ["ROOT_ID", "GRID_ID"],
|
||||
"meta": {"chartId": 10},
|
||||
},
|
||||
}
|
||||
)
|
||||
dash = _mock_dashboard(id=42, position_json=original_position)
|
||||
dash.slices = [Mock(id=10)]
|
||||
mock_get.return_value = dash
|
||||
unreachable_layout = {
|
||||
"DASHBOARD_VERSION_KEY": "v2",
|
||||
"ROOT_ID": {
|
||||
"id": "ROOT_ID",
|
||||
"type": "ROOT",
|
||||
"children": ["TABS-1"],
|
||||
},
|
||||
"TABS-1": {
|
||||
"id": "TABS-1",
|
||||
"type": "TABS",
|
||||
"meta": {},
|
||||
"parents": ["ROOT_ID"],
|
||||
"children": ["TAB-1"],
|
||||
},
|
||||
"TAB-1": {
|
||||
"id": "TAB-1",
|
||||
"type": "TAB",
|
||||
"meta": {"text": "Overview"},
|
||||
"parents": ["ROOT_ID", "TABS-1"],
|
||||
"children": [],
|
||||
},
|
||||
"CHART-10": {
|
||||
"id": "CHART-10",
|
||||
"type": "CHART",
|
||||
"parents": ["ROOT_ID", "TABS-1", "TAB-1"],
|
||||
"meta": {"chartId": 10},
|
||||
},
|
||||
}
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool(
|
||||
"update_dashboard",
|
||||
{
|
||||
"request": {
|
||||
"identifier": 42,
|
||||
"dashboard_title": "Must not be applied",
|
||||
"css": ".must-not-be-applied { color: red; }",
|
||||
"position_json": unreachable_layout,
|
||||
}
|
||||
},
|
||||
)
|
||||
|
||||
payload = json.loads(result.content[0].text)
|
||||
assert payload["error_type"] == "InvalidDashboardLayout"
|
||||
assert "unreachable" in payload["error"]
|
||||
assert dash.position_json == original_position
|
||||
assert dash.dashboard_title == "Test Dashboard"
|
||||
assert dash.css is None
|
||||
mock_session.commit.assert_not_called()
|
||||
|
||||
@patch("superset.daos.dashboard.DashboardDAO.get_by_id_or_slug")
|
||||
@patch("superset.extensions.db.session")
|
||||
@pytest.mark.asyncio
|
||||
|
||||
@@ -419,13 +419,9 @@ def test_put_schema_allows_database_on_report_type(mocker: MockerFixture) -> Non
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
_PATCH_RETRY_FLAG = "superset.reports.schemas.is_feature_enabled"
|
||||
|
||||
|
||||
def test_retry_fields_defaults(mocker: MockerFixture) -> None:
|
||||
"""POST schema: retry fields have correct defaults when omitted."""
|
||||
mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
schema = ReportSchedulePostSchema()
|
||||
result = schema.load(MINIMAL_POST_PAYLOAD)
|
||||
assert result["retry_on_failure"] is False
|
||||
@@ -438,7 +434,6 @@ def test_retry_fields_defaults(mocker: MockerFixture) -> None:
|
||||
def test_retry_fields_accepted(mocker: MockerFixture) -> None:
|
||||
"""POST schema: retry fields are accepted with valid values."""
|
||||
mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
schema = ReportSchedulePostSchema()
|
||||
result = schema.load(
|
||||
{
|
||||
@@ -465,7 +460,6 @@ def test_retry_fields_accepted(mocker: MockerFixture) -> None:
|
||||
def test_retry_max_attempts_out_of_range(mocker: MockerFixture, value: int) -> None:
|
||||
"""POST schema: retry_max_attempts outside 1–10 is rejected."""
|
||||
mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
schema = ReportSchedulePostSchema()
|
||||
with pytest.raises(ValidationError) as exc:
|
||||
schema.load(
|
||||
@@ -486,7 +480,6 @@ def test_retry_max_attempts_out_of_range(mocker: MockerFixture, value: int) -> N
|
||||
def test_retry_max_attempts_boundary_values(mocker: MockerFixture, value: int) -> None:
|
||||
"""POST schema: retry_max_attempts at boundaries (1 and 10) is accepted."""
|
||||
mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
schema = ReportSchedulePostSchema()
|
||||
result = schema.load(
|
||||
{**MINIMAL_POST_PAYLOAD, "retry_on_failure": True, "retry_max_attempts": value}
|
||||
@@ -499,7 +492,6 @@ def test_send_failed_reports_requires_retry_on_failure(
|
||||
) -> None:
|
||||
"""POST schema: send_failed_reports=True with retry_on_failure=False is rejected."""
|
||||
mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
schema = ReportSchedulePostSchema()
|
||||
with pytest.raises(ValidationError) as exc:
|
||||
schema.load(
|
||||
@@ -515,7 +507,6 @@ def test_send_failed_reports_requires_retry_on_failure(
|
||||
def test_put_schema_accepts_retry_fields(mocker: MockerFixture) -> None:
|
||||
"""PUT schema: retry fields are accepted as optional partial updates."""
|
||||
mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
schema = ReportSchedulePutSchema()
|
||||
result = schema.load({"retry_on_failure": True, "retry_max_attempts": 7})
|
||||
assert result["retry_on_failure"] is True
|
||||
@@ -527,7 +518,6 @@ def test_put_schema_retry_max_attempts_out_of_range(
|
||||
) -> None:
|
||||
"""PUT schema: retry_max_attempts outside 1–10 is rejected."""
|
||||
mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG)
|
||||
mocker.patch(_PATCH_RETRY_FLAG, return_value=True)
|
||||
schema = ReportSchedulePutSchema()
|
||||
with pytest.raises(ValidationError) as exc:
|
||||
schema.load({"retry_max_attempts": 11})
|
||||
|
||||
@@ -186,69 +186,6 @@ def test_column_data_types_with_failing_conversion():
|
||||
assert not is_numeric_dtype(df["col3"])
|
||||
|
||||
|
||||
def test_apply_column_types_with_duplicate_column_labels() -> None:
|
||||
"""
|
||||
Test that duplicate column labels do not break the export.
|
||||
|
||||
The verbose_map rename in QueryContextProcessor.get_data can collapse two
|
||||
columns onto the same label, which used to raise
|
||||
"'DataFrame' object has no attribute 'dtype'".
|
||||
"""
|
||||
df = pd.DataFrame(
|
||||
[
|
||||
["1", datetime(2023, 1, 1, 0, 0, tzinfo=timezone.utc), "2"],
|
||||
["3", datetime(2023, 1, 2, 0, 0, tzinfo=timezone.utc), "4"],
|
||||
],
|
||||
columns=["dupe", "dupe", "other"],
|
||||
)
|
||||
coltypes: list[GenericDataType] = [
|
||||
GenericDataType.STRING,
|
||||
GenericDataType.TEMPORAL,
|
||||
GenericDataType.NUMERIC,
|
||||
]
|
||||
|
||||
apply_column_types(df, coltypes)
|
||||
|
||||
# each position is typed independently, despite sharing a label
|
||||
assert not is_numeric_dtype(df.iloc[:, 0])
|
||||
assert df.iloc[:, 1].tolist() == [
|
||||
"2023-01-01 00:00:00+00:00",
|
||||
"2023-01-02 00:00:00+00:00",
|
||||
]
|
||||
assert is_numeric_dtype(df.iloc[:, 2])
|
||||
|
||||
contents = df_to_excel(df, index=False)
|
||||
assert pd.read_excel(contents).shape == (2, 3)
|
||||
|
||||
|
||||
def test_quote_formulas_with_duplicate_column_labels() -> None:
|
||||
"""
|
||||
Test that formulas are quoted even when column labels are duplicated.
|
||||
"""
|
||||
df = pd.DataFrame(
|
||||
[["=SUM(A1:A2)", "@SUM(A1:A2)", "normal"]],
|
||||
columns=["dupe", "dupe", "other"],
|
||||
)
|
||||
|
||||
result = quote_formulas(df)
|
||||
|
||||
assert result.iloc[0].tolist() == ["'=SUM(A1:A2)", "'@SUM(A1:A2)", "normal"]
|
||||
|
||||
|
||||
def test_quote_formulas_with_dedicated_string_dtype() -> None:
|
||||
"""
|
||||
Test that formulas are quoted in columns using the dedicated string dtype.
|
||||
|
||||
pandas 3 gives string columns a ``str`` dtype rather than ``object``, so an
|
||||
object-only dtype check would skip them and leave formulas unquoted.
|
||||
"""
|
||||
df = pd.DataFrame({"formula": pd.array(["=SUM(A1:A2)", "normal"], dtype="string")})
|
||||
|
||||
result = quote_formulas(df)
|
||||
|
||||
assert result["formula"].tolist() == ["'=SUM(A1:A2)", "normal"]
|
||||
|
||||
|
||||
def test_column_data_types_with_large_numeric_values():
|
||||
df = pd.DataFrame(
|
||||
{
|
||||
|
||||
@@ -14,23 +14,9 @@
|
||||
# KIND, either express or implied. See the License for the
|
||||
# specific language governing permissions and limitations
|
||||
# under the License.
|
||||
import hashlib
|
||||
import re
|
||||
|
||||
import pytest
|
||||
|
||||
from superset.utils.file import get_filename, MAX_FILENAME_LENGTH, sanitize_title
|
||||
|
||||
_HASH_RE = re.compile(r"^[0-9a-f]{8}$")
|
||||
|
||||
|
||||
def _expected_skip_id_truncated(
|
||||
slug: str, max_length: int = MAX_FILENAME_LENGTH
|
||||
) -> str:
|
||||
"""Mirror get_filename's skip_id truncation + hash disambiguator."""
|
||||
hash_suffix = f"_{hashlib.sha256(slug.encode()).hexdigest()[:8]}"
|
||||
truncated = slug[: max_length - len(hash_suffix)].rstrip("._-")
|
||||
return f"{truncated}{hash_suffix}" if truncated else hash_suffix[1:]
|
||||
from superset.utils.file import get_filename, sanitize_title
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
@@ -60,178 +46,6 @@ def test_get_filename(
|
||||
assert expected_filename == original_filename
|
||||
|
||||
|
||||
@pytest.mark.parametrize("name_length", [50, 127, 128, 129, 200, 250, 500])
|
||||
@pytest.mark.parametrize("skip_id", [True, False])
|
||||
def test_get_filename_never_exceeds_max_length(name_length: int, skip_id: bool) -> None:
|
||||
"""Names of any length are capped so archives stay extractable on Windows."""
|
||||
assert len(get_filename("a" * name_length, 132, skip_id)) <= MAX_FILENAME_LENGTH
|
||||
|
||||
|
||||
@pytest.mark.parametrize("name_length", [200, 250, 500])
|
||||
def test_get_filename_truncates_long_names(name_length: int) -> None:
|
||||
"""The retained portion is the leading slice of the original name."""
|
||||
slug = "a" * name_length
|
||||
assert get_filename(slug, 132, skip_id=True) == _expected_skip_id_truncated(slug)
|
||||
assert get_filename(slug, 132) == (
|
||||
"a" * (MAX_FILENAME_LENGTH - len("_132")) + "_132"
|
||||
)
|
||||
|
||||
|
||||
def test_get_filename_leaves_short_names_untruncated() -> None:
|
||||
"""Names that already fit are passed through unchanged (no hash added)."""
|
||||
name = "a" * (MAX_FILENAME_LENGTH - len("_132"))
|
||||
assert get_filename(name, 132) == f"{name}_132"
|
||||
assert get_filename("a" * MAX_FILENAME_LENGTH, 132, skip_id=True) == (
|
||||
"a" * MAX_FILENAME_LENGTH
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("model_id", [1, 132, 999999, 2**31 - 1])
|
||||
def test_get_filename_preserves_id_suffix_when_truncating(model_id: int) -> None:
|
||||
"""The id suffix survives truncation, keeping export filenames unique."""
|
||||
filename = get_filename("a" * 500, model_id)
|
||||
|
||||
assert filename.endswith(f"_{model_id}")
|
||||
assert len(filename) == MAX_FILENAME_LENGTH
|
||||
assert filename == "a" * (MAX_FILENAME_LENGTH - len(f"_{model_id}")) + (
|
||||
f"_{model_id}"
|
||||
)
|
||||
|
||||
|
||||
def test_get_filename_skip_id_appends_hash_when_truncating() -> None:
|
||||
"""skip_id truncation keeps a short hash of the full slug as a disambiguator."""
|
||||
slug = "a" * 250
|
||||
filename = get_filename(slug, 132, skip_id=True)
|
||||
|
||||
assert len(filename) == MAX_FILENAME_LENGTH
|
||||
assert filename.startswith("a")
|
||||
hash_part = filename.rsplit("_", 1)[-1]
|
||||
assert _HASH_RE.match(hash_part)
|
||||
assert filename == _expected_skip_id_truncated(slug)
|
||||
|
||||
|
||||
def test_get_filename_skip_id_hash_disambiguates_shared_prefixes() -> None:
|
||||
"""Two long names that only differ past the cut must not collide in an archive."""
|
||||
prefix = "a" * 200
|
||||
left = get_filename(prefix + "left", 1, skip_id=True)
|
||||
right = get_filename(prefix + "right", 2, skip_id=True)
|
||||
|
||||
assert left != right
|
||||
assert len(left) <= MAX_FILENAME_LENGTH
|
||||
assert len(right) <= MAX_FILENAME_LENGTH
|
||||
# Readable prefixes match; only the content hash differs.
|
||||
assert left.rsplit("_", 1)[0] == right.rsplit("_", 1)[0]
|
||||
assert left.rsplit("_", 1)[1] != right.rsplit("_", 1)[1]
|
||||
|
||||
|
||||
def test_get_filename_truncation_is_deterministic() -> None:
|
||||
"""Datasets and their parent database must agree on the truncated folder name."""
|
||||
assert get_filename("b" * 250, 1, skip_id=True) == get_filename(
|
||||
"b" * 250, 2, skip_id=True
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("separators", ["_", "-", ".", "._-", "-_.", "__", "..."])
|
||||
def test_get_filename_strips_trailing_separators(separators: str) -> None:
|
||||
"""Truncating onto a delimiter must not leave a trailing dot, dash or underscore."""
|
||||
# Reserve room for the hash suffix that skip_id truncation appends.
|
||||
hash_suffix_len = 1 + 8 # "_" + 8 hex chars
|
||||
keep = MAX_FILENAME_LENGTH - hash_suffix_len - len(separators)
|
||||
name = "c" * keep + separators + "d" * 50
|
||||
filename = get_filename(name, 7, skip_id=True)
|
||||
|
||||
assert filename == _expected_skip_id_truncated(name)
|
||||
assert not filename.rsplit("_", 1)[0].endswith(tuple("._-"))
|
||||
|
||||
|
||||
@pytest.mark.parametrize("separators", ["_", "-", ".", "._-", "-_.", "__", "..."])
|
||||
def test_get_filename_strips_trailing_separators_before_id(separators: str) -> None:
|
||||
"""No `name-_.123` artifacts: delimiters are stripped before the id is appended."""
|
||||
keep = MAX_FILENAME_LENGTH - len("_132") - len(separators)
|
||||
name = "c" * keep + separators + "d" * 50
|
||||
|
||||
assert get_filename(name, 132) == "c" * keep + "_132"
|
||||
|
||||
|
||||
def test_get_filename_clamps_to_zero_for_oversized_id_suffix() -> None:
|
||||
"""A suffix longer than max_length must not negatively slice the slug."""
|
||||
model_id = 10**130 # 131 digits, longer than MAX_FILENAME_LENGTH
|
||||
|
||||
filename = get_filename("Energy Sankey", model_id)
|
||||
|
||||
# A negative slice would silently chop from the end and yield e.g. "Ener_10..0"
|
||||
assert filename == str(model_id)
|
||||
assert "Energy" not in filename
|
||||
|
||||
|
||||
@pytest.mark.parametrize("max_length", range(0, len("_132") + 1))
|
||||
def test_get_filename_clamps_to_zero_for_small_max_length(max_length: int) -> None:
|
||||
"""`max_length` at or below the suffix width falls back to the id, never a slice."""
|
||||
filename = get_filename("Energy Sankey", 132, max_length=max_length)
|
||||
|
||||
assert filename == "132"
|
||||
|
||||
|
||||
def test_get_filename_respects_custom_max_length() -> None:
|
||||
# Short enough that no truncation (hence no hash) is needed.
|
||||
assert get_filename("Energy", 132, skip_id=True, max_length=6) == "Energy"
|
||||
assert get_filename("Energy Sankey", 132, max_length=10) == "Energy_132"
|
||||
# One character wider than the suffix leaves room for exactly one slug character.
|
||||
assert get_filename("Energy Sankey", 132, max_length=len("_132") + 1) == "E_132"
|
||||
|
||||
|
||||
def test_get_filename_skip_id_falls_back_when_hash_cannot_fit() -> None:
|
||||
"""Degenerate max_length that cannot hold the hash suffix falls back to the id."""
|
||||
assert get_filename("Energy Sankey", 132, skip_id=True, max_length=6) == "132"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"model_name",
|
||||
[
|
||||
"",
|
||||
" ",
|
||||
"...",
|
||||
"///",
|
||||
"___",
|
||||
"..",
|
||||
"\x00\x01\x02",
|
||||
"🥴🥴🥴",
|
||||
"你好",
|
||||
"🥴" * 300,
|
||||
"你" * 300,
|
||||
],
|
||||
)
|
||||
@pytest.mark.parametrize("skip_id", [True, False])
|
||||
def test_get_filename_falls_back_to_id_for_unusable_names(
|
||||
model_name: str, skip_id: bool
|
||||
) -> None:
|
||||
"""Empty, special-character and non-ASCII names degrade to the bare id."""
|
||||
assert get_filename(model_name, 42, skip_id) == "42"
|
||||
|
||||
|
||||
def test_get_filename_keeps_hyphen_only_name() -> None:
|
||||
"""`secure_filename` keeps bare hyphens, unlike dots and underscores."""
|
||||
assert get_filename("---", 42, skip_id=True) == "---"
|
||||
assert get_filename("---", 42) == "---_42"
|
||||
|
||||
|
||||
def test_max_filename_length_fits_filesystem_component_limit() -> None:
|
||||
"""Most filesystems reject a single path component longer than 255 characters."""
|
||||
assert MAX_FILENAME_LENGTH <= 255
|
||||
|
||||
|
||||
def test_chart_export_path_fits_windows_max_path() -> None:
|
||||
"""A long chart name must still unzip on Windows, which caps paths at 260."""
|
||||
filename = get_filename("Quarterly Revenue Breakdown by Region " * 10, 132)
|
||||
|
||||
# Mirrors ExportChartsCommand._file_name plus the archive root written by
|
||||
# ChartRestApi.export and a typical extraction directory.
|
||||
entry = f"chart_export_20240101T000000/charts/{filename}.yaml"
|
||||
extracted = rf"C:\Users\username\Downloads\{entry}"
|
||||
|
||||
assert len(extracted) < 260
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("raw", "expected"),
|
||||
[
|
||||
|
||||
Reference in New Issue
Block a user