Skip to content
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

[Feature] Manually adding errors to ObservableValidator #3750

Closed
sherman89 opened this issue Feb 10, 2021 · 4 comments
Closed

[Feature] Manually adding errors to ObservableValidator #3750

sherman89 opened this issue Feb 10, 2021 · 4 comments
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the Microsoft.Toolkit.Mvvm package
Milestone

Comments

@sherman89
Copy link

Thank you for this library first of all, really enjoying it so far! ❤️

Is it possible to add AddError and ClearErrors methods (or something similar) to ObservableValidator for custom validation? In my case it's not possible to use validation attributes since my validation involves calling an injected service that does complicated validation logic.

Forgive me if there's already a way to do this, but I just couldn't figure it out 😕

Describe the problem this feature would solve

It would allow for more complex validation scenarios where a validation attribute is not enough. For example:

[MaxLength(25, ErrorMessage = "Name is too long!")]
public string Name
{
    get => _name;
    set
    {
        ValidateName(value);
        SetProperty(ref _name, value, true);
    }
}

private void ValidateName(string name)
{
    ClearErrors(nameof(Name));

    bool isValid = _myService.ComplexValidation(name);
    if (!isValid)
    {
        AddError(nameof(Name), "This name is not allowed!");
    }
}

Describe the solution

A way to manually add/clear errors to the error collection manually. It would help in cases where it's not possible or elegant to use validation attributes.

Describe alternatives you've considered

I tried creating a custom attribute and using the ValidationContext somehow to access my service, but decided in the end that it's way too ugly and hacky, and not a very reusable solution. I've also considered simply not using the toolkits implementation of INotifyDataErrorInfo, but it would be really nice if I didn't have to do that.

Additional context & Screenshots

None.

@sherman89 sherman89 added the feature request 📬 A request for new changes to improve functionality label Feb 10, 2021
@ghost
Copy link

ghost commented Feb 10, 2021

Hello, 'sherman89! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

@michael-hawker
Copy link
Member

@Sergio0694 you just added some helpers for this in the latest PR, eh? I think that was from #3742, #3729, and #3665, eh?

@michael-hawker michael-hawker added the mvvm-toolkit 🧰 Issues/PRs for the Microsoft.Toolkit.Mvvm package label Feb 10, 2021
@michael-hawker michael-hawker added this to the 7.0 milestone Feb 10, 2021
@Sergio0694
Copy link
Member

Hey @sherman89, thank you for trying it out, really happy to hear you're enjoying it so far! 😄

I believe there's an easy way to achieve what you're asking for without making modifications to the API surface, in particular I think it would be best to avoid adding API to let consumers manually change the internal state of the validator like that, since the validation APIs are actually meant to be used through the Validator class and in a centralized way, and they're particularly flexible to help out exactly with this. This would also make the code easier to maintain, as you'd always have your validation logic in the same place, without being able to introduce bugs when externally adding errors to a given entity.

What you're asking can actually be done already in two different ways, here's one with the [CustomValidation] attribute:

public class ValidationWithServiceModel : ObservableValidator
{
    private readonly IFancyService service;

    public ValidationWithServiceModel(IFancyService service)
    {
        this.service = service;
    }

    private string name;

    [MaxLength(25, ErrorMessage = "The name is too long")]
    [CustomValidation(typeof(ValidationWithServiceModel), nameof(ValidateName))]
    public string Name
    {
        get => this.name;
        set => SetProperty(ref this.name, value, true);
    }

    public static ValidationResult ValidateName(string name, ValidationContext context)
    {
        bool isValid = ((ValidationWithServiceModel)context.ObjectInstance).service.Validate(name);

        if (isValid)
        {
            return ValidationResult.Success;
        }

        return new ValidationResult("The name contains invalid characters");
    }
}

You can see here the ValidateName can implement the custom logic you mentioned, and it's simply retrieving the injected service through the field it's stored in from the current instance of the viewmodel. This has the advantage of having all the validation being executed at the same time when the property is set, not having to worry about updating the state from the custom validation method (which only needs to worry about actually doing the validation), and it also lets you keep the nice one-liner for the property set, as you now only need to call SetProperty and let it do all the work behind the scenes 🚀

I've added some unit tests in fc39f6f to showcase and test this scenario, let me know if that helps and if this solution works for you! Alternatively you could also pass a custom validation context with an IServiceProvider instance, but that'd be more complex to setup in some cases. This should be pretty easy to wire up instead. Let me know what you think! 😊

@sherman89
Copy link
Author

sherman89 commented Feb 11, 2021

@Sergio0694 Thank you Sergio that's exactly what I needed! Just tested it and works great! I had no idea CustomValidation even existed! 😮

Thank you very much for the quick response as well, I posted the issue and went to sleep, didn't expect to get an answer so quick 😄

Will close this issue now :) Best regards!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the Microsoft.Toolkit.Mvvm package
Projects
None yet
Development

No branches or pull requests

3 participants