Skip to content
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

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

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 2 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
5 changes: 4 additions & 1 deletion 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 @@ -521,4 +521,7 @@
<data name="CannotPopulateCollection" xml:space="preserve">
<value>The collection type '{0}' is abstract, an interface, or is read only, and could not be instantiated and populated.</value>
</data>
</root>
<data name="KeyValuePairPropertyNameInvalid" xml:space="preserve">
<value>The naming policy '{0}' returned an invalid value for a 'KeyValuePair' property name. The value cannot be 'null', cannot be a case-insensitive match for the literal 'Value' when converting the 'Key' property name, and cannot be a case-insensitive match for the literal 'Key' when converting the 'Value' property name.</value>
</data>
</root>
91 changes: 66 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,55 @@
// 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
Member

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

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 ||
string.Equals(_keyName, ValueNameCLR, StringComparison.OrdinalIgnoreCase) ||
string.Equals(_valueName, KeyNameCLR, StringComparison.OrdinalIgnoreCase))
{
ThrowHelper.ThrowInvalidOperationException_KeyValuePairPropertyNameInvalid(namingPolicy);
}
}

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 +76,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 +104,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 +134,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) { }
}
}
7 changes: 7 additions & 0 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 @@ -168,6 +168,13 @@ public static void ThrowInvalidOperationException_SerializerDictionaryKeyNull(Ty
throw new InvalidOperationException(SR.Format(SR.SerializerDictionaryKeyNull, policyType));
}

[DoesNotReturn]
[MethodImpl(MethodImplOptions.NoInlining)]
public static void ThrowInvalidOperationException_KeyValuePairPropertyNameInvalid(JsonNamingPolicy namingPolicy)
{
throw new InvalidOperationException(SR.Format(SR.KeyValuePairPropertyNameInvalid, namingPolicy.GetType()));
}

[DoesNotReturn]
public static void ThrowInvalidOperationException_SerializerConverterFactoryReturnsNull(Type converterType)
{
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