Skip to content

Commit 78b8383

Browse files
ificatorificator
authored
Avoid duplicate instantiation of ODataQueryOptions (#2766)
1 parent bf4931a commit 78b8383

11 files changed

Lines changed: 187 additions & 7 deletions

File tree

‎src/Microsoft.AspNet.OData.Shared/CompatibilityOptions.cs‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
//------------------------------------------------------------------------------
77

88
using System;
9+
using System.Runtime.CompilerServices;
910

1011
namespace Microsoft.AspNet.OData
1112
{
@@ -33,6 +34,29 @@ public enum CompatibilityOptions
3334
/// <summary>
3435
/// Throw exception after logging ModelState error.
3536
/// </summary>
36-
ThrowExceptionAfterLoggingModelStateError = 0x4
37+
ThrowExceptionAfterLoggingModelStateError = 0x4,
38+
39+
/// <summary>
40+
/// Disable the reuse of the ODataQueryOptions instance generated during model binding in EnableQueryAttribute.
41+
/// </summary>
42+
DisableODataQueryOptionsReuse = 0x8,
43+
}
44+
45+
/// <summary>
46+
/// Extension methods for <see cref="CompatibilityOptions"/>.
47+
/// </summary>
48+
public static class CompatibilityOptionsExtensions
49+
{
50+
/// <summary>
51+
/// Determines whether the provided option is set.
52+
/// </summary>
53+
/// <param name="options">The set options.</param>
54+
/// <param name="option">The option to check.</param>
55+
/// <returns>True if the option is set, false otherwise.</returns>
56+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
57+
public static bool HasOption(this CompatibilityOptions options, CompatibilityOptions option)
58+
{
59+
return (options & option) == option;
60+
}
3761
}
3862
}

