Merge branch 'bvl-use-global-ids-graphql' into 'master'

Use global IDs when exposing GraphQL resources

Closes #62650

See merge request gitlab-org/gitlab-ce!29080

16 files changed, 135 insertions, 16 deletions

diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb
index f8ad6bee21b..2e5bdbd79c8 100644
--- a/app/graphql/gitlab_schema.rb
+++ b/app/graphql/gitlab_schema.rb
@@ -45,6 +45,31 @@ class GitlabSchema < GraphQL::Schema
       super(query_str, **kwargs)
     end
 
+    def id_from_object(object)
+      unless object.respond_to?(:to_global_id)
+        # This is an error in our schema and needs to be solved. So raise a
+        # more meaningfull error message
+        raise "#{object} does not implement `to_global_id`. "\
+              "Include `GlobalID::Identification` into `#{object.class}"
+      end
+
+      object.to_global_id
+    end
+
+    def object_from_id(global_id)
+      gid = GlobalID.parse(global_id)
+
+      unless gid
+        raise Gitlab::Graphql::Errors::ArgumentError, "#{global_id} is not a valid GitLab id."
+      end
+
+      if gid.model_class < ApplicationRecord
+        Gitlab::Graphql::Loaders::BatchModelLoader.new(gid.model_class, gid.model_id).find
+      else
+        gid.find
+      end
+    end
+
     private
 
     def max_query_complexity(ctx)
diff --git a/app/graphql/mutations/merge_requests/base.rb b/app/graphql/mutations/merge_requests/base.rb
index 7d0cb777ad1..e85d16fc2c5 100644
--- a/app/graphql/mutations/merge_requests/base.rb
+++ b/app/graphql/mutations/merge_requests/base.rb
@@ -10,7 +10,7 @@ module Mutations
                required: true,
                description: "The project the merge request to mutate is in"
 
-      argument :iid, GraphQL::ID_TYPE,
+      argument :iid, GraphQL::STRING_TYPE,
                required: true,
                description: "The iid of the merge request to mutate"
 
diff --git a/app/graphql/resolvers/issues_resolver.rb b/app/graphql/resolvers/issues_resolver.rb
index 3ee3849f483..6988b451ec3 100644
--- a/app/graphql/resolvers/issues_resolver.rb
+++ b/app/graphql/resolvers/issues_resolver.rb
@@ -2,11 +2,11 @@
 
 module Resolvers
   class IssuesResolver < BaseResolver
-    argument :iid, GraphQL::ID_TYPE,
+    argument :iid, GraphQL::STRING_TYPE,
               required: false,
               description: 'The IID of the issue, e.g., "1"'
 
-    argument :iids, [GraphQL::ID_TYPE],
+    argument :iids, [GraphQL::STRING_TYPE],
               required: false,
               description: 'The list of IIDs of issues, e.g., [1, 2]'
     argument :state, Types::IssuableStateEnum,
diff --git a/app/graphql/resolvers/merge_requests_resolver.rb b/app/graphql/resolvers/merge_requests_resolver.rb
index 90795c797ac..b84e60066e1 100644
--- a/app/graphql/resolvers/merge_requests_resolver.rb
+++ b/app/graphql/resolvers/merge_requests_resolver.rb
@@ -2,11 +2,11 @@
 
 module Resolvers
   class MergeRequestsResolver < BaseResolver
-    argument :iid, GraphQL::ID_TYPE,
+    argument :iid, GraphQL::STRING_TYPE,
               required: false,
               description: 'The IID of the merge request, e.g., "1"'
 
-    argument :iids, [GraphQL::ID_TYPE],
+    argument :iids, [GraphQL::STRING_TYPE],
               required: false,
               description: 'The list of IIDs of issues, e.g., [1, 2]'
 
diff --git a/app/graphql/types/base_object.rb b/app/graphql/types/base_object.rb
index 82b78abd573..e40059c46bb 100644
--- a/app/graphql/types/base_object.rb
+++ b/app/graphql/types/base_object.rb
@@ -6,5 +6,10 @@ module Types
     prepend Gitlab::Graphql::ExposePermissions
 
     field_class Types::BaseField
+
+    # All graphql fields exposing an id, should expose a global id.
+    def id
+      GitlabSchema.id_from_object(object)
+    end
   end
 end
diff --git a/app/graphql/types/ci/pipeline_type.rb b/app/graphql/types/ci/pipeline_type.rb
index de7d6570a3e..cff81e5670b 100644
--- a/app/graphql/types/ci/pipeline_type.rb
+++ b/app/graphql/types/ci/pipeline_type.rb
@@ -10,7 +10,7 @@ module Types
       expose_permissions Types::PermissionTypes::Ci::Pipeline
 
       field :id, GraphQL::ID_TYPE, null: false
