EnableQuery should perform checks before controller execution (#51)

* Porting change to v8

* Adding commit signing from VS

* Removing commit

* Removing the config file since it is not working

* Trying with GitConfig

* Removing gitconfig

* Removing unnecessary usings

* Fixing usings

* Update ODataOptionsTests.cs

Co-authored-by: Sam Xu <saxu@microsoft.com>

src/Microsoft.AspNetCore.OData/Edm/ClrTypeCache.cs

@@ -12,18 +12,34 @@ namespace Microsoft.AspNetCore.OData.Edm
     /// </summary>
     internal class ClrTypeCache
     {
-        private ConcurrentDictionary<Type, IEdmTypeReference> _cache = new ConcurrentDictionary<Type, IEdmTypeReference>();
+        private ConcurrentDictionary<Type, IEdmTypeReference> _clrToEdmTypeCache =
+            new ConcurrentDictionary<Type, IEdmTypeReference>();
+
+        private ConcurrentDictionary<IEdmTypeReference, Type> _edmToClrTypeCache = new ConcurrentDictionary<IEdmTypeReference, Type>();
 
         public IEdmTypeReference GetEdmType(Type clrType, IEdmModel model)
         {
             IEdmTypeReference edmType;
-            if (!_cache.TryGetValue(clrType, out edmType))
+            if (!_clrToEdmTypeCache.TryGetValue(clrType, out edmType))
             {
                 edmType = model.GetEdmTypeReference(clrType);
-                _cache[clrType] = edmType;
+                _clrToEdmTypeCache[clrType] = edmType;
             }
 
             return edmType;
         }
+
+        public Type GetClrType(IEdmTypeReference edmType, IEdmModel edmModel)
+        {
+            Type clrType;
+
+            if (!_edmToClrTypeCache.TryGetValue(edmType, out clrType))
+            {
+                clrType = edmModel.GetClrType(edmType);
+                _edmToClrTypeCache[edmType] = clrType;
+            }
+
+            return clrType;
+        }
     }
 }

src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml

@@ -7267,6 +7267,18 @@
             Gets or sets the maximum number of expressions that can be present in the $orderby.
             </summary>
         </member>
+        <member name="F:Microsoft.AspNetCore.OData.Query.EnableQueryAttribute._queryValidationRunBeforeActionExecution">
+            <summary>
+            Marks if the query validation was run before the action execution. This is not always possible.
+            For cases where the run failed before action execution. We will run validation on result.
+            </summary>
+        </member>
+        <member name="F:Microsoft.AspNetCore.OData.Query.EnableQueryAttribute._processedQueryOptions">
+            <summary>
+            Stores the processed query options to be used later if OnActionExecuting was able to verify the query.
+            This is because ValidateQuery internally modifies query options (expands are prime example of this).
+            </summary>
+        </member>
         <member name="M:Microsoft.AspNetCore.OData.Query.EnableQueryAttribute.OnActionExecuting(Microsoft.AspNetCore.Mvc.Filters.ActionExecutingContext)">
             <summary>
             Performs the query composition before action is executing.

src/Microsoft.AspNetCore.OData/Query/EnableQueryAttribute.cs

@@ -9,6 +9,7 @@
 using System.Linq;
 using System.Net;
 using System.Reflection;
+using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Extensions;
 using Microsoft.AspNetCore.Mvc;
@@ -20,6 +21,7 @@
 using Microsoft.AspNetCore.OData.Extensions;
 using Microsoft.AspNetCore.OData.Formatter;
 using Microsoft.AspNetCore.OData.Results;
+using Microsoft.AspNetCore.OData.Routing;
 using Microsoft.OData;
 using Microsoft.OData.Edm;
 using Microsoft.OData.UriParser;
