diff --git a/lib/graphql/rubocop.rb b/lib/graphql/rubocop.rb index d2dae8c6cd..34bc2f1bf5 100644 --- a/lib/graphql/rubocop.rb +++ b/lib/graphql/rubocop.rb @@ -2,3 +2,4 @@ require "graphql/rubocop/graphql/default_null_true" require "graphql/rubocop/graphql/default_required_true" +require "graphql/rubocop/graphql/field_type_in_block" diff --git a/lib/graphql/rubocop/graphql/field_type_in_block.rb b/lib/graphql/rubocop/graphql/field_type_in_block.rb new file mode 100644 index 0000000000..751deddd4b --- /dev/null +++ b/lib/graphql/rubocop/graphql/field_type_in_block.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true +require_relative "./base_cop" + +module GraphQL + module Rubocop + module GraphQL + # Identify (and auto-correct) any field whose type configuration isn't given + # in the configuration block. + # + # @example + # # bad, immediately causes Rails to load `app/graphql/types/thing.rb` + # field :thing, Types::Thing + # + # # good, defers loading until the file is needed + # field :thing do + # type(Types::Thing) + # end + # + class FieldTypeInBlock < BaseCop + MSG = "type configuration can be moved to a block to defer loading the type's file" + + BUILT_IN_SCALAR_NAMES = ["Float", "Int", "Integer", "String", "ID", "Boolean"] + def_node_matcher :field_config_with_inline_type, <<-Pattern + ( + send {nil? _} :field sym ${const array} ... + ) + Pattern + + def_node_matcher :field_config_with_inline_type_and_block, <<-Pattern + ( + block + (send {nil? _} :field sym ${const array}) ... + (args) + _ + + ) + Pattern + + def on_block(node) + field_config_with_inline_type_and_block(node) do |type_const| + ignore_node(type_const) + type_const_str = get_type_argument_str(node, type_const) + if ignore_inline_type_str?(type_const_str) + # Do nothing ... + else + add_offense(type_const) do |corrector| + cleaned_node_source = delete_type_argument(node, type_const) + field_indent = determine_field_indent(node) + cleaned_node_source.sub!(/(\{|do)/, "\\1\n#{field_indent} type #{type_const_str}") + corrector.replace(node, cleaned_node_source) + end + end + end + end + + def on_send(node) + field_config_with_inline_type(node) do |type_const| + return if ignored_node?(type_const) + type_const_str = get_type_argument_str(node, type_const) + if ignore_inline_type_str?(type_const_str) + # Do nothing -- not loading from another file + else + add_offense(type_const) do |corrector| + cleaned_node_source = delete_type_argument(node, type_const) + field_indent = determine_field_indent(node) + cleaned_node_source += " do\n#{field_indent} type #{type_const_str}\n#{field_indent}end" + corrector.replace(node, cleaned_node_source) + end + end + end + end + + + private + + def ignore_inline_type_str?(type_str) + BUILT_IN_SCALAR_NAMES.include?(type_str) + end + + def get_type_argument_str(send_node, type_const) + first_pos = type_const.location.expression.begin_pos + end_pos = type_const.location.expression.end_pos + node_source = send_node.source_range.source + node_first_pos = send_node.location.expression.begin_pos + + relative_first_pos = first_pos - node_first_pos + end_removal_pos = end_pos - node_first_pos + + node_source[relative_first_pos...end_removal_pos] + end + + def delete_type_argument(send_node, type_const) + first_pos = type_const.location.expression.begin_pos + end_pos = type_const.location.expression.end_pos + node_source = send_node.source_range.source + node_first_pos = send_node.location.expression.begin_pos + + relative_first_pos = first_pos - node_first_pos + end_removal_pos = end_pos - node_first_pos + + begin_removal_pos = relative_first_pos + while node_source[begin_removal_pos] != "," + begin_removal_pos -= 1 + if begin_removal_pos < 1 + raise "Invariant: somehow backtracked to beginning of node looking for a comma (node source: #{node_source.inspect})" + end + end + + node_source[0...begin_removal_pos] + node_source[end_removal_pos..-1] + end + + def determine_field_indent(send_node) + surrounding_node = send_node.parent.parent + surrounding_source = surrounding_node.source + indent_test_idx = send_node.location.expression.begin_pos - surrounding_node.source_range.begin_pos - 1 + field_indent = "".dup + while surrounding_source[indent_test_idx] == " " + field_indent << " " + indent_test_idx -= 1 + if indent_test_idx == 0 + raise "Invariant: somehow backtracted to beginning of class when looking for field indent (source: #{node_source.inspect})" + end + end + field_indent + end + end + end + end +end diff --git a/lib/graphql/schema/addition.rb b/lib/graphql/schema/addition.rb index e5c525d328..3a17687eee 100644 --- a/lib/graphql/schema/addition.rb +++ b/lib/graphql/schema/addition.rb @@ -189,6 +189,7 @@ def add_type(type, owner:, late_types:, path:) add_directives_from(type) if type.kind.fields? type.all_field_definitions.each do |field| + field.ensure_loaded name = field.graphql_name field_type = field.type.unwrap if !field_type.is_a?(GraphQL::Schema::LateBoundType) diff --git a/lib/graphql/schema/field.rb b/lib/graphql/schema/field.rb index b1863c9fd7..2ef5075ec4 100644 --- a/lib/graphql/schema/field.rb +++ b/lib/graphql/schema/field.rb @@ -146,11 +146,16 @@ def connection? Member::BuildType.to_type_name(@return_type_expr) elsif @resolver_class && @resolver_class.type Member::BuildType.to_type_name(@resolver_class.type) - else + elsif type # As a last ditch, try to force loading the return type: type.unwrap.name end - @connection = return_type_name.end_with?("Connection") && return_type_name != "Connection" + if return_type_name + @connection = return_type_name.end_with?("Connection") && return_type_name != "Connection" + else + # TODO set this when type is set by method + false # not loaded yet? + end else @connection end @@ -236,8 +241,8 @@ def initialize(type: nil, name: nil, owner: nil, null: nil, description: NOT_CON raise ArgumentError, "missing first `name` argument or keyword `name:`" end if !(resolver_class) - if type.nil? - raise ArgumentError, "missing second `type` argument or keyword `type:`" + if type.nil? && !block_given? + raise ArgumentError, "missing second `type` argument, keyword `type:`, or a block containing `type(...)`" end end @original_name = name @@ -302,6 +307,7 @@ def initialize(type: nil, name: nil, owner: nil, null: nil, description: NOT_CON @ast_node = ast_node @method_conflict_warning = method_conflict_warning @fallback_value = fallback_value + @definition_block = nil arguments.each do |name, arg| case arg @@ -320,26 +326,14 @@ def initialize(type: nil, name: nil, owner: nil, null: nil, description: NOT_CON @subscription_scope = subscription_scope @extensions = EMPTY_ARRAY - @call_after_define = false - # This should run before connection extension, - # but should it run after the definition block? - if scoped? - self.extension(ScopeExtension) - end - - # The problem with putting this after the definition_block - # is that it would override arguments - if connection? && connection_extension - self.extension(connection_extension) - end - + set_pagination_extensions(connection_extension: connection_extension) # Do this last so we have as much context as possible when initializing them: if extensions.any? - self.extensions(extensions) + self.extensions(extensions, call_after_define: false) end if resolver_class && resolver_class.extensions.any? - self.extensions(resolver_class.extensions) + self.extensions(resolver_class.extensions, call_after_define: false) end if directives.any? @@ -353,15 +347,28 @@ def initialize(type: nil, name: nil, owner: nil, null: nil, description: NOT_CON end if block_given? - if definition_block.arity == 1 - yield self + @definition_block = definition_block + else + self.extensions.each(&:after_define_apply) + end + end + + # Calls the definition block, if one was given. + # This is deferred so that references to the return type + # can be lazily evaluated, reducing Rails boot time. + # @return [self] + # @api private + def ensure_loaded + if @definition_block + if @definition_block.arity == 1 + @definition_block.call(self) else - instance_eval(&definition_block) + instance_eval(&@definition_block) end + self.extensions.each(&:after_define_apply) + @definition_block = nil end - - self.extensions.each(&:after_define_apply) - @call_after_define = true + self end attr_accessor :dynamic_introspection @@ -408,14 +415,14 @@ def description(text = nil) # # @param extensions [Array Hash>>] Add extensions to this field. For hash elements, only the first key/value is used. # @return [Array] extensions to apply to this field - def extensions(new_extensions = nil) + def extensions(new_extensions = nil, call_after_define: !@definition_block) if new_extensions new_extensions.each do |extension_config| if extension_config.is_a?(Hash) extension_class, options = *extension_config.to_a[0] - self.extension(extension_class, options) + self.extension(extension_class, call_after_define: call_after_define, **options) else - self.extension(extension_config) + self.extension(extension_config, call_after_define: call_after_define) end end end @@ -433,12 +440,12 @@ def extensions(new_extensions = nil) # @param extension_class [Class] subclass of {Schema::FieldExtension} # @param options [Hash] if provided, given as `options:` when initializing `extension`. # @return [void] - def extension(extension_class, options = nil) + def extension(extension_class, call_after_define: !@definition_block, **options) extension_inst = extension_class.new(field: self, options: options) if @extensions.frozen? @extensions = @extensions.dup end - if @call_after_define + if call_after_define extension_inst.after_define_apply end @extensions << extension_inst @@ -577,16 +584,29 @@ def default_page_size class MissingReturnTypeError < GraphQL::Error; end attr_writer :type - def type - if @resolver_class - return_type = @return_type_expr || @resolver_class.type_expr - if return_type.nil? - raise MissingReturnTypeError, "Can't determine the return type for #{self.path} (it has `resolver: #{@resolver_class}`, perhaps that class is missing a `type ...` declaration, or perhaps its type causes a cyclical loading issue)" + # Get or set the return type of this field. + # + # It may return nil if no type was configured or if the given definition block wasn't called yet. + # @param new_type [Module, GraphQL::Schema::NonNull, GraphQL::Schema::List] A GraphQL return type + # @return [Module, GraphQL::Schema::NonNull, GraphQL::Schema::List, nil] the configured type for this field + def type(new_type = NOT_CONFIGURED) + if NOT_CONFIGURED.equal?(new_type) + if @resolver_class + return_type = @return_type_expr || @resolver_class.type_expr + if return_type.nil? + raise MissingReturnTypeError, "Can't determine the return type for #{self.path} (it has `resolver: #{@resolver_class}`, perhaps that class is missing a `type ...` declaration, or perhaps its type causes a cyclical loading issue)" + end + nullable = @return_type_null.nil? ? @resolver_class.null : @return_type_null + Member::BuildType.parse_type(return_type, null: nullable) + elsif !@return_type_expr.nil? + @type ||= Member::BuildType.parse_type(@return_type_expr, null: @return_type_null) end - nullable = @return_type_null.nil? ? @resolver_class.null : @return_type_null - Member::BuildType.parse_type(return_type, null: nullable) else - @type ||= Member::BuildType.parse_type(@return_type_expr, null: @return_type_null) + @return_type_expr = new_type + # If `type` is set in the definition block, then the `connection_extension: ...` given as a keyword won't be used, hmm... + # Also, arguments added by `connection_extension` will clobber anything previously defined, + # so `type(...)` should go first. + set_pagination_extensions(connection_extension: self.class.connection_extension) end rescue GraphQL::Schema::InvalidDocumentError, MissingReturnTypeError => err # Let this propagate up @@ -897,6 +917,20 @@ def apply_own_complexity_to(child_complexity, query, nodes) raise ArgumentError, "Invalid complexity for #{self.path}: #{own_complexity.inspect}" end end + + def set_pagination_extensions(connection_extension:) + # This should run before connection extension, + # but should it run after the definition block? + if scoped? + self.extension(ScopeExtension, call_after_define: false) + end + + # The problem with putting this after the definition_block + # is that it would override arguments + if connection? && connection_extension + self.extension(connection_extension, call_after_define: false) + end + end end end end diff --git a/lib/graphql/schema/member/has_fields.rb b/lib/graphql/schema/member/has_fields.rb index ef18af8dc7..0b2a3fd7c4 100644 --- a/lib/graphql/schema/member/has_fields.rb +++ b/lib/graphql/schema/member/has_fields.rb @@ -121,7 +121,7 @@ def fields(context = GraphQL::Query::NullContext.instance) # 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)) - visible_fields[field_name] = f + visible_fields[field_name] = f.ensure_loaded end end end @@ -142,7 +142,7 @@ def get_field(field_name, context = GraphQL::Query::NullContext.instance) visible_interface_implementation?(ancestor, context, warden) && (f_entry = ancestor.own_fields[field_name]) && (skip_visible || (f_entry = Warden.visible_entry?(:visible_field?, f_entry, context, warden))) - return f_entry + return (skip_visible ? f_entry : f_entry.ensure_loaded) end i += 1 end @@ -161,7 +161,7 @@ def fields(context = GraphQL::Query::NullContext.instance) # 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)) - visible_fields[field_name] = f + visible_fields[field_name] = f.ensure_loaded end end end diff --git a/lib/graphql/schema/subset.rb b/lib/graphql/schema/subset.rb index dd28f653c0..f93a98fa4d 100644 --- a/lib/graphql/schema/subset.rb +++ b/lib/graphql/schema/subset.rb @@ -129,7 +129,7 @@ def initialize(context:, schema:) end.compare_by_identity @cached_fields = Hash.new do |h, owner| - h[owner] = non_duplicate_items(owner.all_field_definitions, @cached_visible_fields[owner]) + h[owner] = non_duplicate_items(owner.all_field_definitions.each(&:ensure_loaded), @cached_visible_fields[owner]) end.compare_by_identity @cached_arguments = Hash.new do |h, owner| @@ -213,13 +213,11 @@ def field(owner, field_name) end end end - visible_f + visible_f.ensure_loaded + elsif f && @cached_visible_fields[owner][f.ensure_loaded] + f else - if f && @cached_visible_fields[owner][f] - f - else - nil - end + nil end end @@ -446,6 +444,7 @@ def visit_type(type) # recurse into visible fields t_f = type.all_field_definitions t_f.each do |field| + field.ensure_loaded if @cached_visible[field] visit_directives(field) field_type = field.type.unwrap diff --git a/lib/graphql/schema/types_migration.rb b/lib/graphql/schema/types_migration.rb index 611819aedf..53de9dae77 100644 --- a/lib/graphql/schema/types_migration.rb +++ b/lib/graphql/schema/types_migration.rb @@ -165,6 +165,8 @@ def equivalent_schema_members?(member1, member2) equivalent_schema_members?(inner_member1, inner_member2) end when GraphQL::Schema::Field + member1.ensure_loaded + member2.ensure_loaded if member1.introspection? && member2.introspection? member1.inspect == member2.inspect else diff --git a/spec/fixtures/cop/.rubocop.yml b/spec/fixtures/cop/.rubocop.yml index 7c9a031b09..1bbb534636 100644 --- a/spec/fixtures/cop/.rubocop.yml +++ b/spec/fixtures/cop/.rubocop.yml @@ -10,3 +10,9 @@ GraphQL/DefaultNullTrue: GraphQL/DefaultRequiredTrue: Enabled: true + +GraphQL/FieldTypeInBlock: + Enabled: true + Include: + - field_type.rb + - field_type_autocorrect.rb diff --git a/spec/fixtures/cop/field_type.rb b/spec/fixtures/cop/field_type.rb new file mode 100644 index 0000000000..815d0ab6a8 --- /dev/null +++ b/spec/fixtures/cop/field_type.rb @@ -0,0 +1,18 @@ +class Types::Query + field :current_account, Types::Account, null: false, description: "The account of the current viewer" + + field :find_account, Types::Account do + argument :id, ID + end + + # Don't modify these: + field :current_time, String, description: "The current time in the viewer's timezone" + field :current_time, Integer, description: "The current time in the viewer's timezone" + field :current_time, Int, description: "The current time in the viewer's timezone" + field :current_time, Float, description: "The current time in the viewer's timezone" + field :current_time, Boolean, description: "The current time in the viewer's timezone" + + field(:all_accounts, [Types::Account, null: false]) { + argument :active, Boolean, default_value: false + } +end diff --git a/spec/fixtures/cop/field_type_corrected.rb b/spec/fixtures/cop/field_type_corrected.rb new file mode 100644 index 0000000000..bd4e66cde7 --- /dev/null +++ b/spec/fixtures/cop/field_type_corrected.rb @@ -0,0 +1,22 @@ +class Types::Query + field :current_account, null: false, description: "The account of the current viewer" do + type Types::Account + end + + field :find_account do + type Types::Account + argument :id, ID + end + + # Don't modify these: + field :current_time, String, description: "The current time in the viewer's timezone" + field :current_time, Integer, description: "The current time in the viewer's timezone" + field :current_time, Int, description: "The current time in the viewer's timezone" + field :current_time, Float, description: "The current time in the viewer's timezone" + field :current_time, Boolean, description: "The current time in the viewer's timezone" + + field(:all_accounts) { + type [Types::Account, null: false] + argument :active, Boolean, default_value: false + } +end diff --git a/spec/graphql/cop/field_type_in_block_spec.rb b/spec/graphql/cop/field_type_in_block_spec.rb new file mode 100644 index 0000000000..785bceb818 --- /dev/null +++ b/spec/graphql/cop/field_type_in_block_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe "GraphQL::Cop::FieldTypeInBlock" do + include RubocopTestHelpers + + it "finds and autocorrects field corrections with inline types" do + result = run_rubocop_on("spec/fixtures/cop/field_type.rb") + assert_equal 3, rubocop_errors(result) + + assert_includes result, <<-RUBY + field :current_account, Types::Account, null: false, description: "The account of the current viewer" + ^^^^^^^^^^^^^^ + RUBY + + assert_includes result, <<-RUBY + field :find_account, Types::Account do + ^^^^^^^^^^^^^^ + RUBY + + assert_includes result, <<-RUBY + field(:all_accounts, [Types::Account, null: false]) { + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + RUBY + + assert_rubocop_autocorrects_all("spec/fixtures/cop/field_type.rb") + end +end diff --git a/spec/graphql/schema/directive/one_of_spec.rb b/spec/graphql/schema/directive/one_of_spec.rb index ecf979a23f..ed9e90545b 100644 --- a/spec/graphql/schema/directive/one_of_spec.rb +++ b/spec/graphql/schema/directive/one_of_spec.rb @@ -16,7 +16,7 @@ field :one_of_field, output_type, null: false do argument :one_of_arg, this.one_of_input_object - end + end.ensure_loaded def one_of_field(one_of_arg:) one_of_arg diff --git a/spec/graphql/schema/field_spec.rb b/spec/graphql/schema/field_spec.rb index 12da959ab7..129900bc34 100644 --- a/spec/graphql/schema/field_spec.rb +++ b/spec/graphql/schema/field_spec.rb @@ -32,7 +32,7 @@ underscored_field = GraphQL::Schema::Field.from_options(:underscored_field, String, null: false, camelize: false, owner: nil) do argument :underscored_arg, String, camelize: false - end + end.ensure_loaded arg_name, arg_defn = underscored_field.arguments.first assert_equal 'underscored_arg', arg_name @@ -54,19 +54,45 @@ end it "accepts a block for definition" do + field_defn = nil object = Class.new(Jazz::BaseObject) do graphql_name "JustAName" - field :test, String do + field_defn = field :test do argument :test, String description "A Description." + type String end end + assert_nil field_defn.description, "The block isn't called right away" + assert_nil field_defn.type, "The block isn't called right away" + field_defn.ensure_loaded + assert_equal "String", field_defn.type.graphql_name + assert_equal "test", object.fields["test"].arguments["test"].name assert_equal "A Description.", object.fields["test"].description end + it "sets connection? when type is given in a block" do + field_defn = nil + Class.new(Jazz::BaseObject) do + graphql_name "JustAName" + + field_defn = field :instruments do + type Jazz::InstrumentType.connection_type + end + end + + assert_equal false, field_defn.connection? + assert_equal false, field_defn.scoped? + assert_equal [], field_defn.extensions + field_defn.ensure_loaded + assert_equal true, field_defn.scoped? + assert_equal true, field_defn.connection? + assert_equal [GraphQL::Schema::Field::ScopeExtension, GraphQL::Schema::Field::ConnectionExtension], field_defn.extensions.map(&:class) + end + it "accepts a block for definition and yields the field if the block has an arity of one" do object = Class.new(Jazz::BaseObject) do graphql_name "JustAName" @@ -345,7 +371,7 @@ def argument_details(argument_details:, arg1: nil, arg2:) field :complexityTest, String do complexity 'One hundred and eighty' - end + end.ensure_loaded end end @@ -359,7 +385,7 @@ def argument_details(argument_details:, arg1: nil, arg2:) field :complexityTest, String do complexity ->(one, two) { 52 } - end + end.ensure_loaded end end @@ -726,6 +752,7 @@ def ostruct_results field = GraphQL::Schema::Field.new(name: "blah", owner: nil, resolver_class: resolver, extras: [:blah]) do argument :a, GraphQL::Types::Int end + field.ensure_loaded assert_equal "description 1", field.description assert_equal "[Float!]", field.type.to_type_signature diff --git a/spec/support/dummy/schema.rb b/spec/support/dummy/schema.rb index d9b6eae59b..195ab6fb1a 100644 --- a/spec/support/dummy/schema.rb +++ b/spec/support/dummy/schema.rb @@ -410,7 +410,10 @@ def all_dairy(execution_error_at_index: nil) result end - field :all_edible, [Edible, null: true] + field :all_edible do + type [Edible, null: true] + end + def all_edible CHEESES.values + MILKS.values end @@ -474,7 +477,10 @@ def tracing_scalar ) end - field :deep_non_null, DeepNonNull, null: false + field :deep_non_null, null: false do + type(DeepNonNull) + end + def deep_non_null; :deep_non_null; end field :huge_integer, Integer