Merge branch 'ajk-globalid-compatibilty' into 'master'

[GraphQL] GlobalID / ID compatibility layer

See merge request gitlab-org/gitlab!36209

app/graphql/mutations/base_mutation.rb

@@ -4,6 +4,7 @@ module Mutations
   class BaseMutation < GraphQL::Schema::RelayClassicMutation
     prepend Gitlab::Graphql::Authorize::AuthorizeResource
     prepend Gitlab::Graphql::CopyFieldDescription
+    prepend ::Gitlab::Graphql::GlobalIDCompatibility
 
     ERROR_MESSAGE = 'You cannot perform write operations on a read-only instance'
 

app/graphql/mutations/ci/base.rb

@@ -3,13 +3,18 @@
 module Mutations
   module Ci
     class Base < BaseMutation
-      argument :id, ::Types::GlobalIDType[::Ci::Pipeline],
+      PipelineID = ::Types::GlobalIDType[::Ci::Pipeline]
+
+      argument :id, PipelineID,
                 required: true,
                 description: 'The id of the pipeline to mutate'
 
       private
 
       def find_object(id:)
+        # TODO: remove this line when the compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        id = PipelineID.coerce_isolated_input(id)
         GlobalID::Locator.locate(id)
       end
     end

app/graphql/mutations/design_management/move.rb

@@ -29,11 +29,18 @@ module Mutations
       private
 
       def parameters(**args)
-        args.transform_values { |id| GitlabSchema.find_by_gid(id) }.transform_values(&:sync).tap do |hash|
+        args.transform_values { |id| find_design(id) }.transform_values(&:sync).tap do |hash|
           hash.each { |k, design| not_found(args[k]) unless current_user.can?(:read_design, design) }
         end
       end
 
+      def find_design(id)
+        # TODO: remove this line when the compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        id = DesignID.coerce_isolated_input(id)
+        GitlabSchema.object_from_id(id)
+      end
+
       def not_found(gid)
         raise Gitlab::Graphql::Errors::ResourceNotAvailable, "Resource not available: #{gid}"
       end

app/graphql/resolvers/base_resolver.rb

@@ -4,6 +4,7 @@ module Resolvers
   class BaseResolver < GraphQL::Schema::Resolver
     extend ::Gitlab::Utils::Override
     include ::Gitlab::Utils::StrongMemoize
+    include ::Gitlab::Graphql::GlobalIDCompatibility
 
     def self.single
       @single ||= Class.new(self) do

app/graphql/types/global_id_type.rb

 # frozen_string_literal: true
 
+module GraphQLExtensions
+  module ScalarExtensions
+    # Allow ID to unify with GlobalID Types
+    def ==(other)
+      if name == 'ID' && other.is_a?(self.class) &&
+          other.type_class.ancestors.include?(::Types::GlobalIDType)
+        return true
+      end
+
+      super
+    end
+  end
+end
+
+::GraphQL::ScalarType.prepend(GraphQLExtensions::ScalarExtensions)
+
 module Types
   class GlobalIDType < BaseScalar
     graphql_name 'GlobalID'

app/graphql/types/query_type.rb

@@ -49,8 +49,7 @@ module Types
 
     field :milestone, ::Types::MilestoneType,
           null: true,
-          description: 'Find a milestone',
-          resolve: -> (_obj, args, _ctx) { GitlabSchema.find_by_gid(args[:id]) } do
+          description: 'Find a milestone' do
       argument :id, ::Types::GlobalIDType[Milestone],
                required: true,
                description: 'Find a milestone by its ID'
@@ -86,7 +85,17 @@ module Types
     end
 
     def issue(id:)
-      GitlabSchema.object_from_id(id, expected_type: ::Issue)
+      # TODO: remove this line when the compatibility layer is removed
+      # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+      id = ::Types::GlobalIDType[::Issue].coerce_isolated_input(id)
+      GitlabSchema.find_by_gid(id)
+    end
+
+    def milestone(id:)
+      # TODO: remove this line when the compatibility layer is removed
+      # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+      id = ::Types::GlobalIDType[Milestone].coerce_isolated_input(id)
+      GitlabSchema.find_by_gid(id)
     end
   end
 end

doc/development/api_graphql_styleguide.md

