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

Why does Default UrlResolver throw an exception #115

Closed
waynebrantley opened this issue Dec 8, 2012 · 7 comments
Closed

Why does Default UrlResolver throw an exception #115

waynebrantley opened this issue Dec 8, 2012 · 7 comments

Comments

@waynebrantley
Copy link

The current default UrlResolver tries to resolve all routes with 'default' values and throws an exception if it cannot. I do not believe this is necessary and does cause issues - here is an example use case.

If I have defined a route like {controller}/{action}/{myId} and put a constraint on the myId so it has to be an integer larger than 100.

In the sitemap (either XML or code based), you put this:
c# MvcSiteMapNode(Title = "Some Action", ParentKey = "SomeParent")]c#

When the sitemap is loaded, the base class tries to resolve the 'url' property of every node put in the sitemap, which includes this one.

When the default resolver tries to resolve this, it fails - as it should, because there is no 'myId' parameter which is required. The error message says to provide defaults, etc. This means I could add a default to my routevalue of say 100. If I do this it can resolve it. However, that means if the value is not passed it the action will be called with a value of 100 - sort of defeats the purpose of the constraints!

What I propose is a simple change at the end of the code for UrlResolver. Here is the current code:

if (string.IsNullOrEmpty(returnValue))
{
    throw new UrlResolverException(string.Format(Messages.CouldNotResolve, mvcSiteMapNode.Title, action, controller, mvcSiteMapNode.Route ?? ""));
}

 _urlkey = key;
_url = returnValue;
return returnValue;

We simply do not throw that exception! This is only resolving because it was added to the sitemap and we are of course only going to call it with valid parameters in our code...so this change seems to work fine - and I cannot find any negative impacts.

if (string.IsNullOrEmpty(returnValue))
{
    return Guid.NewGuid().ToString();
}

 _urlkey = key;
_url = returnValue;
return returnValue;

Any thoughts on this? Why did it used to throw an exception anyway? Anything you can think of this will break? Seems to work fine for me.

@visoft
Copy link

visoft commented Feb 22, 2013

@waynebrantley - Thank you so much for this fix! I've been using Restful Routing in my MVC app. I was having issues with my resource based routes (e.g. edit) because it requires an id in the Url. It's not an optional parameter, so thus the default resolver was throwing exceptions. This workaround seems to work perfectly.

@pradeepn
Copy link

The above fix worked for me. Thanx for the fix. But if the site is in virtual directory it
throws error that the url is outside the app path for the website like localhost/site1

if (string.IsNullOrEmpty(returnValue))
{
    return "~/"+ Guid.NewGuid().ToString();
}
 _urlkey = key;
_url = returnValue;
return returnValue;

@NightOwl888
Copy link
Collaborator

Thanks for finding this. There are methods within the .NET framework to build the url using a virtual app path rather than the "~/" syntax that could be used for this purpose. I ran across a few of these methods in an internal static library called UrlPath when extracting the code for v4.

I will make sure this works in a virtual directory before the release.

@NightOwl888
Copy link
Collaborator

FYI - I changed this slightly for the v4 fix:

return VirtualPathUtility.ToAbsolute("~/") + Guid.NewGuid().ToString();

@NightOwl888
Copy link
Collaborator

@maartenba, @waynebrantley, @pradeepn, @visoft

What if we did this instead for this case?

return "#";

This would have been a problem back when this was first included because the route URLs were being added to a dictionary and no duplicates URLs were allowed. However, now that is not being done anymore.

To me it seems like returning a link that does nothing (aside from scrolling to the top) when you click on it would be better than returning one that generates a 404 not found. Neither one gives you a clear indication what is wrong, but at least the former doesn't also cause usability issues.

Thoughts?

@maartenba
Copy link
Owner

I'd vote yes. SOunds like a good compromise.

@visoft
Copy link

visoft commented Mar 7, 2014

Sounds good to me as well.

NightOwl888 added a commit to NightOwl888/MvcSiteMapProvider that referenced this issue Mar 10, 2014
…by default rather than a GUID URL, so it won't cause usability problems when using route constraints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants