From f0feede42b443a2ac66487799255a3c167b984b4 Mon Sep 17 00:00:00 2001 From: VectorTetra Date: Mon, 3 Feb 2025 20:38:31 +0200 Subject: [PATCH 1/5] Implemented ability to add additional errors to resolved fields Covered test case: Add additional error and return value from a non-nullable GraphQL field resolver --- src/FSharp.Data.GraphQL.Server/Execution.fs | 19 ++++++-- src/FSharp.Data.GraphQL.Server/Executor.fs | 3 ++ src/FSharp.Data.GraphQL.Shared/TypeSystem.fs | 23 ++++++++-- .../ExecutionTests.fs | 44 +++++++++++++++---- 4 files changed, 73 insertions(+), 16 deletions(-) diff --git a/src/FSharp.Data.GraphQL.Server/Execution.fs b/src/FSharp.Data.GraphQL.Server/Execution.fs index 0c472d40..b1406e36 100644 --- a/src/FSharp.Data.GraphQL.Server/Execution.fs +++ b/src/FSharp.Data.GraphQL.Server/Execution.fs @@ -364,12 +364,23 @@ and private executeResolvers (ctx : ResolveFieldContext) (path : FieldPath) (par /// This handles all null resolver errors/error propagation. let resolveWith (ctx : ResolveFieldContext) (onSuccess : ResolveFieldContext -> FieldPath -> obj -> obj -> AsyncVal>>) : AsyncVal>> = asyncVal { let! resolved = value |> AsyncVal.rescue path ctx.Schema.ParseError + let additionalErrs = + match ctx.Context.Errors.TryGetValue ctx with + | true, errors -> + errors + |> Seq.map (GQLProblemDetails.OfFieldExecutionError (path |> List.rev)) + |> Seq.toList + | false, _ -> [] match resolved with - | Error errs when ctx.ExecutionInfo.IsNullable -> return Ok (KeyValuePair(name, null), None, errs) - | Ok None when ctx.ExecutionInfo.IsNullable -> return Ok (KeyValuePair(name, null), None, []) - | Error errs -> return Error errs + | Error errs when ctx.ExecutionInfo.IsNullable -> return Ok (KeyValuePair(name, null), None, errs @ additionalErrs) + | Ok None when ctx.ExecutionInfo.IsNullable -> + return Ok (KeyValuePair(name, null), None, additionalErrs) + | Error errs -> return Error (errs @ additionalErrs) | Ok None -> return Error (nullResolverError name path ctx) - | Ok (Some v) -> return! onSuccess ctx path parent v + | Ok (Some v) -> + match! onSuccess ctx path parent v with + | Ok (res, deferred, errs) -> return Ok (res, deferred, errs @ additionalErrs) + | Error errs -> return Error (errs @ additionalErrs) } match info.Kind, returnDef with diff --git a/src/FSharp.Data.GraphQL.Server/Executor.fs b/src/FSharp.Data.GraphQL.Server/Executor.fs index ff36a50b..b332868b 100644 --- a/src/FSharp.Data.GraphQL.Server/Executor.fs +++ b/src/FSharp.Data.GraphQL.Server/Executor.fs @@ -1,5 +1,6 @@ namespace FSharp.Data.GraphQL +open System.Collections.Concurrent open System.Collections.Immutable open System.Collections.Generic open System.Runtime.InteropServices @@ -108,6 +109,7 @@ type Executor<'Root>(schema: ISchema<'Root>, middlewares : IExecutorMiddleware s | Stream (stream) -> GQLExecutionResult.Stream (documentId, stream, res.Metadata) async { try + let errors = ConcurrentDictionary>() let root = data |> Option.map box |> Option.toObj match coerceVariables executionPlan.Variables variables with | Error errs -> return prepareOutput (GQLExecutionResult.Error (documentId, errs, executionPlan.Metadata)) @@ -117,6 +119,7 @@ type Executor<'Root>(schema: ISchema<'Root>, middlewares : IExecutorMiddleware s RootValue = root ExecutionPlan = executionPlan Variables = variables + Errors = errors FieldExecuteMap = fieldExecuteMap Metadata = executionPlan.Metadata } let! res = runMiddlewares (fun x -> x.ExecuteOperationAsync) executionCtx executeOperation |> AsyncVal.toAsync diff --git a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs index 4a5ac580..e632db87 100644 --- a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs +++ b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs @@ -5,9 +5,11 @@ namespace FSharp.Data.GraphQL.Types open System open System.Reflection open System.Collections +open System.Collections.Concurrent open System.Collections.Generic open System.Collections.Immutable open System.Text.Json + open FSharp.Data.GraphQL open FSharp.Data.GraphQL.Ast open FSharp.Data.GraphQL.Extensions @@ -890,11 +892,22 @@ and ExecutionContext = { ExecutionPlan : ExecutionPlan /// Collection of variables provided to execute current operation. Variables : ImmutableDictionary + /// Collection of errors that occurred while executing current operation. + Errors : ConcurrentDictionary> /// A map of all fields of the query and their respective execution operations. FieldExecuteMap : FieldExecuteMap /// A simple dictionary to hold metadata that can be used by execution customizations. Metadata : Metadata -} +} with + + /// Remembers an error, so it can be included in the final response. + member this.AddError (fieldContext, error : IGQLError) : unit = + this.Errors.AddOrUpdate( + fieldContext, + addValueFactory = (fun _ -> ConcurrentBag (Seq.singleton error)), + updateValueFactory = (fun _ (bag)-> bag.Add error; bag) + ) + |> ignore /// An execution context for the particular field, applied as the first /// parameter for target resolve function. @@ -919,9 +932,13 @@ and ResolveFieldContext = { Path : FieldPath } with + /// Remembers an error, so it can be included in the final response. + member this.AddError (error : IGQLError) = + this.Context.AddError (this, error) + /// Tries to find an argument by provided name. - member x.TryArg (name : string) : 't option = - match Map.tryFind name x.Args with + member this.TryArg (name : string) : 't option = + match Map.tryFind name this.Args with | Some o -> Some (o :?> 't) // TODO: Use Convert.ChangeType | None -> None diff --git a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs index 720555f3..21da15dc 100644 --- a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs @@ -394,40 +394,66 @@ let ``Execution when querying returns unique document id with response`` () = | response -> fail $"Expected a 'Direct' GQLResponse but got\n{response}" type InnerNullableTest = { Kaboom : string } -type NullableTest = { Inner : InnerNullableTest } +type NullableTest = { + Inner : InnerNullableTest + InnerPartialSuccess : InnerNullableTest +} [] let ``Execution handles errors: properly propagates errors`` () = - let InnerObj = + let InnerObjType = Define.Object( "Inner", [ Define.Field("kaboom", StringType, fun _ x -> x.Kaboom) ]) + let InnerPartialSuccessObjType = + let resolvePartialSuccess (ctx : ResolveFieldContext) (_ : InnerNullableTest) = + ctx.AddError { new IGQLError with member _.Message = "Some non-critical error" } + "Success" + Define.Object( + "InnerPartialSuccess", [ + Define.Field("kaboom", StringType, resolvePartialSuccess) + ]) let schema = Schema(Define.Object( - "Type", [ - Define.Field("inner", Nullable InnerObj, fun _ x -> Some x.Inner) - ])) + "Type", [ + Define.Field("inner", Nullable InnerObjType, fun _ x -> Some x.Inner) + Define.Field("partialSuccess", Nullable InnerPartialSuccessObjType, fun _ x -> Some x.InnerPartialSuccess) + ])) let expectedData = NameValueLookup.ofList [ "inner", null + "partialSuccess", NameValueLookup.ofList [ + "kaboom", "Success" + ] ] let expectedErrors = [ GQLProblemDetails.CreateWithKind ("Non-Null field kaboom resolved as a null!", Execution, [ box "inner"; "kaboom" ]) + GQLProblemDetails.CreateWithKind ("Some non-critical error", Execution, [ box "partialSuccess"; "kaboom" ]) ] - let result = sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } }", { Inner = { Kaboom = null } }) + let result = + let variables = { Inner = { Kaboom = null }; InnerPartialSuccess = { Kaboom = "Success" } } + sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } partialSuccess { kaboom } }", variables) ensureDirect result <| fun data errors -> result.DocumentId |> notEquals Unchecked.defaultof data |> equals (upcast expectedData) errors |> equals expectedErrors +// TODO: Add other tests with possible error cases +// 1. Add additional error and raise an exception in a nullable GraphQL field resolver +// 2. Add additional error and return None from a nullable GraphQL field resolver +// 3. Add additional error and raise an exception in a non-nullable GraphQL field resolver +// 4. Add additional error and return null from a non-nullable GraphQL field resolver +// Covered // 5.1. Add additional error and return value from a non-nullable GraphQL field resolver +// 5.2. Add additional error and raise an exception in a non-nullable GraphQL field resolver + [] let ``Execution handles errors: exceptions`` () = let schema = Schema(Define.Object( - "Type", [ - Define.Field("a", StringType, fun _ _ -> failwith "Resolver Error!") - ])) + "Type", [ + Define.Field("a", StringType, fun _ _ -> failwith "Resolver Error!") + ])) let expectedError = GQLProblemDetails.CreateWithKind ("Resolver Error!", Execution, [ box "a" ]) let result = sync <| Executor(schema).AsyncExecute("query Test { a }", ()) ensureRequestError result <| fun [ error ] -> error |> equals expectedError From 8c5a23c714ea56deed3d953dd87194632b83ebd2 Mon Sep 17 00:00:00 2001 From: VectorTetra Date: Tue, 4 Feb 2025 16:42:02 +0200 Subject: [PATCH 2/5] Added tests to cover all cases of additional errors in `executeResolvers`/`resolveWith` --- src/FSharp.Data.GraphQL.Server/Execution.fs | 7 +- .../ExecutionTests.fs | 136 ++++++++++++++++-- 2 files changed, 131 insertions(+), 12 deletions(-) diff --git a/src/FSharp.Data.GraphQL.Server/Execution.fs b/src/FSharp.Data.GraphQL.Server/Execution.fs index b1406e36..bb152453 100644 --- a/src/FSharp.Data.GraphQL.Server/Execution.fs +++ b/src/FSharp.Data.GraphQL.Server/Execution.fs @@ -364,7 +364,7 @@ and private executeResolvers (ctx : ResolveFieldContext) (path : FieldPath) (par /// This handles all null resolver errors/error propagation. let resolveWith (ctx : ResolveFieldContext) (onSuccess : ResolveFieldContext -> FieldPath -> obj -> obj -> AsyncVal>>) : AsyncVal>> = asyncVal { let! resolved = value |> AsyncVal.rescue path ctx.Schema.ParseError - let additionalErrs = + let additionalErrs = match ctx.Context.Errors.TryGetValue ctx with | true, errors -> errors @@ -373,10 +373,9 @@ and private executeResolvers (ctx : ResolveFieldContext) (path : FieldPath) (par | false, _ -> [] match resolved with | Error errs when ctx.ExecutionInfo.IsNullable -> return Ok (KeyValuePair(name, null), None, errs @ additionalErrs) - | Ok None when ctx.ExecutionInfo.IsNullable -> - return Ok (KeyValuePair(name, null), None, additionalErrs) + | Ok None when ctx.ExecutionInfo.IsNullable -> return Ok (KeyValuePair(name, null), None, additionalErrs) | Error errs -> return Error (errs @ additionalErrs) - | Ok None -> return Error (nullResolverError name path ctx) + | Ok None -> return Error ((nullResolverError name path ctx) @ additionalErrs) | Ok (Some v) -> match! onSuccess ctx path parent v with | Ok (res, deferred, errs) -> return Ok (res, deferred, errs @ additionalErrs) diff --git a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs index 21da15dc..86b86773 100644 --- a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs @@ -407,6 +407,7 @@ let ``Execution handles errors: properly propagates errors`` () = Define.Field("kaboom", StringType, fun _ x -> x.Kaboom) ]) let InnerPartialSuccessObjType = + // executeResolvers/resolveWith, case 5 let resolvePartialSuccess (ctx : ResolveFieldContext) (_ : InnerNullableTest) = ctx.AddError { new IGQLError with member _.Message = "Some non-critical error" } "Success" @@ -439,14 +440,6 @@ let ``Execution handles errors: properly propagates errors`` () = data |> equals (upcast expectedData) errors |> equals expectedErrors -// TODO: Add other tests with possible error cases -// 1. Add additional error and raise an exception in a nullable GraphQL field resolver -// 2. Add additional error and return None from a nullable GraphQL field resolver -// 3. Add additional error and raise an exception in a non-nullable GraphQL field resolver -// 4. Add additional error and return null from a non-nullable GraphQL field resolver -// Covered // 5.1. Add additional error and return value from a non-nullable GraphQL field resolver -// 5.2. Add additional error and raise an exception in a non-nullable GraphQL field resolver - [] let ``Execution handles errors: exceptions`` () = let schema = @@ -484,3 +477,130 @@ let ``Execution handles errors: nullable list fields`` () = result.DocumentId |> notEquals Unchecked.defaultof data |> equals (upcast expectedData) errors |> equals expectedErrors + + +[] +let ``Execution handles errors: additional error added when exception is rised in a nullable field resolver`` () = + let InnerNullableExceptionObjType = + // executeResolvers/resolveWith, case 1 + let resolveWithException (ctx : ResolveFieldContext) (_ : InnerNullableTest) : string option = + ctx.AddError { new IGQLError with member _.Message = "Non-critical error" } + raise (System.Exception "Unexpected error") + Define.Object( + "InnerNullableException", [ + Define.Field("kaboom", Nullable StringType, resolve = resolveWithException) + ]) + let schema = + Schema(Define.Object( + "Type", [ + Define.Field("inner", Nullable InnerNullableExceptionObjType, fun _ x -> Some x.Inner) + ])) + let expectedData = + NameValueLookup.ofList [ + "inner", NameValueLookup.ofList [ + "kaboom", null + ] + ] + let expectedErrors = + [ + GQLProblemDetails.CreateWithKind ("Unexpected error", Execution, [ box "inner"; "kaboom" ]) + GQLProblemDetails.CreateWithKind ("Non-critical error", Execution, [ box "inner"; "kaboom" ]) + ] + let result = + let variables = { Inner = { Kaboom = null }; InnerPartialSuccess = { Kaboom = "Success" } } + sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } }", variables) + ensureDirect result <| fun data errors -> + result.DocumentId |> notEquals Unchecked.defaultof + data |> equals (upcast expectedData) + errors |> equals expectedErrors + +[] +let ``Execution handles errors: additional error added when None returned from a nullable field resolver`` () = + let InnerNullableNoneObjType = + // executeResolvers/resolveWith, case 2 + let resolveWithNone (ctx : ResolveFieldContext) (_ : InnerNullableTest) : string option = + ctx.AddError { new IGQLError with member _.Message = "Non-critical error" } + None + Define.Object( + "InnerNullableException", [ + Define.Field("kaboom", Nullable StringType, resolve = resolveWithNone) + ]) + let schema = + Schema(Define.Object( + "Type", [ + Define.Field("inner", Nullable InnerNullableNoneObjType, fun _ x -> Some x.Inner) + ])) + let expectedData = + NameValueLookup.ofList [ + "inner", NameValueLookup.ofList [ + "kaboom", null + ] + ] + let expectedErrors = + [ + GQLProblemDetails.CreateWithKind ("Non-critical error", Execution, [ box "inner"; "kaboom" ]) + ] + let result = + let variables = { Inner = { Kaboom = null }; InnerPartialSuccess = { Kaboom = "Success" } } + sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } }", variables) + ensureDirect result <| fun data errors -> + result.DocumentId |> notEquals Unchecked.defaultof + data |> equals (upcast expectedData) + errors |> equals expectedErrors + +[] +let ``Execution handles errors: additional error added when exception is rised in a non-nullable field resolver`` () = + let InnerNonNullableExceptionObjType = + // executeResolvers/resolveWith, case 3 + let resolveWithException (ctx : ResolveFieldContext) (_ : InnerNullableTest) : string = + ctx.AddError { new IGQLError with member _.Message = "Non-critical error" } + raise (System.Exception "Fatal error") + Define.Object( + "InnerNonNullableException", [ + Define.Field("kaboom", StringType, resolve = resolveWithException) + ]) + let schema = + Schema(Define.Object( + "Type", [ + Define.Field("inner", InnerNonNullableExceptionObjType, fun _ x -> x.Inner) + ])) + + let expectedErrors = + [ + GQLProblemDetails.CreateWithKind ("Fatal error", Execution, [ box "inner"; "kaboom" ]) + GQLProblemDetails.CreateWithKind ("Non-critical error", Execution, [ box "inner"; "kaboom" ]) + ] + let result = + let variables = { Inner = { Kaboom = "Explosion" }; InnerPartialSuccess = { Kaboom = "Success" } } + sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } }", variables) + ensureRequestError result <| fun errors -> + result.DocumentId |> notEquals Unchecked.defaultof + errors |> equals expectedErrors + +[] +let ``Execution handles errors: additional error added and when null returned from a non-nullable field resolver`` () = + let InnerNonNullableNullObjType = + // executeResolvers/resolveWith, case 4 + let resolveWithNull (ctx : ResolveFieldContext) (_ : InnerNullableTest) : string = + ctx.AddError { new IGQLError with member _.Message = "Non-critical error" } + null + Define.Object( + "InnerNonNullableNull", [ + Define.Field("kaboom", StringType, resolveWithNull) + ]) + let schema = + Schema(Define.Object( + "Type", [ + Define.Field("inner", InnerNonNullableNullObjType, fun _ x -> x.Inner) + ])) + let expectedErrors = + [ + GQLProblemDetails.CreateWithKind ("Non-Null field kaboom resolved as a null!", Execution, [ box "inner"; "kaboom" ]) + GQLProblemDetails.CreateWithKind ("Non-critical error", Execution, [ box "inner"; "kaboom" ]) + ] + let result = + let variables = { Inner = { Kaboom = "Explosion" }; InnerPartialSuccess = { Kaboom = "Success" } } + sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } }", variables) + ensureRequestError result <| fun errors -> + result.DocumentId |> notEquals Unchecked.defaultof + errors |> equals expectedErrors From 0bd5eda775af171176c79e947db23a9147177210 Mon Sep 17 00:00:00 2001 From: VectorTetra Date: Tue, 4 Feb 2025 20:59:57 +0200 Subject: [PATCH 3/5] PR fixes --- src/FSharp.Data.GraphQL.Server/ErrorTypes.fs | 6 ++++-- src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs | 7 +++++++ src/FSharp.Data.GraphQL.Server/Execution.fs | 10 +++++----- .../FSharp.Data.GraphQL.Server.fsproj | 2 +- src/FSharp.Data.GraphQL.Shared/TypeSystem.fs | 4 ++++ tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs | 14 +++++++------- 6 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/FSharp.Data.GraphQL.Server/ErrorTypes.fs b/src/FSharp.Data.GraphQL.Server/ErrorTypes.fs index f4defba8..316ddb37 100644 --- a/src/FSharp.Data.GraphQL.Server/ErrorTypes.fs +++ b/src/FSharp.Data.GraphQL.Server/ErrorTypes.fs @@ -4,8 +4,10 @@ namespace FSharp.Data.GraphQL open System open System.Collections.Generic open FsToolkit.ErrorHandling +open FSharp.Data.GraphQL.Errors open FSharp.Data.GraphQL.Types + type InputSource = | Variable of VarDef : VarDef | Argument of ArgDef : InputFieldDef @@ -38,7 +40,7 @@ type internal CoercionError = { yield KeyValuePair (CustomErrorFields.Kind, this.ErrorKind |> box) match this.Path with | [] -> () - | path -> yield KeyValuePair (CustomErrorFields.Path, path |> List.rev |> box) + | path -> yield KeyValuePair (CustomErrorFields.Path, normalizedPathToObj(path)) match this.InputSource with | Variable varDef -> @@ -87,7 +89,7 @@ type internal CoercionErrorWrapper = { yield KeyValuePair (CustomErrorFields.Kind, this.ErrorKind |> box) match this.Path with | [] -> () - | path -> yield KeyValuePair (CustomErrorFields.Path, path |> List.rev |> box) + | path -> yield KeyValuePair (CustomErrorFields.Path, normalizedPathToObj(path)) match this.InputSource with | Variable varDef -> diff --git a/src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs b/src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs index ed4ed327..edc28b86 100644 --- a/src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs +++ b/src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs @@ -114,3 +114,10 @@ let splitSeqErrorsList (items: Result<'t, IGQLError list> list) = else let values = items |> getSeqValuesList Ok values + +let internal normalizedPath(fieldPath : FieldPath) = + fieldPath |> List.rev + +let internal normalizedPathToObj(fieldPath : FieldPath) = + fieldPath |> List.rev |> box + diff --git a/src/FSharp.Data.GraphQL.Server/Execution.fs b/src/FSharp.Data.GraphQL.Server/Execution.fs index bb152453..ced70583 100644 --- a/src/FSharp.Data.GraphQL.Server/Execution.fs +++ b/src/FSharp.Data.GraphQL.Server/Execution.fs @@ -101,7 +101,7 @@ let private createFieldContext objdef argDefs ctx (info: ExecutionInfo) (path : Schema = ctx.Schema Args = args Variables = ctx.Variables - Path = path |> List.rev } + Path = normalizedPath path } } let private resolveField (execute: ExecuteField) (ctx: ResolveFieldContext) (parentValue: obj) = @@ -136,7 +136,7 @@ let private raiseErrors errs = AsyncVal.wrap <| Error errs /// Given an error e, call ParseError in the given context's Schema to convert it into /// a list of one or more IGQLErrors, then convert those /// to a list of GQLProblemDetails. -let private resolverError path ctx e = ctx.Schema.ParseError path e |> List.map (GQLProblemDetails.OfFieldExecutionError (path |> List.rev)) +let private resolverError path ctx e = ctx.Schema.ParseError path e |> List.map (GQLProblemDetails.OfFieldExecutionError (normalizedPath path)) // Helper functions for generating more specific GQLProblemDetails. let private nullResolverError name path ctx = resolverError path ctx (GQLMessageException <| sprintf "Non-Null field %s resolved as a null!" name) let private coercionError value tyName path ctx = resolverError path ctx (GQLMessageException <| sprintf "Value '%O' could not be coerced to scalar %s" value tyName) @@ -148,7 +148,7 @@ let private streamListError name tyName path ctx = resolverError path ctx (GQLMe let private resolved name v : AsyncVal>> = KeyValuePair(name, box v) |> ResolverResult.data |> AsyncVal.wrap let deferResults path (res : ResolverResult) : IObservable = - let formattedPath = path |> List.rev + let formattedPath = normalizedPath path match res with | Ok (data, deferred, errs) -> let deferredData = @@ -368,7 +368,7 @@ and private executeResolvers (ctx : ResolveFieldContext) (path : FieldPath) (par match ctx.Context.Errors.TryGetValue ctx with | true, errors -> errors - |> Seq.map (GQLProblemDetails.OfFieldExecutionError (path |> List.rev)) + |> Seq.map (GQLProblemDetails.OfFieldExecutionError (normalizedPath path)) |> Seq.toList | false, _ -> [] match resolved with @@ -462,7 +462,7 @@ let private executeQueryOrMutation (resultSet: (string * ExecutionInfo) []) (ctx Schema = ctx.Schema Args = args Variables = ctx.Variables - Path = path |> List.rev } + Path = normalizedPath path } let execute = ctx.FieldExecuteMap.GetExecute(ctx.ExecutionPlan.RootDef.Name, info.Definition.Name) asyncVal { let! result = diff --git a/src/FSharp.Data.GraphQL.Server/FSharp.Data.GraphQL.Server.fsproj b/src/FSharp.Data.GraphQL.Server/FSharp.Data.GraphQL.Server.fsproj index 3ad275cc..e2576040 100644 --- a/src/FSharp.Data.GraphQL.Server/FSharp.Data.GraphQL.Server.fsproj +++ b/src/FSharp.Data.GraphQL.Server/FSharp.Data.GraphQL.Server.fsproj @@ -36,9 +36,9 @@ + - diff --git a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs index e632db87..4ac6bb68 100644 --- a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs +++ b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs @@ -936,6 +936,10 @@ and ResolveFieldContext = { member this.AddError (error : IGQLError) = this.Context.AddError (this, error) + /// Remembers an error, so it can be included in the final response. + member this.AddError (errorMessage : string) = + this.Context.AddError (this, { new IGQLError with member _.Message = errorMessage } ) + /// Tries to find an argument by provided name. member this.TryArg (name : string) : 't option = match Map.tryFind name this.Args with diff --git a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs index 86b86773..221539cc 100644 --- a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs @@ -410,7 +410,7 @@ let ``Execution handles errors: properly propagates errors`` () = // executeResolvers/resolveWith, case 5 let resolvePartialSuccess (ctx : ResolveFieldContext) (_ : InnerNullableTest) = ctx.AddError { new IGQLError with member _.Message = "Some non-critical error" } - "Success" + "Yes, Rico, Kaboom" Define.Object( "InnerPartialSuccess", [ Define.Field("kaboom", StringType, resolvePartialSuccess) @@ -425,7 +425,7 @@ let ``Execution handles errors: properly propagates errors`` () = NameValueLookup.ofList [ "inner", null "partialSuccess", NameValueLookup.ofList [ - "kaboom", "Success" + "kaboom", "Yes, Rico, Kaboom" ] ] let expectedErrors = [ @@ -433,7 +433,7 @@ let ``Execution handles errors: properly propagates errors`` () = GQLProblemDetails.CreateWithKind ("Some non-critical error", Execution, [ box "partialSuccess"; "kaboom" ]) ] let result = - let variables = { Inner = { Kaboom = null }; InnerPartialSuccess = { Kaboom = "Success" } } + let variables = { Inner = { Kaboom = null }; InnerPartialSuccess = { Kaboom = "Yes, Rico, Kaboom" } } sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } partialSuccess { kaboom } }", variables) ensureDirect result <| fun data errors -> result.DocumentId |> notEquals Unchecked.defaultof @@ -507,7 +507,7 @@ let ``Execution handles errors: additional error added when exception is rised i GQLProblemDetails.CreateWithKind ("Non-critical error", Execution, [ box "inner"; "kaboom" ]) ] let result = - let variables = { Inner = { Kaboom = null }; InnerPartialSuccess = { Kaboom = "Success" } } + let variables = { Inner = { Kaboom = null }; InnerPartialSuccess = { Kaboom = "Yes, Rico, Kaboom" } } sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } }", variables) ensureDirect result <| fun data errors -> result.DocumentId |> notEquals Unchecked.defaultof @@ -541,7 +541,7 @@ let ``Execution handles errors: additional error added when None returned from a GQLProblemDetails.CreateWithKind ("Non-critical error", Execution, [ box "inner"; "kaboom" ]) ] let result = - let variables = { Inner = { Kaboom = null }; InnerPartialSuccess = { Kaboom = "Success" } } + let variables = { Inner = { Kaboom = null }; InnerPartialSuccess = { Kaboom = "Yes, Rico, Kaboom" } } sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } }", variables) ensureDirect result <| fun data errors -> result.DocumentId |> notEquals Unchecked.defaultof @@ -571,7 +571,7 @@ let ``Execution handles errors: additional error added when exception is rised i GQLProblemDetails.CreateWithKind ("Non-critical error", Execution, [ box "inner"; "kaboom" ]) ] let result = - let variables = { Inner = { Kaboom = "Explosion" }; InnerPartialSuccess = { Kaboom = "Success" } } + let variables = { Inner = { Kaboom = "Yes, Rico, Kaboom" }; InnerPartialSuccess = { Kaboom = "Yes, Rico, Kaboom" } } sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } }", variables) ensureRequestError result <| fun errors -> result.DocumentId |> notEquals Unchecked.defaultof @@ -599,7 +599,7 @@ let ``Execution handles errors: additional error added and when null returned fr GQLProblemDetails.CreateWithKind ("Non-critical error", Execution, [ box "inner"; "kaboom" ]) ] let result = - let variables = { Inner = { Kaboom = "Explosion" }; InnerPartialSuccess = { Kaboom = "Success" } } + let variables = { Inner = { Kaboom = "Yes, Rico, Kaboom" }; InnerPartialSuccess = { Kaboom = "Yes, Rico, Kaboom" } } sync <| Executor(schema).AsyncExecute("query Example { inner { kaboom } }", variables) ensureRequestError result <| fun errors -> result.DocumentId |> notEquals Unchecked.defaultof From c8024764fd51cc1a146409d5a6630b71d99bae65 Mon Sep 17 00:00:00 2001 From: VectorTetra Date: Wed, 5 Feb 2025 12:21:01 +0200 Subject: [PATCH 4/5] PR fixes --- src/FSharp.Data.GraphQL.Server/ErrorTypes.fs | 4 ++-- src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs | 6 +----- src/FSharp.Data.GraphQL.Server/Execution.fs | 10 +++++----- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/FSharp.Data.GraphQL.Server/ErrorTypes.fs b/src/FSharp.Data.GraphQL.Server/ErrorTypes.fs index 316ddb37..eaa7778c 100644 --- a/src/FSharp.Data.GraphQL.Server/ErrorTypes.fs +++ b/src/FSharp.Data.GraphQL.Server/ErrorTypes.fs @@ -40,7 +40,7 @@ type internal CoercionError = { yield KeyValuePair (CustomErrorFields.Kind, this.ErrorKind |> box) match this.Path with | [] -> () - | path -> yield KeyValuePair (CustomErrorFields.Path, normalizedPathToObj(path)) + | path -> yield KeyValuePair (CustomErrorFields.Path, normalizeErrorPath path) match this.InputSource with | Variable varDef -> @@ -89,7 +89,7 @@ type internal CoercionErrorWrapper = { yield KeyValuePair (CustomErrorFields.Kind, this.ErrorKind |> box) match this.Path with | [] -> () - | path -> yield KeyValuePair (CustomErrorFields.Path, normalizedPathToObj(path)) + | path -> yield KeyValuePair (CustomErrorFields.Path, normalizeErrorPath path) match this.InputSource with | Variable varDef -> diff --git a/src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs b/src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs index edc28b86..6dbe54f7 100644 --- a/src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs +++ b/src/FSharp.Data.GraphQL.Server/ErrorsProcessing.fs @@ -115,9 +115,5 @@ let splitSeqErrorsList (items: Result<'t, IGQLError list> list) = let values = items |> getSeqValuesList Ok values -let internal normalizedPath(fieldPath : FieldPath) = +let internal normalizeErrorPath(fieldPath : FieldPath) = fieldPath |> List.rev - -let internal normalizedPathToObj(fieldPath : FieldPath) = - fieldPath |> List.rev |> box - diff --git a/src/FSharp.Data.GraphQL.Server/Execution.fs b/src/FSharp.Data.GraphQL.Server/Execution.fs index ced70583..17e23c13 100644 --- a/src/FSharp.Data.GraphQL.Server/Execution.fs +++ b/src/FSharp.Data.GraphQL.Server/Execution.fs @@ -101,7 +101,7 @@ let private createFieldContext objdef argDefs ctx (info: ExecutionInfo) (path : Schema = ctx.Schema Args = args Variables = ctx.Variables - Path = normalizedPath path } + Path = normalizeErrorPath path } } let private resolveField (execute: ExecuteField) (ctx: ResolveFieldContext) (parentValue: obj) = @@ -136,7 +136,7 @@ let private raiseErrors errs = AsyncVal.wrap <| Error errs /// Given an error e, call ParseError in the given context's Schema to convert it into /// a list of one or more IGQLErrors, then convert those /// to a list of GQLProblemDetails. -let private resolverError path ctx e = ctx.Schema.ParseError path e |> List.map (GQLProblemDetails.OfFieldExecutionError (normalizedPath path)) +let private resolverError path ctx e = ctx.Schema.ParseError path e |> List.map (GQLProblemDetails.OfFieldExecutionError (normalizeErrorPath path)) // Helper functions for generating more specific GQLProblemDetails. let private nullResolverError name path ctx = resolverError path ctx (GQLMessageException <| sprintf "Non-Null field %s resolved as a null!" name) let private coercionError value tyName path ctx = resolverError path ctx (GQLMessageException <| sprintf "Value '%O' could not be coerced to scalar %s" value tyName) @@ -148,7 +148,7 @@ let private streamListError name tyName path ctx = resolverError path ctx (GQLMe let private resolved name v : AsyncVal>> = KeyValuePair(name, box v) |> ResolverResult.data |> AsyncVal.wrap let deferResults path (res : ResolverResult) : IObservable = - let formattedPath = normalizedPath path + let formattedPath = normalizeErrorPath path match res with | Ok (data, deferred, errs) -> let deferredData = @@ -368,7 +368,7 @@ and private executeResolvers (ctx : ResolveFieldContext) (path : FieldPath) (par match ctx.Context.Errors.TryGetValue ctx with | true, errors -> errors - |> Seq.map (GQLProblemDetails.OfFieldExecutionError (normalizedPath path)) + |> Seq.map (GQLProblemDetails.OfFieldExecutionError (normalizeErrorPath path)) |> Seq.toList | false, _ -> [] match resolved with @@ -462,7 +462,7 @@ let private executeQueryOrMutation (resultSet: (string * ExecutionInfo) []) (ctx Schema = ctx.Schema Args = args Variables = ctx.Variables - Path = normalizedPath path } + Path = normalizeErrorPath path } let execute = ctx.FieldExecuteMap.GetExecute(ctx.ExecutionPlan.RootDef.Name, info.Definition.Name) asyncVal { let! result = From 9a658ef46f18170ee161726df11b8c769bc54717 Mon Sep 17 00:00:00 2001 From: VectorTetra Date: Wed, 5 Feb 2025 13:13:52 +0200 Subject: [PATCH 5/5] PR fixes --- src/FSharp.Data.GraphQL.Shared/TypeSystem.fs | 2 +- tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs index 4ac6bb68..502cceca 100644 --- a/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs +++ b/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs @@ -905,7 +905,7 @@ and ExecutionContext = { this.Errors.AddOrUpdate( fieldContext, addValueFactory = (fun _ -> ConcurrentBag (Seq.singleton error)), - updateValueFactory = (fun _ (bag)-> bag.Add error; bag) + updateValueFactory = (fun _ bag -> bag.Add error; bag) ) |> ignore diff --git a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs index 221539cc..244bef54 100644 --- a/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs +++ b/tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs @@ -564,7 +564,6 @@ let ``Execution handles errors: additional error added when exception is rised i "Type", [ Define.Field("inner", InnerNonNullableExceptionObjType, fun _ x -> x.Inner) ])) - let expectedErrors = [ GQLProblemDetails.CreateWithKind ("Fatal error", Execution, [ box "inner"; "kaboom" ])