[release/8.0-staging] Fix JsonArray.Add and ReplaceWith regressions. (#…

…94882) * Fix JsonArray.Add and ReplaceWith regressions. * Add comment * Address feedback. * Update packaging metadata * Bring back removed using statement * Fix the build error that was showing up after bumping the ServicingVersion prop: Assembly 'System.Text.Json.TestLibrary.Roslyn3.11' with identity 'System.Text.Json.TestLibrary.Roslyn3.11, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' uses 'System.Text.Json, Version=8.0.0.1, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' which has a higher version than referenced assembly 'System.Text.Json' with identity 'System.Text.Json, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' * Update src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.TestLibrary/System.Text.Json.TestLibrary.targets * Fix incremental servicing props ordering issue --------- Co-authored-by: Eirik Tsarpalis <[email protected]> Co-authored-by: Carlos Sánchez López <[email protected]> Co-authored-by: Viktor Hofer <[email protected]>
dotnet · Nov 22, 2023 · a20ee6f · a20ee6f
1 parent 4fc3df2
commit a20ee6f
Show file tree

Hide file tree

Showing 5 changed files with 119 additions and 21 deletions.
diff --git a/src/libraries/Directory.Build.targets b/src/libraries/Directory.Build.targets
@@ -7,6 +7,18 @@
     <StrongNameKeyId Condition="'$(IsTestProject)' == 'true' or '$(IsTestSupportProject)' == 'true'">$(TestStrongNameKeyId)</StrongNameKeyId>
   </PropertyGroup>
 
+  <!-- Need to be defined before packaging.targets is imported. -->
+  <PropertyGroup>
+    <!-- The source of truth for these IsNETCoreApp* properties is NetCoreAppLibrary.props. -->
+    <IsNETCoreAppSrc Condition="'$(IsSourceProject)' == 'true' and
+                                $(NetCoreAppLibrary.Contains('$(AssemblyName);'))">true</IsNETCoreAppSrc>
+    <IsNETCoreAppRef Condition="('$(IsReferenceAssemblyProject)' == 'true' or '$(IsRuntimeAndReferenceAssembly)' == 'true') and
+                                $(NetCoreAppLibrary.Contains('$(AssemblyName);')) and
+                                '$(IsPrivateAssembly)' != 'true'">true</IsNETCoreAppRef>
+    <IsNETCoreAppAnalyzer Condition="'$(IsGeneratorProject)' == 'true' and
+                                     $(NetCoreAppLibraryGenerator.Contains('$(MSBuildProjectName);'))">true</IsNETCoreAppAnalyzer>
+  </PropertyGroup>
+
   <!-- resources.targets need to be imported before the Arcade SDK. -->
   <Import Project="$(RepositoryEngineeringDir)resources.targets" />
   <Import Project="..\..\Directory.Build.targets" />
@@ -47,14 +59,6 @@
     -->
     <WarningsAsErrors>$(WarningsAsErrors.Replace('NU1605', ''))</WarningsAsErrors>
 
-    <!-- The source of truth for these IsNETCoreApp* properties is NetCoreAppLibrary.props. -->
-    <IsNETCoreAppSrc Condition="'$(IsSourceProject)' == 'true' and
-                                $(NetCoreAppLibrary.Contains('$(AssemblyName);'))">true</IsNETCoreAppSrc>
-    <IsNETCoreAppRef Condition="('$(IsReferenceAssemblyProject)' == 'true' or '$(IsRuntimeAndReferenceAssembly)' == 'true') and
-                                $(NetCoreAppLibrary.Contains('$(AssemblyName);')) and
-                                '$(IsPrivateAssembly)' != 'true'">true</IsNETCoreAppRef>
-    <IsNETCoreAppAnalyzer Condition="'$(IsGeneratorProject)' == 'true' and
-                                     $(NetCoreAppLibraryGenerator.Contains('$(MSBuildProjectName);'))">true</IsNETCoreAppAnalyzer>
     <!-- Inbox analyzers shouldn't use the live targeting / runtime pack. They better depend on an LKG to avoid layering concerns. -->
     <UseLocalTargetingRuntimePack Condition="'$(IsNETCoreAppAnalyzer)' == 'true'">false</UseLocalTargetingRuntimePack>
     <!-- By default, disable implicit framework references for NetCoreAppCurrent libraries. -->

diff --git a/src/libraries/System.Text.Json/src/System.Text.Json.csproj b/src/libraries/System.Text.Json/src/System.Text.Json.csproj
@@ -8,6 +8,8 @@
     <NoWarn>CS8969</NoWarn>
     <IncludeInternalObsoleteAttribute>true</IncludeInternalObsoleteAttribute>
     <IsPackable>true</IsPackable>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
