Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
998 changes: 444 additions & 554 deletions superset-frontend/package-lock.json

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const SupersetClient: SupersetClientInterface = {
request: request => getInstance().request(request),
getCSRFToken: () => getInstance().getCSRFToken(),
getUrl: (...args) => getInstance().getUrl(...args),
postBlob: (endpoint, payload) => getInstance().postBlob(endpoint, payload),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The wrapper uses two separate parameters (endpoint, payload) but callers likely pass a single request object; this will cause the instance postBlob to receive an unexpected shape (the whole request as endpoint and payload undefined), leading to runtime errors—forward the single request argument to the instance method. [logic error]

Severity Level: Critical 🚨
- ❌ Explore export (exportChart) fails producing no download.
- ❌ Dashboard chart/table export fails to download.
- ⚠️ Browser console shows uncaught export-related errors.
- ⚠️ Automated tests for exports may start failing.
Suggested change
postBlob: (endpoint, payload) => getInstance().postBlob(endpoint, payload),
postBlob: request => getInstance().postBlob(request),
Steps of Reproduction ✅
1. Ensure SupersetClient is configured and the singleton exists by calling
SupersetClient.configure(...). (See
superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts around
getInstance()/configure; the wrapper live at line 54.)

2. Trigger a UI chart export that the PR description changed to use postBlob — e.g., click
"Download CSV" in Explore which calls exportChart in exploreUtils (PR changes reference
exploreUtils/index.js:exportChart). That code calls SupersetClient.postBlob(request) with
a single request object (request contains endpoint, payload, filename metadata).

3. At runtime the wrapper in SupersetClient.ts line 54 executes, treating the single
object as the `endpoint` parameter and leaving `payload` undefined, then calls
getInstance().postBlob(endpoint, payload) with unexpected shapes.

4. If SupersetClientClass.postBlob expects a single request object, the instance method
receives wrong arguments and may throw, return an error, or produce malformed network
requests — observed as export failures or uncaught exceptions in the browser console and
failed downloads.

Note: the reproduction maps to the PR description which replaced form submissions with
SupersetClient.postBlob usage in exploreUtils and Dashboard chart export flows; the
wrapper at SupersetClient.ts:54 must forward the same single-argument shape used by those
callers.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/packages/superset-ui-core/src/connection/SupersetClient.ts
**Line:** 54:54
**Comment:**
	*Logic Error: The wrapper uses two separate parameters `(endpoint, payload)` but callers likely pass a single request object; this will cause the instance `postBlob` to receive an unexpected shape (the whole request as `endpoint` and `payload` undefined), leading to runtime errors—forward the single request argument to the instance method.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

};

export default SupersetClient;
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,25 @@ export default class SupersetClientClass {
}
}

