dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.36k stars 9.99k forks source link

CompareAttribute ignored with OnValidSubmit EditForm #10643

Closed smartprogrammer93 closed 5 years ago

smartprogrammer93 commented 5 years ago

Using an EditForm in Blazor client side I have a model with compare attribute. The error message does show when the password and confirm password are not the same but does not prevent form submission for some reason. Once the form is submitted the error message is removed.

Expected behavior

Prevent form submission and keep the error message

smartprogrammer93 commented 5 years ago

To get around this, i used the following code using OnSubmit and EditContext public void Submit() { var isValid = !_editContext.GetValidationMessages().Any(); if (isValid) Vm.ResetPassword(); }

mkArtakMSFT commented 5 years ago

Thank you for filing this issue. In order for us to investigate this issue, please provide a minimalistic repro project that illustrates the problem.

mkArtakMSFT commented 5 years ago

If, however, you believe this was an issue on your end, please close the issue. Thanks!

smartprogrammer93 commented 5 years ago

@mkArtakMSFT Please check this repository https://github.com/smartprogrammer93/CompareTest

mkArtakMSFT commented 5 years ago

Thanks @smartprogrammer93. We'll look into this in the near future.

smartprogrammer93 commented 5 years ago

@mkArtakMSFT Can you please remove the question Label so the issue will be considered later for Backlog

gilbertogwa commented 5 years ago

I found that this occurs when the EditContextDataAnnotationsExtensions.ValidateModel method does not add the field to the "messages" collection. The method does not include this because ValidationResult has no items in the "MemberNames" array when the Compare attribute is used.

See code with comment:

private static void ValidateModel(EditContext editContext, ValidationMessageStore messages)
{

  // <removed code>

   foreach (var validationResult in validationResults)
   {

     **// CompareAttribute will not generate MemberNames, messages will not be updated.**
     foreach (var memberName in validationResult.MemberNames)
     {
        messages.Add(editContext.Field(memberName), validationResult.ErrorMessage);
     }
   }

 // <removed code>

}

To work around this problem, I suggest changing the implementation of the CompareAttribute.IsValid method to include the validated property name in the method return.

Original version


protected override ValidationResult IsValid(object value, ValidationContext validationContext) {

   PropertyInfo otherPropertyInfo = validationContext.ObjectType.GetProperty(OtherProperty);

   if (otherPropertyInfo == null) {
      return new ValidationResult(String.Format(CultureInfo.CurrentCulture, DataAnnotationsResources.CompareAttribute_UnknownProperty, OtherProperty));
   }

   object otherPropertyValue = otherPropertyInfo.GetValue(validationContext.ObjectInstance, null);

   if (!Equals(value, otherPropertyValue)) {
      if (OtherPropertyDisplayName == null) {
         OtherPropertyDisplayName = GetDisplayNameForProperty(validationContext.ObjectType, OtherProperty);
      }
      return new ValidationResult(FormatErrorMessage(validationContext.DisplayName));
   }

   return null;

}

Suggested version:


protected override ValidationResult IsValid(object value, ValidationContext validationContext) {

   PropertyInfo otherPropertyInfo = validationContext.ObjectType.GetProperty(OtherProperty);

   if (otherPropertyInfo == null) {
      // SUGGESTED CHANGE
      var msg = String.Format(CultureInfo.CurrentCulture, DataAnnotationsResources.CompareAttribute_UnknownProperty, OtherProperty);
      return new ValidationResult(msg, new[] { validationContext.MemberName });
   }

   object otherPropertyValue = otherPropertyInfo.GetValue(validationContext.ObjectInstance, null);

   if (!Equals(value, otherPropertyValue)) {
      if (OtherPropertyDisplayName == null) {
         OtherPropertyDisplayName = GetDisplayNameForProperty(validationContext.ObjectType, OtherProperty);
      }
      // SUGGESTED CHANGE
      var msg = FormatErrorMessage(validationContext.DisplayName);
      return new ValidationResult(msg, new[] { validationContext.MemberName });
   }

   return null;

}
EdCharbeneau commented 5 years ago

