Skip to content

Commit

Permalink
Fix JSON deserializer property name escaping and address naming feedb…
Browse files Browse the repository at this point in the history 
…ack (dotnet#37124)
  • Loading branch information
@steveharter
steveharter authored Apr 24, 2019
1 parent c73d2c1 commit cf06248
Show file tree
Hide file tree
Showing 13 changed files with 215 additions and 89 deletions.
4 changes: 2 additions & 2 deletions src/System.Text.Json/src/Resources/Strings.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,9 @@
<value>Cannot write a comment value which contains the end of comment delimiter.</value>
</data>
<data name="SerializerPropertyNameConflict" xml:space="preserve">
<value>The property '{0}.{1}' has the same name as a previous property based on naming or casing policies.</value>
<value>The JSON property name for '{0}.{1}' collides with another property.</value>
</data>
<data name="SerializerPropertyNameNull" xml:space="preserve">
<value>The property name for '{0}.{1}' cannot be null as a result of naming policies.</value>
<value>The JSON property name for '{0}.{1}' cannot be null.</value>
</data>
</root>
4 changes: 2 additions & 2 deletions src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.AddProperty.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ private JsonPropertyInfo AddProperty(Type propertyType, PropertyInfo propertyInf

if (propertyInfo != null)
{
_propertyRefs.Add(new PropertyRef(GetKey(jsonInfo.CompareName), jsonInfo));
_propertyRefs.Add(new PropertyRef(GetKey(jsonInfo.NameUsedToCompare), jsonInfo));
}
else
{
Expand Down Expand Up @@ -60,7 +60,7 @@ internal JsonPropertyInfo CreateProperty(Type declaredPropertyType, Type runtime

JsonPropertyInfo jsonInfo = (JsonPropertyInfo)Activator.CreateInstance(
propertyInfoClassType,
BindingFlags.Instance | BindingFlags.NonPublic,
BindingFlags.Instance | BindingFlags.Public,
binder: null,
new object[] { parentClassType, declaredPropertyType, runtimePropertyType, propertyInfo, collectionElementType, options },
culture: null);
Expand Down
4 changes: 2 additions & 2 deletions src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ internal JsonClassInfo(Type type, JsonSerializerOptions options)
}

// If the JsonPropertyNameAttribute or naming policy results in collisions, throw an exception.
if (!propertyNames.Add(jsonPropertyInfo.CompareNameAsString))
if (!propertyNames.Add(jsonPropertyInfo.NameUsedToCompareAsString))
{
ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(this, jsonPropertyInfo);
}
Expand Down Expand Up @@ -242,7 +242,7 @@ private static bool TryIsPropertyRefEqual(ref PropertyRef propertyRef, ReadOnlyS
{
if (propertyName.Length <= PropertyNameKeyLength ||
// We compare the whole name, although we could skip the first 6 bytes (but it's likely not any faster)
propertyName.SequenceEqual((ReadOnlySpan<byte>)propertyRef.Info.CompareName))
propertyName.SequenceEqual(propertyRef.Info.NameUsedToCompare))
{
info = propertyRef.Info;
return true;
Expand Down
4 changes: 2 additions & 2 deletions src/System.Text.Json/src/System/Text/Json/Serialization/JsonNamingPolicy.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
namespace System.Text.Json.Serialization
{
/// <summary>
/// Determines the naming policy used to convert a JSON name to another format, such as a camel-casing format.
/// Determines the naming policy used to convert a string-based name to another format, such as a camel-casing format.
/// </summary>
public abstract class JsonNamingPolicy
{
Expand All @@ -17,7 +17,7 @@ protected JsonNamingPolicy() { }
public static JsonNamingPolicy CamelCase { get; } = new JsonCamelCaseNamePolicy();

/// <summary>
/// Converts the provided name.
/// When overridden in a derived class, converts the specified name according to the policy.
/// </summary>
/// <param name="name">The name to convert.</param>
/// <returns>The converted name.</returns>
Expand Down
119 changes: 65 additions & 54 deletions src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,37 +17,36 @@ internal abstract class JsonPropertyInfo
private static readonly JsonEnumerableConverter s_jsonArrayConverter = new DefaultArrayConverter();
private static readonly JsonEnumerableConverter s_jsonEnumerableConverter = new DefaultEnumerableConverter();

internal ClassType ClassType;
public ClassType ClassType;

// The name of the property with any casing policy or the name specified from JsonPropertyNameAttribute.
private byte[] _name { get; set; }
internal ReadOnlySpan<byte> Name => _name;
internal string NameAsString { get; private set; }
public ReadOnlySpan<byte> Name => _name;
public string NameAsString { get; private set; }

// Used to support case-insensitive comparison
private byte[] _compareName { get; set; }
internal ReadOnlySpan<byte> CompareName => _compareName;
internal string CompareNameAsString { get; private set; }
private byte[] _nameUsedToCompare { get; set; }
public ReadOnlySpan<byte> NameUsedToCompare => _nameUsedToCompare;
public string NameUsedToCompareAsString { get; private set; }

// The escaped name passed to the writer.
internal byte[] _escapedName { get; private set; }
internal ReadOnlySpan<byte> EscapedName => _escapedName;
public byte[] _escapedName { get; private set; }

internal bool HasGetter { get; set; }
internal bool HasSetter { get; set; }
internal bool ShouldSerialize { get; private set; }
internal bool ShouldDeserialize { get; private set; }
public bool HasGetter { get; set; }
public bool HasSetter { get; set; }
public bool ShouldSerialize { get; private set; }
public bool ShouldDeserialize { get; private set; }

internal bool IgnoreNullValues { get; private set; }
public bool IgnoreNullValues { get; private set; }


// todo: to minimize hashtable lookups, cache JsonClassInfo:
//public JsonClassInfo ClassInfo;

// Constructor used for internal identifiers
internal JsonPropertyInfo() { }
public JsonPropertyInfo() { }

internal JsonPropertyInfo(
public JsonPropertyInfo(
Type parentClassType,
Type declaredPropertyType,
Type runtimePropertyType,
Expand All @@ -70,21 +69,21 @@ internal JsonPropertyInfo(
CanBeNull = IsNullableType || !runtimePropertyType.IsValueType;
}

internal bool CanBeNull { get; private set; }
internal JsonClassInfo ElementClassInfo { get; private set; }
internal JsonEnumerableConverter EnumerableConverter { get; private set; }
public bool CanBeNull { get; private set; }
public JsonClassInfo ElementClassInfo { get; private set; }
public JsonEnumerableConverter EnumerableConverter { get; private set; }

internal bool IsNullableType { get; private set; }
public bool IsNullableType { get; private set; }

internal PropertyInfo PropertyInfo { get; private set; }
public PropertyInfo PropertyInfo { get; private set; }

internal Type ParentClassType { get; private set; }
public Type ParentClassType { get; private set; }

internal Type DeclaredPropertyType { get; private set; }
public Type DeclaredPropertyType { get; private set; }

internal Type RuntimePropertyType { get; private set; }
public Type RuntimePropertyType { get; private set; }

internal virtual void GetPolicies(JsonSerializerOptions options)
public virtual void GetPolicies(JsonSerializerOptions options)
{
DetermineSerializationCapabilities(options);
DeterminePropertyName(options);
Expand All @@ -99,8 +98,8 @@ private void DeterminePropertyName(JsonSerializerOptions options)
if (nameAttribute != null)
{
NameAsString = nameAttribute.Name;
// This is detected and thrown by caller.

// null is not valid; JsonClassInfo throws an InvalidOperationException after this return.
if (NameAsString == null)
{
return;
Expand All @@ -110,7 +109,7 @@ private void DeterminePropertyName(JsonSerializerOptions options)
{
NameAsString = options.PropertyNamingPolicy.ConvertName(PropertyInfo.Name);

// This is detected and thrown by caller.
// null is not valid; JsonClassInfo throws an InvalidOperationException after this return.
if (NameAsString == null)
{
return;
Expand All @@ -127,35 +126,47 @@ private void DeterminePropertyName(JsonSerializerOptions options)
// Set the compare name.
if (options.PropertyNameCaseInsensitive)
{
CompareNameAsString = NameAsString.ToUpperInvariant();
_compareName = Encoding.UTF8.GetBytes(CompareNameAsString);
NameUsedToCompareAsString = NameAsString.ToUpperInvariant();
_nameUsedToCompare = Encoding.UTF8.GetBytes(NameUsedToCompareAsString);
}
else
{
CompareNameAsString = NameAsString;
_compareName = _name;
NameUsedToCompareAsString = NameAsString;
_nameUsedToCompare = _name;
}

// Cache the escaped name.
#if true
// temporary behavior until the writer can accept escaped string.
_escapedName = _name;
#else

int valueIdx = JsonWriterHelper.NeedsEscaping(_name);
if (valueIdx == -1)
{
_escapedName = _name;
}
else
{
byte[] pooledName = null;
int length = JsonWriterHelper.GetMaxEscapedLength(_name.Length, valueIdx);

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

JsonWriterHelper.EscapeString(_name, escapedName, 0, out int written);

JsonWriterHelper.EscapeString(_name, tempArray, valueIdx, out int written);
_escapedName = new byte[written];
tempArray.CopyTo(_escapedName, 0);
_escapedName = escapedName.Slice(0, written).ToArray();

// We clear the array because it is "user data" (although a property name).
new Span<byte>(tempArray, 0, written).Clear();
ArrayPool<byte>.Shared.Return(tempArray);
if (pooledName != null)
{
// We clear the array because it is "user data" (although a property name).
new Span<byte>(pooledName, 0, written).Clear();
ArrayPool<byte>.Shared.Return(pooledName);
}
}
#endif
}
}

Expand Down Expand Up @@ -226,40 +237,40 @@ private void DetermineSerializationCapabilities(JsonSerializerOptions options)
}

// After the property is added, clear any state not used later.
internal void ClearUnusedValuesAfterAdd()
public void ClearUnusedValuesAfterAdd()
{
NameAsString = null;
CompareNameAsString = null;
NameUsedToCompareAsString = null;
}

// Copy any settings defined at run-time to the new property.
internal void CopyRuntimeSettingsTo(JsonPropertyInfo other)
public void CopyRuntimeSettingsTo(JsonPropertyInfo other)
{
other._name = _name;
other._compareName = _compareName;
other._nameUsedToCompare = _nameUsedToCompare;
other._escapedName = _escapedName;
}

internal abstract object GetValueAsObject(object obj, JsonSerializerOptions options);
public abstract object GetValueAsObject(object obj, JsonSerializerOptions options);

internal TAttribute GetAttribute<TAttribute>() where TAttribute : Attribute
public TAttribute GetAttribute<TAttribute>() where TAttribute : Attribute
{
return (TAttribute)PropertyInfo?.GetCustomAttribute(typeof(TAttribute), inherit: false);
}

internal abstract void ApplyNullValue(JsonSerializerOptions options, ref ReadStack state);
internal abstract IList CreateConverterList();
public abstract void ApplyNullValue(JsonSerializerOptions options, ref ReadStack state);

public abstract IList CreateConverterList();

internal abstract Type GetConcreteType(Type interfaceType);
public abstract Type GetConcreteType(Type interfaceType);

internal abstract void Read(JsonTokenType tokenType, JsonSerializerOptions options, ref ReadStack state, ref Utf8JsonReader reader);
internal abstract void ReadEnumerable(JsonTokenType tokenType, JsonSerializerOptions options, ref ReadStack state, ref Utf8JsonReader reader);
internal abstract void SetValueAsObject(object obj, object value, JsonSerializerOptions options);
public abstract void Read(JsonTokenType tokenType, JsonSerializerOptions options, ref ReadStack state, ref Utf8JsonReader reader);
public abstract void ReadEnumerable(JsonTokenType tokenType, JsonSerializerOptions options, ref ReadStack state, ref Utf8JsonReader reader);
public abstract void SetValueAsObject(object obj, object value, JsonSerializerOptions options);

internal abstract void Write(JsonSerializerOptions options, ref WriteStackFrame current, Utf8JsonWriter writer);
public abstract void Write(JsonSerializerOptions options, ref WriteStackFrame current, Utf8JsonWriter writer);

internal abstract void WriteDictionary(JsonSerializerOptions options, ref WriteStackFrame current, Utf8JsonWriter writer);
internal abstract void WriteEnumerable(JsonSerializerOptions options, ref WriteStackFrame current, Utf8JsonWriter writer);
public abstract void WriteDictionary(JsonSerializerOptions options, ref WriteStackFrame current, Utf8JsonWriter writer);
public abstract void WriteEnumerable(JsonSerializerOptions options, ref WriteStackFrame current, Utf8JsonWriter writer);
}
}
20 changes: 10 additions & 10 deletions src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,16 @@ namespace System.Text.Json.Serialization
/// </summary>
internal abstract class JsonPropertyInfoCommon<TClass, TDeclaredProperty, TRuntimeProperty> : JsonPropertyInfo
{
internal bool _isPropertyPolicy;
internal Func<TClass, TDeclaredProperty> Get { get; private set; }
internal Action<TClass, TDeclaredProperty> Set { get; private set; }
public bool _isPropertyPolicy;
public Func<TClass, TDeclaredProperty> Get { get; private set; }
public Action<TClass, TDeclaredProperty> Set { get; private set; }

public JsonValueConverter<TRuntimeProperty> ValueConverter { get; internal set; }

// Constructor used for internal identifiers
internal JsonPropertyInfoCommon() { }
public JsonPropertyInfoCommon() { }

internal JsonPropertyInfoCommon(
public JsonPropertyInfoCommon(
Type parentClassType,
Type declaredPropertyType,
Type runtimePropertyType,
Expand Down Expand Up @@ -63,13 +63,13 @@ internal JsonPropertyInfoCommon(
GetPolicies(options);
}

internal override void GetPolicies(JsonSerializerOptions options)
public override void GetPolicies(JsonSerializerOptions options)
{
ValueConverter = DefaultConverters<TRuntimeProperty>.s_converter;
base.GetPolicies(options);
}

internal override object GetValueAsObject(object obj, JsonSerializerOptions options)
public override object GetValueAsObject(object obj, JsonSerializerOptions options)
{
if (_isPropertyPolicy)
{
Expand All @@ -80,7 +80,7 @@ internal override object GetValueAsObject(object obj, JsonSerializerOptions opti
return Get((TClass)obj);
}

internal override void SetValueAsObject(object obj, object value, JsonSerializerOptions options)
public override void SetValueAsObject(object obj, object value, JsonSerializerOptions options)
{
Debug.Assert(Set != null);
TDeclaredProperty typedValue = (TDeclaredProperty)value;
Expand All @@ -91,13 +91,13 @@ internal override void SetValueAsObject(object obj, object value, JsonSerializer
}
}

internal override IList CreateConverterList()
public override IList CreateConverterList()
{
return new List<TDeclaredProperty>();
}

// Map interfaces to a well-known implementation.
internal override Type GetConcreteType(Type interfaceType)
public override Type GetConcreteType(Type interfaceType)
{
if (interfaceType.IsAssignableFrom(typeof(IDictionary<string, TRuntimeProperty>)) ||
interfaceType.IsAssignableFrom(typeof(IReadOnlyDictionary<string, TRuntimeProperty>)))
Expand Down
Loading

0 comments on commit cf06248

Please sign in to comment.