[OData] Refactor QueryableAttribute's ValidateQuery method to optionally validate the ODataQueryOptions as well

Author: youssefm

src/System.Web.Http.OData/QueryableAttribute.cs

@@ -158,8 +158,6 @@ public override void OnActionExecuted(HttpActionExecutedContext actionExecutedCo
                 if (responseContent.Value != null && request.RequestUri != null &&
                     (!String.IsNullOrWhiteSpace(request.RequestUri.Query) || _resultLimit.HasValue))
                 {
-                    ValidateQuery(request);
-
                     try
                     {
                         IEnumerable query = responseContent.Value as IEnumerable;
@@ -232,16 +230,7 @@ private IQueryable ExecuteQuery(IEnumerable query, HttpRequestMessage request, H
 
             ODataQueryContext queryContext = CreateQueryContext(entityClrType, configuration, actionDescriptor);
             ODataQueryOptions queryOptions = new ODataQueryOptions(queryContext, request);
-
-            // Filter and OrderBy require entity sets.  Top and Skip may accept primitives.
-            if (queryContext.IsPrimitiveClrType && (queryOptions.Filter != null || queryOptions.OrderBy != null))
-            {
-                // An attempt to use a query option not allowed for primitive types
-                // generates a BadRequest with a general message that avoids information disclosure.
-                throw new HttpResponseException(request.CreateErrorResponse(
-                                                    HttpStatusCode.BadRequest,
-                                                    SRResources.OnlySkipAndTopSupported));
-            }
+            ValidateQuery(request, queryOptions);
 
             // apply the query
             IQueryable queryable = query as IQueryable;
@@ -287,22 +276,27 @@ private static ODataQueryContext CreateQueryContext(Type entityClrType, HttpConf
         }
 
         /// <summary>
-        /// Validates that the OData query parameters of the incoming request are supported.
+        /// Validates that the OData query of the incoming request is supported.
         /// </summary>
+        /// <param name="request">The incoming request</param>
+        /// <param name="queryOptions">The <see cref="ODataQueryOptions"/> instance constructed based on the incoming request.</param>
         /// <remarks>
-        /// Override this method to add support for new OData query parameters
-        /// Throw <see cref="HttpResponseException"/> for unsupported query parameters.
+        /// Override this method to perform additional validation of the query. By default, the implementation
+        /// throws an exception if the query contains unsupported query parameters.
         /// </remarks>
-        /// <param name="request">The incoming request</param>
-        /// <exception cref="HttpResponseException">The request contains unsupported OData query parameters.</exception>
         [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Response disposed after being sent.")]
-        public virtual void ValidateQuery(HttpRequestMessage request)
+        public virtual void ValidateQuery(HttpRequestMessage request, ODataQueryOptions queryOptions)
         {
             if (request == null)
             {
                 throw Error.ArgumentNull("request");
             }
 
+            if (queryOptions == null)
+            {
+                throw Error.ArgumentNull("queryOptions");
+            }
+
             IEnumerable<KeyValuePair<string, string>> queryParameters = request.GetQueryNameValuePairs();
             foreach (KeyValuePair<string, string> kvp in queryParameters)
             {
@@ -314,6 +308,16 @@ public virtual void ValidateQuery(HttpRequestMessage request)
                         Error.Format(SRResources.QueryParameterNotSupported, kvp.Key)));
                 }
             }
+
+            // Filter and OrderBy require entity sets.  Top and Skip may accept primitives.
+            if (queryOptions.Context.IsPrimitiveClrType && (queryOptions.Filter != null || queryOptions.OrderBy != null))
+            {
+                // An attempt to use a query option not allowed for primitive types
+                // generates a BadRequest with a general message that avoids information disclosure.
+                throw new HttpResponseException(request.CreateErrorResponse(
+                                                    HttpStatusCode.BadRequest,
+                                                    SRResources.OnlySkipAndTopSupported));
+            }
         }
     }
 }

test/System.Web.Http.OData.Test/QueryableAttributeTests.cs

@@ -11,6 +11,7 @@
 using System.Web.Http.Controllers;
 using System.Web.Http.Filters;
 using System.Web.Http.Hosting;
+using System.Web.Http.OData.Builder;
 using System.Web.Http.OData.Query;
 using System.Web.Http.OData.Query.Controllers;
 using System.Web.Http.OData.TestCommon.Models;
