Merge branch 'cablett-329617-graphql-nullable-args' into 'master'

Add optional `nullable` declaration to GraphQL arguments

See merge request gitlab-org/gitlab!66618

app/graphql/mutations/base_mutation.rb

@@ -31,6 +31,12 @@ module Mutations
     def ready?(**args)
       raise_resource_not_available_error! ERROR_MESSAGE if Gitlab::Database.read_only?
 
+      missing_args = self.class.arguments.values
+        .reject { |arg| arg.accepts?(args.fetch(arg.keyword, :not_given)) }
+        .map(&:graphql_name)
+
+      raise ArgumentError, "Arguments must be provided: #{missing_args.join(", ")}" if missing_args.any?
+
       true
     end
 

app/graphql/mutations/issues/set_due_date.rb

@@ -7,17 +7,8 @@ module Mutations
 
       argument :due_date,
                Types::TimeType,
-               required: false,
-               description: 'The desired due date for the issue, ' \
-               'due date will be removed if absent or set to null'
-
-      def ready?(**args)
-        unless args.key?(:due_date)
-          raise Gitlab::Graphql::Errors::ArgumentError, 'Argument dueDate must be provided (`null` accepted)'
-        end
-
-        super
-      end
+               required: :nullable,
+               description: 'The desired due date for the issue. Due date is removed if null.'
 
       def resolve(project_path:, iid:, due_date:)
         issue = authorized_find!(project_path: project_path, iid: iid)

app/graphql/types/base_argument.rb

@@ -10,7 +10,29 @@ module Types
       @deprecation = gitlab_deprecation(kwargs)
       @doc_reference = kwargs.delete(:see)
 
+      # our custom addition `nullable` which allows us to declare
+      # an argument that must be provided, even if its value is null.
+      # When `required: true` then required arguments must not be null.
+      @gl_required = !!kwargs[:required]
+      @gl_nullable = kwargs[:required] == :nullable
+
+      # Only valid if an argument is also required.
+      if @gl_nullable
+        # Since the framework asserts that "required" means "cannot be null"
+        # we have to switch off "required" but still do the check in `ready?` behind the scenes
+        kwargs[:required] = false
+      end
+
       super(*args, **kwargs, &block)
     end
+
+    def accepts?(value)
+      # if the argument is declared as required, it must be included
+      return false if @gl_required && value == :not_given
+      # if the argument is declared as required, the value can only be null IF it is also nullable.
+      return false if @gl_required && value.nil? && !@gl_nullable
+
+      true
+    end
   end
 end

doc/api/graphql/reference/index.md

@@ -2533,7 +2533,7 @@ Input type: `IssueSetDueDateInput`
 | Name | Type | Description |
 | ---- | ---- | ----------- |
 | <a id="mutationissuesetduedateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
