Skip to content
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

fix for https://github.com/aspnet/AspNetWebStack/issues/173 #176

Merged
merged 2 commits into from
Jul 25, 2018

Conversation

JamesSinclairBiomni
Copy link
Contributor

This change will use the uri scheme of the outer request for all inner requests.

@dnfclas
Copy link

dnfclas commented Jul 5, 2018

CLA assistant check
All CLA requirements met.

@dougbu dougbu self-assigned this Jul 24, 2018
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

This fell through the cracks, sorry.

@@ -187,7 +187,7 @@ public virtual Task<HttpResponseMessage> CreateResponseMessageAsync(IList<HttpRe
foreach (HttpContent httpContent in streamProvider.Contents)
{
cancellationToken.ThrowIfCancellationRequested();
HttpRequestMessage innerRequest = await httpContent.ReadAsHttpRequestMessageAsync();
HttpRequestMessage innerRequest = await httpContent.ReadAsHttpRequestMessageAsync(request.RequestUri.Scheme);
Copy link
Member

Choose a reason for hiding this comment

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

To avoid changes to unrelated tests, please make this null-conditional e.g. request.RequestUri?.Scheme. If the repo disallows new-ish C# features, use a ternary expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using a ternary. It's C# 5 so no ?.

Content = new MultipartContent("mixed")
{
new HttpMessageContent(new HttpRequestMessage(HttpMethod.Get, "http://example.com/")),
new HttpMessageContent(new HttpRequestMessage(HttpMethod.Post, "https://example.com/values"))
Copy link
Member

Choose a reason for hiding this comment

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

Please add a part using a path-only relative URI e.g. /api/values since those are recommended and seem to be more common.

Copy link
Contributor Author

@JamesSinclairBiomni JamesSinclairBiomni Jul 25, 2018

Choose a reason for hiding this comment

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

I can't supply a relative Uri within a batch request. If I do so then ParseBatchRequestsAsync calls ReadAsMultipartAsync (an extension on HttpContent) and that will throw the following error.

Message: System.IO.IOException : Error reading MIME multipart body part.
---- System.InvalidOperationException : This operation is not supported for a relative URI.

Does this suggest that its absolute uri's by design or is this a different issue?

Copy link
Member

Choose a reason for hiding this comment

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

Odd. But, if that's what enforced, so be it. On the off chance this is a bug I should open separately, please paste the full stack trace into this PR.

@dougbu dougbu merged commit dcee8e6 into aspnet:master Jul 25, 2018
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