Skip to content

Add support for non-string Tkey on Dictionary #32909

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

Closed
wants to merge 2 commits into from
Closed

Add support for non-string Tkey on Dictionary #32909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9a36da
Add support for non-string Tkey on Dictionary using custom KeyConvert…
jozkee Feb 27, 2020
53bfb96
* Add ConcurrentDictionary to cache the key converters
jozkee Feb 27, 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
10 changes: 8 additions & 2 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
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<DocumentationFile>$(OutputPath)$(MSBuildProjectName).xml</DocumentationFile>
Expand Down Expand Up @@ -59,7 +59,7 @@
<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\DictionaryOfTKeyTValue.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" />
Expand All @@ -78,6 +78,12 @@
<Compile Include="System\Text\Json\Serialization\Converters\Collection\ListOfTConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\QueueOfTConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Collection\StackOfTConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\KeyConverters\EnumKeyConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\KeyConverters\GuidKeyConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\KeyConverters\Int32KeyConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\KeyConverters\KeyConverterOfTKey.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\KeyConverters\ObjectKeyConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\KeyConverters\StringKeyConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Object\ObjectConverterFactory.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Object\ObjectDefaultConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Value\BooleanConverter.cs" />
Expand Down
27 changes: 27 additions & 0 deletions src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,33 @@ public static string GetUnescapedString(ReadOnlySpan<byte> utf8Source, int idx)
return utf8String;
}

// TODO: Borrowing this from https://github.com/dotnet/runtime/pull/32669/files#diff-82b934371fff193b37c85635239bb6ebR70
// Remove when that PR is in or remove from #32669 if this gets in first.
public static ReadOnlySpan<byte> GetUnescapedSpan(ReadOnlySpan<byte> utf8Source, int idx)
{
// The escaped name is always >= than the unescaped, so it is safe to use escaped name for the buffer length.
int length = utf8Source.Length;
byte[]? pooledName = null;

Span<byte> utf8Unescaped = length <= JsonConstants.StackallocThreshold ?
stackalloc byte[length] :
(pooledName = ArrayPool<byte>.Shared.Rent(length));

Unescape(utf8Source, utf8Unescaped, idx, out int written);
Debug.Assert(written > 0);

ReadOnlySpan<byte> propertyName = utf8Unescaped.Slice(0, written).ToArray();
Debug.Assert(!propertyName.IsEmpty);

if (pooledName != null)
{
new Span<byte>(pooledName, 0, written).Clear();
ArrayPool<byte>.Shared.Return(pooledName);
}

return propertyName;
}

public static bool UnescapeAndCompare(ReadOnlySpan<byte> utf8Source, ReadOnlySpan<byte> other)
{
Debug.Assert(utf8Source.Length >= other.Length && utf8Source.Length / JsonConstants.MaxExpansionFactorWhileEscaping <= other.Length);
Expand Down
10 changes: 10 additions & 0 deletions src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,11 @@ public bool TryGetInt32(out int value)
throw ThrowHelper.GetInvalidOperationException_ExpectedNumber(TokenType);
}

return TryGetInt32AfterValidation(out value);
}

internal bool TryGetInt32AfterValidation(out int value)
{
ReadOnlySpan<byte> span = HasValueSequence ? ValueSequence.ToArray() : ValueSpan;
if (Utf8Parser.TryParse(span, out int tmp, out int bytesConsumed)
&& span.Length == bytesConsumed)
Expand Down Expand Up @@ -945,6 +950,11 @@ public bool TryGetGuid(out Guid value)
throw ThrowHelper.GetInvalidOperationException_ExpectedString(TokenType);
}

return TryGetGuidAfterValidation(out value);
}

