umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.42k stars 2.67k forks source link

V8: ModelsBuilder doesn't play nice with Nested Content -> need to mark element types #4030

Closed kjac closed 5 years ago

kjac commented 5 years ago

If ModelsBuilder runs in LiveAppData mode, Nested Content breaks with this exception (see full stack trace in the end of the issue description):

image

This happens when NestedContentValueConverterBase passes an IPublishedElement to PublishedModelFactory.CreateModel(IPublishedElement element). PublishedModelFactory has an IPublishedContent type registered for the element content type, and thus CreateModel() fails here.

Steps to reproduce

  1. Create some content with a Nested Content property with at least one item in it.
  2. Access the property in the content template (e.g. add @Model.Value("nestedContentPropertyAlias")).
  3. Set Umbraco.ModelsBuilder.ModelsMode to LiveAppData in <appSettings>.
  4. Delete the contents of \src\Umbraco.Web.UI\App_Data\Models.
  5. Use the ModelsBuilder dashboard to regenerate all models.
  6. Include all generated files from \src\Umbraco.Web.UI\App_Data\Models in the Umbraco.Web.UI project.
  7. Rebuild the project.
  8. Browse the content and observe the error.

When ModelsBuilder runs in its default mode (PureLive) it works just fine.

Note: In V7 the Nested Content property value converter uses its own logic to convert the items to the concrete implementations of IPublishedContent, which is why LiveAppData works in V7.

What to do?

I think @zpqrtbnk is the right person to ask that šŸ˜„

In my opinion it makes sense to let Nested Content return IPublishedElement items. Question is if PublishedModelFactory can be tweaked to handle this case, and how it impacts the generated models (currently they can only be constructed from IPublishedContent).

Full stack trace

