dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.99k stars 4.67k forks source link

Unable to use wrapper types with built-in validation attributes #59076

Open anttikes opened 3 years ago

anttikes commented 3 years ago

Description

Validation attributes such as EmailAddressAttribute make use of the "is" keyword. The problem with this keyword is that it completely ignores user-defined conversion operators.

The following example comes from our own implementation where we support JSON Merge Patch:

[JsonConverter(typeof(OptionalConverterFactory))]
public record Optional<T>
{
    internal Optional(T value)
    {
        Value = value;
        HasValue = true;
    }

    public T Value { get; internal set; }
    public bool HasValue { get; internal set; }

    public static implicit operator Optional<T>(T value) => new(value);

    public static implicit operator T(Optional<T> value)
    {
        if (value.HasValue)
            return value.Value;

        return default;
    }
}

The OptionalConverterFactory mentioned in the code, and the generic converter class behind it are not relevant to this discussion as they have already been verified to work correctly.

The problem comes when we attempt to use this new record type with the existing validation attributes. Consider the following request class:

public class UpdateRequest
{
    [EmailAddress]
    public Optional<string> Email { get; init; }

    [Phone]
    public Optional<string> NotProvidedInTheRequest { get; init; }
}

And a Json-based "merge patch" request coming in to an ASP.NET Core Web API:

{
    "email": "new-address@foo.com"
}

The result is that the validation always fails because the EmailAddressAttribute uses the 'is' keyword. This keyword completely ignores the user-provided conversion operator, and thus line 25 of the attribute's validation code will always return false.

Expected behavior

I would expect the validation attributes work correctly even when wrappers are used. Ironically, the 'is' operator is hard-wired to support the Nullable struct but unfortunately for us, Nullable only allows value type parameters. So, we cannot use it as a replacement for the Optional record which allows both value and reference types.

We cannot "do away" without the Optional class as there doesn't seem to be a way to distinguish between "value was not provided" and "value was set to null", especially when using the System.String type.

Configuration

.NET Version: .NET Core v5.0.9 OS info: Windows 10, with all the latest Windows Update patches Architecture: x64

Analysis suggests that this issue is not specific to the OS or architecture configuration. The attribute's validation code is the same everywhere.

Regression?

Not a regression. The .NET Framework counterpart uses the 'as' keyword which suffers from the same problem.

ghost commented 3 years ago

Tagging subscribers to this area: @ajcvickers, @bricelam, @roji See info in area-owners.md if you want to be subscribed.

Issue Details
### Description Validation attributes such as [EmailAddressAttribute](https://github.com/dotnet/runtime/blob/main/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/EmailAddressAttribute.cs#L25) make use of the "is" keyword. The problem with this keyword is that it completely ignores user-defined conversion operators. The following example comes from our own implementation where we support JSON Merge Patch: ``` [JsonConverter(typeof(OptionalConverterFactory))] public record Optional { internal Optional(T value) { Value = value; HasValue = true; } public T Value { get; internal set; } public bool HasValue { get; internal set; } public static implicit operator Optional(T value) => new(value); public static implicit operator T(Optional value) { if (value.HasValue) return value.Value; return default; } } ``` The OptionalConverterFactory mentioned in the code, and the generic converter class behind it are not relevant to this discussion as they have already been verified to work correctly. The problem comes when we attempt to use this new record type with the existing validation attributes. Consider the following request class: ``` public class UpdateRequest { [EmailAddress] public Optional Email { get; init; } [Phone] public Optional NotProvidedInTheRequest { get; init; } } ``` And a Json-based "merge patch" request coming in to an ASP.NET Core Web API: ``` { "email": "new-address@foo.com" } ``` The result is that the validation always fails because the EmailAddressAttribute uses the 'is' keyword. This keyword completely ignores the user-provided conversion operator, and thus line 25 of the attribute's validation code will always return false. ### Expected behavior I would expect the validation attributes work correctly even when wrappers are used. Ironically, the ['is' operator](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/type-testing-and-cast#is-operator) is hard-wired to support the Nullable struct but unfortunately for us, Nullable only allows value type parameters. So, we cannot use it as a replacement for Optional. We cannot "do away" without the Optional class as there doesn't seem to be a way to distinguish between "value was not provided" and "value was set to null", especially when using the System.String type. ### Configuration .NET Version: .NET Core v5.0.9 OS info: Windows 10, with all the latest Windows Update patches Architecture: x64 Analysis suggests that this issue is not specific to the OS or architecture configuration. The attribute's validation code is the same everywhere. ### Regression? Not a regression. The .NET Framework [counterpart](https://referencesource.microsoft.com/#System.ComponentModel.DataAnnotations/DataAnnotations/EmailAddressAttribute.cs,21) uses the 'as' keyword which suffers from the same problem.
Author: anttikes
Assignees: -
Labels: `area-System.ComponentModel.DataAnnotations`, `untriaged`
Milestone: -
anttikes commented 3 years ago

I am already aware of one common workaround, and it is to implement IValidatableObject on the request class, and then in the body of the validation method to manually instantiate each of the validation attributes and provide the "value to be validated" from the Value property of the Optional record.

This workaround works but from the code perspective it is just plain ugly.

anttikes commented 3 years ago

I also found out that not all of the "string"-based built-in validation attributes follow a similar suite. For example:

It is unclear why there's a difference in behavior between the attributes. Logically thinking they should all follow the same approach.