V4

[corneliuskopp]

As discussed in #138, I extended XmlSiteMapResult and XmlSiteMapNode to inject arbitrary additional content into sitemap nodes (for video, picture, news sitemaps) without having to subclass for each type.

[NightOwl888]

Great.

I took a look and there were a few things I noticed:

1. If you are familiar with configuring Ninject, we'd appreciate you contributing a Ninject module to wire up the MvcSiteMapProvider. You left a reference to Ninject in the ExtensibleSiteMap project - I assume that was unintentional.
2. Please put the code for your additions in the same project as MvcSiteMapProvider. I struggled with the question of 1 DLL vs 2 for a while, but when it came down to it, it is unnecessary to have more than 1. People tend to watch the number of dependencies very carefully these days. Its okay if you want to move the sitemaps code to a new namespace and put your changes in a namespace below it as appropriate. "Sitemaps" (plural) is technically the correct term to use for the xml that feeds search engines - it might help if it were changed throughout the project to set it apart from SiteMap.
3. Its probably not a good idea to name something "Extensible...". This implies that the rest of the project is not extensible. There used to be a namespace called Extensibility in v3, but I removed it for the same reason.
4. The SiteMapNode object is not the top level of the hierarchy, so you probably don't want to inherit it. The top level is RequestCacheableSitemapNode. This contains important request caching functionality as well as locking so the SiteMapNode is read-only after it has been populated and cached.
5. The Attributes dictionary is specifically there for extensibility, so you probably should reuse that instead of creating yet another extension point that has the same purpose (although you might want to change it from <string, string> to <string, object>, so it suits your purpose).
6. Thanks for contributing tests. I started a test project for the MvcSiteMapProvider that currently has 2 tests in it. I am not too picky about the conventions or tools as long as they are consistent throughout the project. You can stick with MSTest, but please convert the 2 unit tests I made into MSTest as well, and make the naming conventions consistent throughout. I vote for Moq as a mocking tool, though :). I would also appreciate if you put them into the Unit folder as we might want to add integration tests at some point.

Here is a summary of the tools and conventions I used in the test project:

Tools used:

• NUnit
• Moq

Conventions used:

• Path is [TestType]\[PathToClassUnderTest]\[ClassUnderTest] (with suffix of Test). Example: Unit\Loader\SiteMapLoaderTest.cs. Integration tests can be added later using this convention.
• Test method name is [MemberName]_[Condition]_[ExpectedResult]. Example: GetSiteMap_WithSiteMapCacheKey_ShouldCallGetOrAddWithSiteMapCacheKey()
• Tests follow the Arrange, Act, Assert pattern.

Here is the inheritance hierarchy for the node:

SiteMapNodeSecurityBase
  SiteMapNodePositioningBase
    SiteMapNode
      LockableSiteMapNode - Makes the site map node read-only after it has been populated and cached.
        RequestCacheableSiteMapNode - Caches some expensive members per request so they are not run more than once. Also makes some properties writable for the scope of the request.

[corneliuskopp]

Hi!
Thank you for your feedback on the code. In retrospect I realize my code was a bit "sloppy", re: conventions used in the project.

You are correct as to that the Ninject reference was an accident; I would be happy to contribute a Ninject module, though; I have to write some form of Ninject integration for my work project anyways I would assume. Is there a better place to ask about what kind of wiring up MvcSiteMapProvider needs, than issues / pull request comments? I had a look at the DI setup in the projects but am not 100% familiar with what is already used.

As for all other points mentioned, I will see to implementing them as soon as I can.

(And thank you for writing such helpful, approachable responses; I have met a lot of developers who would "bash noobs", rather than provide constructive feedback like this. It's refreshing (unfortunately). :))

[NightOwl888]

Well, first drafts are almost always sloppy :). Besides, I have only been involved with this project since January, so I am a bit of a noob myself. Most of what I did was refactoring the code that was there previously, but I did all of that just so I could build in the multi-tenant support that I needed.

I will let you know on the Ninject module - I am working with Maarten to try to straighten out the DI configuration code and corresponding Nuget packages. Just so you know what you are in for, have a look at the MvcSiteMapProviderRegistry.cs and MvcRegistry.cs files. These are to configure StructureMap. The former is everything that is needed for a "standard" configuration of MvcSiteMapProvider, the latter is to prevent MVC from throwing an error because instances of controllers are not allowed to be reused. Do note that this code will be injected into the MVC project as code files so they can be altered by the end developer as needed.