[InvalidOperationException: Model Umbraco.Web.PublishedModels.NC1 expects argument of type Umbraco.Core.Models.PublishedContent.IPublishedContent, but got Umbraco.Web.PublishedCache.PublishedElement.]
   Umbraco.Core.Models.PublishedContent.PublishedModelFactory.CreateModel(IPublishedElement element) in C:\Projects\Umbraco-CMS-V8\src\Umbraco.Core\Models\PublishedContent\PublishedModelFactory.cs:95
   Umbraco.Web.PropertyEditors.ValueConverters.NestedContentValueConverterBase.ConvertToElement(JObject sourceObject, PropertyCacheLevel referenceCacheLevel, Boolean preview) in C:\Projects\Umbraco-CMS-V8\src\Umbraco.Web\PropertyEditors\ValueConverters\NestedContentValueConverterBase.cs:59
   Umbraco.Web.PropertyEditors.ValueConverters.NestedContentManyValueConverter.ConvertIntermediateToObject(IPublishedElement owner, PublishedPropertyType propertyType, PropertyCacheLevel referenceCacheLevel, Object inter, Boolean preview) in C:\Projects\Umbraco-CMS-V8\src\Umbraco.Web\PropertyEditors\ValueConverters\NestedContentManyValueConverter.cs:72
   Umbraco.Core.Models.PublishedContent.PublishedPropertyType.ConvertInterToObject(IPublishedElement owner, PropertyCacheLevel referenceCacheLevel, Object inter, Boolean preview) in C:\Projects\Umbraco-CMS-V8\src\Umbraco.Core\Models\PublishedContent\PublishedPropertyType.cs:254
   Umbraco.Web.PublishedCache.NuCache.Property.GetValue(String culture, String segment) in C:\Projects\Umbraco-CMS-V8\src\Umbraco.Web\PublishedCache\NuCache\Property.cs:222
   Umbraco.Web.PublishedContentExtensions.Value(IPublishedContent content, String alias, String culture, String segment, Fallback fallback, Object defaultValue) in C:\Projects\Umbraco-CMS-V8\src\Umbraco.Web\PublishedContentExtensions.cs:190
   ASP._Page_Views_nestedContent_cshtml.Execute() in C:\Projects\Umbraco-CMS-V8\src\Umbraco.Web.UI\Views\nestedContent.cshtml:11
   System.Web.WebPages.WebPageBase.ExecutePageHierarchy() +198
   System.Web.Mvc.WebViewPage.ExecutePageHierarchy() +105
   System.Web.WebPages.WebPageBase.ExecutePageHierarchy(WebPageContext pageContext, TextWriter writer, WebPageRenderingBase startPage) +78
   System.Web.Mvc.RazorView.RenderView(ViewContext viewContext, TextWriter writer, Object instance) +256
   System.Web.Mvc.BuildManagerCompiledView.Render(ViewContext viewContext, TextWriter writer) +107
   Umbraco.Core.Profiling.ProfilingView.Render(ViewContext viewContext, TextWriter writer) in C:\Projects\Umbraco-CMS-V8\src\Umbraco.Web\Mvc\ProfilingView.cs:27
   System.Web.Mvc.ViewResultBase.ExecuteResult(ControllerContext context) +290
   System.Web.Mvc.ControllerActionInvoker.InvokeActionResult(ControllerContext controllerContext, ActionResult actionResult) +13
   System.Web.Mvc.ControllerActionInvoker.InvokeActionResultFilterRecursive(IList`1 filters, Int32 filterIndex, ResultExecutingContext preContext, ControllerContext controllerContext, ActionResult actionResult) +56
   System.Web.Mvc.ControllerActionInvoker.InvokeActionResultFilterRecursive(IList`1 filters, Int32 filterIndex, ResultExecutingContext preContext, ControllerContext controllerContext, ActionResult actionResult) +420
   System.Web.Mvc.ControllerActionInvoker.InvokeActionResultFilterRecursive(IList`1 filters, Int32 filterIndex, ResultExecutingContext preContext, ControllerContext controllerContext, ActionResult actionResult) +420
   System.Web.Mvc.ControllerActionInvoker.InvokeActionResultFilterRecursive(IList`1 filters, Int32 filterIndex, ResultExecutingContext preContext, ControllerContext controllerContext, ActionResult actionResult) +420
   System.Web.Mvc.ControllerActionInvoker.InvokeActionResultWithFilters(ControllerContext controllerContext, IList`1 filters, ActionResult actionResult) +52
   System.Web.Mvc.Async.<>c__DisplayClass3_6.<BeginInvokeAction>b__3() +198
   System.Web.Mvc.Async.<>c__DisplayClass3_1.<BeginInvokeAction>b__5(IAsyncResult asyncResult) +100
   System.Web.Mvc.Async.WrappedAsyncResult`1.CallEndDelegate(IAsyncResult asyncResult) +10
   System.Web.Mvc.Async.WrappedAsyncResultBase`1.End() +49
   System.Web.Mvc.Async.AsyncControllerActionInvoker.EndInvokeAction(IAsyncResult asyncResult) +27
   System.Web.Mvc.<>c.<BeginExecuteCore>b__152_1(IAsyncResult asyncResult, ExecuteCoreState innerState) +11
   System.Web.Mvc.Async.WrappedAsyncVoid`1.CallEndDelegate(IAsyncResult asyncResult) +29
   System.Web.Mvc.Async.WrappedAsyncResultBase`1.End() +49
   System.Web.Mvc.Controller.EndExecuteCore(IAsyncResult asyncResult) +45
   System.Web.Mvc.<>c.<BeginExecute>b__151_2(IAsyncResult asyncResult, Controller controller) +13
   System.Web.Mvc.Async.WrappedAsyncVoid`1.CallEndDelegate(IAsyncResult asyncResult) +22
   System.Web.Mvc.Async.WrappedAsyncResultBase`1.End() +49
   System.Web.Mvc.Controller.EndExecute(IAsyncResult asyncResult) +26
   System.Web.Mvc.Controller.System.Web.Mvc.Async.IAsyncController.EndExecute(IAsyncResult asyncResult) +10
   System.Web.Mvc.<>c.<BeginProcessRequest>b__20_1(IAsyncResult asyncResult, ProcessRequestState innerState) +28
   System.Web.Mvc.Async.WrappedAsyncVoid`1.CallEndDelegate(IAsyncResult asyncResult) +29
   System.Web.Mvc.Async.WrappedAsyncResultBase`1.End() +49
   System.Web.Mvc.MvcHandler.EndProcessRequest(IAsyncResult asyncResult) +28
   System.Web.Mvc.MvcHandler.System.Web.IHttpAsyncHandler.EndProcessRequest(IAsyncResult result) +9
   System.Web.CallHandlerExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +576
   System.Web.HttpApplication.ExecuteStepImpl(IExecutionStep step) +132
   System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +163

requires #4038

kjac commented 5 years ago

One workaround could be to create an IPublishedContent from the IPublishedElement parameter of PublishedModelFactory.CreateModel(IPublishedElement element) - something along these lines:

public IPublishedElement CreateModel(IPublishedElement element)
{
    // fail fast
    if (_modelInfos == null)
        return element;

    if (!_modelInfos.TryGetValue(element.ContentType.Alias, out var modelInfo))
        return element;

    // ReSharper disable once UseMethodIsInstanceOfType
    if (modelInfo.ParameterType.IsAssignableFrom(element.GetType()) == false)
    {
        if (modelInfo.ParameterType == typeof(IPublishedContent))
        {
            element = new PublishedContentSomethingSomething(element);
        }
        else
        {
            throw new InvalidOperationException($"Model {modelInfo.ModelType} expects argument of type {modelInfo.ParameterType.FullName}, but got {element.GetType().FullName}.");
        }
    }

    // can cast, because we checked when creating the ctor
    return (IPublishedElement) modelInfo.Ctor(element);
}

Where PublishedContentSomethingSomething is a private implementation of IPublishedContent:

private class PublishedContentSomethingSomething : IPublishedContent
{
    private readonly IPublishedElement _element;

    public PublishedContentSomethingSomething(IPublishedElement element)
    {
        _element = element;
    }

    public PublishedContentType ContentType => _element.ContentType;
    public Guid Key => _element.Key;
    public IEnumerable<IPublishedProperty> Properties => _element.Properties;
    public IPublishedProperty GetProperty(string alias) => _element.GetProperty(alias);
    public int Id { get; }
    public string Name { get; }
    public string UrlSegment { get; }
    public int SortOrder { get; }
    public int Level { get; }
    public string Path { get; }
    public int TemplateId { get; }
    public int CreatorId { get; }
    public string CreatorName { get; }
    public DateTime CreateDate { get; }
    public int WriterId { get; }
    public string WriterName { get; }
    public DateTime UpdateDate { get; }
    public string Url { get; }
    public string GetUrl(string culture = null) => null;
    public PublishedCultureInfo GetCulture(string culture = null) => null;
    public IReadOnlyDictionary<string, PublishedCultureInfo> Cultures { get; }
    public PublishedItemType ItemType { get; }
    public bool IsDraft(string culture = null) => false;
    public IPublishedContent Parent { get; }
    public IEnumerable<IPublishedContent> Children { get; }
}

It works... but it really feels kinda ugly.

zpqrtbnk commented 5 years ago

going to look into this today - pretty sure it worked

zpqrtbnk commented 5 years ago

oh my - I know - hold on

kjac commented 5 years ago

Of course you know. If you didn't we'd be scr***d. šŸ˜†

zpqrtbnk commented 5 years ago

So... the whole problem comes from... this line in Models Builder source code.

When we generate a model for a content type, how shall we know whether to generate a published content model, or a published element model? Especially since the UI does not support "flagging" elements at the moment? So... Long time ago I implemented a rule that states that

Every content type with an alias ending with "element" is considered an element

And probably made a note "must remember to fix this" - but never fixed.

So,

kjac commented 5 years ago

@zpqrtbnk ah cool. I'll give it a spin soon as I'm able (this afternoon).

kjac commented 5 years ago

@zpqrtbnk it works. But with an unintended side effect: The generated classes lack their "Element" postfix.

Consider this doctype:

image

The alias is "textElement". But the generated class looks like this:

image

This of course made ModelsBuilder throw up over the "Text" property, because its original alias was "text" (same as the generated class). Changing the property alias to "bodyText" made it work again, though.

zpqrtbnk commented 5 years ago

Obviously ;-( Well this is temp, going to deal with it properly.

zpqrtbnk commented 5 years ago

Have pushed a commit to temp8 which updates ModelsBuilder with a version that supports the "is an element" flag from Core - so in the content type editor you can flag a content type as an element type, and then ModelsBuilder generates it as an element model (and does not strip the trailing "Element" from the alias, if it's there).

Moving the task to review.

How to review: create an element content type, generate models = it should be an element model, and Nested Content should be happy.

kjac commented 5 years ago

@zpqrtbnk oh cool. I will give it a spin.

kjac commented 5 years ago

Indeed it looks good and Nested Content is happy with the element models.

image