Skip to content

Commit 59218e1

Browse files
committed
Let a lookahead resolver record an error for a descendant field, closes #1340
- QueryContext#record_lookahead_error lets any lookahead-driven resolver flag itself or a descendant lookahead node as invalid without failing the recording field's whole subtree (which is what raising there would do). - LookaheadErrors keys each record by the flagged node's absolute, index-free response path, found by walking down from context.query.lookahead; a field matches when its current_path, minus list indices, equals that path. See docs/adr/0001-lookahead-error-record-key.md for why that key. - Misuse raises Errors::ConfigError at the call site: a node that isn't selected at or beneath the field being resolved (a node from an unrelated part of the query, or a typo'd Lookahead#selection name, which yields a null object) can't be recorded. Nothing to detect after the fact. - GraphQLAdapterBuilder's resolver lambdas check for a matching record before resolving and return a GraphQL::ExecutionError (halting just that field/subtree) instead of running the registered resolver. - Replaces the two-spot approximatePercentile workaround: Aggregation::QueryAdapter#computation_for now records the error instead of silently omitting the computation, and AggregatedValues#resolve no longer needs its own validation/error-path logic. Because the key ignores list indices, one record can match many response positions -- every bucket of an aggregation, say -- each producing its own error at its own path, which is what lets a client tell an errored null from a real one. That's also why datastore query memoization needs no involvement: a cache hit implies identical lookahead.ast_nodes (which hash by identity), so the response path recorded on the miss still applies. Generated with Claude Code
1 parent 3f1a97c commit 59218e1

14 files changed

Lines changed: 583 additions & 46 deletions