-      field :iid, GraphQL::ID_TYPE, null: false
+      field :iid, GraphQL::STRING_TYPE, null: false
 
       field :sha, GraphQL::STRING_TYPE, null: false
       field :before_sha, GraphQL::STRING_TYPE, null: true
diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb
index 120ffe0dfde..85ac3102442 100644
--- a/app/graphql/types/merge_request_type.rb
+++ b/app/graphql/types/merge_request_type.rb
@@ -11,7 +11,7 @@ module Types
     present_using MergeRequestPresenter
 
     field :id, GraphQL::ID_TYPE, null: false
-    field :iid, GraphQL::ID_TYPE, null: false
+    field :iid, GraphQL::STRING_TYPE, null: false
     field :title, GraphQL::STRING_TYPE, null: false
     field :description, GraphQL::STRING_TYPE, null: true
     field :state, MergeRequestStateEnum, null: false
diff --git a/changelogs/unreleased/bvl-use-global-ids-graphql.yml b/changelogs/unreleased/bvl-use-global-ids-graphql.yml
new file mode 100644
index 00000000000..34cb65e6001
--- /dev/null
+++ b/changelogs/unreleased/bvl-use-global-ids-graphql.yml
@@ -0,0 +1,5 @@
+---
+title: Use global IDs when exposing GraphQL resources
+merge_request: 29080
+author:
+type: added
diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md
index 8d2bfff3a5d..38270af682e 100644
--- a/doc/development/api_graphql_styleguide.md
+++ b/doc/development/api_graphql_styleguide.md
@@ -32,6 +32,21 @@ a new presenter specifically for GraphQL.
 The presenter is initialized using the object resolved by a field, and
 the context.
 