I'm running into the same issue. I was trying to use the compare validator to make an experience similar to GitHub's delete where the user must type a matching name to confirm deletion.

@page "/post/delete/{PostId:int}"
@inject BloggingContext dbContext

<h3>Delete Post</h3>

@if (deleted > 0)
{
    <div class="alert alert-warning" role="alert">
        Your post was deleted.
    </div>
}

<EditForm Model="DeleteConfirmation" OnValidSubmit="HandleValidDelete">
    <DataAnnotationsValidator />
    <ValidationSummary />
    <div class="form-group">
        <label>
            Type the Post Title <span class="alert-danger">@DeleteConfirmation.CompareValue</span> and click to delete this item.
        </label>
        <InputText class="form-control" @bind-Value="DeleteConfirmation.CompareInput" />
    </div>
    <button class="btn btn-danger" type="submit">Delete</button>
</EditForm>

@code {

    [Parameter] public int PostId { get; set; }

    [Parameter] public DeleteConfirmationModel DeleteConfirmation { get; set; }

    int deleted = 0;
    Post post;

    protected override async Task OnInitializedAsync()
    {
        post = await dbContext.Posts.FirstAsync(p => p.PostId == PostId);
        DeleteConfirmation = new DeleteConfirmationModel
        {
            CompareValue = post.Title
        };
    }

    async Task HandleValidDelete()
    {
        // Compare validator doesn't block submission
            dbContext.Posts.Remove(post);
            deleted = await dbContext.SaveChangesAsync();
   }
}
    public class DeleteConfirmationModel
    {
        public string CompareValue { get; set; }

        [Compare(otherProperty: nameof(DeleteConfirmationModel.CompareValue),
            ErrorMessage = "Value did not match reference")]
        [Required]
        public string CompareInput { get; set; }
    }
smartprogrammer93 commented 5 years ago

My bad i pressed the close button by mistake

pranavkm commented 5 years ago

As noted in https://github.com/aspnet/AspNetCore/issues/10643#issuecomment-520236208, on a form submit Blazor's DataAnnotationsValidator fails to record validation results that are not associated with a member. CompareAttribute does this which results in the buggy behavior you're seeing here.

In 3.1, we're doing a couple of things to address this:

1) Validation results without members now get associated with the model instance. In addition we've added support to ValidationSummary to print error messages that are specifically associating with the model. You should be able to do something along the liens of

<EditForm Model="myModel" OnValidSubmit="OnSubmit">
    <DataAnnotationsValidator />
    <InputText Name="Password" /> <ValidationMessage For=@(() => myModel.Password)" />
    <InputText Name="ConfirmPassword" /> <ValidationMessage For=@(() => myModel.ConfirmPassword)" />    

    <ValidationSummary Model="myModel" />
    <button class="btn btn-danger" type="submit">Submit</button>
</EditForm>

The ValidationSummary would additionally display messages produced from models implementing IValidatableObject or using custom ValidationAttribute \ CustomValidationAttribute.

  1. In addition, for the 3.1.0-preview2 release, we plan on introducing an experimental package - Microsoft.AspNetCore.Components.DataAnnotations.Validation - with an ComparePropertyAttribute that behaves like a CompareAttribute but produces a valid MemberName. It should be a drop in replacement to CompareAttribute. Users in 3.0 should be able to copy the type in to their app to resolve the error they're seeing here. Here's the source: https://github.com/aspnet/AspNetCore/blob/c298c94fe1113b64474a3d5fcdef487f02c74991/src/Components/Blazor/Validation/src/ComparePropertyAttribute.cs

This package is marked experimental since our plan is to roll these changes in to CoreFx as part of the 5.0 milestone. Hope this helps.