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.

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

Make parameter matching for custom properties map property Name with parameter #16

Merged
merged 1 commit into from
Jun 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,8 @@ public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name)

internal void CacheMember(JsonPropertyInfo jsonPropertyInfo, JsonPropertyDictionary<JsonPropertyInfo>? propertyCache, ref Dictionary<string, JsonPropertyInfo>? ignoredMembers)
{
string memberName = jsonPropertyInfo.ClrName!;
Debug.Assert(jsonPropertyInfo.ClrName != null, "ClrName can be null in custom JsonPropertyInfo instances and should never be passed in this method");
string memberName = jsonPropertyInfo.ClrName;

// The JsonPropertyNameAttribute or naming policy resulted in a collision.
if (!propertyCache!.TryAdd(jsonPropertyInfo.Name, jsonPropertyInfo))
Expand Down Expand Up @@ -545,7 +546,7 @@ internal void InitializeConstructorParameters(JsonParameterInfoValues[] jsonPara
foreach (KeyValuePair<string, JsonPropertyInfo?> kvp in PropertyCache.List)
{
JsonPropertyInfo jsonProperty = kvp.Value!;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we replace this bang with an assert as well?

Copy link
Owner Author

Choose a reason for hiding this comment

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

this one is ok because we never actually insert nulls there, it's just weird implementation detail of PropertyCache.List

string propertyName = jsonProperty.ClrName!;
string propertyName = jsonProperty.ClrName ?? jsonProperty.Name;

ParameterLookupKey key = new(propertyName, jsonProperty.PropertyType);
ParameterLookupValue value = new(jsonProperty);
Expand Down Expand Up @@ -584,7 +585,8 @@ internal void InitializeConstructorParameters(JsonParameterInfoValues[] jsonPara
else if (DataExtensionProperty != null &&
StringComparer.OrdinalIgnoreCase.Equals(paramToCheck.Name, DataExtensionProperty.Name))
{
ThrowHelper.ThrowInvalidOperationException_ExtensionDataCannotBindToCtorParam(DataExtensionProperty);
Debug.Assert(DataExtensionProperty.ClrName != null, "Custom property info cannot be data extension property");
ThrowHelper.ThrowInvalidOperationException_ExtensionDataCannotBindToCtorParam(DataExtensionProperty.ClrName, DataExtensionProperty);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,8 @@ internal void AddPropertiesUsingSourceGenInfo()
{
if (hasJsonInclude)
{
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(jsonPropertyInfo.ClrName!, jsonPropertyInfo.DeclaringType);
Debug.Assert(jsonPropertyInfo.ClrName != null, "ClrName is not set by source gen");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Debug.Assert(jsonPropertyInfo.ClrName != null, "ClrName is not set by source gen");
Debug.Assert(jsonPropertyInfo.ClrName != null, "ClrName is always set by source gen");

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually nevermind, depends on whether the string reflects the assertion or the failure condition.

Copy link
Owner Author

Choose a reason for hiding this comment

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

yeh, it's more like error message so says exactly opposite😄

ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(jsonPropertyInfo.ClrName, jsonPropertyInfo.DeclaringType);
}

continue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,9 @@ public static void ThrowInvalidOperationException_ConstructorParameterIncomplete
}

[DoesNotReturn]
public static void ThrowInvalidOperationException_ExtensionDataCannotBindToCtorParam(JsonPropertyInfo jsonPropertyInfo)
public static void ThrowInvalidOperationException_ExtensionDataCannotBindToCtorParam(string propertyName, JsonPropertyInfo jsonPropertyInfo)
{
throw new InvalidOperationException(SR.Format(SR.ExtensionDataCannotBindToCtorParam, jsonPropertyInfo.ClrName, jsonPropertyInfo.DeclaringType));
throw new InvalidOperationException(SR.Format(SR.ExtensionDataCannotBindToCtorParam, propertyName, jsonPropertyInfo.DeclaringType));
}

[DoesNotReturn]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,70 @@ public ClassWithParametrizedConstructorAndWritableProperties(int a, string b)
}
}

[Fact]
public static void SerializingTypeWithCustomNonSerializablePropertyAndJsonConstructorWorksCorrectly()
{
var resolver = new DefaultJsonTypeInfoResolver { Modifiers = { ContractModifier } };
var options = new JsonSerializerOptions { TypeInfoResolver = resolver };
string json = JsonSerializer.Serialize(new PocoWithConstructor("str"), options);
Assert.Equal("{}", json);

static void ContractModifier(JsonTypeInfo jti)
{
if (jti.Type == typeof(PocoWithConstructor))
{
jti.Properties.Add(jti.CreateJsonPropertyInfo(typeof(string), "someOtherName"));
}
}
}

[Fact]
public static void SerializingTypeWithCustomSerializablePropertyAndJsonConstructorWorksCorrectly()
{
var resolver = new DefaultJsonTypeInfoResolver { Modifiers = { ContractModifier } };
var options = new JsonSerializerOptions { TypeInfoResolver = resolver };
string json = JsonSerializer.Serialize(new PocoWithConstructor("str"), options);
Assert.Equal("""{"test":"asd"}""", json);

static void ContractModifier(JsonTypeInfo jti)
{
if (jti.Type == typeof(PocoWithConstructor))
{
JsonPropertyInfo pi = jti.CreateJsonPropertyInfo(typeof(string), "test");
pi.Get = (o) => "asd";
jti.Properties.Add(pi);
}
}
}

[Fact]
public static void SerializingTypeWithCustomPropertyAndJsonConstructorBindsParameter()
{
var resolver = new DefaultJsonTypeInfoResolver { Modifiers = { ContractModifier } };
var options = new JsonSerializerOptions { TypeInfoResolver = resolver };
string json = """{"parameter":"asd"}""";
PocoWithConstructor deserialized = JsonSerializer.Deserialize<PocoWithConstructor>(json, options);
Assert.Equal("asd", deserialized.ParameterValue);

static void ContractModifier(JsonTypeInfo jti)
{
if (jti.Type == typeof(PocoWithConstructor))
{
jti.Properties.Add(jti.CreateJsonPropertyInfo(typeof(string), "parameter"));
}
}
}

private class PocoWithConstructor
{
internal string ParameterValue { get; set; }

public PocoWithConstructor(string parameter)
{
ParameterValue = parameter;
}
}

[Fact]
public static void JsonConstructorAttributeIsOverridenAndPropertiesAreSetWhenCreateObjectIsSet_LargeConstructor()
{
Expand Down