U4-5271 - PublishedContentExtended.Extend logic

Created by Lee Kelleher 23 Jul 2014, 16:48:30 Updated by Stephan 31 Aug 2015, 15:40:30

Relates to: U4-7038

Using the new IPublishedContentModelFactory (either the default or custom one) to return content models, I got null-reference exception when using Umbraco.TypedSearch:

It was at Umbraco.Core.Models.PublishedContent.PublishedContentSet.MapContent (line 62). This turned out to be a nested exception, as the real error was throwing an exception.

The value of extend was null.

Fixing the exception message and re-running my code, I got the real exception:

Failed extend a published content of type Umbraco.Core.Models.PublishedContent.PublishedContentExtended. Got when expecting Umbraco.Core.Models.IPublishedContent.

(Note: the extend variable was still null)

Digging around, I found the null reference was being set within PublishedContentExtended.Extend.

For whatever reason, the result from Umbraco.TypedSearch was being processed by MapContent(T t) twice. First to map the returned content models (raw IPublishedContent) ... the second was processing them again (with the already wrapped/extended PublishedContentExtended).

So when the call to .CreateModel() is made, if the original IPublishedContent is returned, (e.g. there was no corresponding model to map), it will fail to match the PublishedContentExtended passed in and can not be cast as IPublishedContentExtended (as var extended2 on line 59).

My workaround is to change line 55 (in PublishedContentExtended.cs) from:

var extended = model == content

to: var extended = !(model is IPublishedContentExtended)


My gut feeling is that there is still an underlying issue with MapContent(T t) being called twice... but I'm not sure how to resolve that (yet).

Hope this all makes sense? I'm happy to provide a pull-request for my patch.

Comments

Stephan 23 Jul 2014, 17:08:29

Ouch. Looking into it.


Stephan 23 Jul 2014, 17:15:16

Quick question... do your models inherit from PublishedContentModel?


Lee Kelleher 23 Jul 2014, 17:27:55

Yes, the models inherit from PublishedContentModel.

But I have some DocTypes that don't have a corresponding model; (where would expect a regular IPublishedContent returned).


Stephan 23 Jul 2014, 17:30:21

OK. It ''is'' quite possible that the factory does not create a model for a doc type, and returns the original IPublishedContent unchanged -- this is why we have the ''model==content'' test in Extend. Trying to repro and understand what's going on. Later tonight though.


Lee Kelleher 23 Jul 2014, 17:36:11

The strange part is that when I get the exception, the value of content is a PublishedContentExtended type, but the factory returns it as a "raw" IPublishedContent object.

I'd have thought it would remain as PublishedContentExtended - unless it gets downcasted some where?


Stephan 23 Jul 2014, 21:04:45

Afraid there's something wrong. Looking into it.


Stephan 23 Jul 2014, 21:31:05

Trying to repro. Is it possible to post the code, from TypedSearch to exception? I'm trying: @foreach (var item in Umbraco.TypedSearch("foo")) {

@item.Name
} {code} And it seems to work fine.


Stephan 23 Jul 2014, 21:40:15

You write "''So when the call to .CreateModel() is made, if the original IPublishedContent is returned, (e.g. there was no corresponding model to map), it will fail to match the PublishedContentExtended passed in and can not be cast as IPublishedContentExtended (as var extended2 on line 59).''"

I'm wondering... if the original IPublishedContent is returned (no model) then we detect it (model == content) and we ''create'' a PublishedContentExtended. And it should then be possible to cast. Could it be that your return the "original" IPublishedContent, as a different object of some sort?

In any case I'll modify the end of the method to check that extended2 != null, else throw a meaningful exception before returning.

But the only reason I can see for the error (though it's late & I'm tired ;-) would be that model != content and ''yet'' model is not a model but a raw IPublishedContent.

?


Lee Kelleher 24 Jul 2014, 09:33:19

@zpqrtbnk I've figured it out... it's a possible bug in my custom model-factory. I'll gather some details and post back shortly.


Stephan 24 Jul 2014, 09:39:03

Feeling relieved ;-) That being said, it raised interesting points.

First, we're not checking that the factory & models behave as we expect them to, so we end up having strange null refs in strange places. Currently working on this for 7.1.5 so we fail faster and with a meaningful error message.

Would be interested if you could point out what your mistake was, and what we can put in place so it's better detected.

Second, at the moment we expect models to inherit from PublishedContentModel mainly because we want them to implement IPublishedContentExtended. Turns out... this is because it allows us to optimize ''some'' cases (eg no need to extend a model because it's already extended). Currently reviewing whether it would make sense to relax that constraint and just ask models to implement IPublishedContent.

There would be a very small penalty in some cases (extending a model that does not implement IPublishedContentExtended would create an extra object). But it would make things easier to understand for all.

Thanks for testing all this!


Lee Kelleher 24 Jul 2014, 09:59:05

I think there are multiple issues around this. Let's take it one by one.

The issue with my custom model-factory is that since I'm using Reflection to populate the model values (via Ditto), I noticed it was being called several times - e.g. when traversing the content tree. So I modified it to use the RequestCache to store the model instance, (using the content's Path as the key).

