Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate that deprecated requirements aren't required #3137

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions lib/graphql/schema/argument.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
validate_deprecated_or_optional(null: @null, deprecation_reason: text)
@deprecation_reason = text
else
@deprecation_reason
end
end

alias_method :deprecation_reason=, :deprecation_reason

def visible?(context)
true
end
Expand Down Expand Up @@ -164,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?)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity what's the expected relationship between new_type.non_null? and @null? Think we should validate that the two are consistent?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure... There are hacks that work like argument(name, type_string, required: false), where type_string may include !. For example, that's how parsing a schema from SDL works; The SDL loader code just passes the type string along as-is, with required: false:

type: type_resolver.call(argument_defn.type),
required: false,

As for @null, I consider it a private implementation detail of Argument. To find out if an argument's type is non-null, you can call argument.type.non_null? (with the exception noted here: GraphQL internal types don't have that available right away -- but by the time you run a query, argument.type.non_null? will work).

validate_deprecated_or_optional(null: !new_type.non_null?, deprecation_reason: deprecation_reason)
end
@type = new_type
end

Expand All @@ -174,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

Expand Down Expand Up @@ -212,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
Expand All @@ -223,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
46 changes: 46 additions & 0 deletions spec/graphql/schema/argument_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -283,6 +285,50 @@ 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

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
Expand Down