Plugin Extensions: Read-only props for extension components (#102079)
* feat: pass read-only props to extension components This initial commit is used to verify that things are broken in case the datasource object is not cloned before passed in as a prop. * chore: update tests --------- Co-authored-by: Hugo Häggmark <hugo.haggmark@gmail.com>
This commit is contained in:
co-authored by
Hugo Häggmark
parent
61a8495485
commit
bcb2a7e36f
@@ -1026,4 +1026,8 @@ export interface FeatureToggles {
|
||||
* @default true
|
||||
*/
|
||||
alertingBulkActionsInUI?: boolean;
|
||||
/**
|
||||
* Use proxy-based read-only objects for plugin extensions instead of deep cloning
|
||||
*/
|
||||
extensionsReadOnlyProxy?: boolean;
|
||||
}
|
||||
|
||||
@@ -1765,6 +1765,15 @@ 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,
|
||||
},
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
@@ -231,3 +231,4 @@ 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
|
||||
|
||||
|
@@ -934,4 +934,8 @@ const (
|
||||
// FlagAlertingBulkActionsInUI
|
||||
// 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"
|
||||
)
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -1,5 +1,4 @@
|
||||
import { AnyAction } from '@reduxjs/toolkit';
|
||||
import { cloneDeep } from 'lodash';
|
||||
import { useMemo } from 'react';
|
||||
import * as React from 'react';
|
||||
|
||||
@@ -199,8 +198,8 @@ export function EditDataSourceView({
|
||||
<div key={Component.meta.id}>
|
||||
<Component
|
||||
context={{
|
||||
dataSource: cloneDeep(dataSource),
|
||||
dataSourceMeta: dataSourceMeta,
|
||||
dataSource,
|
||||
dataSourceMeta,
|
||||
testingStatus,
|
||||
setJsonData: (jsonData) =>
|
||||
onOptionsChange({
|
||||
|
||||
@@ -3,7 +3,7 @@ import { nanoid } from 'nanoid';
|
||||
import { Observable, ReplaySubject } from 'rxjs';
|
||||
|
||||
import { Labels, LogLevel } from '@grafana/data';
|
||||
import { config } from '@grafana/runtime';
|
||||
import { config, createMonitoringLogger } from '@grafana/runtime';
|
||||
|
||||
export type ExtensionsLogItem = {
|
||||
level: LogLevel;
|
||||
@@ -18,6 +18,7 @@ export type ExtensionsLogItem = {
|
||||
const channelName = 'ui-extension-logs';
|
||||
const logsNumberLimit = 1000;
|
||||
const logsRetentionTime = 1000 * 60 * 10;
|
||||
const monitoringLogger = createMonitoringLogger(channelName);
|
||||
|
||||
export class ExtensionsLog {
|
||||
private baseLabels: Labels | undefined;
|
||||
@@ -39,11 +40,15 @@ export class ExtensionsLog {
|
||||
}
|
||||
|
||||
warning(message: string, labels?: Labels): void {
|
||||
monitoringLogger.logWarning(message, { ...this.baseLabels, ...labels });
|
||||
config.buildInfo.env === 'development' && console.warn(message, { ...this.baseLabels, ...labels });
|
||||
this.log(LogLevel.warning, message, labels);
|
||||
}
|
||||
|
||||
error(message: string, labels?: Labels): void {
|
||||
// TODO: If Faro has console instrumentation, then the following will track the same error message twice
|
||||
// (first: `monitoringLogger.logError()`, second: `console.error()` which gets picked up by Faro)
|
||||
monitoringLogger.logError(new Error(message), { ...this.baseLabels, ...labels });
|
||||
console.error(message, { ...this.baseLabels, ...labels });
|
||||
this.log(LogLevel.error, message, labels);
|
||||
}
|
||||
|
||||
@@ -13,27 +13,27 @@ import { ExposedComponentsRegistry } from './registry/ExposedComponentsRegistry'
|
||||
import { PluginExtensionRegistries } from './registry/types';
|
||||
import { useLoadAppPlugins } from './useLoadAppPlugins';
|
||||
import { usePluginComponent } from './usePluginComponent';
|
||||
import { isGrafanaDevMode, wrapWithPluginContext } from './utils';
|
||||
import { isGrafanaDevMode } from './utils';
|
||||
|
||||
jest.mock('./useLoadAppPlugins');
|
||||
jest.mock('app/features/plugins/pluginSettings', () => ({
|
||||
getPluginSettings: jest.fn().mockResolvedValue({
|
||||
id: 'my-app-plugin',
|
||||
enabled: true,
|
||||
jsonData: {},
|
||||
type: 'panel',
|
||||
name: 'My App Plugin',
|
||||
module: 'app/plugins/my-app-plugin/module',
|
||||
}),
|
||||
}));
|
||||
|
||||
jest.mock('./utils', () => ({
|
||||
...jest.requireActual('./utils'),
|
||||
|
||||
// Manually set the dev mode to false
|
||||
// (to make sure that by default we are testing a production scneario)
|
||||
isGrafanaDevMode: jest.fn().mockReturnValue(false),
|
||||
wrapWithPluginContext: jest.fn().mockImplementation((_, component: React.ReactNode) => component),
|
||||
}));
|
||||
|
||||
// See: public/app/features/plugins/extensions/utils.tsx for implementation details
|
||||
jest.mock('react-use', () => ({
|
||||
...jest.requireActual('react-use'),
|
||||
useAsync: jest.fn().mockImplementation(() => ({
|
||||
error: null,
|
||||
loading: false,
|
||||
value: {
|
||||
id: 'my-app-plugin',
|
||||
},
|
||||
})),
|
||||
}));
|
||||
|
||||
jest.mock('./logs/log', () => {
|
||||
@@ -98,8 +98,6 @@ describe('usePluginComponent()', () => {
|
||||
jest.mocked(isGrafanaDevMode).mockReturnValue(false);
|
||||
resetLogMock(log);
|
||||
|
||||
jest.mocked(wrapWithPluginContext).mockClear();
|
||||
|
||||
pluginMeta = {
|
||||
id: pluginId,
|
||||
name: 'Extensions App',
|
||||
@@ -212,9 +210,11 @@ describe('usePluginComponent()', () => {
|
||||
});
|
||||
});
|
||||
|
||||
expect(wrapWithPluginContext).toHaveBeenCalledTimes(0);
|
||||
jest.mocked(isGrafanaDevMode).mockClear();
|
||||
expect(isGrafanaDevMode).toHaveBeenCalledTimes(0);
|
||||
renderHook(() => usePluginComponent(exposedComponentId), { wrapper });
|
||||
await waitFor(() => expect(wrapWithPluginContext).toHaveBeenCalledTimes(1));
|
||||
// The registryState is undefined in the first render, so the isGrafanaDevMode() is called twice
|
||||
await waitFor(() => expect(isGrafanaDevMode).toHaveBeenCalledTimes(2));
|
||||
});
|
||||
|
||||
it('should not validate the meta-info in production mode', () => {
|
||||
@@ -332,4 +332,109 @@ describe('usePluginComponent()', () => {
|
||||
expect(result.current.component).not.toBe(null);
|
||||
expect(log.warning).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should pass a read-only copy of the props (in dev mode)', async () => {
|
||||
config.buildInfo.env = 'development';
|
||||
|
||||
type Props = {
|
||||
a: {
|
||||
b: {
|
||||
c: string;
|
||||
};
|
||||
};
|
||||
override?: boolean;
|
||||
};
|
||||
|
||||
registries.exposedComponentsRegistry.register({
|
||||
pluginId,
|
||||
configs: [
|
||||
{
|
||||
...exposedComponentConfig,
|
||||
// @ts-expect-error - The registry shouldn't be used this way
|
||||
component: (props: Props) => {
|
||||
if (props.override) {
|
||||
props.a.b.c = 'baz';
|
||||
}
|
||||
|
||||
return <span>Foo</span>;
|
||||
},
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
const originalProps = {
|
||||
a: {
|
||||
b: {
|
||||
c: 'bar',
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
const { result } = renderHook(() => usePluginComponent<Props>(exposedComponentId), { wrapper });
|
||||
const Component = result.current.component;
|
||||
|
||||
// Should render normally if it doesn't mutate the props
|
||||
const rendered = render(Component && <Component {...originalProps} />);
|
||||
expect(rendered.getByText('Foo')).toBeVisible();
|
||||
|
||||
// Should throw an error if it mutates the props
|
||||
jest.spyOn(console, 'error').mockImplementation(() => {});
|
||||
expect(() => render(Component && <Component {...originalProps} override />)).toThrow(
|
||||
TypeError("'set' on proxy: trap returned falsish for property 'c'")
|
||||
);
|
||||
jest.spyOn(console, 'error').mockRestore();
|
||||
});
|
||||
|
||||
it('should pass a writable copy of the props (in production mode)', async () => {
|
||||
config.buildInfo.env = 'production';
|
||||
|
||||
type Props = {
|
||||
a: {
|
||||
b: {
|
||||
c: string;
|
||||
};
|
||||
};
|
||||
override?: boolean;
|
||||
};
|
||||
|
||||
registries.exposedComponentsRegistry.register({
|
||||
pluginId,
|
||||
configs: [
|
||||
{
|
||||
...exposedComponentConfig,
|
||||
// @ts-expect-error - The registry shouldn't be used this way
|
||||
component: (props: Props) => {
|
||||
if (props.override) {
|
||||
props.a.b.c = 'baz';
|
||||
}
|
||||
|
||||
return <span>Foo</span>;
|
||||
},
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
const originalProps = {
|
||||
a: {
|
||||
b: {
|
||||
c: 'bar',
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
const { result } = renderHook(() => usePluginComponent<Props>(exposedComponentId), { wrapper });
|
||||
const Component = result.current.component;
|
||||
|
||||
// Should render normally if it doesn't mutate the props
|
||||
const rendered = render(Component && <Component {...originalProps} />);
|
||||
expect(rendered.getByText('Foo')).toBeVisible();
|
||||
|
||||
// Should throw an error if it mutates the props
|
||||
expect(() => render(Component && <Component {...originalProps} override />)).not.toThrow();
|
||||
|
||||
// Should log a warning
|
||||
expect(log.warning).toHaveBeenCalledWith('Attempted to mutate object property "c"', {
|
||||
stack: expect.any(String),
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
import { act, render, renderHook, screen } from '@testing-library/react';
|
||||
import React from 'react';
|
||||
|
||||
import { PluginContextProvider, PluginMeta, PluginType } from '@grafana/data';
|
||||
import { config } from '@grafana/runtime';
|
||||
|
||||
import { ExtensionRegistriesProvider } from './ExtensionRegistriesContext';
|
||||
import { log } from './logs/log';
|
||||
@@ -12,19 +14,9 @@ import { ExposedComponentsRegistry } from './registry/ExposedComponentsRegistry'
|
||||
import { PluginExtensionRegistries } from './registry/types';
|
||||
import { useLoadAppPlugins } from './useLoadAppPlugins';
|
||||
import { usePluginComponents } from './usePluginComponents';
|
||||
import { isGrafanaDevMode, wrapWithPluginContext } from './utils';
|
||||
import { isGrafanaDevMode } from './utils';
|
||||
|
||||
jest.mock('./useLoadAppPlugins');
|
||||
jest.mock('app/features/plugins/pluginSettings', () => ({
|
||||
getPluginSettings: jest.fn().mockResolvedValue({
|
||||
id: 'my-app-plugin',
|
||||
enabled: true,
|
||||
jsonData: {},
|
||||
type: 'panel',
|
||||
name: 'My App Plugin',
|
||||
module: 'app/plugins/my-app-plugin/module',
|
||||
}),
|
||||
}));
|
||||
|
||||
jest.mock('./utils', () => ({
|
||||
...jest.requireActual('./utils'),
|
||||
@@ -32,7 +24,6 @@ jest.mock('./utils', () => ({
|
||||
// Manually set the dev mode to false
|
||||
// (to make sure that by default we are testing a production scneario)
|
||||
isGrafanaDevMode: jest.fn().mockReturnValue(false),
|
||||
wrapWithPluginContext: jest.fn().mockImplementation((_, component: React.ReactNode) => component),
|
||||
}));
|
||||
|
||||
jest.mock('./logs/log', () => {
|
||||
@@ -45,14 +36,28 @@ jest.mock('./logs/log', () => {
|
||||
};
|
||||
});
|
||||
|
||||
// See: public/app/features/plugins/extensions/utils.tsx for implementation details
|
||||
jest.mock('react-use', () => ({
|
||||
...jest.requireActual('react-use'),
|
||||
useAsync: jest.fn().mockImplementation(() => ({
|
||||
error: null,
|
||||
loading: false,
|
||||
value: {
|
||||
id: 'my-app-plugin',
|
||||
},
|
||||
})),
|
||||
}));
|
||||
|
||||
describe('usePluginComponents()', () => {
|
||||
let registries: PluginExtensionRegistries;
|
||||
let wrapper: ({ children }: { children: React.ReactNode }) => JSX.Element;
|
||||
let pluginMeta: PluginMeta;
|
||||
const pluginId = 'myorg-extensions-app';
|
||||
const extensionPointId = `${pluginId}/extension-point/v1`;
|
||||
const originalBuildInfoEnv = config.buildInfo.env;
|
||||
|
||||
beforeEach(() => {
|
||||
config.buildInfo.env = originalBuildInfoEnv;
|
||||
jest.mocked(isGrafanaDevMode).mockReturnValue(false);
|
||||
jest.mocked(useLoadAppPlugins).mockReturnValue({ isLoading: false });
|
||||
|
||||
@@ -64,8 +69,6 @@ describe('usePluginComponents()', () => {
|
||||
addedFunctionsRegistry: new AddedFunctionsRegistry(),
|
||||
};
|
||||
|
||||
jest.mocked(wrapWithPluginContext).mockClear();
|
||||
|
||||
pluginMeta = {
|
||||
id: pluginId,
|
||||
name: 'Extensions App',
|
||||
@@ -109,6 +112,11 @@ describe('usePluginComponents()', () => {
|
||||
);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
jest.clearAllMocks();
|
||||
jest.restoreAllMocks();
|
||||
});
|
||||
|
||||
it('should return an empty array if there are no extensions registered for the extension point', () => {
|
||||
const { result } = renderHook(
|
||||
() =>
|
||||
@@ -203,6 +211,128 @@ describe('usePluginComponents()', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('should pass a read only copy of the props to the components (in dev mode)', async () => {
|
||||
config.buildInfo.env = 'development';
|
||||
|
||||
type Props = {
|
||||
foo: {
|
||||
foo2: {
|
||||
foo3: {
|
||||
foo4: string;
|
||||
};
|
||||
};
|
||||
};
|
||||
override?: boolean;
|
||||
};
|
||||
|
||||
const originalFoo = {
|
||||
foo2: {
|
||||
foo3: {
|
||||
foo4: 'bar',
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
registries.addedComponentsRegistry.register({
|
||||
pluginId,
|
||||
configs: [
|
||||
{
|
||||
targets: extensionPointId,
|
||||
title: '1',
|
||||
description: '1',
|
||||
// @ts-ignore - The register() method is not designed to be called directly like this, and because of that it doesn't have a way to set the type of the Props
|
||||
component: ({ foo, override = false }: Props) => {
|
||||
// Trying to override the prop
|
||||
if (override) {
|
||||
const foo3 = foo.foo2.foo3;
|
||||
foo3.foo4 = 'baz';
|
||||
}
|
||||
|
||||
return <span>Foo</span>;
|
||||
},
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
// Check if it returns the components
|
||||
const { result } = renderHook(() => usePluginComponents<Props>({ extensionPointId }), { wrapper });
|
||||
expect(result.current.components.length).toBe(1);
|
||||
|
||||
const Component = result.current.components[0];
|
||||
|
||||
// Should be possible to render the component if it doesn't want to change the props
|
||||
const rendered = render(<Component foo={originalFoo} />);
|
||||
expect(rendered.getByText('Foo')).toBeVisible();
|
||||
|
||||
// Check if it throws a TypeError due to trying to change the prop
|
||||
jest.spyOn(console, 'error').mockImplementation(() => {});
|
||||
expect(() => render(<Component foo={originalFoo} override />)).toThrow(TypeError);
|
||||
|
||||
// Check if the original property hasn't been changed
|
||||
expect(originalFoo.foo2.foo3.foo4).toBe('bar');
|
||||
});
|
||||
|
||||
it('should pass a copy of the props to the components (in production mode)', async () => {
|
||||
type Props = {
|
||||
foo: {
|
||||
foo2: {
|
||||
foo3: {
|
||||
foo4: string;
|
||||
};
|
||||
};
|
||||
};
|
||||
override?: boolean;
|
||||
};
|
||||
|
||||
const originalFoo = {
|
||||
foo2: {
|
||||
foo3: {
|
||||
foo4: 'bar',
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
registries.addedComponentsRegistry.register({
|
||||
pluginId,
|
||||
configs: [
|
||||
{
|
||||
targets: extensionPointId,
|
||||
title: '1',
|
||||
description: '1',
|
||||
// @ts-ignore - The register() method is not designed to be called directly like this, and because of that it doesn't have a way to set the type of the Props
|
||||
component: ({ foo, override = false }: Props) => {
|
||||
// Trying to override the prop
|
||||
if (override) {
|
||||
const foo3 = foo.foo2.foo3;
|
||||
foo3.foo4 = 'baz';
|
||||
}
|
||||
|
||||
return <span>Foo</span>;
|
||||
},
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
// Check if it returns the components
|
||||
const { result } = renderHook(() => usePluginComponents<Props>({ extensionPointId }), { wrapper });
|
||||
expect(result.current.components.length).toBe(1);
|
||||
|
||||
const Component = result.current.components[0];
|
||||
|
||||
// Should be possible to render the component if it doesn't want to change the props
|
||||
const rendered = render(<Component foo={originalFoo} />);
|
||||
expect(rendered.getByText('Foo')).toBeVisible();
|
||||
|
||||
// Should also render the component if it wants to change the props
|
||||
expect(() => render(<Component foo={originalFoo} override />)).not.toThrow();
|
||||
expect(log.warning).toHaveBeenCalledWith(`Attempted to mutate object property "foo4"`, {
|
||||
stack: expect.any(String),
|
||||
});
|
||||
|
||||
// Check if the original property hasn't been changed
|
||||
expect(originalFoo.foo2.foo3.foo4).toBe('bar');
|
||||
});
|
||||
|
||||
it('should dynamically update the extensions registered for a certain extension point', () => {
|
||||
let { result, rerender } = renderHook(() => usePluginComponents({ extensionPointId }), { wrapper });
|
||||
|
||||
|
||||
@@ -38,10 +38,12 @@ export function usePluginComponents<Props extends object = {}>({
|
||||
extensionPointId,
|
||||
});
|
||||
|
||||
// Only log error for an invalid `extensionPointId` in DEV mode
|
||||
if (enableRestrictions && !isExtensionPointIdValid({ extensionPointId, pluginId })) {
|
||||
pointLog.error(errors.INVALID_EXTENSION_POINT_ID);
|
||||
}
|
||||
|
||||
// Don't show extensions if the extension-point misses meta info (plugin.json) in DEV mode
|
||||
if (enableRestrictions && isExtensionPointMetaInfoMissing(extensionPointId, pluginContext)) {
|
||||
pointLog.error(errors.EXTENSION_POINT_META_INFO_MISSING);
|
||||
return {
|
||||
@@ -85,12 +87,12 @@ export function usePluginComponents<Props extends object = {}>({
|
||||
}, [extensionPointId, limitPerPlugin, pluginContext, registryState, isLoadingAppPlugins]);
|
||||
}
|
||||
|
||||
// exported so it can be used in tests
|
||||
export function createComponentWithMeta<Props extends JSX.IntrinsicAttributes>(
|
||||
registryItem: AddedComponentRegistryItem<Props>,
|
||||
extensionPointId: string
|
||||
): ComponentTypeWithExtensionMeta<Props> {
|
||||
const { component: Component, ...config } = registryItem;
|
||||
|
||||
function ComponentWithMeta(props: Props) {
|
||||
return <Component {...props} />;
|
||||
}
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { render, screen } from '@testing-library/react';
|
||||
import { act, render, screen } from '@testing-library/react';
|
||||
import { type Unsubscribable } from 'rxjs';
|
||||
|
||||
import { dateTime, usePluginContext, PluginLoadingStrategy } from '@grafana/data';
|
||||
@@ -7,6 +7,7 @@ import appEvents from 'app/core/app_events';
|
||||
import { ShowModalReactEvent } from 'app/types/events';
|
||||
|
||||
import { log } from './logs/log';
|
||||
import { resetLogMock } from './logs/testUtils';
|
||||
import {
|
||||
deepFreeze,
|
||||
handleErrorsInFn,
|
||||
@@ -19,6 +20,10 @@ import {
|
||||
getAppPluginIdFromExposedComponentId,
|
||||
getAppPluginDependencies,
|
||||
getExtensionPointPluginMeta,
|
||||
getMutationObserverProxy,
|
||||
readOnlyCopy,
|
||||
isReadOnlyProxy,
|
||||
isMutationObserverProxy,
|
||||
} from './utils';
|
||||
|
||||
jest.mock('app/features/plugins/pluginSettings', () => ({
|
||||
@@ -26,6 +31,16 @@ jest.mock('app/features/plugins/pluginSettings', () => ({
|
||||
getPluginSettings: () => Promise.resolve({ info: { version: '1.0.0' } }),
|
||||
}));
|
||||
|
||||
jest.mock('./logs/log', () => {
|
||||
const { createLogMock } = jest.requireActual('./logs/testUtils');
|
||||
const original = jest.requireActual('./logs/log');
|
||||
|
||||
return {
|
||||
...original,
|
||||
log: createLogMock(),
|
||||
};
|
||||
});
|
||||
|
||||
describe('Plugin Extensions / Utils', () => {
|
||||
describe('deepFreeze()', () => {
|
||||
test('should not fail when called with primitive values', () => {
|
||||
@@ -228,7 +243,7 @@ describe('Plugin Extensions / Utils', () => {
|
||||
|
||||
expect(() => {
|
||||
proxy.a = 'b';
|
||||
}).toThrowError(TypeError);
|
||||
}).toThrow(TypeError);
|
||||
});
|
||||
|
||||
it('should not be possible to modify values in proxied array', () => {
|
||||
@@ -236,7 +251,7 @@ describe('Plugin Extensions / Utils', () => {
|
||||
|
||||
expect(() => {
|
||||
proxy[0] = 2;
|
||||
}).toThrowError(TypeError);
|
||||
}).toThrow(TypeError);
|
||||
});
|
||||
|
||||
it('should not be possible to modify nested objects in proxied object', () => {
|
||||
@@ -249,7 +264,58 @@ describe('Plugin Extensions / Utils', () => {
|
||||
|
||||
expect(() => {
|
||||
proxy.a.c = 'testing';
|
||||
}).toThrowError(TypeError);
|
||||
}).toThrow(TypeError);
|
||||
});
|
||||
|
||||
// This is to record what we are not able to do currently.
|
||||
// (Due to Proxy.get() invariants limitations: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/get#invariants)
|
||||
it('should not work with any objects that are already frozen', () => {
|
||||
const obj = {
|
||||
a: {
|
||||
b: {
|
||||
c: {
|
||||
d: 'd',
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
Object.freeze(obj);
|
||||
Object.freeze(obj.a);
|
||||
Object.freeze(obj.a.b);
|
||||
|
||||
const proxy = getReadOnlyProxy(obj);
|
||||
|
||||
expect(() => {
|
||||
proxy.a.b.c.d = 'testing';
|
||||
}).toThrow(
|
||||
"'get' on proxy: property 'a' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '#<Object>' but got '#<Object>')"
|
||||
);
|
||||
|
||||
expect(obj.a.b.c.d).toBe('d');
|
||||
});
|
||||
|
||||
it('should throw a TypeError if a proxied object is trying to be frozen', () => {
|
||||
const obj = {
|
||||
a: {
|
||||
b: {
|
||||
c: {
|
||||
d: 'd',
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
const proxy = getReadOnlyProxy(obj);
|
||||
|
||||
expect(() => Object.freeze(proxy)).toThrow(TypeError);
|
||||
expect(() => Object.freeze(proxy.a)).toThrow(TypeError);
|
||||
expect(() => Object.freeze(proxy.a.b)).toThrow(TypeError);
|
||||
|
||||
// Check if the original object is not frozen
|
||||
expect(Object.isFrozen(obj)).toBe(false);
|
||||
expect(Object.isFrozen(obj.a)).toBe(false);
|
||||
expect(Object.isFrozen(obj.a.b)).toBe(false);
|
||||
});
|
||||
|
||||
it('should not be possible to modify nested arrays in proxied object', () => {
|
||||
@@ -262,7 +328,7 @@ describe('Plugin Extensions / Utils', () => {
|
||||
|
||||
expect(() => {
|
||||
proxy.a.c[0] = 'testing';
|
||||
}).toThrowError(TypeError);
|
||||
}).toThrow(TypeError);
|
||||
});
|
||||
|
||||
it('should be possible to modify source object', () => {
|
||||
@@ -320,6 +386,178 @@ describe('Plugin Extensions / Utils', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('getMutationObserverProxy()', () => {
|
||||
it('should not be possible to modify values in proxied object, but logs a warning', () => {
|
||||
const proxy = getMutationObserverProxy({ a: 'a' });
|
||||
|
||||
expect(() => {
|
||||
proxy.a = 'b';
|
||||
}).not.toThrow();
|
||||
|
||||
expect(log.warning).toHaveBeenCalledWith(`Attempted to mutate object property "a"`, {
|
||||
stack: expect.any(String),
|
||||
});
|
||||
|
||||
expect(proxy.a).toBe('b');
|
||||
});
|
||||
|
||||
it('should be possible to set new values, but logs a warning', () => {
|
||||
const obj: { a: string; b?: string } = { a: 'a' };
|
||||
const proxy = getMutationObserverProxy(obj);
|
||||
|
||||
expect(() => {
|
||||
Object.defineProperty(proxy, 'b', {
|
||||
value: 'b',
|
||||
writable: false,
|
||||
});
|
||||
}).not.toThrow();
|
||||
|
||||
expect(log.warning).toHaveBeenCalledWith(`Attempted to define object property "b"`, {
|
||||
stack: expect.any(String),
|
||||
});
|
||||
|
||||
expect(proxy.b).toBe('b');
|
||||
});
|
||||
|
||||
it('should be possible to delete properties, but logs a warning', () => {
|
||||
const proxy = getMutationObserverProxy({
|
||||
a: {
|
||||
c: 'c',
|
||||
},
|
||||
b: 'b',
|
||||
});
|
||||
|
||||
expect(() => {
|
||||
// @ts-ignore - This is to test the logic
|
||||
delete proxy.a.c;
|
||||
}).not.toThrow();
|
||||
|
||||
expect(log.warning).toHaveBeenCalledWith(`Attempted to delete object property "c"`, {
|
||||
stack: expect.any(String),
|
||||
});
|
||||
|
||||
expect(proxy.a.c).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('readOnlyCopy()', () => {
|
||||
const originalEnv = config.buildInfo.env;
|
||||
|
||||
beforeEach(() => {
|
||||
jest.spyOn(console, 'warn').mockImplementation();
|
||||
config.featureToggles.extensionsReadOnlyProxy = false;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
config.buildInfo.env = originalEnv;
|
||||
jest.mocked(console.warn).mockClear();
|
||||
});
|
||||
|
||||
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);
|
||||
});
|
||||
|
||||
it('should return a read-only proxy of a 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);
|
||||
|
||||
expect(copy).not.toBe(obj);
|
||||
expect(copy.a).toBe('a');
|
||||
expect(isReadOnlyProxy(copy)).toBe(true);
|
||||
expect(() => {
|
||||
copy.a = 'b';
|
||||
}).toThrow(TypeError);
|
||||
|
||||
// Also test that we can handle frozen objects
|
||||
// (This is not possible with getReadOnlyProxy, as it throws an error when the object is already frozen)
|
||||
const obj2 = {
|
||||
a: {
|
||||
b: {
|
||||
c: {
|
||||
d: 'd',
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
Object.freeze(obj2);
|
||||
Object.freeze(obj2.a);
|
||||
Object.freeze(obj2.a.b);
|
||||
|
||||
const copy2 = readOnlyCopy(obj2);
|
||||
|
||||
expect(() => {
|
||||
copy2.a.b.c.d = 'testing';
|
||||
}).toThrow("'set' on proxy: trap returned falsish for property 'd'");
|
||||
|
||||
expect(copy2.a.b.c.d).toBe('d');
|
||||
});
|
||||
|
||||
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);
|
||||
|
||||
expect(copy).not.toBe(obj);
|
||||
expect(copy.a).toBe('a');
|
||||
expect(isMutationObserverProxy(copy)).toBe(true);
|
||||
expect(() => {
|
||||
copy.a = 'b';
|
||||
}).not.toThrow();
|
||||
|
||||
expect(log.warning).toHaveBeenCalledWith(`Attempted to mutate object property "a"`, {
|
||||
stack: expect.any(String),
|
||||
});
|
||||
|
||||
expect(copy.a).toBe('b');
|
||||
});
|
||||
|
||||
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);
|
||||
|
||||
expect(() => {
|
||||
Object.freeze(copy);
|
||||
Object.freeze(copy.b);
|
||||
}).not.toThrow();
|
||||
|
||||
expect(Object.isFrozen(copy)).toBe(true);
|
||||
expect(Object.isFrozen(copy.b)).toBe(true);
|
||||
expect(copy.b).toEqual({ c: 'c' });
|
||||
|
||||
expect(log.warning).toHaveBeenCalledWith(`Attempted to define object property "a"`, {
|
||||
stack: expect.any(String),
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('createOpenModalFunction()', () => {
|
||||
let renderModalSubscription: Unsubscribable | undefined;
|
||||
|
||||
@@ -417,15 +655,24 @@ describe('Plugin Extensions / Utils', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('wrapExtensionComponentWithContext()', () => {
|
||||
describe('wrapWithPluginContext()', () => {
|
||||
type ExampleComponentProps = {
|
||||
audience?: string;
|
||||
a: {
|
||||
b: {
|
||||
c: string;
|
||||
};
|
||||
};
|
||||
override?: boolean;
|
||||
};
|
||||
|
||||
const ExampleComponent = (props: ExampleComponentProps) => {
|
||||
const pluginContext = usePluginContext();
|
||||
|
||||
const audience = props.audience || 'Grafana';
|
||||
const audience = props.a.b.c || 'Grafana';
|
||||
|
||||
if (props.override) {
|
||||
props.a.b.c = 'OVERRIDE';
|
||||
}
|
||||
|
||||
return (
|
||||
<div>
|
||||
@@ -434,11 +681,15 @@ describe('Plugin Extensions / Utils', () => {
|
||||
);
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
resetLogMock(log);
|
||||
});
|
||||
|
||||
it('should make the plugin context available for the wrapped component', async () => {
|
||||
const pluginId = 'grafana-worldmap-panel';
|
||||
const Component = wrapWithPluginContext(pluginId, ExampleComponent, log);
|
||||
|
||||
render(<Component />);
|
||||
render(<Component a={{ b: { c: 'Grafana' } }} />);
|
||||
|
||||
expect(await screen.findByText('Hello Grafana!')).toBeVisible();
|
||||
expect(screen.getByText('Version: 1.0.0')).toBeVisible();
|
||||
@@ -448,11 +699,52 @@ describe('Plugin Extensions / Utils', () => {
|
||||
const pluginId = 'grafana-worldmap-panel';
|
||||
const Component = wrapWithPluginContext(pluginId, ExampleComponent, log);
|
||||
|
||||
render(<Component audience="folks" />);
|
||||
render(<Component a={{ b: { c: 'Grafana' } }} />);
|
||||
|
||||
expect(await screen.findByText('Hello folks!')).toBeVisible();
|
||||
expect(await screen.findByText('Hello Grafana!')).toBeVisible();
|
||||
expect(screen.getByText('Version: 1.0.0')).toBeVisible();
|
||||
});
|
||||
|
||||
it('should not be possible to mutate the props in development mode, and it also throws an error', async () => {
|
||||
config.buildInfo.env = 'development';
|
||||
const pluginId = 'grafana-worldmap-panel';
|
||||
const Component = wrapWithPluginContext(pluginId, ExampleComponent, log);
|
||||
const props = { a: { b: { c: 'Grafana' } } };
|
||||
|
||||
jest.spyOn(console, 'error').mockImplementation();
|
||||
|
||||
await expect(async () => {
|
||||
await act(async () => {
|
||||
render(<Component {...props} override />);
|
||||
});
|
||||
}).rejects.toThrow(`'set' on proxy: trap returned falsish for property 'c'`);
|
||||
|
||||
// Logs an error
|
||||
expect(console.error).toHaveBeenCalledWith(expect.any(String));
|
||||
|
||||
// Not able to mutate the props in development mode
|
||||
expect(props.a.b.c).toBe('Grafana');
|
||||
});
|
||||
|
||||
it('should not be possible to mutate the props in production mode either, but it logs a warning', async () => {
|
||||
config.buildInfo.env = 'production';
|
||||
const pluginId = 'grafana-worldmap-panel';
|
||||
const Component = wrapWithPluginContext(pluginId, ExampleComponent, log);
|
||||
const props = { a: { b: { c: 'Grafana' } } };
|
||||
|
||||
render(<Component {...props} override />);
|
||||
|
||||
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"`, {
|
||||
stack: expect.any(String),
|
||||
});
|
||||
|
||||
// Not able to mutate the props in production mode either
|
||||
expect(props.a.b.c).toBe('Grafana');
|
||||
});
|
||||
});
|
||||
|
||||
describe('getAppPluginConfigs()', () => {
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { css } from '@emotion/css';
|
||||
import { isArray, isObject } from 'lodash';
|
||||
import { cloneDeep, isArray, isObject } from 'lodash';
|
||||
import * as React from 'react';
|
||||
import { useAsync } from 'react-use';
|
||||
|
||||
@@ -85,7 +85,7 @@ export const wrapWithPluginContext = <T,>(pluginId: string, Component: React.Com
|
||||
|
||||
return (
|
||||
<PluginContextProvider meta={pluginMeta}>
|
||||
<Component {...props} />
|
||||
<Component {...readOnlyCopy(props, log)} />
|
||||
</PluginContextProvider>
|
||||
);
|
||||
};
|
||||
@@ -163,7 +163,15 @@ export function generateExtensionId(pluginId: string, extensionPointId: string,
|
||||
.toString();
|
||||
}
|
||||
|
||||
const _isProxy = Symbol('isReadOnlyProxy');
|
||||
const _isReadOnlyProxy = Symbol('isReadOnlyProxy');
|
||||
const _isMutationObserverProxy = Symbol('isMutationObserverProxy');
|
||||
|
||||
export class ReadOnlyProxyError extends Error {
|
||||
constructor(message?: string) {
|
||||
super(message ?? 'Mutating a read-only proxy object');
|
||||
this.name = 'ReadOnlyProxyError';
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns a proxy that wraps the given object in a way that makes it read only.
|
||||
@@ -185,7 +193,7 @@ export function getReadOnlyProxy<T extends object>(obj: T): T {
|
||||
isExtensible: () => false,
|
||||
set: () => false,
|
||||
get(target, prop, receiver) {
|
||||
if (prop === _isProxy) {
|
||||
if (prop === _isReadOnlyProxy) {
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -210,12 +218,102 @@ export function getReadOnlyProxy<T extends object>(obj: T): T {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns a proxy that logs any attempted mutation to the original object.
|
||||
*
|
||||
* @param obj The object to observe
|
||||
* @returns A new proxy object that logs any attempted mutation to the original object
|
||||
*/
|
||||
export function getMutationObserverProxy<T extends object>(obj: T, _log: ExtensionsLog = log): T {
|
||||
if (!obj || typeof obj !== 'object' || isMutationObserverProxy(obj)) {
|
||||
return obj;
|
||||
}
|
||||
|
||||
const cache = new WeakMap();
|
||||
|
||||
return new Proxy(obj, {
|
||||
deleteProperty(target, prop) {
|
||||
_log.warning(`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)}"`, {
|
||||
stack: new Error().stack ?? '',
|
||||
});
|
||||
Reflect.defineProperty(target, prop, descriptor);
|
||||
return true;
|
||||
},
|
||||
set(target, prop, newValue) {
|
||||
_log.warning(`Attempted to mutate object property "${String(prop)}"`, {
|
||||
stack: new Error().stack ?? '',
|
||||
});
|
||||
Reflect.set(target, prop, newValue);
|
||||
return true;
|
||||
},
|
||||
get(target, prop, receiver) {
|
||||
if (prop === _isMutationObserverProxy) {
|
||||
return true;
|
||||
}
|
||||
|
||||
const value = Reflect.get(target, prop, receiver);
|
||||
|
||||
// Return read-only properties as-is to avoid proxy invariant violations
|
||||
const descriptor = Reflect.getOwnPropertyDescriptor(target, prop);
|
||||
if (descriptor && !descriptor.configurable && !descriptor.writable) {
|
||||
return value;
|
||||
}
|
||||
|
||||
// This will create a clone of the date time object
|
||||
// instead of creating a proxy because the underlying
|
||||
// momentjs object needs to be able to mutate itself.
|
||||
if (isDateTime(value)) {
|
||||
return dateTime(value);
|
||||
}
|
||||
|
||||
if (isObject(value) || isArray(value)) {
|
||||
if (!cache.has(value)) {
|
||||
cache.set(value, getMutationObserverProxy(value, _log));
|
||||
}
|
||||
return cache.get(value);
|
||||
}
|
||||
|
||||
return value;
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
export function readOnlyCopy<T>(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);
|
||||
}
|
||||
|
||||
// In dev mode: we return a read-only proxy (throws errors for any mutation), but with a deep-cloned version of the original object (so no interference with other call-sites)
|
||||
if (isGrafanaDevMode()) {
|
||||
return getReadOnlyProxy(cloneDeep(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);
|
||||
}
|
||||
|
||||
function isRecord(value: unknown): value is Record<string | number | symbol, unknown> {
|
||||
return typeof value === 'object' && value !== null;
|
||||
}
|
||||
|
||||
export function isReadOnlyProxy(value: unknown): boolean {
|
||||
return isRecord(value) && value[_isProxy] === true;
|
||||
return isRecord(value) && value[_isReadOnlyProxy] === true;
|
||||
}
|
||||
|
||||
export function isMutationObserverProxy(value: unknown): boolean {
|
||||
return isRecord(value) && value[_isMutationObserverProxy] === true;
|
||||
}
|
||||
|
||||
export function createAddedLinkConfig<T extends object>(
|
||||
|
||||
Reference in New Issue
Block a user