internal bool TryGetGuidAfterValidation(out Guid value)
{
ReadOnlySpan<byte> span = stackalloc byte[0];

if (HasValueSequence)
Expand Down
33 changes: 24 additions & 9 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 @@ -2,6 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Buffers.Text;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

Expand All @@ -10,13 +13,13 @@ namespace System.Text.Json.Serialization.Converters
/// <summary>
/// Default base class implementation of <cref>JsonDictionaryConverter{TCollection}</cref> .
/// </summary>
internal abstract class DictionaryDefaultConverter<TCollection, TValue>
: JsonDictionaryConverter<TCollection>
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);

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

internal override Type ElementType => typeof(TValue);

internal override Type KeyType => typeof(TKey);

protected static JsonConverter<TValue> GetElementConverter(ref ReadStack state)
{
JsonConverter<TValue> converter = (JsonConverter<TValue>)state.Current.JsonClassInfo.ElementClassInfo!.PolicyProperty!.ConverterBase;
Expand Down Expand Up @@ -62,13 +67,19 @@ protected static JsonConverter<TValue> GetValueConverter(ref WriteStack state)
return converter;
}

protected static KeyConverter<TKey> GetKeyConverter(JsonClassInfo classInfo) => (KeyConverter<TKey>)classInfo.KeyConverter;

internal sealed override bool OnTryRead(
ref Utf8JsonReader reader,
Type typeToConvert,
JsonSerializerOptions options,
ref ReadStack state,
[MaybeNullWhen(false)] out TCollection value)
{
// Get the key converter at the very beginning; will throw NSE if there is no converter for TKey.
// This is performed at the very beginning to avoid processing unsupported types.
KeyConverter<TKey> keyConverter = GetKeyConverter(state.Current.JsonClassInfo);

bool shouldReadPreservedReferences = options.ReferenceHandling.ShouldReadPreservedReferences();

if (!state.SupportContinuation && !shouldReadPreservedReferences)
Expand Down Expand Up @@ -102,11 +113,12 @@ internal sealed override bool OnTryRead(
}

state.Current.JsonPropertyNameAsString = reader.GetString();
Copy link
Contributor

@steveharter steveharter Feb 27, 2020

Choose a reason for hiding this comment

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

Do we still need to set JsonPropertyNameAsString?

Copy link
Member Author

@jozkee jozkee Feb 27, 2020

Choose a reason for hiding this comment

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

Since DefaultDictionaryConverter.OnTryRead is still being used by other dictionaries like ImmutableDictionary or IDictionary; right now it is still needed.

We can remove it once we spread the TKey changes to those converters as well.

Copy link
Contributor

@layomia layomia Mar 2, 2020

Choose a reason for hiding this comment

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

To keep the code clean/consistent (and to avoid the unnecessary GetString() call here), we should extend key support for all the dictionary converters.

keyConverter.OnTryRead(ref reader, keyConverter.TypeToConvert, options, ref state, out TKey key);

// 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 @@ -127,13 +139,14 @@ internal sealed override bool OnTryRead(
ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert);
}

state.Current.JsonPropertyNameAsString = reader.GetString();
state.Current.JsonPropertyNameAsString = reader.GetString()!;
keyConverter.OnTryRead(ref reader, keyConverter.TypeToConvert, options, ref state, out TKey key);

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 @@ -219,16 +232,16 @@ internal sealed override bool OnTryRead(

state.Current.PropertyState = StackFramePropertyState.Name;

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

state.Current.DictionaryKeyName = propertyName.ToArray();
state.Current.JsonPropertyNameAsString = reader.GetString();
}

Expand All @@ -245,6 +258,8 @@ internal sealed override bool OnTryRead(

if (state.Current.PropertyState < StackFramePropertyState.TryRead)
{
keyConverter.OnTryRead(ref reader, keyConverter.TypeToConvert, options, ref state, out TKey key);

// 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 @@ -253,7 +268,7 @@ internal sealed override bool OnTryRead(
return false;
}

Add(element, options, ref state);
Add(key, element, options, ref state);
state.Current.EndElement();
}
}
Expand Down
41 changes: 21 additions & 20 deletions ...tion/DictionaryOfStringTValueConverter.cs → ...ters/Collection/DictionaryOfTKeyTValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,20 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Diagnostics;

