From e755a3513a5c39e59086f1220caed168df37a2e5 Mon Sep 17 00:00:00 2001 From: Scott Walkinshaw Date: Wed, 29 Mar 2023 10:12:01 -0400 Subject: [PATCH] Fix returning [Type, false] from resolve_type https://github.com/rmosolgo/graphql-ruby/pull/4130 fixed returning `[Type, nil]` from a union's `resolve_type`, but it caused a regression for literal `false` values. `Schema#resolve_type` had a `if resolved_value` conditional would be false for `false` values causing it _always_ return a single value (the `resolve_type`). The runtime's `resolve_type` method would then _always_ return the tuple version (in non-lazy cases): `[type, value]`. However, since `Schema#resolve_type` was only returning a single value, the resolved value would be lost resulting in `[type, nil]` being passed on. The fix is to remove the `if resolved_value` conditional entirely and always return the two-value tuple ensuring the original resolved value is correctly passed along. Note: the runtime still has a branch to check if the `resolve_type` value is a tuple or not. The only case where this won't be true is for lazy resolved types. --- lib/graphql/schema.rb | 6 +- spec/graphql/schema/union_spec.rb | 135 ++++++++++++++---- spec/integration/rails/graphql/schema_spec.rb | 2 +- 3 files changed, 109 insertions(+), 34 deletions(-) diff --git a/lib/graphql/schema.rb b/lib/graphql/schema.rb index 6bad880f66..f0a6372eab 100644 --- a/lib/graphql/schema.rb +++ b/lib/graphql/schema.rb @@ -799,11 +799,7 @@ def resolve_type(type, obj, ctx) end if resolved_type.nil? || (resolved_type.is_a?(Module) && resolved_type.respond_to?(:kind)) - if resolved_value - [resolved_type, resolved_value] - else - resolved_type - end + [resolved_type, resolved_value] else raise ".resolve_type should return a type definition, but got #{resolved_type.inspect} (#{resolved_type.class}) from `resolve_type(#{type}, #{obj}, #{ctx})`" end diff --git a/spec/graphql/schema/union_spec.rb b/spec/graphql/schema/union_spec.rb index 5724634d7c..d431314b6a 100644 --- a/spec/graphql/schema/union_spec.rb +++ b/spec/graphql/schema/union_spec.rb @@ -67,7 +67,7 @@ assert_equal "Fragment on Ensemble can't be spread inside PerformingAct", res.to_h["errors"].first["message"] end - describe "two-value type resolution" do + describe "type resolution" do Box = Struct.new(:value) class Schema < GraphQL::Schema @@ -79,12 +79,32 @@ class B < GraphQL::Schema::Object field :b, String, method: :itself end - class MyUnion < GraphQL::Schema::Union - possible_types A, B + class C < GraphQL::Schema::Object + field :c, Boolean, method: :itself + end + + class UnboxedUnion < GraphQL::Schema::Union + possible_types A, C def self.resolve_type(object, ctx) - if object.value == "return-nil" + case object + when FalseClass + C + else + A + end + end + end + + class BoxedUnion < GraphQL::Schema::Union + possible_types A, B, C + + def self.resolve_type(object, ctx) + case object.value + when "return-nil" [B, nil] + when FalseClass + [C, object.value] else [A, object.value] end @@ -92,47 +112,106 @@ def self.resolve_type(object, ctx) end class Query < GraphQL::Schema::Object - field :my_union, MyUnion + field :boxed_union, BoxedUnion - def my_union + def boxed_union Box.new(context[:value]) end + + field :unboxed_union, UnboxedUnion + + def unboxed_union + context[:value] + end end query(Query) end - it "can cast the object after resolving the type" do + describe "two-value resolution" do + it "can cast the object after resolving the type" do - query_str = <<-GRAPHQL - { - myUnion { - ... on A { a } + query_str = <<-GRAPHQL + { + boxedUnion { + ... on A { a } + } } - } - GRAPHQL + GRAPHQL + + res = Schema.execute(query_str, context: { value: "unwrapped" }) + + assert_equal({ + 'data' => { 'boxedUnion' => { 'a' => 'unwrapped' } } + }, res.to_h) + end + + it "uses `false` when returned from resolve_type" do + query_str = <<-GRAPHQL + { + boxedUnion { + ... on C { c } + } + } + GRAPHQL + + res = Schema.execute(query_str, context: { value: false }) - res = Schema.execute(query_str, context: { value: "unwrapped" }) + assert_equal({ + 'data' => { 'boxedUnion' => { 'c' => false } } + }, res.to_h) + end - assert_equal({ - 'data' => { 'myUnion' => { 'a' => 'unwrapped' } } - }, res.to_h) + it "uses `nil` when returned from resolve_type" do + query_str = <<-GRAPHQL + { + boxedUnion { + ... on B { b } + } + } + GRAPHQL + + res = Schema.execute(query_str, context: { value: "return-nil" }) + + assert_equal({ + 'data' => { 'boxedUnion' => { 'b' => nil } } + }, res.to_h) + end end - it "uses `nil` when returned from resolve_type" do - query_str = <<-GRAPHQL - { - myUnion { - ... on B { b } + describe "single-value resolution" do + it "can cast the object after resolving the type" do + + query_str = <<-GRAPHQL + { + unboxedUnion { + ... on A { a } + } } - } - GRAPHQL + GRAPHQL - res = Schema.execute(query_str, context: { value: "return-nil" }) + res = Schema.execute(query_str, context: { value: "string" }) - assert_equal({ - 'data' => { 'myUnion' => { 'b' => nil } } - }, res.to_h) + assert_equal({ + 'data' => { 'unboxedUnion' => { 'a' => 'string' } } + }, res.to_h) + end + + it "works with literal false values" do + query_str = <<-GRAPHQL + { + unboxedUnion { + ... on C { c } + } + } + GRAPHQL + + res = Schema.execute(query_str, context: { value: false }) + + assert_equal({ + 'data' => { 'unboxedUnion' => { 'c' => false } } + }, res.to_h) + end end end end diff --git a/spec/integration/rails/graphql/schema_spec.rb b/spec/integration/rails/graphql/schema_spec.rb index 815842c279..3e6b2902cc 100644 --- a/spec/integration/rails/graphql/schema_spec.rb +++ b/spec/integration/rails/graphql/schema_spec.rb @@ -61,7 +61,7 @@ describe "when the return value is nil" do it "returns nil" do result = relay_schema.resolve_type(123, nil, GraphQL::Query::NullContext) - assert_equal(nil, result) + assert_equal([nil, nil], result) end end