You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2025/09/17 05:10:00 UTC
(superset) branch fix-sql-lab-metadata created (now b253006ecb)
This is an automated email from the ASF dual-hosted git repository.
jli pushed a change to branch fix-sql-lab-metadata
in repository https://gitbox.apache.org/repos/asf/superset.git
at b253006ecb test: Add comprehensive SQL Lab tab state validation tests
This branch includes the following new commits:
new b253006ecb test: Add comprehensive SQL Lab tab state validation tests
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: test: Add comprehensive SQL Lab tab state validation tests
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jli pushed a commit to branch fix-sql-lab-metadata
in repository https://gitbox.apache.org/repos/asf/superset.git
commit b253006ecbaf56ad4b6284990dbb9827c04fc386
Author: Joe Li <jo...@preset.io>
AuthorDate: Tue Sep 16 22:09:18 2025 -0700
test: Add comprehensive SQL Lab tab state validation tests
Add test suite to validate SQL Lab tab state management and backend
persistence behavior, particularly around the interaction between
frontend-generated string IDs and backend integer ID expectations.
Related to issue #34997: These tests reproduce metadata update
failures when users modify queries in newly created tabs.
Test files:
- sqlLab.tabStateValidation.test.js: 8 tests for tab state edge cases
- sqlLab.immediateBackendPersistence.test.js: 6 tests for persistence
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <no...@anthropic.com>
---
.../sqlLab.immediateBackendPersistence.test.js | 254 +++++++++++++++++
.../actions/sqlLab.tabStateValidation.test.js | 305 +++++++++++++++++++++
2 files changed, 559 insertions(+)
diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.immediateBackendPersistence.test.js b/superset-frontend/src/SqlLab/actions/sqlLab.immediateBackendPersistence.test.js
new file mode 100644
index 0000000000..01a41438bb
--- /dev/null
+++ b/superset-frontend/src/SqlLab/actions/sqlLab.immediateBackendPersistence.test.js
@@ -0,0 +1,254 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Tests for SQL Lab immediate backend persistence functionality
+ *
+ * This file tests the addQueryEditorWithBackendSync function that
+ * immediately creates backend tabs when SqllabBackendPersistence is enabled,
+ * ensuring consistent state management and preventing race conditions
+ * between frontend tab creation and backend synchronization.
+ *
+ * Related to issue #34997: Prevents metadata update failures by ensuring
+ * tabs have valid backend IDs before users can modify them.
+ */
+
+import configureMockStore from 'redux-mock-store';
+import thunk from 'redux-thunk';
+import fetchMock from 'fetch-mock';
+
+import { isFeatureEnabled } from '@superset-ui/core';
+import * as actions from './sqlLab';
+
+// Mock feature flags
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ isFeatureEnabled: jest.fn(),
+}));
+
+const mockStore = configureMockStore([thunk]);
+
+describe('SQL Lab immediate backend persistence', () => {
+ beforeEach(() => {
+ fetchMock.reset();
+ });
+
+ afterEach(() => {
+ fetchMock.reset();
+ isFeatureEnabled.mockRestore();
+ });
+
+ describe('with backend persistence enabled', () => {
+ beforeEach(() => {
+ isFeatureEnabled.mockImplementation(
+ feature => feature === 'SQLLAB_BACKEND_PERSISTENCE',
+ );
+ });
+
+ test('creates backend tab immediately and returns tabViewId', async () => {
+ const store = mockStore({});
+ const queryEditor = { name: 'Test Tab', sql: 'SELECT 1' };
+
+ // Mock successful backend creation
+ fetchMock.post('glob:*/tabstateview/', { id: 42 });
+
+ const result = await store.dispatch(
+ actions.addQueryEditorWithBackendSync(queryEditor),
+ );
+
+ // Verify backend call was made
+ const postCalls = fetchMock
+ .calls()
+ .filter(
+ call =>
+ call[0].includes('/tabstateview/') && call[1].method === 'POST',
+ );
+ expect(postCalls).toHaveLength(1);
+
+ // Verify action was dispatched with backend ID
+ const addActions = store
+ .getActions()
+ .filter(action => action.type === 'ADD_QUERY_EDITOR');
+ expect(addActions).toHaveLength(1);
+ expect(addActions[0].queryEditor.tabViewId).toBe('42');
+ expect(addActions[0].queryEditor.inLocalStorage).toBe(false);
+ expect(addActions[0].queryEditor.loaded).toBe(true);
+
+ // Verify returned query editor
+ expect(result.tabViewId).toBe('42');
+ expect(result.inLocalStorage).toBe(false);
+ });
+
+ test('falls back to local storage on backend failure', async () => {
+ const store = mockStore({});
+ const queryEditor = { name: 'Test Tab', sql: 'SELECT 1' };
+
+ // Mock backend failure
+ fetchMock.post('glob:*/tabstateview/', { status: 500 });
+
+ const result = await store.dispatch(
+ actions.addQueryEditorWithBackendSync(queryEditor),
+ );
+
+ // Verify fallback action was dispatched
+ const actionsDispatched = store.getActions();
+ const addActions = actionsDispatched.filter(
+ action => action.type === 'ADD_QUERY_EDITOR',
+ );
+ const toastActions = actionsDispatched.filter(
+ action => action.type === 'ADD_TOAST',
+ );
+
+ expect(addActions).toHaveLength(1);
+ expect(addActions[0].queryEditor.tabViewId).toBeUndefined();
+ expect(addActions[0].queryEditor.inLocalStorage).toBe(true);
+
+ // Should have error toast
+ expect(toastActions).toHaveLength(1);
+
+ // Verify returned query editor (fallback)
+ expect(result.tabViewId).toBeUndefined();
+ expect(result.inLocalStorage).toBe(true);
+ });
+
+ test('SQL updates work immediately after tab creation', async () => {
+ const store = mockStore({
+ sqlLab: {
+ queryEditors: [],
+ unsavedQueryEditor: {},
+ },
+ });
+
+ // Mock backend tab creation
+ fetchMock.post('glob:*/tabstateview/', { id: 123 });
+ // Mock SQL update
+ fetchMock.put('glob:*/tabstateview/123', { status: 200 });
+
+ // Create tab with backend sync
+ const queryEditor = await store.dispatch(
+ actions.addQueryEditorWithBackendSync({
+ name: 'Test',
+ sql: 'SELECT 1',
+ }),
+ );
+
+ // Clear previous actions
+ store.clearActions();
+
+ // Update state for the SQL update call
+ const updatedStore = mockStore({
+ sqlLab: {
+ queryEditors: [queryEditor],
+ unsavedQueryEditor: {},
+ },
+ });
+
+ // Now SQL updates should work immediately
+ await updatedStore.dispatch(
+ actions.queryEditorSetAndSaveSql(queryEditor, 'SELECT 2'),
+ );
+
+ // Verify PUT call was made with correct integer ID
+ const putCalls = fetchMock
+ .calls()
+ .filter(
+ call =>
+ call[0].includes('/tabstateview/') && call[1].method === 'PUT',
+ );
+ expect(putCalls).toHaveLength(1);
+ expect(putCalls[0][0]).toContain('/tabstateview/123');
+ });
+ });
+
+ describe('with backend persistence disabled', () => {
+ beforeEach(() => {
+ isFeatureEnabled.mockImplementation(() => false);
+ });
+
+ test('creates local tab without backend call', async () => {
+ const store = mockStore({});
+ const queryEditor = { name: 'Test Tab', sql: 'SELECT 1' };
+
+ const result = await store.dispatch(
+ actions.addQueryEditorWithBackendSync(queryEditor),
+ );
+
+ // Verify no backend calls were made
+ const calls = fetchMock.calls();
+ expect(calls).toHaveLength(0);
+
+ // Verify local action was dispatched
+ const addActions = store
+ .getActions()
+ .filter(action => action.type === 'ADD_QUERY_EDITOR');
+ expect(addActions).toHaveLength(1);
+ expect(addActions[0].queryEditor.tabViewId).toBeUndefined();
+ expect(addActions[0].queryEditor.inLocalStorage).toBe(true);
+
+ // Verify returned query editor
+ expect(result.tabViewId).toBeUndefined();
+ expect(result.inLocalStorage).toBe(true);
+ });
+ });
+
+ describe('integration with addNewQueryEditor', () => {
+ test('addNewQueryEditor uses backend sync', async () => {
+ isFeatureEnabled.mockImplementation(
+ feature => feature === 'SQLLAB_BACKEND_PERSISTENCE',
+ );
+
+ const store = mockStore({
+ sqlLab: {
+ queryEditors: [],
+ unsavedQueryEditor: {},
+ tabHistory: [],
+ databases: {},
+ },
+ common: {
+ conf: {
+ SQLLAB_DEFAULT_DBID: 1,
+ DEFAULT_SQLLAB_LIMIT: 1000,
+ },
+ },
+ });
+
+ // Mock backend creation
+ fetchMock.post('glob:*/tabstateview/', { id: 999 });
+
+ await store.dispatch(actions.addNewQueryEditor());
+
+ // Verify backend call was made
+ const postCalls = fetchMock
+ .calls()
+ .filter(
+ call =>
+ call[0].includes('/tabstateview/') && call[1].method === 'POST',
+ );
+ expect(postCalls).toHaveLength(1);
+
+ // Verify resulting tab has backend ID
+ const addActions = store
+ .getActions()
+ .filter(action => action.type === 'ADD_QUERY_EDITOR');
+ expect(addActions).toHaveLength(1);
+ expect(addActions[0].queryEditor.tabViewId).toBe('999');
+ expect(addActions[0].queryEditor.inLocalStorage).toBe(false);
+ });
+ });
+});
diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.tabStateValidation.test.js b/superset-frontend/src/SqlLab/actions/sqlLab.tabStateValidation.test.js
new file mode 100644
index 0000000000..687e3a75a6
--- /dev/null
+++ b/superset-frontend/src/SqlLab/actions/sqlLab.tabStateValidation.test.js
@@ -0,0 +1,305 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * Tests for SQL Lab tab state management and validation
+ *
+ * This file validates the behavior of SQL Lab tab state management,
+ * particularly the interaction between frontend-generated IDs and backend
+ * persistence. Tests edge cases and race conditions that can occur when
+ * the SqllabBackendPersistence feature flag is enabled.
+ *
+ * Related to issue #34997: Ensures metadata updates work correctly
+ * when users modify queries in newly created tabs.
+ */
+
+import configureMockStore from 'redux-mock-store';
+import thunk from 'redux-thunk';
+import fetchMock from 'fetch-mock';
+
+import { isFeatureEnabled } from '@superset-ui/core';
+import * as actions from './sqlLab';
+import { defaultQueryEditor } from '../fixtures';
+
+// Mock feature flags
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ isFeatureEnabled: jest.fn(),
+}));
+
+const mockStore = configureMockStore([thunk]);
+
+describe('SQL Lab tab state validation', () => {
+ const updateTabStateEndpoint = 'glob:*/tabstateview/*';
+
+ beforeEach(() => {
+ fetchMock.reset();
+ // Enable backend persistence for these tests
+ isFeatureEnabled.mockImplementation(
+ feature => feature === 'SQLLAB_BACKEND_PERSISTENCE',
+ );
+ });
+
+ afterEach(() => {
+ fetchMock.reset();
+ isFeatureEnabled.mockRestore();
+ });
+
+ describe('New tab creation with string IDs', () => {
+ test('addQueryEditor creates tabs with string nanoid IDs', () => {
+ const queryEditor = { name: 'Test Tab', sql: 'SELECT 1' };
+ const action = actions.addQueryEditor(queryEditor);
+
+ // Verify string ID was generated
+ expect(typeof action.queryEditor.id).toBe('string');
+ expect(action.queryEditor.id).toMatch(/^[a-zA-Z0-9_-]{11}$/);
+ expect(action.queryEditor.inLocalStorage).toBe(true);
+ expect(action.queryEditor.tabViewId).toBeUndefined();
+ });
+
+ test('multiple new tabs get unique string IDs', () => {
+ const editor1 = actions.addQueryEditor({ name: 'Tab 1' });
+ const editor2 = actions.addQueryEditor({ name: 'Tab 2' });
+ const editor3 = actions.addQueryEditor({ name: 'Tab 3' });
+
+ // All should have different string IDs
+ expect(editor1.queryEditor.id).not.toBe(editor2.queryEditor.id);
+ expect(editor2.queryEditor.id).not.toBe(editor3.queryEditor.id);
+ expect(editor1.queryEditor.id).not.toBe(editor3.queryEditor.id);
+
+ // All should be strings
+ expect(typeof editor1.queryEditor.id).toBe('string');
+ expect(typeof editor2.queryEditor.id).toBe('string');
+ expect(typeof editor3.queryEditor.id).toBe('string');
+ });
+ });
+
+ describe('Backend persistence edge cases', () => {
+ test('queryEditorSetAndSaveSql sends string ID to backend', async () => {
+ // Create new tab with string ID (simulating real nanoid output)
+ const newQueryEditor = {
+ ...defaultQueryEditor,
+ id: 'FRRULMQgWHa', // Realistic nanoid(11) output
+ inLocalStorage: true,
+ // No tabViewId - simulates new tab before sync
+ };
+
+ const store = mockStore({
+ sqlLab: {
+ queryEditors: [newQueryEditor],
+ unsavedQueryEditor: {},
+ },
+ });
+
+ // Mock the endpoint to track what ID is sent
+ fetchMock.put(updateTabStateEndpoint, { status: 404 });
+
+ // This should attempt to use the string ID
+ const sql = 'SELECT * FROM test';
+ await store.dispatch(
+ actions.queryEditorSetAndSaveSql(newQueryEditor, sql),
+ );
+
+ // Verify the string ID was sent to backend
+ const putCalls = fetchMock
+ .calls()
+ .filter(
+ call =>
+ call[0].includes('/tabstateview/') && call[1].method === 'PUT',
+ );
+
+ expect(putCalls).toHaveLength(1);
+ expect(putCalls[0][0]).toContain('/tabstateview/FRRULMQgWHa');
+ });
+
+ test('string IDs cause backend endpoint mismatch errors', async () => {
+ const newQueryEditor = {
+ ...defaultQueryEditor,
+ id: 'abc123def456', // String ID
+ inLocalStorage: true,
+ };
+
+ const store = mockStore({
+ sqlLab: {
+ queryEditors: [newQueryEditor],
+ unsavedQueryEditor: {},
+ },
+ });
+
+ // Mock 404 response for string IDs (simulating real backend behavior)
+ fetchMock.put('glob:*/tabstateview/abc123def456', { status: 404 });
+
+ // The function should handle the 404 but dispatch a danger toast
+ await store.dispatch(
+ actions.queryEditorSetAndSaveSql(newQueryEditor, 'SELECT 1'),
+ );
+
+ // Verify error handling occurred (the function catches and handles 404 silently)
+ const actionsDispatched = store.getActions();
+ console.log(
+ 'Actions dispatched:',
+ actionsDispatched.map(a => a.type),
+ );
+
+ // Check that the PUT call was attempted with string ID
+ const putCalls = fetchMock
+ .calls()
+ .filter(
+ call =>
+ call[0].includes('/tabstateview/') && call[1].method === 'PUT',
+ );
+ expect(putCalls).toHaveLength(1);
+ expect(putCalls[0][0]).toContain('/abc123def456');
+ });
+
+ test('existing tabs with tabViewId work correctly', async () => {
+ // Existing tab with proper backend ID
+ const existingQueryEditor = {
+ ...defaultQueryEditor,
+ id: 'frontend-id-123',
+ tabViewId: '42', // Integer ID from backend (as string)
+ inLocalStorage: false,
+ };
+
+ const store = mockStore({
+ sqlLab: {
+ queryEditors: [existingQueryEditor],
+ unsavedQueryEditor: {},
+ },
+ });
+
+ // Mock successful response for integer IDs
+ fetchMock.put('glob:*/tabstateview/42', { status: 200 });
+
+ await store.dispatch(
+ actions.queryEditorSetAndSaveSql(existingQueryEditor, 'SELECT 1'),
+ );
+
+ // Verify correct endpoint was called with integer ID
+ const putCalls = fetchMock
+ .calls()
+ .filter(
+ call =>
+ call[0].includes('/tabstateview/') && call[1].method === 'PUT',
+ );
+
+ expect(putCalls).toHaveLength(1);
+ expect(putCalls[0][0]).toContain('/tabstateview/42');
+ });
+ });
+
+ describe('Race condition scenarios', () => {
+ test('rapid tab creation and SQL editing triggers string ID usage', async () => {
+ const store = mockStore({
+ sqlLab: {
+ queryEditors: [],
+ unsavedQueryEditor: {},
+ tabHistory: [],
+ databases: {},
+ },
+ common: {
+ conf: {
+ SQLLAB_DEFAULT_DBID: 1,
+ DEFAULT_SQLLAB_LIMIT: 1000,
+ },
+ },
+ });
+
+ // Mock endpoints for this specific test
+ fetchMock.put('glob:*/tabstateview/*', { status: 404 });
+ fetchMock.post('glob:*/tabstateview/', { id: 999 });
+
+ // Simulate rapid user actions: create tab then immediately edit SQL
+ await store.dispatch(actions.addNewQueryEditor());
+ const addAction = store
+ .getActions()
+ .find(a => a.type === 'ADD_QUERY_EDITOR');
+ const newTab = addAction.queryEditor;
+
+ // User immediately starts typing (before EditorAutoSync runs)
+ await store.dispatch(
+ actions.queryEditorSetAndSaveSql(newTab, 'SELECT NOW()'),
+ );
+
+ // Should attempt to use string ID
+ const putCalls = fetchMock
+ .calls()
+ .filter(
+ call =>
+ call[0].includes('/tabstateview/') && call[1].method === 'PUT',
+ );
+ expect(putCalls).toHaveLength(1);
+ expect(putCalls[0][0]).toMatch(/\/tabstateview\/[a-zA-Z0-9_-]{11}$/);
+ });
+
+ test('feature flag disabled should not attempt backend updates', async () => {
+ // Disable backend persistence
+ isFeatureEnabled.mockImplementation(() => false);
+
+ const newQueryEditor = {
+ ...defaultQueryEditor,
+ id: 'string-id-123',
+ inLocalStorage: true,
+ };
+
+ const store = mockStore({
+ sqlLab: {
+ queryEditors: [newQueryEditor],
+ unsavedQueryEditor: {},
+ },
+ });
+
+ await store.dispatch(
+ actions.queryEditorSetAndSaveSql(newQueryEditor, 'SELECT 1'),
+ );
+
+ // Should not make any backend calls
+ const putCalls = fetchMock
+ .calls()
+ .filter(call => call[0].includes('/tabstateview/'));
+ expect(putCalls).toHaveLength(0);
+ });
+ });
+
+ describe('Comparison: old vs new behavior', () => {
+ test('old addQueryEditor still creates local tabs', () => {
+ // The original function still works for backward compatibility
+ const action = actions.addQueryEditor({ name: 'New Tab' });
+
+ expect(action.queryEditor.tabViewId).toBeUndefined();
+ expect(action.queryEditor.inLocalStorage).toBe(true);
+ });
+
+ test('FIXED: addQueryEditorWithBackendSync creates backend tabs immediately', async () => {
+ // The new function fixes the race condition
+ const store = mockStore({});
+
+ // Mock successful tab creation
+ fetchMock.post('glob:*/tabstateview/', { id: 123 });
+
+ const queryEditor = await store.dispatch(
+ actions.addQueryEditorWithBackendSync({ name: 'New Tab' }),
+ );
+
+ // After fix: should have tabViewId set immediately
+ expect(queryEditor.tabViewId).toBe('123');
+ expect(queryEditor.inLocalStorage).toBe(false);
+ });
+ });
+});