From c860ddfc17fbd96287b2dcd32eca4ee76589e07d Mon Sep 17 00:00:00 2001
From: Luke Duncalfe <lduncalfe@gitlab.com>
Date: Fri, 5 Aug 2022 00:59:38 +0000
Subject: [PATCH] Update CodeReuse/Worker to cover GraphQL mutations
Mutations are considered the same abstraction level as the REST API.
---
.../container_repositories/destroy.rb | 2 +
doc/development/reusing_abstractions.md | 8 ++-
.../export_requirements.rb | 2 +
rubocop/code_reuse_helpers.rb | 14 ++++-
rubocop/cop/code_reuse/worker.rb | 4 +-
spec/rubocop/code_reuse_helpers_spec.rb | 60 ++++++++++++++++++-
spec/rubocop/cop/code_reuse/worker_spec.rb | 19 +++++-
7 files changed, 101 insertions(+), 8 deletions(-)
diff --git a/app/graphql/mutations/container_repositories/destroy.rb b/app/graphql/mutations/container_repositories/destroy.rb
index 1d8f7b22f8822..2a45291be22d1 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 f3eb1ebcc0c41..87f123f3a571c 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 780c93df49b18..34554396891ba 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 45cfa7ba78d7c..2769da2389c82 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 4902920234f0a..3a1120ac2a16c 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 d437ada85ee7c..0d06d37d67a74 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 8155791a3e374..a548e90d8e1b6 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
--
GitLab