From 2255a5ee9a5323c55670d4ea1f60c56e3c5a876d Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Mon, 21 Sep 2026 01:31:16 +0200 Subject: [PATCH 1/4] Fix validation of an inline fragment without a type condition `... { fields }` applies to the parent type; validation used to read the absent type condition with `ValueOption.Value` and throw, and left the fragment's fields out of the validation context. Co-Authored-By: Claude Fable 5.1 --- src/FSharp.Data.GraphQL.Shared/Validation.fs | 43 +++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/src/FSharp.Data.GraphQL.Shared/Validation.fs b/src/FSharp.Data.GraphQL.Shared/Validation.fs index 4a1b4020..dc1a41d0 100644 --- a/src/FSharp.Data.GraphQL.Shared/Validation.fs +++ b/src/FSharp.Data.GraphQL.Shared/Validation.fs @@ -412,12 +412,13 @@ module Ast = } |> List.singleton | InlineFragment inlineFrag -> - voption { - let! typeCondition = inlineFrag.TypeCondition - let! fragType = ctx.Schema.TryGetTypeByName typeCondition - let fragType = Inline fragType - return getFragSelectionSetInfo visitedFragments fragType inlineFrag.SelectionSet ctx - } + // An inline fragment without a type condition applies to its parent type + let fragType = + match inlineFrag.TypeCondition with + | ValueSome typeCondition -> ctx.Schema.TryGetTypeByName typeCondition |> ValueOption.ofOption + | ValueNone -> ValueSome ctx.FragmentOrParentType + fragType + |> ValueOption.map (fun fragType -> getFragSelectionSetInfo visitedFragments (Inline fragType) inlineFrag.SelectionSet ctx) |> ValueOption.defaultValue List.empty | FragmentSpread fragSpread -> voption { @@ -776,20 +777,22 @@ module Ast = (frag : FragmentDefinition) = let typeConditionsValid = - let fragType = voption { - let! typeCondition = frag.TypeCondition - return! schemaInfo.TryGetTypeByName typeCondition - } - match fragType with - | ValueSome _ -> Success - | ValueNone when frag.Name.IsSome -> - AstError.AsResult - $"Fragment '%s{frag.Name.Value}' has type condition '%s{frag.TypeCondition.Value}', but that type does not exist in the schema." - | ValueNone -> - AstError.AsResult ( - $"Inline fragment has type condition '%s{frag.TypeCondition.Value}', but that type does not exist in the schema.", - path - ) + match frag.TypeCondition with + // An inline fragment without a type condition applies to its parent type + | ValueNone -> Success + | ValueSome typeCondition -> + match schemaInfo.TryGetTypeByName typeCondition with + | Some _ -> Success + | None -> + match frag.Name with + | ValueSome name -> + AstError.AsResult + $"Fragment '%s{name}' has type condition '%s{typeCondition}', but that type does not exist in the schema." + | ValueNone -> + AstError.AsResult ( + $"Inline fragment has type condition '%s{typeCondition}', but that type does not exist in the schema.", + path + ) typeConditionsValid @@ (frag.SelectionSet |> ValidationResult.collect (checkFragmentTypeExistenceInSelection fragmentDefinitions schemaInfo path)) From c929b491a436bea3ba75230318cf93c5a6cc5796 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Mon, 21 Sep 2026 01:31:16 +0200 Subject: [PATCH 2/4] Add @defer on fragment spreads and inline fragments A deferred fragment 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 with the fragment's fields as the `data` object map. 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; two labeled fragments at the same path get distinct ids; fragments at the operation root and on abstract types are supported; an error that propagates up to the fragment completes it with the errors and no data. Co-Authored-By: Claude Fable 5.1 --- RELEASE_NOTES.md | 2 + docs/type-system.md | 2 +- .../GraphQLRequestHandler.fs | 16 +- .../IncrementalDelivery.fs | 76 ++++++-- .../SubscriptionPayloads.fs | 3 +- .../MiddlewareDefinitions.fs | 1 + src/FSharp.Data.GraphQL.Server/Execution.fs | 99 +++++++++- src/FSharp.Data.GraphQL.Server/IO.fs | 12 ++ src/FSharp.Data.GraphQL.Server/Linq.fs | 7 + src/FSharp.Data.GraphQL.Server/Planning.fs | 115 +++++++++--- src/FSharp.Data.GraphQL.Shared/TypeSystem.fs | 27 ++- .../IncrementalDeliveryEndToEndTests.fs | 55 +++++- .../AspNetCore/IncrementalDeliveryTests.fs | 32 +++- .../DeferredTests.fs | 175 +++++++++++++++++- tests/FSharp.Data.GraphQL.Tests/Helpers.fs | 11 +- 15 files changed, 564 insertions(+), 69 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 0f5f1113..6cbddf32 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -292,6 +292,7 @@ * **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 * **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 * **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 +* **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 * **Breaking Change** `BufferedStreamOptions.Interval` and `BufferedStreamOptions.PreferredBatchSize` are now `int voption` * **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 * **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 @@ * Fixed `graphql-transport-ws` never sending `complete` after the `next` of a query or mutation result, as the protocol requires * 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 * 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 +* Fixed validation of an inline fragment without a type condition (`... { … }`), which used to fail with an exception instead of applying to the parent type * Removed the internal `Observable.withCompletionMarker` diff --git a/docs/type-system.md b/docs/type-system.md index acac2d36..f32415e0 100644 --- a/docs/type-system.md +++ b/docs/type-system.md @@ -98,7 +98,7 @@ How the sequence is delivered depends on the query: - With `@defer` on a `Nullable (ListOf ...)` field the complete list is delivered in one deferred payload. - With `@stream` every item is delivered as soon as the sequence produces it and its fields are resolved. The enumeration is cancelled when the client unsubscribes. -Over `graphql-transport-ws`, a `@defer` or `@stream` field is delivered using the `pending`/`incremental`/`completed`/`hasNext` format used by graphql-js 17 and Apollo Client's `GraphQL17Alpha9Handler`. Each field is announced once, in a `pending` entry, and identified afterwards by a short id rather than its path. A deferred field is announced at the path of the object containing it, in the same payload as its own value, and that value is delivered as an object map of the one field, which the client merges into the announced object; a streamed field is announced at its own path as soon as the payload exposing its containing data is sent, and its items are delivered as the `items` of `incremental` entries, always in list order, a batch of items in one entry. The `label` of `@defer` or `@stream` surfaces as `pending.label`. Both directives take `if: Boolean = true`, which executes the field inline when false, and `@stream` takes `initialCount: Int = 0`, the number of items delivered with the initial payload before the rest is streamed. A payload that carries only GraphQL errors omits the top-level `data` property instead of sending `data: null`, matching the existing request-error contract used elsewhere in the transport. +Over `graphql-transport-ws`, a `@defer` or `@stream` field is delivered using the `pending`/`incremental`/`completed`/`hasNext` format used by graphql-js 17 and Apollo Client's `GraphQL17Alpha9Handler`. Each field is announced once, in a `pending` entry, and identified afterwards by a short id rather than its path. A deferred field is announced at the path of the object containing it, in the same payload as its own value, and that value is delivered as an object map of the one field, which the client merges into the announced object; a streamed field is announced at its own path as soon as the payload exposing its containing data is sent, and its items are delivered as the `items` of `incremental` entries, always in list order, a batch of items in one entry. `@defer` on a fragment spread or inline fragment delivers the fragment's fields together, as one payload of the object containing them, announced at that object's path; a field also selected directly on the object is executed with it, and an error propagating up to the fragment completes it with the errors and no data. The `label` of `@defer` or `@stream` surfaces as `pending.label`. Both directives take `if: Boolean = true`, which executes the field inline when false, and `@stream` takes `initialCount: Int = 0`, the number of items delivered with the initial payload before the rest is streamed. A payload that carries only GraphQL errors omits the top-level `data` property instead of sending `data: null`, matching the existing request-error contract used elsewhere in the transport. Streamed items can be grouped into batches. The `preferredBatchSize` argument of `@stream`, available with `SchemaConfig.DefaultWithBufferedStream`, has priority. Otherwise the `batching` parameter of the field applies. It is either a fixed size or a function that reads the size from the source, such as the page size of a paged SDK sequence. The function is evaluated lazily: only for a `@stream` query that does not itself specify `preferredBatchSize`, so it never runs for an ordinary or `@defer` query. diff --git a/src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs b/src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs index 179121e1..ca0c6a78 100644 --- a/src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs +++ b/src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs @@ -81,7 +81,21 @@ and [] GraphQLRequestHandler<'Root> if logger.IsEnabled LogLevel.Trace then logger.LogTrace ("GraphQL deferred errors:\n{errors}\nGraphQL deferred data:\n{data}", errors, serializeIndented data) | DeferredCompleted path -> - logger.LogDebug ("Completed GraphQL deferred field at path: {path}", path |> Seq.map string |> Seq.toArray |> Path.Join)) + logger.LogDebug ("Completed GraphQL deferred field at path: {path}", path |> Seq.map string |> Seq.toArray |> Path.Join) + | DeferredFragmentPending (path, label, fragmentId) -> + logger.LogDebug ( + "Announced GraphQL deferred fragment #{fragmentId} (label: {label}) at path: {path}", + fragmentId, + label |> ValueOption.toObj, + path |> Seq.map string |> Seq.toArray |> Path.Join + ) + | DeferredFragmentResult (data, errors, path, fragmentId) -> + logger.LogDebug ("Produced GraphQL deferred fragment #{fragmentId} result for path: {path}", fragmentId, path |> Seq.map string |> Seq.toArray |> Path.Join) + + if logger.IsEnabled LogLevel.Trace then + logger.LogTrace ("GraphQL deferred fragment errors:\n{errors}\nGraphQL deferred fragment data:\n{data}", errors, serializeIndented (data |> ValueOption.toObj)) + | DeferredFragmentCompleted (path, fragmentId) -> + logger.LogDebug ("Completed GraphQL deferred fragment #{fragmentId} at path: {path}", fragmentId, path |> Seq.map string |> Seq.toArray |> Path.Join)) GQLResponse.Direct (documentId, data, errs) diff --git a/src/FSharp.Data.GraphQL.Server.AspNetCore/IncrementalDelivery.fs b/src/FSharp.Data.GraphQL.Server.AspNetCore/IncrementalDelivery.fs index 2b267842..779ec958 100644 --- a/src/FSharp.Data.GraphQL.Server.AspNetCore/IncrementalDelivery.fs +++ b/src/FSharp.Data.GraphQL.Server.AspNetCore/IncrementalDelivery.fs @@ -39,6 +39,10 @@ module private IncrementalDeliveryPaths = | (:? string as fieldName) :: parentPathRev -> ValueSome (List.rev parentPathRev, fieldName) | _ -> ValueNone +/// Distinguishes a deferred fragment from the fields of the object it belongs to, in the key of its bookkeeping. +[] +type private FragmentKey = FragmentKey of fragmentId : int + /// /// Mutable per-field bookkeeping of IncrementalDelivery, keyed by a field's own path (with any item index or batch removed). /// @@ -104,28 +108,31 @@ type IncrementalDelivery () = | DeferredFieldPath (parentPath, _) -> parentPath | _ -> fieldPath - let stateFor (fieldPath : obj list) (isStream : bool) = - match fields.TryGetValue fieldPath with + /// The bookkeeping under the key, announced at the wire path; a closed key delivered again (a live field's + /// nested deferred fields on a later update) is a new delivery, with a fresh id and a fresh announcement + let stateUnder (key : obj list) (wirePath : obj list) (isStream : bool) = + match fields.TryGetValue key with | true, state when not state.Closed -> state, false | _ -> - // A closed path delivered again (a live field's nested deferred fields on a later update) is a new - // delivery: it gets a fresh id and a fresh announcement - let state = FieldState (string nextId, wirePathOf fieldPath isStream, isStream) + let state = FieldState (string nextId, wirePath, isStream) nextId <- nextId + 1 - fields[fieldPath] <- state + fields[key] <- state state, true + let stateFor (fieldPath : obj list) (isStream : bool) = stateUnder fieldPath (wirePathOf fieldPath isStream) isStream + + /// The key of a deferred fragment: the path of the object it belongs to, distinguished from that object's fields + let fragmentKey (path : obj list) (fragmentId : int) = [ yield! path; yield box (FragmentKey fragmentId) ] + + let stateForFragment (path : obj list) (fragmentId : int) = stateUnder (fragmentKey path fragmentId) path false + let pendingResultFor (state : FieldState) = { Id = state.Id Path = state.WirePath Label = state.Label |> Skippable.ofValueOption } - let announcePending (fieldPath : obj list) (label : string voption) (isStream : bool) (initialCount : int) = - // DeferredCompleted must be able to recover the field id even when a pre-announced stream completes without - // ever producing an item, so every pending announcement creates the per-field state eagerly. - let state, isNew = stateFor fieldPath isStream - + let announce (key : obj list) (label : string voption) (initialCount : int) (state : FieldState, isNew : bool) = match label with | ValueSome _ -> state.Label <- label | ValueNone -> () @@ -133,12 +140,20 @@ type IncrementalDelivery () = if isNew then // The items delivered with the initial payload are never streamed: the stream starts after them state.NextIndex <- initialCount - pending.Add (struct (fieldPath, pendingResultFor state)) + pending.Add (struct (key, pendingResultFor state)) state, isNew + let announcePending (fieldPath : obj list) (label : string voption) (isStream : bool) (initialCount : int) = + // DeferredCompleted must be able to recover the field id even when a pre-announced stream completes without + // ever producing an item, so every pending announcement creates the per-field state eagerly. + stateFor fieldPath isStream |> announce fieldPath label initialCount + let announceStream (fieldPath : obj list) = announcePending fieldPath ValueNone true 0 + let announceFragment (path : obj list) (label : string voption) (fragmentId : int) = + stateForFragment path fragmentId |> announce (fragmentKey path fragmentId) label 0 + let rec pathExistsInData (relativePath : obj list) (data : obj) = match relativePath, data with | [], _ -> true @@ -283,6 +298,32 @@ type IncrementalDelivery () = let pending = [ yield! fieldPending; yield! takePendingVisibleIn wirePath wireData ] ValueSome (SubscriptionExecutionResult.CreateSubsequent (pending, [ incremental ], [], true)) + /// A deferred fragment's fields, delivered together as an object map of the object it belongs to, or the errors + /// that propagated up to the fragment itself, which complete it without data. + let fragmentEvent (path : obj list) (fragmentId : int) (data : Output voption) (errors : GQLProblemDetails list) = + let key = fragmentKey path fragmentId + let state, isNew = stateForFragment path fragmentId + let fieldPending = + match takeFieldPending key with + | [] -> pendingFor state isNew + | pending -> pending + match data with + | ValueSome data -> + let incremental = { + Id = state.Id + SubPath = Skip + Data = Include (ValueSome (box data)) + Items = Skip + Errors = (if errors.IsEmpty then Skip else Include errors) + } + let pending = [ yield! fieldPending; yield! takePendingVisibleIn path (box data) ] + ValueSome (SubscriptionExecutionResult.CreateSubsequent (pending, [ incremental ], [], true)) + | ValueNone -> + // The object the fragment belongs to was already delivered, so nothing can be nulled on the client: the + // fragment is announced, if not yet, and completed with its errors in the same payload + state.Closed <- true + ValueSome (SubscriptionExecutionResult.CreateSubsequent (fieldPending, [], [ { Id = state.Id; Errors = Include errors } ], true)) + /// Closes the field, completing it for the client when the client learned of it. let complete (fieldPath : obj list) (state : FieldState) (errors : GQLProblemDetails list Skippable) = state.Closed <- true @@ -351,6 +392,17 @@ type IncrementalDelivery () = | _ -> // Already closed by a preceding stream failure ValueNone + | DeferredFragmentPending (path, label, fragmentId) -> + announceFragment path label fragmentId |> ignore + ValueNone + | DeferredFragmentResult (data, errors, path, fragmentId) -> fragmentEvent path fragmentId data errors + | DeferredFragmentCompleted (path, fragmentId) -> + let key = fragmentKey path fragmentId + match fields.TryGetValue key with + | true, state when not state.Closed -> complete key state Skip + | _ -> + // Already closed by the errors that propagated up to the fragment + ValueNone /// /// The final payload of the delivery: completes every field the client learned of that has not completed on diff --git a/src/FSharp.Data.GraphQL.Server.AspNetCore/SubscriptionPayloads.fs b/src/FSharp.Data.GraphQL.Server.AspNetCore/SubscriptionPayloads.fs index cb06fbca..c632e7bd 100644 --- a/src/FSharp.Data.GraphQL.Server.AspNetCore/SubscriptionPayloads.fs +++ b/src/FSharp.Data.GraphQL.Server.AspNetCore/SubscriptionPayloads.fs @@ -75,7 +75,8 @@ type internal DeferredPayloads /// member _.TryAbsorbBeforeInitial event = match event with - | DeferredPending _ -> + | DeferredPending _ + | DeferredFragmentPending _ -> // Announced before the initial payload, so it can be part of its pending entries delivery.Apply event |> ignore true diff --git a/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs b/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs index 7b354bef..82975b3f 100644 --- a/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs +++ b/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs @@ -57,6 +57,7 @@ type internal QueryWeightMiddleware (threshold : float, reportToMetadata : bool) | ResolveDeferred info -> checkThreshold current (info :: xs) | ResolveStreamed (info, _) -> checkThreshold current (info :: xs) | ResolveLive info -> checkThreshold current (info :: xs) + | ResolveDeferredFragment (_, _, fields) -> checkThreshold current (fields @ xs) checkThreshold 0.0 fields let error (ctx : ExecutionContext) = GQLExecutionResult.ErrorAsync ( diff --git a/src/FSharp.Data.GraphQL.Server/Execution.fs b/src/FSharp.Data.GraphQL.Server/Execution.fs index d6760bbb..b6e833d4 100644 --- a/src/FSharp.Data.GraphQL.Server/Execution.fs +++ b/src/FSharp.Data.GraphQL.Server/Execution.fs @@ -328,6 +328,35 @@ let private deferResultsCompleted path (res : ResolverResult) : IObservable let completed = Observable.singleton (DeferredCompleted (normalizeErrorPath path)) withNestedEvents ownResult nested (ValueSome completed) +/// +/// The delivery of a fragment deferred with @defer: its fields, resolved together as one object of the object +/// at , announced up front when labeled, then its own nested deferred and streamed +/// fields, exactly as for a deferred field. +/// +let private deferredFragmentEvents + (label : string voption) + (fragmentId : int) + (fragmentPath : FieldPath) + (fragment : AsyncVal>>) + : IObservable + = + let events = + fragment + |> Observable.ofAsyncVal + |> Observable.bind (fun result -> + let ownResult, nested = + match result with + | Ok (data, nested, errs) -> + Observable.singleton (DeferredFragmentResult (ValueSome (data.Value :?> Output), errs, fragmentPath, fragmentId)), nested + // An error propagated up to the fragment itself: the object it belongs to was already delivered, so + // the fragment completes with the errors and delivers no data + | Error errs -> Observable.singleton (DeferredFragmentResult (ValueNone, errs, fragmentPath, fragmentId)), ValueNone + let completed = Observable.singleton (DeferredFragmentCompleted (fragmentPath, fragmentId)) + withNestedEvents ownResult nested (ValueSome completed)) + match label with + | ValueSome _ -> AnnouncedEvents.announced (DeferredFragmentPending (fragmentPath, label, fragmentId)) events + | ValueNone -> events + /// Collect together an array of results using the appropriate execution strategy. let collectFields (strategy : ExecutionStrategy) @@ -816,14 +845,36 @@ and executeObjectFields | Ok fieldCtx -> executeResolvers inputContext fieldCtx fieldPath value (resolveField resolver fieldCtx value) | Error errs -> asyncVal { return Error (errs |> List.map GQLProblemDetails.OfError) } - let! res = + // A deferred fragment stands among the fields of the object; it contributes nothing to the object's own value + // and is delivered afterwards, its fields resolved together against the same object + let ownFields, deferredFragments = fields + |> List.partition (fun field -> + match field.Kind with + | ResolveDeferredFragment _ -> false + | _ -> true) + + let executeDeferredFragment (deferred : IObservable voption) (fragment : ExecutionInfo) = + match fragment.Kind with + | ResolveDeferredFragment (label, fragmentId, fragmentFields) -> + let events = + executeObjectFields fragmentFields objName objDef inputContext ctx path value + |> deferredFragmentEvents label fragmentId (normalizeErrorPath path) + match deferred with + | ValueSome deferred -> ValueSome (AnnouncedEvents.merge deferred events) + | ValueNone -> ValueSome events + | _ -> deferred + + let! res = + ownFields |> Seq.map executeField |> Seq.toArray |> collectFields Parallel match res with | Error errs -> return Error errs - | Ok (kvps, def, errs) -> return Ok (KeyValuePair (objName, box <| NameValueLookup (kvps)), def, errs) + | Ok (kvps, nested, errs) -> + let deferred = deferredFragments |> List.fold executeDeferredFragment nested + return Ok (KeyValuePair (objName, box <| NameValueLookup (kvps)), deferred, errs) } let internal compileSubscriptionField (subfield : SubscriptionFieldDef) = @@ -892,13 +943,42 @@ let private executeQueryOrMutation | Ok r -> return Ok r } + /// A fragment deferred at the operation's root: its fields are root fields, resolved together against the root + /// value once the root's own fields have been delivered + let executeRootDeferredFragment (deferred : IObservable voption) (info : ExecutionInfo) = + match info.Kind with + | ResolveDeferredFragment (label, fragmentId, fragmentFields) -> + let rootCtx = { + ExecutionInfo = info + Context = ctx + ReturnType = objDef + ParentType = objDef + Schema = ctx.Schema + Args = Map.empty + Variables = ctx.Variables + Path = [] + } + let events = + executeObjectFields fragmentFields objDef.Name objDef ctx.GetInputContext rootCtx [] rootValue + |> deferredFragmentEvents label fragmentId [] + match deferred with + | ValueSome deferred -> ValueSome (AnnouncedEvents.merge deferred events) + | ValueNone -> ValueSome events + | _ -> deferred + asyncVal { let documentId = ctx.ExecutionPlan.DocumentId + let rootFields, deferredFragments = + resultSet + |> Array.partition (fun (_, info) -> + match info.Kind with + | ResolveDeferredFragment _ -> false + | _ -> true) // Inline argument coercion is request validation, the same as variable coercion in Executor.eval's // coerceVariables: it rejects the request before any root resolver runs, so its errors must never be // reported as an execution result with null data let coerced = SortedDictionary * IGQLError list)>() - resultSet + rootFields |> Array.iteri (fun i (_, info) -> let argDefs = ctx.FieldExecuteMap.GetArgs (ctx.ExecutionPlan.RootDef.Name, info.Definition.Name) match getArgumentValues argDefs info.Ast.Arguments ctx.GetInputContext ctx.Variables with @@ -913,12 +993,17 @@ let private executeQueryOrMutation else let operations = coerced - |> Seq.map (fun (KeyValue (i, struct (args, _))) -> executeRootOperation resultSet[i] args) + |> Seq.map (fun (KeyValue (i, struct (args, _))) -> executeRootOperation rootFields[i] args) |> Seq.toArray match! operations |> collectFields ctx.ExecutionPlan.Strategy with - | Ok (data, ValueSome deferred, errs) -> - return GQLExecutionResult.Deferred (documentId, NameValueLookup (data), errs, deferred, ctx.Metadata) - | Ok (data, ValueNone, errs) -> return GQLExecutionResult.Direct (documentId, NameValueLookup (data), errs, ctx.Metadata) + | Ok (data, nested, errs) -> + let deferred = + (nested, deferredFragments) + ||> Array.fold (fun deferred (_, info) -> executeRootDeferredFragment deferred info) + match deferred with + | ValueSome deferred -> + return GQLExecutionResult.Deferred (documentId, NameValueLookup (data), errs, deferred, ctx.Metadata) + | ValueNone -> return GQLExecutionResult.Direct (documentId, NameValueLookup (data), errs, ctx.Metadata) // Only a non-null root field failing during execution reaches this branch: an execution result whose // data is null, as the spec requires, unlike the request error returned above for a coercion failure | Error errs -> return GQLExecutionResult.Direct (documentId, null, errs, ctx.Metadata) diff --git a/src/FSharp.Data.GraphQL.Server/IO.fs b/src/FSharp.Data.GraphQL.Server/IO.fs index 6302efc8..09f32fbb 100644 --- a/src/FSharp.Data.GraphQL.Server/IO.fs +++ b/src/FSharp.Data.GraphQL.Server/IO.fs @@ -229,6 +229,18 @@ and GQLDeferredResponseContent = | DeferredErrors of Data : obj * Errors : GQLProblemDetails list * Path : FieldPath /// Marks a deferred or streamed field as fully delivered. | DeferredCompleted of Path : FieldPath + /// Announces a labeled deferred fragment of the object at the path, identified within that object by the id. + | DeferredFragmentPending of Path : FieldPath * Label : string voption * FragmentId : int + /// + /// Delivers the fields of a deferred fragment as an object map to merge into the object at the path. + /// + /// + /// Data is when an error propagated up to the fragment itself: the object containing it + /// was already delivered, so the fragment completes with the errors and delivers no data. + /// + | DeferredFragmentResult of Data : Output voption * Errors : GQLProblemDetails list * Path : FieldPath * FragmentId : int + /// Marks a deferred fragment as fully delivered. + | DeferredFragmentCompleted of Path : FieldPath * FragmentId : int /// Represents events emitted by a live GraphQL subscription. and GQLSubscriptionResponseContent = diff --git a/src/FSharp.Data.GraphQL.Server/Linq.fs b/src/FSharp.Data.GraphQL.Server/Linq.fs index 2f24b366..2c3703f7 100644 --- a/src/FSharp.Data.GraphQL.Server/Linq.fs +++ b/src/FSharp.Data.GraphQL.Server/Linq.fs @@ -425,6 +425,12 @@ let rec private compose inputContext vars ir = /// Get unrelated tracks from current info and its children (if any) /// Returned set of trackers ALWAYS consists of Direct trackers only let rec private getTracks alreadyFound info = + match info.Kind with + // A deferred fragment has no resolver of its own: its fields are tracked as fields of the object containing it + | ResolveDeferredFragment (_, _, fields) -> IR(info, Set.empty, fields |> List.map (getTracks alreadyFound)) + | _ -> getFieldTracks alreadyFound info + +and private getFieldTracks alreadyFound info = let expr = match info.Definition.Resolve.Expr with | (Patterns.WithValue(_,_, (Patterns.Lambda(_, Patterns.Lambda(_, expr))))) -> expr @@ -434,6 +440,7 @@ let rec private getTracks alreadyFound info = |> Set.map(fun track -> Direct(track, [])) |> flip Set.difference alreadyFound match info.Kind with + | ResolveDeferredFragment _ -> getTracks alreadyFound info | ResolveDeferred inner -> getTracks alreadyFound inner | ResolveStreamed (inner,_) -> getTracks alreadyFound inner | ResolveLive inner -> getTracks alreadyFound inner diff --git a/src/FSharp.Data.GraphQL.Server/Planning.fs b/src/FSharp.Data.GraphQL.Server/Planning.fs index 55283851..77d38d96 100644 --- a/src/FSharp.Data.GraphQL.Server/Planning.fs +++ b/src/FSharp.Data.GraphQL.Server/Planning.fs @@ -222,6 +222,7 @@ let private kindName (kind : ExecutionInfoKind) = | ResolveDeferred _ -> nameof ResolveDeferred | ResolveStreamed _ -> nameof ResolveStreamed | ResolveLive _ -> nameof ResolveLive + | ResolveDeferredFragment _ -> nameof ResolveDeferredFragment let private getSelectionFrag = function | SelectFields(fragmentFields) -> fragmentFields @@ -262,11 +263,64 @@ let rec private deepMerge (xs: ExecutionInfo list) (ys: ExecutionInfo list) = |> List.filter(fun y -> not <| List.exists(fun x -> x.Identifier = y.Identifier) xs') xs' @ ys' +/// The state of planning one selection set together with the fragments spread into it: the fragments already +/// planned, each spread once, and the next id of a deferred fragment, unique among the deferred fragments of the +/// selection set. +type private SelectionScope = { + mutable VisitedFragments : string list + mutable NextFragmentId : int +} + +let private newScope () = { VisitedFragments = []; NextFragmentId = 0 } + +/// The @defer directive of a fragment spread or inline fragment, when it applies +let private deferredFragmentDirective (directives : Directive list) = + directives |> List.vtryFind (fun d -> d.Name = "defer" && isEnabledAtPlanning d) + +let private directiveLabel (directive : Directive) = + directive.Arguments + |> List.vtryFind (fun argument -> argument.Name = "label") + |> ValueOption.bind (fun argument -> + match argument.Value with + | StringValue label -> ValueSome label + | _ -> ValueNone) + +let private allocateFragmentId (scope : SelectionScope) = + let fragmentId = scope.NextFragmentId + scope.NextFragmentId <- fragmentId + 1 + fragmentId + +/// The plan entry delivering the fields of a deferred fragment as one payload of the object containing them; it +/// stands among that object's fields under an identifier no field can have +let private deferredFragmentEntry (directive : Directive) (fragmentId : int) (info : ExecutionInfo) (fragmentFields : ExecutionInfo list) = + { info with + Identifier = $"@defer#{fragmentId}" + Kind = ResolveDeferredFragment (directiveLabel directive, fragmentId, fragmentFields) } + +/// A field selected directly on the object is executed with it, so a deferred fragment that also selects it +/// delivers only its other fields; a fragment left without fields delivers nothing and is dropped +let private withoutDirectlySelectedFields (plannedFields : ExecutionInfo list) = + let directlySelected = + plannedFields + |> List.choose (fun field -> + match field.Kind with + | ResolveDeferredFragment _ -> None + | _ -> Some field.Identifier) + |> Set.ofList + plannedFields + |> List.choose (fun field -> + match field.Kind with + | ResolveDeferredFragment (label, fragmentId, fragmentFields) -> + match fragmentFields |> List.filter (fun fragmentField -> not (directlySelected.Contains fragmentField.Identifier)) with + | [] -> None + | remaining -> Some { field with Kind = ResolveDeferredFragment (label, fragmentId, remaining) } + | _ -> Some field) + let rec private plan (ctx : PlanningContext) (info : ExecutionInfo) : ExecutionInfo = match info.ReturnDef with | Leaf _ -> info - | SubscriptionObject _ -> planSelection ctx info.Ast.SelectionSet info (ref []) - | Object _ -> planSelection ctx info.Ast.SelectionSet info (ref []) + | SubscriptionObject _ -> planSelection ctx info.Ast.SelectionSet info (newScope ()) + | Object _ -> planSelection ctx info.Ast.SelectionSet info (newScope ()) | Nullable returnDef -> let inner = plan ctx { info with ParentDef = info.ReturnDef; ReturnDef = downcast returnDef } { inner with IsNullable = true } @@ -275,7 +329,7 @@ let rec private plan (ctx : PlanningContext) (info : ExecutionInfo) : ExecutionI let inner = plan ctx { info with ParentDef = info.ReturnDef; ReturnDef = downcast returnDef; } { info with Kind = ResolveCollection inner } | Abstract _ -> - planAbstraction ctx info.Ast.SelectionSet info (ref []) ValueNone + planAbstraction ctx info.Ast.SelectionSet info (newScope ()) ValueNone | returnDef -> Debug.Fail "Must be prevented by validation" raise ( @@ -283,8 +337,14 @@ let rec private plan (ctx : PlanningContext) (info : ExecutionInfo) : ExecutionI $"Field '%s{info.Identifier}' returns the type definition '{returnDef}' implemented by '%s{returnDef.GetType().FullName}', which is not supported by query planning" ) -and private planSelection (ctx: PlanningContext) (selectionSet: Selection list) (info: ExecutionInfo) visitedFragments : ExecutionInfo = +and private planSelection (ctx: PlanningContext) (selectionSet: Selection list) (info: ExecutionInfo) (scope : SelectionScope) : ExecutionInfo = let parentDef = downcast info.ReturnDef + /// The fields of a fragment merged into the object's selection, or, when the fragment is deferred, delivered + /// later as one payload of the object + let withFragmentFields (fields : ExecutionInfo list) (directives : Directive list) (fragmentFields : ExecutionInfo list) = + match deferredFragmentDirective directives with + | ValueSome directive -> fields @ [ deferredFragmentEntry directive (allocateFragmentId scope) info fragmentFields ] + | ValueNone -> deepMerge fields fragmentFields // filter out already existing fields let plannedFields = selectionSet |> List.fold(fun (fields : ExecutionInfo list) (selection : Selection) -> @@ -306,31 +366,38 @@ and private planSelection (ctx: PlanningContext) (selectionSet: Selection list) | Planned -> fields @ [ executionPlan ] | FragmentSpread spread -> let spreadName = spread.Name - if visitedFragments.Value |> List.exists (fun name -> name = spreadName) + if scope.VisitedFragments |> List.exists (fun name -> name = spreadName) then fields // Fragment already found else - visitedFragments.Value <- spreadName :: visitedFragments.Value + scope.VisitedFragments <- spreadName :: scope.VisitedFragments match ctx.Document.Definitions |> List.tryFind (function FragmentDefinition f -> f.Name.Value = spreadName | _ -> false) with | Some (FragmentDefinition fragment) when doesFragmentTypeApply ctx.Schema fragment parentDef -> // Retrieve fragment data just as it was normal selection set // TODO: Check if the path is correctly defined - let fragmentInfo = planSelection ctx fragment.SelectionSet updatedInfo visitedFragments + let fragmentInfo = planSelection ctx fragment.SelectionSet updatedInfo scope let fragmentFields = getSelectionFrag fragmentInfo.Kind - // filter out already existing fields - deepMerge fields fragmentFields - // List.mergeBy (fun field -> field.Identifier) fields fragmentFields, deferedFields' + withFragmentFields fields spread.Directives fragmentFields | _ -> fields | InlineFragment fragment when doesFragmentTypeApply ctx.Schema fragment parentDef -> // retrieve fragment data just as it was normal selection set - let fragmentInfo = planSelection ctx fragment.SelectionSet updatedInfo visitedFragments + let fragmentInfo = planSelection ctx fragment.SelectionSet updatedInfo scope let fragmentFields = getSelectionFrag fragmentInfo.Kind - // filter out already existing fields - deepMerge fields fragmentFields + withFragmentFields fields fragment.Directives fragmentFields | _ -> fields ) [] - { info with Kind = SelectFields plannedFields } + { info with Kind = SelectFields (withoutDirectlySelectedFields plannedFields) } -and private planAbstraction (ctx:PlanningContext) (selectionSet: Selection list) (info : ExecutionInfo) visitedFragments typeCondition : ExecutionInfo = +and private planAbstraction (ctx:PlanningContext) (selectionSet: Selection list) (info : ExecutionInfo) (scope : SelectionScope) typeCondition : ExecutionInfo = + /// The fields of a fragment merged into every type's selection, or, when the fragment is deferred, delivered + /// later as one payload of the object, whatever its concrete type turns out to be + let withFragmentFields (fields : Map) (directives : Directive list) (fragmentFields : Map) = + match deferredFragmentDirective directives with + | ValueSome directive -> + let fragmentId = allocateFragmentId scope + fragmentFields + |> Map.map (fun _ typeFields -> [ deferredFragmentEntry directive fragmentId info typeFields ]) + |> Map.merge (fun _ -> deepMerge) fields + | ValueNone -> Map.merge (fun _ -> deepMerge) fields fragmentFields // Filter out already existing fields let plannedTypeFields = selectionSet |> List.fold(fun (fields : Map) selection -> @@ -348,28 +415,26 @@ and private planAbstraction (ctx:PlanningContext) (selectionSet: Selection list) | Planned -> Map.merge (fun _ -> deepMerge) fields infoMap | FragmentSpread spread -> let spreadName = spread.Name - if visitedFragments.Value |> List.exists (fun name -> name = spreadName) + if scope.VisitedFragments |> List.exists (fun name -> name = spreadName) then fields // Fragment already found else - visitedFragments.Value <- spreadName :: visitedFragments.Value + scope.VisitedFragments <- spreadName :: scope.VisitedFragments match ctx.Document.Definitions |> List.tryFind (function FragmentDefinition f -> f.Name.Value = spreadName | _ -> false) with | Some (FragmentDefinition fragment) -> // Retrieve fragment data just as it was normal selection set - let fragmentInfo = planAbstraction ctx fragment.SelectionSet innerData visitedFragments fragment.TypeCondition + let fragmentInfo = planAbstraction ctx fragment.SelectionSet innerData scope fragment.TypeCondition let fragmentFields = getAbstractionFrag fragmentInfo.Kind - // Filter out already existing fields - Map.merge (fun _ -> deepMerge) fields fragmentFields + withFragmentFields fields spread.Directives fragmentFields | _ -> fields | InlineFragment fragment -> // Retrieve fragment data just as it was normal selection set - let fragmentInfo = planAbstraction ctx fragment.SelectionSet innerData visitedFragments fragment.TypeCondition + let fragmentInfo = planAbstraction ctx fragment.SelectionSet innerData scope fragment.TypeCondition let fragmentFields = getAbstractionFrag fragmentInfo.Kind - // Filter out already existing fields - Map.merge (fun _ -> deepMerge) fields fragmentFields + withFragmentFields fields fragment.Directives fragmentFields ) Map.empty // Always return ResolveAbstraction kind, even for empty maps. // An empty map is a valid state representing "no fields selected for this type condition." - { info with Kind = ResolveAbstraction plannedTypeFields } + { info with Kind = ResolveAbstraction (plannedTypeFields |> Map.map (fun _ -> withoutDirectlySelectedFields)) } let private planVariables (schema: ISchema) (operation: OperationDefinition) = operation.VariableDefinitions @@ -396,7 +461,7 @@ let internal planOperation (ctx: PlanningContext) : ExecutionPlan = Definition = Unchecked.defaultof Include = incl IsNullable = false } - let resolvedInfo = planSelection ctx ctx.Operation.SelectionSet rootInfo (ref []) + let resolvedInfo = planSelection ctx ctx.Operation.SelectionSet rootInfo (newScope ()) let fields = match resolvedInfo.Kind with | SelectFields tf -> tf diff --git a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs index b9d7d855..e9011a4f 100644 --- a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs +++ b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs @@ -708,6 +708,13 @@ and ExecutionInfo = { /// Get a nested info recognized by path provided as parameter. Path may consist of fields names or aliases. member this.GetPath (keys : string list) : ExecutionInfo voption = + // The fields of a deferred fragment belong to the selection of the object containing it + let rec flattenDeferredFragments (fields : ExecutionInfo list) = + fields + |> Seq.collect (fun f -> + match f.Kind with + | ResolveDeferredFragment (_, _, fragmentFields) -> flattenDeferredFragments fragmentFields + | _ -> Seq.singleton f) let rec path info segments = match segments with | [] -> @@ -721,15 +728,17 @@ and ExecutionInfo = { | ResolveStreamed (inner, _) -> path inner segments | ResolveValue -> ValueNone | ResolveCollection inner -> path inner segments - | SelectFields fields -> + | SelectFields fields + | ResolveDeferredFragment (_, _, fields) -> fields - |> List.vtryFind (fun f -> f.Identifier = head) + |> flattenDeferredFragments + |> Seq.vtryFind (fun f -> f.Identifier = head) |> ValueOption.bind (fun f -> path f tail) | ResolveAbstraction typeMap -> typeMap |> Map.toSeq |> Seq.map snd - |> Seq.collect id + |> Seq.collect flattenDeferredFragments |> Seq.vtryFind (fun f -> f.Identifier = head) |> ValueOption.bind (fun f -> path f tail) path this keys @@ -758,6 +767,15 @@ and ExecutionInfo = { sb.Append("ResolveLive: ").AppendLine (nameAs info) |> ignore str (indent + 1) sb inner + | ResolveDeferredFragment (label, fragmentId, fields) -> + pad indent sb + let labelText = + match label with + | ValueSome label -> $" (label: {label})" + | ValueNone -> "" + sb.Append("ResolveDeferredFragment: ").AppendLine ($"#{fragmentId}{labelText}") + |> ignore + fields |> List.iter (str (indent + 1) sb) | ResolveStreamed (inner, mode) -> pad indent sb sb.Append("ResolveStreamed: ").AppendLine (nameAs info) @@ -810,6 +828,9 @@ and ExecutionInfoKind = | ResolveStreamed of ExecutionInfo * BufferedStreamOptions /// Reduce the current field as a live query. | ResolveLive of ExecutionInfo + /// Reduce a fragment deferred with @defer: its fields are delivered later, as one payload + /// of the object containing them, identified within that object's selection by the id. + | ResolveDeferredFragment of label : string voption * fragmentId : int * fields : ExecutionInfo list /// Buffered stream options. Used to specify how the buffer will behavior in a stream. and BufferedStreamOptions = { diff --git a/tests/FSharp.Data.GraphQL.Tests/AspNetCore/IncrementalDeliveryEndToEndTests.fs b/tests/FSharp.Data.GraphQL.Tests/AspNetCore/IncrementalDeliveryEndToEndTests.fs index 590f565b..050c62f5 100644 --- a/tests/FSharp.Data.GraphQL.Tests/AspNetCore/IncrementalDeliveryEndToEndTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/AspNetCore/IncrementalDeliveryEndToEndTests.fs @@ -31,7 +31,8 @@ let private translate (data : Output) (errors : GQLProblemDetails list) (events payloads.Add (SubscriptionExecutionResult.CreateInitial (data, errors, delivery.TakePendingVisibleIn data)) for event in events do match event with - | DeferredPending _ when not initialSent -> delivery.Apply event |> ignore + | DeferredPending _ + | DeferredFragmentPending _ when not initialSent -> delivery.Apply event |> ignore | event -> sendInitial () delivery.Apply event |> ValueOption.iter payloads.Add @@ -315,6 +316,58 @@ let ``A live field is announced with its first update and only closed by the fin final.HasNext |> equals (Include false) | payloads -> fail $"Expected three payloads but got %A{payloads}" +[] +let ``A deferred fragment is announced and delivered as one payload of its object`` () = + let query = parse """{ + testData { + id + ... @defer(label: "rest") { + a + b + } + } + }""" + let payloads = executor.AsyncExecute(query, getMockInputContext) |> sync |> deliver + assertWellFormed payloads |> ignore + match payloads with + | [ initial; delivered; completed; final ] -> + initial.Data + |> equals (Include (ValueSome (box (NameValueLookup.ofList [ "testData", upcast NameValueLookup.ofList [ "id", upcast "1" ] ])))) + let pending = pendingOf initial |> single + pending.Path |> equals [ box "testData" ] + pending.Label |> equals (Include "rest") + let entry = incrementalOf delivered |> single + entry.Id |> equals pending.Id + entry.Data |> equals (Include (ValueSome (box (NameValueLookup.ofList [ "a", upcast "Apple"; "b", upcast "Banana" ])))) + (completedOf completed |> single).Id |> equals pending.Id + final.HasNext |> equals (Include false) + | payloads -> fail $"Expected four payloads but got %A{payloads}" + +[] +let ``A deferred fragment that fails as a whole is completed with its errors`` () = + let query = parse """{ + testData { + id + ... @defer { + nonNullError + } + } + }""" + let payloads = executor.AsyncExecute(query, getMockInputContext) |> sync |> deliver + assertWellFormed payloads |> ignore + match payloads with + | [ initial; failed; final ] -> + initial.Pending |> equals Skip + let pending = pendingOf failed |> single + pending.Path |> equals [ box "testData" ] + incrementalOf failed |> empty + let completion = completedOf failed |> single + completion.Id |> equals pending.Id + completion.Errors + |> equals (Include [ GQLProblemDetails.CreateWithKind ("Non-null field error!", Execution, [ box "testData"; box "nonNullError" ]) ]) + final.HasNext |> equals (Include false) + | payloads -> fail $"Expected three payloads but got %A{payloads}" + [] let ``Field-level defer is delivered as an object map at the parent's path`` () = let query = parse """{ diff --git a/tests/FSharp.Data.GraphQL.Tests/AspNetCore/IncrementalDeliveryTests.fs b/tests/FSharp.Data.GraphQL.Tests/AspNetCore/IncrementalDeliveryTests.fs index 0556fd64..ed34b638 100644 --- a/tests/FSharp.Data.GraphQL.Tests/AspNetCore/IncrementalDeliveryTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/AspNetCore/IncrementalDeliveryTests.fs @@ -333,15 +333,35 @@ let ``The same deferred field announced twice with the same label is announced t let id = pendingIds payload |> single (incrementalOf payload |> single).Id |> equals id -[] -let ``Distinct labels at the same path are distinct pendings`` () = +[] +let ``Distinct fragments deferred at the same path are distinct pendings`` () = let delivery = IncrementalDelivery () let path = [ box "testData" ] - delivery.Apply (DeferredPending (path, ValueSome "a", false, 0)) |> ignore - delivery.Apply (DeferredPending (path, ValueSome "b", false, 0)) |> ignore - let payload = delivery.Apply (DeferredResult (box (NameValueLookup.ofList [ "a", upcast "Apple" ]), path)) + delivery.Apply (DeferredFragmentPending (path, ValueSome "a", 0)) |> equals ValueNone + delivery.Apply (DeferredFragmentPending (path, ValueSome "b", 1)) |> equals ValueNone + let payload = delivery.Apply (DeferredFragmentResult (ValueSome (upcast NameValueLookup.ofList [ "a", upcast "Apple" ]), [], path, 0)) + // The first fragment's payload exposes the object both fragments belong to, so both are announced with it pendingLabels payload |> equals [ Include "a"; Include "b" ] - pendingIds payload |> List.distinct |> List.length |> equals 2 + let ids = pendingIds payload + ids |> List.distinct |> List.length |> equals 2 + (incrementalOf payload |> single).Id |> equals ids.Head + (incrementalOf payload |> single).Data + |> equals (Include (ValueSome (box (NameValueLookup.ofList [ "a", upcast "Apple" ])))) + (completedOf (delivery.Apply (DeferredFragmentCompleted (path, 1))) |> single).Id |> equals ids[1] + +[] +let ``A deferred fragment failing as a whole is announced and completed with its errors in one payload`` () = + let delivery = IncrementalDelivery () + let path = [ box "testData" ] + let error = fieldError "Non-null field error!" (path @ [ box "nonNullError" ]) + let payload = delivery.Apply (DeferredFragmentResult (ValueNone, [ error ], path, 0)) + let id = pendingIds payload |> single + incrementalOf payload |> empty + let completion = completedOf payload |> single + completion.Id |> equals id + completion.Errors |> equals (Include [ error ]) + delivery.Apply (DeferredFragmentCompleted (path, 0)) |> equals ValueNone + (delivery.Finish ()).Completed |> equals Skip [] let ``A pre-announced stream whose parent is null is neither announced nor completed`` () = diff --git a/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs b/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs index e49e645a..cc923c76 100644 --- a/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs @@ -202,6 +202,8 @@ let DataType = Define.Field("bufferedList", ListOf AsyncDataType, (fun _ d -> d.bufferedList)) Define.Field("nullObject", Nullable AsyncDataType, (fun _ (d: TestSubject) -> d.nullObject)) Define.Field("container", Nullable ContainerType, (fun _ (d: TestSubject) -> Some d.container)) + // A non-null field whose failure propagates to the object containing it + Define.Field("nonNullError", StringType, (fun _ (_ : TestSubject) -> failwith "Non-null field error!")) ]) let data = { @@ -1839,7 +1841,7 @@ let ``Stream directive label is announced in the stream's pending marker`` () = |> Seq.head |> equals (DeferredPending ([ "testData"; "ifaceList" ], ValueSome "friends", true, 0)) -[] +[] let ``Defer directive on an inline fragment defers the fragment's fields as one payload at the parent's path`` () = let expectedDirect = NameValueLookup.ofList [ @@ -1865,12 +1867,12 @@ let ``Defer directive on an inline fragment defers the fragment's fields as one sub.Received |> Seq.toList |> equals [ - DeferredPending ([ "testData" ], ValueSome "rest", false, 0) - DeferredResult (NameValueLookup.ofList [ "a", upcast "Apple"; "b", upcast "Banana" ], [ "testData" ]) - DeferredCompleted [ "testData" ] + DeferredFragmentPending ([ "testData" ], ValueSome "rest", 0) + DeferredFragmentResult (ValueSome (upcast NameValueLookup.ofList [ "a", upcast "Apple"; "b", upcast "Banana" ]), [], [ "testData" ], 0) + DeferredFragmentCompleted ([ "testData" ], 0) ] -[] +[] let ``Defer directive on a fragment spread defers the fragment's fields as one payload`` () = let expectedDirect = NameValueLookup.ofList [ @@ -1897,11 +1899,11 @@ let ``Defer directive on a fragment spread defers the fragment's fields as one p sub.Received |> Seq.toList |> equals [ - DeferredResult (NameValueLookup.ofList [ "a", upcast "Apple"; "b", upcast "Banana" ], [ "testData" ]) - DeferredCompleted [ "testData" ] + DeferredFragmentResult (ValueSome (upcast NameValueLookup.ofList [ "a", upcast "Apple"; "b", upcast "Banana" ]), [], [ "testData" ], 0) + DeferredFragmentCompleted ([ "testData" ], 0) ] -[] +[] let ``The same fragment deferred twice at the same path is delivered once`` () = let query = parse """query { testData { @@ -1920,8 +1922,161 @@ let ``The same fragment deferred twice at the same path is delivered once`` () = sub.Received |> Seq.toList |> equals [ - DeferredResult (NameValueLookup.ofList [ "a", upcast "Apple" ], [ "testData" ]) - DeferredCompleted [ "testData" ] + DeferredFragmentResult (ValueSome (upcast NameValueLookup.ofList [ "a", upcast "Apple" ]), [], [ "testData" ], 0) + DeferredFragmentCompleted ([ "testData" ], 0) + ] + +[] +let ``Two labeled fragments deferred at the same path are delivered as separate payloads`` () = + let query = parse """{ + testData { + id + ... @defer(label: "first") { + a + } + ... @defer(label: "second") { + b + } + } + }""" + let result = executor.AsyncExecute(query, getMockInputContext) |> sync + ensureDeferred result <| fun _ errors deferred -> + empty errors + use sub = Observer.create deferred + sub.WaitCompleted() + sub.Received + |> Seq.toList + |> equals [ + // Both fragments are announced before either delivers + DeferredFragmentPending ([ "testData" ], ValueSome "first", 0) + DeferredFragmentPending ([ "testData" ], ValueSome "second", 1) + DeferredFragmentResult (ValueSome (upcast NameValueLookup.ofList [ "a", upcast "Apple" ]), [], [ "testData" ], 0) + DeferredFragmentCompleted ([ "testData" ], 0) + DeferredFragmentResult (ValueSome (upcast NameValueLookup.ofList [ "b", upcast "Banana" ]), [], [ "testData" ], 1) + DeferredFragmentCompleted ([ "testData" ], 1) + ] + +[] +let ``A field selected both directly and in a deferred fragment is executed with the object`` () = + let expectedDirect = + NameValueLookup.ofList [ + "testData", upcast NameValueLookup.ofList [ + "a", upcast "Apple" + ] + ] + let query = parse """{ + testData { + a + ... @defer { + a + b + } + } + }""" + let result = executor.AsyncExecute(query, getMockInputContext) |> sync + ensureDeferred result <| fun data errors deferred -> + empty errors + data |> equals (upcast expectedDirect) + use sub = Observer.create deferred + sub.WaitCompleted() + sub.Received + |> Seq.toList + |> equals [ + // The fragment delivers only the field the object did not + DeferredFragmentResult (ValueSome (upcast NameValueLookup.ofList [ "b", upcast "Banana" ]), [], [ "testData" ], 0) + DeferredFragmentCompleted ([ "testData" ], 0) + ] + +[] +let ``A fragment deferred at the operation root delivers root fields`` () = + let query = parse """{ + ... @defer { + nullableTestData { + id + } + } + }""" + let result = executor.AsyncExecute(query, getMockInputContext) |> sync + ensureDeferred result <| fun data errors deferred -> + empty errors + data |> equals (upcast NameValueLookup.ofList []) + use sub = Observer.create deferred + sub.WaitCompleted() + sub.Received + |> Seq.toList + |> equals [ + DeferredFragmentResult ( + ValueSome (upcast NameValueLookup.ofList [ "nullableTestData", upcast NameValueLookup.ofList [ "id", upcast "1" ] ]), + [], + [], + 0 + ) + DeferredFragmentCompleted ([], 0) + ] + +[] +let ``A deferred fragment on an abstract type delivers the fields of the matching type`` () = + let expectedDirect = + NameValueLookup.ofList [ + "testData", upcast NameValueLookup.ofList [ + "iface", upcast NameValueLookup.ofList [ + "id", upcast "1000" + ] + ] + ] + let query = parse """{ + testData { + iface { + id + ... on C @defer(label: "c") { + value + } + } + } + }""" + let result = executor.AsyncExecute(query, getMockInputContext) |> sync + ensureDeferred result <| fun data errors deferred -> + empty errors + data |> equals (upcast expectedDirect) + use sub = Observer.create deferred + sub.WaitCompleted() + sub.Received + |> Seq.toList + |> equals [ + DeferredFragmentPending ([ "testData"; "iface" ], ValueSome "c", 0) + DeferredFragmentResult (ValueSome (upcast NameValueLookup.ofList [ "value", upcast "C" ]), [], [ "testData"; "iface" ], 0) + DeferredFragmentCompleted ([ "testData"; "iface" ], 0) + ] + +[] +let ``An error propagating up to a deferred fragment completes it with the errors and no data`` () = + let expectedDirect = + NameValueLookup.ofList [ + "testData", upcast NameValueLookup.ofList [ + "id", upcast "1" + ] + ] + let expectedError = GQLProblemDetails.CreateWithKind ("Non-null field error!", Execution, [ box "testData"; "nonNullError" ]) + let query = parse """{ + testData { + id + ... @defer { + nonNullError + } + } + }""" + let result = executor.AsyncExecute(query, getMockInputContext) |> sync + ensureDeferred result <| fun data errors deferred -> + empty errors + data |> equals (upcast expectedDirect) + use sub = Observer.create deferred + sub.WaitCompleted() + sub.Received + |> Seq.toList + |> equals [ + // The object was already delivered with its own fields, so the fragment has nothing to null: it fails as a whole + DeferredFragmentResult (ValueNone, [ expectedError ], [ "testData" ], 0) + DeferredFragmentCompleted ([ "testData" ], 0) ] [] diff --git a/tests/FSharp.Data.GraphQL.Tests/Helpers.fs b/tests/FSharp.Data.GraphQL.Tests/Helpers.fs index 47635a24..1812a397 100644 --- a/tests/FSharp.Data.GraphQL.Tests/Helpers.fs +++ b/tests/FSharp.Data.GraphQL.Tests/Helpers.fs @@ -181,7 +181,11 @@ module Observer = /// filter this announcement out. /// let withoutPending (events : GQLDeferredResponseContent seq) = - events |> Seq.filter (function DeferredPending _ -> false | _ -> true) + events + |> Seq.filter (function + | DeferredPending _ + | DeferredFragmentPending _ -> false + | _ -> true) /// /// Drops every and @@ -198,7 +202,10 @@ let withoutPending (events : GQLDeferredResponseContent seq) = let withoutCompleted (events : GQLDeferredResponseContent seq) = events |> withoutPending - |> Seq.filter (function DeferredCompleted _ -> false | _ -> true) + |> Seq.filter (function + | DeferredCompleted _ + | DeferredFragmentCompleted _ -> false + | _ -> true) open System.Runtime.CompilerServices From 81c8fde0c98bb3e815c8ce3dc8c8e3d90d9bebd1 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Mon, 21 Sep 2026 02:21:30 +0200 Subject: [PATCH 3/4] 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 --- RELEASE_NOTES.md | 2 +- ...ec-interface-possible-types-keynotfound.md | 95 +++++++++ docs/covariance-validation-test-spec.md | 127 +++++++++++ ...p-data-graphql-empty-input-object-types.md | 135 ++++++++++++ ...data-graphql-parser-fragment-whitespace.md | 127 +++++++++++ docs/type-coercion-guide.md | 199 ++++++++++++++++++ docs/type-system.md | 2 +- .../GraphQLRequestHandler.fs | 20 +- .../MiddlewareDefinitions.fs | 2 +- src/FSharp.Data.GraphQL.Server/Execution.fs | 47 ++++- src/FSharp.Data.GraphQL.Server/Linq.fs | 2 +- src/FSharp.Data.GraphQL.Server/Planning.fs | 114 +++++++--- src/FSharp.Data.GraphQL.Shared/TypeSystem.fs | 20 +- .../DeferredTests.fs | 135 ++++++++++++ .../ExecutionTests.fs | 25 ++- 15 files changed, 995 insertions(+), 57 deletions(-) create mode 100644 docs/bug-spec-interface-possible-types-keynotfound.md create mode 100644 docs/covariance-validation-test-spec.md create mode 100644 docs/fsharp-data-graphql-empty-input-object-types.md create mode 100644 docs/fsharp-data-graphql-parser-fragment-whitespace.md create mode 100644 docs/type-coercion-guide.md diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 6cbddf32..29acc543 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -292,7 +292,7 @@ * **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 * **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 * **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 -* **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 +* **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 * **Breaking Change** `BufferedStreamOptions.Interval` and `BufferedStreamOptions.PreferredBatchSize` are now `int voption` * **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 * **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` diff --git a/docs/bug-spec-interface-possible-types-keynotfound.md b/docs/bug-spec-interface-possible-types-keynotfound.md new file mode 100644 index 00000000..2ee95d9f --- /dev/null +++ b/docs/bug-spec-interface-possible-types-keynotfound.md @@ -0,0 +1,95 @@ +# Bug Spec: KeyNotFoundException in Interface Possible Types Resolution + +## Summary +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. + +## Observed Runtime Evidence +- Exception type: `System.Collections.Generic.KeyNotFoundException` +- Message: `The given key was not present in the dictionary.` +- Top failing library frame: `FSharp.Data.GraphQL.Schema<'Root>.getPossibleTypes` +- Failing code path (`src/FSharp.Data.GraphQL.Server/Schema.fs`): + - `| Interface i -> Map.find i.Name (implementations.Force()) |> Array.ofList` + +## Exact Failure Location +File: `src/FSharp.Data.GraphQL.Server/Schema.fs` +- `getImplementations`: builds `Map` from `objdef.Implements` +- `getPossibleTypes`: uses `Map.find` for interfaces +- `introspectType` for `Interface` calls `getPossibleTypes` and crashes before graceful validation/error reporting + +## Root Cause +`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. +In that case, lookup throws immediately, producing infrastructure exception instead of structured GraphQL/type validation feedback. + +## Why This Is Problematic +1. Hard crash during schema startup/introspection. +2. No actionable validation message identifying which interface is orphaned. +3. Behavior differs from expected robust validation (should return deterministic `ValidationError` or safe empty set depending on policy). + +## Reproduction (Generic, Domain-Agnostic) +1. Define interface `IParentInfo` with at least one field. +2. Register the interface in schema type map. +3. Ensure no object type in type map includes this interface in `interfaces = [ ... ]`. +4. Trigger schema introspection or schema initialization path that builds introspection metadata. +5. Observe `KeyNotFoundException` at `Map.find i.Name (implementations.Force())`. + +## Expected Behavior +One of the following (explicitly chosen policy): +- **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. +- **Alternative**: immediately return structured validation error: `Interface has no implementing object types.` + +No raw `KeyNotFoundException` should escape from schema construction/introspection. + +## Proposed Fix +### Safe Lookup Change +Replace unsafe lookup in `getPossibleTypes` with safe lookup: +- from: `Map.find i.Name (implementations.Force()) |> Array.ofList` +- to: `implementations.Force() |> Map.tryFind i.Name |> Option.defaultValue [] |> Array.ofList` + +### Validation Enhancement +Add explicit validation for orphaned interfaces in type-map validation layer: +- detect interfaces with zero implementing object types +- return deterministic `ValidationError` with interface name + +This keeps runtime stable and preserves strict schema diagnostics. + +## Test Specification +Create dedicated tests in `tests/FSharp.Data.GraphQL.Tests` (new file recommended: `InterfacePossibleTypesValidationTests.fs`). + +### Test 1: Regression Repro (pre-fix behavior) +- Build schema with one interface and no implementors. +- Assert old code throws `KeyNotFoundException` (documented regression test, can be skipped/removed after fix depending policy). + +### Test 2: Safe Introspection (post-fix) +- Same schema as Test 1. +- Assert no `KeyNotFoundException` is thrown during introspection/schema init. + +### Test 3: Validation Error for Orphan Interface +- Same schema as Test 1. +- Run type-map validation entry point. +- Assert deterministic error contains interface name and orphaned-implementation message. + +### Test 4: Normal Interface Implementations +- Interface with one object implementation. +- Assert introspection returns that object in possible types. + +### Test 5: Multiple Implementations +- Interface with two object implementations. +- Assert introspection returns both possible types. + +### Test 6: Mixed Schema Stability +- Include additional unrelated interfaces/unions/objects. +- Assert no crashes and correct possible type resolution across all abstract types. + +## Acceptance Criteria +1. No `KeyNotFoundException` from `getPossibleTypes` for missing interface key. +2. Orphan interface case yields controlled behavior (empty set + validation error, or direct structured validation error per chosen policy). +3. Existing interface/union introspection behavior remains unchanged for valid schemas. +4. Tests cover single/multiple/no implementations and pass consistently. + +## Backward Compatibility Notes +- Safe lookup is non-breaking for valid schemas. +- Invalid schemas move from low-level exception to explicit, actionable diagnostics. + +## Implementation Notes +- Keep error text stable for test assertions. +- Prefer adding tests before/with fix to prevent future regressions. diff --git a/docs/covariance-validation-test-spec.md b/docs/covariance-validation-test-spec.md new file mode 100644 index 00000000..090e5bf6 --- /dev/null +++ b/docs/covariance-validation-test-spec.md @@ -0,0 +1,127 @@ +# Covariance Validation Test Specification + +## Purpose +Define exhaustive tests for GraphQL interface implementation covariance and nullability compatibility in FSharp.Data.GraphQL type validation. + +## Problem Statement (Current Bug) +During schema initialization, executor calls `Validation.Types.validateTypeMap schema.TypeMap` and throws `GQLMessageException` when validation returns errors. + +Observed runtime error pattern: +- `'.' field signature does not match it's definition in interface ` + +Exact failure location in library: +- `src/FSharp.Data.GraphQL.Server/Executor.fs` (schema startup validation) +- `src/FSharp.Data.GraphQL.Shared/Validation.fs`, function `validateImplements` + +Current implementation in `validateImplements` uses strict equality: +- `Some objf when objf = f -> acc` +- otherwise reports signature mismatch + +This equality-based check is stricter than GraphQL spec subtyping rules for interface field return types and nullability covariance. + +## GraphQL Compatibility Rules to Validate +For object type `O implements I`, each interface field `f` must satisfy: +1. Field exists on object with same name. +2. Arguments are compatible (same required args; extra args on object must be optional). +3. Return type on object is equal to or a valid subtype of interface return type. +4. Non-null covariance: `T!` is subtype of `T` (allowed). +5. List/null wrappers must be compared structurally by spec subtyping rules. +6. If interface return type is interface/union, object return type may be a concrete implementing/member type (covariance). + +## Nullable / StructNullable Coverage +In this codebase: +- `Nullable X` and `StructNullable X` both produce nullable GraphQL wrappers. +- Non-wrapper `X` is non-null GraphQL type. + +Tests must cover both wrappers equivalently for compatibility decisions: +- `Nullable InterfaceType` vs concrete non-null implementor type. +- `StructNullable InterfaceType` vs concrete non-null implementor type. +- `Nullable T` vs `Nullable T` exact match. +- `StructNullable T` vs `StructNullable T` exact match. +- Negative cases where nested wrappers are incompatible (e.g., list item nullability mismatch). + +## Generic Test Model (No domain-specific names) +Use neutral names only: + +Interfaces: +- `IParentView` +- `IChildView` + +GraphQL interfaces: +- `IChildInfo` +- `IParentInfo` with field `child: IChildInfo` + +Concrete object types: +- `ChildAInfo implements IChildInfo` +- `ChildBInfo implements IChildInfo` +- `ParentAInfo implements IParentInfo` with `child: ChildAInfo` +- `ParentBInfo implements IParentInfo` with `child: ChildBInfo` + +This model must be reused for all covariance and nullability test cases. + +## Test Matrix (Must Cover All Cases) +### A. Positive covariance cases (must pass) +1. Interface field type `IChildInfo`, object field type `ChildAInfo` (implements `IChildInfo`). +2. Same as A1 for second implementation (`ChildBInfo`). +3. Interface field `Nullable IChildInfo`, object field non-null `ChildAInfo`. +4. Interface field `StructNullable IChildInfo`, object field non-null `ChildAInfo`. +5. Interface field non-null `IChildInfo`, object field same non-null `IChildInfo` (exact). +6. Interface field list `List`, object field list `List` where library supports list covariance by member subtype. +7. Deep wrappers: interface `Nullable(List(Nullable(IChildInfo)))`, object `List(ChildAInfo)` where valid by non-null covariance. + +### B. Negative covariance cases (must fail) +1. Interface field `IChildInfo`, object field unrelated object type `OtherInfo` (not implementing). +2. Interface field non-null `IChildInfo`, object field nullable `Nullable IChildInfo` (wider, invalid). +3. Interface field list `List`, object field scalar `ChildAInfo`. +4. Interface field `List`, object field `List` (invalid nullability widening). +5. Interface field arguments mismatch (missing required arg, type mismatch, extra required arg). + +### C. Nullable vs StructNullable parity (must pass/fail identically) +For each scenario A3, A4, B2, B4 create paired tests: +- one with `Nullable` +- one with `StructNullable` +Expected result must be identical for semantic-equivalent wrappers. + +### D. Existing strict-equality regression (must reproduce old bug) +Create a test where only difference is: +- interface field type = interface def +- object field type = implementing concrete object def + +Expected by spec: Success. +Current behavior before fix: ValidationError with signature mismatch message. +This test documents the bug and prevents reintroduction. + +## Test File Placement +- Extend `tests/FSharp.Data.GraphQL.Tests/TypeValidationTests.fs` for focused unit cases, or +- create `tests/FSharp.Data.GraphQL.Tests/TypeValidationCovarianceTests.fs` if separation is preferred. + +## Assertion Style +- Use `validateImplements` for unit-level behavior. +- Use `validateTypeMap` for end-to-end schema-level validation with multiple types registered. +- Verify exact error strings for negative tests where stable, otherwise verify error contains object+field+interface identifiers. + +## Proposed Fix in Validation Engine +Replace strict `objf = f` signature equality with structural GraphQL compatibility check: +1. Compare field names and argument compatibility by spec rules. +2. Compare return types via `isOutputSubtype(objectType, interfaceType)`. +3. Implement recursive wrapper-aware subtype check: + - `NonNull(A)` subtype of `A` + - `List(A)` subtype of `List(B)` iff `A` subtype of `B` + - object subtype of interface if object implements interface + - object subtype of union if object is a union member + - named scalars/enums require exact type identity + +Pseudo-contract: +- `isFieldImplementationCompatible(objectField, interfaceField) -> bool` +- used by `validateImplements` instead of direct equality. + +## Acceptance Criteria +1. All positive covariance tests pass. +2. All negative compatibility tests fail with deterministic errors. +3. Nullable/StructNullable parity tests pass. +4. No regressions in existing `TypeValidationTests.fs`. +5. Schema initialization no longer throws for valid covariance implementations. + +## Notes for Reviewers +- This is a spec-driven validation correction, not a domain-model workaround. +- Goal is GraphQL spec compliance at type-system validation layer. diff --git a/docs/fsharp-data-graphql-empty-input-object-types.md b/docs/fsharp-data-graphql-empty-input-object-types.md new file mode 100644 index 00000000..a7fb48ca --- /dev/null +++ b/docs/fsharp-data-graphql-empty-input-object-types.md @@ -0,0 +1,135 @@ +# FSharp.Data.GraphQL exposes `File` and `ObjectListFilter` as `INPUT_OBJECT` with zero fields (spec-illegal) + +## Summary + +The server libraries `FSharp.Data.GraphQL.Server.AspNetCore` and `FSharp.Data.GraphQL.Server.Middleware` publish two input types via introspection that violate the GraphQL specification: + +- `input File` — from `FSharp.Data.GraphQL.Server.AspNetCore` (multipart file upload input). +- `input ObjectListFilter` — from `FSharp.Data.GraphQL.Server.Middleware` (generic list-filter middleware). + +Both are surfaced as **`INPUT_OBJECT`** types with an **empty `inputFields` array**. Per [GraphQL spec §3.10 Input Objects](https://spec.graphql.org/October2021/#sec-Input-Objects): + +> An Input Object type must define one or more input fields. + +As a result, **any standards-compliant client that validates the introspected schema rejects the entire schema**, even though the server can execute queries against it. This blocks GraphQL Inspector, GraphQL Code Generator, Apollo tooling, Relay Compiler, `graphql-js`-based validation in general. + +## Environment + +| Item | Value | +|------|-------| +| Library | `FSharp.Data.GraphQL.Server.AspNetCore`, `FSharp.Data.GraphQL.Server.Middleware`, `FSharp.Data.GraphQL.Shared` | +| Version observed | `4.0.0-ci-31608744053` (also present in earlier 4.x CI builds) | +| Host | ASP.NET Core on .NET 10 | +| Endpoint | `POST /GraphQL` | +| Failing client | `@graphql-inspector/cli` (any version using `graphql-js` ≥ 15) | + +## Reproduction + +1. Register a server with either the multipart upload middleware or the `ObjectListFilter` middleware (both are default in `FSharp.Data.GraphQL.Server.AspNetCore` / `.Middleware`). +2. Run: + + ```powershell + graphql-inspector introspect https://localhost:5003/GraphQL --write schema.graphql + ``` + +3. Observe: + + ``` + Error: Input Object type File must define one or more fields. + Input Object type ObjectListFilter must define one or more fields. + at assertValidSchema (…/graphql-js/type/validate.js:91:11) + at assertValidExecutionArguments (…/graphql-js/execution/execute.js:419:35) + at introspectionFromSchema (…/graphql-js/utilities/introspectionFromSchema.js:93:43) + ``` + +The wire-level introspection response contains, among other types: + +```json +{ "kind": "INPUT_OBJECT", "name": "File", "inputFields": [] } +{ "kind": "INPUT_OBJECT", "name": "ObjectListFilter", "inputFields": [] } +``` + +`graphql-js` runs `assertValidSchema` on the reconstructed schema and refuses it because `inputFields` is empty for both types. + +## Expected behavior + +For every `INPUT_OBJECT` type published via introspection, `inputFields` must be non-empty. Equivalently, the type must not be modeled as an input object if it has no static fields — it should be modeled as a **custom scalar** or as an input object with explicit fields. + +## Impact + +- Any client that calls `buildClientSchema` / `assertValidSchema` from `graphql-js` throws — this is the reference implementation used by essentially every JS/TS GraphQL tool. +- Confirmed broken: + - `graphql-inspector introspect | diff | validate | similar` against a live URL. + - `@graphql-codegen/cli` with a URL schema source. + - Apollo Rover / Apollo CLI introspection. + - Relay Compiler when fed the introspected schema. +- The server itself continues to execute queries successfully because it doesn't apply strict spec validation to its own schema; the problem only surfaces in downstream tooling. +- Playground / Altair / GraphiQL / Nitro happen to keep working because they render the raw introspection JSON without going through `assertValidSchema`. + +## Root cause (two separate cases) + +### 1. `File` + +Source: `FSharp.Data.GraphQL.Server.AspNetCore` — the multipart file upload input. + +Semantically this is a **scalar** value: an out-of-band multipart part referenced from a variables JSON blob. The de-facto community standard is [`graphql-multipart-request-spec`](https://github.com/jaydenseric/graphql-multipart-request-spec), which models uploads as a **custom `Upload` scalar**. Every JS client (Apollo Upload Client, `graphql-request`, urql, Relay upload adapters) expects `scalar Upload`, not `input File { … }`. + +Modeling it as an empty `INPUT_OBJECT` is both spec-illegal (must have ≥ 1 field) and interoperability-breaking (no client understands `input File`). + +### 2. `ObjectListFilter` + +Source: `FSharp.Data.GraphQL.Server.Middleware` — the generic list-filter middleware. + +The runtime accepts a recursive tree of operators (`_and`, `_or`, `_not`, `_eq`, `_neq`, `_in`, `_nin`, `_gt`, `_gte`, `_lt`, `_lte`, `_starts_with`, `_ends_with`, `_contains`, etc.) whose *field* names depend on the element type. Because the shape is dynamic, the middleware currently registers the type without declaring any static input fields, leaving `inputFields: []` in introspection. + +## Suggested fixes + +### For `File` + +Rename to **`Upload`** and publish it as a **custom scalar** rather than an input object: + +```graphql +scalar Upload +``` + +- Aligns with the multipart request spec used by the wider ecosystem. +- Fixes the spec violation (scalars have no field requirement). +- Enables all existing JS/TS upload clients to work without server-side changes. +- Server-side: the resolver already receives an opaque value from the multipart form; that value can be surfaced as a scalar just as easily. + +Provide a compatibility shim / opt-in flag for the legacy `File` name if backwards compatibility matters. + +### For `ObjectListFilter` + +Two acceptable options: + +**Option A (preferred): emit explicit operator fields per element type.** +For each list field the middleware attaches to, generate a concrete `ListFilter` input with the fully-typed `_and: [ListFilter!]`, `_or: […]`, `_eq: `, `_in: [!]`, etc. This is what Hasura, PostGraphile, and Marten's own OData-style filters do, and it plays perfectly with codegen. + +**Option B (fallback): expose the filter as a `scalar ObjectListFilter`.** +Carries a JSON value in transit. Simplest patch, loses typed autocompletion in tooling, but restores spec compliance and unblocks every downstream client. + +Whichever route is chosen, do **not** keep the current `INPUT_OBJECT` with an empty `inputFields` list. + +### Regression test + +Add a schema-validation test that reconstructs the introspection output using `graphql-js` (or an F# port) and calls `assertValidSchema`. Minimal repro: + +```js +import { buildClientSchema, assertValidSchema } from 'graphql'; + +const res = await fetch(url, { method: 'POST', headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ query: getIntrospectionQuery() }) }); +const json = await res.json(); +assertValidSchema(buildClientSchema(json.data)); // must not throw +``` + +## References + +- GraphQL spec, [§3.10 Input Objects](https://spec.graphql.org/October2021/#sec-Input-Objects) — input object types must define ≥ 1 field. +- [`graphql-multipart-request-spec`](https://github.com/jaydenseric/graphql-multipart-request-spec) — the de-facto multipart upload spec, based on a custom `Upload` scalar. +- graphql-js [`assertValidSchema`](https://github.com/graphql/graphql-js/blob/main/src/type/validate.ts) — the validator every JS client runs. + +## Related upstream report + +See also the sibling report [`fsharp-data-graphql-parser-fragment-whitespace.md`](./fsharp-data-graphql-parser-fragment-whitespace.md), which covers the parser bug rejecting `}fragment` in minified introspection queries. That bug is fixed in `4.0.0-ci-31608744053`; the two `INPUT_OBJECT` issues documented here are still present in the same build. diff --git a/docs/fsharp-data-graphql-parser-fragment-whitespace.md b/docs/fsharp-data-graphql-parser-fragment-whitespace.md new file mode 100644 index 00000000..6362cc24 --- /dev/null +++ b/docs/fsharp-data-graphql-parser-fragment-whitespace.md @@ -0,0 +1,127 @@ +# FSharp.Data.GraphQL parser rejects `}fragment` without whitespace + +## Summary + +The query parser in `FSharp.Data.GraphQL` fails to parse a **valid, spec-compliant GraphQL document** when a selection set's closing brace `}` is immediately followed by the `fragment` keyword with no whitespace between them (as is standard in minified queries emitted by virtually every GraphQL client). + +This breaks live introspection from all standards-compliant tooling (GraphQL Inspector, GraphQL Code Generator, Apollo CLI, urql, Relay, `graphql-request`, etc.), because they all send a minified introspection query in which the trailing `}}}}` of the `__schema` selection set is directly adjacent to `fragment FullType on __Type`. + +## Environment + +| Item | Value | +|------|-------| +| Library | `FSharp.Data.GraphQL.Server` / `FSharp.Data.GraphQL.Server.AspNetCore` | +| Branch observed | `dev` | +| Host | ASP.NET Core on .NET 10 | +| Endpoint | `POST /GraphQL` (configured in `GraphQLStartup.cs`) | +| Client | `@graphql-inspector/cli` (also reproducible with plain `curl` / `Invoke-WebRequest`) | + +## Reproduction + +### Minimal repro payload + +```http +POST /GraphQL +Content-Type: application/json + +{"query":"{__typename}fragment X on Query{__typename}"} +``` + +### Full introspection repro (as sent by `graphql-inspector introspect`) + +```json +{"query":"query IntrospectionQuery{__schema{queryType{name kind}mutationType{name kind}subscriptionType{name kind}types{...FullType}directives{name description locations args{...InputValue}}}}fragment FullType on __Type{kind name description fields(includeDeprecated:true){name description args{...InputValue}type{...TypeRef}isDeprecated deprecationReason}inputFields{...InputValue}interfaces{...TypeRef}enumValues(includeDeprecated:true){name description isDeprecated deprecationReason}possibleTypes{...TypeRef}}fragment InputValue on __InputValue{name description type{...TypeRef}defaultValue}fragment TypeRef on __Type{kind name ofType{name kind ofType{name kind ofType{name kind ofType{name kind ofType{name kind ofType{name kind ofType{name kind ofType{name kind ofType{name kind}}}}}}}}}}"} +``` + +## Observed behavior + +The server returns **HTTP 400** with: + +``` +{ + "type": "https://tools.ietf.org/html/rfc9110#section-15.5.1", + "title": "Cannot parse GraphQL query", + "status": 400, + "detail": "Error in Ln: 1 Col: 183 + iption locations args{...InputValue}}}}fragment FullType on __Type{kind name de + ^ + Unknown Error(s)", + "instance": "/GraphQL" +} +``` + +Column 183 is the exact offset of the `f` in `fragment` immediately after the `}}}}` that closes the `__schema` selection set. + +## Expected behavior + +The document is **valid** per the GraphQL specification. `}` and `fragment` are separate lexical tokens; the GraphQL grammar allows arbitrary (including zero) whitespace/ignored tokens between them. The server should parse the query and return the introspection result (verified — the same query with a single space inserted between `}` and `fragment` returns HTTP 200 with the full ~400 KB introspection JSON). + +## Impact + +- All live introspection tooling is broken against this server: + - `graphql-inspector introspect ` + - `graphql-inspector diff ` / `validate ` + - `graphql-codegen` with a URL schema source + - Apollo Rover / Apollo CLI introspection + - Any client using `getIntrospectionQuery()` from the `graphql-js` reference implementation (which produces a **minified** string by default in recent versions) +- Playground / Altair / GraphiQL / Nitro still work because their built-in introspection query happens to be pretty-printed, so `}` and `fragment` are separated by whitespace. +- Any user-authored query that omits whitespace between a selection set and a following fragment definition (perfectly legal) will also fail. + +## Root-cause hypothesis + +The parser (FParsec-based, in `FSharp.Data.GraphQL.Shared` / `Parser`) most likely requires at least one whitespace/ignored-token between a `Definition` and the next `Definition` in a `Document`. In the grammar: + +``` +Document ::= Definition+ +Definition ::= OperationDefinition | FragmentDefinition | TypeSystemDefinition +FragmentDefinition ::= "fragment" FragmentName TypeCondition Directives? SelectionSet +``` + +The GraphQL spec defines the token stream so that any two adjacent tokens are separable without whitespace **unless both are name-like tokens** (identifiers, keywords, numbers). `}` is a punctuator, `fragment` is a keyword — they are distinct token classes and require no separator. + +Likely offending patterns in the parser: + +1. The top-level `document` combinator uses something like + `many1 (definition .>> spaces1)` or `sepBy1 definition spaces1` + instead of `many1 (definition .>> spaces)`. +2. Or `fragmentDefinition` is written as `pstring "fragment" >>. spaces1 >>. …` but preceded by a rule that demands whitespace *before* the keyword. +3. Or the top-level parser is `many1 (spaces >>. definition)` where the previous definition parser doesn't consume the closing `}` cleanly, leaving the parser expecting whitespace-then-definition and refusing to accept a punctuator boundary. + +## Suggested fix + +1. In the `document`/`definitions` combinator, ensure inter-definition separators are `spaces` (zero-or-more) rather than `spaces1` (one-or-more). GraphQL's `Ignored` production is `*`, not `+`. +2. Verify the same for all places where two adjacent grammar productions can meet at a punctuator/keyword boundary — notably: + - `SelectionSet` followed by `FragmentDefinition` at the document level (the bug reported here). + - `SelectionSet` followed by another `OperationDefinition` (`}query …`, `}mutation …`). + - `Arguments`/`Directives` transitions at the end of a `Field`. +3. Add regression tests using the exact `graphql-js` output of `getIntrospectionQuery({ descriptions: true })` in its minified form, plus these two smoke tests: + + ```graphql + {__typename}fragment X on Query{__typename} + ``` + + ```graphql + query A{__typename}query B{__typename} + ``` + +Both should parse successfully. + +## References + +- GraphQL spec, [§2.1 Source Text / Ignored Tokens](https://spec.graphql.org/October2021/#sec-Source-Text.Ignored-Tokens): *"Ignored tokens are allowed anywhere between other tokens."* (i.e., zero or more, not one or more.) +- graphql-js [`getIntrospectionQuery`](https://github.com/graphql/graphql-js/blob/main/src/utilities/getIntrospectionQuery.ts) — reference implementation used by essentially every JS/TS client. + +## Workaround for consumers (until fixed) + +- Do not perform live introspection against the server. Instead, run a formatted (whitespace-separated) introspection query manually and commit the resulting SDL/JSON as a static schema artifact; point tooling at the file rather than the URL. + +Example PowerShell one-liner that works today: + +```powershell +[System.Net.ServicePointManager]::ServerCertificateValidationCallback = { $true } +$q = 'query IntrospectionQuery { __schema { queryType { name } mutationType { name } subscriptionType { name } types { ...FullType } directives { name description locations args { ...InputValue } } } } fragment FullType on __Type { kind name description fields(includeDeprecated: true) { name description args { ...InputValue } type { ...TypeRef } isDeprecated deprecationReason } inputFields { ...InputValue } interfaces { ...TypeRef } enumValues(includeDeprecated: true) { name description isDeprecated deprecationReason } possibleTypes { ...TypeRef } } fragment InputValue on __InputValue { name description type { ...TypeRef } defaultValue } fragment TypeRef on __Type { kind name ofType { kind name ofType { kind name ofType { kind name ofType { kind name ofType { kind name ofType { kind name ofType { kind name } } } } } } } }' +$body = @{ query = $q } | ConvertTo-Json -Compress +Invoke-WebRequest -Uri https://localhost:5003/GraphQL -Method Post -ContentType 'application/json' -Body $body -UseBasicParsing | + Select-Object -ExpandProperty Content | + Set-Content -Path introspection.json -Encoding UTF8 +``` diff --git a/docs/type-coercion-guide.md b/docs/type-coercion-guide.md new file mode 100644 index 00000000..363572f8 --- /dev/null +++ b/docs/type-coercion-guide.md @@ -0,0 +1,199 @@ +# Type Coercion in ObjectListFilter + +## Overview + +The `ApplyWithCoercion` extension method automatically converts JSON primitive values (strings, numbers, booleans) to rich CLR types before building LINQ expressions. This prevents type mismatch errors when filtering on properties with types like `Guid`, `DateTime`, `DateOnly`, or F# discriminated unions. + +## Basic Usage + +### Without Coercion (Old Behavior - Can Fail) +```fsharp +type User = { Id: Guid; Name: string } + +// This would fail at runtime - comparing string "550e8400..." with Guid property +let filter = "id" === "550e8400-e29b-41d4-a716-446655440000" +let users = query.Apply filter // ❌ Type mismatch error +``` + +### With Coercion (New Feature) +```fsharp +type User = { Id: Guid; Name: string } + +// Automatically converts string to Guid before comparison +let filter = "id" === "550e8400-e29b-41d4-a716-446655440000" +let users = query.ApplyWithCoercion filter // ✅ Works! +``` + +## Supported Types (Built-in) + +- **Guid** - from string via `Guid.Parse` +- **DateTime** - from string via `DateTime.Parse` +- **DateTimeOffset** - from string via `DateTimeOffset.Parse` +- **DateOnly** - from string via `DateOnly.Parse` (.NET 6+) +- **F# Single-Case DUs** - e.g., `type UserId = UserId of Guid` +- **F# Multi-Case Fieldless DUs** - e.g., `type Status = Active | Inactive | Pending` + +## F# Discriminated Union Examples + +### Single-Case DU (Wrapper Types) +```fsharp +type UserId = UserId of Guid +type User = { Id: UserId; Name: string } + +// String is converted to Guid, then wrapped in UserId +let filter = "id" === "550e8400-e29b-41d4-a716-446655440000" +let users = query.ApplyWithCoercion filter // ✅ Works! +``` + +### Multi-Case Fieldless DU (Enums) +```fsharp +type Status = Active | Inactive | Pending +type User = { Id: Guid; Status: Status } + +// String "active" is case-insensitively matched to Status.Active case +let filter = "status" === "active" +let users = query.ApplyWithCoercion filter // ✅ Works! + +// In operator with multiple values +let filter = "status" =~= ["active"; "pending"] // Converted to: status == Active OR status == Pending +let users = query.ApplyWithCoercion filter // ✅ Works! +``` + +## Custom Type Coercers + +For types not supported out-of-the-box (e.g., NodaTime, custom value objects), you can provide custom coercion logic: + +### Example: NodaTime.Instant Support + +```fsharp +open NodaTime +open NodaTime.Text + +// Define a custom coercer +let nodaTimeCoercer : FilterValueCoercer = fun targetType value -> + if targetType = typeof then + match value with + | :? string as s -> + let parsed = InstantPattern.ExtendedIso.Parse s + if parsed.Success then + ValueSome (box parsed.Value) + else + ValueNone + | _ -> ValueNone + else + ValueNone + +// Use it with options +let options = ObjectListFilterLinqOptions([nodaTimeCoercer]) +let filter = "eventTime" >>> "2024-01-01T00:00:00Z" +let events = query.ApplyWithCoercion(filter, options) // ✅ Works! +``` + +### Example: Custom Value Object + +```fsharp +type EmailAddress = private EmailAddress of string + with static member TryCreate(s: string) = + if s.Contains("@") then Some (EmailAddress s) else None + +let emailCoercer : FilterValueCoercer = fun targetType value -> + if targetType = typeof then + match value with + | :? string as s -> + EmailAddress.TryCreate s + |> Option.map box + |> ValueOption.ofOption + | _ -> ValueNone + else + ValueNone + +let options = ObjectListFilterLinqOptions([emailCoercer]) +let filter = "email" === "user@example.com" +let users = query.ApplyWithCoercion(filter, options) +``` + +## Validation Errors + +When coercion fails for multi-case DU fields, a descriptive `ObjectListFilterValidationException` is raised: + +```fsharp +type Status = Active | Inactive | Pending +type User = { Status: Status } + +// Invalid status value +let filter = "status" === "unknown" +let users = query.ApplyWithCoercion filter +// ❌ Throws: Invalid value 'unknown' for filter field 'status' of type 'Status'. +// Valid values: Active, Inactive, Pending. +``` + +## Operator Suffix Handling + +The middleware appends suffixes like `_eq`, `_gte`, `_starts_with` to field names during parsing. These are automatically stripped before property lookup: + +```fsharp +type User = { Age: int } + +// Middleware creates: "age_gte" field name +let filter = "age" ===> 18 // >= operator + +// Coercion strips "_gte" suffix and finds "Age" property correctly +let users = query.ApplyWithCoercion filter // ✅ Works! +``` + +## Backward Compatibility + +The original `.Apply()` method remains unchanged. Use `.ApplyWithCoercion()` explicitly when you need automatic type conversion: + +```fsharp +// Old code continues to work +let filter = buildFilter() +let results = query.Apply(filter) // No coercion + +// Opt-in to coercion +let results = query.ApplyWithCoercion(filter) // With coercion +``` + +## Performance Considerations + +Type coercion adds a preprocessing pass over the filter tree. For optimal performance: +- Use `.Apply()` when your filter values already match property types +- Use `.ApplyWithCoercion()` only when needed (e.g., GraphQL input from JSON) +- Custom coercers are called in order until one succeeds - keep the list short + +## API Reference + +### Extension Methods + +```fsharp +type IQueryable<'T> with + /// Applies filter with automatic type coercion + member ApplyWithCoercion : + filter:ObjectListFilter * + [] options:ObjectListFilterLinqOptions<'T, 'D> + -> IQueryable<'T> + +type ObjectListFilter with + /// Applies filter to query with automatic type coercion + member ApplyToWithCoercion : + query:IQueryable<'T> * + [] options:ObjectListFilterLinqOptions<'T, 'D> + -> IQueryable<'T> +``` + +### Types + +```fsharp +/// Function signature for custom value coercion +type FilterValueCoercer = Type -> obj -> obj voption + +/// Options for filter application with custom coercers +type ObjectListFilterLinqOptions<'T, 'D> = + new : customCoercers:FilterValueCoercer list -> ObjectListFilterLinqOptions<'T, 'D> + member CustomCoercers : FilterValueCoercer list + +/// Exception raised when filter validation fails during coercion +type ObjectListFilterValidationException = + inherit GQLMessageExceptionBase + new : message:string * ?extensions:Dictionary -> ObjectListFilterValidationException +``` diff --git a/docs/type-system.md b/docs/type-system.md index f32415e0..0d9e7162 100644 --- a/docs/type-system.md +++ b/docs/type-system.md @@ -98,7 +98,7 @@ How the sequence is delivered depends on the query: - With `@defer` on a `Nullable (ListOf ...)` field the complete list is delivered in one deferred payload. - With `@stream` every item is delivered as soon as the sequence produces it and its fields are resolved. The enumeration is cancelled when the client unsubscribes. -Over `graphql-transport-ws`, a `@defer` or `@stream` field is delivered using the `pending`/`incremental`/`completed`/`hasNext` format used by graphql-js 17 and Apollo Client's `GraphQL17Alpha9Handler`. Each field is announced once, in a `pending` entry, and identified afterwards by a short id rather than its path. A deferred field is announced at the path of the object containing it, in the same payload as its own value, and that value is delivered as an object map of the one field, which the client merges into the announced object; a streamed field is announced at its own path as soon as the payload exposing its containing data is sent, and its items are delivered as the `items` of `incremental` entries, always in list order, a batch of items in one entry. `@defer` on a fragment spread or inline fragment delivers the fragment's fields together, as one payload of the object containing them, announced at that object's path; a field also selected directly on the object is executed with it, and an error propagating up to the fragment completes it with the errors and no data. The `label` of `@defer` or `@stream` surfaces as `pending.label`. Both directives take `if: Boolean = true`, which executes the field inline when false, and `@stream` takes `initialCount: Int = 0`, the number of items delivered with the initial payload before the rest is streamed. A payload that carries only GraphQL errors omits the top-level `data` property instead of sending `data: null`, matching the existing request-error contract used elsewhere in the transport. +Over `graphql-transport-ws`, a `@defer` or `@stream` field is delivered using the `pending`/`incremental`/`completed`/`hasNext` format used by graphql-js 17 and Apollo Client's `GraphQL17Alpha9Handler`. Each field is announced once, in a `pending` entry, and identified afterwards by a short id rather than its path. A deferred field is announced at the path of the object containing it, in the same payload as its own value, and that value is delivered as an object map of the one field, which the client merges into the announced object; a streamed field is announced at its own path as soon as the payload exposing its containing data is sent, and its items are delivered as the `items` of `incremental` entries, always in list order, a batch of items in one entry. `@defer` on a fragment spread or inline fragment delivers the fragment's fields together, as one payload of the object containing them, announced at that object's path; a field also selected directly on the object is executed with it, and an error propagating up to the fragment completes it with the errors and no data. The `label` of `@defer` or `@stream` surfaces as `pending.label`. Both directives take `if: Boolean = true`, which executes the field inline, or the fragment's fields with the object, when false, and `@stream` takes `initialCount: Int = 0`, the number of items delivered with the initial payload before the rest is streamed. A payload that carries only GraphQL errors omits the top-level `data` property instead of sending `data: null`, matching the existing request-error contract used elsewhere in the transport. Streamed items can be grouped into batches. The `preferredBatchSize` argument of `@stream`, available with `SchemaConfig.DefaultWithBufferedStream`, has priority. Otherwise the `batching` parameter of the field applies. It is either a fixed size or a function that reads the size from the source, such as the page size of a paged SDK sequence. The function is evaluated lazily: only for a `@stream` query that does not itself specify `preferredBatchSize`, so it never runs for an ordinary or `@defer` query. diff --git a/src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs b/src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs index ca0c6a78..7b00bd2f 100644 --- a/src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs +++ b/src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs @@ -16,6 +16,12 @@ open FsToolkit.ErrorHandling open FSharp.Data.GraphQL.Server open FSharp.Data.GraphQL.Shared +[] +module private DeferredEventLogging = + + /// The path of a deferred event as one string for the log, its segments joined as GraphQL error paths print them + let formatPath (path : obj list) = String.Join ("/", path) + /// Handles GraphQL requests using a provided root schema. type DefaultGraphQLRequestHandler<'Root> /// @@ -66,36 +72,36 @@ and [] GraphQLRequestHandler<'Root> |> Observable.add (function | DeferredPending (path, label, isStream, _) -> let fieldKind = if isStream then "streamed" else "deferred" - logger.LogDebug ("Announced GraphQL deferred field at path: {path}", path |> Seq.map string |> Seq.toArray |> Path.Join) + logger.LogDebug ("Announced GraphQL deferred field at path: {path}", formatPath path) match label with | ValueSome label -> logger.LogDebug ("Deferred field label: {label}; kind: {kind}", label, fieldKind) | ValueNone -> logger.LogDebug ("Deferred field kind: {kind}", fieldKind) | DeferredResult (data, path) -> - logger.LogDebug ("Produced GraphQL deferred result for path: {path}", path |> Seq.map string |> Seq.toArray |> Path.Join) + logger.LogDebug ("Produced GraphQL deferred result for path: {path}", formatPath path) if logger.IsEnabled LogLevel.Trace then logger.LogTrace ("GraphQL deferred data:\n{data}", serializeIndented data) | DeferredErrors (data, errors, path) -> - logger.LogDebug ("Produced GraphQL deferred errors for path: {path}", path |> Seq.map string |> Seq.toArray |> Path.Join) + logger.LogDebug ("Produced GraphQL deferred errors for path: {path}", formatPath path) if logger.IsEnabled LogLevel.Trace then logger.LogTrace ("GraphQL deferred errors:\n{errors}\nGraphQL deferred data:\n{data}", errors, serializeIndented data) | DeferredCompleted path -> - logger.LogDebug ("Completed GraphQL deferred field at path: {path}", path |> Seq.map string |> Seq.toArray |> Path.Join) + logger.LogDebug ("Completed GraphQL deferred field at path: {path}", formatPath path) | DeferredFragmentPending (path, label, fragmentId) -> logger.LogDebug ( "Announced GraphQL deferred fragment #{fragmentId} (label: {label}) at path: {path}", fragmentId, label |> ValueOption.toObj, - path |> Seq.map string |> Seq.toArray |> Path.Join + formatPath path ) | DeferredFragmentResult (data, errors, path, fragmentId) -> - logger.LogDebug ("Produced GraphQL deferred fragment #{fragmentId} result for path: {path}", fragmentId, path |> Seq.map string |> Seq.toArray |> Path.Join) + logger.LogDebug ("Produced GraphQL deferred fragment #{fragmentId} result for path: {path}", fragmentId, formatPath path) if logger.IsEnabled LogLevel.Trace then logger.LogTrace ("GraphQL deferred fragment errors:\n{errors}\nGraphQL deferred fragment data:\n{data}", errors, serializeIndented (data |> ValueOption.toObj)) | DeferredFragmentCompleted (path, fragmentId) -> - logger.LogDebug ("Completed GraphQL deferred fragment #{fragmentId} at path: {path}", fragmentId, path |> Seq.map string |> Seq.toArray |> Path.Join)) + logger.LogDebug ("Completed GraphQL deferred fragment #{fragmentId} at path: {path}", fragmentId, formatPath path)) GQLResponse.Direct (documentId, data, errs) diff --git a/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs b/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs index 82975b3f..6d0f182c 100644 --- a/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs +++ b/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs @@ -57,7 +57,7 @@ type internal QueryWeightMiddleware (threshold : float, reportToMetadata : bool) | ResolveDeferred info -> checkThreshold current (info :: xs) | ResolveStreamed (info, _) -> checkThreshold current (info :: xs) | ResolveLive info -> checkThreshold current (info :: xs) - | ResolveDeferredFragment (_, _, fields) -> checkThreshold current (fields @ xs) + | ResolveDeferredFragment (_, _, _, fields) -> checkThreshold current [ yield! fields; yield! xs ] checkThreshold 0.0 fields let error (ctx : ExecutionContext) = GQLExecutionResult.ErrorAsync ( diff --git a/src/FSharp.Data.GraphQL.Server/Execution.fs b/src/FSharp.Data.GraphQL.Server/Execution.fs index b6e833d4..b9744345 100644 --- a/src/FSharp.Data.GraphQL.Server/Execution.fs +++ b/src/FSharp.Data.GraphQL.Server/Execution.fs @@ -357,6 +357,27 @@ let private deferredFragmentEvents | ValueSome _ -> AnnouncedEvents.announced (DeferredFragmentPending (fragmentPath, label, fragmentId)) events | ValueNone -> events +/// +/// The fields with every deferred fragment whose if argument is with the variables of +/// the request replaced by its own fields, merged into the selection as if the directive were absent; a fragment +/// still deferred stays among the fields as it is. +/// +let private inlineDisabledFragments (variables : ImmutableDictionary) (fields : ExecutionInfo list) = + let rec inlineDisabled (fields : ExecutionInfo list) = + ([], fields) + ||> List.fold (fun fields field -> + match field.Kind with + | ResolveDeferredFragment (_, _, enabled, fragmentFields) when enabled variables = Ok false -> + Planning.deepMerge fields (inlineDisabled fragmentFields) + | _ -> [ yield! fields; yield field ]) + inlineDisabled fields + +/// The root fields a deferred fragment at the operation's root selects, those of the fragments nested in it included. +let rec private rootFieldsOfFragment (info : ExecutionInfo) = + match info.Kind with + | ResolveDeferredFragment (_, _, _, fragmentFields) -> fragmentFields |> Seq.collect rootFieldsOfFragment + | _ -> Seq.singleton info + /// Collect together an array of results using the appropriate execution strategy. let collectFields (strategy : ExecutionStrategy) @@ -849,6 +870,7 @@ and executeObjectFields // and is delivered afterwards, its fields resolved together against the same object let ownFields, deferredFragments = fields + |> inlineDisabledFragments ctx.Variables |> List.partition (fun field -> match field.Kind with | ResolveDeferredFragment _ -> false @@ -856,7 +878,7 @@ and executeObjectFields let executeDeferredFragment (deferred : IObservable voption) (fragment : ExecutionInfo) = match fragment.Kind with - | ResolveDeferredFragment (label, fragmentId, fragmentFields) -> + | ResolveDeferredFragment (label, fragmentId, _, fragmentFields) -> let events = executeObjectFields fragmentFields objName objDef inputContext ctx path value |> deferredFragmentEvents label fragmentId (normalizeErrorPath path) @@ -947,7 +969,7 @@ let private executeQueryOrMutation /// value once the root's own fields have been delivered let executeRootDeferredFragment (deferred : IObservable voption) (info : ExecutionInfo) = match info.Kind with - | ResolveDeferredFragment (label, fragmentId, fragmentFields) -> + | ResolveDeferredFragment (label, fragmentId, _, fragmentFields) -> let rootCtx = { ExecutionInfo = info Context = ctx @@ -984,10 +1006,21 @@ let private executeQueryOrMutation match getArgumentValues argDefs info.Ast.Arguments ctx.GetInputContext ctx.Variables with | Ok args -> coerced.Add (i, struct (args, [])) | Error errs -> coerced.Add (i, struct (Map.empty, errs))) - let coercionErrors = - coerced.Values - |> Seq.collect (fun struct (_, errs) -> errs) - |> Seq.toList + // The root fields of a deferred fragment are validated the same way, so that an argument one of them rejects + // fails the request before any root resolver runs instead of surfacing later as a deferred execution error; + // the fragment coerces them again when it executes, since it resolves its fields as any object's + let fragmentCoercionErrors = + deferredFragments + |> Seq.collect (snd >> rootFieldsOfFragment) + |> Seq.collect (fun info -> + let argDefs = ctx.FieldExecuteMap.GetArgs (ctx.ExecutionPlan.RootDef.Name, info.Definition.Name) + match getArgumentValues argDefs info.Ast.Arguments ctx.GetInputContext ctx.Variables with + | Ok _ -> [] + | Error errs -> errs) + let coercionErrors = [ + yield! coerced.Values |> Seq.collect (fun struct (_, errs) -> errs) + yield! fragmentCoercionErrors + ] if not coercionErrors.IsEmpty then return GQLExecutionResult.Error (documentId, coercionErrors, ctx.Metadata) else @@ -1194,6 +1227,8 @@ let internal coerceVariables let internal executeOperation (ctx : ExecutionContext) : AsyncVal = let includeResults = ctx.ExecutionPlan.Fields + // A root fragment disabled with `if: false` through a variable contributes its fields to the root selection + |> inlineDisabledFragments ctx.Variables |> List.map (fun info -> info.Include ctx.Variables |> Result.map (fun include -> struct (info, include))) diff --git a/src/FSharp.Data.GraphQL.Server/Linq.fs b/src/FSharp.Data.GraphQL.Server/Linq.fs index 2c3703f7..9d1e1240 100644 --- a/src/FSharp.Data.GraphQL.Server/Linq.fs +++ b/src/FSharp.Data.GraphQL.Server/Linq.fs @@ -427,7 +427,7 @@ let rec private compose inputContext vars ir = let rec private getTracks alreadyFound info = match info.Kind with // A deferred fragment has no resolver of its own: its fields are tracked as fields of the object containing it - | ResolveDeferredFragment (_, _, fields) -> IR(info, Set.empty, fields |> List.map (getTracks alreadyFound)) + | ResolveDeferredFragment (_, _, _, fields) -> IR(info, Set.empty, fields |> List.map (getTracks alreadyFound)) | _ -> getFieldTracks alreadyFound info and private getFieldTracks alreadyFound info = diff --git a/src/FSharp.Data.GraphQL.Server/Planning.fs b/src/FSharp.Data.GraphQL.Server/Planning.fs index 77d38d96..d4eee2fe 100644 --- a/src/FSharp.Data.GraphQL.Server/Planning.fs +++ b/src/FSharp.Data.GraphQL.Server/Planning.fs @@ -236,7 +236,9 @@ let private getAbstractionFrag = function Debug.Fail "Must be prevented by validation" raise (InvalidOperationException $"Expected a fragment to be planned as {nameof ResolveAbstraction}, but it was planned as {kindName kind}") -let rec private deepMerge (xs: ExecutionInfo list) (ys: ExecutionInfo list) = +/// The fields of both selections as one: a field selected by both has the selections under it merged, in the order +/// of the first selection, and the fields only the second selects follow. +let rec internal deepMerge (xs: ExecutionInfo list) (ys: ExecutionInfo list) = let rec merge (x: ExecutionInfo) (y: ExecutionInfo) = match x.Kind, y.Kind with | ResolveValue, ResolveValue -> x @@ -263,20 +265,53 @@ let rec private deepMerge (xs: ExecutionInfo list) (ys: ExecutionInfo list) = |> List.filter(fun y -> not <| List.exists(fun x -> x.Identifier = y.Identifier) xs') xs' @ ys' -/// The state of planning one selection set together with the fragments spread into it: the fragments already -/// planned, each spread once, and the next id of a deferred fragment, unique among the deferred fragments of the -/// selection set. +/// +/// The state of planning one selection set together with the fragments spread into it: the fragments already spread +/// into the object's own selection, the fragments already spread deferred, and the next id of a deferred fragment, +/// unique among the deferred fragments of the selection set. +/// +/// +/// A fragment is spread once. A deferred spread never stands in for a direct spread of the same fragment, whichever +/// comes first in the document, so the two are tracked apart: a direct spread is skipped only after a direct spread, +/// a deferred spread after a spread of either kind. +/// type private SelectionScope = { mutable VisitedFragments : string list + mutable DeferredFragments : string list mutable NextFragmentId : int } -let private newScope () = { VisitedFragments = []; NextFragmentId = 0 } +/// The scope of a selection set no fragment was spread into yet. +let private newScope () = { VisitedFragments = []; DeferredFragments = []; NextFragmentId = 0 } -/// The @defer directive of a fragment spread or inline fragment, when it applies +/// +/// Whether the fragment spread is planned, recording it in the scope when it is, as documented on +/// . +/// +let private tryVisitSpread (scope : SelectionScope) (spreadName : string) (isDeferred : bool) = + let spreadDirectly = scope.VisitedFragments |> List.contains spreadName + if isDeferred then + if spreadDirectly || scope.DeferredFragments |> List.contains spreadName then + false + else + scope.DeferredFragments <- spreadName :: scope.DeferredFragments + true + elif spreadDirectly then + false + else + scope.VisitedFragments <- spreadName :: scope.VisitedFragments + true + +/// +/// The @defer directive of a fragment spread or inline fragment, when it applies as far as planning can tell. +/// let private deferredFragmentDirective (directives : Directive list) = directives |> List.vtryFind (fun d -> d.Name = "defer" && isEnabledAtPlanning d) +/// +/// The label argument of the directive: a string literal, as validation requires, so there is none for any +/// other value. +/// let private directiveLabel (directive : Directive) = directive.Arguments |> List.vtryFind (fun argument -> argument.Name = "label") @@ -285,6 +320,21 @@ let private directiveLabel (directive : Directive) = | StringValue label -> ValueSome label | _ -> ValueNone) +/// +/// Whether the directive is enabled with the variables of a request: a literal if was decided by +/// , so only an if given through a variable is evaluated here, the same way +/// the execution engine evaluates it for a deferred field. +/// +let private directiveEnabledAtExecution (directive : Directive) : Includer = + match directive.Arguments |> List.vtryFind (fun argument -> argument.Name = "if") with + | ValueSome { Value = VariableName name } -> + fun variables -> + match variables.TryGetValue name with + | true, (:? bool as enabled) -> Ok enabled + | _ -> Ok true + | _ -> incl + +/// The next id of a deferred fragment of the selection set. let private allocateFragmentId (scope : SelectionScope) = let fragmentId = scope.NextFragmentId scope.NextFragmentId <- fragmentId + 1 @@ -295,26 +345,32 @@ let private allocateFragmentId (scope : SelectionScope) = let private deferredFragmentEntry (directive : Directive) (fragmentId : int) (info : ExecutionInfo) (fragmentFields : ExecutionInfo list) = { info with Identifier = $"@defer#{fragmentId}" - Kind = ResolveDeferredFragment (directiveLabel directive, fragmentId, fragmentFields) } + Kind = ResolveDeferredFragment (directiveLabel directive, fragmentId, directiveEnabledAtExecution directive, fragmentFields) } /// A field selected directly on the object is executed with it, so a deferred fragment that also selects it -/// delivers only its other fields; a fragment left without fields delivers nothing and is dropped +/// delivers only its other fields, and whatever the fragment selects under that field is selected on the object's +/// own field instead; a fragment left without fields delivers nothing and is dropped let private withoutDirectlySelectedFields (plannedFields : ExecutionInfo list) = let directlySelected = plannedFields - |> List.choose (fun field -> + |> List.vchoose (fun field -> match field.Kind with - | ResolveDeferredFragment _ -> None - | _ -> Some field.Identifier) + | ResolveDeferredFragment _ -> ValueNone + | _ -> ValueSome field.Identifier) |> Set.ofList - plannedFields - |> List.choose (fun field -> - match field.Kind with - | ResolveDeferredFragment (label, fragmentId, fragmentFields) -> - match fragmentFields |> List.filter (fun fragmentField -> not (directlySelected.Contains fragmentField.Identifier)) with - | [] -> None - | remaining -> Some { field with Kind = ResolveDeferredFragment (label, fragmentId, remaining) } - | _ -> Some field) + let ownFields, fragments = + plannedFields + |> List.fold (fun (ownFields, fragments) field -> + match field.Kind with + | ResolveDeferredFragment (label, fragmentId, enabled, fragmentFields) -> + let overlapping, remaining = + fragmentFields |> List.partition (fun fragmentField -> directlySelected.Contains fragmentField.Identifier) + let ownFields = deepMerge ownFields overlapping + match remaining with + | [] -> ownFields, fragments + | remaining -> ownFields, { field with Kind = ResolveDeferredFragment (label, fragmentId, enabled, remaining) } :: fragments + | _ -> [ yield! ownFields; yield field ], fragments) ([], []) + [ yield! ownFields; yield! List.rev fragments ] let rec private plan (ctx : PlanningContext) (info : ExecutionInfo) : ExecutionInfo = match info.ReturnDef with @@ -343,7 +399,7 @@ and private planSelection (ctx: PlanningContext) (selectionSet: Selection list) /// later as one payload of the object let withFragmentFields (fields : ExecutionInfo list) (directives : Directive list) (fragmentFields : ExecutionInfo list) = match deferredFragmentDirective directives with - | ValueSome directive -> fields @ [ deferredFragmentEntry directive (allocateFragmentId scope) info fragmentFields ] + | ValueSome directive -> [ yield! fields; yield deferredFragmentEntry directive (allocateFragmentId scope) info fragmentFields ] | ValueNone -> deepMerge fields fragmentFields // filter out already existing fields let plannedFields = selectionSet @@ -366,10 +422,9 @@ and private planSelection (ctx: PlanningContext) (selectionSet: Selection list) | Planned -> fields @ [ executionPlan ] | FragmentSpread spread -> let spreadName = spread.Name - if scope.VisitedFragments |> List.exists (fun name -> name = spreadName) + if not (tryVisitSpread scope spreadName (deferredFragmentDirective spread.Directives).IsSome) then fields // Fragment already found else - scope.VisitedFragments <- spreadName :: scope.VisitedFragments match ctx.Document.Definitions |> List.tryFind (function FragmentDefinition f -> f.Name.Value = spreadName | _ -> false) with | Some (FragmentDefinition fragment) when doesFragmentTypeApply ctx.Schema fragment parentDef -> // Retrieve fragment data just as it was normal selection set @@ -394,9 +449,15 @@ and private planAbstraction (ctx:PlanningContext) (selectionSet: Selection list) match deferredFragmentDirective directives with | ValueSome directive -> let fragmentId = allocateFragmentId scope - fragmentFields - |> Map.map (fun _ typeFields -> [ deferredFragmentEntry directive fragmentId info typeFields ]) - |> Map.merge (fun _ -> deepMerge) fields + // One entry per concrete type, appended to that type's fields: an entry's identifier is no field's, so + // there is nothing to merge + (fields, fragmentFields) + ||> Map.fold (fun fields typeName typeFields -> + let entry = deferredFragmentEntry directive fragmentId info typeFields + fields + |> Map.change typeName (function + | Some existing -> Some [ yield! existing; yield entry ] + | None -> Some [ entry ])) | ValueNone -> Map.merge (fun _ -> deepMerge) fields fragmentFields // Filter out already existing fields let plannedTypeFields = selectionSet @@ -415,10 +476,9 @@ and private planAbstraction (ctx:PlanningContext) (selectionSet: Selection list) | Planned -> Map.merge (fun _ -> deepMerge) fields infoMap | FragmentSpread spread -> let spreadName = spread.Name - if scope.VisitedFragments |> List.exists (fun name -> name = spreadName) + if not (tryVisitSpread scope spreadName (deferredFragmentDirective spread.Directives).IsSome) then fields // Fragment already found else - scope.VisitedFragments <- spreadName :: scope.VisitedFragments match ctx.Document.Definitions |> List.tryFind (function FragmentDefinition f -> f.Name.Value = spreadName | _ -> false) with | Some (FragmentDefinition fragment) -> // Retrieve fragment data just as it was normal selection set diff --git a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs index e9011a4f..9be296a5 100644 --- a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs +++ b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs @@ -713,7 +713,7 @@ and ExecutionInfo = { fields |> Seq.collect (fun f -> match f.Kind with - | ResolveDeferredFragment (_, _, fragmentFields) -> flattenDeferredFragments fragmentFields + | ResolveDeferredFragment (_, _, _, fragmentFields) -> flattenDeferredFragments fragmentFields | _ -> Seq.singleton f) let rec path info segments = match segments with @@ -729,7 +729,7 @@ and ExecutionInfo = { | ResolveValue -> ValueNone | ResolveCollection inner -> path inner segments | SelectFields fields - | ResolveDeferredFragment (_, _, fields) -> + | ResolveDeferredFragment (_, _, _, fields) -> fields |> flattenDeferredFragments |> Seq.vtryFind (fun f -> f.Identifier = head) @@ -767,13 +767,15 @@ and ExecutionInfo = { sb.Append("ResolveLive: ").AppendLine (nameAs info) |> ignore str (indent + 1) sb inner - | ResolveDeferredFragment (label, fragmentId, fields) -> + | ResolveDeferredFragment (label, fragmentId, _, fields) -> pad indent sb let labelText = match label with | ValueSome label -> $" (label: {label})" | ValueNone -> "" - sb.Append("ResolveDeferredFragment: ").AppendLine ($"#{fragmentId}{labelText}") + sb + |> _.Append("ResolveDeferredFragment: ") + |> _.AppendLine($"#{fragmentId}{labelText}") |> ignore fields |> List.iter (str (indent + 1) sb) | ResolveStreamed (inner, mode) -> @@ -828,9 +830,13 @@ and ExecutionInfoKind = | ResolveStreamed of ExecutionInfo * BufferedStreamOptions /// Reduce the current field as a live query. | ResolveLive of ExecutionInfo - /// Reduce a fragment deferred with @defer: its fields are delivered later, as one payload - /// of the object containing them, identified within that object's selection by the id. - | ResolveDeferredFragment of label : string voption * fragmentId : int * fields : ExecutionInfo list + /// + /// Reduce a fragment deferred with @defer: its fields are delivered later, as one payload of the object + /// containing them, identified within that object's selection by the id. When the directive's if argument + /// evaluates to with the variables of the request, the fields are resolved with the object + /// instead, as if the directive were absent. + /// + | ResolveDeferredFragment of label : string voption * fragmentId : int * enabled : Includer * fields : ExecutionInfo list /// Buffered stream options. Used to specify how the buffer will behavior in a stream. and BufferedStreamOptions = { diff --git a/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs b/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs index cc923c76..47049122 100644 --- a/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs @@ -1987,6 +1987,141 @@ let ``A field selected both directly and in a deferred fragment is executed with DeferredFragmentCompleted ([ "testData" ], 0) ] +[] +let ``A deferred fragment selecting under a field selected directly adds its selection to that field`` () = + let expectedDirect = + NameValueLookup.ofList [ + "testData", upcast NameValueLookup.ofList [ + "innerList", upcast [ + NameValueLookup.ofList [ + "a", upcast "Inner A" + "innerList", upcast [ + NameValueLookup.ofList [ "a", upcast "Inner B" ] + NameValueLookup.ofList [ "a", upcast "Inner C" ] + ] + ] + ] + ] + ] + let query = parse """{ + testData { + innerList { + a + } + ... @defer { + innerList { + innerList { + a + } + } + } + } + }""" + let result = executor.AsyncExecute(query, getMockInputContext) |> sync + // The fragment selects nothing but the field the object selects itself, so it delivers nothing + ensureDirect result <| fun data errors -> + empty errors + data |> equals (upcast expectedDirect) + +[] +let ``A fragment spread directly is resolved with the object whichever spread of it comes first`` () = + let expectedDirect = + NameValueLookup.ofList [ + "testData", upcast NameValueLookup.ofList [ + "a", upcast "Apple" + "b", upcast "Banana" + ] + ] + let execute (selection : string) = + let query = parse $"""query {{ + testData {{ + {selection} + }} + }} + fragment Rest on Data {{ + a + b + }}""" + executor.AsyncExecute(query, getMockInputContext) |> sync + for selection in [ "...Rest @defer ...Rest"; "...Rest ...Rest @defer" ] do + ensureDirect (execute selection) <| fun data errors -> + empty errors + data |> equals (upcast expectedDirect) + +[] +let ``Defer directive on a fragment with if false through a variable resolves the fragment's fields with the object`` () = + let query = parse """query ($d: Boolean!) { + testData { + id + ... @defer(if: $d) { + a + b + } + } + }""" + let variables = ImmutableDictionary.Empty.Add ("d", JsonDocument.Parse("false").RootElement) + let result = executor.AsyncExecute(query, getMockInputContext, variables = variables) |> sync + ensureDirect result <| fun data errors -> + empty errors + data + |> equals ( + upcast NameValueLookup.ofList [ + "testData", upcast NameValueLookup.ofList [ + "id", upcast "1" + "a", upcast "Apple" + "b", upcast "Banana" + ] + ] + ) + +[] +let ``Defer directive on a fragment with if true through a variable defers the fragment's fields`` () = + let query = parse """query ($d: Boolean!) { + testData { + id + ... @defer(if: $d) { + a + } + } + }""" + let variables = ImmutableDictionary.Empty.Add ("d", JsonDocument.Parse("true").RootElement) + let result = executor.AsyncExecute(query, getMockInputContext, variables = variables) |> sync + ensureDeferred result <| fun data errors deferred -> + empty errors + data |> equals (upcast NameValueLookup.ofList [ "testData", upcast NameValueLookup.ofList [ "id", upcast "1" ] ]) + use sub = Observer.create deferred + sub.WaitCompleted() + sub.Received + |> Seq.toList + |> equals [ + DeferredFragmentResult (ValueSome (upcast NameValueLookup.ofList [ "a", upcast "Apple" ]), [], [ "testData" ], 0) + DeferredFragmentCompleted ([ "testData" ], 0) + ] + +[] +let ``A root fragment with if false through a variable resolves its root fields with the root`` () = + let query = parse """query ($d: Boolean!) { + testData { + id + } + ... @defer(if: $d) { + nullableTestData { + id + } + } + }""" + let variables = ImmutableDictionary.Empty.Add ("d", JsonDocument.Parse("false").RootElement) + let result = executor.AsyncExecute(query, getMockInputContext, variables = variables) |> sync + ensureDirect result <| fun data errors -> + empty errors + data + |> equals ( + upcast NameValueLookup.ofList [ + "testData", upcast NameValueLookup.ofList [ "id", upcast "1" ] + "nullableTestData", upcast NameValueLookup.ofList [ "id", upcast "1" ] + ] + ) + [] let ``A fragment deferred at the operation root delivers root fields`` () = let query = parse """{ diff --git a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs index deef59dc..f8a8b3ec 100644 --- a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs @@ -478,21 +478,34 @@ let CoercionGuardInputType = | "US" -> Success | _ -> ValidationError [ { new IGQLError with member _.Message = "Unsupported country" } ]) +/// A query type whose `boom` resolver counts its calls and fails, and whose `bad` field rejects any country but US +let private coercionGuardSchema (boomCalls : int ref) = + Schema(Define.Object( + "Query", [ + Define.Field("boom", StringType, (fun _ _ -> boomCalls.Value <- boomCalls.Value + 1; failwith "Resolver Error!")) + Define.Field("bad", Nullable StringType, [ Define.Input("input", CoercionGuardInputType) ], fun _ _ -> None) + ])) + [] let ``Execution rejects inline argument coercion failures on one root field before running another root field's resolver`` () = let boomCalls = ref 0 - let schema = - Schema(Define.Object( - "Query", [ - Define.Field("boom", StringType, (fun _ _ -> boomCalls.Value <- boomCalls.Value + 1; failwith "Resolver Error!")) - Define.Field("bad", Nullable StringType, [ Define.Input("input", CoercionGuardInputType) ], fun _ _ -> None) - ])) + let schema = coercionGuardSchema boomCalls let query = """query Test { boom bad(input: { country: "FR" }) }""" let result = sync <| Executor(schema).AsyncExecute(query, getMockInputContext, ()) ensureRequestError result <| fun [ error ] -> error |> ensureInputObjectValidationError (Argument "input") "Unsupported country" [] "CoercionGuardInput!" Assert.Equal(0, boomCalls.Value) +[] +let ``Execution rejects inline argument coercion failures on a root field of a deferred fragment before running another root field's resolver`` () = + let boomCalls = ref 0 + let schema = coercionGuardSchema boomCalls + let query = """query Test { boom ... @defer { bad(input: { country: "FR" }) } }""" + let result = sync <| Executor(schema).AsyncExecute(query, getMockInputContext, ()) + ensureRequestError result <| fun [ error ] -> + error |> ensureInputObjectValidationError (Argument "input") "Unsupported country" [] "CoercionGuardInput!" + Assert.Equal(0, boomCalls.Value) + [] let ``Execution handles errors: nullable list fields`` () = let InnerObject = From 1e1c4e0f47be9a3c38d210e46736ee65d7ab8ac3 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Mon, 21 Sep 2026 03:22:26 +0200 Subject: [PATCH 4/4] Honor fragment includers and merge overlaps in any order A deferred fragment standing before the field it selects under now adds its selection to that field too: the object's own fields are collected first and the fragments' overlapping selections are merged into them afterwards, so the result does not depend on document order. The deferred fragment entry carries the includer of its spread or inline fragment, so `@skip`/`@include` on the fragment exclude it at the root and inside any object. Co-Authored-By: Claude Fable 5.1 --- src/FSharp.Data.GraphQL.Server/Execution.fs | 5 +- src/FSharp.Data.GraphQL.Server/Planning.fs | 42 ++++++------ .../DeferredTests.fs | 67 +++++++++++++++++++ 3 files changed, 93 insertions(+), 21 deletions(-) diff --git a/src/FSharp.Data.GraphQL.Server/Execution.fs b/src/FSharp.Data.GraphQL.Server/Execution.fs index b9744345..117d1c17 100644 --- a/src/FSharp.Data.GraphQL.Server/Execution.fs +++ b/src/FSharp.Data.GraphQL.Server/Execution.fs @@ -867,7 +867,8 @@ and executeObjectFields | Error errs -> asyncVal { return Error (errs |> List.map GQLProblemDetails.OfError) } // A deferred fragment stands among the fields of the object; it contributes nothing to the object's own value - // and is delivered afterwards, its fields resolved together against the same object + // and is delivered afterwards, its fields resolved together against the same object, unless `@skip`/`@include` + // on the fragment excludes it let ownFields, deferredFragments = fields |> inlineDisabledFragments ctx.Variables @@ -875,6 +876,8 @@ and executeObjectFields match field.Kind with | ResolveDeferredFragment _ -> false | _ -> true) + let deferredFragments = + deferredFragments |> List.filter (fun fragment -> fragment.Include ctx.Variables <> Ok false) let executeDeferredFragment (deferred : IObservable voption) (fragment : ExecutionInfo) = match fragment.Kind with diff --git a/src/FSharp.Data.GraphQL.Server/Planning.fs b/src/FSharp.Data.GraphQL.Server/Planning.fs index d4eee2fe..0287b8d7 100644 --- a/src/FSharp.Data.GraphQL.Server/Planning.fs +++ b/src/FSharp.Data.GraphQL.Server/Planning.fs @@ -349,27 +349,28 @@ let private deferredFragmentEntry (directive : Directive) (fragmentId : int) (in /// A field selected directly on the object is executed with it, so a deferred fragment that also selects it /// delivers only its other fields, and whatever the fragment selects under that field is selected on the object's -/// own field instead; a fragment left without fields delivers nothing and is dropped +/// own field instead, wherever in the selection the fragment stands; a fragment left without fields delivers nothing +/// and is dropped let private withoutDirectlySelectedFields (plannedFields : ExecutionInfo list) = - let directlySelected = - plannedFields - |> List.vchoose (fun field -> - match field.Kind with - | ResolveDeferredFragment _ -> ValueNone - | _ -> ValueSome field.Identifier) - |> Set.ofList let ownFields, fragments = plannedFields - |> List.fold (fun (ownFields, fragments) field -> + |> List.partition (fun field -> match field.Kind with + | ResolveDeferredFragment _ -> false + | _ -> true) + let directlySelected = ownFields |> List.map _.Identifier |> Set.ofList + let ownFields, fragments = + ((ownFields, []), fragments) + ||> List.fold (fun (ownFields, fragments) fragment -> + match fragment.Kind with | ResolveDeferredFragment (label, fragmentId, enabled, fragmentFields) -> let overlapping, remaining = fragmentFields |> List.partition (fun fragmentField -> directlySelected.Contains fragmentField.Identifier) let ownFields = deepMerge ownFields overlapping match remaining with | [] -> ownFields, fragments - | remaining -> ownFields, { field with Kind = ResolveDeferredFragment (label, fragmentId, enabled, remaining) } :: fragments - | _ -> [ yield! ownFields; yield field ], fragments) ([], []) + | remaining -> ownFields, { fragment with Kind = ResolveDeferredFragment (label, fragmentId, enabled, remaining) } :: fragments + | _ -> ownFields, fragments) [ yield! ownFields; yield! List.rev fragments ] let rec private plan (ctx : PlanningContext) (info : ExecutionInfo) : ExecutionInfo = @@ -396,10 +397,11 @@ let rec private plan (ctx : PlanningContext) (info : ExecutionInfo) : ExecutionI and private planSelection (ctx: PlanningContext) (selectionSet: Selection list) (info: ExecutionInfo) (scope : SelectionScope) : ExecutionInfo = let parentDef = downcast info.ReturnDef /// The fields of a fragment merged into the object's selection, or, when the fragment is deferred, delivered - /// later as one payload of the object - let withFragmentFields (fields : ExecutionInfo list) (directives : Directive list) (fragmentFields : ExecutionInfo list) = + /// later as one payload of the object; the entry carries the fragment's own includer, so `@skip`/`@include` on + /// the spread or inline fragment decide whether it is delivered at all + let withFragmentFields (fields : ExecutionInfo list) (fragmentInfo : ExecutionInfo) (directives : Directive list) (fragmentFields : ExecutionInfo list) = match deferredFragmentDirective directives with - | ValueSome directive -> [ yield! fields; yield deferredFragmentEntry directive (allocateFragmentId scope) info fragmentFields ] + | ValueSome directive -> [ yield! fields; yield deferredFragmentEntry directive (allocateFragmentId scope) fragmentInfo fragmentFields ] | ValueNone -> deepMerge fields fragmentFields // filter out already existing fields let plannedFields = selectionSet @@ -431,13 +433,13 @@ and private planSelection (ctx: PlanningContext) (selectionSet: Selection list) // TODO: Check if the path is correctly defined let fragmentInfo = planSelection ctx fragment.SelectionSet updatedInfo scope let fragmentFields = getSelectionFrag fragmentInfo.Kind - withFragmentFields fields spread.Directives fragmentFields + withFragmentFields fields updatedInfo spread.Directives fragmentFields | _ -> fields | InlineFragment fragment when doesFragmentTypeApply ctx.Schema fragment parentDef -> // retrieve fragment data just as it was normal selection set let fragmentInfo = planSelection ctx fragment.SelectionSet updatedInfo scope let fragmentFields = getSelectionFrag fragmentInfo.Kind - withFragmentFields fields fragment.Directives fragmentFields + withFragmentFields fields updatedInfo fragment.Directives fragmentFields | _ -> fields ) [] { info with Kind = SelectFields (withoutDirectlySelectedFields plannedFields) } @@ -445,7 +447,7 @@ and private planSelection (ctx: PlanningContext) (selectionSet: Selection list) and private planAbstraction (ctx:PlanningContext) (selectionSet: Selection list) (info : ExecutionInfo) (scope : SelectionScope) typeCondition : ExecutionInfo = /// The fields of a fragment merged into every type's selection, or, when the fragment is deferred, delivered /// later as one payload of the object, whatever its concrete type turns out to be - let withFragmentFields (fields : Map) (directives : Directive list) (fragmentFields : Map) = + let withFragmentFields (fields : Map) (fragmentInfo : ExecutionInfo) (directives : Directive list) (fragmentFields : Map) = match deferredFragmentDirective directives with | ValueSome directive -> let fragmentId = allocateFragmentId scope @@ -453,7 +455,7 @@ and private planAbstraction (ctx:PlanningContext) (selectionSet: Selection list) // there is nothing to merge (fields, fragmentFields) ||> Map.fold (fun fields typeName typeFields -> - let entry = deferredFragmentEntry directive fragmentId info typeFields + let entry = deferredFragmentEntry directive fragmentId fragmentInfo typeFields fields |> Map.change typeName (function | Some existing -> Some [ yield! existing; yield entry ] @@ -484,13 +486,13 @@ and private planAbstraction (ctx:PlanningContext) (selectionSet: Selection list) // Retrieve fragment data just as it was normal selection set let fragmentInfo = planAbstraction ctx fragment.SelectionSet innerData scope fragment.TypeCondition let fragmentFields = getAbstractionFrag fragmentInfo.Kind - withFragmentFields fields spread.Directives fragmentFields + withFragmentFields fields innerData spread.Directives fragmentFields | _ -> fields | InlineFragment fragment -> // Retrieve fragment data just as it was normal selection set let fragmentInfo = planAbstraction ctx fragment.SelectionSet innerData scope fragment.TypeCondition let fragmentFields = getAbstractionFrag fragmentInfo.Kind - withFragmentFields fields fragment.Directives fragmentFields + withFragmentFields fields innerData fragment.Directives fragmentFields ) Map.empty // Always return ResolveAbstraction kind, even for empty maps. // An empty map is a valid state representing "no fields selected for this type condition." diff --git a/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs b/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs index 47049122..5e831325 100644 --- a/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs @@ -2023,6 +2023,73 @@ let ``A deferred fragment selecting under a field selected directly adds its sel empty errors data |> equals (upcast expectedDirect) +[] +let ``A deferred fragment standing before the field it selects under still adds its selection to that field`` () = + let expectedDirect = + NameValueLookup.ofList [ + "testData", upcast NameValueLookup.ofList [ + "innerList", upcast [ + NameValueLookup.ofList [ + "a", upcast "Inner A" + "innerList", upcast [ + NameValueLookup.ofList [ "a", upcast "Inner B" ] + NameValueLookup.ofList [ "a", upcast "Inner C" ] + ] + ] + ] + ] + ] + let query = parse """{ + testData { + ... @defer { + innerList { + innerList { + a + } + } + } + innerList { + a + } + } + }""" + let result = executor.AsyncExecute(query, getMockInputContext) |> sync + ensureDirect result <| fun data errors -> + empty errors + data |> equals (upcast expectedDirect) + +[] +let ``A deferred fragment skipped with a directive is not delivered`` () = + let query = parse """{ + testData { + id + ... @defer @skip(if: true) { + a + } + } + }""" + let result = executor.AsyncExecute(query, getMockInputContext) |> sync + ensureDirect result <| fun data errors -> + empty errors + data |> equals (upcast NameValueLookup.ofList [ "testData", upcast NameValueLookup.ofList [ "id", upcast "1" ] ]) + +[] +let ``A deferred root fragment excluded with a directive is not delivered`` () = + let query = parse """{ + testData { + id + } + ... @defer @include(if: false) { + nullableTestData { + id + } + } + }""" + let result = executor.AsyncExecute(query, getMockInputContext) |> sync + ensureDirect result <| fun data errors -> + empty errors + data |> equals (upcast NameValueLookup.ofList [ "testData", upcast NameValueLookup.ofList [ "id", upcast "1" ] ]) + [] let ``A fragment spread directly is resolved with the object whichever spread of it comes first`` () = let expectedDirect =