See my code here: https://github.com/leekelleher/umbraco-ditto/blob/6ed7f782f6f8eab6a639c4805b8a1802894a9f71/src/Our.Umbraco.Ditto/ModelFactory/DittoPublishedContentModelFactory.cs#L41

This approach worked fine for most of my pages. As if the content had a corresponding model, it would be returned. Otherwise the original IPublishedContent would be returned.


With the call to Umbraco.TypedSearch, the model-factory seems to be called twice. Once with the original IPublishedContent, the second with the same nodes as PublishedContentExtended.

At this point, the RequestCache in my custom model-factory has the original IPublishedContent stored against the node's Path (cache-key) ... so when the PublishedContentExtended version is passed in - the node Path is looked up, returning the original IPublishedContent.

Which then fails the (model == content) condition.


I can definitely improve my custom model-factory to handle this scenario better... and/or should the Core handle it differently?


Stephan 24 Jul 2014, 10:15:34

Several points (and I'm putting details here so I don't forget):

  • The factory is called twice. First time is when the content is retrieved from the cache, we need to create a model. Second is when it's put into a ContentSet and extended, we create a model again.

  • Why is the output of a TypedSearch a ContentSet? Because it contains extended content. Why extended content? Because it's the "default" IPublishedContent + an extra "examineScore" property.

  • What's extending: it's creating a wrapper around an IPublishedContent. But then, because you want to see your models, we need to create a model for that wrapper.

So the cache returns (Model (IPublishedContent)) and the extend method unwraps it to (IPublishedContent) then tries to create a ''new'' model so we have (Model2 (IPublishedContent)) and because a model is an extended, we can add the extra property to it.

Because of that logic, ie using the models as extended, the factory should never ever cache its models and create new models each time it is asked for one. Because we might extend a content several times (in fact, each time it would be involved in a ToContentSet() call).

So the conclusion for you would be

  • yes it's running twice
  • don't cache models

Currently looking into whether we could relax some of these constraints. They might be "premature optimization" gone wrong.


Lee Kelleher 24 Jul 2014, 10:26:46

Thanks @zpqrtbnk! re: factory caching - noted!

Difficult part (for Ditto) is with the Reflection being expensive per model creation... hence why I did the caching, but that's my (Ditto's) problem to solve :-)


Stephan 24 Jul 2014, 10:43:56

Makes sense. Don't have a magic workaround for 7.1.4 - but for 7.1.5 what I'm thinking is that we do have complete control over what TypedSearch does, so there ''should'' be a way to make sure we don't double-map in there.

So I'm currently looking into two things:

  • prevent double-mapping in TypedSearch (and Search I guess)
  • relax the constraints on models so they just need to inherit from IPublishedContent

That being said: the way ContentSet works, is by wrapping the original content into a... wrapper, which is a PublishedContentExtended, so we can indicate that content that it has a different "set" from the original one. And then we need to create a model on top of it.

So anytime a user does... foo.Children().Where(...).ToContentSet() we ''have'' to create models for a ''different'' content object.

So if I can remove the double-mapping of TypedSearch then it would still leave you with a performance penalty for ToContentSet() that I'm not sure I know how to fix. That being said... you say you have a reflection perf. penalty... are you using raw reflection to set the fields? You could generate dynamic methods or compiled expressions (see what we do in PublishedContentModelFactory.ctor) and then it's much faster.


Stephan 24 Jul 2014, 10:47:28

Note to self: preventing double-mapping could be done with the current XML cache because cache.GetById() creates a new model each time it's called. But we have work in progress that would allow the cache to also cache the models, potentially across requests, and everything that comes from that cache would ''have'' to be extended. So... no.


Lee Kelleher 24 Jul 2014, 10:56:26

re: Reflection, we iterating over type.GetProperties(), then using .SetValue() on the propertyInfo object. I know this can be improved - definitely a work-in-progress. I'll take a look the PublishedContentModelFactory constructor.

Thanks again @zpqrtbnk! #h5yr


Stephan 25 Jul 2014, 09:12:24

Have pushed commit c2816e759b4fcdffa96f314faf920cbcb0d0dded into 7.1.5 - does not change how things work, but will detect issues with models & factory faster, and will fail with more explicit error messages.

@leekelleher Is it OK to close this issue?


Lee Kelleher 25 Jul 2014, 09:16:29

@zpqrtbnk - yes, good to close this ticket. The exceptions help identify the root cause. Thanks again!


Priority: Normal

Type: Bug

State: Fixed

Assignee: Stephan

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version:

Sprint:

Story Points:

Cycle: