Skip to content

Commit cc4ceeb

Browse files
Chris Martinezcommonsensesoftware
Chris Martinez
authored and
commonsensesoftware
committed
Fix selecting the assumed, default API version in OData. Fixes #471.
1 parent 1f09f3a commit cc4ceeb

File tree

12 files changed

+251
-74
lines changed

12 files changed

+251
-74
lines changed

Diff for: ‎src/Microsoft.AspNet.OData.Versioning/Microsoft.Web.Http.Versioning/ODataApiVersionRequestProperties.cs

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
namespace Microsoft.Web.Http.Versioning
2+
{
3+
using System;
4+
using System.Collections.Generic;
5+
6+
/// <summary>
7+
/// Represents current OData API versioning request properties.
8+
/// </summary>
9+
public class ODataApiVersionRequestProperties
10+
{
11+
/// <summary>
12+
/// Gets a collection of API version to route name mappings that have been matched in the current request.
13+
/// </summary>
14+
/// <value>A <see cref="IDictionary{TKey, TValue}">collection</see> of key/value pairs representing the mapping
15+
/// of <see cref="ApiVersion">API versions</see> to route names that have been matched in the current request.</value>
16+
public IDictionary<ApiVersion, string> MatchingRoutes { get; } = new Dictionary<ApiVersion, string>();
17+
}
18+
}

Diff for: ‎src/Microsoft.AspNet.OData.Versioning/Routing/UnversionedODataPathRouteConstraint.cs

+39
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22
{
33
using Microsoft.AspNet.OData.Extensions;
44
using Microsoft.Web.Http;
5+
using Microsoft.Web.Http.Versioning;
56
using System.Collections.Generic;
67
using System.Diagnostics.Contracts;
8+
using System.Linq;
79
using System.Net.Http;
810
using System.Web.Http;
911
using System.Web.Http.Routing;
@@ -42,8 +44,42 @@ public bool Match( HttpRequestMessage request, IHttpRoute route, string paramete
4244
return false;
4345
}
4446

47+
var properties = request.ApiVersionProperties();
48+
49+
// determine whether this constraint can match any api version and no api version has otherwise been matched
50+
if ( MatchAnyVersion && properties.RequestedApiVersion == null )
51+
{
52+
var options = request.GetApiVersioningOptions();
53+
54+
// is implicitly matching an api version allowed?
55+
if ( options.AssumeDefaultVersionWhenUnspecified || IsServiceDocumentOrMetadataRoute( values ) )
56+
{
57+
var odata = request.ODataApiVersionProperties();
58+
var model = new ApiVersionModel( odata.MatchingRoutes.Keys, Enumerable.Empty<ApiVersion>() );
59+
var selector = options.ApiVersionSelector;
60+
var requestedApiVersion = properties.RequestedApiVersion = selector.SelectVersion( request, model );
61+
62+
// if an api version is selected, determine if it corresponds to a route that has been previously matched
63+
if ( requestedApiVersion != null && odata.MatchingRoutes.TryGetValue( requestedApiVersion, out var routeName ) )
64+
{
65+
// create a new versioned path constraint on the fly and evaluate it. this sets up the underlying odata
66+
// infrastructure such as the container, edm, etc. this has no bearing the action selector which will
67+
// already select the correct action. without this the response may be incorrect, even if the correct
68+
// action is selected and executed.
69+
var constraint = new VersionedODataPathRouteConstraint( routeName, requestedApiVersion );
70+
return constraint.Match( request, route, parameterName, values, routeDirection );
71+
}
72+
}
73+
}
74+
else if ( !MatchAnyVersion && properties.RequestedApiVersion != apiVersion )
75+
{
76+
return false;
77+
}
78+
4579
request.DeleteRequestContainer( true );
4680

81+
// by evaluating the remaining unversioned constraints, this will ultimately determine whether 400 or 404
82+
// is returned for an odata request
4783
foreach ( var constraint in innerConstraints )
4884
{
4985
if ( constraint.Match( request, route, parameterName, values, routeDirection ) )
@@ -54,5 +90,8 @@ public bool Match( HttpRequestMessage request, IHttpRoute route, string paramete
5490

5591
return false;
5692
}
93+
94+
static bool IsServiceDocumentOrMetadataRoute( IDictionary<string, object> values ) =>
95+
values.TryGetValue( "odataPath", out var value ) && ( value == null || Equals( value, "$metadata" ) );
5796
}
5897
}

Diff for: ‎src/Microsoft.AspNet.OData.Versioning/Routing/VersionedODataPathRouteConstraint.cs

