Visibility missing from IMvcSiteMapNodeAttribute

[MacHack23]

It appears that the visibility option is missing from the IMvcSiteMapNodeAttribute. As a result I am unable to use the .NET Attributes to generate the sitemap of items.

[NightOwl888]

Actually, visibility is not missing - it is a custom attribute, and is supposed to be declared using the Attributes property.

However, thanks to your report I have discovered that this isn't working as expected. Apparently, the CLR limits the types that can be used so that IDictionary<string, object> is not allowed to be used on a .NET attribute type. Oddly, this doesn't show up as a compiler error until you attempt to use the Attributes property, which is why it went unnoticed until now.

As a workaround until we get this sorted out, it is possible to set the Attributes property per-request using the static SiteMaps object. You could therefore set these very early in the request lifecycle, such as in the Application_BeginRequest event of the Global.asax file and they will remain set for the duration of the request.

protected void Application_BeginRequest(Object sender, EventArgs e)
{
    var siteMap = MvcSiteMapProvider.SiteMaps.Current;

    siteMap.FindSiteMapNodeFromKey("Home").Attributes["visibility"] = "MenuHelper,!*";
    siteMap.FindSiteMapNodeFromKey("About").Attributes["visibility"] = "SiteMapPathHelper,!*";
}

You will have to give each attribute an explicit key (by setting the Key property on the .NET attribute), but doing so is typically required when using .NET attributes so they can be nested properly, so it shouldn't be a problem. You then use the explicit key in the FindSiteMapNodeFromKey() method to lookup the node and set the visibility attribute (as shown above).

Admittedly, this isn't pretty but for now this is the only way.

@maartenba

This SO post explains why this cannot be done. Dictionaries are not allowed to be used on .NET Attributes along with most other higher-order types. This means the following properties cannot currently be set on the MvcSiteMapNodeAttribute.

• Attributes
• LastModifiedDate

LastModifiedDate could be worked around by converting it to a string representation of a date, however, I would like your opinion on what to do about the Attributes dictionary.

So far I have come up with using XML or JSON through a string property as discussed in my SO question and I am leaning in the direction of JSON because it is much more concise syntactically.

Unfortunately, neither of these is self documenting, though - this will require some guidance to do. If there is a better alternative to this, I would certainly like to know about it.

[maartenba]

@NightOwl888 JSON would be a good option, however it is still fairly bloated to put that data in a string. For one or two attributes, I guess it would work but if I had 6 or more I'd become crazy.

XML sounds crazy, imagine 10 attributes :-)

Here are some other options I would consider:

• JSON
• Attributes can be specified a number of times, so perhaps a new attribute [MvcSiteMapAttribute("key here", "value here")] can be created and applied several times. Very readable, but also pretty heavy on the Reflection side
• A params array. The last parameter of the [MvcSiteMapNodeAttribute] could be a params array, so attributes can be specified by just adding tons of additional parameters. This may bite us in thelongrun though, as we can't really add additional parameters to the attribute without breaking someone's code.

Going over this list, I'd lean towards [MvcSiteMapAttribute] or doing it as JSON. Both come with the advantage of being easy to understand, and both have a slight performance issue (deserializing vs. reflection).

[NightOwl888]

Let's take a close look at these options. I don't think that it is realistic to need to support more than 5 custom parameters per node because customizing the visibility and HTML helpers will likely only need 1 or 2 parameters each.

Params Array

Actually, I don't see how this can be an option because an array is not the same as a dictionary. Besides, arrays (of primitive types, strings, and public enums) are already allowed as properties of the Attribute, so this really is only shifting the problem to the constructor without any real benefit.

I suppose that another option to consider is that since we could use an array of strings, we could just use a separator character within the string to separate the values. The disadvantage is that whatever the separator string is, it wouldn't be able to be used in either the key or value strings without confusing the code, not to mention, it has a similar problem as JSON or XML in that it isn't very clear how to use the property. The advantage is that this would be lighter weight to process than using JSON or XML serialization.

