From b39d629142ea2c4959a47c81a2f97b59ac710f26 Mon Sep 17 00:00:00 2001 From: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> Date: Thu, 29 Sep 2022 13:32:01 +0200 Subject: [PATCH] Loki: Show invalid fields in label filter (#55751) * Loki: Show invalid fields in Label filter * Update * Update comment * Update comment --- .../components/LokiQueryBuilder.test.tsx | 3 +- .../components/LokiQueryBuilder.tsx | 14 +++-- .../querybuilder/shared/LabelFilterItem.tsx | 27 +++++++++- .../querybuilder/shared/LabelFilters.test.tsx | 52 ++++++++++++------- .../querybuilder/shared/LabelFilters.tsx | 23 ++++++-- 5 files changed, 88 insertions(+), 31 deletions(-) diff --git a/public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilder.test.tsx b/public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilder.test.tsx index b417a4e4c21..62e3c0cb171 100644 --- a/public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilder.test.tsx +++ b/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 = { diff --git a/public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilder.tsx b/public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilder.tsx index b135eb14b62..af584675408 100644 --- a/public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilder.tsx +++ b/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(({ datasource, query, onChange, onRunQuery, showExplain }) => { const [sampleData, setSampleData] = useState(); const [highlightedOp, setHighlightedOp] = useState(undefined); @@ -77,16 +75,16 @@ export const LokiQueryBuilder = React.memo(({ 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(({ datasource, query, onChange } labelsFilters={query.labels} onChange={onChangeLabels} - error={labelFilterError} + labelFilterRequired={labelFilterRequired} /> {showExplain && ( diff --git a/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilterItem.tsx b/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilterItem.tsx index ee4cdccdc7f..15ad6e9ae14 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilterItem.tsx +++ b/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) => Promise; onGetLabelValues: (forLabel: Partial) => Promise; 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 (
@@ -72,6 +94,7 @@ export function LabelFilterItem({ item, defaultOp, onChange, onDelete, onGetLabe } as any as QueryBuilderLabelFilter); } }} + invalid={invalidLabel} /> diff --git a/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.test.tsx b/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.test.tsx index a675052237b..0c47b17f5d6 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.test.tsx +++ b/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 { QueryBuilderLabelFilter } from './types'; +import { LabelFilters, MISSING_LABEL_FILTER_ERROR_MESSAGE } from './LabelFilters'; describe('LabelFilters', () => { it('renders empty input without labels', async () => { @@ -18,11 +17,13 @@ describe('LabelFilters', () => { }); it('renders multiple labels', async () => { - setup([ - { label: 'foo', op: '=', value: 'bar' }, - { label: 'baz', op: '!=', value: 'qux' }, - { label: 'quux', op: '=~', value: 'quuz' }, - ]); + setup({ + labelsFilters: [ + { label: 'foo', op: '=', value: 'bar' }, + { 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([ - { label: 'bar', op: '!~', value: 'baz|bat|bau' }, - { label: 'foo', op: '!~', value: 'fop|for|fos' }, - ]); + setup({ + labelsFilters: [ + { 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[] = []) { - const props = { +function setup(propOverrides?: Partial>) { + 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(); + const props = { ...defaultProps, ...propOverrides }; + + const { rerender } = render(); return { ...props, rerender }; } diff --git a/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.tsx b/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.tsx index bea7aca2183..6f5daf76c56 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.tsx +++ b/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) => Promise; onGetLabelValues: (forLabel: Partial) => Promise; - 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>>([{ op: defaultOp }]); @@ -38,9 +47,15 @@ export function LabelFilters({ labelsFilters, onChange, onGetLabelNames, onGetLa } }; + const hasLabelFilter = items.some((item) => item.label && item.value); + return ( - + )} />