Skip to content

Commit 0ecdcdd

Browse files
author
Chris Martinez
committed
Refactored error respones to return 405 over 400 when appropriate. Fixes #65 
1 parent a8174e7 commit 0ecdcdd

19 files changed

+345
-344
lines changed

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

Lines changed: 14 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ private sealed class ActionSelectorCacheItem
3535
private readonly CandidateAction[] combinedCandidateActions;
3636
private readonly IDictionary<HttpActionDescriptor, string[]> actionParameterNames = new Dictionary<HttpActionDescriptor, string[]>();
3737
private readonly ILookup<string, HttpActionDescriptor> combinedActionNameMapping;
38+
private readonly HashSet<HttpMethod> allowedMethods = new HashSet<HttpMethod>();
3839
private StandardActionSelectionCache standardActions;
3940

4041
internal ActionSelectorCacheItem( HttpControllerDescriptor controllerDescriptor )
@@ -54,6 +55,7 @@ internal ActionSelectorCacheItem( HttpControllerDescriptor controllerDescriptor
5455
var actionDescriptor = new ReflectedHttpActionDescriptor( controllerDescriptor, method );
5556
var actionBinding = actionDescriptor.ActionBinding;
5657

58+
allowedMethods.AddRange( actionDescriptor.SupportedHttpMethods );
5759
combinedCandidateActions[i] = new CandidateAction( actionDescriptor );
5860

5961
actionParameterNames.Add(
@@ -203,19 +205,18 @@ private HttpResponseMessage CreateSelectionError( HttpControllerContext controll
203205
{
204206
Contract.Ensures( Contract.Result<HttpResponseMessage>() != null );
205207

206-
if ( !controllerContext.ControllerDescriptor.GetApiVersionModel().IsApiVersionNeutral )
207-
{
208-
return CreateBadRequestResponse( controllerContext );
209-
}
210-
211208
var actionsFoundByParams = FindMatchingActions( controllerContext, ignoreVerbs: true );
212209

213-
if ( actionsFoundByParams.Count > 0 )
210+
if ( actionsFoundByParams.Count == 0 )
214211
{
215-
return CreateMethodNotAllowedResponse( controllerContext, actionsFoundByParams );
212+
return CreateActionNotFoundResponse( controllerContext );
216213
}
217214

218-
return CreateActionNotFoundResponse( controllerContext );
215+
var request = controllerContext.Request;
216+
var versionNeutral = controllerContext.ControllerDescriptor.GetApiVersionModel().IsApiVersionNeutral;
217+
var exceptionFactory = new HttpResponseExceptionFactory( request );
218+
219+
return exceptionFactory.CreateMethodNotAllowedResponse( versionNeutral, allowedMethods );
219220
}
220221

221222
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Caller is responsible for disposing of response instance." )]
@@ -229,30 +230,6 @@ private static HttpResponseMessage CreateBadRequestResponse( HttpControllerConte
229230
return exceptionFactory.CreateBadRequestResponse( request.GetRequestedApiVersion() );
230231
}
231232

232-
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Caller is responsible for disposing of response instance." )]
233-
private static HttpResponseMessage CreateMethodNotAllowedResponse( HttpControllerContext controllerContext, IEnumerable<HttpActionDescriptor> allowedCandidates )
234-
{
235-
Contract.Requires( controllerContext != null );
236-
Contract.Requires( allowedCandidates != null );
237-
Contract.Ensures( Contract.Result<HttpResponseMessage>() != null );
238-
239-
var incomingMethod = controllerContext.Request.Method;
240-
var response = controllerContext.Request.CreateErrorResponse( MethodNotAllowed, SR.ApiControllerActionSelector_HttpMethodNotSupported.FormatDefault( incomingMethod ) );
241-
var methods = new HashSet<HttpMethod>();
242-
243-
foreach ( var candidate in allowedCandidates )
244-
{
245-
methods.UnionWith( candidate.SupportedHttpMethods );
246-
}
247-
248-
foreach ( var method in methods )
249-
{
250-
response.Content.Headers.Allow.Add( method.ToString() );
251-
}
252-
253-
return response;
254-
}
255-
256233
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Handled by the caller." )]
257234
private HttpResponseMessage CreateActionNotFoundResponse( HttpControllerContext controllerContext )
258235
{
@@ -264,22 +241,6 @@ private HttpResponseMessage CreateActionNotFoundResponse( HttpControllerContext
264241
return controllerContext.Request.CreateErrorResponse( NotFound, message, messageDetail );
265242
}
266243

267-
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Handled by the caller." )]
268-
private HttpResponseMessage CreateActionNotFoundResponse( HttpControllerContext controllerContext, string actionName )
269-
{
270-
Contract.Requires( controllerContext != null );
271-
Contract.Ensures( Contract.Result<HttpResponseMessage>() != null );
272-
273-
if ( !controllerContext.ControllerDescriptor.GetApiVersionModel().IsApiVersionNeutral )
274-
{
275-
return CreateBadRequestResponse( controllerContext );
276-
}
277-
278-
var message = SR.ResourceNotFound.FormatDefault( controllerContext.Request.RequestUri );
279-
var messageDetail = SR.ApiControllerActionSelector_ActionNameNotFound.FormatDefault( controllerDescriptor.ControllerName, actionName );
280-
return controllerContext.Request.CreateErrorResponse( NotFound, message, messageDetail );
281-
}
282-
283244
private static List<CandidateActionWithParams> GetInitialCandidateWithParameterListForDirectRoutes( HttpControllerContext controllerContext, IEnumerable<IHttpRouteData> subRoutes, bool ignoreVerbs )
284245
{
285246
Contract.Requires( controllerContext != null );
@@ -344,7 +305,11 @@ private CandidateAction[] GetInitialCandidateList( HttpControllerContext control
344305

345306
if ( actionsFoundByName.Length == 0 )
346307
{
347-
throw new HttpResponseException( CreateActionNotFoundResponse( controllerContext, actionName ) );
308+
var request = controllerContext.Request;
309+
var versionNeutral = controllerContext.ControllerDescriptor.GetApiVersionModel().IsApiVersionNeutral;
310+
var exceptionFactory = new HttpResponseExceptionFactory( request );
311+
312+
throw exceptionFactory.NewMethodNotAllowedException( versionNeutral, allowedMethods );
348313
}
349314

350315
var candidatesFoundByName = new CandidateAction[actionsFoundByName.Length];

‎src/Microsoft.AspNet.WebApi.Versioning/Dispatcher/ApiVersionControllerSelector.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,8 @@ private static void EnsureRequestHasValidApiVersion( HttpRequestMessage request
165165
catch ( AmbiguousApiVersionException ex )
166166
{
167167
var options = request.GetApiVersioningOptions();
168-
throw new HttpResponseException( options.CreateBadRequest( request, "AmbiguousApiVersion", ex.Message, null ) );
168+
var context = new ErrorResponseContext( request, "AmbiguousApiVersion", ex.Message, messageDetail: null );
169+
throw new HttpResponseException( options.ErrorResponses.BadRequest( context ) );
169170
}
170171
}
171172
}

‎src/Microsoft.AspNet.WebApi.Versioning/Dispatcher/HttpResponseExceptionFactory.cs

Lines changed: 78 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,23 @@
11
namespace Microsoft.Web.Http.Dispatcher
22
{
3+
using System;
4+
using System.Collections.Generic;
35
using System.Diagnostics.CodeAnalysis;
46
using System.Diagnostics.Contracts;
7+
using System.Linq;
58
using System.Net.Http;
69
using System.Web.Http;
10+
using System.Web.Http.Controllers;
711
using System.Web.Http.Dispatcher;
812
using System.Web.Http.Tracing;
913
using Versioning;
1014
using static ApiVersion;
1115
using static System.Net.HttpStatusCode;
16+
using static System.String;
1217

1318
internal sealed class HttpResponseExceptionFactory
1419
{
20+
const string Allow = nameof( Allow );
1521
private static readonly string ControllerSelectorCategory = typeof( IHttpControllerSelector ).FullName;
1622
private readonly HttpRequestMessage request;
1723

@@ -30,8 +36,9 @@ internal HttpResponseException NewNotFoundOrBadRequestException( ControllerSelec
3036
CreateBadRequest( conventionRouteResult, directRouteResult ) ?? CreateNotFound( conventionRouteResult );
3137

3238
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Created exception cannot be disposed. Handled by the caller." )]
33-
internal HttpResponseMessage CreateBadRequestResponse( ApiVersion requestedVersion ) =>
34-
requestedVersion == null ? CreateBadRequestForUnspecifiedApiVersionOrInvalidApiVersion() : CreateBadRequestForUnsupportedApiVersion( requestedVersion );
39+
internal HttpResponseMessage CreateBadRequestResponse( ApiVersion requestedVersion ) => requestedVersion == null ?
40+
CreateBadRequestForUnspecifiedApiVersionOrInvalidApiVersion( versionNeutral: false ) :
41+
CreateBadRequestForUnsupportedApiVersion( requestedVersion );
3542

3643
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Created exception cannot be disposed. Handled by the caller." )]
3744
internal HttpResponseException CreateBadRequest( ApiVersion requestedVersion ) => new HttpResponseException( CreateBadRequestResponse( requestedVersion ) );
@@ -60,17 +67,24 @@ private HttpResponseException CreateBadRequest( ControllerSelectionResult conven
6067
}
6168

6269
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Created exception cannot be disposed. Handled by the caller." )]
63-
private HttpResponseMessage CreateBadRequestForUnspecifiedApiVersionOrInvalidApiVersion()
70+
private HttpResponseMessage CreateBadRequestForUnspecifiedApiVersionOrInvalidApiVersion( bool versionNeutral )
6471
{
6572
var requestedVersion = request.GetRawRequestedApiVersion();
6673
var parsedVersion = default( ApiVersion );
6774
var message = default( string );
75+
var context = default( ErrorResponseContext );
6876

69-
if ( string.IsNullOrEmpty( requestedVersion ) )
77+
if ( IsNullOrEmpty( requestedVersion ) )
7078
{
79+
if ( versionNeutral )
80+
{
81+
return null;
82+
}
83+
7184
message = SR.ApiVersionUnspecified;
7285
TraceWriter.Info( request, ControllerSelectorCategory, message );
73-
return Options.CreateBadRequest( request, "ApiVersionUnspecified", message, messageDetail: null );
86+
context = new ErrorResponseContext( request, "ApiVersionUnspecified", message, messageDetail: null );
87+
return Options.ErrorResponses.BadRequest( context );
7488
}
7589
else if ( TryParse( requestedVersion, out parsedVersion ) )
7690
{
@@ -79,10 +93,11 @@ private HttpResponseMessage CreateBadRequestForUnspecifiedApiVersionOrInvalidApi
7993

8094
message = SR.VersionedResourceNotSupported.FormatDefault( request.RequestUri, requestedVersion );
8195
var messageDetail = SR.VersionedControllerNameNotFound.FormatDefault( request.RequestUri, requestedVersion );
96+
context = new ErrorResponseContext( request, "InvalidApiVersion", message, messageDetail );
8297

8398
TraceWriter.Info( request, ControllerSelectorCategory, message );
8499

85-
return Options.CreateBadRequest( request, "InvalidApiVersion", message, messageDetail );
100+
return Options.ErrorResponses.BadRequest( context );
86101
}
87102

88103
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Created exception cannot be disposed. Handled by the caller." )]
@@ -93,12 +108,67 @@ private HttpResponseMessage CreateBadRequestForUnsupportedApiVersion( ApiVersion
93108

94109
var message = SR.VersionedResourceNotSupported.FormatDefault( request.RequestUri, requestedVersion );
95110
var messageDetail = SR.VersionedControllerNameNotFound.FormatDefault( request.RequestUri, requestedVersion );
111+
var context = new ErrorResponseContext( request, "UnsupportedApiVersion", message, messageDetail );
112+
113+
TraceWriter.Info( request, ControllerSelectorCategory, message );
114+
115+
return Options.ErrorResponses.BadRequest( context );
116+
}
117+
118+
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Created exception cannot be disposed. Handled by the caller." )]
119+
internal HttpResponseMessage CreateMethodNotAllowedResponse( bool versionNeutral, IEnumerable<HttpMethod> allowedMethods )
120+
{
121+
Contract.Requires( allowedMethods != null );
122+
123+
var response = CreateBadRequestForUnspecifiedApiVersionOrInvalidApiVersion( versionNeutral );
124+
125+
if ( response != null )
126+
{
127+
return response;
128+
}
129+
130+
var requestedMethod = request.Method;
131+
var version = request.GetRequestedApiVersion()?.ToString() ?? "(null)";
132+
var message = default( string );
133+
var messageDetail = default( string );
134+
135+
if ( versionNeutral )
136+
{
137+
message = SR.VersionedResourceNotSupported.FormatDefault( request.RequestUri, version );
138+
messageDetail = SR.VersionedControllerNameNotFound.FormatDefault( request.RequestUri, version );
139+
}
140+
else
141+
{
142+
message = SR.VersionedMethodNotSupported.FormatDefault( version, requestedMethod );
143+
messageDetail = SR.VersionedActionNameNotFound.FormatDefault( request.RequestUri, requestedMethod, version );
144+
}
96145

97146
TraceWriter.Info( request, ControllerSelectorCategory, message );
98147

99-
return Options.CreateBadRequest( request, "UnsupportedApiVersion", message, messageDetail );
148+
var context = new ErrorResponseContext( request, "UnsupportedApiVersion", message, messageDetail );
149+
150+
response = Options.ErrorResponses.MethodNotAllowed( context );
151+
152+
if ( response.Content == null )
153+
{
154+
response.Content = new StringContent( Empty );
155+
response.Content.Headers.ContentType = null;
156+
}
157+
158+
var headers = response.Content.Headers;
159+
160+
if ( headers.Allow.Count == 0 )
161+
{
162+
headers.Allow.AddRange( allowedMethods.Select( m => m.Method ) );
163+
}
164+
165+
return response;
100166
}
101167

168+
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Created exception cannot be disposed. Handled by the caller." )]
169+
internal HttpResponseException NewMethodNotAllowedException( bool versionNeutral, IEnumerable<HttpMethod> allowedMethods ) =>
170+
new HttpResponseException( CreateMethodNotAllowedResponse( versionNeutral, allowedMethods ) );
171+
102172
[SuppressMessage( "Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Created exception cannot be disposed. Handled by the caller." )]
103173
private HttpResponseException CreateNotFound( ControllerSelectionResult conventionRouteResult )
104174
{
@@ -107,7 +177,7 @@ private HttpResponseException CreateNotFound( ControllerSelectionResult conventi
107177
var message = SR.ResourceNotFound.FormatDefault( request.RequestUri );
108178
var messageDetail = default( string );
109179

110-
if ( string.IsNullOrEmpty( conventionRouteResult.ControllerName ) )
180+
if ( IsNullOrEmpty( conventionRouteResult.ControllerName ) )
111181
{
112182
messageDetail = SR.ControllerNameNotFound.FormatDefault( request.RequestUri );
113183
}

0 commit comments

Comments
 (0)