/**
* POST request that returns a blob for file downloads.
* Unlike postForm, this uses AJAX so errors can be caught and handled.
* @param endpoint - API endpoint
* @param payload - Request payload
* @returns Promise resolving to Response with blob
*/
async postBlob(
endpoint: string,
payload: Record<string, any>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Replace Record<string, any> with a stricter payload type (such as an existing reusable request payload type or Record<string, unknown>) so the new API method does not introduce any. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The new postBlob method is TypeScript code and its payload parameter uses Record<string, any>, which directly introduces the any type. This matches the rule against new or modified TypeScript/TSX code using any, so the suggestion is valid.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
**Line:** 162:162
**Comment:**
	*Custom Rule: Replace `Record<string, any>` with a stricter payload type (such as an existing reusable request payload type or `Record<string, unknown>`) so the new API method does not introduce `any`.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

): Promise<Response> {
await this.ensureAuth();
return this.post({
endpoint,
postPayload: payload,
parseMethod: 'raw',
});
Comment on lines +165 to +169

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: postBlob forwards postPayload without overriding the default stringify: true, so string fields are JSON-stringified again by callApi. Export callers already pass form_data as a JSON string, which becomes double-encoded (extra quotes), and the backend then parses it as a string instead of an object, causing export requests to fail (typically 400) instead of downloading. Pass stringify: false (or send a prebuilt FormData) to preserve raw form field values. [api mismatch]

Severity Level: Critical 🚨
- ❌ Dashboard chart CSV export returns HTTP 400, no download.
- ❌ Explore view chart exports fail against /api/v1/chart/data.
- ⚠️ Users see error toasts instead of downloaded export files.
Steps of Reproduction ✅
1. Open a dashboard and trigger CSV export for a chart; tests at
`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx:8-31` show
that clicking "Export to .CSV" calls `exportChart(...)` from
`src/explore/exploreUtils/index.ts`.

2. In
`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:19-37,559-567`,
the `exportChart` function is invoked with `resultFormat: 'csv'` and `resultType: 'full'`,
and `onStartStreamingExport` left as `null`, so the non-streaming export path is used.

3. Inside `superset-frontend/src/explore/exploreUtils/index.ts:18-69`, `exportChart`
builds the payload and, for the v1 API (`url = '/api/v1/chart/data'`), calls
`SupersetClient.postBlob(url as string, { form_data: safeStringify(payload) })` at lines
41-47 and 65-67, where `safeStringify` (see
`plugins/preset-chart-deckgl/src/utils/safeStringify.ts:4-24`) returns a JSON string.

4. `SupersetClient.postBlob` in
`packages/superset-ui-core/src/connection/SupersetClientClass.ts:160-169` forwards this
payload via `this.post({ endpoint, postPayload: payload, parseMethod: 'raw' })` without
specifying `stringify`, so `request` flows into `callApiAndParseWithTimeout` and then
`callApi` in `packages/superset-ui-core/src/connection/callApi/callApi.ts:8-23,80-100`,
where `stringify` defaults to `true` and each form field value is transformed with
`valueString = stringify ? JSON.stringify(value) : String(value)`, causing `form_data`
(already a JSON string) to be JSON-stringified again and sent as `'"{...}"'` in the
multipart form body.

5. On the backend, `superset/charts/data/api.py:47-55` handles `/api/v1/chart/data` POST
requests by checking `request.form.get("form_data")` and running
`json.loads(request.form["form_data"])`; with the double-encoded payload this produces a
plain string instead of a dict, so `_create_query_context_from_form` (which expects a
dict, see `superset/tasks/async_queries.py:1-14`) raises a `ValidationError`, and
`ChartDataRestApi.data` catches this and returns `self.response_400(...)`, causing the
chart export HTTP 400 error and preventing the file from downloading.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
**Line:** 165:169
**Comment:**
	*Api Mismatch: `postBlob` forwards `postPayload` without overriding the default `stringify: true`, so string fields are JSON-stringified again by `callApi`. Export callers already pass `form_data` as a JSON string, which becomes double-encoded (extra quotes), and the backend then parses it as a string instead of an object, causing export requests to fail (typically 400) instead of downloading. Pass `stringify: false` (or send a prebuilt `FormData`) to preserve raw form field values.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

}
Comment on lines +160 to +170

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Unit Tests for postBlob

The new postBlob method lacks unit tests. Rule [11730] requires comprehensive unit tests for new tools covering success paths, error scenarios, and edge cases. Compare with postForm which has tests at lines 668-775 in the same test file.

Code Review Run #d807d9


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them