+    <ServicingVersion>1</ServicingVersion>
     <PackageDescription>Provides high-performance and low-allocating types that serialize objects to JavaScript Object Notation (JSON) text and deserialize JSON text to objects, with UTF-8 support built-in. Also provides types to read and write JSON text encoded as UTF-8, and to create an in-memory document object model (DOM), that is read-only, for random access of the JSON elements within a structured view of the data.
 
 The System.Text.Json library is built-in as part of the shared framework in .NET Runtime. The package can be installed when you need to use it in other target frameworks.</PackageDescription>

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonArray.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonArray.cs
@@ -174,13 +174,7 @@ internal JsonArray(JsonElement element, JsonNodeOptions? options = null) : base(
         [RequiresDynamicCode(JsonValue.CreateDynamicCodeMessage)]
         public void Add<T>(T? value)
         {
-            JsonNode? nodeToAdd = value switch
-            {
-                null => null,
-                JsonNode node => node,
-                _ => JsonValue.Create(value, Options)
-            };
-
+            JsonNode? nodeToAdd = ConvertFromValue(value, Options);
             Add(nodeToAdd);
         }
 

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonNode.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonNode.cs
@@ -3,6 +3,8 @@
 
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
+using System.Text.Json.Serialization.Converters;
+using System.Text.Json.Serialization.Metadata;
 
 namespace System.Text.Json.Nodes
 {
@@ -316,17 +318,16 @@ public static bool DeepEquals(JsonNode? node1, JsonNode? node2)
         [RequiresDynamicCode(JsonValue.CreateDynamicCodeMessage)]
         public void ReplaceWith<T>(T value)
         {
+            JsonNode? node;
             switch (_parent)
             {
-                case null:
-                    return;
                 case JsonObject jsonObject:
-                    JsonValue? jsonValue = JsonValue.Create(value);
-                    jsonObject.SetItem(GetPropertyName(), jsonValue);
+                    node = ConvertFromValue(value);
+                    jsonObject.SetItem(GetPropertyName(), node);
                     return;
                 case JsonArray jsonArray:
-                    JsonValue? jValue = JsonValue.Create(value);
-                    jsonArray.SetItem(GetElementIndex(), jValue);
+                    node = ConvertFromValue(value);
+                    jsonArray.SetItem(GetElementIndex(), node);
                     return;
             }
         }
@@ -351,5 +352,33 @@ internal void AssignParent(JsonNode parent)
 
             Parent = parent;
         }
+
+        /// <summary>
+        /// Adaptation of the equivalent JsonValue.Create factory method extended
+        /// to support arbitrary <see cref="JsonElement"/> and <see cref="JsonNode"/> values.
+        /// TODO consider making public cf. https://github.com/dotnet/runtime/issues/70427
+        /// </summary>
+        [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
+        [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
+        internal static JsonNode? ConvertFromValue<T>(T? value, JsonNodeOptions? options = null)
+        {
+            if (value is null)
+            {
+                return null;
+            }
+
+            if (value is JsonNode node)
+            {
+                return node;
+            }
+
+            if (value is JsonElement element)
+            {
+                return JsonNodeConverter.Create(element, options);
+            }
+
+            var jsonTypeInfo = (JsonTypeInfo<T>)JsonSerializerOptions.Default.GetTypeInfo(typeof(T));
+            return new JsonValueCustomized<T>(value, jsonTypeInfo, options);
+        }
     }
 }
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonArrayTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonArrayTests.cs
@@ -680,5 +680,74 @@ public static void ReplaceWith()
             Assert.Null(jValue.Parent);
             Assert.Equal("[5]", jArray.ToJsonString());
         }
+
+        [Theory]
+        [InlineData("null")]
+        [InlineData("1")]
+        [InlineData("false")]
+        [InlineData("\"str\"")]
+        [InlineData("""{"test":"hello world"}""")]
+        [InlineData("[1,2,3]")]
+        public static void AddJsonElement(string json)
+        {
+            // Regression test for https://github.com/dotnet/runtime/issues/94842
+            using var jdoc = JsonDocument.Parse(json);
+            var array = new JsonArray();
+
+            array.Add(jdoc.RootElement);
+
+            JsonNode arrayElement = Assert.Single(array);
+            switch (jdoc.RootElement.ValueKind)
+            {
+                case JsonValueKind.Object:
+                    Assert.IsAssignableFrom<JsonObject>(arrayElement);
+                    break;
+                case JsonValueKind.Array:
+                    Assert.IsAssignableFrom<JsonArray>(arrayElement);
+                    break;
+                case JsonValueKind.Null:
+                    Assert.Null(arrayElement);
+                    break;
+                default:
+                    Assert.IsAssignableFrom<JsonValue>(arrayElement);
+                    break;
+            }
+            Assert.Equal($"[{json}]", array.ToJsonString());
+        }
+
+        [Theory]
+        [InlineData("null")]
+        [InlineData("1")]
+        [InlineData("false")]
+        [InlineData("\"str\"")]
+        [InlineData("""{"test":"hello world"}""")]
+        [InlineData("[1,2,3]")]
+        public static void ReplaceWithJsonElement(string json)
+        {
+            // Regression test for https://github.com/dotnet/runtime/issues/94842
+            using var jdoc = JsonDocument.Parse(json);
+            var array = new JsonArray { 1 };
+
+            array[0].ReplaceWith(jdoc.RootElement);
+
+            JsonNode arrayElement = Assert.Single(array);
+            switch (jdoc.RootElement.ValueKind)
+            {
+                case JsonValueKind.Object:
+                    Assert.IsAssignableFrom<JsonObject>(arrayElement);
+                    break;
+                case JsonValueKind.Array:
+                    Assert.IsAssignableFrom<JsonArray>(arrayElement);
+                    break;
+                case JsonValueKind.Null:
+                    Assert.Null(arrayElement);
+                    break;
+                default:
+                    Assert.IsAssignableFrom<JsonValue>(arrayElement);
+                    break;
+            }
+
+            Assert.Equal($"[{json}]", array.ToJsonString());
+        }
     }
 }