[MvcSiteMapNode(Title = "Home", Key="Home", Attributes = { "key1=value1", "key2=value2" })]

Another Custom .NET Attribute with Key/Value

This sounds OK at first, until you dig a little deeper. First of all, it means that declaring a [MvcSiteMapProvider] custom attribute will require adding another .NET attribute to the action method or controller class.

[MvcSiteMap(Name = "key1", Value = "value1")]
[MvcSiteMap(Name = "key2", Value = "value2")]
[MvcSiteMapNode(Title = "Home", Key = "Home")]

Unless I have missed something about how you intend this to work, this makes things less intuitive because the attributes are no longer declared on the node. The only way I can see this being intuitive would be to break pretty much every property down into a custom attribute instead of declaring them all on the same one.

Also, there is also the delicate matter of marrying this "extra data" with the MvcSiteMapNode that it belongs to. Keep in mind that MvcSiteMapNodeAttribute can be declared multiple times per action method or controller. We could infer this based on the action or controller that it is declare on when there is 1 SiteMapNodeAttribute, but when there are more than one we would need extra information to indicate what node the data belongs to. So to do this, we would need to also specify the key the custom attribute belongs to so the code that processes the attributes can add the [MvcSiteMapProvider] custom attributes.

[MvcSiteMap(Key = "Home", Name = "key1", Value = "value1")]
[MvcSiteMap(Key = "Home", Name = "key2", Value = "value2")]
[MvcSiteMapNode(Title = "Home", Key = "Home")]
[MvcSiteMap(Key = "About", Name = "key1", Value = "value1")]
[MvcSiteMap(Key = "About", Name = "key2", Value = "value2")]
[MvcSiteMapNode(Title = "About", Key = "About")]

Clearly, this is going to add some overhead to the reflection processing whether or not you actually declare custom attributes in the code.

Another Custom Attribute that Infers the Name from the Attribute

Another idea that came out of my SO question would be to use an interface so the reflection code knows which .NET attributes are MvcSiteMapProvider custom attributes. For example, we could make an interface to base all custom attributes on, like this:

public interface ISiteMapNodeCustomAttribute
{
    string Key { get; set; }
    string Value { get; set; }
}

Then each custom attribute could be named using c# rather than a property.

[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, AllowMultiple = false, Inherited = false)]
public class VisibilityAttribute : Attribute, ISiteMapNodeCustomAttribute
{
    public string Key { get; set; }
    public string Value { get; set; }
}

Then it would be a bit more readable to declare the attributes.

[Visibility("Home", "SiteMapPathHelper,!*")]
[SomeCustomAttribute(Key = "Home", Value = "value2")]
[MvcSiteMapNode(Title = "Home", Key = "Home")]
[Visibility("About", "SiteMapPathHelper,!*")]
[SomeCustomAttribute(Key = "About", Value = "value2")]
[MvcSiteMapNode(Title = "About", Key = "About")]

While this may look good in the declaration, it is still separating the attributes physically from the node declaration, in addition to the other disadvantages of doing it with a stock attribute.

In addition, this means that instead of simply declaring a user-defined attribute when defining MvcSiteMapNodeAttribute, a custom class must also be created for each custom attribute type. This not only puts a heavy tax on using custom attributes, it makes them significantly different to create than the rest of the MvcSiteMapProvider framework.

Also, one other disadvantage of inferring the name of the MvcSiteMapProvider custom attribute from the .NET attribute name like this is that we typically declare a .NET attribute using Pascal case (TheAttribute) and we typically declare an XML or JSON attribute using camel case (theAttribute), so we would either have to deviate from long established conventions or make the code that parses the name force the first letter to lower case.

Finally, just from this discussion you should see the other disadvantage of either of the above options - we are overloading the term "custom attribute", making it extremely difficult to have a conversation about it without continually misunderstanding what is being referred to.

