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.

Sign up for GitHub

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

Support for controller-level attribute routes. Resolves #44 #45

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support for controller-level attribute routes. Resolves #44 #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d8849d
support for controller attribute routing
joaopbnogueira Feb 20, 2017
17b472e
support for controller-level route attribute
joaopbnogueira Feb 22, 2017
39e7055
limit line lengths to 120 characters
joaopbnogueira Feb 22, 2017
832a202
delete UpgradeLog.htm
joaopbnogueira Feb 22, 2017
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
12 changes: 10 additions & 2 deletions Hyprlinkr.UnitTest/Controllers/RouteAttributeController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,22 @@

namespace Ploeh.Hyprlinkr.UnitTest.Controllers
{
[Route("/controllerCustomRoute", Name = ControllerRouteName)]
public class RouteAttributeController : ApiController
{
public const string RouteName = "CustomRoute";
public const string ControllerRouteName = "ControllerCustomRoute";

[Route("/customRoute", Name = RouteName)]
public const string ActionRouteName = "CustomRoute";

[Route("/customRoute", Name = ActionRouteName)]
public object GetDefault()
{
return new object();
}

public object GetDefaultWithoutActionRouteName()
{
return new object();
}
}
}
15 changes: 13 additions & 2 deletions Hyprlinkr.UnitTest/DefaultRouteDispatcherTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public void DispatchReturnsResultWithCustomRouteName(
{
var actual = sut.Dispatch(method, routeValues);
Assert.Equal(sut.RouteName, actual.RouteName);
}
}