@@ -35,6 +37,18 @@ namespace Microsoft.AspNetCore.OData.Query
     [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, Inherited = true, AllowMultiple = false)]
     public partial class EnableQueryAttribute : ActionFilterAttribute
     {
+        /// <summary>
+        /// Marks if the query validation was run before the action execution. This is not always possible.
+        /// For cases where the run failed before action execution. We will run validation on result.
+        /// </summary>
+        private bool _queryValidationRunBeforeActionExecution;
+
+        /// <summary>
+        /// Stores the processed query options to be used later if OnActionExecuting was able to verify the query.
+        /// This is because ValidateQuery internally modifies query options (expands are prime example of this).
+        /// </summary>
+        private ODataQueryOptions _processedQueryOptions;
+
         /// <summary>
         /// Performs the query composition before action is executing.
         /// </summary>
@@ -46,7 +60,143 @@ public override void OnActionExecuting(ActionExecutingContext actionExecutingCon
                 throw new ArgumentNullException(nameof(actionExecutingContext));
             }
 
-            // TODO: We should put the validation here.
+            base.OnActionExecuting(actionExecutingContext);
+
+            HttpRequest request = actionExecutingContext.HttpContext.Request;
+            ODataPath path = request.ODataFeature().Path;
+
+            ODataQueryContext queryContext;
+
+            // For OData based controllers.
+            if (path != null)
+            {
+                IEdmType edmType = path.GetEdmType();
+
+                // When $count is at the end, the return type is always int. Trying to instead fetch the return type of the actual type being counted on.
+                if (request.IsCountRequest())
+                {
+                    ODataPathSegment[] pathSegments = path.ToArray();
+                    edmType = pathSegments[pathSegments.Length - 2].EdmType;
+                }
+
+                IEdmType elementType = edmType.AsElementType();
+
+                IEdmModel edmModel = request.GetModel();
+
+                // For Swagger metadata request. elementType is null.
+                if (elementType == null || edmModel == null)
+                {
+                    _queryValidationRunBeforeActionExecution = false;
+                    return;
+                }
+
+                Type clrType = edmModel.GetTypeMappingCache().GetClrType(
+                    elementType.ToEdmTypeReference(isNullable: false),
+                    edmModel);
+
+                // CLRType can be missing if untyped registrations were made.
+                if (clrType != null)
+                {
+                    queryContext = new ODataQueryContext(edmModel, clrType, path);
+                }
+                else
+                {
+                    // In case where CLRType is missing, $count, $expand verifications cannot be done.
+                    // More importantly $expand required ODataQueryContext with clrType which cannot be done
+                    // If the model is untyped. Hence for such cases, letting the validation run post action.
+                    _queryValidationRunBeforeActionExecution = false;
+                    return;
+                }
+
+                _queryValidationRunBeforeActionExecution = true;
+            }
+            else
+            {
+                // For non-OData Json based controllers.
+                // For these cases few options are supported like IEnumerable<T>, Task<IEnumerable<T>>, T, Task<T>
+                // Other cases where we cannot determine the return type upfront, are not supported
+                // Like IActionResult, SingleResult. For such cases, the validation is run in OnActionExecuted
+                // When we have the result.
+                ControllerActionDescriptor controllerActionDescriptor = actionExecutingContext.ActionDescriptor as ControllerActionDescriptor;
+
+                if (controllerActionDescriptor == null)
+                {
+                    _queryValidationRunBeforeActionExecution = false;
+                    return;
+                }
+
+                Type returnType = controllerActionDescriptor.MethodInfo.ReturnType;
+                Type elementType;
+
+                // For Task<> get the base object.
+                if (returnType.IsGenericType && returnType.GetGenericTypeDefinition() == typeof(Task<>))
+                {
+                    returnType = returnType.GetGenericArguments().First();
+                }
+
+                // For NetCore2.2+ new type ActionResult<> was created which encapculates IActionResult and T result.
+                if (returnType.IsGenericType && returnType.GetGenericTypeDefinition() == typeof(ActionResult<>))
+                {
+                    returnType = returnType.GetGenericArguments().First();
+                }
+
+                if (TypeHelper.IsCollection(returnType))
+                {
+                    elementType = TypeHelper.GetImplementedIEnumerableType(returnType);
+                }
+                else if (TypeHelper.IsGenericType(returnType) && returnType.GetGenericTypeDefinition() == typeof(Task<>))
+                {
+                    elementType = returnType.GetGenericArguments().First();
+                }
+                else
+                {
+                    _queryValidationRunBeforeActionExecution = false;
+                    return;
+                }
+
+                IEdmModel edmModel = GetModel(
+                    elementType,
+                    request,
+                    controllerActionDescriptor);
+
+                queryContext = new ODataQueryContext(
+                    edmModel,
+                    elementType);
+                _queryValidationRunBeforeActionExecution = true;
+            }
+
+            // Create and validate the query options.
+            _processedQueryOptions = new ODataQueryOptions(queryContext, request);
+
+            try
+            {
+                ValidateQuery(request, _processedQueryOptions);
+            }
+            catch (ArgumentOutOfRangeException e)
+            {
+                actionExecutingContext.Result = CreateBadRequestResult(
+                    Error.Format(SRResources.QueryParameterNotSupported, e.Message),
+                    e);
+            }
+            catch (NotImplementedException e)
+            {
+                actionExecutingContext.Result = CreateBadRequestResult(
+                    Error.Format(SRResources.UriQueryStringInvalid, e.Message),
+                    e);
+            }
+            catch (NotSupportedException e)
+            {
+                actionExecutingContext.Result = CreateBadRequestResult(
+                    Error.Format(SRResources.UriQueryStringInvalid, e.Message),
+                    e);
+            }
+            catch (InvalidOperationException e)
+            {
+                // Will also catch ODataException here because ODataException derives from InvalidOperationException.
+                actionExecutingContext.Result = CreateBadRequestResult(
+                    Error.Format(SRResources.UriQueryStringInvalid, e.Message),
+                    e);
+            }
         }
 
         /// <summary>
