-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Models Builder: Fix nested generic type handling in WriteClrType
#21429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Models Builder: Fix nested generic type handling in WriteClrType
#21429
Conversation
WriteClrType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in the ModelsBuilder's WriteClrType method that incorrectly handled nested generic types. The string-based overload was using naive comma splitting, which broke when parsing types like List<Tuple<string, string>> because it split on all commas regardless of bracket nesting depth.
Changes:
- Added
SplitGenericArgumentshelper method that properly tracks bracket depth when parsing generic type arguments - Fixed the string-based
WriteClrTypemethod to use the new helper instead of naive string splitting - Renamed existing test methods to clarify which overload they test (
WriteClrType_Type_*vsWriteClrType_String_*) - Added comprehensive unit tests for the string-based
WriteClrTypeoverload covering simple, nested, and deeply nested generics - Marked the parameterless
TextBuilderconstructor andWriteClrType(StringBuilder, Type)method as obsolete for V19 - Applied minor code modernizations (range operators, removed unused parameter)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Umbraco.Infrastructure/ModelsBuilder/Building/TextBuilder.cs | Fixed nested generic type parsing by adding SplitGenericArguments helper, marked constructors/methods as obsolete, applied code modernizations |
| tests/Umbraco.Tests.UnitTests/Umbraco.ModelsBuilder.Embedded/BuilderTests.cs | Renamed existing tests to clarify overloads, added comprehensive tests for string-based WriteClrType, added obsolete warnings |
kjac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested manually (successfully!) using a custom value converter with various property value types.
Here's an example:
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Models.PublishedContent;
using Umbraco.Cms.Core.PropertyEditors;
namespace Umbraco.Cms.Web.UI.Custom;
public class MyTextStringValueConverter : PropertyValueConverterBase
{
public override bool IsConverter(IPublishedPropertyType propertyType)
=> propertyType.EditorAlias == Constants.PropertyEditors.Aliases.TextBox;
public override Type GetPropertyValueType(IPublishedPropertyType propertyType)
=> typeof(IEnumerable<Tuple<string, IDictionary<string, List<decimal>>>>);
}...which generates:
public global::System.Collections.Generic.IEnumerable<global::System.Tuple<string, global::System.Collections.Generic.IDictionary<string, global::System.Collections.Generic.List<decimal>>>> Title => this.Value<global::System.Collections.Generic.IEnumerable<global::System.Tuple<string, global::System.Collections.Generic.IDictionary<string, global::System.Collections.Generic.List<decimal>>>>>(_publishedValueFallback, "title");
Description
This is an alternative attempt to resolve the issue that triggered the proposed changes in #21287, which, as per my comments in review, I don't think the approach taken there is correct as it's currently generating invalid C#.
So rather than changing the workings of the
TextBuilderclass that fundamentally, I've just implemented theTODOaround handling nested generic types, which is currently indicated as not supported.Change Summary
WriteClrTypemethod which was using naïve comma splitting to parse generic type arguments.List<Tuple<string, string>>because splitting on,produced invalid fragments.SplitGenericArgumentshelper method that tracks bracket depth to correctly split generic arguments.WriteClrTypeoverload (this includes additional tests for existing functionality, as well as initially failing tests for nested generic types that now pass).WriteClrType_Type_*vsWriteClrType_String_*)Test plan
Unit tests are provided to cover the change.
Manually I've regenerated models for an existing site and don't see any changes to the generated C# code (other than expected things relating to versions in comments).
I've also used the testing approach suggested in #21287 to register a custom property value converter for the text string property, that turns the value into a list of tuples. This is correct when looking at the generated C# and also works when rendering in the front-end.