Merge branch 'master' into lazy-root-types

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"

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

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)

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<Class, Hash<Class => Hash>>] Add extensions to this field. For hash elements, only the first key/value is used.
       # @return [Array<GraphQL::Schema::FieldExtension>] 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

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

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