3
\$\begingroup\$

I have a binding model where at least one of two fields must be specified. I can't just set them both as [Required]. Here is what I came up with:

using System;
using System.ComponentModel.DataAnnotations;

namespace MyApp.Utils.DataAnnotations
{
    [AttributeUsage(AttributeTargets.Class, AllowMultiple = true)]
    public class EitherRequiredAttribute : ValidationAttribute
    {
        private readonly string _field1;
        private readonly string _field2;

        public EitherRequiredAttribute(string field1, string field2) => (_field1, _field2) = (field1, field2);

        protected override ValidationResult IsValid(object value, ValidationContext validationContext)
        {
            var property1 = validationContext.ObjectType.GetProperty(_field1);
            if (property1 == null)
                return new ValidationResult(string.Format("Unknown property: {0}", _field1), new[] { _field1 });
            var property2 = validationContext.ObjectType.GetProperty(_field2);
            if (property2 == null)
                return new ValidationResult(string.Format("Unknown property: {0}", _field2), new[] { _field2 });

            var value1 = property1.GetValue(validationContext.ObjectInstance);

            if (value1 != null)
                return ValidationResult.Success;

            var value2 = property2.GetValue(validationContext.ObjectInstance);

            if (value2 != null)
                return ValidationResult.Success;

            return new ValidationResult(string.Format("Either or both of \"{0}\" and \"{1}\" are required", _field1, _field2), new[] { _field1, _field2 });
        }
    }
}

Use case:

[EitherRequired("A", "B")]
public class SomeModel
{
    public int? A {get; set;}
    public int? B {get; set;}
}
\$\endgroup\$
4
\$\begingroup\$

Fields and properties are different things. You are mixing them up: the naming in the attribute code suggests that you want to operate on fields, but the use site is supplying property names. The naming of field1 and field2 should be changed to propertyName1 and propertyName2 respectively.

Don't forget your BindingFlags! Right now your code will look up any public property with the given name, including static properties. You probably only want public instance properties:

var property1 = validationContext.ObjectType.GetProperty(_field1, BindingFlags.Instance | BindingFlags.Public);

Depending on your performance constraints, reflection might be too slow for you. If so, it's common practice to cache the results of calls to GetProperty since that information is unchanging at runtime.

It's good practice to use nameof for getting string representations of identifiers, because if SomeModel.A gets renamed then the program won't compile until you also update the attribute argument.

[EitherRequired(nameof(A), nameof(B))]
public class SomeModel
{
    public int? A {get; set;}
    public int? B {get; set;}
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I stumbled over the naming as well without mentioning it. Good call, but propertyName1 would be better than property1. Nevertheless +1 \$\endgroup\$ – Heslacher Nov 20 '19 at 14:30
  • \$\begingroup\$ Agreed on propertyName1. Updated. \$\endgroup\$ – Jeff Nov 20 '19 at 15:01
3
\$\begingroup\$

Your code looks almost neat and clean, but there are still some areas where you could improve it.

  • Althought braces may be optional for single line if statements your code will get less error-prone by using them.
  • Some more vertical spacing (new lines) would improve the readability of the code.
  • The class EitherRequiredAttribute could be named better. I would suggest to name it AtLeastOneRequiredAttribute. Using this name makes the purpose of the class and the annotation more clear.
  • You only check if valueX == null hence you could omit both value1 and value2 and just check if the call to GetValue() returns null.
  • Instead of having

    var property1 = validationContext.ObjectType.GetProperty(_field1);
    if (property1 == null)
        return new ValidationResult(string.Format("Unknown property: {0}", _field1), new[] { _field1 });  
    var property2 = validationContext.ObjectType.GetProperty(_field2);
    if (property2 == null)
        return new ValidationResult(string.Format("Unknown property: {0}", _field2), new[] { _field2 });  
    

    you could introduce a method bool TryGetProperty(string, ValidationContext, out PropertyInfo).

Implementing the mentioned points would lead to

[AttributeUsage(AttributeTargets.Class, AllowMultiple = true)]
public class AtLeastOneRequiredAttribute : ValidationAttribute
{
    private readonly string _field1;
    private readonly string _field2;

    public EitherRequiredAttribute(string field1, string field2) => (_field1, _field2) = (field1, field2);

    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        if (!TryGetProperty(_field1, validationContext, out var property1))
        {
            return new ValidationResult(string.Format("Unknown property: {0}", _field1), new[] { _field1 });
        }

        if (!TryGetProperty(_field2, validationContext, out var property2))
        {
            return new ValidationResult(string.Format("Unknown property: {0}", _field2), new[] { _field1 });
        }

        if (property1.GetValue(validationContext.ObjectInstance) != null ||
            property2.GetValue(validationContext.ObjectInstance) != null)
        {
            return ValidationResult.Success;
        }

        return new ValidationResult(string.Format("Either or both of \"{0}\" and \"{1}\" are required", _field1, _field2), new[] { _field1, _field2 });
    }
    private bool TryGetProperty(string fieldName, ValidationContext validateionContext, out PropertyInfo propertyInfo)
    {
        return (propertyInfo = validateionContext.ObjectType.GetProperty(fieldName)) != null;
    }
}

Althought methods named TryGetXXX are indicating that no exception should be thrown by calling them, this method will throw if fieldName is either null or WhitheSpace.

\$\endgroup\$
1
  • \$\begingroup\$ Regarding your last comment, you probably should validate that in the constructor. \$\endgroup\$ – JAD Nov 20 '19 at 9:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.