Versions that are mapped only should not be discoverable. Fixes #735

commonsensesoftware · commonsensesoftware · commit 2c0ab866bd08 · 2022-03-20T19:46:52.000-07:00
diff --git a/src/Common/CollectionExtensions.cs b/src/Common/CollectionExtensions.cs
@@ -51,9 +51,29 @@ internal static IReadOnlyList<T> ToSortedReadOnlyList<T>( this IEnumerable<T> se
 
         internal static void AddRange<T>( this ICollection<T> collection, IEnumerable<T> items )
         {
-            foreach ( var item in items )
+            switch ( items )
             {
-                collection.Add( item );
+                case IList<T> list:
+                    for ( var i = 0; i < list.Count; i++ )
+                    {
+                        collection.Add( list[i] );
+                    }
+
+                    break;
+                case IReadOnlyList<T> list:
+                    for ( var i = 0; i < list.Count; i++ )
+                    {
+                        collection.Add( list[i] );
+                    }
+
+                    break;
+                default:
+                    foreach ( var item in items )
+                    {
+                        collection.Add( item );
+                    }
+
+                    break;
             }
         }
 
@@ -81,12 +101,38 @@ internal static void UnionWith<T>( this ICollection<T> collection, IEnumerable<T
             }
             else
             {
-                foreach ( var item in other )
+                switch ( other )
                 {
-                    if ( !collection.Contains( item ) )
-                    {
-                        collection.Add( item );
-                    }
+                    case IList<T> list:
+                        for ( var i = 0; i < list.Count; i++ )
+                        {
+                            if ( !collection.Contains( list[i] ) )
+                            {
+                                collection.Add( list[i] );
+                            }
+                        }
+
+                        break;
+                    case IReadOnlyList<T> list:
+                        for ( var i = 0; i < list.Count; i++ )
+                        {
+                            if ( !collection.Contains( list[i] ) )
+                            {
+                                collection.Add( list[i] );
+                            }
+                        }
+
+                        break;
+                    default:
+                        foreach ( var item in other )
+                        {
+                            if ( !collection.Contains( item ) )
+                            {
+                                collection.Add( item );
+                            }
+                        }
+
+                        break;
                 }
             }
         }
diff --git a/src/Common/Versioning/ApiVersionModel.cs b/src/Common/Versioning/ApiVersionModel.cs
@@ -19,9 +19,9 @@ public sealed partial class ApiVersionModel
         const int DefaultModel = 0;
         const int NeutralModel = 1;
         const int EmptyModel = 2;
-        static readonly Lazy<ApiVersionModel> defaultVersion = new Lazy<ApiVersionModel>( () => new ApiVersionModel( DefaultModel ) );
-        static readonly Lazy<ApiVersionModel> neutralVersion = new Lazy<ApiVersionModel>( () => new ApiVersionModel( NeutralModel ) );
-        static readonly Lazy<ApiVersionModel> emptyVersion = new Lazy<ApiVersionModel>( () => new ApiVersionModel( EmptyModel ) );
+        static readonly Lazy<ApiVersionModel> defaultVersion = new( () => new( DefaultModel ) );
+        static readonly Lazy<ApiVersionModel> neutralVersion = new( () => new( NeutralModel ) );
+        static readonly Lazy<ApiVersionModel> emptyVersion = new( () => new( EmptyModel ) );
 #if WEBAPI
         static readonly IReadOnlyList<ApiVersion> emptyVersions = new ApiVersion[0];
 #else
