diff --git a/frontend/src/components/QueryBuilderV2/utils.ts b/frontend/src/components/QueryBuilderV2/utils.ts index a5fee05f9c1..73d66dfd24e 100644 --- a/frontend/src/components/QueryBuilderV2/utils.ts +++ b/frontend/src/components/QueryBuilderV2/utils.ts @@ -525,6 +525,34 @@ export const convertFiltersToExpressionWithExistingQuery = ( }; }; +/** + * Canonical name for a comparison's operator, limited to the equality and + * membership forms. Every other shape (LIKE, BETWEEN, EXISTS, CONTAINS, REGEXP, + * the ordering operators) returns undefined, so an operator-restricted removal + * leaves it in place. + * + * The ANTLR4 runtime returns null for an absent token or rule despite the + * non-nullable TypeScript signatures. + */ +const getComparisonOperator = (ctx: ComparisonContext): string | undefined => { + if ((ctx.inClause() as unknown) !== null) { + return 'in'; + } + if ((ctx.notInClause() as unknown) !== null) { + return 'not in'; + } + if ((ctx.EQUALS() as unknown) !== null) { + return '='; + } + if ( + (ctx.NOT_EQUALS() as unknown) !== null || + (ctx.NEQ() as unknown) !== null + ) { + return '!='; + } + return undefined; +}; + /** * Removes clauses for specified keys from a filter query expression. * @@ -542,12 +570,16 @@ export const convertFiltersToExpressionWithExistingQuery = ( * - `true`: removes only the first clause whose value contains any `$`. * - `string` (e.g. `"$service.name"`): removes only the clause whose value exactly * matches that string — preferred when the specific variable reference is known. + * @param operatorsToRemove - When given, restricts removal to clauses whose operator + * is in this set (`=`, `!=`, `in`, `not in`); every other clause on the key is kept. + * Omit to remove a matching key's clauses whatever their operator. * @returns The rewritten expression, or an empty string if all clauses were removed. */ export const removeKeysFromExpression = ( expression: string, keysToRemove: string[], removeOnlyVariableExpressions: string | boolean = false, + operatorsToRemove?: string[], ): string => { if (!keysToRemove || keysToRemove.length === 0) { return expression; @@ -557,6 +589,9 @@ export const removeKeysFromExpression = ( } const keysSet = new Set(keysToRemove.map((k) => k.trim().toLowerCase())); + const operatorsSet = operatorsToRemove + ? new Set(operatorsToRemove.map((op) => op.trim().toLowerCase())) + : null; // Tracks keys for which a variable expression has already been removed. // Having multiple $-value clauses for the same key is invalid; we remove at most one. const removedVariableKeys = new Set(); @@ -658,6 +693,13 @@ export const removeKeysFromExpression = ( return src(ctx); } + if (operatorsSet) { + const operator = getComparisonOperator(ctx); + if (!operator || !operatorsSet.has(operator)) { + return src(ctx); + } + } + if (removeOnlyVariableExpressions) { // Scope the value check to value nodes only — not the full comparison text — // so a key that contains '$' does not trigger removal when the value is a diff --git a/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/checkboxFilterQuery.test.ts b/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/checkboxFilterQuery.test.ts new file mode 100644 index 00000000000..bff4e46084c --- /dev/null +++ b/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/checkboxFilterQuery.test.ts @@ -0,0 +1,526 @@ +import { + convertFiltersToExpression, + convertFiltersToExpressionWithExistingQuery, +} from 'components/QueryBuilderV2/utils'; +import { QuickFiltersSource } from 'components/QuickFilters/types'; +import { + Query, + TagFilter, + TagFilterItem, +} from 'types/api/queryBuilder/queryBuilderData'; + +import { + applyCheckboxToggle, + clearFilterFromQuery, + deriveCheckboxState, + getNotInOperator, +} from './checkboxFilterQuery'; +import { CheckedState } from '../../types'; +import { SectionType } from './v2/itemRules'; + +const KEY = 'service.name'; + +/** + * Mini test framework + * ------------------- + * `filters.items` is the source of truth the checkbox algebra mutates. + * `filter.expression` is the derived value the backend actually reads, and it is + * authoritatively rebuilt from the items on every URL round trip + * (`useGetCompositeQueryParam` -> `convertFiltersToExpressionWithExistingQuery`). + * That rebuild is additive, so `applyCheckboxToggle` re-derives its own clauses + * into the expression itself: otherwise the round trip resurrects a clause the + * toggle removed, or appends a duplicate of one it replaced. + * + * So a case does not assert the intermediate expression the toggle emits. It + * asserts the pair that has to stay consistent: + * - `items` : exact structured clauses after the toggle + * - `expression` : the expression AFTER the round trip, which is what ships + * + * `runToggle` runs the real reducer, then feeds its output through the real + * converter to get the shipped expression. + */ + +type SimpleItem = { + key: string; + op: string; + value: TagFilterItem['value']; +}; + +function toTagItem(item: SimpleItem, idx: number): TagFilterItem { + return { + id: `id-${idx}`, + key: { key: item.key, type: 'tag' } as TagFilterItem['key'], + op: item.op, + value: item.value, + }; +} + +// Serialises items into an expression (via the app's own converter) so a case's +// starting state is self-consistent (items and expression agree), the way it +// would be in the app after a prior round trip. +const serializeItems = (items: SimpleItem[]): string => + convertFiltersToExpression({ items: items.map(toTagItem), op: 'AND' }) + .expression; + +function buildQuery(items: SimpleItem[], expression: string): Query { + return { + builder: { + queryData: [ + { + filters: { items: items.map(toTagItem), op: 'AND' }, + filter: { expression }, + }, + ], + }, + } as unknown as Query; +} + +// Simulates the URL round trip: rebuild the shipped expression from the items, +// reconciled against whatever expression the toggle left behind. Trimmed to +// absorb a converter quirk that leaves a trailing space when it widens an +// operator in place (e.g. `=` -> `IN`). +function roundTripExpression( + items: TagFilterItem[], + emittedExpression: string, +): string { + const filters: TagFilter = { items, op: 'AND' }; + const { filter } = convertFiltersToExpressionWithExistingQuery( + filters, + emittedExpression, + ); + return (filter?.expression ?? '').trim(); +} + +interface ToggleAction { + value: string; + checked: boolean; + isOnlyOrAllClicked?: boolean; + previousState?: CheckedState; + sectionType?: SectionType; + source?: QuickFiltersSource; + attributeValues?: string[]; +} + +interface ToggleCase { + name: string; + initial?: { items?: SimpleItem[]; expression?: string }; + action: ToggleAction; + expected: { items: SimpleItem[]; expression: string }; +} + +function runToggle(c: ToggleCase): { items: SimpleItem[]; expression: string } { + const initialItems = c.initial?.items ?? []; + const initialExpression = + c.initial?.expression ?? serializeItems(initialItems); + + const result = applyCheckboxToggle({ + currentQuery: buildQuery(initialItems, initialExpression), + activeQueryIndex: 0, + filter: { attributeKey: { key: KEY, type: 'tag' } } as never, + source: c.action.source ?? QuickFiltersSource.LOGS_EXPLORER, + attributeValues: c.action.attributeValues ?? ['a', 'b', 'c'], + value: c.action.value, + checked: c.action.checked, + isOnlyOrAllClicked: c.action.isOnlyOrAllClicked ?? false, + previousState: c.action.previousState, + sectionType: c.action.sectionType, + }); + + const active = result.builder.queryData[0]; + const items = active?.filters?.items ?? []; + return { + items: items.map((item) => ({ + key: item.key?.key ?? '', + op: item.op, + value: item.value, + })), + expression: roundTripExpression(items, active?.filter?.expression ?? ''), + }; +} + +// Flat list. Every row asserts both the structured items and the shipped +// (round-tripped) expression, which must stay in sync. +const TOGGLE_CASES: ToggleCase[] = [ + { + name: 'no clause, checked -> IN', + action: { value: 'a', checked: true }, + expected: { + items: [{ key: KEY, op: 'in', value: 'a' }], + expression: `service.name in ['a']`, + }, + }, + { + name: 'no clause, unchecked -> NOT IN', + action: { value: 'a', checked: false }, + expected: { + items: [{ key: KEY, op: 'not in', value: 'a' }], + expression: `service.name not in ['a']`, + }, + }, + { + name: 'no clause, unchecked on infra -> not in', + action: { + value: 'a', + checked: false, + source: QuickFiltersSource.INFRA_MONITORING, + }, + // `nin` is what the source asks for, but re-deriving the expression + // normalises it. Nothing observes the difference: both infra pages send + // `filter.expression` and never `filters.items`. + expected: { + items: [{ key: KEY, op: 'not in', value: 'a' }], + expression: `service.name not in ['a']`, + }, + }, + { + name: 'IN, check another value -> appended', + initial: { items: [{ key: KEY, op: 'in', value: ['a'] }] }, + action: { value: 'b', checked: true }, + expected: { + items: [{ key: KEY, op: 'in', value: ['a', 'b'] }], + expression: `service.name in ['a', 'b']`, + }, + }, + { + name: 'IN, check when value is scalar -> promoted to array', + initial: { items: [{ key: KEY, op: 'in', value: 'a' }] }, + action: { value: 'b', checked: true }, + expected: { + items: [{ key: KEY, op: 'in', value: ['a', 'b'] }], + expression: `service.name in ['a', 'b']`, + }, + }, + { + name: 'IN, uncheck one of many -> filtered out', + initial: { items: [{ key: KEY, op: 'in', value: ['a', 'b'] }] }, + action: { value: 'a', checked: false }, + expected: { + items: [{ key: KEY, op: 'in', value: ['b'] }], + expression: `service.name in ['b']`, + }, + }, + { + name: 'IN, uncheck last value in array -> clause gone', + initial: { items: [{ key: KEY, op: 'in', value: ['a'] }] }, + action: { value: 'a', checked: false }, + expected: { items: [], expression: '' }, + }, + { + name: 'IN, uncheck scalar value -> clause gone', + initial: { items: [{ key: KEY, op: 'in', value: 'a' }] }, + action: { value: 'a', checked: false }, + expected: { items: [], expression: '' }, + }, + { + name: 'IN, uncheck in RELATED section -> replaced by NOT IN for that value', + initial: { items: [{ key: KEY, op: 'in', value: ['a', 'b'] }] }, + action: { value: 'a', checked: false, sectionType: SectionType.RELATED }, + expected: { + items: [{ key: KEY, op: 'not in', value: 'a' }], + expression: `service.name not in ['a']`, + }, + }, + { + name: 'NOT IN, was unchecked then checked -> replaced by IN for that value', + initial: { items: [{ key: KEY, op: 'not in', value: ['a'] }] }, + action: { value: 'b', checked: true, previousState: 'unchecked' }, + expected: { + items: [{ key: KEY, op: 'in', value: 'b' }], + expression: `service.name in ['b']`, + }, + }, + { + name: 'NOT IN, re-checking an excluded value clears it, not flips it to IN', + initial: { items: [{ key: KEY, op: 'not in', value: ['a'] }] }, + action: { value: 'a', checked: true, previousState: 'unchecked' }, + expected: { items: [], expression: '' }, + }, + { + name: 'NOT IN, re-checking one of several excluded values keeps the rest', + initial: { items: [{ key: KEY, op: 'not in', value: ['a', 'b'] }] }, + action: { value: 'a', checked: true, previousState: 'unchecked' }, + expected: { + items: [{ key: KEY, op: 'not in', value: ['b'] }], + expression: `service.name not in ['b']`, + }, + }, + { + name: 'NOT IN, exclude another value -> appended', + initial: { items: [{ key: KEY, op: 'not in', value: ['a'] }] }, + action: { value: 'b', checked: false }, + expected: { + items: [{ key: KEY, op: 'not in', value: ['a', 'b'] }], + expression: `service.name not in ['a', 'b']`, + }, + }, + { + name: 'NOT IN, exclude when scalar -> promoted to array', + initial: { items: [{ key: KEY, op: 'not in', value: 'a' }] }, + action: { value: 'b', checked: false }, + expected: { + items: [{ key: KEY, op: 'not in', value: ['a', 'b'] }], + expression: `service.name not in ['a', 'b']`, + }, + }, + { + name: 'NOT IN, check an excluded value -> removed from array', + initial: { items: [{ key: KEY, op: 'not in', value: ['a', 'b'] }] }, + action: { value: 'a', checked: true }, + expected: { + items: [{ key: KEY, op: 'not in', value: ['b'] }], + expression: `service.name not in ['b']`, + }, + }, + { + name: 'NOT IN, check last excluded value in array -> clause gone', + initial: { items: [{ key: KEY, op: 'not in', value: ['a'] }] }, + action: { value: 'a', checked: true }, + expected: { items: [], expression: '' }, + }, + { + name: 'NOT IN, check excluded scalar value -> clause gone', + initial: { items: [{ key: KEY, op: 'not in', value: 'a' }] }, + action: { value: 'a', checked: true }, + expected: { items: [], expression: '' }, + }, + { + name: '= check another value -> promoted to IN array', + initial: { items: [{ key: KEY, op: '=', value: 'a' }] }, + action: { value: 'b', checked: true }, + expected: { + items: [{ key: KEY, op: 'in', value: ['a', 'b'] }], + expression: `service.name in ['a', 'b']`, + }, + }, + { + name: '= uncheck -> clause gone', + initial: { items: [{ key: KEY, op: '=', value: 'a' }] }, + action: { value: 'a', checked: false }, + expected: { items: [], expression: '' }, + }, + { + name: '!= exclude another value -> promoted to NOT IN array', + initial: { items: [{ key: KEY, op: '!=', value: 'a' }] }, + action: { value: 'b', checked: false }, + expected: { + items: [{ key: KEY, op: 'not in', value: ['a', 'b'] }], + expression: `service.name not in ['a', 'b']`, + }, + }, + { + name: '!= exclude another value on infra -> not in array', + initial: { items: [{ key: KEY, op: '!=', value: 'a' }] }, + action: { + value: 'b', + checked: false, + source: QuickFiltersSource.INFRA_MONITORING, + }, + expected: { + items: [{ key: KEY, op: 'not in', value: ['a', 'b'] }], + expression: `service.name not in ['a', 'b']`, + }, + }, + { + name: '!= check -> clause gone', + initial: { items: [{ key: KEY, op: '!=', value: 'a' }] }, + action: { value: 'a', checked: true }, + expected: { items: [], expression: '' }, + }, + { + name: 'Only with no clause -> IN scalar', + action: { value: 'a', checked: true, isOnlyOrAllClicked: true }, + expected: { + items: [{ key: KEY, op: 'in', value: 'a' }], + expression: `service.name in ['a']`, + }, + }, + { + name: 'Only replaces a multi-value IN with a single value', + initial: { items: [{ key: KEY, op: 'in', value: ['a', 'b'] }] }, + action: { value: 'a', checked: true, isOnlyOrAllClicked: true }, + expected: { + items: [{ key: KEY, op: 'in', value: 'a' }], + expression: `service.name in ['a']`, + }, + }, + { + name: 'All (clicking the sole selected value) -> clause gone', + initial: { items: [{ key: KEY, op: 'in', value: ['a'] }] }, + action: { value: 'a', checked: true, isOnlyOrAllClicked: true }, + expected: { items: [], expression: '' }, + }, + { + name: 'dropping the last clause keeps other keys in the expression', + initial: { + items: [{ key: KEY, op: 'in', value: 'a' }], + expression: `${KEY} = 'a' AND http.method = 'GET'`, + }, + action: { value: 'a', checked: false }, + // The seeded items omit the http.method clause the expression carries; + // re-deriving reconciles it back, which is why items is not empty here. + expected: { + items: [{ key: 'http.method', op: '=', value: 'GET' }], + expression: `http.method = 'GET'`, + }, + }, + { + name: 'dropping the last clause strips the prefixed spelling too', + initial: { + items: [{ key: 'resource.service.name', op: 'in', value: 'a' }], + expression: `resource.service.name = 'a'`, + }, + action: { value: 'a', checked: false }, + expected: { items: [], expression: '' }, + }, + { + name: 'removing the value must keep a free-form clause on the same key', + initial: { + items: [{ key: KEY, op: '=', value: 'a' }], + expression: `${KEY} = 'a' AND ${KEY} CONTAINS 'keepme'`, + }, + action: { value: 'a', checked: false }, + expected: { + items: [{ key: KEY, op: 'contains', value: 'keepme' }], + expression: `service.name CONTAINS 'keepme'`, + }, + }, + { + name: 'a second clause on the same key must not survive an add', + initial: { + items: [{ key: KEY, op: 'in', value: ['a'] }], + expression: `${KEY} IN ['a'] AND ${KEY} != 'z'`, + }, + action: { value: 'b', checked: true }, + expected: { + items: [{ key: KEY, op: 'in', value: ['a', 'b'] }], + expression: `service.name in ['a', 'b']`, + }, + }, +]; + +describe('applyCheckboxToggle (items + shipped expression stay in sync)', () => { + it.each(TOGGLE_CASES)('$name', (c) => { + const got = runToggle(c); + expect(got.items).toStrictEqual(c.expected.items); + expect(got.expression).toBe(c.expected.expression); + }); +}); + +describe('getNotInOperator', () => { + it('returns short "nin" for infra monitoring', () => { + expect(getNotInOperator(QuickFiltersSource.INFRA_MONITORING)).toBe('nin'); + }); + + it('returns long "not in" for other sources', () => { + expect(getNotInOperator(QuickFiltersSource.LOGS_EXPLORER)).toBe('not in'); + expect(getNotInOperator(QuickFiltersSource.TRACES_EXPLORER)).toBe('not in'); + }); +}); + +describe('deriveCheckboxState', () => { + const attributeValues = ['a', 'b', 'c']; + + const state = (items: TagFilterItem[] | undefined): Record => + deriveCheckboxState({ attributeValues, filterItems: items, filterKey: KEY }); + + it('no clause for key -> everything checked', () => { + expect(state([])).toStrictEqual({ a: true, b: true, c: true }); + expect(state(undefined)).toStrictEqual({ a: true, b: true, c: true }); + }); + + it('unrelated clause only -> everything checked', () => { + expect( + state([toTagItem({ key: 'other', op: 'in', value: ['a'] }, 0)]), + ).toStrictEqual({ a: true, b: true, c: true }); + }); + + it('IN [list] -> only listed values checked', () => { + expect( + state([toTagItem({ key: KEY, op: 'in', value: ['a', 'c'] }, 0)]), + ).toStrictEqual({ a: true, b: false, c: true }); + }); + + it('= "value" -> only that value checked', () => { + expect( + state([toTagItem({ key: KEY, op: '=', value: 'b' }, 0)]), + ).toStrictEqual({ a: false, b: true, c: false }); + }); + + it('NOT IN [list] -> everything except excluded checked', () => { + expect( + state([toTagItem({ key: KEY, op: 'not in', value: ['a'] }, 0)]), + ).toStrictEqual({ a: false, b: true, c: true }); + }); + + it('!= "value" -> everything except that value checked', () => { + expect( + state([toTagItem({ key: KEY, op: '!=', value: 'b' }, 0)]), + ).toStrictEqual({ a: true, b: false, c: true }); + }); + + it('matches by base key across context prefixes', () => { + expect( + state([ + toTagItem({ key: 'resource.service.name', op: 'in', value: ['a'] }, 0), + ]), + ).toStrictEqual({ a: true, b: false, c: false }); + }); + + it('coerces boolean / number values to string keys', () => { + expect( + deriveCheckboxState({ + attributeValues: ['true', '42'], + filterItems: [toTagItem({ key: KEY, op: '=', value: true }, 0)], + filterKey: KEY, + }), + ).toStrictEqual({ true: true, '42': false }); + }); +}); + +describe('clearFilterFromQuery', () => { + it('removes the key from items and expression at the active index only', () => { + const query = { + builder: { + queryData: [ + { + filters: { + items: [ + toTagItem({ key: KEY, op: 'in', value: ['a'] }, 0), + toTagItem({ key: 'http.method', op: '=', value: 'GET' }, 1), + ], + op: 'AND', + }, + filter: { expression: `${KEY} = 'a' AND http.method = 'GET'` }, + }, + { + filters: { + items: [toTagItem({ key: KEY, op: 'in', value: ['a'] }, 2)], + op: 'AND', + }, + filter: { expression: `${KEY} = 'a'` }, + }, + ], + }, + } as unknown as Query; + + const result = clearFilterFromQuery({ + currentQuery: query, + filter: { attributeKey: { key: KEY, type: 'tag' } } as never, + activeQueryIndex: 0, + }); + + const active = result.builder.queryData[0]; + expect(active.filters?.items).toStrictEqual([ + expect.objectContaining({ + key: expect.objectContaining({ key: 'http.method' }), + }), + ]); + expect(active.filter?.expression).toBe(`http.method = 'GET'`); + + // Other queries keep both halves: stripping their expression while leaving + // their items alone only churned a clause the round trip put straight back. + const other = result.builder.queryData[1]; + expect(other.filters?.items).toHaveLength(1); + expect(other.filter?.expression).toBe(`${KEY} = 'a'`); + }); +}); diff --git a/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/checkboxFilterQuery.ts b/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/checkboxFilterQuery.ts index 1e855557fb5..1fc1a2d62d2 100644 --- a/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/checkboxFilterQuery.ts +++ b/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/checkboxFilterQuery.ts @@ -1,5 +1,8 @@ /* eslint-disable sonarjs/no-identical-functions */ -import { removeKeysFromExpression } from 'components/QueryBuilderV2/utils'; +import { + convertFiltersToExpressionWithExistingQuery, + removeKeysFromExpression, +} from 'components/QueryBuilderV2/utils'; import { IQuickFiltersConfig, QuickFiltersSource, @@ -10,13 +13,33 @@ import { cloneDeep, isArray } from 'lodash-es'; import { Query, TagFilterItem } from 'types/api/queryBuilder/queryBuilderData'; import { v4 as uuid } from 'uuid'; -import { isKeyMatch } from './utils'; +import { getKeySpellings, isKeyMatch } from './utils'; import { CheckedState } from '../../types'; import { SectionType } from './v2/itemRules'; export const SELECTED_OPERATORS = [OPERATORS['='], 'in']; export const NON_SELECTED_OPERATORS = [OPERATORS['!='], 'not in', 'nin']; +// The operators this algebra emits, and so the only ones it may rewrite out of an +// expression. A hand-written clause on the same key (CONTAINS, EXISTS, a range) is +// none of its business and has to survive a toggle. +const MANAGED_OPERATORS = [OPERATORS['='], OPERATORS['!='], 'in', 'not in']; + +/** + * Drops this filter's own clauses for `key` from `expression`, leaving every other + * key and any clause the checkbox does not manage untouched. Matches all context + * prefixes, since `isKeyMatch` treats `service.name` and `resource.service.name` as + * the same filter but expression rewrites match keys literally. + */ +function removeManagedClauses(expression: string, key: string): string { + return removeKeysFromExpression( + expression, + getKeySpellings(key), + false, + MANAGED_OPERATORS, + ); +} + // Sources that use backend APIs expecting short operator format (e.g., 'nin' instead of 'not in') const SOURCES_WITH_SHORT_OPERATORS = [QuickFiltersSource.INFRA_MONITORING]; @@ -102,8 +125,8 @@ export function deriveCheckboxState({ } /** - * Returns a new query with every clause for this attribute key removed, both - * from the structured filter items and the raw filter expression. + * Returns a new query with this filter's clauses for the attribute key removed from + * the active query, both from the structured filter items and the raw expression. */ export function clearFilterFromQuery({ currentQuery, @@ -118,24 +141,28 @@ export function clearFilterFromQuery({ ...currentQuery, builder: { ...currentQuery.builder, - queryData: currentQuery.builder.queryData.map((item, idx) => ({ - ...item, - filter: { - expression: removeKeysFromExpression(item.filter?.expression ?? '', [ - filter.attributeKey.key, - ]), - }, - filters: { - ...item.filters, - items: - idx === activeQueryIndex - ? item.filters?.items?.filter( - (fil) => !isKeyMatch(fil.key?.key, filter.attributeKey.key), - ) || [] - : [...(item.filters?.items || [])], - op: item.filters?.op || 'AND', - }, - })), + queryData: currentQuery.builder.queryData.map((item, idx) => { + if (idx !== activeQueryIndex) { + return item; + } + return { + ...item, + filter: { + expression: removeManagedClauses( + item.filter?.expression ?? '', + filter.attributeKey.key, + ), + }, + filters: { + ...item.filters, + items: + item.filters?.items?.filter( + (fil) => !isKeyMatch(fil.key?.key, filter.attributeKey.key), + ) || [], + op: item.filters?.op || 'AND', + }, + }; + }), }, }; } @@ -194,12 +221,6 @@ export function applyCheckboxToggle({ (q) => !isKeyMatch(q.key?.key, filter.attributeKey.key), ); - if (query.filter?.expression) { - query.filter.expression = removeKeysFromExpression(query.filter.expression, [ - filter.attributeKey.key, - ]); - } - if (isOnlyOrAll === 'Only') { const newFilterItem: TagFilterItem = { id: uuid(), @@ -267,12 +288,6 @@ export function applyCheckboxToggle({ } return item; }); - if (query.filter?.expression) { - query.filter.expression = removeKeysFromExpression( - query.filter.expression, - [filter.attributeKey.key], - ); - } } else if (isArray(currentFilter.value)) { // if we are removing some value when the running operator is IN we filter. // example - key IN [value1,currentSelectedValue] becomes key IN [value1] in case of array @@ -309,9 +324,10 @@ export function applyCheckboxToggle({ ? currentFilter.value.includes(value) : currentFilter.value === value; - // When clicking unchecked "Other" item, user wants to SELECT it - // Replace NOT IN filter with IN [value] - if (previousState === 'unchecked' && checked) { + // When clicking an unchecked value that is not itself excluded, the user + // wants to SELECT it: replace the NOT IN filter with IN [value]. A value + // that IS in the exclusion list falls through to the removal branch below. + if (previousState === 'unchecked' && checked && !isValueInFilter) { const newFilter: TagFilterItem = { id: uuid(), op: getOperatorValue(OPERATORS.IN), @@ -324,12 +340,6 @@ export function applyCheckboxToggle({ } return item; }); - if (query.filter?.expression) { - query.filter.expression = removeKeysFromExpression( - query.filter.expression, - [filter.attributeKey.key], - ); - } } else if (!checked || !isValueInFilter) { // Add to NOT IN when: // - checked=false (user explicitly unchecked to exclude) @@ -369,12 +379,6 @@ export function applyCheckboxToggle({ query.filters.items = query.filters.items.filter( (item) => !isKeyMatch(item.key?.key, filter.attributeKey.key), ); - if (query.filter?.expression) { - query.filter.expression = removeKeysFromExpression( - query.filter.expression, - [filter.attributeKey.key], - ); - } } else { query.filters.items = query.filters.items.map((item) => { if (isKeyMatch(item.key?.key, filter.attributeKey.key)) { @@ -384,16 +388,6 @@ export function applyCheckboxToggle({ }); } } else { - const newFilter = { - ...currentFilter, - value: currentFilter.value === value ? null : currentFilter.value, - }; - if (newFilter.value === null && query.filter?.expression) { - query.filter.expression = removeKeysFromExpression( - query.filter.expression, - [filter.attributeKey.key], - ); - } query.filters.items = query.filters.items.filter( (item) => !isKeyMatch(item.key?.key, filter.attributeKey.key), ); @@ -456,6 +450,18 @@ export function applyCheckboxToggle({ } } + if (query) { + const synced = convertFiltersToExpressionWithExistingQuery( + query.filters ?? { items: [], op: 'AND' }, + removeManagedClauses( + query.filter?.expression ?? '', + filter.attributeKey.key, + ), + ); + query.filter = synced.filter; + query.filters = synced.filters; + } + return { ...currentQuery, builder: { diff --git a/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/utils.ts b/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/utils.ts index c3dc8d004b3..e17e5b13b26 100644 --- a/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/utils.ts +++ b/frontend/src/components/QuickFilters/FilterRenderers/Checkbox/utils.ts @@ -39,3 +39,16 @@ export function isKeyMatch( ): boolean { return getKeyWithoutPrefix(itemKey) === getKeyWithoutPrefix(filterKey); } + +/** + * Every spelling of a key that `isKeyMatch` treats as equal: the base name plus + * each context-prefixed form. Expression rewrites match keys literally, so they + * need the whole list where the items side only needs `isKeyMatch`. + */ +export function getKeySpellings(key: string | undefined): string[] { + const base = getKeyWithoutPrefix(key); + if (!base) { + return []; + } + return [base, ...FIELD_CONTEXT_PREFIXES.map((prefix) => `${prefix}.${base}`)]; +} diff --git a/pkg/alertmanager/alertmanagernotify/jira/jira.go b/pkg/alertmanager/alertmanagernotify/jira/jira.go index 638dfacb4f7..352cff3ed85 100644 --- a/pkg/alertmanager/alertmanagernotify/jira/jira.go +++ b/pkg/alertmanager/alertmanagernotify/jira/jira.go @@ -433,7 +433,7 @@ func (n *Notifier) resolveAPIBaseURL(ctx context.Context) (string, bool, error) // resolveCloudID fetches the site's cloud id from its unauthenticated // tenant_info endpoint; transport failures are retryable, bad responses are not. func (n *Notifier) resolveCloudID(ctx context.Context) (string, bool, error) { - url := strings.TrimRight(n.conf.Site, "/") + "/_edge/tenant_info" + url := n.conf.Site + "/_edge/tenant_info" req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return "", false, err diff --git a/pkg/types/alertmanagertypes/jira.go b/pkg/types/alertmanagertypes/jira.go index 08d9e558c1e..396cc394795 100644 --- a/pkg/types/alertmanagertypes/jira.go +++ b/pkg/types/alertmanagertypes/jira.go @@ -80,12 +80,18 @@ func (c *JiraReceiverConfig) UnmarshalYAML(unmarshal func(any) error) error { c.Description = DefaultJiraDescriptionTemplate } - site := strings.TrimRight(strings.TrimSpace(c.Site), "/") - u, err := url.Parse(site) - if site == "" || err != nil || u.Scheme != "https" || !strings.HasSuffix(strings.ToLower(u.Hostname()), jiraCloudHostSuffix) { + // Values are stored and sent exactly as configured, so anything that is + // not already canonical is rejected rather than rewritten. + if c.Site != strings.TrimSpace(c.Site) { + return errors.New(errors.TypeInvalidInput, errors.CodeInvalidInput, "jira site must not have leading or trailing whitespace") + } + u, err := url.Parse(c.Site) + if c.Site == "" || err != nil || u.Scheme != "https" || !strings.HasSuffix(strings.ToLower(u.Hostname()), jiraCloudHostSuffix) { return errors.New(errors.TypeInvalidInput, errors.CodeInvalidInput, fmt.Sprintf("jira site must be a Jira Cloud URL (https://%s)", jiraCloudHostSuffix)) } - c.Site = site + if strings.HasSuffix(c.Site, "/") { + return errors.New(errors.TypeInvalidInput, errors.CodeInvalidInput, "jira site must not end with a trailing slash") + } if c.Project == "" { return errors.New(errors.TypeInvalidInput, errors.CodeInvalidInput, "jira project is required") @@ -115,5 +121,5 @@ func (c *JiraReceiverConfig) APIBaseURL(cloudID string) string { if cloudID != "" { return fmt.Sprintf("%s%s/rest/api/3", jiraGatewayBaseURL, cloudID) } - return fmt.Sprintf("%s/rest/api/3", strings.TrimRight(c.Site, "/")) + return fmt.Sprintf("%s/rest/api/3", c.Site) } diff --git a/pkg/types/alertmanagertypes/jira_test.go b/pkg/types/alertmanagertypes/jira_test.go index d3cf95b065b..4ca17163baa 100644 --- a/pkg/types/alertmanagertypes/jira_test.go +++ b/pkg/types/alertmanagertypes/jira_test.go @@ -87,13 +87,6 @@ func TestJiraIsServiceAccount(t *testing.T) { assert.False(t, (&JiraReceiverConfig{}).IsServiceAccount()) } -func TestJiraReceiverConfigTrailingSlashSite(t *testing.T) { - r, err := NewReceiver(jiraReceiverJSON("https://acme.atlassian.net/", "KAN", "Task", true)) - require.NoError(t, err) - assert.Equal(t, "https://acme.atlassian.net", r.JiraConfigs[0].Site) - assert.Equal(t, "https://acme.atlassian.net/rest/api/3", r.JiraConfigs[0].APIBaseURL("")) -} - func TestJiraReceiverConfigValidation(t *testing.T) { cases := []struct { name string @@ -104,6 +97,8 @@ func TestJiraReceiverConfigValidation(t *testing.T) { {"non-cloud host", jiraReceiverJSON("https://jira.acme.com", "KAN", "Task", true)}, {"lookalike host suffix", jiraReceiverJSON("https://www.iamnotatlassian.net", "KAN", "Task", true)}, {"bare atlassian.net", jiraReceiverJSON("https://atlassian.net", "KAN", "Task", true)}, + {"trailing slash site", jiraReceiverJSON("https://acme.atlassian.net/", "KAN", "Task", true)}, + {"padded site", jiraReceiverJSON(" https://acme.atlassian.net ", "KAN", "Task", true)}, {"missing project", jiraReceiverJSON("https://acme.atlassian.net", "", "Task", true)}, {"missing issue_type", jiraReceiverJSON("https://acme.atlassian.net", "KAN", "", true)}, {"missing basic auth", jiraReceiverJSON("https://acme.atlassian.net", "KAN", "Task", false)}, diff --git a/pkg/valuer/unset_or_non_empty_string.go b/pkg/valuer/unset_or_non_empty_string.go new file mode 100644 index 00000000000..ecf52f2c923 --- /dev/null +++ b/pkg/valuer/unset_or_non_empty_string.go @@ -0,0 +1,113 @@ +package valuer + +import ( + "database/sql/driver" + "encoding/json" + + "github.com/SigNoz/signoz/pkg/errors" +) + +var _ Valuer = (*UnsetOrNonEmptyString)(nil) + +// UnsetOrNonEmptyString separates a field left out of the input from one +// explicitly set to "". Decoding rejects "", and json calls UnmarshalJSON only +// for a field that is present, so a field holding "" is a field nobody set. +type UnsetOrNonEmptyString struct { + val string +} + +func NewUnsetOrNonEmptyString(val string) (UnsetOrNonEmptyString, error) { + if val == "" { + return UnsetOrNonEmptyString{}, errors.Newf(errors.TypeInvalidInput, ErrCodeInvalidValuer, "string must not be empty") + } + + return UnsetOrNonEmptyString{val: val}, nil +} + +func MustNewUnsetOrNonEmptyString(val string) UnsetOrNonEmptyString { + nonEmptyString, err := NewUnsetOrNonEmptyString(val) + if err != nil { + panic(err) + } + + return nonEmptyString +} + +func (enum UnsetOrNonEmptyString) IsZero() bool { + return enum.val == "" +} + +func (enum UnsetOrNonEmptyString) StringValue() string { + return enum.val +} + +func (enum UnsetOrNonEmptyString) String() string { + return enum.val +} + +func (enum UnsetOrNonEmptyString) MarshalJSON() ([]byte, error) { + return json.Marshal(enum.StringValue()) +} + +func (enum *UnsetOrNonEmptyString) UnmarshalJSON(data []byte) error { + var str string + if err := json.Unmarshal(data, &str); err != nil { + return err + } + + var err error + *enum, err = NewUnsetOrNonEmptyString(str) + if err != nil { + return err + } + + return nil +} + +func (enum UnsetOrNonEmptyString) Value() (driver.Value, error) { + return enum.StringValue(), nil +} + +func (enum *UnsetOrNonEmptyString) Scan(val any) error { + if enum == nil { + return errors.Newf(errors.TypeInternal, ErrCodeUnknownValuerScan, "unset_or_non_empty_string: (nil \"%T\")", enum) + } + + str, ok := val.(string) + if !ok { + return errors.Newf(errors.TypeInternal, ErrCodeUnknownValuerScan, "unset_or_non_empty_string: (non-string \"%T\")", val) + } + + var err error + *enum, err = NewUnsetOrNonEmptyString(str) + if err != nil { + return err + } + + return nil +} + +func (enum *UnsetOrNonEmptyString) UnmarshalText(text []byte) error { + var err error + *enum, err = NewUnsetOrNonEmptyString(string(text)) + if err != nil { + return err + } + + return nil +} + +func (enum UnsetOrNonEmptyString) MarshalText() (text []byte, err error) { + return []byte(enum.StringValue()), nil +} + +func (enum *UnsetOrNonEmptyString) UnmarshalParam(param string) error { + nonEmptyString, err := NewUnsetOrNonEmptyString(param) + if err != nil { + return err + } + + *enum = nonEmptyString + + return nil +} diff --git a/pkg/valuer/unset_or_non_empty_string_test.go b/pkg/valuer/unset_or_non_empty_string_test.go new file mode 100644 index 00000000000..47f0179992a --- /dev/null +++ b/pkg/valuer/unset_or_non_empty_string_test.go @@ -0,0 +1,92 @@ +package valuer + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewUnsetOrNonEmptyString(t *testing.T) { + testCases := []struct { + description string + value string + expectedError bool + expectedWrapped UnsetOrNonEmptyString + }{ + {description: "plain value", value: "oncall", expectedWrapped: UnsetOrNonEmptyString{val: "oncall"}}, + {description: "case and surrounding space are kept", value: " On Call ", expectedWrapped: UnsetOrNonEmptyString{val: " On Call "}}, + {description: "whitespace alone is not empty", value: " ", expectedWrapped: UnsetOrNonEmptyString{val: " "}}, + {description: "empty", value: "", expectedError: true}, + } + + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + nonEmptyString, err := NewUnsetOrNonEmptyString(testCase.value) + if testCase.expectedError { + assert.Error(t, err) + return + } + + require.NoError(t, err) + assert.Equal(t, testCase.expectedWrapped, nonEmptyString) + }) + } +} + +func TestUnsetOrNonEmptyStringUnmarshalJSONRejectsAnEmptyString(t *testing.T) { + var target struct { + Title UnsetOrNonEmptyString `json:"title"` + } + + require.NoError(t, json.Unmarshal([]byte(`{"title":"Alert"}`), &target)) + assert.Equal(t, "Alert", target.Title.StringValue()) + + assert.Error(t, json.Unmarshal([]byte(`{"title":""}`), &target)) +} + +// The zero value is the only way an empty UnsetOrNonEmptyString comes about, and it is +// what lets a caller omit the field and take a default filled in elsewhere. +func TestUnsetOrNonEmptyStringUnmarshalJSONLeavesAnAbsentFieldZero(t *testing.T) { + var target struct { + Title UnsetOrNonEmptyString `json:"title"` + } + + require.NoError(t, json.Unmarshal([]byte(`{}`), &target)) + assert.True(t, target.Title.IsZero()) +} + +func TestUnsetOrNonEmptyStringMarshalJSON(t *testing.T) { + raw, err := json.Marshal(MustNewUnsetOrNonEmptyString("Alert")) + require.NoError(t, err) + assert.JSONEq(t, `"Alert"`, string(raw)) +} + +func TestUnsetOrNonEmptyStringScanRejectsAnEmptyString(t *testing.T) { + var nonEmptyString UnsetOrNonEmptyString + + require.NoError(t, nonEmptyString.Scan("oncall")) + assert.Equal(t, "oncall", nonEmptyString.StringValue()) + + assert.Error(t, nonEmptyString.Scan("")) + assert.Error(t, nonEmptyString.Scan(nil)) +} + +func TestUnsetOrNonEmptyStringUnmarshalTextRejectsAnEmptyString(t *testing.T) { + var nonEmptyString UnsetOrNonEmptyString + + require.NoError(t, nonEmptyString.UnmarshalText([]byte("oncall"))) + assert.Equal(t, "oncall", nonEmptyString.StringValue()) + + assert.Error(t, nonEmptyString.UnmarshalText([]byte(""))) +} + +func TestUnsetOrNonEmptyStringUnmarshalParamRejectsAnEmptyString(t *testing.T) { + var nonEmptyString UnsetOrNonEmptyString + + require.NoError(t, nonEmptyString.UnmarshalParam("oncall")) + assert.Equal(t, "oncall", nonEmptyString.StringValue()) + + assert.Error(t, nonEmptyString.UnmarshalParam("")) +}