Skip to content

Commit

Permalink
fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TI…
Browse files Browse the repository at this point in the history 
…MEOUT (apache#28018)
  • Loading branch information
@eschutho @jzhao62
eschutho authored and jzhao62 committed May 16, 2024
1 parent 9346a6e commit 3e9ed3e
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 27 deletions.
1 change: 1 addition & 0 deletions superset-frontend/src/SqlLab/fixtures.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,7 @@ export const initialState = {
DISPLAY_MAX_ROW: 100,
SQLALCHEMY_DOCS_URL: 'test_SQLALCHEMY_DOCS_URL',
SQLALCHEMY_DISPLAY_TEXT: 'test_SQLALCHEMY_DISPLAY_TEXT',
SUPERSET_WEBSERVER_TIMEOUT: '300',
},
},
};
Expand Down
21 changes: 13 additions & 8 deletions superset-frontend/src/components/Chart/chartAction.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,17 +248,20 @@ export async function getChartDataRequest({

export function runAnnotationQuery({
annotation,
timeout = 60,
timeout,
formData = null,
key,
isDashboardRequest = false,
force = false,
}) {
return function (dispatch, getState) {
const sliceKey = key || Object.keys(getState().charts)[0];
const { charts, common } = getState();
const sliceKey = key || Object.keys(charts)[0];
const queryTimeout = timeout || common.conf.SUPERSET_WEBSERVER_TIMEOUT;

// make a copy of formData, not modifying original formData
const fd = {
...(formData || getState().charts[sliceKey].latestQueryFormData),
...(formData || charts[sliceKey].latestQueryFormData),
};

if (!annotation.sourceType) {
Expand Down Expand Up @@ -309,7 +312,7 @@ export function runAnnotationQuery({
return SupersetClient.post({
url,
signal,
timeout: timeout * 1000,
timeout: queryTimeout * 1000,
headers: { 'Content-Type': 'application/json' },
jsonPayload: buildV1ChartDataPayload({
formData: fd,
Expand Down Expand Up @@ -396,18 +399,20 @@ export function handleChartDataResponse(response, json, useLegacyApi) {
export function exploreJSON(
formData,
force = false,
timeout = 60,
timeout,
key,
dashboardId,
ownState,
) {
return async dispatch => {
return async (dispatch, getState) => {
const logStart = Logger.getTimestamp();
const controller = new AbortController();
const queryTimeout =
timeout || getState().common.conf.SUPERSET_WEBSERVER_TIMEOUT;

const requestParams = {
signal: controller.signal,
timeout: timeout * 1000,
timeout: queryTimeout * 1000,
};
if (dashboardId) requestParams.dashboard_id = dashboardId;

Expand Down Expand Up @@ -519,7 +524,7 @@ export const POST_CHART_FORM_DATA = 'POST_CHART_FORM_DATA';
export function postChartFormData(
formData,
force = false,
timeout = 60,
timeout,
key,
dashboardId,
ownState,
Expand Down
117 changes: 98 additions & 19 deletions superset-frontend/src/components/Chart/chartActions.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,27 @@ import * as actions from 'src/components/Chart/chartAction';
import * as asyncEvent from 'src/middleware/asyncEvent';
import { handleChartDataResponse } from 'src/components/Chart/chartAction';

import configureMockStore from 'redux-mock-store';
import thunk from 'redux-thunk';
import { initialState } from 'src/SqlLab/fixtures';

const middlewares = [thunk];
const mockStore = configureMockStore(middlewares);

const mockGetState = () => ({
charts: {
chartKey: {
latestQueryFormData: {
time_grain_sqla: 'P1D',
granularity_sqla: 'Date',
},
},
},
common: {
conf: {},
},
});

describe('chart actions', () => {
const MOCK_URL = '/mockURL';
let dispatch;
Expand Down Expand Up @@ -94,7 +115,7 @@ describe('chart actions', () => {

it('should query with the built query', async () => {
const actionThunk = actions.postChartFormData({}, null);
await actionThunk(dispatch);
await actionThunk(dispatch, mockGetState);

expect(fetchMock.calls(MOCK_URL)).toHaveLength(1);
expect(fetchMock.calls(MOCK_URL)[0][1].body).toBe(
Expand Down Expand Up @@ -165,7 +186,7 @@ describe('chart actions', () => {
it('should dispatch CHART_UPDATE_STARTED action before the query', () => {
const actionThunk = actions.postChartFormData({});

return actionThunk(dispatch).then(() => {
return actionThunk(dispatch, mockGetState).then(() => {
// chart update, trigger query, update form data, success
expect(dispatch.callCount).toBe(5);
expect(fetchMock.calls(MOCK_URL)).toHaveLength(1);
Expand All @@ -175,7 +196,7 @@ describe('chart actions', () => {

it('should dispatch TRIGGER_QUERY action with the query', () => {
const actionThunk = actions.postChartFormData({});
return actionThunk(dispatch).then(() => {
return actionThunk(dispatch, mockGetState).then(() => {
// chart update, trigger query, update form data, success
expect(dispatch.callCount).toBe(5);
expect(fetchMock.calls(MOCK_URL)).toHaveLength(1);
Expand All @@ -185,7 +206,7 @@ describe('chart actions', () => {

it('should dispatch UPDATE_QUERY_FORM_DATA action with the query', () => {
const actionThunk = actions.postChartFormData({});
return actionThunk(dispatch).then(() => {
return actionThunk(dispatch, mockGetState).then(() => {
// chart update, trigger query, update form data, success
expect(dispatch.callCount).toBe(5);
expect(fetchMock.calls(MOCK_URL)).toHaveLength(1);
Expand All @@ -195,7 +216,7 @@ describe('chart actions', () => {

it('should dispatch logEvent async action', () => {
const actionThunk = actions.postChartFormData({});
return actionThunk(dispatch).then(() => {
return actionThunk(dispatch, mockGetState).then(() => {
// chart update, trigger query, update form data, success
expect(dispatch.callCount).toBe(5);
expect(fetchMock.calls(MOCK_URL)).toHaveLength(1);
Expand All @@ -209,7 +230,7 @@ describe('chart actions', () => {

it('should dispatch CHART_UPDATE_SUCCEEDED action upon success', () => {
const actionThunk = actions.postChartFormData({});
return actionThunk(dispatch).then(() => {
return actionThunk(dispatch, mockGetState).then(() => {
// chart update, trigger query, update form data, success
expect(dispatch.callCount).toBe(5);
expect(fetchMock.calls(MOCK_URL)).toHaveLength(1);
Expand All @@ -226,7 +247,7 @@ describe('chart actions', () => {
const timeoutInSec = 1 / 1000;
const actionThunk = actions.postChartFormData({}, false, timeoutInSec);

return actionThunk(dispatch).then(() => {
return actionThunk(dispatch, mockGetState).then(() => {
// chart update, trigger query, update form data, fail
expect(fetchMock.calls(MOCK_URL)).toHaveLength(1);
expect(dispatch.callCount).toBe(5);
Expand All @@ -245,7 +266,7 @@ describe('chart actions', () => {
const timeoutInSec = 100; // Set to a time that is longer than the time this will take to fail
const actionThunk = actions.postChartFormData({}, false, timeoutInSec);

return actionThunk(dispatch).then(() => {
return actionThunk(dispatch, mockGetState).then(() => {
// chart update, trigger query, update form data, fail
expect(dispatch.callCount).toBe(5);
const updateFailedAction = dispatch.args[4][0];
Expand Down Expand Up @@ -278,17 +299,6 @@ describe('chart actions', () => {

describe('runAnnotationQuery', () => {
const mockDispatch = jest.fn();
const mockGetState = () => ({
charts: {
chartKey: {
latestQueryFormData: {
time_grain_sqla: 'P1D',
granularity_sqla: 'Date',
},
},
},
});

beforeEach(() => {
jest.clearAllMocks();
});
Expand Down Expand Up @@ -342,3 +352,72 @@ describe('chart actions', () => {
});
});
});

describe('chart actions timeout', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('should use the timeout from arguments when given', () => {
const postSpy = jest.spyOn(SupersetClient, 'post');
postSpy.mockImplementation(() => Promise.resolve({ json: { result: [] } }));
const timeout = 10; // Set the timeout value here
const formData = { datasource: 'table__1' }; // Set the formData here
const key = 'chartKey'; // Set the chart key here

const store = mockStore(initialState);
store.dispatch(
actions.runAnnotationQuery({
annotation: {
value: 'annotationValue',
sourceType: 'Event',
overrides: {},
},
timeout,
formData,
key,
}),
);

const expectedPayload = {
url: expect.any(String),
signal: expect.any(AbortSignal),
timeout: timeout * 1000,
headers: { 'Content-Type': 'application/json' },
jsonPayload: expect.any(Object),
};

expect(postSpy).toHaveBeenCalledWith(expectedPayload);
});

it('should use the timeout from common.conf when not passed as an argument', () => {
const postSpy = jest.spyOn(SupersetClient, 'post');
postSpy.mockImplementation(() => Promise.resolve({ json: { result: [] } }));
const formData = { datasource: 'table__1' }; // Set the formData here
const key = 'chartKey'; // Set the chart key here

const store = mockStore(initialState);
store.dispatch(
actions.runAnnotationQuery({
annotation: {
value: 'annotationValue',
sourceType: 'Event',
overrides: {},
},
undefined,
formData,
key,
}),
);

const expectedPayload = {
url: expect.any(String),
signal: expect.any(AbortSignal),
timeout: initialState.common.conf.SUPERSET_WEBSERVER_TIMEOUT * 1000,
headers: { 'Content-Type': 'application/json' },
jsonPayload: expect.any(Object),
};

expect(postSpy).toHaveBeenCalledWith(expectedPayload);
});
});

0 comments on commit 3e9ed3e

Please sign in to comment.