From 6cc6dcabe16c4e3dedba6b501ae045494eee57cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Fri, 18 Jul 2025 12:14:27 +0200 Subject: [PATCH] Chore: removes props mutation (#2056) While investigating some potential mutations [here](https://ops.grafana-ops.net/d/83f4951f-2ef3-4260-91a0-39a031992b75/getmutationobserverproxy-logs) I was able to find these mutating [lines](https://github.com/grafana/grafana-zabbix/blob/main/src/datasource/components/ConfigEditor.tsx#L55-L57) Although this works right now, this might not work in future Grafana versions. This PR makes sure we don't mutate the props. I haven't been able to test this manually so I could use some help to make sure the plugin works as expected. --------- Co-authored-by: ivanahuckova --- .changeset/metal-bottles-sin.md | 5 + .../components/ConfigEditor.test.tsx | 102 ++++++++++++++++++ src/datasource/components/ConfigEditor.tsx | 9 +- src/test-setup/jest-setup.js | 3 + 4 files changed, 115 insertions(+), 4 deletions(-) create mode 100644 .changeset/metal-bottles-sin.md create mode 100644 src/datasource/components/ConfigEditor.test.tsx diff --git a/.changeset/metal-bottles-sin.md b/.changeset/metal-bottles-sin.md new file mode 100644 index 0000000..36da3a9 --- /dev/null +++ b/.changeset/metal-bottles-sin.md @@ -0,0 +1,5 @@ +--- +'grafana-zabbix': patch +--- + +Fix: Remove props mutation in config editor diff --git a/src/datasource/components/ConfigEditor.test.tsx b/src/datasource/components/ConfigEditor.test.tsx new file mode 100644 index 0000000..e9ac9a4 --- /dev/null +++ b/src/datasource/components/ConfigEditor.test.tsx @@ -0,0 +1,102 @@ +import React from 'react'; +import { render } from '@testing-library/react'; +import { ConfigEditor, Props } from './ConfigEditor'; + +jest.mock('@grafana/runtime', () => ({ + ...jest.requireActual('@grafana/runtime'), + config: {}, +})); + +describe('ConfigEditor', () => { + describe('on initial render', () => { + it('should not mutate the options object', () => { + const options = Object.freeze({ ...getDefaultOptions() }); // freezing the options to prevent mutations + Object.freeze(options.jsonData); + Object.freeze(options.secureJsonData); + Object.freeze(options.secureJsonFields); + const onOptionsChangeSpy = jest.fn(); + + expect(() => render()).not.toThrow(); + }); + + it('should call onOptionsChange with the correct values', () => { + const options = Object.freeze({ ...getDefaultOptions() }); // freezing the options to prevent mutations + Object.freeze(options.jsonData); + Object.freeze(options.secureJsonData); + Object.freeze(options.secureJsonFields); + const onOptionsChangeSpy = jest.fn(); + + expect(() => render()).not.toThrow(); + expect(onOptionsChangeSpy).toBeCalledTimes(1); + expect(onOptionsChangeSpy).toHaveBeenCalledWith({ + ...getDefaultOptions(), + jsonData: { + ...getDefaultOptions().jsonData, + authType: 'userLogin', + timeout: undefined, + password: undefined, // password should be missing from jsonData + }, + secureJsonData: { + ...getDefaultOptions().secureJsonData, + password: 'a password', // password should be present in secureJsonData + }, + }); + }); + + it('should not update password in secureJsonData if the field already exists in secureJsonFields', () => { + const options = Object.freeze({ ...getDefaultOptions(), secureJsonFields: { password: true } }); // freezing the options to prevent mutations + Object.freeze(options.jsonData); + Object.freeze(options.secureJsonData); + Object.freeze(options.secureJsonFields); + const onOptionsChangeSpy = jest.fn(); + + expect(() => render()).not.toThrow(); + expect(onOptionsChangeSpy).toBeCalledTimes(1); + expect(onOptionsChangeSpy).toHaveBeenCalledWith({ + ...getDefaultOptions(), + jsonData: { + ...getDefaultOptions().jsonData, + authType: 'userLogin', + timeout: undefined, + password: undefined, // password should be missing from jsonData + }, + secureJsonData: {}, // password should be missing from secureJsonData + secureJsonFields: { ...getDefaultOptions().secureJsonFields, password: true }, + }); + }); + }); +}); + +function getDefaultOptions(): Props['options'] { + return { + id: 1, + orgId: 1, + uid: '', + name: '', + typeLogoUrl: '', + type: '', + typeName: '', + access: '', + url: '', + user: '', + database: '', + basicAuth: false, + basicAuthUser: '', + isDefault: false, + jsonData: { + cacheTTL: '', + dbConnectionEnable: false, + disableDataAlignment: false, + disableReadOnlyUsersAck: false, + trends: false, + trendsFrom: '', + trendsRange: '', + username: '', + password: 'a password', + }, + readOnly: false, + secureJsonData: {}, + secureJsonFields: {}, + withCredentials: false, + }; +} diff --git a/src/datasource/components/ConfigEditor.tsx b/src/datasource/components/ConfigEditor.tsx index 2d79810..200ee3a 100644 --- a/src/datasource/components/ConfigEditor.tsx +++ b/src/datasource/components/ConfigEditor.tsx @@ -50,11 +50,11 @@ export const ConfigEditor = (props: Props) => { // Set secureJsonFields.password to password and then remove it from config const { password, ...restJsonData } = jsonData; + + // Create new secureJsonData object + const newSecureJsonData = { ...options.secureJsonData }; if (!secureJsonFields?.password) { - if (!options.secureJsonData) { - options.secureJsonData = {}; - } - options.secureJsonData.password = password; + newSecureJsonData.password = password; } onOptionsChange({ @@ -69,6 +69,7 @@ export const ConfigEditor = (props: Props) => { disableDataAlignment: false, ...restJsonData, }, + secureJsonData: { ...newSecureJsonData }, }); if (options.jsonData.dbConnectionEnable) { diff --git a/src/test-setup/jest-setup.js b/src/test-setup/jest-setup.js index 1064ede..a8a964f 100644 --- a/src/test-setup/jest-setup.js +++ b/src/test-setup/jest-setup.js @@ -1,4 +1,7 @@ import { PanelCtrl, MetricsPanelCtrl } from './panelStub'; +import { TextEncoder, TextDecoder } from 'util'; + +Object.assign(global, { TextDecoder, TextEncoder }); jest.mock( 'grafana/app/features/templating/template_srv',