Hello,

When you get homePage.PickedContent the property value converter for that property will run, and therefore do cache.GetById(id) to find and return the picked content. The GetById(id) method will in turn initialize the returned IPublishedContent instance, and that may imply loading its parent, ie homePage. Bit convoluted, but that's how the XML cache works, for the time being.

Now obviously if homePage gets its own properties when being initialized, it will get homePage.PickedContent... and you're entering an endless loop.

The way I designed it, IPublishedContent models initialization should NOT do much, esp. not do anything with properties that will cause other IPublishedContent models to initialize. Otherwise there is a risk of endless loops.

We could try to find a solution to that issue by keeping a graph of models that have already been created, in order to break out of the loop when needed, but I haven't tried to implement that yet. Also, having a different cache (to replace the XML cache) might help. That's all work in progress.

Back to your problem: why is the homePage constructor trying to get the value of the property?

Deleted my comment as I hadn't realised you'd already tried a custom TypeConverter.

Hi Jeroen,

Stephan is absolutely correct about the endless loops scenario.

The current architecture of Ditto and the Model Factory does not sit well together, especially when a POCO model contains a circular reference. (e.g. in your case, the homepage model calls the container model, which internally calls the 'parent node' aka homepage model, and so forth)

As for potential solutions, there's only one thing that I can think of - that is to replace your ContentPicker property with a read-only one (e.g. get; private set;). I'll explain...

I assume that you have a property like this?

public SomeCustomModel ContentPicker { get; set; }

Now using Ditto, it would take the POCO property and try to map it with the node's value, (and running it through whatever PropertyValueConverters and/or TypeConverters).

That is exactly where the problem lies.

Instead, I suggest making the property to do this:

[UmbracoProperty("contentPicker")]
public int ContentPickerId { get; set; }

[DittoIgnore]
public SomeCustomModel ContentPicker
{
    get
    {
        return new UmbracoHelper().TypedContent(this.ContentPickerId).As<SomeCustomModel>();
    }
}

Please note that the inner logic may need some tidy-up - you may have a static helper elsewhere for UmbracoHelper?

The difference here is that you are instructing Ditto to set the node ID value in a separate property, then to ignore the main property (so it doesn't attempt to map the value) ... then finally when you call @Model.ContentPicker it will perform the mapping at that point - which I believe would avoid the circular reference.

I hope all this makes sense?

Good luck!

Cheers,
- Lee

Thanks Lee and Stephan for the extensive comments. This morning I also checked Stephans model builder and realized that my problem is indeed a combination of the Ditto framework and my use of the PropertyValueConverter. I realized IPublishedContentModelFactory better fits a late binding senario than an early bind senario (like what Ditto does) especially for these kind of properties. The only "third" solution where I can came up with was indeed the solution Lee already explained. It's that or not using the Ditto framework at all. But that's not an option for me because I really like the way the Ditto framework works. Thanks again.

-- @Lee keep up the good work of the Ditto framework! :-) ..

I'll be looking into the factory and see if there's any chance we can detect loops and not go into endless processing...

@Stephen thanks, that would be nice.. Keep me posted via this thread if you have found a solution.

The last couple of months I worked around the problem from time to time. But finally I decide to implement Castle Proxy to get all the advantages of late binding and all the advantages of POCO's (where you keep the objects as clean as possible without all the code in the getters).. see https://bitbucket.org/ismarvelous/muffin (currently in the reengineering branch) for a solution. This results in a solution where we do not depend on Ditto but can still use it for some senarios if we want to.James told me on Twitter that Ditto is going into "the proxy" direction as well.

Hi all, here's the thing,... as stephen says

IPublishedContent models initialization should NOT do much, esp. not do anything with properties that will cause other IPublishedContent models to initialize.

The other reason IPublishedContent or the Models you create from them shouldn't try to be more than it is is because you will end up with deep object graphs, someone is going to then cache these and suddenly you have memory problems. Even worse is now that you have it cached, how are you going to refresh this cache? Especially when it contains references of references of references, there's no way for you to accurately and efficiently refresh that cache without just killing the whole cache which in itself is expensive and inefficient.

Apparently ditto is already caching these objects which isn't good because now you have IPublishedContent objects cached and then these deep object graphs cached on top of that. The other problem with this is that IPublishedContent is not necessarily cached globally, the IPublishedContent created and returned is actual contextual based because another thread could be operating on a slightly different content cache if a request is being executed during the time that another thread is publishing and updating the cache. So if you are caching these custom models at a global level, you'll run into the same issues that we've already solved with IPublishedContent. The models produced from the model factory should just be created very efficiently, they should not be cached because the underlying IPublishedContent has already done that.

