From 233a169766716a5c6ea50ec21ece20205662c7f0 Mon Sep 17 00:00:00 2001 From: Joel Turkel Date: Tue, 8 Sep 2020 11:47:54 -0400 Subject: [PATCH 1/2] Validate that deprecated requirements aren't required --- lib/graphql/schema/argument.rb | 7 ++++--- spec/graphql/schema/argument_spec.rb | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/lib/graphql/schema/argument.rb b/lib/graphql/schema/argument.rb index 0fd9e37d70..6d38d985f7 100644 --- a/lib/graphql/schema/argument.rb +++ b/lib/graphql/schema/argument.rb @@ -61,7 +61,7 @@ def initialize(arg_name = nil, type_expr = nil, desc = nil, required:, type: nil @ast_node = ast_node @from_resolver = from_resolver @method_access = method_access - @deprecation_reason = deprecation_reason + self.deprecation_reason = deprecation_reason if definition_block if definition_block.arity == 1 @@ -91,17 +91,18 @@ def description(text = nil) end end - attr_writer :deprecation_reason - # @return [String] Deprecation reason for this argument def deprecation_reason(text = nil) if text + raise ArgumentError, "Required arguments cannot be deprecated: #{path}." unless @null @deprecation_reason = text else @deprecation_reason end end + alias_method :deprecation_reason=, :deprecation_reason + def visible?(context) true end diff --git a/spec/graphql/schema/argument_spec.rb b/spec/graphql/schema/argument_spec.rb index 752442d50e..04e53153ca 100644 --- a/spec/graphql/schema/argument_spec.rb +++ b/spec/graphql/schema/argument_spec.rb @@ -270,6 +270,8 @@ def self.object_from_id(id, ctx) describe "deprecation_reason:" do let(:arg) { SchemaArgumentTest::Query.fields["field"].arguments["arg"] } + let(:required_arg) { SchemaArgumentTest::Query.fields["field"].arguments["requiredWithDefaultArg"] } + it "sets deprecation reason" do arg.deprecation_reason "new deprecation reason" assert_equal "new deprecation reason", arg.deprecation_reason @@ -283,6 +285,28 @@ def self.object_from_id(id, ctx) arg.deprecation_reason = "another new deprecation reason" assert_equal "another new deprecation reason", arg.deprecation_reason end + + it "disallows deprecating required arguments in the constructor" do + err = assert_raises ArgumentError do + Class.new(GraphQL::Schema::InputObject) do + graphql_name 'MyInput' + argument :foo, String, required: true, deprecation_reason: "Don't use me" + end + end + assert_equal "Required arguments cannot be deprecated: MyInput.foo.", err.message + end + + it "disallows deprecating required arguments in deprecation_reason=" do + assert_raises ArgumentError do + required_arg.deprecation_reason = "Don't use me" + end + end + + it "disallows deprecating required arguments in deprecation_reason" do + assert_raises ArgumentError do + required_arg.deprecation_reason("Don't use me") + end + end end describe "invalid input types" do From 5cd54b324d876d06f9b83385a812007fda8b1339 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Wed, 9 Sep 2020 08:49:30 -0400 Subject: [PATCH 2/2] Also validate arguments when types are assigned later --- lib/graphql/schema/argument.rb | 19 ++++++++++++++++--- spec/graphql/schema/argument_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/lib/graphql/schema/argument.rb b/lib/graphql/schema/argument.rb index 6d38d985f7..5047232881 100644 --- a/lib/graphql/schema/argument.rb +++ b/lib/graphql/schema/argument.rb @@ -94,7 +94,7 @@ def description(text = nil) # @return [String] Deprecation reason for this argument def deprecation_reason(text = nil) if text - raise ArgumentError, "Required arguments cannot be deprecated: #{path}." unless @null + validate_deprecated_or_optional(null: @null, deprecation_reason: text) @deprecation_reason = text else @deprecation_reason @@ -165,6 +165,11 @@ def to_graphql def type=(new_type) validate_input_type(new_type) + # This isn't true for LateBoundTypes, but we can assume those will + # be updated via this codepath later in schema setup. + if new_type.respond_to?(:non_null?) + validate_deprecated_or_optional(null: !new_type.non_null?, deprecation_reason: deprecation_reason) + end @type = new_type end @@ -175,8 +180,8 @@ def type rescue StandardError => err raise ArgumentError, "Couldn't build type for Argument #{@owner.name}.#{name}: #{err.class.name}: #{err.message}", err.backtrace end - validate_input_type(parsed_type) - parsed_type + # Use the setter method to get validations + self.type = parsed_type end end @@ -213,6 +218,8 @@ def prepare_value(obj, value, context: nil) end end + private + def validate_input_type(input_type) if input_type.is_a?(String) || input_type.is_a?(GraphQL::Schema::LateBoundType) # Do nothing; assume this will be validated later @@ -224,6 +231,12 @@ def validate_input_type(input_type) # It's an input type, we're OK end end + + def validate_deprecated_or_optional(null:, deprecation_reason:) + if deprecation_reason && !null + raise ArgumentError, "Required arguments cannot be deprecated: #{path}." + end + end end end end diff --git a/spec/graphql/schema/argument_spec.rb b/spec/graphql/schema/argument_spec.rb index 04e53153ca..1b02665ffe 100644 --- a/spec/graphql/schema/argument_spec.rb +++ b/spec/graphql/schema/argument_spec.rb @@ -307,6 +307,28 @@ def self.object_from_id(id, ctx) required_arg.deprecation_reason("Don't use me") end end + + it "disallows deprecated required arguments whose type is a string" do + input_obj = Class.new(GraphQL::Schema::InputObject) do + graphql_name 'MyInput2' + argument :foo, "String!", required: false, deprecation_reason: "Don't use me" + end + + query_type = Class.new(GraphQL::Schema::Object) do + graphql_name "Query" + field :f, String, null: true do + argument :arg, input_obj, required: false + end + end + + err = assert_raises ArgumentError do + Class.new(GraphQL::Schema) do + query(query_type) + end + end + + assert_equal "Required arguments cannot be deprecated: MyInput2.foo.", err.message + end end describe "invalid input types" do