Return 400 instead of 404 for action that can match in at least one API version. Fixes #26 (#30)

commonsensesoftware · web-flow · commit 144ed886e861 · 2016-09-17T13:56:39.000-07:00
diff --git a/src/Microsoft.AspNet.WebApi.Versioning/Controllers/ActionSelectorCacheItem.cs b/src/Microsoft.AspNet.WebApi.Versioning/Controllers/ActionSelectorCacheItem.cs
@@ -1,5 +1,6 @@
 ﻿namespace Microsoft.Web.Http.Controllers
 {
+    using Dispatcher;
     using Routing;
     using System;
     using System.Collections.Generic;
@@ -202,18 +203,34 @@ private HttpResponseMessage CreateSelectionError( HttpControllerContext controll
             {
                 Contract.Ensures( Contract.Result<HttpResponseMessage>() != null );
 
+                if ( !controllerContext.ControllerDescriptor.GetApiVersionModel().IsApiVersionNeutral )
+                {
+                    return CreateBadRequestResponse( controllerContext );
+                }
+
                 var actionsFoundByParams = FindMatchingActions( controllerContext, ignoreVerbs: true );
 
                 if ( actionsFoundByParams.Count > 0 )
                 {
-                    return Create405Response( controllerContext, actionsFoundByParams );
+                    return CreateMethodNotAllowedResponse( controllerContext, actionsFoundByParams );
                 }
 
                 return CreateActionNotFoundResponse( controllerContext );
             }
 
             [SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Caller is responsible for disposing of response instance." )]
-            private static HttpResponseMessage Create405Response( HttpControllerContext controllerContext, IEnumerable<HttpActionDescriptor> allowedCandidates )
+            private static HttpResponseMessage CreateBadRequestResponse( HttpControllerContext controllerContext )
+            {
+                Contract.Requires( controllerContext != null );
+                Contract.Ensures( Contract.Result<HttpResponseMessage>() != null );
+
+                var request = controllerContext.Request;
+                var exceptionFactory = new HttpResponseExceptionFactory( request );
+                return exceptionFactory.CreateBadRequestResponseForUnsupportedApiVersion( request.GetRequestedApiVersion() );
+            }
+
+            [SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Caller is responsible for disposing of response instance." )]
+            private static HttpResponseMessage CreateMethodNotAllowedResponse( HttpControllerContext controllerContext, IEnumerable<HttpActionDescriptor> allowedCandidates )
             {
                 Contract.Requires( controllerContext != null );
                 Contract.Requires( allowedCandidates != null );
@@ -253,6 +270,11 @@ private HttpResponseMessage CreateActionNotFoundResponse( HttpControllerContext
                 Contract.Requires( controllerContext != null );
                 Contract.Ensures( Contract.Result<HttpResponseMessage>() != null );
 
+                if ( !controllerContext.ControllerDescriptor.GetApiVersionModel().IsApiVersionNeutral )
+                {
+                    return CreateBadRequestResponse( controllerContext );
+                }
+
                 var message = SR.ResourceNotFound.FormatDefault( controllerContext.Request.RequestUri );
                 var messageDetail = SR.ApiControllerActionSelector_ActionNameNotFound.FormatDefault( controllerDescriptor.ControllerName, actionName );
                 return controllerContext.Request.CreateErrorResponse( NotFound, message, messageDetail );
@@ -515,7 +537,7 @@ private static void FindActionsForVerbWorker( HttpMethod verb, CandidateAction[]
                 }
             }
 
-            private static string CreateAmbiguousMatchList( IEnumerable<CandidateHttpActionDescriptor> ambiguousCandidates )
+            internal static string CreateAmbiguousMatchList( IEnumerable<HttpActionDescriptor> ambiguousCandidates )
             {
                 Contract.Requires( ambiguousCandidates != null );
                 Contract.Ensures( Contract.Result<string>() != null );
diff --git a/src/Microsoft.AspNet.WebApi.Versioning/Controllers/AggregatedActionMapping.cs b/src/Microsoft.AspNet.WebApi.Versioning/Controllers/AggregatedActionMapping.cs
@@ -0,0 +1,35 @@
+﻿namespace Microsoft.Web.Http.Controllers
+{
+    using System.Collections;
+    using System.Collections.Generic;
+    using System.Diagnostics.Contracts;
+    using System.Linq;
+    using System.Web.Http.Controllers;
+
+    /// <content>
+    /// Provides additional content for the <see cref="ApiVersionActionSelector"/> class.
+    /// </content>
+    public partial class ApiVersionActionSelector
+    {
+        private sealed class AggregatedActionMapping : ILookup<string, HttpActionDescriptor>
+        {
+            private readonly IReadOnlyList<ILookup<string, HttpActionDescriptor>> actionMappings;
+
+            internal AggregatedActionMapping( IReadOnlyList<ILookup<string, HttpActionDescriptor>> actionMappings )
+            {
+                Contract.Requires( actionMappings != null );
+                this.actionMappings = actionMappings;
+            }
+
+            public IEnumerable<HttpActionDescriptor> this[string key] => actionMappings.Where( am => am.Contains( key ) ).SelectMany( am => am[key] );
+
+            public int Count => actionMappings[0].Count;
+
+            public bool Contains( string key ) => actionMappings.Any( am => am.Contains( key ) );
+
+            public IEnumerator<IGrouping<string, HttpActionDescriptor>> GetEnumerator() => actionMappings[0].GetEnumerator();
+
+            IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+        }
+    }
+}
diff --git a/src/Microsoft.AspNet.WebApi.Versioning/Controllers/ApiVersionActionSelector.cs b/src/Microsoft.AspNet.WebApi.Versioning/Controllers/ApiVersionActionSelector.cs
@@ -1,5 +1,7 @@
 ﻿namespace Microsoft.Web.Http.Controllers
 {
+    using Dispatcher;
+    using System;
     using System.Collections.Generic;
     using System.Diagnostics.CodeAnalysis;
     using System.Diagnostics.Contracts;
@@ -63,7 +65,14 @@ protected virtual HttpActionDescriptor SelectActionVersion( HttpControllerContex
             Arg.NotNull( controllerContext, nameof( controllerContext ) );
             Arg.NotNull( candidateActions, nameof( candidateActions ) );
 
-            var requestedVersion = controllerContext.Request.GetRequestedApiVersion();
+            if ( candidateActions.Count == 0 )
+            {
+                return null;
+            }
+
+            var request = controllerContext.Request;
+            var requestedVersion = request.GetRequestedApiVersion();
+            var exceptionFactory = new HttpResponseExceptionFactory( request );
 
             if ( candidateActions.Count == 1 )
             {
@@ -93,14 +102,31 @@ protected virtual HttpActionDescriptor SelectActionVersion( HttpControllerContex
             switch ( explicitMatches.Count )
             {
                 case 0:
-                    return implicitMatches.Count == 1 ? implicitMatches[0] : null;
+                    switch ( implicitMatches.Count )
+                    {
+                        case 0:
+                            break;
+                        case 1:
+                            return implicitMatches[0];
+                        default:
+                            throw CreateAmbiguousActionException( implicitMatches );
+                    }
+                    break;
                 case 1:
                     return explicitMatches[0];
+                default:
+                    throw CreateAmbiguousActionException( explicitMatches );
             }
 
             return null;
         }
 
+        private Exception CreateAmbiguousActionException( IEnumerable<HttpActionDescriptor> matches )
+        {
+            var ambiguityList = ActionSelectorCacheItem.CreateAmbiguousMatchList( matches );
+            return new InvalidOperationException( SR.ApiControllerActionSelector_AmbiguousMatch.FormatDefault( ambiguityList ) );
+        }
+
         /// <summary>
         /// Selects and returns the action descriptor to invoke given the provided controller context.
         /// </summary>
@@ -129,7 +155,19 @@ public virtual ILookup<string, HttpActionDescriptor> GetActionMapping( HttpContr
             Contract.Ensures( Contract.Result<ILookup<string, HttpActionDescriptor>>() != null );
 
             var internalSelector = GetInternalSelector( controllerDescriptor );
-            return internalSelector.GetActionMapping();
+            var actionMappings = new List<ILookup<string, HttpActionDescriptor>>();
+
+            actionMappings.Add( internalSelector.GetActionMapping() );
+
+            foreach ( var relatedControllerDescriptor in controllerDescriptor.GetRelatedCandidates() )
+            {
+                if ( relatedControllerDescriptor != controllerDescriptor )
+                {
+                    actionMappings.Add( GetInternalSelector( relatedControllerDescriptor ).GetActionMapping() );
+                }
+            }
+
+            return actionMappings.Count == 1 ? actionMappings[0] : new AggregatedActionMapping( actionMappings );
         }
     }
-}
+}
diff --git a/src/Microsoft.AspNet.WebApi.Versioning/Dispatcher/ConventionRouteControllerSelector.cs b/src/Microsoft.AspNet.WebApi.Versioning/Dispatcher/ConventionRouteControllerSelector.cs
@@ -139,6 +139,7 @@ private static HttpControllerDescriptor GetVersionedController( ApiVersionContro
                 controller.SetApiVersionModel( aggregator.AllVersions );
             }
 
+            controller.SetRelatedCandidates( candidates );
             return controller;
         }
 
diff --git a/src/Microsoft.AspNet.WebApi.Versioning/System.Web.Http/HttpControllerDescriptorExtensions.cs b/src/Microsoft.AspNet.WebApi.Versioning/System.Web.Http/HttpControllerDescriptorExtensions.cs
@@ -4,6 +4,7 @@
     using Controllers;
     using Diagnostics.CodeAnalysis;
     using Diagnostics.Contracts;
+    using Linq;
     using Microsoft;
     using Microsoft.Web.Http;
     using Microsoft.Web.Http.Versioning;
@@ -16,6 +17,7 @@ public static class HttpControllerDescriptorExtensions
         private const string AttributeRoutedPropertyKey = "MS_IsAttributeRouted";
         private const string ApiVersionInfoKey = "MS_ApiVersionInfo";
         private const string ConventionsApiVersionInfoKey = "MS_ConventionsApiVersionInfo";
+        private const string RelatedControllerCandidatesKey = "MS_RelatedControllerCandidates";
 
         internal static bool IsAttributeRouted( this HttpControllerDescriptor controllerDescriptor )
         {
@@ -105,6 +107,12 @@ internal static void SetApiVersionModel( this HttpControllerDescriptor controlle
         internal static void SetConventionsApiVersionModel( this HttpControllerDescriptor controllerDescriptor, ApiVersionModel model ) =>
             controllerDescriptor.Properties.AddOrUpdate( ConventionsApiVersionInfoKey, model, ( key, currentModel ) => ( (ApiVersionModel) currentModel ).Aggregate( model ) );
 
+        internal static IEnumerable<HttpControllerDescriptor> GetRelatedCandidates( this HttpControllerDescriptor controllerDescriptor ) =>
+            (IEnumerable<HttpControllerDescriptor>) controllerDescriptor.Properties.GetOrAdd( RelatedControllerCandidatesKey, key => Enumerable.Empty<HttpControllerDescriptor>() );
+
+        internal static void SetRelatedCandidates( this HttpControllerDescriptor controllerDescriptor, IEnumerable<HttpControllerDescriptor> value ) =>
+            controllerDescriptor.Properties.AddOrUpdate( RelatedControllerCandidatesKey, value, ( key, oldValue ) => value );
+
         /// <summary>
         /// Gets a value indicating whether the controller is API version neutral.
         /// </summary>

Original file line number	Diff line number	Diff line change
`@@ -139,6 +139,7 @@ private static HttpControllerDescriptor GetVersionedController( ApiVersionContro`
`139`	`139`	`controller.SetApiVersionModel( aggregator.AllVersions );`
`140`	`140`	`}`
`141`	`141`
	`142`	`+ controller.SetRelatedCandidates( candidates );`
`142`	`143`	`return controller;`
`143`	`144`	`}`
`144`	`145`