Skip to content

Honor PropertyNamingPolicy, PropertyNameCaseInsensitive, & Encoder options when (de)serializing KeyValuePair instances #36424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 15, 2020
Merged

Honor PropertyNamingPolicy, PropertyNameCaseInsensitive, & Encoder options when (de)serializing KeyValuePair instances #36424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f876095
Move serializer KeyValuePair tests to dedicated file
layomia May 13, 2020
3b44403
Honor PropertyNamingPolicy, PropertyNameCaseInsensitive, & Encoder op…
layomia May 13, 2020
aea91eb
Remove unnessary guard against switched property names
layomia May 14, 2020
ee45b87
Merge remote-tracking branch 'upstream/master' into kvp_policy
layomia May 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libraries/System.Text.Json/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,8 +396,8 @@
<data name="SerializationConverterWrite" xml:space="preserve">
<value>The converter '{0}' wrote too much or not enough.</value>
</data>
<data name="SerializerDictionaryKeyNull" xml:space="preserve">
<value>The dictionary key policy '{0}' cannot return a null key.</value>
<data name="NamingPolicyReturnNull" xml:space="preserve">
<value>The naming policy '{0}' cannot return null.</value>
Copy link
Member

@jozkee jozkee May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include the naming policy in the message for the PropertyNamingPolicy used on CLR propertiesas well? Right now it says The JSON property name for '{0}.{1}' cannot be null..

Copy link
Contributor Author

@layomia layomia May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we could throw a more specific message (in a clean up PR), although right now it shouldn't be hard for a caller to tell what caused an exception with this message.

</data>
<data name="SerializationDuplicateAttribute" xml:space="preserve">
<value>The attribute '{0}' cannot exist more than once on '{1}'.</value>
Expand Down
2 changes: 1 addition & 1 deletion ...on/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ protected string GetKeyName(string key, ref WriteStack state, JsonSerializerOpti

if (key == null)
{
ThrowHelper.ThrowInvalidOperationException_SerializerDictionaryKeyNull(options.DictionaryKeyPolicy.GetType());
ThrowHelper.ThrowInvalidOperationException_NamingPolicyReturnNull(options.DictionaryKeyPolicy);
}
}

Expand Down
88 changes: 63 additions & 25 deletions ...em.Text.Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,52 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Text.Encodings.Web;

namespace System.Text.Json.Serialization.Converters
{
internal sealed class KeyValuePairConverter<TKey, TValue> : JsonValueConverter<KeyValuePair<TKey, TValue>>
{
private const string KeyName = "Key";
private const string ValueName = "Value";
private const string KeyNameCLR = "Key";
private const string ValueNameCLR = "Value";

// todo: https://github.com/dotnet/runtime/issues/1197
// move these to JsonSerializerOptions and use the proper encoding.
private static readonly JsonEncodedText _keyName = JsonEncodedText.Encode(KeyName, encoder: null);
private static readonly JsonEncodedText _valueName = JsonEncodedText.Encode(ValueName, encoder: null);
// Property name for "Key" and "Value" with Options.PropertyNamingPolicy applied.
private string _keyName = null!;
private string _valueName = null!;

// _keyName and _valueName as JsonEncodedText.
private JsonEncodedText _keyNameEncoded;
private JsonEncodedText _valueNameEncoded;

// todo: https://github.com/dotnet/runtime/issues/32352
// it is possible to cache the underlying converters since this is an internal converter and
Copy link
Contributor

@steveharter steveharter May 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment pertains to looking up the System.String converter and caching that. This is in case someone changes the converter.

IMO we should either fix it in this PR\pass or close the issue for 5.0 if we don't want to forward to the "proper" converter.

Copy link
Contributor Author

@layomia layomia May 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll prioritize making a PR to close this.

// an instance is created only once for each JsonSerializerOptions instance.

internal override void Initialize(JsonSerializerOptions options)
{
JsonNamingPolicy? namingPolicy = options.PropertyNamingPolicy;

if (namingPolicy == null)
{
_keyName = KeyNameCLR;
_valueName = ValueNameCLR;
}
else
{
_keyName = namingPolicy.ConvertName(KeyNameCLR);
_valueName = namingPolicy.ConvertName(ValueNameCLR);

if (_keyName == null || _valueName == null)
{
ThrowHelper.ThrowInvalidOperationException_NamingPolicyReturnNull(namingPolicy);
Copy link
Member

@jozkee jozkee May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider using ThrowInvalidOperationException_SerializerPropertyNameNull?

Copy link
Contributor Author

@layomia layomia May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did. It's achievable with some refactoring. We don't create a JsonPropertyInfo for the "Key" and "Value" properties, so we'd have to pass the property names by hand.

}
}

JavaScriptEncoder? encoder = options.Encoder;
_keyNameEncoded = JsonEncodedText.Encode(_keyName, encoder);
_valueNameEncoded = JsonEncodedText.Encode(_valueName, encoder);
}

internal override bool OnTryRead(
ref Utf8JsonReader reader,
Type typeToConvert, JsonSerializerOptions options,
Expand All @@ -44,17 +73,19 @@ internal override bool OnTryRead(
ThrowHelper.ThrowJsonException();
}

bool caseInsensitiveMatch = options.PropertyNameCaseInsensitive;

string propertyName = reader.GetString()!;
if (propertyName == KeyName)
if (FoundKeyProperty(propertyName, caseInsensitiveMatch))
{
reader.ReadWithVerify();
k = JsonSerializer.Deserialize<TKey>(ref reader, options, ref state, KeyName);
k = JsonSerializer.Deserialize<TKey>(ref reader, options, ref state, _keyName);
keySet = true;
}
else if (propertyName == ValueName)
else if (FoundValueProperty(propertyName, caseInsensitiveMatch))
{
reader.ReadWithVerify();
v = JsonSerializer.Deserialize<TValue>(ref reader, options, ref state, ValueName);
v = JsonSerializer.Deserialize<TValue>(ref reader, options, ref state, _valueName);
valueSet = true;
}
else
Expand All @@ -70,28 +101,21 @@ internal override bool OnTryRead(
}

propertyName = reader.GetString()!;
if (propertyName == KeyName)
if (!keySet && FoundKeyProperty(propertyName, caseInsensitiveMatch))
{
reader.ReadWithVerify();
k = JsonSerializer.Deserialize<TKey>(ref reader, options, ref state, KeyName);
keySet = true;
k = JsonSerializer.Deserialize<TKey>(ref reader, options, ref state, _keyName);
}
else if (propertyName == ValueName)
else if (!valueSet && FoundValueProperty(propertyName, caseInsensitiveMatch))
{
reader.ReadWithVerify();
v = JsonSerializer.Deserialize<TValue>(ref reader, options, ref state, ValueName);
valueSet = true;
v = JsonSerializer.Deserialize<TValue>(ref reader, options, ref state, _valueName);
}
else
{
ThrowHelper.ThrowJsonException();
}

if (!keySet || !valueSet)
{
ThrowHelper.ThrowJsonException();
}

reader.ReadWithVerify();

if (reader.TokenType != JsonTokenType.EndObject)
Expand All @@ -107,14 +131,28 @@ internal override bool OnTryWrite(Utf8JsonWriter writer, KeyValuePair<TKey, TVal
{
writer.WriteStartObject();

writer.WritePropertyName(_keyName);
JsonSerializer.Serialize(writer, value.Key, options, ref state, KeyName);
writer.WritePropertyName(_keyNameEncoded);
JsonSerializer.Serialize(writer, value.Key, options, ref state, _keyName);

writer.WritePropertyName(_valueName);
JsonSerializer.Serialize(writer, value.Value, options, ref state, ValueName);
writer.WritePropertyName(_valueNameEncoded);
JsonSerializer.Serialize(writer, value.Value, options, ref state, _valueName);

writer.WriteEndObject();
return true;
}

private bool FoundKeyProperty(string propertyName, bool caseInsensitiveMatch)
{
return propertyName == _keyName ||
(caseInsensitiveMatch && string.Equals(propertyName, _keyName, StringComparison.OrdinalIgnoreCase)) ||
propertyName == KeyNameCLR;
}

private bool FoundValueProperty(string propertyName, bool caseInsensitiveMatch)
{
return propertyName == _valueName ||
(caseInsensitiveMatch && string.Equals(propertyName, _valueName, StringComparison.OrdinalIgnoreCase)) ||
propertyName == ValueNameCLR;
}
}
}
2 changes: 2 additions & 0 deletions ....Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverterFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public override JsonConverter CreateConverter(Type type, JsonSerializerOptions o
args: null,
culture: null)!;

converter.Initialize(options);

return converter;
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,7 @@ internal bool ShouldFlush(Utf8JsonWriter writer, ref WriteStack state)
internal virtual bool ConstructorIsParameterized => false;

internal ConstructorInfo? ConstructorInfo { get; set; }

internal virtual void Initialize(JsonSerializerOptions options) { }
}
}
4 changes: 2 additions & 2 deletions src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,9 @@ public static void ThrowInvalidOperationException_SerializerPropertyNameNull(Typ

[DoesNotReturn]
[MethodImpl(MethodImplOptions.NoInlining)]
public static void ThrowInvalidOperationException_SerializerDictionaryKeyNull(Type policyType)
public static void ThrowInvalidOperationException_NamingPolicyReturnNull(JsonNamingPolicy namingPolicy)
{
throw new InvalidOperationException(SR.Format(SR.SerializerDictionaryKeyNull, policyType));
throw new InvalidOperationException(SR.Format(SR.NamingPolicyReturnNull, namingPolicy));
}

[DoesNotReturn]
Expand Down
161 changes: 0 additions & 161 deletions ...ries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Generic.Read.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,167 +962,6 @@ public static void ReadSimpleSortedSetT()
Assert.Equal(0, result.Count());
}

[Fact]
public static void ReadSimpleKeyValuePairFail()
{
// Invalid form: no Value
Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<KeyValuePair<string, int>>(@"{""Key"": 123}"));

// Invalid form: extra property
Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<KeyValuePair<string, int>>(@"{""Key"": ""Key"", ""Value"": 123, ""Value2"": 456}"));

// Invalid form: does not contain both Key and Value properties
Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<KeyValuePair<string, int>>(@"{""Key"": ""Key"", ""Val"": 123"));
}

[Fact]
public static void ReadListOfKeyValuePair()
{
List<KeyValuePair<string, int>> input = JsonSerializer.Deserialize<List<KeyValuePair<string, int>>>(@"[{""Key"": ""123"", ""Value"": 123},{""Key"": ""456"", ""Value"": 456}]");

Assert.Equal(2, input.Count);
Assert.Equal("123", input[0].Key);
Assert.Equal(123, input[0].Value);
Assert.Equal("456", input[1].Key);
Assert.Equal(456, input[1].Value);
}

[Fact]
public static void ReadKeyValuePairOfList()
{
KeyValuePair<string, List<int>> input = JsonSerializer.Deserialize<KeyValuePair<string, List<int>>>(@"{""Key"":""Key"", ""Value"":[1, 2, 3]}");

Assert.Equal("Key", input.Key);
Assert.Equal(3, input.Value.Count);
Assert.Equal(1, input.Value[0]);
Assert.Equal(2, input.Value[1]);
Assert.Equal(3, input.Value[2]);
}

[Theory]
[InlineData(@"{""Key"":""Key"", ""Value"":{""Key"":1, ""Value"":2}}")]
[InlineData(@"{""Key"":""Key"", ""Value"":{""Value"":2, ""Key"":1}}")]
[InlineData(@"{""Value"":{""Key"":1, ""Value"":2}, ""Key"":""Key""}")]
[InlineData(@"{""Value"":{""Value"":2, ""Key"":1}, ""Key"":""Key""}")]
public static void ReadKeyValuePairOfKeyValuePair(string json)
{
KeyValuePair<string, KeyValuePair<int, int>> input = JsonSerializer.Deserialize<KeyValuePair<string, KeyValuePair<int, int>>>(json);

Assert.Equal("Key", input.Key);
Assert.Equal(1, input.Value.Key);
Assert.Equal(2, input.Value.Value);
}

[Fact]
public static void ReadKeyValuePairWithNullValues()
{
{
KeyValuePair<string, string> kvp = JsonSerializer.Deserialize<KeyValuePair<string, string>>(@"{""Key"":""key"",""Value"":null}");
Assert.Equal("key", kvp.Key);
Assert.Null(kvp.Value);
}

{
KeyValuePair<string, object> kvp = JsonSerializer.Deserialize<KeyValuePair<string, object>>(@"{""Key"":""key"",""Value"":null}");
Assert.Equal("key", kvp.Key);
Assert.Null(kvp.Value);
}

{
KeyValuePair<string, SimpleClassWithKeyValuePairs> kvp = JsonSerializer.Deserialize<KeyValuePair<string, SimpleClassWithKeyValuePairs>>(@"{""Key"":""key"",""Value"":null}");
Assert.Equal("key", kvp.Key);
Assert.Null(kvp.Value);
}

{
KeyValuePair<string, KeyValuePair<string, string>> kvp = JsonSerializer.Deserialize<KeyValuePair<string, KeyValuePair<string, string>>>(@"{""Key"":""key"",""Value"":{""Key"":""key"",""Value"":null}}");
Assert.Equal("key", kvp.Key);
Assert.Equal("key", kvp.Value.Key);
Assert.Null(kvp.Value.Value);
}

{
KeyValuePair<string, KeyValuePair<string, object>> kvp = JsonSerializer.Deserialize<KeyValuePair<string, KeyValuePair<string, object>>>(@"{""Key"":""key"",""Value"":{""Key"":""key"",""Value"":null}}");
Assert.Equal("key", kvp.Key);
Assert.Equal("key", kvp.Value.Key);
Assert.Null(kvp.Value.Value);
}

{
KeyValuePair<string, KeyValuePair<string, SimpleClassWithKeyValuePairs>> kvp = JsonSerializer.Deserialize<KeyValuePair<string, KeyValuePair<string, SimpleClassWithKeyValuePairs>>>(@"{""Key"":""key"",""Value"":{""Key"":""key"",""Value"":null}}");
Assert.Equal("key", kvp.Key);
Assert.Equal("key", kvp.Value.Key);
Assert.Null(kvp.Value.Value);
}
}

[Fact]
public static void ReadClassWithNullKeyValuePairValues()
{
string json =
@"{" +
@"""KvpWStrVal"":{" +
@"""Key"":""key""," +
@"""Value"":null" +
@"}," +
@"""KvpWObjVal"":{" +
@"""Key"":""key""," +
@"""Value"":null" +
@"}," +
@"""KvpWClassVal"":{" +
@"""Key"":""key""," +
@"""Value"":null" +
@"}," +
@"""KvpWStrKvpVal"":{" +
@"""Key"":""key""," +
@"""Value"":{" +
@"""Key"":""key""," +
@"""Value"":null" +
@"}" +
@"}," +
@"""KvpWObjKvpVal"":{" +
@"""Key"":""key""," +
@"""Value"":{" +
@"""Key"":""key""," +
@"""Value"":null" +
@"}" +
@"}," +
@"""KvpWClassKvpVal"":{" +
@"""Key"":""key""," +
@"""Value"":{" +
@"""Key"":""key""," +
@"""Value"":null" +
@"}" +
@"}" +
@"}";
SimpleClassWithKeyValuePairs obj = JsonSerializer.Deserialize<SimpleClassWithKeyValuePairs>(json);

Assert.Equal("key", obj.KvpWStrVal.Key);
Assert.Equal("key", obj.KvpWObjVal.Key);
Assert.Equal("key", obj.KvpWClassVal.Key);
Assert.Equal("key", obj.KvpWStrKvpVal.Key);
Assert.Equal("key", obj.KvpWObjKvpVal.Key);
Assert.Equal("key", obj.KvpWClassKvpVal.Key);
Assert.Equal("key", obj.KvpWStrKvpVal.Value.Key);
Assert.Equal("key", obj.KvpWObjKvpVal.Value.Key);
Assert.Equal("key", obj.KvpWClassKvpVal.Value.Key);

Assert.Null(obj.KvpWStrVal.Value);
Assert.Null(obj.KvpWObjVal.Value);
Assert.Null(obj.KvpWClassVal.Value);
Assert.Null(obj.KvpWStrKvpVal.Value.Value);
Assert.Null(obj.KvpWObjKvpVal.Value.Value);
Assert.Null(obj.KvpWClassKvpVal.Value.Value);
}

[Fact]
public static void Kvp_NullKeyIsFine()
{
KeyValuePair<string, string> kvp = JsonSerializer.Deserialize<KeyValuePair<string, string>>(@"{""Key"":null,""Value"":null}");
Assert.Null(kvp.Key);
Assert.Null(kvp.Value);
}

[Fact]
public static void ReadSimpleTestClass_GenericCollectionWrappers()
{
Expand Down
Loading