-| <a id="mutationissuesetduedateduedate"></a>`dueDate` | [`Time`](#time) | The desired due date for the issue, due date will be removed if absent or set to null. |
+| <a id="mutationissuesetduedateduedate"></a>`dueDate` | [`Time`](#time) | The desired due date for the issue. Due date is removed if null. |
 | <a id="mutationissuesetduedateiid"></a>`iid` | [`String!`](#string) | The IID of the issue to mutate. |
 | <a id="mutationissuesetduedateprojectpath"></a>`projectPath` | [`ID!`](#id) | The project the issue to mutate is in. |
 

doc/development/api_graphql_styleguide.md

@@ -1366,6 +1366,31 @@ argument :my_arg, GraphQL::Types::String,
          description: "A description of the argument."
 ```
 
+#### Nullability
+
+Arguments can be marked as `required: true` which means the value must be present and not `null`.
+If a required argument's value can be `null`, use the `required: :nullable` declaration.
+
+Example:
+
+```ruby
+argument :due_date,
+         Types::TimeType,
+         required: :nullable,
+         description: 'The desired due date for the issue. Due date is removed if null.'
+```
+
+In the above example, the `due_date` argument must be given, but unlike the GraphQL spec, the value can be `null`.
+This allows 'unsetting' the due date in a single mutation rather than creating a new mutation for removing the due date.
+
+```ruby
+{ due_date: null } # => OK
+{ due_date: "2025-01-10" } # => OK
+{  } # => invalid (not given)
+```
+
+#### Keywords
+
 Each GraphQL `argument` defined is passed to the `#resolve` method
 of a mutation as keyword arguments.
 
@@ -1377,6 +1402,8 @@ def resolve(my_arg:)
 end
 ```
 
+#### Input Types
+
 `graphql-ruby` wraps up arguments into an
 [input type](https://graphql.org/learn/schema/#input-types).
 

spec/graphql/mutations/base_mutation_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Mutations::BaseMutation do
+  include GraphqlHelpers
+
+  describe 'argument nullability' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:context)  { { current_user: user } }
+
+    subject(:mutation) { mutation_class.new(object: nil, context: context, field: nil) }
+
+    describe 'when using a mutation with correct argument declarations' do
+      context 'when argument is nullable and required' do
+        let(:mutation_class) do
+          Class.new(described_class) do
+            argument :foo, GraphQL::STRING_TYPE, required: :nullable
+          end
+        end
+
+        specify do
+          expect { subject.ready? }.to raise_error(ArgumentError, /must be provided: foo/)
+        end
+
+        specify do
+          expect { subject.ready?(foo: nil) }.not_to raise_error
+        end
+
+        specify do
+          expect { subject.ready?(foo: "bar") }.not_to raise_error
+        end
+      end
+
+      context 'when argument is required and NOT nullable' do
+        let(:mutation_class) do
+          Class.new(described_class) do
+            argument :foo, GraphQL::STRING_TYPE, required: true
+          end
+        end
+
+        specify do
+          expect { subject.ready? }.to raise_error(ArgumentError, /must be provided/)
+        end
+
+        specify do
+          expect { subject.ready?(foo: nil) }.to raise_error(ArgumentError, /must be provided/)
+        end
+
+        specify do
+          expect { subject.ready?(foo: "bar") }.not_to raise_error
+        end
+      end
+    end
+  end
+end

spec/graphql/mutations/ci/runner/delete_spec.rb

@@ -41,7 +41,7 @@ RSpec.describe Mutations::Ci::Runner::Delete do
       let(:mutation_params) { {} }
 
       it 'raises an error' do
-        expect { subject }.to raise_error(ArgumentError, "missing keyword: :id")
+        expect { subject }.to raise_error(ArgumentError, "Arguments must be provided: id")
       end
     end
 

spec/graphql/mutations/ci/runner/update_spec.rb

@@ -43,7 +43,7 @@ RSpec.describe Mutations::Ci::Runner::Update do
       let(:mutation_params) { {} }
 
       it 'raises an error' do
-        expect { subject }.to raise_error(ArgumentError, "missing keyword: :id")
+        expect { subject }.to raise_error(ArgumentError, "Arguments must be provided: id")
       end
     end
 

spec/graphql/types/base_argument_spec.rb

@@ -3,15 +3,41 @@
 require 'spec_helper'
 
 RSpec.describe Types::BaseArgument do
-  include_examples 'Gitlab-style deprecations' do
-    let_it_be(:field) do
-      Types::BaseField.new(name: 'field', type: String, null: true)
+  let_it_be(:field) do
+    Types::BaseField.new(name: 'field', type: String, null: true)
+  end
+
+  let(:base_args) { { name: 'test', type: String, required: false, owner: field } }
+
+  def subject(args = {})
+    described_class.new(**base_args.merge(args))
+  end
+
+  include_examples 'Gitlab-style deprecations'
+
+  describe 'required argument declarations' do
+    it 'accepts nullable, required arguments' do
+      arguments = base_args.merge({ required: :nullable })
+
+      expect { subject(arguments) }.not_to raise_error
+    end
+
+    it 'accepts required, non-nullable arguments' do
+      arguments = base_args.merge({ required: true })
+
+      expect { subject(arguments) }.not_to raise_error
+    end
+
+    it 'accepts non-required arguments' do
+      arguments = base_args.merge({ required: false })
+
+      expect { subject(arguments) }.not_to raise_error
     end
 
-    let(:base_args) { { name: 'test', type: String, required: false, owner: field } }
+    it 'accepts no required argument declaration' do
+      arguments = base_args
 
-    def subject(args = {})
-      described_class.new(**base_args.merge(args))
+      expect { subject(arguments) }.not_to raise_error
     end
   end
 end

spec/requests/api/graphql/mutations/issues/set_due_date_spec.rb

@@ -68,7 +68,7 @@ RSpec.describe 'Setting Due Date of an issue' do
     it 'returns an error' do
       post_graphql_mutation(mutation, current_user: current_user)
 
-      expect(graphql_errors).to include(a_hash_including('message' => /Argument dueDate must be provided/))
+      expect(graphql_errors).to include(a_hash_including('message' => /Arguments must be provided: dueDate/))
     end
   end