Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.79%. Comparing base (bf2e9ef) to head (14c6a57).
Report is 3 commits behind head on v6.

@@            Coverage Diff             @@
##               v6    #9180      +/-   ##
==========================================
- Coverage   88.82%   88.79%   -0.03%     
==========================================
  Files         416      394      -22     
  Lines       12356    12302      -54     
  Branches     2458     2449       -9     
==========================================
- Hits        10975    10924      -51     
+ Misses        850      847       -3     
  Partials      531      531              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sorry, I did not mean to push the second set of changes with commit message "Add handling for validation expressions that refer to non-nullable properties" onto this PR, I've reverted that commit.

@henon should this include a couple of test cases?

Yes please add a test that fails without the fix and works with it.

I've found one more spot where this issue arises (ExpressionExtensions.GetLabelString), added a fix as well as unit tests for all changes that fail without them and suceed with them.

Let me expand a little bit more on the reason for this PR now that I have a better understanding of the code. Some components, such as MudDatePicker, only allow binding to a DateTime? field. However, if the corressponding database field is marked as NOT NULL, tools such as EF Core Power Tools generate a non-nullable backing field public DateTime DeliveryDay { get; set; }. To work around this, it is possible to create a component that includes a MudDatePicker and uses an intermediate field to enable the required functionality. For example, "ZenDatePicker.cs":

<MudDatePicker @bind-Date="InternalDate" For="@For" Required="true" />

@code {
	DateTime? InternalDate
	{
		get => new DateTime?(DateRequired);
		set
		{
			if (!EqualityComparer<DateTime>.Default.Equals(value ?? default, DateRequired))
			{
				DateRequired = value ?? DefaultValue ?? default;
				DateRequiredChanged.InvokeAsync(DateRequired);
			}
		}
	}

	[Parameter]
	public DateTime DateRequired { get; set; }

	[Parameter] public EventCallback<DateTime> DateRequiredChanged { get; set; }

	[Parameter]
	public DateTime? DefaultValue { get; set; } = DateTime.Now;

	[Parameter]
	public Expression<Func<DateTime?>>? For { get; set; }
}

This component can then be bound to the non-nullable backing field:
<ZenDatePicker @bind-DateRequired="Entity.DeliveryDay" For="@(() => Entity.DeliveryDay)" />

Note how the type for the For property has to be DateTime? because MudFormComponent<T, U> uses the same type parameter T both for the For property as well as the value property:

public Expression<Func<T>>? For { get; set; }
protected T? _value;

and MudDatePicker inherits from MudPicker with T=DateTime?: MudBaseDatePicker : MudPicker<DateTime?>

So basically another solution to this problem could be introducing a separate type parameter into MudFormComponent to enable inheriting classes to set differing types for the value and the for fields: MudFormComponent<TValue, TFor, U> and then also adjusting components that inherit from MudFormComponent. This could even be useful in situations where, say, one wants to make use of validators defined on a model for one non-nullable field, but bind the control to a different (nullable) field. But this would be a lot more work so it's easiest to just make sure that the framework can deal with situations where For is a Expression<Func<DateTime?>> that refers to a non-nullable field.

But this would be a lot more work so it's easiest to just make sure that the framework can deal with situations where For is a Expression<Func<DateTime?>> that refers to a non-nullable field.

Yes I agree. There is always the option of composition (wrapping) vs inheriting which should also solve the problem.

Just noticed this is a PR against v6. That makes it even more reasonable not to do any complicated PRs.

To get the code quality fixed please execute dotnet format on the VS developer console.

To get the code quality fixed please execute dotnet format on the VS developer console.

I think It would be better to do as separate PR and update this one, otherwise too much unrelated changes will be in the PR. I will do that.

Just noticed this is a PR against v6

Yes, I think we should leave it for v6 only for now. Since we plan to rework the validation anyway, we also have this #9016

I will do that.

One PR down #9216
But there is also need to port this change #8518

There is always the option of composition (wrapping) vs inheriting which should also solve the problem.

Wrapping is better in case of Blazor, otherwise you will hit this problem #9052 (comment)

There is always the option of composition (wrapping) vs inheriting which should also solve the problem.

Wrapping is better in case of Blazor, otherwise you will hit this problem #9052 (comment)

Not sure where this thread came from, I am using wrapping not inheritance as you can see the code in the "Describe alternatives you've considered" section of #8931.

The CreateAccessor local func comes from the main branch of the aspnetcore repo (defined inside GetModelFromMemberAccess) here: https://github.com/dotnet/aspnetcore/blob/main/src/Components/Forms/src/FieldIdentifier.cs

I should have noted this in the comments.

The CreateAccessor local func comes from the main branch of the aspnetcore repo (defined inside GetModelFromMemberAccess) here: https://github.com/dotnet/aspnetcore/blob/main/src/Components/Forms/src/FieldIdentifier.cs

The question wasn't really about its origin, but rather whether my suggested alternative would work, as it's "safer". However, I'm not going to insist on it since the risk of the application code being trimmed is low.

@henon, this is ready for merge.

Thanks everyone

Made v6.21.0 release with this change.