-
Notifications
You must be signed in to change notification settings - Fork 0
Code Review Bench PR #64636 - Blazor supports DisplayName for models
#4
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
base: base_pr_64636_20260125_3172
Are you sure you want to change the base?
Changes from all commits
d9e6138
98348b1
79aa210
f2a48e9
9c21366
1403178
d93ac50
c284b24
ba6aa50
b031f3b
ffa638d
047e7b6
e026ea0
cfea382
62ac049
ca4d4dd
6d720c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.ComponentModel; | ||
| using System.ComponentModel.DataAnnotations; | ||
| using System.Linq.Expressions; | ||
| using Microsoft.AspNetCore.Components.Rendering; | ||
|
|
||
| namespace Microsoft.AspNetCore.Components.Forms | ||
| { | ||
| /// <summary> | ||
| /// Displays the display name for a specified field, reading from <see cref="DisplayAttribute"/> | ||
| /// or <see cref="DisplayNameAttribute"/> if present, or falling back to the property name. | ||
| /// </summary> | ||
| /// <typeparam name="TValue">The type of the field.</typeparam> | ||
| public class DisplayName<TValue> : IComponent | ||
| { | ||
|
|
||
| private RenderHandle _renderHandle; | ||
| private Expression<Func<TValue>>? _previousFieldAccessor; | ||
| private string? _displayName; | ||
|
|
||
| /// <summary> | ||
| /// Specifies the field for which the display name should be shown. | ||
| /// </summary> | ||
| [Parameter, EditorRequired] | ||
| public Expression<Func<TValue>>? For { get; set; } | ||
| /// <inheritdoc /> | ||
| void IComponent.Attach(RenderHandle renderHandle) | ||
| { | ||
| _renderHandle = renderHandle; | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| Task IComponent.SetParametersAsync(ParameterView parameters) | ||
| { | ||
| parameters.SetParameterProperties(this); | ||
|
|
||
| if (For is null) | ||
| { | ||
| throw new InvalidOperationException($"{GetType()} requires a value for the " + | ||
| $"{nameof(For)} parameter."); | ||
| } | ||
|
|
||
| // Only recalculate if the expression changed | ||
| if (For != _previousFieldAccessor) | ||
| { | ||
| var newDisplayName = ExpressionMemberAccessor.GetDisplayName(For); | ||
|
|
||
| _displayName = newDisplayName; | ||
| _renderHandle.Render(BuildRenderTree); | ||
|
|
||
| _previousFieldAccessor = For; | ||
| } | ||
|
|
||
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| private void BuildRenderTree(RenderTreeBuilder builder) | ||
| { | ||
| builder.AddContent(0, _displayName); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,87 @@ | ||||||||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||
|
|
||||||||||||||
| using System.Collections.Concurrent; | ||||||||||||||
| using System.ComponentModel; | ||||||||||||||
| using System.ComponentModel.DataAnnotations; | ||||||||||||||
| using System.Linq.Expressions; | ||||||||||||||
| using System.Reflection; | ||||||||||||||
| using Microsoft.AspNetCore.Components.HotReload; | ||||||||||||||
|
|
||||||||||||||
| namespace Microsoft.AspNetCore.Components.Forms; | ||||||||||||||
|
|
||||||||||||||
| internal static class ExpressionMemberAccessor | ||||||||||||||
| { | ||||||||||||||
| private static readonly ConcurrentDictionary<Expression, MemberInfo> _memberInfoCache = new(); | ||||||||||||||
| private static readonly ConcurrentDictionary<MemberInfo, string> _displayNameCache = new(); | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||
|
|
||||||||||||||
| static ExpressionMemberAccessor() | ||||||||||||||
| { | ||||||||||||||
| if (HotReloadManager.Default.MetadataUpdateSupported) | ||||||||||||||
| { | ||||||||||||||
| HotReloadManager.Default.OnDeltaApplied += ClearCache; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private static MemberInfo GetMemberInfo<TValue>(Expression<Func<TValue>> accessor) | ||||||||||||||
| { | ||||||||||||||
| ArgumentNullException.ThrowIfNull(accessor); | ||||||||||||||
|
|
||||||||||||||
| return _memberInfoCache.GetOrAdd(accessor, static expr => | ||||||||||||||
| { | ||||||||||||||
| var lambdaExpression = (LambdaExpression)expr; | ||||||||||||||
| var accessorBody = lambdaExpression.Body; | ||||||||||||||
|
|
||||||||||||||
| if (accessorBody is UnaryExpression unaryExpression | ||||||||||||||
| && unaryExpression.NodeType == ExpressionType.Convert | ||||||||||||||
| && unaryExpression.Type == typeof(object)) | ||||||||||||||
| { | ||||||||||||||
| accessorBody = unaryExpression.Operand; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (accessorBody is not MemberExpression memberExpression) | ||||||||||||||
| { | ||||||||||||||
| throw new ArgumentException( | ||||||||||||||
| $"The provided expression contains a {accessorBody.GetType().Name} which is not supported. " + | ||||||||||||||
| $"Only simple member accessors (fields, properties) of an object are supported."); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return memberExpression.Member; | ||||||||||||||
| }); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static string GetDisplayName(MemberInfo member) | ||||||||||||||
| { | ||||||||||||||
| ArgumentNullException.ThrowIfNull(member); | ||||||||||||||
|
|
||||||||||||||
| var displayAttribute = member.GetCustomAttribute<DisplayAttribute>(); | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||
| if (displayAttribute is not null) | ||||||||||||||
| { | ||||||||||||||
| var name = displayAttribute.GetName(); | ||||||||||||||
| if (name is not null) | ||||||||||||||
| { | ||||||||||||||
| return name; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| var displayNameAttribute = member.GetCustomAttribute<DisplayNameAttribute>(); | ||||||||||||||
| if (displayNameAttribute?.DisplayName is not null) | ||||||||||||||
| { | ||||||||||||||
| return displayNameAttribute.DisplayName; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return member.Name; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static string GetDisplayName<TValue>(Expression<Func<TValue>> accessor) | ||||||||||||||
| { | ||||||||||||||
| ArgumentNullException.ThrowIfNull(accessor); | ||||||||||||||
| var member = GetMemberInfo(accessor); | ||||||||||||||
| return GetDisplayName(member); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private static void ClearCache() | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||
| private static void ClearCache() | |
| private static void ClearCache() | |
| { | |
| _memberInfoCache.Clear(); | |
| _displayNameCache.Clear(); | |
| } |
- Apply suggested fix
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,232 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.ComponentModel; | ||
| using System.ComponentModel.DataAnnotations; | ||
| using Microsoft.AspNetCore.Components.Rendering; | ||
| using Microsoft.AspNetCore.Components.Test.Helpers; | ||
|
|
||
| namespace Microsoft.AspNetCore.Components.Forms; | ||
|
|
||
| public class DisplayNameTest | ||
| { | ||
| [Fact] | ||
| public async Task ThrowsIfNoForParameterProvided() | ||
| { | ||
| // Arrange | ||
| var rootComponent = new TestHostComponent | ||
| { | ||
| InnerContent = builder => | ||
| { | ||
| builder.OpenComponent<DisplayName<string>>(0); | ||
| builder.CloseComponent(); | ||
| } | ||
| }; | ||
|
|
||
| var testRenderer = new TestRenderer(); | ||
| var componentId = testRenderer.AssignRootComponentId(rootComponent); | ||
|
|
||
| // Act & Assert | ||
| var ex = await Assert.ThrowsAsync<InvalidOperationException>( | ||
| async () => await testRenderer.RenderRootComponentAsync(componentId)); | ||
| Assert.Contains("For", ex.Message); | ||
| Assert.Contains("parameter", ex.Message); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task DisplaysPropertyNameWhenNoAttributePresent() | ||
| { | ||
| // Arrange | ||
| var model = new TestModel(); | ||
| var rootComponent = new TestHostComponent | ||
| { | ||
| InnerContent = builder => | ||
| { | ||
| builder.OpenComponent<DisplayName<string>>(0); | ||
| builder.AddComponentParameter(1, "For", (System.Linq.Expressions.Expression<Func<string>>)(() => model.PlainProperty)); | ||
| builder.CloseComponent(); | ||
| } | ||
| }; | ||
|
|
||
| // Act | ||
| var output = await RenderAndGetOutput(rootComponent); | ||
|
|
||
| // Assert | ||
| Assert.Equal("PlainProperty", output); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task DisplaysDisplayAttributeName() | ||
| { | ||
| // Arrange | ||
| var model = new TestModel(); | ||
| var rootComponent = new TestHostComponent | ||
| { | ||
| InnerContent = builder => | ||
| { | ||
| builder.OpenComponent<DisplayName<string>>(0); | ||
| builder.AddComponentParameter(1, "For", (System.Linq.Expressions.Expression<Func<string>>)(() => model.PropertyWithDisplayAttribute)); | ||
| builder.CloseComponent(); | ||
| } | ||
| }; | ||
|
|
||
| // Act | ||
| var output = await RenderAndGetOutput(rootComponent); | ||
|
|
||
| // Assert | ||
| Assert.Equal("Custom Display Name", output); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task DisplaysDisplayNameAttributeName() | ||
| { | ||
| // Arrange | ||
| var model = new TestModel(); | ||
| var rootComponent = new TestHostComponent | ||
| { | ||
| InnerContent = builder => | ||
| { | ||
| builder.OpenComponent<DisplayName<string>>(0); | ||
| builder.AddComponentParameter(1, "For", (System.Linq.Expressions.Expression<Func<string>>)(() => model.PropertyWithDisplayNameAttribute)); | ||
| builder.CloseComponent(); | ||
| } | ||
| }; | ||
|
|
||
| // Act | ||
| var output = await RenderAndGetOutput(rootComponent); | ||
|
|
||
| // Assert | ||
| Assert.Equal("Custom DisplayName", output); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task DisplayAttributeTakesPrecedenceOverDisplayNameAttribute() | ||
| { | ||
| // Arrange | ||
| var model = new TestModel(); | ||
| var rootComponent = new TestHostComponent | ||
| { | ||
| InnerContent = builder => | ||
| { | ||
| builder.OpenComponent<DisplayName<string>>(0); | ||
| builder.AddComponentParameter(1, "For", (System.Linq.Expressions.Expression<Func<string>>)(() => model.PropertyWithBothAttributes)); | ||
| builder.CloseComponent(); | ||
| } | ||
| }; | ||
|
|
||
| // Act | ||
| var output = await RenderAndGetOutput(rootComponent); | ||
|
|
||
| // Assert | ||
| // DisplayAttribute should take precedence per MVC conventions | ||
| Assert.Equal("Display Takes Precedence", output); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task WorksWithDifferentPropertyTypes() | ||
| { | ||
| // Arrange | ||
| var model = new TestModel(); | ||
| var intComponent = new TestHostComponent | ||
| { | ||
| InnerContent = builder => | ||
| { | ||
| builder.OpenComponent<DisplayName<int>>(0); | ||
| builder.AddComponentParameter(1, "For", (System.Linq.Expressions.Expression<Func<int>>)(() => model.IntProperty)); | ||
| builder.CloseComponent(); | ||
| } | ||
| }; | ||
| var dateComponent = new TestHostComponent | ||
| { | ||
| InnerContent = builder => | ||
| { | ||
| builder.OpenComponent<DisplayName<DateTime>>(0); | ||
| builder.AddComponentParameter(1, "For", (System.Linq.Expressions.Expression<Func<DateTime>>)(() => model.DateProperty)); | ||
| builder.CloseComponent(); | ||
| } | ||
| }; | ||
|
|
||
| // Act | ||
| var intOutput = await RenderAndGetOutput(intComponent); | ||
| var dateOutput = await RenderAndGetOutput(dateComponent); | ||
|
|
||
| // Assert | ||
| Assert.Equal("Integer Value", intOutput); | ||
| Assert.Equal("Date Value", dateOutput); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task SupportsLocalizationWithResourceType() | ||
| { | ||
| var model = new TestModel(); | ||
| var rootComponent = new TestHostComponent | ||
| { | ||
| InnerContent = builder => | ||
| { | ||
| builder.OpenComponent<DisplayName<string>>(0); | ||
| builder.AddComponentParameter(1, "For", (System.Linq.Expressions.Expression<Func<string>>)(() => model.PropertyWithResourceBasedDisplay)); | ||
| builder.CloseComponent(); | ||
| } | ||
| }; | ||
|
|
||
| var output = await RenderAndGetOutput(rootComponent); | ||
| Assert.Equal("Localized Display Name", output); | ||
| } | ||
|
|
||
| private static async Task<string> RenderAndGetOutput(TestHostComponent rootComponent) | ||
| { | ||
| var testRenderer = new TestRenderer(); | ||
| var componentId = testRenderer.AssignRootComponentId(rootComponent); | ||
| await testRenderer.RenderRootComponentAsync(componentId); | ||
|
|
||
| var batch = testRenderer.Batches.Single(); | ||
| var displayLabelComponentFrame = batch.ReferenceFrames | ||
| .First(f => f.FrameType == RenderTree.RenderTreeFrameType.Component && | ||
| f.Component is DisplayName<string> or DisplayName<int> or DisplayName<DateTime>); | ||
|
|
||
| // Find the text content frame within the component | ||
| var textFrame = batch.ReferenceFrames | ||
| .First(f => f.FrameType == RenderTree.RenderTreeFrameType.Text); | ||
|
|
||
| return textFrame.TextContent; | ||
| } | ||
|
|
||
| private class TestHostComponent : ComponentBase | ||
| { | ||
| public RenderFragment InnerContent { get; set; } | ||
|
|
||
| protected override void BuildRenderTree(RenderTreeBuilder builder) | ||
| { | ||
| InnerContent(builder); | ||
| } | ||
| } | ||
|
|
||
| private class TestModel | ||
| { | ||
| public string PlainProperty { get; set; } = string.Empty; | ||
|
|
||
| [Display(Name = "Custom Display Name")] | ||
| public string PropertyWithDisplayAttribute { get; set; } = string.Empty; | ||
|
|
||
| [DisplayName("Custom DisplayName")] | ||
| public string PropertyWithDisplayNameAttribute { get; set; } = string.Empty; | ||
|
|
||
| [Display(Name = "Display Takes Precedence")] | ||
| [DisplayName("This Should Not Be Used")] | ||
| public string PropertyWithBothAttributes { get; set; } = string.Empty; | ||
|
|
||
| [Display(Name = "Integer Value")] | ||
| public int IntProperty { get; set; } | ||
|
|
||
| [Display(Name = "Date Value")] | ||
| public DateTime DateProperty { get; set; } | ||
|
|
||
| [Display(Name = nameof(TestResources.LocalizedDisplayName), ResourceType = typeof(TestResources))] | ||
| public string PropertyWithResourceBasedDisplay { get; set; } = string.Empty; | ||
| } | ||
|
|
||
| public static class TestResources | ||
| { | ||
| public static string LocalizedDisplayName => "Localized Display Name"; | ||
| } | ||
| } |
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.
_memberInfoCachekeyed onExpressionuses reference equality_memberInfoCacheis aConcurrentDictionary<Expression, MemberInfo>. TheExpressiontype does not overrideEquals/GetHashCode, so the dictionary uses default reference equality. This means:Expression<Func<TValue>>instance (e.g.,() => model.Name), so the cache key will be a different reference each time.GetOrAddfactory runs on every call, performing the expression tree parsing every time.Compare with
FieldIdentifier, which caches by(Type ModelType, MemberInfo Member)— a value tuple that uses structural equality — not by theExpressionobject itself.Fix: Cache by
MemberInfodirectly, or restructure so that after parsing the expression once (which is cheap), you cache the expensive part (display name resolution) byMemberInfo. Since_displayNameCachealready usesMemberInfoas the key but is unused, the simplest fix is to remove_memberInfoCacheentirely and use_displayNameCacheproperly. The expression-to-member parsing is not expensive enough to warrant caching.Was this helpful? React with 👍 / 👎