Loki Derived Fields: Refactor legacy form components and add validation (#68015)

* Derived fields: validate duplicated names * Derived fields: rename prop * Update tests * Derived fields: do not validate empty names as repeated * Derived Field: use non-legacy Field and Input * Derived Field: integrate name validation * Derived field: align delete button * Derived Field: add tooltips * Derived Field: migrate and style internal link field * Update tests * Derived Field: ask user to select data source Otherwise it's bugged * Remove unnecessary onchange handler * Initialize all derived fields attributes Otherwise we trigger controlled-to-uncontrolled React errors * Update public/app/plugins/datasource/loki/configuration/DerivedField.tsx Co-authored-by: Gábor Farkas <gabor.farkas@gmail.com> --------- Co-authored-by: Gábor Farkas <gabor.farkas@gmail.com>
2025-02-12 08:35:43 -06:00 · 2023-05-15 12:48:50 +02:00 · 2023-05-15 12:48:50 +02:00 · 9441692fe9
commit 9441692fe9
parent 07f2aec576
5 changed files with 202 additions and 105 deletions
--- a/public/app/plugins/datasource/loki/configuration/ConfigEditor.tsx
+++ b/public/app/plugins/datasource/loki/configuration/ConfigEditor.tsx
@ -47,7 +47,7 @@ export const ConfigEditor = (props: Props) => {
      />

      <DerivedFields
-        value={options.jsonData.derivedFields}
+        fields={options.jsonData.derivedFields}
        onChange={(value) => onOptionsChange(setDerivedFields(options, value))}
      />
    </>
--- a/public/app/plugins/datasource/loki/configuration/DerivedField.test.tsx
+++ b/public/app/plugins/datasource/loki/configuration/DerivedField.test.tsx
@ -1,4 +1,5 @@
 import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 import React from 'react';

 import { DataSourceInstanceSettings, DataSourcePluginMeta } from '@grafana/data';
@ -8,6 +9,7 @@ import { setDataSourceSrv } from '@grafana/runtime';
 import { DerivedField } from './DerivedField';

 const mockList = jest.fn();
+const validateMock = jest.fn();

 describe('DerivedField', () => {
  beforeEach(() => {
@ -54,7 +56,15 @@ describe('DerivedField', () => {
    };
    // Render and wait for the Name field to be visible
    // using findBy to wait for asynchronous operations to complete
-    render(<DerivedField value={value} onChange={() => {}} onDelete={() => {}} suggestions={[]} />);
+    render(
+      <DerivedField
+        validateName={validateMock}
+        value={value}
+        onChange={() => {}}
+        onDelete={() => {}}
+        suggestions={[]}
+      />
+    );
    expect(await screen.findByText('Name')).toBeInTheDocument();

    expect(screen.getByLabelText(selectors.components.DataSourcePicker.inputV2)).toBeInTheDocument();
@ -68,7 +78,15 @@ describe('DerivedField', () => {
    };
    // Render and wait for the Name field to be visible
    // using findBy to wait for asynchronous operations to complete
-    render(<DerivedField value={value} onChange={() => {}} onDelete={() => {}} suggestions={[]} />);
+    render(
+      <DerivedField
+        validateName={validateMock}
+        value={value}
+        onChange={() => {}}
+        onDelete={() => {}}
+        suggestions={[]}
+      />
+    );
    expect(await screen.findByText('Name')).toBeInTheDocument();

    expect(screen.queryByLabelText(selectors.components.DataSourcePicker.inputV2)).not.toBeInTheDocument();
@ -82,7 +100,15 @@ describe('DerivedField', () => {
    };
    // Render and wait for the Name field to be visible
    // using findBy to wait for asynchronous operations to complete
-    render(<DerivedField value={value} onChange={() => {}} onDelete={() => {}} suggestions={[]} />);
+    render(
+      <DerivedField
+        validateName={validateMock}
+        value={value}
+        onChange={() => {}}
+        onDelete={() => {}}
+        suggestions={[]}
+      />
+    );
    expect(await screen.findByText('Name')).toBeInTheDocument();
    expect(mockList).toHaveBeenCalledWith(
      expect.objectContaining({
@ -90,4 +116,19 @@ describe('DerivedField', () => {
      })
    );
  });
+
+  it('validates the field name', async () => {
+    const value = {
+      matcherRegex: '',
+      name: 'field-name',
+      datasourceUid: 'test',
+    };
+    const validate = jest.fn().mockReturnValue(false);
+    render(
+      <DerivedField validateName={validate} value={value} onChange={() => {}} onDelete={() => {}} suggestions={[]} />
+    );
+    userEvent.click(await screen.findByDisplayValue(value.name));
+
+    expect(await screen.findByText('The name is already in use')).toBeInTheDocument();
+  });
 });
--- a/public/app/plugins/datasource/loki/configuration/DerivedField.tsx
+++ b/public/app/plugins/datasource/loki/configuration/DerivedField.tsx
@ -1,15 +1,13 @@
 import { css } from '@emotion/css';
-import React, { useEffect, useState } from 'react';
+import React, { ChangeEvent, useEffect, useState } from 'react';
 import { usePrevious } from 'react-use';

 import { GrafanaTheme2, VariableSuggestion } from '@grafana/data';
 import { DataSourcePicker } from '@grafana/runtime';
-import { Button, DataLinkInput, LegacyForms, useStyles2 } from '@grafana/ui';
+import { Button, DataLinkInput, Field, Icon, Input, Label, Tooltip, useStyles2, Switch } from '@grafana/ui';

 import { DerivedFieldConfig } from '../types';

-const { Switch, FormField } = LegacyForms;
-
 const getStyles = (theme: GrafanaTheme2) => ({
  row: css`
    display: flex;
@ -17,9 +15,11 @@ const getStyles = (theme: GrafanaTheme2) => ({
  `,
  nameField: css`
    flex: 2;
+    margin-right: ${theme.spacing(0.5)};
  `,
  regexField: css`
    flex: 3;
+    margin-right: ${theme.spacing(0.5)};
  `,
  urlField: css`
    flex: 1;
@ -28,6 +28,10 @@ const getStyles = (theme: GrafanaTheme2) => ({
  urlDisplayLabelField: css`
    flex: 1;
  `,
+  internalLink: css`
+    margin-right: ${theme.spacing(1)};
+  `,
+  dataSource: css``,
 });

 type Props = {
@ -36,9 +40,10 @@ type Props = {
  onDelete: () => void;
  suggestions: VariableSuggestion[];
  className?: string;
+  validateName: (name: string) => boolean;
 };
 export const DerivedField = (props: Props) => {
-  const { value, onChange, onDelete, suggestions, className } = props;
+  const { value, onChange, onDelete, suggestions, className, validateName } = props;
  const styles = useStyles2(getStyles);
  const [showInternalLink, setShowInternalLink] = useState(!!value.datasourceUid);
  const previousUid = usePrevious(value.datasourceUid);
@ -60,101 +65,107 @@ export const DerivedField = (props: Props) => {
    });
  };

+  const invalidName = !validateName(value.name);
+
  return (
    <div className={className} data-testid="derived-field">
      <div className="gf-form">
-        <FormField
-          labelWidth={10}
-          className={styles.nameField}
-          // A bit of a hack to prevent using default value for the width from FormField
-          inputWidth={null}
-          label="Name"
-          type="text"
-          value={value.name}
-          onChange={handleChange('name')}
-        />
-        <FormField
-          labelWidth={10}
+        <Field className={styles.nameField} label="Name" invalid={invalidName} error="The name is already in use">
+          <Input value={value.name} onChange={handleChange('name')} placeholder="Field name" invalid={invalidName} />
+        </Field>
+        <Field
          className={styles.regexField}
-          inputWidth={null}
-          label="Regex"
-          type="text"
-          value={value.matcherRegex}
-          onChange={handleChange('matcherRegex')}
-          tooltip={
-            'Use to parse and capture some part of the log message. You can use the captured groups in the template.'
+          label={
+            <TooltipLabel
+              label="Regex"
+              content="Use to parse and capture some part of the log message. You can use the captured groups in the template."
+            />
          }
-        />
-        <Button
-          variant="destructive"
-          title="Remove field"
-          icon="times"
-          onClick={(event) => {
-            event.preventDefault();
-            onDelete();
-          }}
-        />
+        >
+          <Input value={value.matcherRegex} onChange={handleChange('matcherRegex')} />
+        </Field>
+        <Field label="">
+          <Button
+            variant="destructive"
+            title="Remove field"
+            icon="times"
+            onClick={(event) => {
+              event.preventDefault();
+              onDelete();
+            }}
+          />
+        </Field>
      </div>

      <div className="gf-form">
-        <FormField
-          labelWidth={10}
-          label={showInternalLink ? 'Query' : 'URL'}
-          inputEl={
-            <DataLinkInput
-              placeholder={showInternalLink ? '${__value.raw}' : 'http://example.com/${__value.raw}'}
-              value={value.url || ''}
-              onChange={(newValue) =>
-                onChange({
-                  ...value,
-                  url: newValue,
-                })
-              }
-              suggestions={suggestions}
-            />
-          }
-          className={styles.urlField}
-        />
-        <FormField
-          className={styles.urlDisplayLabelField}
-          labelWidth={10}
-          inputWidth={null}
-          label="URL Label"
-          type="text"
-          value={value.urlDisplayLabel}
-          onChange={handleChange('urlDisplayLabel')}
-          tooltip={'Use to override the button label when this derived field is found in a log.'}
-        />
-      </div>
-
-      <div className={styles.row}>
-        <Switch
-          label="Internal link"
-          checked={showInternalLink}
-          onChange={() => {
-            if (showInternalLink) {
+        <Field label={showInternalLink ? 'Query' : 'URL'} className={styles.urlField}>
+          <DataLinkInput
+            placeholder={showInternalLink ? '${__value.raw}' : 'http://example.com/${__value.raw}'}
+            value={value.url || ''}
+            onChange={(newValue) =>
              onChange({
                ...value,
-                datasourceUid: undefined,
-              });
-            }
-            setShowInternalLink(!showInternalLink);
-          }}
-        />
-
-        {showInternalLink && (
-          <DataSourcePicker
-            tracing={true}
-            onChange={(ds) =>
-              onChange({
-                ...value,
-                datasourceUid: ds.uid,
+                url: newValue,
              })
            }
-            current={value.datasourceUid}
+            suggestions={suggestions}
          />
+        </Field>
+        <Field
+          className={styles.urlDisplayLabelField}
+          label={
+            <TooltipLabel
+              label="URL Label"
+              content="Use to override the button label when this derived field is found in a log."
+            />
+          }
+        >
+          <Input value={value.urlDisplayLabel} onChange={handleChange('urlDisplayLabel')} />
+        </Field>
+      </div>
+
+      <div className="gf-form">
+        <Field label="Internal link" className={styles.internalLink}>
+          <Switch
+            value={showInternalLink}
+            onChange={(e: ChangeEvent<HTMLInputElement>) => {
+              const { checked } = e.currentTarget;
+              if (!checked) {
+                onChange({
+                  ...value,
+                  datasourceUid: undefined,
+                });
+              }
+              setShowInternalLink(checked);
+            }}
+          />
+        </Field>
+
+        {showInternalLink && (
+          <Field label="" className={styles.dataSource}>
+            <DataSourcePicker
+              tracing={true}
+              onChange={(ds) =>
+                onChange({
+                  ...value,
+                  datasourceUid: ds.uid,
+                })
+              }
+              current={value.datasourceUid}
+              noDefault
+            />
+          </Field>
        )}
      </div>
    </div>
  );
 };
+
+const TooltipLabel = ({ content, label }: { content: string; label: string }) => (
+  <Label>
+    {label}
+    <Tooltip placement="top" content={content} theme="info">
+      <Icon tabIndex={0} name="info-circle" size="sm" style={{ marginLeft: '10px' }} />
+    </Tooltip>
+  </Label>
+);
--- a/public/app/plugins/datasource/loki/configuration/DerivedFields.test.tsx
+++ b/public/app/plugins/datasource/loki/configuration/DerivedFields.test.tsx
@ -1,4 +1,5 @@
-import { render, screen, waitFor, fireEvent } from '@testing-library/react';
+import { render, screen, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 import React from 'react';

 import { DerivedFields } from './DerivedFields';
@ -23,7 +24,7 @@ describe('DerivedFields', () => {
  });

  it('renders correctly when there are fields', async () => {
-    render(<DerivedFields value={testValue} onChange={() => {}} />);
+    render(<DerivedFields fields={testFields} onChange={() => {}} />);

    await waitFor(() => expect(screen.getAllByTestId('derived-field')).toHaveLength(2));
    expect(screen.getByText('Add')).toBeInTheDocument();
@ -34,22 +35,58 @@ describe('DerivedFields', () => {
    const onChange = jest.fn();
    render(<DerivedFields onChange={onChange} />);

-    fireEvent.click(screen.getByText('Add'));
+    userEvent.click(screen.getByText('Add'));

    await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1));
  });

  it('removes a field', async () => {
    const onChange = jest.fn();
-    render(<DerivedFields value={testValue} onChange={onChange} />);
+    render(<DerivedFields fields={testFields} onChange={onChange} />);

-    fireEvent.click((await screen.findAllByTitle('Remove field'))[0]);
+    userEvent.click((await screen.findAllByTitle('Remove field'))[0]);

-    await waitFor(() => expect(onChange).toHaveBeenCalledWith([testValue[1]]));
+    await waitFor(() => expect(onChange).toHaveBeenCalledWith([testFields[1]]));
+  });
+
+  it('validates duplicated field names', async () => {
+    const repeatedFields = [
+      {
+        matcherRegex: '',
+        name: 'repeated',
+      },
+      {
+        matcherRegex: '',
+        name: 'repeated',
+      },
+    ];
+    render(<DerivedFields onChange={jest.fn()} fields={repeatedFields} />);
+
+    userEvent.click(screen.getAllByPlaceholderText('Field name')[0]);
+
+    expect(await screen.findAllByText('The name is already in use')).toHaveLength(2);
+  });
+
+  it('does not validate empty names as repeated', () => {
+    const repeatedFields = [
+      {
+        matcherRegex: '',
+        name: '',
+      },
+      {
+        matcherRegex: '',
+        name: '',
+      },
+    ];
+    render(<DerivedFields onChange={jest.fn()} fields={repeatedFields} />);
+
+    userEvent.click(screen.getAllByPlaceholderText('Field name')[0]);
+
+    expect(screen.queryByText('The name is already in use')).not.toBeInTheDocument();
  });
 });

-const testValue = [
+const testFields = [
  {
    matcherRegex: 'regex1',
    name: 'test1',
--- a/public/app/plugins/datasource/loki/configuration/DerivedFields.tsx
+++ b/public/app/plugins/datasource/loki/configuration/DerivedFields.tsx
@ -1,5 +1,5 @@
 import { css } from '@emotion/css';
-import React, { useState } from 'react';
+import React, { useCallback, useState } from 'react';

 import { GrafanaTheme2, VariableOrigin, DataLinkBuiltInVars } from '@grafana/data';
 import { Button, useTheme2 } from '@grafana/ui';
@ -20,16 +20,23 @@ const getStyles = (theme: GrafanaTheme2) => ({
 });

 type Props = {
-  value?: DerivedFieldConfig[];
+  fields?: DerivedFieldConfig[];
  onChange: (value: DerivedFieldConfig[]) => void;
 };

-export const DerivedFields = ({ value = [], onChange }: Props) => {
+export const DerivedFields = ({ fields = [], onChange }: Props) => {
  const theme = useTheme2();
  const styles = getStyles(theme);

  const [showDebug, setShowDebug] = useState(false);

+  const validateName = useCallback(
+    (name: string) => {
+      return fields.filter((field) => field.name && field.name === name).length <= 1;
+    },
+    [fields]
+  );
+
  return (
    <>
      <h3 className="page-heading">Derived fields</h3>
@ -39,22 +46,23 @@ export const DerivedFields = ({ value = [], onChange }: Props) => {
      </div>

      <div className="gf-form-group">
-        {value.map((field, index) => {
+        {fields.map((field, index) => {
          return (
            <DerivedField
              className={styles.derivedField}
              key={index}
              value={field}
              onChange={(newField) => {
-                const newDerivedFields = [...value];
+                const newDerivedFields = [...fields];
                newDerivedFields.splice(index, 1, newField);
                onChange(newDerivedFields);
              }}
              onDelete={() => {
-                const newDerivedFields = [...value];
+                const newDerivedFields = [...fields];
                newDerivedFields.splice(index, 1);
                onChange(newDerivedFields);
              }}
+              validateName={validateName}
              suggestions={[
                {
                  value: DataLinkBuiltInVars.valueRaw,
@ -75,14 +83,14 @@ export const DerivedFields = ({ value = [], onChange }: Props) => {
            icon="plus"
            onClick={(event) => {
              event.preventDefault();
-              const newDerivedFields = [...value, { name: '', matcherRegex: '' }];
+              const newDerivedFields = [...fields, { name: '', matcherRegex: '', urlDisplayLabel: '', url: '' }];
              onChange(newDerivedFields);
            }}
          >
            Add
          </Button>

-          {value.length > 0 && (
+          {fields.length > 0 && (
            <Button variant="secondary" type="button" onClick={() => setShowDebug(!showDebug)}>
              {showDebug ? 'Hide example log message' : 'Show example log message'}
            </Button>
@ -96,7 +104,7 @@ export const DerivedFields = ({ value = [], onChange }: Props) => {
            className={css`
              margin-bottom: 10px;
            `}
-            derivedFields={value}
+            derivedFields={fields}
          />
        </div>
      )}