Skip to content

Commit f8bcb5e

Browse files
commonsensesoftwarecommonsensesoftware
committed
Handle OData services being registered in expected order. Fixes #694 
1 parent e239406 commit f8bcb5e

File tree

9 files changed

+115
-40
lines changed

9 files changed

+115
-40
lines changed

‎src/Microsoft.AspNetCore.Mvc.Versioning/Microsoft.Extensions.DependencyInjection/IServiceCollectionExtensions.cs

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ static void AddApiVersioningServices( IServiceCollection services )
5555
services.Add( Singleton( sp => (IApiVersionParameterSource) sp.GetRequiredService<IOptions<ApiVersioningOptions>>().Value.ApiVersionReader ) );
5656
services.Add( Singleton( sp => sp.GetRequiredService<IOptions<ApiVersioningOptions>>().Value.ApiVersionSelector ) );
5757
services.Add( Singleton( sp => sp.GetRequiredService<IOptions<ApiVersioningOptions>>().Value.ErrorResponses ) );
58-
services.Replace( Singleton<IActionSelector, ApiVersionActionSelector>() );
58+
services.TryAddOrReplace( Singleton<IActionSelector, ApiVersionActionSelector>() );
5959
services.TryAddSingleton<IApiVersionRoutePolicy, DefaultApiVersionRoutePolicy>();
6060
services.TryAddSingleton<IApiControllerFilter, DefaultApiControllerFilter>();
6161
services.TryAddSingleton<ReportApiVersionsAttribute>();
@@ -72,6 +72,28 @@ static void AddApiVersioningServices( IServiceCollection services )
7272
services.Replace( WithLinkGeneratorDecorator( services ) );
7373
}
7474

75+
static IServiceCollection TryAddOrReplace( this IServiceCollection services, ServiceDescriptor descriptor )
76+
{
77+
var currentDescriptor = services.FirstOrDefault( sd => sd.ServiceType == descriptor.ServiceType );
78+
79+
if ( currentDescriptor == null )
80+
{
81+
services.Add( descriptor );
82+
}
83+
else if ( !descriptor.GetImplementationType().IsAssignableFrom( currentDescriptor.GetImplementationType() ) )
84+
{
85+
// the only known case where this can happen is if 'services.AddOData()' is called before
86+
// 'services.AddApiVersioning()'. this is because even with endpoint routing in OData 7.x,
87+
// it still uses IActionSelector behind the scenes. IActionSelector cannot be decorated or
88+
// otherwise composed and MUST replaced with a specific concrete implementation. if services
89+
// are registered out of order, this will cause ApiVersionActionSelector to replace
90+
// ODataApiVersionActionSelector, will cause all versioned OData routes to fail
91+
services.Replace( descriptor );
92+
}
93+
94+
return services;
95+
}
96+
7597
static IReportApiVersions OnRequestIReportApiVersions( IServiceProvider serviceProvider )
7698
{
7799
var options = serviceProvider.GetRequiredService<IOptions<ApiVersioningOptions>>().Value;
@@ -160,12 +182,34 @@ static ServiceDescriptor WithLinkGeneratorDecorator( IServiceCollection services
160182
}
161183

162184
var lifetime = descriptor.Lifetime;
163-
var decoratedType = descriptor.GetImplementationType();
164-
var decoratorType = typeof( ApiVersionLinkGenerator<> ).MakeGenericType( decoratedType );
185+
var factory = descriptor.ImplementationFactory;
165186

166-
services.Replace( Describe( decoratedType, decoratedType, lifetime ) );
187+
if ( factory == null )
188+
{
189+
var decoratedType = descriptor.GetImplementationType();
190+
var decoratorType = typeof( ApiVersionLinkGenerator<> ).MakeGenericType( decoratedType );
191+
192+
services.Replace( Describe( decoratedType, decoratedType, lifetime ) );
193+
194+
return Describe( typeof( LinkGenerator ), decoratorType, lifetime );
195+
}
196+
else
197+
{
198+
LinkGenerator NewFactory( IServiceProvider serviceProvider )
199+
{
200+
var instance = (LinkGenerator) factory( serviceProvider );
201+
var source = serviceProvider.GetRequiredService<IApiVersionParameterSource>();
202+
203+
if ( source.VersionsByUrlSegment() )
204+
{
205+
instance = new ApiVersionLinkGenerator( instance );
206+
}
167207

168-
return Describe( typeof( LinkGenerator ), decoratorType, lifetime );
208+
return instance;
209+
}
210+
211+
return Describe( typeof( LinkGenerator ), NewFactory, lifetime );
212+
}
169213
}
170214
}
171215
}

‎src/Microsoft.AspNetCore.OData.Versioning/AspNet.OData/Extensions/IEndpointRouteBuilderExtensions.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
using Microsoft.AspNet.OData.Builder;
55
using Microsoft.AspNet.OData.Routing;
66
using Microsoft.AspNet.OData.Routing.Conventions;
7-
using Microsoft.AspNetCore.Mvc;
87
using Microsoft.AspNetCore.Routing;
98
using Microsoft.Extensions.DependencyInjection;
109
using Microsoft.OData;

‎src/Microsoft.AspNetCore.OData.Versioning/AspNet.OData/Routing/ODataRouteCollectionProvider.cs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,7 @@
1010
[CLSCompliant( false )]
1111
public sealed class ODataRouteCollectionProvider : IODataRouteCollectionProvider
1212
{
13-
readonly ODataRouteCollection items = new ODataRouteCollection();
14-
15-
/// <summary>
16-
/// Initializes a new instance of the <see cref="ODataRouteCollectionProvider"/> class.
17-
/// </summary>
18-
public ODataRouteCollectionProvider() { }
13+
readonly ODataRouteCollection items = new();
1914

2015
/// <inheritdoc />
2116
public IODataRouteCollection Items => items;
@@ -25,7 +20,7 @@ public ODataRouteCollectionProvider() { }
2520

2621
sealed class ODataRouteCollection : IODataRouteCollection
2722
{
28-
private readonly List<ODataRouteMapping> items = new List<ODataRouteMapping>();
23+
readonly List<ODataRouteMapping> items = new();
2924

3025
public ODataRouteMapping this[int index] => items[index];
3126

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

Lines changed: 46 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
namespace Microsoft.AspNetCore.Mvc
22
{
3+
using Microsoft.AspNet.OData.Routing;
34
using Microsoft.AspNetCore.Mvc.Infrastructure;
45
using Microsoft.Extensions.Primitives;
56
using System;
@@ -16,16 +17,21 @@
1617
/// MapVersionedODataRoute or MapVersionedODataRoutes calls have been made.</remarks>
1718
public sealed class ODataActionDescriptorChangeProvider : IActionDescriptorChangeProvider
1819
{
19-
readonly List<ChangeToken> changeTokens = new List<ChangeToken>();
20-
readonly object syncRoot = new object();
21-
22-
ODataActionDescriptorChangeProvider() { }
20+
readonly object syncRoot = new();
21+
readonly IODataRouteCollectionProvider routeCollectionProvider;
22+
int version;
23+
List<ChangeToken>? changeTokens;
2324

2425
/// <summary>
25-
/// Gets the change provider instance.
26+
/// Initializes a new instance of the <see cref="ODataActionDescriptorChangeProvider"/> class.
2627
/// </summary>
27-
/// <value>The singleton <see cref="ODataActionDescriptorChangeProvider"/> instance.</value>
28-
public static ODataActionDescriptorChangeProvider Instance { get; } = new ODataActionDescriptorChangeProvider();
28+
/// <param name="routeCollectionProvider">The corresponding <see cref="IODataRouteCollectionProvider">
29+
/// OData route collection provider</see>.</param>
30+
[CLSCompliant( false )]
31+
public ODataActionDescriptorChangeProvider( IODataRouteCollectionProvider routeCollectionProvider ) =>
32+
this.routeCollectionProvider = routeCollectionProvider;
33+
34+
int CurrentVersion => routeCollectionProvider.Items.Count;
2935

3036
internal bool HasChanged { get; private set; }
3137

@@ -40,6 +46,7 @@ public IChangeToken GetChangeToken()
4046

4147
lock ( syncRoot )
4248
{
49+
changeTokens ??= new();
4350
changeTokens.Add( changeToken );
4451
}
4552

@@ -55,6 +62,19 @@ public void NotifyChanged()
5562
{
5663
lock ( syncRoot )
5764
{
65+
if ( changeTokens == null )
66+
{
67+
return;
68+
}
69+
70+
var currentVersion = CurrentVersion;
71+
72+
if ( version == currentVersion )
73+
{
74+
return;
75+
}
76+
77+
version = currentVersion;
5878
HasChanged = true;
5979

6080
var tokens = changeTokens.ToArray();
@@ -65,8 +85,7 @@ public void NotifyChanged()
6585
{
6686
for ( var i = 0; i < tokens.Length; i++ )
6787
{
68-
var token = tokens[i];
69-
token.Callback();
88+
tokens[i].Callback();
7089
}
7190
}
7291
finally
@@ -79,8 +98,8 @@ public void NotifyChanged()
7998
sealed class ChangeToken : IChangeToken
8099
{
81100
readonly ODataActionDescriptorChangeProvider provider;
82-
readonly HashSet<(Action<object> Callback, object State)> callbacks = new HashSet<(Action<object> Callback, object State)>();
83-
readonly object syncRoot = new object();
101+
readonly object syncRoot = new();
102+
HashSet<(Action<object> Callback, object State)>? callbacks;
84103

85104
internal ChangeToken( ODataActionDescriptorChangeProvider provider ) => this.provider = provider;
86105

@@ -94,6 +113,7 @@ public IDisposable RegisterChangeCallback( Action<object> callback, object state
94113

95114
lock ( syncRoot )
96115
{
116+
callbacks ??= new();
97117
callbacks.Add( item );
98118
}
99119

@@ -104,7 +124,10 @@ internal void Remove( (Action<object> Callback, object State) item )
104124
{
105125
lock ( syncRoot )
106126
{
107-
callbacks.Remove( item );
127+
if ( callbacks != null )
128+
{
129+
callbacks.Remove( item );
130+
}
108131
}
109132
}
110133

@@ -114,14 +137,21 @@ internal void Callback()
114137

115138
lock ( syncRoot )
116139
{
117-
items = callbacks.ToArray();
118-
callbacks.Clear();
140+
if ( callbacks == null )
141+
{
142+
items = Array.Empty<(Action<object> Callback, object State)>();
143+
}
144+
else
145+
{
146+
items = callbacks.ToArray();
147+
callbacks.Clear();
148+
}
119149
}
120150

121151
for ( var i = 0; i < items.Length; i++ )
122152
{
123-
var item = items[i];
124-
item.Callback( item.State );
153+
var (callback, state) = items[i];
154+
callback( state );
125155
}
126156
}
127157
}

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ namespace Microsoft.AspNetCore.Mvc
99
using Microsoft.AspNetCore.Mvc.ModelBinding;
1010
using Microsoft.AspNetCore.Mvc.Versioning;
1111
using Microsoft.Extensions.Options;
12+
using static Microsoft.AspNetCore.Mvc.Versioning.ODataApplicationModelProvider;
1213

1314
sealed class ODataActionDescriptorProvider : IActionDescriptorProvider
1415
{
@@ -26,7 +27,7 @@ public ODataActionDescriptorProvider(
2627
this.options = options;
2728
}
2829

29-
public int Order => 0;
30+
public int Order => AfterApiVersioningConventions;
3031

3132
public void OnProvidersExecuted( ActionDescriptorProviderContext context )
3233
{
@@ -44,9 +45,7 @@ public void OnProvidersExecuted( ActionDescriptorProviderContext context )
4445

4546
for ( var i = results.Count - 1; i >= 0; i-- )
4647
{
47-
var result = results[i];
48-
49-
if ( !( result is ControllerActionDescriptor action ) ||
48+
if ( results[i] is not ControllerActionDescriptor action ||
5049
!action.ControllerTypeInfo.IsODataController() )
5150
{
5251
continue;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ namespace Microsoft.AspNetCore.Mvc.Versioning
77

88
sealed class ODataApplicationModelProvider : IApplicationModelProvider
99
{
10-
const int AfterApiVersioningConventions = -100;
1110
readonly IOptions<ODataApiVersioningOptions> options;
11+
internal const int AfterApiVersioningConventions = -100;
1212

1313
public ODataApplicationModelProvider( IOptions<ODataApiVersioningOptions> options ) => this.options = options;
1414

‎src/Microsoft.AspNetCore.OData.Versioning/Extensions.DependencyInjection/IODataBuilderExtensions.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
using Microsoft.AspNetCore.Mvc.Versioning;
1414
using Microsoft.AspNetCore.Routing;
1515
using Microsoft.Extensions.DependencyInjection.Extensions;
16-
using Microsoft.Extensions.Options;
1716
using System;
1817
using System.Linq;
1918
using static Microsoft.AspNetCore.Mvc.Versioning.ApiVersionParameterLocation;
@@ -23,7 +22,7 @@
2322
/// Provides extension methods for the <see cref="IODataBuilder"/> interface.
2423
/// </summary>
2524
[CLSCompliant( false )]
26-
public static partial class IODataBuilderExtensions
25+
public static class IODataBuilderExtensions
2726
{
2827
/// <summary>
2928
/// Enables service API versioning for the specified OData configuration.
@@ -80,7 +79,8 @@ static void AddODataServices( IServiceCollection services )
8079
services.TryAdd( Singleton<IODataRouteCollectionProvider, ODataRouteCollectionProvider>() );
8180
services.AddTransient<IApplicationModelProvider, ODataApplicationModelProvider>();
8281
services.AddTransient<IActionDescriptorProvider, ODataActionDescriptorProvider>();
83-
services.AddSingleton<IActionDescriptorChangeProvider>( ODataActionDescriptorChangeProvider.Instance );
82+
services.AddSingleton<ODataActionDescriptorChangeProvider>();
83+
services.AddSingleton<IActionDescriptorChangeProvider>( sp => sp.GetRequiredService<ODataActionDescriptorChangeProvider>() );
8484
services.TryAddEnumerable( Transient<IApiControllerSpecification, ODataControllerSpecification>() );
8585
services.AddTransient<IStartupFilter, RaiseVersionedODataRoutesMapped>();
8686
services.AddModelConfigurationsAsServices( partManager );

‎src/Microsoft.AspNetCore.OData.Versioning/Extensions.DependencyInjection/RaiseVersionedODataRoutesMapped.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ namespace Microsoft.Extensions.DependencyInjection
99

1010
sealed class RaiseVersionedODataRoutesMapped : IStartupFilter
1111
{
12+
readonly ODataActionDescriptorChangeProvider changeProvider;
13+
14+
public RaiseVersionedODataRoutesMapped( ODataActionDescriptorChangeProvider changeProvider ) =>
15+
this.changeProvider = changeProvider;
16+
1217
public Action<IApplicationBuilder> Configure( Action<IApplicationBuilder> next )
1318
{
1419
return app =>
@@ -21,7 +26,7 @@ public Action<IApplicationBuilder> Configure( Action<IApplicationBuilder> next )
2126
// until one or more routes have been mapped. if anyone has subscribed changes, notify them now.
2227
// this might do unnecessary work, but we assume that if you're using api versioning and odata, then
2328
// at least one call to MapVersionedODataRoute or some other means added a route to the IODataRouteCollection
24-
ODataActionDescriptorChangeProvider.Instance.NotifyChanged();
29+
changeProvider.NotifyChanged();
2530
};
2631
}
2732
}

‎test/Microsoft.AspNetCore.OData.Versioning.Tests/Extensions.DependencyInjection/IODataBuilderExtensionsTest.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using Microsoft.AspNet.OData.Builder;
55
using Microsoft.AspNet.OData.Interfaces;
66
using Microsoft.AspNet.OData.Routing;
7+
using Microsoft.AspNetCore.Mvc;
78
using Microsoft.AspNetCore.Mvc.Abstractions;
89
using Microsoft.AspNetCore.Mvc.ApplicationModels;
910
using Microsoft.AspNetCore.Mvc.Infrastructure;
@@ -35,7 +36,8 @@ public void enable_api_versioning_should_register_expected_services()
3536
services.Single( sd => sd.ServiceType == typeof( IODataRouteCollectionProvider ) ).ImplementationType.Should().Be( typeof( ODataRouteCollectionProvider ) );
3637
services.Any( sd => sd.ServiceType == typeof( IApplicationModelProvider ) && sd.ImplementationType.Name == "ODataApplicationModelProvider" ).Should().BeTrue();
3738
services.Any( sd => sd.ServiceType == typeof( IActionDescriptorProvider ) && sd.ImplementationType.Name == "ODataActionDescriptorProvider" ).Should().BeTrue();
38-
services.Any( sd => sd.ServiceType == typeof( IActionDescriptorChangeProvider ) && sd.ImplementationInstance.GetType().Name == "ODataActionDescriptorChangeProvider" ).Should().BeTrue();
39+
services.Single( sd => sd.ServiceType == typeof( ODataActionDescriptorChangeProvider ) ).ImplementationType.Should().Be( typeof( ODataActionDescriptorChangeProvider ) );
40+
services.Any( sd => sd.ServiceType == typeof( IActionDescriptorChangeProvider ) && sd.ImplementationFactory != null ).Should().BeTrue();
3941
services.Any( sd => sd.ServiceType == typeof( IApiControllerSpecification ) && sd.ImplementationType == typeof( ODataControllerSpecification ) ).Should().BeTrue();
4042
}
4143

@@ -60,7 +62,8 @@ public void enable_api_versioning_should_configure_custom_options()
6062
services.Single( sd => sd.ServiceType == typeof( IODataRouteCollectionProvider ) ).ImplementationType.Should().Be( typeof( ODataRouteCollectionProvider ) );
6163
services.Any( sd => sd.ServiceType == typeof( IApplicationModelProvider ) && sd.ImplementationType.Name == "ODataApplicationModelProvider" ).Should().BeTrue();
6264
services.Any( sd => sd.ServiceType == typeof( IActionDescriptorProvider ) && sd.ImplementationType.Name == "ODataActionDescriptorProvider" ).Should().BeTrue();
63-
services.Any( sd => sd.ServiceType == typeof( IActionDescriptorChangeProvider ) && sd.ImplementationInstance.GetType().Name == "ODataActionDescriptorChangeProvider" ).Should().BeTrue();
65+
services.Single( sd => sd.ServiceType == typeof( ODataActionDescriptorChangeProvider ) ).ImplementationType.Should().Be( typeof( ODataActionDescriptorChangeProvider ) );
66+
services.Any( sd => sd.ServiceType == typeof( IActionDescriptorChangeProvider ) && sd.ImplementationFactory != null ).Should().BeTrue();
6467
services.Any( sd => sd.ServiceType == typeof( IApiControllerSpecification ) && sd.ImplementationType == typeof( ODataControllerSpecification ) ).Should().BeTrue();
6568
}
6669
}

0 commit comments

Comments
 (0)