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 support for multiple non-string TKey on dictionaries #38056

Merged
merged 18 commits into from
Jul 7, 2020
Merged

Add support for multiple non-string TKey on dictionaries #38056

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f222f23
Add support for multiple non-string TKey on dictionaries
jozkee Jun 17, 2020
25e654b
Fix build errors
jozkee Jun 17, 2020
77b7a4a
Fix performance regression
jozkee Jun 19, 2020
225f27a
Merge branch 'master' of https://github.com/dotnet/runtime into tkey_…
jozkee Jun 19, 2020
6bcb25e
Add Read/WriteWithQuotes to the remaining supported types
jozkee Jun 22, 2020
c272684
Add support for a few more missing types
jozkee Jun 23, 2020
eb75a96
Add policy support for Enum keys
jozkee Jun 23, 2020
8d4fd95
Address performance regression.
jozkee Jun 24, 2020
f9b507c
Fix test error on netstandard
jozkee Jun 25, 2020
3f2a4e7
Merge branch 'master' of https://github.com/dotnet/runtime into tkey_…
jozkee Jun 25, 2020
578138e
1. Add support for escaped characters on Read and avoid escaping on W…
jozkee Jun 30, 2020
2d56bfb
Remove TryGet*Core methods to mitigate perf regression
jozkee Jun 30, 2020
637c68f
Remove duplicated escaping tests
jozkee Jun 30, 2020
7201ccb
Cache TKey and TValue converters
jozkee Jul 2, 2020
2b30299
Remove duplicated code used to read the dictionary key
jozkee Jul 2, 2020
ee17991
Bring back TryGet*Core
jozkee Jul 2, 2020
8cd787b
Fix test errors with floating point types
jozkee Jul 2, 2020
8b30e75
Address nits and feedback from steveharter
jozkee Jul 6, 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
8 changes: 4 additions & 4 deletions src/libraries/System.Text.Json/src/System.Text.Json.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ConcurrentQueueOfTConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ConcurrentStackOfTConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\DictionaryDefaultConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\DictionaryOfStringTValueConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\DictionaryOfTKeyTValueConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ICollectionOfTConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IDictionaryConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IDictionaryOfStringTValueConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IDictionaryOfTKeyTValueConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IEnumerableConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IEnumerableConverterFactory.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IEnumerableConverterFactoryHelpers.cs" />
Expand All @@ -77,9 +77,9 @@
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IEnumerableWithAddMethodConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IListConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IListOfTConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ImmutableDictionaryOfStringTValueConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ImmutableDictionaryOfTKeyTValueConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ImmutableEnumerableOfTConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IReadOnlyDictionaryOfStringTValueConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\IReadOnlyDictionaryOfTKeyTValueConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ISetOfTConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\JsonCollectionConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\JsonDictionaryConverter.cs" />
Expand Down
67 changes: 35 additions & 32 deletions ...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 @@ -10,13 +10,14 @@ namespace System.Text.Json.Serialization.Converters
/// <summary>
/// Default base class implementation of <cref>JsonDictionaryConverter{TCollection}</cref> .
/// </summary>
internal abstract class DictionaryDefaultConverter<TCollection, TValue>
internal abstract class DictionaryDefaultConverter<TCollection, TKey, TValue>
: JsonDictionaryConverter<TCollection>
where TKey : notnull
{
/// <summary>
/// When overridden, adds the value to the collection.
/// </summary>
protected abstract void Add(TValue value, JsonSerializerOptions options, ref ReadStack state);
protected abstract void Add(TKey key, TValue value, JsonSerializerOptions options, ref ReadStack state);
layomia marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// When overridden, converts the temporary collection held in state.Current.ReturnValue to the final collection.
Expand All @@ -30,6 +31,7 @@ protected virtual void ConvertCollection(ref ReadStack state, JsonSerializerOpti
protected virtual void CreateCollection(ref Utf8JsonReader reader, ref ReadStack state) { }

internal override Type ElementType => typeof(TValue);
internal override Type KeyType => typeof(TKey);

protected static JsonConverter<TValue> GetElementConverter(ref ReadStack state)
{
Expand All @@ -39,25 +41,21 @@ protected static JsonConverter<TValue> GetElementConverter(ref ReadStack state)
return converter;
}

protected string GetKeyName(string key, ref WriteStack state, JsonSerializerOptions options)
protected static JsonConverter<TValue> GetValueConverter(ref WriteStack state)
jozkee marked this conversation as resolved.
Show resolved Hide resolved
{
if (options.DictionaryKeyPolicy != null && !state.Current.IgnoreDictionaryKeyPolicy)
{
key = options.DictionaryKeyPolicy.ConvertName(key);

if (key == null)
{
ThrowHelper.ThrowInvalidOperationException_NamingPolicyReturnNull(options.DictionaryKeyPolicy);
}
}
JsonConverter<TValue> converter = (JsonConverter<TValue>)state.Current.JsonClassInfo.ElementClassInfo!.PropertyInfoForClassInfo.ConverterBase;
Debug.Assert(converter != null); // It should not be possible to have a null converter at this point.

return key;
return converter;
}

protected static JsonConverter<TValue> GetValueConverter(ref WriteStack state)
protected static JsonConverter<TKey> GetKeyConverter(JsonClassInfo classInfo)
{
JsonConverter<TValue> converter = (JsonConverter<TValue>)state.Current.DeclaredJsonPropertyInfo!.ConverterBase;
Debug.Assert(converter != null); // It should not be possible to have a null converter at this point.
var converter = (JsonConverter<TKey>)classInfo.KeyClassInfo!.PropertyInfoForClassInfo.ConverterBase;
if (!converter.CanBeDictionaryKey)
{
ThrowHelper.ThrowNotSupportedException_SerializationNotSupported(converter.TypeToConvert);
}

return converter;
}
Expand Down Expand Up @@ -97,12 +95,17 @@ internal sealed override bool OnTryRead(
// Read method would have thrown if otherwise.
Debug.Assert(reader.TokenType == JsonTokenType.PropertyName);

state.Current.JsonPropertyNameAsString = reader.GetString();
ReadOnlySpan<byte> propertyName = JsonSerializer.GetPropertyName(ref state, ref reader, options);
// Copy key name to JsonPropertyName for JSON Path support in case of error.
state.Current.JsonPropertyName = propertyName.ToArray();
jozkee marked this conversation as resolved.
Show resolved Hide resolved

JsonConverter<TKey> keyConverter = GetKeyConverter(state.Current.JsonClassInfo);
TKey key = keyConverter.ReadWithQuotes(propertyName);

// Read the value and add.
reader.ReadWithVerify();
TValue element = elementConverter.Read(ref reader, typeof(TValue), options);
Add(element!, options, ref state);
Add(key, element!, options, ref state);
}
}
else
Expand All @@ -121,13 +124,18 @@ internal sealed override bool OnTryRead(
// Read method would have thrown if otherwise.
Debug.Assert(reader.TokenType == JsonTokenType.PropertyName);

state.Current.JsonPropertyNameAsString = reader.GetString();
ReadOnlySpan<byte> propertyName = JsonSerializer.GetPropertyName(ref state, ref reader, options);
// Copy key name to JsonPropertyName for JSON Path support in case of error.
state.Current.JsonPropertyName = propertyName.ToArray();

JsonConverter<TKey> keyConverter = GetKeyConverter(state.Current.JsonClassInfo);
TKey key = keyConverter.ReadWithQuotes(propertyName);

reader.ReadWithVerify();

// Get the value from the converter and add it.
elementConverter.TryRead(ref reader, typeof(TValue), options, ref state, out TValue element);
Add(element!, options, ref state);
Add(key, element!, options, ref state);
}
}
}
Expand Down Expand Up @@ -212,17 +220,10 @@ internal sealed override bool OnTryRead(

state.Current.PropertyState = StackFramePropertyState.Name;

// Verify property doesn't contain metadata.
if (preserveReferences)
{
ReadOnlySpan<byte> propertyName = reader.GetSpan();
if (propertyName.Length > 0 && propertyName[0] == '$')
{
ThrowHelper.ThrowUnexpectedMetadataException(propertyName, ref reader, ref state);
}
}

state.Current.JsonPropertyNameAsString = reader.GetString();
ReadOnlySpan<byte> propertyName = JsonSerializer.GetPropertyName(ref state, ref reader, options);
state.Current.DictionaryKeyName = propertyName.ToArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is only used for continuation cases...

// Copy key name to JsonPropertyName for JSON Path support in case of error.
state.Current.JsonPropertyName = state.Current.DictionaryKeyName;
}

