Fail IPublishedContentFactory faster

src/Umbraco.Core/Models/PublishedContent/PublishedContentExtended.cs

@@ -43,6 +43,11 @@ namespace Umbraco.Core.Models.PublishedContent
 
         internal static IPublishedContentExtended Extend(IPublishedContent content, IEnumerable<IPublishedContent> contentSet)
         {
+            // first unwrap content down to the lowest possible level, ie either the deepest inner
+            // IPublishedContent or the first extended that has added properties. this is to avoid
+            // nesting extended objects as much as possible, so we try to re-extend that lowest
+            // object.
+
             var wrapped = content as PublishedContentExtended;
             while (wrapped != null && ((IPublishedContentExtended)wrapped).HasAddedProperties == false)
                 wrapped = (content = wrapped.Unwrap()) as PublishedContentExtended;
@@ -51,14 +56,38 @@ namespace Umbraco.Core.Models.PublishedContent
             // a model, and then that model has to inherit from PublishedContentExtended,
             // => implements the internal IPublishedContentExtended.
 
+            // here we assume that either the factory just created a model that implements
+            // IPublishedContentExtended and therefore does not need to be extended again,
+            // because it can carry the extra property - or that it did *not* create a
+            // model and therefore returned the original content unchanged.
+
             var model = content.CreateModel();
             var extended = model == content // == means the factory did not create a model
               ? new PublishedContentExtended(content) // so we have to extend
               : model; // else we can use what the factory returned
 
+            // so extended should always implement IPublishedContentExtended, however if
+            // by mistake the factory returned a different object that does not implement
+            // IPublishedContentExtended (which would be an error), throw.
+            //
+            // see also PublishedContentExtensionsForModels.CreateModel
+
+            // NOTE
+            // could we lift that constraint and accept that models just be IPublishedContent?
+            // would then mean that we cannot assume a model is IPublishedContentExtended, so
+            // either it is, or we need to wrap it. so instead of having
+            //   (Model:IPublishedContentExtended (IPublishedContent))
+            // we'd have
+            //   (PublishedContentExtended (Model (IPublishedContent)))
+            // and it is that bad? any other consequences?
+            //
+            // would also allow the factory to cache the model (though that should really
+            // be done by the content cache, not by the factory).
+
             var extended2 = extended as IPublishedContentExtended;
-            if (extended2 != null) // always true, but keeps Resharper happy
-                extended2.SetContentSet(contentSet);
+            if (extended2 == null)
+                throw new Exception("Extended does not implement IPublishedContentExtended.");
+            extended2.SetContentSet(contentSet);
             return extended2;
         }
 

src/Umbraco.Core/Models/PublishedContent/PublishedContentExtensionsForModels.cs

@@ -1,4 +1,6 @@
-namespace Umbraco.Core.Models.PublishedContent
+using System;
+
+namespace Umbraco.Core.Models.PublishedContent
 {
     /// <summary>
     /// Provides strongly typed published content models services.
@@ -12,9 +14,32 @@
         /// <returns>The strongly typed published content model.</returns>
         public static IPublishedContent CreateModel(this IPublishedContent content)
         {
-            return PublishedContentModelFactoryResolver.Current.HasValue
-                ? PublishedContentModelFactoryResolver.Current.Factory.CreateModel(content)
-                : content;
+            if (content == null)
+                return null;
+
+            if (PublishedContentModelFactoryResolver.Current.HasValue == false)
+                return content;
+
+            // get model
+            // if factory returns nothing, throw
+            // if factory just returns what it got, return
+            var model = PublishedContentModelFactoryResolver.Current.Factory.CreateModel(content);
+            if (model == null)
+                throw new Exception("IPublishedContentFactory returned null.");
+            if (ReferenceEquals(model, content))
+                return content;
+
+            // at the moment, other parts of our code assume that all models will
+            // somehow implement IPublishedContentExtended and not just be IPublishedContent,
+            // so we'd better check this here to fail as soon as we can.
+            //
+            // see also PublishedContentExtended.Extend
+
+            var extended = model as IPublishedContentExtended;
+            if (extended == null)
+                throw new Exception("IPublishedContentFactory created an object that does not implement IPublishedContentModelExtended.");
+
+            return model;
         }
     }
 }