feat(search-syntax): Successfully parse and show invalid messages for… (#75452)

Previously, adding an extra `,` in the function parameters would break
the parser. I've modified the search grammar so that the aggregate
filter still matches even when there are extra commas.

While doing so, I modified the numeric and string list filters (e.g.
`[1,2,3]`) to work in the same way. Those filters didn't break
completely with an extra comma, but did show the wrong error message
until this change.

fixtures/search-syntax/invalid_aggregate_empty_parameter.json

@@ -0,0 +1,48 @@
+[
+  {
+    "query": "percentile(measurements.fp,):3.3s",
+    "result": [
+      {"type": "spaces", "value": ""},
+      {
+        "type": "filter",
+        "filter": "aggregateDuration",
+        "negated": false,
+        "key": {
+          "type": "keyAggregate",
+          "name": {"type": "keySimple", "value": "percentile", "quoted": false},
+          "args": {
+            "type": "keyAggregateArgs",
+            "args": [
+              {
+                "separator": "",
+                "value": {
+                  "type": "keyAggregateParam",
+                  "value": "measurements.fp",
+                  "quoted": false
+                }
+              },
+              {
+                "separator": ",",
+                "value": null
+              }
+            ]
+          },
+          "argsSpaceBefore": {"type": "spaces", "value": ""},
+          "argsSpaceAfter": {"type": "spaces", "value": ""}
+        },
+        "operator": "",
+        "value": {
+          "type": "valueDuration",
+          "value": "3.3",
+          "unit": "s",
+          "parsed": {"value": 3300}
+        },
+        "invalid":  {
+           "reason": "Function parameters should not have empty values",
+           "type": "empty-parameter-not-allowed"
+         }
+      },
+      {"type": "spaces", "value": ""}
+    ]
+  }
+]

static/app/components/searchSyntax/grammar.pegjs

@@ -241,7 +241,7 @@ aggregate_key
 
 function_args
   = arg1:aggregate_param
-    args:(spaces comma spaces !comma aggregate_param?)* {
+    args:(spaces comma spaces (!comma aggregate_param)?)* {
       return tc.tokenKeyAggregateArgs(arg1, args);
     }
 
@@ -300,7 +300,7 @@ boolean_value
 text_in_list
   = open_bracket
     item1:text_in_value
-    items:(spaces comma spaces !comma text_in_value?)*
+    items:(spaces comma spaces (!comma text_in_value)?)*
     closed_bracket
     &end_value {
       return tc.tokenValueTextList(item1, items);
@@ -309,7 +309,7 @@ text_in_list
 numeric_in_list
   = open_bracket
     item1:numeric_value
-    items:(spaces comma spaces !comma numeric_value?)*
+    items:(spaces comma spaces (!comma numeric_value)?)*
     closed_bracket
     &end_value {
       return tc.tokenValueNumberList(item1, items);

static/app/components/searchSyntax/parser.tsx

@@ -21,14 +21,15 @@ type ListItem<V> = [
   space: ReturnType<TokenConverter['tokenSpaces']>,
   comma: string,
   space: ReturnType<TokenConverter['tokenSpaces']>,
-  notComma: undefined,
-  value: V | null,
+  value?: [notComma: undefined, value: V | null],
 ];
 
-const listJoiner = <K,>([s1, comma, s2, _, value]: ListItem<K>) => ({
-  separator: [s1.value, comma, s2.value].join(''),
-  value,
-});
+const listJoiner = <K,>([s1, comma, s2, value]: ListItem<K>) => {
+  return {
+    separator: [s1.value, comma, s2.value].join(''),
+    value: value ? value[1] : null,
+  };
+};
 
 /**
  * A token represents a node in the syntax tree. These are all extrapolated
@@ -266,6 +267,7 @@ export enum InvalidReason {
   INVALID_FILE_SIZE = 'invalid-file-size',
   INVALID_NUMBER = 'invalid-number',
   EMPTY_VALUE_IN_LIST_NOT_ALLOWED = 'empty-value-in-list-not-allowed',
+  EMPTY_PARAMETER_NOT_ALLOWED = 'empty-parameter-not-allowed',
   INVALID_KEY = 'invalid-key',
   INVALID_DURATION = 'invalid-duration',
   INVALID_DATE_FORMAT = 'invalid-date-format',
@@ -331,6 +333,13 @@ type FilterMap = {
 
 type TextFilter = FilterMap[FilterType.TEXT];
 type InFilter = FilterMap[FilterType.TEXT_IN] | FilterMap[FilterType.NUMERIC_IN];
+type AggregateFilterType =
+  | FilterMap[FilterType.AGGREGATE_DATE]
+  | FilterMap[FilterType.AGGREGATE_DURATION]
+  | FilterMap[FilterType.AGGREGATE_NUMERIC]
+  | FilterMap[FilterType.AGGREGATE_PERCENTAGE]
+  | FilterMap[FilterType.AGGREGATE_RELATIVE_DATE]
+  | FilterMap[FilterType.AGGREGATE_SIZE];
 
 /**
  * The Filter type discriminates on the FilterType enum using the `filter` key.
@@ -501,11 +510,13 @@ export class TokenConverter {
   tokenKeyAggregateArgs = (
     arg1: ReturnType<TokenConverter['tokenKeyAggregateParam']>,
     args: ListItem<ReturnType<TokenConverter['tokenKeyAggregateParam']>>[]
-  ) => ({
-    ...this.defaultTokenFields,
-    type: Token.KEY_AGGREGATE_ARGS as const,
-    args: [{separator: '', value: arg1}, ...args.map(listJoiner)],
-  });
+  ) => {
+    return {
+      ...this.defaultTokenFields,
+      type: Token.KEY_AGGREGATE_ARGS as const,
+      args: [{separator: '', value: arg1}, ...args.map(listJoiner)],
+    };
+  };
 
   tokenValueIso8601Date = (
     value: string,
@@ -812,6 +823,10 @@ export class TokenConverter {
       return this.checkInvalidInFilter(value as InFilter['value']);
     }
 
+    if ('name' in key) {
+      return this.checkInvalidAggregateKey(key);
+    }
+
     return null;
   };
 
@@ -936,6 +951,19 @@ export class TokenConverter {
 
     return null;
   };
+
+  checkInvalidAggregateKey = (key: AggregateFilterType['key']) => {
+    const hasEmptyParameter = key.args?.args.some(arg => arg.value === null);
+
+    if (hasEmptyParameter) {
+      return {
+        type: InvalidReason.EMPTY_PARAMETER_NOT_ALLOWED,
+        reason: this.config.invalidMessages[InvalidReason.EMPTY_PARAMETER_NOT_ALLOWED],
+      };
+    }
+
+    return null;
+  };
 }
 
 function parseDate(input: string): {value: Date} {
@@ -1182,14 +1210,7 @@ export type ParseResultToken =
  */
 export type ParseResult = ParseResultToken[];
 
-export type AggregateFilter = (
-  | FilterMap[FilterType.AGGREGATE_DATE]
-  | FilterMap[FilterType.AGGREGATE_DURATION]
-  | FilterMap[FilterType.AGGREGATE_NUMERIC]
-  | FilterMap[FilterType.AGGREGATE_PERCENTAGE]
-  | FilterMap[FilterType.AGGREGATE_RELATIVE_DATE]
-  | FilterMap[FilterType.AGGREGATE_SIZE]
-) & {
+export type AggregateFilter = AggregateFilterType & {
   location: LocationRange;
   text: string;
 };
@@ -1335,6 +1356,9 @@ export const defaultConfig: SearchConfig = {
     [InvalidReason.INVALID_NUMBER]: t(
       'Invalid number. Expected number then optional k, m, or b suffix (e.g. 500k)'
     ),
+    [InvalidReason.EMPTY_PARAMETER_NOT_ALLOWED]: t(
+      'Function parameters should not have empty values'
+    ),
     [InvalidReason.EMPTY_VALUE_IN_LIST_NOT_ALLOWED]: t(
       'Lists should not have empty values'
     ),