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

CompareAttribute ignored with OnValidSubmit EditForm #10643

Closed
smartprogrammer93 opened this issue May 30, 2019 · 10 comments
Closed

CompareAttribute ignored with OnValidSubmit EditForm #10643

smartprogrammer93 opened this issue May 30, 2019 · 10 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. component ecosystem Indicates an issue which also has impact on 3rd party component ecosystem Done This issue has been fixed

Comments

@smartprogrammer93
Copy link

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 smartprogrammer93 changed the title CompareAttribute ignored with OnValidSubmit EditForm [Blazor] CompareAttribute ignored with OnValidSubmit EditForm May 30, 2019
@smartprogrammer93
Copy link
Author

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(); }

@Eilon Eilon added the area-blazor Includes: Blazor, Razor Components label May 30, 2019
@mkArtakMSFT
Copy link
Member

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
Copy link
Member

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

@mkArtakMSFT mkArtakMSFT added Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. question labels May 31, 2019
@smartprogrammer93
Copy link
Author

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

@mkArtakMSFT mkArtakMSFT removed the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Jun 7, 2019
@mkArtakMSFT
Copy link
Member

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

@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview8 milestone Jun 7, 2019
@smartprogrammer93
Copy link
Author

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

@mkArtakMSFT mkArtakMSFT added this to Triage in Blazor via automation Jun 24, 2019
@mkArtakMSFT mkArtakMSFT moved this from Triage to Preview9 in Blazor Jun 26, 2019
@mkArtakMSFT mkArtakMSFT modified the milestones: 3.0.0-preview9, 3.1.0 Jul 17, 2019
@SteveSandersonMS SteveSandersonMS moved this from Preview9 to 3.1 in Blazor Jul 18, 2019
@mkArtakMSFT mkArtakMSFT modified the milestones: 3.1.0, 3.1.0-preview1 Aug 5, 2019
@gilbertogwa
Copy link

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
Copy link

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; }
    }

@danroth27 danroth27 added the component ecosystem Indicates an issue which also has impact on 3rd party component ecosystem label Sep 4, 2019
@danroth27 danroth27 changed the title [Blazor] CompareAttribute ignored with OnValidSubmit EditForm CompareAttribute ignored with OnValidSubmit EditForm Sep 6, 2019
@danroth27 danroth27 mentioned this issue Sep 7, 2019
10 tasks
@mkArtakMSFT mkArtakMSFT added this to the 3.1.0-preview3 milestone Sep 25, 2019
@danroth27 danroth27 added the bug This issue describes a behavior which is not expected - a bug. label Sep 25, 2019
Blazor 3.1 automation moved this from To Do (3.1.0 Preview2) to Triage Oct 1, 2019
@smartprogrammer93
Copy link
Author

My bad i pressed the close button by mistake

@mkArtakMSFT mkArtakMSFT moved this from Triage to 3.1.0 Preview3 in Blazor 3.1 Oct 1, 2019
@mkArtakMSFT mkArtakMSFT moved this from To Do (3.1.0 Preview2) to In progress in Blazor 3.1 Oct 11, 2019
pranavkm added a commit that referenced this issue Oct 11, 2019
* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
pranavkm added a commit that referenced this issue Oct 14, 2019
* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
pranavkm added a commit that referenced this issue Oct 16, 2019
* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
pranavkm added a commit that referenced this issue Oct 17, 2019
* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
pranavkm added a commit that referenced this issue Oct 18, 2019
* Validation fixes for Blazor

* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
@pranavkm
Copy link
Contributor

pranavkm commented Oct 18, 2019

As noted in #10643 (comment), 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.

@pranavkm pranavkm moved this from In progress to Done in Blazor 3.1 Oct 18, 2019
@pranavkm pranavkm added Done This issue has been fixed and removed Working labels Oct 18, 2019
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 3, 2019
@jaredpar jaredpar removed this from Done in Blazor 3.1 Jan 7, 2020
3dots pushed a commit to 3dots/aspnetcore-Web.JS that referenced this issue Feb 19, 2024
* Validation fixes for Blazor

* Ensure validation result that are not associated with a member are recorded. Fixes dotnet/aspnetcore#10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes dotnet/aspnetcore#10526
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. component ecosystem Indicates an issue which also has impact on 3rd party component ecosystem Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

8 participants