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

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(<CustomHeadersSettings {...props} />);
+  render(<CustomHeadersSettings {...props} />);
+  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();
+    setup();
-    const addButton = wrapper.find('Button').at(0);
+    const b = screen.getByRole('button', { name: 'Add header' });
-    addButton.simulate('click', { preventDefault: () => {} });
+    expect(b).toBeInTheDocument();
-    expect(wrapper.find('FormField').exists()).toBeTruthy();
+    assertRowCount(0, 0);
-    expect(wrapper.find('SecretFormField').exists()).toBeTruthy();
+
+    userEvent.click(b);
+    assertRowCount(0, 1);
   });
 
   it('add header button should not submit the form', () => {
-    const wrapper = setup();
+    setup();
-    expect(wrapper.find(Button).getDOMNode()).toHaveAttribute('type', 'button');
+    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);
+    const b = screen.getByRole('button', { name: 'Remove header' });
-    removeButton.simulate('click', { preventDefault: () => {} });
+    expect(b).toBeInTheDocument();
-    expect(wrapper.find('FormField').exists()).toBeFalsy();
+
-    expect(wrapper.find('SecretFormField').exists()).toBeFalsy();
+    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 b = screen.getByRole('button', { name: 'Reset' });
-    const { isConfigured } = wrapper.find('SecretFormField').props() as any;
+    expect(b).toBeInTheDocument();
-    expect(isConfigured).toBeFalsy();
+
+    assertRowCount(1, 0);
+    userEvent.click(b);
+    assertRowCount(0, 1);
   });
 });

packages/grafana-ui/src/components/DataSourceSettings/CustomHeadersSettings.tsx

@@ -78,7 +78,13 @@ const CustomHeaderRow: React.FC<CustomHeaderRowProps> = ({ header, onBlur, onCha
         onChange={(e) => onChange({ ...header, value: e.target.value })}
         onBlur={onBlur}
       />
-      <Button variant="secondary" size="xs" onClick={(_e) => onRemove(header.id)}>
+      <Button
+        type="button"
+        aria-label="Remove header"
+        variant="secondary"
+        size="xs"
+        onClick={(_e) => onRemove(header.id)}
+      >
         <Icon name="trash-alt" />
       </Button>
     </div>
@@ -112,27 +118,31 @@ export class CustomHeadersSettings extends PureComponent<Props, State> {
 
   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,
+      jsonData: newJsonData,
-      secureJsonData,
+      secureJsonData: newSecureJsonData,
     });
   };