Merge branch 'psi-gql-type-iteration' into 'master'

Use stricter typed global IDs for iterations

See merge request gitlab-org/gitlab!47045

.rubocop_manual_todo.yml

@@ -23,8 +23,6 @@ FactoryBot/InlineAssociation:
 Graphql/IDType:
   Exclude:
     - 'ee/app/graphql/ee/mutations/issues/update.rb'
-    - 'ee/app/graphql/mutations/iterations/update.rb'
-    - 'ee/app/graphql/resolvers/iterations_resolver.rb'
     - 'app/graphql/mutations/boards/issues/issue_move_list.rb'
     - 'app/graphql/mutations/metrics/dashboard/annotations/delete.rb'
     - 'app/graphql/resolvers/design_management/design_at_version_resolver.rb'

doc/api/graphql/reference/gitlab_schema.graphql

@@ -9159,17 +9159,17 @@ type Group {
     first: Int
 
     """
-    The ID of the Iteration to look up
+    Global ID of the Iteration to look up.
     """
     id: ID
 
     """
-    The internal ID of the Iteration to look up
+    Internal ID of the Iteration to look up.
     """
     iid: ID
 
     """
-    Whether to include ancestor iterations. Defaults to true
+    Whether to include ancestor iterations. Defaults to true.
     """
     includeAncestors: Boolean
 
@@ -9186,7 +9186,7 @@ type Group {
     startDate: Time
 
     """
-    Filter iterations by state
+    Filter iterations by state.
     """
     state: IterationState
 
@@ -9196,7 +9196,7 @@ type Group {
     timeframe: Timeframe
 
     """
-    Fuzzy search by title
+    Fuzzy search by title.
     """
     title: String
   ): IterationConnection
@@ -15843,17 +15843,17 @@ type Project {
     first: Int
 
     """
-    The ID of the Iteration to look up
+    Global ID of the Iteration to look up.
     """
     id: ID
 
     """
-    The internal ID of the Iteration to look up
+    Internal ID of the Iteration to look up.
     """
     iid: ID
 
     """
-    Whether to include ancestor iterations. Defaults to true
+    Whether to include ancestor iterations. Defaults to true.
     """
     includeAncestors: Boolean
 
@@ -15870,7 +15870,7 @@ type Project {
     startDate: Time
 
     """
-    Filter iterations by state
+    Filter iterations by state.
     """
     state: IterationState
 
@@ -15880,7 +15880,7 @@ type Project {
     timeframe: Timeframe
 
     """
-    Fuzzy search by title
+    Fuzzy search by title.
     """
     title: String
   ): IterationConnection
@@ -22557,32 +22557,32 @@ input UpdateIterationInput {
   clientMutationId: String
 
   """
-  The description of the iteration
+  Description of the iteration.
   """
   description: String
 
   """
-  The end date of the iteration
+  End date of the iteration.
   """
   dueDate: String
 
   """
-  The group of the iteration
+  Group of the iteration.
   """
   groupPath: ID!
 
   """
-  The id of the iteration
+  Global ID of the iteration.
   """
   id: ID!
 
   """
-  The start date of the iteration
+  Start date of the iteration.
   """
   startDate: String
 
   """
-  The title of the iteration
+  Title of the iteration.
   """
   title: String
 }
@@ -22602,7 +22602,7 @@ type UpdateIterationPayload {
   errors: [String!]!
 
   """
-  The updated iteration
+  Updated iteration.
   """
   iteration: Iteration
 }

doc/api/graphql/reference/gitlab_schema.json

@@ -25140,7 +25140,7 @@
                 },
                 {
                   "name": "state",
-                  "description": "Filter iterations by state",
+                  "description": "Filter iterations by state.",
                   "type": {
                     "kind": "ENUM",
                     "name": "IterationState",
@@ -25150,7 +25150,7 @@
                 },
                 {
                   "name": "title",
-                  "description": "Fuzzy search by title",
+                  "description": "Fuzzy search by title.",
                   "type": {
                     "kind": "SCALAR",
                     "name": "String",
@@ -25160,7 +25160,7 @@
                 },
                 {
                   "name": "id",
-                  "description": "The ID of the Iteration to look up",
+                  "description": "Global ID of the Iteration to look up.",
                   "type": {
                     "kind": "SCALAR",
                     "name": "ID",
@@ -25170,7 +25170,7 @@
                 },
                 {
                   "name": "iid",
-                  "description": "The internal ID of the Iteration to look up",
+                  "description": "Internal ID of the Iteration to look up.",
                   "type": {
                     "kind": "SCALAR",
                     "name": "ID",
@@ -25180,7 +25180,7 @@
                 },
                 {
                   "name": "includeAncestors",
-                  "description": "Whether to include ancestor iterations. Defaults to true",
+                  "description": "Whether to include ancestor iterations. Defaults to true.",
                   "type": {
                     "kind": "SCALAR",
                     "name": "Boolean",
@@ -46391,7 +46391,7 @@
                 },
                 {
                   "name": "state",
-                  "description": "Filter iterations by state",
+                  "description": "Filter iterations by state.",
                   "type": {
                     "kind": "ENUM",
                     "name": "IterationState",
@@ -46401,7 +46401,7 @@
                 },
                 {
                   "name": "title",
-                  "description": "Fuzzy search by title",
+                  "description": "Fuzzy search by title.",
                   "type": {
                     "kind": "SCALAR",
                     "name": "String",
@@ -46411,7 +46411,7 @@
                 },
                 {
                   "name": "id",
-                  "description": "The ID of the Iteration to look up",
+                  "description": "Global ID of the Iteration to look up.",
                   "type": {
                     "kind": "SCALAR",
                     "name": "ID",
@@ -46421,7 +46421,7 @@
                 },
                 {
                   "name": "iid",
-                  "description": "The internal ID of the Iteration to look up",
+                  "description": "Internal ID of the Iteration to look up.",
                   "type": {
                     "kind": "SCALAR",
                     "name": "ID",
@@ -46431,7 +46431,7 @@
                 },
                 {
                   "name": "includeAncestors",
-                  "description": "Whether to include ancestor iterations. Defaults to true",
+                  "description": "Whether to include ancestor iterations. Defaults to true.",
                   "type": {
                     "kind": "SCALAR",
                     "name": "Boolean",
@@ -65527,7 +65527,7 @@
           "inputFields": [
             {
               "name": "groupPath",
-              "description": "The group of the iteration",
+              "description": "Group of the iteration.",
               "type": {
                 "kind": "NON_NULL",
                 "name": null,
@@ -65541,7 +65541,7 @@
             },
             {
               "name": "id",
-              "description": "The id of the iteration",
+              "description": "Global ID of the iteration.",
               "type": {
                 "kind": "NON_NULL",
                 "name": null,
@@ -65555,7 +65555,7 @@
             },
             {
               "name": "title",
-              "description": "The title of the iteration",
+              "description": "Title of the iteration.",
               "type": {
                 "kind": "SCALAR",
                 "name": "String",
@@ -65565,7 +65565,7 @@
             },
             {
               "name": "description",
-              "description": "The description of the iteration",
+              "description": "Description of the iteration.",
               "type": {
                 "kind": "SCALAR",
                 "name": "String",
@@ -65575,7 +65575,7 @@
             },
             {
               "name": "startDate",
-              "description": "The start date of the iteration",
+              "description": "Start date of the iteration.",
               "type": {
                 "kind": "SCALAR",
                 "name": "String",
@@ -65585,7 +65585,7 @@
             },
             {
               "name": "dueDate",
-              "description": "The end date of the iteration",
+              "description": "End date of the iteration.",
               "type": {
                 "kind": "SCALAR",
                 "name": "String",
@@ -65655,7 +65655,7 @@
             },
             {
               "name": "iteration",
-              "description": "The updated iteration",
+              "description": "Updated iteration.",
               "args": [
 
               ],

doc/api/graphql/reference/index.md

@@ -3392,7 +3392,7 @@ Autogenerated return type of UpdateIteration.
 | ----- | ---- | ----------- |
 | `clientMutationId` | String | A unique identifier for the client performing the mutation. |
 | `errors` | String! => Array | Errors encountered during execution of the mutation. |
-| `iteration` | Iteration | The updated iteration |
+| `iteration` | Iteration | Updated iteration. |
 
 ### UpdateNotePayload
 

ee/app/assets/javascripts/iterations/components/iteration_form.vue

@@ -4,7 +4,6 @@ import { deprecatedCreateFlash as createFlash } from '~/flash';
 import { visitUrl } from '~/lib/utils/url_utility';
 import { __ } from '~/locale';
 import MarkdownField from '~/vue_shared/components/markdown/field.vue';
-import { getIdFromGraphQLId } from '~/graphql_shared/utils';
 import createIteration from '../queries/create_iteration.mutation.graphql';
 import updateIteration from '../queries/update_iteration.mutation.graphql';
 import DueDateSelectors from '~/due_date_select';
@@ -109,7 +108,7 @@ export default {
           variables: {
             input: {
               ...this.variables.input,
-              id: getIdFromGraphQLId(this.iteration.id),
+              id: this.iteration.id,
             },
           },
         })

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

@@ -13,39 +13,42 @@ module Mutations
       field :iteration,
             Types::IterationType,
             null: true,
-            description: 'The updated iteration'
+            description: 'Updated iteration.'
 
       argument :group_path, GraphQL::ID_TYPE,
                required: true,
-               description: "The group of the iteration"
+               description: 'Group of the iteration.'
 
+      # rubocop:disable Graphql/IDType
       argument :id,
                GraphQL::ID_TYPE,
                required: true,
-               description: 'The id of the iteration'
+               description: 'Global ID of the iteration.'
+      # rubocop:enable Graphql/IDType
 
       argument :title,
                GraphQL::STRING_TYPE,
                required: false,
-               description: 'The title of the iteration'
+               description: 'Title of the iteration.'
 
       argument :description,
                GraphQL::STRING_TYPE,
                required: false,
-               description: 'The description of the iteration'
+               description: 'Description of the iteration.'
 
       argument :start_date,
                GraphQL::STRING_TYPE,
                required: false,
-               description: 'The start date of the iteration'
+               description: 'Start date of the iteration.'
 
       argument :due_date,
                GraphQL::STRING_TYPE,
                required: false,
-               description: 'The end date of the iteration'
+               description: 'End date of the iteration.'
 
       def resolve(args)
         validate_arguments!(args)
+        args[:id] = id_from_args(args)
 
         parent = resolve_group(full_path: args[:group_path]).try(:sync)
         iteration = authorized_find!(parent: parent, id: args[:id])
@@ -64,12 +67,21 @@ module Mutations
       private
 
       def find_object(parent:, id:)
-        ::Resolvers::IterationsResolver.new(object: parent, context: context, field: nil).resolve(id: id).items.first
+        ::Resolvers::IterationsResolver.new(object: parent, context: context, field: nil)
+          .resolve(id: id).items.first
       end
 
       def validate_arguments!(args)
         raise Gitlab::Graphql::Errors::ArgumentError, 'The list of iteration attributes is empty' if args.except(:group_path, :id).empty?
       end
+
+      # Originally accepted a raw model id. Now accept a gid, but allow a raw id
+      # for backward compatibility
+      def id_from_args(args)
+        GitlabSchema.parse_gid(args[:id], expected_type: ::Iteration)
+      rescue Gitlab::Graphql::Errors::ArgumentError
+        ::Gitlab::GlobalId.as_global_id(args[:id].to_i, model_name: 'Iteration')
+      end
     end
   end
 end

ee/app/graphql/resolvers/iterations_resolver.rb

@@ -7,27 +7,32 @@ module Resolvers
 
     argument :state, Types::IterationStateEnum,
              required: false,
-             description: 'Filter iterations by state'
+             description: 'Filter iterations by state.'
     argument :title, GraphQL::STRING_TYPE,
              required: false,
-             description: 'Fuzzy search by title'
+             description: 'Fuzzy search by title.'
+
+    # rubocop:disable Graphql/IDType
     argument :id, GraphQL::ID_TYPE,
              required: false,
-             description: 'The ID of the Iteration to look up'
+             description: 'Global ID of the Iteration to look up.'
+    # rubocop:enable Graphql/IDType
+
     argument :iid, GraphQL::ID_TYPE,
              required: false,
-             description: 'The internal ID of the Iteration to look up'
+             description: 'Internal ID of the Iteration to look up.'
     argument :include_ancestors, GraphQL::BOOLEAN_TYPE,
              required: false,
-             description: 'Whether to include ancestor iterations. Defaults to true'
+             description: 'Whether to include ancestor iterations. Defaults to true.'
 
-    type Types::IterationType, null: true
+    type Types::IterationType.connection_type, null: true
 
     def resolve(**args)
       validate_timeframe_params!(args)
 
       authorize!
 
+      args[:id] = id_from_args(args)
       args[:include_ancestors] = true if args[:include_ancestors].nil? && args[:iid].nil?
 
       iterations = IterationsFinder.new(context[:current_user], iterations_finder_params(args)).execute
@@ -58,5 +63,15 @@ module Resolvers
     def authorize!
       Ability.allowed?(context[:current_user], :read_iteration, parent) || raise_resource_not_available_error!
     end
+
+    # Originally accepted a raw model id. Now accept a gid, but allow a raw id
+    # for backward compatibility
+    def id_from_args(args)
+      return unless args[:id].present?
+
+      GitlabSchema.parse_gid(args[:id], expected_type: ::Iteration).model_id
+    rescue Gitlab::Graphql::Errors::ArgumentError
+      args[:id]
+    end
   end
 end

ee/spec/frontend/iterations/components/iteration_form_spec.js

@@ -186,7 +186,7 @@ describe('Iteration Form', () => {
         variables: {
           input: {
             groupPath,
-            id,
+            id: iteration.id,
             title,
             description,
             startDate,

ee/spec/graphql/resolvers/iterations_resolver_spec.rb

@@ -43,13 +43,23 @@ RSpec.describe Resolvers::IterationsResolver do
           start_date = now
           end_date = start_date + 1.hour
           search = 'wow'
-          id = 1
+          id = '1'
           iid = 2
           params = { id: id, iid: iid, group_ids: group.id, state: 'closed', start_date: start_date, end_date: end_date, search_title: search }
 
           expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 
-          resolve_group_iterations(start_date: start_date, end_date: end_date, state: 'closed', title: search, id: id, iid: iid)
+          resolve_group_iterations(start_date: start_date, end_date: end_date, state: 'closed', title: search, id: 'gid://gitlab/Iteration/1', iid: iid)
+        end
+
+        it 'accepts a raw model id for backward compatibility' do
+          id = 1
+          iid = 2
+          params = { id: id, iid: iid, group_ids: group.id, state: 'all', start_date: nil, end_date: nil, search_title: nil }
+
+          expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
+
+          resolve_group_iterations(id: id, iid: iid)
         end
       end
 

ee/spec/requests/api/graphql/iteration_spec.rb

@@ -50,6 +50,7 @@ RSpec.describe 'Querying an Iteration' do
     let_it_be(:project_iteration) { create(:iteration, :skip_project_validation, project: project) }
 
     shared_examples 'scoped path' do
+      let(:queried_iteration_id) { queried_iteration.to_global_id.to_s }
       let(:iteration_nodes) do
         nodes = <<~NODES
           nodes {
@@ -58,7 +59,7 @@ RSpec.describe 'Querying an Iteration' do
           }
         NODES
 
-        query_graphql_field('iterations', { id: queried_iteration.id }, nodes)
+        query_graphql_field('iterations', { id: queried_iteration_id }, nodes)
       end
 
       before_all do
@@ -68,6 +69,14 @@ RSpec.describe 'Querying an Iteration' do
       specify do
         expect(subject).to include('scopedPath' => expected_scope_path, 'scopedUrl' => expected_scope_url)
       end
+
+      context 'when given a raw model id (backward compatibility)' do
+        let(:queried_iteration_id) { queried_iteration.id }
+
+        specify do
+          expect(subject).to include('scopedPath' => expected_scope_path, 'scopedUrl' => expected_scope_url)
+        end
+      end
     end
 
     context 'inside a project context' do

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

@@ -21,7 +21,7 @@ RSpec.describe 'Updating an Iteration' do
   end
 
   let(:mutation) do
-    params = { group_path: group.full_path, id: iteration.id }.merge(attributes)
+    params = { group_path: group.full_path, id: iteration.to_global_id.to_s }.merge(attributes)
 
     graphql_mutation(:update_iteration, params)
   end
@@ -53,9 +53,12 @@ RSpec.describe 'Updating an Iteration' do
         stub_licensed_features(iterations: false)
       end
 
-      it_behaves_like 'a mutation that returns top-level errors',
-                      errors: ['The resource that you are attempting to access does not '\
-                 'exist or you don\'t have permission to perform this action']
+      it_behaves_like 'a mutation that returns top-level errors' do
+        let(:match_errors) do
+          include('The resource that you are attempting to access does not '\
+                  'exist or you don\'t have permission to perform this action')
+        end
+      end
     end
 
     context 'when iterations are enabled' do
@@ -92,6 +95,18 @@ RSpec.describe 'Updating an Iteration' do
         end
       end
 
+      context 'when given a raw model id (backward compatibility)' do
+        let(:attributes) { { id: iteration.id, title: 'title' } }
+
+        it 'updates the iteration' do
+          post_graphql_mutation(mutation, current_user: current_user)
+
+          iteration_hash = mutation_response['iteration']
+          expect(iteration_hash['title']).to eq('title')
+          expect(iteration.reload.title).to eq('title')
+        end
+      end
+
       context 'when the list of attributes is empty' do
         let(:attributes) { {} }