+29-19
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
namespace Microsoft.AspNet.OData.Routing
22
{
3+
using Microsoft.AspNet.OData.Extensions;
34
using Microsoft.OData;
45
using Microsoft.Web.Http;
56
using Microsoft.Web.Http.Versioning;
7+
using System;
68
using System.Collections.Generic;
79
using System.Diagnostics.Contracts;
810
using System.Net.Http;
@@ -52,42 +54,50 @@ public override bool Match( HttpRequestMessage request, IHttpRoute route, string
5254
return base.Match( request, route, parameterName, values, routeDirection );
5355
}
5456

55-
var properties = request.ApiVersionProperties();
56-
var requestedVersion = GetRequestedApiVersionOrReturnBadRequest( request, properties );
57+
var requestedVersion = GetRequestedApiVersionOrReturnBadRequest( request );
58+
var matched = false;
5759

58-
if ( requestedVersion != null )
60+
try
5961
{
60-
if ( ApiVersion == requestedVersion && base.Match( request, route, parameterName, values, routeDirection ) )
61-
{
62-
DecorateUrlHelperWithApiVersionRouteValueIfNecessary( request, values );
63-
return true;
64-
}
62+
matched = base.Match( request, route, parameterName, values, routeDirection );
63+
}
64+
catch ( InvalidOperationException )
65+
{
66+
// note: the base implementation of Match will setup the container. if this happens more
67+
// than once, an exception is thrown. this most often occurs when policy allows implicitly
68+
// matching an api version and all routes must be visited to determine their candidacy. if
69+
// this happens, delete the container and retry.
70+
request.DeleteRequestContainer( true );
71+
matched = base.Match( request, route, parameterName, values, routeDirection );
72+
}
6573

74+
if ( !matched )
75+
{
6676
return false;
6777
}
6878

69-
var options = request.GetApiVersioningOptions();
70-
71-
if ( options.DefaultApiVersion != ApiVersion || !base.Match( request, route, parameterName, values, routeDirection ) )
79+
if ( requestedVersion == null )
7280
{
81+
// we definitely matched the route, but not necessarily the api version so
82+
// track this route as a matching candidate
83+
request.ODataApiVersionProperties().MatchingRoutes[ApiVersion] = RouteName;
7384
return false;
7485
}
7586

76-
if ( options.AssumeDefaultVersionWhenUnspecified || IsServiceDocumentOrMetadataRoute( values ) )
87+
if ( ApiVersion == requestedVersion )
7788
{
78-
properties.RequestedApiVersion = ApiVersion;
89+
DecorateUrlHelperWithApiVersionRouteValueIfNecessary( request, values );
90+
return true;
7991
}
8092

81-
return true;
93+
return false;
8294
}
8395

