Merge branch '321101-dont-wipe-payloadAttributeMappings-when-omitted' into 'master'

Don't process payload_attribute_mapping when it's blank

See merge request gitlab-org/gitlab!55511

app/models/alert_management/http_integration.rb

@@ -27,6 +27,7 @@ class HttpIntegration < ApplicationRecord
     before_validation :prevent_token_assignment
     before_validation :prevent_endpoint_identifier_assignment
     before_validation :ensure_token
+    before_validation :ensure_payload_example_not_nil
     scope :for_endpoint_identifier, -> (endpoint_identifier) { where(endpoint_identifier: endpoint_identifier) }
     scope :active, -> { where(active: true) }
@@ -74,5 +75,9 @@ def prevent_endpoint_identifier_assignment
         self.endpoint_identifier = endpoint_identifier_was
       end
     end
+
+    def ensure_payload_example_not_nil
+      self.payload_example ||= {}
+    end
   end
 end

ee/app/graphql/ee/mutations/alert_management/http_integration/http_integration_base.rb

@@ -10,12 +10,6 @@ module HttpIntegrationBase
           private
-          def payload_attribute_mapping(mappings)
-            Array(mappings).each_with_object({}) do |m, h|
-              h[m.field_name] = { path: m.path, type: m.type, label: m.label }
-            end
-          end
-
           def validate_payload_example!(payload_example)
             return if ::Gitlab::Utils::DeepSize.new(payload_example).valid?
@@ -30,9 +24,23 @@ def http_integration_params(project, args)
             validate_payload_example!(args[:payload_example])
-            args.slice(*base_args.keys, :payload_example).merge(
-              payload_attribute_mapping: payload_attribute_mapping(args[:payload_attribute_mappings])
-            )
+            args
+              .slice(*base_args.keys, :payload_example)
+              .merge(payload_attribute_mapping_params(args))
+          end
+
+          def payload_attribute_mapping_params(args)
+            # Don't process payload_attribute_mapping when it's not part of a mutation params.
+            # Otherwise, it's going to reset already persisted value.
+            return {} unless args.key?(:payload_attribute_mappings)
+
+            { payload_attribute_mapping: payload_attribute_mapping(args[:payload_attribute_mappings]) }
+          end
+
+          def payload_attribute_mapping(mappings)
+            Array(mappings).each_with_object({}) do |m, h|
+              h[m.field_name] = { path: m.path, type: m.type, label: m.label }
+            end
           end
         end
       end

ee/changelogs/unreleased/321101-dont-wipe-payloadAttributeMappings-when-omitted.yml

+---
+title: Prevents HttpIntegration.payload_attribute_mapping from being reset when it was not a part of a GraphQL mutation input.
+merge_request: 55511
+author:
+type: fixed

ee/spec/requests/api/graphql/mutations/alert_management/http_integration/update_spec.rb

@@ -33,14 +33,18 @@
     }
     graphql_mutation(:http_integration_update, variables) do
       <<~QL
-         clientMutationId
-         errors
-         integration {
-           id
-           name
-           active
-           url
-         }
+        clientMutationId
+        errors
+        integration {
+          id
+          name
+          active
+          url
+          payloadExample
+          payloadAttributeMappings {
+            fieldName
+          }
+        }
       QL
     end
   end
@@ -48,12 +52,14 @@
   let(:mutation_response) { graphql_mutation_response(:http_integration_update) }
   shared_examples 'ignoring the custom mapping' do
-    it 'updates integration without the custom mapping params' do
+    it 'updates integration without the custom mapping params', :aggregate_failures do
       post_graphql_mutation(mutation, current_user: current_user)
+      integration_response = mutation_response['integration']
+
       expect(response).to have_gitlab_http_status(:success)
-      expect(integration.payload_example).to eq({})
-      expect(integration.payload_attribute_mapping).to eq({})
+      expect(integration_response['payloadExample']).to eq('{}')
+      expect(integration_response['payloadAttributeMappings']).to be_empty
     end
   end
@@ -67,6 +73,116 @@
   it_behaves_like 'validating the payload_example'
   it_behaves_like 'validating the payload_attribute_mappings'
