Exception Thrown When Umbraco Property is Named "Item"

[Nicholas-Westby]

I have a model that looks like this:

namespace Wiski.App.Mapping.Models.Pages
{
    using Our.Umbraco.Ditto;
    public class Typical
    {
        [UmbracoProperty]
        public string Item { get; set; }
    }
}

When I attempt to map a page using that model, I get an exception relating to reflection. The problem seems to be at this line: https://github.com/leekelleher/umbraco-ditto/blob/81e60f123df69a6c45bba2036a7ea9418bb26c7a/src/Our.Umbraco.Ditto/ComponentModel/Processors/UmbracoPropertyAttribute.cs#L123

It seems that code is attempting to invoke the indexer on PublishedContentBase: https://github.com/umbraco/Umbraco-CMS/blob/75c2b07ad3a093b5b65b6ebd45697687c062f62a/src/Umbraco.Web/Models/PublishedContentBase.cs#L178

That is because the indexer compiles to a method that is something like "get_Item", and when you use reflection to ask for a property named "Item", C# interprets "get_Item" as the getter method for a readonly method called "Item". Or so that's what seems to be happening.

My question is why the UmbracoPropertyAttribute would even be attempting to use reflection on IPublishedContent to get the value of an Umbraco property? My only guess is that this is being done for the built-in properties (e.g., creation date) that may not be accessible by calling IPublishedContent.GetPropertyValue.

If that is the case, I would recommend adding some extra checks in there for edge cases like this that refer to an existing property that has an unexpected signature that causes an exception to be thrown. The easiest option would be to wrap that line in a try/catch block, but maybe you could figure out something more clever.

[mattbrailsford]

It is there for built in properties yes, like you say, name, create date, etc. In theory it will also work with models builder properties too, though I haven't really tried that. The only thing I can think of would be to keep a physical black list, but not sure how good a solution that is.

[Nicholas-Westby]

Given the fact that you could potentially apply this to an instance of any type, I'd say a blacklist wouldn't be all that effective.

If there is a way with reflection to check the signature of a property (i.e., that there is an overload for a property available that does not contain any parameters), I'd say that's probably the most effective solution.

As an alternative, the try/catch I mentioned should suffice.

[Nicholas-Westby]

For this particular case, you could probably use GetIndexParameters and ensure that it returns 0 items: https://msdn.microsoft.com/en-us/library/system.reflection.propertyinfo.getindexparameters(v=vs.110).aspx

[JimBobSquarePants]

@Nicholas-Westby Yeah, that looks viable. We should probably turn that into an extension method along with the other restrictions in order to be consistent since we also loop through properties in PublishedContentExstensions.

[Nicholas-Westby]

Looks like somebody else ran into a similar issue: leekelleher/umbraco-ditto-labs#6

Though, they ran into the issue with a different property, Children. I wonder if this code should be looking for the existence of an Umbraco property first, then falling back to a C# property only if the Umbraco property does not exist.

