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

[GraphQL] Discussions: use Global-ID scalar

See merge request gitlab-org/gitlab!36101

app/graphql/types/notes/discussion_type.rb

@@ -3,24 +3,26 @@
 module Types
   module Notes
     class DiscussionType < BaseObject
+      DiscussionID = ::Types::GlobalIDType[::Discussion]
+
       graphql_name 'Discussion'
 
       authorize :read_note
 
       implements(Types::ResolvableInterface)
 
-      field :id, GraphQL::ID_TYPE, null: false,
+      field :id, DiscussionID, null: false,
             description: "ID of this discussion"
-      field :reply_id, GraphQL::ID_TYPE, null: false,
+      field :reply_id, DiscussionID, null: false,
             description: 'ID used to reply to this discussion'
       field :created_at, Types::TimeType, null: false,
             description: "Timestamp of the discussion's creation"
       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.
+      # DiscussionID.coerce_result is suitable here, but will always mark this
+      # as being a 'Discussion'. Using `GlobalId.build` guarantees that we get
+      # the correct class, and that it matches `id`.
       def reply_id
         ::Gitlab::GlobalId.build(object, id: object.reply_id)
       end

app/graphql/types/notes/note_type.rb

@@ -11,7 +11,7 @@ module Types
 
       implements(Types::ResolvableInterface)
 
-      field :id, GraphQL::ID_TYPE, null: false,
+      field :id, ::Types::GlobalIDType[::Note], null: false,
             description: 'ID of the note'
 
       field :project, Types::ProjectType,

doc/api/graphql/reference/gitlab_schema.graphql

