From cfc8067c0865fe3eea46d6b29d12ddde15bad52b Mon Sep 17 00:00:00 2001
From: Luke Duncalfe <lduncalfe@eml.cc>
Date: Mon, 6 May 2024 13:15:58 +1200
Subject: [PATCH] Add cop to ban using GraphQL ID type for IIDs
This enforces some of the guidelines added around use of `ID` type
added in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/152002.
---
.../mutations/design_management/base.rb | 2 +-
.../resolvers/ci/project_pipeline_resolver.rb | 2 +-
app/graphql/resolvers/deployment_resolver.rb | 2 +-
app/graphql/types/milestone_type.rb | 2 +-
ee/app/graphql/mutations/epics/base.rb | 2 +-
ee/app/graphql/resolvers/epics_resolver.rb | 2 +-
.../graphql/resolvers/iterations_resolver.rb | 3 +-
.../requirements_resolver.rb | 2 +-
rubocop/cop/graphql/id_type.rb | 27 +++++++---
spec/rubocop/cop/graphql/id_type_spec.rb | 54 ++++++++++++++++++-
10 files changed, 82 insertions(+), 16 deletions(-)
diff --git a/app/graphql/mutations/design_management/base.rb b/app/graphql/mutations/design_management/base.rb
index 56889166f4425..8cda2e6db3c00 100644
--- a/app/graphql/mutations/design_management/base.rb
+++ b/app/graphql/mutations/design_management/base.rb
@@ -9,7 +9,7 @@ class Base < ::Mutations::BaseMutation
required: true,
description: "Project where the issue is to upload designs for."
- argument :iid, GraphQL::Types::ID,
+ argument :iid, GraphQL::Types::ID, # rubocop:disable Graphql/IDType -- Legacy argument using ID type kept for backwards compatibility
required: true,
description: "IID of the issue to modify designs for."
diff --git a/app/graphql/resolvers/ci/project_pipeline_resolver.rb b/app/graphql/resolvers/ci/project_pipeline_resolver.rb
index e60118a053e9b..6b50de3259e87 100644
--- a/app/graphql/resolvers/ci/project_pipeline_resolver.rb
+++ b/app/graphql/resolvers/ci/project_pipeline_resolver.rb
@@ -14,7 +14,7 @@ class ProjectPipelineResolver < BaseResolver
description: 'Global ID of the Pipeline. For example, "gid://gitlab/Ci::Pipeline/314".',
prepare: ->(pipeline_id, _ctx) { pipeline_id.model_id }
- argument :iid, GraphQL::Types::ID,
+ argument :iid, GraphQL::Types::ID, # rubocop:disable Graphql/IDType -- Legacy argument using ID type kept for backwards compatibility
required: false,
description: 'IID of the Pipeline. For example, "1".'
diff --git a/app/graphql/resolvers/deployment_resolver.rb b/app/graphql/resolvers/deployment_resolver.rb
index d0101c26c0da5..9798bc43c6832 100644
--- a/app/graphql/resolvers/deployment_resolver.rb
+++ b/app/graphql/resolvers/deployment_resolver.rb
@@ -2,7 +2,7 @@
module Resolvers
class DeploymentResolver < BaseResolver
- argument :iid,
+ argument :iid, # rubocop:disable Graphql/IDType -- Legacy argument using ID type kept for backwards compatibility
GraphQL::Types::ID,
required: true,
description: 'Project-level internal ID of the Deployment.'
diff --git a/app/graphql/types/milestone_type.rb b/app/graphql/types/milestone_type.rb
index e8b17a7470a48..36c7ee286e6a9 100644
--- a/app/graphql/types/milestone_type.rb
+++ b/app/graphql/types/milestone_type.rb
@@ -14,7 +14,7 @@ class MilestoneType < BaseObject
field :id, GraphQL::Types::ID, null: false,
description: 'ID of the milestone.'
- field :iid, GraphQL::Types::ID, null: false,
+ field :iid, GraphQL::Types::ID, null: false, # rubocop:disable Graphql/IDType -- Legacy argument using ID type kept for backwards compatibility
description: "Internal ID of the milestone."
field :title, GraphQL::Types::String, null: false,
diff --git a/ee/app/graphql/mutations/epics/base.rb b/ee/app/graphql/mutations/epics/base.rb
index 467e457c89d74..0a3dce4deb870 100644
--- a/ee/app/graphql/mutations/epics/base.rb
+++ b/ee/app/graphql/mutations/epics/base.rb
@@ -5,7 +5,7 @@ module Epics
class Base < ::Mutations::BaseMutation
include Mutations::ResolvesIssuable
- argument :iid, GraphQL::Types::ID,
+ argument :iid, GraphQL::Types::ID, # rubocop:disable Graphql/IDType -- Legacy argument using ID type kept for backwards compatibility
required: true,
description: "IID of the epic to mutate."
diff --git a/ee/app/graphql/resolvers/epics_resolver.rb b/ee/app/graphql/resolvers/epics_resolver.rb
index 8e871286fa3f0..ab48d012ecd4e 100644
--- a/ee/app/graphql/resolvers/epics_resolver.rb
+++ b/ee/app/graphql/resolvers/epics_resolver.rb
@@ -6,7 +6,7 @@ class EpicsResolver < BaseResolver
include SearchArguments
include LooksAhead
- argument :iid, GraphQL::Types::ID,
+ argument :iid, GraphQL::Types::ID, # rubocop:disable Graphql/IDType -- Legacy argument using ID type kept for backwards compatibility
required: false,
description: 'IID of the epic, e.g., "1".'
diff --git a/ee/app/graphql/resolvers/iterations_resolver.rb b/ee/app/graphql/resolvers/iterations_resolver.rb
index b52f72f858d29..c4e7a20105aa9 100644
--- a/ee/app/graphql/resolvers/iterations_resolver.rb
+++ b/ee/app/graphql/resolvers/iterations_resolver.rb
@@ -29,9 +29,10 @@ class IterationsResolver < BaseResolver
description: 'Global ID of the Iteration to look up.'
# rubocop:enable Graphql/IDType
- argument :iid, GraphQL::Types::ID,
+ argument :iid, GraphQL::Types::ID, # rubocop:disable Graphql/IDType -- Legacy argument using ID type kept for backwards compatibility
required: false,
description: 'Internal ID of the Iteration to look up.'
+
argument :include_ancestors, GraphQL::Types::Boolean,
required: false,
description: 'Whether to include ancestor iterations. Defaults to true.'
diff --git a/ee/app/graphql/resolvers/requirements_management/requirements_resolver.rb b/ee/app/graphql/resolvers/requirements_management/requirements_resolver.rb
index 288951ae14d5d..80fc959315f96 100644
--- a/ee/app/graphql/resolvers/requirements_management/requirements_resolver.rb
+++ b/ee/app/graphql/resolvers/requirements_management/requirements_resolver.rb
@@ -8,7 +8,7 @@ class RequirementsResolver < BaseResolver
type ::Types::RequirementsManagement::RequirementType.connection_type, null: true
- argument :iid, GraphQL::Types::ID,
+ argument :iid, GraphQL::Types::ID, # rubocop:disable Graphql/IDType -- Legacy argument using ID type kept for backwards compatibility
required: false,
deprecated: { reason: 'Use work_item_iid instead', milestone: '15.8' },
description: 'IID of the requirement, for example, "1".'
diff --git a/rubocop/cop/graphql/id_type.rb b/rubocop/cop/graphql/id_type.rb
index 430f69ff67c08..2a1cbdca6c8cf 100644
--- a/rubocop/cop/graphql/id_type.rb
+++ b/rubocop/cop/graphql/id_type.rb
@@ -4,27 +4,40 @@ module RuboCop
module Cop
module Graphql
class IDType < RuboCop::Cop::Base
- MSG = 'Do not use GraphQL::Types::ID, use a specific GlobalIDType instead'
+ MSG_USE_STRING_FOR_IID = 'Do not use GraphQL::Types::ID for IIDs, use GraphQL::Types::String instead'
+ MSG_USE_GLOBAL_ID = 'Do not use GraphQL::Types::ID, use a specific GlobalIDType instead'
ALLOWLISTED_ARGUMENTS = %i[
- iid full_path project_path group_path target_project_path target_group_path target_path namespace_path
+ full_path project_path group_path target_project_path target_group_path target_path namespace_path
context_namespace_path
].freeze
- def_node_search :graphql_id_type?, <<~PATTERN
+ def_node_matcher :iid_with_id?, <<~PATTERN
+ (send nil? {:field :argument}
+ (sym #iid?)
+ (const (const (const nil? :GraphQL) :Types) :ID)
+ (...)?)
+ PATTERN
+
+ def_node_search :graphql_id_allowed?, <<~PATTERN
(send nil? :argument (_ #does_not_match?) (const (const (const nil? :GraphQL) :Types) :ID) ...)
PATTERN
def on_send(node)
- return unless graphql_id_type?(node)
+ return add_offense(node, message: MSG_USE_STRING_FOR_IID) if iid_with_id?(node)
+ return unless graphql_id_allowed?(node)
- add_offense(node)
+ add_offense(node, message: MSG_USE_GLOBAL_ID)
end
private
- def does_not_match?(arg)
- !ALLOWLISTED_ARGUMENTS.include?(arg) # rubocop:disable Rails/NegateInclude
+ def does_not_match?(arg_name)
+ ALLOWLISTED_ARGUMENTS.exclude?(arg_name)
+ end
+
+ def iid?(arg_name)
+ arg_name == :iid || arg_name.end_with?('_iid')
end
end
end
diff --git a/spec/rubocop/cop/graphql/id_type_spec.rb b/spec/rubocop/cop/graphql/id_type_spec.rb
index 6eb4890c06447..4f35c0594bb30 100644
--- a/spec/rubocop/cop/graphql/id_type_spec.rb
+++ b/spec/rubocop/cop/graphql/id_type_spec.rb
@@ -4,7 +4,7 @@
require_relative '../../../../rubocop/cop/graphql/id_type'
-RSpec.describe RuboCop::Cop::Graphql::IDType do
+RSpec.describe RuboCop::Cop::Graphql::IDType, feature_category: :api do
it 'adds an offense when GraphQL::Types::ID is used as a param to #argument' do
expect_offense(<<~TYPE)
argument :some_arg, GraphQL::Types::ID, some: other, params: do_not_matter
@@ -27,4 +27,56 @@
argument :some_arg, ::Types::GlobalIDType[::Awardable], some: other, params: do_not_matter
TYPE
end
+
+ it 'adds an offense when GraphQL::Types::ID is used with an IID field' do
+ expect_offense(<<~TYPE)
+ field :iid, GraphQL::Types::ID, some: other, params: do_not_matter
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use GraphQL::Types::ID for IIDs, use GraphQL::Types::String instead
+ TYPE
+ end
+
+ it 'adds an offense when GraphQL::Types::ID is used with an IID-like field' do
+ expect_offense(<<~TYPE)
+ field :issue_iid, GraphQL::Types::ID, some: other, params: do_not_matter
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use GraphQL::Types::ID for IIDs, use GraphQL::Types::String instead
+ TYPE
+ end
+
+ it 'adds an offense when GraphQL::Types::ID is used with an IID argument' do
+ expect_offense(<<~TYPE)
+ argument :iid, GraphQL::Types::ID, some: other, params: do_not_matter
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use GraphQL::Types::ID for IIDs, use GraphQL::Types::String instead
+ TYPE
+ end
+
+ it 'adds an offense when GraphQL::Types::ID is used with an IID-like argument' do
+ expect_offense(<<~TYPE)
+ argument :merge_request_iid, GraphQL::Types::ID, some: other, params: do_not_matter
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use GraphQL::Types::ID for IIDs, use GraphQL::Types::String instead
+ TYPE
+ end
+
+ it 'does not add an offense when GraphQL::Types::String is used with an IID field' do
+ expect_no_offenses(<<~TYPE)
+ field :iid, GraphQL::Types::String, some: other, params: do_not_matter
+ TYPE
+ end
+
+ it 'does not add an offense when GraphQL::Types::String is used with an IID-like field' do
+ expect_no_offenses(<<~TYPE)
+ field :issue_iid, GraphQL::Types::String, some: other, params: do_not_matter
+ TYPE
+ end
+
+ it 'does not add an offense when GraphQL::Types::String is used with an IID argument' do
+ expect_no_offenses(<<~TYPE)
+ argument :iid, GraphQL::Types::String, some: other, params: do_not_matter
+ TYPE
+ end
+
+ it 'does not add an offense when GraphQL::Types::String is used with an IID-like argument' do
+ expect_no_offenses(<<~TYPE)
+ argument :merge_request_iid, GraphQL::Types::String, some: other, params: do_not_matter
+ TYPE
+ end
end
--
GitLab