Runtime: KeyValuePair is not properly serialized with JsonNamingPolicy.CamelCase - System.Text.Json

[untaught]

When serializing a collection of KeyValuePair with CamelCase enabled the properties are serialized in PascalCase:

MVC Configuration is:
```c#
.AddJsonOptions(options =>
{
options.JsonSerializerOptions.DictionaryKeyPolicy = JsonNamingPolicy.CamelCase;
options.JsonSerializerOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
}

The result is:

{ Key: "Test", Value: "Test1" }

Expected result:

{ key: "Test", value: "Test1" }
```

[ahsonkhan]

I believe the camel case option for DictionaryKeyPolicy only works for Dictionary/IDictionary (which KeyValuePair isn't).
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializeroptions.dictionarykeypolicy?view=netcore-3.0

https://github.com/dotnet/corefx/blob/ac99a1b7168bd32046a954c3f06012c0fa909bed/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs#L151-L160

@steveharter - can you clarify what your expectations are for that feature? Is there anything we can do to support this scenario?

I'd imagine this can be resolved by a custom converter.

cc @layomia

[steveharter]

This would require a new naming policy. I don't think we can expand DictionaryKeyPolicy to cover KeyValuePair since that would break existing users.

e.g. we'd need to add a KeyValuePairNamingPolicy

[steveharter]

As a work-around it is possible to create a new converter that does this by coping the converter code from JsonValueConverterKeyValuePair.cs and KeyValuePairConverter and modifying appropriately.

[ahsonkhan]

This would require a new naming policy.

Is KeyValuePair the only type that isn't covered by current options? Where else can the concept of "key" show up in .NET types? I don't know if adding a new policy just for a specific type makes sense (unless it happens to be a commonly used type/pattern). Alternatively, we have "KeyPolicy" which works for all "keys" (dictionary, keyvaluepair, etc.).

[steveharter]

Note that Dictionary<,> uses KeyValuePair<,> internally so a new policy KeyValuePairPolicy could be implied for both Dictionary and KeyValuePair.

[JamesNK]

{ Key: "Test", Value: "Test1" }

Key and Value are just properties on the struct. PropertyNamingPolicy should apply to them.

[ahsonkhan]

Key and Value are just properties on the struct. PropertyNamingPolicy should apply to them.

That's a good point. It looks like because we have a built-in converter for KeyValuePair, PropertyInfo is null when we try to determine the property name to emit and we bail out. When creating a JsonClassInfo, since ClassType is identified as Value, we don't reflect over the KVP object and get it's properties.

https://github.com/dotnet/corefx/blob/b832f6ccdebd18469e4b6d03c1c9b2c9c6c14374/src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs#L88-L114

For user-defined structs, that isn't the case, and we (correctly) go down the code-path which calls PropertyNamingPolicy.ConvertName

A trivial fix would be to update JsonValueConverterKeyValuePair to honor PropertyNamingPolicy but I don't think that's the right place to apply the fix (especially since it would force us to go back from JsonEncodedText to string). I think us identifying an object (like KVP, or any type that has a built-in converter) as a ClassType.Value is probably not the right approach and we should re-evaluate that code path.

https://github.com/dotnet/corefx/blob/b832f6ccdebd18469e4b6d03c1c9b2c9c6c14374/src/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonValueConverterKeyValuePair.cs#L113-L117

C# if (options.PropertyNamingPolicy != null) { // Not great, but would work string modifiedName = options.PropertyNamingPolicy.ConvertName(name.ToString()); writer.WritePropertyName(modifiedName); } else { writer.WritePropertyName(name); }

[ArrowCase]

@ahsonkhan I'm interested in seeing this fixed and can contribute.

What is the purpose of JsonValueConverterKeyValuePair? It seems like if it were removed, there would be no behavioral difference, except that PropertyNamingPolicy would start being respected.

If the converter is required for some reason, then its design is simply flawed, since it is not capable of respecting PropertyNamingPolicy while keeping statically cached property name values.

[ahsonkhan]

What is the purpose of JsonValueConverterKeyValuePair? It seems like if it were removed, there would be no behavioral difference, except that PropertyNamingPolicy would start being respected.

@steveharter, @layomia - can you please help here.

[ahsonkhan]

From @vladinvancouver in https://github.com/dotnet/corefx/issues/41813

CamelCase naming policy is not applied to list of KeyValuePair.

Code to reproduce:

JsonSerializerOptions options = new JsonSerializerOptions()
{
WriteIndented = true,
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
DictionaryKeyPolicy = JsonNamingPolicy.CamelCase
};

Dictionary dict = new Dictionary();
dict.Add("Item1", "Value1");
dict.Add("Item2", 22);

string json = JsonSerializer.Serialize(dict.ToList(), options);

Console.WriteLine(json);

Actual results:

[
{
"Key": "Item1",
"Value": "Value1"
},
{
"Key": "Item2",
"Value": 22
}
]

Expected results:

[
{
"key": "Item1",
"value": "Value1"
},
{
"key": "Item2",
"value": 22
}
]

[layomia]

What is the purpose of JsonValueConverterKeyValuePair? It seems like if it were removed, there would be no behavioral difference, except that PropertyNamingPolicy would start being respected.

The serializer only supports public properties with public getters and setters for deserialization. KeyValuePair's Key and Value properties are readonly (no setter), so we use a converter to create them with their constructor.

If the converter is required for some reason, then its design is simply flawed, since it is not capable of respecting PropertyNamingPolicy while keeping statically cached property name values.

A solution here might be to obtain and cache the correct Key and Value property names during the construction of the JsonKeyValuePairConverter factory, and pass those to each created instance of the JsonConverter<TKey, TValue> converters.

@ahsonkhan @ArrowCase

[ArrowCase]

@layomia If done at that point, we would only be able to include support for Default and CamelCase, correct? No support for user-defined policies, and no ability to automatically use new built-in policies that may be added in the future.

[layomia]

@ArrowCase options.PropertyNamingPolicy will have the specified naming policy. I'm imagining a new constructor for JsonKeyValuePairConverter which takes an options instance. The call to the constructor would look like this:

c# converters.Add(new JsonKeyValuePairConverter(this));

With this constructor, the JsonEncodedText values for Key and Value could be obtained from the policy, cached in the factory, then passed to created constructors in the CreateConverter method (this means adding a new constructor for the JsonValueKeyValuePairConverter<,>).

[ericstj]

@layomia can you please file a breaking change issue: https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

[ericstj]

Removing breaking change as it is tracked with https://github.com/dotnet/runtime/pull/36424