Skip to content

Commit

Permalink
fix(Arguments) use the type's coerce method, raise an error if the co…
Browse files Browse the repository at this point in the history
…ercion fails
  • Loading branch information
rmosolgo committed Oct 6, 2015
1 parent 86c3d6d commit fa7a2db
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 4 deletions.
14 changes: 14 additions & 0 deletions lib/graphql/base_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,5 +65,19 @@ def to_s
end

alias :inspect :to_s

# Coerce `input_value` according to this type's `coerce` method.
# Raise an error if the value becomes nil.
# @param [Object] Incoming query value
# @return [Object] Coerced value for query execution
def coerce!(input_value)

This comment has been minimized.

Copy link
@dylanahsmith

dylanahsmith Oct 6, 2015

Contributor

It seems like the coerce methods should be the ones that raise, that way it can provide an appropriate error message. Also, nil might be an acceptable value, unless it is a non-null type.

This comment has been minimized.

Copy link
@rmosolgo

rmosolgo Oct 6, 2015

Author Owner

I think there's no such thing as null input for graphql. Although there is an open PR for it, then this will need a refactor: graphql/graphql-spec#83

This comment has been minimized.

Copy link
@dylanahsmith

dylanahsmith Oct 7, 2015

Contributor

There is no such things as a null literal, but variables could still have a null value, it just gets handled as if the argument is missing. However, the behaviour in the spec isn't very clear about variable validation, which is why graphql-js doesn't properly validate variables (graphql/graphql-js#187). It even says

When a Non Null input has its value set using a variable, the coerced value should be null if the provided value is null-like in the provided representation, or if the provided value is omitted. Otherwise, the coerced value is the result of running the wrapped type’s input coercion on the provided value.

which oddly doesn't mention anything about an error even though that is a description of input coercion for a non-null type. In fact, it looks like graphql-js does exactly this, assuming variables have been validated when they haven't been (variables values aren't even passed into the validate function).

coerced_value = coerce(input_value)

if coerced_value.nil?
raise GraphQL::ExecutionError.new("Couldn't coerce #{input_value.inspect} to #{self.unwrap.name}")
end

coerced_value
end
end
end
10 changes: 7 additions & 3 deletions lib/graphql/query/arguments.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@ def initialize(ast_arguments, argument_hash, variables)
end
end

def_delegators :@hash, :keys, :values, :inspect, :to_h
def_delegators :@hash, :keys, :values, :inspect, :to_h, :key?, :has_key?

# Find an argument by name.
# (Coerce to strings because we use strings internally.)
# @param [String, Symbol] Argument name to access
def [](key)
@hash[key.to_s]
end
Expand All @@ -21,9 +24,10 @@ def [](key)

def reduce_value(value, arg_defn, variables)
if value.is_a?(GraphQL::Language::Nodes::VariableIdentifier)
value = variables[value.name]
raw_value = variables[value.name]
value = arg_defn.type.coerce!(raw_value)
elsif value.is_a?(GraphQL::Language::Nodes::Enum)
value = arg_defn.type.coerce(value.name)
value = arg_defn.type.coerce!(value.name)
elsif value.is_a?(GraphQL::Language::Nodes::InputObject)
wrapped_type = arg_defn.type.unwrap
value = self.class.new(value.pairs, wrapped_type.input_fields, variables)
Expand Down
27 changes: 26 additions & 1 deletion spec/graphql/query_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@
|}
let(:debug) { false }
let(:operation_name) { nil }
let(:query_variables) { {"cheeseId" => 2} }
let(:query) { GraphQL::Query.new(
DummySchema,
query_string,
variables: {"cheeseId" => 2},
variables: query_variables,
debug: debug,
operation_name: operation_name,
)}
Expand Down Expand Up @@ -194,4 +195,28 @@
end
end
end

describe "query variables" do
let(:query_string) {%|
query getCheese($cheeseId: Int!){
cheese(id: $cheeseId) { flavor }
}
|}

describe "when they can be coerced" do
let(:query_variables) { {"cheeseId" => 2.0} }

it "coerces them on the way in" do
assert("Gouda", result["data"]["cheese"]["flavor"])
end
end

describe "when they can't be coerced" do
let(:query_variables) { {"cheeseId" => "2"} }

This comment has been minimized.

Copy link
@lifeiscontent

lifeiscontent Jan 29, 2020

@rmosolgo why on earth wouldn't this be converted to an int? JSON.parse("2") works...


it "raises an error" do
assert(result["errors"][0]["message"].include?(%{Couldn't coerce "2" to Int}))
end
end
end
end

0 comments on commit fa7a2db

Please sign in to comment.