Declare that GraphQL response data can be optional and skippable - #611
Merged
Merged
Conversation
Test Results 9 files 9 suites 13m 21s ⏱️ Results for commit e3883fd. ♻️ This comment has been updated with latest results. |
This was referenced Sep 18, 2026
Change GQLResponse.Data to Skippable<Output voption>, replacing the previous nullable-obj-based Direct/DeferredErrors/SubscriptionErrors payload representation with an explicit voption. Propagate the necessary changes to callers that construct or pattern-match these response cases (Execution.fs, GraphQLRequestHandler.fs) and to the ensureDirect test helper (ErrorHelpers.fs), which is split into ensureDirect/ensureDirectNullData to keep its ValueSome/ValueNone matching meaningful. Includes only the narrowly required adaptation in GraphQLWebsocketMiddleware.fs (pattern-match updates from null/bare data to ValueNone/ValueSome plus ValueOption.toObj conversions at call sites into the still-nullable-based SubscriptionExecutionResult API) so the AspNetCore project continues to compile against this contract change; no other WebSocket lifecycle/serialization behavior is touched. IO.fs is carried whole because its Fantomas record-brace reformatting is entangled with the voption change across every GQLExecutionResult static member; splitting it further would leave a non-compiling intermediate file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
xperiandri
force-pushed
the
graphql-response-data-skippable
branch
from
September 18, 2026 19:54
1b5faa8 to
87b1f27
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Trace logging in GraphQLRequestHandler still serializes the new Output voption wrapper directly, which can produce misleading logs (and may be incompatible with configured JSON converters) rather than the actual GraphQL data payload.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates the server’s GraphQL response contract to make data explicitly skippable vs present-null by changing GQLResponse.Data to Skippable<Output voption>, and then threads that three-state semantics through execution results, transport layers, and tests.
Changes:
- Updated core response/result DU/record shapes (
GQLResponse,GQLResponseContent, deferred/subscription error payloads) to usevoptionfor nullability andSkippablefor presence. - Propagated the new contract through executor/result shaping and ASP.NET Core HTTP + WebSocket response construction.
- Refactored/updated tests and test helpers to assert
ValueNone(present-null) vsSkip(absent) consistently.
File summaries
| File | Description |
|---|---|
| tests/FSharp.Data.GraphQL.Tests/TaskSeqFieldTests.fs | Updates assertions for “root data becomes null” cases using ensureDirectNullData and ValueNone deferred error data. |
| tests/FSharp.Data.GraphQL.Tests/Relay/ConnectionTests.fs | Adjusts Direct pattern matches to require ValueSome data and explicitly fail on ValueNone. |
| tests/FSharp.Data.GraphQL.Tests/MutationTests.fs | Updates mutation result matching for Direct (ValueSome data, errors) with explicit failure on null data. |
| tests/FSharp.Data.GraphQL.Tests/LazyEnumerationExceptionTests.fs | Switches to ensureDirectNullData when enumeration failure propagates to root-null data. |
| tests/FSharp.Data.GraphQL.Tests/ExecutorMiddlewareTests.fs | Updates Direct matching to handle ValueSome vs ValueNone. |
| tests/FSharp.Data.GraphQL.Tests/ExecutionTests.fs | Updates error-path tests to assert Direct + null root data via ensureDirectNullData. |
| tests/FSharp.Data.GraphQL.Tests/ErrorHelpers.fs | Introduces ensureDirectNullData and tightens ensureDirect to ValueSome only. |
| tests/FSharp.Data.GraphQL.Tests/DeferredTests.fs | Updates deferred error payload expectations to ValueNone for “no data” incremental error payloads. |
| src/FSharp.Data.GraphQL.Server/IO.fs | Core API change: GQLResponse.Data : Skippable<Output voption> and propagation to Direct/deferred/subscription content cases. |
| src/FSharp.Data.GraphQL.Server/Executor.fs | Converts Direct result Output voption into nullable Output for GQLExecutionResult.Direct construction. |
| src/FSharp.Data.GraphQL.Server/Execution.fs | Updates deferred/subscription error payload construction to use obj voption / Output voption consistently. |
| src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLWebsocketMiddleware.fs | Updates WebSocket execution/deferred/subscription flows to use ValueNone/ValueSome and ValueOption.toObj where needed. |
| src/FSharp.Data.GraphQL.Server.AspNetCore/GraphQLRequestHandler.fs | Updates HTTP response construction to pass ValueOption.toObj into GQLResponse.Direct; updates deferred/subscription pattern matches for voption. |
| RELEASE_NOTES.md | Documents the breaking signature changes to voption-based nullability in result/response types. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
GQLExecutionResult.Empty currently passes Map.empty where Output is IDictionary<string,obj>, which should not type-check and indicates an incorrect empty-output construction.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The diff consistently applies the new “skippable vs null” data contract across execution/transport paths and updates tests/helpers to assert the intended semantics without leaving mixed representations behind.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
GQLResponse.DatatoSkippable<Output voption>and propagates that contract through the minimum required execution and test surfaces.Review scope
Please review only response-data presence/nullability semantics. Generic streaming fixes, middleware changes, WebSocket lifecycle, and active-pattern changes are intentionally excluded into separate PRs.
Generic streaming follow-up: #612
Verification
Relevant projects build successfully; focused response/execution tests passed (88 passed, 1 pre-existing skip).
git diff --checkis clean.