‎src/Microsoft.AspNet.OData.Shared/GetNextPageHelper.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ internal static Uri GetNextPageLink(Uri requestUri, IEnumerable<KeyValuePair<str
4343
if (Int32.TryParse(value, out top))
4444
{
4545
// We decrease top by the pageSize because that's the number of results we're returning in the current page. If the $top query option's value is less than or equal to the page size, there is no next page.
46-
if ((options & CompatibilityOptions.AllowNextLinkWithNonPositiveTopValue) != 0 || top > pageSize)
46+
if (options.HasOption(CompatibilityOptions.AllowNextLinkWithNonPositiveTopValue) || top > pageSize)
4747
{
4848
value = (top - pageSize).ToString(CultureInfo.InvariantCulture);
4949
}

‎src/Microsoft.AspNet.OData.Shared/Query/ODataQueryOptions.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,7 @@ private static void ExtractGroupingProperties(List<string> result, IEnumerable<G
531531
public virtual object ApplyTo(object entity, ODataQuerySettings querySettings, AllowedQueryOptions ignoreQueryOptions)
532532
{
533533
_ignoreQueryOptions = ignoreQueryOptions;
534-
return ApplyTo(entity, new ODataQuerySettings());
534+
return ApplyTo(entity, querySettings);
535535
}
536536

537537
/// <summary>

‎src/Microsoft.AspNet.OData/EnableQueryAttribute.cs‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
using Microsoft.AspNet.OData.Adapters;
2020
using Microsoft.AspNet.OData.Common;
2121
using Microsoft.AspNet.OData.Extensions;
22-
using Microsoft.AspNet.OData.Formatter;
2322
using Microsoft.AspNet.OData.Query;
2423
using Microsoft.OData.Edm;
2524

@@ -184,7 +183,22 @@ private static string GenerateExpandQueryFromPayload(HttpActionContext context)
184183
/// <returns></returns>
185184
private ODataQueryOptions CreateAndValidateQueryOptions(HttpRequestMessage request, ODataQueryContext queryContext)
186185
{
187-
ODataQueryOptions queryOptions = new ODataQueryOptions(queryContext, request);
186+
ODataQueryOptions queryOptions = null;
187+
188+
CompatibilityOptions compatibilityOptions = request.GetCompatibilityOptions();
189+
if (!compatibilityOptions.HasOption(CompatibilityOptions.DisableODataQueryOptionsReuse))
190+
{
191+
queryOptions = request.GetODataQueryOptions();
192+
}
193+
194+
// Only create new query options if we haven't already.
195+
if (queryOptions == null)
196+
{
197+
queryOptions = new ODataQueryOptions(queryContext, request);
198+
}
199+
200+
// Even if we didn't generate a new set of query options here we'll still validate because we cannot
201+
// guarantee that it's already been done.
188202
ValidateQuery(request, queryOptions);
189203

190204
return queryOptions;

‎src/Microsoft.AspNet.OData/Extensions/HttpRequestMessageExtensions.cs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ namespace Microsoft.AspNet.OData.Extensions
3636
[EditorBrowsable(EditorBrowsableState.Never)]
3737
public static class HttpRequestMessageExtensions
3838
{
39+
private const string ODataQueryOptionsKey = "Microsoft.AspNet.OData.ODataQueryOptions";
3940
private const string PropertiesKey = "Microsoft.AspNet.OData.Properties";
4041
private const string RequestContainerKey = "Microsoft.AspNet.OData.RequestContainer";
4142
private const string RequestScopeKey = "Microsoft.AspNet.OData.RequestScope";
@@ -239,6 +240,17 @@ public static Uri GetNextPageLink(this HttpRequestMessage request, int pageSize,
239240
return GetNextPageHelper.GetNextPageLink(request.RequestUri, request.GetQueryNameValuePairs(), pageSize, instance, objToSkipTokenValue, options);
240241
}
241242

243+
/// <summary>
244+
/// Gets the ODataQueryOptions for the request.
245+
/// </summary>
246+
/// <param name="request">The request.</param>
247+
/// <returns>The query options.</returns>
248+
public static ODataQueryOptions GetODataQueryOptions(this HttpRequestMessage request)
249+
{
250+
request.Properties.TryGetValue(ODataQueryOptionsKey, out object untypedQueryOptions);
251+
return untypedQueryOptions as ODataQueryOptions;
252+
}
253+
242254
/// <summary>
243255
/// Gets the dependency injection container for the OData request.
244256
/// </summary>
@@ -432,6 +444,16 @@ public static IExpandQueryBuilder GetExpandQueryBuilder(this HttpRequestMessage
432444
return request.GetRequestContainer().GetRequiredService<IExpandQueryBuilder>();
433445
}
434446

447+
/// <summary>
448+
/// Sets the ODataQueryOptions for the request.
449+
/// </summary>
450+
/// <param name="request">The request.</param>
451+
/// <param name="queryOptions">The query options.</param>
452+
public static void SetODataQueryOptions(this HttpRequestMessage request, ODataQueryOptions queryOptions)
453+
{
454+
request.Properties[ODataQueryOptionsKey] = queryOptions;
455+
}
456+
435457
/// <summary>
436458
/// Checks whether the request is a POST targeted at a resource path ending in /$query.
437459
/// </summary>

‎src/Microsoft.AspNet.OData/Formatter/Deserialization/ODataDeserializerContext.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public HttpRequestMessage Request
3636

3737
// We add this setting via CompatibilityOptions
3838
CompatibilityOptions options = webApiRequestMessage != null ? webApiRequestMessage.Configuration.GetCompatibilityOptions() : CompatibilityOptions.None;
39-
DisableCaseInsensitiveRequestPropertyBinding = options.HasFlag(CompatibilityOptions.DisableCaseInsensitiveRequestPropertyBinding) ? true : false;
39+
DisableCaseInsensitiveRequestPropertyBinding = options.HasOption(CompatibilityOptions.DisableCaseInsensitiveRequestPropertyBinding);
4040
}
4141
}
4242

‎src/Microsoft.AspNet.OData/ODataQueryParameterBindingAttribute.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,9 @@ public override Task ExecuteBindingAsync(ModelMetadataProvider metadataProvider,
9090
ODataQueryOptions parameterValue = createODataQueryOptions(entitySetContext, request);
9191
SetValue(actionContext, parameterValue);
9292

93+
// Make sure the instance is saved on the request so it can be used later instead of reparsing.
94+
request.SetODataQueryOptions(parameterValue);
95+
9396
return TaskHelpers.Completed();
9497
}
9598

‎test/UnitTest/Microsoft.AspNet.OData.Test.Shared/Query/EnableQueryAttributeTest.cs‎

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1154,6 +1154,71 @@ public async Task OnActionExecuted_SingleResult_WithMoreThanASingleQueryResult_R
11541154
responseString);
11551155
}
11561156

1157+
#if !NETCORE
1158+
[Fact]
1159+
public void OnActionExecuted_UseCachedODataQueryOptions()
1160+
{
1161+
var model = new CustomersModelWithInheritance();
1162+
model.Model.SetAnnotationValue(model.Customer, new ClrTypeAnnotation(typeof(Customer)));
1163+
1164+
Customer customer = new Customer();
1165+
SingleResult singleResult = new SingleResult<Customer>(new Customer[] { customer }.AsQueryable());
1166+
HttpActionExecutedContext actionExecutedContext = GetActionExecutedContext("http://localhost/", singleResult);
1167+
1168+
ODataQueryOptions actualQueryOptions = null;
1169+
ODataQueryOptions expectedQueryOptions = new ODataQueryOptions(
1170+
new ODataQueryContext(model.Model, typeof(Customer)),
1171+
actionExecutedContext.Request);
1172+
1173+
actionExecutedContext.Request.SetODataQueryOptions(expectedQueryOptions);
1174+
1175+
var mockAttribute = new Mock<EnableQueryAttribute>
1176+
{
1177+
CallBase = true,
1178+
};
1179+
mockAttribute
1180+
.Setup(x => x.ValidateQuery(It.IsAny<HttpRequestMessage>(), It.IsAny<ODataQueryOptions>()))
1181+
.Callback<HttpRequestMessage, ODataQueryOptions>((r, o) => { actualQueryOptions = o; });
1182+
1183+
mockAttribute.Object.OnActionExecuted(actionExecutedContext);
1184+
1185+
Assert.Same(expectedQueryOptions, actualQueryOptions);
1186+
}
1187+
1188+
[Fact]
1189+
public void OnActionExecuted_UseCachedODataQueryOptionsDisabled()
1190+
{
1191+
var model = new CustomersModelWithInheritance();
1192+
model.Model.SetAnnotationValue(model.Customer, new ClrTypeAnnotation(typeof(Customer)));
1193+
1194+
Customer customer = new Customer();
1195+
SingleResult singleResult = new SingleResult<Customer>(new Customer[] { customer }.AsQueryable());
1196+
HttpActionExecutedContext actionExecutedContext = GetActionExecutedContext(
1197+
"http://localhost/",
1198+
singleResult,
1199+
CompatibilityOptions.DisableODataQueryOptionsReuse);
1200+
1201+
ODataQueryOptions actualQueryOptions = null;
1202+
ODataQueryOptions expectedQueryOptions = new ODataQueryOptions(
1203+
new ODataQueryContext(model.Model, typeof(Customer)),
1204+
actionExecutedContext.Request);
1205+
1206+
actionExecutedContext.Request.SetODataQueryOptions(expectedQueryOptions);
1207+
1208+
var mockAttribute = new Mock<EnableQueryAttribute>
1209+
{
1210+
CallBase = true,
1211+
};
1212+
mockAttribute
1213+
.Setup(x => x.ValidateQuery(It.IsAny<HttpRequestMessage>(), It.IsAny<ODataQueryOptions>()))
1214+
.Callback<HttpRequestMessage, ODataQueryOptions>((r, o) => { actualQueryOptions = o; });
1215+
1216+
mockAttribute.Object.OnActionExecuted(actionExecutedContext);
1217+
1218+
Assert.NotSame(expectedQueryOptions, actualQueryOptions);
1219+
}
1220+
#endif
1221+
11571222
[Theory]
11581223
[InlineData("$filter=ID eq 1")]
11591224
[InlineData("$orderby=ID")]
@@ -1238,14 +1303,23 @@ private void SomeAction()
12381303
{
12391304
}
12401305

1241-
private static HttpActionExecutedContext GetActionExecutedContext<TResponse>(string uri, TResponse result)
1306+
private static HttpActionExecutedContext GetActionExecutedContext<TResponse>(
1307+
string uri,
1308+
TResponse result,
1309+
CompatibilityOptions? compatibilityOptions = null)
12421310
{
12431311
var request = new HttpRequestMessage(HttpMethod.Get, uri);
12441312
request.EnableODataDependencyInjectionSupport();
12451313
var actionContext = ContextUtil.CreateActionContext(ContextUtil.CreateControllerContext(request: request));
12461314
var response = request.CreateResponse<TResponse>(HttpStatusCode.OK, result);
12471315
var actionExecutedContext = new HttpActionExecutedContext { ActionContext = actionContext, Response = response };
12481316
actionContext.ActionDescriptor.Configuration = request.GetConfiguration();
1317+
1318+
if (compatibilityOptions.HasValue)
1319+
{
1320+
actionContext.ActionDescriptor.Configuration.SetCompatibilityOptions(compatibilityOptions.GetValueOrDefault());
1321+
}
1322+
12491323
return actionExecutedContext;
12501324
}
12511325
#endif

‎test/UnitTest/Microsoft.AspNet.OData.Test/PublicApi/Microsoft.AspNet.OData.PublicApi.bsl‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ FlagsAttribute(),
44
public enum Microsoft.AspNet.OData.CompatibilityOptions : int {
55
AllowNextLinkWithNonPositiveTopValue = 1
66
DisableCaseInsensitiveRequestPropertyBinding = 2
7+
DisableODataQueryOptionsReuse = 8
78
None = 0
89
ThrowExceptionAfterLoggingModelStateError = 4
910
}
@@ -218,6 +219,16 @@ public abstract class Microsoft.AspNet.OData.TypedDelta : Delta, IDynamicMetaObj
218219
System.Type StructuredType { public abstract get; }
219220
}
220221

