Detect non-nullable NRT properties only when both read and write are non-nullable

[roji]

For properties where read and write differs (i.e. [AllowNull]), our current NRT convention considers only read nullability; the following property is detected as non-nullable:

[AllowNull]
public string NonNullablePropertyAllowNull { get; set; }

I'm not sure how intentional this was (see test verifying this)... The logic may have been that since null isn't returned when reading, the property setter likely coalesces the given value to some non-null value (e.g. empty string). But the same logic can theoretically work on the read side as well (e.g. property getter can return null for empty string in the database).

We should consider changing to only mark properties as non-nullable if both read and write are non-nullable, requiring users to be explicit in other cases. This would be a breaking change, though I doubt lots of people are doing different read/write nullability on entity properties. Properties which are read-only or write-only would only need to be non-nullable on the supported operation, obviously.

[AndriySvyryd]

Since it is non-null on read then it will be persisted as non-null, so it makes sense to treat the column as required. This pattern could be useful when lazily cleaning nulls in store data. So we should keep current behavior.

[roji]

That's true, but as the property is written above, users can assign null to the property just fine (no compiler warning) but will then get an exception when doing SaveChanges; that seems like a bit of a pit of failure. Since this is all a bit ambiguous/arbitrary, it may be safer to err on the side of nullability (which is partially express via [AllowNull]), users can always explicitly define requiredness as they want.

I don't have very strong opinions here - will bring to design.

[AndriySvyryd]

That's true, but as the property is written above, users can assign null to the property just fine (no compiler warning) but will then get an exception when doing SaveChanges; that seems like a bit of a pit of failure.

I'd consider the above definition to be user error, so an exception is warranted. Also not making a breaking change is a plus.

[roji]

Well, if it really is a user error to define such a property (i.e. use [AllowNull]), wouldn't it be better to change what it means to make it actually useful? Disregarding the breaking change aspect for a second, isn't there more value for treating this as nullable by default?

(if we end up not doing this because it's breaking, that's fine - just want to make sure we're aligned on the reasoning)

[roji]

Design decision: we'll keep the current behavior to only look at the read side. Aside from the breaking change, it makes sense to annotate this way when a column default is defined in the database: you set the property to null, causing the default value to be generated.

The only downside is that without a column default (or other null-coalescing mechanism), this allows people to set null, but throwing at runtime - no compile-time protection. If we see people have trouble with this, we can consider a model validation warning.