[NightOwl888]

Update:

I have added additional projects to the solution to make building DI wiring code easier to contribute and maintain. https://github.com/maartenba/MvcSiteMapProvider/tree/v4/src/MvcSiteMapProvider/CodeAsConfiguration

I have also added a skeleton project for Ninject - all that is left is to implement the "default" wiring code for Ninject the same way it is for StructureMap:

• MvcSiteMapProviderRegistry.cs --> MvcSiteMapProviderModule.cs
• MvcRegistry.cs --> MvcModule.cs

A few things to be aware of in the projects in the CodeAsConfiguration folder:

• The conditional symbols are required to ensure we can support every version of MVC > 2 and .NET > 3.5
• The build process doesn't support the #region statement because of the way it handles the conditional compilation symbols. This only applies to the source files in the CodeAsConfiguration projects.
• You can add or remove modules to the project as needed, but always start the namespace with the name of the root folder, not the project. For example, you will need to change namespace Ninject.DI.Ninject.Modules to namespace DI.Ninject.Modules.
• The build process will not automatically include any files located outside of the App_Start and DI folders.
• The code can be run in debug mode by adding "Ninject" (without the quotes) as a conditional compilation symbol in the MvcMusicStore project properties.
• We are planning to support both IControllerFactory and IDependencyResolver MVC integration points for DI. Both are wired up already and IControllerFactory is the default. To test IDependencyResolver, add "DependencyResolver" (without the quotes) as a compilation symbol to the MvcMusicStore project properties. Note that IControllerFactory and IDependencyResolver cannot be used at the same time in MVC.

[corneliuskopp]

Hi!

An update for me as well - unfortunately, as so often, priorities have changed on the project I work on. I have to postpone working on this for a while. Potentially a few weeks. I am sorry for this back-and-forth.

[NightOwl888]

Not a problem. This is the nature of open source :).

FYI - I just finished (I hope) the pre-release Nuget packages today. You can view them on this feed: https://www.myget.org/F/mvcsitemapprovider/. The ones that don't match version numbers with the highest version are no longer valid - we ended up changing the package structure a few times because Nuget doesn't support multiple versions of MVC.

[NightOwl888]

Update

I have created a functional Ninject module for MvcSiteMapProvider.

However, it needs to be a little safer to interact with a real-world configuration. Namely, it should only auto-register types for interfaces that are part of MvcSiteMapProvider or MVC/.NET interfaces that are defined by MvcSiteMapProvider. This could be accomplished by not registering any interfaces that are already registered with Ninject. There is probably a really simple way to accomplish that, but I am a bit clueless when it comes to Ninject.

Could you please have a look and see if you can work out how to make this code a little safer?

Do note that I realize the named instances are not required for this particular configuration, but I put them in so another developer can easily find the path to providing additional implementations of the same interfaces that are explicitly injected into the right place by providing additional named instances. The support I put in was specifically for multi-tenant applications, but our stock configuration is just single-tenant.

You can run MvcMusicStore in debug mode with the "Ninject" compilation symbol in order to try out any changes you may do.

Also note the Ninject Nuget packages at https://www.myget.org/F/mvcsitemapprovider/ now contain functional code as well in case you want to see how it the code ends up in the final application. Just be sure to set the pre-release switch and to grab the highest versioned package.

Don't worry about DependencyResolver - I am seriously considering dropping support for it because it requires a bunch of extra garbage above and beyond what DI really needs. If someone really wants to fuss with it they are welcome to grab the Nuget packages that contain modules only.

[NightOwl888]

Never mind. I decided to go a different route. I created a couple of container agnostic conventions that I can use to ensure I don't inadvertently register stuff that isn't required. Not only does this make sure we will play nice with other DI modules, it also means we don't have to learn the drastically different APIs for each DI container's auto-registration methodology or their special quirks. Ninject even puts their conventions in a separate Nuget package which we now also don't need to include as a dependency.

The simplest solutions turn out to be the best ones.

So, in short I won't need your help to put this together because I am sure it is working. I am leaving your original issue #138 open, but being that this pull request is now out of date I am marking it closed. Feel free to submit another pull request when you get the chance to complete your implementation.