Merge pull request rmosolgo#4869 from rmosolgo/validate-orphan-types

Validate incoming orphan_types are object types

Author: rmosolgo

lib/graphql/schema.rb

@@ -861,6 +861,17 @@ def extra_types(*new_extra_types)
       def orphan_types(*new_orphan_types)
         if new_orphan_types.any?
           new_orphan_types = new_orphan_types.flatten
+          non_object_types = new_orphan_types.reject { |ot| ot.is_a?(Class) && ot < GraphQL::Schema::Object }
+          if non_object_types.any?
+            raise ArgumentError, <<~ERR
+              Only object type classes should be added as `orphan_types(...)`.
+
+              - Remove these no-op types from `orphan_types`: #{non_object_types.map { |t| "#{t.inspect} (#{t.kind.name})"}.join(", ")}
+              - See https://graphql-ruby.org/type_definitions/interfaces.html#orphan-types
+
+              To add other types to your schema, you might want `extra_types`: https://graphql-ruby.org/schema/definition.html#extra-types
+            ERR
+          end
           add_type_and_traverse(new_orphan_types, root: false)
           own_orphan_types.concat(new_orphan_types.flatten)
         end

lib/graphql/schema/build_from_definition.rb

@@ -120,10 +120,12 @@ def build(schema_superclass, document, default_resolve:, using: {}, relay:)
 
           builder = self
 
+          found_types = types.values
           schema_class = Class.new(schema_superclass) do
             begin
               # Add these first so that there's some chance of resolving late-bound types
-              orphan_types types.values
+              add_type_and_traverse(found_types, root: false)
+              orphan_types(found_types.select { |t| t.respond_to?(:kind) && t.kind.object? })
               query query_root_type
               mutation mutation_root_type
               subscription subscription_root_type

lib/graphql/schema/loader.rb

@@ -32,7 +32,8 @@ def load(introspection_result)
         end
 
         Class.new(GraphQL::Schema) do
-          orphan_types(types.values)
+          add_type_and_traverse(types.values, root: false)
+          orphan_types(types.values.select { |t| t.kind.object? })
           directives(directives)
           description(schema["description"])
 

spec/graphql/introspection/schema_type_spec.rb

@@ -32,6 +32,7 @@
             {"name"=>"dairy"},
             {"name"=>"deepNonNull"},
             {"name"=>"error"},
+            {"name"=>"exampleBeverage"},
             {"name"=>"executionError"},
             {"name"=>"executionErrorWithExtensions"},
             {"name"=>"executionErrorWithOptions"},

spec/graphql/schema/dynamic_members_spec.rb

@@ -329,6 +329,10 @@ def actor
       def yell(scream:)
         scream
       end
+
+      # just to attach these to the schema:
+      field :example_locale, Locale
+      field :example_region, Region
     end
 
     class BaseMutation < GraphQL::Schema::RelayClassicMutation
@@ -364,7 +368,7 @@ class Mutation < BaseObject
 
     query(Query)
     mutation(Mutation)
-    orphan_types(Place, LegacyPlace, Locale, Region, Country)
+    orphan_types(Place, LegacyPlace, Country)
 
     def self.object_from_id(id, ctx)
       { id: id, database_id: id, uuid: "thing-#{id}", legacy_price: "⚛︎#{id}00", price: { amount: id.to_i * 100, currency: "⚛︎" }}
