Efcore: Remaining reference nullability work

[divega]

We are going to use this issue to track any additional changes necessary to properly support nullable reference types in 3.0, in particular react to any API changes to the C# 8.0 compiler generated attributes and support for custom attributes.

[roji]

Some first things that we need to look into:

Preconditions and postconditions ([AllowNull], [DisallowNull], [MaybeNull], [NotNull], see https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md). These can go on:

• (Generic) properties
• Property getters and setters. We can probably ignore these as have the nullability of the property itself (although I'm not sure what it means to have a nullable property with a getter and setter that are nullable).
• [NullablePublicOnly] on the module. This can tell us that the model was compiled without metadata on non-public members. We may want to emit a warning if we see a mapped, non-public property in a module without non-public metadata. The API shape for this attribute is being finalized.

[sjb-sjb]

Question, is there a document or discussion that explains how "string" vs "string?" will be treated by EF Core once non-nullable reference types are available, and how the [Required] attribute will be interpreted for each of them?

[roji]

@sjb-sjb we don't have anything documented yet AFAIK, but if you opt into the nullable reference types feature, non-nullable types (e.g. string) will be treated as if they have [Required] on them. Placing an explicit [Required] attribute will override that allow you to manually specify whether a column in the database is nullable or not.

[sjb-sjb]

Do you mean that if I have string? then I can also add [Required] to force it to be non-nullable in the database -- in other words if I opt in to non-nullable reference types then "[Required] string?" and just "string" would be treated the same way? In that case the meaning of just plain "string" would change in both the EF database mapping and in C# when I opt into non-nullable reference types -- which would make sense.

[roji]

if I opt in to non-nullable reference types then "[Required] string?" and just "string" would be treated the same way

That's correct - as far as EF and the database are concerned. A non-nullable database column would be created in migrations, and EF would expect to never see a null when writing entities.

[ajcvickers]

@roji To find link to remaining change.

[roji]

Possible (but not certain) further changes in the nullability attributes: https://github.com/dotnet/corefx/issues/36222#issuecomment-511973898.

Apart from that there's also pre- and post-conditions.

[roji]

Got confirmation that no further changes to nullability metadata. Keeping open to track pre- and post-conditions.

[roji]

Note: the nullability context attribute will no longer appear at the module level (https://github.com/dotnet/roslyn/pull/37610), so simplified our convention to not handle that.

[sjb-sjb]

How would I specify a string that cannot be null but which can be empty ("")? If non-nullable string is interpreted by EF in the same way as [Required] string? then it would have to be a string of length at least one. Possibly-empty but non-null strings could be desirable for eliminating a lot of null-pointer tests after materialization.

[ajcvickers]

@sjb-sjb I think you're confusing what Required does as a validation attribute with how it is interpreted by EF mapping. In EF mapping required _only_ influences nulability.

[sjb-sjb]

Got it, thanks.

[roji]

Here is a note regarding pre- and post-conditions (see these docs). In a nutshell, there are four attributes that allow you to override the default nullability setting on a get/set basis (e.g. a non-nullable property can still return or be set to null).

Within the EF Core context, I thought about it a little bit and came up with the following spec.

| Scenario | Result |
|-------------|----------|
| Non-nullable property with [MaybeNull] | Thje property may return nulls, so the EF property must be nullable. This means we will be writing nulls into the property when materializing, violating the property's contract (doesn't seem to be an issue). |
| Non-nullable property with [AllowNull] | The property allows setting null but will never return null, so the EF property should be non-nullable. Useful for properties that support setting null but translate internally it to some non-null value (e.g. empty string). |
| Nullable property with [NotNull] | The property will never return null, but the explicit nullability expresses user intent, so the EF property should be nullable |
| Nullable property with [DisallowNull] | The property doesn't allow setting null, but can return null, so the property must be nullable. |

According to the table above, the only attributes EF actually needs to recognize is [MaybeNull], since it means that a non-nullable property may actually return nulls (which need to be written to the database).

This logic is implemented in #16985, but it would be good to confirm the above.

[roji]

~Following the pre- and post-conditions discussed above, we also need to take care of backing fields - the current nullability implementation looks only at the CLR property, but a backing field may be configured with differing nullability. It seems like we should look at the configured PropertyAccessMode, and examine the appropriate member (field or property). FieldDuringConstruction should also examine the property's nullability rather than the field.~

Decision: like with other modelling aspects, we only consult the public-facing property, so the field's reference nullability will not be taken into account when deciding on the EF property's requiredness. If only a field is configured without a property, the field's nullability will of course be the determining factor. This means that users can make EF aware only of the field, and do whatever they want with the property.

[ajcvickers]

@roji Agree with your analysis/decisions on the pre/post conditions.

[roji]

Closing as [MaybeNull] has been handled in #16985, and we've decided not to look at backing fields for the purpose of requiredness.

[sjb-sjb]

@ajcvickers, on your note that [Required] on a nullable property causes EF to make the database field non-nullable .... does it have to be RequiredAttribute or could it be an attribute derived from RequiredAttribute ?

[ajcvickers]

@sjb-sjb Might work with a derived type. Have you tried it?