Skip to content

Fixed streaming execution and middleware behavior - #612

Merged
xperiandri merged 3 commits into
devfrom
task-seq-field-streaming-fixes
Sep 18, 2026
Merged

xperiandri merged 3 commits into
devfrom
task-seq-field-streaming-fixes

Conversation

@xperiandri

@xperiandri xperiandri commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Contains the non-WebSocket fixes extracted semantically from the former mixed #608 diff: generic streaming execution, async-enumerator cancellation/error handling, middleware behavior, schema/type-system adjustments, and focused non-WebSocket tests.

Review scope

Please review only generic streaming and middleware behavior introduced on top of #611. The GQLResponse.Data : Skippable<Output voption> contract is reviewed in #611. WebSocket transport/lifecycle and serialization are reviewed separately in #608.

Base contract PR: #611
WebSocket follow-up: #608

Verification

Relevant projects build successfully; the full unit suite passed (660 passed, 5 pre-existing skips). git diff --check is clean.

@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown

Test Results

    9 files      9 suites   13m 20s ⏱️
  770 tests   765 ✅  5 💤 0 ❌
2 310 runs  2 295 ✅ 15 💤 0 ❌

Results for commit b57bf2d.

♻️ This comment has been updated with latest results.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Middleware validation remains reporting-dependent, and several race tests are nondeterministic.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 3 Medium severity

Open (4)
What changed in this PR

Improves generic streaming reliability, nullable task-sequence support, and middleware request-error handling.

Changes:

  • Hardens async enumeration cancellation and failure handling.
  • Adds voption-based TaskSeqField overloads.
  • Validates root arguments and filter coercion earlier.
File Description
tests/​FSharp.Data.GraphQL.Tests/​TaskSeqFieldTests.fs Expands task-sequence regression coverage.
tests/​FSharp.Data.GraphQL.Tests/​MiddlewareTests.fs Tests filter coercion request errors.
tests/​FSharp.Data.GraphQL.Tests/​Helpers and Extensions/​ObservableExtensionsTests.fs Tests enumeration cancellation and failures.
tests/​FSharp.Data.GraphQL.Tests/​ErrorHelpers.fs Reformats test helpers.
src/​FSharp.Data.GraphQL.Shared/​TypeSystem.fs Supports value-option task sequences.
src/​FSharp.Data.GraphQL.Shared/​SchemaDefinitions.fs Adds struct-nullable field overloads.
src/​FSharp.Data.GraphQL.Server/​ObservableExtensions.fs Hardens concurrent async enumeration.
src/​FSharp.Data.GraphQL.Server/​Execution.fs Pre-validates root field arguments.
src/​FSharp.Data.GraphQL.Server.Middleware/​MiddlewareDefinitions.fs Returns request errors for invalid filters.
RELEASE_NOTES.md Reorders relevant fix notes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs
xperiandri and others added 2 commits September 19, 2026 00:46
Bring in the remaining non-WebSocket, non-typo changes from the
original branch on top of the response-data-skippable contract:
generic streaming/async-enumerator fixes and reformatting in
Execution.fs, Executor.fs and ObservableExtensions.fs; schema/
type-system additions and documentation in SchemaDefinitions.fs and
TypeSystem.fs; the ObjectListFilter middleware behavior change and
reformatting in MiddlewareDefinitions.fs; the ErrorMessages.fs
'indetifier' rename and the 'indicies' rename in Execution.fs/
TaskSeqFieldTests.fs (incidental identifier renames, not documentation
typo fixes); RELEASE_NOTES.md bullet-list reordering; and non-WebSocket
test additions/reformatting across DeferredTests.fs, ExecutionTests.fs,
ExecutorMiddlewareTests.fs, MutationTests.fs, Relay/ConnectionTests.fs,
LazyEnumerationExceptionTests.fs, MiddlewareTests.fs,
ObservableExtensionsTests.fs, ErrorHelpers.fs and TaskSeqFieldTests.fs.

Excludes WebSocket transport/lifecycle/serialization changes (kept for
the next commit) and typo/wording corrections (there are none in the
original diff; see the Fix streaming-related typos commit for details).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Middleware traversal remains incomplete, and one streaming regression test does not deterministically exercise its intended path.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (4)

Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The object-list filter preflight does not accurately traverse or restrict execution-plan fields.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preflight skips wrapped/nested filters and misidentifies unrelated arguments

src/​FSharp.Data.GraphQL.Server.Middleware/​MiddlewareDefinitions.fs:141

The new unconditional preflight does not follow the executable plan accurately. It stops at @defer/@stream/@live wrappers and at collection element plans, so wrapped or nested list filters still bypass request-level coercion. Conversely, it ignores Include and treats every collection argument named filter as this middleware's type, so a skipped subtree or an unrelated filter: String field can reject a valid request. Traverse wrappers and collection elements transparently, skip excluded nodes, and coerce only fields whose definition has the middleware-added nullable ObjectListFilter argument.

@xperiandri xperiandri changed the title Fix streaming execution and middleware behavior Fixed streaming execution and middleware behavior Sep 18, 2026
@xperiandri
xperiandri merged commit d5cf4e5 into dev Sep 18, 2026
5 checks passed
@xperiandri
xperiandri deleted the task-seq-field-streaming-fixes branch September 18, 2026 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants