Browse Source

feat(search-syntax): Add option to flatten paren groups (#70954)

Ref https://github.com/getsentry/sentry/issues/70953

For the new search component, parens will need to be parsed as separate
tokens and the parser cannot error out when faced with mismatched
parens. Therefore the parser should be updated to allow for this.

I've added a new token type, `PAREN`, which may be returned as part of
the parse result. When there are mismatched parentheses, this is now
matched which allows the parser to return a successful result. A new
config option has been added, `flattenParenGroups` (name could be
workshopped), which causes the `PAREN` token to take precedence over
`LOGIC_GROUP` and thus return parens separately from the contents when
enabled. When disabled (the default), logic groups are still returned as
you would expect.

Previous behavior:

- Unmatched parens will throw an error and return nothing from the
parser
- Matched parens are parsed as a `LOGIC_GROUP` with boolean operators,
free text, and filters inside the `token.inner` array.

New behavior (with the default `flattenParenGroups=false`):

- Unmatched parens will not throw an error. The parser will return a
normal result, with the unmatched parens parsed as a `PAREN` token.
 - Matched parens will return a `LOGIC_GROUP` as normal.

New behavior (with the new `flattenParenGroups=true`):

- Unmatched parens will not throw an error. The parser will return a
normal result, with the unmatched parens parsed as a `PAREN` token.
- Matched parens will _not_ return a `LOGIC_GROUP`

You can see now that syntax highlighting is no longer broken when adding
a mismatched paren.
Malachi Willey 9 months ago
parent
commit
0ba16d9d41

+ 9 - 4
static/app/components/searchSyntax/grammar.pegjs

@@ -11,7 +11,10 @@ search
     }
 
 term
-  = (boolean_operator / paren_group / filter / free_text) spaces
+  = (boolean_operator / paren_group / open_paren / closed_paren / filter / free_text) spaces
+
+term_no_paren
+  = (boolean_operator / paren_group /  filter / free_text) spaces
 
 boolean_operator
   = (or_operator / and_operator) {
@@ -19,7 +22,9 @@ boolean_operator
     }
 
 paren_group
-  = open_paren spaces:spaces inner:term+ closed_paren {
+  = open_paren spaces:spaces inner:term_no_paren* closed_paren &{
+    return tc.predicateParenGroup();
+  } {
       return tc.tokenLogicGroup([spaces, ...inner].flat());
     }
 
@@ -377,8 +382,8 @@ operator       = ">=" / "<=" / ">" / "<" / "=" / "!="
 or_operator    = "OR"i  &end_value
 and_operator   = "AND"i &end_value
 numeric        = [0-9]+ ("." [0-9]*)? { return text(); }
-open_paren     = "("
-closed_paren   = ")"
+open_paren     = "(" { return tc.tokenLParen(text()); }
+closed_paren   = ")" { return tc.tokenRParen(text()); }
 open_bracket   = "["
 closed_bracket = "]"
 sep            = ":"

+ 131 - 0
static/app/components/searchSyntax/parser.spec.tsx

@@ -207,4 +207,135 @@ describe('searchSyntax/parser', function () {
       reason: 'Custom message',
     });
   });
+
+  describe('flattenParenGroups', () => {
+    it('tokenizes mismatched parens with flattenParenGroups=true', () => {
+      const result = parseSearch('foo(', {
+        flattenParenGroups: true,
+      });
+
+      if (result === null) {
+        throw new Error('Parsed result as null');
+      }
+
+      // foo( is parsed as free text a single paren
+      expect(result).toEqual([
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({type: Token.FREE_TEXT}),
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({
+          type: Token.L_PAREN,
+          value: '(',
+        }),
+        expect.objectContaining({type: Token.SPACES}),
+      ]);
+    });
+
+    it('tokenizes matching parens with flattenParenGroups=true', () => {
+      const result = parseSearch('(foo)', {
+        flattenParenGroups: true,
+      });
+
+      if (result === null) {
+        throw new Error('Parsed result as null');
+      }
+
+      // (foo) is parsed as free text and two parens
+      expect(result).toEqual([
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({
+          type: Token.L_PAREN,
+          value: '(',
+        }),
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({type: Token.FREE_TEXT}),
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({
+          type: Token.R_PAREN,
+          value: ')',
+        }),
+        expect.objectContaining({type: Token.SPACES}),
+      ]);
+    });
+
+    it('tokenizes mismatched left paren with flattenParenGroups=false', () => {
+      const result = parseSearch('foo(', {
+        flattenParenGroups: false,
+      });
+
+      if (result === null) {
+        throw new Error('Parsed result as null');
+      }
+
+      // foo( is parsed as free text and a paren
+      expect(result).toEqual([
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({type: Token.FREE_TEXT}),
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({
+          type: Token.L_PAREN,
+          value: '(',
+        }),
+        expect.objectContaining({type: Token.SPACES}),
+      ]);
+    });
+
+    it('tokenizes mismatched right paren with flattenParenGroups=false', () => {
+      const result = parseSearch('foo)', {
+        flattenParenGroups: false,
+      });
+
+      if (result === null) {
+        throw new Error('Parsed result as null');
+      }
+
+      // foo( is parsed as free text and a paren
+      expect(result).toEqual([
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({type: Token.FREE_TEXT}),
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({
+          type: Token.R_PAREN,
+          value: ')',
+        }),
+        expect.objectContaining({type: Token.SPACES}),
+      ]);
+    });
+
+    it('parses matching parens as logic group with flattenParenGroups=false', () => {
+      const result = parseSearch('(foo)', {
+        flattenParenGroups: false,
+      });
+
+      if (result === null) {
+        throw new Error('Parsed result as null');
+      }
+
+      // (foo) is parsed as a logic group
+      expect(result).toEqual([
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({type: Token.LOGIC_GROUP}),
+        expect.objectContaining({type: Token.SPACES}),
+      ]);
+    });
+
+    it('tokenizes empty matched parens and flattenParenGroups=false', () => {
+      const result = parseSearch('()', {
+        flattenParenGroups: false,
+      });
+
+      if (result === null) {
+        throw new Error('Parsed result as null');
+      }
+
+      expect(result).toEqual([
+        expect.objectContaining({type: Token.SPACES}),
+        expect.objectContaining({
+          type: Token.LOGIC_GROUP,
+          inner: [expect.objectContaining({type: Token.SPACES})],
+        }),
+        expect.objectContaining({type: Token.SPACES}),
+      ]);
+    });
+  });
 });

