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.Validate method does not create a ValidationResult with MemberNames #29214

Closed
ChrisJWoodcock opened this issue Apr 9, 2019 · 1 comment · Fixed by #38867
Closed
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.ComponentModel.DataAnnotations bug
Milestone

Comments

@ChrisJWoodcock
Copy link

ChrisJWoodcock commented Apr 9, 2019

When the CompareAttribute creates its ValidationResult it does not include the MemberNames of the context - I believe when this is used from Model Binding the actual property name which is the MemberName is added later, so the CompareAttribute can be used to correctly populate an error on a View (I can't locate where this correction occurs in the source); in the ValidationAttribute the MemberNames are used correctly.

This is preventing me from writing a unit test that checks that a Compare is made between two properties by specifying the property name exactly.


@pranavkm:

Blazor relies on this attribute and ran in to this issue. Our workaround was to produce an alternate attribute that passed the member in. Given the reluctance to change behavior in this area of the code, my proposal is to move the attribute to the runtime that has the behavior

API:

The new API is a clone of CompareAttribute with the "fixed" behavior:

   [System.AttributeUsageAttribute(System.AttributeTargets.Property, AllowMultiple=false)]
    public partial class ComparePropertyAttribute : System.ComponentModel.DataAnnotations.ValidationAttribute
    {
        public ComparePropertyAttribute(string otherProperty) { }
        public string OtherProperty { get { throw null; } }
        public string OtherPropertyDisplayName { get { throw null; } }
        public override bool RequiresValidationContext { get { throw null; } }
        public override string FormatErrorMessage(string name) { throw null; }
        protected override System.ComponentModel.DataAnnotations.ValidationResult IsValid(object value, System.ComponentModel.DataAnnotations.ValidationContext validationContext) { throw null; }
    }

Alternatives proposals

  • Fix the implementation. The behavior isn't documented so it might be safe to change this. However, the library targets ns2.1 (used outside .net core), has been stable for ages, isn't very actively maintained and the owner doesn't want to incur any additional support costs.
  • Add a property that opts CompareAttribute to use the updated implementation. Nothing against this, I just personally prefer creating a new attribute to adding switches to smooth over implementation detals.
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@terrajobst
Copy link
Member

Tagging area owners @lajones @ajcvickers

@pranavkm pranavkm added api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work and removed untriaged New issue has not been triaged by the area owner labels Jun 9, 2020
@bartonjs bartonjs added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review labels Jun 18, 2020
@bartonjs bartonjs removed the blocking Marks issues that we want to fast track in order to unblock other important work label Jun 18, 2020
@ajcvickers ajcvickers modified the milestones: Future, 5.0.0 Jun 22, 2020
pranavkm added a commit to pranavkm/runtime that referenced this issue Jul 7, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.ComponentModel.DataAnnotations bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants