Merge branch '34758-fix-code-reuse-clusters-applications_controller' into 'master'

Fix code reuse issue in Projects::Clusters::ApplicationController#index

See merge request gitlab-org/gitlab-ce!22182

app/controllers/projects/clusters/applications_controller.rb

@@ -2,31 +2,20 @@
 
 class Projects::Clusters::ApplicationsController < Projects::ApplicationController
   before_action :cluster
-  before_action :application_class, only: [:create]
   before_action :authorize_read_cluster!
   before_action :authorize_create_cluster!, only: [:create]
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def create
-    application = @application_class.find_or_initialize_by(cluster: @cluster)
-
-    if application.has_attribute?(:hostname)
-      application.hostname = params[:hostname]
-    end
-
-    if application.respond_to?(:oauth_application)
-      application.oauth_application = create_oauth_application(application)
-    end
-
-    application.save!
-
-    Clusters::Applications::ScheduleInstallationService.new(project, current_user).execute(application)
+    Clusters::Applications::CreateService
+      .new(@cluster, current_user, create_cluster_application_params)
+      .execute(request)
 
     head :no_content
+  rescue Clusters::Applications::CreateService::InvalidApplicationError
+    render_404
   rescue StandardError
     head :bad_request
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   private
 
@@ -34,18 +23,7 @@ class Projects::Clusters::ApplicationsController < Projects::ApplicationControll
     @cluster ||= project.clusters.find(params[:id]) || render_404
   end
 
-  def application_class
-    @application_class ||= Clusters::Cluster::APPLICATIONS[params[:application]] || render_404
-  end
-
-  def create_oauth_application(application)
-    oauth_application_params = {
-      name: params[:application],
-      redirect_uri: application.callback_url,
-      scopes: 'api read_user openid',
-      owner: current_user
-    }
-
-    Applications::CreateService.new(current_user, oauth_application_params).execute(request)
+  def create_cluster_application_params
+    params.permit(:application, :hostname)
   end
 end

app/services/applications/create_service.rb

@@ -9,6 +9,7 @@ module Applications
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    # EE would override and use `request` arg
     def execute(request)
       Doorkeeper::Application.create(@params)
     end

app/services/clusters/applications/create_service.rb

+# frozen_string_literal: true
+
+module Clusters
+  module Applications
+    class CreateService
+      InvalidApplicationError = Class.new(StandardError)
+
+      attr_reader :cluster, :current_user, :params
+
+      def initialize(cluster, user, params = {})
+        @cluster = cluster
+        @current_user = user
+        @params = params.dup
+      end
+
+      def execute(request)
+        create_application.tap do |application|
+          if application.has_attribute?(:hostname)
+            application.hostname = params[:hostname]
+          end
+
+          if application.respond_to?(:oauth_application)
+            application.oauth_application = create_oauth_application(application, request)
+          end
+
+          application.save!
+
+          Clusters::Applications::ScheduleInstallationService.new(application).execute
+        end
+      end
+
+      private
+
+      def create_application
+        builder.call(@cluster)
+      end
+
+      def builder
+        builders[application_name] || raise(InvalidApplicationError, "invalid application: #{application_name}")
+      end
+
+      def builders
+        {
+          "helm" => -> (cluster) { cluster.application_helm || cluster.build_application_helm },
+          "ingress" => -> (cluster) { cluster.application_ingress || cluster.build_application_ingress },
+          "prometheus" => -> (cluster) { cluster.application_prometheus || cluster.build_application_prometheus },
+          "runner" => -> (cluster) { cluster.application_runner || cluster.build_application_runner },
+          "jupyter" => -> (cluster) { cluster.application_jupyter || cluster.build_application_jupyter }
+        }
+      end
+
+      def application_name
+        params[:application]
+      end
+
+      def create_oauth_application(application, request)
+        oauth_application_params = {
+          name: params[:application],
+          redirect_uri: application.callback_url,
+          scopes: 'api read_user openid',
+          owner: current_user
+        }
+
+        ::Applications::CreateService.new(current_user, oauth_application_params).execute(request)
+      end
+    end
+  end
+end

app/services/clusters/applications/schedule_installation_service.rb

@@ -2,8 +2,14 @@
 
 module Clusters
   module Applications
