feat: onError error behavior (PROPAGATE/NULL/HALT) + Service Capabilities introspection#1562
Open
jensneuse wants to merge 5 commits into
Open
feat: onError error behavior (PROPAGATE/NULL/HALT) + Service Capabilities introspection#1562jensneuse wants to merge 5 commits into
jensneuse wants to merge 5 commits into
Conversation
Add client-selectable GraphQL error behavior (PROPAGATE / NULL / HALT) per the onError proposal (graphql-spec#1163), defaulting to PROPAGATE with zero change for existing callers. - ErrorBehavior enum + MapErrorBehavior validator (empty => PROPAGATE) - ExecutionOptions.ErrorBehavior plumbed into Resolvable, normalized at both Resolve and ResolveNode entry points (nil-ctx safe) and reset in Reset - NULL: erroredPosition() sets the errored position null in place on the validation pass and renders null on the print pass; error recorded once (all add-error sites guarded with !r.print) - HALT: render-time only; forces data:null and trims errors to one via keepFirstErrorOnly() - Applied across all leaf/object/array/enum walkers incl. coercion and invalid/inaccessible enum sites Includes the RFC and the three phase implementation plans, plus a Phase 1 reviewer document. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Config-gated Service Capabilities introspection (graphql-spec#1163) so the
server can advertise graphql.onError and graphql.defaultErrorBehavior via
__schema { capabilities }. Byte-identical to today when disabled.
- introspection: Capability type, optional Schema.Capabilities (omitempty),
BuildServiceCapabilities(enabled, default), idempotent AddCapabilityType()
- asttransform: MergeDefinitionWithBaseSchemaOptions adds the __Capability
type and a nullable __Schema.capabilities: [__Capability!] field only when
ServiceCapabilities is enabled; existing MergeDefinitionWithBaseSchema
delegates with the feature off
- introspection_datasource: NewIntrospectionConfigFactoryWithOptions populates
Schema.Capabilities and registers the capabilities/__Capability child nodes
when enabled; source.go unchanged (already marshals the whole Schema)
Tests cover model marshaling, the builder, on-demand type registration, SDL
gating (enabled/disabled), Load serialization, and child-node registration.
Full execution e2e and router integration are deferred to Phase 3 / the router.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Prove the onError implementation across the behavior x position x nullability matrix, that HALT returns a single error, that the operator default is applied and stays in sync with introspection, and that the feature-off path is byte-identical. - resolve: table-driven matrix (PROPAGATE/NULL/HALT x leaf/object/list-item), HALT single-error, unset==PROPAGATE regression guard, operator-default test - introspection: graphql.defaultErrorBehavior syncs with configured default - execution engine: add WithErrorBehavior option (only production change) and an end-to-end test with an erroring subgraph asserting the full client response under each behavior All expected strings are pinned to observed output with full assert.Equal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds onError-based GraphQL execution error handling, introspection service-capabilities advertisement, and the supporting RFC, review, and test coverage across resolver and introspection code paths. ChangesRFC Documentation
Resolver Error Behavior
Service Capabilities Introspection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutionEngine
participant Resolvable
participant Subgraph
Client->>ExecutionEngine: Execute request with WithErrorBehavior(...)
ExecutionEngine->>Resolvable: ResolveNode / Resolve with ExecutionOptions.ErrorBehavior
Resolvable->>Subgraph: Fetch data
Subgraph-->>Resolvable: Response with null + error at non-null field
alt ErrorBehavior = NULL
Resolvable->>Resolvable: erroredPosition sets field to null in-place
else ErrorBehavior = HALT
Resolvable->>Resolvable: keepFirstErrorOnly and force data = null
else ErrorBehavior = PROPAGATE
Resolvable->>Resolvable: propagate existing non-null bubble-up behavior
end
Resolvable-->>ExecutionEngine: Rendered JSON response
ExecutionEngine-->>Client: Response
sequenceDiagram
participant Client
participant IntrospectionConfigFactory
participant BaseSchemaMerger
participant Schema
Client->>IntrospectionConfigFactory: Create with ServiceCapabilities option
IntrospectionConfigFactory->>BaseSchemaMerger: MergeDefinitionWithBaseSchemaOptions(...)
BaseSchemaMerger->>Schema: AddCapabilityType when enabled
IntrospectionConfigFactory->>Schema: BuildServiceCapabilities(enabled, defaultErrorBehavior)
Schema-->>Client: Introspection JSON with capabilities
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CI golangci-lint (gci) flagged the import ordering in the new e2e test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex local review surfaced two genuine NULL gaps for empty-path / abstract positions; both fixed with regression tests: - walkFloat: hoist the type-mismatch check out of `if !r.print` so it reaches erroredPosition on the print pass. A non-null Float array item (empty path) with a type mismatch under NULL previously leaked the raw invalid value because the validation pass cannot null an item by empty path. - walkObject: route the invalid-abstract-__typename non-null branch through erroredPosition instead of r.err(), so under NULL the object is nulled in place rather than propagating like PROPAGATE. PROPAGATE/HALT output is unchanged (erroredPosition returns r.err() for both). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
docs/rfcs/plans/2026-07-01-onerror-phase3-tests-hardening.md (2)
58-126: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the deep chain-to-root row.
The matrix still stops at leaf/object/list-item cases; it never exercises the deeply nested non-null chain-to-root path called out in RFC §7, so a final
data: nullcollapse regression could slip through. Please add one row for that path before freezing the expected outputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rfcs/plans/2026-07-01-onerror-phase3-tests-hardening.md` around lines 58 - 126, The TestErrorBehaviorMatrix cases cover leaf, object, and list-item failures, but they miss the deep non-null chain-to-root path from RFC §7 that should collapse all the way to data: null. Add a new row to the cases slice in TestErrorBehaviorMatrix that exercises that nested non-null propagation scenario and asserts the final null-root response, using the existing resolveWith helper and the same ErrorBehavior variants as needed.
191-200: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTurn this into an execution-level assertion.
As written, the test just replays the fallback decision in the assertion body; it never resolves a fixture with
ExecutionOptions.ErrorBehaviorset, so it would still pass if the engine ignored the field. Please drive a tiny resolve call instead so the operator-default path is actually covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rfcs/plans/2026-07-01-onerror-phase3-tests-hardening.md` around lines 191 - 200, The test in TestErrorBehavior_OperatorDefaultApplied is only re-implementing the fallback logic instead of verifying engine behavior. Replace the inline MapErrorBehavior/operatorDefault check with a tiny execution-level resolve using a fixture whose ExecutionOptions.ErrorBehavior is unset, so the operator-default path is actually exercised. Keep the assertion on the resolved effective behavior, and use the existing test helper/fixture setup in this test file to locate the runtime path being validated.v2/pkg/introspection/introspection.go (1)
97-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
__Capabilitydefined in one place.Schema.AddCapabilityType()is only used by tests, while production builds__Capabilityfromasttransform.baseschemaServiceCapabilitiesviaNewGenerator(). That duplicates the same field shape in SDL and Go, so the two can drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/introspection/introspection.go` around lines 97 - 121, Keep __Capability defined from a single source of truth and remove the duplicate field definition in Schema.AddCapabilityType(). Update AddCapabilityType() to reuse the same capability type construction used by NewGenerator() and asttransform.baseschemaServiceCapabilities, so tests and production share the exact schema shape instead of maintaining separate __Capability field lists.v2/pkg/engine/resolve/error_behavior_matrix_test.go (1)
13-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
NewContext()instead of aContext{}literal.Same as in
resolvable_test.go: this helper constructsContextvia a struct literal rather thanNewContext(), bypassing internal field initialization.♻️ Suggested fix
func resolveWith(t *testing.T, behavior ErrorBehavior, data string, root *Object) string { t.Helper() res := NewResolvable(nil, ResolvableOptions{}) - err := res.Init(&Context{ExecutionOptions: ExecutionOptions{ErrorBehavior: behavior}}, - []byte(data), ast.OperationTypeQuery) + ctx := NewContext(context.Background()) + ctx.ExecutionOptions = ExecutionOptions{ErrorBehavior: behavior} + err := res.Init(ctx, []byte(data), ast.OperationTypeQuery) assert.NoError(t, err) out := &bytes.Buffer{} assert.NoError(t, res.Resolve(context.Background(), root, nil, out)) return out.String() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/error_behavior_matrix_test.go` around lines 13 - 22, The resolveWith helper is constructing Context with a struct literal, which bypasses the initialization done by NewContext(). Update resolveWith to create the execution context through NewContext() and then set the ExecutionOptions.ErrorBehavior on that context before calling res.Init, matching the pattern used in resolvable_test.go and keeping Context initialization consistent.Source: Learnings
v2/pkg/engine/resolve/resolvable_test.go (1)
1629-1656: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
NewContext()instead ofContext{}literals.
ctx := &Context{}(line 1631) and theContext{ExecutionOptions: ...}literals innewNullResolvable/newHaltResolvablebypassNewContext()'s field initialization (e.g.subgraphErrors). The codebase convention is to always constructContextviaNewContext()and set fields afterward, to fail fast on misuse rather than relying on defensive nil checks.♻️ Suggested fix
- ctx := &Context{} // ExecutionOptions zero value + ctx := NewContext(context.Background()) // ExecutionOptions zero valuefunc newNullResolvable(t *testing.T, data string) *Resolvable { t.Helper() res := NewResolvable(nil, ResolvableOptions{}) - err := res.Init(&Context{ExecutionOptions: ExecutionOptions{ErrorBehavior: ErrorBehaviorNull}}, - []byte(data), ast.OperationTypeQuery) + ctx := NewContext(context.Background()) + ctx.ExecutionOptions = ExecutionOptions{ErrorBehavior: ErrorBehaviorNull} + err := res.Init(ctx, []byte(data), ast.OperationTypeQuery) assert.NoError(t, err) return res }Same applies to
newHaltResolvable.Also applies to: 1717-1724
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/resolvable_test.go` around lines 1629 - 1656, The test helpers are constructing Context with literals instead of NewContext(), which bypasses required initialization like subgraphErrors. Update TestResolvable_ErrorBehaviorDefaultsToPropagate, newNullResolvable, and newHaltResolvable to create contexts via NewContext() and then set ExecutionOptions fields afterward. Keep the existing behavior assertions intact while ensuring all Context setup follows the same initialization path used by the rest of the codebase.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rfcs/plans/2026-07-01-onerror-phase1-core-resolver.md`:
- Around line 221-226: The HALT single-error selection is still order-dependent
because it uses r.errors[0], which can vary when subgraph errors arrive
concurrently. Update the resolver’s first-error choice logic in the core
resolution path so it uses a stable deterministic rule instead of append order,
and make the failing tests in resolvable_test.go cover concurrent/error-order
cases. Ensure the implementation consistently selects the same primary error
across runs before HALT trims the response.
In `@docs/rfcs/plans/2026-07-01-onerror-phase2-introspection.md`:
- Around line 307-308: The __Schema.capabilities field is being tightened to
non-null in the RFC text, but it must remain nullable to match the documented
contract. Update the introspection type definition in the relevant
__Schema/__Capability section so capabilities is declared as [__Capability!]
rather than [__Capability!]!, and keep the surrounding planner/runtime guidance
aligned with that nullable shape in the introspectionData and
BuildServiceCapabilities flow.
In `@docs/rfcs/plans/2026-07-01-onerror-phase3-tests-hardening.md`:
- Around line 243-247: The error assertion in the error-behavior fixture is too
specific for the default subgraph propagation behavior. Update the test harness
around the modeled subgraph response and the three
ExecutionOptions.ErrorBehavior cases (PROPAGATE, NULL, HALT) so it either
configures the subgraph to emit the raw boom message or asserts against the
wrapped upstream error shape/message instead. Keep the checks anchored on the
existing subgraph model and client response assertions, and make the expectation
consistent across the full response for each behavior.
In `@v2/pkg/engine/resolve/error_behavior_test.go`:
- Around line 29-38: The operator-default simulation in
TestErrorBehavior_OperatorDefaultApplied is using the mapped enum from
MapErrorBehavior to infer omission, which makes explicit PROPAGATE
indistinguishable from a missing field. Update the test logic to check the raw
request value for emptiness before calling MapErrorBehavior, then apply
ErrorBehaviorNull only when the input is actually omitted; keep explicit
PROPAGATE unchanged and use the existing symbols MapErrorBehavior,
ErrorBehaviorPropagate, and ErrorBehaviorNull to anchor the fix.
---
Nitpick comments:
In `@docs/rfcs/plans/2026-07-01-onerror-phase3-tests-hardening.md`:
- Around line 58-126: The TestErrorBehaviorMatrix cases cover leaf, object, and
list-item failures, but they miss the deep non-null chain-to-root path from RFC
§7 that should collapse all the way to data: null. Add a new row to the cases
slice in TestErrorBehaviorMatrix that exercises that nested non-null propagation
scenario and asserts the final null-root response, using the existing
resolveWith helper and the same ErrorBehavior variants as needed.
- Around line 191-200: The test in TestErrorBehavior_OperatorDefaultApplied is
only re-implementing the fallback logic instead of verifying engine behavior.
Replace the inline MapErrorBehavior/operatorDefault check with a tiny
execution-level resolve using a fixture whose ExecutionOptions.ErrorBehavior is
unset, so the operator-default path is actually exercised. Keep the assertion on
the resolved effective behavior, and use the existing test helper/fixture setup
in this test file to locate the runtime path being validated.
In `@v2/pkg/engine/resolve/error_behavior_matrix_test.go`:
- Around line 13-22: The resolveWith helper is constructing Context with a
struct literal, which bypasses the initialization done by NewContext(). Update
resolveWith to create the execution context through NewContext() and then set
the ExecutionOptions.ErrorBehavior on that context before calling res.Init,
matching the pattern used in resolvable_test.go and keeping Context
initialization consistent.
In `@v2/pkg/engine/resolve/resolvable_test.go`:
- Around line 1629-1656: The test helpers are constructing Context with literals
instead of NewContext(), which bypasses required initialization like
subgraphErrors. Update TestResolvable_ErrorBehaviorDefaultsToPropagate,
newNullResolvable, and newHaltResolvable to create contexts via NewContext() and
then set ExecutionOptions fields afterward. Keep the existing behavior
assertions intact while ensuring all Context setup follows the same
initialization path used by the rest of the codebase.
In `@v2/pkg/introspection/introspection.go`:
- Around line 97-121: Keep __Capability defined from a single source of truth
and remove the duplicate field definition in Schema.AddCapabilityType(). Update
AddCapabilityType() to reuse the same capability type construction used by
NewGenerator() and asttransform.baseschemaServiceCapabilities, so tests and
production share the exact schema shape instead of maintaining separate
__Capability field lists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82361a47-5ad8-4b24-8356-4df4a0b456bc
📒 Files selected for processing (21)
docs/rfcs/2026-07-01-onerror-error-behavior.mddocs/rfcs/plans/2026-07-01-onerror-phase1-core-resolver.mddocs/rfcs/plans/2026-07-01-onerror-phase2-introspection.mddocs/rfcs/plans/2026-07-01-onerror-phase3-tests-hardening.mddocs/rfcs/reviews/phase1-core-resolver.mddocs/rfcs/reviews/phase2-introspection.mddocs/rfcs/reviews/phase3-tests-hardening.mdexecution/engine/error_behavior_e2e_test.goexecution/engine/execution_engine.gov2/pkg/asttransform/baseschema.gov2/pkg/asttransform/baseschema_capabilities_test.gov2/pkg/engine/datasource/introspection_datasource/capabilities_test.gov2/pkg/engine/datasource/introspection_datasource/config_factory.gov2/pkg/engine/resolve/context.gov2/pkg/engine/resolve/error_behavior.gov2/pkg/engine/resolve/error_behavior_matrix_test.gov2/pkg/engine/resolve/error_behavior_test.gov2/pkg/engine/resolve/resolvable.gov2/pkg/engine/resolve/resolvable_test.gov2/pkg/introspection/introspection.gov2/pkg/introspection/introspection_capabilities_test.go
Comment on lines
+221
to
+226
|
|
||
| - [ ] **Step 1: Write the failing tests** (leaf, object, list-item, nested, siblings preserved, with and without upstream error) | ||
|
|
||
| ```go | ||
| // append to v2/pkg/engine/resolve/resolvable_test.go | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make HALT’s single-error selection deterministic.
Choosing r.errors[0] by append order is still nondeterministic if subgraph errors are appended concurrently, and the plan explicitly says fetches are not cancelled. HALT responses can then vary across runs. Please pick a stable first-error rule before trimming here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/rfcs/plans/2026-07-01-onerror-phase1-core-resolver.md` around lines 221
- 226, The HALT single-error selection is still order-dependent because it uses
r.errors[0], which can vary when subgraph errors arrive concurrently. Update the
resolver’s first-error choice logic in the core resolution path so it uses a
stable deterministic rule instead of append order, and make the failing tests in
resolvable_test.go cover concurrent/error-order cases. Ensure the implementation
consistently selects the same primary error across runs before HALT trims the
response.
Comment on lines
+307
to
+308
| - If capabilities live under `__schema`: extend the introspection type definition (from Step 1) to declare `capabilities: [__Capability!]!` on `__Schema` and the `__Capability` type; no `source.go` change needed (it already marshals the whole `Schema`). Ensure the code path that builds `introspectionData` calls `AddCapabilityType()` and sets `Schema.Capabilities = BuildServiceCapabilities(enabled, defaultBehavior)`. | ||
| - If a separate `service` root is required: add `serviceFieldName = "service"` in `input.go`, a `case serviceFieldName` in `planner.go` `EnterField`, a `ServiceRequestType` + input branch, and a `source.go` branch returning `json.Marshal(struct{ Capabilities []introspection.Capability }{...})`. Follow the exact `__schema` pattern in those three files. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep __Schema.capabilities nullable.
This bullet makes capabilities non-null ([__Capability!]!), but the RFC/review elsewhere defines it as a nullable list ([__Capability!]). Tightening it here changes the contract and will force planner/runtime assumptions that don’t match the documented disabled/empty behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/rfcs/plans/2026-07-01-onerror-phase2-introspection.md` around lines 307
- 308, The __Schema.capabilities field is being tightened to non-null in the RFC
text, but it must remain nullable to match the documented contract. Update the
introspection type definition in the relevant __Schema/__Capability section so
capabilities is declared as [__Capability!] rather than [__Capability!]!, and
keep the surrounding planner/runtime guidance aligned with that nullable shape
in the introspectionData and BuildServiceCapabilities flow.
Comment on lines
+243
to
+247
| Model a subgraph that returns `{"data":{"hero":{"name":null}},"errors":[{"message":"boom","path":["hero","name"]}]}` for a query with a non-null `name`. Run the same operation three times with `ExecutionOptions.ErrorBehavior` = `PROPAGATE`, `NULL`, `HALT`, and assert the full client response each time: | ||
| - `PROPAGATE`: `hero` (or `data`) collapses per nullability, with the subgraph error. | ||
| - `NULL`: `{"errors":[{"message":"boom",...}],"data":{"hero":{"name":null}}}` — sibling data preserved. | ||
| - `HALT`: `{"errors":[{"message":"boom",...}],"data":null}` — single error. | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pin the wrapped subgraph error, or switch the harness.
This fixture expects the raw boom message, but the review notes that the default subgraph propagation mode wraps upstream errors. Unless the test reconfigures that mode or asserts the wrapped message, the exact-string check here will be wrong.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/rfcs/plans/2026-07-01-onerror-phase3-tests-hardening.md` around lines
243 - 247, The error assertion in the error-behavior fixture is too specific for
the default subgraph propagation behavior. Update the test harness around the
modeled subgraph response and the three ExecutionOptions.ErrorBehavior cases
(PROPAGATE, NULL, HALT) so it either configures the subgraph to emit the raw
boom message or asserts against the wrapped upstream error shape/message
instead. Keep the checks anchored on the existing subgraph model and client
response assertions, and make the expectation consistent across the full
response for each behavior.
Comment on lines
+29
to
+38
| func TestErrorBehavior_OperatorDefaultApplied(t *testing.T) { | ||
| // simulate: no request onError, operator default = NULL => router sets NULL. | ||
| effective, ok := MapErrorBehavior("") // request omitted | ||
| assert.True(t, ok) | ||
| operatorDefault := ErrorBehaviorNull | ||
| if effective == ErrorBehaviorPropagate { // request omitted -> apply operator default | ||
| effective = operatorDefault | ||
| } | ||
| assert.Equal(t, ErrorBehaviorNull, effective) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Operator-default heuristic can't distinguish "omitted" from explicit PROPAGATE.
MapErrorBehavior("") and MapErrorBehavior("PROPAGATE") both yield ErrorBehaviorPropagate. Using effective == ErrorBehaviorPropagate as a proxy for "request omitted onError" (line 34) means a client that explicitly requests PROPAGATE would be indistinguishable from one that omitted the field — an operator default of NULL would silently override the client's explicit choice, undermining the "client-selectable" guarantee this feature is built around.
This test models a pattern that a future router implementation (noted as a follow-up) could copy verbatim. Operator-default resolution should check the raw incoming request value for emptiness before calling MapErrorBehavior, not compare the already-mapped enum value.
🛠️ Suggested fix for the simulation logic
func TestErrorBehavior_OperatorDefaultApplied(t *testing.T) {
// simulate: no request onError, operator default = NULL => router sets NULL.
- effective, ok := MapErrorBehavior("") // request omitted
+ rawRequestValue := "" // request omitted
+ effective, ok := MapErrorBehavior(rawRequestValue)
assert.True(t, ok)
operatorDefault := ErrorBehaviorNull
- if effective == ErrorBehaviorPropagate { // request omitted -> apply operator default
+ if rawRequestValue == "" { // request omitted -> apply operator default
effective = operatorDefault
}
assert.Equal(t, ErrorBehaviorNull, effective)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestErrorBehavior_OperatorDefaultApplied(t *testing.T) { | |
| // simulate: no request onError, operator default = NULL => router sets NULL. | |
| effective, ok := MapErrorBehavior("") // request omitted | |
| assert.True(t, ok) | |
| operatorDefault := ErrorBehaviorNull | |
| if effective == ErrorBehaviorPropagate { // request omitted -> apply operator default | |
| effective = operatorDefault | |
| } | |
| assert.Equal(t, ErrorBehaviorNull, effective) | |
| } | |
| func TestErrorBehavior_OperatorDefaultApplied(t *testing.T) { | |
| // simulate: no request onError, operator default = NULL => router sets NULL. | |
| rawRequestValue := "" // request omitted | |
| effective, ok := MapErrorBehavior(rawRequestValue) | |
| assert.True(t, ok) | |
| operatorDefault := ErrorBehaviorNull | |
| if rawRequestValue == "" { // request omitted -> apply operator default | |
| effective = operatorDefault | |
| } | |
| assert.Equal(t, ErrorBehaviorNull, effective) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/pkg/engine/resolve/error_behavior_test.go` around lines 29 - 38, The
operator-default simulation in TestErrorBehavior_OperatorDefaultApplied is using
the mapped enum from MapErrorBehavior to infer omission, which makes explicit
PROPAGATE indistinguishable from a missing field. Update the test logic to check
the raw request value for emptiness before calling MapErrorBehavior, then apply
ErrorBehaviorNull only when the input is actually omitted; keep explicit
PROPAGATE unchanged and use the existing symbols MapErrorBehavior,
ErrorBehaviorPropagate, and ErrorBehaviorNull to anchor the fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the GraphQL onError proposal (graphql-spec#1163): client-selectable error behavior in the resolver plus config-gated Service Capabilities introspection so a gateway/router can advertise support. Defaults to today's behavior with zero change for existing callers.
Three error behaviors:
PROPAGATE(default) — current null-bubbling to the nearest nullable ancestor.NULL— set the errored position tonullin place (even if non-nullable), no propagation, error still recorded.HALT— abort assembly:data: nullwith a single error.Delivered in three reviewable commits, each with a reviewer document under
docs/rfcs/reviews/.Phase 1 — core resolver (
03de476)ErrorBehaviorenum +MapErrorBehavior(empty ⇒PROPAGATE).ExecutionOptions.ErrorBehaviorthreaded intoResolvable, normalized at bothResolve/ResolveNodeentry points (nil-ctx safe), reset inReset.NULLviaerroredPositionapplied across all leaf/object/array/enum walkers incl. coercion + invalid/inaccessible enum; error recorded once (validation-pass-guarded),nullrendered on the print pass.HALTrender-time only:keepFirstErrorOnly+data:nullwiring.Phase 2 — introspection Service Capabilities (
ef47e4e)introspection:Capabilitytype, optionalSchema.Capabilities(omitempty),BuildServiceCapabilities(enabled, default), idempotentAddCapabilityType().asttransform:MergeDefinitionWithBaseSchemaOptionsaddstype __Capability+ nullable__Schema.capabilities: [__Capability!]only when enabled; the existingMergeDefinitionWithBaseSchemadelegates with the feature off (byte-identical).introspection_datasource:NewIntrospectionConfigFactoryWithOptionspopulatesSchema.Capabilitiesand registers thecapabilities/__Capabilitychild nodes when enabled;source.gounchanged.Phase 3 — tests & hardening (
a52a0d0)executionengine:WithErrorBehavioroption (the only production change here) + an end-to-end test with an erroring subgraph asserting the full client response under each behavior.Design & decisions
RFC:
docs/rfcs/2026-07-01-onerror-error-behavior.md. Per-phase reviewer notes indocs/rfcs/reviews/.resolvable(no fetch cancellation).graphql.defaultErrorBehaviorstays synchronized with the operator default.Testing
cd v2 && go test ./pkg/engine/...— green.cd v2 && go test ./pkg/introspection/ ./pkg/asttransform/ ./pkg/engine/datasource/introspection_datasource/— green (no golden drift).cd execution && go test ./engine/— green (incl. the new e2e).Follow-ups (out of scope)
MergeOptionsandIntrospectionOptions) lives in the router repo.__schema { capabilities }.🤖 Generated with Claude Code