Automatic merge of gitlab-org/gitlab-ce master

.rubocop.yml

@@ -275,3 +275,8 @@ RSpec/BeSuccessMatcher:
     - 'ee/spec/support/shared_examples/controllers/**/*'
     - 'spec/support/controllers/**/*'
     - 'ee/spec/support/controllers/**/*'
+Scalability/FileUploads:
+  Enabled: true
+  Include:
+    - 'lib/api/**/*.rb'
+    - 'ee/lib/api/**/*.rb'

config/initializers/sidekiq.rb

@@ -60,7 +60,7 @@ Sidekiq.configure_server do |config|
     # Sidekiq (e.g. in an initializer).
     ActiveRecord::Base.clear_all_connections!
 
-    Gitlab::SidekiqMonitor.instance.start if enable_sidekiq_monitor
+    Gitlab::SidekiqDaemon::Monitor.instance.start if enable_sidekiq_monitor
   end
 
   if enable_reliable_fetch?

doc/administration/troubleshooting/sidekiq.md

@@ -270,7 +270,7 @@ is interrupted mid-execution and it is not guaranteed
 that proper rollback of transactions is implemented.
 
 ```ruby
-Gitlab::SidekiqMonitor.cancel_job('job-id')
+Gitlab::SidekiqDaemon::Monitor.cancel_job('job-id')
 ```
 
 > This requires the Sidekiq to be run with `SIDEKIQ_MONITOR_WORKER=1`

lib/api/helpers/projects_helpers.rb

@@ -38,7 +38,8 @@ module API
         optional :only_allow_merge_if_pipeline_succeeds, type: Boolean, desc: 'Only allow to merge if builds succeed'
         optional :only_allow_merge_if_all_discussions_are_resolved, type: Boolean, desc: 'Only allow to merge if all discussions are resolved'
         optional :tag_list, type: Array[String], desc: 'The list of tags for a project'
-        optional :avatar, type: File, desc: 'Avatar image for project'
+        # TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab-ee/issues/14960
+        optional :avatar, type: File, desc: 'Avatar image for project' # rubocop:disable Scalability/FileUploads
         optional :printing_merge_request_link_enabled, type: Boolean, desc: 'Show link to create/view merge request when pushing from the command line'
         optional :merge_method, type: String, values: %w(ff rebase_merge merge), desc: 'The merge method used when merging merge requests'
         optional :initialize_with_readme, type: Boolean, desc: "Initialize a project with a README.md"

lib/api/pages_domains.rb

@@ -90,8 +90,11 @@ module API
       end
       params do
         requires :domain, type: String, desc: 'The domain'
+        # rubocop:disable Scalability/FileUploads
+        # TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab-ee/issues/14960
         optional :certificate, allow_blank: false, types: [File, String], desc: 'The certificate', as: :user_provided_certificate
         optional :key, allow_blank: false, types: [File, String], desc: 'The key', as: :user_provided_key
+        # rubocop:enable Scalability/FileUploads
         all_or_none_of :user_provided_certificate, :user_provided_key
       end
       post ":id/pages/domains" do
@@ -111,8 +114,11 @@ module API
       desc 'Updates a pages domain'
       params do
         requires :domain, type: String, desc: 'The domain'
+        # rubocop:disable Scalability/FileUploads
+        # TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab-ee/issues/14960
         optional :certificate, allow_blank: false, types: [File, String], desc: 'The certificate', as: :user_provided_certificate
         optional :key, allow_blank: false, types: [File, String], desc: 'The key', as: :user_provided_key
+        # rubocop:enable Scalability/FileUploads
       end
       put ":id/pages/domains/:domain", requirements: PAGES_DOMAINS_ENDPOINT_REQUIREMENTS do
         authorize! :update_pages, user_project

lib/api/project_import.rb

@@ -27,7 +27,8 @@ module API
     resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
       params do
         requires :path, type: String, desc: 'The new project path and name'
-        requires :file, type: File, desc: 'The project export file to be imported'
+        # TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab-ee/issues/14960
+        requires :file, type: File, desc: 'The project export file to be imported' # rubocop:disable Scalability/FileUploads
         optional :namespace, type: String, desc: "The ID or name of the namespace that the project will be imported into. Defaults to the current user's namespace."
         optional :overwrite, type: Boolean, default: false, desc: 'If there is a project in the same namespace and with the same name overwrite it'
         optional :override_params,