-    class ScheduleInstallationService < ::BaseService
-      def execute(application)
+    class ScheduleInstallationService
+      attr_reader :application
+
+      def initialize(application)
+        @application = application
+      end
+
+      def execute
         application.make_scheduled!
 
         ClusterInstallAppWorker.perform_async(application.name, application.id)

spec/services/applications/create_service_spec.rb

+# frozen_string_literal: true
+
 require "spec_helper"
 
 describe ::Applications::CreateService do
+  include TestRequestHelpers
+
   let(:user) { create(:user) }
   let(:params) { attributes_for(:application) }
-  let(:request) do
-    if Gitlab.rails5?
-      ActionController::TestRequest.new({ remote_ip: "127.0.0.1" }, ActionController::TestSession.new)
-    else
-      ActionController::TestRequest.new(remote_ip: "127.0.0.1")
-    end
-  end
 
   subject { described_class.new(user, params) }
 
-  it { expect { subject.execute(request) }.to change { Doorkeeper::Application.count }.by(1) }
+  it { expect { subject.execute(test_request) }.to change { Doorkeeper::Application.count }.by(1) }
 end

spec/services/clusters/applications/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Clusters::Applications::CreateService do
+  include TestRequestHelpers
+
+  let(:cluster) { create(:cluster, :project, :provided_by_gcp) }
+  let(:user) { create(:user) }
+  let(:params) { { application: 'helm' } }
+  let(:service) { described_class.new(cluster, user, params) }
+
+  describe '#execute' do
+    before do
+      allow(ClusterInstallAppWorker).to receive(:perform_async)
+    end
+
+    subject { service.execute(test_request) }
+
+    it 'creates an application' do
+      expect do
+        subject
+
+        cluster.reload
+      end.to change(cluster, :application_helm)
+    end
+
+    it 'schedules an install via worker' do
+      expect(ClusterInstallAppWorker).to receive(:perform_async).with('helm', anything).once
+
+      subject
+    end
+
+    context 'jupyter application' do
+      let(:params) do
+        {
+          application: 'jupyter',
+          hostname: 'example.com'
+        }
+      end
+
+      before do
+        allow_any_instance_of(Clusters::Applications::ScheduleInstallationService).to receive(:execute)
+      end
+
+      it 'creates the application' do
+        expect do
+          subject
+
+          cluster.reload
+        end.to change(cluster, :application_jupyter)
+      end
+
+      it 'sets the hostname' do
+        expect(subject.hostname).to eq('example.com')
+      end
+
+      it 'sets the oauth_application' do
+        expect(subject.oauth_application).to be_present
+      end
+    end
+
+    context 'invalid application' do
+      let(:params) { { application: 'non-existent' } }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(Clusters::Applications::CreateService::InvalidApplicationError)
+      end
+    end
+  end
+end

spec/services/clusters/applications/schedule_installation_service_spec.rb

@@ -10,14 +10,13 @@ describe Clusters::Applications::ScheduleInstallationService do
       expect(ClusterInstallAppWorker).not_to receive(:perform_async)
       count_before = count_scheduled
 
-      expect { service.execute(application) }.to raise_error(StandardError)
+      expect { service.execute }.to raise_error(StandardError)
       expect(count_scheduled).to eq(count_before)
     end
   end
 
   describe '#execute' do
-    let(:project) { double(:project) }
-    let(:service) { described_class.new(project, nil) }
+    let(:service) { described_class.new(application) }
 
     context 'when application is installable' do
       let(:application) { create(:clusters_applications_helm, :installable) }
@@ -25,7 +24,7 @@ describe Clusters::Applications::ScheduleInstallationService do
       it 'make the application scheduled' do
         expect(ClusterInstallAppWorker).to receive(:perform_async).with(application.name, kind_of(Numeric)).once
 
-        expect { service.execute(application) }.to change { application.class.with_status(:scheduled).count }.by(1)
+        expect { service.execute }.to change { application.class.with_status(:scheduled).count }.by(1)
       end
     end
 

spec/support/helpers/test_request_helpers.rb

+# frozen_string_literal: true
+
+module TestRequestHelpers
+  def test_request(remote_ip: '127.0.0.1')
+    if Gitlab.rails5?
+      ActionController::TestRequest.new({ remote_ip: remote_ip }, ActionController::TestSession.new)
+    else
+      ActionController::TestRequest.new(remote_ip: remote_ip)
+    end
+  end
+end