diff --git a/app/graphql/mutations/container_repositories/destroy.rb b/app/graphql/mutations/container_repositories/destroy.rb index 1d8f7b22f8822fdc517567276b1213e24e872522..2a45291be22d1ca57261cfa50fa0e3832b64d7fd 100644 --- a/app/graphql/mutations/container_repositories/destroy.rb +++ b/app/graphql/mutations/container_repositories/destroy.rb @@ -21,7 +21,9 @@ def resolve(id:) container_repository = authorized_find!(id: id) container_repository.delete_scheduled! + # rubocop:disable CodeReuse/Worker DeleteContainerRepositoryWorker.perform_async(current_user.id, container_repository.id) + # rubocop:enable CodeReuse/Worker track_event(:delete_repository, :container) { diff --git a/doc/development/reusing_abstractions.md b/doc/development/reusing_abstractions.md index f3eb1ebcc0c413e85fceffc55ec893bb4856bda1..87f123f3a571cd35f52e6159785722a7d7081894 100644 --- a/doc/development/reusing_abstractions.md +++ b/doc/development/reusing_abstractions.md @@ -109,7 +109,7 @@ the various abstractions and what they can (not) reuse: | Abstraction | Service classes | Finders | Presenters | Serializers | Model instance method | Model class methods | Active Record | Worker |:-----------------------|:-----------------|:---------|:------------|:--------------|:------------------------|:----------------------|:----------------|:-------- -| Controller | Yes | Yes | Yes | Yes | Yes | No | No | No +| Controller/API endpoint| Yes | Yes | Yes | Yes | Yes | No | No | No | Service class | Yes | Yes | No | No | Yes | No | No | Yes | Finder | No | No | No | No | Yes | Yes | No | No | Presenter | No | Yes | No | No | Yes | Yes | No | No @@ -125,9 +125,11 @@ Everything in `app/controllers`. Controllers should not do much work on their own, instead they simply pass input to other classes and present the results. -### Grape endpoint +### API endpoints -Everything in `lib/api`. +Everything in `lib/api` (the REST API) and `app/graphql` (the GraphQL API). + +API endpoints have the same abstraction level as controllers. ### Service classes diff --git a/ee/app/graphql/mutations/requirements_management/export_requirements.rb b/ee/app/graphql/mutations/requirements_management/export_requirements.rb index 780c93df49b185e053bc957bda91b0da17c1e9ee..34554396891ba22cf30ab01cc7a3111be18c762d 100644 --- a/ee/app/graphql/mutations/requirements_management/export_requirements.rb +++ b/ee/app/graphql/mutations/requirements_management/export_requirements.rb @@ -38,7 +38,9 @@ def resolve(args) project_path = args.delete(:project_path) project = authorized_find!(project_path) + # rubocop:disable CodeReuse/Worker IssuableExportCsvWorker.perform_async(:requirement, current_user.id, project.id, args) + # rubocop:enable CodeReuse/Worker { errors: [] diff --git a/rubocop/code_reuse_helpers.rb b/rubocop/code_reuse_helpers.rb index 45cfa7ba78d7c945fb336af53449d5f3a26747a1..2769da2389c82a014259478510650755ad3fe834 100644 --- a/rubocop/code_reuse_helpers.rb +++ b/rubocop/code_reuse_helpers.rb @@ -76,10 +76,15 @@ def in_controller?(node) in_app_directory?(node, 'controllers') end + # Returns true if the given node resides in app/graphql or ee/app/graphql. + def in_graphql?(node) + in_app_directory?(node, 'graphql') + end + # Returns true if the given node resides in app/graphql/types, # ee/app/graphql/types, or ee/app/graphql/ee/types. def in_graphql_types?(node) - in_app_directory?(node, 'graphql/types') || in_app_directory?(node, 'graphql/ee/types') + in_graphql_directory?(node, 'types') end # Returns true if the given node resides in lib/api or ee/lib/api. @@ -113,6 +118,13 @@ def in_lib_directory?(node, directory) ) end + # Returns true if the given node resides in app/graphql/{directory}, + # ee/app/graphql/{directory}, or ee/app/graphql/ee/{directory}. + def in_graphql_directory?(node, directory) + in_app_directory?(node, "graphql/#{directory}") || + in_app_directory?(node, "graphql/ee/#{directory}") + end + # Returns the receiver name of a send node. # # For the AST node `(send (const nil? :Foo) ...)` this would return diff --git a/rubocop/cop/code_reuse/worker.rb b/rubocop/cop/code_reuse/worker.rb index 4902920234f0a1044b9337329a00ccd2f3677b39..3a1120ac2a16ca4f94f43221e4e37937ec04c53b 100644 --- a/rubocop/cop/code_reuse/worker.rb +++ b/rubocop/cop/code_reuse/worker.rb @@ -10,7 +10,7 @@ class Worker < RuboCop::Cop::Cop include CodeReuseHelpers IN_CONTROLLER = 'Workers can not be used in a controller.' - IN_API = 'Workers can not be used in a Grape API.' + IN_API = 'Workers can not be used in an API endpoint.' IN_FINDER = 'Workers can not be used in a Finder.' IN_PRESENTER = 'Workers can not be used in a Presenter.' IN_SERIALIZER = 'Workers can not be used in a Serializer.' @@ -32,7 +32,7 @@ def check_all_send_nodes(node) message = if in_controller?(node) IN_CONTROLLER - elsif in_api?(node) + elsif in_api?(node) || in_graphql?(node) IN_API elsif in_finder?(node) IN_FINDER diff --git a/spec/rubocop/code_reuse_helpers_spec.rb b/spec/rubocop/code_reuse_helpers_spec.rb index d437ada85ee7ceb627ec0186429009f08b5501e7..0d06d37d67a743ef9dd7e1f1f1a0ea0c67dfeb8d 100644 --- a/spec/rubocop/code_reuse_helpers_spec.rb +++ b/spec/rubocop/code_reuse_helpers_spec.rb @@ -152,6 +152,26 @@ def build_and_parse_source(source, path = 'foo.rb') end end + describe '#in_graphql?' do + it 'returns true for a node in the FOSS GraphQL directory' do + node = build_and_parse_source('10', rails_root_join('app', 'graphql', 'foo.rb')) + + expect(cop.in_graphql?(node)).to eq(true) + end + + it 'returns true for a node in the EE GraphQL directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'foo.rb')) + + expect(cop.in_graphql?(node)).to eq(true) + end + + it 'returns false for a node outside the GraphQL directory' do + node = build_and_parse_source('10', rails_root_join('app', 'foo', 'foo.rb')) + + expect(cop.in_graphql?(node)).to eq(false) + end + end + describe '#in_graphql_types?' do %w[ app/graphql/types @@ -169,7 +189,7 @@ def build_and_parse_source(source, path = 'foo.rb') app/graphql/resolvers app/foo ].each do |path| - it "returns true for a node in #{path}" do + it "returns false for a node in #{path}" do node = build_and_parse_source('10', rails_root_join(path, 'foo.rb')) expect(cop.in_graphql_types?(node)).to eq(false) @@ -255,6 +275,44 @@ def build_and_parse_source(source, path = 'foo.rb') end end + describe '#in_graphql_directory?' do + it 'returns true for a directory in the FOSS app/graphql directory' do + node = build_and_parse_source('10', rails_root_join('app', 'graphql', 'subdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(true) + end + + it 'returns true for a directory in the EE app/graphql directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'subdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(true) + end + + it 'returns true for a directory in the EE app/graphql/ee directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'ee', 'subdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(true) + end + + it 'returns false for a directory in the FOSS app/graphql directory' do + node = build_and_parse_source('10', rails_root_join('app', 'graphql', 'anotherdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(false) + end + + it 'returns false for a directory in the EE app/graphql directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'anotherdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(false) + end + + it 'returns false for a directory in the EE app/graphql/ee directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'ee', 'anotherdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(false) + end + end + describe '#name_of_receiver' do it 'returns the name of a send receiver' do node = build_and_parse_source('Foo.bar') diff --git a/spec/rubocop/cop/code_reuse/worker_spec.rb b/spec/rubocop/cop/code_reuse/worker_spec.rb index 8155791a3e374f0b8f894a09ce48d316e0507c83..a548e90d8e1b65c8c326a4e976688dd894fee84b 100644 --- a/spec/rubocop/cop/code_reuse/worker_spec.rb +++ b/spec/rubocop/cop/code_reuse/worker_spec.rb @@ -31,7 +31,24 @@ class Foo < Grape::API::Instance resource :projects do get '/' do FooWorker.perform_async - ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in a Grape API. + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in an API endpoint. + end + end + end + SOURCE + end + + it 'flags the use of a worker in GraphQL' do + allow(cop) + .to receive(:in_graphql?) + .and_return(true) + + expect_offense(<<~SOURCE) + module Mutations + class Foo < BaseMutation + def resolve + FooWorker.perform_async + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in an API endpoint. end end end