Commit 810df4fb authored by Jan Provaznik's avatar Jan Provaznik

Merge branch '62826-graphql-note-mutations' into 'master'

GraphQL mutations for managing Notes

See merge request gitlab-org/gitlab-ce!30210
parents 227d8b44 073c8b25
......@@ -66,6 +66,8 @@ class GitlabSchema < GraphQL::Schema
if gid.model_class < ApplicationRecord
Gitlab::Graphql::Loaders::BatchModelLoader.new(gid.model_class, gid.model_id).find
elsif gid.model_class.respond_to?(:lazy_find)
gid.model_class.lazy_find(gid.model_id)
else
gid.find
end
......
# frozen_string_literal: true
module Mutations
module Notes
class Base < BaseMutation
include Gitlab::Graphql::Authorize::AuthorizeResource
field :note,
Types::Notes::NoteType,
null: true,
description: 'The note after mutation'
private
def find_object(id:)
GitlabSchema.object_from_id(id)
end
def check_object_is_noteable!(object)
unless object.is_a?(Noteable)
raise Gitlab::Graphql::Errors::ResourceNotAvailable,
'Cannot add notes to this resource'
end
end
def check_object_is_note!(object)
unless object.is_a?(Note)
raise Gitlab::Graphql::Errors::ResourceNotAvailable,
'Resource is not a note'
end
end
end
end
end
# frozen_string_literal: true
module Mutations
module Notes
module Create
# This is a Base class for the Note creation Mutations and is not
# mounted as a GraphQL mutation itself.
class Base < Mutations::Notes::Base
authorize :create_note
argument :noteable_id,
GraphQL::ID_TYPE,
required: true,
description: 'The global id of the resource to add a note to'
argument :body,
GraphQL::STRING_TYPE,
required: true,
description: copy_field_description(Types::Notes::NoteType, :body)
private
def resolve(args)
noteable = authorized_find!(id: args[:noteable_id])
check_object_is_noteable!(noteable)
note = ::Notes::CreateService.new(
noteable.project,
current_user,
create_note_params(noteable, args)
).execute
{
note: (note if note.persisted?),
errors: errors_on_object(note)
}
end
def create_note_params(noteable, args)
{
noteable: noteable,
note: args[:body]
}
end
end
end
end
end
# frozen_string_literal: true
module Mutations
module Notes
module Create
class DiffNote < Base
graphql_name 'CreateDiffNote'
argument :position,
Types::Notes::DiffPositionInputType,
required: true,
description: copy_field_description(Types::Notes::NoteType, :position)
private
def create_note_params(noteable, args)
super(noteable, args).merge({
type: 'DiffNote',
position: position(noteable, args)
})
end
def position(noteable, args)
position = args[:position].to_h
position[:position_type] = 'text'
position.merge!(position[:paths].to_h)
Gitlab::Diff::Position.new(position)
end
end
end
end
end
# frozen_string_literal: true
module Mutations
module Notes
module Create
class ImageDiffNote < Base
graphql_name 'CreateImageDiffNote'
argument :position,
Types::Notes::DiffImagePositionInputType,
required: true,
description: copy_field_description(Types::Notes::NoteType, :position)
private
def create_note_params(noteable, args)
super(noteable, args).merge({
type: 'DiffNote',
position: position(noteable, args)
})
end
def position(noteable, args)
position = args[:position].to_h
position[:position_type] = 'image'
position.merge!(position[:paths].to_h)
Gitlab::Diff::Position.new(position)
end
end
end
end
end
# frozen_string_literal: true
module Mutations
module Notes
module Create
class Note < Base
graphql_name 'CreateNote'
argument :discussion_id,
GraphQL::ID_TYPE,
required: false,
description: 'The global id of the discussion this note is in reply to'
private
def create_note_params(noteable, args)
discussion_id = nil
if args[:discussion_id]
discussion = GitlabSchema.object_from_id(args[:discussion_id])
authorize_discussion!(discussion)
discussion_id = discussion.id
end
super(noteable, args).merge({
in_reply_to_discussion_id: discussion_id
})
end
def authorize_discussion!(discussion)
unless Ability.allowed?(current_user, :read_note, discussion, scope: :user)
raise Gitlab::Graphql::Errors::ResourceNotAvailable,
"The discussion does not exist or you don't have permission to perform this action"
end
end
end
end
end
end
# frozen_string_literal: true
module Mutations
module Notes
class Destroy < Base
graphql_name 'DestroyNote'
authorize :admin_note
argument :id,
GraphQL::ID_TYPE,
required: true,
description: 'The global id of the note to destroy'
def resolve(id:)
note = authorized_find!(id: id)
check_object_is_note!(note)
::Notes::DestroyService.new(note.project, current_user).execute(note)
{
errors: []
}
end
end
end
end
# frozen_string_literal: true
module Mutations
module Notes
class Update < Base
graphql_name 'UpdateNote'
authorize :admin_note
argument :id,
GraphQL::ID_TYPE,
required: true,
description: 'The global id of the note to update'
argument :body,
GraphQL::STRING_TYPE,
required: true,
description: copy_field_description(Types::Notes::NoteType, :body)
def resolve(args)
note = authorized_find!(id: args[:id])
check_object_is_note!(note)
note = ::Notes::UpdateService.new(
note.project,
current_user,
{ note: args[:body] }
).execute(note)
{
note: note.reset,
errors: errors_on_object(note)
}
end
end
end
end
......@@ -2,5 +2,6 @@
module Types
class BaseInputObject < GraphQL::Schema::InputObject
prepend Gitlab::Graphql::CopyFieldDescription
end
end
# frozen_string_literal: true
module Types
# rubocop: disable Graphql/AuthorizeTypes
class DiffPathsInputType < BaseInputObject
argument :old_path, GraphQL::STRING_TYPE, required: false,
description: 'The path of the file on the start sha'
argument :new_path, GraphQL::STRING_TYPE, required: false,
description: 'The path of the file on the head sha'
end
# rubocop: enable Graphql/AuthorizeTypes
end
# frozen_string_literal: true
module Types
# rubocop: disable Graphql/AuthorizeTypes
# Types that use DiffRefsType should have their own authorization
class DiffRefsType < BaseObject
graphql_name 'DiffRefs'
field :head_sha, GraphQL::STRING_TYPE, null: false, description: 'The sha of the head at the time the comment was made'
field :base_sha, GraphQL::STRING_TYPE, null: false, description: 'The merge base of the branch the comment was made on'
field :start_sha, GraphQL::STRING_TYPE, null: false, description: 'The sha of the branch being compared against'
end
# rubocop: enable Graphql/AuthorizeTypes
end
......@@ -23,6 +23,7 @@ module Types
field :updated_at, Types::TimeType, null: false
field :source_project, Types::ProjectType, null: true
field :target_project, Types::ProjectType, null: false
field :diff_refs, Types::DiffRefsType, null: true
# Alias for target_project
field :project, Types::ProjectType, null: false
field :project_id, GraphQL::INT_TYPE, null: false, method: :target_project_id
......
......@@ -10,5 +10,10 @@ module Types
mount_mutation Mutations::AwardEmojis::Remove
mount_mutation Mutations::AwardEmojis::Toggle
mount_mutation Mutations::MergeRequests::SetWip, calls_gitaly: true
mount_mutation Mutations::Notes::Create::Note, calls_gitaly: true
mount_mutation Mutations::Notes::Create::DiffNote, calls_gitaly: true
mount_mutation Mutations::Notes::Create::ImageDiffNote, calls_gitaly: true
mount_mutation Mutations::Notes::Update
mount_mutation Mutations::Notes::Destroy
end
end
# frozen_string_literal: true
module Types
module Notes
# rubocop: disable Graphql/AuthorizeTypes
class DiffImagePositionInputType < DiffPositionBaseInputType
graphql_name 'DiffImagePositionInput'
argument :x, GraphQL::INT_TYPE, required: true,
description: copy_field_description(Types::Notes::DiffPositionType, :x)
argument :y, GraphQL::INT_TYPE, required: true,
description: copy_field_description(Types::Notes::DiffPositionType, :y)
argument :width, GraphQL::INT_TYPE, required: true,
description: copy_field_description(Types::Notes::DiffPositionType, :width)
argument :height, GraphQL::INT_TYPE, required: true,
description: copy_field_description(Types::Notes::DiffPositionType, :height)
end
# rubocop: enable Graphql/AuthorizeTypes
end
end
# frozen_string_literal: true
module Types
module Notes
# rubocop: disable Graphql/AuthorizeTypes
class DiffPositionBaseInputType < BaseInputObject
argument :head_sha, GraphQL::STRING_TYPE, required: true,
description: copy_field_description(Types::DiffRefsType, :head_sha)
argument :base_sha, GraphQL::STRING_TYPE, required: false,
description: copy_field_description(Types::DiffRefsType, :base_sha)
argument :start_sha, GraphQL::STRING_TYPE, required: true,
description: copy_field_description(Types::DiffRefsType, :start_sha)
argument :paths,
Types::DiffPathsInputType,
required: true,
description: 'The paths of the file that was changed. ' \
'Both of the properties of this input are optional, but at least one of them is required'
end
# rubocop: enable Graphql/AuthorizeTypes
end
end
# frozen_string_literal: true
module Types
module Notes
# rubocop: disable Graphql/AuthorizeTypes
class DiffPositionInputType < DiffPositionBaseInputType
graphql_name 'DiffPositionInput'
argument :old_line, GraphQL::INT_TYPE, required: false,
description: copy_field_description(Types::Notes::DiffPositionType, :old_line)
argument :new_line, GraphQL::INT_TYPE, required: true,
description: copy_field_description(Types::Notes::DiffPositionType, :new_line)
end
# rubocop: enable Graphql/AuthorizeTypes
end
end
......@@ -7,12 +7,7 @@ module Types
class DiffPositionType < BaseObject
graphql_name 'DiffPosition'
field :head_sha, GraphQL::STRING_TYPE, null: false,
description: "The sha of the head at the time the comment was made"
field :base_sha, GraphQL::STRING_TYPE, null: true,
description: "The merge base of the branch the comment was made on"
field :start_sha, GraphQL::STRING_TYPE, null: false,
description: "The sha of the branch being compared against"
field :diff_refs, Types::DiffRefsType, null: false
field :file_path, GraphQL::STRING_TYPE, null: false,
description: "The path of the file that was changed"
......
......@@ -8,8 +8,16 @@ module Types
authorize :read_note
field :id, GraphQL::ID_TYPE, null: false
field :reply_id, GraphQL::ID_TYPE, null: false, description: 'The ID used to reply to this discussion'
field :created_at, Types::TimeType, null: false
field :notes, Types::Notes::NoteType.connection_type, null: false, description: "All notes in the discussion"
# The gem we use to generate Global IDs is hard-coded to work with
# `id` properties. To generate a GID for the `reply_id` property,
# we must use the ::Gitlab::GlobalId module.
def reply_id
::Gitlab::GlobalId.build(object, id: object.reply_id)
end
end
end
end
......@@ -38,6 +38,17 @@ class Discussion
grouped_notes.values.map { |notes| build(notes, context_noteable) }
end
def self.lazy_find(discussion_id)
BatchLoader.for(discussion_id).batch do |discussion_ids, loader|
results = Note.where(discussion_id: discussion_ids).fresh.to_a.group_by(&:discussion_id)
results.each do |discussion_id, notes|
next if notes.empty?
loader.call(discussion_id, Discussion.build(notes))
end
end
end
# Returns an alphanumeric discussion ID based on `build_discussion_id`
def self.discussion_id(note)
Digest::SHA1.hexdigest(build_discussion_id(note).join("-"))
......
......@@ -158,6 +158,8 @@ class Note < ApplicationRecord
Discussion.build_collection(all.includes(:noteable).fresh, context_noteable)
end
# Note: Where possible consider using Discussion#lazy_find to return
# Discussions in order to benefit from having records batch loaded.
def find_discussion(discussion_id)
notes = where(discussion_id: discussion_id).fresh.to_a
......
---
title: GraphQL mutations for managing Notes
merge_request: 30210
author:
type: added
# frozen_string_literal: true
module Gitlab
module GlobalId
def self.build(object = nil, model_name: nil, id: nil, params: nil)
if object
model_name ||= object.class.name
id ||= object.id
end
::URI::GID.build(app: GlobalID.app, model_name: model_name, model_id: id, params: params)
end
end
end
......@@ -143,6 +143,26 @@ describe GitlabSchema do
end
end
context 'for classes that are not ActiveRecord subclasses and have implemented .lazy_find' do
it 'returns the correct record' do
note = create(:discussion_note_on_merge_request)
result = described_class.object_from_id(note.to_global_id)
expect(result.__sync).to eq(note)
end
it 'batchloads the queries' do
note1 = create(:discussion_note_on_merge_request)
note2 = create(:discussion_note_on_merge_request)
expect do
[described_class.object_from_id(note1.to_global_id),
described_class.object_from_id(note2.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`
......
# frozen_string_literal: true
require 'spec_helper'
describe GitlabSchema.types['DiffRefs'] do
it { expect(described_class.graphql_name).to eq('DiffRefs') }
it { expect(described_class).to have_graphql_fields(:base_sha, :head_sha, :start_sha) }
end
......@@ -13,7 +13,7 @@ describe GitlabSchema.types['MergeRequest'] do
description_html state created_at updated_at source_project target_project
project project_id source_project_id target_project_id source_branch
target_branch work_in_progress merge_when_pipeline_succeeds diff_head_sha
merge_commit_sha user_notes_count should_remove_source_branch
merge_commit_sha user_notes_count should_remove_source_branch diff_refs
force_remove_source_branch merge_status in_progress_merge_commit_sha
merge_error allow_collaboration should_be_rebased rebase_commit_sha
rebase_in_progress merge_commit_message default_merge_commit_message
......
......@@ -3,7 +3,7 @@ require 'spec_helper'
describe GitlabSchema.types['DiffPosition'] do
it 'exposes the expected fields' do
expected_fields = [:head_sha, :base_sha, :start_sha, :file_path, :old_path,
expected_fields = [:diff_refs, :file_path, :old_path,
:new_path, :position_type, :old_line, :new_line, :x, :y,
:width, :height]
......
......@@ -2,7 +2,7 @@
require 'spec_helper'
describe GitlabSchema.types['Discussion'] do
it { is_expected.to have_graphql_fields(:id, :created_at, :notes) }
it { is_expected.to have_graphql_fields(:id, :created_at, :notes, :reply_id) }
it { is_expected.to require_graphql_authorizations(:read_note) }
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::GlobalId do
describe '.build' do
set(:object) { create(:issue) }
it 'returns a standard GlobalId if only object is passed' do
expect(described_class.build(object).to_s).to eq(object.to_global_id.to_s)
end
it 'returns a GlobalId from params' do
expect(described_class.build(model_name: 'MyModel', id: 'myid').to_s).to eq(
'gid://gitlab/MyModel/myid'
)
end
it 'returns a GlobalId from object and `id` param' do
expect(described_class.build(object, id: 'myid').to_s).to eq(
'gid://gitlab/Issue/myid'
)
end
it 'returns a GlobalId from object and `model_name` param' do
expect(described_class.build(object, model_name: 'MyModel').to_s).to eq(
"gid://gitlab/MyModel/#{object.id}"
)
end
it 'returns an error if model_name and id are not able to be determined' do
expect { described_class.build(id: 'myid') }.to raise_error(URI::InvalidComponentError)
expect { described_class.build(model_name: 'MyModel') }.to raise_error(URI::InvalidComponentError)
expect { described_class.build }.to raise_error(URI::InvalidComponentError)
end
end
end
......@@ -10,6 +10,20 @@ describe Discussion do
let(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
let(:third_note) { create(:diff_note_on_merge_request) }
describe '.lazy_find' do
let!(:note1) { create(:discussion_note_on_merge_request).to_discussion }
let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note1).to_discussion }
subject { [note1, note2].map { |note| described_class.lazy_find(note.discussion_id) } }
it 'batches requests' do
expect do
[described_class.lazy_find(note1.id),
described_class.lazy_find(note2.id)].map(&:__sync)
end.not_to exceed_query_limit(1)
end
end
describe '.build' do
it 'returns a discussion of the right type' do
discussion = described_class.build([first_note, second_note], merge_request)
......
# frozen_string_literal: true
require 'spec_helper'
describe 'Adding a DiffNote' do
include GraphqlHelpers
set(:current_user) { create(:user) }
let(:noteable) { create(:merge_request, source_project: project, target_project: project) }
let(:project) { create(:project, :repository) }
let(:diff_refs) { noteable.diff_refs }
let(:mutation) do
variables = {
noteable_id: GitlabSchema.id_from_object(noteable).to_s,
body: 'Body text',
position: {
paths: {
old_path: 'files/ruby/popen.rb',
new_path: 'files/ruby/popen2.rb'
},
new_line: 14,
base_sha: diff_refs.base_sha,
head_sha: diff_refs.head_sha,
start_sha: diff_refs.start_sha
}
}
graphql_mutation(:create_diff_note, variables)
end
def mutation_response
graphql_mutation_response(:create_diff_note)
end
it_behaves_like 'a Note mutation when the user does not have permission'
context 'when the user has permission' do
before do
project.add_developer(current_user)
end
it_behaves_like 'a Note mutation that creates a Note'
it_behaves_like 'a Note mutation when there are active record validation errors', model: DiffNote
context do
let(:diff_refs) { build(:merge_request).diff_refs } # Allow fake diff refs so arguments are valid
it_behaves_like 'a Note mutation when the given resource id is not for a Noteable'
end
it 'returns the note with the correct position' do
post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response['note']['body']).to eq('Body text')
mutation_position_response = mutation_response['note']['position']
expect(mutation_position_response['positionType']).to eq('text')
expect(mutation_position_response['filePath']).to eq('files/ruby/popen2.rb')
expect(mutation_position_response['oldPath']).to eq('files/ruby/popen.rb')
expect(mutation_position_response['newPath']).to eq('files/ruby/popen2.rb')
expect(mutation_position_response['newLine']).to eq(14)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe 'Adding an image DiffNote' do
include GraphqlHelpers
set(:current_user) { create(:user) }
let(:noteable) { create(:merge_request, source_project: project, target_project: project) }
let(:project) { create(:project, :repository) }
let(:diff_refs) { noteable.diff_refs }
let(:mutation) do
variables = {
noteable_id: GitlabSchema.id_from_object(noteable).to_s,
body: 'Body text',
position: {
paths: {
old_path: 'files/images/any_image.png',
new_path: 'files/images/any_image2.png'
},
width: 100,
height: 200,
x: 1,
y: 2,
base_sha: diff_refs.base_sha,
head_sha: diff_refs.head_sha,
start_sha: diff_refs.start_sha
}
}
graphql_mutation(:create_image_diff_note, variables)
end