So some things to consider:

• Custom models should not store state - especially the state of another instance of IPublishedContent (which will most likely be duplicated)
• Any extra data that a property returns that might be large data should never end up in cache
• Any property that returns another instance of IPublishedContent (or a model created from one) should never store this value inside of it, it needs to be returned from a method. This method could be an internal method and used via a read-only property, but this should never be get/set, since that would mean it stores state.
• Custom models should not be cached

In my opinion, it is safest and clearest for the user of your model to retrieve an instance of IPublishedContent from a property from a method. This clearly tells the user of the API that if they will be referencing the result of this method that they need to store it in a variable instead of calling the method over and over again. This also explicitly means that the model is not storing state, this also means the model can more easily be serialized and that this value cannot be set.

You need to consider all of these cases, especially memory usage. If you are caching object graphs, you have memory leaks.

Keep it simple :)

Whow thanks for your clarification.. Am I right if I conclude that writing an PropertyValueConverter which returns an IPublishedContent is bad practice in your opinion? Or just do no caching on models created by the factory?

Just to clarify.

Ditto does not cache the any mapped POCO's or properties.

In a very early form Ditto used to cache the constructed model in the context cache to reduce the overload caused by using reflection to loop through the POCO's properties etc. This hasn't been the case now for several versions. We came to the same conclusions regarding caching as you did Shannon. (Great minds** ;) )

The problem as described above, was caused by Ditto's eager loading of properties.

Ditto now offers a solution to this by lazy loading properties on demand using a dynamically generated proxy which intercepts property calls. This essentially returns the property my means of a method internally called via an instance of Lazy<T>. This is safe, efficient, does not require caching, and works perfectly with the IPublishedContentModelFactory.

To utilise lazy loading add the virtual keyword to the property declaration. If you are familiar at all with Entity Framework you will recognise this pattern.

public virtual SomeCustomModel ContentPicker { get; set; }

If you are using Ditto I would strongly recommend using this approach if you are mapping any properties that refer to directly or could contain content referring to another instance of IPublishedContent.

I agree with Shannon on that caching of mapped POCO's should be avoided, rather any conversion should be efficient. Remember we are already mapping from a cached object graph. This should be fast enough.

Hopefully that helps readers.

James

** My mum always said "Fools rarely differ" but I'm more of a glass is half full kind of guy.

This is more or less the same approach we use in muffin. The proxy object returned by the contentfactory just does simple GetPropertyValue calls (which are lazy) on the properties. Which is in essence the same as what the Zbu.Modelsbuilder does, but without put it into the generated classes but only on one single point (the interceptor).

Since we use TypeConverters in muffin we don't use PropertyValueConverters any more, but that's a whole different discussion.

So this keeps my question valid; I realy like to know if Shannon advice is; creating a PropertyValueConverter which returns an IPublishedContent is bad practice or not?

IPublishedProperty is readonly/immutable, same goes for IPublishedContent, a PropertyValueConverter resolves it's value lazily when the IPublishedProperty.Value is reached. So, you can return whatever type of value you'd like from the PropertyValueConverter...

AFAIK, currently we cache the XML and we create IPublishedContent when required from that XML fragment. IMO, you should be able to return whatever you want from a PropertyValueConverter and this should not affect how the underlying cache works since it shouldn't be a part of it. The PropertyValueConverter is a factory and in a web context, the value returned from it should have a request based lifetime and should not exist in memory for any longer than that period.

I would need to check with @Stephen about how we are caching IPublishedProperty with "Le cache nouveau" because if we end up caching the result of the value converter and it's a large object graph then we'd have memory problems of our own. I'm going to assume Stephen has done some clever work (since he's a very clever guy!) to avoid this issue but we'll see what he says when he's back from holidays.

If someone wants to have a play with NuCache ("Le cache nouveau") I've got a small 1-1 multilingual example project which runs on it: https://github.com/jbreuer/1-1-multilingual-example/tree/NuCache

In this video I explain how I've upgraded the project to the Umbraco 7.4 NuCache branch: https://youtu.be/DWjbJiIUQdk?t=31m8s

One of the things which it does cache is the URL. In the 1-1 multilingual example project I'm using a UrlProvider which assumed that the UrlProvider is called each time .Url is done on an IPublishedContent. That's how it's been working for years now, but in NuCache it's cached. As a workaround I call the UmbracoContext.Current.UrlProvider.GetUrl(id) method directly.

I also created a project page for it: https://our.umbraco.org/projects/developer-tools/1-1-multilingual-example/

Jeroen

is working on a reply...