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

Block List settings exception if Models builder is disabled. #11712

Closed a-karandashov closed 2 years ago

a-karandashov commented 2 years ago

Which exact Umbraco version are you using? For example: 9.0.1 - don't just write v9

9.0.1, 9.1.1

Bug summary

If models builder is disabled and you're trying to get BlockListModel value from the property, it throws an error if "Settings" element is set: System.InvalidOperationException: Don't know how to map ModelType with content type alias "baseElementSettings".

Specifics

No response

Steps to reproduce

  1. Create element type and and Block List data type
  2. Add this element type to Block List as content and Add "Settings" element type.
  3. Switch off models builder in appSettings.
    "ModelsBuilder": {
        "ModelsMode": "Nothing"
      }
    1. Try to get a value from Block List.
    2. It throws an exception.

Expected result / actual result

No response

a-karandashov commented 2 years ago

I created a PR for this https://github.com/umbraco/Umbraco-CMS/pull/11725 Forgot to add it here.

a-karandashov commented 2 years ago

Hej @nul800sebastiaan I'm not sure if it's critical for Umbraco, but still can't disable models builder and use block list editor with settings block. At least if someone wants to use Anaximapper that @AndyButland built - they should see this error :) Let me know if it's possible to check my PR and if needed - I can finish it :)

nul800sebastiaan commented 2 years ago

@AndreyKarandashovUKAD PR https://github.com/umbraco/Umbraco-CMS/pull/11725 is now merged for 9.5.0, sorry we didn't get to it earlier!

nul800sebastiaan commented 2 years ago

Reopened since we reverted https://github.com/umbraco/Umbraco-CMS/pull/11725

umbrabot commented 2 years ago

Hi @AndreyKarandashovUKAD,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly Umbraco GitHub bot :-)

ronaldbarendse commented 2 years ago

As I mentioned in https://github.com/umbraco/Umbraco-CMS/pull/11725#issuecomment-1096473371, changing the settings type based on the ModelsBuilder mode isn't the correct fix, as that can cause other exceptions to be thrown.

A common workflow is to generate models during development using SourceCodeAuto/SourceCodeManual, publish your application (thereby compiling the generated models into your application assembly) and set the mode to Nothing to disable the generation of useless source code files on your production environment (because you can't rebuild your published application; you have to republish from source code again). In that case, your compiled application still contains the generated models and might expect a specific model type, but (with that PR applied) suddenly gets an IPublishedElement instead...

However, you still discovered a bug that is surfaced when your application doesn't have a model type (inherited from the abstract PublishedElementModel or PublishedContentModel class) for the specified Block List settings type. This will be the case if you always use ModelsBuilder mode Nothing, change it from InMemoryAuto to Nothing or if you don't rebuild your application after creating a new type (which seems to be the case in your reproduction steps).


This bug seems to be introduced in PR https://github.com/umbraco/Umbraco-CMS/pull/8854, because that added a call to IPublishedModelFactory.MapModelType() that throws the reported exception if it can't find the model type...

I've already created a PR that fixes this issue and adds some optimizations as well: https://github.com/umbraco/Umbraco-CMS/pull/12256.


The following work-arounds can be used in the meantime (written for v9, but can be adopted for v8).

If you don't want to use model types (e.g. always use ModelsMode Nothing), you can swap out the default PublishedModelFactory with the built-in NoopPublishedModelFactory:

using Microsoft.Extensions.DependencyInjection;
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Cms.Core.Models.PublishedContent;

public class NoopPublishedModelFactoryComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder) => builder.Services.AddSingleton<IPublishedModelFactory, NoopPublishedModelFactory>();
}

Or you can register a custom IPublishedModelFactory that includes a basic version of the fix that's done in the linked PR:

using System;
using System.Collections;
using Microsoft.Extensions.DependencyInjection;
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Cms.Core.Models.PublishedContent;
using Umbraco.Extensions;

public class FixedPublishedModelFactoryComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder) => builder.Services.AddSingleton<IPublishedModelFactory, FixedPublishedModelFactory>();

    private class FixedPublishedModelFactory : IPublishedModelFactory
    {
        private readonly PublishedModelFactory publishedModelFactory;

        public FixedPublishedModelFactory(IServiceProvider serviceProvider)
            => publishedModelFactory = serviceProvider.CreateDefaultPublishedModelFactory();

        public IPublishedElement CreateModel(IPublishedElement element) => publishedModelFactory.CreateModel(element);

        public IList CreateModelList(string alias) => publishedModelFactory.CreateModelList(alias);

        public Type MapModelType(Type type)
            => type is ModelType modelType
            ? publishedModelFactory.CreateModelList(modelType.ContentTypeAlias).GetType().GenericTypeArguments[0]
            : publishedModelFactory.MapModelType(type);
    }
}
nikolajlauridsen commented 2 years ago

I'll go ahead and close this, since it should be fixed in all versions 💪

V8: #12594 V9: #12256 V10: #12342