Merge branch '13135-call-to-validate_query-in-custom-metrics-form-is-not-retried' into 'master'

Call to validate_query in custom metrics form is not retried

Closes #13135

See merge request gitlab-org/gitlab!18769

changelogs/unreleased/13135-call-to-validate_query-in-custom-metrics-form-is-not-retried.yml

+---
+title: Fix query validation in custom metrics form
+merge_request: 18769
+author:
+type: fixed

ee/app/assets/javascripts/custom_metrics/components/custom_metrics_form_fields.vue

 <script>
-import { GlFormInput, GlButton, GlLink, GlFormGroup, GlFormRadioGroup } from '@gitlab/ui';
+import {
+  GlFormInput,
+  GlButton,
+  GlLink,
+  GlFormGroup,
+  GlFormRadioGroup,
+  GlLoadingIcon,
+} from '@gitlab/ui';
 import { debounce } from 'underscore';
-import axios from '~/lib/utils/axios_utils';
 import { __, s__ } from '~/locale';
 import Icon from '~/vue_shared/components/icon.vue';
 import csrf from '~/lib/utils/csrf';
+import axios from '~/lib/utils/axios_utils';
+import statusCodes from '~/lib/utils/http_status';
+import { backOff } from '~/lib/utils/common_utils';
 import { queryTypes, formDataValidator } from '../constants';
 
