diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index b1f2ffb85035e7d65936a833d0f4c6657c898537..d66a2333d119a936869572cd4b5addf915eb8c65 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -78,6 +78,13 @@ def object_from_id(global_id, ctx = {}) 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 diff --git a/app/graphql/types/base_object.rb b/app/graphql/types/base_object.rb index 70e665f8fc3986133c92a56272dd7a1cfdef4933..9c36c83d4a337d745842eae94a6def3fafb2f428 100644 --- a/app/graphql/types/base_object.rb +++ b/app/graphql/types/base_object.rb @@ -8,6 +8,12 @@ class BaseObject < GraphQL::Schema::Object 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 @@ def id 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 diff --git a/app/graphql/types/board_type.rb b/app/graphql/types/board_type.rb index f5dc9e084270d103f164b8fafe2618bef5dc892f..2a7b318e28351a9831faa08c1bcbd080b7353113 100644 --- a/app/graphql/types/board_type.rb +++ b/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, diff --git a/app/graphql/types/global_id_type.rb b/app/graphql/types/global_id_type.rb index 9ae9ba32c132e0bda1574a226b1d99d45f01e14a..4c51d4248dde3125faf0c5e429e741e156195cf4 100644 --- a/app/graphql/types/global_id_type.rb +++ b/app/graphql/types/global_id_type.rb @@ -30,6 +30,8 @@ def self.coerce_result(value, _ctx) # @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 diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 0b3335c0c2b70693ec854098bb3b32c22b7d7a80..a99ce3999ca0ace12d41855ccda11217031d411b 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/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 diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index 3bc4dbaec574795b0d73cc29a9b130c273875a34..be784e3a20b03e086d573a175131d7bdab2a5bcd 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/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 diff --git a/ee/app/graphql/ee/mutations/issues/update.rb b/ee/app/graphql/ee/mutations/issues/update.rb index beaca69fc0837f06a28e4b3a228fa24c3521b2ac..68a5a083dbad80755ba61371d92f12aca1ec4863 100644 --- a/ee/app/graphql/ee/mutations/issues/update.rb +++ b/ee/app/graphql/ee/mutations/issues/update.rb @@ -9,15 +9,16 @@ module Update 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 diff --git a/ee/app/graphql/types/epic_type.rb b/ee/app/graphql/types/epic_type.rb index 2ea67c7ba54d812104e861d20d73eb04424ae04f..87a78cf9900d0c2e24d2a66d80ac9b72df54c461 100644 --- a/ee/app/graphql/types/epic_type.rb +++ b/ee/app/graphql/types/epic_type.rb @@ -6,7 +6,7 @@ class EpicType < BaseObject graphql_name 'Epic' description 'Represents an epic' - + accepts ::Epic authorize :read_epic expose_permissions Types::PermissionTypes::Epic diff --git a/ee/spec/graphql/mutations/issues/update_spec.rb b/ee/spec/graphql/mutations/issues/update_spec.rb index 378bcdfd4d920e95b598338aaababd951dde2cfc..021f7779a116c4a9abae7d4e008fdae671c0ad56 100644 --- a/ee/spec/graphql/mutations/issues/update_spec.rb +++ b/ee/spec/graphql/mutations/issues/update_spec.rb @@ -15,26 +15,34 @@ 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 @@ 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 @@ 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) diff --git a/ee/spec/requests/api/graphql/mutations/issues/update_spec.rb b/ee/spec/requests/api/graphql/mutations/issues/update_spec.rb index e33b4721502bdcdf5e962cb055c30557ce53c4d6..337473db0bbd7f6af0dcadf5bc49df6ad6a1d70f 100644 --- a/ee/spec/requests/api/graphql/mutations/issues/update_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/issues/update_spec.rb @@ -5,8 +5,9 @@ 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 @@ 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 @@ 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 diff --git a/spec/graphql/types/global_id_type_spec.rb b/spec/graphql/types/global_id_type_spec.rb index 7589b0e285ef8b053d334f2deb9e343c6a24bd12..cb129868f7ea0d7a839b7d92efc0d6456ed7723e 100644 --- a/spec/graphql/types/global_id_type_spec.rb +++ b/spec/graphql/types/global_id_type_spec.rb @@ -45,8 +45,7 @@ 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