222+
[
223+
ExtensionAttribute(),
224+
]
225+
public sealed class Microsoft.AspNet.OData.CompatibilityOptionsExtensions {
226+
[
227+
ExtensionAttribute(),
228+
]
229+
public static bool HasOption (CompatibilityOptions options, CompatibilityOptions option)
230+
}
231+
221232
[
222233
EditorBrowsableAttribute(),
223234
ExtensionAttribute(),
@@ -2193,6 +2204,11 @@ public sealed class Microsoft.AspNet.OData.Extensions.HttpRequestMessageExtensio
21932204
]
21942205
public static System.Uri GetNextPageLink (System.Net.Http.HttpRequestMessage request, int pageSize, object instance, System.Func`2[[System.Object],[System.String]] objToSkipTokenValue)
21952206

2207+
[
2208+
ExtensionAttribute(),
2209+
]
2210+
public static ODataQueryOptions GetODataQueryOptions (System.Net.Http.HttpRequestMessage request)
2211+
21962212
[
21972213
ExtensionAttribute(),
21982214
]
@@ -2227,6 +2243,11 @@ public sealed class Microsoft.AspNet.OData.Extensions.HttpRequestMessageExtensio
22272243
ExtensionAttribute(),
22282244
]
22292245
public static HttpRequestMessageProperties ODataProperties (System.Net.Http.HttpRequestMessage request)
2246+
2247+
[
2248+
ExtensionAttribute(),
2249+
]
2250+
public static void SetODataQueryOptions (System.Net.Http.HttpRequestMessage request, ODataQueryOptions queryOptions)
22302251
}
22312252

22322253
[

‎test/UnitTest/Microsoft.AspNetCore.OData.Test/PublicApi/Microsoft.AspNetCore.OData.PublicApi.bsl‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ FlagsAttribute(),
44
public enum Microsoft.AspNet.OData.CompatibilityOptions : int {
55
AllowNextLinkWithNonPositiveTopValue = 1
66
DisableCaseInsensitiveRequestPropertyBinding = 2
7+
DisableODataQueryOptionsReuse = 8
78
None = 0
89
ThrowExceptionAfterLoggingModelStateError = 4
910
}
@@ -233,6 +234,16 @@ public abstract class Microsoft.AspNet.OData.TypedDelta : Delta, IDynamicMetaObj
233234
System.Type StructuredType { public abstract get; }
234235
}
235236

237+
[
238+
ExtensionAttribute(),
239+
]
240+
public sealed class Microsoft.AspNet.OData.CompatibilityOptionsExtensions {
241+
[
242+
ExtensionAttribute(),
243+
]
244+
public static bool HasOption (CompatibilityOptions options, CompatibilityOptions option)
245+
}
246+
236247
[
237248
EditorBrowsableAttribute(),
238249
ExtensionAttribute(),

0 commit comments

Comments
 (0)