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

Add option to interpret a result of custom url resolver as MVC url #392

Closed
lvv83 opened this issue May 7, 2015 · 12 comments
Closed

Add option to interpret a result of custom url resolver as MVC url #392

lvv83 opened this issue May 7, 2015 · 12 comments

Comments

@lvv83
Copy link

lvv83 commented May 7, 2015

Hello everyone!

I work under integration between MvcSiteMapProvider and MvcCodeRouting projects. This is technically available because your library has very good architecture based on the dependency injection pattern. So, my respects to you! :)

I met one issue, which workaround doesn't seems nice (I got it). The problem is custom url resolver result always treat as non-MVC url (see): node.UsesDefaultUrlResolver() returns false, but I need true, because the node doesn't have url, it is a original SiteMapNode which has passed through specific filter.

Any thoughts? Thanks!

@NightOwl888
Copy link
Collaborator

In general, I don't recommend using a custom URL resolver. The only use case this makes sense is when you want MvcSiteMapProvider to resolve the URLs differently than MVC, which isn't normally the case. I haven't worked with MvcCodeRouting, so I am not sure how it differs from .NET routing but as long as it is just a wrapper around the .NET routing instead of a replacement for it you should be able to follow this advice.

Unfortunately, there were some problems in the initial 4.x releases of MvcSiteMapProvider when it couldn't determine the difference between a URL-based route and an MVC-based route. The proper way to fix them required a breaking interface change in order to pass the information about what the source of the URL was (or better yet, use 2 different interfaces), which is something I plan to do in the next major version. But in the meantime, using a custom URL resolver is less than ideal.

That said, there are 2 ways to address this if you must go down this road:

  1. You can subclass the SiteMap class and override AddNodeInternal with a custom implementation. You can then create and inject a custom ISiteMapFactory so MvcSiteMapProvider creates instances of your custom SiteMap class.
  2. You could fork and modify the source code to meet your needs and use the build script to build your own NuGet package. Note that if you are using continuous integration to build your MVC site, you can create a free MyGet account to host the NuGet feed so you can download the packages during build rather than checking the binaries into source control. The build script is already set up to work with MyGet and it can be set up in a few minutes to build your custom NuGet packages.

@lvv83
Copy link
Author

lvv83 commented May 7, 2015

Thank you, Shad.

I'm already going to first way with AddNodeInternal override. And its works. However 95% lines of code stay unchanged in this implementation.

Integration between MvcSiteMapProvider and MvcCodeRouting based on passing instance of RequestContext class with portion of modifications - some data stored inside RouteData.DataTokens dictionary.

This data extracted by my implementation of ReflectionSiteMapNodeProvider class from controller type and stored to ISiteMapNode.Attributes dictionary.

And finally SiteMapNodeUrlResolver implementation - point, where RequestContext and ISiteMapNode instances meet each other.

So, if there some another place inside MvcSiteMapProvider to transfer data between RequestContext and ISiteMapNode except ISiteMapNodeUrlResolver then I'll try to use it.

@NightOwl888
Copy link
Collaborator

Oops, I misspoke before. You should subclass RequestCacheableSiteMap, not SiteMap. Or at the very least you should subclass LockableSiteMap and include the caching logic from RequestCacheableSiteMap or you will run into performance and other strange issues.

The SiteMap interacts with RequestContext in 2 places:

  1. During SiteMapNode.PreserveRouteParameters the values of the keys that are configured as preserved route parameters are copied from the query string and/or RouteValueDictionary of the request. This happens per request and is request cached.
  2. In SiteMap.FindSiteMapNode it compares the current request with each node until it finds a match.

As the data loaded from any ISiteMapNodeProvider is cached (and should be cached for performance reasons), you should never interact with the current request there. Instead, you should subclass RequestCacheableSiteMapNode and override PreserveRouteParameters to transfer any required data from the current request into the node.

There is a demo showing how to interact with the user's session. While that is not exactly what you want, it does demonstrate an approach you can use to interact with a different part of the RequestContext than RouteData.

