Loki: Show invalid fields in label filter (#55751)

* Loki: Show invalid fields in Label filter

* Update

* Update comment

* Update comment

public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilder.test.tsx

@@ -4,10 +4,11 @@ import React from 'react';
 
 import { DataSourceInstanceSettings, DataSourcePluginMeta } from '@grafana/data';
 
+import { MISSING_LABEL_FILTER_ERROR_MESSAGE } from '../../../prometheus/querybuilder/shared/LabelFilters';
 import { LokiDatasource } from '../../datasource';
 import { LokiOperationId, LokiVisualQuery } from '../types';
 
-import { MISSING_LABEL_FILTER_ERROR_MESSAGE, LokiQueryBuilder } from './LokiQueryBuilder';
+import { LokiQueryBuilder } from './LokiQueryBuilder';
 import { EXPLAIN_LABEL_FILTER_CONTENT } from './LokiQueryBuilderExplained';
 
 const defaultQuery: LokiVisualQuery = {

public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilder.tsx

@@ -31,8 +31,6 @@ export interface Props {
   onChange: (update: LokiVisualQuery) => void;
   onRunQuery: () => void;
 }
-export const MISSING_LABEL_FILTER_ERROR_MESSAGE = 'Select at least 1 label filter (label and value)';
-
 export const LokiQueryBuilder = React.memo<Props>(({ datasource, query, onChange, onRunQuery, showExplain }) => {
   const [sampleData, setSampleData] = useState<PanelData>();
   const [highlightedOp, setHighlightedOp] = useState<QueryBuilderOperation | undefined>(undefined);
@@ -77,16 +75,16 @@ export const LokiQueryBuilder = React.memo<Props>(({ datasource, query, onChange
     return values ? values.map((v) => escapeLabelValueInSelector(v, forLabel.op)) : []; // Escape values in return
   };
 
-  const labelFilterError: string | undefined = useMemo(() => {
+  const labelFilterRequired: boolean = useMemo(() => {
     const { labels, operations: op } = query;
     if (!labels.length && op.length) {
-      // We don't want to show error for initial state with empty line contains operation
+      // Filter is required when operations are present (empty line contains operation is exception)
       if (op.length === 1 && op[0].id === LokiOperationId.LineContains && op[0].params[0] === '') {
-        return undefined;
+        return false;
       }
-      return MISSING_LABEL_FILTER_ERROR_MESSAGE;
+      return true;
     }
-    return undefined;
+    return false;
   }, [query]);
 
   useEffect(() => {
@@ -113,7 +111,7 @@ export const LokiQueryBuilder = React.memo<Props>(({ datasource, query, onChange
           }
           labelsFilters={query.labels}
           onChange={onChangeLabels}
-          error={labelFilterError}
+          labelFilterRequired={labelFilterRequired}
         />
       </EditorRow>
       {showExplain && (

public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilterItem.tsx

@@ -1,3 +1,4 @@
+import { css } from '@emotion/css';
 import { uniqBy } from 'lodash';
 import React, { useState } from 'react';
 
@@ -14,9 +15,20 @@ export interface Props {
   onGetLabelNames: (forLabel: Partial<QueryBuilderLabelFilter>) => Promise<SelectableValue[]>;
   onGetLabelValues: (forLabel: Partial<QueryBuilderLabelFilter>) => Promise<SelectableValue[]>;
   onDelete: () => void;
+  invalidLabel?: boolean;
+  invalidValue?: boolean;
 }
 
-export function LabelFilterItem({ item, defaultOp, onChange, onDelete, onGetLabelNames, onGetLabelValues }: Props) {
+export function LabelFilterItem({
+  item,
+  defaultOp,
+  onChange,
+  onDelete,
+  onGetLabelNames,
+  onGetLabelValues,
+  invalidLabel,
+  invalidValue,
+}: Props) {
   const [state, setState] = useState<{
     labelNames?: SelectableValue[];
     labelValues?: SelectableValue[];
@@ -46,6 +58,16 @@ export function LabelFilterItem({ item, defaultOp, onChange, onDelete, onGetLabe
     return uniqBy([...selectedOptions, ...labelValues], 'value');
   };
 
+  /**
+   * !important here is necessary to show invalid border on all 4 sides of select.
+   * Without it, the invalid state is only visible on 3 sides as the right side is overridden in InputGroup.
+   */
+  const invalidClassNameOverride = invalidLabel
+    ? css`
+        margin-left: 0 !important;
+      `
+    : '';
+
   return (
     <div data-testid="prometheus-dimensions-filter-item">
       <InputGroup>
@@ -72,6 +94,7 @@ export function LabelFilterItem({ item, defaultOp, onChange, onDelete, onGetLabe
               } as any as QueryBuilderLabelFilter);
             }
           }}
+          invalid={invalidLabel}
         />
 
         <Select
@@ -84,6 +107,7 @@ export function LabelFilterItem({ item, defaultOp, onChange, onDelete, onGetLabe
               onChange({ ...item, op: change.value } as any as QueryBuilderLabelFilter);
             }
           }}
+          className={invalidClassNameOverride}
         />
 
         <Select
@@ -121,6 +145,7 @@ export function LabelFilterItem({ item, defaultOp, onChange, onDelete, onGetLabe
               onChange({ ...item, value: changes, op: item.op ?? defaultOp } as any as QueryBuilderLabelFilter);
             }
           }}
+          invalid={invalidValue}
         />
         <AccessoryButton aria-label="remove" icon="times" variant="secondary" onClick={onDelete} />
       </InputGroup>

public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.test.tsx

@@ -1,12 +1,11 @@
 import { render, screen } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
-import React from 'react';
+import React, { ComponentProps } from 'react';
 import { selectOptionInTest } from 'test/helpers/selectOptionInTest';
 
 import { getLabelSelects } from '../testUtils';
 
-import { LabelFilters } from './LabelFilters';
+import { LabelFilters, MISSING_LABEL_FILTER_ERROR_MESSAGE } from './LabelFilters';
-import { QueryBuilderLabelFilter } from './types';
 
 describe('LabelFilters', () => {
   it('renders empty input without labels', async () => {
@@ -18,11 +17,13 @@ describe('LabelFilters', () => {
   });
 
   it('renders multiple labels', async () => {
-    setup([
+    setup({
-      { label: 'foo', op: '=', value: 'bar' },
+      labelsFilters: [
-      { label: 'baz', op: '!=', value: 'qux' },
+        { label: 'foo', op: '=', value: 'bar' },
-      { label: 'quux', op: '=~', value: 'quuz' },
+        { label: 'baz', op: '!=', value: 'qux' },
-    ]);
+        { label: 'quux', op: '=~', value: 'quuz' },
+      ],
+    });
     expect(screen.getByText(/foo/)).toBeInTheDocument();
     expect(screen.getByText(/bar/)).toBeInTheDocument();
     expect(screen.getByText(/baz/)).toBeInTheDocument();
@@ -33,10 +34,12 @@ describe('LabelFilters', () => {
   });
 
   it('renders multiple values for regex selectors', async () => {
-    setup([
+    setup({
-      { label: 'bar', op: '!~', value: 'baz|bat|bau' },
+      labelsFilters: [
-      { label: 'foo', op: '!~', value: 'fop|for|fos' },
+        { label: 'bar', op: '!~', value: 'baz|bat|bau' },
-    ]);
+        { label: 'foo', op: '!~', value: 'fop|for|fos' },
+      ],
+    });
     expect(screen.getByText(/bar/)).toBeInTheDocument();
     expect(screen.getByText(/baz/)).toBeInTheDocument();
     expect(screen.getByText(/bat/)).toBeInTheDocument();
@@ -48,7 +51,7 @@ describe('LabelFilters', () => {
   });
 
   it('adds new label', async () => {
-    const { onChange } = setup([{ label: 'foo', op: '=', value: 'bar' }]);
+    const { onChange } = setup({ labelsFilters: [{ label: 'foo', op: '=', value: 'bar' }] });
     await userEvent.click(getAddButton());
     expect(screen.getAllByText('Select label')).toHaveLength(1);
     expect(screen.getAllByText('Select value')).toHaveLength(1);
@@ -62,13 +65,13 @@ describe('LabelFilters', () => {
   });
 
   it('removes label', async () => {
-    const { onChange } = setup([{ label: 'foo', op: '=', value: 'bar' }]);
+    const { onChange } = setup({ labelsFilters: [{ label: 'foo', op: '=', value: 'bar' }] });
     await userEvent.click(screen.getByLabelText(/remove/));
     expect(onChange).toBeCalledWith([]);
   });
 
   it('renders empty input when labels are deleted from outside ', async () => {
-    const { rerender } = setup([{ label: 'foo', op: '=', value: 'bar' }]);
+    const { rerender } = setup({ labelsFilters: [{ label: 'foo', op: '=', value: 'bar' }] });
     expect(screen.getByText(/foo/)).toBeInTheDocument();
     expect(screen.getByText(/bar/)).toBeInTheDocument();
     rerender(
@@ -79,10 +82,20 @@ describe('LabelFilters', () => {
     expect(screen.getByText(/=/)).toBeInTheDocument();
     expect(getAddButton()).toBeInTheDocument();
   });
+
+  it('shows error when filter with empty strings  and label filter is required', async () => {
+    setup({ labelsFilters: [{ label: '', op: '=', value: '' }], labelFilterRequired: true });
+    expect(screen.getByText(MISSING_LABEL_FILTER_ERROR_MESSAGE)).toBeInTheDocument();
+  });
+
+  it('shows error when no filter and label filter is required', async () => {
+    setup({ labelsFilters: [], labelFilterRequired: true });
+    expect(screen.getByText(MISSING_LABEL_FILTER_ERROR_MESSAGE)).toBeInTheDocument();
+  });
 });
 
-function setup(labels: QueryBuilderLabelFilter[] = []) {
+function setup(propOverrides?: Partial<ComponentProps<typeof LabelFilters>>) {
-  const props = {
+  const defaultProps = {
     onChange: jest.fn(),
     onGetLabelNames: async () => [
       { label: 'foo', value: 'foo' },
@@ -94,9 +107,12 @@ function setup(labels: QueryBuilderLabelFilter[] = []) {
       { label: 'qux', value: 'qux' },
       { label: 'quux', value: 'quux' },
     ],
+    labelsFilters: [],
   };
 
-  const { rerender } = render(<LabelFilters {...props} labelsFilters={labels} />);
+  const props = { ...defaultProps, ...propOverrides };
+
+  const { rerender } = render(<LabelFilters {...props} />);
   return { ...props, rerender };
 }
 

public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.tsx

@@ -8,15 +8,24 @@ import { QueryBuilderLabelFilter } from '../shared/types';
 
 import { LabelFilterItem } from './LabelFilterItem';
 
+export const MISSING_LABEL_FILTER_ERROR_MESSAGE = 'Select at least 1 label filter (label and value)';
+
 export interface Props {
   labelsFilters: QueryBuilderLabelFilter[];
   onChange: (labelFilters: QueryBuilderLabelFilter[]) => void;
   onGetLabelNames: (forLabel: Partial<QueryBuilderLabelFilter>) => Promise<SelectableValue[]>;
   onGetLabelValues: (forLabel: Partial<QueryBuilderLabelFilter>) => Promise<SelectableValue[]>;
-  error?: string;
+  /** If set to true, component will show error message until at least 1 filter is selected */
+  labelFilterRequired?: boolean;
 }
 
-export function LabelFilters({ labelsFilters, onChange, onGetLabelNames, onGetLabelValues, error }: Props) {
+export function LabelFilters({
+  labelsFilters,
+  onChange,
+  onGetLabelNames,
+  onGetLabelValues,
+  labelFilterRequired,
+}: Props) {
   const defaultOp = '=';
   const [items, setItems] = useState<Array<Partial<QueryBuilderLabelFilter>>>([{ op: defaultOp }]);
 
@@ -38,9 +47,15 @@ export function LabelFilters({ labelsFilters, onChange, onGetLabelNames, onGetLa
     }
   };
 
+  const hasLabelFilter = items.some((item) => item.label && item.value);
+
   return (
     <EditorFieldGroup>
-      <EditorField label="Label filters" error={error} invalid={!!error}>
+      <EditorField
+        label="Label filters"
+        error={MISSING_LABEL_FILTER_ERROR_MESSAGE}
+        invalid={labelFilterRequired && !hasLabelFilter}
+      >
         <EditorList
           items={items}
           onChange={onLabelsChange}
@@ -52,6 +67,8 @@ export function LabelFilters({ labelsFilters, onChange, onGetLabelNames, onGetLa
               onDelete={onDelete}
               onGetLabelNames={onGetLabelNames}
               onGetLabelValues={onGetLabelValues}
+              invalidLabel={labelFilterRequired && !item.label}
+              invalidValue={labelFilterRequired && !item.value}
             />
           )}
         />