Skip to content

Commit

Permalink
fix(sqllab): race condition when updating cursor position (apache#30154)
Browse files Browse the repository at this point in the history
  • Loading branch information
justinpark authored Sep 4, 2024
1 parent acea58e commit 2097b71
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,29 @@
*/
import configureStore from 'redux-mock-store';
import thunk from 'redux-thunk';
import { render, waitFor } from 'spec/helpers/testing-library';
import reducerIndex from 'spec/helpers/reducerIndex';
import { render, waitFor, createStore } from 'spec/helpers/testing-library';
import { QueryEditor } from 'src/SqlLab/types';
import { Store } from 'redux';
import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures';
import AceEditorWrapper from 'src/SqlLab/components/AceEditorWrapper';
import { AsyncAceEditorProps } from 'src/components/AsyncAceEditor';
import {
AsyncAceEditorProps,
FullSQLEditor,
} from 'src/components/AsyncAceEditor';
import {
queryEditorSetCursorPosition,
queryEditorSetDb,
} from 'src/SqlLab/actions/sqlLab';
import fetchMock from 'fetch-mock';

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

fetchMock.get('glob:*/api/v1/database/*/function_names/', {
function_names: [],
});

jest.mock('src/components/Select/Select', () => () => (
<div data-test="mock-deprecated-select-select" />
));
Expand All @@ -36,9 +49,11 @@ jest.mock('src/components/Select/AsyncSelect', () => () => (
));

jest.mock('src/components/AsyncAceEditor', () => ({
FullSQLEditor: (props: AsyncAceEditorProps) => (
<div data-test="react-ace">{JSON.stringify(props)}</div>
),
FullSQLEditor: jest
.fn()
.mockImplementation((props: AsyncAceEditorProps) => (
<div data-test="react-ace">{JSON.stringify(props)}</div>
)),
}));

const setup = (queryEditor: QueryEditor, store?: Store) =>
Expand All @@ -59,6 +74,10 @@ const setup = (queryEditor: QueryEditor, store?: Store) =>
);

describe('AceEditorWrapper', () => {
beforeEach(() => {
(FullSQLEditor as any as jest.Mock).mockClear();
});

it('renders ace editor including sql value', async () => {
const { getByTestId } = setup(defaultQueryEditor, mockStore(initialState));
await waitFor(() => expect(getByTestId('react-ace')).toBeInTheDocument());
Expand Down Expand Up @@ -91,4 +110,19 @@ describe('AceEditorWrapper', () => {
JSON.stringify({ value: defaultQueryEditor.sql }).slice(1, -1),
);
});

it('skips rerendering for updating cursor position', () => {
const store = createStore(initialState, reducerIndex);
setup(defaultQueryEditor, store);

expect(FullSQLEditor).toHaveBeenCalled();
const renderCount = (FullSQLEditor as any as jest.Mock).mock.calls.length;
const updatedCursorPosition = { row: 1, column: 9 };
store.dispatch(
queryEditorSetCursorPosition(defaultQueryEditor, updatedCursorPosition),
);
expect(FullSQLEditor).toHaveBeenCalledTimes(renderCount);
store.dispatch(queryEditorSetDb(defaultQueryEditor, 1));
expect(FullSQLEditor).toHaveBeenCalledTimes(renderCount + 1);
});
});
24 changes: 14 additions & 10 deletions superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/
import { useState, useEffect, useRef } from 'react';
import type { IAceEditor } from 'react-ace/lib/types';
import { useDispatch } from 'react-redux';
import { shallowEqual, useDispatch, useSelector } from 'react-redux';
import { css, styled, usePrevious, useTheme } from '@superset-ui/core';
import { Global } from '@emotion/react';

Expand All @@ -27,7 +27,7 @@ import { queryEditorSetSelectedText } from 'src/SqlLab/actions/sqlLab';
import { FullSQLEditor as AceEditor } from 'src/components/AsyncAceEditor';
import type { KeyboardShortcut } from 'src/SqlLab/components/KeyboardShortcutButton';
import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
import type { CursorPosition } from 'src/SqlLab/types';
import { SqlLabRootState, type CursorPosition } from 'src/SqlLab/types';
import { useAnnotations } from './useAnnotations';
import { useKeywords } from './useKeywords';

Expand Down Expand Up @@ -77,11 +77,20 @@ const AceEditorWrapper = ({
'catalog',
'schema',
'templateParams',
'cursorPosition',
]);
// Prevent a maximum update depth exceeded error
// by skipping access the unsaved query editor state
const cursorPosition = useSelector<SqlLabRootState, CursorPosition>(
({ sqlLab: { queryEditors } }) => {
const { cursorPosition } = {
...queryEditors.find(({ id }) => id === queryEditorId),
};
return cursorPosition ?? { row: 0, column: 0 };
},
shallowEqual,
);

const currentSql = queryEditor.sql ?? '';
const cursorPosition = queryEditor.cursorPosition ?? { row: 0, column: 0 };
const [sql, setSql] = useState(currentSql);

// The editor changeSelection is called multiple times in a row,
Expand Down Expand Up @@ -145,12 +154,7 @@ const AceEditorWrapper = ({

editor.selection.on('changeCursor', () => {
const cursor = editor.getCursorPosition();
const { row, column } = cursorPosition;
// Prevent a maximum update depth exceeded error
// by skipping repeated updates to the same cursor position
if (cursor.row !== row && cursor.column !== column) {
onCursorPositionChange(cursor);
}
onCursorPositionChange(cursor);
});

const { row, column } = cursorPosition;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,12 @@ import { VALIDATION_DEBOUNCE_MS } from 'src/SqlLab/constants';
import {
FetchValidationQueryParams,
useQueryValidationsQuery,
ValidationResult,
} from 'src/hooks/apiResources';
import { useDebounceValue } from 'src/hooks/useDebounceValue';

const EMPTY = [] as ValidationResult[];

export function useAnnotations(params: FetchValidationQueryParams) {
const { sql, dbId, schema, templateParams } = params;
const debouncedSql = useDebounceValue(sql, VALIDATION_DEBOUNCE_MS);
Expand Down Expand Up @@ -73,7 +76,7 @@ export function useAnnotations(params: FetchValidationQueryParams) {
text: `The server failed to validate your query.\n${message}`,
},
]
: [],
: EMPTY,
};
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export type FetchValidationQueryParams = {
templateParams?: string;
};

type ValidationResult = {
export type ValidationResult = {
end_column: number | null;
line_number: number | null;
message: string | null;
Expand Down

0 comments on commit 2097b71

Please sign in to comment.