-
Notifications
You must be signed in to change notification settings - Fork 473
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 #2294: batch processing loses HttpContext for main request. #2385
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -12,11 +12,14 @@ | |
using Microsoft.AspNet.OData.Extensions; | ||
using Microsoft.AspNet.OData.Test.Abstraction; | ||
using Microsoft.AspNet.OData.Test.Common; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.DependencyInjection.Extensions; | ||
using Xunit; | ||
#if !NETCORE | ||
using System.Web.Http; | ||
using System.Web.Http.Routing; | ||
#else | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Mvc; | ||
using Newtonsoft.Json; | ||
#endif | ||
|
@@ -727,7 +730,7 @@ public async Task SendAsync_CorrectlyHandlesCookieHeader() | |
var changesetRef = $"changeset_{Guid.NewGuid()}"; | ||
var endpoint = "http://localhost"; | ||
|
||
Type[] controllers = new[] { typeof(BatchTestCustomersController), typeof(BatchTestOrdersController), }; | ||
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 did we get rid of the test that used two controllers? Isn't this still a valid scenario? Don't we need to make sure it works as expected? 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've added unused controller with #2379, it is just a cleanup. |
||
Type[] controllers = new[] { typeof(BatchTestOrdersController), }; | ||
var server = TestServerFactory.Create(controllers, (config) => | ||
{ | ||
var builder = ODataConventionModelBuilderFactory.Create(config); | ||
|
@@ -782,8 +785,73 @@ public async Task SendAsync_CorrectlyHandlesCookieHeader() | |
var response = await client.SendAsync(batchRequest); | ||
|
||
ExceptionAssert.DoesNotThrow(() => response.EnsureSuccessStatusCode()); | ||
} | ||
|
||
[Fact] | ||
public async Task ProcessBatchAsync_PreservesHttpContext() | ||
{ | ||
var batchRef = $"batch_{Guid.NewGuid()}"; | ||
var changesetRef = $"changeset_{Guid.NewGuid()}"; | ||
var endpoint = "http://localhost"; | ||
|
||
Type[] controllers = new[] { typeof(BatchTestOrdersController), }; | ||
var server = TestServerFactory.Create( | ||
controllers, | ||
config => | ||
{ | ||
var builder = ODataConventionModelBuilderFactory.Create(config); | ||
builder.EntitySet<BatchTestOrder>("BatchTestOrders"); | ||
|
||
config.MapODataServiceRoute("odata", null, builder.GetEdmModel(), new CustomODataBatchHandler()); | ||
config.Expand(); | ||
config.EnableDependencyInjection(); | ||
}, | ||
config => | ||
{ | ||
config.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>(); | ||
}); | ||
|
||
var client = TestServerFactory.CreateClient(server); | ||
|
||
var orderId = 2; | ||
var createOrderPayload = $@"{{""@odata.type"":""Microsoft.AspNet.OData.Test.Batch.BatchTestOrder"",""Id"":{orderId},""Amount"":50}}"; | ||
|
||
var batchRequest = new HttpRequestMessage(HttpMethod.Post, $"{endpoint}/$batch"); | ||
batchRequest.Headers.Accept.Add(MediaTypeWithQualityHeaderValue.Parse("text/plain")); | ||
|
||
var batchContent = $@" | ||
--{batchRef} | ||
Content-Type: multipart/mixed;boundary={changesetRef} | ||
|
||
--{changesetRef} | ||
Content-Type: application/http | ||
Content-Transfer-Encoding: binary | ||
Content-ID: 1 | ||
|
||
POST {endpoint}/BatchTestOrders HTTP/1.1 | ||
Content-Type: application/json;type=entry | ||
Prefer: return=representation | ||
|
||
// TODO: assert somehow? | ||
{createOrderPayload} | ||
--{changesetRef}-- | ||
--{batchRef} | ||
Content-Type: application/http | ||
Content-Transfer-Encoding: binary | ||
|
||
GET {endpoint}/BatchTestOrders({orderId}) HTTP/1.1 | ||
Content-Type: application/json;type=entry | ||
Prefer: return=representation | ||
|
||
--{batchRef}-- | ||
"; | ||
|
||
var httpContent = new StringContent(batchContent); | ||
httpContent.Headers.ContentType = MediaTypeHeaderValue.Parse($"multipart/mixed;boundary={batchRef}"); | ||
httpContent.Headers.ContentLength = batchContent.Length; | ||
batchRequest.Content = httpContent; | ||
var response = await client.SendAsync(batchRequest); | ||
|
||
ExceptionAssert.DoesNotThrow(() => response.EnsureSuccessStatusCode()); | ||
} | ||
#endif | ||
} | ||
|
@@ -824,6 +892,13 @@ public class BatchTestOrder | |
return new List<BatchTestOrder> { order01 }; | ||
}); | ||
|
||
|
||
[EnableQuery] | ||
public SingleResult<BatchTestOrder> Get([FromODataUri]int key) | ||
{ | ||
return SingleResult.Create(Orders.Where(d => d.Id.Equals(key)).AsQueryable()); | ||
} | ||
|
||
public static IList<BatchTestOrder> Orders | ||
{ | ||
get | ||
|
@@ -927,5 +1002,22 @@ public class BatchTestHeadersCustomer | |
{ | ||
public int Id { get; set; } | ||
} | ||
|
||
public class CustomODataBatchHandler : DefaultODataBatchHandler | ||
{ | ||
/// <inheritdoc /> | ||
public override async Task ProcessBatchAsync(HttpContext context, RequestDelegate nextHandler) | ||
{ | ||
// Retrieve current httpcontext. | ||
var httpContextAccessor = context.RequestServices.GetService<IHttpContextAccessor>(); | ||
var beforeContext = httpContextAccessor?.HttpContext; | ||
await base.ProcessBatchAsync(context, nextHandler); | ||
var afterContext = httpContextAccessor?.HttpContext; | ||
if (httpContextAccessor != null) | ||
{ | ||
Assert.Equal(beforeContext, afterContext); | ||
} | ||
} | ||
} | ||
#endif | ||
} |
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.
What are the ramifications of not using the context factory? Doesn't that mean that, if someone adds their own context factory, we don't use it for batch? Couldn't that be a problem if the developer is expecting their custom httpcontext to be used? Do we instead need a way to create the context from the factory without resetting the IHttpContextAccessor.HttpContext?
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.
I do not see any other reason to use the factory for creating
HttpContext
from a set of sub-requests, but dependency injection. And it is unclear why a global factory (that creates context for an actual http request) should be used to instance batch sub-contexts.If the factory is used we should make a
closure
of original context after processing batch changesets like this one:But still a closure is more like a workaround than THE solution.
Keyword
new
is kinda bad in common, especially in the name of DI, instead we can introduce an interfaceIBatchHttpContextFactory
to create a context of the changeset (silly refactor)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.
Hi @mikepizzo & @NicholasNoise, I tested the pr with the two suggestions. I guess my question would be for Sub requests created during batch processing should we give users more control over the creation of the context? Since we do copy the features, headers, and cookie information. One of the issues with the current approach I do see is if the user used their own IHttpContextAccessor and IHttpContextFactory then this process would need them to do more refactors if they needed to change how batch sub requests are created by extending the BatchMiddleware.
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.
Hi @marabooy
Yeah, we should give more control of the context creation. But as I've pointed out
IHttpContextFactory
is not a way to do so. I suggest a new abstractionIBatchHttpContextFactory
for this matter.