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

Enabling attribute routes and RouteCollectionRoute #273

Merged
merged 1 commit into from Feb 9, 2014

Conversation

@giggio
Copy link
Contributor

commented Feb 6, 2014

If you use attribute routing then the SiteMap class will not select the correct RouteData on method FindSiteMapNodeFromMvc. This is because with attribute routing you may get a RouteCollectionRoute when requesting it through GetRouteData(httpContext). This Collection will have route matches on the the MS_DirectRouteMatches value. Then you need to get the correct route data from it this matches. I picked the first one.
This is only two lines of code long and easy to merge. I hope it helps.

@NightOwl888

This comment has been minimized.

Copy link
Collaborator

commented Feb 7, 2014

Thanks for the patch. I have a few questions though:

  1. Is this for the MS flavor of attribute routing (in MVC 5), the open source project, or will it work for both?
  2. I notice you are completely overwriting routeValues when the "MS_DirectRouteMatches" exists. That makes me wonder if this will work in a project that uses both attribute routing and "standard" routing. Have you tried this?
  3. Are you 100% sure that there is always a single value in the "MS_DirectRouteMatches" IEnumerable<RouteData> and the correct choice is to use the first one?
  4. If the value of "MS_DirectRouteMatches" is null or the IEnumerable<RouteData> is empty, the First() method will throw an exception. Shouldn't there be a guard to protect against these potential problems?
if (routeData.Values.ContainsKey("MS_DirectRouteMatches") &&
    routeData.Values["MS_DirectRouteMatches"] != null &&
    ((IEnumerable<RouteData>)routeData.Values["MS_DirectRouteMatches"]).Count == 1))
@giggio

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2014

Hello there,

  1. This is for the MS flavor. I have not tried the other one, but if it does something similar using RouteCollectionRoute it probably will not work, as I am checking for a specific MS key.
  2. It does, as it only overriding the RouteData on a specific search for a route on the FindSiteMapNodeFromMvc method. The route collection is never changed. The project I have this working on has both types of routes, traditional and attribute.
  3. There is only one, I have checked. This is the RouteCollectionRoute source:
    http://aspnetwebstack.codeplex.com/SourceControl/latest#src/System.Web.Http/Routing/RouteCollectionRoute.cs
    This is the only place where it is used:
    http://aspnetwebstack.codeplex.com/SourceControl/latest#src/System.Web.Http/Routing/AttributeRoutingMapper.cs
    It is added only once, if, and only if, you are using attribute routing.
  4. This cannot happen, the value cannot be null or empty. On the same source code for RouteCollectionRoute you can see that this key is only added if there is an Array of at least one value. If there are no matches they key would not be added. The count should not be compared to 1 because more than one route could be matched, and for this reason I am selecting the first.

I have been running it for a while on my app, and so far no problems.

I hope it helps.

@NightOwl888 NightOwl888 merged commit 436fada into maartenba:master Feb 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.