Conversation
Signed-off-by: Kenny Pflug <[email protected]>
…e CloudEvents integration Signed-off-by: Kenny Pflug <[email protected]>
23 tasks
There was a problem hiding this comment.
Pull request overview
This pull request refactors the HTTP write integration for ASP.NET Core Minimal APIs and MVC by introducing a wrapper-based serialization pattern. The changes replace the previous custom converter architecture with stateless, pipeline-friendly JSON converters that bundle result data with resolved serialization options. This approach follows the existing CloudEvents pattern and addresses two key issues: (1) custom converters can now properly extend the serialization behavior, and (2) serialization options are passed through the standard System.Text.Json pipeline rather than out-of-band.
Changes:
- Introduced intermediary wrapper structs (
HttpResultForWritingandHttpResultForWriting<T>) that bundle results with frozenResolvedHttpWriteOptions - Implemented stateless JSON converters that serialize wrapper types through the standard System.Text.Json pipeline
- Updated ASP.NET Core integration layers to construct wrappers and resolve options once per request
- Simplified module registration by removing the need to inject options into converter constructors
- Updated metadata serialization helpers to accept annotation parameters directly instead of
JsonSerializerOptions
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Light.Results/Http/Writing/ResolvedHttpWriteOptions.cs | New frozen struct capturing per-request HTTP write options |
| src/Light.Results/Http/Writing/HttpResultForWriting.cs | New wrapper structs bundling results with resolved options |
| src/Light.Results/Http/Writing/HttpResultForWritingExtensions.cs | Extension methods for creating wrapper structs |
| src/Light.Results/Http/Writing/ResolvedHttpWriteOptionsExtensions.cs | Extension method for freezing mutable options |
| src/Light.Results/Http/Writing/Json/HttpResultForWritingJsonConverter.cs | New stateless converters for wrapper types |
| src/Light.Results/Http/Writing/Json/HttpResultForWritingJsonConverterFactory.cs | Factory for creating generic wrapper converters |
| src/Light.Results/Http/Writing/Json/SerializerExtensions.cs | Updated to accept ResolvedHttpWriteOptions parameter |
| src/Light.Results/Http/Writing/Module.cs | Simplified to register stateless converters without options parameter |
| src/Light.Results/SharedJsonSerialization/Writing/MetadataExtensions.cs | Refactored to accept annotation parameter instead of JsonSerializerOptions |
| src/Light.Results/SharedJsonSerialization/Writing/ErrorsExtensions.cs | Updated to use JsonSerializer.Serialize with JsonTypeInfo |
| src/Light.Results/SharedJsonSerialization/Writing/SystemTextJsonWritingExtensions.cs | Updated to use JsonSerializer.Serialize with JsonTypeInfo |
| src/Light.Results/CloudEvents/Writing/Json/JsonCloudEventsExtensions.cs | Updated metadata writing to use annotation parameter |
| src/Light.Results.AspNetCore.Shared/HttpExtensions.cs | Removed options parameter from SetMetadataValuesAsHeadersIfNecessary |
| src/Light.Results.AspNetCore.MinimalApis/BaseLightResult.cs | Resolve options once and pass to SetHeaders and WriteBodyAsync |
| src/Light.Results.AspNetCore.MinimalApis/LightResult.cs | Construct wrapper and serialize through standard pipeline |
| src/Light.Results.AspNetCore.MinimalApis/Module.cs | Simplified JSON options configuration |
| src/Light.Results.AspNetCore.MinimalApis/Serialization/LightResultsMinimalApiJsonContext.cs | Updated to include HttpResultForWriting in source generation |
| src/Light.Results.AspNetCore.Mvc/BaseLightActionResult.cs | Resolve options once and pass to SetHeaders and WriteBodyAsync |
| src/Light.Results.AspNetCore.Mvc/LightActionResult.cs | Construct wrapper and serialize through standard pipeline |
| src/Light.Results.AspNetCore.Mvc/Module.cs | Simplified JSON options configuration |
| tests/Light.Results.Tests/Http/Writing/HttpResultForWritingSerializationTests.cs | New comprehensive tests for wrapper serialization |
| tests/Light.Results.Tests/Http/Writing/ModuleTests.cs | Updated to remove options parameter from converter registration tests |
| tests/Light.Results.Tests/Http/JsonConverterDirectionTests.cs | Updated to test new wrapper converters |
| tests/Light.Results.Tests/SharedJsonSerialization/Writing/SharedWritingExtensionsTests.cs | Updated to test annotation-based metadata writing |
| tests/Light.Results.AspNetCore.Shared.Tests/Serialization/ResultJsonConverterReadTests.cs | Updated to test wrapper type deserialization (which throws) |
| tests/Light.Results.AspNetCore.MinimalApis.Tests/LightResultSerializationTests.cs | Updated to test custom serializer options with new architecture |
| tests/Light.Results.AspNetCore.MinimalApis.Tests/IntegrationTests/ExtendedMinimalApiApp.cs | Updated test endpoints and removed custom converters |
| tests/Light.Results.AspNetCore.MinimalApis.Tests/ExtendedMinimalApiJsonContext.cs | Updated to include HttpResultForWriting types in source generation |
| tests/Light.Results.AspNetCore.Mvc.Tests/IntegrationTests/ExtendedMvcController.cs | Updated test endpoints and removed custom converters |
| benchmarks/Benchmarks/HttpWriteSerializationBenchmarks.cs | New benchmarks for wrapper-based serialization |
| ai-plans/0017-HTTP-write-integration-streamlining.md | Detailed plan documenting the refactoring approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Kenny Pflug <[email protected]>
Minimum allowed line rate is |
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.
Closes #17
This pull request streamlines the HTTP write integration for ASP.NET Core Minimal APIs and MVC by adopting a new wrapper-based serialization pattern. The changes replace the previous custom converter and out-of-band options passing with stateless, pipeline-friendly JSON converters, improving extensibility and simplifying the codebase. The most important changes are:
Serialization Pipeline Refactor
HttpResultForWritingandHttpResultForWriting<T>) that bundle result data with resolved serialization options, following the CloudEvents pattern. (ai-plans/0017-HTTP-write-integration-streamlining.md, ai-plans/0017-HTTP-write-integration-streamlining.mdR1-R209)ResolvedHttpWriteOptionsas a frozen, per-request snapshot of serialization options, replacing mutable options during serialization. (ai-plans/0017-HTTP-write-integration-streamlining.md, ai-plans/0017-HTTP-write-integration-streamlining.mdR1-R209)JSON Converter Improvements
HttpResultForWritingJsonConverter, generic variant, and factory) that serialize the wrapper types through the standard System.Text.Json pipeline, removing the need for customSerializemethods or converter downcasts. (ai-plans/0017-HTTP-write-integration-streamlining.md, ai-plans/0017-HTTP-write-integration-streamlining.mdR1-R209)ASP.NET Core Integration Updates
BaseLightResultandLightResultto resolve and freeze options once per request, passingResolvedHttpWriteOptionsto header and body writing methods. Updated method signatures and removed legacy converter downcast logic. (src/Light.Results.AspNetCore.MinimalApis/BaseLightResult.cs, [1] [2];src/Light.Results.AspNetCore.MinimalApis/LightResult.cs, [3]LightResultto use the new wrapper and pipeline, removing custom converter logic and fallback paths. (src/Light.Results.AspNetCore.MinimalApis/LightResult.cs, src/Light.Results.AspNetCore.MinimalApis/LightResult.csR38-R55)Benchmark and Registration Adjustments
benchmarks/Benchmarks/HttpWriteSerializationBenchmarks.cs, benchmarks/Benchmarks/HttpWriteSerializationBenchmarks.csR1-R164)ai-plans/0017-HTTP-write-integration-streamlining.md, ai-plans/0017-HTTP-write-integration-streamlining.mdR1-R209)