ploeh / Hyprlinkr

A URI building helper library for ASP.NET Web API
MIT License
197 stars 34 forks source link

Support for attribute-based route names #44

Open joaopbnogueira opened 7 years ago

joaopbnogueira commented 7 years ago

I am using attribute routing in my controllers, using the [Route] attribute with route names, and I noticed that Hyprlinkr always assumes that there's a route named 'DefaultApi' to generate the Uris, which does not work with attribute routing.

Is that correct, or am I missing something?

Thank you

ploeh commented 7 years ago

IIRC, that's what IRouteDispatcher is for. There's a bit of a discussion in #30.

joaopbnogueira commented 7 years ago

Hi, yes, I believe that you are right, but I meant for Controller-level attribute routing.

Like so :

[Route("/controllerCustomRoute", Name = ControllerRouteName)] 
public class RouteAttributeController : ApiController

I forked your code and did some modifications but the build validation step is failing with the following error:

c:\CloudStation\MyDocuments\Projects\Hyprlinkr\Hyprlinkr\DefaultRouteDispatcher.cs(138): error CA1308: Microsoft.Globalization : In method 'DefaultRouteDispatcher.AddControllerNameAndMethodToRouteValues(MethodCallExpression, IDictionary<string, object>)', replace the call to 'string.ToLowerInvariant()' with String.ToUpperInvariant(). [C:\CloudStation\MyDocuments\Projects\Hyprlinkr\Hyprlinkr\Hyprlinkr.csproj]

Any idea why? The existing code base is using ToLowerInvariant() ...

The offending code is:

var newRouteValues = new Dictionary<string, object>(routeValues);
var controllerName = callExpression.Object.Type.Name.ToLowerInvariant().Replace("controller", "");
newRouteValues["controller"] = controllerName;
newRouteValues["action"] = callExpression.Method.Name.ToLowerInvariant();
ploeh commented 7 years ago

It could be a tool versioning issue. CA is getting more and more fucked up, because different versions of Visual Studio come with different CA profiles. Which version of VS are you using?

joaopbnogueira commented 7 years ago

I'm using Visual Studio 2015 Enterprise Update 3, the IDE builds the code & runs the tests just fine.

ploeh commented 7 years ago

Sounds like my setup as well...

ploeh commented 7 years ago

I dug a bit deeper, and there's only one case of ToLowerInvariant in the existing code base, and it has a [SuppressMessage] attribute that explicitly suppresses CA1308, and also explains the reason. Now that I look at it, though, I think that the reason given there is incorrectly worded, but I'll have to look closer into that...

ploeh commented 7 years ago

It turns out that that suppression reason is fairly accurate after all. This is easy to discover if you remove that .ToLowerInvariant() line of code. When I do that, I get 14 failing tests. Here's one example:

Test 'Ploeh.Hyprlinkr.UnitTest.RouteLinkerTests.GetUriForGetMethodWithParameters(request: Method: methodfe5f3ab3-364a-4a99-b0f7-f2cca0aebc78, RequestUri: 'http://2f358f48-300e-479c-9449-67ac460d20ea/', Version: 0.0, Content: Castle.Proxies.HttpContentProxy, Headers:
{
}, sut: Ploeh.Hyprlinkr.RouteLinker, id: 233)' failed:
    Assert.Equal() Failure
Expected: http://2f358f48-300e-479c-9449-67ac460d20ea/api/foo/233
Actual:   http://2f358f48-300e-479c-9449-67ac460d20ea/api/FooController/233
    RouteLinkerTests.cs(91,0): at Ploeh.Hyprlinkr.UnitTest.RouteLinkerTests.GetUriForGetMethodWithParameters(HttpRequestMessage request, RouteLinker sut, Int32 id)

ASP.NET Web API wants the controller route value to be lower-cased, it seems. If you replace .ToLowerInvariant() with .ToUpperInvariant(), you have the same sort of problem.

It's been years since I wrote that, so I can't remember all of the details, so I wonder if the controller URL segment always has to be lower-case, or if that's only an artefact of the way I wrote the tests...

joaopbnogueira commented 7 years ago

You're right, there was a suppress attribute that I missed when I moved that particular bit of code to its own method.

How important are the F# tests? I don't have F# installed so I only added a test in the c# test project.

ploeh commented 7 years ago

The F# tests aren't that important; they're mostly there in order to verify that the GetUri method also works from F#, against Controller classes written in F#.

I can always add one or two for new methods, if I feel like it.

joaopbnogueira commented 7 years ago

Thanks, I just submitted a pull request (#45)