+  it 'updates the custom mapping params', :aggregate_failures do
+    post_graphql_mutation(mutation, current_user: current_user)
+
+    expect(response).to have_gitlab_http_status(:success)
+    integration.reload
+    expect(integration.payload_example).to eq(Gitlab::Json.parse(payload_example))
+    expect(integration.payload_attribute_mapping).to eq(
+      'start_time' => {
+        'label' => 'Start time',
+        'path' => %w[started_at],
+        'type' => 'datetime'
+      },
+      'title' => {
+        'label' => nil,
+        'path' => %w[alert name],
+        'type' => 'string'
+      }
+    )
+  end
+
+  context 'when the integration already has custom mapping params' do
+    let_it_be(:current_payload_example) do
+      { 'alert' => { 'name' => 'Test alert', 'desc' => 'Description' } }
+    end
+
+    let_it_be(:current_mapping) do
+      {
+        'title' => { 'path' => %w(alert name), 'type' => 'string', 'label' => 'Title' },
+        'description' => { 'path' => %w(alert desc), 'type' => 'string' }
+      }
+    end
+
+    let_it_be(:integration) do
+      create(:alert_management_http_integration, project: project, payload_example: current_payload_example, payload_attribute_mapping: current_mapping)
+    end
+
+    context 'when the custom mappings attributes are blank' do
+      let(:payload_example) { "{}" }
+      let(:payload_attribute_mappings) { [] }
+
+      it 'resets the custom mapping params', :aggregate_failures do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        integration_response = mutation_response['integration']
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(integration_response['id']).to eq(GitlabSchema.id_from_object(integration).to_s)
+        expect(integration_response['name']).to eq('Modified Name')
+        expect(integration_response['payloadExample']).to eq('{}')
+        expect(integration_response['payloadAttributeMappings']).to be_empty
+      end
+    end
+
+    context 'when the custom mappings attributes are nils' do
+      let(:payload_example) { nil }
+      let(:payload_attribute_mappings) { nil }
+
+      it 'resets the custom mapping params', :aggregate_failures do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        integration_response = mutation_response['integration']
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(integration_response['id']).to eq(GitlabSchema.id_from_object(integration).to_s)
+        expect(integration_response['name']).to eq('Modified Name')
+        expect(integration_response['payloadExample']).to eq('{}')
+        expect(integration_response['payloadAttributeMappings']).to be_empty
+      end
+    end
+
+    context 'when the custom mappings attributes are not part of the mutation variables' do
+      let(:mutation) do
+        variables = {
+          id: GitlabSchema.id_from_object(integration).to_s,
+          name: 'Modified Name'
+        }
+        graphql_mutation(:http_integration_update, variables) do
+          <<~QL
+            clientMutationId
+            errors
+            integration {
+              id
+              name
+              payloadExample
+              payloadAttributeMappings {
+                fieldName
+              }
+            }
+          QL
+        end
+      end
+
+      it 'does not reset the custom mapping attributes', :aggregate_failures do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        integration_response = mutation_response['integration']
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(integration_response['id']).to eq(GitlabSchema.id_from_object(integration).to_s)
+        expect(integration_response['name']).to eq('Modified Name')
+        expect(integration_response['payloadExample']).not_to eq('{}')
+        expect(integration_response['payloadAttributeMappings']).to be_present
+
+        integration.reload
+        expect(integration.payload_example).to eq(current_payload_example)
+        expect(integration.payload_attribute_mapping).to eq(current_mapping)
+      end
+    end
+  end
+
   context 'with the custom mappings feature unavailable' do
     before do
       stub_licensed_features(multiple_alert_http_integrations: false)

spec/models/alert_management/http_integration_spec.rb

@@ -81,6 +81,32 @@
     end
   end
+  describe 'before validation' do
+    describe '#ensure_payload_example_not_nil' do
+      subject(:integration) { build(:alert_management_http_integration, payload_example: payload_example) }
+
+      context 'when the payload_example is nil' do
+        let(:payload_example) { nil }
+
+        it 'sets the payload_example to empty JSON' do
+          integration.valid?
+
+          expect(integration.payload_example).to eq({})
+        end
+      end
+
+      context 'when the payload_example is not nil' do
+        let(:payload_example) { { 'key' => 'value' } }
+
+        it 'sets the payload_example to specified value' do
+          integration.valid?
+
+          expect(integration.payload_example).to eq(payload_example)
+        end
+      end
+    end
+  end
+
   describe '#token' do
     subject { integration.token }