In the demo, there is a custom URL resolver because it needs a way to ignore the data from session state when building the URL. However, if your use case does not have any extra data that has nothing to do with building the URL you likely won't need a custom URL resolver.

As for the matching, there are 2 different strategies - one to one and one to many, and there is a post about it here.

@lvv83
Copy link
Author

lvv83 commented May 11, 2015

Well. I decide to keep my first solution with RequestCacheableSiteMap.AddNodeInternal override and custom ISiteMapNodeUrlResolver.

  1. I need to pass data from ISiteMapNode to the RequestContext (not vice versa).
  2. RequestContext instance should be modified before this line where it passed to the ASP.NET MVC infrastructure.
public class MySiteMapNodeUrlResolver : SiteMapNodeUrlResolver
{
    public MySiteMapNodeUrlResolver(IMvcContextFactory mvcContextFactory, IUrlPath urlPath)
        : base(mvcContextFactory, urlPath)
    {
    }

    protected override RequestContext CreateRequestContext(ISiteMapNode node, TextWriter writer)
    {
        var context = base.CreateRequestContext(node, writer);
        if (node.Attributes["MvcCodeRouting.RouteContext"] != null)
            context.RouteData.DataTokens["MvcCodeRouting.RouteContext"] = node.Attributes["MvcCodeRouting.RouteContext"];

        return context;
    }
}

@anve
Copy link

anve commented Jun 9, 2015

Ivv83, care to share your full implementation for solving this? I find myself in the very same situation...!

@lvv83
Copy link
Author

lvv83 commented Jun 9, 2015

Hello, anve.

Which implementation you need? Full integration sample with MvcCodeRouting? Or solution for this issue?

@anve
Copy link

anve commented Jun 9, 2015

Hi! I do use MvcCodeRouting together with MvcSitemapProvider and suffer from the same problem. The node's URL doesn't resolve correctly, which I believe is due to missing out on the MvcCodeRouting data in RouteData.DataTokens, not being picked up by the default Url Resolver.

What I'm looking for, I guess, is your subclass of RequestCacheableSiteMap (.AddNodeInternal), as well as the custom ISiteMapNodeUrlResolver. And ReflectionSiteMapNodeProvider as well :)

I guess I'll have to setup MvcSiteMapProvider with an external DI (structuremap already in place in the project in general) in order to get those custom types/overrides in place, right? If so, anything special to put in the DI configuration module (e.g. MvcSiteMapProviderRegistry or similar)?

Thanks for your help!

@lvv83
Copy link
Author

lvv83 commented Jun 9, 2015

Welcome to my repo!

You should consider, that:

  • Sitemap should be configured via MvcSiteMapNode attribute;
  • XML-files not supported;
  • Markup for main menu is very simple and ugly (no dropdowns or css-styling)
  • External Dependency Injection engine should be enabled (SimpleInjector here).

@anve
Copy link

anve commented Jun 10, 2015

Much appreciated! With some minor adjustments to fit this into structuremap, this was exactly was I was looking for and now everything behaves correctly!

@NightOwl888
Copy link
Collaborator

@lvv83

Thanks for providing a working demo. I was able to reverse-engineer it and make the default URL resolver provide the missing piece required by MvcCodeRouting. See the diff here. I would appreciate if you could give it a test drive to see if it meets your needs.

https://github.com/maartenba/MvcSiteMapProvider/tree/dev

It also occurred to me that there could be deeper integration with MvcCodeRouting. .NET Routing does not provide a hierarchy. MvcCodeRouting does. Therefore, it should be possible to use Reflection to get each of the controller types and drive the key/parent key relationships directly from the namespace structure (or directory structure). That basically means you won't have to provide a configuration at all for MvcSiteMapProvider when using MvcCodeRouting.

My thought is to create another ISiteMapNodeProvider implementation named MvcCodeRoutingSiteMapNodeProvider that is similar to the ReflectionSiteMapNodeProvider, but it has a default behavior that is completely convention based.

The MvcSiteMapNodeAttribute could be reused to override the conventions as well as to provide properties that are not supported by the conventions.