if (state.Current.PropertyState < StackFramePropertyState.ReadValue)
Expand All @@ -238,6 +239,8 @@ internal sealed override bool OnTryRead(

if (state.Current.PropertyState < StackFramePropertyState.TryRead)
{
JsonConverter<TKey> keyConverter = GetKeyConverter(state.Current.JsonClassInfo);
TKey key = keyConverter.ReadWithQuotes(state.Current.DictionaryKeyName);
// Get the value from the converter and add it.
bool success = elementConverter.TryRead(ref reader, typeof(TValue), options, ref state, out TValue element);
if (!success)
Expand All @@ -246,7 +249,7 @@ internal sealed override bool OnTryRead(
return false;
}

Add(element!, options, ref state);
Add(key, element!, options, ref state);
state.Current.EndElement();
}
}
Expand Down
26 changes: 15 additions & 11 deletions ...tion/DictionaryOfStringTValueConverter.cs → ...ection/DictionaryOfTKeyTValueConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ namespace System.Text.Json.Serialization.Converters
/// Converter for Dictionary{string, TValue} that (de)serializes as a JSON object with properties
/// representing the dictionary element key and value.
/// </summary>
internal sealed class DictionaryOfStringTValueConverter<TCollection, TValue>
: DictionaryDefaultConverter<TCollection, TValue>
where TCollection : Dictionary<string, TValue>
internal sealed class DictionaryOfTKeyTValueConverter<TCollection, TKey, TValue>
: DictionaryDefaultConverter<TCollection, TKey, TValue>
where TCollection : Dictionary<TKey, TValue>
where TKey : notnull
{
protected override void Add(TValue value, JsonSerializerOptions options, ref ReadStack state)
protected override void Add(TKey key, TValue value, JsonSerializerOptions options, ref ReadStack state)
{
string key = state.Current.JsonPropertyNameAsString!;
//string key = state.Current.JsonPropertyNameAsString!;
((TCollection)state.Current.ReturnValue!)[key] = value;
}

Expand All @@ -36,7 +37,7 @@ protected internal override bool OnWriteResume(
JsonSerializerOptions options,
ref WriteStack state)
{
Dictionary<string, TValue>.Enumerator enumerator;
Dictionary<TKey, TValue>.Enumerator enumerator;
if (state.Current.CollectionEnumerator == null)
{
enumerator = value.GetEnumerator();
Expand All @@ -47,17 +48,19 @@ protected internal override bool OnWriteResume(
}
else
{
enumerator = (Dictionary<string, TValue>.Enumerator)state.Current.CollectionEnumerator;
enumerator = (Dictionary<TKey, TValue>.Enumerator)state.Current.CollectionEnumerator;
}

JsonConverter<TKey> keyConverter = GetKeyConverter(state.Current.JsonClassInfo);
JsonConverter<TValue> converter = GetValueConverter(ref state);
if (!state.SupportContinuation && converter.CanUseDirectReadOrWrite)
{
// Fast path that avoids validation and extra indirection.
do
{
string key = GetKeyName(enumerator.Current.Key, ref state, options);
writer.WritePropertyName(key);
TKey key = enumerator.Current.Key;
keyConverter.WriteWithQuotes(writer, key, options, ref state);

converter.Write(writer, enumerator.Current.Value, options);
} while (enumerator.MoveNext());
}
Expand All @@ -74,8 +77,9 @@ protected internal override bool OnWriteResume(
if (state.Current.PropertyState < StackFramePropertyState.Name)
{
state.Current.PropertyState = StackFramePropertyState.Name;
string key = GetKeyName(enumerator.Current.Key, ref state, options);
writer.WritePropertyName(key);

TKey key = enumerator.Current.Key;
keyConverter.WriteWithQuotes(writer, key, options, ref state);
}

TValue element = enumerator.Current.Value;
Expand Down
23 changes: 10 additions & 13 deletions ...ext.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@ namespace System.Text.Json.Serialization.Converters
/// representing the dictionary element key and value.
/// </summary>
internal sealed class IDictionaryConverter<TCollection>
: DictionaryDefaultConverter<TCollection, object?>
: DictionaryDefaultConverter<TCollection, string, object?>
where TCollection : IDictionary
{
protected override void Add(object? value, JsonSerializerOptions options, ref ReadStack state)
protected override void Add(string key, object? value, JsonSerializerOptions options, ref ReadStack state)
Copy link
Contributor

Choose a reason for hiding this comment

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

string key

Are there any guarantees against an InvalidCastException here?

Copy link
Member Author

Choose a reason for hiding this comment

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

base method signature is generic, it is not possible to pass an incompatible type to this method.

protected abstract void Add(TKey key, TValue value, JsonSerializerOptions options, ref ReadStack state)

In case the JSON property name fails to cast to TKey we would throw on the key converter.

{
string key = state.Current.JsonPropertyNameAsString!;
((IDictionary)state.Current.ReturnValue!)[key] = value;
}

Expand Down Expand Up @@ -68,6 +67,12 @@ protected internal override bool OnWriteResume(Utf8JsonWriter writer, TCollectio
enumerator = (IDictionaryEnumerator)state.Current.CollectionEnumerator;
}

// Avoid using GetKeyConverter here
jozkee marked this conversation as resolved.
Show resolved Hide resolved
// IDictionary is a spacial case since it has polymorphic object semantics on serialization.
// But needs to use JsonConverter<string> on deserialization.
JsonClassInfo objectKeyClassInfo = options.GetOrAddClass(typeof(object));
JsonConverter<object> keyConverter = (JsonConverter<object>)objectKeyClassInfo.PropertyInfoForClassInfo.ConverterBase;

JsonConverter<object?> converter = GetValueConverter(ref state);
do
{
Expand All @@ -80,16 +85,8 @@ protected internal override bool OnWriteResume(Utf8JsonWriter writer, TCollectio
if (state.Current.PropertyState < StackFramePropertyState.Name)
{
state.Current.PropertyState = StackFramePropertyState.Name;

if (enumerator.Key is string key)
{
key = GetKeyName(key, ref state, options);
writer.WritePropertyName(key);
}
else
{
ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(state.Current.DeclaredJsonPropertyInfo!.RuntimePropertyType!);
}
object key = enumerator.Key;
keyConverter.WriteWithQuotes(writer, key, options, ref state);
}

object? element = enumerator.Value;
Expand Down
21 changes: 11 additions & 10 deletions ...ion/IDictionaryOfStringTValueConverter.cs → ...ction/IDictionaryOfTKeyTValueConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ namespace System.Text.Json.Serialization.Converters
/// Converter for <cref>System.Collections.Generic.IDictionary{string, TValue}</cref> that
/// (de)serializes as a JSON object with properties representing the dictionary element key and value.
/// </summary>
internal sealed class IDictionaryOfStringTValueConverter<TCollection, TValue>
: DictionaryDefaultConverter<TCollection, TValue>
where TCollection : IDictionary<string, TValue>
internal sealed class IDictionaryOfTKeyTValueConverter<TCollection, TKey, TValue>
: DictionaryDefaultConverter<TCollection, TKey, TValue>
Copy link
Member

Choose a reason for hiding this comment

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

Nice job that we can use use a single class for this.

where TCollection : IDictionary<TKey, TValue>
where TKey : notnull
{
protected override void Add(TValue value, JsonSerializerOptions options, ref ReadStack state)
protected override void Add(TKey key, TValue value, JsonSerializerOptions options, ref ReadStack state)
{
string key = state.Current.JsonPropertyNameAsString!;
((TCollection)state.Current.ReturnValue!)[key] = value;
}

Expand Down Expand Up @@ -57,7 +57,7 @@ protected internal override bool OnWriteResume(
JsonSerializerOptions options,
ref WriteStack state)
{
IEnumerator<KeyValuePair<string, TValue>> enumerator;
IEnumerator<KeyValuePair<TKey, TValue>> enumerator;
if (state.Current.CollectionEnumerator == null)
{
enumerator = value.GetEnumerator();
Expand All @@ -68,9 +68,10 @@ protected internal override bool OnWriteResume(
}
else
{
enumerator = (IEnumerator<KeyValuePair<string, TValue>>)state.Current.CollectionEnumerator;
enumerator = (IEnumerator<KeyValuePair<TKey, TValue>>)state.Current.CollectionEnumerator;
}

JsonConverter<TKey> keyConverter = GetKeyConverter(state.Current.JsonClassInfo);
JsonConverter<TValue> converter = GetValueConverter(ref state);
do
{
Expand All @@ -83,8 +84,8 @@ protected internal override bool OnWriteResume(
if (state.Current.PropertyState < StackFramePropertyState.Name)
{
state.Current.PropertyState = StackFramePropertyState.Name;
string key = GetKeyName(enumerator.Current.Key, ref state, options);
writer.WritePropertyName(key);
TKey key = enumerator.Current.Key;
keyConverter.WriteWithQuotes(writer, key, options, ref state);
}

TValue element = enumerator.Current.Value;
Expand All @@ -106,7 +107,7 @@ internal override Type RuntimeType
{
if (TypeToConvert.IsAbstract || TypeToConvert.IsInterface)
{
return typeof(Dictionary<string, TValue>);
return typeof(Dictionary<TKey, TValue>);
}

return TypeToConvert;
Expand Down
Loading