Merge branch '30101-fix-performance' into 'master'

Add reactive-cache to pipeline config variables

See merge request gitlab-org/gitlab!47490

app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue

@@ -19,7 +19,9 @@ import {
 import { s__, __, n__ } from '~/locale';
 import axios from '~/lib/utils/axios_utils';
 import { redirectTo } from '~/lib/utils/url_utility';
-import { VARIABLE_TYPE, FILE_TYPE } from '../constants';
+import { VARIABLE_TYPE, FILE_TYPE, CONFIG_VARIABLES_TIMEOUT } from '../constants';
+import { backOff } from '~/lib/utils/common_utils';
+import httpStatusCodes from '~/lib/utils/http_status';
 
 export default {
   typeOptions: [
@@ -211,32 +213,49 @@ export default {
     },
 
     fetchConfigVariables(refValue) {
-      if (gon?.features?.newPipelineFormPrefilledVars) {
-        this.isLoading = true;
+      if (!gon?.features?.newPipelineFormPrefilledVars) {
+        return Promise.resolve({ params: {}, descriptions: {} });
+      }
+
+      this.isLoading = true;
 
-        return axios
+      return backOff((next, stop) => {
+        axios
           .get(this.configVariablesPath, {
             params: {
               sha: refValue,
             },
           })
-          .then(({ data }) => {
-            const params = {};
-            const descriptions = {};
+          .then(({ data, status }) => {
+            if (status === httpStatusCodes.NO_CONTENT) {
+              next();
+            } else {
+              this.isLoading = false;
+              stop(data);
+            }
+          })
+          .catch(error => {
+            stop(error);
+          });
+      }, CONFIG_VARIABLES_TIMEOUT)
+        .then(data => {
+          const params = {};
+          const descriptions = {};
 
-            Object.entries(data).forEach(([key, { value, description }]) => {
-              if (description !== null) {
-                params[key] = value;
-                descriptions[key] = description;
-              }
-            });
+          Object.entries(data).forEach(([key, { value, description }]) => {
+            if (description !== null) {
+              params[key] = value;
+              descriptions[key] = description;
+            }
+          });
 
-            this.isLoading = false;
+          return { params, descriptions };
+        })
+        .catch(() => {
+          this.isLoading = false;
 
-            return { params, descriptions };
-          });
-      }
-      return Promise.resolve({ params: {}, descriptions: {} });
+          return { params: {}, descriptions: {} };
+        });
     },
     createPipeline() {
       const filteredVariables = this.variables

app/assets/javascripts/pipeline_new/constants.js

 export const VARIABLE_TYPE = 'env_var';
 export const FILE_TYPE = 'file';
+export const CONFIG_VARIABLES_TIMEOUT = 5000;

app/controllers/projects/pipelines_controller.rb

@@ -214,7 +214,9 @@ class Projects::PipelinesController < Projects::ApplicationController
   def config_variables
     respond_to do |format|
       format.json do
-        render json: Ci::ListConfigVariablesService.new(@project, current_user).execute(params[:sha])
+        result = Ci::ListConfigVariablesService.new(@project, current_user).execute(params[:sha])
+
+        result.nil? ? head(:no_content) : render(json: result)
       end
     end
   end

app/services/ci/list_config_variables_service.rb

@@ -2,7 +2,26 @@
 
 module Ci
   class ListConfigVariablesService < ::BaseService
+    include ReactiveCaching
+
+    self.reactive_cache_key = ->(service) { [service.class.name, service.id] }
+    self.reactive_cache_work_type = :external_dependency
+    self.reactive_cache_worker_finder = ->(id, *_args) { from_cache(id) }
+
+    def self.from_cache(id)
+      project_id, user_id = id.split('-')
+
+      project = Project.find(project_id)
+      user = User.find(user_id)
+
+      new(project, user)
+    end
+
     def execute(sha)
+      with_reactive_cache(sha) { |result| result }
+    end
+
+    def calculate_reactive_cache(sha)
       config = project.ci_config_for(sha)
       return {} unless config
 
@@ -12,5 +31,10 @@ module Ci
 
       result.valid? ? result.variables_with_data : {}
     end
+
+    # Required for ReactiveCaching, it is also used in `reactive_cache_worker_finder`
+    def id
+      "#{project.id}-#{current_user.id}"
+    end
   end
 end

spec/controllers/projects/pipelines_controller_spec.rb

@@ -1149,11 +1149,17 @@ RSpec.describe Projects::PipelinesController do
     end
   end
 
-  describe 'GET config_variables.json' do
+  describe 'GET config_variables.json', :use_clean_rails_memory_store_caching do
+    include ReactiveCachingHelpers
+
     let(:result) { YAML.dump(ci_config) }
+    let(:service) { Ci::ListConfigVariablesService.new(project, user) }
 
     before do
       stub_gitlab_ci_yml_for_sha(sha, result)
+      allow(Ci::ListConfigVariablesService)
+        .to receive(:new)
+        .and_return(service)
     end
 
     context 'when sending a valid sha' do
@@ -1170,6 +1176,10 @@ RSpec.describe Projects::PipelinesController do
         }
       end
 
+      before do
+        synchronous_reactive_cache(service)
+      end
+
       it 'returns variable list' do
         get_config_variables
 
@@ -1182,6 +1192,10 @@ RSpec.describe Projects::PipelinesController do
       let(:sha) { 'invalid-sha' }
       let(:ci_config) { nil }
 
+      before do
+        synchronous_reactive_cache(service)
+      end
+
       it 'returns empty json' do
         get_config_variables
 
@@ -1204,6 +1218,10 @@ RSpec.describe Projects::PipelinesController do
         }
       end
 
+      before do
+        synchronous_reactive_cache(service)
+      end
+
       it 'returns empty result' do
         get_config_variables
 
@@ -1212,6 +1230,27 @@ RSpec.describe Projects::PipelinesController do
       end
     end
 
+    context 'when the cache is empty' do
+      let(:sha) { 'master' }
+      let(:ci_config) do
+        {
+          variables: {
+            KEY1: { value: 'val 1', description: 'description 1' }
+          },
+          test: {
+            stage: 'test',
+            script: 'echo'
+          }
+        }
+      end
+
+      it 'returns no content' do
+        get_config_variables
+
+        expect(response).to have_gitlab_http_status(:no_content)
+      end
+    end
+
     private
 
     def stub_gitlab_ci_yml_for_sha(sha, result)

spec/services/ci/list_config_variables_service_spec.rb

@@ -2,7 +2,9 @@
 
 require 'spec_helper'
 
-RSpec.describe Ci::ListConfigVariablesService do
+RSpec.describe Ci::ListConfigVariablesService, :use_clean_rails_memory_store_caching do
+  include ReactiveCachingHelpers
+
   let(:project) { create(:project, :repository) }
   let(:user) { project.creator }
   let(:service) { described_class.new(project, user) }
@@ -31,6 +33,10 @@ RSpec.describe Ci::ListConfigVariablesService do
       }
     end
 
+    before do
+      synchronous_reactive_cache(service)
+    end
+
     it 'returns variable list' do
       expect(subject['KEY1']).to eq({ value: 'val 1', description: 'description 1' })
       expect(subject['KEY2']).to eq({ value: 'val 2', description: '' })
@@ -65,6 +71,8 @@ RSpec.describe Ci::ListConfigVariablesService do
           HEREDOC
         end
       end
+
+      synchronous_reactive_cache(service)
     end
 
     it 'returns variable list' do
@@ -77,6 +85,10 @@ RSpec.describe Ci::ListConfigVariablesService do
     let(:sha) { 'invalid-sha' }
     let(:ci_config) { nil }
 
+    before do
+      synchronous_reactive_cache(service)
+    end
+
     it 'returns empty json' do
       expect(subject).to eq({})
     end
@@ -96,11 +108,44 @@ RSpec.describe Ci::ListConfigVariablesService do
       }
     end
 
+    before do
+      synchronous_reactive_cache(service)
+    end
+
     it 'returns empty result' do
       expect(subject).to eq({})
     end
   end
 
+  context 'when reading from cache' do
+    let(:sha) { 'master' }
+    let(:ci_config) { {} }
+    let(:reactive_cache_params) { [sha] }
+    let(:return_value) { { 'KEY1' => { value: 'val 1', description: 'description 1' } } }
+
+    before do
+      stub_reactive_cache(service, return_value, reactive_cache_params)
+    end
+
+    it 'returns variable list' do
+      expect(subject).to eq(return_value)
+    end
+  end
+
+  context 'when the cache is empty' do
+    let(:sha) { 'master' }
+    let(:ci_config) { {} }
+    let(:reactive_cache_params) { [sha] }
+
+    it 'returns nil and enquques the worker to fill cache' do
+      expect(ExternalServiceReactiveCachingWorker)
+        .to receive(:perform_async)
+        .with(service.class, service.id, *reactive_cache_params)
+
+      expect(subject).to be_nil
+    end
+  end
+
   private
 
   def stub_gitlab_ci_yml_for_sha(sha, result)