Merge branch...

Merge branch '227575-fe-implementation-convert-rest-api-to-graphql-mutation-for-mr-creation-flow' into 'master'

Replace POST with GraphQL mutation in SAST Configuration

See merge request gitlab-org/gitlab!41995

ee/app/assets/javascripts/security_configuration/components/app.vue

@@ -67,7 +67,7 @@ export default {
       required: false,
       default: false,
     },
-    // TODO: Remove as part of https://gitlab.com/gitlab-org/gitlab/-/issues/227575
+    // TODO: Remove as part of https://gitlab.com/gitlab-org/gitlab/-/issues/241377
     createSastMergeRequestPath: {
       type: String,
       required: true,

ee/app/assets/javascripts/security_configuration/components/manage_feature.vue

@@ -29,7 +29,7 @@ export default {
     canConfigureFeature() {
       return Boolean(this.glFeatures.sastConfigurationUi && this.feature.configuration_path);
     },
-    // TODO: Remove as part of https://gitlab.com/gitlab-org/gitlab/-/issues/227575
+    // TODO: Remove as part of https://gitlab.com/gitlab-org/gitlab/-/issues/241377
     canCreateSASTMergeRequest() {
       return Boolean(this.feature.type === 'sast' && this.createSastMergeRequestPath);
     },
@@ -71,7 +71,7 @@ export default {
     >{{ s__('SecurityConfiguration|Enable') }}</gl-button
   >
 
-  <!-- TODO: Remove as part of https://gitlab.com/gitlab-org/gitlab/-/issues/227575 -->
+  <!-- TODO: Remove as part of https://gitlab.com/gitlab-org/gitlab/-/issues/241377 -->
   <create-merge-request-button
     v-else-if="canCreateSASTMergeRequest"
     :auto-devops-enabled="autoDevopsEnabled"

ee/app/assets/javascripts/security_configuration/sast/components/app.vue

@@ -3,7 +3,6 @@ import { GlAlert, GlLink, GlLoadingIcon, GlSprintf } from '@gitlab/ui';
 import { s__ } from '~/locale';
 import sastCiConfigurationQuery from '../graphql/sast_ci_configuration.query.graphql';
 import ConfigurationForm from './configuration_form.vue';
-import { extractSastConfigurationEntities } from './utils';
 
 export default {
   components: {
@@ -24,16 +23,18 @@ export default {
     },
   },
   apollo: {
-    sastConfigurationEntities: {
+    sastCiConfiguration: {
       query: sastCiConfigurationQuery,
       variables() {
         return {
           fullPath: this.projectPath,
         };
       },
-      update: extractSastConfigurationEntities,
+      update({ project }) {
+        return project?.sastCiConfiguration;
+      },
       result({ loading }) {
-        if (!loading && this.sastConfigurationEntities.length === 0) {
+        if (!loading && !this.sastCiConfiguration) {
           this.onError();
         }
       },
@@ -44,7 +45,7 @@ export default {
   },
   data() {
     return {
-      sastConfigurationEntities: [],
+      sastCiConfiguration: null,
       hasLoadingError: false,
       showFeedbackAlert: true,
     };
@@ -114,6 +115,6 @@ export default {
       >{{ $options.i18n.loadingErrorText }}</gl-alert
     >
 
-    <configuration-form v-else :entities="sastConfigurationEntities" />
+    <configuration-form v-else :sast-ci-configuration="sastCiConfiguration" />
   </article>
 </template>

ee/app/assets/javascripts/security_configuration/sast/components/configuration_form.vue

@@ -3,10 +3,10 @@ import { GlAlert, GlButton } from '@gitlab/ui';
 import * as Sentry from '@sentry/browser';
 import { cloneDeep } from 'lodash';
 import { __, s__ } from '~/locale';
-import axios from '~/lib/utils/axios_utils';
 import { redirectTo } from '~/lib/utils/url_utility';
 import DynamicFields from './dynamic_fields.vue';
-import { isValidConfigurationEntity } from './utils';
+import configureSastMutation from '../graphql/configure_sast.mutation.graphql';
+import { toSastCiConfigurationEntityInput } from './utils';
 
 export default {
   components: {
@@ -23,17 +23,22 @@ export default {
       from: 'securityConfigurationPath',
       default: '',
     },
+    projectPath: {
+      from: 'projectPath',
+      default: '',
+    },
   },
   props: {
-    entities: {
-      type: Array,
+    // A SastCiConfiguration GraphQL object
+    sastCiConfiguration: {
+      type: Object,
       required: true,
-      validator: value => value.every(isValidConfigurationEntity),
     },
   },
   data() {
     return {
-      formEntities: cloneDeep(this.entities),
+      globalConfiguration: cloneDeep(this.sastCiConfiguration.global.nodes),
+      pipelineConfiguration: cloneDeep(this.sastCiConfiguration.pipeline.nodes),
       hasSubmissionError: false,
       isSubmitting: false,
     };
@@ -43,16 +48,25 @@ export default {
       this.isSubmitting = true;
       this.hasSubmissionError = false;
 
-      return axios
-        .post(this.createSastMergeRequestPath, this.getFormData())
+      return this.$apollo
+        .mutate({
+          mutation: configureSastMutation,
+          variables: {
+            input: {
+              projectPath: this.projectPath,
+              configuration: this.getMutationConfiguration(),
+            },
+          },
+        })
         .then(({ data }) => {
-          const { filePath } = data;
-          if (!filePath) {
+          const { errors, successPath } = data.configureSast;
+
+          if (errors.length > 0 || !successPath) {
             // eslint-disable-next-line @gitlab/require-i18n-strings
-            throw new Error('SAST merge request creation failed');
+            throw new Error('SAST merge request creation mutation failed');
           }
 
-          redirectTo(filePath);
+          redirectTo(successPath);
         })
         .catch(error => {
           this.isSubmitting = false;
@@ -60,11 +74,11 @@ export default {
           Sentry.captureException(error);
         });
     },
-    getFormData() {
-      return this.formEntities.reduce((acc, { field, value }) => {
-        acc[field] = value;
-        return acc;
-      }, {});
+    getMutationConfiguration() {
+      return {
+        global: this.globalConfiguration.map(toSastCiConfigurationEntityInput),
+        pipeline: this.pipelineConfiguration.map(toSastCiConfigurationEntityInput),
+      };
     },
   },
   i18n: {
@@ -79,7 +93,8 @@ export default {
 
 <template>
   <form @submit.prevent="onSubmit">
-    <dynamic-fields v-model="formEntities" />
+    <dynamic-fields v-model="globalConfiguration" class="gl-m-0" />
+    <dynamic-fields v-model="pipelineConfiguration" class="gl-m-0" />
 
     <hr />
 

ee/app/assets/javascripts/security_configuration/sast/components/utils.js

@@ -28,11 +28,14 @@ export const isValidAnalyzerEntity = object => {
   return isString(name) && isString(label) && isString(description) && isBoolean(enabled);
 };
 
-export const extractSastConfigurationEntities = ({ project }) => {
-  if (!project?.sastCiConfiguration) {
-    return [];
-  }
-
-  const { global, pipeline } = project.sastCiConfiguration;
-  return [...global.nodes, ...pipeline.nodes];
-};
+/**
+ * Given a SastCiConfigurationEntity, returns a SastCiConfigurationEntityInput
+ * suitable for use in the configureSast GraphQL mutation.
+ * @param {SastCiConfigurationEntity}
+ * @returns {SastCiConfigurationEntityInput}
+ */
+export const toSastCiConfigurationEntityInput = ({ field, defaultValue, value }) => ({
+  field,
+  defaultValue,
+  value,
+});

ee/app/assets/javascripts/security_configuration/sast/graphql/configure_sast.mutation.graphql

+mutation configureSast($input: ConfigureSastInput!) {
+  configureSast(input: $input) {
+    successPath
+    errors
+  }
+}

ee/app/graphql/mutations/security/ci_configuration/configure_sast.rb

@@ -26,7 +26,6 @@ module Mutations
 
         def resolve(project_path:, configuration:)
           project = authorized_find!(full_path: project_path)
-          validate_flag!(project)
 
           sast_create_service_params = format_for_service(configuration)
           result = ::Security::CiConfiguration::SastCreateService.new(project, current_user, sast_create_service_params).execute
@@ -35,12 +34,6 @@ module Mutations
 
         private
 
-        def validate_flag!(project)
-          return if ::Feature.enabled?(:security_sast_configuration, project)
-
-          raise Gitlab::Graphql::Errors::ResourceNotAvailable, 'security_sast_configuration flag is not enabled on this project'
-        end
-
         def find_object(full_path:)
           resolve_project(full_path: full_path)
         end

ee/config/feature_flags/development/security_sast_configuration.yml

----
-name: security_sast_configuration
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40637
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/235929
-group: group::static analysis
-type: development
-default_enabled: false

ee/spec/frontend/security_configuration/sast/components/app_spec.js

@@ -2,7 +2,7 @@ import { shallowMount } from '@vue/test-utils';
 import { GlLink, GlLoadingIcon, GlSprintf } from '@gitlab/ui';
 import SASTConfigurationApp from 'ee/security_configuration/sast/components/app.vue';
 import ConfigurationForm from 'ee/security_configuration/sast/components/configuration_form.vue';
-import { makeEntities } from './helpers';
+import { makeSastCiConfiguration } from './helpers';
 
 const sastDocumentationPath = '/help/sast';
 const projectPath = 'namespace/project';
@@ -11,11 +11,10 @@ describe('SAST Configuration App', () => {
   let wrapper;
 
   const createComponent = ({
-    provide = {},
     stubs = {},
-    loading = false,
+    loading = true,
     hasLoadingError = false,
-    sastConfigurationEntities = [],
+    sastCiConfiguration = null,
   } = {}) => {
     wrapper = shallowMount(SASTConfigurationApp, {
       mocks: { $apollo: { loading } },
@@ -23,16 +22,16 @@ describe('SAST Configuration App', () => {
       provide: {
         sastDocumentationPath,
         projectPath,
-        ...provide,
       },
-    });
-
-    // While setData is usually frowned upon, it is the documented way of
-    // mocking GraphQL response data:
+      // While setting data is usually frowned upon, it is the documented way
+      // of mocking GraphQL response data:
       // https://docs.gitlab.com/ee/development/fe_guide/graphql.html#testing
-    wrapper.setData({
+      data() {
+        return {
           hasLoadingError,
-      sastConfigurationEntities,
+          sastCiConfiguration,
+        };
+      },
     });
   };
 
@@ -119,6 +118,7 @@ describe('SAST Configuration App', () => {
   describe('when loading failed', () => {
     beforeEach(() => {
       createComponent({
+        loading: false,
         hasLoadingError: true,
       });
     });
@@ -137,12 +137,13 @@ describe('SAST Configuration App', () => {
   });
 
   describe('when loaded', () => {
-    let entities;
+    let sastCiConfiguration;
 
     beforeEach(() => {
-      entities = makeEntities(3);
+      sastCiConfiguration = makeSastCiConfiguration();
       createComponent({
-        sastConfigurationEntities: entities,
+        loading: false,
+        sastCiConfiguration,
       });
     });
 
@@ -154,8 +155,8 @@ describe('SAST Configuration App', () => {
       expect(findConfigurationForm().exists()).toBe(true);
     });
 
-    it('passes the sastConfigurationEntities to the entities prop', () => {
-      expect(findConfigurationForm().props('entities')).toBe(entities);
+    it('passes the sastCiConfiguration to the sastCiConfiguration prop', () => {
+      expect(findConfigurationForm().props('sastCiConfiguration')).toBe(sastCiConfiguration);
     });
 
     it('does not display an alert message', () => {

ee/spec/frontend/security_configuration/sast/components/configuration_form_spec.js

 import * as Sentry from '@sentry/browser';
-import AxiosMockAdapter from 'axios-mock-adapter';
 import { GlAlert } from '@gitlab/ui';
-import waitForPromises from 'helpers/wait_for_promises';
 import { shallowMount } from '@vue/test-utils';
 import ConfigurationForm from 'ee/security_configuration/sast/components/configuration_form.vue';
 import DynamicFields from 'ee/security_configuration/sast/components/dynamic_fields.vue';
+import configureSastMutation from 'ee/security_configuration/sast/graphql/configure_sast.mutation.graphql';
 import { redirectTo } from '~/lib/utils/url_utility';
-import axios from '~/lib/utils/axios_utils';
-import { makeEntities } from './helpers';
+import { makeEntities, makeSastCiConfiguration } from './helpers';
 
 jest.mock('~/lib/utils/url_utility', () => ({
   redirectTo: jest.fn(),
 }));
 
-const createSastMergeRequestPath = '/merge_request/create';
+const projectPath = 'group/project';
 const securityConfigurationPath = '/security/configuration';
 const newMergeRequestPath = '/merge_request/new';
 
 describe('ConfigurationForm component', () => {
   let wrapper;
-  let entities;
-  let axiosMock;
+  let sastCiConfiguration;
 
-  const createComponent = ({ props = {} } = {}) => {
-    entities = makeEntities(3, { value: 'foo' });
+  let pendingPromiseResolvers;
+  const fulfillPendingPromises = () => {
+    pendingPromiseResolvers.forEach(resolve => resolve());
+  };
+
+  const createComponent = ({ mutationResult } = {}) => {
+    sastCiConfiguration = makeSastCiConfiguration();
 
     wrapper = shallowMount(ConfigurationForm, {
       provide: {
-        createSastMergeRequestPath,
+        projectPath,
         securityConfigurationPath,
       },
       propsData: {
-        entities,
-        ...props,
+        sastCiConfiguration,
+      },
+      mocks: {
+        $apollo: {
+          mutate: jest.fn(
+            () =>
+              new Promise(resolve => {
+                pendingPromiseResolvers.push(() =>
+                  resolve({
+                    data: { configureSast: mutationResult },
+                  }),
+                );
+              }),
+          ),
+        },
       },
     });
   };
@@ -41,52 +56,73 @@ describe('ConfigurationForm component', () => {
   const findSubmitButton = () => wrapper.find({ ref: 'submitButton' });
   const findErrorAlert = () => wrapper.find(GlAlert);
   const findCancelButton = () => wrapper.find({ ref: 'cancelButton' });
-  const findDynamicFieldsComponent = () => wrapper.find(DynamicFields);
+  const findDynamicFieldsComponents = () => wrapper.findAll(DynamicFields);
 
   const expectPayloadForEntities = () => {
-    const { post } = axiosMock.history;
-
-    expect(post).toHaveLength(1);
+    const expectedPayload = {
+      mutation: configureSastMutation,
+      variables: {
+        input: {
+          projectPath,
+          configuration: {
+            global: [
+              {
+                field: 'field0',
+                defaultValue: 'defaultValue0',
+                value: 'value0',
+              },
+            ],
+            pipeline: [
+              {
+                field: 'field1',
+                defaultValue: 'defaultValue1',
+                value: 'value1',
+              },
+            ],
+          },
+        },
+      },
+    };
 
-    const postedData = JSON.parse(post[0].data);
-    entities.forEach(entity => {
-      expect(postedData[entity.field]).toBe(entity.value);
-    });
+    expect(wrapper.vm.$apollo.mutate.mock.calls).toEqual([[expectedPayload]]);
   };
 
   beforeEach(() => {
-    axiosMock = new AxiosMockAdapter(axios);
+    pendingPromiseResolvers = [];
   });
 
   afterEach(() => {
     wrapper.destroy();
-    wrapper = null;
-    axiosMock.restore();
   });
 
-  describe('the DynamicFields component', () => {
+  describe.each`
+    type          | expectedPosition
+    ${'global'}   | ${0}
+    ${'pipeline'} | ${1}
+  `('the $type DynamicFields component', ({ type, expectedPosition }) => {
+    let dynamicFields;
+
     beforeEach(() => {
       createComponent();
+      dynamicFields = findDynamicFieldsComponents().at(expectedPosition);
     });
 
     it('renders', () => {
-      expect(findDynamicFieldsComponent().exists()).toBe(true);
+      expect(dynamicFields.exists()).toBe(true);
     });
 
-    it('recieves a copy of the entities prop', () => {
-      const entitiesProp = findDynamicFieldsComponent().props('entities');
+    it(`receives a copy of the ${type} entities`, () => {
+      const entitiesProp = dynamicFields.props('entities');
 
-      expect(entitiesProp).not.toBe(entities);
-      expect(entitiesProp).toEqual(entities);
+      expect(entitiesProp).not.toBe(sastCiConfiguration[type].nodes);
+      expect(entitiesProp).toEqual(sastCiConfiguration[type].nodes);
     });
 
-    describe('when the dynamic fields component emits an input event', () => {
-      let dynamicFields;
+    describe('when it emits an input event', () => {
       let newEntities;
 
       beforeEach(() => {
-        dynamicFields = findDynamicFieldsComponent();
-        newEntities = makeEntities(3, { value: 'foo' });
+        newEntities = makeEntities(1);
         dynamicFields.vm.$emit(DynamicFields.model.event, newEntities);
       });
 
@@ -102,15 +138,17 @@ describe('ConfigurationForm component', () => {
     });
 
     describe.each`
-      context                    | filePath               | statusCode | partialErrorMessage
-      ${'a response error code'} | ${newMergeRequestPath} | ${500}     | ${'500'}
-      ${'no filePath'}           | ${''}                  | ${200}     | ${/merge request.*fail/}
-    `(
-      'given an unsuccessful endpoint response due to $context',
-      ({ filePath, statusCode, partialErrorMessage }) => {
+      context             | successPath | errors
+      ${'no successPath'} | ${''}       | ${[]}
+      ${'any errors'}     | ${''}       | ${['an error']}
+    `('given an unsuccessful endpoint response due to $context', ({ successPath, errors }) => {
       beforeEach(() => {
-          axiosMock.onPost(createSastMergeRequestPath).replyOnce(statusCode, { filePath });
-          createComponent();
+        createComponent({
+          mutationResult: {
+            successPath,
+            errors,
+          },
+        });
 
         findForm().trigger('submit');
       });
@@ -122,7 +160,7 @@ describe('ConfigurationForm component', () => {
       });
 
       describe('after async tasks', () => {
-          beforeEach(() => waitForPromises());
+        beforeEach(fulfillPendingPromises);
 
         it('does not call redirectTo', () => {
           expect(redirectTo).not.toHaveBeenCalled();
@@ -134,7 +172,7 @@ describe('ConfigurationForm component', () => {
 
         it('sends the error to Sentry', () => {
           expect(Sentry.captureException.mock.calls).toMatchObject([
-              [{ message: expect.stringMatching(partialErrorMessage) }],
+            [{ message: expect.stringMatching(/merge request.*fail/) }],
           ]);
         });
 
@@ -152,15 +190,16 @@ describe('ConfigurationForm component', () => {
           });
         });
       });
-      },
-    );
+    });
 
     describe('given a successful endpoint response', () => {
       beforeEach(() => {
-        axiosMock
-          .onPost(createSastMergeRequestPath)
-          .replyOnce(200, { filePath: newMergeRequestPath });
-        createComponent();
+        createComponent({
+          mutationResult: {
+            successPath: newMergeRequestPath,
+            errors: [],
+          },
+        });
 
         findForm().trigger('submit');
       });
@@ -172,7 +211,7 @@ describe('ConfigurationForm component', () => {
       });
 
       describe('after async tasks', () => {
-        beforeEach(() => waitForPromises());
+        beforeEach(fulfillPendingPromises);
 
         it('calls redirectTo', () => {
           expect(redirectTo).toHaveBeenCalledWith(newMergeRequestPath);

ee/spec/frontend/security_configuration/sast/components/helpers.js

@@ -14,10 +14,30 @@ export const makeEntities = (count, changes) =>
     field: `field${i}`,
     label: `label${i}`,
     type: 'string',
-    value: `defaultValue${i}`,
+    value: `value${i}`,
     ...changes,
   }));
 
+/**
+ * Creates a mock SastCiConfiguration GraphQL object instance.
+ *
+ * @param {number} totalEntities - The total number of entities to create.
+ * @returns {SastCiConfiguration}
+ */
+export const makeSastCiConfiguration = (totalEntities = 2) => {
+  // Call makeEntities just once to ensure unique fields
+  const entities = makeEntities(totalEntities);
+
+  return {
+    global: {
+      nodes: entities.slice(0, totalEntities - 1),
+    },
+    pipeline: {
+      nodes: entities.slice(totalEntities - 1),
+    },
+  };
+};
+
 /**
  * Creates an array of objects matching the shape of a GraphQl
  * SastCiConfigurationAnalyzersEntity.

ee/spec/frontend/security_configuration/sast/components/utils_spec.js

 import {
   isValidConfigurationEntity,
   isValidAnalyzerEntity,
-  extractSastConfigurationEntities,
+  toSastCiConfigurationEntityInput,
 } from 'ee/security_configuration/sast/components/utils';
 import { makeEntities, makeAnalyzerEntities } from './helpers';
 
@@ -54,36 +54,20 @@ describe('isValidAnalyzerEntity', () => {
   });
 });
 
-describe('extractSastConfigurationEntities', () => {
-  describe.each`
-    context                    | response
-    ${'which is empty'}        | ${{}}
-    ${'with no project'}       | ${{ project: null }}
-    ${'with no configuration'} | ${{ project: {} }}
-  `('given a response $context', ({ response }) => {
-    it('returns an empty array', () => {
-      expect(extractSastConfigurationEntities(response)).toEqual([]);
-    });
-  });
-
-  describe('given a valid response', () => {
-    it('returns an array of entities from the global and pipeline sections', () => {
-      const globalEntities = makeEntities(3, { description: 'global' });
-      const pipelineEntities = makeEntities(3, { description: 'pipeline' });
+describe('toSastCiConfigurationEntityInput', () => {
+  let entity = makeEntities(1);
 
-      const response = {
-        project: {
-          sastCiConfiguration: {
-            global: { nodes: globalEntities },
-            pipeline: { nodes: pipelineEntities },
-          },
-        },
-      };
+  describe('given a SastCiConfigurationEntity', () => {
+    beforeEach(() => {
+      [entity] = makeEntities(1);
+    });
 
-      expect(extractSastConfigurationEntities(response)).toEqual([
-        ...globalEntities,
-        ...pipelineEntities,
-      ]);
+    it('returns a SastCiConfigurationEntityInput object', () => {
+      expect(toSastCiConfigurationEntityInput(entity)).toEqual({
+        field: entity.field,
+        defaultValue: entity.defaultValue,
+        value: entity.value,
+      });
     });
   });
 });

ee/spec/graphql/mutations/security/ci_configuration/configure_sast_spec.rb

@@ -32,10 +32,6 @@ RSpec.describe Mutations::Security::CiConfiguration::ConfigureSast do
     )
   end
 
-  before do
-    stub_feature_flags(security_sast_configuration: true)
-  end
-
   specify { expect(described_class).to require_graphql_authorizations(:push_code) }
 
   describe '#resolve' do
@@ -86,14 +82,6 @@ RSpec.describe Mutations::Security::CiConfiguration::ConfigureSast do
         end
       end
 
-      context 'when sast configuration feature is not enabled' do
-        it 'raises an exception' do
-          stub_feature_flags(security_sast_configuration: false)
-
-          expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
-        end
-      end
-
       context 'when service can not generate any path to create a new merge request' do
         it 'returns an array of errors' do
           allow_next_instance_of(::Security::CiConfiguration::SastCreateService) do |service|