-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#1971 #928 Avoid content if original request has no content and avoid Transfer-Encoding: chunked if Content-Length is known #1972
Changes from all commits
381fd2d
231f648
b6b4086
f15c8ef
80df6bb
4fba4b5
e30ca4d
53e49d1
35b8339
3a6c144
3a17f8b
d077a01
dcbbab7
81e1ca9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,87 +1,91 @@ | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Http.Extensions; | ||
using Microsoft.Extensions.Primitives; | ||
using Ocelot.Configuration; | ||
|
||
namespace Ocelot.Request.Mapper; | ||
|
||
public class RequestMapper : IRequestMapper | ||
{ | ||
private static readonly HashSet<string> UnsupportedHeaders = new(StringComparer.OrdinalIgnoreCase) { "host" }; | ||
private static readonly string[] ContentHeaders = { "Content-Length", "Content-Language", "Content-Location", "Content-Range", "Content-MD5", "Content-Disposition", "Content-Encoding" }; | ||
|
||
public HttpRequestMessage Map(HttpRequest request, DownstreamRoute downstreamRoute) | ||
{ | ||
var requestMessage = new HttpRequestMessage | ||
{ | ||
Content = MapContent(request), | ||
Method = MapMethod(request, downstreamRoute), | ||
RequestUri = MapUri(request), | ||
Version = downstreamRoute.DownstreamHttpVersion, | ||
}; | ||
|
||
MapHeaders(request, requestMessage); | ||
|
||
return requestMessage; | ||
} | ||
|
||
private static HttpContent MapContent(HttpRequest request) | ||
{ | ||
// TODO We should check if we really need to call HttpRequest.Body.Length | ||
// But we assume that if CanSeek is true, the length is calculated without an important overhead | ||
if (request.Body is null or { CanSeek: true, Length: <= 0 }) | ||
{ | ||
return null; | ||
} | ||
|
||
var content = new StreamHttpContent(request.HttpContext); | ||
|
||
AddContentHeaders(request, content); | ||
|
||
return content; | ||
} | ||
|
||
private static void AddContentHeaders(HttpRequest request, HttpContent content) | ||
{ | ||
if (!string.IsNullOrEmpty(request.ContentType)) | ||
{ | ||
content.Headers | ||
.TryAddWithoutValidation("Content-Type", new[] { request.ContentType }); | ||
} | ||
|
||
// The performance might be improved by retrieving the matching headers from the request | ||
// instead of calling request.Headers.TryGetValue for each used content header | ||
var matchingHeaders = ContentHeaders.Where(header => request.Headers.ContainsKey(header)); | ||
|
||
foreach (var key in matchingHeaders) | ||
{ | ||
if (!request.Headers.TryGetValue(key, out var value)) | ||
{ | ||
continue; | ||
} | ||
|
||
content.Headers.TryAddWithoutValidation(key, value.ToArray()); | ||
} | ||
} | ||
|
||
private static HttpMethod MapMethod(HttpRequest request, DownstreamRoute downstreamRoute) => | ||
!string.IsNullOrEmpty(downstreamRoute?.DownstreamHttpMethod) ? | ||
new HttpMethod(downstreamRoute.DownstreamHttpMethod) : new HttpMethod(request.Method); | ||
|
||
// TODO Review this method, request.GetEncodedUrl() could throw a NullReferenceException | ||
private static Uri MapUri(HttpRequest request) => new(request.GetEncodedUrl()); | ||
|
||
private static void MapHeaders(HttpRequest request, HttpRequestMessage requestMessage) | ||
{ | ||
foreach (var header in request.Headers) | ||
{ | ||
if (IsSupportedHeader(header)) | ||
{ | ||
requestMessage.Headers.TryAddWithoutValidation(header.Key, header.Value.ToArray()); | ||
} | ||
} | ||
} | ||
|
||
private static bool IsSupportedHeader(KeyValuePair<string, StringValues> header) => | ||
!UnsupportedHeaders.Contains(header.Key); | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Http.Extensions; | ||
using Microsoft.Extensions.Primitives; | ||
using Ocelot.Configuration; | ||
|
||
namespace Ocelot.Request.Mapper; | ||
|
||
public class RequestMapper : IRequestMapper | ||
{ | ||
private static readonly HashSet<string> UnsupportedHeaders = new(StringComparer.OrdinalIgnoreCase) { "host", "transfer-encoding" }; | ||
private static readonly string[] ContentHeaders = { "Content-Length", "Content-Language", "Content-Location", "Content-Range", "Content-MD5", "Content-Disposition", "Content-Encoding" }; | ||
|
||
public HttpRequestMessage Map(HttpRequest request, DownstreamRoute downstreamRoute) | ||
{ | ||
var requestMessage = new HttpRequestMessage | ||
{ | ||
Content = MapContent(request), | ||
Method = MapMethod(request, downstreamRoute), | ||
RequestUri = MapUri(request), | ||
Version = downstreamRoute.DownstreamHttpVersion, | ||
}; | ||
|
||
MapHeaders(request, requestMessage); | ||
|
||
return requestMessage; | ||
} | ||
raman-m marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private static HttpContent MapContent(HttpRequest request) | ||
{ | ||
HttpContent content; | ||
|
||
// No content if we have no body or if the request has no content according to RFC 2616 section 4.3 | ||
if (request.Body == null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use this instead, it's imo more compact. if (request.Body == null || (!request.ContentLength.HasValue && StringValues.IsNullOrEmpty(request.Headers.TransferEncoding)))
{
return null;
}
HttpContent content = request.ContentLength is 0
? new ByteArrayContent([])
: new StreamHttpContent(request.HttpContext);
AddContentHeaders(request, content);
return content; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ggnaegi Gui, is it resolved? |
||
|| (!request.ContentLength.HasValue && StringValues.IsNullOrEmpty(request.Headers.TransferEncoding))) | ||
Comment on lines
+33
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old version - // TODO We should check if we really need to call HttpRequest.Body.Length
- // But we assume that if CanSeek is true, the length is calculated without an important overhead
- if (request.Body is null or { CanSeek: true, Length: <= 0 })
+ // no content if we have no body or if the request has no content according to RFC 2616 section 4.3
+ if (request.Body == null
+ || (!request.ContentLength.HasValue && StringValues.IsNullOrEmpty(request.Headers.TransferEncoding)))
New version |
||
{ | ||
return null; | ||
} | ||
|
||
content = request.ContentLength is 0 | ||
? new ByteArrayContent(Array.Empty<byte>()) | ||
: new StreamHttpContent(request.HttpContext); | ||
Comment on lines
+39
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old version - var content = new StreamHttpContent(request.HttpContext);
+ content = request.ContentLength is 0
+ ? new ByteArrayContent(Array.Empty<byte>())
+ : new StreamHttpContent(request.HttpContext); New version |
||
|
||
AddContentHeaders(request, content); | ||
|
||
return content; | ||
} | ||
|
||
private static void AddContentHeaders(HttpRequest request, HttpContent content) | ||
{ | ||
if (!string.IsNullOrEmpty(request.ContentType)) | ||
{ | ||
content.Headers | ||
.TryAddWithoutValidation("Content-Type", new[] { request.ContentType }); | ||
} | ||
|
||
// The performance might be improved by retrieving the matching headers from the request | ||
// instead of calling request.Headers.TryGetValue for each used content header | ||
var matchingHeaders = ContentHeaders.Where(header => request.Headers.ContainsKey(header)); | ||
|
||
foreach (var key in matchingHeaders) | ||
{ | ||
if (!request.Headers.TryGetValue(key, out var value)) | ||
{ | ||
continue; | ||
} | ||
|
||
content.Headers.TryAddWithoutValidation(key, value.ToArray()); | ||
} | ||
} | ||
|
||
private static HttpMethod MapMethod(HttpRequest request, DownstreamRoute downstreamRoute) => | ||
!string.IsNullOrEmpty(downstreamRoute?.DownstreamHttpMethod) ? | ||
new HttpMethod(downstreamRoute.DownstreamHttpMethod) : new HttpMethod(request.Method); | ||
|
||
// TODO Review this method, request.GetEncodedUrl() could throw a NullReferenceException | ||
private static Uri MapUri(HttpRequest request) => new(request.GetEncodedUrl()); | ||
|
||
private static void MapHeaders(HttpRequest request, HttpRequestMessage requestMessage) | ||
{ | ||
foreach (var header in request.Headers) | ||
{ | ||
if (IsSupportedHeader(header)) | ||
{ | ||
requestMessage.Headers.TryAddWithoutValidation(header.Key, header.Value.ToArray()); | ||
} | ||
} | ||
} | ||
|
||
private static bool IsSupportedHeader(KeyValuePair<string, StringValues> header) => | ||
!UnsupportedHeaders.Contains(header.Key); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,25 +8,24 @@ public class StreamHttpContent : HttpContent | |
private const int DefaultBufferSize = 65536; | ||
public const long UnknownLength = -1; | ||
private readonly HttpContext _context; | ||
private readonly long _contentLength; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, it's fine like this, but I would prefer using Headers.ContentLength. This value is set when calling AddContentHeaders method and setting the content headers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And... |
||
|
||
public StreamHttpContent(HttpContext context) | ||
{ | ||
_context = context ?? throw new ArgumentNullException(nameof(context)); | ||
_contentLength = context.Request.ContentLength ?? UnknownLength; | ||
} | ||
|
||
protected override async Task SerializeToStreamAsync(Stream stream, TransportContext context, | ||
CancellationToken cancellationToken) | ||
=> await CopyAsync(_context.Request.Body, stream, Headers.ContentLength ?? UnknownLength, false, | ||
cancellationToken); | ||
protected override Task SerializeToStreamAsync(Stream stream, TransportContext context, CancellationToken cancellationToken) | ||
=> CopyAsync(_context.Request.Body, stream, _contentLength, false, cancellationToken); | ||
|
||
protected override async Task SerializeToStreamAsync(Stream stream, TransportContext context) | ||
=> await CopyAsync(_context.Request.Body, stream, Headers.ContentLength ?? UnknownLength, false, | ||
CancellationToken.None); | ||
protected override Task SerializeToStreamAsync(Stream stream, TransportContext context) | ||
=> CopyAsync(_context.Request.Body, stream, _contentLength, false, CancellationToken.None); | ||
|
||
protected override bool TryComputeLength(out long length) | ||
{ | ||
length = -1; | ||
return false; | ||
length = _contentLength; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the headers do not contains the Content-Length header, when it will try to Calculate the content length using TryComputeLength. And on chunked content, there is no Content-Length header set, so it will result in a stack overflow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer this approach, so we are relying again on Microsoft implementations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the implementation of Headers.ContentLength: https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System/net/System/Net/Http/Headers/HttpContentHeaders.cs#L76 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, that's your point of view, at the end, it's a content header, so it's per se part of the content There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Alexander's concern: the code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are very sorry, but you've provided the link to the repo with the code 5 years old!!! 🤣 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid link is: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs#L42 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we finish debates here? |
||
return length >= 0; | ||
} | ||
|
||
// This is used internally by HttpContent.ReadAsStreamAsync(...) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ggnaegi Note 1
"transfer-encoding"