Skip to content

Commit 30f05ca

Browse files
commonsensesoftwarecommonsensesoftware
committed
IEdmModelSelector should use IApiVersionSelector when possible for unspecified API version. Fixes #745 
1 parent 9fbf520 commit 30f05ca

File tree

8 files changed

+135
-53
lines changed

8 files changed

+135
-53
lines changed

‎src/Common.OData/OData.Edm/EdmModelSelector.cs

Lines changed: 97 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,60 +3,105 @@
33
#if !WEBAPI
44
using Microsoft.AspNetCore.Http;
55
using Microsoft.AspNetCore.Mvc;
6+
using Microsoft.AspNetCore.Mvc.Versioning;
67
#endif
78
using Microsoft.Extensions.DependencyInjection;
89
#if WEBAPI
910
using Microsoft.Web.Http;
11+
using Microsoft.Web.Http.Versioning;
1012
#endif
1113
using System;
1214
using System.Collections.Generic;
15+
using System.Linq;
1316
#if WEBAPI
1417
using System.Net.Http;
1518
using System.Web.Http;
19+
using HttpRequest = System.Net.Http.HttpRequestMessage;
1620
#endif
1721

1822
/// <summary>
1923
/// Represents an <see cref="IEdmModelSelector">EDM model selector</see>.
2024
/// </summary>
21-
#if WEBAPI
25+
#if !WEBAPI
2226
[CLSCompliant( false )]
2327
#endif
2428
public class EdmModelSelector : IEdmModelSelector
2529
{
2630
readonly ApiVersion maxVersion;
31+
readonly IApiVersionSelector selector;
2732

2833
/// <summary>
2934
/// Initializes a new instance of the <see cref="EdmModelSelector"/> class.
3035
/// </summary>
3136
/// <param name="models">The <see cref="IEnumerable{T}">sequence</see> of <see cref="IEdmModel">models</see> to select from.</param>
3237
/// <param name="defaultApiVersion">The default <see cref="ApiVersion">API version</see>.</param>
38+
[Obsolete( "This constructor will be removed in the next major version. Use the constructor with IApiVersionSelector instead." )]
3339
public EdmModelSelector( IEnumerable<IEdmModel> models, ApiVersion defaultApiVersion )
40+
: this( models, defaultApiVersion, new ConstantApiVersionSelector( defaultApiVersion ) ) { }
41+
42+
/// <summary>
43+
/// Initializes a new instance of the <see cref="EdmModelSelector"/> class.
44+
/// </summary>
45+
/// <param name="models">The <see cref="IEnumerable{T}">sequence</see> of <see cref="IEdmModel">models</see> to select from.</param>
46+
/// <param name="defaultApiVersion">The default <see cref="ApiVersion">API version</see>.</param>
47+
/// <param name="apiVersionSelector">The <see cref="IApiVersionSelector">selector</see> used to choose API versions.</param>
48+
public EdmModelSelector( IEnumerable<IEdmModel> models, ApiVersion defaultApiVersion, IApiVersionSelector apiVersionSelector )
3449
{
35-
var versions = new List<ApiVersion>();
36-
var collection = new Dictionary<ApiVersion, IEdmModel>();
50+
if ( models == null )
51+
{
52+
throw new ArgumentNullException( nameof( models ) );
53+
}
54+
55+
if ( defaultApiVersion == null )
56+
{
57+
throw new ArgumentNullException( nameof( defaultApiVersion ) );
58+
}
59+
60+
selector = apiVersionSelector ?? throw new ArgumentNullException( nameof( apiVersionSelector ) );
3761

38-
foreach ( var model in models ?? throw new ArgumentNullException( nameof( models ) ) )
62+
List<ApiVersion> versions;
63+
Dictionary<ApiVersion, IEdmModel> collection;
64+
65+
switch ( models )
3966
{
40-
var annotation = model.GetAnnotationValue<ApiVersionAnnotation>( model );
67+
case IList<IEdmModel> list:
68+
versions = new( list.Count );
69+
collection = new( list.Count );
4170

42-
if ( annotation == null )
43-
{
44-
throw new ArgumentException( LocalSR.MissingAnnotation.FormatDefault( typeof( ApiVersionAnnotation ).Name ) );
45-
}
71+
for ( var i = 0; i < list.Count; i++ )
72+
{
73+
AddVersionFromModel( list[i], versions, collection );
74+
}
4675

47-
var version = annotation.ApiVersion;
76+
break;
77+
case IReadOnlyList<IEdmModel> list:
78+
versions = new( list.Count );
79+
collection = new( list.Count );
4880

49-
collection.Add( version, model );
50-
versions.Add( version );
81+
for ( var i = 0; i < list.Count; i++ )
82+
{
83+
AddVersionFromModel( list[i], versions, collection );
84+
}
85+
86+
break;
87+
default:
88+
versions = new();
89+
collection = new();
90+
91+
foreach ( var model in models )
92+
{
93+
AddVersionFromModel( model, versions, collection );
94+
}
95+
#if !WEBAPI
96+
collection.TrimExcess();
97+
#endif
98+
break;
5199
}
52100

53101
versions.Sort();
54102
#pragma warning disable IDE0056 // Use index operator (cannot be used in web api)
55103
maxVersion = versions.Count == 0 ? defaultApiVersion : versions[versions.Count - 1];
56104
#pragma warning restore IDE0056
57-
#if !WEBAPI
58-
collection.TrimExcess();
59-
#endif
60105
ApiVersions = versions.ToArray();
61106
Models = collection;
62107
}
@@ -85,13 +130,45 @@ public EdmModelSelector( IEnumerable<IEdmModel> models, ApiVersion defaultApiVer
85130
return default;
86131
}
87132

88-
#if WEBAPI
89-
var version = serviceProvider.GetService<HttpRequestMessage>()?.GetRequestedApiVersion();
90-
#else
91-
var version = serviceProvider.GetService<HttpRequest>()?.HttpContext.GetRequestedApiVersion();
133+
var request = serviceProvider.GetService<HttpRequest>();
134+
135+
if ( request is null )
136+
{
137+
return Models[maxVersion];
138+
}
139+
140+
var version = request
141+
#if !WEBAPI
142+
.HttpContext
92143
#endif
144+
.GetRequestedApiVersion();
145+
146+
if ( version is null )
147+
{
148+
var model = new ApiVersionModel( ApiVersions, Enumerable.Empty<ApiVersion>() );
149+
150+
if ( ( version = selector.SelectVersion( request, model ) ) is null )
151+
{
152+
return Models[maxVersion];
153+
}
154+
}
155+
156+
return Models.TryGetValue( version, out var edm ) ? edm : Models[maxVersion];
157+
}
158+
159+
static void AddVersionFromModel( IEdmModel model, IList<ApiVersion> versions, IDictionary<ApiVersion, IEdmModel> collection )
160+
{
161+
var annotation = model.GetAnnotationValue<ApiVersionAnnotation>( model );
162+
163+
if ( annotation == null )
164+
{
165+
throw new ArgumentException( LocalSR.MissingAnnotation.FormatDefault( typeof( ApiVersionAnnotation ).Name ) );
166+
}
167+
168+
var version = annotation.ApiVersion;
93169

94-
return version != null && Models.TryGetValue( version, out var model ) ? model : Models[maxVersion];
170+
collection.Add( version, model );
171+
versions.Add( version );
95172
}
96173
}
97174
}