lib/api/projects.rb

@@ -478,7 +478,8 @@ module API
 
       desc 'Upload a file'
       params do
-        requires :file, type: File, desc: 'The file to be uploaded'
+        # TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab-ee/issues/14960
+        requires :file, type: File, desc: 'The file to be uploaded' # rubocop:disable Scalability/FileUploads
       end
       post ":id/uploads" do
         UploadService.new(user_project, params[:file]).execute.to_h

lib/api/users.rb

@@ -50,7 +50,8 @@ module API
           optional :admin, type: Boolean, desc: 'Flag indicating the user is an administrator'
           optional :can_create_group, type: Boolean, desc: 'Flag indicating the user can create groups'
           optional :external, type: Boolean, desc: 'Flag indicating the user is an external user'
-          optional :avatar, type: File, desc: 'Avatar image for user'
+          # TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab-ee/issues/14960
+          optional :avatar, type: File, desc: 'Avatar image for user' # rubocop:disable Scalability/FileUploads
           optional :private_profile, type: Boolean, default: false, desc: 'Flag indicating the user has a private profile'
           all_or_none_of :extern_uid, :provider
 

lib/gitlab/sidekiq_daemon/monitor.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module SidekiqDaemon
+    class Monitor < Daemon
+      include ::Gitlab::Utils::StrongMemoize
+
+      NOTIFICATION_CHANNEL = 'sidekiq:cancel:notifications'
+      CANCEL_DEADLINE = 24.hours.seconds
+      RECONNECT_TIME = 3.seconds
+
+      # We use exception derived from `Exception`
+      # to consider this as an very low-level exception
+      # that should not be caught by application
+      CancelledError = Class.new(Exception) # rubocop:disable Lint/InheritException
+
+      attr_reader :jobs_thread
+      attr_reader :jobs_mutex
+
+      def initialize
+        super
+
+        @jobs_thread = {}
+        @jobs_mutex = Mutex.new
+      end
+
+      def within_job(jid, queue)
+        jobs_mutex.synchronize do
+          jobs_thread[jid] = Thread.current
+        end
+
+        if cancelled?(jid)
+          Sidekiq.logger.warn(
+            class: self.class.to_s,
+            action: 'run',
+            queue: queue,
+            jid: jid,
+            canceled: true
+          )
+          raise CancelledError
+        end
+
+        yield
+      ensure
+        jobs_mutex.synchronize do
+          jobs_thread.delete(jid)
+        end
+      end
+
+      def self.cancel_job(jid)
+        payload = {
+          action: 'cancel',
+          jid: jid
+        }.to_json
+
+        ::Gitlab::Redis::SharedState.with do |redis|
+          redis.setex(cancel_job_key(jid), CANCEL_DEADLINE, 1)
+          redis.publish(NOTIFICATION_CHANNEL, payload)
+        end
+      end
+
+      private
+
+      def start_working
+        Sidekiq.logger.info(
+          class: self.class.to_s,
+          action: 'start',
+          message: 'Starting Monitor Daemon'
+        )
+
+        while enabled?
+          process_messages
+          sleep(RECONNECT_TIME)
+        end
+
+      ensure
+        Sidekiq.logger.warn(
+          class: self.class.to_s,
+          action: 'stop',
+          message: 'Stopping Monitor Daemon'
+        )
+      end
+
+      def stop_working
+        thread.raise(Interrupt) if thread.alive?
+      end
+
+      def process_messages
+        ::Gitlab::Redis::SharedState.with do |redis|
+          redis.subscribe(NOTIFICATION_CHANNEL) do |on|
+            on.message do |channel, message|
+              process_message(message)
+            end
+          end
+        end
+      rescue Exception => e # rubocop:disable Lint/RescueException
+        Sidekiq.logger.warn(
+          class: self.class.to_s,
+          action: 'exception',
+          message: e.message
+        )
+
+        # we re-raise system exceptions
+        raise e unless e.is_a?(StandardError)
+      end
+
+      def process_message(message)
+        Sidekiq.logger.info(
+          class: self.class.to_s,
+          channel: NOTIFICATION_CHANNEL,
+          message: 'Received payload on channel',
+          payload: message
+        )
+
+        message = safe_parse(message)
+        return unless message
+
+        case message['action']
+        when 'cancel'
+          process_job_cancel(message['jid'])
+        else
+          # unknown message
+        end
+      end
+
+      def safe_parse(message)
+        JSON.parse(message)
+      rescue JSON::ParserError
+      end
+
+      def process_job_cancel(jid)
+        return unless jid
+
+        # try to find thread without lock
+        return unless find_thread_unsafe(jid)
+
+        Thread.new do
+          # try to find a thread, but with guaranteed
+          # that handle for thread corresponds to actually
+          # running job
+          find_thread_with_lock(jid) do |thread|
+            Sidekiq.logger.warn(
+              class: self.class.to_s,
+              action: 'cancel',
+              message: 'Canceling thread with CancelledError',
+              jid: jid,
+              thread_id: thread.object_id
+            )
+
+            thread&.raise(CancelledError)
+          end
+        end
+      end
+
+      # This method needs to be thread-safe
+      # This is why it passes thread in block,
+      # to ensure that we do process this thread
+      def find_thread_unsafe(jid)
+        jobs_thread[jid]
+      end
+
+      def find_thread_with_lock(jid)
+        # don't try to lock if we cannot find the thread
+        return unless find_thread_unsafe(jid)
+
+        jobs_mutex.synchronize do
+          find_thread_unsafe(jid).tap do |thread|
+            yield(thread) if thread
+          end
+        end
+      end
+
+      def cancelled?(jid)
+        ::Gitlab::Redis::SharedState.with do |redis|
+          redis.exists(self.class.cancel_job_key(jid))
+        end
+      end
+
+      def self.cancel_job_key(jid)
+        "sidekiq:cancel:#{jid}"
+      end
+    end
+  end
+end

lib/gitlab/sidekiq_middleware/monitor.rb

@@ -4,10 +4,10 @@ module Gitlab
   module SidekiqMiddleware
     class Monitor
       def call(worker, job, queue)
-        Gitlab::SidekiqMonitor.instance.within_job(job['jid'], queue) do
+        Gitlab::SidekiqDaemon::Monitor.instance.within_job(job['jid'], queue) do
           yield
         end
-      rescue Gitlab::SidekiqMonitor::CancelledError
+      rescue Gitlab::SidekiqDaemon::Monitor::CancelledError
         # push job to DeadSet
         payload = ::Sidekiq.dump_json(job)
         ::Sidekiq::DeadSet.new.kill(payload, notify_failure: false)

lib/gitlab/sidekiq_monitor.rb

-# frozen_string_literal: true
-
-module Gitlab
-  class SidekiqMonitor < Daemon
-    include ::Gitlab::Utils::StrongMemoize
-
-    NOTIFICATION_CHANNEL = 'sidekiq:cancel:notifications'
-    CANCEL_DEADLINE = 24.hours.seconds
-    RECONNECT_TIME = 3.seconds
-
-    # We use exception derived from `Exception`
-    # to consider this as an very low-level exception
-    # that should not be caught by application
-    CancelledError = Class.new(Exception) # rubocop:disable Lint/InheritException
-
-    attr_reader :jobs_thread
-    attr_reader :jobs_mutex
-
-    def initialize
-      super
-
-      @jobs_thread = {}
-      @jobs_mutex = Mutex.new
-    end
-
-    def within_job(jid, queue)
-      jobs_mutex.synchronize do
-        jobs_thread[jid] = Thread.current
-      end
-
-      if cancelled?(jid)
-        Sidekiq.logger.warn(
-          class: self.class.to_s,
-          action: 'run',
-          queue: queue,
-          jid: jid,
-          canceled: true
-        )
-        raise CancelledError
-      end
-
-      yield
-    ensure
-      jobs_mutex.synchronize do
-        jobs_thread.delete(jid)
-      end
-    end
-
-    def self.cancel_job(jid)
-      payload = {
-        action: 'cancel',
-        jid: jid
-      }.to_json
-
-      ::Gitlab::Redis::SharedState.with do |redis|
-        redis.setex(cancel_job_key(jid), CANCEL_DEADLINE, 1)
-        redis.publish(NOTIFICATION_CHANNEL, payload)
-      end
-    end
-
-    private
-
-    def start_working
-      Sidekiq.logger.info(
-        class: self.class.to_s,
-        action: 'start',
-        message: 'Starting Monitor Daemon'
-      )
-
-      while enabled?
-        process_messages
-        sleep(RECONNECT_TIME)
-      end
-
-    ensure
-      Sidekiq.logger.warn(
-        class: self.class.to_s,
-        action: 'stop',
-        message: 'Stopping Monitor Daemon'
-      )
-    end
-
-    def stop_working
-      thread.raise(Interrupt) if thread.alive?
-    end
-
-    def process_messages
-      ::Gitlab::Redis::SharedState.with do |redis|
-        redis.subscribe(NOTIFICATION_CHANNEL) do |on|
-          on.message do |channel, message|
-            process_message(message)
-          end
-        end
-      end
-    rescue Exception => e # rubocop:disable Lint/RescueException
-      Sidekiq.logger.warn(
-        class: self.class.to_s,
-        action: 'exception',
-        message: e.message
-      )
-
-      # we re-raise system exceptions
-      raise e unless e.is_a?(StandardError)
-    end
-
-    def process_message(message)
-      Sidekiq.logger.info(
-        class: self.class.to_s,
-        channel: NOTIFICATION_CHANNEL,
-        message: 'Received payload on channel',
-        payload: message
-      )
-
-      message = safe_parse(message)
-      return unless message
-
-      case message['action']
-      when 'cancel'
-        process_job_cancel(message['jid'])
-      else
-        # unknown message
-      end
-    end
-
-    def safe_parse(message)
-      JSON.parse(message)
-    rescue JSON::ParserError
-    end
-
-    def process_job_cancel(jid)
-      return unless jid
-
-      # try to find thread without lock
-      return unless find_thread_unsafe(jid)
-
-      Thread.new do
-        # try to find a thread, but with guaranteed
-        # that handle for thread corresponds to actually
-        # running job
-        find_thread_with_lock(jid) do |thread|
-          Sidekiq.logger.warn(
-            class: self.class.to_s,
-            action: 'cancel',
-            message: 'Canceling thread with CancelledError',
-            jid: jid,
-            thread_id: thread.object_id
-          )
-
-          thread&.raise(CancelledError)
-        end
-      end
-    end
-
-    # This method needs to be thread-safe
-    # This is why it passes thread in block,
-    # to ensure that we do process this thread
-    def find_thread_unsafe(jid)
-      jobs_thread[jid]
-    end
-
-    def find_thread_with_lock(jid)
-      # don't try to lock if we cannot find the thread
-      return unless find_thread_unsafe(jid)
-
-      jobs_mutex.synchronize do
-        find_thread_unsafe(jid).tap do |thread|
-          yield(thread) if thread
-        end
-      end
-    end
-
-    def cancelled?(jid)
-      ::Gitlab::Redis::SharedState.with do |redis|
-        redis.exists(self.class.cancel_job_key(jid))
-      end
-    end
-
-    def self.cancel_job_key(jid)
-      "sidekiq:cancel:#{jid}"
-    end
-  end
-end

rubocop/cop/scalability/file_uploads.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Scalability
+      # This cop checks for `File` params in API
+      #
+      # @example
+      #
+      #   # bad
+      #   params do
+      #     requires :file, type: File
+      #   end
+      #
+      #   params do
+      #     optional :file, type: File
+      #   end
+      #
+      #   # good
+      #   params do
+      #     requires :file, type: ::API::Validations::Types::WorkhorseFile
+      #   end
+      #
+      #   params do
+      #     optional :file, type: ::API::Validations::Types::WorkhorseFile
+      #   end
+      #
+      class FileUploads < RuboCop::Cop::Cop
+        MSG = 'Do not upload files without workhorse acceleration. Please refer to https://docs.gitlab.com/ee/development/uploads.html'
+
+        def_node_search :file_type_params?, <<~PATTERN
+          (send nil? {:requires :optional} (sym _) (hash <(pair (sym :type)(const nil? :File)) ...>))
+        PATTERN
+
+        def_node_search :file_types_params?, <<~PATTERN
+          (send nil? {:requires :optional} (sym _) (hash <(pair (sym :types)(array <(const nil? :File) ...>)) ...>))
+        PATTERN
+
+        def be_file_param_usage?(node)
+          file_type_params?(node) || file_types_params?(node)
+        end
+
+        def on_send(node)
+          return unless be_file_param_usage?(node)
+
+          add_offense(find_file_param(node), location: :expression)
+        end
+
+        private
+
+        def find_file_param(node)
+          node.each_descendant.find { |children| file_node_pattern.match(children) }
+        end
+
+        def file_node_pattern
+          @file_node_pattern ||= RuboCop::NodePattern.new("(const nil? :File)")
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -37,6 +37,7 @@ require_relative 'cop/rspec/factories_in_migration_specs'
 require_relative 'cop/rspec/top_level_describe_path'
 require_relative 'cop/qa/element_with_pattern'
 require_relative 'cop/sidekiq_options_queue'
