You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2025/10/15 11:43:06 UTC
(superset) branch 5.0-extensions updated: fix: no fs logging of extensions unless flag is set (#35612)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 5.0-extensions
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/5.0-extensions by this push:
new bfd31ccba7 fix: no fs logging of extensions unless flag is set (#35612)
bfd31ccba7 is described below
commit bfd31ccba770da92ff48a8093c5b9ced2fe2ebff
Author: Ville Brofeldt <33...@users.noreply.github.com>
AuthorDate: Tue Oct 14 12:11:43 2025 -0700
fix: no fs logging of extensions unless flag is set (#35612)
Co-authored-by: Ville Brofeldt <v_...@apple.com>
(cherry picked from commit bd48e87eeb2c7c53ef60c498f065e34d1d097330)
---
.../src/extensions/ExtensionsManager.ts | 10 +-
.../src/extensions/ExtensionsStartup.test.tsx | 169 ++++++++++++++++-----
.../src/extensions/ExtensionsStartup.tsx | 17 ++-
3 files changed, 139 insertions(+), 57 deletions(-)
diff --git a/superset-frontend/src/extensions/ExtensionsManager.ts b/superset-frontend/src/extensions/ExtensionsManager.ts
index 6222a82094..61cc3e7e6e 100644
--- a/superset-frontend/src/extensions/ExtensionsManager.ts
+++ b/superset-frontend/src/extensions/ExtensionsManager.ts
@@ -16,12 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import {
- FeatureFlag,
- SupersetClient,
- isFeatureEnabled,
- logging,
-} from '@superset-ui/core';
+import { SupersetClient, logging } from '@superset-ui/core';
import type { contributions, core } from '@apache-superset/core';
import { ExtensionContext } from '../core/models';
@@ -62,9 +57,6 @@ class ExtensionsManager {
* @throws Error if initialization fails.
*/
public async initializeExtensions(): Promise<void> {
- if (!isFeatureEnabled(FeatureFlag.EnableExtensions)) {
- return;
- }
const response = await SupersetClient.get({
endpoint: '/api/v1/extensions/',
});
diff --git a/superset-frontend/src/extensions/ExtensionsStartup.test.tsx b/superset-frontend/src/extensions/ExtensionsStartup.test.tsx
index 02010a4e5b..d1af44098f 100644
--- a/superset-frontend/src/extensions/ExtensionsStartup.test.tsx
+++ b/superset-frontend/src/extensions/ExtensionsStartup.test.tsx
@@ -17,10 +17,21 @@
* under the License.
*/
import { render, waitFor } from 'spec/helpers/testing-library';
-import { logging } from '@superset-ui/core';
+import { logging, FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import fetchMock from 'fetch-mock';
import ExtensionsStartup from './ExtensionsStartup';
import ExtensionsManager from './ExtensionsManager';
+// Mock the isFeatureEnabled function
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ isFeatureEnabled: jest.fn(),
+}));
+
+const mockIsFeatureEnabled = isFeatureEnabled as jest.MockedFunction<
+ typeof isFeatureEnabled
+>;
+
const mockInitialState = {
user: { userId: 1 },
};
@@ -36,12 +47,26 @@ beforeEach(() => {
// Clear any existing ExtensionsManager instance
(ExtensionsManager as any).instance = undefined;
+
+ // Reset feature flag mock to enabled by default
+ mockIsFeatureEnabled.mockReset();
+ mockIsFeatureEnabled.mockReturnValue(true);
+
+ // Setup fetch mocks for API calls
+ fetchMock.restore();
+ fetchMock.get('glob:*/api/v1/extensions/', {
+ result: [],
+ });
});
afterEach(() => {
// Clean up after each test
delete (window as any).superset;
(ExtensionsManager as any).instance = undefined;
+
+ // Reset mocks
+ mockIsFeatureEnabled.mockReset();
+ fetchMock.restore();
});
test('renders without crashing', () => {
@@ -55,6 +80,12 @@ test('renders without crashing', () => {
});
test('sets up global superset object when user is logged in', async () => {
+ // Mock initializeExtensions to avoid API calls in this test
+ const manager = ExtensionsManager.getInstance();
+ const initializeSpy = jest
+ .spyOn(manager, 'initializeExtensions')
+ .mockImplementation(() => Promise.resolve());
+
render(<ExtensionsStartup />, {
useRedux: true,
initialState: mockInitialState,
@@ -70,6 +101,8 @@ test('sets up global superset object when user is logged in', async () => {
expect((window as any).superset.extensions).toBeDefined();
expect((window as any).superset.sqlLab).toBeDefined();
});
+
+ initializeSpy.mockRestore();
});
test('does not set up global superset object when user is not logged in', async () => {
@@ -85,18 +118,26 @@ test('does not set up global superset object when user is not logged in', async
});
test('initializes ExtensionsManager when user is logged in', async () => {
+ // Mock initializeExtensions to avoid API calls, but track that it was called
+ const manager = ExtensionsManager.getInstance();
+ const initializeSpy = jest
+ .spyOn(manager, 'initializeExtensions')
+ .mockImplementation(() => Promise.resolve());
+
render(<ExtensionsStartup />, {
useRedux: true,
initialState: mockInitialState,
});
await waitFor(() => {
- // Verify ExtensionsManager has been initialized by checking if it has extensions loaded
- const manager = ExtensionsManager.getInstance();
+ // Verify ExtensionsManager initialization was called
+ expect(initializeSpy).toHaveBeenCalledTimes(1);
// The manager should exist and be ready to use
expect(manager).toBeDefined();
expect(manager.getExtensions).toBeDefined();
});
+
+ initializeSpy.mockRestore();
});
test('does not initialize ExtensionsManager when user is not logged in', async () => {
@@ -114,36 +155,47 @@ test('does not initialize ExtensionsManager when user is not logged in', async (
});
});
-test('handles ExtensionsManager initialization errors gracefully', async () => {
- const errorSpy = jest.spyOn(logging, 'error').mockImplementation();
+test('only initializes once even with multiple renders', async () => {
+ // Track calls to the manager's public API
+ const manager = ExtensionsManager.getInstance();
+ const originalInitialize = manager.initializeExtensions;
+ let initializeCallCount = 0;
- // Mock the initializeExtensions method to throw an error
- const originalInitialize = ExtensionsManager.prototype.initializeExtensions;
- ExtensionsManager.prototype.initializeExtensions = jest
- .fn()
- .mockImplementation(() => {
- throw new Error('Test initialization error');
- });
+ manager.initializeExtensions = jest.fn().mockImplementation(() => {
+ initializeCallCount += 1;
+ return Promise.resolve();
+ });
- render(<ExtensionsStartup />, {
+ const { rerender } = render(<ExtensionsStartup />, {
useRedux: true,
initialState: mockInitialState,
});
await waitFor(() => {
- // Verify error was logged
- expect(errorSpy).toHaveBeenCalledWith(
- 'Error setting up extensions:',
- expect.any(Error),
- );
+ expect(initializeCallCount).toBe(1);
});
+ // Re-render the component
+ rerender(<ExtensionsStartup />);
+
+ // Wait for any potential async operations, but expect no additional calls
+ await waitFor(() => {
+ expect(initializeCallCount).toBe(1);
+ });
+
+ // Verify initialization is still only called once
+ expect(initializeCallCount).toBe(1);
+
// Restore original method
- ExtensionsManager.prototype.initializeExtensions = originalInitialize;
- errorSpy.mockRestore();
+ manager.initializeExtensions = originalInitialize;
});
-test('logs success message when ExtensionsManager initializes successfully', async () => {
+test('initializes ExtensionsManager and logs success when EnableExtensions feature flag is enabled', async () => {
+ // Ensure feature flag is enabled
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.EnableExtensions,
+ );
+
const infoSpy = jest.spyOn(logging, 'info').mockImplementation();
// Mock the initializeExtensions method to succeed
@@ -158,6 +210,14 @@ test('logs success message when ExtensionsManager initializes successfully', asy
});
await waitFor(() => {
+ // Verify feature flag was checked
+ expect(mockIsFeatureEnabled).toHaveBeenCalledWith(
+ FeatureFlag.EnableExtensions,
+ );
+ // Verify initialization was called
+ expect(
+ ExtensionsManager.prototype.initializeExtensions,
+ ).toHaveBeenCalledTimes(1);
// Verify success message was logged
expect(infoSpy).toHaveBeenCalledWith(
'Extensions initialized successfully.',
@@ -169,37 +229,66 @@ test('logs success message when ExtensionsManager initializes successfully', asy
infoSpy.mockRestore();
});
-test('only initializes once even with multiple renders', async () => {
- // Track calls to the manager's public API
- const manager = ExtensionsManager.getInstance();
- const originalInitialize = manager.initializeExtensions;
- let initializeCallCount = 0;
+test('does not initialize ExtensionsManager when EnableExtensions feature flag is disabled', async () => {
+ // Disable the feature flag
+ mockIsFeatureEnabled.mockReturnValue(false);
- manager.initializeExtensions = jest.fn().mockImplementation(() => {
- initializeCallCount += 1;
- return Promise.resolve();
- });
+ const manager = ExtensionsManager.getInstance();
+ const initializeSpy = jest
+ .spyOn(manager, 'initializeExtensions')
+ .mockImplementation();
- const { rerender } = render(<ExtensionsStartup />, {
+ render(<ExtensionsStartup />, {
useRedux: true,
initialState: mockInitialState,
});
await waitFor(() => {
- expect(initializeCallCount).toBe(1);
+ // Verify feature flag was checked
+ expect(mockIsFeatureEnabled).toHaveBeenCalledWith(
+ FeatureFlag.EnableExtensions,
+ );
+ // Verify the global superset object is still set up
+ expect((window as any).superset).toBeDefined();
+ // But extensions should not be initialized
+ expect(initializeSpy).not.toHaveBeenCalled();
});
- // Re-render the component
- rerender(<ExtensionsStartup />);
+ initializeSpy.mockRestore();
+});
- // Wait for any potential async operations, but expect no additional calls
- await waitFor(() => {
- expect(initializeCallCount).toBe(1);
+test('logs error when ExtensionsManager initialization fails', async () => {
+ // Ensure feature flag is enabled
+ mockIsFeatureEnabled.mockReturnValue(true);
+
+ const errorSpy = jest.spyOn(logging, 'error').mockImplementation();
+
+ // Mock the initializeExtensions method to throw an error
+ const originalInitialize = ExtensionsManager.prototype.initializeExtensions;
+ ExtensionsManager.prototype.initializeExtensions = jest
+ .fn()
+ .mockImplementation(() => {
+ throw new Error('Test initialization error');
+ });
+
+ render(<ExtensionsStartup />, {
+ useRedux: true,
+ initialState: mockInitialState,
});
- // Verify initialization is still only called once
- expect(initializeCallCount).toBe(1);
+ await waitFor(() => {
+ // Verify feature flag was checked
+ expect(mockIsFeatureEnabled).toHaveBeenCalledWith(
+ FeatureFlag.EnableExtensions,
+ );
+ // Verify error was logged
+ expect(errorSpy).toHaveBeenCalledWith(
+ 'Error setting up extensions:',
+ expect.any(Error),
+ );
+ });
// Restore original method
- manager.initializeExtensions = originalInitialize;
+ ExtensionsManager.prototype.initializeExtensions = originalInitialize;
+ errorSpy.mockRestore();
});
diff --git a/superset-frontend/src/extensions/ExtensionsStartup.tsx b/superset-frontend/src/extensions/ExtensionsStartup.tsx
index 2706bb7312..41bb842a89 100644
--- a/superset-frontend/src/extensions/ExtensionsStartup.tsx
+++ b/superset-frontend/src/extensions/ExtensionsStartup.tsx
@@ -19,7 +19,7 @@
import { useEffect, useState } from 'react';
// eslint-disable-next-line no-restricted-syntax
import * as supersetCore from '@apache-superset/core';
-import { logging } from '@superset-ui/core';
+import { FeatureFlag, isFeatureEnabled, logging } from '@superset-ui/core';
import {
authentication,
core,
@@ -75,14 +75,15 @@ const ExtensionsStartup = () => {
};
// Initialize extensions
- try {
- ExtensionsManager.getInstance().initializeExtensions();
- logging.info('Extensions initialized successfully.');
- } catch (error) {
- logging.error('Error setting up extensions:', error);
- } finally {
- setInitialized(true);
+ if (isFeatureEnabled(FeatureFlag.EnableExtensions)) {
+ try {
+ ExtensionsManager.getInstance().initializeExtensions();
+ logging.info('Extensions initialized successfully.');
+ } catch (error) {
+ logging.error('Error setting up extensions:', error);
+ }
}
+ setInitialized(true);
}, [initialized, userId]);
return null;