‎src/Microsoft.AspNet.OData.Versioning/OData/IContainerBuilderExtensions.cs

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,11 @@ public static IContainerBuilder AddApiVersioning( this IContainerBuilder builder
2626
.AddService( Transient, sp => sp.GetRequiredService<IEdmModelSelector>().SelectModel( sp ) )
2727
.AddService(
2828
Singleton,
29-
sp => (IEdmModelSelector) new EdmModelSelector(
30-
models,
31-
sp.GetRequiredService<HttpConfiguration>().GetApiVersioningOptions().DefaultApiVersion ) )
29+
sp =>
30+
{
31+
var options = sp.GetRequiredService<HttpConfiguration>().GetApiVersioningOptions();
32+
return (IEdmModelSelector) new EdmModelSelector( models, options.DefaultApiVersion, options.ApiVersionSelector );
33+
} )
3234
.AddService(
3335
Singleton,
3436
sp => CreateDefaultWithAttributeRouting(
@@ -50,9 +52,11 @@ public static IContainerBuilder AddApiVersioning(
5052
.AddService( Transient, sp => sp.GetRequiredService<IEdmModelSelector>().SelectModel( sp ) )
5153
.AddService(
5254
Singleton,
53-
sp => (IEdmModelSelector) new EdmModelSelector(
54-
models,
55-
sp.GetRequiredService<HttpConfiguration>().GetApiVersioningOptions().DefaultApiVersion ) )
55+
sp =>
56+
{
57+
var options = sp.GetRequiredService<HttpConfiguration>().GetApiVersioningOptions();
58+
return (IEdmModelSelector) new EdmModelSelector( models, options.DefaultApiVersion, options.ApiVersionSelector );
59+
} )
5660
.AddService( Singleton, sp => AddOrUpdate( routingConventions.ToList() ).AsEnumerable() );
5761
}
5862
}

