Skip to content

Commit fb49cbd

Browse files
xperiandriclaude
xperiandri
and
claude
committed
Address the review of the deferred fragments
- `ResolveDeferredFragment` carries the directive's `if` condition, so a fragment disabled through a variable is resolved with the object, at the root too, instead of always being deferred. - A fragment selecting under a field the object selects directly adds that selection to the direct field instead of losing it. - A fragment spread directly is resolved with the object whichever of its spreads comes first: direct and deferred spreads are tracked apart, as graphql-js does. - The root fields of a deferred root fragment are coerced up front, so an argument they reject fails the request before any resolver runs. - Review suggestions applied: `String.Join` over the path instead of materializing it for `Path.Join`, `List.vchoose`, `yield!`, entries added to the abstraction map directly, the `StringBuilder` pipeline, XML comments on the planning helpers, and the fragment release note marked as a breaking change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 218d954 commit fb49cbd

15 files changed

Lines changed: 995 additions & 57 deletions

File tree

‎RELEASE_NOTES.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,7 +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 and left out of the fragment; a fragment spread twice at the same place is delivered once; 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
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
296296
* **Breaking Change** `BufferedStreamOptions.Interval` and `BufferedStreamOptions.PreferredBatchSize` are now `int voption`
297297
* **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
298298
* **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`

‎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)