From 2fd6ed5cf81c3357ee7b7da0e737e4903c21e561 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Farkas?= Date: Tue, 6 Apr 2021 11:34:05 +0200 Subject: [PATCH] Datasource config: correctly remove single custom http header (#32445) * grafana-ui: data-source-settings: fix remove-last-http-header case * adjust code to not-mutate props-data * improved tests and testability * datasource: custom-http-headers: cleanup secure-values too --- .../CustomHeadersSettings.test.tsx | 92 ++++++++++++++----- .../CustomHeadersSettings.tsx | 40 +++++--- 2 files changed, 96 insertions(+), 36 deletions(-) diff --git a/packages/grafana-ui/src/components/DataSourceSettings/CustomHeadersSettings.test.tsx b/packages/grafana-ui/src/components/DataSourceSettings/CustomHeadersSettings.test.tsx index 4dacf41b895..11b31f6fa20 100644 --- a/packages/grafana-ui/src/components/DataSourceSettings/CustomHeadersSettings.test.tsx +++ b/packages/grafana-ui/src/components/DataSourceSettings/CustomHeadersSettings.test.tsx @@ -1,9 +1,10 @@ import React from 'react'; -import { mount } from 'enzyme'; import { CustomHeadersSettings, Props } from './CustomHeadersSettings'; -import { Button } from '../Button'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; const setup = (propOverrides?: object) => { + const onChange = jest.fn(); const props: Props = { dataSourceConfig: { id: 4, @@ -33,29 +34,44 @@ const setup = (propOverrides?: object) => { secureJsonFields: {}, readOnly: true, }, - onChange: jest.fn(), + onChange, ...propOverrides, }; - return mount(); + render(); + return { onChange }; }; +function assertRowCount(configuredInputCount: number, passwordInputCount: number) { + const inputs = screen.queryAllByPlaceholderText('X-Custom-Header'); + const passwordInputs = screen.queryAllByPlaceholderText('Header Value'); + const configuredInputs = screen.queryAllByDisplayValue('configured'); + expect(inputs.length).toBe(passwordInputs.length + configuredInputs.length); + + expect(passwordInputs).toHaveLength(passwordInputCount); + expect(configuredInputs).toHaveLength(configuredInputCount); +} + describe('Render', () => { it('should add a new header', () => { - const wrapper = setup(); - const addButton = wrapper.find('Button').at(0); - addButton.simulate('click', { preventDefault: () => {} }); - expect(wrapper.find('FormField').exists()).toBeTruthy(); - expect(wrapper.find('SecretFormField').exists()).toBeTruthy(); + setup(); + const b = screen.getByRole('button', { name: 'Add header' }); + expect(b).toBeInTheDocument(); + assertRowCount(0, 0); + + userEvent.click(b); + assertRowCount(0, 1); }); it('add header button should not submit the form', () => { - const wrapper = setup(); - expect(wrapper.find(Button).getDOMNode()).toHaveAttribute('type', 'button'); + setup(); + const b = screen.getByRole('button', { name: 'Add header' }); + expect(b).toBeInTheDocument(); + expect(b.getAttribute('type')).toBe('button'); }); it('should remove a header', () => { - const wrapper = setup({ + const { onChange } = setup({ dataSourceConfig: { jsonData: { httpHeaderName1: 'X-Custom-Header', @@ -65,14 +81,45 @@ describe('Render', () => { }, }, }); - const removeButton = wrapper.find('Button').at(1); - removeButton.simulate('click', { preventDefault: () => {} }); - expect(wrapper.find('FormField').exists()).toBeFalsy(); - expect(wrapper.find('SecretFormField').exists()).toBeFalsy(); + const b = screen.getByRole('button', { name: 'Remove header' }); + expect(b).toBeInTheDocument(); + + assertRowCount(1, 0); + + userEvent.click(b); + assertRowCount(0, 0); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange.mock.calls[0][0].jsonData).toStrictEqual({}); + }); + + it('when removing a just-created header, it should clean up secureJsonData', () => { + const { onChange } = setup({ + dataSourceConfig: { + jsonData: { + httpHeaderName1: 'name1', + }, + secureJsonData: { + httpHeaderValue1: 'value1', + }, + }, + }); + + // we remove the row + const removeButton = screen.getByRole('button', { name: 'Remove header' }); + expect(removeButton).toBeInTheDocument(); + userEvent.click(removeButton); + assertRowCount(0, 0); + expect(onChange).toHaveBeenCalled(); + + // and we verify the onChange-data + const lastCall = onChange.mock.calls[onChange.mock.calls.length - 1]; + expect(lastCall[0].jsonData).not.toHaveProperty('httpHeaderName1'); + expect(lastCall[0].secureJsonData).not.toHaveProperty('httpHeaderValue1'); }); it('should reset a header', () => { - const wrapper = setup({ + setup({ dataSourceConfig: { jsonData: { httpHeaderName1: 'X-Custom-Header', @@ -82,9 +129,12 @@ describe('Render', () => { }, }, }); - const resetButton = wrapper.find('button').at(0); - resetButton.simulate('click', { preventDefault: () => {} }); - const { isConfigured } = wrapper.find('SecretFormField').props() as any; - expect(isConfigured).toBeFalsy(); + + const b = screen.getByRole('button', { name: 'Reset' }); + expect(b).toBeInTheDocument(); + + assertRowCount(1, 0); + userEvent.click(b); + assertRowCount(0, 1); }); }); diff --git a/packages/grafana-ui/src/components/DataSourceSettings/CustomHeadersSettings.tsx b/packages/grafana-ui/src/components/DataSourceSettings/CustomHeadersSettings.tsx index 91529e01498..945e64c7894 100644 --- a/packages/grafana-ui/src/components/DataSourceSettings/CustomHeadersSettings.tsx +++ b/packages/grafana-ui/src/components/DataSourceSettings/CustomHeadersSettings.tsx @@ -78,7 +78,13 @@ const CustomHeaderRow: React.FC = ({ header, onBlur, onCha onChange={(e) => onChange({ ...header, value: e.target.value })} onBlur={onBlur} /> - @@ -112,27 +118,31 @@ export class CustomHeadersSettings extends PureComponent { updateSettings = () => { const { headers } = this.state; - const { jsonData } = this.props.dataSourceConfig; - const secureJsonData = this.props.dataSourceConfig.secureJsonData || {}; + + // we remove every httpHeaderName* field + const newJsonData = Object.fromEntries( + Object.entries(this.props.dataSourceConfig.jsonData).filter(([key, val]) => !key.startsWith('httpHeaderName')) + ); + + // we remove every httpHeaderValue* field + const newSecureJsonData = Object.fromEntries( + Object.entries(this.props.dataSourceConfig.secureJsonData || {}).filter( + ([key, val]) => !key.startsWith('httpHeaderValue') + ) + ); + + // then we add the current httpHeader-fields for (const [index, header] of headers.entries()) { - jsonData[`httpHeaderName${index + 1}`] = header.name; + newJsonData[`httpHeaderName${index + 1}`] = header.name; if (!header.configured) { - secureJsonData[`httpHeaderValue${index + 1}`] = header.value; + newSecureJsonData[`httpHeaderValue${index + 1}`] = header.value; } - Object.keys(jsonData) - .filter( - (key) => - key.startsWith('httpHeaderName') && parseInt(key.substring('httpHeaderName'.length), 10) > headers.length - ) - .forEach((key) => { - delete jsonData[key]; - }); } this.props.onChange({ ...this.props.dataSourceConfig, - jsonData, - secureJsonData, + jsonData: newJsonData, + secureJsonData: newSecureJsonData, }); };