+const MAX_REQUESTS = 4;
+const axiosCancelToken = axios.CancelToken;
+let cancelTokenSource;
+
+function backOffRequest(makeRequestCallback) {
+  let requestsCount = 0;
+  return backOff((next, stop) => {
+    makeRequestCallback()
+      .then(resp => {
+        if (resp.status === statusCodes.OK) {
+          stop(resp);
+        } else {
+          requestsCount += 1;
+          if (requestsCount < MAX_REQUESTS) {
+            next();
+          } else {
+            stop(resp);
+          }
+        }
+      })
+      // If the request is cancelled by axios
+      // then consider it as noop so that its not
+      // caught by subsequent catches
+      .catch(thrown => (axios.isCancel(thrown) ? undefined : stop(thrown)));
+  });
+}
+
 export default {
   components: {
     GlFormInput,
@@ -14,6 +50,7 @@ export default {
     GlLink,
     GlFormGroup,
     GlFormRadioGroup,
+    GlLoadingIcon,
     Icon,
   },
   props: {
@@ -49,6 +86,7 @@ export default {
 
     return {
       queryIsValid: null,
+      queryValidateInFlight: false,
       ...this.formData,
       group,
     };
@@ -81,27 +119,54 @@ export default {
     }
   },
   methods: {
-    requestValidation() {
-      return axios.post(this.validateQueryPath, {
-        query: this.query,
-      });
+    requestValidation(query, cancelToken) {
+      return backOffRequest(() =>
+        axios.post(
+          this.validateQueryPath,
+          {
+            query,
+          },
+          {
+            cancelToken,
+          },
+        ),
+      );
+    },
+    setFormState(isValid, inFlight, message) {
+      this.queryIsValid = isValid;
+      this.queryValidateInFlight = inFlight;
+      this.errorMessage = message;
     },
     validateQuery() {
-      this.requestValidation()
+      if (!this.query) {
+        this.setFormState(null, false, '');
+        return;
+      }
+      this.setFormState(null, true, '');
+      // cancel previously dispatched backoff request
+      if (cancelTokenSource) {
+        cancelTokenSource.cancel();
+      }
+      // Creating a new token for each request because
+      // if a single token is used it can cancel existing requests
+      // as well.
+      cancelTokenSource = axiosCancelToken.source();
+      this.requestValidation(this.query, cancelTokenSource.token)
         .then(res => {
           const response = res.data;
           const { valid, error } = response.query;
-
           if (response.success) {
-            this.errorMessage = valid ? '' : error;
-            this.queryIsValid = valid;
+            this.setFormState(valid, false, valid ? '' : error);
           } else {
             throw new Error(__('There was an error trying to validate your query'));
           }
         })
         .catch(() => {
-          this.errorMessage = s__('Metrics|There was an error trying to validate your query');
-          this.queryIsValid = false;
+          this.setFormState(
+            false,
+            false,
+            s__('Metrics|There was an error trying to validate your query'),
+          );
         });
     },
     debouncedValidateQuery: debounce(function checkQuery() {
@@ -150,7 +215,7 @@ export default {
     >
       <gl-form-input
         id="prometheus_metric_query"
-        v-model="query"
+        v-model.trim="query"
         name="prometheus_metric[query]"
         class="form-control"
         :placeholder="s__('Metrics|e.g. rate(http_requests_total[5m])')"
@@ -158,12 +223,16 @@ export default {
         :state="queryIsValid"
         @input="debouncedValidateQuery"
       />
-      <slot name="valid-feedback">
+      <span v-if="queryValidateInFlight" class="form-text text-muted">
+        <gl-loading-icon :inline="true" class="mr-1 align-middle" />
+        {{ s__('Metrics|Validating query') }}
+      </span>
+      <slot v-if="!queryValidateInFlight" name="valid-feedback">
         <span class="form-text cgreen">
           {{ validQueryMsg }}
         </span>
       </slot>
-      <slot name="invalid-feedback">
+      <slot v-if="!queryValidateInFlight" name="invalid-feedback">
         <span class="form-text cred">
           {{ invalidQueryMsg }}
         </span>

ee/spec/frontend/custom_metrics/components/custom_metrics_form_fields_spec.js

 import { mount } from '@vue/test-utils';
 import axios from '~/lib/utils/axios_utils';
+import MockAdapter from 'axios-mock-adapter';
 import { TEST_HOST } from 'helpers/test_constants';
 import CustomMetricsFormFields from 'ee/custom_metrics/components/custom_metrics_form_fields.vue';
 
+const { CancelToken } = axios;
+
 describe('custom metrics form fields component', () => {
   let component;
+  let mockAxios;
+
   const getNamedInput = name => component.element.querySelector(`input[name="${name}"]`);
   const validateQueryPath = `${TEST_HOST}/mock/path`;
   const validQueryResponse = { data: { success: true, query: { valid: true, error: '' } } };
@@ -22,7 +27,7 @@ describe('custom metrics form fields component', () => {
       ...data,
     },
   });
-  const mountComponent = props => {
+  const mountComponent = (props, methods = {}) => {
     component = mount(CustomMetricsFormFields, {
       propsData: {
         formOperation,
@@ -31,19 +36,18 @@ describe('custom metrics form fields component', () => {
       },
       csrfToken,
       sync: false,
-      methods: {
-        debouncedValidateQuery: debouncedValidateQueryMock,
-      },
+      methods,
     });
   };
 
   beforeEach(() => {
-    jest.spyOn(axios, 'post').mockResolvedValue(validQueryResponse);
+    mockAxios = new MockAdapter(axios);
+    mockAxios.onPost(validateQueryPath).reply(validQueryResponse);
   });
 
   afterEach(() => {
-    axios.post.mockRestore();
     component.destroy();
+    mockAxios.restore();
   });
 
   it('checks form validity', done => {
@@ -58,7 +62,7 @@ describe('custom metrics form fields component', () => {
     });
 
     component.vm.$nextTick(() => {
-      expect(component.vm.formIsValid).toBe(true);
+      expect(component.vm.formIsValid).toBe(false);
       done();
     });
   });
@@ -105,33 +109,112 @@ describe('custom metrics form fields component', () => {
   });
 
   describe('query input', () => {
-    const name = 'prometheus_metric[query]';
+    const queryInputName = 'prometheus_metric[query]';
+    beforeEach(() => {
+      mockAxios.onPost(validateQueryPath).reply(validQueryResponse);
+    });
 
     it('is empty by default', () => {
       mountComponent();
 
-      expect(getNamedInput(name).value).toBe('');
+      expect(getNamedInput(queryInputName).value).toBe('');
     });
 
     it('receives and validates a persisted value', () => {
       const query = 'persistedQuery';
+      const axiosPost = jest.spyOn(axios, 'post');
+      const source = CancelToken.source();
       mountComponent({ metricPersisted: true, ...makeFormData({ query }) });
 
-      expect(axios.post).toHaveBeenCalledWith(validateQueryPath, { query });
-      expect(getNamedInput(name).value).toBe(query);
+      expect(axiosPost).toHaveBeenCalledWith(
+        validateQueryPath,
+        { query },
+        { cancelToken: source.token },
+      );
+      expect(getNamedInput(queryInputName).value).toBe(query);
       jest.runAllTimers();
     });
 
     it('checks validity on user input', () => {
       const query = 'changedQuery';
-      mountComponent();
-      const queryInput = component.find(`input[name="${name}"]`);
+      mountComponent(
+        {},
+        {
+          debouncedValidateQuery: debouncedValidateQueryMock,
+        },
+      );
+      const queryInput = component.find(`input[name="${queryInputName}"]`);
       queryInput.setValue(query);
       queryInput.trigger('input');
 
       expect(debouncedValidateQueryMock).toHaveBeenCalledWith(query);
     });
 
+    describe('when query validation is in flight', () => {
+      beforeEach(() => {
+        jest.useFakeTimers();
+        mountComponent(
+          { metricPersisted: true, ...makeFormData({ query: 'validQuery' }) },
+          {
+            requestValidation: jest.fn().mockImplementation(
+              () =>
+                new Promise(resolve =>
+                  setTimeout(() => {
+                    resolve(validQueryResponse);
+                  }, 4000),
+                ),
+            ),
+          },
+        );
+      });
+
+      afterEach(() => {
+        jest.clearAllTimers();
+      });
+
+      it('expect queryValidateInFlight is in flight', done => {
+        const queryInput = component.find(`input[name="${queryInputName}"]`);
+        queryInput.setValue('query');
+        queryInput.trigger('input');
+
+        component.vm.$nextTick(() => {
+          expect(component.vm.queryValidateInFlight).toBe(true);
+          jest.runOnlyPendingTimers();
+          component.vm.$nextTick(() => {
+            expect(component.vm.queryValidateInFlight).toBe(false);
+            expect(component.vm.queryIsValid).toBe(true);
+            done();
+          });
+        });
+      });
+
+      it('expect loading message to display', done => {
+        const queryInput = component.find(`input[name="${queryInputName}"]`);
+        queryInput.setValue('query');
+        queryInput.trigger('input');
+        component.vm.$nextTick(() => {
+          expect(component.text()).toContain('Validating query');
+          jest.runOnlyPendingTimers();
+          done();
+        });
+      });
+
+      it('expect loading message to disappear', done => {
+        const queryInput = component.find(`input[name="${queryInputName}"]`);
+        queryInput.setValue('query');
+        queryInput.trigger('input');
+        component.vm.$nextTick(() => {
+          jest.runOnlyPendingTimers();
+          component.vm.$nextTick(() => {
+            expect(component.vm.queryValidateInFlight).toBe(false);
+            expect(component.vm.queryIsValid).toBe(true);
+            expect(component.vm.errorMessage).toBe('');
+            done();
+          });
+        });
+      });
+    });
+
     describe('when query is invalid', () => {
       const errorMessage = 'mockErrorMessage';
       const invalidQueryResponse = {
@@ -139,25 +222,42 @@ describe('custom metrics form fields component', () => {
       };
 
       beforeEach(() => {
-        axios.post.mockResolvedValue(invalidQueryResponse);
-        mountComponent({ metricPersisted: true, ...makeFormData({ query: 'invalidQuery' }) });
+        mountComponent(
+          { metricPersisted: true, ...makeFormData({ query: 'invalidQuery' }) },
+          {
+            requestValidation: jest
+              .fn()
+              .mockImplementation(() => Promise.resolve(invalidQueryResponse)),
+          },
+        );
       });
 
       it('sets queryIsValid to false', done => {
         component.vm.$nextTick(() => {
+          expect(component.vm.queryValidateInFlight).toBe(false);
           expect(component.vm.queryIsValid).toBe(false);
           done();
         });
       });
 
-      it('shows invalid query message', () => {
-        expect(component.text()).toContain(errorMessage);
+      it('shows invalid query message', done => {
+        component.vm.$nextTick(() => {
+          expect(component.text()).toContain(errorMessage);
+          done();
+        });
       });
     });
 
     describe('when query is valid', () => {
       beforeEach(() => {
-        mountComponent({ metricPersisted: true, ...makeFormData({ query: 'validQuery' }) });
+        mountComponent(
+          { metricPersisted: true, ...makeFormData({ query: 'validQuery' }) },
+          {
+            requestValidation: jest
+              .fn()
+              .mockImplementation(() => Promise.resolve(validQueryResponse)),
+          },
+        );
       });
 
       it('sets queryIsValid to true when query is valid', done => {

locale/gitlab.pot

@@ -10858,6 +10858,9 @@ msgstr ""
 msgid "Metrics|Used if the query returns a single series. If it returns multiple series, their legend labels will be picked up from the response."
 msgstr ""
 
+msgid "Metrics|Validating query"
+msgstr ""
+
 msgid "Metrics|Y-axis label"
 msgstr ""