Runtime: DataAnnotations/EmailAddressAttribute should not fail on when string is empty

[chris5287]

[EmailAddress] fails when the string value is empty, which is annoying as [Required] should handle this case instead.

[RegularExpression] does this correctly, see https://github.com/dotnet/corefx/blob/master/src/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/RegularExpressionAttribute.cs/#L58 for an example of how to fix

[Kurira]

Most of the data annotation attributes are skipping null/empty values, this seems like a bug.

[karelz]

Moving design discussion from PR here:
Reposting https://github.com/dotnet/corefx/pull/12553#issuecomment-256526118 from @divega:

I have started looking into this and so far I tend to believe that the current behavior is by design and that we shouldn't change it, but I am not done with the analysis. Here are a few concerns I have come up with so far:

1. There are other validation attributes that have similar behavior, e.g. [Phone] and [Url]. We should be consistent.
2. Conceptually the error you get for these validation attributes that don't accept empty strings seems perfectly reasonable: an empty string is not a valid email address/phone number/URL. So the proposed behavior seems more surprising than the current one.
3. As @ajcvickers already mentioned this change could break assumptions in existing code. E.g. an existing application using [EmailAddress] could be written in a way that cannot handle empty email addresses downstream (that said, the attribute currently allows for the target to be null, and the likelihood of being able to handle nulls but not empty strings seems low).
4. A separate mechanism controlled via [DisplayFormat(ConvertEmptyStringToNull = true)] can be used to remove empty strings from the input. This is handy when working with input from UI controls which will return empty strings if users haven't typed anything on them. Note that the conversion from empty strings to null is not performed in the validation code but by frameworks such as MVC.
5. More in general, when evaluating any behavioral or API changes to data annotations we need to consider compatibility across different implementations of .NET. I actually hope to get more detailed guidance from the CoreFx folks on what bar or set of principles to apply in this case and on some of the other active issues on DataAnnotations. E.g. assuming a specific attribute is part of some version of .NET Standard that libraries can target:
   
   
   
   • What kind of behavior differences are acceptable between .NET Core and in .NET Framework?
   
   
   • Can any two different implementations of .NET Standard have different AttributeUsage for the same attribute?
   
   
   • Can an attribute be sealed in .NET Framework and not in .NET Core?
   
   
   • Should we work on a plan so that all changes made to this codebase are eventually ported to .NET Framework?
   
   
   • Etc.
   
   

cc-ing @karelz in case he can answer some of the questions in (5) or mention this to others that can answer them.

[divega]

[@karelz - Update yes/no to reject/accept to be able to comprehend and reason about it]

Here is a more complete list of validation attributes and whether they accept/reject empty strings:

• [Phone] rejects (empty string)
• [EmailAddress]: rejects
• [Url]: rejects
• [FileExtensions]: rejects
• [MinLength]: rejects (unless the specified length is 0)
• [Required]: rejects by default, but can be controlled by AllowEmptyStrings property
• [CreditCard]: accepts (empty string)
• [EnumDataType]: accepts
• [Range]: accepts
• [Compare]: accepts
• [RegularExpression]: accepts

Considering the following:

