Skip to content

Commit 5d608ce

Browse files
xperiandriclaude
xperiandri
and
claude
authored
Make GraphQLRequestHandler extensible via virtual members (#602)
Turn the introspection/operation/dispatch steps of `GraphQLRequestHandler` from private `let`-bound closures into `abstract`/`default` members, so a derived handler can override a single step (e.g. per-request introspection, or custom operation execution) without reimplementing the whole class: - `ExecuteIntrospectionQuery` and `ExecuteOperation` are now virtual, each resolving `options.CurrentValue.SchemaExecutor` itself instead of receiving it as a parameter (dropping the now-pointless generic `<'Root>` from `ExecuteOperation`, which used to shadow the class's own `'Root` without being constrained by it). - `CheckOperationType` becomes a public member (was a private local function), with a `<remarks>` note that it consumes the request body, so calling it more than once on the same request fails to bind on the second call. - `HandleAsync` is now virtual too, and dispatches to the other members through `handler` instead of closing over local functions. Also move the class-level `<summary>` from the primary constructor's doc comment to a `///` line on the type itself, and give the constructor its own "Initializes a new instance..." comment - the constructor comment documented the class, not the constructor. Add `RequestHandlerExtensibilityTests.fs` to prove `HandleAsync` actually reaches these members through virtual dispatch rather than closed-over `let` functions, so the extensibility contract can't silently regress: - `RecordingHandler` overrides `ExecuteIntrospectionQuery`/ `ExecuteOperation`, records each call, then defers to the base implementation. - `SentinelHandler` overrides only `HandleAsync`, to check `HandleAsync` itself is reached through a `GraphQLRequestHandler`- typed reference. - `createHandler` wires a handler through the real `AddGraphQL` DI registration (`ServiceCollection` + `AddGraphQL` + `CreateScope`), against a `DefaultHttpContext` with a given method/body, using the existing `AspNetCore.TestSchema`. It resolves the handler as the base `GraphQLRequestHandler<Root>` type before calling `HandleAsync`, so a regression to non-virtual dispatch cannot pass unnoticed. - Four tests cover: GET (introspection, no body), POST introspection query, POST operation query, and the `HandleAsync` override. Verified the tests actually guard the contract: temporarily bypassed the virtual `ExecuteIntrospectionQuery` call in `HandleAsync` (inlining its body directly), which failed the two introspection tests with "Expected: 1, Actual: 0" while leaving the operation and sentinel tests green, then reverted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 147cb7a commit 5d608ce

3 files changed

Lines changed: 173 additions & 14 deletions

File tree

‎src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs‎

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@ open FsToolkit.ErrorHandling
1616
open FSharp.Data.GraphQL.Server
1717
open FSharp.Data.GraphQL.Shared
1818

19+
/// Handles GraphQL requests using a provided root schema.
1920
type DefaultGraphQLRequestHandler<'Root>
2021
/// <summary>
21-
/// Handles GraphQL requests using a provided root schema.
22+
/// Initializes a new instance of the <see cref="DefaultGraphQLRequestHandler{T}"/> class.
2223
/// </summary>
2324
/// <param name="httpContextAccessor">The accessor to the current HTTP context.</param>
2425
/// <param name="options">The options monitor for GraphQL options.</param>
@@ -30,9 +31,10 @@ type DefaultGraphQLRequestHandler<'Root>
3031
) =
3132
inherit GraphQLRequestHandler<'Root> (httpContextAccessor, options, logger)
3233

34+
/// Provides logic to parse and execute GraphQL requests.
3335
and [<AbstractClass>] GraphQLRequestHandler<'Root>
3436
/// <summary>
35-
/// Provides logic to parse and execute GraphQL requests.
37+
/// Initializes a new instance of the <see cref="GraphQLRequestHandler{T}"/> class.
3638
/// </summary>
3739
/// <param name="httpContextAccessor">The accessor to the current HTTP context.</param>
3840
/// <param name="options">The options monitor for GraphQL options.</param>
@@ -134,7 +136,10 @@ and [<AbstractClass>] GraphQLRequestHandler<'Root>
134136
}
135137

