This is an automated email from the ASF dual-hosted git repository. diegopucci pushed a commit to branch geido/fix/sqllab-tabstateview in repository https://gitbox.apache.org/repos/asf/superset.git
commit 161a1c9c1dbae01b8dcc8ddb45e76ccda226a617 Author: Diego Pucci <[email protected]> AuthorDate: Tue Sep 30 17:01:28 2025 +0300 fix(SqlLab): Hit tableschemaview with a valid queryEditorId --- superset-frontend/src/SqlLab/actions/sqlLab.js | 5 +- .../components/TableElement/TableElement.test.tsx | 232 +++++++++++++++++++++ .../src/SqlLab/components/TableElement/index.tsx | 38 +++- 3 files changed, 263 insertions(+), 12 deletions(-) diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index 2c6a6e47b5..d7bf0bffbf 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -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 } }); diff --git a/superset-frontend/src/SqlLab/components/TableElement/TableElement.test.tsx b/superset-frontend/src/SqlLab/components/TableElement/TableElement.test.tsx index 96f68b6666..c50da8d497 100644 --- a/superset-frontend/src/SqlLab/components/TableElement/TableElement.test.tsx +++ b/superset-frontend/src/SqlLab/components/TableElement/TableElement.test.tsx @@ -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'), @@ -79,6 +81,27 @@ const mockedProps = { activeKey: [table.id], }; +const createStateWithQueryEditor = (queryEditor: 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); }); @@ -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(); +}); diff --git a/superset-frontend/src/SqlLab/components/TableElement/index.tsx b/superset-frontend/src/SqlLab/components/TableElement/index.tsx index 1bef2037d9..309c74d8b9 100644 --- a/superset-frontend/src/SqlLab/components/TableElement/index.tsx +++ b/superset-frontend/src/SqlLab/components/TableElement/index.tsx @@ -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, @@ -103,6 +103,19 @@ const TableElement = ({ table, ...props }: TableElementProps) => { }, { skip: !expanded }, ); + const tableData = { + ...tableMetadata, + ...tableExtendedMetadata, + }; + const queryEditors = useSelector<SqlLabRootState, QueryEditor[]>( + state => state.sqlLab.queryEditors, + ); + 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) { @@ -112,16 +125,16 @@ 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)); } }); @@ -129,7 +142,12 @@ const TableElement = ({ table, ...props }: TableElementProps) => { if (isMetadataSuccess && isExtraMetadataSuccess) { syncTableMetadata(); } - }, [isMetadataSuccess, isExtraMetadataSuccess, syncTableMetadata]); + }, [ + isMetadataSuccess, + isExtraMetadataSuccess, + currentQueryEditorId, + syncTableMetadata, + ]); const [sortColumns, setSortColumns] = useState(false); const [hovered, setHovered] = useState(false);