84-
static bool IsServiceDocumentOrMetadataRoute( IDictionary<string, object> values ) =>
85-
values.TryGetValue( "odataPath", out var value ) && ( value == null || Equals( value, "$metadata" ) );
86-
87-
static ApiVersion GetRequestedApiVersionOrReturnBadRequest( HttpRequestMessage request, ApiVersionRequestProperties properties )
96+
static ApiVersion GetRequestedApiVersionOrReturnBadRequest( HttpRequestMessage request )
8897
{
8998
Contract.Requires( request != null );
90-
Contract.Requires( properties != null );
99+
100+
var properties = request.ApiVersionProperties();
91101

92102
try
93103
{

Diff for: ‎src/Microsoft.AspNet.OData.Versioning/System.Web.Http/HttpRequestMessageExtensions.cs

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
namespace System.Web.Http
2+
{
3+
using Microsoft;
4+
using Microsoft.Web.Http.Versioning;
5+
using System.Diagnostics.Contracts;
6+
using System.Net.Http;
7+
8+
/// <summary>
9+
/// Provides extension methods for the <see cref="HttpRequestMessage"/> class.
10+
/// </summary>
11+
public static class HttpRequestMessageExtensions
12+
{
13+
const string ODataApiVersionPropertiesKey = "MS_" + nameof( ODataApiVersionRequestProperties );
14+
15+
/// <summary>
16+
/// Gets the current OData API versioning request properties.
17+
/// </summary>
18+
/// <param name="request">The <see cref="HttpRequestMessage">request</see> to get the OData API versioning properties for.</param>
19+
/// <returns>The current <see cref="ODataApiVersionRequestProperties">OData API versioning properties</see>.</returns>
20+
public static ODataApiVersionRequestProperties ODataApiVersionProperties( this HttpRequestMessage request )
21+
{
22+
Arg.NotNull( request, nameof( request ) );
23+
Contract.Ensures( Contract.Result<ODataApiVersionRequestProperties>() != null );
24+
25+
if ( !request.Properties.TryGetValue( ODataApiVersionPropertiesKey, out var value ) || !( value is ODataApiVersionRequestProperties properties ) )
26+
{
27+
request.Properties[ODataApiVersionPropertiesKey] = properties = new ODataApiVersionRequestProperties();
28+
}
29+
30+
return properties;
31+
}
32+
}
33+
}

Diff for: ‎src/Microsoft.AspNetCore.OData.Versioning/AspNet.OData/Routing/UnversionedODataPathRouteConstraint.cs

+34-4
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
using Microsoft.AspNetCore.Mvc;
66
using Microsoft.AspNetCore.Mvc.Versioning;
77
using Microsoft.AspNetCore.Routing;
8+
using Microsoft.Extensions.DependencyInjection;
9+
using Microsoft.Extensions.Options;
10+
using System;
811
using System.Collections.Generic;
912
using System.Diagnostics.Contracts;
1013
using static Microsoft.AspNetCore.Routing.RouteDirection;
@@ -37,18 +40,42 @@ public bool Match( HttpContext httpContext, IRouter route, string routeKey, Rout
3740
return true;
3841
}
3942

40-
if ( !MatchAnyVersion )
43+
var feature = httpContext.Features.Get<IApiVersioningFeature>();
44+
45+
// determine whether this constraint can match any api version and no api version has otherwise been matched
46+
if ( MatchAnyVersion && feature.RequestedApiVersion == null )
4147
{
42-
var feature = httpContext.Features.Get<IApiVersioningFeature>();
48+
var options = httpContext.RequestServices.GetRequiredService<IOptions<ApiVersioningOptions>>().Value;
4349

44-
if ( feature.RequestedApiVersion != apiVersion )
50+
// is implicitly matching an api version allowed?
51+
if ( options.AssumeDefaultVersionWhenUnspecified || IsServiceDocumentOrMetadataRoute( values ) )
4552
{
46-
return false;
53+
var odata = httpContext.Features.Get<IODataVersioningFeature>();
54+
var model = new ApiVersionModel( odata.MatchingRoutes.Keys, Array.Empty<ApiVersion>() );
55+
var selector = httpContext.RequestServices.GetRequiredService<IApiVersionSelector>();
56+
var requestedApiVersion = feature.RequestedApiVersion = selector.SelectVersion( httpContext.Request, model );
57+
58+
// if an api version is selected, determine if it corresponds to a route that has been previously matched
59+
if ( requestedApiVersion != null && odata.MatchingRoutes.TryGetValue( requestedApiVersion, out var routeName ) )
60+
{
61+
// create a new versioned path constraint on the fly and evaluate it. this sets up the underlying odata
62+
// infrastructure such as the container, edm, etc. this has no bearing the action selector which will
63+
// already select the correct action. without this the response may be incorrect, even if the correct
64+
// action is selected and executed.
65+
var constraint = new VersionedODataPathRouteConstraint( routeName, requestedApiVersion );
66+
return constraint.Match( httpContext, route, routeKey, values, routeDirection );
67+
}
4768
}
4869
}
70+
else if ( !MatchAnyVersion && feature.RequestedApiVersion != apiVersion )
71+
{
72+
return false;
73+
}
4974

5075
httpContext.Request.DeleteRequestContainer( true );
5176

77+
// by evaluating the remaining unversioned constraints, this will ultimately determine whether 400 or 404
78+
// is returned for an odata request
5279
foreach ( var constraint in innerConstraints )
5380
{
5481
if ( constraint.Match( httpContext, route, routeKey, values, routeDirection ) )
@@ -59,5 +86,8 @@ public bool Match( HttpContext httpContext, IRouter route, string routeKey, Rout
5986

6087
return false;
6188
}
89+
90+
static bool IsServiceDocumentOrMetadataRoute( RouteValueDictionary values ) =>
91+
values.TryGetValue( "odataPath", out var value ) && ( value == null || Equals( value, "$metadata" ) );
6292
}
6393
}

Diff for: ‎src/Microsoft.AspNetCore.OData.Versioning/AspNet.OData/Routing/VersionedAttributeRoutingConvention.cs

+1-2
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
using System.Diagnostics.Contracts;
1616
using System.Linq;
1717
using System.Reflection;
18-
using static Microsoft.AspNetCore.Mvc.Versioning.ApiVersionMapping;
1918

2019
/// <content>
2120
/// Provides additional implementation specific to ASP.NET Core.
@@ -79,7 +78,7 @@ IDictionary<ODataPathTemplate, ControllerActionDescriptor> AttributeMappings
7978
/// </summary>
8079
/// <param name="action">The <see cref="ControllerActionDescriptor">controller action descriptor</see> to evaluate.</param>
8180
/// <returns>True if the <paramref name="action"/> should be mapped as an OData action or function; otherwise, false.</returns>
82-
/// <remarks>This method will match any OData action that explicitly or implicitly matches matches the API version applied
81+
/// <remarks>This method will match any OData action that explicitly or implicitly matches the API version applied
8382
/// to the associated <see cref="ApiVersionModel">model</see>.</remarks>
8483
public virtual bool ShouldMapAction( ControllerActionDescriptor action )
8584
{

Diff for: ‎src/Microsoft.AspNetCore.OData.Versioning/AspNet.OData/Routing/VersionedODataPathRouteConstraint.cs

+24-24
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
11
namespace Microsoft.AspNet.OData.Routing
22
{
3+
using Microsoft.AspNet.OData.Extensions;
34
using Microsoft.AspNetCore.Http;
45
using Microsoft.AspNetCore.Mvc;
56
using Microsoft.AspNetCore.Mvc.Versioning;
67
using Microsoft.AspNetCore.Routing;
7-
using Microsoft.Extensions.DependencyInjection;
8-
using Microsoft.Extensions.Options;
9-
using Microsoft.OData;
108
using System;
119
using System.Diagnostics.Contracts;
1210
using static Microsoft.AspNetCore.Routing.RouteDirection;
@@ -50,48 +48,50 @@ public override bool Match( HttpContext httpContext, IRouter route, string route
5048
Arg.NotNull( route, nameof( route ) );
5149
Arg.NotNull( values, nameof( values ) );
5250

53-
if ( routeDirection == UrlGeneration )
51+
if ( routeDirection == UrlGeneration || !TryGetRequestedApiVersion( httpContext, out var requestedVersion ) )
5452
{
53+
// note: if an error occurs reading the api version, still let the base constraint
54+
// match the request. the IActionSelector will produce 400 during action selection.
5555
return base.Match( httpContext, route, routeKey, values, routeDirection );
5656
}
5757

58-
var request = httpContext.Request;
59-
var feature = httpContext.Features.Get<IApiVersioningFeature>();
58+
var matched = false;
6059

61-
if ( !TryGetRequestedApiVersion( httpContext, feature, out var requestedVersion ) )
60+
try
6261
{
63-
// note: if an error occurs reading the api version, still let the base constraint
64-
// match the request. the IActionSelector will produce 400 during action selection.
65-
return base.Match( httpContext, route, routeKey, values, routeDirection );
62+
matched = base.Match( httpContext, route, routeKey, values, routeDirection );
6663
}
67-
68-
if ( requestedVersion != null )
64+
catch ( InvalidOperationException )
6965
{
70-
return ApiVersion == requestedVersion && base.Match( httpContext, route, routeKey, values, routeDirection );
66+
// note: the base implementation of Match will setup the container. if this happens more
67+
// than once, an exception is thrown. this most often occurs when policy allows implicitly
68+
// matching an api version and all routes must be visited to determine their candidacy. if
69+
// this happens, delete the container and retry.
70+
httpContext.Request.DeleteRequestContainer( true );
71+
matched = base.Match( httpContext, route, routeKey, values, routeDirection );
7172
}
7273

73-
var options = httpContext.RequestServices.GetRequiredService<IOptions<ApiVersioningOptions>>().Value;
74-
75-
if ( options.DefaultApiVersion != ApiVersion || !base.Match( httpContext, route, routeKey, values, routeDirection ) )
74+
if ( !matched )
7675
{
7776
return false;
7877
}
7978

80-
if ( options.AssumeDefaultVersionWhenUnspecified || IsServiceDocumentOrMetadataRoute( values ) )
79+
if ( requestedVersion == null )
8180
{
82-
feature.RequestedApiVersion = ApiVersion;
81+
// we definitely matched the route, but not necessarily the api version so
82+
// track this route as a matching candidate
83+
httpContext.ODataVersioningFeature().MatchingRoutes[ApiVersion] = RouteName;
84+
return false;
8385
}
8486

85-
return true;
87+
return ApiVersion == requestedVersion;
8688
}
8789

88-
static bool IsServiceDocumentOrMetadataRoute( RouteValueDictionary values ) =>
89-
values.TryGetValue( "odataPath", out var value ) && ( value == null || Equals( value, "$metadata" ) );
90-
91-
static bool TryGetRequestedApiVersion( HttpContext httpContext, IApiVersioningFeature feature, out ApiVersion apiVersion )
90+
static bool TryGetRequestedApiVersion( HttpContext httpContext, out ApiVersion apiVersion )
9291
{
9392
Contract.Requires( httpContext != null );
94-
Contract.Requires( feature != null );
93+
94+
var feature = httpContext.Features.Get<IApiVersioningFeature>();
9595

9696
try
9797
{

0 commit comments

Comments
 (0)