New Execution Module

[rmosolgo]

Taking a crack at #5507

Much inspiration taken from https://github.com/gmac/graphql-breadth-exec, as well as #5389 and the existing Interpreter::Runtime module.

I'm especially indebted to @gmac's work in https://github.com/gmac/graphql-breadth-exec for:

• The overall batched flow where list indices are ignored from execution's standpoint. I think I will run into trouble with Lazy/Dataloader support but it's brilliant so far.
• Propagating nulls during a second pass over the result only when some errors are present. This is awesome because it reduces the necessary bookkeeping at runtime and moves the overhead to the cases where an error is present (presumably less common and less important than the happy path). One downside here is that execution no longer stops on branches with errors. This will be documented as a "breaking" change.
• Using gather_selections to isolate mutations, it's perfect but I think it would have taken a while to click for me.

I plan to iterate on this in "draft" state until it's basically implementing the GraphQL spec, then merge it and continue working on it.

Known TODOs:

• Fix this failing test: SEED="61509" GRAPHQL_FUTURE=1 be rake test TEST=spec/graphql/execution/lazy_spec.rb
• Review tests & code to make sure changes are isolated to the new module
  • Do I still need Argument#original_keyword? See if that can be removed
• Revert any temporary changes to tests
• Review opt-in mechanisms and unify them somewhat ? I will do this later
• Fragment spreads
• Abstract-typed inline fragments
• Hook up static validation
• static input coercion
• leaf result coercion
• non-null enforcement
• variables
• input objects
• @skip/@include
• Introspection
• Mutation isolation
• Automatic connection argument handling somehow
• Resolver loads: compatiblity

[gmac]

The overall batched flow where list indices are ignored from execution's standpoint. I think I will run into trouble with Lazy/Dataloader support but it's brilliant so far

Breadth can do paths fairly efficiently if you index as you go. I've got a prototype floating around that builds the indices on-demand, and it's still pretty fast (though considerable slower than without them). Basically you just build a multi-layer index:

Scope {
   @indices = [
     [0, 0, 1, 1], # maps object in this scope to the one above it
     [0, 1, 2, 3], # maps positions in a first-order list
     [i, i, i, i]  # maps positions in a second-order list, etc...
   ]
}

With that done, you can alway map breadth objects by going to their position within their scope, reading that index position from bottom-up, insert the parent field key and transition to the parent scope indexing, then follow that one up and repeat.

We've never needed the indices badly enough to commit to adding them. If we do support a feature like @defer that needs runtime paths, we'd still probably only add them as an on-demand computation. We can do execution without paths, and it avoids developer temptations to do strange things based on tree order.

[rmosolgo]

index as you go

I read this last night and thought, "I'll try to understand it tomorrow morning after a cup of coffee." Now, here I am, and I still can't grok it 😆

Let me see ... @indices in the pseudo-code contains two kinds of entries:

• The first array links an item in the scope's own objects to an item in the parent scope's objects. It may contain duplicates if the object in the parent scope returned a list of items; in that case, multiple child objects have the same parent.

  The first array is always present.

• If a second array is present, it means that objects in this scope came from a list result of a parent object, and they indicate where, in the parent list, this object came from. IIUC, this second array may contain duplicates, for example:

  @objects 
  # [a, b, c, d, e, f]  <- given these 6 objects...
  @indices 
  # [
  #   [0, 0, 0, 1, 1, 1], <- from two parent objects, [obj0, obj1], each one returned 3 non-nil children
  #   [0, 1, 2, 0, 2, 3] <- obj0 returned [a, b, c]; obj1 returned [d, nil, e, f]; the nil was dropped
  # ]

• If a third array is present, it means that the current objects were members of nested arrays. The third array indicates where the object occurred in the nested array. The second array indicates where the nested array occurred in the top-level array. For example:

  @objects 
  # [a, b, c, d, e, f]  <- given these 6 objects...
  @indices 
  # [
  #   [0, 0, 0, 1, 1, 1], <- from two parent objects, [obj0, obj1], each one returned 3 non-nil children
  #   [0, 0, 1, 0, 1, 1] 
  #   [0, 1, 0, 0, 0, 1]<- obj0 returned [[a, b], [c]]; obj1 returned [[d, nil], [e, f]]; the nil was dropped
  # ]

Ok... did I get it?

Brilliant. You basically add 1 array per result set, minimum, plus an array per list depth. And it gives you what you need in order to build context[:current_path] on the fly. I'm not going to implement it on this branch but I will try -- preserving current_path would be awesome.

[rmosolgo]

breadth abort logic

Thanks for sharing this. I think there are specs which test aborting and when I break them, I will dig back into it!

[gmac]

