From daf7810e2ee323e39e3cc0b1c6f3fe15a9977a14 Mon Sep 17 00:00:00 2001 From: Alessio Caiazza <acaiazza@gitlab.com> Date: Tue, 10 Sep 2019 16:24:10 +0000 Subject: [PATCH] Add Scalability/FileUploads cop This cop prevents you from using file in API, it points you to the development documentation about workhorse file acceleration. --- .rubocop.yml | 5 ++ lib/api/helpers/projects_helpers.rb | 3 +- lib/api/pages_domains.rb | 6 ++ lib/api/project_import.rb | 3 +- lib/api/projects.rb | 3 +- lib/api/users.rb | 3 +- rubocop/cop/scalability/file_uploads.rb | 61 +++++++++++++++++++ rubocop/rubocop.rb | 1 + .../cop/scalability/file_uploads_spec.rb | 54 ++++++++++++++++ 9 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 rubocop/cop/scalability/file_uploads.rb create mode 100644 spec/rubocop/cop/scalability/file_uploads_spec.rb 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 -- 2.30.9