1. We are currently very inconsistent across validation attributes
2. While working with user input it is very common to deal with empty strings
3. You have to rely on a separate component (e.g. MVC model binding) to have empty strings converted to null
4. The original intent seems to have been that other validation attributes would compose well with [Required]
5. The chances that existing code could rely on this behavior (reject empty strings but don't reject nulls) aren't that high,

I am leaning towards agreeing with the position that validator attributes should accept empty strings, with the exception of [MinLength(length)] with a specified length greater than zero.

If we decided to go with this decision the remaining concerns are about compatibility across .NET implementations, e.g. as I mentioned in my initial post:

... assuming a specific attribute is part of some version of .NET Standard that libraries can target:

• What kind of behavior differences are acceptable between .NET Core and in .NET Framework?
• Can any two different implementations of .NET Standard have different AttributeUsage for the same attribute?
• Can an attribute be sealed in .NET Framework and not in .NET Core?
• Should we work on a plan so that all changes made to this codebase are eventually ported to .NET Framework?
  ...

@karelz any take on those or on who could help us answer them?

[karelz]

4.The original intent seems to have been that other validation attributes would compose well with '[Required]'

I am not sure what this one means.

5.The chances that existing code could rely on this behavior (reject empty strings but don't reject nulls) aren't that high,

How common is it that the user code never gets null, yet can get empty string? Therefore making the "rejecting nulls" theoretical only.

Regarding your compat questions:

What kind of behavior differences are acceptable between .NET Core and in .NET Framework?

The more differences in behavior, the harder time for portable libraries targeting .NET Standard. Also the harder time for folks migrating code from full .NET to .NET Core (eventually leading to death by thousands paper cuts). So it is truly judgement call of benefit vs. impact on developers.

Can any two different implementations of .NET Standard have different 'AttributeUsage' for the same attribute?

I would be very cautious about it -- it can quickly lead to inconsistencies, allowing the attribute to be basically non-transferable between platforms. Especially if the attribute is part of .NET Standard itself.

Can an attribute be sealed in .NET Framework and not in .NET Core?

Again, this feels like slippery slope. If it is unsealed in .NET Core implementation only, but sealed in .NET Standard, then maybe. The question is how valuable it is.

Should we work on a plan so that all changes made to this codebase are eventually ported to .NET Framework? ...

It's hard to answer. There are cases where we make intentional breaking changes against .NET Framework in .NET Core, when it is the right thing to do, the benefit is high, it justifies the incompatibility burden/impact and also because the impact of .NET Core breaking changes between versions is not so bad as a breaking change in .NET Framework (which basically forces update on every app without its consent). Many of such intentional breaking changes would be rejected in .NET Framework.

Here's some context on compat.

cc: @weshaggard @terrajobst

[karelz]

@weshaggard @terrajobst any thoughts here?

[divega]

Data Annotations Triage: the main course of action we have considered to address this issue is to allow opting-in the proposed behavior. We don't want to change the default behavior because we think that could break the assumption potentially baked into many existing applications that these attributes are enough to guard against empty strings.

In such design, a new AllowEmptyStrings flag would be added to several of these attributes. The semantics would be similar to the ones used for AllowEmptyStrings in RequiredAttribute. The default value of the flag would match the current behavior of each attribute, e.g. in PhoneAttribute would default to false and in RangeAttribute it would default to true.

However after discussing this proposal in more depth we came to the conclusion that it does not add enough value:

• It fails to bring consistency to all validation attributes
• It is not much more discoverable than the available workaround of adding [DisplayFormat(ConvertEmptyStringToNull = true)].

For this reason we are closing this issue as a won't fix. However we should keep monitoring feedback from the community. If there is enough interest, we can reactivate the issue and reconsider the opt-in option.

cc @lajones @ajcvickers

[DrGriff]

I believe that "[Required] & [EmailAddress]" should ensure that a) a value is provided and that b) it is valid, whereas [EmailAddress] on its own should have empty string or null as acceptable, and this should have been how the other validators worked.

But, I appreciate that this would be a breaking change.

So, can you allow us to inject a boolean flag to say whether null/empty are acceptable, e.g.

[EmailAddress] = [EmailAddress(false)]
and have
[EmailAddress(true)] to allow for nulls & empty strings, and add this functionality to [Phone] etc, etc.

[andrensairr]

I stumbled on this unfortunate case when using an EF POCO against a legacy database with a non-nullable column which was mapped using [Required(AllowEmptyStrings = true)] because it is treated as optional in code, and to which I recently added [EmailAddress] to ensure any values set to it were properly validated. Needless to say it resulted in validation errors for all existing empty string values. The proposed work-around of [DisplayFormat(ConvertEmptyStringToNull = true)] is obviously not suitable in this case, though I did resolve it with a forwarding attribute instead. It's not ideal, but acceptable enough.

[DrGriff]

I decided to write my own private NuGet library of custom validation attributes. There's lots of info on the web for how to do this, but for example:

public class IsEmailAddressAttribute : ValidationAttribute
    {
        private static readonly Regex EmailRegex = new Regex(@"[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?", RegexOptions.Compiled | RegexOptions.IgnoreCase);
        private readonly bool isRequired;
        private readonly int maxLength;

    public IsEmailAddressAttribute(bool isRequired = false, int maxLength = 255)
    {
        (this.isRequired, this.maxLength) = (isRequired, maxLength);
    }
    public override bool IsValid(object value)
    {
        ....
    }
    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        ....
    }
}

So to consume this, you can have [IsEmailAddress] or the non-default values [IsEmailAddress(true)], [IsEmailAddress(true, 179)]

I've omitted the actual logic in the methods, you may not agree with my implementation [though all my tests pass :-)].

FYI: my initial thought was to inherit from EmailAddressAttribute, but of course, this class is sealed.