Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 405 handler #65

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions src/BotwinExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
namespace Botwin
{
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Threading.Tasks;
using FluentValidation;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
Expand Down Expand Up @@ -35,23 +37,58 @@ public static IApplicationBuilder UseBotwin(this IApplicationBuilder builder, Bo
ApplyGlobalAfterHook(builder, options);

var routeBuilder = new RouteBuilder(builder);
var systemRoutes = new List<(string verb, string route)>();

//Create a "startup scope" to resolve modules from
using (var scope = builder.ApplicationServices.CreateScope())
{
var modules = scope.ServiceProvider.GetServices<BotwinModule>();

//Get all instances of BotwinModule to fetch and register declared routes
foreach (var module in scope.ServiceProvider.GetServices<BotwinModule>())
foreach (var module in modules)
{
var moduleType = module.GetType();

foreach (var route in module.Routes.Keys)
{
routeBuilder.MapVerb(route.verb, route.path, CreateRouteHandler(route, moduleType));

var strippedPath = route.path.EndsWith("/") ? route.path.Substring(0, route.path.Length - 1) : route.path;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what David would say about this (allocations!?!?!), but maybe just route.path.TrimEnd('/')?

systemRoutes.Add((route.verb, "/" + strippedPath));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be cheaper to leave off the leading slash here, and trim the leading slash from the current request later instead.

}
}

builder.UseRouter(routeBuilder.Build());

return builder.Use((ctx, next) => GetMethodNotAllowedHandler(ctx, next, systemRoutes));
}
}

/// <summary>
/// This method is only called if it's a valid 404 or 405
/// </summary>
/// <param name="context"></param>
/// <param name="next"></param>
/// <param name="systemRoutes"></param>
/// <returns></returns>
private static async Task GetMethodNotAllowedHandler(HttpContext context, Func<Task> next, IEnumerable<(string verb, string route)> systemRoutes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this does not return a handler function (which is what I was originally suggesting) but is the handler, the Get feels out of place here. HandleMethodNotAllowed? Or maybe you should just move this out to a custom middleware class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah tried your approach but no joy

{
//Call the final pipeline which gets ASP.Net Core status code, usually a 404 in this case
await next();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you continuing the pipeline here at the beginning? Shouldn’t this handler already be kind of the final one in the pipeline? Do you need to know that it produced a 404? Wouldn’t it be safe to assume that if this handler runs after the routing middleware, then a route was not found? (again: I have no actual idea about the routing middleware)

Copy link
Member Author

@jchannon jchannon Nov 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oddly before that is called the response status code is 200, if you call next it's then 404 from the router. Not sure how because as you say I'd expect it to be already at the end of the pipeline

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think you actually need to check the status code here though. This should not run if the router succeeded in the pipeline step before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly if I remove the next() call it will always be 200 on a route not defined in Botwin. If I take the GetMethodNotAllowedHandler out completely I get a 404 so I assume there must be one last pipeline called somewhere


return builder.UseRouter(routeBuilder.Build());
var strippedPath = context.Request.Path.Value.EndsWith("/") && context.Request.Path.Value.Length > 1
? context.Request.Path.Value.Substring(0, context.Request.Path.Value.Length - 1)
: context.Request.Path.Value;

//ASP.Net Core will set a 405 response to 404. Let's check if it's a valid 404 first otherwise if we know about the route it's most likely a 405
if (context.Response.StatusCode == 404 && systemRoutes.Any(x => x.route == strippedPath))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use HashSet or Dictionary for constant time lookup. Also, I don’t know if the router middleware is case insensitive itself but if it is, then pure string equality will not work (you will have to use the RouteComparer we came up with earlier).

{
var verbsForPath = systemRoutes.Where(x => x.route == strippedPath).Select(y => y.verb);
if (verbsForPath.All(x => x != context.Request.Method))
{
context.Response.StatusCode = 405;
}
}
}

private static RequestDelegate CreateRouteHandler((string verb, string path) route, Type moduleType)
Expand Down
25 changes: 21 additions & 4 deletions test/BotwinModuleTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@ public class BotwinModuleTests
public BotwinModuleTests()
{
this.server = new TestServer(new WebHostBuilder()
.ConfigureServices(x =>
{
x.AddBotwin(typeof(TestModule).GetTypeInfo().Assembly);
})
.ConfigureServices(x => { x.AddBotwin(typeof(TestModule).GetTypeInfo().Assembly); })
.Configure(x => x.UseBotwin())
);
this.httpClient = this.server.CreateClient();
Expand Down Expand Up @@ -261,5 +258,25 @@ public async Task Should_return_GET_requests_with_parsed_quersytring_with_defaul
Assert.Equal(200, (int)response.StatusCode);
Assert.True(body.Contains("Managed to parse default int 69"));
}

[Theory]
[InlineData("/405test")]
[InlineData("/405test/")]
[InlineData("/405testwithslash")]
[InlineData("/405testwithslash/")]
public async Task Should_return_405_if_path_not_found_for_supplied_method(string path)
{
var response = await this.httpClient.PostAsync(path, new StringContent(""));

Assert.Equal(405, (int)response.StatusCode);
}

[Fact]
public async Task Should_return_404_if_path_not_found()
{
var response = await this.httpClient.GetAsync("/flibbertygibbert");

Assert.Equal(404, (int)response.StatusCode);
}
}
}
3 changes: 3 additions & 0 deletions test/TestModule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ public TestModule()
await ctx.Response.WriteAsync(content);
});

this.Get("405test", context => context.Response.WriteAsync("hi"));
this.Get("405testwithslash/", context => context.Response.WriteAsync("hi"));

this.Post("/", async (ctx) => { await ctx.Response.WriteAsync("Hello"); });
this.Put("/", async (ctx) => { await ctx.Response.WriteAsync("Hello"); });
this.Delete("/", async (ctx) => { await ctx.Response.WriteAsync("Hello"); });
Expand Down