Skip to content

Commit 399e8f7

Browse files
xperiandriclaude
xperiandri
and
claude
committed
Add @defer on fragment spreads and inline fragments (#615)
Add @defer on fragment spreads and inline fragments A fragment spread or inline fragment carrying `@defer` is planned as the new `ResolveDeferredFragment` kind: its fields stand among the fields of the object containing them, are resolved together against that object once the object's own fields were delivered, and go out as one payload announced at the object's path with the fragment's label. The engine reports fragments through the new `DeferredFragmentPending`, `DeferredFragmentResult` and `DeferredFragmentCompleted` events, and the graphql-transport-ws translator delivers them as `pending`/`incremental`/`completed` entries whose `data` is the object map of the fragment's fields. Semantics, aligned with graphql-js: - A field also selected directly on the object is executed with it, together with whatever the fragment selects under it, and left out of the fragment, wherever in the selection the fragment stands. - A fragment spread directly anywhere in the selection is resolved with the object, whichever of its spreads comes first: direct and deferred spreads are tracked apart. - A fragment spread twice deferred at the same place is delivered once; two labeled fragments at the same path get distinct ids. - `if: false`, literal or through a variable, resolves the fragment's fields with the object; `@skip`/`@include` on the fragment exclude it entirely, at the root and inside any object. - Fragments at the operation root and on abstract types are supported; the root fields of a deferred root fragment are coerced up front, so an argument they reject fails the request before any resolver runs. - An error propagating up to the fragment completes it with the errors and no data. Also fixes validation of an inline fragment without a type condition (`... { fields }`), which applies to the parent type and used to throw on the absent condition while leaving the fragment's fields out of the validation context. Breaking: `GQLDeferredResponseContent` and `ExecutionInfoKind` gain cases, which affects exhaustive matches. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 parent c0ba6d4 commit 399e8f7

22 files changed

Lines changed: 1629 additions & 103 deletions

‎RELEASE_NOTES.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,7 @@
292292
* **Breaking Change** Removed `SubscriptionsDict`, `SubscriptionUnsubscriber` and `OnUnsubscribeAction` from `FSharp.Data.GraphQL.Shared.WebSockets`: the `graphql-transport-ws` middleware keeps its subscriptions in a per-connection registry owned by a single loop
293293
* **Breaking Change** `@defer` and `@stream` now declare the arguments the incremental delivery specification requires: `if: Boolean = true` and `label: String` on both, `initialCount: Int = 0` on `@stream`. `@stream` is allowed on `FIELD` only and `@defer` on `FIELD`, `FRAGMENT_SPREAD` and `INLINE_FRAGMENT`, no longer on `FRAGMENT_DEFINITION`. `if: false`, literal or through a variable, executes the field inline; `initialCount` delivers the first items with the initial payload and streams the rest; `label` is carried by the `pending` entry announcing the field
294294
* **Breaking Change** `GQLDeferredResponseContent.DeferredPending` gained `InitialCount`, the number of items of a streamed field delivered with the initial payload, so that the `graphql-transport-ws` translator expects the streamed items from that index
295+
* **Breaking Change** Added `@defer` on fragment spreads and inline fragments, as the incremental delivery specification defines it: the fragment's fields are resolved together and delivered as one payload of the object containing them, announced at that object's path with the fragment's `label`; a field also selected directly on the object is executed with it, together with whatever the fragment selects under it, and left out of the fragment; a fragment spread directly anywhere in the selection is resolved with the object, whichever spread of it comes first; a fragment spread twice deferred at the same place is delivered once; a fragment whose `if` is `false` through a variable is resolved with the object; a fragment on an abstract type delivers the fields of the concrete type; an error propagating up to the fragment completes it with the errors and no data. The engine reports fragments through the new `DeferredFragmentPending`, `DeferredFragmentResult` and `DeferredFragmentCompleted` events, and plans them as the new `ResolveDeferredFragment` kind
295296
* **Breaking Change** `BufferedStreamOptions.Interval` and `BufferedStreamOptions.PreferredBatchSize` are now `int voption`
296297
* **Breaking Change** `ServerMessage.Error` and `ServerRawPayload.ErrorMessages` now carry `GQLProblemDetails list` instead of `NameValueLookup list`, so an `error` message's `payload` is a standard GraphQL error array as the `graphql-transport-ws` protocol requires
297298
* **Breaking Change** A query or mutation whose non-null root field fails during execution now produces a `Direct` (execution) result with `null` data instead of a `RequestError`, which is now only ever produced for a request rejected before execution (validation, planning, variable or inline argument coercion, a middleware, or the executor itself failing); HTTP and `graphql-transport-ws` responses for such a failure now carry `data: null` as the spec requires, instead of omitting `data` entirely. This also changes the public `GQLResponse.Data`, `GQLResponseContent.Direct.Data`, `DeferredErrors.Data`, and `SubscriptionErrors.Data` signatures to use `voption`
@@ -322,4 +323,5 @@
322323
* Fixed `graphql-transport-ws` never sending `complete` after the `next` of a query or mutation result, as the protocol requires
323324
* Fixed `graphql-transport-ws` sending a request error (rejected before execution: validation, planning, variable coercion, a middleware, or the executor itself failing) as a `next` result followed by `complete`, instead of the terminal `error` message the protocol requires for it; a query or mutation whose non-null root field fails during execution still gets `next` + `complete`, since it is a result, not a request error
324325
* Fixed `graphql-transport-ws` throwing while serializing an `error` message or a `pong` carrying a payload, since neither was written under the `payload` property name `Utf8JsonWriter` requires
326+
* Fixed validation of an inline fragment without a type condition (`... { … }`), which used to fail with an exception instead of applying to the parent type
325327
* Removed the internal `Observable.withCompletionMarker`

‎docs/bug-spec-interface-possible-types-keynotfound.md‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
# Bug Spec: KeyNotFoundException in Interface Possible Types Resolution
2+
3+
## Summary
4+
Schema introspection may crash with `System.Collections.Generic.KeyNotFoundException` when resolving possible types for an interface with no registered object implementations in the computed implementations map.
5+
6+
## Observed Runtime Evidence
7+
- Exception type: `System.Collections.Generic.KeyNotFoundException`
8+
- Message: `The given key was not present in the dictionary.`
9+
- Top failing library frame: `FSharp.Data.GraphQL.Schema<'Root>.getPossibleTypes`
10+
- Failing code path (`src/FSharp.Data.GraphQL.Server/Schema.fs`):
11+
- `| Interface i -> Map.find i.Name (implementations.Force()) |> Array.ofList`
12+
13+
## Exact Failure Location
14+
File: `src/FSharp.Data.GraphQL.Server/Schema.fs`
15+
- `getImplementations`: builds `Map<string, ObjectDef list>` from `objdef.Implements`
16+
- `getPossibleTypes`: uses `Map.find` for interfaces
17+
- `introspectType` for `Interface` calls `getPossibleTypes` and crashes before graceful validation/error reporting
18+
19+
## Root Cause
20+
`Map.find` assumes every interface name exists as a key in `implementations` map. This assumption is false when at least one schema interface has zero object implementations in the discovered type map.
21+
In that case, lookup throws immediately, producing infrastructure exception instead of structured GraphQL/type validation feedback.
22+
23+
## Why This Is Problematic
24+
1. Hard crash during schema startup/introspection.
25+
2. No actionable validation message identifying which interface is orphaned.
26+
3. Behavior differs from expected robust validation (should return deterministic `ValidationError` or safe empty set depending on policy).
27+
28+
## Reproduction (Generic, Domain-Agnostic)
29+
1. Define interface `IParentInfo` with at least one field.
30+
2. Register the interface in schema type map.
31+
3. Ensure no object type in type map includes this interface in `interfaces = [ ... ]`.
32+
4. Trigger schema introspection or schema initialization path that builds introspection metadata.
33+
5. Observe `KeyNotFoundException` at `Map.find i.Name (implementations.Force())`.
34+
35+
## Expected Behavior
36+
One of the following (explicitly chosen policy):
37+
- **Preferred**: do not throw; treat no implementations as empty set for possible types, and surface a validation error later if this is invalid by policy.
38+
- **Alternative**: immediately return structured validation error: `Interface <name> has no implementing object types.`
39+
40+
No raw `KeyNotFoundException` should escape from schema construction/introspection.
41+
42+
## Proposed Fix
43+
### Safe Lookup Change
44+
Replace unsafe lookup in `getPossibleTypes` with safe lookup:
45+
- from: `Map.find i.Name (implementations.Force()) |> Array.ofList`
46+
- to: `implementations.Force() |> Map.tryFind i.Name |> Option.defaultValue [] |> Array.ofList`
47+
48+
### Validation Enhancement
49+
Add explicit validation for orphaned interfaces in type-map validation layer:
50+
- detect interfaces with zero implementing object types
51+
- return deterministic `ValidationError` with interface name
52+
53+
This keeps runtime stable and preserves strict schema diagnostics.
54+
55+
## Test Specification
56+
Create dedicated tests in `tests/FSharp.Data.GraphQL.Tests` (new file recommended: `InterfacePossibleTypesValidationTests.fs`).
57+
58+
### Test 1: Regression Repro (pre-fix behavior)
59+
- Build schema with one interface and no implementors.
60+
- Assert old code throws `KeyNotFoundException` (documented regression test, can be skipped/removed after fix depending policy).
61+
62+
### Test 2: Safe Introspection (post-fix)
63+
- Same schema as Test 1.
64+
- Assert no `KeyNotFoundException` is thrown during introspection/schema init.
65+
66+
### Test 3: Validation Error for Orphan Interface
67+
- Same schema as Test 1.
68+
- Run type-map validation entry point.
69+
- Assert deterministic error contains interface name and orphaned-implementation message.
70+
71+
### Test 4: Normal Interface Implementations
72+
- Interface with one object implementation.
73+
- Assert introspection returns that object in possible types.
74+
75+
### Test 5: Multiple Implementations
76+
- Interface with two object implementations.
77+
- Assert introspection returns both possible types.
78+
79+
### Test 6: Mixed Schema Stability
80+
- Include additional unrelated interfaces/unions/objects.
81+
- Assert no crashes and correct possible type resolution across all abstract types.
82+
83+
## Acceptance Criteria
84+
1. No `KeyNotFoundException` from `getPossibleTypes` for missing interface key.
85+
2. Orphan interface case yields controlled behavior (empty set + validation error, or direct structured validation error per chosen policy).
86+
3. Existing interface/union introspection behavior remains unchanged for valid schemas.
87+
4. Tests cover single/multiple/no implementations and pass consistently.
88+
89+
## Backward Compatibility Notes
90+
- Safe lookup is non-breaking for valid schemas.
91+
- Invalid schemas move from low-level exception to explicit, actionable diagnostics.
92+
93+
## Implementation Notes
94+
- Keep error text stable for test assertions.
95+
- Prefer adding tests before/with fix to prevent future regressions.

‎docs/covariance-validation-test-spec.md‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
# Covariance Validation Test Specification
2+
3+
## Purpose
4+
Define exhaustive tests for GraphQL interface implementation covariance and nullability compatibility in FSharp.Data.GraphQL type validation.
5+
6+
## Problem Statement (Current Bug)
7+
During schema initialization, executor calls `Validation.Types.validateTypeMap schema.TypeMap` and throws `GQLMessageException` when validation returns errors.
8+
9+
Observed runtime error pattern:
10+
- `'<Object>.<field>' field signature does not match it's definition in interface <Interface>`
11+
12+
Exact failure location in library:
13+
- `src/FSharp.Data.GraphQL.Server/Executor.fs` (schema startup validation)
14+
- `src/FSharp.Data.GraphQL.Shared/Validation.fs`, function `validateImplements`
15+
16+
Current implementation in `validateImplements` uses strict equality:
17+
- `Some objf when objf = f -> acc`
18+
- otherwise reports signature mismatch
19+
20+
This equality-based check is stricter than GraphQL spec subtyping rules for interface field return types and nullability covariance.
21+
22+
## GraphQL Compatibility Rules to Validate
23+
For object type `O implements I`, each interface field `f` must satisfy:
24+
1. Field exists on object with same name.
25+
2. Arguments are compatible (same required args; extra args on object must be optional).
26+
3. Return type on object is equal to or a valid subtype of interface return type.
27+
4. Non-null covariance: `T!` is subtype of `T` (allowed).
28+
5. List/null wrappers must be compared structurally by spec subtyping rules.
29+
6. If interface return type is interface/union, object return type may be a concrete implementing/member type (covariance).
30+
31+
## Nullable / StructNullable Coverage
32+
In this codebase:
33+
- `Nullable X` and `StructNullable X` both produce nullable GraphQL wrappers.
34+
- Non-wrapper `X` is non-null GraphQL type.
35+
36+
Tests must cover both wrappers equivalently for compatibility decisions:
37+
- `Nullable InterfaceType` vs concrete non-null implementor type.
38+
- `StructNullable InterfaceType` vs concrete non-null implementor type.
39+
- `Nullable T` vs `Nullable T` exact match.
40+
- `StructNullable T` vs `StructNullable T` exact match.
41+
- Negative cases where nested wrappers are incompatible (e.g., list item nullability mismatch).
42+
43+
## Generic Test Model (No domain-specific names)
44+
Use neutral names only:
45+
46+
Interfaces:
47+
- `IParentView`
48+
- `IChildView`
49+
50+
GraphQL interfaces:
51+
- `IChildInfo`
52+
- `IParentInfo` with field `child: IChildInfo`
53+
54+
Concrete object types:
55+
- `ChildAInfo implements IChildInfo`
56+
- `ChildBInfo implements IChildInfo`
57+
- `ParentAInfo implements IParentInfo` with `child: ChildAInfo`
58+
- `ParentBInfo implements IParentInfo` with `child: ChildBInfo`
59+
60+
This model must be reused for all covariance and nullability test cases.
61+
62+
## Test Matrix (Must Cover All Cases)
63+
### A. Positive covariance cases (must pass)
64+
1. Interface field type `IChildInfo`, object field type `ChildAInfo` (implements `IChildInfo`).
65+
2. Same as A1 for second implementation (`ChildBInfo`).
66+
3. Interface field `Nullable IChildInfo`, object field non-null `ChildAInfo`.
67+
4. Interface field `StructNullable IChildInfo`, object field non-null `ChildAInfo`.
68+
5. Interface field non-null `IChildInfo`, object field same non-null `IChildInfo` (exact).
69+
6. Interface field list `List<IChildInfo>`, object field list `List<ChildAInfo>` where library supports list covariance by member subtype.
70+
7. Deep wrappers: interface `Nullable(List(Nullable(IChildInfo)))`, object `List(ChildAInfo)` where valid by non-null covariance.
71+
72+
### B. Negative covariance cases (must fail)
73+
1. Interface field `IChildInfo`, object field unrelated object type `OtherInfo` (not implementing).
74+
2. Interface field non-null `IChildInfo`, object field nullable `Nullable IChildInfo` (wider, invalid).
75+
3. Interface field list `List<IChildInfo>`, object field scalar `ChildAInfo`.
76+
4. Interface field `List<NonNull IChildInfo>`, object field `List<Nullable ChildAInfo>` (invalid nullability widening).
77+
5. Interface field arguments mismatch (missing required arg, type mismatch, extra required arg).
78+
79+
### C. Nullable vs StructNullable parity (must pass/fail identically)
80+
For each scenario A3, A4, B2, B4 create paired tests:
81+
- one with `Nullable`
82+
- one with `StructNullable`
83+
Expected result must be identical for semantic-equivalent wrappers.
84+
85+
### D. Existing strict-equality regression (must reproduce old bug)
86+
Create a test where only difference is:
87+
- interface field type = interface def
88+
- object field type = implementing concrete object def
89+
90+
Expected by spec: Success.
91+
Current behavior before fix: ValidationError with signature mismatch message.
92+
This test documents the bug and prevents reintroduction.
93+
94+
## Test File Placement
95+
- Extend `tests/FSharp.Data.GraphQL.Tests/TypeValidationTests.fs` for focused unit cases, or
96+
- create `tests/FSharp.Data.GraphQL.Tests/TypeValidationCovarianceTests.fs` if separation is preferred.
97+
98+
## Assertion Style
99+
- Use `validateImplements` for unit-level behavior.
100+
- Use `validateTypeMap` for end-to-end schema-level validation with multiple types registered.
101+
- Verify exact error strings for negative tests where stable, otherwise verify error contains object+field+interface identifiers.
102+
103+
## Proposed Fix in Validation Engine
104+
Replace strict `objf = f` signature equality with structural GraphQL compatibility check:
105+
1. Compare field names and argument compatibility by spec rules.
106+
2. Compare return types via `isOutputSubtype(objectType, interfaceType)`.
107+
3. Implement recursive wrapper-aware subtype check:
108+
- `NonNull(A)` subtype of `A`
109+
- `List(A)` subtype of `List(B)` iff `A` subtype of `B`
110+
- object subtype of interface if object implements interface
111+
- object subtype of union if object is a union member
112+
- named scalars/enums require exact type identity
113+
114+
Pseudo-contract:
115+
- `isFieldImplementationCompatible(objectField, interfaceField) -> bool`
116+
- used by `validateImplements` instead of direct equality.
117+
118+
## Acceptance Criteria
119+
1. All positive covariance tests pass.
120+
2. All negative compatibility tests fail with deterministic errors.
121+
3. Nullable/StructNullable parity tests pass.
122+
4. No regressions in existing `TypeValidationTests.fs`.
123+
5. Schema initialization no longer throws for valid covariance implementations.
124+
125+
## Notes for Reviewers
126+
- This is a spec-driven validation correction, not a domain-model workaround.
127+
- Goal is GraphQL spec compliance at type-system validation layer.

0 commit comments

Comments
 (0)