fix: improved query and VRL function validation in alerts (#4308)

fix #4305 #4304 #4315  ## 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.
2026-11-06 08:09:24 +00:00 · 2024-08-23 17:12:51 +05:30
parent a5dd7a341e
commit 8d72af70a6
3 changed files with 195 additions and 55 deletions
@@ -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,
@@ -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>
@@ -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 (