From 51fcde75278fa6fdf0e7843fee1c41a40ce87f12 Mon Sep 17 00:00:00 2001
From: Oliver Gupte <oliver.gupte@elastic.co>
Date: Fri, 28 Oct 2022 23:15:53 -0400
Subject: [PATCH] improvements to kuery validation

---
 .../plugins/apm/common/service_groups.test.ts | 14 +++----
 x-pack/plugins/apm/common/service_groups.ts   | 42 ++++++++++---------
 .../service_group_save/select_services.tsx    |  9 +---
 .../apm/server/routes/service_groups/route.ts |  6 ++-
 4 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/x-pack/plugins/apm/common/service_groups.test.ts b/x-pack/plugins/apm/common/service_groups.test.ts
index 65f0c48dd73b3d..856eec4ef2e3f5 100644
--- a/x-pack/plugins/apm/common/service_groups.test.ts
+++ b/x-pack/plugins/apm/common/service_groups.test.ts
@@ -39,16 +39,16 @@ describe('service_groups common utils', () => {
       const result = validateServiceGroupKuery(
         `service.name: testbeans* or agent.name: "nodejs"`
       );
-      expect(result).toHaveProperty('isValid', true);
-      expect(result).toHaveProperty('isParsingError', false);
-      expect(result).toHaveProperty('message', '');
+      expect(result).toHaveProperty('isValidFields', true);
+      expect(result).toHaveProperty('isValidSyntax', true);
+      expect(result).not.toHaveProperty('message');
     });
     it('should return validation error when unsupported fields are used', () => {
       const result = validateServiceGroupKuery(
         `service.name: testbeans* or agent.name: "nodejs" or transaction.type: request`
       );
-      expect(result).toHaveProperty('isValid', false);
-      expect(result).toHaveProperty('isParsingError', false);
+      expect(result).toHaveProperty('isValidFields', false);
+      expect(result).toHaveProperty('isValidSyntax', true);
       expect(result).toHaveProperty(
         'message',
         'Query filter for service group does not support fields [transaction.type]'
@@ -58,8 +58,8 @@ describe('service_groups common utils', () => {
       const result = validateServiceGroupKuery(
         `service.name: testbeans* or agent.name: "nod`
       );
-      expect(result).toHaveProperty('isValid', false);
-      expect(result).toHaveProperty('isParsingError', true);
+      expect(result).toHaveProperty('isValidFields', false);
+      expect(result).toHaveProperty('isValidSyntax', false);
       expect(result).toHaveProperty('message');
       expect(result).not.toBe('');
     });
diff --git a/x-pack/plugins/apm/common/service_groups.ts b/x-pack/plugins/apm/common/service_groups.ts
index 6a48f7167273d2..4b2ba1288ecaef 100644
--- a/x-pack/plugins/apm/common/service_groups.ts
+++ b/x-pack/plugins/apm/common/service_groups.ts
@@ -48,31 +48,35 @@ export function isSupportedField(fieldName: string) {
   );
 }
 
-export function validateServiceGroupKuery(kuery: string) {
+export function validateServiceGroupKuery(kuery: string): {
+  isValidFields: boolean;
+  isValidSyntax: boolean;
+  message?: string;
+} {
   try {
     const kueryFields = getKueryFields([fromKueryExpression(kuery)]);
     const unsupportedKueryFields = kueryFields.filter(
       (fieldName) => !isSupportedField(fieldName)
     );
-    if (unsupportedKueryFields.length > 0) {
-      return {
-        isValid: false,
-        isParsingError: false,
-        message: i18n.translate(
-          'xpack.apm.serviceGroups.invalidFields.message',
-          {
-            defaultMessage:
-              'Query filter for service group does not support fields [{unsupportedFieldNames}]',
-            values: {
-              unsupportedFieldNames: unsupportedKueryFields.join(', '),
-            },
-          }
-        ),
-      };
-    } else {
-      return { isValid: true, isParsingError: false, message: '' };
+    if (unsupportedKueryFields.length === 0) {
+      return { isValidFields: true, isValidSyntax: true };
     }
+    return {
+      isValidFields: false,
+      isValidSyntax: true,
+      message: i18n.translate('xpack.apm.serviceGroups.invalidFields.message', {
+        defaultMessage:
+          'Query filter for service group does not support fields [{unsupportedFieldNames}]',
+        values: {
+          unsupportedFieldNames: unsupportedKueryFields.join(', '),
+        },
+      }),
+    };
   } catch (error) {
-    return { isValid: false, isParsingError: true, message: error.message };
+    return {
+      isValidFields: false,
+      isValidSyntax: false,
+      message: error.message,
+    };
   }
 }
diff --git a/x-pack/plugins/apm/public/components/app/service_groups/service_group_save/select_services.tsx b/x-pack/plugins/apm/public/components/app/service_groups/service_group_save/select_services.tsx
index 14c881e162974d..96b0b47a38a1b0 100644
--- a/x-pack/plugins/apm/public/components/app/service_groups/service_group_save/select_services.tsx
+++ b/x-pack/plugins/apm/public/components/app/service_groups/service_group_save/select_services.tsx
@@ -82,13 +82,8 @@ export function SelectServices({
     if (!stagedKuery) {
       return;
     }
-    const { isValid, isParsingError, message } =
-      validateServiceGroupKuery(stagedKuery);
-    if (isValid || isParsingError) {
-      setKueryValidationMessage(undefined);
-    } else {
-      setKueryValidationMessage(message);
-    }
+    const { message } = validateServiceGroupKuery(stagedKuery);
+    setKueryValidationMessage(message);
   }, [stagedKuery]);
 
   const { start, end } = useMemo(
diff --git a/x-pack/plugins/apm/server/routes/service_groups/route.ts b/x-pack/plugins/apm/server/routes/service_groups/route.ts
index 6207aa2a6aa6bb..dde307efa7c4b7 100644
--- a/x-pack/plugins/apm/server/routes/service_groups/route.ts
+++ b/x-pack/plugins/apm/server/routes/service_groups/route.ts
@@ -124,8 +124,10 @@ const serviceGroupSaveRoute = createApmServerRoute({
     const {
       savedObjects: { client: savedObjectsClient },
     } = await context.core;
-    const { isValid, message } = validateServiceGroupKuery(params.body.kuery);
-    if (!isValid) {
+    const { isValidFields, isValidSyntax, message } = validateServiceGroupKuery(
+      params.body.kuery
+    );
+    if (!(isValidFields && isValidSyntax)) {
       throw Boom.badRequest(message);
     }