+### Exposing Global ids
+
+When exposing an `id` field on a type, we will by default try to
+expose a global id by calling `to_global_id` on the resource being
+rendered.
+
+To override this behaviour, you can implement an `id` method on the
+type for which you are exposing an id. Please make sure that when
+exposing a `GraphQL::ID_TYPE` using a custom method that it is
+globally unique.
+
+The records that are exposing a `full_path` as an `ID_TYPE` are one of
+these exceptions. Since the full path is a unique identifier for a
+`Project` or `Namespace`.
+
 ### Connection Types
 
 GraphQL uses [cursor based
@@ -79,14 +94,14 @@ look like this:
           {
             "cursor": "Nzc=",
             "node": {
-              "id": "77",
+              "id": "gid://gitlab/Pipeline/77",
               "status": "FAILED"
             }
           },
           {
             "cursor": "Njc=",
             "node": {
-              "id": "67",
+              "id": "gid://gitlab/Pipeline/67",
               "status": "FAILED"
             }
           }
@@ -330,7 +345,7 @@ argument :project_path, GraphQL::ID_TYPE,
          required: true,
          description: "The project the merge request to mutate is in"
 
-argument :iid, GraphQL::ID_TYPE,
+argument :iid, GraphQL::STRING_TYPE,
          required: true,
          description: "The iid of the merge request to mutate"
 
diff --git a/lib/gitlab/graphql/loaders/batch_model_loader.rb b/lib/gitlab/graphql/loaders/batch_model_loader.rb
index 5a0099dc6b1..50d3293fcbb 100644
--- a/lib/gitlab/graphql/loaders/batch_model_loader.rb
+++ b/lib/gitlab/graphql/loaders/batch_model_loader.rb
@@ -12,7 +12,7 @@ module Gitlab
 
         # rubocop: disable CodeReuse/ActiveRecord
         def find
-          BatchLoader.for({ model: model_class, id: model_id }).batch do |loader_info, loader|
+          BatchLoader.for({ model: model_class, id: model_id.to_i }).batch do |loader_info, loader|
             per_model = loader_info.group_by { |info| info[:model] }
             per_model.each do |model, info|
               ids = info.map { |i| i[:id] }
diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb
index f5eb628a982..c427893f9cc 100644
--- a/spec/graphql/features/authorization_spec.rb
+++ b/spec/graphql/features/authorization_spec.rb
@@ -282,7 +282,7 @@ describe 'Gitlab::Graphql::Authorization' do
       issue_ids = issue_edges.map { |issue_edge| issue_edge['node']&.fetch('id') }
 
       expect(issue_edges.size).to eq(visible_issues.size)
-      expect(issue_ids).to eq(visible_issues.map { |i| i.id.to_s })
+      expect(issue_ids).to eq(visible_issues.map { |i| i.to_global_id.to_s })
     end
 
     it 'does not check access on fields that will not be rendered' do
diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb
index e9149f4250f..4076c1f824b 100644
--- a/spec/graphql/gitlab_schema_spec.rb
+++ b/spec/graphql/gitlab_schema_spec.rb
@@ -107,6 +107,64 @@ describe GitlabSchema do
     end
   end
 
+  describe '.id_from_object' do
+    it 'returns a global id' do
+      expect(described_class.id_from_object(build(:project, id: 1))).to be_a(GlobalID)
+    end
+
+    it "raises a meaningful error if a global id couldn't be generated" do
+      expect { described_class.id_from_object(build(:commit)) }
+        .to raise_error(RuntimeError, /include `GlobalID::Identification` into/i)
+    end
+  end
+
+  describe '.object_from_id' do
+    context 'for subclasses of `ApplicationRecord`' do
+      it 'returns the correct record' do
+        user = create(:user)
+
+        result = described_class.object_from_id(user.to_global_id.to_s)
+
+        expect(result.__sync).to eq(user)
+      end
+
+      it 'batchloads the queries' do
+        user1 = create(:user)
+        user2 = create(:user)
+
+        expect do
+          [described_class.object_from_id(user1.to_global_id),
+           described_class.object_from_id(user2.to_global_id)].map(&:__sync)
+        end.not_to exceed_query_limit(1)
+      end
+    end
+
+    context 'for other classes' do
+      # We cannot use an anonymous class here as `GlobalID` expects `.name` not
+      # to return `nil`
+      class TestGlobalId
+        include GlobalID::Identification
+        attr_accessor :id
+
+        def initialize(id)
+          @id = id
+        end
+      end
+
+      it 'falls back to a regular find' do
+        result = TestGlobalId.new(123)
+
+        expect(TestGlobalId).to receive(:find).with("123").and_return(result)
+
+        expect(described_class.object_from_id(result.to_global_id)).to eq(result)
+      end
+    end
+
+    it 'raises the correct error on invalid input' do
+      expect { described_class.object_from_id("bogus id") }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
+    end
+  end
+
   def field_instrumenters
     described_class.instrumenters[:field] + described_class.instrumenters[:field_after_built_ins]
   end
diff --git a/spec/requests/api/graphql/gitlab_schema_spec.rb b/spec/requests/api/graphql/gitlab_schema_spec.rb
index b6ca9246399..28676bb02f4 100644
--- a/spec/requests/api/graphql/gitlab_schema_spec.rb
+++ b/spec/requests/api/graphql/gitlab_schema_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe 'GitlabSchema configurations' do
   include GraphqlHelpers
 
-  let(:project) { create(:project) }
+  set(:project) { create(:project) }
 
   shared_examples 'imposing query limits' do
     describe '#max_complexity' do
@@ -136,4 +136,15 @@ describe 'GitlabSchema configurations' do
       post_graphql(query, current_user: nil)
     end
   end
+
+  context "global id's" do
+    it 'uses GlobalID to expose ids' do
+      post_graphql(graphql_query_for('project', { 'fullPath' => project.full_path }, %w(id)),
+                   current_user: project.owner)
+
+      parsed_id = GlobalID.parse(graphql_data['project']['id'])
+
+      expect(parsed_id).to eq(project.to_global_id)
+    end
+  end
 end
diff --git a/spec/requests/api/graphql/group_query_spec.rb b/spec/requests/api/graphql/group_query_spec.rb
index db9f2ac9dd0..e0f1e4dbe9e 100644
--- a/spec/requests/api/graphql/group_query_spec.rb
+++ b/spec/requests/api/graphql/group_query_spec.rb
@@ -56,7 +56,7 @@ describe 'getting group information' do
         post_graphql(group_query(group1), current_user: user1)
 
         expect(response).to have_gitlab_http_status(200)
-        expect(graphql_data['group']['id']).to eq(group1.id.to_s)
+        expect(graphql_data['group']['id']).to eq(group1.to_global_id.to_s)
         expect(graphql_data['group']['name']).to eq(group1.name)
         expect(graphql_data['group']['path']).to eq(group1.path)
         expect(graphql_data['group']['description']).to eq(group1.description)
diff --git a/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb
index 8f427d71a32..d75f0df9fd3 100644
--- a/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb
+++ b/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb
@@ -11,7 +11,7 @@ describe 'Setting WIP status of a merge request' do
   let(:mutation) do
     variables = {
       project_path: project.full_path,
-      iid: merge_request.iid
+      iid: merge_request.iid.to_s
     }
     graphql_mutation(:merge_request_set_wip, variables.merge(input))
   end
diff --git a/spec/requests/api/graphql/namespace/projects_spec.rb b/spec/requests/api/graphql/namespace/projects_spec.rb
index e05273da4bd..de1cd9586b6 100644
--- a/spec/requests/api/graphql/namespace/projects_spec.rb
+++ b/spec/requests/api/graphql/namespace/projects_spec.rb
@@ -60,7 +60,7 @@ describe 'getting projects', :nested_groups do
 
         expect(graphql_data['namespace']['projects']['edges'].size).to eq(1)
         project = graphql_data['namespace']['projects']['edges'][0]['node']
-        expect(project['id']).to eq(public_project.id.to_s)
+        expect(project['id']).to eq(public_project.to_global_id.to_s)
       end
     end
   end