Aspnetcore: Options Validation: Support DataAnnotations

[HaoK]

From Experience review with @ajcvickers @DamianEdwards @Eilon @davidfowl :

We should support DataAnnotations on the POCO for 2.2:

for example:

public class MyOptions 
{
        [Required]
        public string Key { get; set; }

        [Range(18,99, ErrorMessage="Age should be between 18 and 99")] 
        public int Age { get; set; } 

        [DataType(DataType.PhoneNumber)] 
        [Phone] 
        Public string PhoneNumber { get; set; } 

        [DataType(DataType.EmailAddress)] 
        [EmailAddress] 
        Public string Email { get; set; } 
}

Need to figure out how to opt in, via new default overload?
services.AddOptions<MyOptions>.[Auto]Validate()

[thiagolunardi]

The ability to add a binding name will be handy. Like JsonProperty.

public class MyOptions
{
    [JsonProperty("ASPNETCORE_ENVIRONMENT")]
    public string Environment { get; set; }
}

Sometimes the host environment set some values, and I'd like to read it using my app language.

[gautelo]

@thiagolunardi I agree. This was suggested in issue #775. It was decided to implement this sort of thing through the BinderOptions instead as part of #789.

Personally I feel like implementing yet another way to serialize properties instead of using what's already there is a mistake. I would much rather use attributes for this.

I believe the counter argument is that sometimes you don't own the options type and can't add attributes to it. In my mind that would be a good argument for implementing 789 as a fallback. But attributes should be the primary solution.

Especially now that we're adding validation through standardized attributes, I feel like that sets a precedent for also using attributes for serialization/property mapping. After all, the same argument could apply to validation - that rules could be added to BinderOptions or some other structure disconnected from the model in case one does not own the options model.

So, I feel like the decision to scrap 775 Should be reconsidered. Attributes should be the common approach for serialization and validation alike, with BinderOptions as a fallback only.

[bdovaz]

I agree with @gautelo., Json.NET supports DataMember attribute although it has already JsonProperty attribute itself.

[HaoK]

Options and Configuration are decoupled, we can certainly consider revisiting and unifying the binder with whatever we end up for Options Validation as the binder primarily exists to bind config to options.

[HaoK]

It likely would end up similar to how options validation works, it would be a sugar method that generated the appropriate underlying binderoptions, as opposed to a replacement for the binder options.

[gautelo]

@HaoK Sure, either way works. As a library consumer I'm not really fussed if DataMembers are sugar on top of BinderOptions or the other way around, as long as I can do either one.

[HaoK]

Fixed in https://github.com/aspnet/Options/pull/272

[bdominguez]

@HaoK did you included DataMember attribute so we can change a property binding name as we discussed in this post? I don't see it.

Thanks.

[HaoK]

The binder is unrelated to options validation, the binder is something that's solely part of configuration and any future binder work is not really directly related to this

[HaoK]

The binder is just one of many different ways you can set options values, options validation is a new lower level feature.

[bdominguez]

But it's something that are you going to do? We discussed it above and thought that it would be implemented at some point.

[HaoK]

Its not particularly high priority right now, we most likely are just going accept some form of the existing binder PR (https://github.com/aspnet/Configuration/pull/789) that adds the ability to map properties.

The built in binder is meant to be pretty simple, we always envisioned more complex binding to be handled via custom code.

[HaoK]

The main difference from the discussion above, is that the binder is not at all targeted for "serialization", which is why going down the path of [DataMember] is not really a good fit as those are serialization attributes and that's not really something the binder is intended for

[gautelo]

sigh Why can't we have nice things... Would it be better if the request came in its own thread, one dedicated to Serialization through data contracts or something along those lines? Basically it feels like we're being met with the sort of response saying "this does not conform to our narrow definition of what the thing should do, hence the request is not reasonable and thusly rejected". Frustrating!