fix(query-builder): Handle commas in multi-select filter values (#77866)

Commas in filter values were a problem because we use `,` as a separator
between values. I was naively parsing the filter value with a
`str.split(',')` which was not sufficient for handling quoted values
with commas.

This change adds a parser that is similar to the one used for the syntax
tokenizer, but a bit more permissive since we clean the user input
before committing the value. This is a much more robust solution which
handles quoted values.

static/app/components/searchQueryBuilder/index.spec.tsx

@@ -1718,7 +1718,8 @@ describe('SearchQueryBuilder', function () {
         ['spaces', 'a b', '"a b"'],
         ['quotes', 'a"b', '"a\\"b"'],
         ['parens', 'foo()', '"foo()"'],
-      ])('tag values escape %s', async (_, value, expected) => {
+        ['commas', '"a,b"', '"a,b"'],
+      ])('typed tag values escape %s', async (_, value, expected) => {
         const mockOnChange = jest.fn();
         render(
           <SearchQueryBuilder
@@ -1742,6 +1743,52 @@ describe('SearchQueryBuilder', function () {
         });
       });
 
+      it.each([
+        ['spaces', 'a b', '"a b"'],
+        ['quotes', 'a"b', '"a\\"b"'],
+        ['parens', 'foo()', '"foo()"'],
+        ['commas', 'a,b', '"a,b"'],
+      ])('selected tag value suggestions escape %s', async (_, value, expected) => {
+        const mockOnChange = jest.fn();
+        const mockGetTagValues = jest.fn().mockResolvedValue([value]);
+        render(
+          <SearchQueryBuilder
+            {...defaultProps}
+            onChange={mockOnChange}
+            initialQuery="custom_tag_name:"
+            getTagValues={mockGetTagValues}
+          />
+        );
+
+        await userEvent.click(
+          screen.getByRole('button', {name: 'Edit value for filter: custom_tag_name'})
+        );
+        await userEvent.click(await screen.findByRole('option', {name: value}));
+
+        // Value should be surrounded by quotes and escaped
+        await waitFor(() => {
+          expect(mockOnChange).toHaveBeenCalledWith(
+            `custom_tag_name:${expected}`,
+            expect.anything()
+          );
+        });
+
+        // Open menu again and check to see if value is correct
+        await userEvent.click(
+          screen.getByRole('button', {name: 'Edit value for filter: custom_tag_name'})
+        );
+
+        // Input value should have the escaped value (with a trailing comma)
+        expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveValue(
+          expected + ','
+        );
+
+        // The original value should be selected in the dropdown
+        expect(
+          within(await screen.findByRole('option', {name: value})).getByRole('checkbox')
+        ).toBeChecked();
+      });
+
       it('can replace a value with a new one', async function () {
         render(
           <SearchQueryBuilder {...defaultProps} initialQuery="browser.name:[1,c,3]" />

static/app/components/searchQueryBuilder/index.stories.tsx

@@ -48,7 +48,7 @@ const FILTER_KEYS: TagCollection = {
     name: 'Browser Name',
     kind: FieldKind.FIELD,
     predefined: true,
-    values: ['Chrome', 'Firefox', 'Safari', 'Edge'],
+    values: ['Chrome', 'Firefox', 'Safari', 'Edge', 'Internet Explorer', 'Opera 1,2'],
   },
   [FieldKey.IS]: {
     key: FieldKey.IS,

static/app/components/searchQueryBuilder/tokens/filter/parametersCombobox.tsx

@@ -7,7 +7,7 @@ import {getEscapedKey} from 'sentry/components/compactSelect/utils';
 import {useSearchQueryBuilder} from 'sentry/components/searchQueryBuilder/context';
 import {SearchQueryBuilderCombobox} from 'sentry/components/searchQueryBuilder/tokens/combobox';
 import {FunctionDescription} from 'sentry/components/searchQueryBuilder/tokens/filter/functionDescription';
-import {replaceCommaSeparatedValue} from 'sentry/components/searchQueryBuilder/tokens/filter/utils';
+import {replaceCommaSeparatedValue} from 'sentry/components/searchQueryBuilder/tokens/filter/replaceCommaSeparatedValue';
 import type {AggregateFilter} from 'sentry/components/searchSyntax/parser';
 import {t} from 'sentry/locale';
 import {FieldKind, FieldValueType} from 'sentry/utils/fields';

static/app/components/searchQueryBuilder/tokens/filter/parsers/string/grammar.pegjs

@@ -0,0 +1,44 @@
+{
+    const { TokenConverter, config = {} } = options;
+    const tc = new TokenConverter({text, location, config});
+}
+
+text_in_list
+  = item1:text_in_value
+    items:item* {
+      return tc.tokenValueTextList(item1, items);
+    }
+
+item = s1:spaces c:comma s2:spaces value:(!comma text_in_value)? {
+  return [s1, c, s2, value ?? [undefined, tc.tokenValueText('', false)]];
+}
+
+text_in_value
+  = quoted_value / in_value / empty_value
+
+empty_value
+  = spaces {
+    return tc.tokenValueText(text(), false);
+  }
+
+in_value
+  = (in_value_char)+ {
+    return tc.tokenValueText(text(), false);
+  }
+
+quoted_value
+  = '"' value:('\\"' / [^"])* '"' {
+    return tc.tokenValueText(value.join(''), true);
+  }
+
+in_value_termination
+  = in_value_char (!in_value_end in_value_char)* in_value_end
+
+in_value_char
+  = [^,]
+
+in_value_end
+  = (spaces comma)
+
+comma = ","
+spaces = " "*

static/app/components/searchQueryBuilder/tokens/filter/parsers/string/parser.spec.tsx

@@ -0,0 +1,97 @@
+import {parseMultiSelectFilterValue} from 'sentry/components/searchQueryBuilder/tokens/filter/parsers/string/parser';
+
+describe('parseMultiSelectValue', function () {
+  it('single value', function () {
+    const result = parseMultiSelectFilterValue('a');
+
+    expect(result).not.toBeNull();
+
+    expect(result!.items).toHaveLength(1);
+    expect(result?.items[0].value?.value).toEqual('a');
+  });
+
+  it('multiple value', function () {
+    const result = parseMultiSelectFilterValue('a,b,c');
+
+    expect(result).not.toBeNull();
+
+    expect(result!.items).toHaveLength(3);
+    expect(result?.items[0].value?.value).toEqual('a');
+    expect(result?.items[1].value?.value).toEqual('b');
+    expect(result?.items[2].value?.value).toEqual('c');
+  });
+
+  it('quoted value', function () {
+    const result = parseMultiSelectFilterValue('a,"b",c');
+
+    expect(result).not.toBeNull();
+
+    expect(result!.items).toHaveLength(3);
+    expect(result?.items[0].value?.value).toEqual('a');
+
+    expect(result?.items[1].value?.value).toEqual('b');
+    expect(result?.items[1].value?.text).toEqual('"b"');
+    expect(result?.items[1].value?.quoted).toBe(true);
+
+    expect(result?.items[2].value?.value).toEqual('c');
+  });
+
+  it('just quotes', function () {
+    const result = parseMultiSelectFilterValue('""');
+
+    expect(result).not.toBeNull();
+
+    expect(result!.items).toHaveLength(1);
+    const item = result!.items[0];
+
+    expect(item.value?.value).toEqual('');
+    expect(item.value?.text).toEqual('""');
+    expect(item.value?.quoted).toBe(true);
+  });
+
+  it('single empty value', function () {
+    const result = parseMultiSelectFilterValue('');
+
+    expect(result).not.toBeNull();
+
+    expect(result!.items).toHaveLength(1);
+    const item = result!.items[0];
+
+    expect(item.value!.value).toBe('');
+  });
+
+  it('multiple empty value', function () {
+    const result = parseMultiSelectFilterValue('a,,b');
+
+    expect(result).not.toBeNull();
+
+    expect(result!.items).toHaveLength(3);
+
+    expect(result?.items[0].value?.value).toEqual('a');
+    expect(result?.items[1].value?.value).toBe('');
+    expect(result?.items[2].value?.value).toEqual('b');
+  });
+
+  it('trailing comma', function () {
+    const result = parseMultiSelectFilterValue('a,');
+
+    expect(result).not.toBeNull();
+
+    expect(result!.items).toHaveLength(2);
+
+    expect(result?.items[0].value?.value).toEqual('a');
+    expect(result?.items[1].value?.value).toBe('');
+  });
+
+  it('spaces', function () {
+    const result = parseMultiSelectFilterValue('a,b c,d');
+
+    expect(result).not.toBeNull();
+
+    expect(result!.items).toHaveLength(3);
+
+    expect(result?.items[0].value?.value).toEqual('a');
+    expect(result?.items[1].value?.value).toEqual('b c');
+    expect(result?.items[2].value?.value).toEqual('d');
+  });
+});

static/app/components/searchQueryBuilder/tokens/filter/parsers/string/parser.tsx

@@ -0,0 +1,24 @@
+import {
+  type Token,
+  TokenConverter,
+  type TokenResult,
+} from 'sentry/components/searchSyntax/parser';
+
+import grammar from './grammar.pegjs';
+
+/**
+ * Parses the user input value of a multi select filter.
+ *
+ * This is different from the search syntax parser in the following ways:
+ * - Does not look for surrounding []
+ * - Does not disallow spaces or parens outside of quoted values
+ */
+export function parseMultiSelectFilterValue(
+  value: string
+): TokenResult<Token.VALUE_TEXT_LIST> | null {
+  try {
+    return grammar.parse(value, {TokenConverter});
+  } catch (e) {
+    return null;
+  }
+}

static/app/components/searchQueryBuilder/tokens/filter/replaceCommaSeparatedValue.spec.tsx

@@ -0,0 +1,31 @@
+import {replaceCommaSeparatedValue} from 'sentry/components/searchQueryBuilder/tokens/filter/replaceCommaSeparatedValue';
+
+describe('replaceCommaSeparatedValue', function () {
+  it('replaces a value without commas', function () {
+    expect(replaceCommaSeparatedValue('foo', 3, 'bar')).toBe('bar');
+  });
+
+  it('replaces an empty value at end', function () {
+    expect(replaceCommaSeparatedValue('foo,', 4, 'bar')).toBe('foo,bar');
+  });
+
+  it('replaces an empty value at start', function () {
+    expect(replaceCommaSeparatedValue(',foo', 0, 'bar')).toBe('bar,foo');
+  });
+
+  it('replaces an empty value in middle', function () {
+    expect(replaceCommaSeparatedValue('foo,,baz', 4, 'bar')).toBe('foo,bar,baz');
+  });
+
+  it('replaces an non-empty value at end', function () {
+    expect(replaceCommaSeparatedValue('foo,abc', 4, 'bar')).toBe('foo,bar');
+  });
+
+  it('replaces an non-empty value at start', function () {
+    expect(replaceCommaSeparatedValue('abc,foo', 0, 'bar')).toBe('bar,foo');
+  });
+
+  it('replaces an non-empty value in middle', function () {
+    expect(replaceCommaSeparatedValue('foo,abc,baz', 4, 'bar')).toBe('foo,bar,baz');
+  });
+});

static/app/components/searchQueryBuilder/tokens/filter/replaceCommaSeparatedValue.tsx

@@ -0,0 +1,41 @@
+import {parseMultiSelectFilterValue} from 'sentry/components/searchQueryBuilder/tokens/filter/parsers/string/parser';
+
+/**
+ * Replaces the focused parameter (at cursorPosition) with the new value.
+ * If cursorPosition is null, will default to the end of the string.
+ *
+ * Example:
+ * replaceCommaSeparatedValue('foo,bar,baz', 5, 'new') => 'foo,new,baz'
+ */
+export function replaceCommaSeparatedValue(
+  value: string,
+  cursorPosition: number | null,
+  replacement: string
+) {
+  const parsed = parseMultiSelectFilterValue(value);
+
+  if (!parsed) {
+    return value;
+  }
+
+  if (cursorPosition === null) {
+    cursorPosition = value.length;
+  }
+
+  const matchingIndex = parsed.items.findIndex(
+    item =>
+      item.value &&
+      item.value?.location.start.offset <= cursorPosition &&
+      item.value?.location.end.offset >= cursorPosition
+  );
+
+  if (matchingIndex === -1) {
+    return replacement;
+  }
+
+  return [
+    ...parsed.items.slice(0, matchingIndex).map(item => item.value?.text ?? ''),
+    replacement,
+    ...parsed.items.slice(matchingIndex + 1).map(item => item.value?.text ?? ''),
+  ].join(',');
+}

static/app/components/searchQueryBuilder/tokens/filter/utils.tsx

@@ -16,7 +16,7 @@ import {
   getFieldDefinition,
 } from 'sentry/utils/fields';
 
-const SHOULD_ESCAPE_REGEX = /[\s"()]/;
+const SHOULD_ESCAPE_REGEX = /[\s"(),]/;
 
 export function isAggregateFilterToken(
   token: TokenResult<Token.FILTER>
@@ -91,32 +91,6 @@ export function formatFilterValue(token: TokenResult<Token.FILTER>['value']): st
   }
 }
 
-/**
- * Replaces the focused parameter (at cursorPosition) with the new value.
- * If cursorPosition is null, will default to the end of the string.
- *
- * Example:
- * replaceCommaSeparatedValue('foo,bar,baz', 5, 'new') => 'foo,new,baz'
- */
-export function replaceCommaSeparatedValue(
-  value: string,
-  cursorPosition: number | null,
-  replacement: string
-) {
-  const items = value.split(',');
-
-  let characterCount = 0;
-  for (let i = 0; i < items.length; i++) {
-    characterCount += items[i].length + 1;
-    if (characterCount > (cursorPosition ?? value.length + 1)) {
-      const newItems = [...items.slice(0, i), replacement, ...items.slice(i + 1)];
-      return newItems.map(item => item.trim()).join(',');
-    }
-  }
-
-  return value;
-}
-
 /**
  * Gets the value type for a given token.
  *

static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx

@@ -12,12 +12,13 @@ import {
   type CustomComboboxMenu,
   SearchQueryBuilderCombobox,
 } from 'sentry/components/searchQueryBuilder/tokens/combobox';
+import {parseMultiSelectFilterValue} from 'sentry/components/searchQueryBuilder/tokens/filter/parsers/string/parser';
+import {replaceCommaSeparatedValue} from 'sentry/components/searchQueryBuilder/tokens/filter/replaceCommaSeparatedValue';
 import SpecificDatePicker from 'sentry/components/searchQueryBuilder/tokens/filter/specificDatePicker';
 import {
   escapeTagValue,
   formatFilterValue,
   getFilterValueType,
-  replaceCommaSeparatedValue,
   unescapeTagValue,
 } from 'sentry/components/searchQueryBuilder/tokens/filter/utils';
 import {ValueListBox} from 'sentry/components/searchQueryBuilder/tokens/filter/valueListBox';
@@ -74,15 +75,19 @@ function isStringFilterValues(
 }
 
 function getMultiSelectInputValue(token: TokenResult<Token.FILTER>) {
+  // Even if this is a multi-select filter, it won't be parsed as such if only a single value is provided
   if (
     token.value.type !== Token.VALUE_TEXT_LIST &&
     token.value.type !== Token.VALUE_NUMBER_LIST
   ) {
-    const value = token.value.value;
-    return value ? value + ',' : '';
+    if (!token.value.value) {
+      return '';
+    }
+
+    return token.value.text + ',';
   }
 
-  const items = token.value.items.map(item => item.value.value);
+  const items = token.value.items.map(item => item.value?.text ?? '');
 
   if (items.length === 0) {
     return '';
@@ -92,21 +97,45 @@ function getMultiSelectInputValue(token: TokenResult<Token.FILTER>) {
 }
 
 function prepareInputValueForSaving(valueType: FieldValueType, inputValue: string) {
-  const values = uniq(
-    inputValue
-      .split(',')
-      .map(v => cleanFilterValue({valueType, value: v.trim()}))
-      .filter(v => v && v.length > 0)
-  );
+  const parsed = parseMultiSelectFilterValue(inputValue);
+
+  if (!parsed) {
+    return '""';
+  }
+
+  const values =
+    parsed.items
+      .map(item =>
+        item.value?.quoted
+          ? item.value?.text ?? ''
+          : cleanFilterValue({valueType, value: item.value?.text ?? ''})
+      )
+      .filter(text => text?.length) ?? [];
 
-  return values.length > 1 ? `[${values.join(',')}]` : values[0] ?? '""';
+  const uniqueValues = uniq(values);
+
+  return uniqueValues.length > 1
+    ? `[${uniqueValues.join(',')}]`
+    : uniqueValues[0] ?? '""';
 }
 
-function getSelectedValuesFromText(text: string) {
-  return text
-    .split(',')
-    .map(v => unescapeTagValue(v.trim()))
-    .filter(v => v.length > 0);
+function getSelectedValuesFromText(
+  text: string,
+  {escaped = true}: {escaped?: boolean} = {}
+) {
+  const parsed = parseMultiSelectFilterValue(text);
+
+  if (!parsed) {
+    return [];
+  }
+
+  return parsed.items
+    .filter(item => item.value?.value)
+    .map(item => {
+      return (
+        (escaped ? item.value?.text : unescapeTagValue(item.value?.value ?? '')) ?? ''
+      );
+    });
 }
 
 function getValueAtCursorPosition(text: string, cursorPosition: number | null) {
@@ -489,8 +518,11 @@ export function SearchQueryBuilderValueCombobox({
     ? getValueAtCursorPosition(inputValue, selectionIndex)
     : inputValue;
 
-  const selectedValues = useMemo(
-    () => (canSelectMultipleValues ? getSelectedValuesFromText(inputValue) : []),
+  const selectedValuesUnescaped = useMemo(
+    () =>
+      canSelectMultipleValues
+        ? getSelectedValuesFromText(inputValue, {escaped: false})
+        : [],
     [canSelectMultipleValues, inputValue]
   );
 
@@ -517,7 +549,7 @@ export function SearchQueryBuilderValueCombobox({
   const {items, suggestionSectionItems, isFetching} = useFilterSuggestions({
     token,
     filterValue,
-    selectedValues,
+    selectedValues: selectedValuesUnescaped,
     ctrlKeyPressed,
   });
 
@@ -553,11 +585,15 @@ export function SearchQueryBuilderValueCombobox({
       }
 
       if (canSelectMultipleValues) {
-        if (selectedValues.includes(value)) {
+        if (selectedValuesUnescaped.includes(value)) {
           const newValue = prepareInputValueForSaving(
             getFilterValueType(token, fieldDefinition),
-            selectedValues.filter(v => v !== value).join(',')
+            selectedValuesUnescaped
+              .filter(v => v !== value)
+              .map(escapeTagValue)
+              .join(',')
           );
+
           dispatch({
             type: 'UPDATE_TOKEN_VALUE',
             token: token,
@@ -576,7 +612,7 @@ export function SearchQueryBuilderValueCombobox({
           token: token,
           value: prepareInputValueForSaving(
             getFilterValueType(token, fieldDefinition),
-            replaceCommaSeparatedValue(inputValue, selectionIndex, value)
+            replaceCommaSeparatedValue(inputValue, selectionIndex, escapeTagValue(value))
           ),
         });
 
@@ -595,16 +631,16 @@ export function SearchQueryBuilderValueCombobox({
       return true;
     },
     [
-      analyticsData,
+      token,
+      fieldDefinition,
       canSelectMultipleValues,
+      analyticsData,
+      selectedValuesUnescaped,
       dispatch,
-      fieldDefinition,
       inputValue,
-      onCommit,
-      selectedValues,
       selectionIndex,
-      token,
       ctrlKeyPressed,
+      onCommit,
     ]
   );