namespace System.Text.Json.Serialization.Converters
{
/// <summary>
/// Converter for Dictionary{string, TValue} that (de)serializes as a JSON object with properties
/// Converter for Dictionary{TKey, 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)
{
Debug.Assert(state.Current.ReturnValue is TCollection);

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

Expand All @@ -39,7 +36,12 @@ protected internal override bool OnWriteResume(
JsonSerializerOptions options,
ref WriteStack state)
{
Dictionary<string, TValue>.Enumerator enumerator;
// Get the key converter at the very beginning; will throw NSE if there is no converter for TKey.
// This is performed at the very beginning to avoid processing unsupported types.
KeyConverter<TKey> keyConverter = GetKeyConverter(state.Current.JsonClassInfo);

Dictionary<TKey, TValue>.Enumerator enumerator;

if (state.Current.CollectionEnumerator == null)
{
enumerator = value.GetEnumerator();
Expand All @@ -50,20 +52,19 @@ protected internal override bool OnWriteResume(
}
else
{
Debug.Assert(state.Current.CollectionEnumerator is Dictionary<string, TValue>.Enumerator);
enumerator = (Dictionary<string, TValue>.Enumerator)state.Current.CollectionEnumerator;
enumerator = (Dictionary<TKey, TValue>.Enumerator)state.Current.CollectionEnumerator;
}

JsonConverter<TValue> converter = GetValueConverter(ref state);
if (!state.SupportContinuation && converter.CanUseDirectReadOrWrite)
JsonConverter<TValue> valueConverter = GetValueConverter(ref state);

if (!state.SupportContinuation && valueConverter.CanUseDirectReadOrWrite)
{
// Fast path that avoids validation and extra indirection.
do
{
string key = GetKeyName(enumerator.Current.Key, ref state, options);
writer.WritePropertyName(key);
keyConverter.OnTryWrite(writer, enumerator.Current.Key, options, ref state);
// TODO: https://github.com/dotnet/runtime/issues/32523
converter.Write(writer, enumerator.Current.Value!, options);
valueConverter.Write(writer, enumerator.Current.Value!, options);
} while (enumerator.MoveNext());
}
else
Expand All @@ -77,14 +78,14 @@ protected internal override bool OnWriteResume(
}

TValue element = enumerator.Current.Value;

if (state.Current.PropertyState < StackFramePropertyState.Name)
{
state.Current.PropertyState = StackFramePropertyState.Name;
string key = GetKeyName(enumerator.Current.Key, ref state, options);
writer.WritePropertyName(key);
keyConverter.OnTryWrite(writer, enumerator.Current.Key, options, ref state);
}

if (!converter.TryWrite(writer, element, options, ref state))
if (!valueConverter.TryWrite(writer, element, options, ref state))
{
state.Current.CollectionEnumerator = enumerator;
return false;
Expand Down
4 changes: 2 additions & 2 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 @@ -13,10 +13,10 @@ 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 _, object? value, JsonSerializerOptions options, ref ReadStack state)
Copy link
Contributor

@layomia layomia Mar 2, 2020

Choose a reason for hiding this comment

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

Would should use the key parsed with the key converter in DictionaryDefaultConverter instead of JsonPropertyNameAsString.

{
Debug.Assert(state.Current.ReturnValue is IDictionary);

Expand Down
4 changes: 2 additions & 2 deletions ...ystem/Text/Json/Serialization/Converters/Collection/IDictionaryOfStringTValueConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ namespace System.Text.Json.Serialization.Converters
/// (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>
: DictionaryDefaultConverter<TCollection, string, TValue>
where TCollection : IDictionary<string, TValue>
{
protected override void Add(TValue value, JsonSerializerOptions options, ref ReadStack state)
protected override void Add(string _, TValue value, JsonSerializerOptions options, ref ReadStack state)
{
Debug.Assert(state.Current.ReturnValue is TCollection);

Expand Down
Loading