Model validation of Dictionary<string, string> having null values for some key passes

[levroz]

Describe the bug

background: nullable annotation context is enabled at project level
Actual behavior: A string-to-string dictionary with null-values for some keys passes model validation
Expected behavior: it should fail the model validation

To Reproduce

1. Define an API-controller action that takes the below DTO as an argument
2. Pass the below JSON to is using PostMan or a similar tool.

public class Dto
{
	public IDictionary<string, string> Map { get; set; }
}

{ 
    "Map": { 
        "ok": "ok",
        "oktoo": "5",
        "notok": null
    }
}

Further technical details

• .NET Core 3.1
• Maybe related to API Validation error for Dictionary with null value since Preview 8 #13512
• See DictionaryModelBinderIntegrationTest.DictionaryModelBinder_DictionaryOfSimpleType_NullValue_DoesNotResultInRequiredValidation() - it validates the current behavior, which seems to me to be wrong, if nullable annotation context is enabled

[levroz]

Theory (wild guess):
It seems that #13551 has changed the behavior in a way that lets the model validation to pass in similar scenario (the mentioned scenario there was dictionary<string, object>), if nullable annotation context is disabled. It seems that as part of the fix, you have decided to let it pass regardless of the annotation context.

If you feel that this is the right behavior, please, help us to understand how should we annotate our Dto class or the 'Map' property in it to achieve what we want - non-nullability of keys and values.

[levroz]

@mkArtakMSFT, @pranavkm, this issue seems to be even wider - it addition to string dictionaries, it seems to also reproduce itself with the string lists. Namely, string lists with null values (which should never contain null values, if we enable nullable annotation context) pass model validation as well.

See below - the JSON has succesfully passes the model validation and the controller receives an invalid (containing null values in the 'List' property) instance of the DTO class...

JSON:
{ "List": ["ok", "oktoo", null] }

C#:
public class Dto { public IList<string> List { get; set; } }

The validation works correctly for simple nullable and not-nullable reference types - objects, strings, but seems to be completely broken for generic collections.

Should we start looking into implementing custom validators for string lists and string dictionaries? Do I miss something?

[pranavkm]

@levroz #13551 (comment) has a bit of background on why this doesn't work well generics. Inferring Required attributes is meant to supplement, not substitute being able to specify RequiredAttribute on targets. In most other cases, we'd recommend annotating the property or parameter with RequiredAttribute, but since there isn't a way to annotate generic type parameters, authoring a custom validator sounds like a reasonable solution.

[levroz]

@pranavkm , do I understand correctly, that we can not rely on ASP.NET core model validation's ability to fail the validation in case of 'null' entries entering to any generic collection with items of not-nullable reference type? So, for any such collection, we need to create a custom validator? List - custom validator, Dictionary<string, string> - custom validator? Do I understand you answer correctly?

[levroz]

@pranavkm, @mkArtakMSFT, don't you feel that this makes the model validation functionality completely unusable for projects utilizing nullable-reference types functionality of C#8?

[pranavkm]

So, for any such collection, we need to create a custom validator?

If it's critical to your scenario to ensure values in a collection are non-null, I would recommend doing so.

[levroz]

@pranavkm, if it would not be critical, the customer would not enable the nullable annotation context. That's what I am saying, the model validation is broken for generic collections under enabled nullable annotation contexts. @mkArtakMSFT, this is not an enhancement - this is a serious regression.

[levroz]

@pranavkm, @mkArtakMSFT,

I would like to stress one point here - situation, where ASP.NET core model validation allows to pass null-values to not-nullable reference types is a bug. A serious bug. It breaks services relying on not-nullability of their model. But even more importantly, it breaks them in an unpredictable and difficult to detect way - crashes, data corruption, you name it.

The only reason why the ASP.NET core team doesn't receive multiple mails from the customers complaining about this issue now, is the relatively small amount of engineering organizations that have already adapted the nullable refence types functionality of C# 8. The moment the amount of projects relying on enabled nullable annotation context will grow above some threshold, this will change.

Please prioritize this as a high-severity bug and fix.