@@ -357,9 +358,21 @@ public void ValidateQuery_Throws_With_Null_Request()
         {
             // Arrange
             QueryableAttribute attribute = new QueryableAttribute();
+            var model = new ODataModelBuilder().Add_Customer_EntityType().Add_Customers_EntitySet().GetEdmModel();
+            var options = new ODataQueryOptions(new ODataQueryContext(model, typeof(System.Web.Http.OData.Builder.TestModels.Customer), "Customers"), new HttpRequestMessage());
 
             // Act & Assert
-            Assert.ThrowsArgumentNull(() => attribute.ValidateQuery(null), "request");
+            Assert.ThrowsArgumentNull(() => attribute.ValidateQuery(null, options), "request");
+        }
+
+        [Fact]
+        public void ValidateQuery_Throws_WithNullQueryOptions()
+        {
+            // Arrange
+            QueryableAttribute attribute = new QueryableAttribute();
+
+            // Act & Assert
+            Assert.ThrowsArgumentNull(() => attribute.ValidateQuery(new HttpRequestMessage(), null), "queryOptions");
         }
 
         [Theory]
@@ -368,10 +381,12 @@ public void ValidateQuery_Accepts_All_Supported_QueryNames(string queryName)
         {
             // Arrange
             QueryableAttribute attribute = new QueryableAttribute();
-            HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/?" + queryName);
+            HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/?" + queryName + "=x");
+            var model = new ODataModelBuilder().Add_Customer_EntityType().Add_Customers_EntitySet().GetEdmModel();
+            var options = new ODataQueryOptions(new ODataQueryContext(model, typeof(System.Web.Http.OData.Builder.TestModels.Customer), "Customers"), request);
 
             // Act & Assert
-            attribute.ValidateQuery(request);
+            attribute.ValidateQuery(request, options);
         }
 
         [Theory]
@@ -381,10 +396,12 @@ public void ValidateQuery_Sends_BadRequest_For_Unsupported_QueryNames(string que
             // Arrange
             QueryableAttribute attribute = new QueryableAttribute();
             HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/?" + queryName);
+            var model = new ODataModelBuilder().Add_Customer_EntityType().Add_Customers_EntitySet().GetEdmModel();
+            var options = new ODataQueryOptions(new ODataQueryContext(model, typeof(System.Web.Http.OData.Builder.TestModels.Customer), "Customers"), request);
 
             // Act & Assert
             HttpResponseException responseException = Assert.Throws<HttpResponseException>(
-                                                                () => attribute.ValidateQuery(request));
+                                                                () => attribute.ValidateQuery(request, options));
 
             Assert.Equal(HttpStatusCode.BadRequest, responseException.Response.StatusCode);
         }
@@ -395,10 +412,12 @@ public void ValidateQuery_Sends_BadRequest_For_Unrecognized_QueryNames()
             // Arrange
             QueryableAttribute attribute = new QueryableAttribute();
             HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/?$xxx");
+            var model = new ODataModelBuilder().Add_Customer_EntityType().Add_Customers_EntitySet().GetEdmModel();
+            var options = new ODataQueryOptions(new ODataQueryContext(model, typeof(System.Web.Http.OData.Builder.TestModels.Customer), "Customers"), request);
 
             // Act & Assert
             HttpResponseException responseException = Assert.Throws<HttpResponseException>(
-                                                                () => attribute.ValidateQuery(request));
+                                                                () => attribute.ValidateQuery(request, options));
 
             Assert.Equal(HttpStatusCode.BadRequest, responseException.Response.StatusCode);
         }
@@ -408,13 +427,10 @@ public void ValidateQuery_Can_Override_Base()
         {
             // Arrange
             Mock<QueryableAttribute> mockAttribute = new Mock<QueryableAttribute>();
-            mockAttribute.Setup(m => m.ValidateQuery(It.IsAny<HttpRequestMessage>())).Callback(() => { }).Verifiable();
-
-            QueryableAttribute attribute = new QueryableAttribute();
-            HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/?$xxx");
+            mockAttribute.Setup(m => m.ValidateQuery(It.IsAny<HttpRequestMessage>(), It.IsAny<ODataQueryOptions>())).Callback(() => { }).Verifiable();
 
             // Act & Assert
-            mockAttribute.Object.ValidateQuery(null);
+            mockAttribute.Object.ValidateQuery(null, null);
             mockAttribute.Verify();
         }