Skip to content
Merged
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
5 changes: 3 additions & 2 deletions superset-frontend/src/SqlLab/actions/sqlLab.js
Original file line number Diff line number Diff line change
Expand Up @@ -1017,12 +1017,13 @@ export function runTablePreviewQuery(newTable, runPreviewOnly) {
};
}

export function syncTable(table, tableMetadata) {
export function syncTable(table, tableMetadata, finalQueryEditorId) {
return function (dispatch) {
const finalTable = { ...table, queryEditorId: finalQueryEditorId };
const sync = isFeatureEnabled(FeatureFlag.SqllabBackendPersistence)
? SupersetClient.post({
endpoint: encodeURI('/tableschemaview/'),
postPayload: { table: { ...tableMetadata, ...table } },
postPayload: { table: { ...tableMetadata, ...finalTable } },
})
: Promise.resolve({ json: { id: table.id } });

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
import TableElement, { Column } from 'src/SqlLab/components/TableElement';
import { table, initialState } from 'src/SqlLab/fixtures';
import { render, waitFor, fireEvent } from 'spec/helpers/testing-library';
import * as sqlLabActions from 'src/SqlLab/actions/sqlLab';
import { QueryEditor } from 'src/SqlLab/types';

jest.mock('@superset-ui/core', () => ({
...jest.requireActual('@superset-ui/core'),
Expand Down Expand Up @@ -79,6 +81,27 @@ const mockedProps = {
activeKey: [table.id],
};

const createStateWithQueryEditor = (queryEditor: Partial<QueryEditor>) => ({
...initialState,
sqlLab: {
...initialState.sqlLab,
queryEditors: [queryEditor],
},
});

const setupSyncTableTest = () => {
const spy = jest.spyOn(sqlLabActions, 'syncTable');
mockedIsFeatureEnabled.mockImplementation(
featureFlag => featureFlag === FeatureFlag.SqllabBackendPersistence,
);
fetchMock.post(
updateTableSchemaEndpoint,
{ id: 100 },
{ overwriteRoutes: true },
);
return spy;
};

test('renders', () => {
expect(isValidElement(<TableElement table={table} />)).toBe(true);
});
Expand Down Expand Up @@ -207,3 +230,212 @@ test('refreshes table metadata when triggered', async () => {
expect(fetchMock.calls(updateTableSchemaEndpoint)).toHaveLength(1),
);
});

test('calls syncTable with valid backend ID when query editor has tabViewId', async () => {
const syncTableSpy = setupSyncTableTest();
const testTable = {
...table,
initialized: false,
queryEditorId: 'temp-id-123',
};

const state = createStateWithQueryEditor({
id: 'temp-id-123',
tabViewId: '42',
inLocalStorage: false,
name: 'Test Editor',
});

render(<TableElement table={testTable} activeKey={[testTable.id]} />, {
useRedux: true,
initialState: state,
});

await waitFor(() => {
expect(syncTableSpy).toHaveBeenCalledWith(
expect.any(Object),
expect.any(Object),
'42', // finalQueryEditorId
);
});

syncTableSpy.mockRestore();
});

test('does not call syncTable when query editor is in localStorage', async () => {
const syncTableSpy = setupSyncTableTest();
const testTable = {
...table,
initialized: false,
queryEditorId: 'local-id',
};

const state = createStateWithQueryEditor({
id: 'local-id',
tabViewId: undefined,
inLocalStorage: true,
name: 'Local Editor',
});

render(<TableElement table={testTable} activeKey={[testTable.id]} />, {
useRedux: true,
initialState: state,
});

await waitFor(() => {
expect(fetchMock.calls(getTableMetadataEndpoint)).toHaveLength(1);
});

await new Promise(resolve => setTimeout(resolve, 100));
expect(syncTableSpy).not.toHaveBeenCalled();

syncTableSpy.mockRestore();
});

test('does not call syncTable with non-numeric queryEditorId', async () => {
const syncTableSpy = setupSyncTableTest();
const testTable = {
...table,
initialized: false,
queryEditorId: 'not-a-number',
};

const state = createStateWithQueryEditor({
id: 'not-a-number',
tabViewId: 'also-not-a-number',
inLocalStorage: false,
name: 'Invalid Editor',
});

render(<TableElement table={testTable} activeKey={[testTable.id]} />, {
useRedux: true,
initialState: state,
});

await waitFor(() => {
expect(fetchMock.calls(getTableMetadataEndpoint)).toHaveLength(1);
});

await new Promise(resolve => setTimeout(resolve, 100));
expect(syncTableSpy).not.toHaveBeenCalled();

syncTableSpy.mockRestore();
});

test('does not call syncTable for already initialized tables', async () => {
const syncTableSpy = setupSyncTableTest();
const testTable = {
...table,
initialized: true, // Already initialized
queryEditorId: '789',
};

const state = createStateWithQueryEditor({
id: '789',
tabViewId: '789',
inLocalStorage: false,
name: 'Initialized Editor',
});

render(<TableElement table={testTable} activeKey={[testTable.id]} />, {
useRedux: true,
initialState: state,
});

await waitFor(() => {
expect(fetchMock.calls(getTableMetadataEndpoint)).toHaveLength(1);
});

await new Promise(resolve => setTimeout(resolve, 100));
expect(syncTableSpy).not.toHaveBeenCalled();

syncTableSpy.mockRestore();
});

test('calls syncTable after query editor is migrated from localStorage', async () => {
const syncTableSpy = setupSyncTableTest();
const testTable = {
...table,
initialized: false,
queryEditorId: 'temp-editor-id',
};

// Start with editor in localStorage
const localState = createStateWithQueryEditor({
id: 'temp-editor-id',
tabViewId: undefined,
inLocalStorage: true,
name: 'Temp Editor',
});

const { rerender } = render(
<TableElement table={testTable} activeKey={[testTable.id]} />,
{
useRedux: true,
initialState: localState,
},
);

await new Promise(resolve => setTimeout(resolve, 100));
expect(syncTableSpy).not.toHaveBeenCalled();

const migratedState = createStateWithQueryEditor({
id: 'temp-editor-id',
tabViewId: '999',
inLocalStorage: false,
name: 'Temp Editor',
});

rerender(<TableElement table={testTable} activeKey={[testTable.id]} />);

const { unmount } = render(
<TableElement table={testTable} activeKey={[testTable.id]} />,
{
useRedux: true,
initialState: migratedState,
},
);

await waitFor(() => {
expect(syncTableSpy).toHaveBeenCalledWith(
expect.any(Object),
expect.any(Object),
'999',
);
});

unmount();
syncTableSpy.mockRestore();
});

test('passes numeric queryEditorId validation', async () => {
const syncTableSpy = setupSyncTableTest();
const testTable = {
...table,
initialized: false,
queryEditorId: 'editor-123',
};

const state = createStateWithQueryEditor({
id: 'editor-123',
tabViewId: '456',
inLocalStorage: false,
name: 'Valid Editor',
});

render(<TableElement table={testTable} activeKey={[testTable.id]} />, {
useRedux: true,
initialState: state,
});

await waitFor(() => {
expect(syncTableSpy).toHaveBeenCalled();
const [, , finalQueryEditorId] = syncTableSpy.mock.calls[0];
// Verify it's a valid numeric string
expect(Number.isNaN(Number(finalQueryEditorId))).toBe(false);
expect(typeof finalQueryEditorId).toBe('string');
expect(finalQueryEditorId).toMatch(/^\d+$/);
});

syncTableSpy.mockRestore();
});
38 changes: 28 additions & 10 deletions superset-frontend/src/SqlLab/components/TableElement/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
* under the License.
*/
import { useState, useRef, useEffect } from 'react';
import { useDispatch } from 'react-redux';
import type { Table } from 'src/SqlLab/types';
import { useDispatch, useSelector } from 'react-redux';
import type { QueryEditor, SqlLabRootState, Table } from 'src/SqlLab/types';
import {
ButtonGroup,
Card,
Expand Down Expand Up @@ -103,6 +103,19 @@ const TableElement = ({ table, ...props }: TableElementProps) => {
},
{ skip: !expanded },
);
const tableData = {
...tableMetadata,
...tableExtendedMetadata,
};
const queryEditors = useSelector<SqlLabRootState, QueryEditor[]>(
state => state.sqlLab.queryEditors,
);
Comment on lines +110 to +112

This comment was marked as resolved.

const currentTable = { ...tableData, ...table };
const { queryEditorId } = currentTable;
const queryEditor = queryEditors.find(
qe => qe.id === queryEditorId || qe.tabViewId === queryEditorId,
);
const currentQueryEditorId = queryEditor?.tabViewId || queryEditorId;

useEffect(() => {
if (hasMetadataError || hasExtendedMetadataError) {
Expand All @@ -112,24 +125,29 @@ const TableElement = ({ table, ...props }: TableElementProps) => {
}
}, [hasMetadataError, hasExtendedMetadataError, dispatch]);

const tableData = {
...tableMetadata,
...tableExtendedMetadata,
};

// TODO: migrate syncTable logic by SIP-93
const syncTableMetadata = useEffectEvent(() => {
const { initialized } = table;
if (!initialized) {
dispatch(syncTable(table, tableData));
// if not a valid number, wait for backend to assign one
const hasFinalQueryEditorId =
currentQueryEditorId &&
!Number.isNaN(Number(currentQueryEditorId)) &&
currentTable.queryEditorId !== currentQueryEditorId;
if (!initialized && hasFinalQueryEditorId) {
dispatch(syncTable(currentTable, tableData, currentQueryEditorId));
}
});

useEffect(() => {
if (isMetadataSuccess && isExtraMetadataSuccess) {
syncTableMetadata();
}
}, [isMetadataSuccess, isExtraMetadataSuccess, syncTableMetadata]);
}, [
isMetadataSuccess,
isExtraMetadataSuccess,
currentQueryEditorId,
syncTableMetadata,
]);

const [sortColumns, setSortColumns] = useState(false);
const [hovered, setHovered] = useState(false);
Expand Down
Loading