From a06986b46fe54fae48d6658ceb55748830a8a9fc Mon Sep 17 00:00:00 2001 From: Sychev Vadim Date: Fri, 18 Jun 2021 21:44:45 +0300 Subject: [PATCH 1/4] Add DictionaryKeyPolicy support for EnumConverter [#47765] --- .../Converters/Value/EnumConverter.cs | 11 ++++- .../CollectionTests.Dictionary.KeyPolicy.cs | 43 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs index 64e891b5b0f1f..57464bbb86258 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs @@ -60,7 +60,16 @@ public EnumConverter(EnumConverterOptions converterOptions, JsonNamingPolicy? na T value = (T)values.GetValue(i)!; ulong key = ConvertToUInt64(value); - string name = names[i]; + string name; + + if (serializerOptions.DictionaryKeyPolicy != null) + { + name = serializerOptions.DictionaryKeyPolicy.ConvertName(names[i]); + } + else + { + name = names[i]; + } _nameCache.TryAdd( key, diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs index d2ea8ea547809..063889d61103f 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs @@ -184,6 +184,49 @@ public static void CustomNameSerialize() Assert.Equal(JsonCustomKey, json); } + public enum ETestEnum + { + TestValue1 = 1, + TestValue2 = 2, + } + + [Fact] + public static void EnumSerialization_DictionaryPolicy_Honored_CamelCase() + { + var options = new JsonSerializerOptions + { + DictionaryKeyPolicy = JsonNamingPolicy.CamelCase, + }; + + Dictionary dict = new Dictionary { [ETestEnum.TestValue1] = ETestEnum.TestValue1 }; + string value = JsonSerializer.Serialize(dict, options); + Assert.Equal("{\"testValue1\":1}", value); + + dict = new Dictionary { [ETestEnum.TestValue2] = ETestEnum.TestValue2 }; + value = JsonSerializer.Serialize(dict, options); + Assert.Equal("{\"testValue2\":2}", value); + + dict = new Dictionary { [ETestEnum.TestValue1] = ETestEnum.TestValue1, [ETestEnum.TestValue2] = ETestEnum.TestValue2 }; + value = JsonSerializer.Serialize(dict, options); + Assert.Equal("{\"testValue1\":1,\"testValue2\":2}", value); + } + + [Fact] + public static void EnumSerialization_DictionaryPolicy_Honored_None() + { + Dictionary dict = new Dictionary { [ETestEnum.TestValue1] = ETestEnum.TestValue1 }; + string value = JsonSerializer.Serialize(dict); + Assert.Equal("{\"TestValue1\":1}", value); + + dict = new Dictionary { [ETestEnum.TestValue2] = ETestEnum.TestValue2 }; + value = JsonSerializer.Serialize(dict); + Assert.Equal("{\"TestValue2\":2}", value); + + dict = new Dictionary { [ETestEnum.TestValue1] = ETestEnum.TestValue1, [ETestEnum.TestValue2] = ETestEnum.TestValue2 }; + value = JsonSerializer.Serialize(dict); + Assert.Equal("{\"TestValue1\":1,\"TestValue2\":2}", value); + } + [Fact] public static void NullNamePolicy() { From a6aebeca5d4e33659772f785767927893652f253 Mon Sep 17 00:00:00 2001 From: Sychev Vadim Date: Fri, 2 Jul 2021 02:22:06 +0300 Subject: [PATCH 2/4] Moved DictionaryKeyPolicy parsing code to WriteWithQuotes [#47765] --- .../Converters/Value/EnumConverter.cs | 62 +++++++++++++++---- .../CollectionTests.Dictionary.KeyPolicy.cs | 54 +++++++++++++++- 2 files changed, 103 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs index 57464bbb86258..37b0c215e9948 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs @@ -25,10 +25,14 @@ internal sealed class EnumConverter : JsonConverter private readonly ConcurrentDictionary _nameCache; + private readonly Lazy> _dictionaryKeyPolicyCache = new Lazy>(); + // This is used to prevent flooding the cache due to exponential bitwise combinations of flags. // Since multiple threads can add to the cache, a few more values might be added. private const int NameCacheSizeSoftLimit = 64; + private const int DictionaryKeyPolicyCacheSizeSoftLimit = 64; + public override bool CanConvert(Type type) { return type.IsEnum; @@ -60,16 +64,7 @@ public EnumConverter(EnumConverterOptions converterOptions, JsonNamingPolicy? na T value = (T)values.GetValue(i)!; ulong key = ConvertToUInt64(value); - string name; - - if (serializerOptions.DictionaryKeyPolicy != null) - { - name = serializerOptions.DictionaryKeyPolicy.ConvertName(names[i]); - } - else - { - name = names[i]; - } + string name = names[i]; _nameCache.TryAdd( key, @@ -334,13 +329,56 @@ internal override void WriteWithQuotes(Utf8JsonWriter writer, T value, JsonSeria ulong key = ConvertToUInt64(value); - if (_nameCache.TryGetValue(key, out JsonEncodedText formatted)) + JsonEncodedText formatted; + string original; + + if (options.DictionaryKeyPolicy != null && !state.Current.IgnoreDictionaryKeyPolicy) + { + if (_dictionaryKeyPolicyCache.Value.TryGetValue(key, out formatted)) + { + writer.WritePropertyName(formatted); + return; + } + + original = value.ToString(); + + if (IsValidIdentifier(original)) + { + original = options.DictionaryKeyPolicy.ConvertName(original); + + if (original == null) + { + ThrowHelper.ThrowInvalidOperationException_NamingPolicyReturnNull(options.DictionaryKeyPolicy); + } + + if (_dictionaryKeyPolicyCache.Value.Count < DictionaryKeyPolicyCacheSizeSoftLimit) + { + JavaScriptEncoder? encoder = options.Encoder; + + formatted = JsonEncodedText.Encode(original, encoder); + + writer.WritePropertyName(formatted); + + _dictionaryKeyPolicyCache.Value.TryAdd(key, formatted); + } + else + { + // We also do not create a JsonEncodedText instance here because passing the string + // directly to the writer is cheaper than creating one and not caching it for reuse. + writer.WritePropertyName(original); + } + + return; + } + } + + if (_nameCache.TryGetValue(key, out formatted)) { writer.WritePropertyName(formatted); return; } - string original = value.ToString(); + original = value.ToString(); if (IsValidIdentifier(original)) { // We are dealing with a combination of flag constants since diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs index 063889d61103f..3d8b705889d80 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs @@ -189,7 +189,7 @@ public enum ETestEnum TestValue1 = 1, TestValue2 = 2, } - + [Fact] public static void EnumSerialization_DictionaryPolicy_Honored_CamelCase() { @@ -227,6 +227,58 @@ public static void EnumSerialization_DictionaryPolicy_Honored_None() Assert.Equal("{\"TestValue1\":1,\"TestValue2\":2}", value); } + public class ClassWithEnumProperties + { + public ETestEnum TestEnumProperty1 { get; } = ETestEnum.TestValue2; + public DayOfWeek TestEnumProperty2 { get; } = DayOfWeek.Monday; + } + + [Fact] + public static void EnumSerialization_DictionaryPolicy_NotApplied_WhenEnumsAreSerialized() + { + var options = new JsonSerializerOptions + { + DictionaryKeyPolicy = JsonNamingPolicy.CamelCase, + }; + + string value = JsonSerializer.Serialize(DayOfWeek.Friday, options); + + Assert.Equal("5", value); + + value = JsonSerializer.Serialize(ETestEnum.TestValue2, options); + + Assert.Equal("2", value); + + + value = JsonSerializer.Serialize(new ClassWithEnumProperties(), options); + + Assert.Equal("{\"TestEnumProperty1\":2,\"TestEnumProperty2\":1}", value); + + value = JsonSerializer.Serialize(new List { DayOfWeek.Sunday, DayOfWeek.Monday, DayOfWeek.Tuesday, DayOfWeek.Wednesday, DayOfWeek.Thursday, DayOfWeek.Friday, DayOfWeek.Saturday}, options); + + Assert.Equal("[0,1,2,3,4,5,6]", value); + } + + public class CustomJsonNamingPolicy : JsonNamingPolicy + { + public override string ConvertName(string name) => null; + } + + [Fact] + public static void EnumSerialization_DictionaryPolicy_ThrowsException_WhenNamingPolicyReturnsNull() + { + var options = new JsonSerializerOptions + { + DictionaryKeyPolicy = new CustomJsonNamingPolicy(), + }; + + Dictionary dict = new Dictionary { [ETestEnum.TestValue1] = ETestEnum.TestValue1 }; + + InvalidOperationException ex = Assert.Throws(() => JsonSerializer.Serialize(dict, options)); + + Assert.Contains(typeof(CustomJsonNamingPolicy).ToString(), ex.Message); + } + [Fact] public static void NullNamePolicy() { From 235c7ef7f0f4a7bd4f64165784949cecbc1c3c83 Mon Sep 17 00:00:00 2001 From: Sychev Vadim Date: Tue, 27 Jul 2021 18:00:28 +0300 Subject: [PATCH 3/4] Refactored the bugfix in accordance to the comments (#47765) --- .../Converters/Value/EnumConverter.cs | 84 ++++++++++--------- 1 file changed, 45 insertions(+), 39 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs index 37b0c215e9948..a3c095435ae3d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs @@ -25,14 +25,12 @@ internal sealed class EnumConverter : JsonConverter private readonly ConcurrentDictionary _nameCache; - private readonly Lazy> _dictionaryKeyPolicyCache = new Lazy>(); + private ConcurrentDictionary? _dictionaryKeyPolicyCache; // This is used to prevent flooding the cache due to exponential bitwise combinations of flags. // Since multiple threads can add to the cache, a few more values might be added. private const int NameCacheSizeSoftLimit = 64; - private const int DictionaryKeyPolicyCacheSizeSoftLimit = 64; - public override bool CanConvert(Type type) { return type.IsEnum; @@ -329,20 +327,31 @@ internal override void WriteWithQuotes(Utf8JsonWriter writer, T value, JsonSeria ulong key = ConvertToUInt64(value); - JsonEncodedText formatted; - string original; + //Try to obtain values from caches + if (options.DictionaryKeyPolicy != null) + { + Debug.Assert(!state.Current.IgnoreDictionaryKeyPolicy); - if (options.DictionaryKeyPolicy != null && !state.Current.IgnoreDictionaryKeyPolicy) + if (_dictionaryKeyPolicyCache != null && _dictionaryKeyPolicyCache.TryGetValue(key, out JsonEncodedText formatted)) + { + writer.WritePropertyName(formatted); + return; + } + } + else { - if (_dictionaryKeyPolicyCache.Value.TryGetValue(key, out formatted)) + if (_nameCache.TryGetValue(key, out JsonEncodedText formatted)) { writer.WritePropertyName(formatted); return; } + } - original = value.ToString(); - - if (IsValidIdentifier(original)) + //if there are not cached values + string original = value.ToString(); + if (IsValidIdentifier(original)) + { + if (options.DictionaryKeyPolicy != null) { original = options.DictionaryKeyPolicy.ConvertName(original); @@ -351,15 +360,20 @@ internal override void WriteWithQuotes(Utf8JsonWriter writer, T value, JsonSeria ThrowHelper.ThrowInvalidOperationException_NamingPolicyReturnNull(options.DictionaryKeyPolicy); } - if (_dictionaryKeyPolicyCache.Value.Count < DictionaryKeyPolicyCacheSizeSoftLimit) + if (_dictionaryKeyPolicyCache == null) + { + _dictionaryKeyPolicyCache = new ConcurrentDictionary(); + } + + if (_dictionaryKeyPolicyCache.Count < NameCacheSizeSoftLimit) { JavaScriptEncoder? encoder = options.Encoder; - formatted = JsonEncodedText.Encode(original, encoder); + JsonEncodedText formatted = JsonEncodedText.Encode(original, encoder); writer.WritePropertyName(formatted); - _dictionaryKeyPolicyCache.Value.TryAdd(key, formatted); + _dictionaryKeyPolicyCache.TryAdd(key, formatted); } else { @@ -370,37 +384,29 @@ internal override void WriteWithQuotes(Utf8JsonWriter writer, T value, JsonSeria return; } - } - - if (_nameCache.TryGetValue(key, out formatted)) - { - writer.WritePropertyName(formatted); - return; - } + else + { + // We are dealing with a combination of flag constants since + // all constant values were cached during warm-up. + JavaScriptEncoder? encoder = options.Encoder; - original = value.ToString(); - if (IsValidIdentifier(original)) - { - // We are dealing with a combination of flag constants since - // all constant values were cached during warm-up. - JavaScriptEncoder? encoder = options.Encoder; + if (_nameCache.Count < NameCacheSizeSoftLimit) + { + JsonEncodedText formatted = JsonEncodedText.Encode(original, encoder); - if (_nameCache.Count < NameCacheSizeSoftLimit) - { - formatted = JsonEncodedText.Encode(original, encoder); + writer.WritePropertyName(formatted); - writer.WritePropertyName(formatted); + _nameCache.TryAdd(key, formatted); + } + else + { + // We also do not create a JsonEncodedText instance here because passing the string + // directly to the writer is cheaper than creating one and not caching it for reuse. + writer.WritePropertyName(original); + } - _nameCache.TryAdd(key, formatted); - } - else - { - // We also do not create a JsonEncodedText instance here because passing the string - // directly to the writer is cheaper than creating one and not caching it for reuse. - writer.WritePropertyName(original); + return; } - - return; } switch (s_enumTypeCode) From 718fc714c6258b9359c50723ff4f39c4419d2d47 Mon Sep 17 00:00:00 2001 From: Sychev Vadim Date: Wed, 28 Jul 2021 00:18:09 +0300 Subject: [PATCH 4/4] Made few minor corrections (#47765) --- .../Converters/Value/EnumConverter.cs | 24 ++++++++----------- .../CollectionTests.Dictionary.KeyPolicy.cs | 2 +- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs index a3c095435ae3d..b4e666bc96b8a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs @@ -327,7 +327,7 @@ internal override void WriteWithQuotes(Utf8JsonWriter writer, T value, JsonSeria ulong key = ConvertToUInt64(value); - //Try to obtain values from caches + // Try to obtain values from caches if (options.DictionaryKeyPolicy != null) { Debug.Assert(!state.Current.IgnoreDictionaryKeyPolicy); @@ -338,16 +338,14 @@ internal override void WriteWithQuotes(Utf8JsonWriter writer, T value, JsonSeria return; } } - else + else if (_nameCache.TryGetValue(key, out JsonEncodedText formatted)) { - if (_nameCache.TryGetValue(key, out JsonEncodedText formatted)) - { - writer.WritePropertyName(formatted); - return; - } + writer.WritePropertyName(formatted); + return; } - //if there are not cached values + + // if there are not cached values string original = value.ToString(); if (IsValidIdentifier(original)) { @@ -360,10 +358,7 @@ internal override void WriteWithQuotes(Utf8JsonWriter writer, T value, JsonSeria ThrowHelper.ThrowInvalidOperationException_NamingPolicyReturnNull(options.DictionaryKeyPolicy); } - if (_dictionaryKeyPolicyCache == null) - { - _dictionaryKeyPolicyCache = new ConcurrentDictionary(); - } + _dictionaryKeyPolicyCache ??= new ConcurrentDictionary(); if (_dictionaryKeyPolicyCache.Count < NameCacheSizeSoftLimit) { @@ -386,8 +381,9 @@ internal override void WriteWithQuotes(Utf8JsonWriter writer, T value, JsonSeria } else { - // We are dealing with a combination of flag constants since - // all constant values were cached during warm-up. + // We might be dealing with a combination of flag constants since all constant values were + // likely cached during warm - up(assuming the number of constants <= NameCacheSizeSoftLimit). + JavaScriptEncoder? encoder = options.Encoder; if (_nameCache.Count < NameCacheSizeSoftLimit) diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs index 3d8b705889d80..e933f510054d4 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CollectionTests/CollectionTests.Dictionary.KeyPolicy.cs @@ -212,7 +212,7 @@ public static void EnumSerialization_DictionaryPolicy_Honored_CamelCase() } [Fact] - public static void EnumSerialization_DictionaryPolicy_Honored_None() + public static void EnumSerializationAsDictKey_NoDictionaryKeyPolicy() { Dictionary dict = new Dictionary { [ETestEnum.TestValue1] = ETestEnum.TestValue1 }; string value = JsonSerializer.Serialize(dict);