Enable support for nested generic types in Models Builder

[skttl]

Prerequisites

• I have added steps to test this contribution in the description below

Description

This fixes an issue in ModelsBuilder, where it generates invalid C# if you have a value converter generating nested generic types. Eg. leekelleher/umbraco-contentment#514.

Instead of passing in a the type as a string, which then gets corrupted in the string replacements being made, it passes in the type, so the method that takes the type can generate the C# code correctly.

Previously, ModelsBuilder used a string-based parser for generics (WriteClrType(StringBuilder, string)), which would split on commas and flatten inner generic arguments, producing invalid C# like:

IEnumerable<Tuple<string>, string, string>

Changes made:

• All property type rendering now uses the Type-based WriteClrType(StringBuilder, Type) method, which correctly preserves nested generics and tuples.
• Removed the now unused internal method WriteClrType(StringBuilder, string), which was broken for nested generics and no longer referenced anywhere.
• Updated one of the tests, as the test output had changed.

How to test:

• Simply enough - use a property editor that returns a nested generic type :) As there is none in Umbraco at the moment, and the one in Contentment is WIP, I've cooked up a custom value converter for the textstring property, that turns the value into a list of tuples.

public class ModelsComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.PropertyValueConverters().Remove<TextStringValueConverter>().Append<DummyTextstringValueConverter>();
    }
}

public class DummyTextstringValueConverter : TextStringValueConverter
{
    public DummyTextstringValueConverter(HtmlLocalLinkParser linkParser, HtmlUrlParser urlParser) : base(linkParser, urlParser)
    {
    }

    public override bool IsConverter(IPublishedPropertyType propertyType) => propertyType.EditorAlias.InvariantEquals(Constants.PropertyEditors.Aliases.TextBox);

    public override Type GetPropertyValueType(IPublishedPropertyType propertyType) => typeof(IEnumerable<Tuple<string, string>>);

    public override object ConvertIntermediateToObject(
    IPublishedElement owner,
    IPublishedPropertyType propertyType,
    PropertyCacheLevel referenceCacheLevel,
    object? inter,
    bool preview)
    {
        return new List<Tuple<string, string>>
        {
            new Tuple<string, string>(inter?.ToString() ?? "", "1"),
            new Tuple<string, string>(inter?.ToString() ?? "", "2"),
            new Tuple<string, string>(inter?.ToString() ?? "", "3"),
        };
    }
}

• Generate models with that value converter in place and notice that the value type of text string properties is going to be:

public virtual global::System.Collections.Generic.IEnumerable<global::System.Tuple<string, string>> DummyText => 

Without this change it would have been

  public virtual global::System.Collections.Generic.IEnumerable<global::System.Tuple<string>, global:: System.String> DummyText

Note the syntax error after System.Tuple

I tried generating models on an existing site with this change, and it didn't change anything in the existing properties.

[github-actions]

Hi there @skttl, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[AndyButland]

This amend to the test doesn't look right to me @skttl. This string needs to be valid C#. Seems like this will change:

IEnumerable<global::Umbraco.Cms.Web.Common.PublishedModels.Foo>

to:

IEnumerable<global::{foo}>

Which won't compile.

This is test code of course, but the test is no longer valid (even if it passes). And it makes me concerned that you having to change this test is masking a regression bug.

[AndyButland]

@skttl (and @leekelleher, as I see you are discussing this on leekelleher/umbraco-contentment#514) - as I found some issues with this proposal in, I've tackled the underlying issue in a different way in #21429. Can you let me know what you think please? If it looks OK, we could proceed with this I feel.

[skttl]

My thinking was that it would probably be better if the tests were using the same TextBuilder method. But your fix looks fine to me too.

[kjac]

Closing this in favor of #21429 👍 thanks @skttl 💯