@@ -55,16 +55,16 @@ public sealed partial class ApiVersionModel
 
         internal ApiVersionModel( ApiVersionModel original, IReadOnlyList<ApiVersion> implemented, IReadOnlyList<ApiVersion> supported, IReadOnlyList<ApiVersion> deprecated )
         {
+            DeclaredApiVersions = original.DeclaredApiVersions;
+
             if ( IsApiVersionNeutral = implemented.Count == 0 )
             {
-                DeclaredApiVersions = emptyVersions;
                 ImplementedApiVersions = emptyVersions;
                 SupportedApiVersions = emptyVersions;
                 DeprecatedApiVersions = emptyVersions;
             }
             else
             {
-                DeclaredApiVersions = original.DeclaredApiVersions;
                 ImplementedApiVersions = implemented;
                 SupportedApiVersions = supported;
                 DeprecatedApiVersions = deprecated;
diff --git a/src/Common/Versioning/Conventions/ActionApiVersionConventionBuilderBase.cs b/src/Common/Versioning/Conventions/ActionApiVersionConventionBuilderBase.cs
@@ -6,7 +6,6 @@ namespace Microsoft.AspNetCore.Mvc.Versioning.Conventions
 {
     using System;
     using System.Collections.Generic;
-    using System.Linq;
 #if WEBAPI
     using static Microsoft.Web.Http.Versioning.ApiVersionProviderOptions;
 #else
@@ -30,15 +29,18 @@ protected ActionApiVersionConventionBuilderBase() { }
         protected ICollection<ApiVersion> MappedVersions { get; } = new HashSet<ApiVersion>();
 
         /// <inheritdoc />
-        protected override void MergeAttributesWithConventions( IEnumerable<object> attributes )
+        protected override void MergeAttributesWithConventions( IReadOnlyList<object> attributes )
         {
-            base.MergeAttributesWithConventions( attributes );
+            if ( attributes == null )
+            {
+                throw new ArgumentNullException( nameof( attributes ) );
+            }
 
-            var providers = attributes.OfType<IApiVersionProvider>();
+            base.MergeAttributesWithConventions( attributes );
 
-            foreach ( var provider in providers )
+            for ( var i = 0; i < attributes.Count; i++ )
             {
-                if ( provider.Options == Mapped )
+                if ( attributes[i] is IApiVersionProvider provider && provider.Options == Mapped )
                 {
                     MappedVersions.UnionWith( provider.Versions );
                 }
diff --git a/src/Common/Versioning/Conventions/ApiVersionConventionBuilder.cs b/src/Common/Versioning/Conventions/ApiVersionConventionBuilder.cs
@@ -113,9 +113,9 @@ bool InternalApplyTo( Model model )
                 builder = new ControllerApiVersionConventionBuilder( model.ControllerType );
             }
 
-            foreach ( var convention in ControllerConventions )
+            for ( var i = 0; i < ControllerConventions.Count; i++ )
             {
-                applied |= convention.Apply( builder!, model );
+                applied |= ControllerConventions[i].Apply( builder!, model );
             }
 
             if ( applied )
diff --git a/src/Common/Versioning/Conventions/ApiVersionConventionBuilderBase.cs b/src/Common/Versioning/Conventions/ApiVersionConventionBuilderBase.cs
@@ -57,34 +57,92 @@ protected ApiVersionConventionBuilderBase() { }
         /// Merges API version information from the specified attributes with the current conventions.
         /// </summary>
         /// <param name="attributes">The <see cref="IEnumerable{T}">sequence</see> of attributes to merge.</param>
-        protected virtual void MergeAttributesWithConventions( IEnumerable<object> attributes )
+        protected virtual void MergeAttributesWithConventions( IEnumerable<object> attributes ) =>
+            MergeAttributesWithConventions( ( attributes as IReadOnlyList<object> ) ?? attributes.ToArray() );
+
+        /// <summary>
+        /// Merges API version information from the specified attributes with the current conventions.
+        /// </summary>
+        /// <param name="attributes">The <see cref="IReadOnlyList{T}">read-only list</see> of attributes to merge.</param>
+        protected virtual void MergeAttributesWithConventions( IReadOnlyList<object> attributes )
         {
-            if ( VersionNeutral |= attributes.OfType<IApiVersionNeutral>().Any() )
+            if ( attributes == null )
+            {
+                throw new ArgumentNullException( nameof( attributes ) );
+            }
+
+            if ( VersionNeutral )
             {
                 return;
             }
 
             const ApiVersionProviderOptions DeprecatedAdvertised = Deprecated | Advertised;
-            var providers = attributes.OfType<IApiVersionProvider>();
+            var supported = default( List<ApiVersion> );
+            var deprecated = default( List<ApiVersion> );
+            var advertised = default( List<ApiVersion> );
+            var deprecatedAdvertised = default( List<ApiVersion> );
 
-            foreach ( var provider in providers )
+            for ( var i = 0; i < attributes.Count; i++ )
             {
-                switch ( provider.Options )
+                switch ( attributes[i] )
                 {
-                    case None:
-                        SupportedVersions.UnionWith( provider.Versions );
-                        break;
-                    case Deprecated:
-                        DeprecatedVersions.UnionWith( provider.Versions );
-                        break;
-                    case Advertised:
-                        AdvertisedVersions.UnionWith( provider.Versions );
-                        break;
-                    case DeprecatedAdvertised:
-                        DeprecatedAdvertisedVersions.UnionWith( provider.Versions );
+                    case IApiVersionNeutral:
+                        VersionNeutral = true;
+                        return;
+                    case IApiVersionProvider provider:
+                        List<ApiVersion> target;
+                        IReadOnlyList<ApiVersion> source;
+
+                        switch ( provider.Options )
+                        {
+                            case None:
+                                target = supported ??= new();
+                                source = provider.Versions;
+                                break;
+                            case Deprecated:
+                                target = deprecated ??= new();
+                                source = provider.Versions;
+                                break;
+                            case Advertised:
+                                target = advertised ??= new();
+                                source = provider.Versions;
+                                break;
+                            case DeprecatedAdvertised:
+                                target = deprecatedAdvertised ??= new();
+                                source = provider.Versions;
+                                break;
+                            default:
+                                continue;
+                        }
+
+                        for ( var j = 0; j < source.Count; j++ )
+                        {
+                            target.Add( source[j] );
+                        }
+
                         break;
                 }
             }
+
+            if ( supported is not null && supported.Count > 0 )
+            {
+                SupportedVersions.UnionWith( supported );
+            }
+
+            if ( deprecated is not null && deprecated.Count > 0 )
+            {
+                DeprecatedVersions.UnionWith( deprecated );
+            }
+
+            if ( advertised is not null && advertised.Count > 0 )
+            {
+                AdvertisedVersions.UnionWith( advertised );
+            }
+
+            if ( deprecatedAdvertised is not null && deprecatedAdvertised.Count > 0 )
+            {
+                DeprecatedAdvertisedVersions.UnionWith( deprecatedAdvertised );
+            }
         }
     }
 }
diff --git a/src/Microsoft.AspNet.WebApi.Versioning/System.Web.Http/HttpActionDescriptorExtensions.cs b/src/Microsoft.AspNet.WebApi.Versioning/System.Web.Http/HttpActionDescriptorExtensions.cs
@@ -79,15 +79,31 @@ public static ApiVersionMapping MappingTo( this HttpActionDescriptor action, Api
 
             var model = action.GetApiVersionModel();
 
-            if ( model.IsApiVersionNeutral || ( apiVersion != null && model.DeclaredApiVersions.Contains( apiVersion ) ) )
+            if ( model.IsApiVersionNeutral )
             {
                 return Explicit;
             }
-            else if ( model.DeclaredApiVersions.Count == 0 )
+
+            if ( apiVersion is null )
+            {
+                return None;
+            }
+
+            var mappedWithImplementation = model.DeclaredApiVersions.Contains( apiVersion ) &&
+                                           model.ImplementedApiVersions.Contains( apiVersion );
+
+            if ( mappedWithImplementation )
+            {
+                return Explicit;
+            }
+
+            var deriveFromParent = model.DeclaredApiVersions.Count == 0;
+
+            if ( deriveFromParent )
             {
                 model = action.ControllerDescriptor.GetApiVersionModel();
 
-                if ( apiVersion != null && model.DeclaredApiVersions.Contains( apiVersion ) )
+                if ( model.DeclaredApiVersions.Contains( apiVersion ) )
                 {
                     return Implicit;
                 }
diff --git a/src/Microsoft.AspNet.WebApi.Versioning/Versioning/Conventions/ActionApiVersionConventionBuilderBase.cs b/src/Microsoft.AspNet.WebApi.Versioning/Versioning/Conventions/ActionApiVersionConventionBuilderBase.cs
@@ -37,7 +37,7 @@ public virtual void ApplyTo( HttpActionDescriptor actionDescriptor )
                 return;
             }
 
-            var versionModel = default( ApiVersionModel );
+            ApiVersionModel? versionModel;
 
             if ( MappedVersions.Count == 0 )
             {
diff --git a/src/Microsoft.AspNetCore.Mvc.Versioning/Abstractions/ActionDescriptorExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Versioning/Abstractions/ActionDescriptorExtensions.cs
@@ -68,15 +68,31 @@ public static ApiVersionMapping MappingTo( this ActionDescriptor action, ApiVers
         {
             var model = action.GetApiVersionModel();
 
-            if ( model.IsApiVersionNeutral || ( apiVersion != null && model.DeclaredApiVersions.Contains( apiVersion ) ) )
+            if ( model.IsApiVersionNeutral )
             {
                 return Explicit;
             }
-            else if ( model.DeclaredApiVersions.Count == 0 )
+
+            if ( apiVersion is null )
+            {
+                return None;
+            }
+
+            var mappedWithImplementation = model.DeclaredApiVersions.Contains( apiVersion ) &&
+                                           model.ImplementedApiVersions.Contains( apiVersion );
+
+            if ( mappedWithImplementation )
+            {
+                return Explicit;
+            }
+
+            var deriveFromParent = model.DeclaredApiVersions.Count == 0;
+
+            if ( deriveFromParent )
             {
-                var parentModel = action.GetProperty<ControllerModel>()?.GetProperty<ApiVersionModel>();
+                model = action.GetProperty<ControllerModel>()?.GetProperty<ApiVersionModel>();
 
-                if ( parentModel != null && ( apiVersion != null && parentModel.DeclaredApiVersions.Contains( apiVersion ) ) )
+                if ( model is not null && model.DeclaredApiVersions.Contains( apiVersion ) )
                 {
                     return Implicit;
                 }
diff --git a/src/Microsoft.AspNetCore.Mvc.Versioning/Versioning/Conventions/ApiVersionConventionBuilder.cs b/src/Microsoft.AspNetCore.Mvc.Versioning/Versioning/Conventions/ApiVersionConventionBuilder.cs
@@ -2,7 +2,6 @@
 {
     using Microsoft.AspNetCore.Mvc.ApplicationModels;
     using System;
-    using System.Linq;
     using System.Reflection;
 
     /// <content>
@@ -32,18 +31,18 @@ public virtual bool ApplyTo( ControllerModel controllerModel )
 
         static bool HasDecoratedActions( ControllerModel controllerModel )
         {
-            foreach ( var action in controllerModel.Actions )
+            for ( var i = 0; i < controllerModel.Actions.Count; i++ )
             {
-                var attributes = action.Attributes;
+                var action = controllerModel.Actions[i];
 
-                if ( attributes.OfType<IApiVersionNeutral>().Any() )
+                for ( var j = 0; j < action.Attributes.Count; j++ )
                 {
-                    return true;
-                }
+                    var attribute = action.Attributes[j];
 
-                if ( attributes.OfType<IApiVersionProvider>().Any() )
-                {
-                    return true;
+                    if ( attribute is IApiVersionProvider || attribute is IApiVersionNeutral )
+                    {
+                        return true;
+                    }
                 }
             }
 
diff --git a/test/Microsoft.AspNet.WebApi.Acceptance.Tests/Http/Basic/Controllers/HelloWorldController.cs b/test/Microsoft.AspNet.WebApi.Acceptance.Tests/Http/Basic/Controllers/HelloWorldController.cs
@@ -15,5 +15,10 @@ public class HelloWorldController : ApiController
 
         [Route]
         public IHttpActionResult Post() => CreatedAtRoute( "GetMessageById", new { id = 42 }, default( object ) );
+
+        [HttpGet]
+        [Route( nameof( Unreachable ) )]
+        [MapToApiVersion( "42.0" )]
+        public IHttpActionResult Unreachable( ApiVersion apiVersion ) => Ok( new { Controller = GetType().Name, Version = apiVersion.ToString() } );
     }
 }
diff --git a/test/Microsoft.AspNet.WebApi.Acceptance.Tests/Http/Basic/given a versioned ApiController/when a version is mapped only.cs b/test/Microsoft.AspNet.WebApi.Acceptance.Tests/Http/Basic/given a versioned ApiController/when a version is mapped only.cs
diff --git a/test/Microsoft.AspNetCore.Mvc.Acceptance.Tests/Mvc/Basic/Controllers/HelloWorldController.cs b/test/Microsoft.AspNetCore.Mvc.Acceptance.Tests/Mvc/Basic/Controllers/HelloWorldController.cs
diff --git a/test/Microsoft.AspNetCore.Mvc.Acceptance.Tests/Mvc/Basic/given a versioned Controller/when a version is mapped only.cs b/test/Microsoft.AspNetCore.Mvc.Acceptance.Tests/Mvc/Basic/given a versioned Controller/when a version is mapped only.cs

Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,6 @@ namespace Microsoft.AspNetCore.Mvc.Versioning.Conventions`
`6`	`6`	`{`
`7`	`7`	`using System;`
`8`	`8`	`using System.Collections.Generic;`
`9`		`- using System.Linq;`
`10`	`9`	`#if WEBAPI`
`11`	`10`	`using static Microsoft.Web.Http.Versioning.ApiVersionProviderOptions;`
`12`	`11`	`#else`
`@@ -30,15 +29,18 @@ protected ActionApiVersionConventionBuilderBase() { }`
`30`	`29`	`protected ICollection<ApiVersion> MappedVersions { get; } = new HashSet<ApiVersion>();`
`31`	`30`
`32`	`31`	`/// <inheritdoc />`
`33`		`- protected override void MergeAttributesWithConventions( IEnumerable<object> attributes )`
	`32`	`+ protected override void MergeAttributesWithConventions( IReadOnlyList<object> attributes )`
`34`	`33`	`{`
`35`		`- base.MergeAttributesWithConventions( attributes );`
	`34`	`+ if ( attributes == null )`
	`35`	`+ {`
	`36`	`+ throw new ArgumentNullException( nameof( attributes ) );`
	`37`	`+ }`
`36`	`38`
`37`		`- var providers = attributes.OfType<IApiVersionProvider>();`
	`39`	`+ base.MergeAttributesWithConventions( attributes );`
`38`	`40`
`39`		`- foreach ( var provider in providers )`
	`41`	`+ for ( var i = 0; i < attributes.Count; i++ )`
`40`	`42`	`{`
`41`		`- if ( provider.Options == Mapped )`
	`43`	`+ if ( attributes[i] is IApiVersionProvider provider && provider.Options == Mapped )`
`42`	`44`	`{`
`43`	`45`	`MappedVersions.UnionWith( provider.Versions );`
`44`	`46`	`}`
Original file line number	Diff line number	Diff line change
`@@ -113,9 +113,9 @@ bool InternalApplyTo( Model model )`
`113`	`113`	`builder = new ControllerApiVersionConventionBuilder( model.ControllerType );`
`114`	`114`	`}`
`115`	`115`
`116`		`- foreach ( var convention in ControllerConventions )`
	`116`	`+ for ( var i = 0; i < ControllerConventions.Count; i++ )`
`117`	`117`	`{`
`118`		`- applied \|= convention.Apply( builder!, model );`
	`118`	`+ applied \|= ControllerConventions[i].Apply( builder!, model );`
`119`	`119`	`}`
`120`	`120`
`121`	`121`	`if ( applied )`
Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ public virtual void ApplyTo( HttpActionDescriptor actionDescriptor )`
`37`	`37`	`return;`
`38`	`38`	`}`
`39`	`39`
`40`		`- var versionModel = default( ApiVersionModel );`
	`40`	`+ ApiVersionModel? versionModel;`
`41`	`41`
`42`	`42`	`if ( MappedVersions.Count == 0 )`
`43`	`43`	`{`
Original file line number	Diff line number	Diff line change
`@@ -68,15 +68,31 @@ public static ApiVersionMapping MappingTo( this ActionDescriptor action, ApiVers`
`68`	`68`	`{`
`69`	`69`	`var model = action.GetApiVersionModel();`
`70`	`70`
`71`		`- if ( model.IsApiVersionNeutral \|\| ( apiVersion != null && model.DeclaredApiVersions.Contains( apiVersion ) ) )`
	`71`	`+ if ( model.IsApiVersionNeutral )`
`72`	`72`	`{`
`73`	`73`	`return Explicit;`
`74`	`74`	`}`
`75`		`- else if ( model.DeclaredApiVersions.Count == 0 )`
	`75`	`+`
	`76`	`+ if ( apiVersion is null )`
	`77`	`+ {`
	`78`	`+ return None;`
	`79`	`+ }`
	`80`	`+`
	`81`	`+ var mappedWithImplementation = model.DeclaredApiVersions.Contains( apiVersion ) &&`
	`82`	`+ model.ImplementedApiVersions.Contains( apiVersion );`
	`83`	`+`
	`84`	`+ if ( mappedWithImplementation )`
	`85`	`+ {`
	`86`	`+ return Explicit;`
	`87`	`+ }`
	`88`	`+`
	`89`	`+ var deriveFromParent = model.DeclaredApiVersions.Count == 0;`
	`90`	`+`
	`91`	`+ if ( deriveFromParent )`
`76`	`92`	`{`
`77`		`- var parentModel = action.GetProperty<ControllerModel>()?.GetProperty<ApiVersionModel>();`
	`93`	`+ model = action.GetProperty<ControllerModel>()?.GetProperty<ApiVersionModel>();`
`78`	`94`
`79`		`- if ( parentModel != null && ( apiVersion != null && parentModel.DeclaredApiVersions.Contains( apiVersion ) ) )`
	`95`	`+ if ( model is not null && model.DeclaredApiVersions.Contains( apiVersion ) )`
`80`	`96`	`{`
`81`	`97`	`return Implicit;`
`82`	`98`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,6 @@`
`2`	`2`	`{`
`3`	`3`	`using Microsoft.AspNetCore.Mvc.ApplicationModels;`
`4`	`4`	`using System;`
`5`		`- using System.Linq;`
`6`	`5`	`using System.Reflection;`
`7`	`6`
`8`	`7`	`/// <content>`
`@@ -32,18 +31,18 @@ public virtual bool ApplyTo( ControllerModel controllerModel )`
`32`	`31`
`33`	`32`	`static bool HasDecoratedActions( ControllerModel controllerModel )`
`34`	`33`	`{`
`35`		`- foreach ( var action in controllerModel.Actions )`
	`34`	`+ for ( var i = 0; i < controllerModel.Actions.Count; i++ )`
`36`	`35`	`{`
`37`		`- var attributes = action.Attributes;`
	`36`	`+ var action = controllerModel.Actions[i];`
`38`	`37`
`39`		`- if ( attributes.OfType<IApiVersionNeutral>().Any() )`
	`38`	`+ for ( var j = 0; j < action.Attributes.Count; j++ )`
`40`	`39`	`{`
`41`		`- return true;`
`42`		`- }`
	`40`	`+ var attribute = action.Attributes[j];`
`43`	`41`
`44`		`- if ( attributes.OfType<IApiVersionProvider>().Any() )`
`45`		`- {`
`46`		`- return true;`
	`42`	`+ if ( attribute is IApiVersionProvider \|\| attribute is IApiVersionNeutral )`
	`43`	`+ {`
	`44`	`+ return true;`
	`45`	`+ }`
`47`	`46`	`}`
`48`	`47`	`}`
`49`	`48`
Original file line number	Diff line number	Diff line change
`@@ -15,5 +15,10 @@ public class HelloWorldController : ApiController`
`15`	`15`
`16`	`16`	`[Route]`
`17`	`17`	`public IHttpActionResult Post() => CreatedAtRoute( "GetMessageById", new { id = 42 }, default( object ) );`
	`18`	`+`
	`19`	`+ [HttpGet]`
	`20`	`+ [Route( nameof( Unreachable ) )]`
	`21`	`+ [MapToApiVersion( "42.0" )]`
	`22`	`+ public IHttpActionResult Unreachable( ApiVersion apiVersion ) => Ok( new { Controller = GetType().Name, Version = apiVersion.ToString() } );`
`18`	`23`	`}`
`19`	`24`	`}`