‎src/Microsoft.AspNet.WebApi.Versioning/Controllers/ApiVersionActionSelector.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
/// </summary>
1515
public partial class ApiVersionActionSelector : IHttpActionSelector
1616
{
17-
readonly object cacheKey = new object();
17+
readonly object cacheKey = new();
1818
ActionSelectorCacheItem? fastCache;
1919

2020
/// <summary>

‎src/Microsoft.AspNetCore.OData.Versioning/AspNetCore.Mvc/Versioning/ODataApiVersionActionSelector.cs

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ public override IReadOnlyList<ActionDescriptor> SelectCandidates( RouteContext c
106106
{
107107
foreach ( var candidate in candidates )
108108
{
109-
if ( !( candidate.AttributeRouteInfo is ODataAttributeRouteInfo info ) ||
109+
if ( candidate.AttributeRouteInfo is not ODataAttributeRouteInfo info ||
110110
!comparer.Equals( routePrefix, info.RoutePrefix ) )
111111
{
112112
continue;
@@ -237,16 +237,11 @@ static bool RequestHasBody( HttpContext context )
237237
{
238238
var method = context.Request.Method.ToUpperInvariant();
239239

240-
switch ( method )
240+
return method switch
241241
{
242-
case "POST":
243-
case "PUT":
244-
case "PATCH":
245-
case "MERGE":
246-
return true;
247-
}
248-
249-
return false;
242+
"POST" or "PUT" or "PATCH" or "MERGE" => true,
243+
_ => false,
244+
};
250245
}
251246

252247
static bool ActionAcceptsMethod( ActionDescriptor action, string method ) =>

‎src/Microsoft.AspNetCore.OData.Versioning/OData/IContainerBuilderExtensions.cs

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,11 @@ public static IContainerBuilder AddApiVersioning( this IContainerBuilder builder
3232
Singleton,
3333
child => child.WithParent(
3434
serviceProvider,
35-
sp => (IEdmModelSelector) new EdmModelSelector(
36-
models,
37-
sp.GetRequiredService<IOptions<ApiVersioningOptions>>().Value.DefaultApiVersion ) ) )
35+
sp =>
36+
{
37+
var options = sp.GetRequiredService<IOptions<ApiVersioningOptions>>().Value;
38+
return (IEdmModelSelector) new EdmModelSelector( models, options.DefaultApiVersion, options.ApiVersionSelector );
39+
} ) )
3840
.AddService(
3941
Singleton,
4042
child => child.WithParent( serviceProvider, sp => CreateDefaultWithAttributeRouting( routeName, sp ).AsEnumerable() ) );
@@ -59,9 +61,11 @@ public static IContainerBuilder AddApiVersioning(
5961
Singleton,
6062
child => child.WithParent(
6163
serviceProvider,
62-
sp => (IEdmModelSelector) new EdmModelSelector(
63-
models,
64-
sp.GetRequiredService<IOptions<ApiVersioningOptions>>().Value.DefaultApiVersion ) ) )
64+
sp =>
65+
{
66+
var options = sp.GetRequiredService<IOptions<ApiVersioningOptions>>().Value;
67+
return (IEdmModelSelector) new EdmModelSelector( models, options.DefaultApiVersion, options.ApiVersionSelector );
68+
} ) )
6569
.AddService( Singleton, child => child.WithParent( serviceProvider, sp => AddOrUpdate( routingConventions.ToList() ).AsEnumerable() ) );
6670
}
6771
}

‎test/Acceptance.Test.Shared/AcceptanceTest.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ namespace Microsoft.AspNetCore.Mvc
2121
public abstract partial class AcceptanceTest
2222
{
2323
const string JsonMediaType = "application/json";
24-
static readonly HttpMethod Patch = new HttpMethod( "PATCH" );
24+
static readonly HttpMethod Patch = new( "PATCH" );
2525
readonly HttpServerFixture fixture;
2626

2727
protected AcceptanceTest( HttpServerFixture fixture ) => this.fixture = fixture;

‎test/Microsoft.AspNet.OData.Versioning.ApiExplorer.Tests/AspNet.OData/DefaultModelTypeBuilderTest.cs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using Microsoft.Extensions.DependencyInjection;
66
using Microsoft.OData.Edm;
77
using Microsoft.Web.Http;
8+
using Microsoft.Web.Http.Versioning;
89
using System;
910
using System.Collections.Generic;
1011
using System.Linq;
@@ -234,7 +235,7 @@ public void substitute_should_generate_type_for_action_parameters()
234235

235236
model.SetAnnotationValue( model, new ApiVersionAnnotation( ApiVersion.Default ) );
236237

237-
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default );
238+
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default, new ConstantApiVersionSelector( ApiVersion.Default ) );
238239
var context = NewContext( model );
239240
var qualifiedName = $"{model.EntityContainer.Namespace}.{action.Name}";
240241
var operation = (IEdmAction) model.FindDeclaredOperations( qualifiedName ).Single();
@@ -272,7 +273,7 @@ public void substitute_should_generate_type_for_action_parameters_with_substitut
272273