There are also a number of other possibilities to consider (e.g., provide an option to skip C# properties, or provide an alternate processor that never looks at C# processors), but my first choice would be to check Umbraco properties before C# properties.

[JimBobSquarePants]

Just to note [DittoIgnore] exists to skip properties just now. Not sure how we'd no any checking otherwise.

[Nicholas-Westby]

@JimBobSquarePants FYI, I wasn't suggesting that mapping properties be skipped. I was suggesting that C# properties on IPublishedContent be skipped (i.e., not on the model being mapped). I can't annotate C# properties on IPublishedContent with DittoIgnore.

[Nicholas-Westby]

With regard to checking for Umbraco properties before checking for C# properties, you could use IPublishedContent.GetProperty. If it returns null, you know that the Umbraco property does not exist and so you can check the C# property with reflection (as you do already).

[mattbrailsford]

I'll need to check with Lee (although he is currently on holiday) but not sure what effect it will have swapping the order. The only thing that springs to mind is for uses of things like "Name" if you would want the property to be checked first, or if you would want to check the properties collection?

[mattbrailsford]

I just did a quick test swapping the order and using HasProperty and it breaks a whole heap of unit tests. It looks like something is going wrong in HasProperty causing a null value error. Will have to look into it fully when I have some more time free.

[Nicholas-Westby]

In the general case, I'd want the Umbraco properties collection to be checked first, then if no property exists (which, by the way, is not the same as whether or not the property has a value), I'd want the C# property to be checked. This would hold true for "Name" just as it would for any other property name.

Of course, you could run into situations where creating an Umbraco property could then essentially block access to mapping the C# property. If you'd want to support that situation, it might be worth having a parameter that explicitly indicates which source (i.e., C# property or Umbraco property) to inspect first. Alternatively, one might consider that this could be a case of a processor being too smart for its own good, and perhaps this could be split into two different processors.

[mattbrailsford]

I guess another reason to want to use properties first is if you were using it with Models Builder, you might want to use the explicit properties rather than the properties collection as those values are already passed through value resolvers and are cached, so you might want to try those first before doing another conversion. Just a thought off the top of my head.

[Nicholas-Westby]

@mattbrailsford I'm not familiar with how Models Builder works. Are you saying that you would use Ditto to map from a class generated by Models Builder to another class?

If so, yes, that would be a good use case for C# properties to be inspected before Umbraco properties.

[mattbrailsford]

@Nicholas-Westby that's correct yes. Models Buidler generates a class with declared, strongly typed properties on it. But you can consider this to be similar to a domain model. Ditto is a view models mapper and allows you to convert your domain model (be that the Models Builder model or an IPublishedContent) into one or many different view models. This is the fundamental difference between Models Builder and Ditto, and the reason why you might use Ditto even if you are using Models Builder, because it allows you to convert the Domain Models into more meaningful view models.

Now, all Models Builder models are still IPublishedContent, so in theory you could bypass the declared properties, and just use the standard IPublishedContent properties collection instead. But considering this would potentially re-run any Umbraco Value Converters (i'm not sure what the build in caching strategy on this is), it could add a small overhead, which is why I think it would be best to try and use strongly typed, already converted properties as your preferred option, then fall back to the properties collection if that fails. We obviously just need to make this more reliable than it currently is.

[Nicholas-Westby]

Makes sense. It seems like the solution for "Item" is pretty easy (i.e., can just call GetIndexParameters).

The more tricky one will be cases like "Children" where there is not necessarily a generic way to determine whether or not to skip the C# property. In that case, there will likely need to be some way to explicitly indicate where to get the data from.

[mattbrailsford]

I think Children can be left as meaning the Children property on IPublishedContent, we may just have document this fact. "Item" does indeed need addressing though.

[Nicholas-Westby]

@mattbrailsford Not sure what you mean by "Children can be left as meaning the Children property on IPublishedContent". Do you mean to say that it is the expected behavior that it will be impossible to get the value of an Umbraco (or Archetype) property named "Children" due to the fact that IPublishedContent.Children hides that during the mapping process?

Did you see this other issue in which somebody was attempting to map Children and was not getting the expected data? leekelleher/umbraco-ditto-labs#6

[mattbrailsford]

yes, similarly you couldn't have a property on your doctype with the alias "name" or "version". Call them "reserved aliases" if you like, but if it's a property name of IPublishedContent, then it's deemed to be reserved.

[mattbrailsford]

Best I could suggest here is maybe detect when in debug mode and if you are, look for any reserved aliases in a node, and log a warning saying "Property with alias 'x' is inaccessible as it is masked by the reserved property X of IPublishedContent" or something?

[Nicholas-Westby]

I suspect we can find a better solution than forcing people to avoid naming things with "reserved" aliases. This could force people into some uncomfortable situations (e.g., if they build out all their data first, then attempt to implement the mapping only to find that it doesn't work).

It seems to me this situation of reserved aliases is an unintended byproduct of a performance optimization. Supposing that's the case, I would expect there to be a way of correctly getting the data rather than deferring to the side effect of the performance optimization.

[mattbrailsford]

Open to suggestions :)

[mattbrailsford]

For the "item" issue, this is all a moot point, as that just needs fixing. For "Children" on the other than, this ultimately can be resolved 2 ways, so you either need to say, you can't use it, or give them an option to choose which (you'll probably need to allow them to choose at point of mapping, because you won't want it to be a global settings), it's just how we give them that option which a) isn't ugly and b) is discoverable.

One option could be to have an additional flag on the UmbracoPropertyAttribute

[UmbracoProperty(SkipPropertyCheck = true)]
public IEnumerable<MyObject> Children { get; setl }

or

[UmbracoProperty(PropertyCheckOrder = PropertyCheckOrder.OwnPropertiesFirst)]
public IEnumerable<MyObject> Children { get; setl }

This could work, but the likelihood is, you'll do it without initially, then get stuck and raise an issue, then we'll tell you about the extra property, and you'll go "ahh". In addition, it's ugly as hell.

Maybe we could do something like how dictionary items works and allow property alias prefixing to choose a specific method for retrieving a property

[UmbracoProperty("[Children]")]
public IEnumerable<MyObject> Children { get; setl }

But this is a bit undiscoverable, and a bit too magic.

Like I say, if anyone has any better suggestions, I'm open.

[Nicholas-Westby]

Yes, a solution which doesn't lead developers down a rabbit hole before discovering the solution would indeed be ideal.

Perhaps the default case could be to use the non-optimized implementation that "just works" with properties named "Children" (and so on).

Then, you could provide an optimized version that looks at C# properties first. That could be in a number of forms (e.g., an alternate processor, a processor parameter, a special syntax in the string for the alias, or a setting in the web.config or in a static variable). Here would be an example:

[UmbracoProperty(Optimized = true)]
public IEnumerable<MyObject> Children { get; set; }

That would do what is currently happening (i.e., fetch the child nodes). This version would get the value of the "Children" property:

[UmbracoProperty]
public IEnumerable<MyObject> Children { get; set; }

Of course, I realize that it's also not great to have to opt-in to the performance optimization. On the other hand, I'm not sure how necessary the performance optimization actually is and what real impact it will have on developers. There are also potentially many other opportunities for performance optimization (e.g., output caching, storing mapping models to a cache, and so on).

[mattbrailsford]

@JimBobSquarePants I've commited a fix for the "item" issue here 9893605. I don't know if you want to take a look to see if some method caching should be taking place or not?

[mattbrailsford]

I'm going to close this issue now though as the exact issue reported here has now been fixed. I'll raise another issue regarding the other property mappings.

[JimBobSquarePants]

Fix looks good to me for this issue.