From 8d72af70a6595047804daa484e8dae4b34cdf2b9 Mon Sep 17 00:00:00 2001
From: Omkar Kesarkhane <127078473+omkarK06@users.noreply.github.com>
Date: Fri, 23 Aug 2024 17:12:51 +0530
Subject: [PATCH] fix: improved query and VRL function validation in alerts
 (#4308)

fix #4305 #4304 #4315

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Enhanced error handling for SQL queries with specific error messages
for better user feedback.
- Introduced a section for VRL function errors with a toggle feature for
expanded details, improving clarity.
- Added a `debounceTime` property to optimize input processing in the
query editor.

- **Improvements**
- Streamlined error message display for SQL queries, moving from generic
to context-sensitive messages.
- Adjusted control flow to ensure proper validation before form
submission, preventing invalid entries.
- Improved notification messages for clarity, specifically regarding
search operations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---
 web/src/components/alerts/AddAlert.vue       | 125 ++++++++++++++-----
 web/src/components/alerts/ScheduledAlert.vue |  84 +++++++++++--
 web/src/composables/useLogs.ts               |  41 +++---
 3 files changed, 195 insertions(+), 55 deletions(-)

diff --git a/web/src/components/alerts/AddAlert.vue b/web/src/components/alerts/AddAlert.vue
index 49e60834dd..d28f92187a 100644
--- a/web/src/components/alerts/AddAlert.vue
+++ b/web/src/components/alerts/AddAlert.vue
@@ -188,7 +188,8 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
                 :columns="filteredColumns"
                 :conditions="formData.query_condition.conditions"
                 :alertData="formData"
-                :isValidSqlQuery="isValidSqlQuery"
+                :sqlQueryErrorMsg="sqlQueryErrorMsg"
+                :vrlFunctionError="vrlFunctionError"
                 v-model:trigger="formData.trigger_condition"
                 v-model:sql="formData.query_condition.sql"
                 v-model:promql="formData.query_condition.promql"
@@ -204,6 +205,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
                 @field:remove="removeField"
                 @input:update="onInputUpdate"
                 @validate-sql="validateSqlQuery"
+                @update:showVrlFunction="updateFunctionVisibility"
                 class="q-mt-sm"
               />
             </div>
@@ -493,7 +495,7 @@ const defaultValue: any = () => {
         },
       },
       promql_condition: null,
