diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index 8b2b0e0ad0419..8a571e258a4f6 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -662,6 +662,9 @@ The metadata property names '$id', '$ref', and '$values' are reserved and cannot be used as custom type discriminator property names. + + The type '{0}' contains property '{1}' that conflicts with an existing metadata property name. Consider either renaming it or ignoring it with JsonIgnoreAttribute. + Polymorphic configuration for type '{0}' should specify at least one derived type. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index 4077c89fe9789..29e11071fb7eb 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -443,7 +443,7 @@ internal void Configure() } else { - CacheNameAsUtf8BytesAndEscapedNameSection(); + ValidateAndCachePropertyName(); } if (IsRequired) @@ -472,10 +472,21 @@ internal void Configure() [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] internal abstract void DetermineReflectionPropertyAccessors(MemberInfo memberInfo, bool useNonPublicAccessors); - private void CacheNameAsUtf8BytesAndEscapedNameSection() + private void ValidateAndCachePropertyName() { Debug.Assert(Name != null); + if (Options.ReferenceHandlingStrategy is ReferenceHandlingStrategy.Preserve && + this is { DeclaringType.IsValueType: false, IsIgnored: false, IsExtensionData: false } && + Name is JsonSerializer.IdPropertyName or JsonSerializer.RefPropertyName) + { + // Validate potential conflicts with reference preservation metadata property names. + // Conflicts with polymorphic type discriminators are contextual and need to be + // handled separately by the PolymorphicTypeResolver type. + + ThrowHelper.ThrowInvalidOperationException_PropertyConflictsWithMetadataPropertyName(DeclaringType, Name); + } + NameAsUtf8Bytes = Encoding.UTF8.GetBytes(Name); EscapedNameSection = JsonHelpers.GetEscapedPropertyNameSection(NameAsUtf8Bytes, Options.Encoder); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/PolymorphicTypeResolver.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/PolymorphicTypeResolver.cs index b0ed4b24a60e2..075ea31a65a38 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/PolymorphicTypeResolver.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/PolymorphicTypeResolver.cs @@ -42,16 +42,17 @@ public PolymorphicTypeResolver(JsonSerializerOptions options, JsonPolymorphismOp ThrowHelper.ThrowInvalidOperationException_DerivedTypeNotSupported(BaseType, derivedType); } - var derivedJsonTypeInfo = new DerivedJsonTypeInfo(derivedType, typeDiscriminator); + JsonTypeInfo derivedTypeInfo = options.GetTypeInfoInternal(derivedType); + DerivedJsonTypeInfo derivedTypeInfoHolder = new(typeDiscriminator, derivedTypeInfo); - if (!_typeToDiscriminatorId.TryAdd(derivedType, derivedJsonTypeInfo)) + if (!_typeToDiscriminatorId.TryAdd(derivedType, derivedTypeInfoHolder)) { ThrowHelper.ThrowInvalidOperationException_DerivedTypeIsAlreadySpecified(BaseType, derivedType); } if (typeDiscriminator is not null) { - if (!(_discriminatorIdtoType ??= new()).TryAdd(typeDiscriminator, derivedJsonTypeInfo)) + if (!(_discriminatorIdtoType ??= new()).TryAdd(typeDiscriminator, derivedTypeInfoHolder)) { ThrowHelper.ThrowInvalidOperationException_TypeDicriminatorIdIsAlreadySpecified(BaseType, typeDiscriminator); } @@ -69,6 +70,8 @@ public PolymorphicTypeResolver(JsonSerializerOptions options, JsonPolymorphismOp if (UsesTypeDiscriminators) { + Debug.Assert(_discriminatorIdtoType != null, "Discriminator index must have been populated."); + if (!converterCanHaveMetadata) { ThrowHelper.ThrowNotSupportedException_BaseConverterDoesNotSupportMetadata(BaseType); @@ -88,6 +91,21 @@ public PolymorphicTypeResolver(JsonSerializerOptions options, JsonPolymorphismOp CustomTypeDiscriminatorPropertyNameUtf8 = utf8EncodedName; CustomTypeDiscriminatorPropertyNameJsonEncoded = JsonEncodedText.Encode(propertyName, options.Encoder); } + + // Check if the discriminator property name conflicts with any derived property names. + foreach (DerivedJsonTypeInfo derivedTypeInfo in _discriminatorIdtoType.Values) + { + if (derivedTypeInfo.JsonTypeInfo.Kind is JsonTypeInfoKind.Object) + { + foreach (JsonPropertyInfo property in derivedTypeInfo.JsonTypeInfo.Properties) + { + if (property is { IsIgnored: false, IsExtensionData: false } && property.Name == propertyName) + { + ThrowHelper.ThrowInvalidOperationException_PropertyConflictsWithMetadataPropertyName(derivedTypeInfo.JsonTypeInfo.Type, propertyName); + } + } + } + } } } @@ -136,7 +154,7 @@ public bool TryGetDerivedJsonTypeInfo(Type runtimeType, [NotNullWhen(true)] out } else { - jsonTypeInfo = result.GetJsonTypeInfo(_options); + jsonTypeInfo = result.JsonTypeInfo; typeDiscriminator = result.TypeDiscriminator; return true; } @@ -151,7 +169,7 @@ public bool TryGetDerivedJsonTypeInfo(object typeDiscriminator, [NotNullWhen(tru if (_discriminatorIdtoType.TryGetValue(typeDiscriminator, out DerivedJsonTypeInfo? result)) { Debug.Assert(typeDiscriminator.Equals(result.TypeDiscriminator)); - jsonTypeInfo = result.GetJsonTypeInfo(_options); + jsonTypeInfo = result.JsonTypeInfo; return true; } @@ -218,7 +236,7 @@ public static bool IsSupportedDerivedType(Type baseType, Type? derivedType) => } else { - ThrowHelper.ThrowNotSupportedException_RuntimeTypeDiamondAmbiguity(BaseType, type, result.DerivedType, interfaceResult.DerivedType); + ThrowHelper.ThrowNotSupportedException_RuntimeTypeDiamondAmbiguity(BaseType, type, result.JsonTypeInfo.Type, interfaceResult.JsonTypeInfo.Type); } } } @@ -307,24 +325,20 @@ public static bool IsSupportedDerivedType(Type baseType, Type? derivedType) => } /// - /// Lazy JsonTypeInfo result holder for a derived type. + /// JsonTypeInfo result holder for a derived type. /// private sealed class DerivedJsonTypeInfo { - private volatile JsonTypeInfo? _jsonTypeInfo; - - public DerivedJsonTypeInfo(Type type, object? typeDiscriminator) + public DerivedJsonTypeInfo(object? typeDiscriminator, JsonTypeInfo derivedTypeInfo) { Debug.Assert(typeDiscriminator is null or int or string); - DerivedType = type; TypeDiscriminator = typeDiscriminator; + JsonTypeInfo = derivedTypeInfo; } - public Type DerivedType { get; } public object? TypeDiscriminator { get; } - public JsonTypeInfo GetJsonTypeInfo(JsonSerializerOptions options) - => _jsonTypeInfo ??= options.GetTypeInfoInternal(DerivedType); + public JsonTypeInfo JsonTypeInfo { get; } } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs index fed9efb84d031..b93651e0cddb6 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs @@ -903,6 +903,12 @@ public static void ThrowInvalidOperationException_InvalidCustomTypeDiscriminator throw new InvalidOperationException(SR.Polymorphism_InvalidCustomTypeDiscriminatorPropertyName); } + [DoesNotReturn] + public static void ThrowInvalidOperationException_PropertyConflictsWithMetadataPropertyName(Type type, string propertyName) + { + throw new InvalidOperationException(SR.Format(SR.Polymorphism_PropertyConflictsWithMetadataPropertyName, type, propertyName)); + } + [DoesNotReturn] public static void ThrowInvalidOperationException_PolymorphicTypeConfigurationDoesNotSpecifyDerivedTypes(Type baseType) { diff --git a/src/libraries/System.Text.Json/tests/Common/ReferenceHandlerTests/ReferenceHandlerTests.cs b/src/libraries/System.Text.Json/tests/Common/ReferenceHandlerTests/ReferenceHandlerTests.cs index d0db5d9c58525..c455278ea1299 100644 --- a/src/libraries/System.Text.Json/tests/Common/ReferenceHandlerTests/ReferenceHandlerTests.cs +++ b/src/libraries/System.Text.Json/tests/Common/ReferenceHandlerTests/ReferenceHandlerTests.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Text.Encodings.Web; +using System.Text.Json.Nodes; using System.Threading.Tasks; using Newtonsoft.Json; using Xunit; @@ -849,5 +850,76 @@ public IEnumerator GetEnumerator() IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); } + + [Theory] + [InlineData(typeof(ClassWithConflictingRefProperty), "$ref")] + [InlineData(typeof(ClassWithConflictingIdProperty), "$id")] + public async Task ClassWithConflictingMetadataProperties_ThrowsInvalidOperationException(Type type, string propertyName) + { + InvalidOperationException ex; + object value = Activator.CreateInstance(type); + + ex = Assert.Throws(() => Serializer.GetTypeInfo(type, s_serializerOptionsPreserve)); + ValidateException(ex); + + ex = await Assert.ThrowsAsync(() => Serializer.SerializeWrapper(value, type, s_serializerOptionsPreserve)); + ValidateException(ex); + + ex = await Assert.ThrowsAsync(() => Serializer.DeserializeWrapper("{}", type, s_serializerOptionsPreserve)); + ValidateException(ex); + + void ValidateException(InvalidOperationException ex) + { + Assert.Contains($"The type '{type}' contains property '{propertyName}' that conflicts with an existing metadata property name.", ex.Message); + } + } + + public class ClassWithConflictingRefProperty + { + [JsonPropertyName("$ref")] + public int Value { get; set; } + } + + public class ClassWithConflictingIdProperty + { + [JsonPropertyName("$id")] + public int Value { get; set; } + } + + [Fact] + public async Task ClassWithIgnoredConflictingProperty_Supported() + { + ClassWithIgnoredConflictingProperty value = new(); + string json = await Serializer.SerializeWrapper(value, s_serializerOptionsPreserve); + Assert.Equal("""{"$id":"1"}""", json); + + value = await Serializer.DeserializeWrapper(json, s_serializerOptionsPreserve); + Assert.NotNull(value); + } + + public class ClassWithIgnoredConflictingProperty + { + [JsonPropertyName("$id"), JsonIgnore] + public int Value { get; set; } + } + + [Fact] + public async Task ClassWithExtensionDataConflictingProperty_Supported() + { + ClassWithExtensionDataConflictingProperty value = new(); + string json = await Serializer.SerializeWrapper(value, s_serializerOptionsPreserve); + Assert.Equal("""{"$id":"1"}""", json); + + value = await Serializer.DeserializeWrapper("""{"$id":"1","extraProp":null}""", s_serializerOptionsPreserve); + Assert.NotNull(value); + Assert.Equal(1, value.Value.Count); + Assert.Contains("extraProp", value.Value); + } + + public class ClassWithExtensionDataConflictingProperty + { + [JsonPropertyName("$id"), JsonExtensionData] + public JsonObject Value { get; set; } + } } } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ReferenceHandlerTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ReferenceHandlerTests.cs index 001ecf921c8a0..03c1ede64586f 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ReferenceHandlerTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ReferenceHandlerTests.cs @@ -135,6 +135,10 @@ public ReferenceHandlerTests_Metadata(JsonSerializerWrapper serializer) [JsonSerializable(typeof(LinkedList))] [JsonSerializable(typeof(LinkedList))] [JsonSerializable(typeof(LinkedList))] + [JsonSerializable(typeof(ClassWithConflictingRefProperty))] + [JsonSerializable(typeof(ClassWithConflictingIdProperty))] + [JsonSerializable(typeof(ClassWithIgnoredConflictingProperty))] + [JsonSerializable(typeof(ClassWithExtensionDataConflictingProperty))] internal sealed partial class ReferenceHandlerTestsContext_Metadata : JsonSerializerContext { } @@ -273,6 +277,10 @@ public ReferenceHandlerTests_Default(JsonSerializerWrapper serializer) [JsonSerializable(typeof(LinkedList))] [JsonSerializable(typeof(LinkedList))] [JsonSerializable(typeof(LinkedList))] + [JsonSerializable(typeof(ClassWithConflictingRefProperty))] + [JsonSerializable(typeof(ClassWithConflictingIdProperty))] + [JsonSerializable(typeof(ClassWithIgnoredConflictingProperty))] + [JsonSerializable(typeof(ClassWithExtensionDataConflictingProperty))] internal sealed partial class ReferenceHandlerTestsContext_Default : JsonSerializerContext { } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/PolymorphicTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/PolymorphicTests.cs index 9e14737051a2e..861522b881d79 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/PolymorphicTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/PolymorphicTests.cs @@ -4,6 +4,7 @@ using System.Collections; using System.Collections.Generic; using System.Collections.Immutable; +using System.Text.Json.Nodes; using System.Text.Json.Serialization.Metadata; using System.Threading.Tasks; using Xunit; @@ -577,5 +578,102 @@ class MyDerivedThing : MyThing class MyThingCollection : List { } class MyThingDictionary : Dictionary { } + + [Theory] + [InlineData(typeof(PolymorphicTypeWithConflictingPropertyNameAtBase), typeof(PolymorphicTypeWithConflictingPropertyNameAtBase.Derived))] + [InlineData(typeof(PolymorphicTypeWithConflictingPropertyNameAtDerived), typeof(PolymorphicTypeWithConflictingPropertyNameAtDerived.Derived))] + public async Task PolymorphicTypesWithConflictingPropertyNames_ThrowsInvalidOperationException(Type baseType, Type derivedType) + { + InvalidOperationException ex; + object value = Activator.CreateInstance(derivedType); + + ex = Assert.Throws(() => Serializer.GetTypeInfo(baseType)); + ValidateException(ex); + + ex = await Assert.ThrowsAsync(() => Serializer.SerializeWrapper(value, baseType)); + ValidateException(ex); + + ex = await Assert.ThrowsAsync(() => Serializer.DeserializeWrapper("{}", baseType)); + ValidateException(ex); + + void ValidateException(InvalidOperationException ex) + { + Assert.Contains($"The type '{derivedType}' contains property 'Type' that conflicts with an existing metadata property name.", ex.Message); + } + } + + [JsonPolymorphic(TypeDiscriminatorPropertyName = "Type")] + [JsonDerivedType(typeof(Derived), nameof(Derived))] + public abstract class PolymorphicTypeWithConflictingPropertyNameAtBase + { + public string Type { get; set; } + + public class Derived : PolymorphicTypeWithConflictingPropertyNameAtBase + { + public string Name { get; set; } + } + } + + [JsonPolymorphic(TypeDiscriminatorPropertyName = "Type")] + [JsonDerivedType(typeof(Derived), nameof(Derived))] + public abstract class PolymorphicTypeWithConflictingPropertyNameAtDerived + { + public class Derived : PolymorphicTypeWithConflictingPropertyNameAtDerived + { + public string Type { get; set; } + } + } + + [Fact] + public async Task PolymorphicTypeWithIgnoredConflictingPropertyName_Supported() + { + PolymorphicTypeWithIgnoredConflictingPropertyName value = new PolymorphicTypeWithIgnoredConflictingPropertyName.Derived(); + + JsonTypeInfo typeInfo = Serializer.GetTypeInfo(typeof(PolymorphicTypeWithIgnoredConflictingPropertyName)); + Assert.NotNull(typeInfo); + + string json = await Serializer.SerializeWrapper(value); + Assert.Equal("""{"Type":"Derived"}""", json); + + value = await Serializer.DeserializeWrapper(json); + Assert.IsType(value); + } + + [JsonPolymorphic(TypeDiscriminatorPropertyName = "Type")] + [JsonDerivedType(typeof(Derived), nameof(Derived))] + public abstract class PolymorphicTypeWithIgnoredConflictingPropertyName + { + [JsonIgnore] + public string Type { get; set; } + + public class Derived : PolymorphicTypeWithIgnoredConflictingPropertyName; + } + + [Fact] + public async Task PolymorphicTypeWithExtensionDataConflictingPropertyName_Supported() + { + PolymorphicTypeWithExtensionDataConflictingPropertyName value = new PolymorphicTypeWithExtensionDataConflictingPropertyName.Derived(); + + JsonTypeInfo typeInfo = Serializer.GetTypeInfo(typeof(PolymorphicTypeWithIgnoredConflictingPropertyName)); + Assert.NotNull(typeInfo); + + string json = await Serializer.SerializeWrapper(value); + Assert.Equal("""{"Type":"Derived"}""", json); + + value = await Serializer.DeserializeWrapper("""{"Type":"Derived","extraProp":null}"""); + Assert.IsType(value); + Assert.Equal(1, value.Type.Count); + Assert.Contains("extraProp", value.Type); + } + + [JsonPolymorphic(TypeDiscriminatorPropertyName = "Type")] + [JsonDerivedType(typeof(Derived), nameof(Derived))] + public abstract class PolymorphicTypeWithExtensionDataConflictingPropertyName + { + [JsonExtensionData] + public JsonObject Type { get; set; } + + public class Derived : PolymorphicTypeWithExtensionDataConflictingPropertyName; + } } }