There would also need to be an MvcSiteMapIgnoreAttribute to opt out of the default convention and we could potentially use MvcCodeRouting attributes to detect when the conventions need to be changed. For example, we could possibly use the FromRoute attribute to populate the PreservedRouteParameters collection.

Thoughts? Would you be interested in contributing?

@lvv83
Copy link
Author

lvv83 commented Oct 15, 2015

@NightOwl888, thank you for efforts!

I just submit updates to the demo application.
Simple Injector 3 also installed, but in the initializer module I excluded this line because an exception thrown.
Everything else looks like fine!

With deep integration with MvcCodeRouting, there are many questions for me:

  1. Should we add MvcCodeRouting reference to MvcSiteMapProvider project?
  2. How MvcCodeRouting will work under ASP.NET 5?
  3. Should MvcCodeRouting play together with xml sitemap provider?
  4. Authorization not used yet in the demo. I think, we need some modifications in AclModules for MvcCodeRouting support.

@NightOwl888
Copy link
Collaborator

Simple Injector 3 also installed, but in the initializer module I excluded this line because an exception thrown.

Exception is because you didn't include the line to exclude IMvcContextFactory from auto-registration. In my tests, without doing so Simple Injector will sometimes throw a warning about a lifestyle conflict.

Should we add MvcCodeRouting reference to MvcSiteMapProvider project?

No.

I am thinking we can for the most part use the same conventions they do. Whether or not we have a reference to the project, .NET reflection is involved with getting attributes, so it is better not to reference the project. There is an option when using reflection to get the type based on a string representation of the name without actually checking if the library is referenced (I used that technique here). If the type doesn't exist, you just get a null result. Since nodes only load periodically, I am not to worried about being a little heavy on reflection.

Should MvcCodeRouting play together with xml sitemap provider?

Yes.

It is a good thing you brought it up because the XML file owns the root node when enabled. However, there is an option called includeRootNode in the constructor of XmlSiteMapNodeProvider which can be set to false if the MvcSiteMapProvider_EnableMvcCodeRouting option is enabled. I am not sure it will work, though - I think when I put that in there I made the assumption that the XML configuration had a parentKey attribute, but it doesn't, so there would be no way to attach the rest of the XML nodes to the others in the SiteMap.

The bigger question is whether or not it should be used with the ReflectionSiteMapNodeProvider. If both use the same attributes, how could we tell the difference? My original thought was to turn this feature off when using MvcCodeRouting.

Of course, the default settings shouldn't change from the way they are now. The internal DI container will need these new configurations built in. External DI people will need to follow a document to enable MvcCodeRouting.

Authorization not used yet in the demo. I think, we need some modifications in AclModules for MvcCodeRouting support.

I don't think so, but maybe. I checked and everything seemed fine, but I am no expert on all of the different configurations MvcCodeRouting supports. The AuthorizeAttributeAclModule has its own controller type resolver which seems to work fine with MvcCodeRouting - routing is not involved anywhere except to build the URL. And the URL is always built by the URL resolver.

How MvcCodeRouting will work under ASP.NET 5 ?

Probably moot.

One of the options on the table right now is to let MvcSiteMapProvider be phased out with MVC5 and create a new design for DNX core. But most likely we will come up with some option for users to make the switch to ASP.NET 5 first before working on a new version (probably not named MvcSiteMapProvider).

There will still be a need for MVC4 and 5 support for another 2-3 years because the leap onto the new platform is a big one. It is almost as big of a leap as from VB6 to .NET was :).

I made one attempt to wrap the new ASP.NET 5 Context types into the old interfaces, but there are still a few incompatibilities that are preventing a build. The release today addressed some of those incompatibilities.

Anyway, perhaps the new design could include something like MvcCodeRouting so everything "just works" with little or no configuration (and lots of conventions). Of course, there should be a way to disable the hierarchical controller part, but it sounds like a reasonable default to me.

Also, the changes to the routing infrastructure are relatively minor. I think MvcCodeRouting will have an easier time supporting DNX core than we will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants