summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAlessio Caiazza <acaiazza@gitlab.com>2019-09-10 16:24:10 +0000
committerRémy Coutable <remy@rymai.me>2019-09-10 16:24:10 +0000
commitdaf7810e2ee323e39e3cc0b1c6f3fe15a9977a14 (patch)
treead30eb329787116653940d22a95cad1e99ebb0ab
parent0078ea44c292cd0e5eb7f4ae52358087c8ee34db (diff)
downloadgitlab-ce-daf7810e2ee323e39e3cc0b1c6f3fe15a9977a14.tar.gz
Add Scalability/FileUploads cop
This cop prevents you from using file in API, it points you to the development documentation about workhorse file acceleration.
-rw-r--r--.rubocop.yml5
-rw-r--r--lib/api/helpers/projects_helpers.rb3
-rw-r--r--lib/api/pages_domains.rb6
-rw-r--r--lib/api/project_import.rb3
-rw-r--r--lib/api/projects.rb3
-rw-r--r--lib/api/users.rb3
-rw-r--r--rubocop/cop/scalability/file_uploads.rb61
-rw-r--r--rubocop/rubocop.rb1
-rw-r--r--spec/rubocop/cop/scalability/file_uploads_spec.rb54
9 files changed, 135 insertions, 4 deletions
diff --git a/.rubocop.yml b/.rubocop.yml
index f24cbb6ce92..73743ebf9a2 100644
--- a/.rubocop.yml
+++ b/.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'
diff --git a/lib/api/helpers/projects_helpers.rb b/lib/api/helpers/projects_helpers.rb
index 51b7cf05c8f..c1e7af33235 100644
--- a/lib/api/helpers/projects_helpers.rb
+++ b/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"
diff --git a/lib/api/pages_domains.rb b/lib/api/pages_domains.rb
index 4227a106a95..40b133e8959 100644
--- a/lib/api/pages_domains.rb
+++ b/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
diff --git a/lib/api/project_import.rb b/lib/api/project_import.rb
index bb1b037c08f..9b5e0727184 100644
--- a/lib/api/project_import.rb
+++ b/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,
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 3073c14b341..63bfa8db61c 100644
--- a/lib/api/projects.rb
+++ b/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
diff --git a/lib/api/users.rb b/lib/api/users.rb
index a4ac5b629b8..99295888c8c 100644
--- a/lib/api/users.rb
+++ b/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
diff --git a/rubocop/cop/scalability/file_uploads.rb b/rubocop/cop/scalability/file_uploads.rb
new file mode 100644
index 00000000000..83017217e32
--- /dev/null
+++ b/rubocop/cop/scalability/file_uploads.rb
@@ -0,0 +1,61 @@
+# 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
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index c342df6d6c9..9d97aa86bf6 100644
--- a/rubocop/rubocop.rb
+++ b/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'
diff --git a/spec/rubocop/cop/scalability/file_uploads_spec.rb b/spec/rubocop/cop/scalability/file_uploads_spec.rb
new file mode 100644
index 00000000000..2a94fde5ba2
--- /dev/null
+++ b/spec/rubocop/cop/scalability/file_uploads_spec.rb
@@ -0,0 +1,54 @@
+# 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