@@ -397,7 +547,13 @@ public virtual object ApplyQuery(object entity, ODataQueryOptions queryOptions)
         /// <returns></returns>
         private ODataQueryOptions CreateAndValidateQueryOptions(HttpRequest request, ODataQueryContext queryContext)
         {
+            if (_queryValidationRunBeforeActionExecution)
+            {
+                return _processedQueryOptions;
+            }
+
             ODataQueryOptions queryOptions = new ODataQueryOptions(queryContext, request);
+
             ValidateQuery(request, queryOptions);
 
             return queryOptions;

test/Microsoft.AspNetCore.OData.E2E.Tests/Microsoft.AspNetCore.OData.E2E.Tests.csproj

@@ -43,5 +43,9 @@
     <ProjectReference Include="..\..\src\Microsoft.AspNetCore.OData\Microsoft.AspNetCore.OData.csproj" />
     <ProjectReference Include="..\Microsoft.AspNetCore.OData.TestCommon\Microsoft.AspNetCore.OData.TestCommon.csproj" />
   </ItemGroup>
+
+  <ItemGroup>
+    <Folder Include="Query\" />
+  </ItemGroup>
 
 </Project>

test/Microsoft.AspNetCore.OData.E2E.Tests/Query/ActionResult/ActionResultController.cs

@@ -0,0 +1,33 @@
+// Copyright (c) Microsoft Corporation.  All rights reserved.
+// Licensed under the MIT License.  See License.txt in the project root for license information.
+
+using System.Collections.Generic;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.OData.Query;
+
+namespace Microsoft.AspNetCore.OData.E2E.Tests.Query.ActionResult
+{
+    public class CustomersController : ControllerBase
+    {
+        [HttpGet]
+        [EnableQuery(AllowedQueryOptions = AllowedQueryOptions.Expand)]
+        public async Task<ActionResult<IEnumerable<Customer>>> Get()
+        {
+            return await Task.FromResult(new List<Customer>
+            { 
+                new Customer
+                {
+                    Id = "CustId",
+                    Books = new List<Book>
+                    {
+                        new Book
+                        {
+                            Id = "BookId",
+                        },
+                    },
+                },
+            });
+        }
+    }
+}

test/Microsoft.AspNetCore.OData.E2E.Tests/Query/ActionResult/ActionResultEdmModel.cs

@@ -0,0 +1,18 @@
+// Copyright (c) Microsoft Corporation.  All rights reserved.
+// Licensed under the MIT License.  See License.txt in the project root for license information.
+
+using Microsoft.OData.Edm;
+using Microsoft.OData.ModelBuilder;
+
+namespace Microsoft.AspNetCore.OData.E2E.Tests.Query.ActionResult
+{
+    public class ActionResultEdmModel
+    {
+        public static IEdmModel GetEdmModel()
+        {
+            var builder = new ODataConventionModelBuilder();
+            builder.EntitySet<Customer>("Customers");
+            return builder.GetEdmModel();
+        }
+    }
+}

test/Microsoft.AspNetCore.OData.E2E.Tests/Query/ActionResult/ActionResultODataModel.cs

@@ -0,0 +1,19 @@
+// Copyright (c) Microsoft Corporation.  All rights reserved.
+// Licensed under the MIT License.  See License.txt in the project root for license information.
+
+using System.Collections.Generic;
+
+namespace Microsoft.AspNetCore.OData.E2E.Tests.Query.ActionResult
+{
+    public class Customer
+    {
+        public string Id { get; set; }
+
+        public IEnumerable<Book> Books { get; set; }
+    }
+
+    public class Book
+    {
+        public string Id { get; set; }
+    }
+}