[Theory, AutoHypData]
public void DispatchReturnsResultWithRouterAttributeRouteName(
Expand All @@ -70,7 +70,18 @@ public void DispatchReturnsResultWithRouterAttributeRouteName(
Expression<Action<RouteAttributeController>> exp = c => c.GetDefault();
var method = (MethodCallExpression)exp.Body;
var actual = sut.Dispatch(method, routeValues);
Assert.Equal(RouteAttributeController.RouteName, actual.RouteName);
Assert.Equal(RouteAttributeController.ActionRouteName, actual.RouteName);
}

[Theory, AutoHypData]
public void DispatchReturnsResultWithRouterAttributeControllerRouteName(
DefaultRouteDispatcher sut,
IDictionary<string, object> routeValues)
{
Expression<Action<RouteAttributeController>> exp = c => c.GetDefaultWithoutActionRouteName();
var method = (MethodCallExpression)exp.Body;
var actual = sut.Dispatch(method, routeValues);
Assert.Equal(RouteAttributeController.ControllerRouteName, actual.RouteName);
}

[Theory, AutoHypData]
Expand Down
91 changes: 73 additions & 18 deletions Hyprlinkr/DefaultRouteDispatcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,34 +70,21 @@ public DefaultRouteDispatcher(string routeName)
/// <paramref name="method" />.
/// </para>
/// </remarks>
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Globalization", "CA1308:NormalizeStringsToUppercase", Justification = "This method should produce URIs with lower-case letters, so ultimately, it would have to invoke some sort of ToLower method.")]
public Rouple Dispatch(
MethodCallExpression method,
IDictionary<string, object> routeValues)
{
if (method == null)
throw new ArgumentNullException("method");

var methodRouteAttribute = method.Method.GetCustomAttribute<RouteAttribute>(false);

var routeAttribute = method
.Method
.GetCustomAttribute<RouteAttribute>(false);

if (routeAttribute != null && routeAttribute.Name != null)
if (methodRouteAttribute != null && methodRouteAttribute.Name != null)
{
return new Rouple(routeAttribute.Name, routeValues);
return new Rouple(methodRouteAttribute.Name, routeValues);
}

var newRouteValues = new Dictionary<string, object>(routeValues);

var controllerName = method
.Object
.Type
.Name
.ToLowerInvariant()
.Replace("controller", "");
newRouteValues["controller"] = controllerName;

return new Rouple(this.routeName, newRouteValues);
return ExtractRoupleFromController(method, routeValues);
}

/// <summary>
Expand All @@ -108,5 +95,73 @@ public string RouteName
{
get { return this.routeName; }
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Globalization", "CA1308:NormalizeStringsToUppercase", Justification = "This method should produce URIs with lower-case letters, so ultimately, it would have to invoke some sort of ToLower method.")]
private Rouple ExtractRoupleFromController(
MethodCallExpression callExpression,
IDictionary<string, object> routeValues)
{
var controllerType = callExpression.Object.Type;

var controllerRouteAttribute = controllerType.GetCustomAttribute<RouteAttribute>(false);
if (controllerRouteAttribute != null)
{
return ExtractRoupleFromControllerRouteAttribute(callExpression, routeValues, controllerRouteAttribute);
}

var controllerName = controllerType.Name.ToLowerInvariant().Replace("controller", "");

var newRouteValues = new Dictionary<string, object>(routeValues);
newRouteValues["controller"] = controllerName;

return new Rouple(this.routeName, newRouteValues);
}

private Rouple ExtractRoupleFromControllerRouteAttribute(
MethodCallExpression callExpression,
IDictionary<string, object> routeValues,
RouteAttribute controllerRouteAttribute)
{
var controllerRouteName = this.routeName;
if (controllerRouteAttribute.Name != null)
{
controllerRouteName = controllerRouteAttribute.Name;
}

var newRouteValues = AddControllerNameAndMethodToRouteValues(callExpression, routeValues);
newRouteValues = RemoveControllerAndActionRouteValuesIfNeeded(controllerRouteAttribute, newRouteValues);
Copy link
Owner

Choose a reason for hiding this comment

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

It's not that I want to be a killjoy here, but I can remove this method call without any tests failing.

It's not that I predict that a lot of work will be done on Hyprlinkr in the future, but I tend to rely heavily on unit tests when I refactor. My modus operandi is that if all tests are still green after a refactoring, then I broke nothing.

There's a risk, then, that I (or someone else) might break a feature for you in the future, because this isn't covered by tests. If this is important to you, would it be possible to add some tests? Or perhaps remove this for now, and then come back to it in a later pull request?

return new Rouple(controllerRouteName, newRouteValues);
}

private static Dictionary<string, object> RemoveControllerAndActionRouteValuesIfNeeded(
RouteAttribute controllerRouteAttribute,
Dictionary<string, object> routeValues)
{
var controllerRouteTemplate = controllerRouteAttribute.Template;
if (controllerRouteTemplate != null)
{
if (!controllerRouteTemplate.Contains("{controller}"))
{
routeValues.Remove("controller");
}
if (!controllerRouteTemplate.Contains("{action}"))
{
routeValues.Remove("action");
}
}
return routeValues;
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Globalization", "CA1308:NormalizeStringsToUppercase", Justification = "This method should produce URIs with lower-case letters, so ultimately, it would have to invoke some sort of ToLower method.")]
private static Dictionary<string, object> AddControllerNameAndMethodToRouteValues(
MethodCallExpression callExpression,
IDictionary<string, object> routeValues)
{
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();
Copy link
Owner

Choose a reason for hiding this comment

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

I can remove the above three statements without breaking any tests. What value do these lines of code add?

Copy link
Author

Choose a reason for hiding this comment

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

No problem, I should have added additional test cases.

Those methods manage the "controller" and "action" KVPs which may or may not be necessary to create the actual route Uri, depending on the actual Route Template.

The problem is that if they are put into the RouteValues dictionary and they are not needed, they will show up incorrectly as querystrings, which is not desirable.

I'll write the test cases that illustrate these issues.

Copy link
Owner

Choose a reason for hiding this comment

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

Here's a suggestion, which will make things go smoother:

  1. Decline this pull request. You can do this yourself.
  2. Send a new pull request without these extra features, but only handling of Controller-level attributes.
  3. Send another pull request later with the other changes.

Making pull requests smaller tend to make the entire process faster.

return newRouteValues;
}
}
}