+require_relative 'cop/scalability/file_uploads'
 require_relative 'cop/destroy_all'
 require_relative 'cop/ruby_interpolation_in_translation'
 require_relative 'code_reuse_helpers'

spec/lib/gitlab/sidekiq_monitor_spec.rb → spec/lib/gitlab/sidekiq_daemon/monitor_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::SidekiqMonitor do
+describe Gitlab::SidekiqDaemon::Monitor do
   let(:monitor) { described_class.new }
 
   describe '#within_job' do
@@ -43,7 +43,7 @@ describe Gitlab::SidekiqMonitor do
     before do
       # we want to run at most once cycle
       # we toggle `enabled?` flag after the first call
-      stub_const('Gitlab::SidekiqMonitor::RECONNECT_TIME', 0)
+      stub_const('Gitlab::SidekiqDaemon::Monitor::RECONNECT_TIME', 0)
       allow(monitor).to receive(:enabled?).and_return(true, false)
 
       allow(Sidekiq.logger).to receive(:info)

spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb

@@ -10,8 +10,8 @@ describe Gitlab::SidekiqMiddleware::Monitor do
     let(:job) { { 'jid' => 'job-id' } }
     let(:queue) { 'my-queue' }
 
-    it 'calls SidekiqMonitor' do
-      expect(Gitlab::SidekiqMonitor.instance).to receive(:within_job)
+    it 'calls Gitlab::SidekiqDaemon::Monitor' do
+      expect(Gitlab::SidekiqDaemon::Monitor.instance).to receive(:within_job)
         .with('job-id', 'my-queue')
         .and_call_original
 
@@ -29,7 +29,7 @@ describe Gitlab::SidekiqMiddleware::Monitor do
     context 'when cancel happens' do
       subject do
         monitor.call(worker, job, queue) do
-          raise Gitlab::SidekiqMonitor::CancelledError
+          raise Gitlab::SidekiqDaemon::Monitor::CancelledError
         end
       end
 

spec/rubocop/cop/scalability/file_uploads_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rubocop'
+require_relative '../../../support/helpers/expect_offense'
+require_relative '../../../../rubocop/cop/scalability/file_uploads'
+
+describe RuboCop::Cop::Scalability::FileUploads do
+  include CopHelper
+  include ExpectOffense
+
+  subject(:cop) { described_class.new }
+  let(:message) { 'Do not upload files without workhorse acceleration. Please refer to https://docs.gitlab.com/ee/development/uploads.html' }
+
+  context 'with required params' do
+    it 'detects File in types array' do
+      expect_offense(<<~PATTERN.strip_indent)
+      params do
+        requires :certificate, allow_blank: false, types: [String, File]
+                                                                   ^^^^ #{message}
+      end
+      PATTERN
+    end
+
+    it 'detects File as type argument' do
+      expect_offense(<<~PATTERN.strip_indent)
+      params do
+        requires :attachment, type: File
+                                    ^^^^ #{message}
+      end
+      PATTERN
+    end
+  end
+
+  context 'with optional params' do
+    it 'detects File in types array' do
+      expect_offense(<<~PATTERN.strip_indent)
+      params do
+        optional :certificate, allow_blank: false, types: [String, File]
+                                                                   ^^^^ #{message}
+      end
+      PATTERN
+    end
+
+    it 'detects File as type argument' do
+      expect_offense(<<~PATTERN.strip_indent)
+      params do
+        optional :attachment, type: File
+                                    ^^^^ #{message}
+      end
+      PATTERN
+    end
+  end
+end