@@ -49,6 +49,20 @@ See also:
 - [Exposing Global IDs](#exposing-global-ids).
 - [Mutation arguments](#object-identifier-arguments).
 
+We have a custom scalar type (`Types::GlobalIDType`) which should be used as the
+type of input and output arguments when the value is a `GlobalID`. The benefits
+of using this type instead of `ID` are:
+
+- it validates that the value is a `GlobalID`
+- it parses it into a `GlobalID` before passing it to user code
+- it can be parameterized on the type of the object (e.g.
+  `GlobalIDType[Project]`) which offers even better validation and security.
+  
+Consider using this type for all new arguments and result types. Remember that
+it is perfectly possible to parameterize this type with a concern or a
+supertype, if you want to accept a wider range of objects (e.g.
+`GlobalIDType[Issuable]` vs `GlobalIDType[Issue]`).
+
 ## Types
 
 We use a code-first schema, and we declare what type everything is in Ruby.

ee/app/graphql/ee/mutations/boards/issues/issue_move_list.rb

@@ -8,8 +8,10 @@ module EE
           extend ActiveSupport::Concern
           extend ::Gitlab::Utils::Override
 
+          EpicID = ::Types::GlobalIDType[::Epic]
+
           prepended do
-            argument :epic_id, ::Types::GlobalIDType[::Epic],
+            argument :epic_id, EpicID,
                       required: false,
                       description: 'The ID of the parent epic. NULL when removing the association'
           end
@@ -28,10 +30,12 @@ module EE
 
           override :move_arguments
           def move_arguments(args)
-            allowed_args = super
-            allowed_args[:epic_id] = args[:epic_id]&.model_id if args.has_key?(:epic_id)
+            # TODO: remove this line once the compatibility layer is removed
+            # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+            coerce_global_id_arguments!(args)
+            epic_arguments = args.slice(:epic_id).transform_values { |id| id&.model_id }
 
-            allowed_args
+            super.merge!(epic_arguments)
           end
         end
       end

ee/app/graphql/ee/types/project_type.rb

@@ -87,7 +87,10 @@ module EE
               ::Types::DastSiteProfileType,
               null: true,
               resolve: -> (obj, args, _ctx) do
-                DastSiteProfilesFinder.new(project_id: obj.id, id: args[:id].model_id).execute.first
+                # TODO: remove this coercion when the compatibility layer is removed
+                # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+                gid = ::Types::GlobalIDType[::DastSiteProfile].coerce_isolated_input(args[:id])
+                DastSiteProfilesFinder.new(project_id: obj.id, id: gid.model_id).execute.first
               end,
               description: 'DAST Site Profile associated with the project' do
                 argument :id, ::Types::GlobalIDType[::DastSiteProfile], required: true, description: 'ID of the site profile'

ee/app/graphql/ee/types/query_type.rb

@@ -8,7 +8,6 @@ module EE
       prepended do
         field :iteration, ::Types::IterationType,
               null: true,
-              resolve: -> (_obj, args, _ctx) { ::GitlabSchema.find_by_gid(args[:id]) },
               description: 'Find an iteration' do
           argument :id, ::Types::GlobalIDType[::Iteration],
                    required: true,
@@ -24,8 +23,7 @@ module EE
         field :vulnerability,
               ::Types::VulnerabilityType,
               null: true,
-              description: "Find a vulnerability",
-              resolve: -> (_obj, args, _ctx) { ::GitlabSchema.find_by_gid(args[:id]) } do
+              description: "Find a vulnerability" do
           argument :id, ::Types::GlobalIDType[::Vulnerability],
                    required: true,
                    description: 'The Global ID of the Vulnerability'
@@ -54,6 +52,20 @@ module EE
               resolver: ::Resolvers::InstanceSecurityDashboardResolver,
               description: 'Fields related to Instance Security Dashboard'
       end
+
+      def vulnerability(id:)
+        # TODO: remove this line when the compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        id = ::Types::GlobalIDType[::Vulnerability].coerce_isolated_input(id)
+        ::GitlabSchema.find_by_gid(id)
+      end
+
+      def iteration(id:)
+        # TODO: remove this line when the compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        id = ::Types::GlobalIDType[Iteration].coerce_isolated_input(id)
+        ::GitlabSchema.find_by_gid(id)
+      end
     end
   end
 end

ee/app/graphql/mutations/clusters/agent_tokens/create.rb

@@ -8,8 +8,9 @@ module Mutations
 
         authorize :create_cluster
 
-        argument :cluster_agent_id,
-                 ::Types::GlobalIDType[::Clusters::Agent],
+        ClusterAgentID = ::Types::GlobalIDType[::Clusters::Agent]
+
+        argument :cluster_agent_id, ClusterAgentID,
                  required: true,
                  description: 'Global ID of the cluster agent that will be associated with the new token'
 
@@ -42,6 +43,9 @@ module Mutations
         private
 
         def find_object(id:)
+          # TODO: remove this line when the compatibility layer is removed
+          # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+          id = ClusterAgentID.coerce_isolated_input(id)
           GitlabSchema.find_by_gid(id)
         end
       end

ee/app/graphql/mutations/clusters/agent_tokens/delete.rb

@@ -8,8 +8,9 @@ module Mutations
 
         authorize :admin_cluster
 
-        argument :id,
-                 ::Types::GlobalIDType[::Clusters::AgentToken],
+        TokenID = ::Types::GlobalIDType[::Clusters::AgentToken]
+
+        argument :id, TokenID,
                  required: true,
                  description: 'Global ID of the cluster agent token that will be deleted'
 
@@ -23,6 +24,9 @@ module Mutations
         private
 
         def find_object(id:)
+          # TODO: remove this line when the compatibility layer is removed
+          # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+          id = TokenID.coerce_isolated_input(id)
           GitlabSchema.find_by_gid(id)
         end
       end

ee/app/graphql/mutations/clusters/agents/delete.rb

@@ -8,8 +8,9 @@ module Mutations
 
         authorize :admin_cluster
 
-        argument :id,
-                 ::Types::GlobalIDType[::Clusters::Agent],
+        AgentID = ::Types::GlobalIDType[::Clusters::Agent]
+
+        argument :id, AgentID,
                  required: true,
                  description: 'Global id of the cluster agent that will be deleted'
 
@@ -27,6 +28,9 @@ module Mutations
         private
 
         def find_object(id:)
+          # TODO: remove this line when the compatibility layer is removed
+          # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+          id = AgentID.coerce_isolated_input(id)
           GitlabSchema.find_by_gid(id)
         end
       end

ee/app/graphql/mutations/dast_on_demand_scans/create.rb

@@ -30,6 +30,10 @@ module Mutations
       def resolve(full_path:, dast_site_profile_id:, **args)
         project = authorized_find_project!(full_path: full_path)
 
+        # TODO: remove explicit coercion once compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        dast_site_profile_id = ::Types::GlobalIDType[::DastSiteProfile].coerce_isolated_input(dast_site_profile_id)
+
         dast_site_profile = find_dast_site_profile(project: project, dast_site_profile_id: dast_site_profile_id)
         dast_site = dast_site_profile.dast_site
         dast_scanner_profile = find_dast_scanner_profile(project: project, dast_scanner_profile_id: args[:dast_scanner_profile_id])
@@ -63,6 +67,11 @@ module Mutations
       def find_dast_scanner_profile(project:, dast_scanner_profile_id:)
         return unless dast_scanner_profile_id
 
+        # TODO: remove explicit coercion once compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        dast_scanner_profile_id = ::Types::GlobalIDType[::DastScannerProfile]
+          .coerce_isolated_input(dast_scanner_profile_id)
+
         project
           .dast_scanner_profiles
           .find(dast_scanner_profile_id.model_id)

ee/app/graphql/mutations/dast_scanner_profiles/delete.rb

@@ -7,17 +7,23 @@ module Mutations
 
       graphql_name 'DastScannerProfileDelete'
 
+      ScannerProfileID = ::Types::GlobalIDType[::DastScannerProfile]
+
       argument :full_path, GraphQL::ID_TYPE,
                 required: true,
                 description: 'Full path for the project the scanner profile belongs to.'
 
-      argument :id, ::Types::GlobalIDType[::DastScannerProfile],
+      argument :id, ScannerProfileID,
                 required: true,
                 description: 'ID of the scanner profile to be deleted.'
 
       authorize :create_on_demand_dast_scan
 
       def resolve(full_path:, id:)
+        # TODO: remove this line once the compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        id = ScannerProfileID.coerce_isolated_input(id)
+
         project = authorized_find_project!(full_path: full_path)
 
         service = ::DastScannerProfiles::DestroyService.new(project, current_user)

ee/app/graphql/mutations/dast_scanner_profiles/update.rb

@@ -34,10 +34,14 @@ module Mutations
       authorize :create_on_demand_dast_scan
 
       def resolve(full_path:, **service_args)
+        # TODO: remove this explicit coercion once the compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        gid = ::Types::GlobalIDType[::DastScannerProfile].coerce_isolated_input(service_args[:id])
+
         project = authorized_find!(full_path: full_path)
 
         service = ::DastScannerProfiles::UpdateService.new(project, current_user)
-        result = service.execute({ **service_args, id: service_args[:id].model_id })
+        result = service.execute({ **service_args, id: gid.model_id })
 
         if result.success?
           { id: result.payload.to_global_id, errors: [] }

ee/app/graphql/mutations/dast_site_profiles/delete.rb

@@ -19,6 +19,10 @@ module Mutations
 
       def resolve(full_path:, id:)
         project = authorized_find_project!(full_path: full_path)
+        # TODO: remove explicit coercion once compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        id = ::Types::GlobalIDType[::DastSiteProfile].coerce_isolated_input(id)
+
         dast_site_profile = find_dast_site_profile(project: project, global_id: id)
 
         return { errors: dast_site_profile.errors.full_messages } unless dast_site_profile.destroy

ee/app/graphql/mutations/dast_site_profiles/update.rb

@@ -29,7 +29,10 @@ module Mutations
 
       authorize :create_on_demand_dast_scan
 
-      def resolve(full_path:, **service_args)
+      def resolve(full_path:, id:, **service_args)
+        # TODO: remove explicit coercion once compatibility layer has been removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        service_args[:id] = ::Types::GlobalIDType[::DastSiteProfile].coerce_isolated_input(id).model_id
         project = authorized_find_project!(full_path: full_path)
 
         service = ::DastSiteProfiles::UpdateService.new(project, current_user)

ee/app/graphql/mutations/vulnerabilities/confirm.rb

@@ -33,6 +33,9 @@ module Mutations
       end
 
       def find_object(id:)
+        # TODO: remove this line once the compatibility layer is removed.
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        id = ::Types::GlobalIDType[::Vulnerability].coerce_isolated_input(id)
         GitlabSchema.find_by_gid(id)
       end
     end

ee/app/graphql/mutations/vulnerabilities/resolve.rb

@@ -33,6 +33,9 @@ module Mutations
       end
 
       def find_object(id:)
+        # TODO: remove this line once the compatibility layer is removed
+        # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+        id = ::Types::GlobalIDType[::Vulnerability].coerce_isolated_input(id)
         GitlabSchema.find_by_gid(id)
       end
     end

ee/app/services/dast_site_profiles/update_service.rb

@@ -30,7 +30,7 @@ module DastSiteProfiles
 
     # rubocop: disable CodeReuse/ActiveRecord
     def find_dast_site_profile!(id)
-      DastSiteProfilesFinder.new(project_id: project.id, id: id.model_id).execute.first!
+      DastSiteProfilesFinder.new(project_id: project.id, id: id).execute.first!
     end
     # rubocop: enable CodeReuse/ActiveRecord
   end

ee/spec/graphql/mutations/clusters/agent_tokens/create_spec.rb

@@ -52,7 +52,7 @@ RSpec.describe Mutations::Clusters::AgentTokens::Create do
         subject { mutation.resolve(cluster_agent_id: cluster_agent.id) }
 
         it 'generates an error message when id invalid', :aggregate_failures do
-          expect { subject }.to raise_error(NoMethodError)
+          expect { subject }.to raise_error(::GraphQL::CoercionError)
         end
       end
     end

ee/spec/graphql/mutations/clusters/agent_tokens/delete_spec.rb

@@ -44,7 +44,7 @@ RSpec.describe Mutations::Clusters::AgentTokens::Delete do
       let(:global_id) { token.id }
 
       it 'raises an error if the cluster agent id is invalid', :aggregate_failures do
-        expect { subject }.to raise_error(NoMethodError)
+        expect { subject }.to raise_error(::GraphQL::CoercionError)
         expect { token.reload }.not_to raise_error
       end
     end

ee/spec/graphql/mutations/clusters/agents/delete_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe Mutations::Clusters::Agents::Delete do
     context 'without user permissions' do
       it 'fails to delete the cluster agent', :aggregate_failures do
         expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
-        expect { cluster_agent.reload }.not_to raise_error(ActiveRecord::RecordNotFound)
+        expect { cluster_agent.reload }.not_to raise_error
       end
     end
 
@@ -43,8 +43,8 @@ RSpec.describe Mutations::Clusters::Agents::Delete do
       subject { mutation.resolve(id: cluster_agent.id) }
 
       it 'raises an error if the cluster agent id is invalid', :aggregate_failures do
-        expect { subject }.to raise_error(NoMethodError)
-        expect { cluster_agent.reload }.not_to raise_error(ActiveRecord::RecordNotFound)
+        expect { subject }.to raise_error(::GraphQL::CoercionError)
+        expect { cluster_agent.reload }.not_to raise_error
       end
     end
   end

ee/spec/services/dast_site_profiles/update_service_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe DastSiteProfiles::UpdateService do
   describe '#execute' do
     subject do
       described_class.new(project, user).execute(
-        id: dast_site_profile.to_global_id,
+        id: dast_site_profile.id,
         profile_name: new_profile_name,
         target_url: new_target_url
       )

lib/gitlab/graphql/global_id_compatibility.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module GlobalIDCompatibility
+      # TODO: remove this module once the compatibility layer is no longer needed.
+      # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+      def coerce_global_id_arguments!(args)
+        global_id_arguments = self.class.arguments.values.select do |arg|
+          arg.type.is_a?(Class) && arg.type <= ::Types::GlobalIDType
+        end
+
+        global_id_arguments.each do |arg|
+          k = arg.keyword
+          args[k] &&= arg.type.coerce_isolated_input(args[k])
+        end
+      end
+    end
+  end
+end

spec/graphql/types/global_id_type_spec.rb

@@ -99,8 +99,6 @@ RSpec.describe Types::GlobalIDType do
   end
 
   describe 'compatibility' do
-    # Simplified schema to test compatibility
-
     def query(doc, vars)
       GraphQL::Query.new(schema, document: doc, context: {}, variables: vars)
     end
@@ -112,6 +110,7 @@ RSpec.describe Types::GlobalIDType do
     all_types = [::GraphQL::ID_TYPE, ::Types::GlobalIDType, ::Types::GlobalIDType[::Project]]
 
     shared_examples 'a working query' do
+      # Simplified schema to test compatibility
       let!(:schema) do
         # capture values so they can be closed over
         arg_type = argument_type
@@ -135,10 +134,21 @@ RSpec.describe Types::GlobalIDType do
               argument :id, arg_type, required: true
             end
 
+            # This is needed so that all types are always registered as input types
+            field :echo, String, null: true do
+              argument :id, ::GraphQL::ID_TYPE, required: false
+              argument :gid, ::Types::GlobalIDType, required: false
+              argument :pid, ::Types::GlobalIDType[::Project], required: false
+            end
+
             def project_by_id(id:)
               gid = ::Types::GlobalIDType[::Project].coerce_isolated_input(id)
               gid.model_class.find(gid.model_id)
             end
+
+            def echo(id: nil, gid: nil, pid: nil)
+              "id: #{id}, gid: #{gid}, pid: #{pid}"
+            end
           end)
         end
       end
@@ -152,7 +162,7 @@ RSpec.describe Types::GlobalIDType do
       end
     end
 
-    context 'when the argument is declared as ID' do
+    context 'when the client declares the argument as ID the actual argument can be any type' do
       let(:document) do
         <<-GRAPHQL
         query($projectId: ID!){
@@ -163,16 +173,16 @@ RSpec.describe Types::GlobalIDType do
         GRAPHQL
       end
 
-      let(:argument_type) { ::GraphQL::ID_TYPE }
-
-      where(:result_type) { all_types }
+      where(:result_type, :argument_type) do
+        all_types.flat_map { |arg_type| all_types.zip([arg_type].cycle) }
+      end
 
       with_them do
         it_behaves_like 'a working query'
       end
     end
 
-    context 'when the argument is declared as GlobalID' do
+    context 'when the client passes the argument as GlobalID' do
       let(:document) do
         <<-GRAPHQL
         query($projectId: GlobalID!) {
@@ -192,7 +202,7 @@ RSpec.describe Types::GlobalIDType do
       end
     end
 
-    context 'when the argument is declared as ProjectID' do
+    context 'when the client passes the argument as ProjectID' do
       let(:document) do
         <<-GRAPHQL
         query($projectId: ProjectID!) {