Commit daf7810e authored by Alessio Caiazza's avatar Alessio Caiazza Committed by Rémy Coutable

Add Scalability/FileUploads cop

This cop prevents you from using file in API, it points you to the
development documentation about workhorse file acceleration.
parent 0078ea44
...@@ -275,3 +275,8 @@ RSpec/BeSuccessMatcher: ...@@ -275,3 +275,8 @@ RSpec/BeSuccessMatcher:
- 'ee/spec/support/shared_examples/controllers/**/*' - 'ee/spec/support/shared_examples/controllers/**/*'
- 'spec/support/controllers/**/*' - 'spec/support/controllers/**/*'
- 'ee/spec/support/controllers/**/*' - 'ee/spec/support/controllers/**/*'
Scalability/FileUploads:
Enabled: true
Include:
- 'lib/api/**/*.rb'
- 'ee/lib/api/**/*.rb'
...@@ -38,7 +38,8 @@ module API ...@@ -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_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 :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 :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 :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 :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" optional :initialize_with_readme, type: Boolean, desc: "Initialize a project with a README.md"
......
...@@ -90,8 +90,11 @@ module API ...@@ -90,8 +90,11 @@ module API
end end
params do params do
requires :domain, type: String, desc: 'The domain' 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 :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 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 all_or_none_of :user_provided_certificate, :user_provided_key
end end
post ":id/pages/domains" do post ":id/pages/domains" do
...@@ -111,8 +114,11 @@ module API ...@@ -111,8 +114,11 @@ module API
desc 'Updates a pages domain' desc 'Updates a pages domain'
params do params do
requires :domain, type: String, desc: 'The domain' 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 :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 optional :key, allow_blank: false, types: [File, String], desc: 'The key', as: :user_provided_key
# rubocop:enable Scalability/FileUploads
end end
put ":id/pages/domains/:domain", requirements: PAGES_DOMAINS_ENDPOINT_REQUIREMENTS do put ":id/pages/domains/:domain", requirements: PAGES_DOMAINS_ENDPOINT_REQUIREMENTS do
authorize! :update_pages, user_project authorize! :update_pages, user_project
......
...@@ -27,7 +27,8 @@ module API ...@@ -27,7 +27,8 @@ module API
resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
params do params do
requires :path, type: String, desc: 'The new project path and name' 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 :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 :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, optional :override_params,
......
...@@ -478,7 +478,8 @@ module API ...@@ -478,7 +478,8 @@ module API
desc 'Upload a file' desc 'Upload a file'
params do 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 end
post ":id/uploads" do post ":id/uploads" do
UploadService.new(user_project, params[:file]).execute.to_h UploadService.new(user_project, params[:file]).execute.to_h
......
...@@ -50,7 +50,8 @@ module API ...@@ -50,7 +50,8 @@ module API
optional :admin, type: Boolean, desc: 'Flag indicating the user is an administrator' 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 :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 :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' optional :private_profile, type: Boolean, default: false, desc: 'Flag indicating the user has a private profile'
all_or_none_of :extern_uid, :provider all_or_none_of :extern_uid, :provider
......
# 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
...@@ -37,6 +37,7 @@ require_relative 'cop/rspec/factories_in_migration_specs' ...@@ -37,6 +37,7 @@ require_relative 'cop/rspec/factories_in_migration_specs'
require_relative 'cop/rspec/top_level_describe_path' require_relative 'cop/rspec/top_level_describe_path'
require_relative 'cop/qa/element_with_pattern' require_relative 'cop/qa/element_with_pattern'
require_relative 'cop/sidekiq_options_queue' require_relative 'cop/sidekiq_options_queue'
require_relative 'cop/scalability/file_uploads'
require_relative 'cop/destroy_all' require_relative 'cop/destroy_all'
require_relative 'cop/ruby_interpolation_in_translation' require_relative 'cop/ruby_interpolation_in_translation'
require_relative 'code_reuse_helpers' require_relative 'code_reuse_helpers'
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment