Adds inferred [Required] for non-null ref types

[rynowak]

Follow up from #9194

This change adds the automatic inference of [Required] for non-nullable
properties and parameters. This means that if you opt into nullable
context in C#8, we'll start treating those types as-if you put
[Required] on them.

This provides a nice invariant to rely on, namely that MVC will honor
your declared nullability contract OR report a validation error. This
reinforces the guidance already published by the C# team for using
POCOs/DTOs with nullability. See
https://github.com/aspnet/specs/blob/master/notes/3_0/nullable.md for my
analysis on the topic.

[rynowak]

One other idea (that I didn't do, and think we shouldn't do) would be to use the nullability information to create related objects. So for the following, we could auto-create Address even if the data was missing:

public class Person
{
    public string FirstName { get; set; } = default!;
    public Address Address { get; set; } = default!;
}

public class Address { }

I didn't do this, because IMO you have a better option if you want this behaviour.

public class Person
{
    public string FirstName { get; set; } = default!;
    public Address Address { get; } = new Address();
}

public class Address { }

[rynowak]

@roji FYI

[rynowak]

Will add some functional tests tomorrow to capture the E2E scenario. I'm confident that this is working, but we need tests in place that capture the whole workflow to make sure there aren't other blockers.

[pranavkm]

Does this have trouble with collection types?

[Nullable(new[] { 1, 2 })] string[] NotNullMaybeNull; // string?[]!

if not, we should add some comments explaining how this works when more than one value is present in the NullableAttribute byte array

[rynowak]

Yeah, I hadn't thought about this case, but it's worth discussion.

Note that we don't have anything like this today so this would be a one-off thing or something built into the model binders for collections. We don't support you placing [Required] on a type parameter.

[rynowak]

There's actually another problem here.

Consider a case like:

    class Program
    {
        public string Foo { get; set; } = default!;

        public Container<string?> Bar { get; set; } = default!;
    }

    class Container<T>
    {
        public T Item { get; set; } = default!;
    }

ModelMetadata is an API you can use to get the metadata for a property, parameter or type. We're primarily interested in the property case, where you pass in the container type and property info. However, with nnrt, this isn't enough information. In this example for property Item the only place where the metadata reflect the nnrt information is on property Bar.

