add 405 fix handler

[jchannon]

Fixes #51

[JoeStead]

Dictionary<path, IEnumerable<verb> would probably read nicer in this method, and might be quicker than all of the linq too

[jchannon]

to load the dictionary you'd have to do systemRoutes.Add(route.path, module.Routes.Where(x=>x.path == route.path).Select(x=>x.verb));

i think this whole piece of code could be improved though

[JoeStead]

Hmm, you're not wrong. There's gotta be a nicer way of getting that code in there

[fdipuma]

Wouldn't something like this be more efficient in the long run?

var systemRoutes = srvs.SelectMany(m => m.Routes)
    .GroupBy(r => "/" + r.path)
    .ToDictionary(
        g => g.Key, 
        g => new HashSet<string>(
            g.Select(r => r.verb).Distinct()
        )
    ); // IDictionary<string, HashSet<string>>

We than could just do:

if (systemRoutes.ContainsKey(strippedPath) &&
    !systemRoutes[strippedPath].Contains(context.Request.Method)) // almost O(1) access time
{
    context.Response.StatusCode = 405;
    return;
}

(the LINQ part may be simplified into a foreach if it appears too cryptical)

[jchannon]

I did play with something similar after this PR. My actual concern is the string comparison of the paths for example a route might be defined as /home/ but the route that comes in might be /home or vice versa. Only way I can think is to strip both the registered path and the incoming path of the end slash but that seems inefficient to me but might be maybe only option we have

…

On 7 November 2017 at 22:58, Federico Dipuma ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/BotwinExtensions.cs <#52 (comment)>: > routeBuilder.MapVerb(route.verb, route.path, finalHandler); } } return builder.UseRouter(routeBuilder.Build()); } - + private static void Add405Handler(IApplicationBuilder builder, IEnumerable<BotwinModule> srvs) + { + var systemRoutes = new List<(string verb, string route)>(); Wouldn't something like this be more efficient in the long run? var systemRoutes = srvs.SelectMany(m => m.Routes) .GroupBy(r => "/" + r.path) .ToDictionary( g => g.Key, g => new HashSet<string>( g.Select(r => r.verb).Distinct() ) ); // IDictionary<string, HashSet<string>> We than could just do: if (systemRoutes.ContainsKey(strippedPath) && !systemRoutes[strippedPath].Contains(context.Request.Method)) // almost O(1) access time { context.Response.StatusCode = 405; return; } (the LINQ part may be simplified into a foreach if it appears too cryptical) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#52 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGapjXrmBvNWzWcFpCAx9ggupXIvxUdks5s0OCrgaJpZM4QQUML> .

[jchannon]

Pushed a change which should make the checks more precise but still not happy about the stripping per path on each request approach

[jchannon]

@davidfowl any better perf suggestions?

[jchannon]

Moved to #65