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.

Sign up for GitHub

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

Jump to bottom

Fix behaviour on status code requests. #2512

Merged
merged 1 commit into from
Jul 15, 2021
Merged

Fix behaviour on status code requests. #2512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 37 additions & 39 deletions src/Microsoft.AspNetCore.OData/EnableQueryAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public override void OnActionExecuting(ActionExecutingContext context)
{
elementType = TypeHelper.GetImplementedIEnumerableType(returnType);
}
else if(TypeHelper.IsGenericType(returnType) && returnType.GetGenericTypeDefinition() == typeof(Task<>))
else if (TypeHelper.IsGenericType(returnType) && returnType.GetGenericTypeDefinition() == typeof(Task<>))
{
elementType = returnType.GetGenericArguments().First();
}
Expand Down Expand Up @@ -228,46 +228,44 @@ public override void OnActionExecuted(ActionExecutedContext actionExecutedContex
{
// actionExecutedContext.Result might also indicate a status code that has not yet
// been applied to the result; make sure it's also successful.
StatusCodeResult statusCodeResult = actionExecutedContext.Result as StatusCodeResult;
if (statusCodeResult == null || IsSuccessStatusCode(statusCodeResult.StatusCode))
ObjectResult responseContent = actionExecutedContext.Result as ObjectResult;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there other return result types other than ObjectResult that we should care about? I see that IStatusCodeActionResult is the common interface of all action results that have a status code. Why not use that?

Copy link
Contributor

@habbes habbes Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Looking further, it seems that there are a few other result types that have a Value, such as JsonResult, and there's also ContentResult which has Content. Is the assumption here that ObjectResult is the only one that has a Value that can be processed by [EnableQuery] and so we don't care about the rest?

Copy link
Member Author

Choose a reason for hiding this comment

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

The interface exists for net core 2.2 and above. I just went through the code and patched the behaviour as we don't look at other items such as JsonResult and Content result.


if (responseContent != null && (responseContent.StatusCode == null || IsSuccessStatusCode(responseContent.StatusCode.Value)))
{
ObjectResult responseContent = actionExecutedContext.Result as ObjectResult;
if (responseContent != null)

//throw Error.Argument("actionExecutedContext", SRResources.QueryingRequiresObjectContent,
// actionExecutedContext.Result.GetType().FullName);
Comment on lines +236 to +237
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could remove this commented code.

It seems hat it as meant to throw an exception if the result was not an ObjectResult. I wonder why it was commented out. But I think it should be removed if it's not used.


// Get collection from SingleResult.
IQueryable singleResultCollection = null;
SingleResult singleResult = responseContent.Value as SingleResult;
if (singleResult != null)
{
// This could be a SingleResult, which has the property Queryable.
// But it could be a SingleResult() or SingleResult<T>. Sort by number of parameters
// on the property and get the one with the most parameters.
PropertyInfo propInfo = responseContent.Value.GetType().GetProperties()
.OrderBy(p => p.GetIndexParameters().Count())
.Where(p => p.Name.Equals("Queryable"))
.LastOrDefault();

singleResultCollection = propInfo.GetValue(singleResult) as IQueryable;
}

// Execution the action.
object queryResult = OnActionExecuted(
responseContent.Value,
singleResultCollection,
new WebApiActionDescriptor(actionDescriptor as ControllerActionDescriptor),
new WebApiRequestMessage(request),
(elementClrType) => GetModel(elementClrType, request, actionDescriptor),
(queryContext) => CreateAndValidateQueryOptions(request, queryContext),
(statusCode) => actionExecutedContext.Result = new StatusCodeResult((int)statusCode),
(statusCode, message, exception) => actionExecutedContext.Result = CreateBadRequestResult(message, exception));

if (queryResult != null)
{
//throw Error.Argument("actionExecutedContext", SRResources.QueryingRequiresObjectContent,
// actionExecutedContext.Result.GetType().FullName);

// Get collection from SingleResult.
IQueryable singleResultCollection = null;
SingleResult singleResult = responseContent.Value as SingleResult;
if (singleResult != null)
{
// This could be a SingleResult, which has the property Queryable.
// But it could be a SingleResult() or SingleResult<T>. Sort by number of parameters
// on the property and get the one with the most parameters.
PropertyInfo propInfo = responseContent.Value.GetType().GetProperties()
.OrderBy(p => p.GetIndexParameters().Count())
.Where(p => p.Name.Equals("Queryable"))
.LastOrDefault();

singleResultCollection = propInfo.GetValue(singleResult) as IQueryable;
}

// Execution the action.
object queryResult = OnActionExecuted(
responseContent.Value,
singleResultCollection,
new WebApiActionDescriptor(actionDescriptor as ControllerActionDescriptor),
new WebApiRequestMessage(request),
(elementClrType) => GetModel(elementClrType, request, actionDescriptor),
(queryContext) => CreateAndValidateQueryOptions(request, queryContext),
(statusCode) => actionExecutedContext.Result = new StatusCodeResult((int)statusCode),
(statusCode, message, exception) => actionExecutedContext.Result = CreateBadRequestResult(message, exception));

if (queryResult != null)
{
responseContent.Value = queryResult;
}
responseContent.Value = queryResult;
}
}
}
Expand Down
77 changes: 76 additions & 1 deletion test/UnitTest/Microsoft.AspNet.OData.Test.Shared/Query/EnableQueryAttributeTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,25 @@
// Licensed under the MIT License. See License.txt in the project root for license information.

#if NETCORE
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using Microsoft.AspNet.OData.Extensions;
using Microsoft.AspNet.OData.Query;
using Microsoft.AspNet.OData.Routing;
using Microsoft.AspNet.OData.Test.Abstraction;
using Microsoft.AspNet.OData.Test.Common;
using Microsoft.AspNet.OData.Test.Common.Models;
using Microsoft.AspNet.OData.Test.Query.Controllers;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Abstractions;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.OData;
using Microsoft.OData.Edm;
using Xunit;
#else
using System;
Expand Down Expand Up @@ -254,8 +267,70 @@ public void OnActionExecuted_Throws_Null_Context()
{
ExceptionAssert.ThrowsArgumentNull(() => new EnableQueryAttribute().OnActionExecuted(null), "actionExecutedContext");
}

#if NETCORE // Following functionality is only supported in NetCore.
[Fact]
public void OnActionExecuted_HandlesStatusCodesCorrectly()
{
// Arrange
HttpContext httpContext = new DefaultHttpContext();
httpContext.Request.Method = "Get";
ActionDescriptor actionDescriptor = new ActionDescriptor();
ActionContext actionContext = new ActionContext(httpContext, new RouteData(), actionDescriptor);

ActionExecutedContext context = new ActionExecutedContext(actionContext, new List<IFilterMetadata>(), "someController");
context.Result = new ObjectResult(new { Error = "Error", Message = "Message" }) { StatusCode = 500 };

EnableQueryAttribute attribute = new EnableQueryAttribute();

// Act and Assert
ExceptionAssert.DoesNotThrow(() => attribute.OnActionExecuted(context));
}

[Fact]
public void OnActionExecuted_HandlesRequestsNormally()
{
// Arrange
var routeName = "odata";
IEdmModel model = new CustomersModelWithInheritance().Model;
var configuration = RoutingConfigurationFactory.Create();

configuration.Filter();

var request = RequestFactory.CreateFromModel(model, "http://localhost/odata/Customers?$filter=Id eq 1", routeName, new ODataPath());

IServiceProvider serviceProvider = GetServiceProvider(configuration, model, routeName);
request.ODataFeature().RequestContainer = serviceProvider;
HttpContext httpContext = request.HttpContext;
httpContext.RequestServices = serviceProvider;

ActionDescriptor actionDescriptor = ControllerDescriptorFactory
.Create(configuration, "CustomersController", typeof(CustomersController))
.First(descriptor => descriptor.ActionName.StartsWith("Get", StringComparison.OrdinalIgnoreCase));
ActionContext actionContext = new ActionContext(httpContext, new RouteData(), actionDescriptor);

ActionExecutedContext context = new ActionExecutedContext(actionContext, new List<IFilterMetadata>(), new CustomersController());
context.Result = new ObjectResult(new List<Customer>()) { StatusCode = 200 };

EnableQueryAttribute attribute = new EnableQueryAttribute();

// Act and Assert
ExceptionAssert.DoesNotThrow(() => attribute.OnActionExecuted(context));

Assert.NotNull(context.Result as ObjectResult);
}

private IServiceProvider GetServiceProvider(IRouteBuilder builder, IEdmModel model, string routeName)
{
IPerRouteContainer perRouteContainer = builder.ServiceProvider.GetRequiredService<IPerRouteContainer>();

// Create an service provider for this route. Add the default services to the custom configuration actions.
Action<IContainerBuilder> builderAction = ODataRouteBuilderExtensions.ConfigureDefaultServices(builder, b =>
{
b.AddService(Microsoft.OData.ServiceLifetime.Singleton, sp => model);
});
return perRouteContainer.CreateODataRootContainer(routeName, builderAction);
}

[Fact]
public void OnActionExecuting_Throws_Null_Context()
{
Expand Down