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

Jump to bottom

Add logic to properly honor naming policy when serializing flag enums #36726

Merged
merged 6 commits into from
Jun 3, 2020
Merged

Add logic to properly honor naming policy when serializing flag enums #36726

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d21c3f9
Add logic to properly honor naming policy when serializing flag enums
layomia May 19, 2020
060f478
Cache JsonEncodedText, add optimization to-do, and use .Contains
layomia May 20, 2020
fc21885
Fix CI failure
layomia May 20, 2020
39df64f
Apply feedback, use enum value as lookup key, cache result even when …
layomia May 20, 2020
7c93170
Use ulong as name cache key and perform caching at warm up
layomia May 28, 2020
e10b878
Address feedback - set cap for name cache, compute cache key properly…
layomia Jun 2, 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
60 changes: 43 additions & 17 deletions ...ies/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Concurrent;
using System.Diagnostics;
using System.Globalization;
using System.Runtime.CompilerServices;

Expand All @@ -16,9 +17,11 @@ internal class EnumConverter<T> : JsonConverter<T>
// Odd type codes are conveniently signed types (for enum backing types).
private static readonly string? s_negativeSign = ((int)s_enumTypeCode % 2) == 0 ? null : NumberFormatInfo.CurrentInfo.NegativeSign;
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's incorrect to cache any thread-local state (such as information about the thread-current culture) into a global static. The end result of this is that the very first time this code is executed, the executing thread's negative sign will be read and will be applied to all subsequent operations, regardless of what culture the other threads are running as.

If you wanted this to be properly culture-aware, you need to query the current thread's negative sign on every single call to IsValidIdentifier.

Copy link
Member

Choose a reason for hiding this comment

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

This would be incorrect even if it was a thread-static, of course.


private const string ValueSeparator = ", ";

private readonly EnumConverterOptions _converterOptions;
private readonly JsonNamingPolicy _namingPolicy;
private readonly ConcurrentDictionary<string, string>? _nameCache;
private readonly JsonNamingPolicy? _namingPolicy;
private readonly ConcurrentDictionary<T, JsonEncodedText> _nameCache = new ConcurrentDictionary<T, JsonEncodedText>();
layomia marked this conversation as resolved.
Show resolved Hide resolved

public override bool CanConvert(Type type)
{
Expand All @@ -33,14 +36,6 @@ public EnumConverter(EnumConverterOptions options)
public EnumConverter(EnumConverterOptions options, JsonNamingPolicy? namingPolicy)
{
_converterOptions = options;
if (namingPolicy != null)
{
_nameCache = new ConcurrentDictionary<string, string>();
}
else
{
namingPolicy = JsonNamingPolicy.Default;
}
_namingPolicy = namingPolicy;
}

Expand Down Expand Up @@ -157,21 +152,21 @@ public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions
// If strings are allowed, attempt to write it out as a string value
if (_converterOptions.HasFlag(EnumConverterOptions.AllowStrings))
{
string original = value.ToString();
if (_nameCache != null && _nameCache.TryGetValue(original, out string? transformed))
if (_nameCache.TryGetValue(value, out JsonEncodedText transformed))
{
writer.WriteStringValue(transformed);
return;
}

string original = value.ToString();
if (IsValidIdentifier(original))
{
transformed = _namingPolicy.ConvertName(original);
transformed = _namingPolicy == null
? JsonEncodedText.Encode(original, options.Encoder)
: FormatEnumValue(original, options);

writer.WriteStringValue(transformed);
if (_nameCache != null)
{
_nameCache.TryAdd(original, transformed);
}
_nameCache.TryAdd(value, transformed);
return;
}
}
Expand Down Expand Up @@ -212,5 +207,36 @@ public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions
break;
}
}

private JsonEncodedText FormatEnumValue(string value, JsonSerializerOptions options)
{
Debug.Assert(_namingPolicy != null);
string converted;

if (!value.Contains(ValueSeparator))
layomia marked this conversation as resolved.
Show resolved Hide resolved
{
converted = _namingPolicy.ConvertName(value);
}
else
{
// todo: optimize implementation here by leveraging https://github.com/dotnet/runtime/issues/934.
string[] enumValues = value.Split(
#if BUILDING_INBOX_LIBRARY
ValueSeparator
#else
new string[] { ValueSeparator }, StringSplitOptions.None
#endif
);

for (int i = 0; i < enumValues.Length; i++)
{
enumValues[i] = _namingPolicy.ConvertName(enumValues[i]);
}

converted = string.Join(ValueSeparator, enumValues);
}

return JsonEncodedText.Encode(converted, options.Encoder);
}
}
}
15 changes: 15 additions & 0 deletions src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,21 @@ public void ConvertFileAttributes()
options = new JsonSerializerOptions();
options.Converters.Add(new JsonStringEnumConverter(allowIntegerValues: false));
Assert.Throws<JsonException>(() => JsonSerializer.Serialize((FileAttributes)(-1), options));

// Flag values honor naming policy correctly
options = new JsonSerializerOptions();
options.Converters.Add(new JsonStringEnumConverter(new SimpleSnakeCasePolicy()));

json = JsonSerializer.Serialize(
FileAttributes.Directory | FileAttributes.Compressed | FileAttributes.IntegrityStream,
options);
Assert.Equal(@"""directory, compressed, integrity_stream""", json);

json = JsonSerializer.Serialize(FileAttributes.Compressed & FileAttributes.Device, options);
Assert.Equal(@"0", json);

json = JsonSerializer.Serialize(FileAttributes.Directory & FileAttributes.Compressed | FileAttributes.IntegrityStream, options);
Assert.Equal(@"""integrity_stream""", json);
}

public class FileState
Expand Down