diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index c94a3165b08..fd21c30aa12 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -976,10 +976,6 @@ export interface FeatureToggles { */ alertingBulkActionsInUI?: boolean; /** - * Use proxy-based read-only objects for plugin extensions instead of deep cloning - */ - extensionsReadOnlyProxy?: boolean; - /** * Registers AuthZ /apis endpoint */ kubernetesAuthzApis?: boolean; diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 85fc598fad4..9e99c99bb3a 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -1675,15 +1675,6 @@ var ( HideFromDocs: true, Expression: "true", // enabled by default }, - { - Name: "extensionsReadOnlyProxy", - Description: "Use proxy-based read-only objects for plugin extensions instead of deep cloning", - Stage: FeatureStageExperimental, - Owner: grafanaPluginsPlatformSquad, - HideFromAdminPage: true, - HideFromDocs: true, - FrontendOnly: true, - }, { Name: "kubernetesAuthzApis", Description: "Registers AuthZ /apis endpoint", diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index 2278cb1fb50..cd2d6706a08 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -219,7 +219,6 @@ multiTenantFrontend,experimental,@grafana/grafana-frontend-platform,false,false, alertingListViewV2PreviewToggle,privatePreview,@grafana/alerting-squad,false,false,true alertRuleUseFiredAtForStartsAt,experimental,@grafana/alerting-squad,false,false,false alertingBulkActionsInUI,GA,@grafana/alerting-squad,false,false,true -extensionsReadOnlyProxy,experimental,@grafana/plugins-platform-backend,false,false,true kubernetesAuthzApis,experimental,@grafana/identity-access-team,false,false,false restoreDashboards,experimental,@grafana/grafana-frontend-platform,false,false,false skipTokenRotationIfRecent,privatePreview,@grafana/identity-access-team,false,false,false diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index fbd6e7ab88a..7125fa63ebd 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -887,10 +887,6 @@ const ( // Enables the alerting bulk actions in the UI FlagAlertingBulkActionsInUI = "alertingBulkActionsInUI" - // FlagExtensionsReadOnlyProxy - // Use proxy-based read-only objects for plugin extensions instead of deep cloning - FlagExtensionsReadOnlyProxy = "extensionsReadOnlyProxy" - // FlagKubernetesAuthzApis // Registers AuthZ /apis endpoint FlagKubernetesAuthzApis = "kubernetesAuthzApis" diff --git a/pkg/services/featuremgmt/toggles_gen.json b/pkg/services/featuremgmt/toggles_gen.json index dda8e2005c4..052b3b22280 100644 --- a/pkg/services/featuremgmt/toggles_gen.json +++ b/pkg/services/featuremgmt/toggles_gen.json @@ -1110,7 +1110,8 @@ "metadata": { "name": "extensionsReadOnlyProxy", "resourceVersion": "1750434297879", - "creationTimestamp": "2025-05-06T04:55:23Z" + "creationTimestamp": "2025-05-06T04:55:23Z", + "deletionTimestamp": "2025-06-30T08:24:11Z" }, "spec": { "description": "Use proxy-based read-only objects for plugin extensions instead of deep cloning", diff --git a/public/app/features/datasources/components/DataSourcePluginSettings.tsx b/public/app/features/datasources/components/DataSourcePluginSettings.tsx index 0dfd48bb5bf..80098048692 100644 --- a/public/app/features/datasources/components/DataSourcePluginSettings.tsx +++ b/public/app/features/datasources/components/DataSourcePluginSettings.tsx @@ -1,7 +1,7 @@ import { createElement, PureComponent } from 'react'; import { DataSourcePluginMeta, DataSourceSettings } from '@grafana/data'; -import { readOnlyCopy } from 'app/features/plugins/extensions/utils'; +import { writableProxy } from 'app/features/plugins/extensions/utils'; import { GenericDataSourcePlugin } from '../types'; @@ -34,7 +34,7 @@ export class DataSourcePluginSettings extends PureComponent {
{plugin.components.ConfigEditor && createElement(plugin.components.ConfigEditor, { - options: readOnlyCopy(dataSource), + options: writableProxy(dataSource), onOptionsChange: this.onModelChanged, })}
diff --git a/public/app/features/plugins/extensions/usePluginComponent.test.tsx b/public/app/features/plugins/extensions/usePluginComponent.test.tsx index 2e8e92f7321..266a9f602c0 100644 --- a/public/app/features/plugins/extensions/usePluginComponent.test.tsx +++ b/public/app/features/plugins/extensions/usePluginComponent.test.tsx @@ -380,8 +380,8 @@ describe('usePluginComponent()', () => { // Should not throw an error if it mutates the props expect(() => render(Component && )).not.toThrow(); - // Should log a warning - expect(log.warning).toHaveBeenCalledWith('Attempted to mutate object property "c"', { + // Should log an error in dev mode + expect(log.error).toHaveBeenCalledWith('Attempted to mutate object property "c"', { stack: expect.any(String), }); }); diff --git a/public/app/features/plugins/extensions/usePluginComponents.test.tsx b/public/app/features/plugins/extensions/usePluginComponents.test.tsx index c2ab7ec6845..99ebda0eef5 100644 --- a/public/app/features/plugins/extensions/usePluginComponents.test.tsx +++ b/public/app/features/plugins/extensions/usePluginComponents.test.tsx @@ -266,7 +266,7 @@ describe('usePluginComponents()', () => { // Should also render the component if it wants to change the props expect(() => render()).not.toThrow(); - expect(log.warning).toHaveBeenCalledWith(`Attempted to mutate object property "foo4"`, { + expect(log.error).toHaveBeenCalledWith(`Attempted to mutate object property "foo4"`, { stack: expect.any(String), }); diff --git a/public/app/features/plugins/extensions/utils.test.tsx b/public/app/features/plugins/extensions/utils.test.tsx index e1edf9bd3c8..b41ec1c99f0 100644 --- a/public/app/features/plugins/extensions/utils.test.tsx +++ b/public/app/features/plugins/extensions/utils.test.tsx @@ -21,8 +21,7 @@ import { getAppPluginDependencies, getExtensionPointPluginMeta, getMutationObserverProxy, - readOnlyCopy, - isReadOnlyProxy, + writableProxy, isMutationObserverProxy, } from './utils'; @@ -401,7 +400,7 @@ describe('Plugin Extensions / Utils', () => { expect(proxy.a).toBe('b'); }); - it('should be possible to set new values, but logs a warning', () => { + it('should be possible to set new values, but logs a debug message', () => { const obj: { a: string; b?: string } = { a: 'a' }; const proxy = getMutationObserverProxy(obj); @@ -412,7 +411,7 @@ describe('Plugin Extensions / Utils', () => { }); }).not.toThrow(); - expect(log.warning).toHaveBeenCalledWith(`Attempted to define object property "b"`, { + expect(log.debug).toHaveBeenCalledWith(`Attempted to define object property "b"`, { stack: expect.any(String), }); @@ -440,12 +439,11 @@ describe('Plugin Extensions / Utils', () => { }); }); - describe('readOnlyCopy()', () => { + describe('writableProxy()', () => { const originalEnv = config.buildInfo.env; beforeEach(() => { jest.spyOn(console, 'warn').mockImplementation(); - config.featureToggles.extensionsReadOnlyProxy = false; }); afterEach(() => { @@ -454,34 +452,19 @@ describe('Plugin Extensions / Utils', () => { }); it('should return the same value for primitive types', () => { - expect(readOnlyCopy(1)).toBe(1); - expect(readOnlyCopy('a')).toBe('a'); - expect(readOnlyCopy(true)).toBe(true); - expect(readOnlyCopy(false)).toBe(false); - expect(readOnlyCopy(null)).toBe(null); - expect(readOnlyCopy(undefined)).toBe(undefined); - }); - - it('should return a read-only proxy of the original object if the feature flag is enabled', () => { - config.featureToggles.extensionsReadOnlyProxy = true; - - const obj = { a: 'a' }; - const copy = readOnlyCopy(obj); - - expect(copy).not.toBe(obj); - expect(copy.a).toBe('a'); - expect(isReadOnlyProxy(copy)).toBe(true); - expect(() => { - copy.a = 'b'; - }).toThrow(TypeError); + expect(writableProxy(1)).toBe(1); + expect(writableProxy('a')).toBe('a'); + expect(writableProxy(true)).toBe(true); + expect(writableProxy(false)).toBe(false); + expect(writableProxy(null)).toBe(null); + expect(writableProxy(undefined)).toBe(undefined); }); it('should return a writable deep-copy of the original object in dev mode', () => { - config.featureToggles.extensionsReadOnlyProxy = false; config.buildInfo.env = 'development'; const obj = { a: 'a' }; - const copy = readOnlyCopy(obj); + const copy = writableProxy(obj); expect(copy).not.toBe(obj); expect(copy.a).toBe('a'); @@ -498,11 +481,10 @@ describe('Plugin Extensions / Utils', () => { }); it('should return a writable deep-copy of the original object in production mode', () => { - config.featureToggles.extensionsReadOnlyProxy = false; config.buildInfo.env = 'production'; const obj = { a: 'a' }; - const copy = readOnlyCopy(obj); + const copy = writableProxy(obj); expect(copy).not.toBe(obj); expect(copy.a).toBe('a'); @@ -519,11 +501,10 @@ describe('Plugin Extensions / Utils', () => { }); it('should allow freezing the object in production mode', () => { - config.featureToggles.extensionsReadOnlyProxy = false; config.buildInfo.env = 'production'; const obj = { a: 'a', b: { c: 'c' } }; - const copy = readOnlyCopy(obj); + const copy = writableProxy(obj); expect(() => { Object.freeze(copy); @@ -534,7 +515,7 @@ describe('Plugin Extensions / Utils', () => { expect(Object.isFrozen(copy.b)).toBe(true); expect(copy.b).toEqual({ c: 'c' }); - expect(log.warning).toHaveBeenCalledWith(`Attempted to define object property "a"`, { + expect(log.debug).toHaveBeenCalledWith(`Attempted to define object property "a"`, { stack: expect.any(String), }); }); @@ -687,7 +668,7 @@ describe('Plugin Extensions / Utils', () => { expect(screen.getByText('Version: 1.0.0')).toBeVisible(); }); - it('should not be possible to mutate the props in development mode, but it logs a warning', async () => { + it('should not be possible to mutate the props in development mode, but it logs an error', async () => { config.buildInfo.env = 'development'; const pluginId = 'grafana-worldmap-panel'; const Component = wrapWithPluginContext(pluginId, ExampleComponent, log); @@ -700,8 +681,8 @@ describe('Plugin Extensions / Utils', () => { expect(await screen.findByText('Hello Grafana!')).toBeVisible(); // Logs a warning - expect(log.warning).toHaveBeenCalledTimes(1); - expect(log.warning).toHaveBeenCalledWith(`Attempted to mutate object property "c"`, { + expect(log.error).toHaveBeenCalledTimes(1); + expect(log.error).toHaveBeenCalledWith(`Attempted to mutate object property "c"`, { stack: expect.any(String), }); diff --git a/public/app/features/plugins/extensions/utils.tsx b/public/app/features/plugins/extensions/utils.tsx index 72f17ee6974..423308dbe63 100644 --- a/public/app/features/plugins/extensions/utils.tsx +++ b/public/app/features/plugins/extensions/utils.tsx @@ -85,7 +85,7 @@ export const wrapWithPluginContext = (pluginId: string, Component: React.Com return ( - + ); }; @@ -230,24 +230,27 @@ export function getMutationObserverProxy(obj: T, _log: Extensi } const cache = new WeakMap(); + const logFunction = isGrafanaDevMode() ? _log.error.bind(_log) : _log.warning.bind(_log); // should show error during local development return new Proxy(obj, { deleteProperty(target, prop) { - _log.warning(`Attempted to delete object property "${String(prop)}"`, { + logFunction(`Attempted to delete object property "${String(prop)}"`, { stack: new Error().stack ?? '', }); Reflect.deleteProperty(target, prop); return true; }, defineProperty(target, prop, descriptor) { - _log.warning(`Attempted to define object property "${String(prop)}"`, { + // because immer (used by RTK) calls Object.isFrozen and Object.freeze we know that defineProperty will be called + // behind the scenes as well so we only log message with debug level to minimize the noise and false positives + _log.debug(`Attempted to define object property "${String(prop)}"`, { stack: new Error().stack ?? '', }); Reflect.defineProperty(target, prop, descriptor); return true; }, set(target, prop, newValue) { - _log.warning(`Attempted to mutate object property "${String(prop)}"`, { + logFunction(`Attempted to mutate object property "${String(prop)}"`, { stack: new Error().stack ?? '', }); Reflect.set(target, prop, newValue); @@ -285,16 +288,12 @@ export function getMutationObserverProxy(obj: T, _log: Extensi }); } -export function readOnlyCopy(value: T, _log: ExtensionsLog = log): T { +export function writableProxy(value: T, _log: ExtensionsLog = log): T { // Primitive types are read-only by default if (!value || typeof value !== 'object') { return value; } - if (config.featureToggles.extensionsReadOnlyProxy) { - return getReadOnlyProxy(value); - } - // Default: we return a proxy of a deep-cloned version of the original object, which logs warnings when mutation is attempted return getMutationObserverProxy(cloneDeep(value), _log); }