CommunityToolkit / CommunityToolkit/dotnet

Refactor ObservableValidator to be AOT Compatible

Open
#1,180 2 comments 6 reactions 0 assignees View on GitHub
feature request :mailbox_with_mail:
Dominant language
C#
Stars
3.8k
Forks
400
PR merge metrics
No merged PRs in 30d

Description

### Overview

I am primarily using the MVVM Toolkit for its AOT compatibility. I love using it, and it works great! However, the functionality falls apart when using the `ObservableValidator`. Even though you can manually annotate your ViewModels in a way that properties are preserved and then suppress the warnings, you know are now irrelevant, this process is tedious and error-prone. Therefore, I would like to improve the implementation of the ObservableValidator.

Basically, as far as I can see, the current implementation has trimming/dynamic code annotations for
- Constructing a new ObservableValidator (The ValidationContext when it's constructed)
- Validating a new property (The Validator.TryValidateProperty searches for attributes on a property that it has not seen before / the ValidationContext needs DisplayName / MemberName)
- The ValidateAll method tries to find all properties to validate (there's already a source generator that addresses this in theory)

The goal would be to have an `ObservableValidator` that no longer needs any of these annotations.

The base assumptions, I have:
1. Using a ValidationContext is generally safe if DisplayName and MemberName are set manually (to my knowledge, the Options generator relies on this as well)
2. Instead of relying on the internal `ValidationAttributeStore` used by the Validator, we can replace this functionality with a source-generated discovery
3. Instead of just providing the fast-path with a source generator, we only provide the source-generated version
4. The ObservableValidator source generation should be able to live in parallel to the existing ObservableProperty source generation, since the `ValidateProperty` methods are independent

### API breakdown

The API would be identical to the current version, except that every class inheriting the `ObservableValidator` would need to be `partial`.

Internally, I see multiple different options for the code emitted by the generator. Right now, my proposal would be virtual methods for `TryValidatePropertyCore` and `ValidateAllPropertiesCore` which are chained when inheritance comes into play.
Attributes could be discovered reflection and a helper `GetValidationAttributes` method.

### Usage example

```csharp
public partial class MyTestViewModel : ObservableValidator
{
private string? userName;
private string? password;

// Could also be a partial method/field annotated with [ObservableProperty] with the get/set pattern auto-generated by the other source generator
[Required(ErrorMessage = "Missing {0}.")]
[Display(Name = "User Name")]
[StringLength(20, MinimumLength = 3)]
public string? UserName
{
get => this.userName;
set => SetProperty(ref this.userName, value, validate: true);
}

[Required]
[StringLength(20, MinimumLength = 3)]
public string? Password
{
get => this.password;
set => SetProperty(ref this.password, value, validate: true);
}
}

public sealed partial class SecondTestViewModel : MyTestViewModel
{
private int? counter;
private double test;

public SecondTestViewModel() => ValidateAllProperties();

[Range(10, 20)]
public int? Counter
{
get => this.counter;
set => SetProperty(ref this.counter, value, validate: true);
}

[CustomValidation(typeof(SecondTestViewModel), nameof(Validate))]
public double Test
{
get => this.test;
set => SetProperty(ref this.test, value, validate: true);
}

public static ValidationResult? Validate(double value, ValidationContext context)
{
SecondTestViewModel instance = (SecondTestViewModel)context.ObjectInstance;
return value < instance.Counter ? ValidationResult.Success : new ValidationResult("Invalid value");
}
}

// Example generation
partial class MyTestViewModel
{
private static readonly DisplayAttribute __UserNameDisplayAttribute =
typeof(MyTestViewModel).GetProperty(nameof(UserName))!.GetCustomAttribute()!;
private static readonly IEnumerable ___UserNameValidationAttributes =
typeof(MyTestViewModel).GetProperty(nameof(UserName))!.GetValidationAttributes();
private static readonly IEnumerable ___PasswordValidationAttributes =
typeof(MyTestViewModel).GetProperty(nameof(Password))!.GetValidationAttributes();

///
protected override void ValidateAllPropertiesCore()
{
ValidateProperty(UserName, nameof(UserName));
ValidateProperty(Password, nameof(Password));
base.ValidateAllPropertiesCore();
}

///
protected override ValidationStatus TryValidatePropertyCore(object? value, string propertyName, ICollection errors)
{
return (propertyName) switch
{
nameof(UserName) => TryValidateValue(value, nameof(UserName), __UserNameDisplayAttribute.GetName() ?? "UserName", ___UserNameValidationAttributes, errors),
nameof(Password) => TryValidateValue(value, nameof(Password), "Password", ___PasswordValidationAttributes, errors),
_ => base.TryValidatePropertyCore(value, propertyName, errors)
};
}
}

partial class SecondTestViewModel
{
private static readonly IEnumerable __CounterValidationAttributes =
typeof(SecondTestViewModel).GetProperty(nameof(Counter))!.GetValidationAttributes();
private static readonly IEnumerable __TestValidationAttributes =
typeof(SecondTestViewModel).GetProperty(nameof(Test))!.GetValidationAttributes();

///
protected override void ValidateAllPropertiesCore()
{
ValidateProperty(Counter, nameof(Counter));
ValidateProperty(Test, nameof(Test));
base.ValidateAllPropertiesCore();
}

///
protected override ValidationStatus TryValidatePropertyCore(object? value, string propertyName, ICollection errors)
{
return (propertyName) switch
{
nameof(Counter) => TryValidateValue(value, nameof(Counter), "Counter", __CounterValidationAttributes, errors),
nameof(Test) => TryValidateValue(value, nameof(Test), "Test", __TestValidationAttributes, errors),
_ => base.TryValidatePropertyCore(value, propertyName, errors)
};
}
}
```

### Breaking change?

I'm not sure

### Alternatives

Alternatives I have thought of (there are probably a lot more):
- Emitting code per class into a helper class (similar to the ValidateAllProperties generator right now) and use reflection to get the type info the first time we are constructing the `ObservableValidator`
- Emitting validation attributes directly instead of using reflection to discover them (the discovery should be saved, but it's still reflection)

Additionally, I have considered just dumping the MVVM toolkit for this and writing a separate package for this. However, if there's a reasonable path to get this into the MVVM toolkit, I would prefer that since its much more difficult to get a decent integration with the ObservableProperty generator when using a separate package.

### Additional context

_No response_

### Help us help you

Yes, I'd like to be assigned to work on this item

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.