136138
/// Execute default or custom introspection query
137-
let executeIntrospectionQuery (executor : Executor<_>) (ast : Ast.Document voption) : Task<IResult> = task {
139+
abstract ExecuteIntrospectionQuery : ast : Ast.Document voption -> Task<IResult>
140+
141+
default _.ExecuteIntrospectionQuery (ast : Ast.Document voption) : Task<IResult> = task {
142+
let executor = options.CurrentValue.SchemaExecutor
138143
let! result =
139144
match ast with
140145
| ValueNone -> executor.AsyncExecute (IntrospectionQuery.Definition, getInputContext)
@@ -144,12 +149,18 @@ and [<AbstractClass>] GraphQLRequestHandler<'Root>
144149
return (TypedResults.Ok response) :> IResult
145150
}
146151

147-
/// <summary>Check if the request is an introspection query
148-
/// by first checking on such properties as `GET` method or `empty request body`
152+
/// <summary>
153+
/// Check if the request is an introspection query
154+
/// by first checking on such properties as <c>GET</c> method or <c>empty request body</c>
149155
/// and lastly by parsing document AST for introspection operation definition.
150156
/// </summary>
157+
/// <remarks>
158+
/// This consumes the request body: after JSON binding, the body stream position stays at its end,
159+
/// so calling this more than once (e.g. once from a derived handler and again from
160+
/// <see cref="HandleAsync"/>) will fail to bind the request on the second call.
161+
/// </remarks>
151162
/// <returns>Result of check of <see cref="OperationType"/></returns>
152-
let checkOperationType () = taskResult {
163+
member _.CheckOperationType () = taskResult {
153164

154165
let checkAnonymousFieldsOnly (ctx : HttpContext) = taskResult {
155166
let! gqlRequest = ctx.TryBindJsonAsync<GQLRequestContent>(GQLRequestContent.expectedJSON)
@@ -203,10 +214,10 @@ and [<AbstractClass>] GraphQLRequestHandler<'Root>
203214
return! checkAnonymousFieldsOnly ctx
204215
}
205216

206-
abstract ExecuteOperation<'Root> : executor : Executor<'Root> * content : ParsedGQLQueryRequestContent -> Task<IResult>
207-
208217
/// Execute the operation for given request
209-
default _.ExecuteOperation<'Root> (executor : Executor<'Root>, content) = task {
218+
abstract ExecuteOperation : content : ParsedGQLQueryRequestContent -> Task<IResult>
219+
220+
default _.ExecuteOperation (content) = task {
210221

211222
let operationName =
212223
content.OperationName
@@ -228,6 +239,7 @@ and [<AbstractClass>] GraphQLRequestHandler<'Root>
228239
let root = options.CurrentValue.RootFactory ctx
229240

230241
let! result =
242+
let executor = options.CurrentValue.SchemaExecutor
231243
Async.StartImmediateAsTask (
232244
executor.AsyncExecute (content.Ast, getInputContext, root, ?variables = variables, ?operationName = operationName),
233245
cancellationToken = ctx.RequestAborted
@@ -237,12 +249,14 @@ and [<AbstractClass>] GraphQLRequestHandler<'Root>
237249
return (TypedResults.Ok response) :> IResult
238250
}
239251

240-
member handler.HandleAsync () : Task<Result<IResult, IResult>> = taskResult {
252+
/// Handle the request and return the result
253+
abstract HandleAsync : unit -> Task<Result<IResult, IResult>>
254+
255+
default handler.HandleAsync () : Task<Result<IResult, IResult>> = taskResult {
241256
if ctx.RequestAborted.IsCancellationRequested then
242257
return TypedResults.Empty
243258
else
244-
let executor = options.CurrentValue.SchemaExecutor
245-
match! checkOperationType () with
246-
| IntrospectionQuery optionalAstDocument -> return! executeIntrospectionQuery executor optionalAstDocument
247-
| OperationQuery content -> return! handler.ExecuteOperation (executor, content)
259+
match! handler.CheckOperationType () with
260+
| IntrospectionQuery optionalAstDocument -> return! handler.ExecuteIntrospectionQuery optionalAstDocument
261+
| OperationQuery content -> return! handler.ExecuteOperation (content)
248262
}

‎tests/FSharp.Data.GraphQL.Tests/AspNetCore/RequestHandlerExtensibilityTests.fs‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
module FSharp.Data.GraphQL.Tests.AspNetCore.RequestHandlerExtensibilityTests
2+
3+
open System
4+
open System.IO
5+
open System.Text
6+
open System.Text.Json
7+
open System.Threading.Tasks
8+
open Microsoft.AspNetCore.Http
9+
open Microsoft.Extensions.DependencyInjection
10+
open Microsoft.Extensions.Logging
11+
open Microsoft.Extensions.Options
12+
open Xunit
13+
14+
open FSharp.Data.GraphQL
15+
open FSharp.Data.GraphQL.Server.AspNetCore
16+
open FSharp.Data.GraphQL.Shared
17+
18+
/// A handler that records every call to the overridable execution steps, then defers to the base
19+
/// implementation. Asserting on the recorded calls proves `HandleAsync` reaches these members through
20+
/// virtual dispatch rather than through closed-over `let` functions, which no override could intercept.
21+
type private RecordingHandler
22+
(httpContextAccessor : IHttpContextAccessor, options : IOptionsMonitor<GraphQLOptions<Root>>, logger : ILogger<RecordingHandler>) =
23+
inherit GraphQLRequestHandler<Root> (httpContextAccessor, options, logger)
24+
25+
let introspectionCalls = ResizeArray<Ast.Document voption>()
26+
let operationCalls = ResizeArray<ParsedGQLQueryRequestContent>()
27+
28+
member _.IntrospectionCalls = introspectionCalls
29+
member _.OperationCalls = operationCalls
30+
31+
override _.ExecuteIntrospectionQuery (ast : Ast.Document voption) : Task<IResult> =
32+
introspectionCalls.Add ast
33+
base.ExecuteIntrospectionQuery ast
34+
35+
override _.ExecuteOperation (content : ParsedGQLQueryRequestContent) : Task<IResult> =
36+
operationCalls.Add content
37+
base.ExecuteOperation content
38+
39+
/// A handler that overrides only `HandleAsync`, to check that `HandleAsync` itself is reached through a
40+
/// `GraphQLRequestHandler` reference rather than always running the base class's own implementation.
41+
type private SentinelHandler
42+
(httpContextAccessor : IHttpContextAccessor, options : IOptionsMonitor<GraphQLOptions<Root>>, logger : ILogger<SentinelHandler>) =
43+
inherit GraphQLRequestHandler<Root> (httpContextAccessor, options, logger)
44+
45+
override _.HandleAsync () : Task<Result<IResult, IResult>> = Task.FromResult (Ok (TypedResults.Ok "sentinel" :> IResult))
46+
47+
let private requestBody (query : string) = JsonSerializer.Serialize {| query = query |}
48+
49+
/// Builds a handler of the given type wired through the real `AddGraphQL` DI registration (the same path
50+
/// a hosted app uses), backed by an `HttpContext` with the given method and JSON body. Returns it both as
51+
/// the base type (to prove `HandleAsync` dispatches virtually) and downcast to the concrete type (to
52+
/// assert on what it recorded), plus the DI scope to dispose once the test is done with it.
53+
let private createHandler<'Handler when 'Handler :> GraphQLRequestHandler<Root> and 'Handler : not struct>
54+
(method : string)
55+
(body : string voption)
56+
: GraphQLRequestHandler<Root> * 'Handler * IDisposable =
57+
let services = ServiceCollection ()
58+
services.AddLogging () |> ignore
59+
services.AddGraphQL<Root, 'Handler>(TestSchema.executor, (fun _ -> { RequestId = "test" }))
60+
|> ignore
61+
let scope = services.BuildServiceProvider().CreateScope()
62+
let serviceProvider = scope.ServiceProvider
63+
64+
let ctx = DefaultHttpContext (RequestServices = serviceProvider)
65+
ctx.Request.Method <- method
66+
ctx.Request.Path <- PathString "/graphql"
67+
68+
body
69+
|> ValueOption.iter (fun json ->
70+
ctx.Request.ContentType <- "application/json"
71+
let bytes = Encoding.UTF8.GetBytes (json : string)
72+
ctx.Request.Body <- new MemoryStream (bytes)
73+
ctx.Request.ContentLength <- int64 bytes.Length)
74+
75+
// The handler captures `httpContextAccessor.HttpContext` in its own constructor, so the accessor
76+
// must carry the request's HttpContext before the handler is resolved from the container.
77+
serviceProvider.GetRequiredService<IHttpContextAccessor>().HttpContext <- ctx
78+
79+
let handler = serviceProvider.GetRequiredService<GraphQLRequestHandler<Root>>()
80+
handler, (handler :?> 'Handler), (scope :> IDisposable)
81+
82+
let private assertOkResponse (outcome : Result<IResult, IResult>) =
83+
match outcome with
84+
| Ok iresult ->
85+
Assert.IsType<Microsoft.AspNetCore.Http.HttpResults.Ok<GQLResponse>> iresult
86+
|> ignore
87+
| Error iresult -> fail $"Expected HandleAsync to succeed, but it returned an error result: %A{iresult}"
88+
89+
[<Fact>]
90+
let ``GET request is dispatched to overridden ExecuteIntrospectionQuery`` () : Task = task {
91+
let handler, recorder, scope = createHandler<RecordingHandler> HttpMethods.Get ValueNone
92+
use _ = scope
93+
94+
let! outcome = handler.HandleAsync ()
95+
96+
Assert.Equal (1, recorder.IntrospectionCalls.Count)
97+
Assert.Equal (ValueNone, recorder.IntrospectionCalls[0])
98+
Assert.Equal (0, recorder.OperationCalls.Count)
99+
assertOkResponse outcome
100+
}
101+
102+
[<Fact>]
103+
let ``POST introspection query is dispatched to overridden ExecuteIntrospectionQuery with the parsed document`` () : Task = task {
104+
let body = requestBody "{ __schema { queryType { name } } }"
105+
let handler, recorder, scope = createHandler<RecordingHandler> HttpMethods.Post (ValueSome body)
106+
use _ = scope
107+
108+
let! outcome = handler.HandleAsync ()
109+
110+
Assert.Equal (1, recorder.IntrospectionCalls.Count)
111+
let ast = wantValueSome recorder.IntrospectionCalls[0]
112+
Assert.False (ast.IsEmpty, "Expected the parsed introspection document to have at least one definition")
113+
Assert.Equal (0, recorder.OperationCalls.Count)
114+
assertOkResponse outcome
115+
}
116+
117+
[<Fact>]
118+
let ``POST operation query is dispatched to overridden ExecuteOperation`` () : Task = task {
119+
let query = "query { hero(id: \"1000\") { id name } }"
120+
let handler, recorder, scope =
121+
createHandler<RecordingHandler> HttpMethods.Post (ValueSome (requestBody query))
122+
use _ = scope
123+
124+
let! outcome = handler.HandleAsync ()
125+
126+
Assert.Equal (0, recorder.IntrospectionCalls.Count)
127+
Assert.Equal (1, recorder.OperationCalls.Count)
128+
Assert.Equal (query, recorder.OperationCalls[0].Query)
129+
assertOkResponse outcome
130+
}
131+
132+
[<Fact>]
133+
let ``Overridden HandleAsync is used when the handler is resolved as GraphQLRequestHandler`` () : Task = task {
134+
let handler, _, scope = createHandler<SentinelHandler> HttpMethods.Get ValueNone
135+
use _ = scope
136+
137+
let! outcome = handler.HandleAsync ()
138+
139+
match outcome with
140+
| Ok iresult ->
141+
let ok = Assert.IsType<Microsoft.AspNetCore.Http.HttpResults.Ok<string>> iresult
142+
Assert.Equal ("sentinel", ok.Value)
143+
| Error iresult -> fail $"Expected HandleAsync to succeed, but it returned an error result: %A{iresult}"
144+
}

‎tests/FSharp.Data.GraphQL.Tests/FSharp.Data.GraphQL.Tests.fsproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@
9595
<Compile Include="AspNetCore/TestSchema.fs" />
9696
<Compile Include="AspNetCore/InvalidMessageTests.fs" />
9797
<Compile Include="AspNetCore/SerializationTests.fs" />
98+
<Compile Include="AspNetCore/RequestHandlerExtensibilityTests.fs" />
9899
<Compile Include="Program.fs" />
99100
</ItemGroup>
100101

0 commit comments

Comments
 (0)