-      vrl_function: "",
+      vrl_function: null,
     },
     trigger_condition: {
       period: 10,
@@ -586,7 +588,7 @@ export default defineComponent({
 
     const previewQuery = ref("");
 
-    const isValidSqlQuery = ref(true);
+    const sqlQueryErrorMsg = ref("");
 
     const validateSqlQueryPromise = ref<Promise<unknown>>();
 
@@ -601,6 +603,8 @@ export default defineComponent({
 
     let parser: any = null;
 
+    const vrlFunctionError = ref("");
+
     onBeforeMount(async () => {
       await importSqlParser();
       await getAllFunctions();
@@ -944,16 +948,24 @@ export default defineComponent({
     };
     const getParser = (sqlQuery: string) => {
       try {
-        const columns = parser.astify(sqlQuery).columns;
+        // As default is a reserved keyword in sql-parser, we are replacing it with default1
+        const regex = /\bdefault\b/g;
+        const columns = parser.astify(
+          sqlQuery.replace(regex, "default1"),
+        ).columns;
         for (const column of columns) {
           if (column.expr.column === "*") {
+            sqlQueryErrorMsg.value = "Selecting all columns is not allowed";
             return false;
           }
         }
         return true;
       } catch (error) {
+        // In catch block we are returning true, as we just wanted to validate if user have added * in the query to select all columns
+        // select field from default // here default is not wrapped in "" so node sql parser will throw error as default is a reserved keyword. But our Backend supports this query without quotes
+        // Query will be validated in the backend
         console.log(error);
-        return false;
+        return true;
       }
     };
 
@@ -1009,9 +1021,11 @@ export default defineComponent({
         payload.query_condition.sql = "";
       }
 
-      payload.query_condition.vrl_function = b64EncodeUnicode(
-        formData.value.query_condition.vrl_function,
-      );
+      if (formData.value.query_condition.vrl_function) {
+        payload.query_condition.vrl_function = b64EncodeUnicode(
+          formData.value.query_condition.vrl_function,
+        );
+      }
 
       if (beingUpdated) {
         payload.updatedAt = new Date().toISOString();
@@ -1090,7 +1104,14 @@ export default defineComponent({
       return true;
     };
 
-    const validateSqlQuery = () => {
+    const validateSqlQuery = async () => {
+      // Delaying the validation by 300ms, as editor has debounce of 300ms. Else old value will be used for validation
+      await new Promise((resolve) => setTimeout(resolve, 300));
+
+      if (!getParser(formData.value.query_condition.sql)) {
+        return;
+      }
+
       const query = buildQueryPayload({
         sqlMode: true,
         streamName: formData.value.stream_name,
@@ -1100,6 +1121,11 @@ export default defineComponent({
 
       query.query.sql = formData.value.query_condition.sql;
 
+      if (formData.value.query_condition.vrl_function)
+        query.query.query_fn = b64EncodeUnicode(
+          formData.value.query_condition.vrl_function,
+        );
+
       validateSqlQueryPromise.value = new Promise((resolve, reject) => {
         searchService
           .search({
@@ -1108,25 +1134,47 @@ export default defineComponent({
             page_type: "logs",
           })
           .then((res: any) => {
-            isValidSqlQuery.value = true;
+            sqlQueryErrorMsg.value = "";
+
+            if (res.data?.function_error) {
+              vrlFunctionError.value = res.data.function_error;
+              q.notify({
+                type: "negative",
+                message: "Invalid VRL Function",
+                timeout: 3000,
+              });
+              reject("function_error");
+            } else vrlFunctionError.value = "";
+
             resolve("");
           })
           .catch((err: any) => {
-            if (err.response.data.code === 500) {
-              isValidSqlQuery.value = false;
+            sqlQueryErrorMsg.value = err.response?.data?.message
+              ? err.response?.data?.message
+              : "Invalid SQL Query";
+
+            // Show error only if it is not real time alert
+            // This case happens when user enters invalid query and then switches to real time alert
+            if (formData.value.query_condition.type === "sql")
               q.notify({
                 type: "negative",
-                message: "Invalid SQL Query : " + err.response.data.message,
+                message: "Invalid SQL Query : " + err.response?.data?.message,
                 timeout: 3000,
               });
-              reject("");
-            } else isValidSqlQuery.value = true;
 
-            resolve("");
+            reject("sql_error");
           });
       });
     };
 
+    const updateFunctionVisibility = () => {
+      // if validateSqlQueryPromise has error "function_error" then reset the promise when function is disabled
+      if (!showVrlFunction.value && vrlFunctionError.value) {
+        validateSqlQueryPromise.value = Promise.resolve("");
+        vrlFunctionError.value = "";
+      }
+    };
+
     return {
       t,
       q,
@@ -1182,9 +1230,11 @@ export default defineComponent({
       getTimezoneOffset,
       showVrlFunction,
       validateSqlQuery,
-      isValidSqlQuery,
       validateSqlQueryPromise,
       isValidResourceName,
+      sqlQueryErrorMsg,
+      vrlFunctionError,
+      updateFunctionVisibility,
     };
   },
 
@@ -1206,12 +1256,12 @@ export default defineComponent({
       this.disableColor = "grey-5";
       this.formData = cloneDeep(this.modelValue);
       this.isAggregationEnabled = !!this.formData.query_condition.aggregation;
-      this.formData.query_condition.vrl_function = b64DecodeUnicode(
-        this.formData.query_condition.vrl_function,
-      );
 
-      if (this.formData.query_condition.vrl_function?.trim()?.length) {
+      if (this.formData.query_condition.vrl_function) {
         this.showVrlFunction = true;
+        this.formData.query_condition.vrl_function = b64DecodeUnicode(
+          this.formData.query_condition.vrl_function,
+        );
       }
     }
 
@@ -1250,8 +1300,13 @@ export default defineComponent({
     },
 
     async onSubmit() {
+      // Delaying submission by 500ms to allow the form to validate, as query is validated in validateSqlQuery method
+      // When user updated query and click on save
+      await new Promise((resolve) => setTimeout(resolve, 500));
+
       if (
-        this.formData.is_real_time == "false" && this.formData.query_condition.type == 'sql' && 
+        this.formData.is_real_time == "false" &&
+        this.formData.query_condition.type == "sql" &&
         !this.getParser(this.formData.query_condition.sql)
       ) {
         this.q.notify({
@@ -1261,6 +1316,7 @@ export default defineComponent({
         });
         return false;
       }
+
       if (this.formData.stream_name == "") {
         this.q.notify({
           type: "negative",
@@ -1277,13 +1333,7 @@ export default defineComponent({
         this.formData.tz_offset = this.getTimezoneOffset();
       }
 
-      try {
-        await this.validateSqlQueryPromise;
-      } catch (e) {
-        return false;
-      }
-
-      this.addAlertForm.validate().then((valid: any) => {
+      this.addAlertForm.validate().then(async (valid: any) => {
         if (!valid) {
           return false;
         }
@@ -1297,6 +1347,23 @@ export default defineComponent({
           timeout: 2000,
         });
 
+        if (
+          this.formData.is_real_time == "false" &&
+          this.formData.query_condition.type == "sql"
+        ) {
+          try {
+            // Wait for the promise to resolve
+            // Storing the SQL query validation promise in a variable
+            // Case: When user edits the query and directly saves it without waiting for the validation to complete
+            // So waiting here for sql validation to complete
+            await this.validateSqlQueryPromise;
+          } catch (error) {
+            dismiss();
+            console.log("Error while validating sql query");
+            return false;
+          }
+        }
+
         if (this.beingUpdated) {
           callAlert = alertsService.update(
             this.store.state.selectedOrganization.identifier,
diff --git a/web/src/components/alerts/ScheduledAlert.vue b/web/src/components/alerts/ScheduledAlert.vue
index a02bb87acc..9c87020a5c 100644
--- a/web/src/components/alerts/ScheduledAlert.vue
+++ b/web/src/components/alerts/ScheduledAlert.vue
@@ -68,6 +68,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
           title="Toggle Function Editor"
           class="q-pl-xs"
           size="30px"
+          @update:model-value="updateFunctionVisibility"
           :disable="tab === 'promql'"
         />
       </div>
@@ -80,6 +81,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
               ref="queryEditorRef"
               editor-id="alerts-query-editor"
               class="monaco-editor"
+              :debounceTime="300"
               v-model:query="query"
               :class="
                 query == '' && queryEditorPlaceholderFlag ? 'empty-query' : ''
@@ -88,8 +90,10 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
               @focus="queryEditorPlaceholderFlag = false"
               @blur="onBlurQueryEditor"
             />
-            <div class="text-negative q-mb-xs" style="height: 21px">
-              <span v-show="!isValidSqlQuery"> Invalid SQL Query</span>
+            <div class="text-negative q-mb-xs invalid-sql-error">
+              <span v-show="!!sqlQueryErrorMsg">
+                Error: {{ sqlQueryErrorMsg }}</span
+              >
             </div>
           </div>
         </template>
@@ -99,6 +103,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
             ref="queryEditorRef"
             editor-id="alerts-query-editor"
             class="monaco-editor q-mb-md"
+            :debounceTime="300"
             v-model:query="promqlQuery"
             @update:query="updateQueryValue"
           />
@@ -143,6 +148,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
             ref="fnEditorRef"
             editor-id="fnEditor"
             class="monaco-editor"
+            :debounceTime="300"
             v-model:query="vrlFunctionContent"
             :class="
               vrlFunctionContent == '' && functionEditorPlaceholderFlag
@@ -151,8 +157,43 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
             "
             language="ruby"
             @focus="functionEditorPlaceholderFlag = false"
-            @blur="functionEditorPlaceholderFlag = true"
+            @blur="onBlurFunctionEditor"
           />
+
+          <div
+            class="text-subtitle2 q-pb-sm"
+            style="min-height: 21px; width: 500px"
+          >
+            <div v-if="vrlFunctionError">
+              <div class="text-negative q-mb-xs flex items-center">
+                <q-btn
+                  :icon="
+                    isFunctionErrorExpanded ? 'expand_more' : 'chevron_right'
+                  "
+                  dense
+                  size="xs"
+                  flat
+                  class="q-mr-xs"
+                  data-test="table-row-expand-menu"
+                  @click.stop="toggleExpandFunctionError"
+                />
+                <div>
+                  <span v-show="vrlFunctionError">Invalid VRL function</span>
+                </div>
+              </div>
+              <div
+                v-if="isFunctionErrorExpanded"
+                class="q-px-sm q-pb-sm"
+                :class="
+                  store.state.theme === 'dark' ? 'bg-grey-10' : 'bg-grey-2'
+                "
+              >
+                <pre class="q-my-none" style="white-space: pre-wrap">{{
+                  vrlFunctionError
+                }}</pre>
+              </div>
+            </div>
+          </div>
         </div>
       </div>
     </template>
@@ -729,7 +770,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 </template>
 
 <script setup lang="ts">
-import { ref, watch, computed, type Ref, defineAsyncComponent } from "vue";
+import { ref, watch, computed, defineAsyncComponent } from "vue";
 import FieldsInput from "@/components/alerts/FieldsInput.vue";
 import { useI18n } from "vue-i18n";
 import {
@@ -738,8 +779,6 @@ import {
 } from "@quasar/extras/material-icons-outlined";
 import { useStore } from "vuex";
 import { getImageURL } from "@/utils/zincutils";
-import useQuery from "@/composables/useQuery";
-import searchService from "@/services/search";
 import { useQuasar } from "quasar";
 
 const QueryEditor = defineAsyncComponent(
@@ -759,10 +798,11 @@ const props = defineProps([
   "promql_condition",
   "vrl_function",
   "showVrlFunction",
-  "isValidSqlQuery",
+  "sqlQueryErrorMsg",
   "disableThreshold",
   "disableVrlFunction",
   "disableQueryTypeSelection",
+  "vrlFunctionError",
 ]);
 
 const emits = defineEmits([
@@ -799,6 +839,8 @@ const functionEditorPlaceholderFlag = ref(true);
 
 const queryEditorPlaceholderFlag = ref(true);
 
+const isFunctionErrorExpanded = ref(false);
+
 const metricFunctions = ["p50", "p75", "p90", "p95", "p99"];
 const regularFunctions = ["avg", "max", "min", "sum", "count"];
 
@@ -818,6 +860,8 @@ const aggregationData = ref(props.aggregation);
 
 const filteredFields = ref(props.columns);
 
+const fnEditorRef = ref<any>(null);
+
 const getNumericColumns = computed(() => {
   if (
     _isAggregationEnabled.value &&
@@ -892,7 +936,7 @@ watch(
 
 const vrlFunctionContent = computed({
   get() {
-    return props.vrl_function;
+    return props.vrl_function || "";
   },
   set(value) {
     emits("update:vrl_function", value);
@@ -905,9 +949,17 @@ const isVrlFunctionEnabled = computed({
   },
   set(value) {
     emits("update:showVrlFunction", value);
+    updateFunctionVisibility(value);
   },
 });
 
+const updateFunctionVisibility = (isEnabled: boolean) => {
+  if (!isEnabled) {
+    vrlFunctionContent.value = null;
+    selectedFunction.value = "";
+  }
+};
+
 const updateQuery = () => {
   if (tab.value === "promql") {
     const condition = !props.promql_condition
@@ -998,7 +1050,7 @@ const filterFunctionOptions = (val: string, update: any) => {
   });
 };
 
-const onBlurQueryEditor = () => {
+const onBlurQueryEditor = async () => {
   queryEditorPlaceholderFlag.value = true;
   emits("validate-sql");
 };
@@ -1054,6 +1106,16 @@ const validateInputs = (notify: boolean = true) => {
 
   return true;
 };
+
+const onBlurFunctionEditor = async () => {
+  functionEditorPlaceholderFlag.value = true;
+  emits("validate-sql");
+};
+
+const toggleExpandFunctionError = () => {
+  isFunctionErrorExpanded.value = !isFunctionErrorExpanded.value;
+};
+
 defineExpose({
   tab,
   validateInputs,
@@ -1109,4 +1171,8 @@ defineExpose({
     background-size: 170px;
   }
 }
+
+.invalid-sql-error {
+  min-height: 21px;
+}
 </style>
diff --git a/web/src/composables/useLogs.ts b/web/src/composables/useLogs.ts
index aca3a8baf8..c24fc2ca76 100644
--- a/web/src/composables/useLogs.ts
+++ b/web/src/composables/useLogs.ts
@@ -1727,7 +1727,7 @@ const useLogs = () => {
                 searchObj.data.histogramQuery.query.start_time = partition[0];
                 searchObj.data.histogramQuery.query.end_time = partition[1];
                 await getHistogramQueryData(searchObj.data.histogramQuery);
-                if(partitions.length > 1) {
+                if (partitions.length > 1) {
                   setTimeout(async () => {
                     await generateHistogramData();
                     refreshPartitionPagination(true);
@@ -1738,7 +1738,7 @@ const useLogs = () => {
             }
           }
           await generateHistogramData();
-          refreshPartitionPagination(true);          
+          refreshPartitionPagination(true);
         } else if (searchObj.meta.sqlMode && !isNonAggregatedQuery(parsedSQL)) {
           searchObj.data.histogram = {
             xData: [],
@@ -2372,7 +2372,9 @@ const useLogs = () => {
         searchObj.loadingHistogram = false;
         searchObj.data.isOperationCancelled = false;
 
-        notificationMsg.value = "Search operation was cancelled";
+        notificationMsg.value = "Search query was cancelled";
+        searchObj.data.histogram.errorMsg = "Search query was cancelled";
+        searchObj.data.histogram.errorDetail = "Search query was cancelled";
         return;
       }
 
@@ -2396,8 +2398,8 @@ const useLogs = () => {
             removeTraceId(traceId);
             searchObjDebug["histogramProcessingStartTime"] = performance.now();
             searchObj.loading = false;
-            if(searchObj.data.queryResults.aggs == null)  {
-                searchObj.data.queryResults.aggs = [];                
+            if (searchObj.data.queryResults.aggs == null) {
+              searchObj.data.queryResults.aggs = [];
             }
             searchObj.data.queryResults.aggs.push(...res.data.hits);
             searchObj.data.queryResults.scan_size += res.data.scan_size;
@@ -2440,7 +2442,7 @@ const useLogs = () => {
           .catch((err) => {
             searchObj.loadingHistogram = false;
             let trace_id = "";
-            
+
             if (err?.request?.status != 429) {
               searchObj.data.histogram.errorMsg =
                 typeof err == "string" && err
@@ -2479,6 +2481,13 @@ const useLogs = () => {
               }
             }
 
+            if (err?.request?.status >= 429) {
+              notificationMsg.value = err?.response?.data?.message;
+              searchObj.data.histogram.errorMsg = err?.response?.data?.message;
+              searchObj.data.histogram.errorDetail =
+                err?.response?.data?.error_detail;
+            }
+
             reject(false);
           })
           .finally(() => {
@@ -2858,19 +2867,17 @@ const useLogs = () => {
               commonSchemaFields.unshift("dummylabel");
               // searchObj.data.stream.expandGroupRowsFieldCount["common"] = searchObj.data.stream.expandGroupRowsFieldCount["common"] + 1;
             }
-            //here we check whether timestamp field is present or not 
+            //here we check whether timestamp field is present or not
             //as we append timestamp dynamically for userDefined schema we need to check this
-              if(userDefineSchemaSettings.includes(
+            if (
+              userDefineSchemaSettings.includes(
                 store.state.zoConfig?.timestamp_column,
-              )){
-                searchObj.data.hasSearchDataTimestampField = true;
-
-              }
-              else{
-                searchObj.data.hasSearchDataTimestampField = false;
-
-              }
-
+              )
+            ) {
+              searchObj.data.hasSearchDataTimestampField = true;
+            } else {
+              searchObj.data.hasSearchDataTimestampField = false;
+            }
 
             // check for user defined schema is false then only consider checking new fields from result set
             if (