You need to know the property, the containing type and now the containing property in order to create a correct model metadata. You could create a deeper, more complicated example by adding N layers .Put another way, nnrt + generics is just type erasure :(

Basically with generics in the picture, our current architecture can't support this usage. We would have to define the concept of a root (Controller/Page type) in MM and always walk from the root.

[roji]

Note the almost identical logic about to go into EF Core: https://github.com/aspnet/EntityFrameworkCore/pull/15499/files#diff-0ebd43bbbecf10e14f32e65b224af763R28

I actually cache some type information about NullableAttribute for better perf, while supporting multiple NullableAttribute (since it may be synthesized several times into different participating assemblies...).

[rynowak]

@roji does you code need to be thread-safe? That appears to not be...

[roji]

Nope, the "convention" you are seeing is a composable piece of code that runs once at startup, during EF Core's model building, and instances aren't supposed to be thread-safe. However, we've seen cases of EF Core models which were so big that the model building process was quite slow, which is why I'm doing this caching.

[roji]

Some very similar nullability handling code going into ASP.NET and EF at the very same time (dotnet/efcore#15499)...

One other idea (that I didn't do, and think we shouldn't do) would be to use the nullability information to create related objects. So for the following, we could auto-create Address even if the data was missing:

I agree, the less magic going on, the better...

[rynowak]

I agree, the less magic going on, the better...

The reason I thought about this at all.... I want to be able to make statements like: "if we violate your nulllability contract then we give you a validation error". I could see someone interpreting that rule in favor of this, but I think it would have serious unintended consequences. Model binding creates objects based on the presence of data in the request that matches the model. Doing this based on nullabitity contracts could lead to infinite recursion instead 😆

[rynowak]

@JeremySkinner

[rynowak]

Upated

[JamesNK]

Thinking about this some more and it feels a little odd that non-nullable reference types are now required, but non-nullable value types are not. Consider:

    public class Person
    {
        public string Name { get; set; } = default!;
        public int Age { get; set; }
    }

Name is required, Age is not.

[rynowak]

[Required] means not-null.

When we look at the types of properties and parameters, we're only talking about the allowed state of the model. MVC has the ability to validate the state of the model using [Required]. This makes sense because the contract implied by nnrt is about the legal state of a property/parameter/variable. I think that this is the most natural way to think about nnrt by far.

Also note that we really have two meanings for required. Value types like int generate client validation messages that require you to specify the values in the form. When validation runs we don't validate the non-null-ness of your int property because it's always in a legal state already.

---

We also have the ability to specify value must be present on the wire ([BindRequired]) - but this is a little bit of a trap. Most use cases actually want [Required] or want something like JSON/XML schema validation.

• [BindRequired] isn't understood by serializers
• [BindRequired] allows explicit null

Usually when we get feedback about these things, they outcome is kinda unsatisfying.

Schema-based validation is a better fit when you care deeply about exposing/maintaining a contract - I consider code-first schema validation to be schema validation all the same. I say this because once you care about these details, you are already thinking about documenting a schema.

[Required] or another data-annotation validation is a better fit when you want to validate something about the input's format or range.

Often this feedback comes down to users wanting a semantic difference between an omitted value, and a present and explicit null value. I try to dissuade folks from this because it's not good API design. It's also not a good fit for any of our features.

TLDR read your serializer's docs and do that.

[JamesNK]

I see. I'm use to looking at required from a deserialization viewpoint. Often it requires the presence of the data as well as the value. Required here only cares about the .NET value.

I thought Newtonsoft.Json use [Required] but it doesn't. There is a [JsonRequired] attribute that requires the data exists in JSON AND it is not null.

So required on a non-nullable value type would never do anything:

    public class Person
    {
        [Required]
        public int Age { get; set; }
    }

[JamesNK]

https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.requiredattribute?view=netframework-4.8#remarks

The RequiredAttribute attribute specifies that when a field on a form is validated, the field must contain a value. A validation exception is raised if the property is null, contains an empty string (""), or contains only white-space characters.

The added required attribute should set AllowEmptyStrings to true. And add unit tests for empty and white-space only strings

[rynowak]

The added required attribute should set AllowEmptyStrings to true. And add unit tests for empty and white-space only strings

I'll do it for the sake of #Pedantry. In practice it only will matter if you do something wierd. MVC already handles this case for you be treating whitespace/empty strings as null (ConvertEmptyStringToNull
defaults to true) https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinder.cs#L71

[JamesNK]

Ah ok, I wasn't sure if that flag was being used by the validation logic. If the metadata is not exposed and it is never used internally then there is no point setting it 😬

A comment and test would be nice tho for clarity

[rynowak]

I'm making the change, it's a little more correct so we might as well.

This means that if someone default-initializes this to "" it will be considered valid. Personally I think that pattern is wrong no matter way, but I guess I can help them out...

[JeremySkinner]

@rynowak One question on this...will it behave in the same way as the current handling for non-nullable value types? (ie if no required rule is explicitly run, or the DataAnnotations provider is disabled, then the default "A value is required" message will be added instead?)

[rynowak]

@rynowak One question on this...will it behave in the same way as the current handling for non-nullable value types? (ie if no required rule is explicitly run, or the DataAnnotations provider is disabled, then the default "A value is required" message will be added instead?)

This is a super set.

ModelMetadata.IsRequired is the thing that adds client side validation messages using jQuery validation. This does ModelMetadata.IsRequired and adds an implicit [Required]. Is there a specific case or scenario you are worried about?

[JeremySkinner]

Is there a specific case or scenario you are worried about?

No, was just curious - should all work fine for me. Thanks!

[rynowak]

/azp run AspNetCore-ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).