File tree

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
# Identify lookahead errors by response path, not by field or AST node
2+
3+
A resolver that builds its datastore query from a lookahead can discover that a *descendant* field is
4+
invalid (e.g. `approximatePercentile(percentile: 150)`). It cannot raise `GraphQL::ExecutionError` for
5+
that descendant — the raise would fail the recording field's entire subtree instead of the one bad
6+
leaf. So the recording field records a **lookahead error** and ElasticGraph fails the flagged field
7+
when it is resolved. This ADR records how a recorded error is matched to the field it is about.
8+
9+
Terms used here:
10+
11+
- **Recording field** — the field whose resolver holds the lookahead and observes the problem.
12+
- **Flagged node** — the lookahead node, at any depth beneath the recording field, that the recording
13+
field identified as erroneous.
14+
- **Lookahead error** — an error about a flagged node, discovered by a recording field rather than by
15+
the flagged node's own resolver.
16+
17+
**Decision**: a record is keyed by the flagged node's **absolute, index-free response path**, found by
18+
walking down from the query's root lookahead (`context.query.lookahead`) to the flagged node's AST
19+
nodes. At resolve time a field matches if its `context.current_path`, with list indices dropped, equals
20+
that path.
21+
22+
## Considered Options
23+
24+
| Option | Why rejected |
25+
|---|---|
26+
| `[schema field, args]` | A schema field is static, so it cannot distinguish an invalid selection from a valid one of the same field elsewhere in the query. |
27+
| AST node identity | Would require the `:ast_node` extra on every field ElasticGraph resolves — a cost paid by every query for a rare feature. |
28+
| Recording field's exact path (indices included) + relative path | Expresses a per-list-element distinction that cannot arise (see below), at the cost of the recording field having to bind its records to a path, which datastore query memoization then has to participate in. |
29+
30+
## Consequences
31+
32+
- **The caller passes only the flagged node**, and the path is derived from `context.current_path`, so
33+
there is no way to misidentify the recording field. A node that isn't selected at or beneath
34+
`current_path` — a typo'd `Lookahead#selection` name, or a node from an unrelated part of the query —
35+
raises `Errors::ConfigError` at the point of misuse. That leaves nothing to detect after the fact.
36+
- **A record may match several response positions.** A list *between* the query root and the flagged
37+
node collapses its indices, so one record can match every bucket of an aggregation. Each match
38+
produces its own error with its own path, which is what the [GraphQL
39+
spec](https://spec.graphql.org/October2021/#sec-Errors) requires: an execution error occurs at a
40+
specific response position, and its `path` is what lets clients tell an errored `null` from a real
41+
one. Nothing is lost, because a lookahead error is a property of the query, not of the data — a
42+
recording field cannot legitimately mean "bucket 1 only." `Resolvers::QueryAdapter` enforces as much
43+
for datastore query adapters, which must be pure functions of `(field, args, lookahead)`.
44+
- **A record may match nothing**, legitimately, when execution never reaches the flagged position
45+
(e.g. the recording field resolved to an empty list). Nothing to report: unlike a raised error, an
46+
unreached lookahead error is simply inert.
47+
- **A node can flag itself, not just a descendant**, since its own path matches. This lets any
48+
`:lookahead`-accepting resolver report its own invalid args through the same mechanism it would use
49+
for a descendant (by returning `QueryContext#matching_lookahead_error`) rather than raising — handy
50+
when the resolver wants one consistent way to fail a field.
51+
- **Datastore query memoization needs no involvement.** Queries are memoized on
52+
`(field, args, lookahead)`, so on a cache hit the build — and its recording — is skipped. That's
53+
correct precisely because the key is index-free: a cache hit implies identical `lookahead.ast_nodes`
54+
(which hash by identity), hence the same document position, hence the same index-free path as the
55+
record made on the miss.

elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/function_adapter.rb

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@ module Aggregation
1616
# the datastore omitted.
1717
#
1818
# When an adapter's `extract_args` is given args it considers invalid, it yields an error
19-
# message instead of returning extracted args. Each caller passes a block that exits
20-
# non-locally (the args of an invalid field are never used), which lets a field with invalid
21-
# args be handled without disrupting the sibling fields being processed alongside it.
19+
# message instead of returning extracted args. The caller (`QueryAdapter#computation_for`)
20+
# passes a block that exits non-locally (the args of an invalid field are never used), which
21+
# lets a field with invalid args be handled without disrupting the sibling fields being
22+
# processed alongside it.
2223
#
2324
# @private
2425
module FunctionAdapter

elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/query_adapter.rb

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ def call(query:, lookahead:, args:, field:, context:)
6363
aggregation_query = build_aggregation_query_for(
6464
aggregations_node,
6565
field: field,
66+
context: context,
6667
grouping_adapter: CompositeGroupingAdapter,
6768
# Filters on root aggregations applied to the search query body itself instead of
6869
# using a filter aggregation, like sub-aggregations do, so we don't want a filter
@@ -95,7 +96,7 @@ def extract_aggregation_node(lookahead, field, graphql_query)
9596
)
9697
end
9798

98-
def build_aggregation_query_for(aggregations_node, field:, grouping_adapter:, nested_path: [], unfiltered: false)
99+
def build_aggregation_query_for(aggregations_node, field:, context:, grouping_adapter:, nested_path: [], unfiltered: false)
99100
aggregation_name = name_of(_ = aggregations_node.ast_nodes.first)
100101

101102
# Get the AST node for the `nodes` subfield (e.g. from `fooAggregations { nodes { ... } }`)
@@ -130,8 +131,8 @@ def build_aggregation_query_for(aggregations_node, field:, grouping_adapter:, ne
130131
Query.new(
131132
name: aggregation_name,
132133
groupings: build_groupings_from(node_node, aggregation_name, from_field_path: nested_path),
133-
computations: build_computations_from(node_node, from_field_path: nested_path),
134-
sub_aggregations: build_sub_aggregations_from(node_node, parent_nested_path: nested_path),
134+
computations: build_computations_from(node_node, context: context, from_field_path: nested_path),
135+
sub_aggregations: build_sub_aggregations_from(node_node, context: context, parent_nested_path: nested_path),
135136
needs_doc_count: count_detail_node.selected? || node_node.selects?(element_names.count),
136137
needs_doc_count_error: needs_doc_count_error,
137138
paginator: build_paginator_for(aggregations_node),
@@ -179,22 +180,22 @@ def transform_node_to_clauses(node, parent_path: [], &clause_builder)
179180
end
180181
end
181182

182-
def build_computations_from(node_node, from_field_path: [])
183+
def build_computations_from(node_node, context:, from_field_path: [])
183184
aggregated_values_node = node_node.selection(element_names.aggregated_values)
184185

185186
build_clauses_from(aggregated_values_node) do |node, field, field_path|
186187
if field.aggregated?
187188
field_path = from_field_path + field_path
188-
get_children_nodes(node).filter_map { |fn_node| computation_for(fn_node, field_path) }
189+
get_children_nodes(node).filter_map { |fn_node| computation_for(fn_node, field_path, context: context) }
189190
end
190191
end
191192
end
192193

193194
# Builds the `Computation` for an aggregated value function node, or returns `nil` if the node
194-
# has invalid args (e.g. an out-of-range `percentile`). We omit the computation rather than
195-
# failing the whole aggregations field here: the resolver detects the same invalid args when it
196-
# resolves this specific field, and can attribute the error to that field's precise path.
197-
def computation_for(fn_node, field_path)
195+
# has invalid args (e.g. an out-of-range `percentile`). We record a lookahead error (rather
196+
# than failing the whole aggregations field here) and omit the computation; ElasticGraph fails
197+
# just this one leaf, at its precise path, when GraphQL execution reaches it.
198+
def computation_for(fn_node, field_path, context:)
198199
computed_field = field_from_node(fn_node)
199200
function_adapter = computed_field.function_adapter # : FunctionAdapter::adapter
200201
args = computed_field.args_to_schema_form(fn_node.arguments)
@@ -203,7 +204,10 @@ def computation_for(fn_node, field_path)
203204
source_field_path: field_path,
204205
leaf: PathSegment.for(field: computed_field, lookahead: fn_node),
205206
function_adapter: function_adapter,
206-
function_args: function_adapter.extract_args(args, element_names) { return nil }
207+
function_args: function_adapter.extract_args(args, element_names) do |message|
208+
context.record_lookahead_error(fn_node, message)
209+
return nil
210+
end
207211
)
208212
end
209213

@@ -306,7 +310,7 @@ def name_of(ast_node)
306310
ast_node.alias || ast_node.name
307311
end
308312

309-
def build_sub_aggregations_from(node_node, parent_nested_path: [])
313+
def build_sub_aggregations_from(node_node, context:, parent_nested_path: [])
310314
key_sub_agg_pairs =
311315
build_clauses_from(node_node.selection(element_names.sub_aggregations)) do |node, field, field_path|
312316
if field.type.elasticgraph_category == :nested_sub_aggregation_connection
@@ -316,6 +320,7 @@ def build_sub_aggregations_from(node_node, parent_nested_path: [])
316320
query: build_aggregation_query_for(
317321
node,
318322
field: field,
323+
context: context,
319324
grouping_adapter: sub_aggregation_grouping_adapter,
320325
nested_path: nested_path
321326
)

elasticgraph-graphql/lib/elastic_graph/graphql/aggregation/resolvers/aggregated_values.rb

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
require "elastic_graph/graphql/aggregation/key"
1010
require "elastic_graph/graphql/aggregation/path_segment"
1111
require "elastic_graph/support/hash_util"
12-
require "graphql"
1312

1413
module ElasticGraph
1514
class GraphQL
@@ -21,29 +20,6 @@ def resolve(field:, object:, args:, context:, lookahead:)
2120

2221
function_adapter = field.function_adapter # : FunctionAdapter::adapter
2322

24-
# `QueryAdapter` detected any invalid args when the query was built, but it can't report an
25-
# error there without failing resolution of the entire aggregations field rather than just
26-
# this one leaf--so it omits the datastore clause for an invalid field instead. Here, at
27-
# resolve time, we're resolving this specific field, so we can report the error at the
28-
# correct, precise path.
29-
#
30-
# `args` is already in schema form here (`GraphQLAdapterBuilder` converts it before calling
31-
# `resolve`), unlike at query-build time where `QueryAdapter` converts the raw AST arguments
32-
# itself--so, unlike there, we pass `args` through as-is rather than re-converting it.
33-
function_adapter.extract_args(args, schema.element_names) do |message|
34-
error = ::GraphQL::ExecutionError.new(message)
35-
36-
# Neither `context.add_error` nor `context.execution_errors.add` sets `path` for us--that
37-
# only happens automatically when an `ExecutionError` is raised and returned as a field's
38-
# own resolution result, which isn't the case here since we're continuing on to resolve
39-
# sibling fields normally. So we set `path` ourselves from `context.current_path`, which
40-
# is already the precise path to this field (e.g. `[..., "aggregatedValues", "amountCents",
41-
# "p150"]`), so the error in the response is attributed to this specific field.
42-
error.path = context.current_path
43-
context.add_error(error)
44-
return nil
45-
end
46-
4723
key = Key::AggregatedValue.new(
4824
aggregation_name: aggregation_name,
4925
field_path: field_path.map(&:name_in_graphql_query),
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
# Copyright 2024 - 2026 Block, Inc.
2+
#
3+
# Use of this source code is governed by an MIT-style
4+
# license that can be found in the LICENSE file or at
5+
# https://opensource.org/licenses/MIT.
6+
#
7+
# frozen_string_literal: true
8+
9+
require "elastic_graph/errors"
10+
require "graphql"
11+
12+
module ElasticGraph
13+
class GraphQL
14+
# Tracks "lookahead errors": errors that an ancestor resolver discovers about a *descendant*
15+
# field while walking its lookahead to build a datastore query, but cannot raise for (raising
16+
# there would fail the ancestor's entire subtree instead of just the one bad descendant).
17+
#
18+
# See `docs/adr/0001-lookahead-error-record-key.md` for the design rationale behind the
19+
# record key used here.
20+
class LookaheadErrors
21+
# A recorded error about the field selected at `path`--an absolute response path from the root
22+
# of the query, with no list indices.
23+
Record = ::Data.define(:path, :message, :extensions) do
24+
# @implements Record
25+
26+
# Indices are skipped rather than compared because the flagged node's list positions don't
27+
# exist yet when it is recorded, so one record can match many response positions (e.g. every
28+
# bucket of an aggregation).
29+
def matches?(current_path)
30+
current_path.grep_v(::Integer) == path
31+
end
32+
end
33+
34+
def initialize
35+
@records = [] # : Array[Record]
36+
end
37+
38+
# Records that `node` (a lookahead node at or beneath the field being resolved at
39+
# `recording_path`) is invalid, with the given `message`, making it matchable via
40+
# `#matching_error_for`. `root_lookahead` is the lookahead of the query as a whole, which we
41+
# walk to resolve `node` to its absolute path.
42+
def record(node, message, root_lookahead:, recording_path:, extensions: nil)
43+
# A node reached through a fragment spread that's used in multiple places has multiple paths;
44+
# only the one(s) under the field being resolved are ours to flag.
45+
prefix = recording_path.grep_v(::Integer)
46+
target_ast_nodes = node.selected? ? node.ast_nodes.to_set : ::Set.new # : ::Set[::GraphQL::Language::Nodes::Field]
47+
paths = find_paths(root_lookahead, target_ast_nodes, []).select { |path| path.first(prefix.size) == prefix }
48+
49+
if paths.empty?
50+
raise Errors::ConfigError, "`record_lookahead_error` was given a lookahead node that is not " \
51+
"selected at or beneath #{prefix.inspect}, the field being resolved. Note that " \
52+
"`Lookahead#selection` returns a null object for an unselected (or misspelled) field."
53+
end
54+
55+
@records.concat(paths.map { |path| Record.new(path: path, message: message, extensions: extensions) })
56+
end
57+
58+
# Returns a `GraphQL::ExecutionError` for the first recorded error matching `current_path` (the
59+
# path of the field about to be resolved), or `nil` if none match. Cheap (an `Array#find` over
60+
# `@records`, empty for the overwhelmingly common query that never records a lookahead error)
61+
# with no separate guard needed at call sites. A record legitimately matches nothing at all when
62+
# execution never reaches the flagged position (e.g. its parent resolved to an empty list).
63+
def matching_error_for(current_path)
64+
record = @records.find { |r| r.matches?(current_path) }
65+
return nil unless record
66+
67+
::GraphQL::ExecutionError.new(record.message, extensions: record.extensions)
68+
end
69+
70+
private
71+
72+
# Finds the index-free response-key path(s) from `current` down to a node whose `ast_nodes`
73+
# intersect `target_ast_nodes`, including `current` itself (so a node can flag its own
74+
# recording field, yielding that field's own path).
75+
def find_paths(current, target_ast_nodes, path_so_far)
76+
return [path_so_far] if current.ast_nodes.to_set.intersect?(target_ast_nodes)
77+
78+
current.selections.flat_map do |child|
79+
# `Lookahead#name` returns the field's method name, not its response key, so aliased
80+
# selections would collide under it. `#selections` already groups by response key
81+
# (`alias || name`), so pull the response key straight off the underlying AST node.
82+
first_ast_node = child.ast_nodes.first
83+
response_key = first_ast_node.alias || first_ast_node.name
84+
find_paths(child, target_ast_nodes, path_so_far + [response_key])
85+
end
86+
end
87+
end
88+
end
89+
end

elasticgraph-graphql/lib/elastic_graph/graphql/query_context.rb

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
require "elastic_graph/errors"
1010
require "elastic_graph/graphql/client"
11+
require "elastic_graph/graphql/lookahead_errors"
1112
require "elastic_graph/graphql/query_details_tracker"
1213
require "graphql"
1314

@@ -92,6 +93,42 @@ def fetch(key, *args, &block)
9293
super
9394
end
9495

96+
# Records that `node`, a `GraphQL::Execution::Lookahead` node at or beneath the field currently
97+
# being resolved, is invalid, with the given `message`. Intended for a custom lookahead-driven
98+
# resolver that discovers a problem with itself or a *descendant* field while building its
99+
# datastore query, and cannot raise a `GraphQL::ExecutionError` for it there (doing so would
100+
# fail this resolver's entire subtree instead of just the one bad field).
101+
#
102+
# `extensions`, if provided, is included verbatim under the `"extensions"` key of the error
103+
# in the GraphQL response; when omitted, no `"extensions"` key is added.
104+
#
105+
# `node` is resolved into a matching field the next time GraphQL execution reaches it: instead
106+
# of running the registered resolver, ElasticGraph fails that field with a
107+
# `GraphQL::ExecutionError` built from `message`/`extensions`, without disrupting sibling
108+
# fields. A resolver that flags *itself* can report the error by returning
109+
# `#matching_lookahead_error`, giving it one consistent way to fail a field regardless of
110+
# whether the problem is with itself or a descendant.
111+
#
112+
# Must be called while the recording field is being resolved, since `node` is resolved to a
113+
# response path relative to `#current_path`. Raises `Errors::ConfigError` if `node` is not
114+
# selected at or beneath that path.
115+
def record_lookahead_error(node, message, extensions: nil)
116+
current_path = self.current_path # : Array[String | Integer]
117+
lookahead_errors.record(node, message, root_lookahead: query.lookahead, recording_path: current_path, extensions: extensions)
118+
end
119+
120+
# Returns a `GraphQL::ExecutionError` for the field about to be resolved (at `#current_path`)
121+
# if an ancestor resolver recorded a lookahead error matching it, or `nil` otherwise.
122+
def matching_lookahead_error
123+
current_path = self.current_path # : Array[String | Integer]
124+
lookahead_errors.matching_error_for(current_path)
125+
end
126+
127+
# Lazily builds (and memoizes) the `LookaheadErrors` collector for this query.
128+
def lookahead_errors
129+
@lookahead_errors ||= LookaheadErrors.new
130+
end
131+
95132
private
96133

97134
def raise_if_removed_key(key)

0 commit comments

Comments
 (0)