From 712a854616fa574c13d4e9baf19b707cdf01dd9a Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 31 Oct 2024 15:43:48 -0400 Subject: [PATCH 1/8] Require object types to have fields --- lib/graphql/schema/member/has_fields.rb | 10 +++++++++- spec/graphql/schema/printer_spec.rb | 21 --------------------- 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/lib/graphql/schema/member/has_fields.rb b/lib/graphql/schema/member/has_fields.rb index 3a5065500f..86c811264a 100644 --- a/lib/graphql/schema/member/has_fields.rb +++ b/lib/graphql/schema/member/has_fields.rb @@ -155,9 +155,11 @@ def fields(context = GraphQL::Query::NullContext.instance) warden = Warden.from_context(context) # Local overrides take precedence over inherited fields visible_fields = {} + had_any_fields_at_all = false for ancestor in ancestors if ancestor.respond_to?(:own_fields) && visible_interface_implementation?(ancestor, context, warden) ancestor.own_fields.each do |field_name, fields_entry| + had_any_fields_at_all = true # Choose the most local definition that passes `.visible?` -- # stop checking for fields by name once one has been found. if !visible_fields.key?(field_name) && (f = Warden.visible_entry?(:visible_field?, fields_entry, context, warden)) @@ -166,7 +168,13 @@ def fields(context = GraphQL::Query::NullContext.instance) end end end - visible_fields + if !had_any_fields_at_all + raise GraphQL::Error, "Object types must have fields, but #{graphql_name} doesn't have any. Define a field for this type or remove it from your schema." + elsif visible_fields.empty? + raise GraphQL::Error, "Object types must have fields, but #{graphql_name}'s fields were all hidden. Implement `.visible?` to hide the type, too." + else + visible_fields + end end end diff --git a/spec/graphql/schema/printer_spec.rb b/spec/graphql/schema/printer_spec.rb index 49676d133f..9435ef6ad6 100644 --- a/spec/graphql/schema/printer_spec.rb +++ b/spec/graphql/schema/printer_spec.rb @@ -73,12 +73,6 @@ class MediaRating < GraphQL::Schema::Enum value :BOO_HISS end - class NoFields < GraphQL::Schema::Object - end - - class NoArguments < GraphQL::Schema::InputObject - end - class Query < GraphQL::Schema::Object description "The query root of this schema" @@ -89,10 +83,6 @@ class Query < GraphQL::Schema::Object argument :deprecated_arg, String, required: false, deprecation_reason: "Use something else" end - field :no_fields_type, NoFields do - argument :no_arguments_input, NoArguments - end - field :example_media, Media end @@ -566,10 +556,6 @@ class Subscription < GraphQL::Schema::Object ): CreatePostPayload } -input NoArguments - -type NoFields - interface Node { id: ID! } @@ -590,7 +576,6 @@ class Subscription < GraphQL::Schema::Object """ type Query { exampleMedia: Media - noFieldsType(noArgumentsInput: NoArguments!): NoFields post( deprecatedArg: String @deprecated(reason: "Use something else") @@ -710,7 +695,6 @@ def self.visible?(member, ctx) assert_equal expected, custom_filter_schema.to_definition(context: context) end - it "applies an `except` filter" do expected = < Date: Thu, 31 Oct 2024 15:47:44 -0400 Subject: [PATCH 2/8] Remove test for EmptyType --- spec/graphql/schema/build_from_definition_spec.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/spec/graphql/schema/build_from_definition_spec.rb b/spec/graphql/schema/build_from_definition_spec.rb index 7fa9ba157a..0a6d8b5f51 100644 --- a/spec/graphql/schema/build_from_definition_spec.rb +++ b/spec/graphql/schema/build_from_definition_spec.rb @@ -18,15 +18,12 @@ def assert_schema_and_compare_output(definition) query: HelloScalars } -type EmptyType - type HelloScalars { bool: Boolean float: Float id: ID int: Int str: String! - t: EmptyType } SCHEMA From 180fe3fefd4455e1463762ba8cc43ec92a6adf6c Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 31 Oct 2024 16:18:46 -0400 Subject: [PATCH 3/8] Update some other tests --- spec/graphql/logger_spec.rb | 2 ++ spec/graphql/schema/directive/flagged_spec.rb | 2 ++ spec/support/jazz.rb | 4 +++- 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/spec/graphql/logger_spec.rb b/spec/graphql/logger_spec.rb index c811067685..f63ad88c78 100644 --- a/spec/graphql/logger_spec.rb +++ b/spec/graphql/logger_spec.rb @@ -78,6 +78,8 @@ class Query < GraphQL::Schema::Object def node(id:) end + + field :something_else, String end query(Query) end diff --git a/spec/graphql/schema/directive/flagged_spec.rb b/spec/graphql/schema/directive/flagged_spec.rb index e794f5f9a2..466adfdd83 100644 --- a/spec/graphql/schema/directive/flagged_spec.rb +++ b/spec/graphql/schema/directive/flagged_spec.rb @@ -41,6 +41,8 @@ def antarctica; true; end field :santas_workshop, Boolean, null: false, directives: { GraphQL::Schema::Directive::Flagged => { by: ["northPole"] } } def santas_workshop; true; end + + field :something_not_flagged, String end query(Query) diff --git a/spec/support/jazz.rb b/spec/support/jazz.rb index ac0706c73a..385c487247 100644 --- a/spec/support/jazz.rb +++ b/spec/support/jazz.rb @@ -911,7 +911,9 @@ def self.object_from_id(id, ctx) GloballyIdentifiableType.find(id) end - BlogPost = Class.new(GraphQL::Schema::Object) + class BlogPost < GraphQL::Schema::Object + field :title, String + end extra_types BlogPost use GraphQL::Dataloader use GraphQL::Schema::Warden if ADD_WARDEN From 5d54f471e8c7602d6fc1e777b0ca44783c39e9e8 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 1 Nov 2024 10:36:09 -0400 Subject: [PATCH 4/8] Update warden tests --- spec/graphql/schema/warden_spec.rb | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/spec/graphql/schema/warden_spec.rb b/spec/graphql/schema/warden_spec.rb index 4e7eeadfe0..bde1b2c1a7 100644 --- a/spec/graphql/schema/warden_spec.rb +++ b/spec/graphql/schema/warden_spec.rb @@ -550,6 +550,7 @@ def self.visible?(ctx) class Query < GraphQL::Schema::Object field :bag, BagOfThings, null: false + field :something_not_hidden, String end query(Query) @@ -575,16 +576,16 @@ def self.visible?(member, context) res = schema.execute(query_string) assert res["data"]["BagOfThings"] - assert_equal ["bag"], res["data"]["Query"]["fields"].map { |f| f["name"] } + assert_equal ["bag", "somethingNotHidden"], res["data"]["Query"]["fields"].map { |f| f["name"] } # Hide the union when all its possible types are gone. This will cause the field to be hidden too. res = schema.execute(query_string, context: { except: ->(m, _) { ["A", "B", "C"].include?(m.graphql_name) } }) assert_nil res["data"]["BagOfThings"] - assert_equal [], res["data"]["Query"]["fields"] + assert_equal [{ "name" => "somethingNotHidden" }], res["data"]["Query"]["fields"] res = schema.execute(query_string, context: { except: ->(m, _) { m.graphql_name == "bag" } }) assert_nil res["data"]["BagOfThings"] - assert_equal [], res["data"]["Query"]["fields"] + assert_equal [{ "name" => "somethingNotHidden" }], res["data"]["Query"]["fields"] # Unreferenced but still visible because extra type schema.extra_types([schema.find("BagOfThings")]) @@ -597,6 +598,7 @@ def self.visible?(member, context) type Query { node: Node a: A + notHidden: String } type A implements Node { @@ -636,25 +638,25 @@ def self.visible?(member, context) res = schema.execute(query_string) assert res["data"]["Node"] - assert_equal ["a", "node"], res["data"]["Query"]["fields"].map { |f| f["name"] } + assert_equal ["a", "node", "notHidden"], res["data"]["Query"]["fields"].map { |f| f["name"] } res = schema.execute(query_string, context: { skip_visibility_migration_error: true, except: ->(m, _) { ["A", "B", "C"].include?(m.graphql_name) } }) if GraphQL::Schema.use_visibility_profile? # Node is still visible even though it has no possible types assert res["data"]["Node"] - assert_equal [{ "name" => "node" }], res["data"]["Query"]["fields"] + assert_equal [{ "name" => "node" }, { "name" => "notHidden" }], res["data"]["Query"]["fields"] else # When the possible types are all hidden, hide the interface and fields pointing to it assert_nil res["data"]["Node"] - assert_equal [], res["data"]["Query"]["fields"] + assert_equal [{ "name" => "notHidden" }], res["data"]["Query"]["fields"] end # Even when it's not the return value of a field, # still show the interface since it allows code reuse res = schema.execute(query_string, context: { except: ->(m, _) { m.graphql_name == "node" } }) assert_equal "Node", res["data"]["Node"]["name"] - assert_equal [{"name" => "a"}], res["data"]["Query"]["fields"] + assert_equal [{"name" => "a"}, { "name" => "notHidden" }], res["data"]["Query"]["fields"] end it "can't be a fragment condition" do @@ -1057,6 +1059,7 @@ def self.visible?(_ctx); false; end visible_account = Class.new(hidden_account) do graphql_name "NewAccount" + field :username, String def self.visible?(_ctx); true; end end From 296895242dfaf942a173d248c08acfa630859f39 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 1 Nov 2024 11:14:18 -0400 Subject: [PATCH 5/8] Update more tests, add has_no_fields(true) for backwards compat --- benchmark/run.rb | 1 + lib/graphql/schema/member/has_fields.rb | 15 ++++++--- spec/graphql/schema/object_spec.rb | 44 +++++++++++++++++++++++++ spec/graphql/schema/printer_spec.rb | 22 +++++++++++++ spec/graphql/schema/warden_spec.rb | 18 +++++----- spec/support/jazz.rb | 5 ++- 6 files changed, 88 insertions(+), 17 deletions(-) diff --git a/benchmark/run.rb b/benchmark/run.rb index ad9a49d531..8052e0486f 100644 --- a/benchmark/run.rb +++ b/benchmark/run.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true require "graphql" +ADD_WARDEN = false require "jazz" require "benchmark/ips" require "stackprof" diff --git a/lib/graphql/schema/member/has_fields.rb b/lib/graphql/schema/member/has_fields.rb index 86c811264a..68441da2b0 100644 --- a/lib/graphql/schema/member/has_fields.rb +++ b/lib/graphql/schema/member/has_fields.rb @@ -79,6 +79,14 @@ def global_id_field(field_name, **kwargs) end end + def has_no_fields(new_has_no_fields) + @has_no_fields = new_has_no_fields + end + + def has_no_fields? + @has_no_fields + end + # @return [Hash GraphQL::Schema::Field, Array>] Fields defined on this class _specifically_, not parent classes def own_fields @own_fields ||= {} @@ -168,10 +176,8 @@ def fields(context = GraphQL::Query::NullContext.instance) end end end - if !had_any_fields_at_all - raise GraphQL::Error, "Object types must have fields, but #{graphql_name} doesn't have any. Define a field for this type or remove it from your schema." - elsif visible_fields.empty? - raise GraphQL::Error, "Object types must have fields, but #{graphql_name}'s fields were all hidden. Implement `.visible?` to hide the type, too." + if !had_any_fields_at_all && !has_no_fields? + raise GraphQL::Error, "Object types must have fields, but #{graphql_name} doesn't have any. Define a field for this type, remove it from your schema, or add `has_no_fields(true)` to its definition." else visible_fields end @@ -196,6 +202,7 @@ def inherited(subclass) subclass.class_eval do @own_fields ||= nil @field_class ||= nil + @has_no_fields ||= false end end diff --git a/spec/graphql/schema/object_spec.rb b/spec/graphql/schema/object_spec.rb index 6f7e24f49e..c59f48f2d0 100644 --- a/spec/graphql/schema/object_spec.rb +++ b/spec/graphql/schema/object_spec.rb @@ -512,4 +512,48 @@ def self.wrap(obj, ctx) assert_nil obj1.comment end end + + describe "when defined with no fields" do + class NoFieldsSchema < GraphQL::Schema + class NoFieldsThing < GraphQL::Schema::Object + end + + class NoFieldsCompatThing < GraphQL::Schema::Object + has_no_fields(true) + end + + class Query < GraphQL::Schema::Object + field :no_fields_thing, NoFieldsThing + field :no_fields_compat_thing, NoFieldsCompatThing + end + + query(Query) + end + + it "raises an error at runtime and printing" do + refute NoFieldsSchema::NoFieldsThing.has_no_fields? + + err = assert_raises GraphQL::Error do + NoFieldsSchema.execute("{ noFieldsThing { blah } }") + end + expected_message = "Object types must have fields, but NoFieldsThing doesn't have any. Define a field for this type, remove it from your schema, or add `has_no_fields(true)` to its definition." + assert_equal expected_message, err.message + + err = assert_raises GraphQL::Error do + NoFieldsSchema.to_definition + end + assert_equal expected_message, err.message + + err = assert_raises GraphQL::Error do + NoFieldsSchema.to_json + end + assert_equal expected_message, err.message + end + + it "doesn't raise an error if has_no_fields(true)" do + assert NoFieldsSchema::NoFieldsCompatThing.has_no_fields? + res = NoFieldsSchema.execute("{ noFieldsCompatThing { blah } }") + assert_equal ["Field 'blah' doesn't exist on type 'NoFieldsCompatThing'"], res["errors"].map { |e| e["message"] } + end + end end diff --git a/spec/graphql/schema/printer_spec.rb b/spec/graphql/schema/printer_spec.rb index 9435ef6ad6..ebebb4a225 100644 --- a/spec/graphql/schema/printer_spec.rb +++ b/spec/graphql/schema/printer_spec.rb @@ -73,6 +73,14 @@ class MediaRating < GraphQL::Schema::Enum value :BOO_HISS end + + class NoFields < GraphQL::Schema::Object + has_no_fields(true) + end + + class NoArguments < GraphQL::Schema::InputObject + end + class Query < GraphQL::Schema::Object description "The query root of this schema" @@ -83,6 +91,10 @@ class Query < GraphQL::Schema::Object argument :deprecated_arg, String, required: false, deprecation_reason: "Use something else" end + field :no_fields_type, NoFields do + argument :no_arguments_input, NoArguments + end + field :example_media, Media end @@ -556,6 +568,10 @@ class Subscription < GraphQL::Schema::Object ): CreatePostPayload } +input NoArguments + +type NoFields + interface Node { id: ID! } @@ -576,6 +592,7 @@ class Subscription < GraphQL::Schema::Object """ type Query { exampleMedia: Media + noFieldsType(noArgumentsInput: NoArguments!): NoFields post( deprecatedArg: String @deprecated(reason: "Use something else") @@ -757,6 +774,10 @@ def self.visible?(member, ctx) ): CreatePostPayload } +input NoArguments + +type NoFields + interface Node { id: ID! } @@ -776,6 +797,7 @@ def self.visible?(member, ctx) """ type Query { exampleMedia: Media + noFieldsType(noArgumentsInput: NoArguments!): NoFields post( """ Post ID diff --git a/spec/graphql/schema/warden_spec.rb b/spec/graphql/schema/warden_spec.rb index bde1b2c1a7..ce8b523d35 100644 --- a/spec/graphql/schema/warden_spec.rb +++ b/spec/graphql/schema/warden_spec.rb @@ -550,7 +550,6 @@ def self.visible?(ctx) class Query < GraphQL::Schema::Object field :bag, BagOfThings, null: false - field :something_not_hidden, String end query(Query) @@ -576,16 +575,16 @@ def self.visible?(member, context) res = schema.execute(query_string) assert res["data"]["BagOfThings"] - assert_equal ["bag", "somethingNotHidden"], res["data"]["Query"]["fields"].map { |f| f["name"] } + assert_equal ["bag"], res["data"]["Query"]["fields"].map { |f| f["name"] } # Hide the union when all its possible types are gone. This will cause the field to be hidden too. res = schema.execute(query_string, context: { except: ->(m, _) { ["A", "B", "C"].include?(m.graphql_name) } }) assert_nil res["data"]["BagOfThings"] - assert_equal [{ "name" => "somethingNotHidden" }], res["data"]["Query"]["fields"] + assert_equal [], res["data"]["Query"]["fields"] res = schema.execute(query_string, context: { except: ->(m, _) { m.graphql_name == "bag" } }) assert_nil res["data"]["BagOfThings"] - assert_equal [{ "name" => "somethingNotHidden" }], res["data"]["Query"]["fields"] + assert_equal [], res["data"]["Query"]["fields"] # Unreferenced but still visible because extra type schema.extra_types([schema.find("BagOfThings")]) @@ -598,7 +597,6 @@ def self.visible?(member, context) type Query { node: Node a: A - notHidden: String } type A implements Node { @@ -638,25 +636,25 @@ def self.visible?(member, context) res = schema.execute(query_string) assert res["data"]["Node"] - assert_equal ["a", "node", "notHidden"], res["data"]["Query"]["fields"].map { |f| f["name"] } + assert_equal ["a", "node"], res["data"]["Query"]["fields"].map { |f| f["name"] } res = schema.execute(query_string, context: { skip_visibility_migration_error: true, except: ->(m, _) { ["A", "B", "C"].include?(m.graphql_name) } }) if GraphQL::Schema.use_visibility_profile? # Node is still visible even though it has no possible types assert res["data"]["Node"] - assert_equal [{ "name" => "node" }, { "name" => "notHidden" }], res["data"]["Query"]["fields"] + assert_equal [{ "name" => "node" }], res["data"]["Query"]["fields"] else # When the possible types are all hidden, hide the interface and fields pointing to it assert_nil res["data"]["Node"] - assert_equal [{ "name" => "notHidden" }], res["data"]["Query"]["fields"] + assert_equal [], res["data"]["Query"]["fields"] end # Even when it's not the return value of a field, # still show the interface since it allows code reuse res = schema.execute(query_string, context: { except: ->(m, _) { m.graphql_name == "node" } }) assert_equal "Node", res["data"]["Node"]["name"] - assert_equal [{"name" => "a"}, { "name" => "notHidden" }], res["data"]["Query"]["fields"] + assert_equal [{"name" => "a"}], res["data"]["Query"]["fields"] end it "can't be a fragment condition" do @@ -1059,7 +1057,7 @@ def self.visible?(_ctx); false; end visible_account = Class.new(hidden_account) do graphql_name "NewAccount" - field :username, String + has_no_fields(true) def self.visible?(_ctx); true; end end diff --git a/spec/support/jazz.rb b/spec/support/jazz.rb index 385c487247..0c71c2e5d6 100644 --- a/spec/support/jazz.rb +++ b/spec/support/jazz.rb @@ -911,9 +911,8 @@ def self.object_from_id(id, ctx) GloballyIdentifiableType.find(id) end - class BlogPost < GraphQL::Schema::Object - field :title, String - end + BlogPost = Class.new(GraphQL::Schema::Object) + BlogPost.has_no_fields(true) extra_types BlogPost use GraphQL::Dataloader use GraphQL::Schema::Warden if ADD_WARDEN From 6bfe27b6154e4d351aec872a4b1787c376124f7c Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 1 Nov 2024 14:42:18 -0400 Subject: [PATCH 6/8] Require at least one argument for input objects, add has_no_arguments(true) for backwards compat --- .../schema/has_single_input_argument.rb | 2 + lib/graphql/schema/input_object.rb | 73 ++++++++++++------- lib/graphql/schema/member/has_arguments.rb | 10 +-- lib/graphql/schema/member/has_fields.rb | 4 + .../schema/has_single_input_argument_spec.rb | 1 - spec/graphql/schema/input_object_spec.rb | 51 +++++++++++++ spec/graphql/schema/printer_spec.rb | 1 + 7 files changed, 109 insertions(+), 33 deletions(-) diff --git a/lib/graphql/schema/has_single_input_argument.rb b/lib/graphql/schema/has_single_input_argument.rb index 26c266840f..bf98d4d930 100644 --- a/lib/graphql/schema/has_single_input_argument.rb +++ b/lib/graphql/schema/has_single_input_argument.rb @@ -136,6 +136,8 @@ def description(new_desc = nil) super || "Autogenerated input type of #{self.mutation.graphql_name}" end end + # For compatibility, in case no arguments are defined: + has_no_arguments(true) mutation(mutation_class) # these might be inherited: mutation_args.each do |arg| diff --git a/lib/graphql/schema/input_object.rb b/lib/graphql/schema/input_object.rb index 92f87293c4..e47c8e4a22 100644 --- a/lib/graphql/schema/input_object.rb +++ b/lib/graphql/schema/input_object.rb @@ -56,33 +56,6 @@ def prepare end end - def self.authorized?(obj, value, ctx) - # Authorize each argument (but this doesn't apply if `prepare` is implemented): - if value.respond_to?(:key?) - ctx.types.arguments(self).each do |input_obj_arg| - if value.key?(input_obj_arg.keyword) && - !input_obj_arg.authorized?(obj, value[input_obj_arg.keyword], ctx) - return false - end - end - end - # It didn't early-return false: - true - end - - def self.one_of - if !one_of? - if all_argument_definitions.any? { |arg| arg.type.non_null? } - raise ArgumentError, "`one_of` may not be used with required arguments -- add `required: false` to argument definitions to use `one_of`" - end - directive(GraphQL::Schema::Directive::OneOf) - end - end - - def self.one_of? - false # Re-defined when `OneOf` is added - end - def unwrap_value(value) case value when Array @@ -121,6 +94,33 @@ def to_kwargs end class << self + def authorized?(obj, value, ctx) + # Authorize each argument (but this doesn't apply if `prepare` is implemented): + if value.respond_to?(:key?) + ctx.types.arguments(self).each do |input_obj_arg| + if value.key?(input_obj_arg.keyword) && + !input_obj_arg.authorized?(obj, value[input_obj_arg.keyword], ctx) + return false + end + end + end + # It didn't early-return false: + true + end + + def one_of + if !one_of? + if all_argument_definitions.any? { |arg| arg.type.non_null? } + raise ArgumentError, "`one_of` may not be used with required arguments -- add `required: false` to argument definitions to use `one_of`" + end + directive(GraphQL::Schema::Directive::OneOf) + end + end + + def one_of? + false # Re-defined when `OneOf` is added + end + def argument(*args, **kwargs, &block) argument_defn = super(*args, **kwargs, &block) if one_of? @@ -246,6 +246,25 @@ def coerce_result(value, ctx) result end + # @param new_has_no_arguments [Boolean] Call with `true` to make this InputObject type ignore the requirement to have any defined arguments. + # @return [void] + def has_no_arguments(new_has_no_arguments) + @has_no_arguments = new_has_no_arguments + nil + end + + # @return [Boolean] `true` if `has_no_arguments(true)` was configued + def has_no_arguments? + @has_no_arguments + end + + def arguments(context = GraphQL::Query::NullContext.instance, require_defined_arguments = true) + if require_defined_arguments && !has_no_arguments? && !any_arguments? + raise GraphQL::Error, "Input Object types must have arguments, but #{graphql_name} doesn't have any. Define an argument for this type, remove it from your schema, or add `has_no_arguments(true)` to its definition." + end + super(context, false) + end + private # Suppress redefinition warning for objectId arguments diff --git a/lib/graphql/schema/member/has_arguments.rb b/lib/graphql/schema/member/has_arguments.rb index 126c406ab8..3aa52a56ea 100644 --- a/lib/graphql/schema/member/has_arguments.rb +++ b/lib/graphql/schema/member/has_arguments.rb @@ -76,7 +76,7 @@ def remove_argument(arg_defn) end # @return [Hash GraphQL::Schema::Argument] Arguments defined on this thing, keyed by name. Includes inherited definitions - def arguments(context = GraphQL::Query::NullContext.instance) + def arguments(context = GraphQL::Query::NullContext.instance, _require_defined_arguments = nil) if own_arguments.any? own_arguments_that_apply = {} own_arguments.each do |name, args_entry| @@ -100,9 +100,9 @@ def inherited(child_class) end module InheritedArguments - def arguments(context = GraphQL::Query::NullContext.instance) - own_arguments = super - inherited_arguments = superclass.arguments(context) + def arguments(context = GraphQL::Query::NullContext.instance, require_defined_arguments = true) + own_arguments = super(context, require_defined_arguments) + inherited_arguments = superclass.arguments(context, false) if own_arguments.any? if inherited_arguments.any? @@ -149,7 +149,7 @@ def get_argument(argument_name, context = GraphQL::Query::NullContext.instance) end module FieldConfigured - def arguments(context = GraphQL::Query::NullContext.instance) + def arguments(context = GraphQL::Query::NullContext.instance, _require_defined_arguments = nil) own_arguments = super if @resolver_class inherited_arguments = @resolver_class.field_arguments(context) diff --git a/lib/graphql/schema/member/has_fields.rb b/lib/graphql/schema/member/has_fields.rb index 68441da2b0..7c1faf1189 100644 --- a/lib/graphql/schema/member/has_fields.rb +++ b/lib/graphql/schema/member/has_fields.rb @@ -79,10 +79,14 @@ def global_id_field(field_name, **kwargs) end end + # @param new_has_no_fields [Boolean] Call with `true` to make this Object type ignore the requirement to have any defined fields. + # @return [void] def has_no_fields(new_has_no_fields) @has_no_fields = new_has_no_fields + nil end + # @return [Boolean] `true` if `has_no_fields(true)` was configued def has_no_fields? @has_no_fields end diff --git a/spec/graphql/schema/has_single_input_argument_spec.rb b/spec/graphql/schema/has_single_input_argument_spec.rb index e250b27c7e..5a242d8568 100644 --- a/spec/graphql/schema/has_single_input_argument_spec.rb +++ b/spec/graphql/schema/has_single_input_argument_spec.rb @@ -124,7 +124,6 @@ def resolve name: 'name', } end - end class Mutation < GraphQL::Schema::Object field_class GraphQL::Schema::Field diff --git a/spec/graphql/schema/input_object_spec.rb b/spec/graphql/schema/input_object_spec.rb index 81a788b636..61e6dea16c 100644 --- a/spec/graphql/schema/input_object_spec.rb +++ b/spec/graphql/schema/input_object_spec.rb @@ -1419,4 +1419,55 @@ def result(values:) end end end + + describe "when no arguments are defined" do + describe "when defined with no fields" do + class NoArgumentsSchema < GraphQL::Schema + class NoArgumentsInput < GraphQL::Schema::InputObject + end + + class NoArgumentsCompatInput < GraphQL::Schema::InputObject + has_no_arguments(true) + end + + class Query < GraphQL::Schema::Object + field :no_arguments, String do + argument :input, NoArgumentsInput + end + + field :no_arguments_compat, String, fallback_value: "OK" do + argument :input, NoArgumentsCompatInput + end + end + + query(Query) + end + + it "raises an error at runtime and printing" do + refute NoArgumentsSchema::NoArgumentsInput.has_no_arguments? + + err = assert_raises GraphQL::Error do + NoArgumentsSchema.execute("{ noArguments(input: {}) }") + end + expected_message = "Input Object types must have arguments, but NoArgumentsInput doesn't have any. Define an argument for this type, remove it from your schema, or add `has_no_arguments(true)` to its definition." + assert_equal expected_message, err.message + + err = assert_raises GraphQL::Error do + NoArgumentsSchema.to_definition + end + assert_equal expected_message, err.message + + err = assert_raises GraphQL::Error do + NoArgumentsSchema.to_json + end + assert_equal expected_message, err.message + end + + it "doesn't raise an error if has_no_arguments(true)" do + assert NoArgumentsSchema::NoArgumentsCompatInput.has_no_arguments? + res = NoArgumentsSchema.execute("{ noArgumentsCompat(input: {}) }") + assert_equal "OK", res["data"]["noArgumentsCompat"] + end + end + end end diff --git a/spec/graphql/schema/printer_spec.rb b/spec/graphql/schema/printer_spec.rb index ebebb4a225..b00cce7847 100644 --- a/spec/graphql/schema/printer_spec.rb +++ b/spec/graphql/schema/printer_spec.rb @@ -79,6 +79,7 @@ class NoFields < GraphQL::Schema::Object end class NoArguments < GraphQL::Schema::InputObject + has_no_arguments(true) end class Query < GraphQL::Schema::Object From 3b18e2b806b98554af4437a67108f21941a489af Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Wed, 20 Nov 2024 13:51:29 -0500 Subject: [PATCH 7/8] Add dedicated error classes --- lib/graphql/schema/input_object.rb | 10 +++++++++- lib/graphql/schema/member/has_fields.rb | 2 +- lib/graphql/schema/object.rb | 8 ++++++++ spec/graphql/schema/input_object_spec.rb | 6 +++--- spec/graphql/schema/object_spec.rb | 6 +++--- 5 files changed, 24 insertions(+), 8 deletions(-) diff --git a/lib/graphql/schema/input_object.rb b/lib/graphql/schema/input_object.rb index e47c8e4a22..56916ee01b 100644 --- a/lib/graphql/schema/input_object.rb +++ b/lib/graphql/schema/input_object.rb @@ -10,6 +10,14 @@ class InputObject < GraphQL::Schema::Member include GraphQL::Dig + # Raised when an InputObject doesn't have any arguments defined and hasn't explicitly opted out of this requirement + class ArgumentsAreRequiredError < GraphQL::Error + def initialize(input_object_type) + message = "Input Object types must have arguments, but #{input_object_type.graphql_name} doesn't have any. Define an argument for this type, remove it from your schema, or add `has_no_arguments(true)` to its definition." + super(message) + end + end + # @return [GraphQL::Query::Context] The context for this query attr_reader :context # @return [GraphQL::Execution::Interpereter::Arguments] The underlying arguments instance @@ -260,7 +268,7 @@ def has_no_arguments? def arguments(context = GraphQL::Query::NullContext.instance, require_defined_arguments = true) if require_defined_arguments && !has_no_arguments? && !any_arguments? - raise GraphQL::Error, "Input Object types must have arguments, but #{graphql_name} doesn't have any. Define an argument for this type, remove it from your schema, or add `has_no_arguments(true)` to its definition." + raise GraphQL::Schema::InputObject::ArgumentsAreRequiredError.new(self) end super(context, false) end diff --git a/lib/graphql/schema/member/has_fields.rb b/lib/graphql/schema/member/has_fields.rb index 7c1faf1189..269568de2b 100644 --- a/lib/graphql/schema/member/has_fields.rb +++ b/lib/graphql/schema/member/has_fields.rb @@ -181,7 +181,7 @@ def fields(context = GraphQL::Query::NullContext.instance) end end if !had_any_fields_at_all && !has_no_fields? - raise GraphQL::Error, "Object types must have fields, but #{graphql_name} doesn't have any. Define a field for this type, remove it from your schema, or add `has_no_fields(true)` to its definition." + raise GraphQL::Schema::Object::FieldsAreRequiredError.new(self) else visible_fields end diff --git a/lib/graphql/schema/object.rb b/lib/graphql/schema/object.rb index e778c331c1..25dafdd239 100644 --- a/lib/graphql/schema/object.rb +++ b/lib/graphql/schema/object.rb @@ -8,6 +8,14 @@ class Object < GraphQL::Schema::Member extend GraphQL::Schema::Member::HasFields extend GraphQL::Schema::Member::HasInterfaces + # Raised when an Object doesn't have any field defined and hasn't explicitly opted out of this requirement + class FieldsAreRequiredError < GraphQL::Error + def initialize(object_type) + message = "Object types must have fields, but #{object_type.graphql_name} doesn't have any. Define a field for this type, remove it from your schema, or add `has_no_fields(true)` to its definition." + super(message) + end + end + # @return [Object] the application object this type is wrapping attr_reader :object diff --git a/spec/graphql/schema/input_object_spec.rb b/spec/graphql/schema/input_object_spec.rb index 61e6dea16c..8f2a332e6c 100644 --- a/spec/graphql/schema/input_object_spec.rb +++ b/spec/graphql/schema/input_object_spec.rb @@ -1446,18 +1446,18 @@ class Query < GraphQL::Schema::Object it "raises an error at runtime and printing" do refute NoArgumentsSchema::NoArgumentsInput.has_no_arguments? - err = assert_raises GraphQL::Error do + err = assert_raises GraphQL::Schema::InputObject::ArgumentsAreRequiredError do NoArgumentsSchema.execute("{ noArguments(input: {}) }") end expected_message = "Input Object types must have arguments, but NoArgumentsInput doesn't have any. Define an argument for this type, remove it from your schema, or add `has_no_arguments(true)` to its definition." assert_equal expected_message, err.message - err = assert_raises GraphQL::Error do + err = assert_raises GraphQL::Schema::InputObject::ArgumentsAreRequiredError do NoArgumentsSchema.to_definition end assert_equal expected_message, err.message - err = assert_raises GraphQL::Error do + err = assert_raises GraphQL::Schema::InputObject::ArgumentsAreRequiredError do NoArgumentsSchema.to_json end assert_equal expected_message, err.message diff --git a/spec/graphql/schema/object_spec.rb b/spec/graphql/schema/object_spec.rb index c59f48f2d0..b2c5019063 100644 --- a/spec/graphql/schema/object_spec.rb +++ b/spec/graphql/schema/object_spec.rb @@ -533,18 +533,18 @@ class Query < GraphQL::Schema::Object it "raises an error at runtime and printing" do refute NoFieldsSchema::NoFieldsThing.has_no_fields? - err = assert_raises GraphQL::Error do + err = assert_raises GraphQL::Schema::Object::FieldsAreRequiredError do NoFieldsSchema.execute("{ noFieldsThing { blah } }") end expected_message = "Object types must have fields, but NoFieldsThing doesn't have any. Define a field for this type, remove it from your schema, or add `has_no_fields(true)` to its definition." assert_equal expected_message, err.message - err = assert_raises GraphQL::Error do + err = assert_raises GraphQL::Schema::Object::FieldsAreRequiredError do NoFieldsSchema.to_definition end assert_equal expected_message, err.message - err = assert_raises GraphQL::Error do + err = assert_raises GraphQL::Schema::Object::FieldsAreRequiredError do NoFieldsSchema.to_json end assert_equal expected_message, err.message From 9e07f52d82c4e013cb955670789512ef04daa814 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Wed, 20 Nov 2024 14:01:34 -0500 Subject: [PATCH 8/8] Emit a warning instead of an error --- lib/graphql/schema/input_object.rb | 2 +- lib/graphql/schema/member/has_fields.rb | 5 ++--- spec/graphql/schema/input_object_spec.rb | 21 ++++++++++++--------- spec/graphql/schema/object_spec.rb | 20 ++++++++++++-------- spec/spec_helper.rb | 8 ++++++++ 5 files changed, 35 insertions(+), 21 deletions(-) diff --git a/lib/graphql/schema/input_object.rb b/lib/graphql/schema/input_object.rb index 56916ee01b..a04c1877ad 100644 --- a/lib/graphql/schema/input_object.rb +++ b/lib/graphql/schema/input_object.rb @@ -268,7 +268,7 @@ def has_no_arguments? def arguments(context = GraphQL::Query::NullContext.instance, require_defined_arguments = true) if require_defined_arguments && !has_no_arguments? && !any_arguments? - raise GraphQL::Schema::InputObject::ArgumentsAreRequiredError.new(self) + warn(GraphQL::Schema::InputObject::ArgumentsAreRequiredError.new(self).message + "\n\nThis will raise an error in a future GraphQL-Ruby version.") end super(context, false) end diff --git a/lib/graphql/schema/member/has_fields.rb b/lib/graphql/schema/member/has_fields.rb index 269568de2b..be041b1698 100644 --- a/lib/graphql/schema/member/has_fields.rb +++ b/lib/graphql/schema/member/has_fields.rb @@ -181,10 +181,9 @@ def fields(context = GraphQL::Query::NullContext.instance) end end if !had_any_fields_at_all && !has_no_fields? - raise GraphQL::Schema::Object::FieldsAreRequiredError.new(self) - else - visible_fields + warn(GraphQL::Schema::Object::FieldsAreRequiredError.new(self).message + "\n\nThis will raise an error in a future GraphQL-Ruby version.") end + visible_fields end end diff --git a/spec/graphql/schema/input_object_spec.rb b/spec/graphql/schema/input_object_spec.rb index 8f2a332e6c..9b5f01a716 100644 --- a/spec/graphql/schema/input_object_spec.rb +++ b/spec/graphql/schema/input_object_spec.rb @@ -1431,7 +1431,7 @@ class NoArgumentsCompatInput < GraphQL::Schema::InputObject end class Query < GraphQL::Schema::Object - field :no_arguments, String do + field :no_arguments, String, fallback_value: "NO_ARGS" do argument :input, NoArgumentsInput end @@ -1446,26 +1446,29 @@ class Query < GraphQL::Schema::Object it "raises an error at runtime and printing" do refute NoArgumentsSchema::NoArgumentsInput.has_no_arguments? - err = assert_raises GraphQL::Schema::InputObject::ArgumentsAreRequiredError do + expected_message = "Input Object types must have arguments, but NoArgumentsInput doesn't have any. Define an argument for this type, remove it from your schema, or add `has_no_arguments(true)` to its definition. + +This will raise an error in a future GraphQL-Ruby version. +" + res = assert_warns(expected_message) do NoArgumentsSchema.execute("{ noArguments(input: {}) }") end - expected_message = "Input Object types must have arguments, but NoArgumentsInput doesn't have any. Define an argument for this type, remove it from your schema, or add `has_no_arguments(true)` to its definition." - assert_equal expected_message, err.message + assert_equal "NO_ARGS", res["data"]["noArguments"] - err = assert_raises GraphQL::Schema::InputObject::ArgumentsAreRequiredError do + assert_warns(expected_message) do NoArgumentsSchema.to_definition end - assert_equal expected_message, err.message - err = assert_raises GraphQL::Schema::InputObject::ArgumentsAreRequiredError do + assert_warns(expected_message) do NoArgumentsSchema.to_json end - assert_equal expected_message, err.message end it "doesn't raise an error if has_no_arguments(true)" do assert NoArgumentsSchema::NoArgumentsCompatInput.has_no_arguments? - res = NoArgumentsSchema.execute("{ noArgumentsCompat(input: {}) }") + res = assert_warns("") do + NoArgumentsSchema.execute("{ noArgumentsCompat(input: {}) }") + end assert_equal "OK", res["data"]["noArgumentsCompat"] end end diff --git a/spec/graphql/schema/object_spec.rb b/spec/graphql/schema/object_spec.rb index b2c5019063..15589978d9 100644 --- a/spec/graphql/schema/object_spec.rb +++ b/spec/graphql/schema/object_spec.rb @@ -533,26 +533,30 @@ class Query < GraphQL::Schema::Object it "raises an error at runtime and printing" do refute NoFieldsSchema::NoFieldsThing.has_no_fields? - err = assert_raises GraphQL::Schema::Object::FieldsAreRequiredError do + expected_message = "Object types must have fields, but NoFieldsThing doesn't have any. Define a field for this type, remove it from your schema, or add `has_no_fields(true)` to its definition. + +This will raise an error in a future GraphQL-Ruby version. +" + res = assert_warns(expected_message) do NoFieldsSchema.execute("{ noFieldsThing { blah } }") end - expected_message = "Object types must have fields, but NoFieldsThing doesn't have any. Define a field for this type, remove it from your schema, or add `has_no_fields(true)` to its definition." - assert_equal expected_message, err.message + assert_equal ["Field 'blah' doesn't exist on type 'NoFieldsThing'"], res["errors"].map { |err| err["message"] } - err = assert_raises GraphQL::Schema::Object::FieldsAreRequiredError do + assert_warns(expected_message) do NoFieldsSchema.to_definition end - assert_equal expected_message, err.message - err = assert_raises GraphQL::Schema::Object::FieldsAreRequiredError do + assert_warns(expected_message) do NoFieldsSchema.to_json end - assert_equal expected_message, err.message end it "doesn't raise an error if has_no_fields(true)" do assert NoFieldsSchema::NoFieldsCompatThing.has_no_fields? - res = NoFieldsSchema.execute("{ noFieldsCompatThing { blah } }") + + res = assert_warns "" do + NoFieldsSchema.execute("{ noFieldsCompatThing { blah } }") + end assert_equal ["Field 'blah' doesn't exist on type 'NoFieldsCompatThing'"], res["errors"].map { |e| e["message"] } end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index bd5d43158d..380e8603d8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -170,3 +170,11 @@ def trace(key, data) if !USING_C_PARSER && defined?(GraphQL::CParser::Parser) raise "Load error: didn't opt in to C parser but GraphQL::CParser::Parser was defined" end + +def assert_warns(warning, printing = "") + return_val = nil + stdout, stderr = capture_io { return_val = yield } + assert_equal warning, stderr, "It produced the expected stderr" + assert_equal stdout, printing, "It produced the expected stdout" + return_val +end