Remove server-side byte-range emission surface#522
Merged
Conversation
Trellis serves general business web services, not media servers. The server-side byte-range emission helpers (PartialContentHttpResult, PartialContentResult, RangeRequestEvaluator, RangeOutcome, HttpResponseOptionsBuilder<T>.WithRange / .WithAcceptRanges, RepresentationMetadata.AcceptRanges) duplicate Microsoft.AspNetCore.Http.Results.File(enableRangeProcessing: true) and add no Trellis-specific value. Consumers who need byte-range responses should call ASP.NET Core directly. The client-side typed-error vocabulary HttpError.RangeNotSatisfiable, the 416 incoming mapping in HttpResponseExtensions, and the Content-Range companion-header emission in ResponseFailureWriter remain so inbound 416 responses still round-trip through Result chains. Breaking API removals: PartialContentHttpResult, PartialContentResult, RangeRequestEvaluator, RangeOutcome, HttpResponseOptionsBuilder<T>.WithRange, HttpResponseOptionsBuilder<T>.WithAcceptRanges, HttpResponseOptions<T>.RangeSelector, HttpResponseOptions<T>.StaticRange, HttpResponseOptions<T>.AcceptRanges, RepresentationMetadata.AcceptRanges, RepresentationMetadata.Builder.SetAcceptRanges, the Status206PartialContent ProducesResponseTypeMetadata entry on TrellisHttpResult<TDomain, TBody>.
Test Results6 100 tests - 29 6 085 ✅ - 29 5m 1s ⏱️ -43s Results for commit 9c45782. ± Comparison against base commit 65ec76f. This pull request removes 30 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #522 +/- ##
==========================================
+ Coverage 86.16% 86.17% +0.01%
==========================================
Files 322 319 -3
Lines 13271 13145 -126
Branches 2845 2820 -25
==========================================
- Hits 11435 11328 -107
+ Misses 1210 1195 -15
+ Partials 626 622 -4
🚀 New features to boost your workflow:
|
Issue: The CHANGELOG had no entry for the breaking removal of the server-side byte-range emission surface, and three articles still listed 206 in positive descriptions of Trellis-emitted status codes (Cache-Control table row, ProducesResponseType union, VaryForActor cache list). Fix: Add an Unreleased breaking-change entry listing the removed public API, the migration to Results.File(enableRangeProcessing: true) and direct HttpContext.Response.Headers writes, and the preserved client-side HttpError.RangeNotSatisfiable round-trip. Remove 206 from the three status lists.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes Trellis’s server-side byte-range emission surface area (206 helpers, range evaluation, and related options/metadata) to avoid duplicating ASP.NET Core’s built-in Results.File(..., enableRangeProcessing: true) behavior, while preserving the client-side 416 typed-error vocabulary and server-side 416 companion header emission.
Changes:
- Removed
WithRange/WithAcceptRanges,RangeRequestEvaluator/RangeOutcome, andPartialContent*results fromTrellis.Asp, plus updated response metadata to no longer advertise206. - Removed
AcceptRangesfromRepresentationMetadataand stopped emitting theAccept-Rangesheader from Trellis response writers. - Updated docs/samples/cookbook recipe/ADR to direct consumers to ASP.NET Core for byte-range responses.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Trellis.Http.Abstractions/tests/RepresentationMetadataTests.cs | Removes AcceptRanges assertions from metadata tests. |
| Trellis.Http.Abstractions/src/RepresentationMetadata.cs | Drops AcceptRanges from the metadata bag and builder. |
| Trellis.Core/src/Pagination/Page.cs | Updates guidance to use ASP.NET Core Results.File(... enableRangeProcessing: true) for binary range downloads. |
| Trellis.Asp/tests/WithCacheControlTests.cs | Removes 206/range-specific cache-control test. |
| Trellis.Asp/tests/TrellisWriteOutcomeResultTests.cs | Removes Accept-Ranges expectations and builder usage. |
| Trellis.Asp/tests/TrellisHttpResultMetadataTests.cs | Updates produced-response-type status set to exclude 206. |
| Trellis.Asp/tests/TrellisHttpResultExtraTests.cs | Removes range-related test coverage and adjusts header tests. |
| Trellis.Asp/tests/RangeRequestEvaluatorTests.cs | Deletes tests for the removed range evaluator. |
| Trellis.Asp/tests/HttpResponseOptionsBuilderTests.cs | Removes tests for WithRange and WithAcceptRanges. |
| Trellis.Asp/tests/HttpPartialContentResultTest.cs | Deletes tests for the removed MVC 206 result. |
| Trellis.Asp/src/Response/TrellisWriteOutcomeResult.cs | Stops emitting Accept-Ranges from metadata/options. |
| Trellis.Asp/src/Response/TrellisHttpResult.cs | Removes range evaluation/206 execution path and drops 206 from endpoint metadata. |
| Trellis.Asp/src/Response/HttpResponseOptionsBuilder.cs | Removes WithRange / WithAcceptRanges and related option fields. |
| Trellis.Asp/src/RangeRequestEvaluator.cs | Deletes the range evaluation API and outcome types. |
| Trellis.Asp/src/PartialContentResult.cs | Deletes MVC PartialContentResult implementation. |
| Trellis.Asp/src/PartialContentHttpResult.cs | Deletes Minimal API PartialContentHttpResult implementation. |
| Trellis.Asp/SAMPLES.md | Removes sample section demonstrating Content-Range/206 pagination. |
| Examples/CookbookSnippets/Recipe06_ConditionalGet.cs | Updates recipe snippet to remove byte-range example and focus on conditional GET. |
| docs/docfx_project/articles/integration-aspnet.md | Removes range entries/sections from the integration guide (but still needs follow-up wording cleanup per comments). |
| docs/docfx_project/articles/asp-tohttpresponse.md | Removes range references from the ToHttpResponse guide (but still needs a Cache-Control row tweak per comments). |
| docs/docfx_project/api_reference/trellis-api-http-abstractions.md | Updates RepresentationMetadata docs to remove Accept-Ranges. |
| docs/docfx_project/api_reference/trellis-api-cookbook.md | Updates Recipe 6 to remove range support and document ASP.NET Core Results.File for byte ranges. |
| docs/docfx_project/api_reference/trellis-api-asp.md | Removes range surface documentation and adds explicit “byte ranges out of scope” note. |
| docs/docfx_project/adr/ADR-002-v2-redesign-plan.md | Updates ADR narrative to reflect full removal of server-side byte-range emission surface. |
| .github/copilot-instructions.md | Updates contributor guidance to remove “range handling” mentions for ASP docs. |
Comment on lines
127
to
131
| b.WithETag(_ => "x") | ||
| .WithLastModified(_ => DateTimeOffset.UtcNow) | ||
| .Vary("Accept") | ||
| .WithContentLanguage("en") | ||
| .WithContentLocation(_ => "/x") |
Comment on lines
13
to
17
| /// <summary> | ||
| /// Outcome-level coverage for <c>ToHttpResponse</c> branches not exercised by the higher-level | ||
| /// <c>ToHttpResponseTests</c>: metadata headers, range handling, conditional-request decisions, | ||
| /// <c>ToHttpResponseTests</c>: metadata headers, conditional-request decisions, | ||
| /// location resolution, and per-call error-mapping overrides. Every test drives the public | ||
| /// extension and asserts on what a caller observes (status code, headers, body bytes). |
Comment on lines
59
to
63
| | `Vary(params string[])` | Appends to `Vary` (preserves existing values; case-insensitive dedupe). | | ||
| | `WithContentLanguage(params string[])` / `WithContentLocation(Func<T, string>)` / `WithAcceptRanges(string)` | Sets the matching response header. | | ||
| | `WithCacheControl(CacheControlHeaderValue)` | Sets `Cache-Control` on success (200 / 201 / 204 / 206 / 304 / WriteOutcome / paged) **and on failure** responses, so a sensitive endpoint declaring `WithCacheControl(CacheControl.NoStore())` protects 404 / 403 / 412 / 422 from intermediate-cache leakage just as much as the 200. Throws `ArgumentNullException` on null. Use the [`CacheControl`](../api_reference/trellis-api-asp.md#cachecontrol) presets (`NoStore()`, `NoCache()`, `Public(TimeSpan)`, `Private(TimeSpan)`, `Immutable(TimeSpan)`) for common shapes. | | ||
| | `WithContentLanguage(params string[])` / `WithContentLocation(Func<T, string>)` | Sets the matching response header. | | ||
| | `WithCacheControl(CacheControlHeaderValue)` | Sets `Cache-Control` on success (200 / 201 / 204 / 304 / WriteOutcome / paged) **and on failure** responses, so a sensitive endpoint declaring `WithCacheControl(CacheControl.NoStore())` protects 404 / 403 / 412 / 422 from intermediate-cache leakage just as much as the 200. Throws `ArgumentNullException` on null. Use the [`CacheControl`](../api_reference/trellis-api-asp.md#cachecontrol) presets (`NoStore()`, `NoCache()`, `Public(TimeSpan)`, `Private(TimeSpan)`, `Immutable(TimeSpan)`) for common shapes. | | ||
| | `WithCacheControl(Func<T, CacheControlHeaderValue?>)` | Per-domain selector — success path only (failures carry no domain value, and no-payload write outcomes like `UpdatedNoContent` / `AcceptedNoContent` also skip the selector since they carry no `T`). Returning `null` from the selector skips the per-domain header; when the static-value overload is also configured, the static value remains in place. | | ||
| | `Created(string literal)` / `Created(Func<T, string>)` | `201 Created` with literal or value-derived `Location`. | |
Owner
Author
There was a problem hiding this comment.
Already addressed in 30957bb — Cache-Control row no longer lists 206.
Comment on lines
23
to
27
| | Conditional `GET`/`HEAD` (`If-None-Match`, `If-Modified-Since`) | `opts.WithETag(...).EvaluatePreconditions()` | [Conditional requests](#conditional-requests) | | ||
| | Honor `Prefer: return=minimal` / `return=representation` | `opts.HonorPrefer()` on a `WriteOutcome` response | [Prefer header](#prefer-header) | | ||
| | Emit `201 Created` with a `Location` header | `opts.CreatedAtRoute(name, values)` (AOT-safe) / `Created(...)` / `CreatedAtAction(...)` | [Created responses](#created-responses) | | ||
| | Return paginated JSON + RFC 8288 `Link` header | `Result<Page<T>>.ToHttpResponse(nextUrlBuilder, body)` | [Pagination](#pagination) | | ||
| | Return `206 Partial Content` for byte / item ranges | `opts.WithRange(from, to, total)` / `opts.WithRange(selector)` | [Range responses](#range-responses) | | ||
| | Validate scalar value objects (route, query, JSON body) | `AddScalarValueValidation` + `UseScalarValueValidation` + `WithScalarValueValidation` | [Scalar value validation](#scalar-value-validation) | |
Owner
Author
| /// <c>Content-Location</c>, and <c>Accept-Ranges</c> response headers consistently | ||
| /// across MVC and Minimal API responses (200, 201, 206, 304). | ||
| /// and <c>Content-Location</c> response headers consistently | ||
| /// across MVC and Minimal API responses (200, 201, 304). |
Owner
Author
There was a problem hiding this comment.
Fixed in e2f5099 — summary now reads (200, 201, 204, 304).
| // Domain-dependent selectors (ETag / Last-Modified / Content-Location / Cache-Control) | ||
| // must short-circuit in that case — invoking them with null would NPE on the first | ||
| // member access. Non-domain headers (Vary / ContentLanguage / AcceptRanges) still apply. | ||
| // Non-domain headers (Vary / ContentLanguage) still apply. |
Owner
Author
There was a problem hiding this comment.
Fixed in e2f5099 — comment rewritten to be complete (…would NPE on the first member access) and Cache-Control dropped from the selector list since ApplyMetadata does not apply it.
| - `Trellis.Asp.PartialContentResult` (MVC `ObjectResult`) | ||
| - `Trellis.Asp.RangeRequestEvaluator` and the `RangeOutcome` closed union (`FullRepresentation` / `PartialContent` / `NotSatisfiable`) | ||
| - `HttpResponseOptionsBuilder<T>.WithRange(Func<T, ContentRangeHeaderValue>)`, `WithRange(long, long, long)`, `WithAcceptRanges(string)` | ||
| - `HttpResponseOptions<T>.RangeSelector`, `StaticRange`, `AcceptRanges` |
Owner
Author
There was a problem hiding this comment.
Fixed in e2f5099 — internal-type bullet removed; the public-facing WithRange / WithAcceptRanges builder bullet covers the user-visible removal.
Issue: The first byte-range removal commits left small inconsistencies: two test files retained an unused System.Net.Http.Headers import; the RepresentationMetadata XML summary omitted 204 from the list of target responses; the ApplyMetadata comment in TrellisHttpResult was truncated mid-sentence and still referenced Cache-Control even though this method does not apply it; the CHANGELOG listed the internal HttpResponseOptions<T> properties under removed public API; and integration-aspnet.md still introduced Trellis.Asp as handling ranges and Range semantics. Fix: Remove the unused usings; add 204 to RepresentationMetadata's summary; rewrite the ApplyMetadata comment to be complete and to drop Cache-Control from the list; drop the internal-type bullet from the CHANGELOG entry; remove range/ranges from integration-aspnet.md's intro and audience bullet.
Comment on lines
14
to
16
| public static void Map(IEndpointRouteBuilder app) => | ||
| app.MapGet("/blobs/{id:guid}", async (System.Guid id, HttpRequest req, IBlobRepository repo, CancellationToken ct) => | ||
| app.MapGet("/blobs/{id:guid}", async (System.Guid id, IBlobRepository repo, CancellationToken ct) => | ||
| { |
Issue: After the Recipe 06 rewrite dropped the HttpRequest parameter and the byte-range branch, the Microsoft.AspNetCore.Http using remained even though no symbols from that namespace are referenced. Fix: Drop the unused using.
| **Problem.** Serve a resource with strong-ETag conditional GET so clients can revalidate cheaply. | ||
|
|
||
| ```csharp | ||
| using Microsoft.AspNetCore.Http; |
Owner
Author
There was a problem hiding this comment.
Fixed in f4b3604 — unused using Microsoft.AspNetCore.Http; removed from the cookbook snippet.
| - `Trellis.Asp.PartialContentResult` (MVC `ObjectResult`) | ||
| - `Trellis.Asp.RangeRequestEvaluator` and the `RangeOutcome` closed union (`FullRepresentation` / `PartialContent` / `NotSatisfiable`) | ||
| - `HttpResponseOptionsBuilder<T>.WithRange(Func<T, ContentRangeHeaderValue>)`, `WithRange(long, long, long)`, `WithAcceptRanges(string)` | ||
| - `RepresentationMetadata.AcceptRanges` and `RepresentationMetadata.Builder.SetAcceptRanges(string)` (the constructor and builder still accept every other parameter unchanged) |
Owner
Author
There was a problem hiding this comment.
Fixed in f4b3604 — reworded to (every other RepresentationMetadata member is unchanged) so the note no longer references a non-existent public constructor.
Issue: The Recipe 6 cookbook snippet still imported Microsoft.AspNetCore.Http even though no symbols from that namespace are referenced. The CHANGELOG bullet for the AcceptRanges removal described a non-existent public constructor on RepresentationMetadata. Fix: Drop the unused using from the snippet so it mirrors the underlying code file. Reword the AcceptRanges bullet to state that every other RepresentationMetadata member is unchanged, without implying a public constructor.
| @@ -311,94 +311,6 @@ public class JobsController : ControllerBase | |||
| } | |||
| ``` | |||
|
|
|||
Owner
Author
There was a problem hiding this comment.
Fixed in 9c45782 — TOC entry removed. The deleted Pagination Support section documented the old item-range wire shape (206 Partial Content with Content-Range: items 0-49/1000), which this PR intentionally eliminates. Current pagination guidance lives in trellis-api-cookbook.md Recipe 9 and integration-aspnet.md (Result<Page<T>>.ToHttpResponse(nextUrlBuilder, body) + RFC 8288 Link header), so SAMPLES.md does not need to duplicate it.
Issue: The SAMPLES.md Table of Contents still linked to a Pagination Support section whose body was removed in this PR (it documented emitting 206 Partial Content with Content-Range: items, which is the anti-pattern this PR eliminates). The TOC link became a dangling anchor. Fix: Drop the Pagination Support TOC entry. The current pagination guidance (Result<Page<T>>.ToHttpResponse(nextUrlBuilder, body) + RFC 8288 Link header) lives in trellis-api-cookbook.md (Recipe 9) and integration-aspnet.md; SAMPLES.md does not need to duplicate it.
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.
Issue
Trellis is positioned for general business web services. The server-side byte-range emission surface (
PartialContent*,RangeRequestEvaluator,RangeOutcome,HttpResponseOptionsBuilder<T>.WithRange,.WithAcceptRanges,RepresentationMetadata.AcceptRanges) duplicatesMicrosoft.AspNetCore.Http.Results.File(enableRangeProcessing: true)and adds no Trellis-specific value. Consumers who need byte-range responses should call ASP.NET Core directly.Fix
Delete the server-side range-evaluation / 206 helpers and their tests, docs, and examples. The client-side typed-error vocabulary stays —
HttpError.RangeNotSatisfiablecontinues to surface inbound 416s, andResponseFailureWriterstill writes a416+Content-Range: bytes */Ncompanion header when such a fault propagates through aResultchain.Breaking API removals
PartialContentHttpResult,PartialContentResult,RangeRequestEvaluator,RangeOutcomeHttpResponseOptionsBuilder<T>.WithRange(...)(both overloads),.WithAcceptRanges(string)HttpResponseOptions<T>.RangeSelector/.StaticRange/.AcceptRangesRepresentationMetadata.AcceptRanges/Builder.SetAcceptRanges(string)Status206PartialContentProducesResponseTypeMetadataentry onTrellisHttpResult<TDomain, TBody>Migration. For binary downloads, call ASP.NET Core's
Results.File(stream, enableRangeProcessing: true)directly. For advisory headers (e.g.Accept-Ranges: none), set the header onHttpContext.Response.Headersfrom middleware or the endpoint handler.