So if I'm reading this sequence correctly, this is leaving money on the table with an implementation that still follows a depth-based mindset and overlooks one of breadth's superpowers. Basically – lazies/promises are expensive (promises seem to scale particularly badly as set sizes grow; I've benchmarked a 20x slowdown for 100 promises vs 1 using GraphQL::Batch / Cardinal lazy sets). They have both allocation and intermediary resolution cost; and here we seem to be following the depth pattern where we resolve a "batch" of promises, then still resort to resolving them each individually. In the breadth model, we can build and resolve lazys at a many-to-one standard rather than a one-to-one standard:

Following the above model, it looks like you've now consolidated all the resolver calls down into one (which is good), but you still end up with a set of lazies that each need to be resolved... enter good old depth-based slowness. So, breadth's superpower is to bind ALL the work to a single intermediary lazy that resolves the set later.

[gmac]

Also, doesn't appear to support multi-dimensional arrays? I'd expect recursion anytime we traverse an array result. But again, maybe I'm misreading what's happening here.

[rmosolgo]

I definitely follow that a single-promise approach would be the best. But for compatibility's sake, I'd like to continue supporting one-promise-per-object, especially if it can be done without slowing down the code when it's not in use.

The implementation here adds almost zero overhead when no lazy_resolve ... configurations are added, but it does check every object when such a configuration is added. Maybe the compatibility layer could be improved to have two levels:

• Promises everywhere (legacy): any result may be a Promise, or Array of Promises, or (mentioned elsewhere) an Array of Arrays of Promises, n-deep.
• Batch resolvers may return promises (new, better, and IIUC, what's described above): fields may return Promise instances.

Third, possibly even better option: fields returning promises must be flagged in their configuration, otherwise the object will be handled without checking.

Does the compatibility issue make a little sense of this implementation? What do you think about more granular support for Promises like that?

[gmac]

I don’t think you need that much configuration…? It looks like the way you have it, resolvers always return a mapped set. Cardinal permits two returns: mapped set, or field promise; so that’s a binary result evaluation on its own. The field promise must yield the mapped set of values. Sounds like you’ve got this configuration layer then to check if the resolved set should be scanned for lazies, which I get from a backwards-comp standpoint. If you’re serious about squeezing the juice though, that should unquestionably be off by default.

This is good validation on why this should exist as OSS independently. We know what WE need to be fast, and we’re better off without even having the option to compromise floating around.

Note that Cardinal does have its own proprietary loader that I believe was sketched out in the initial prototype (it’s a lot more sophisticated now, as is everything from that prototype aside from the core algorithm design). The loader is fairly unique in its design to manage and fulfill breadth set intersections. Would make sense as a core lib component.

[gmac]

did I get it?

Yep! You got it, I think. This is prototype code, we've never actually used it; we don't think we need current_path (our whole stack now runs without it!), and it's not worth the overhead to build inline indices for something with no legitimate internal uses. This prototype just indexes a Cardinal scope's subtree lazily on-demand, which is slower than if we'd built indices inline as part of flat-mapping results. Anywho – if it's helpful, here's the algorithm for reference. Might as well establish it as part of the OSS paradigm.

#  Builds paths to specific breadth object positions as exact object paths, ex:
#  - Scope path (namespace): ["products", "variants"]
#  - Object path (exact path): ["products", 0, "variants", 1]
#  Object pathing assembles breadth indices for all scopes ascending from the targeted scope, ex:
#  {
#    <TargetExecScope> => {
#      0 => [0, 1, 1, 2], # << zero index always maps objects in this scope to objects in the parent scope
#      1 => [0, 0, 1, 0], # << maps a first-order list in this scope
#      2 => [ ... ], # << maps a second-order list in this scope, etc...
#    },
#    <ParentExecScope> => { ... },
#  }
#  All numeric indexing arrays have size matching the number of objects in their scope.
#  To read an object path, we follow the Nth position of each scope's index map ascending up the scope tree.
#  Building these indices adds execution overhead that isn't needed regularly, so this is only done as part of deferred work.
class ObjectPathBuilder
  #: Hash[Executor::ExecutionScope, Hash[Integer, Array[Integer]]]
  attr_reader :indices_by_scope

  def initialize
    @indices_by_scope = Hash.new { |h1, scope| h1[scope] = Hash.new { |h2, index| h2[index] = [] } }.compare_by_identity
  end

  #: (Executor::ExecutionScope, Integer) -> error_path
  def object_path(exec_scope, index)
    current_path = []

    scope = exec_scope #: Executor::ExecutionScope?
    breadth_index = index
    while scope
      # index the scope unless it has already been done
      scope_indices = @indices_by_scope[scope]
      index_scope(scope, scope_indices) if scope_indices.empty?

      # loop backward through all the scope's indices...
      # - all scopes have at least one index that defines the parent scope position
      # - list scopes have additional indices for each layer of list wrapping
      i = scope_indices.length - 1
      while i >= 0
        if i.zero?
          # at the lowest index, recalibrate for the next highest scope
          breadth_index = scope_indices[i][breadth_index] #: as Integer
        else
          # higher indices add list positions to the current path
          current_path.prepend(scope_indices[i][breadth_index])
        end
        i -= 1
      end

      # before going up a scope, add the parent field key into current path
      key = scope.parent_field&.key
      current_path.prepend(key) if key
      scope = scope.parent
    end

    current_path
  end

  private

  #: (Executor::ExecutionScope, Hash[Integer, Array[Integer]]) -> void
  def index_scope(exec_scope, scope_indices)
    raise ArgumentError, "Scope must not be indexed" unless scope_indices.empty?
    raise ArgumentError, "Scope must be executed" unless exec_scope.executed?

    parent_objects = exec_scope.objects
    current_type = exec_scope.parent_type
    if (parent_field = exec_scope.parent_field)
      parent_objects = parent_field.result #: as !nil
      current_type = parent_field.type
    end

    object_path = []
    i = 0
    while i < parent_objects.length
      object_path[0] = i
      build_indices(current_type, parent_objects[i], object_path, scope_indices)
      i += 1
    end

    scope_indices.each(&:freeze)
    scope_indices.freeze
  end

  #: (singleton(GraphQL::Schema::Member), untyped, Array[Integer], Hash[Integer, Array[Integer]]) -> void
  def build_indices(current_type, object, object_path, next_indices)
    return if object.nil? || object.is_a?(ExecutionError)

    if current_type.list?
      raise ImplementationError, "Expected Array, got #{object.class}" unless object.is_a?(Array)

      current_type = Util.unwrap_non_null(current_type)

      i = 0
      while i < object.length
        object_path << i
        build_indices(current_type.of_type, object[i], object_path, next_indices)
        object_path.pop
        i += 1
      end
    else
      i = 0
      while i < object_path.length
        next_index = object_path[i] #: as !nil
        next_map = next_indices[i] #: as !nil
        next_map << next_index
        i += 1
      end
    end
  end
end

[gmac]

MySchema.batch_execute would be a better name, IMO. Or – I would encourage keeping the "breadth" nomenclature. There are so many "batching" utilities out there. Everything batches. Breadth describes the execution paradigm, which itself is considerably more than just delivering on some batching. This is particularly relevant in the GraphQL Ruby implementation where you may be choosing to give up features to run breadt-first. This is a lot different than, say, Air BnB’s Aquaduct where they openly acknowledged that “batch” resolvers are just a framework wrapper around dataloader patterns.

[gmac]

but it's worth it for the performance gain

... if your schema regularly resolves big lists. If you're a fairly basic SOAP service that just fetches single objects, then breadth batching is basically moot. I've even seen cases where a deep, flat tree of objects with no lists is about 5% faster in classic GraphQL Ruby because that's the case that recursion-based traversal is best optimized for. This trade-off is negligible though because without repetitions, execution was sufficiently fast and had no scaling factor, so losing 5% on practically nothing is a moot point.

[gmac]

I'd love to collaborate on this spot here... this is probably the messiest spot in Cardinal's core engine because we couldn't just isolate the interpretation logic into the shim. This section ended up turning into this to make our full test suites pass:

object = next_objects[i]

object_type = begin
  resolved_type, resolved_object = @query.resolve_type(abstract_type, object)
  if resolved_type && @query.schema.lazy?(resolved_type)
    resolved_type, resolved_object = @query.schema.sync_lazy(resolved_type)
  end
  if resolved_type.nil? || !possible_types.include?(resolved_type)
    err_class = abstract_type.const_get(:UnresolvedTypeError)
    type_error = err_class.new(resolved_object, exec_field.definition, abstract_type, resolved_type, possible_types)
    @query.schema.type_error(type_error, @context)
  end

  if resolved_object && object.object_id != resolved_object.object_id
    next_objects[i] = resolved_object
    object = resolved_object
  end
  resolved_type
end

Am I doing this wrong? How are you calling schema.resolve_type(@static_type, next_object, @runner.context) with a static type when the parent type we're holding is an abstract? We're trying to evaluate the static type, unless I'm missing something.

In general, the abstract type resolution process would be a nice one to collaborate on to make it faster and simpler. Our Node type has 800 implementations and it's slow to resolve... and digging into the code, it's a mess of layers that all contribute to assessing type resolution. I also think the "coerced type object" feature is something that should be deprecated in the fast path. Taking the litmus test of, "do you need this to do GraphQL" (ie: does graphql-js implement this?), the answer is no. It's a convenience. I think we're aligned that the future trajectory here says conveniences are treated as expensive add-ons. All that said, if we could get to one super simple fast path for type resolution and share it, that'd be 💰 💰.

FWIW: what I have in mind would be some kind of constants table pattern; something like this:

schema.abstracts = {
  Types::Node => {
    Product => Types::Product,
    Variant => Types::ProductVariant,
    File => ->(obj, ctx) { obj.video_url ? Types::Video : Types::Image }
  }
}

schema.resolve_type(abstract_type, object, context) do
  types_by_object_class = schema.abstracts[abstract_type]
  concrete_type = types_by_object_class[object.class]
  
  unless concrete_type
    concrete_type = types_by_object_class.each_value.find { object.is_a?(_1) }
  end

  concrete_type.is_a?(Proc) ? concrete_type.call(object, context) : concrete_type
end

[rmosolgo]

You're doing it right -- @query.resolve_type hits a cache (which made a big difference in the previous implementation, but I think it could still help in this implementation with fragment spreads). I just didn't have a Query instance in my first pass at implementing this.

static type

Maybe I'm misusing this term... I mean it like this:

query($id: ID!) {
  node(id: $id) {
    # What type is this object?
    # Statically, we know it's a `Node` based on the query 
    # Dynamically, we'll call `resolve_type` and determine the object type 
    # to populate the __typename filed below. 
     __typename 
  }

  currentUser {
    # What type is this object? 
    # Statically we know it's a User because that's `currentUser`'s return type 
    # At runtime, we won't bother calling `resolve_type`, instead we assume that 
    # the returned object will work fine. 
    username 
  }
}

What do you think of that distinction between "static" and "dynamic" (or "runtime") ?

"coerced type object" feature is something that should be deprecated in the fast path.

I am game for this, making it opt-in.

constants table

What's necessary to make this work, beyond implementing def self.resolve_type in the schema class? I would expect that any valid implementation could be dropped in there. Right now it probably still allocates an Array for the multiple return, I'm not sure.

[gmac]

What do you think of that distinction between "static" and "dynamic" (or "runtime") ?

aaah. I’d generally think the of those as composite_type versus object_type, using type_kinds.rb as the basis for nomenclature.

[gmac]

Benchmark this. We took it out because we found we were slower trying to do fancy optimization checks rather than just traversing the tree. It also made the code a lot simpler. As a point of trivia, this bubbling algorithm was lifted almost verbatim from the stitching gem's shaper lib. Full-circle!

[gmac]

Actually, here: our final tuned traversal logic that ended up passing all internal tests is this... we had to make a bunch of small tweaks, nuanced to the point that I don't remember them all, and subtle enough that it'll probably take a long time to find all the quirks without a giant monolith to give you feedback. Might as well just get it tuned now. I also adjusted the design so that it could be run repeatedly on partial object subtrees (@defer experiment) while maintaining a shared internal cache across runs.

class State
  #: Array[Hash[String, untyped]]
  attr_reader :errors

  #: error_path
  attr_reader :actual_path

  #: error_path
  attr_reader :base_path

  #: (?error_path) -> void
  def initialize(base_path = EMPTY_ARRAY)
    @base_path = base_path
    @actual_path = []
    @errors = []
  end

  #: -> error_path
  def current_path
    @base_path + @actual_path
  end
end

#: (
#|   executor: Executor,
#|   invalidated_results: Hash[untyped, ExecutionError],
#|   abstract_result_types: Hash[untyped, singleton(GraphQL::Schema::Object)],
#| ) -> void
def initialize(
  executor:,
  invalidated_results:,
  abstract_result_types:
)
  @executor = executor
  @context = executor.context
  @invalidated_results = invalidated_results
  @abstract_result_types = abstract_result_types
end

#: (
#|   singleton(GraphQL::Schema::Object),
#|   Array[GraphQL::Language::Nodes::AbstractNode],
#|   Hash[String, untyped],
#|   ?Array[String | Integer]
#| ) -> [Hash[String, untyped]?, Array[error_hash]]
def format_object(parent_type, selections, data, base_path = EMPTY_ARRAY)
  return [data, EMPTY_ARRAY] if @invalidated_results.empty?

  state = State.new(base_path)

  # Check if root data is invalidated (either inlined error or marked result)
  if (err = @invalidated_results[data])
    add_formatted_error(err, state)
    return [nil, state.errors]
  end

  data = propagate_object_scope_errors(
    data,
    parent_type,
    selections,
    state,
  )

  [data, state.errors]
end

private

#: (untyped, singleton(GraphQL::Schema::Object), Array[GraphQL::Language::Nodes::AbstractNode], State) -> untyped
def propagate_object_scope_errors(raw_object, parent_type, selections, state)
  return nil if raw_object.nil?

  selections.each do |node|
    case node
    when GraphQL::Language::Nodes::Field
      field_key = node.alias || node.name

      state.actual_path << field_key

      begin
        node_type = @context.types.field(parent_type, node.name).type
        named_type = node_type.unwrap
        raw_value = raw_object.fetch(field_key, Executor::UNDEFINED)

        # Aborted subtrees may have undefined fields that didn't execute.
        # Ignore these rather than considering them invalid by the schema.
        next if raw_value.equal?(Executor::UNDEFINED)

        # Check for invalidated positions (inlined errors or marked results)
        raw_object[field_key] = if (err = @invalidated_results[raw_value])
          add_formatted_error(err, state)
          nil
        elsif node_type.list?
          propagate_list_scope_errors(raw_value, node_type, node.selections, state)
        elsif named_type.kind.leaf?
          raw_value
        else
          propagate_object_scope_errors(raw_value, named_type, node.selections, state)
        end

        return nil if node_type.non_null? && raw_object[field_key].nil?
      ensure
        state.actual_path.pop
      end

    when GraphQL::Language::Nodes::InlineFragment
      fragment_type = node.type ? @context.types.type(node.type.name) : parent_type
      next unless result_of_type?(raw_object, parent_type, fragment_type)

      result = propagate_object_scope_errors(raw_object, fragment_type, node.selections, state)
      return nil if result.nil?
    when GraphQL::Language::Nodes::FragmentSpread
      fragment = @executor.fragments[node.name] #: as !nil
      fragment_type = @context.types.type(fragment.type.name)
      next unless result_of_type?(raw_object, parent_type, fragment_type)

      result = propagate_object_scope_errors(raw_object, fragment_type, fragment.selections, state)
      return nil if result.nil?
    else
      raise DocumentError, "Invalid selection node type"
    end
  end

  raw_object
end

#: (Array[untyped]?, singleton(GraphQL::Schema::Member), Array[GraphQL::Language::Nodes::AbstractNode], State) -> Array[untyped]?
def propagate_list_scope_errors(raw_list, current_node_type, selections, state)
  return nil if raw_list.nil?

  item_node_type = Util.unwrap_non_null(current_node_type).of_type
  named_type = item_node_type.unwrap

  resolved_list = raw_list.map!.with_index do |raw_list_element, index|
    state.actual_path << index

    begin
      # Check for invalidated positions (inlined errors or marked results)
      result = if (err = @invalidated_results[raw_list_element])
        add_formatted_error(err, state)
        nil
      elsif item_node_type.list?
        propagate_list_scope_errors(raw_list_element, item_node_type, selections, state)
      elsif named_type.kind.leaf?
        raw_list_element
      else
        propagate_object_scope_errors(raw_list_element, named_type, selections, state)
      end

      return nil if result.nil? && item_node_type.non_null?

      result
    ensure
      state.actual_path.pop
    end
  end

  resolved_list
end

#: (untyped, singleton(GraphQL::Schema::Member), singleton(GraphQL::Schema::Member)) -> bool
def result_of_type?(result, current_type, inquiry_type)
  # result_type must be concrete...
  result_type = current_type.kind.abstract? ? @abstract_result_types[result] : current_type
  raise ImplementationError, "No type annotation recorded for abstract result" if result_type.nil?

  if inquiry_type.kind.abstract?
    # abstract inquiry contains the concrete result type?
    @context.types.possible_types(inquiry_type).include?(result_type)
  else
    # concrete result type matches the concrete inquiry type?
    result_type == inquiry_type
  end
end

[gmac]

Tests to pass:

class ErrorResultFormatterTest < Minitest::Test
TEST_RESOLVERS = {
  "Node" => {
    "id" => HashKeyResolver.new("id"),
    "__type__" => ->(obj, ctx) { ctx.types.type(obj["__typename__"]) },
  },
  "Test" => {
    "id" => HashKeyResolver.new("id"),
    "req" => HashKeyResolver.new("req"),
    "opt" => HashKeyResolver.new("opt"),
  },
  "Query" => {
    "node" => HashKeyResolver.new("node"),
    "test" => HashKeyResolver.new("test"),
    "reqField" => HashKeyResolver.new("reqField"),
    "anotherField" => HashKeyResolver.new("anotherField"),
  },
}.freeze

def test_basic_object_structure
  schema = "type Test { req: String! opt: String } type Query { test: Test }"
  source = {
    "test" => {
      "req" => "yes",
      "opt" => nil
    }
  }
  expected = {
    "data" => {
      "test" => {
        "req" => "yes",
        "opt" => nil
      }
    }
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_errors_render_above_data_in_result
  schema = "type Test { req: String! opt: String } type Query { test: Test }"
  source = { "test" => { "req" => nil } }

  assert_equal ["errors", "data"], exec_test(schema, "{ test { req } }", source).keys
end

def test_bubbles_null_for_single_object_scopes
  schema = "type Test { req: String! opt: String } type Query { test: Test }"
  source = {
    "test" => {
      "req" => nil,
      "opt" => "yes"
    },
  }
  expected = {
    "data" => {
      "test" => nil,
    },
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubbles_null_for_nested_non_null_object_scopes
  schema = "type Test { req: String! opt: String } type Query { test: Test! }"
  source = {
    "test" => {
      "req" => nil,
      "opt" => "yes"
    }
  }
  expected = {
    "data" => nil,
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_basic_list_structure
  schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
  source = {
    "test" => [
      { "req" => "yes", "opt" => nil },
      { "req" => "yes", "opt" => "yes" },
    ],
  }
  expected = {
    "data" => {
      "test" => [
        { "req" => "yes", "opt" => nil },
        { "req" => "yes", "opt" => "yes" },
      ],
    },
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubbles_null_for_list_elements
  schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
  source = {
    "test" => [
      { "req" => "yes", "opt" => nil },
      { "req" => nil, "opt" => "yes" },
    ],
  }
  expected = {
    "data" => {
      "test" => [
        { "req" => "yes", "opt" => nil },
        nil,
      ],
    },
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", 1, "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubbles_null_for_required_list_elements
  schema = "type Test { req: String! opt: String } type Query { test: [Test!] }"
  source = {
    "test" => [
      { "req" => "yes", "opt" => nil },
      { "req" => nil, "opt" => "yes" },
    ]
  }
  expected = {
    "data" => {
      "test" => nil,
    },
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", 1, "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubbles_null_for_required_lists
  schema = "type Test { req: String! opt: String } type Query { test: [Test!]! }"
  source = {
    "test" => [
      { "req" => "yes", "opt" => nil },
      { "req" => nil, "opt" => "yes" },
    ],
  }
  expected = {
    "data" => nil,
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", 1, "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_basic_nested_list_structure
  schema = "type Test { req: String! opt: String } type Query { test: [[Test]] }"
  source = {
    "test" => [
      [{ "req" => "yes", "opt" => nil }],
      [{ "req" => "yes", "opt" => "yes" }],
    ],
  }
  expected = {
    "data" => {
      "test" => [
        [{ "req" => "yes", "opt" => nil }],
        [{ "req" => "yes", "opt" => "yes" }],
      ],
    },
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubbles_null_for_nested_list_elements
  schema = "type Test { req: String! opt: String } type Query { test: [[Test]] }"
  source = {
    "test" => [
      [{ "req" => "yes", "opt" => nil }],
      [{ "req" => nil, "opt" => "yes" }],
    ],
  }
  expected = {
    "data" => {
      "test" => [
        [{ "req" => "yes", "opt" => nil }],
        [nil],
      ],
    },
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", 1, 0, "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubbles_null_for_nested_required_list_elements
  schema = "type Test { req: String! opt: String } type Query { test: [[Test!]] }"
  source = {
    "test" => [
      [{ "req" => "yes", "opt" => nil }],
      [{ "req" => nil, "opt" => "yes" }],
    ],
  }
  expected = {
    "data" => {
      "test" => [
        [{ "req" => "yes", "opt" => nil }],
        nil,
      ],
    },
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", 1, 0, "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubbles_null_for_inner_required_lists
  schema = "type Test { req: String! opt: String } type Query { test: [[Test!]!] }"
  source = {
    "test" => [
      [{ "req" => "yes", "opt" => nil }],
      [{ "req" => nil, "opt" => "yes" }],
    ],
  }
  expected = {
    "data" => {
      "test" => nil,
    },
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", 1, 0, "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubbles_null_through_nested_required_list_scopes
  schema = "type Test { req: String! opt: String } type Query { test: [[Test!]!]! }"
  source = {
    "test" => [
      [{ "req" => "yes", "opt" => nil }],
      [{ "req" => nil, "opt" => "yes" }],
    ],
  }
  expected = {
    "data" => nil,
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", 1, 0, "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubble_through_inline_fragment
  schema = "type Test { req: String! opt: String } type Query { test: Test }"
  source = {
    "test" => {
      "req" => nil,
      "opt" => nil
    },
  }
  expected = {
    "data" => {
      "test" => nil,
    },
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_bubble_through_fragment_spreads
  schema = "type Test { req: String! opt: String } type Query { test: Test }"
  source = {
    "test" => {
      "req" => nil,
      "opt" => nil
    },
  }
  expected = {
    "data" => {
      "test" => nil,
    },
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.req",
      "path" => ["test", "req"],
      "locations" => [{ "line" => 1, "column" => 10 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_inline_errors_in_null_positions_report
  schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
  source = {
    "test" => [
      { "req" => "yes", "opt" => nil },
      { "req" => "yes", "opt" => GraphQL::Cardinal::ExecutionError.new("Not okay!") },
    ],
  }
  expected = {
    "data" => {
      "test" => [
        { "req" => "yes", "opt" => nil },
        { "req" => "yes", "opt" => nil },
      ],
    },
    "errors" => [{
      "message" => "Not okay!",
      "locations" => [{ "line" => 1, "column" => 14 }],
      "path" => ["test", 1, "opt"],
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)

  inline_fragment_errors = [{
    "message" => "Not okay!",
    "locations" => [{ "line" => 1, "column" => 28 }],
    "path" => ["test", 1, "opt"],
  }]

  result = exec_test(schema, "{ ...on Query { test { req opt } } }", source)
  assert_equal expected["data"], result["data"]
  assert_equal inline_fragment_errors, result["errors"]

  fragment_errors = [{
    "message" => "Not okay!",
    "locations" => [{ "line" => 1, "column" => 59 }],
    "path" => ["test", 1, "opt"],
  }]

  result = exec_test(schema, "{ ...Selection } fragment Selection on Query { test { req opt } }", source)
  assert_equal expected["data"], result["data"]
  assert_equal fragment_errors, result["errors"]
end

def test_abstract_fragments_on_concrete_results_interpret_type
  schema = %|
    interface Node {
      id: ID!
    }
    type Test implements Node {
      id: ID!
    }
    type Query {
      node: Node
      test: Test
    }
  |

  query = %|
    query {
      test {
        ... on Node { id }
        ... NodeAttrs
      }
    }
    fragment NodeAttrs on Node { id }
  |

  source = {
    "test" => {},
  }

  expected = {
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.id",
      "locations" => [{ "line" => 4, "column" => 31 }, { "line" => 8, "column" => 42 }],
      "extensions" => { "code" => "INVALID_NULL" },
      "path" => ["test", "id"],
    }],
    "data" => { "test" => nil },
  }

  assert_equal expected, exec_test(schema, query, source)
end

def test_concrete_fragments_on_abstract_results_interpret_type
  schema = %|
    interface Node {
      id: ID!
    }
    type Test implements Node {
      id: ID!
    }
    type Query {
      node: Node
      test: Test
    }
  |

  query = %|
    query {
      node {
        ... on Test { id }
        ... TestAttrs
      }
    }
    fragment TestAttrs on Test { id }
  |

  source = {
    "node" => { "__typename__" => "Test" },
  }

  expected = {
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Test.id",
      "locations" => [{ "line" => 4, "column" => 31 }, { "line" => 8, "column" => 42 }],
      "extensions" => { "code" => "INVALID_NULL" },
      "path" => ["node", "id"],
    }],
    "data" => { "node" => nil },
  }

  assert_equal expected, exec_test(schema, query, source)
end

def test_inline_errors_in_non_null_positions_report_and_propagate
  schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
  source = {
    "test" => [
      { "req" => "yes", "opt" => nil },
      { "req" => GraphQL::Cardinal::ExecutionError.new("Not okay!"), "opt" => nil },
    ],
  }
  expected = {
    "data" => {
      "test" => [
        { "req" => "yes", "opt" => nil },
        nil,
      ],
    },
    "errors" => [{
      "message" => "Not okay!",
      "locations" => [{ "line" => 1, "column" => 10 }],
      "path" => ["test", 1, "req"],
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_multiple_offenses_for_null_position_report_all_instances
  schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
  source = {
    "test" => [
      { "req" => "yes", "opt" => nil },
      { "req" => "yes", "opt" => GraphQL::Cardinal::ExecutionError.new("Not okay!") },
      { "req" => "yes", "opt" => GraphQL::Cardinal::ExecutionError.new("Not okay!") },
    ],
  }
  expected = {
    "data" => {
      "test" => [
        { "req" => "yes", "opt" => nil },
        { "req" => "yes", "opt" => nil },
        { "req" => "yes", "opt" => nil },
      ],
    },
    "errors" => [{
      "message" => "Not okay!",
      "locations" => [{ "line" => 1, "column" => 14 }],
      "path" => ["test", 1, "opt"],
    }, {
      "message" => "Not okay!",
      "locations" => [{ "line" => 1, "column" => 14 }],
      "path" => ["test", 2, "opt"],
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_multiple_offenses_for_non_null_position_without_intersecting_propagation_report_all_instances
  schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
  source = {
    "test" => [
      { "req" => "yes", "opt" => nil },
      { "req" => GraphQL::Cardinal::ExecutionError.new("Not okay!"), "opt" => "yes" },
      { "req" => GraphQL::Cardinal::ExecutionError.new("Not okay!"), "opt" => "yes" },
    ],
  }
  expected = {
    "data" => {
      "test" => [
        { "req" => "yes", "opt" => nil },
        nil,
        nil,
      ],
    },
    "errors" => [{
      "message" => "Not okay!",
      "locations" => [{ "line" => 1, "column" => 10 }],
      "path" => ["test", 1, "req"],
    }, {
      "message" => "Not okay!",
      "locations" => [{ "line" => 1, "column" => 10 }],
      "path" => ["test", 2, "req"],
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_multiple_offenses_for_non_null_position_with_intersecting_propagation_report_first_instance
  schema = "type Test { req: String! opt: String } type Query { test: [Test!] }"
  source = {
    "test" => [
      { "req" => "yes", "opt" => nil },
      { "req" => GraphQL::Cardinal::ExecutionError.new("first"), "opt" => "yes" },
      { "req" => GraphQL::Cardinal::ExecutionError.new("second"), "opt" => "yes" },
    ],
  }
  expected = {
    "data" => {
      "test" => nil,
    },
    "errors" => [{
      "message" => "first",
      "locations" => [{ "line" => 1, "column" => 10 }],
      "path" => ["test", 1, "req"],
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end

def test_multiple_locations_for_duplicate_field_selections
  schema = "type Query { reqField: String! }"
  source = {
    "reqField" => nil,
  }

  query = <<~GRAPHQL
    {
      reqField
      reqField
    }
  GRAPHQL

  expected = {
    "data" => nil,
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Query.reqField",
      "extensions" => { "code" => "INVALID_NULL" },
      "path" => ["reqField"],
      "locations" => [
        { "line" => 2, "column" => 3 },
        { "line" => 3, "column" => 3 },
      ],
    }],
  }

  assert_equal expected, exec_test(schema, query, source)
end

def test_multiple_locations_with_fragments
  schema = "type Query { reqField: String! anotherField: String }"
  source = {
    "reqField" => nil,
    "anotherField" => "value",
  }

  query = <<~GRAPHQL
    {
      reqField
      ...Fields
    }

    fragment Fields on Query {
      reqField
      anotherField
    }
  GRAPHQL

  expected = {
    "data" => nil,
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Query.reqField",
      "extensions" => { "code" => "INVALID_NULL" },
      "path" => ["reqField"],
      "locations" => [
        { "line" => 2, "column" => 3 },
        { "line" => 7, "column" => 3 },
      ],
    }],
  }

  assert_equal expected, exec_test(schema, query, source)
end

def test_multiple_locations_with_inline_fragments
  schema = "type Query { reqField: String! }"
  source = {
    "reqField" => nil,
  }

  query = <<~GRAPHQL
    {
      reqField
      ... on Query {
        reqField
      }
    }
  GRAPHQL

  expected = {
    "data" => nil,
    "errors" => [{
      "message" => "Cannot return null for non-nullable field Query.reqField",
      "extensions" => { "code" => "INVALID_NULL" },
      "path" => ["reqField"],
      "locations" => [
        { "line" => 2, "column" => 3 },
        { "line" => 4, "column" => 5 },
      ],
    }],
  }

  assert_equal expected, exec_test(schema, query, source)
end

def test_formats_errors_with_extensions
  schema = "type Query { test: String! }"
  source = {
    "test" => GraphQL::Cardinal::ExecutionError.new("Not okay!", extensions: {
      "code" => "TEST",
      reason: "sorry",
    })
  }
  expected = {
    "data" => nil,
    "errors" => [{
      "message" => "Not okay!",
      "locations" => [{ "line" => 1, "column" => 3 }],
      "extensions" => { "code" => "TEST", "reason" => "sorry" },
      "path" => ["test"],
    }],
  }

  assert_equal expected, exec_test(schema, "{ test }", source)
end

def test_formats_error_message_for_non_null_list_items
  schema = "type Test { req: String! } type Query { test: [Test!]! }"
  source = {
    "test" => [nil],
  }
  expected = {
    "data" => nil,
    "errors" => [{
      "message" => "Cannot return null for non-nullable element of type 'Test!' for Query.test",
      "path" => ["test", 0],
      "locations" => [{ "line" => 1, "column" => 3 }],
      "extensions" => { "code" => "INVALID_NULL" },
    }],
  }

  assert_equal expected, exec_test(schema, "{ test { req } }", source)
end

[rmosolgo]

Thanks for this awesome detailed spec. I adapted it and hacked in some fixes in 3b5825a.

One spec still evades me:

  1) Failure:
ErrorResultFormatterTest#test_multiple_offenses_for_non_null_position_with_intersecting_propagation_report_first_instance [spec/graphql/execution/batching/errors_spec.rb:593]:
--- expected
+++ actual
@@ -1 +1 @@
-{"data" => {"test" => nil}, "errors" => [{"message" => "first", "locations" => [{"line" => 1, "column" => 10}], "path" => ["test", 1, "req"]}]}
+#<GraphQL::Query::Result @query=... @to_h={"errors" => [{"message" => "first", "locations" => [{"line" => 1, "column" => 10}], "path" => ["test", 1, "req"]}, {"message" => "second", "locations" => [{"line" => 1, "column" => 10}], "path" => ["test", "req"]}], "data" => {"test" => nil}}>

It looks like it expects the first error in the list to be present in the response, but not the second error.

I reviewed the "Handling Execution Errors" in the spec and didn't immediately find a rationale for it. What's the inspiration for that spec?

[gmac]

What's the inspiration for that spec?

Aaaah. Uncharted territory here... that test involves subtree abort sequences, which weren't in the POC. So what happens there is we have multiple failures across the breadth of the request which all executed at once. HOWEVER, the later failure would never have been discovered in a depth traversal because execution would have aborted during the first failed subtree. So, there are different ways you could handle this, which the spec never considers:

1. Report all discovered errors, regardless of where they are based on execution order.
2. Follow the spec implementation and only report errors expected from depth-first perspective.

I went with the second option in blind interests of making our full test suite pass verbatim, but you could certainly argue that the first option is more correct. I think the second is correct though, because frequently the entire breadth of a field will all fail with the same error (which we obviously don't want to report).

To make the second option work, we decoupled execution errors from reported errors – which actually makes execution errors WAY simpler: the executor can just report errors left and right with complete disregard for their "correctness". It over-reports. Then the error formatter pass combs through the error log and selects which errors to report in the result; and we tuned that to behave like a conventional depth execution engine for consistency with existing tests.

Like I said... uncharted territory. Breadth is weird. And fun! :)

[gmac]

This needs to be wrapped in some kind of execute_with_directives block to address operation-level directives. Setup an issue if you want to get into how runtime directives work in breadth, or at least how we solved them.

[rmosolgo]

Sure, I'll look into it when I get to supporting this 👍

[gmac]

Suggested change

	@path ||= [*@selections_step.path, @key].freeze
	@path ||= (@selections_step.path + @key).freeze

Slightly faster...

[rmosolgo]

Hmm, this 💥s in my code:

TypeError: no implicit conversion of String into Array
    lib/graphql/execution/batching/field_resolve_step.rb:30:in 'Array#+'
    lib/graphql/execution/batching/field_resolve_step.rb:30:in 'GraphQL::Execution::Batching::FieldResolveStep#path'

@selections_step.path is an Array of Strings, @key is another String. I need a new array including all members of the previous .path, plus @key in the end. I would have done:

@path ||= begin
  new_path = @selections_step.path.dup 
  new_path << @key 
  new_path.freeze
end 

But I saw the [*..., @key] approach in graphql-breadth-exec and copied it 😅 . As far as I can tell it's doing what I want: creating exactly one new object.

Or, is there another way that + can be made to work?

[gmac]

Oh, ha, you’re not always using arrays. Right. Ours is always an array. Claude coached us that the dynamic spread created more objects than simple concatenation, so we’ve been updating the pattern where we see it. It’s not that big a deal. Micro-ops, though these are all the little corners that cut out garbage