Runtime: What is the correct way to use Dictionary with FormUrlEncodedContent in .NET 5 without compiler warnings?

[martincostello]

As part of updating some OAuth provider libraries to target .NET 5 using preview 6 (see https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/pull/438), I've come across an issue with new nullable annotations I'm not sure of how to resolve correctly.

The FormUrlEncodedContent type has been updated to add nullable annotations (see #33268) and now the constructor takes an IEnumerable<KeyValuePair<string?, string?>>.

A common existing pattern to populate the values for an instance of FormUrlEncodedContent was to use a Dictionary<string, string>, but that now generates an CS8620 warning.

However simply changing the dictionary to use string? then generates CS8714 because the key cannot be null.

It's possible to fix the warning by casting the dictionary to IEnumerable<KeyValuePair<string?, string?>>, but then Visual Studio suggests the cast is redundant and can be removed.

The redundant cast and warnings can be eliminated by using a list or array of KeyValuePair<string?, string?>, but that's much more verbose.

Below is a simple program that demonstrates different approaches I took to try and fix the warning.

using System.Collections.Generic;
using System.Net.Http;

#nullable enable

namespace FormUrlEncodedContentNullable
{
    class Program
    {
        static void Main()
        {
            var dictionary = new Dictionary<string, string>
            {
                ["foo"] = "bar"
            };

            _ = new FormUrlEncodedContent(dictionary); // CS8620
            _ = new FormUrlEncodedContent((IEnumerable<KeyValuePair<string?, string?>>)dictionary); // OK, but says cast is redundant

            var nullableValueDictionary = new Dictionary<string, string?>
            {
                ["foo"] = "bar"
            };

            _ = new FormUrlEncodedContent(nullableValueDictionary); // CS8620

            var nullableDictionary = new Dictionary<string?, string?> // CS8714
            {
                ["foo"] = "bar"
            };

            _ = new FormUrlEncodedContent(nullableDictionary);

            var listOfKvps = new List<KeyValuePair<string?, string?>>
            {
                new KeyValuePair<string?, string?>("foo", "bar")
            };

            _ = new FormUrlEncodedContent(listOfKvps); // OK, but verbose

            var arrayOfKvps = new[]
            {
                new KeyValuePair<string?, string?>("foo", "bar")
            };

            _ = new FormUrlEncodedContent(arrayOfKvps); // OK, but verbose
        }
    }
}

For now I've settled with the cast, but none of the above approaches seem correct to me.

What is the "correct" fix for this code pattern for .NET 5 when nullable is in use? It doesn't seem to be resolvable without suppressing warnings or making the code a lot more verbose that it was before.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[msftbot[bot]]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[aik-jahoda]

I see no reason why support null for keys. Values are discussable too. Instead on null key the key should be an empty string.

With the assumption made above it should be:
```c#
var nullableDictionary = new Dictionary
{
["foo"] = "bar"
};

_ = new FormUrlEncodedContent(nullableDictionary);
```

Proposal for change in libraries: #40267

[karelz]

Fix attempted in #40267, but reverted by #40711

[ManickaP]

Changing nullability of the existing IEnumerable<KeyValuePair<string?, string?>> API is not a correct solution. We internally handle nulls (in keys as well as in values) and encode them as String.Empty, see method Encode. This has been allowed in previous versions on .NET Core. By changing it, we might actually break someone.

Proper solution to this problem could be to introduce a new ctor overload accepting Dictionary<string, string>. But that requires API review and probably more discussion.

Moving to 6.0.

[martincostello]

So for .NET 5.0, what is the official recommendation to fix compiler warnings/errors caused by these new nullability annotations for existing uses of Dictionary as the value to the constructor?

[ManickaP]

Use the cast for now. Once, the API is in, you would just remove it.
BTW, I just tried that and I'm not getting any warnings about redundant cast.

[dotMorten]

I'm finding I have to significantly rewrite my code because of this issue. I can't just add a ? to my key for dictionary as that isn't allowed, so now have to use IEnumerable or List of KeyValuePair<string?,string?>.
I don't even get how a nullable key for FormUrlEncodedContent makes sense ?

[ManickaP]

@dotMorten you don't have to rewrite your code everywhere. You only need to use explicitly cast when initializing the content, for instance:
C# new FormUrlEncodedContent((IEnumerable<KeyValuePair<string?, string?>>)dictionary);

My proposition is to add a new constructor accepting Dictionary/IDictionary instead of changing the current API.