273274
model.SetAnnotationValue( model, new ApiVersionAnnotation( ApiVersion.Default ) );
274275

275-
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default );
276+
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default, new ConstantApiVersionSelector( ApiVersion.Default ) );
276277
var context = NewContext( model );
277278
var qualifiedName = $"{model.EntityContainer.Namespace}.{action.Name}";
278279
var operation = (IEdmAction) model.FindDeclaredOperations( qualifiedName ).Single();
@@ -315,7 +316,7 @@ public void substitute_should_generate_type_for_action_parameters_with_collectio
315316

316317
model.SetAnnotationValue( model, new ApiVersionAnnotation( ApiVersion.Default ) );
317318

318-
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default );
319+
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default, new ConstantApiVersionSelector( ApiVersion.Default ) );
319320
var context = NewContext( model );
320321
var qualifiedName = $"{model.EntityContainer.Namespace}.{action.Name}";
321322
var operation = (IEdmAction) model.FindDeclaredOperations( qualifiedName ).Single();
@@ -357,7 +358,7 @@ public void substitute_should_generate_types_for_actions_with_the_same_name_in_d
357358

358359
model.SetAnnotationValue( model, new ApiVersionAnnotation( ApiVersion.Default ) );
359360

360-
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default );
361+
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default, new ConstantApiVersionSelector( ApiVersion.Default ) );
361362
var context = NewContext( model );
362363
var qualifiedName = $"{model.EntityContainer.Namespace}.{action.Name}";
363364
var operations = model.FindDeclaredOperations( qualifiedName ).Select( o => (IEdmAction) o ).ToArray();

‎test/Microsoft.AspNetCore.OData.Versioning.ApiExplorer.Tests/AspNet.OData/DefaultModelTypeBuilderTest.cs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,17 @@
22
{
33
using FluentAssertions;
44
using FluentAssertions.Common;
5-
using System.Runtime.Serialization;
65
using Microsoft.AspNet.OData.Builder;
76
using Microsoft.AspNetCore.Mvc;
7+
using Microsoft.AspNetCore.Mvc.Versioning;
88
using Microsoft.Extensions.DependencyInjection;
99
using Microsoft.OData.Edm;
1010
using System;
1111
using System.Collections.Generic;
1212
using System.Linq;
1313
using System.Reflection;
1414
using System.Reflection.Emit;
15+
using System.Runtime.Serialization;
1516
using Xunit;
1617

1718
public class DefaultModelTypeBuilderTest
@@ -236,7 +237,7 @@ public void substitute_should_generate_type_for_action_parameters()
236237

237238
model.SetAnnotationValue( model, new ApiVersionAnnotation( ApiVersion.Default ) );
238239

239-
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default );
240+
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default, new ConstantApiVersionSelector( ApiVersion.Default ) );
240241
var context = NewContext( model );
241242
var qualifiedName = $"{model.EntityContainer.Namespace}.{action.Name}";
242243
var operation = (IEdmAction) model.FindDeclaredOperations( qualifiedName ).Single();
@@ -274,7 +275,7 @@ public void substitute_should_generate_type_for_action_parameters_with_substitut
274275

275276
model.SetAnnotationValue( model, new ApiVersionAnnotation( ApiVersion.Default ) );
276277

277-
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default );
278+
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default, new ConstantApiVersionSelector( ApiVersion.Default ) );
278279
var context = NewContext( model );
279280
var qualifiedName = $"{model.EntityContainer.Namespace}.{action.Name}";
280281
var operation = (IEdmAction) model.FindDeclaredOperations( qualifiedName ).Single();
@@ -317,7 +318,7 @@ public void substitute_should_generate_type_for_action_parameters_with_collectio
317318

318319
model.SetAnnotationValue( model, new ApiVersionAnnotation( ApiVersion.Default ) );
319320

320-
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default );
321+
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default, new ConstantApiVersionSelector( ApiVersion.Default ) );
321322
var context = NewContext( model );
322323
var qualifiedName = $"{model.EntityContainer.Namespace}.{action.Name}";
323324
var operation = (IEdmAction) model.FindDeclaredOperations( qualifiedName ).Single();
@@ -359,7 +360,7 @@ public void substitute_should_generate_types_for_actions_with_the_same_name_in_d
359360

360361
model.SetAnnotationValue( model, new ApiVersionAnnotation( ApiVersion.Default ) );
361362

362-
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default );
363+
var selector = (IEdmModelSelector) new EdmModelSelector( new[] { model }, ApiVersion.Default, new ConstantApiVersionSelector( ApiVersion.Default ) );
363364
var context = NewContext( model );
364365
var qualifiedName = $"{model.EntityContainer.Namespace}.{action.Name}";
365366
var operations = model.FindDeclaredOperations( qualifiedName ).Select( o => (IEdmAction) o ).ToArray();

0 commit comments

Comments
 (0)