PluginExtensions: Make context read only with a proxy instead of object freeze (#67781)

* quick poc on how we could use proxies instead of object freeze for the extension context.

* wip

* wip

* added tests for readOnlyProxy.

* Changed so we use the read only proxy for the context instead of the frozen object.

* updated names according to feedback.
This commit is contained in:
Marcus Andersson 2023-05-05 15:50:40 +02:00 committed by GitHub
parent 29e4df6a33
commit b8336fe910
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 154 additions and 10 deletions

View File

@ -2,6 +2,7 @@ import { PluginExtensionLinkConfig, PluginExtensionTypes } from '@grafana/data';
import { createPluginExtensionRegistry } from './createPluginExtensionRegistry';
import { getPluginExtensions } from './getPluginExtensions';
import { isReadOnlyProxy } from './utils';
import { assertPluginExtensionLink } from './validators';
describe('getPluginExtensions()', () => {
@ -96,19 +97,19 @@ describe('getPluginExtensions()', () => {
expect(link2.configure).toHaveBeenCalledTimes(1);
expect(extensions).toHaveLength(0);
});
test('should pass a frozen copy of the context to the configure() function', () => {
test('should pass a read only context to the configure() function', () => {
const context = { title: 'New title from the context!' };
const registry = createPluginExtensionRegistry([{ pluginId, extensionConfigs: [link2] }]);
const { extensions } = getPluginExtensions({ registry, context, extensionPointId: extensionPoint2 });
const [extension] = extensions;
const frozenContext = (link2.configure as jest.Mock).mock.calls[0][0];
const readOnlyContext = (link2.configure as jest.Mock).mock.calls[0][0];
assertPluginExtensionLink(extension);
expect(link2.configure).toHaveBeenCalledTimes(1);
expect(Object.isFrozen(frozenContext)).toBe(true);
expect(isReadOnlyProxy(readOnlyContext)).toBe(true);
expect(() => {
frozenContext.title = 'New title';
readOnlyContext.title = 'New title';
}).toThrow();
expect(context.title).toBe('New title from the context!');
});
@ -247,7 +248,7 @@ describe('getPluginExtensions()', () => {
expect(global.console.warn).toHaveBeenCalledWith('[Plugin Extensions] Something went wrong!');
});
test('should pass a frozen copy of the context to the onClick() function', () => {
test('should pass a read only context to the onClick() function', () => {
const context = { title: 'New title from the context!' };
link2.path = undefined;
@ -263,13 +264,13 @@ describe('getPluginExtensions()', () => {
const helpers = (link2.onClick as jest.Mock).mock.calls[0][1];
expect(link2.configure).toHaveBeenCalledTimes(1);
expect(Object.isFrozen(helpers.context)).toBe(true);
expect(isReadOnlyProxy(helpers.context)).toBe(true);
expect(() => {
helpers.context.title = 'New title';
}).toThrow();
});
test('should should not freeze the original context', () => {
test('should should not make original context read only', () => {
const context = {
title: 'New title from the context!',
nested: { title: 'title' },

View File

@ -6,7 +6,13 @@ import {
} from '@grafana/data';
import type { PluginExtensionRegistry } from './types';
import { isPluginExtensionLinkConfig, deepFreeze, logWarning, generateExtensionId, getEventHelpers } from './utils';
import {
isPluginExtensionLinkConfig,
getReadOnlyProxy,
logWarning,
generateExtensionId,
getEventHelpers,
} from './utils';
import { assertIsNotPromise, assertLinkPathIsValid, assertStringProps, isPromise } from './validators';
type GetExtensions = ({
@ -21,7 +27,7 @@ type GetExtensions = ({
// Returns with a list of plugin extensions for the given extension point
export const getPluginExtensions: GetExtensions = ({ context, extensionPointId, registry }) => {
const frozenContext = context ? deepFreeze(context) : {};
const frozenContext = context ? getReadOnlyProxy(context) : {};
const registryItems = registry[extensionPointId] ?? [];
// We don't return the extensions separated by type, because in that case it would be much harder to define a sort-order for them.
const extensions: PluginExtension[] = [];

View File

@ -1,6 +1,6 @@
import { PluginExtensionLinkConfig, PluginExtensionTypes } from '@grafana/data';
import { deepFreeze, isPluginExtensionLinkConfig, handleErrorsInFn } from './utils';
import { deepFreeze, isPluginExtensionLinkConfig, handleErrorsInFn, getReadOnlyProxy } from './utils';
describe('Plugin Extensions / Utils', () => {
describe('deepFreeze()', () => {
@ -219,4 +219,92 @@ describe('Plugin Extensions / Utils', () => {
}).not.toThrow();
});
});
describe('getReadOnlyProxy()', () => {
it('should not be possible to modify values in proxied object', () => {
const proxy = getReadOnlyProxy({ a: 'a' });
expect(() => {
proxy.a = 'b';
}).toThrowError(TypeError);
});
it('should not be possible to modify values in proxied array', () => {
const proxy = getReadOnlyProxy([1, 2, 3]);
expect(() => {
proxy[0] = 2;
}).toThrowError(TypeError);
});
it('should not be possible to modify nested objects in proxied object', () => {
const proxy = getReadOnlyProxy({
a: {
c: 'c',
},
b: 'b',
});
expect(() => {
proxy.a.c = 'testing';
}).toThrowError(TypeError);
});
it('should not be possible to modify nested arrays in proxied object', () => {
const proxy = getReadOnlyProxy({
a: {
c: ['c', 'd'],
},
b: 'b',
});
expect(() => {
proxy.a.c[0] = 'testing';
}).toThrowError(TypeError);
});
it('should be possible to modify source object', () => {
const source = { a: 'b' };
getReadOnlyProxy(source);
source.a = 'c';
expect(source.a).toBe('c');
});
it('should be possible to modify source array', () => {
const source = ['a', 'b'];
getReadOnlyProxy(source);
source[0] = 'c';
expect(source[0]).toBe('c');
});
it('should be possible to modify nedsted objects in source object', () => {
const source = { a: { b: 'c' } };
getReadOnlyProxy(source);
source.a.b = 'd';
expect(source.a.b).toBe('d');
});
it('should be possible to modify nedsted arrays in source object', () => {
const source = { a: { b: ['c', 'd'] } };
getReadOnlyProxy(source);
source.a.b[0] = 'd';
expect(source.a.b[0]).toBe('d');
});
it('should be possible to call functions in proxied object', () => {
const proxy = getReadOnlyProxy({
a: () => 'testing',
});
expect(proxy.a()).toBe('testing');
});
});
});

View File

@ -1,3 +1,4 @@
import { isArray, isObject } from 'lodash';
import React from 'react';
import {
@ -110,3 +111,51 @@ export function generateExtensionId(pluginId: string, extensionConfig: PluginExt
.reduce((s, c) => (Math.imul(31, s) + c.charCodeAt(0)) | 0, 0)
.toString();
}
const _isProxy = Symbol('isReadOnlyProxy');
/**
* Returns a proxy that wraps the given object in a way that makes it read only.
* If you try to modify the object a TypeError exception will be thrown.
*
* @param obj The object to make read only
* @returns A new read only object, does not modify the original object
*/
export function getReadOnlyProxy<T extends object>(obj: T): T {
if (!obj || typeof obj !== 'object' || isReadOnlyProxy(obj)) {
return obj;
}
const cache = new WeakMap();
return new Proxy(obj, {
defineProperty: () => false,
deleteProperty: () => false,
isExtensible: () => false,
set: () => false,
get(target, prop, receiver) {
if (prop === _isProxy) {
return true;
}
const value = Reflect.get(target, prop, receiver);
if (isObject(value) || isArray(value)) {
if (!cache.has(value)) {
cache.set(value, getReadOnlyProxy(value));
}
return cache.get(value);
}
return value;
},
});
}
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;
}