async reAuthenticate() {
return this.init(true);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ export interface SupersetClientInterface extends Pick<
| 'get'
| 'post'
| 'postForm'
| 'postBlob'
| 'put'
| 'request'
| 'init'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Adding 'postBlob' to the Pick list assumes SupersetClientClass already declares that method; if it doesn't, the Pick will not include it and consumers expecting postBlob will break. Make postBlob an optional member on the exported interface (typed to the correct request/response types) or ensure the class exposes the method. This preserves backward compatibility while providing proper types for the new API. [type error]

Severity Level: Critical 🚨
- ❌ TypeScript build failure blocking CI pipelines.
- ❌ Explore exportChart runtime TypeError on export.
- ❌ Dashboard chart export runtime errors.
- ⚠️ Consumers relying on SupersetClient typings mismatched.
Suggested change
| 'init'
| 'put'
| 'request'
| 'init'
| 'isAuthenticated'
| 'reAuthenticate'
| 'getGuestToken'
> {
// `postBlob` may be a new API implemented on SupersetClientClass.
// Declare it optional here to avoid breaking consumers if the class
// implementation isn't present at compile time, and provide a precise type.
postBlob?: (request: RequestConfig) => Promise<SupersetClientResponse>;
Steps of Reproduction ✅
1. Run TypeScript build for the frontend package: from repo root run `cd superset-frontend
&& npm run build` (this triggers tsc for packages). Type errors originate from
`superset-frontend/packages/superset-ui-core/src/connection/types.ts:152` where the Pick
includes `'postBlob'`. Expect TypeScript error like: "Type '"postBlob"' is not assignable
to parameter of type 'keyof SupersetClientClass'".

2. Inspect the imported class at
`superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass` (import
path from `types.ts` line 1: `import SupersetClientClass from './SupersetClientClass';`).
If that class file does not declare a `postBlob` member, the Pick in `types.ts` will fail
compilation as shown in step 1.

3. If TypeScript checking is skipped (e.g., during a Babel-only transpile or in a runtime
test environment), consumer code added in this PR calls `SupersetClient.postBlob(...)`
from Explore export functions (PR description references
`superset-frontend/src/explore/exploreUtils/index.js` exportChart usage). At runtime, if
`SupersetClientClass` instance lacks `postBlob`, this results in a runtime TypeError:
"SupersetClient.postBlob is not a function" when `exportChart` calls it.

4. Confirm by opening the PR-modified consumers:
`superset-frontend/src/explore/exploreUtils/index.js` (exportChart),
`superset-frontend/src/dashboard/Chart.jsx` (table export), and
`superset-frontend/src/hooks/useExploreAdditionalActionsMenu` (CSV/Excel/JSON exports). If
these files call `SupersetClient.postBlob` but `SupersetClientClass` does not declare it,
either TypeScript build fails (step 1) or runtime failures occur (step 3). If the addition
to the Pick was intentional, verify `SupersetClientClass` implements `postBlob` in
`superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts`.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/packages/superset-ui-core/src/connection/types.ts
**Line:** 158:158
**Comment:**
	*Type Error: Adding `'postBlob'` to the Pick list assumes `SupersetClientClass` already declares that method; if it doesn't, the Pick will not include it and consumers expecting `postBlob` will break. Make `postBlob` an optional member on the exported interface (typed to the correct request/response types) or ensure the class exposes the method. This preserves backward compatibility while providing proper types for the new API.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,13 @@ describe('SupersetClient', () => {
getUrl: (...args: unknown[]) => string;
};

test('exposes configure, init, get, post, postForm, delete, put, request, reset, getGuestToken, getCSRFToken, getUrl, isAuthenticated, and reAuthenticate methods', () => {
test('exposes configure, init, get, post, postForm, postBlob, delete, put, request, reset, getGuestToken, getCSRFToken, getUrl, isAuthenticated, and reAuthenticate methods', () => {
expect(typeof SupersetClient.configure).toBe('function');
expect(typeof SupersetClient.init).toBe('function');
expect(typeof SupersetClient.get).toBe('function');
expect(typeof SupersetClient.post).toBe('function');
expect(typeof SupersetClient.postForm).toBe('function');
expect(typeof SupersetClient.postBlob).toBe('function');
expect(typeof SupersetClient.delete).toBe('function');
expect(typeof SupersetClient.put).toBe('function');
expect(typeof SupersetClient.request).toBe('function');
Expand All @@ -53,11 +54,12 @@ describe('SupersetClient', () => {
expect(typeof SupersetClient.reAuthenticate).toBe('function');
});

test('throws if you call init, get, post, postForm, delete, put, request, getGuestToken, getCSRFToken, getUrl, isAuthenticated, or reAuthenticate before configure', () => {
test('throws if you call init, get, post, postForm, postBlob, delete, put, request, getGuestToken, getCSRFToken, getUrl, isAuthenticated, or reAuthenticate before configure', () => {
expect(SupersetClient.init).toThrow();
expect(SupersetClient.get).toThrow();
expect(SupersetClient.post).toThrow();
expect(SupersetClient.postForm).toThrow();
expect(SupersetClient.postBlob).toThrow();
expect(SupersetClient.delete).toThrow();
expect(SupersetClient.put).toThrow();
expect(SupersetClient.request).toThrow();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -780,4 +780,74 @@ describe('SupersetClientClass', () => {
expect(authSpy).toHaveBeenCalledTimes(0);
});
});

describe('.postBlob()', () => {
const protocol = 'https:';
const host = 'host';
const mockPostBlobEndpoint = '/api/v1/chart/data';
const mockPostBlobUrl = `${protocol}//${host}${mockPostBlobEndpoint}`;
const postBlobPayload = { form_data: '{"viz_type":"table"}' };

let authSpy: jest.SpyInstance;
let client: SupersetClientClass;

beforeEach(async () => {
fetchMock.removeRoute(LOGIN_GLOB);
fetchMock.get(LOGIN_GLOB, { result: 1234 }, { name: LOGIN_GLOB });

client = new SupersetClientClass({ protocol, host });
await client.init();
authSpy = jest.spyOn(SupersetClientClass.prototype, 'ensureAuth');
});

afterEach(() => {
jest.restoreAllMocks();
});

test('calls ensureAuth and delegates to post with raw parseMethod', async () => {
const mockResponse = new Response('csv data', { status: 200 });
const postSpy = jest.spyOn(client, 'post').mockResolvedValue(mockResponse);

const response = await client.postBlob(
mockPostBlobEndpoint,
postBlobPayload,
);

expect(authSpy).toHaveBeenCalledTimes(1);
expect(postSpy).toHaveBeenCalledWith({
endpoint: mockPostBlobEndpoint,
postPayload: postBlobPayload,
parseMethod: 'raw',
});
expect(response).toBe(mockResponse);
});

test('passes payload in request body', async () => {
fetchMock.post(mockPostBlobUrl, {
status: 200,
body: 'csv data',
});

await client.postBlob(mockPostBlobEndpoint, postBlobPayload);

const fetchRequest = fetchMock.callHistory.calls(mockPostBlobUrl)[0]
.options as CallApi;
const formData = fetchRequest.body as FormData;

expect(formData.get('form_data')).toBe(
JSON.stringify(postBlobPayload.form_data),
);
});

test('rejects when response is not ok', async () => {
fetchMock.post(mockPostBlobUrl, {
status: 413,
body: 'Payload Too Large',
});

await expect(
client.postBlob(mockPostBlobEndpoint, postBlobPayload),
).rejects.toMatchObject({ status: 413 });
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ const Chart = (props: ChartProps) => {
(formData as JsonObject).dashboardId = dashboardInfo.id;

const exportTable = useCallback(
(format: string, isFullCSV: boolean, isPivot = false) => {
async (format: string, isFullCSV: boolean, isPivot = false) => {
const logAction =
format === 'csv'
? LOG_ACTIONS_EXPORT_CSV_DASHBOARD_CHART
Expand Down Expand Up @@ -555,24 +555,48 @@ const Chart = (props: ChartProps) => {
}
: baseOwnState;

exportChart({
formData:
exportFormData as unknown as import('@superset-ui/core').QueryFormData,
resultType,
resultFormat: format,
force: true,
ownState: exportOwnState,
onStartStreamingExport: shouldUseStreaming
? (exportParams: JsonObject) => {
setIsStreamingModalVisible(true);
startExport({
...(exportParams as Record<string, unknown>),
filename,
expectedRows: actualRowCount,
} as Parameters<typeof startExport>[0]);
}
: null,
});
try {
await exportChart({
formData:
exportFormData as unknown as import('@superset-ui/core').QueryFormData,
resultType,
resultFormat: format,
force: true,
ownState: exportOwnState,
onStartStreamingExport: shouldUseStreaming
? (exportParams: JsonObject) => {
setIsStreamingModalVisible(true);
startExport({
...(exportParams as Record<string, unknown>),
filename,
expectedRows: actualRowCount,
} as Parameters<typeof startExport>[0]);
}
: null,
});
} catch (error) {
const exportError = error as Error & {
status?: number;
statusText?: string;
response?: { status?: number };
};
const status = exportError.status || exportError.response?.status;
if (status === 413) {
boundActionCreators.addDangerToast(
t(
'The chart data is too large to download. Please try reducing the date range, limiting rows, or using fewer columns.',
),
);
} else {
const errorMessage =
exportError.message ||
exportError.statusText ||
t(
'Failed to export chart data. Please try again or contact your administrator.',
);
boundActionCreators.addDangerToast(errorMessage);
}
}
},
[
sliceSliceId,
Expand All @@ -584,6 +608,7 @@ const Chart = (props: ChartProps) => {
chartState,
props.id,
boundActionCreators.logEvent,
boundActionCreators.addDangerToast,
queriesResponse,
startExport,
resetExport,
Expand Down
Loading