+ 29 - 1
static/app/components/searchSyntax/parser.tsx

@@ -45,6 +45,8 @@ export enum Token {
   KEY_AGGREGATE = 'keyAggregate',
   KEY_AGGREGATE_ARGS = 'keyAggregateArgs',
   KEY_AGGREGATE_PARAMS = 'keyAggregateParam',
+  L_PAREN = 'lParen',
+  R_PAREN = 'rParen',
   VALUE_ISO_8601_DATE = 'valueIso8601Date',
   VALUE_RELATIVE_DATE = 'valueRelativeDate',
   VALUE_DURATION = 'valueDuration',
@@ -415,6 +417,18 @@ export class TokenConverter {
     };
   };
 
+  tokenLParen = (value: '(') => ({
+    ...this.defaultTokenFields,
+    type: Token.L_PAREN as const,
+    value,
+  });
+
+  tokenRParen = (value: ')') => ({
+    ...this.defaultTokenFields,
+    type: Token.R_PAREN as const,
+    value,
+  });
+
   tokenFreeText = (value: string, quoted: boolean) => ({
     ...this.defaultTokenFields,
     type: Token.FREE_TEXT as const,
@@ -665,6 +679,14 @@ export class TokenConverter {
   predicateTextOperator = (key: TextFilter['key']) =>
     this.config.textOperatorKeys.has(getKeyName(key));
 
+  /**
+   * When flattenParenGroups is enabled, paren groups should not be parsed,
+   * instead parsing the parens and inner group as individual tokens.
+   */
+  predicateParenGroup = (): boolean => {
+    return !this.config.flattenParenGroups;
+  };
+
   /**
    * Checks the validity of a free text based on the provided search configuration
    */
@@ -1117,7 +1139,9 @@ export type ParseResultToken =
   | TokenResult<Token.LOGIC_GROUP>
   | TokenResult<Token.FILTER>
   | TokenResult<Token.FREE_TEXT>
-  | TokenResult<Token.SPACES>;
+  | TokenResult<Token.SPACES>
+  | TokenResult<Token.L_PAREN>
+  | TokenResult<Token.R_PAREN>;
 
 /**
  * Result from parsing a search query.
@@ -1173,6 +1197,10 @@ export type SearchConfig = {
    * Text filter keys we allow to have operators
    */
   textOperatorKeys: Set<string>;
+  /**
+   * When true, the parser will not parse paren groups and will return individual paren tokens
+   */
+  flattenParenGroups?: boolean;
   /**
    * A function that returns a warning message for a given filter token key
    */

+ 8 - 0
static/app/components/searchSyntax/renderer.tsx

@@ -67,6 +67,10 @@ function renderToken(token: TokenResult<Token>, cursor: number) {
     case Token.FREE_TEXT:
       return <FreeTextToken token={token} cursor={cursor} />;
 
+    case Token.L_PAREN:
+    case Token.R_PAREN:
+      return <Paren>{token.text}</Paren>;
+
     default:
       return token.text;
   }
@@ -388,6 +392,10 @@ const ListComma = styled('span')`
   color: ${p => p.theme.gray300};
 `;
 
+const Paren = styled('span')`
+  color: ${p => p.theme.gray300};
+`;
+
 const InList = styled('span')`
   &:before {
     content: '[';