JSON

For the sake of this discussion, let's just eliminate the XML option altogether, as we both agree it is too verbose to consider.

The JSON declaration would look something like this:

[MvcSiteMapNode(Title = "Home", Key="Home", Attributes = @"{ ""key1"": ""value1"", ""key2"": ""value2"" }")]

It is extremely clear which node these attributes belong to, and they are pretty easy to read. That said, they are not quite as easy to write without a document or error message to guide you. No reflection code must change to do this - the serialization can be handled within the processing of the node. In addition, this string can be easily validated using a regular expression. Finally, there is a definite performance benefit to using this approach because the deserialization will only need to happen if you are actually using the property. The deserialization is a single line of code.

string json = "{\"id\":\"13\", \"value\": true}";

var serializer = new JavaScriptSerializer(); //using System.Web.Script.Serialization;

Dictionary<string, string> values = serializer.Deserialize<Dictionary<string, string>>(json);

The only real disadvantage is that when faced with a string named Attributes, what should you put in it? Either the documentation will need to be consulted or the developer must guess and see what the error message says to do. That said, it is a disadvantage that carries considerable weight.

Eliminating allowing Custom Attributes from MvcSiteMapNodeAttribute

Another option we should perhaps consider is simply not allowing custom attributes to be used when using MvcSiteMapNodeAttribute. This avoids any of the headaches with any of the other options completely.

Do note that the best we would be able to do anyway is to make a dictionary of string values because the object datatype would be extremely difficult to support with JSON. This does mean if you want to pass custom CSS data into your HTML helpers in conjunction with MvcSiteMapNodeAttribute you would be out of luck.

Of course, this means we would need to make the Visibility into a property on MvcSiteMapNodeAttribute in order to fix the original issue. Perhaps we should even consider making it a property on the ISiteMapNode as well to make it into a first-class feature, rather than considering it a "custom" attribute.

[MvcSiteMapNode(Title = "Home", Key="Home", Visibility= "SiteMapPathHelper,!*")]

[maartenba]

Looks like that very last option actually may be the cleanest.

[NightOwl888]

I am still more inclined to choose JSON because it means not cutting the feature entirely. It also more closely resembles how it is configured using XML or code and more closely resembles how the LastModifiedDate will be made to work. I think that custom attributes are important for those who need to customize their HTML helper templates or who are using custom visibility providers.

That said, the fact this went unreported for 6 months after it went into production should indicate how much custom attributes are actually needed (almost never). However, eliminating MvcSiteMapProvider custom attributes just because Microsoft decided that .NET attributes are a second-class feature feels wrong.

Given the circumstances, JSON feels like the best tool for the job.

Making visibility into a first-class feature also seems like it would be in order, but perhaps we should wait until v5 to sort that out to avoid breaking existing custom visibility providers.

[remcoros]

,,That said, the fact this went unreported for 6 months after it went into production should indicate how much custom attributes are actually needed (almost never).''

I am just starting to use this project. And I already ran into the missing 'Visibilty' property on MvcSiteMapNodeAttribute :-)

I vote for at least adding 'Visibility' as a native property on MvcSiteMapNodeAttribute. It's very useful to exclude Account/Profile routes from the sitemap, while still using these nodes for the sitemap path helper.

[NightOwl888]

@remcoros

As indicated by the commit history above, this issue has already been addressed in the source (as a JSON string property). The documentation problem was addressed with intellisense. However, the fix was too big of a change to go into a patch, so it will need to be in the next minor release.

The release will also contain some required updates to the external DI configuration which will make it possible to put issues like this into production sooner in the future. It will be a breaking change for the external DI folks, but I think it is better to break it once than to continually have to choose between fixing bugs and breaking DI configurations.

I was hoping to release this yesterday, but the release was held up by the discovery of some node matching bugs. It should be released within a couple of days, though. Please be patient.