@@ -7878,7 +7878,7 @@ type Discussion implements ResolvableInterface {
   """
   ID of this discussion
   """
-  id: ID!
+  id: DiscussionID!
 
   """
   All notes in the discussion
@@ -7908,7 +7908,7 @@ type Discussion implements ResolvableInterface {
   """
   ID used to reply to this discussion
   """
-  replyId: ID!
+  replyId: DiscussionID!
 
   """
   Indicates if the object can be resolved
@@ -16154,7 +16154,7 @@ type Note implements ResolvableInterface {
   """
   ID of the note
   """
-  id: ID!
+  id: NoteID!
 
   """
   The position of this note on a diff

doc/api/graphql/reference/gitlab_schema.json

@@ -21766,7 +21766,7 @@
                 "name": null,
                 "ofType": {
                   "kind": "SCALAR",
-                  "name": "ID",
+                  "name": "DiscussionID",
                   "ofType": null
                 }
               },
@@ -21841,7 +21841,7 @@
                 "name": null,
                 "ofType": {
                   "kind": "SCALAR",
-                  "name": "ID",
+                  "name": "DiscussionID",
                   "ofType": null
                 }
               },
@@ -47730,7 +47730,7 @@
                 "name": null,
                 "ofType": {
                   "kind": "SCALAR",
-                  "name": "ID",
+                  "name": "NoteID",
                   "ofType": null
                 }
               },

doc/api/graphql/reference/index.md

@@ -1301,9 +1301,9 @@ Aggregated summary of changes.
 | Field | Type | Description |
 | ----- | ---- | ----------- |
 | `createdAt` | Time! | Timestamp of the discussion's creation |
-| `id` | ID! | ID of this discussion |
+| `id` | DiscussionID! | ID of this discussion |
 | `notes` | NoteConnection! | All notes in the discussion |
-| `replyId` | ID! | ID used to reply to this discussion |
+| `replyId` | DiscussionID! | ID used to reply to this discussion |
 | `resolvable` | Boolean! | Indicates if the object can be resolved |
 | `resolved` | Boolean! | Indicates if the object is resolved |
 | `resolvedAt` | Time | Timestamp of when the object was resolved |
@@ -2429,7 +2429,7 @@ Autogenerated return type of NamespaceIncreaseStorageTemporarily.
 | `confidential` | Boolean | Indicates if this note is confidential |
 | `createdAt` | Time! | Timestamp of the note creation |
 | `discussion` | Discussion | The discussion this note is a part of |
-| `id` | ID! | ID of the note |
+| `id` | NoteID! | ID of the note |
 | `position` | DiffPosition | The position of this note on a diff |
 | `project` | Project | Project associated with the note |
 | `resolvable` | Boolean! | Indicates if the object can be resolved |

lib/gitlab/global_id.rb

@@ -19,8 +19,8 @@ module Gitlab
         value
       when URI::GID
         GlobalID.new(value)
-      when Integer
-        raise CoerceError, 'Cannot coerce Integer' unless model_name.present?
+      when Integer, String
+        raise CoerceError, "Cannot coerce #{value.class}" unless model_name.present?
 
         GlobalID.new(::Gitlab::GlobalId.build(model_name: model_name, id: value))
       else

spec/requests/api/graphql/issue/issue_spec.rb

@@ -24,6 +24,20 @@ RSpec.describe 'Query.issue(id)' do
     end
   end
 
+  it_behaves_like 'a noteable graphql type we can query' do
+    let(:noteable) { issue }
+    let(:project) { issue.project }
+    let(:path_to_noteable) { [:issue] }
+
+    before do
+      project.add_guest(current_user)
+    end
+
+    def query(fields)
+      graphql_query_for('issue', issue_params, fields)
+    end
+  end
+
   context 'when the user does not have access to the issue' do
     it 'returns nil' do
       project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)

spec/requests/api/graphql/project/issue/designs/notes_spec.rb

@@ -22,6 +22,23 @@ RSpec.describe 'Getting designs related to an issue' do
     end
   end
 
+  it_behaves_like 'a noteable graphql type we can query' do
+    let(:noteable) { design }
+    let(:note_factory) { :diff_note_on_design }
+    let(:discussion_factory) { :diff_note_on_design }
+    let(:path_to_noteable) { [:issue, :design_collection, :designs, :nodes, 0] }
+
+    before do
+      project.add_developer(current_user)
+    end
+
+    def query(fields)
+      graphql_query_for(:issue, { id: global_id_of(issue) }, <<~FIELDS)
+        designCollection { designs { nodes { #{fields} } } }
+      FIELDS
+    end
+  end
+
   it 'is not too deep for anonymous users' do
     note_fields = <<~FIELDS
       id
@@ -37,7 +54,7 @@ RSpec.describe 'Getting designs related to an issue' do
     expect(note_data['id']).to eq(note.to_global_id.to_s)
   end
 
-  def query(note_fields = all_graphql_fields_for(Note))
+  def query(note_fields = all_graphql_fields_for(Note, max_depth: 1))
     design_node = <<~NODE
     designs {
       nodes {

spec/support/helpers/graphql_helpers.rb

@@ -510,9 +510,13 @@ module GraphqlHelpers
     end
   end
 
-  def global_id_of(model)
+  def global_id_of(model, id: nil, model_name: nil)
+    if id || model_name
+      ::Gitlab::GlobalId.build(model, id: id, model_name: model_name).to_s
+    else
       model.to_global_id.to_s
     end
+  end
 
   def missing_required_argument(path, argument)
     a_hash_including(

spec/support/shared_examples/requests/api/graphql/noteable_shared_examples.rb

+# frozen_string_literal: true
+
+# Requires `query(fields)`, `path_to_noteable`, `project`, and `noteable` bindings
+RSpec.shared_examples 'a noteable graphql type we can query' do
+  let(:note_factory) { :note }
+  let(:discussion_factory) { :discussion_note }
+
+  describe '.discussions' do
+    let(:fields) do
+      "discussions { nodes { #{all_graphql_fields_for('Discussion')} } }"
+    end
+
+    def expected
+      noteable.discussions.map do |discussion|
+        include(
+          'id' => global_id_of(discussion),
+          'replyId' => global_id_of(discussion, id: discussion.reply_id),
+          'createdAt' => discussion.created_at.iso8601,
+          'notes' => include(
+            'nodes' => have_attributes(size: discussion.notes.size)
+          )
+        )
+      end
+    end
+
+    it 'can fetch discussions' do
+      create(discussion_factory, project: project, noteable: noteable)
+
+      post_graphql(query(fields), current_user: current_user)
+
+      expect(graphql_data_at(*path_to_noteable, :discussions, :nodes))
+        .to match_array(expected)
+    end
+  end
+
+  describe '.notes' do
+    let(:fields) do
+      "notes { nodes { #{all_graphql_fields_for('Note', max_depth: 2)} } }"
+    end
+
+    def expected
+      noteable.notes.map do |note|
+        include(
+          'id' => global_id_of(note),
+          'project' => include('id' => global_id_of(project)),
+          'author' => include('id' => global_id_of(note.author)),
+          'createdAt' => note.created_at.iso8601,
+          'body' => eq(note.note)
+        )
+      end
+    end
+
+    it 'can fetch notes' do
+      create(note_factory, project: project, noteable: noteable)
+
+      post_graphql(query(fields), current_user: current_user)
+
+      expect(graphql_data_at(*path_to_noteable, :notes, :nodes))
+        .to match_array(expected)
+    end
+  end
+end