Merge branch 'ajk-nullable-global-id-type' into 'master'

Add specific type to epic_id for updateIssue

See merge request gitlab-org/gitlab!46491

app/graphql/gitlab_schema.rb

@@ -78,6 +78,13 @@ class GitlabSchema < GraphQL::Schema
       find_by_gid(gid)
     end
 
+    def resolve_type(type, object, ctx = :__undefined__)
+      tc = type.metadata[:type_class]
+      return if tc.respond_to?(:assignable?) && !tc.assignable?(object)
+
+      super
+    end
+
     # Find an object by looking it up from its 'GlobalID'.
     #
     # * For `ApplicationRecord`s, this is equivalent to

app/graphql/types/base_object.rb

@@ -8,6 +8,12 @@ module Types
 
     field_class Types::BaseField
 
+    def self.accepts(*types)
+      @accepts ||= []
+      @accepts += types
+      @accepts
+    end
+
     # All graphql fields exposing an id, should expose a global id.
     def id
       GitlabSchema.id_from_object(object)
@@ -16,5 +22,13 @@ module Types
     def current_user
       context[:current_user]
     end
+
+    def self.assignable?(object)
+      assignable = accepts
+
+      return true if assignable.blank?
+
+      assignable.any? { |cls| object.is_a?(cls) }
+    end
   end
 end

app/graphql/types/board_type.rb

@@ -4,7 +4,7 @@ module Types
   class BoardType < BaseObject
     graphql_name 'Board'
     description 'Represents a project or group board'
-
+    accepts ::Board
     authorize :read_board
 
     field :id, type: GraphQL::ID_TYPE, null: false,

app/graphql/types/global_id_type.rb

@@ -30,6 +30,8 @@ module Types
     # @param value [String]
     # @return [GID]
     def self.coerce_input(value, _ctx)
+      return if value.nil?
+
       gid = GlobalID.parse(value)
       raise GraphQL::CoercionError, "#{value.inspect} is not a valid Global ID" if gid.nil?
       raise GraphQL::CoercionError, "#{value.inspect} is not a Gitlab Global ID" unless gid.app == GlobalID.app

doc/api/graphql/reference/gitlab_schema.graphql

@@ -21475,7 +21475,7 @@ input UpdateIssueInput {
   """
   The ID of the parent epic. NULL when removing the association
   """
-  epicId: ID
+  epicId: EpicID
 
   """
   The desired health status

doc/api/graphql/reference/gitlab_schema.json

@@ -62412,7 +62412,7 @@
               "description": "The ID of the parent epic. NULL when removing the association",
               "type": {
                 "kind": "SCALAR",
-                "name": "ID",
+                "name": "EpicID",
                 "ofType": null
               },
               "defaultValue": null

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

@@ -9,15 +9,16 @@ module EE
         prepended do
           include ::Mutations::Issues::CommonEEMutationArguments
 
-          argument :epic_id, GraphQL::ID_TYPE,
+          argument :epic_id, ::Types::GlobalIDType[::Epic],
                    required: false,
+                   loads: ::Types::EpicType,
                    description: 'The ID of the parent epic. NULL when removing the association'
         end
 
-        def resolve(project_path:, iid:, **args)
-          args[:epic_id] = ::GitlabSchema.parse_gid(args[:epic_id], expected_type: ::Epic).model_id if args[:epic_id]
-
+        def resolve(**args)
           super
+        rescue ::Gitlab::Access::AccessDeniedError
+          raise_resource_not_available_error!
         end
       end
     end

ee/app/graphql/types/epic_type.rb

@@ -6,7 +6,7 @@ module Types
 
     graphql_name 'Epic'
     description 'Represents an epic'
-
+    accepts ::Epic
     authorize :read_epic
 
     expose_permissions Types::PermissionTypes::Epic

ee/spec/graphql/mutations/issues/update_spec.rb

@@ -15,26 +15,34 @@ RSpec.describe Mutations::Issues::Update do
     let_it_be(:issue) { create(:issue, project: project) }
     let_it_be(:epic) { create(:epic, group: group) }
 
-    let(:epic_id) { epic.to_global_id.to_s }
     let(:params) do
       {
         project_path: project.full_path,
         iid: issue.iid,
-        epic_id: epic_id,
         weight: 10
-      }
+      }.merge(epic_params)
+    end
+
+    let(:epic_params) do
+      { epic: epic }
     end
 
     let(:mutated_issue) { subject[:issue] }
-    let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+    let(:current_user) { user }
+    let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) }
 
     subject { mutation.resolve(params) }
 
+    before do
+      group.clear_memoization(:feature_available)
+      group.add_developer(user)
+    end
+
     context 'when epics feature is disabled' do
       it 'raises an error' do
         group.add_developer(user)
 
-        expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
+        expect { subject }.to raise_error(::Gitlab::Graphql::Errors::ResourceNotAvailable)
       end
     end
 
@@ -44,16 +52,14 @@ RSpec.describe Mutations::Issues::Update do
       end
 
       context 'for user without permissions' do
+        let(:current_user) { create(:user) }
+
         it 'raises an error' do
           expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
         end
       end
 
       context 'for user with correct permissions' do
-        before do
-          group.add_developer(user)
-        end
-
         context 'when a valid epic is given' do
           it 'updates the epic' do
             expect { subject }.to change { issue.reload.epic }.from(nil).to(epic)
@@ -70,7 +76,7 @@ RSpec.describe Mutations::Issues::Update do
             issue.update!(epic: epic)
           end
 
-          let(:epic_id) { nil }
+          let(:epic_params) { { epic: nil } }
 
           it 'set the epic to nil' do
             expect { subject }.to change { issue.reload.epic }.from(epic).to(nil)

ee/spec/requests/api/graphql/mutations/issues/update_spec.rb

@@ -5,8 +5,9 @@ require 'spec_helper'
 RSpec.describe 'Update of an existing issue' do
   include GraphqlHelpers
 
+  let_it_be(:group) { create(:group) }
   let_it_be(:current_user) { create(:user) }
-  let_it_be(:project) { create(:project, :public) }
+  let_it_be(:project) { create(:project, :public, group: group) }
   let_it_be(:issue) { create(:issue, project: project) }
   let(:input) do
     {
@@ -17,7 +18,7 @@ RSpec.describe 'Update of an existing issue' do
   end
 
   let(:mutation) { graphql_mutation(:update_issue, input.merge(project_path: project.full_path)) }
-  let(:mutation_response) { graphql_mutation_response(:update_issue) }
+  let(:mutated_issue) { graphql_mutation_response(:update_issue)['issue'] }
 
   before do
     stub_licensed_features(issuable_health_status: true)
@@ -28,6 +29,117 @@ RSpec.describe 'Update of an existing issue' do
     post_graphql_mutation(mutation, current_user: current_user)
 
     expect(response).to have_gitlab_http_status(:success)
-    expect(mutation_response['issue']).to include(input)
+    expect(mutated_issue).to include(input)
+  end
+
+  context 'setting epic' do
+    let(:epic) { create(:epic, group: group) }
+
+    let(:input) do
+      { iid: issue.iid.to_s, epic_id: global_id_of(epic) }
+    end
+
+    before do
+      stub_licensed_features(epics: true)
+      group.add_developer(current_user)
+    end
+
+    it 'sets the epic' do
+      post_graphql_mutation(mutation, current_user: current_user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(graphql_errors).to be_blank
+      expect(mutated_issue).to include(
+        'epic' => include('id' => global_id_of(epic))
+      )
+    end
+
+    context 'the epic is not readable to the current user' do
+      let(:epic) { create(:epic) }
+
+      it 'does not set the epic' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(graphql_errors).not_to be_blank
+      end
+    end
+
+    context 'the epic is not an epic' do
+      let(:epic) { create(:user) }
+
+      it 'does not set the epic' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(graphql_errors).not_to be_blank
+      end
+    end
+
+    # TODO: remove when the global-ID compatibility layer is removed,
+    # at which point this error becomes unreachable because the query will
+    # be rejected as ill-typed.
+    context 'the epic is not an epic, using the ID type' do
+      let(:mutation) do
+        query = <<~GQL
+        mutation($epic: ID!, $path: ID!, $iid: String!) {
+          updateIssue(input: { epicId: $epic, projectPath: $path, iid: $iid }) {
+            errors
+          }
+        }
+        GQL
+
+        ::GraphqlHelpers::MutationDefinition.new(query, {
+          epic: global_id_of(create(:user)),
+          iid: issue.iid.to_s,
+          path: project.full_path
+        })
+      end
+
+      it 'does not set the epic' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(graphql_errors).to contain_exactly(
+          a_hash_including('message' => /No object found/)
+        )
+      end
+    end
+  end
+
+  context 'removing epic' do
+    let(:epic) { create(:epic, group: group) }
+
+    let(:input) do
+      { iid: issue.iid.to_s, epic_id: nil }
+    end
+
+    before do
+      stub_licensed_features(epics: true)
+      group.add_developer(current_user)
+      issue.update!(epic: epic)
+    end
+
+    it 'removes the epic' do
+      post_graphql_mutation(mutation, current_user: current_user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(graphql_errors).to be_blank
+      expect(mutated_issue).to include('epic' => be_nil)
+    end
+
+    context 'the epic argument is not provided' do
+      let(:input) do
+        { iid: issue.iid.to_s, weight: 1 }
+      end
+
+      it 'does not remove the epic' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(graphql_errors).to be_blank
+        expect(mutated_issue).to include('epic' => be_present)
+      end
+    end
   end
 end

spec/graphql/types/global_id_type_spec.rb

@@ -45,8 +45,7 @@ RSpec.describe Types::GlobalIDType do
     end
 
     it 'rejects nil' do
-      expect { described_class.coerce_isolated_input(nil) }
-        .to raise_error(GraphQL::CoercionError)
+      expect(described_class.coerce_isolated_input(nil)).to be_nil
     end
 
     it 'rejects gids from different apps' do