Skip to content

Revert CQRS middleware response serialization responsibility approach - #802

Merged
Dragemil merged 8 commits into
v10.0-previewfrom
fix/cqrs-response-serialization-approach
Dec 3, 2025
Merged

Revert CQRS middleware response serialization responsibility approach#802
Dragemil merged 8 commits into
v10.0-previewfrom
fix/cqrs-response-serialization-approach

Conversation

@Dragemil

@Dragemil Dragemil commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

So now we kind of went back with what we're requiring from the CQRS middlewares. They either:

  • need to set the ExecutionResult on the HttpContext
  • provide (set?) the whole HttpResponse if they know what they're doing, as it might begin to be sent to client right away

So CQRSMiddleware will again serialize the ExecutionResult, if non other middleware has "claimed" the HttpResponse (detected with hopefully reasonable heuristics), which is a new twist I guess.

To ensure that OutputCachingMiddleware always has the HttpResponse prepared to be stored into cache, a CQRSResponseSerializerMiddleware is always added after it, which has the same serialization logic as in CQRSMiddleware.

Aaaand the detected case, of pipeline returning status code 200 after some exception was thrown after CQRSPipelineFinalizer was run, has a dedicated test.

@github-actions

github-actions Bot commented Dec 3, 2025

Copy link
Copy Markdown

test: Run #2249

Tests 📝 Passed ✅ Failed ❌ Skipped ⏭️ Pending ⏳ Other ❓ Flaky 🍂 Duration ⏱️
778 777 0 1 0 0 0 1m48s

🎉 All tests passed!

Github Test Reporter

🔄 This comment has been updated

@codecov

codecov Bot commented Dec 3, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.07%. Comparing base (dace6a9) to head (c5a52f4).
⚠️ Report is 82 commits behind head on v10.0-preview.

Files with missing lines Patch % Lines
...S/LeanCode.CQRS.Execution/HttpContextExtensions.cs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           v10.0-preview     #802      +/-   ##
=================================================
+ Coverage          80.05%   80.07%   +0.01%     
=================================================
  Files                230      231       +1     
  Lines               4343     4356      +13     
  Branches             344      338       -6     
=================================================
+ Hits                3477     3488      +11     
- Misses               782      784       +2     
  Partials              84       84              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Pull request overview

This pull request reverts the CQRS middleware response serialization responsibility approach. The architectural change shifts serialization responsibility back to CQRSMiddleware (and CQRSResponseSerializerMiddleware for output caching scenarios), while middlewares now only need to set the ExecutionResult on the HttpContext or provide the complete HTTP response directly.

Key Changes

  • Replaced CompleteCQRSExecutionResult() with SetCQRSExecutionResult() - middlewares now set the execution result without handling serialization
  • Added CQRSResponseSerializerMiddleware to handle serialization after OutputCache middleware, ensuring cached responses are properly serialized
  • Introduced IsHttpResponseSet() heuristic to detect if a middleware has already claimed the HTTP response, allowing CQRSMiddleware to skip serialization when appropriate
  • Enhanced test coverage with a new parameterized test for exception handling after CQRSPipelineFinalizer execution

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/CQRS/LeanCode.CQRS.AspNetCore.Tests/Middleware/TestHelpers.cs Removed null-forgiving operator and extra whitespace
test/CQRS/LeanCode.CQRS.AspNetCore.Tests/Middleware/CQRSValidationMiddlewareTests.cs Updated tests to use SetCQRSExecutionResult() and removed serialization assertions
test/CQRS/LeanCode.CQRS.AspNetCore.Tests/Middleware/CQRSSecurityMiddlewareTests.cs Updated tests to use SetCQRSExecutionResult() and removed response checks
test/CQRS/LeanCode.CQRS.AspNetCore.Tests/Middleware/CQRSOutputCachingObservabilityMiddlewareTests.cs Added CQRSResponseSerializerMiddleware to middleware chain and updated to use SetCQRSExecutionResult()
test/CQRS/LeanCode.CQRS.AspNetCore.Tests/Middleware/CQRSMiddlewareTests.cs Converted exception test to Theory with two cases (before/after finalizer) and updated all tests to use SetCQRSExecutionResult()
test/CQRS/LeanCode.CQRS.AspNetCore.Tests/Middleware/CQRSExceptionTranslationMiddlewareTests.cs Updated tests to use SetCQRSExecutionResult() and removed serialization assertions
test/CQRS/LeanCode.CQRS.AspNetCore.Tests/Local/MiddlewareBasedLocalExecutorTests.cs Updated to use SetCQRSExecutionResult()
test/CQRS/LeanCode.CQRS.AspNetCore.Tests.Integration/OutputCache/RemoteCQRSOutputCachingTests.cs Changed dictionary initialization syntax from collection initializer to indexer syntax
src/CQRS/LeanCode.CQRS.Execution/HttpContextExtensions.cs Added SetCQRSExecutionResult() method with duplicate check; updated GetCQRSRequiredExecutionResult() to use custom exception
src/CQRS/LeanCode.CQRS.Execution/ExecutionResult.cs Changed default status code from magic number 200 to StatusCodes.Status200OK
src/CQRS/LeanCode.CQRS.AspNetCore/Middleware/CQRSValidationMiddleware.cs Updated to use SetCQRSExecutionResult() instead of CompleteCQRSExecutionResult()
src/CQRS/LeanCode.CQRS.AspNetCore/Middleware/CQRSSecurityMiddleware.cs Updated to use SetCQRSExecutionResult() instead of CompleteCQRSExecutionResult()
src/CQRS/LeanCode.CQRS.AspNetCore/Middleware/CQRSResponseSerializerMiddleware.cs New middleware for serializing responses after OutputCache middleware
src/CQRS/LeanCode.CQRS.AspNetCore/Middleware/CQRSPipelineFinalizer.cs Updated to use SetCQRSExecutionResult() instead of CompleteCQRSExecutionResult()
src/CQRS/LeanCode.CQRS.AspNetCore/Middleware/CQRSMiddleware.cs Added conditional serialization logic using IsHttpResponseSet() check
src/CQRS/LeanCode.CQRS.AspNetCore/Middleware/CQRSExceptionTranslationMiddleware.cs Updated to use SetCQRSExecutionResult() instead of CompleteCQRSExecutionResult()
src/CQRS/LeanCode.CQRS.AspNetCore/HttpContextExtensions.cs Refactored serialization into public SerializeCQRSResultAsync() and added IsHttpResponseSet() heuristic
src/CQRS/LeanCode.CQRS.AspNetCore/CQRSApplicationBuilder.cs Added CQRSResponseSerializerMiddleware to CacheOutput() chain
docs/cqrs/pipeline/index.md Updated documentation to reflect new serialization approach and middleware responsibilities
docs/cqrs/pipeline/adding_custom_middlewares.md Updated custom middleware guidance with new SetCQRSExecutionResult() API and short-circuiting patterns

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/CQRS/LeanCode.CQRS.Execution/HttpContextExtensions.cs Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Dragemil
Dragemil merged commit 3919e3a into v10.0-preview Dec 3, 2025
9 checks passed
@Dragemil
Dragemil deleted the fix/cqrs-response-serialization-approach branch December 3, 2025 13:30
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