@@ -487,6 +491,8 @@ def legacy_schema_sdl
 type Query {
   actor: Actor
   add(left: Float!, right: Float!): String!
+  exampleLocale: Locale
+  exampleRegion: Region
   f1: String
   favoriteLanguage(lang: Language): Language!
   legacyThing(id: ID!): LegacyThing!
@@ -890,7 +896,6 @@ def thing
       end
 
       query(Query)
-      orphan_types(ThingScalar, ThingEnum, ThingInput, ThingObject, ThingUnion, ThingInterface)
     end
   end
 

spec/graphql/schema/printer_spec.rb

@@ -92,6 +92,8 @@ class Query < GraphQL::Schema::Object
       field :no_fields_type, NoFields do
         argument :no_arguments_input, NoArguments
       end
+
+      field :example_media, Media
     end
 
     class CreatePost < GraphQL::Schema::RelayClassicMutation
@@ -114,7 +116,6 @@ class Subscription < GraphQL::Schema::Object
     query(Query)
     mutation(Mutation)
     subscription(Subscription)
-    orphan_types [Media]
     extra_types [MediaRating]
   end
 
@@ -588,6 +589,7 @@ class Subscription < GraphQL::Schema::Object
 The query root of this schema
 """
 type Query {
+  exampleMedia: Media
   noFieldsType(noArgumentsInput: NoArguments!): NoFields
   post(
     deprecatedArg: String @deprecated(reason: "Use something else")
@@ -792,6 +794,7 @@ def self.visible?(member, ctx)
 The query root of this schema
 """
 type Query {
+  exampleMedia: Media
   noFieldsType(noArgumentsInput: NoArguments!): NoFields
   post(
     """

spec/graphql/schema/union_spec.rb

@@ -282,10 +282,12 @@ def unboxed_union
       possible_types object_type, GraphQL::Schema::LateBoundType.new("SomeInterface")
     end
 
+    object_type.field(:u, union_type)
+    object_type.field(:i, interface_type)
+
     err2 = assert_raises ArgumentError do
       Class.new(GraphQL::Schema) do
         query(object_type)
-        orphan_types(union_type, interface_type)
       end
     end
 

spec/graphql/schema/warden_spec.rb

@@ -566,8 +566,8 @@ def self.visible?(member, context)
       assert_nil res["data"]["BagOfThings"]
       assert_equal [], res["data"]["Query"]["fields"]
 
-      # Unreferenced but still visible because orphan type
-      schema.orphan_types([schema.find("BagOfThings")])
+      # Unreferenced but still visible because extra type
+      schema.extra_types([schema.find("BagOfThings")])
       res = schema.execute(query_string, context: { except: ->(m, _) { m.graphql_name == "bag" } })
       assert res["data"]["BagOfThings"]
     end

spec/graphql/schema_spec.rb

@@ -145,6 +145,26 @@ class CustomSubscriptions < GraphQL::Subscriptions::ActionCableSubscriptions
     end
   end
 
+  class ExampleOptionEnum < GraphQL::Schema::Enum
+  end
+  it "rejects non-object types to orphan_types" do
+    object_type = Class.new(GraphQL::Schema::Object)
+    err = assert_raises ArgumentError do
+      Class.new(GraphQL::Schema) do
+        orphan_types(ExampleOptionEnum, object_type)
+      end
+    end
+
+    expected_msg = "Only object type classes should be added as `orphan_types(...)`.
+
+- Remove these no-op types from `orphan_types`: ExampleOptionEnum (ENUM)
+- See https://graphql-ruby.org/type_definitions/interfaces.html#orphan-types
+
+To add other types to your schema, you might want `extra_types`: https://graphql-ruby.org/schema/definition.html#extra-types
+"
+    assert_equal expected_msg, err.message
+  end
+
   describe "merged, inherited caches" do
     METHODS_TO_CACHE = {
       types: 1,

spec/support/dummy/schema.rb

@@ -467,6 +467,8 @@ def deep_non_null; :deep_non_null; end
     def huge_integer
       GraphQL::Types::Int::MAX + 1
     end
+
+    field :example_beverage, Beverage # just to add this type to the schema
   end
 
   class AdminDairyAppQuery < BaseObject
@@ -517,7 +519,7 @@ class Schema < GraphQL::Schema
     mutation DairyAppMutation
     subscription Subscription
     max_depth 5
-    orphan_types Honey, Beverage
+    orphan_types Honey
     trace_with GraphQL::Tracing::CallLegacyTracers
 
     rescue_from(NoSuchDairyError) { |err| raise GraphQL::ExecutionError, err.message  }